diff --git a/changes/issue-3950-validate-authorization-for-run-query b/changes/issue-3950-validate-authorization-for-run-query new file mode 100644 index 0000000000..5e85f2d448 --- /dev/null +++ b/changes/issue-3950-validate-authorization-for-run-query @@ -0,0 +1 @@ +* Validate at the authorization check that user is allowed to target the specified team(s) when running a query. diff --git a/server/authz/policy.rego b/server/authz/policy.rego index 465bb5a355..a83e4ec856 100644 --- a/server/authz/policy.rego +++ b/server/authz/policy.rego @@ -291,14 +291,14 @@ allow { action == write } -# Global admins and (team) maintainers can run any +# Global admins and maintainers can run any allow { - object.type == "query" + object.type == "targeted_query" subject.global_role == admin action = run } allow { - object.type == "query" + object.type == "targeted_query" subject.global_role == maintainer action = run } @@ -314,13 +314,31 @@ allow { } # Team admin and maintainer running a non-observers_can_run query must have the targets -# filtered to only teams that they maintain. That check is not validated by this rego -# file, it is a filter that is applied at the datastore level (in HostIDsInTargets). +# filtered to only teams that they maintain. allow { - object.type == "query" - # If role is maintainer on any team - team_role(subject, subject.teams[_].id) == [admin,maintainer][_] + object.type == "targeted_query" + object.observer_can_run == false + is_null(subject.global_role) action == run + + not is_null(object.host_targets.teams) + ok_teams := { tmid | tmid := object.host_targets.teams[_]; team_role(subject, tmid) == [admin,maintainer][_] } + count(ok_teams) == count(object.host_targets.teams) +} + +# Team admin and maintainer running a non-observers_can_run query when no target teams +# are specified. +allow { + object.type == "targeted_query" + object.observer_can_run == false + is_null(subject.global_role) + action == run + + # If role is admin or maintainer on any team + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] + + # and there are no team targets + is_null(object.host_targets.teams) } # Team admin and maintainer can run a new query @@ -331,22 +349,40 @@ allow { action == run_new } -# (Team) observers can run only if observers_can_run +# Observers can run only if observers_can_run allow { - object.type == "query" - object.observer_can_run == true - subject.global_role == observer - action = run + object.type == "targeted_query" + object.observer_can_run == true + subject.global_role == observer + action = run } + # Team observer running a observers_can_run query must have the targets -# filtered to only teams that they observe. That check is not validated by this rego -# file, it is a filter that is applied at the datastore level (in HostIDsInTargets). +# filtered to only teams that they observe. allow { - object.type == "query" - object.observer_can_run == true - # If role is observer on any team - team_role(subject, subject.teams[_].id) == observer - action == run + object.type == "targeted_query" + object.observer_can_run == true + is_null(subject.global_role) + action == run + + not is_null(object.host_targets.teams) + ok_teams := { tmid | tmid := object.host_targets.teams[_]; team_role(subject, tmid) == [admin,maintainer,observer][_] } + count(ok_teams) == count(object.host_targets.teams) +} + +# Team observer running a observers_can_run query and there are no +# target teams. +allow { + object.type == "targeted_query" + object.observer_can_run == true + is_null(subject.global_role) + action == run + + # If role is admin, maintainer or observer on any team + team_role(subject, subject.teams[_].id) == [admin,maintainer,observer][_] + + # and there are no team targets + is_null(object.host_targets.teams) } ## diff --git a/server/authz/policy_test.go b/server/authz/policy_test.go index c1630eb2d5..337ce174b5 100644 --- a/server/authz/policy_test.go +++ b/server/authz/policy_test.go @@ -2,6 +2,7 @@ package authz import ( "encoding/json" + "fmt" "testing" "github.com/fleetdm/fleet/v4/server/fleet" @@ -17,6 +18,7 @@ const ( write = fleet.ActionWrite writeRole = fleet.ActionWriteRole run = fleet.ActionRun + runNew = fleet.ActionRunNew ) var auth *Authorizer @@ -32,7 +34,7 @@ func init() { type authTestCase struct { user *fleet.User object interface{} - action interface{} + action string allow bool } @@ -340,75 +342,190 @@ func TestAuthorizeQuery(t *testing.T) { t.Parallel() teamMaintainer := &fleet.User{ + ID: 100, Teams: []fleet.UserTeam{ {Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}, }, } + teamAdmin := &fleet.User{ + ID: 101, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + }, + } teamObserver := &fleet.User{ + ID: 102, Teams: []fleet.UserTeam{ {Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}, }, } - query := &fleet.Query{} + twoTeamsAdminObs := &fleet.User{ + ID: 103, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}, + }, + } + + query := &fleet.Query{ObserverCanRun: false} + emptyTquery := &fleet.TargetedQuery{Query: query} + team1Query := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1}}, Query: query} + team12Query := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1, 2}}, Query: query} + team2Query := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{2}}, Query: query} + team123Query := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1, 2, 3}}, Query: query} + observerQuery := &fleet.Query{ObserverCanRun: true} + emptyTobsQuery := &fleet.TargetedQuery{Query: observerQuery} + team1ObsQuery := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1}}, Query: observerQuery} + team12ObsQuery := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1, 2}}, Query: observerQuery} + team2ObsQuery := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{2}}, Query: observerQuery} + team123ObsQuery := &fleet.TargetedQuery{HostTargets: fleet.HostTargets{TeamIDs: []uint{1, 2, 3}}, Query: observerQuery} + + teamAdminQuery := &fleet.Query{ID: 1, AuthorID: ptr.Uint(teamAdmin.ID), ObserverCanRun: false} + teamMaintQuery := &fleet.Query{ID: 2, AuthorID: ptr.Uint(teamMaintainer.ID), ObserverCanRun: false} + globalAdminQuery := &fleet.Query{ID: 3, AuthorID: ptr.Uint(test.UserAdmin.ID), ObserverCanRun: false} + runTestCases(t, []authTestCase{ // No access {user: nil, object: query, action: read, allow: false}, {user: nil, object: query, action: write, allow: false}, - {user: nil, object: query, action: run, allow: false}, + {user: nil, object: teamAdminQuery, action: write, allow: false}, + {user: nil, object: emptyTquery, action: run, allow: false}, + {user: nil, object: team1Query, action: run, allow: false}, + {user: nil, object: query, action: runNew, allow: false}, {user: nil, object: observerQuery, action: read, allow: false}, {user: nil, object: observerQuery, action: write, allow: false}, - {user: nil, object: observerQuery, action: run, allow: false}, + {user: nil, object: emptyTobsQuery, action: run, allow: false}, + {user: nil, object: team1ObsQuery, action: run, allow: false}, + {user: nil, object: observerQuery, action: runNew, allow: false}, // User can still read queries with no roles {user: test.UserNoRoles, object: query, action: read, allow: true}, {user: test.UserNoRoles, object: query, action: write, allow: false}, - {user: test.UserNoRoles, object: query, action: run, allow: false}, + {user: test.UserNoRoles, object: teamAdminQuery, action: write, allow: false}, + {user: test.UserNoRoles, object: emptyTquery, action: run, allow: false}, + {user: test.UserNoRoles, object: team1Query, action: run, allow: false}, + {user: test.UserNoRoles, object: query, action: runNew, allow: false}, {user: test.UserNoRoles, object: observerQuery, action: read, allow: true}, {user: test.UserNoRoles, object: observerQuery, action: write, allow: false}, - {user: test.UserNoRoles, object: query, action: run, allow: false}, + {user: test.UserNoRoles, object: emptyTobsQuery, action: run, allow: false}, + {user: test.UserNoRoles, object: team1ObsQuery, action: run, allow: false}, + {user: test.UserNoRoles, object: observerQuery, action: runNew, allow: false}, // Global observer can read {user: test.UserObserver, object: query, action: read, allow: true}, {user: test.UserObserver, object: query, action: write, allow: false}, - {user: test.UserObserver, object: query, action: run, allow: false}, + {user: test.UserObserver, object: teamAdminQuery, action: write, allow: false}, + {user: test.UserObserver, object: emptyTquery, action: run, allow: false}, + {user: test.UserObserver, object: team1Query, action: run, allow: false}, + {user: test.UserObserver, object: query, action: runNew, allow: false}, {user: test.UserObserver, object: observerQuery, action: read, allow: true}, {user: test.UserObserver, object: observerQuery, action: write, allow: false}, - // Can run observer query - {user: test.UserObserver, object: observerQuery, action: run, allow: true}, + {user: test.UserObserver, object: emptyTobsQuery, action: run, allow: true}, // can run observer query + {user: test.UserObserver, object: team1ObsQuery, action: run, allow: true}, // can run observer query + {user: test.UserObserver, object: team12ObsQuery, action: run, allow: true}, // can run observer query + {user: test.UserObserver, object: observerQuery, action: runNew, allow: false}, - // Global maintainer can read/write/run + // Global maintainer can read/write (even not authored by them)/run any {user: test.UserMaintainer, object: query, action: read, allow: true}, {user: test.UserMaintainer, object: query, action: write, allow: true}, - {user: test.UserMaintainer, object: query, action: run, allow: true}, + {user: test.UserMaintainer, object: teamMaintQuery, action: write, allow: true}, + {user: test.UserMaintainer, object: globalAdminQuery, action: write, allow: true}, + {user: test.UserMaintainer, object: emptyTquery, action: run, allow: true}, + {user: test.UserMaintainer, object: team1Query, action: run, allow: true}, + {user: test.UserMaintainer, object: query, action: runNew, allow: true}, {user: test.UserMaintainer, object: observerQuery, action: read, allow: true}, {user: test.UserMaintainer, object: observerQuery, action: write, allow: true}, - {user: test.UserMaintainer, object: observerQuery, action: run, allow: true}, + {user: test.UserMaintainer, object: emptyTobsQuery, action: run, allow: true}, + {user: test.UserMaintainer, object: team1ObsQuery, action: run, allow: true}, + {user: test.UserMaintainer, object: observerQuery, action: runNew, allow: true}, - // Global admin can read/write + // Global admin can read/write (even not authored by them)/run any {user: test.UserAdmin, object: query, action: read, allow: true}, {user: test.UserAdmin, object: query, action: write, allow: true}, - {user: test.UserAdmin, object: query, action: run, allow: true}, + {user: test.UserAdmin, object: teamMaintQuery, action: write, allow: true}, + {user: test.UserAdmin, object: globalAdminQuery, action: write, allow: true}, + {user: test.UserAdmin, object: emptyTquery, action: run, allow: true}, + {user: test.UserAdmin, object: team1Query, action: run, allow: true}, + {user: test.UserAdmin, object: query, action: runNew, allow: true}, {user: test.UserAdmin, object: observerQuery, action: read, allow: true}, {user: test.UserAdmin, object: observerQuery, action: write, allow: true}, - {user: test.UserAdmin, object: observerQuery, action: run, allow: true}, + {user: test.UserAdmin, object: emptyTobsQuery, action: run, allow: true}, + {user: test.UserAdmin, object: team1ObsQuery, action: run, allow: true}, + {user: test.UserAdmin, object: observerQuery, action: runNew, allow: true}, - // Team observer read + // Team observer can read and run observer_can_run only {user: teamObserver, object: query, action: read, allow: true}, {user: teamObserver, object: query, action: write, allow: false}, - {user: teamObserver, object: query, action: run, allow: false}, + {user: teamObserver, object: teamAdminQuery, action: write, allow: false}, + {user: teamObserver, object: emptyTquery, action: run, allow: false}, + {user: teamObserver, object: team1Query, action: run, allow: false}, + {user: teamObserver, object: query, action: runNew, allow: false}, {user: teamObserver, object: observerQuery, action: read, allow: true}, {user: teamObserver, object: observerQuery, action: write, allow: false}, - // Can run observer query - {user: teamObserver, object: observerQuery, action: run, allow: true}, + {user: teamObserver, object: emptyTobsQuery, action: run, allow: true}, // can run observer query with no targeted team + {user: teamObserver, object: team1ObsQuery, action: run, allow: true}, // can run observer query filtered to observed team + {user: teamObserver, object: team12ObsQuery, action: run, allow: false}, // not filtered only to observed teams + {user: teamObserver, object: team2ObsQuery, action: run, allow: false}, // not filtered only to observed teams + {user: teamObserver, object: observerQuery, action: runNew, allow: false}, - // Team maintainer can read/write + // Team maintainer can read/write their own queries/run queries filtered on their team(s) {user: teamMaintainer, object: query, action: read, allow: true}, {user: teamMaintainer, object: query, action: write, allow: true}, - {user: teamMaintainer, object: query, action: run, allow: true}, + {user: teamMaintainer, object: teamMaintQuery, action: write, allow: true}, + {user: teamMaintainer, object: teamAdminQuery, action: write, allow: false}, + {user: teamMaintainer, object: emptyTquery, action: run, allow: true}, + {user: teamMaintainer, object: team1Query, action: run, allow: true}, + {user: teamMaintainer, object: team12Query, action: run, allow: false}, + {user: teamMaintainer, object: team2Query, action: run, allow: false}, + {user: teamMaintainer, object: query, action: runNew, allow: true}, {user: teamMaintainer, object: observerQuery, action: read, allow: true}, {user: teamMaintainer, object: observerQuery, action: write, allow: true}, - {user: teamMaintainer, object: observerQuery, action: run, allow: true}, + {user: teamMaintainer, object: emptyTobsQuery, action: run, allow: true}, + {user: teamMaintainer, object: team1ObsQuery, action: run, allow: true}, + {user: teamMaintainer, object: team12ObsQuery, action: run, allow: false}, + {user: teamMaintainer, object: team2ObsQuery, action: run, allow: false}, + {user: teamMaintainer, object: observerQuery, action: runNew, allow: true}, + + // Team admin can read/write their own queries/run queries filtered on their team(s) + {user: teamAdmin, object: query, action: read, allow: true}, + {user: teamAdmin, object: query, action: write, allow: true}, + {user: teamAdmin, object: teamAdminQuery, action: write, allow: true}, + {user: teamAdmin, object: teamMaintQuery, action: write, allow: false}, + {user: teamAdmin, object: globalAdminQuery, action: write, allow: false}, + {user: teamAdmin, object: emptyTquery, action: run, allow: true}, + {user: teamAdmin, object: team1Query, action: run, allow: true}, + {user: teamAdmin, object: team12Query, action: run, allow: false}, + {user: teamAdmin, object: team2Query, action: run, allow: false}, + {user: teamAdmin, object: query, action: runNew, allow: true}, + {user: teamAdmin, object: observerQuery, action: read, allow: true}, + {user: teamAdmin, object: observerQuery, action: write, allow: true}, + {user: teamAdmin, object: emptyTobsQuery, action: run, allow: true}, + {user: teamAdmin, object: team1ObsQuery, action: run, allow: true}, + {user: teamAdmin, object: team12ObsQuery, action: run, allow: false}, + {user: teamAdmin, object: team2ObsQuery, action: run, allow: false}, + {user: teamAdmin, object: observerQuery, action: runNew, allow: true}, + + // User admin on team 1, observer on team 2 + {user: twoTeamsAdminObs, object: query, action: read, allow: true}, + {user: twoTeamsAdminObs, object: query, action: write, allow: true}, + {user: twoTeamsAdminObs, object: teamAdminQuery, action: write, allow: false}, + {user: twoTeamsAdminObs, object: teamMaintQuery, action: write, allow: false}, + {user: twoTeamsAdminObs, object: globalAdminQuery, action: write, allow: false}, + {user: twoTeamsAdminObs, object: emptyTquery, action: run, allow: true}, + {user: twoTeamsAdminObs, object: team1Query, action: run, allow: true}, + {user: twoTeamsAdminObs, object: team12Query, action: run, allow: false}, // user is only observer on team 2 + {user: twoTeamsAdminObs, object: team2Query, action: run, allow: false}, + {user: twoTeamsAdminObs, object: team123Query, action: run, allow: false}, + {user: twoTeamsAdminObs, object: query, action: runNew, allow: true}, + {user: twoTeamsAdminObs, object: observerQuery, action: read, allow: true}, + {user: twoTeamsAdminObs, object: observerQuery, action: write, allow: true}, + {user: twoTeamsAdminObs, object: emptyTobsQuery, action: run, allow: true}, + {user: twoTeamsAdminObs, object: team1ObsQuery, action: run, allow: true}, + {user: twoTeamsAdminObs, object: team12ObsQuery, action: run, allow: true}, // user is at least observer on both teams + {user: twoTeamsAdminObs, object: team2ObsQuery, action: run, allow: true}, + {user: twoTeamsAdminObs, object: team123ObsQuery, action: run, allow: false}, // not member of team 3 + {user: twoTeamsAdminObs, object: observerQuery, action: runNew, allow: true}, }) } @@ -594,13 +711,15 @@ func TestAuthorizePolicies(t *testing.T) { func assertAuthorized(t *testing.T, user *fleet.User, object, action interface{}) { t.Helper() - assert.NoError(t, auth.Authorize(test.UserContext(user), object, action), "should be authorized\n%v\n%v\n%v", user, object, action) + b, _ := json.MarshalIndent(map[string]interface{}{"subject": user, "object": object, "action": action}, "", " ") + assert.NoError(t, auth.Authorize(test.UserContext(user), object, action), "should be authorized\n%s", string(b)) } func assertUnauthorized(t *testing.T, user *fleet.User, object, action interface{}) { t.Helper() - assert.Error(t, auth.Authorize(test.UserContext(user), object, action), "should be unauthorized\n%v\n%v\n%v", user, object, action) + b, _ := json.MarshalIndent(map[string]interface{}{"subject": user, "object": object, "action": action}, "", " ") + assert.Error(t, auth.Authorize(test.UserContext(user), object, action), "should be unauthorized\n%s", string(b)) } func runTestCases(t *testing.T, testCases []authTestCase) { @@ -608,7 +727,35 @@ func runTestCases(t *testing.T, testCases []authTestCase) { for _, tt := range testCases { tt := tt - t.Run("", func(t *testing.T) { + + // build a useful test name from user role, object, action and expected result + action := tt.action + role := "none" + if tt.user != nil { + if tt.user.GlobalRole != nil { + role = "g:" + *tt.user.GlobalRole + } else if len(tt.user.Teams) > 0 { + role = "" + for _, tm := range tt.user.Teams { + if role != "" { + role += "," + } + role += tm.Role + } + } + } + + obj := fmt.Sprintf("%T", tt.object) + if at, ok := tt.object.(AuthzTyper); ok { + obj = at.AuthzType() + } + + result := "allow" + if !tt.allow { + result = "deny" + } + + t.Run(action+"_"+obj+"_"+role+"_"+result, func(t *testing.T) { t.Parallel() if tt.allow { assertAuthorized(t, tt.user, tt.object, tt.action) diff --git a/server/fleet/queries.go b/server/fleet/queries.go index 260aa0fe98..e89260c767 100644 --- a/server/fleet/queries.go +++ b/server/fleet/queries.go @@ -70,6 +70,15 @@ func (q *Query) Verify() error { return nil } +type TargetedQuery struct { + *Query + HostTargets HostTargets `json:"host_targets"` +} + +func (tq *TargetedQuery) AuthzType() string { + return "targeted_query" +} + var ( validateSQLRegexp = regexp.MustCompile(`(?i)attach[^\w]+.*[^\w]+as[^\w]+`) errQueryEmptyName = errors.New("query name cannot be empty") diff --git a/server/service/campaigns.go b/server/service/campaigns.go index 874d127a13..b145488a7b 100644 --- a/server/service/campaigns.go +++ b/server/service/campaigns.go @@ -81,7 +81,8 @@ func (svc *Service) NewDistributedQueryCampaign(ctx context.Context, queryString } } - if err := svc.authz.Authorize(ctx, query, fleet.ActionRun); err != nil { + tq := &fleet.TargetedQuery{Query: query, HostTargets: targets} + if err := svc.authz.Authorize(ctx, tq, fleet.ActionRun); err != nil { return nil, err } diff --git a/server/service/campaigns_test.go b/server/service/campaigns_test.go index 91aa7b8ecb..6b02e14727 100644 --- a/server/service/campaigns_test.go +++ b/server/service/campaigns_test.go @@ -29,6 +29,7 @@ func (nopLiveQuery) QueriesForHost(hostID uint) (map[string]string, error) { func (nopLiveQuery) QueryCompletedByHost(name string, hostID uint) error { return nil } + func TestLiveQueryAuth(t *testing.T) { ds := new(mock.Store) qr := pubsub.NewInmemQueryResults() @@ -49,10 +50,17 @@ func TestLiveQueryAuth(t *testing.T) { Query: "SELECT 2", ObserverCanRun: false, } - _ = query2ObsCannotRun + var lastCreatedQuery *fleet.Query ds.NewQueryFunc = func(ctx context.Context, query *fleet.Query, opts ...fleet.OptionalArg) (*fleet.Query, error) { - return query, nil + q := *query + vw, ok := viewer.FromContext(ctx) + q.ID = 123 + if ok { + q.AuthorID = ptr.Uint(vw.User.ID) + } + lastCreatedQuery = &q + return &q, nil } ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { return &fleet.AppConfig{ServerSettings: fleet.ServerSettings{LiveQueryDisabled: false}}, nil @@ -85,6 +93,11 @@ func TestLiveQueryAuth(t *testing.T) { if id == 2 { return query2ObsCannotRun, nil } + if lastCreatedQuery != nil { + q := lastCreatedQuery + lastCreatedQuery = nil + return q, nil + } return &fleet.Query{ID: 8888, AuthorID: ptr.Uint(6666)}, nil } @@ -97,88 +110,104 @@ func TestLiveQueryAuth(t *testing.T) { shouldFailRunObsCannot bool }{ { - "global admin", - &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, - nil, - false, - false, - false, + name: "global admin", + user: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, + teamID: nil, + shouldFailRunNew: false, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: false, }, { - "global maintainer", - &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, - nil, - false, - false, - false, + name: "global maintainer", + user: &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, + teamID: nil, + shouldFailRunNew: false, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: false, }, { - "global observer", - &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, - nil, - true, - false, - true, + name: "global observer", + user: &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, + teamID: nil, + shouldFailRunNew: true, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: true, }, { - "team maintainer", - teamMaintainer, - nil, - false, - false, - false, + name: "team maintainer", + user: teamMaintainer, + teamID: nil, + shouldFailRunNew: false, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: false, }, - // NOTE: this specific case is not covered by the rego authorization policy, - // it is at the datastore level that a filter is applied to only consider - // hosts that the user can see (that is, a fleet.TeamFilter is passed to - // ds.HostIDsInTargets and that call applies the filter to return only - // allowed hosts). - /* - { - "team admin, target not set to own team", - &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, - ptr.Uint(2), - false, - false, - true, - }, - */ { - "team admin, target set to own team", - &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, - ptr.Uint(1), - false, - false, - false, + name: "team admin, no team target", + user: &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + teamID: nil, + shouldFailRunNew: false, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: false, }, - // NOTE: same as the note above. - /* - { - "team observer, target not set to own team", - &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, - ptr.Uint(2), - true, - true, - true, - }, - */ { - "team observer, target set to own team", - &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, - ptr.Uint(1), - true, - false, - true, + name: "team admin, target not set to own team", + user: &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + teamID: ptr.Uint(2), + shouldFailRunNew: false, + shouldFailRunObsCan: true, // fails observer can run, as they are not part of that team, even as observer + shouldFailRunObsCannot: true, + }, + { + name: "team admin, target set to own team", + user: &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + teamID: ptr.Uint(1), + shouldFailRunNew: false, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: false, + }, + { + name: "team observer, no team target", + user: &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + teamID: nil, + shouldFailRunNew: true, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: true, + }, + { + name: "team observer, target not set to own team", + user: &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + teamID: ptr.Uint(2), + shouldFailRunNew: true, + shouldFailRunObsCan: true, + shouldFailRunObsCannot: true, + }, + { + name: "team observer, target set to own team", + user: &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + teamID: ptr.Uint(1), + shouldFailRunNew: true, + shouldFailRunObsCan: false, + shouldFailRunObsCannot: true, }, } for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { ctx := viewer.NewContext(context.Background(), viewer.Viewer{User: tt.user}) - _, err := svc.NewDistributedQueryCampaign(ctx, query1ObsCanRun.Query, nil, fleet.HostTargets{}) + var tms []uint + // Testing RunNew is tricky, because RunNew authorization is done, then + // the query is created, and then the Run authorization is applied to + // that now-existing query, so we have to make sure that the Run does not + // cause a Forbidden error. To this end, the ds.NewQuery mock always sets + // the AuthorID to the context user, and if the user is member of a team, + // always set that team as a host target. This will prevent the Run + // action from failing, if RunNew did succeed. + if len(tt.user.Teams) > 0 { + tms = []uint{tt.user.Teams[0].ID} + } + _, err := svc.NewDistributedQueryCampaign(ctx, query1ObsCanRun.Query, nil, fleet.HostTargets{TeamIDs: tms}) checkAuthErr(t, tt.shouldFailRunNew, err) - var tms []uint if tt.teamID != nil { tms = []uint{*tt.teamID} } @@ -188,14 +217,18 @@ func TestLiveQueryAuth(t *testing.T) { _, err = svc.NewDistributedQueryCampaign(ctx, query2ObsCannotRun.Query, ptr.Uint(query2ObsCannotRun.ID), fleet.HostTargets{TeamIDs: tms}) checkAuthErr(t, tt.shouldFailRunObsCannot, err) - _, err = svc.NewDistributedQueryCampaignByNames(ctx, query1ObsCanRun.Query, nil, nil, nil) - checkAuthErr(t, tt.shouldFailRunNew, err) + // tests with a team target cannot run the "ByNames" calls, as there's no way + // to pass a team target with this call. + if tt.teamID == nil { + _, err = svc.NewDistributedQueryCampaignByNames(ctx, query1ObsCanRun.Query, nil, nil, nil) + checkAuthErr(t, tt.shouldFailRunNew, err) - _, err = svc.NewDistributedQueryCampaignByNames(ctx, query1ObsCanRun.Query, ptr.Uint(query1ObsCanRun.ID), nil, nil) - checkAuthErr(t, tt.shouldFailRunObsCan, err) + _, err = svc.NewDistributedQueryCampaignByNames(ctx, query1ObsCanRun.Query, ptr.Uint(query1ObsCanRun.ID), nil, nil) + checkAuthErr(t, tt.shouldFailRunObsCan, err) - _, err = svc.NewDistributedQueryCampaignByNames(ctx, query2ObsCannotRun.Query, ptr.Uint(query2ObsCannotRun.ID), nil, nil) - checkAuthErr(t, tt.shouldFailRunObsCannot, err) + _, err = svc.NewDistributedQueryCampaignByNames(ctx, query2ObsCannotRun.Query, ptr.Uint(query2ObsCannotRun.ID), nil, nil) + checkAuthErr(t, tt.shouldFailRunObsCannot, err) + } }) } } diff --git a/server/service/service_campaigns.go b/server/service/service_campaigns.go index 6c4dad4657..6db6b3cf64 100644 --- a/server/service/service_campaigns.go +++ b/server/service/service_campaigns.go @@ -39,7 +39,7 @@ func (svc Service) StreamCampaignResults(ctx context.Context, conn *websocket.Co // Explicitly set ObserverCanRun: true in this check because we check that the user trying to // read results is the same user that initiated the query. This means the observer check already // happened with the actual value for this query. - if err := svc.authz.Authorize(ctx, &fleet.Query{ObserverCanRun: true}, fleet.ActionRun); err != nil { + if err := svc.authz.Authorize(ctx, &fleet.TargetedQuery{Query: &fleet.Query{ObserverCanRun: true}}, fleet.ActionRun); err != nil { level.Info(svc.logger).Log("err", "stream results authorization failed") conn.WriteJSONError(authz.ForbiddenErrorMessage) return diff --git a/server/service/service_osquery_test.go b/server/service/service_osquery_test.go index 82e6c87383..30910abdad 100644 --- a/server/service/service_osquery_test.go +++ b/server/service/service_osquery_test.go @@ -1948,13 +1948,14 @@ func TestTeamMaintainerCanRunNewDistributedCampaigns(t *testing.T) { ds.QueryFunc = func(ctx context.Context, id uint) (*fleet.Query, error) { return &fleet.Query{ ID: 42, + AuthorID: ptr.Uint(99), Name: "query", Query: "select 1;", ObserverCanRun: false, }, nil } viewerCtx := viewer.NewContext(context.Background(), viewer.Viewer{ - User: &fleet.User{ID: 0, Teams: []fleet.UserTeam{{Role: fleet.RoleMaintainer}}}, + User: &fleet.User{ID: 99, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 123}, Role: fleet.RoleMaintainer}}}, }) q := "select year, month, day, hour, minutes, seconds from time" @@ -1980,7 +1981,7 @@ func TestTeamMaintainerCanRunNewDistributedCampaigns(t *testing.T) { return nil } lq.On("RunQuery", "0", "select year, month, day, hour, minutes, seconds from time", []uint{1, 3, 5}).Return(nil) - _, err := svc.NewDistributedQueryCampaign(viewerCtx, q, nil, fleet.HostTargets{HostIDs: []uint{2}, LabelIDs: []uint{1}}) + _, err := svc.NewDistributedQueryCampaign(viewerCtx, q, nil, fleet.HostTargets{HostIDs: []uint{2}, LabelIDs: []uint{1}, TeamIDs: []uint{123}}) require.NoError(t, err) }