From 98d9f1b068dce99743431be3a7c1b4a65ccc0901 Mon Sep 17 00:00:00 2001 From: Juan Fernandez Date: Thu, 8 Jun 2023 17:30:34 -0400 Subject: [PATCH] If user is a global/team observer/observer+, 'teams' endpoints should not include secrets (#12216) Fixed auth. issue with Obs/Obs+ --- changes/bug-2939-obs-shouldnot-see-secrets | 1 + ee/server/service/teams.go | 84 +++++++- ee/server/service/teams_test.go | 96 +++++++++ server/fleet/users.go | 19 ++ server/fleet/users_test.go | 81 ++++++++ server/service/integration_enterprise_test.go | 184 +++++++++++++++++- 6 files changed, 460 insertions(+), 5 deletions(-) create mode 100644 changes/bug-2939-obs-shouldnot-see-secrets create mode 100644 ee/server/service/teams_test.go diff --git a/changes/bug-2939-obs-shouldnot-see-secrets b/changes/bug-2939-obs-shouldnot-see-secrets new file mode 100644 index 0000000000..e40a64aab5 --- /dev/null +++ b/changes/bug-2939-obs-shouldnot-see-secrets @@ -0,0 +1 @@ +- Users with Observer/Observer+ role should not be able to see team secrets. diff --git a/ee/server/service/teams.go b/ee/server/service/teams.go index 058eb10cf2..524c7eb3ef 100644 --- a/ee/server/service/teams.go +++ b/ee/server/service/teams.go @@ -21,6 +21,34 @@ import ( "github.com/go-kit/kit/log/level" ) +func obfuscateSecrets(user *fleet.User, teams []*fleet.Team) error { + if user == nil { + return &authz.Forbidden{} + } + + isGlobalObs := user.IsGlobalObserver() + + teamMemberships := user.TeamMembership(func(t fleet.UserTeam) bool { + return true + }) + obsMembership := user.TeamMembership(func(t fleet.UserTeam) bool { + return t.Role == fleet.RoleObserver || t.Role == fleet.RoleObserverPlus + }) + + for _, t := range teams { + if t == nil { + continue + } + // User does not belong to the team or is a global/team observer/observer+ + if isGlobalObs || user.GlobalRole == nil && (!teamMemberships[t.ID] || obsMembership[t.ID]) { + for _, s := range t.Secrets { + s.Secret = fleet.MaskedPassword + } + } + } + return nil +} + func (svc *Service) NewTeam(ctx context.Context, p fleet.TeamPayload) (*fleet.Team, error) { if err := svc.authz.Authorize(ctx, &fleet.Team{}, fleet.ActionWrite); err != nil { return nil, err @@ -372,7 +400,16 @@ func (svc *Service) ListTeams(ctx context.Context, opt fleet.ListOptions) ([]*fl } filter := fleet.TeamFilter{User: vc.User, IncludeObserver: true} - return svc.ds.ListTeams(ctx, filter, opt) + teams, err := svc.ds.ListTeams(ctx, filter, opt) + if err != nil { + return nil, err + } + + if err = obfuscateSecrets(vc.User, teams); err != nil { + return nil, err + } + + return teams, nil } func (svc *Service) ListAvailableTeamsForUser(ctx context.Context, user *fleet.User) ([]*fleet.TeamSummary, error) { @@ -475,7 +512,21 @@ func (svc *Service) GetTeam(ctx context.Context, teamID uint) (*fleet.Team, erro logging.WithExtras(ctx, "id", teamID) - return svc.ds.Team(ctx, teamID) + vc, ok := viewer.FromContext(ctx) + if !ok { + return nil, fleet.ErrNoContext + } + + team, err := svc.ds.Team(ctx, teamID) + if err != nil { + return nil, err + } + + if err = obfuscateSecrets(vc.User, []*fleet.Team{team}); err != nil { + return nil, err + } + + return team, nil } func (svc *Service) TeamEnrollSecrets(ctx context.Context, teamID uint) ([]*fleet.EnrollSecret, error) { @@ -483,7 +534,34 @@ func (svc *Service) TeamEnrollSecrets(ctx context.Context, teamID uint) ([]*flee return nil, err } - return svc.ds.TeamEnrollSecrets(ctx, teamID) + vc, ok := viewer.FromContext(ctx) + if !ok { + return nil, fleet.ErrNoContext + } + + secrets, err := svc.ds.TeamEnrollSecrets(ctx, teamID) + if err != nil { + return nil, err + } + + isGlobalObs := vc.User.IsGlobalObserver() + teamMemberships := vc.User.TeamMembership(func(t fleet.UserTeam) bool { + return true + }) + obsMembership := vc.User.TeamMembership(func(t fleet.UserTeam) bool { + return t.Role == fleet.RoleObserver || t.Role == fleet.RoleObserverPlus + }) + + for _, s := range secrets { + if s == nil { + continue + } + if isGlobalObs || vc.User.GlobalRole == nil && (!teamMemberships[*s.TeamID] || obsMembership[*s.TeamID]) { + s.Secret = fleet.MaskedPassword + } + } + + return secrets, nil } func (svc *Service) ModifyTeamEnrollSecrets(ctx context.Context, teamID uint, secrets []fleet.EnrollSecret) ([]*fleet.EnrollSecret, error) { diff --git a/ee/server/service/teams_test.go b/ee/server/service/teams_test.go new file mode 100644 index 0000000000..265a514c6f --- /dev/null +++ b/ee/server/service/teams_test.go @@ -0,0 +1,96 @@ +package service + +import ( + "testing" + + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/ptr" + "github.com/stretchr/testify/require" +) + +func TestObfuscateSecrets(t *testing.T) { + buildTeams := func(n int) []*fleet.Team { + r := make([]*fleet.Team, 0, n) + for i := 1; i <= n; i++ { + r = append(r, &fleet.Team{ + ID: uint(i), + Secrets: []*fleet.EnrollSecret{ + {Secret: "abc"}, + {Secret: "123"}, + }, + }) + } + return r + } + + t.Run("no user", func(t *testing.T) { + err := obfuscateSecrets(nil, nil) + require.Error(t, err) + }) + + t.Run("no teams", func(t *testing.T) { + user := fleet.User{} + err := obfuscateSecrets(&user, nil) + require.NoError(t, err) + }) + + t.Run("user is not a global observer", func(t *testing.T) { + user := fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)} + teams := buildTeams(3) + + err := obfuscateSecrets(&user, teams) + require.NoError(t, err) + + for _, team := range teams { + for _, s := range team.Secrets { + require.NotEqual(t, fleet.MaskedPassword, s.Secret) + } + } + }) + + t.Run("user is global observer", func(t *testing.T) { + roles := []*string{ptr.String(fleet.RoleObserver), ptr.String(fleet.RoleObserverPlus)} + for _, r := range roles { + user := &fleet.User{GlobalRole: r} + teams := buildTeams(3) + + err := obfuscateSecrets(user, teams) + require.NoError(t, err) + + for _, team := range teams { + for _, s := range team.Secrets { + require.Equal(t, fleet.MaskedPassword, s.Secret) + } + } + } + }) + + t.Run("user is observer in some teams", func(t *testing.T) { + teams := buildTeams(4) + + // Make user an observer in the 'even' teams + user := &fleet.User{Teams: []fleet.UserTeam{ + { + Team: *teams[1], + Role: fleet.RoleObserver, + }, + { + Team: *teams[2], + Role: fleet.RoleAdmin, + }, + { + Team: *teams[3], + Role: fleet.RoleObserverPlus, + }, + }} + + err := obfuscateSecrets(user, teams) + require.NoError(t, err) + + for i, team := range teams { + for _, s := range team.Secrets { + require.Equal(t, fleet.MaskedPassword == s.Secret, i == 0 || i == 1 || i == 3) + } + } + }) +} diff --git a/server/fleet/users.go b/server/fleet/users.go index 26cf12fdfb..48b4502f24 100644 --- a/server/fleet/users.go +++ b/server/fleet/users.go @@ -32,6 +32,25 @@ type User struct { Teams []UserTeam `json:"teams"` } +// IsGlobalObserver returns true if user is either a Global Observer or a Global Observer+ +func (u *User) IsGlobalObserver() bool { + if u.GlobalRole == nil { + return false + } + return *u.GlobalRole == RoleObserver || *u.GlobalRole == RoleObserverPlus +} + +// TeamMembership returns a map whose keys are the TeamIDs of the teams for which pred evaluates to true +func (u *User) TeamMembership(pred func(UserTeam) bool) map[uint]bool { + result := make(map[uint]bool) + for _, t := range u.Teams { + if pred(t) { + result[t.ID] = true + } + } + return result +} + func (u *User) IsAdminForcedPasswordReset() bool { if u.SSOEnabled { return false diff --git a/server/fleet/users_test.go b/server/fleet/users_test.go index ae18351020..a1c9c12c78 100644 --- a/server/fleet/users_test.go +++ b/server/fleet/users_test.go @@ -10,6 +10,87 @@ import ( "golang.org/x/crypto/bcrypt" ) +func TestIsGlobalObserver(t *testing.T) { + testCases := []struct { + GlobalRole *string + Expected bool + }{ + { + GlobalRole: nil, + }, + { + GlobalRole: ptr.String(RoleAdmin), + }, + { + GlobalRole: ptr.String(RoleObserver), + Expected: true, + }, + { + GlobalRole: ptr.String(RoleObserverPlus), + Expected: true, + }, + } + + for _, tC := range testCases { + sut := User{GlobalRole: tC.GlobalRole} + require.Equal(t, sut.IsGlobalObserver(), tC.Expected) + } +} + +func TestTeamMembership(t *testing.T) { + teams := []UserTeam{ + { + Role: RoleAdmin, + Team: Team{ + ID: 1, + }, + }, + { + Role: RoleGitOps, + Team: Team{ + ID: 2, + }, + }, + { + Role: RoleObserver, + Team: Team{ + ID: 3, + }, + }, + { + Role: RoleObserver, + Team: Team{ + ID: 4, + }, + }, + } + + sut := User{} + require.Empty(t, sut.TeamMembership(func(ut UserTeam) bool { + return true + })) + + sut.Teams = teams + + var result []uint + pred := func(ut UserTeam) bool { + return ut.Role == RoleGitOps || ut.Role == RoleObserver + } + for k := range sut.TeamMembership(pred) { + result = append(result, k) + } + require.ElementsMatch(t, result, []uint{2, 3, 4}) + + result = make([]uint, 0, len(teams)) + pred = func(ut UserTeam) bool { + return true + } + for k := range sut.TeamMembership(pred) { + result = append(result, k) + } + require.ElementsMatch(t, result, []uint{1, 2, 3, 4}) +} + func TestValidatePassword(t *testing.T) { passwordTests := []struct { Password, Email string diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 1e95016eab..b8643f21e5 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -764,7 +764,7 @@ func (s *integrationEnterpriseTestSuite) TestTeamEndpoints() { defaultFeatures := fleet.Features{} defaultFeatures.ApplyDefaultsForNewInstalls() mysql.ExecAdhocSQL(t, s.ds, func(db sqlx.ExtContext) error { - _, err := db.ExecContext(context.Background(), `UPDATE teams SET config = NULL WHERE id = ? `, team.ID) + _, err := db.ExecContext(context.Background(), `UPDATE teams SET config = NULL WHERE id = ? `, tm1ID) return err }) tmResp.Team = nil @@ -773,7 +773,7 @@ func (s *integrationEnterpriseTestSuite) TestTeamEndpoints() { // modify a team with an empty config mysql.ExecAdhocSQL(t, s.ds, func(db sqlx.ExtContext) error { - _, err := db.ExecContext(context.Background(), `UPDATE teams SET config = '{}' WHERE id = ? `, team.ID) + _, err := db.ExecContext(context.Background(), `UPDATE teams SET config = '{}' WHERE id = ? `, tm1ID) return err }) tmResp.Team = nil @@ -919,6 +919,186 @@ func (s *integrationEnterpriseTestSuite) TestTeamEndpoints() { s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/teams/%d", tm1ID), nil, http.StatusNotFound, &delResp) } +func (s *integrationEnterpriseTestSuite) TestTeamSecretsAreObfuscated() { + t := s.T() + + // ----------------- + // Set up test data + // ----------------- + teams := []*fleet.Team{ + { + Name: "Team One", + Description: "Team description", + Secrets: []*fleet.EnrollSecret{{Secret: "DEF"}}, + }, + { + Name: "Team Two", + Description: "Team Two description", + Secrets: []*fleet.EnrollSecret{{Secret: "ABC"}}, + }, + } + for _, team := range teams { + _, err := s.ds.NewTeam(context.Background(), team) + require.NoError(t, err) + } + + global_obs := &fleet.User{ + Name: "Global Obs", + Email: "global_obs@example.com", + GlobalRole: ptr.String(fleet.RoleObserver), + } + global_obs_plus := &fleet.User{ + Name: "Global Obs+", + Email: "global_obs_plus@example.com", + GlobalRole: ptr.String(fleet.RoleObserverPlus), + } + team_obs := &fleet.User{ + Name: "Team Obs", + Email: "team_obs@example.com", + Teams: []fleet.UserTeam{ + { + Team: *teams[0], + Role: fleet.RoleObserver, + }, + { + Team: *teams[1], + Role: fleet.RoleAdmin, + }, + }, + } + team_obs_plus := &fleet.User{ + Name: "Team Obs Plus", + Email: "team_obs_plus@example.com", + Teams: []fleet.UserTeam{ + { + Team: *teams[0], + Role: fleet.RoleAdmin, + }, + { + Team: *teams[1], + Role: fleet.RoleObserverPlus, + }, + }, + } + users := []*fleet.User{global_obs, global_obs_plus, team_obs, team_obs_plus} + for _, u := range users { + require.NoError(t, u.SetPassword(test.GoodPassword, 10, 10)) + _, err := s.ds.NewUser(context.Background(), u) + require.NoError(t, err) + } + + // -------------------------------------------------------------------- + // Global obs/obs+ should not be able to see any team secrets + // -------------------------------------------------------------------- + for _, u := range []*fleet.User{global_obs, global_obs_plus} { + + s.setTokenForTest(t, u.Email, test.GoodPassword) + + // list all teams + var listResp listTeamsResponse + s.DoJSON("GET", "/api/latest/fleet/teams", nil, http.StatusOK, &listResp) + + require.Len(t, listResp.Teams, len(teams)) + require.NoError(t, listResp.Err) + + for _, team := range listResp.Teams { + for _, secret := range team.Secrets { + require.Equal(t, fleet.MaskedPassword, secret.Secret) + } + } + + // listing a team / team secrets + for _, team := range teams { + var getResp getTeamResponse + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), nil, http.StatusOK, &getResp) + + require.NoError(t, getResp.Err) + for _, secret := range getResp.Team.Secrets { + require.Equal(t, fleet.MaskedPassword, secret.Secret) + } + + var secResp teamEnrollSecretsResponse + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/teams/%d/secrets", team.ID), nil, http.StatusOK, &secResp) + + require.Len(t, secResp.Secrets, 1) + require.NoError(t, secResp.Err) + for _, secret := range secResp.Secrets { + require.Equal(t, fleet.MaskedPassword, secret.Secret) + } + } + } + + // -------------------------------------------------------------------- + // Team obs/obs+ should not be able to see their team secrets + // -------------------------------------------------------------------- + for _, u := range []*fleet.User{team_obs, team_obs_plus} { + + s.setTokenForTest(t, u.Email, test.GoodPassword) + + // list all teams + var listResp listTeamsResponse + s.DoJSON("GET", "/api/latest/fleet/teams", nil, http.StatusOK, &listResp) + + require.Len(t, listResp.Teams, len(u.Teams)) + require.NoError(t, listResp.Err) + + for _, team := range listResp.Teams { + for _, secret := range team.Secrets { + // team_obs has RoleObserver in Team 1, and an RoleAdmin in Team 2 + // so it should be able to see the secrets in Team 1 + if u.ID == team_obs.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[0].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[1].ID) + } + + // team_obs_plus should not be able to see any Team Secret + if u.ID == team_obs_plus.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[1].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[0].ID) + } + } + } + + // listing a team / team secrets + for _, team := range teams { + var getResp getTeamResponse + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), nil, http.StatusOK, &getResp) + + require.NoError(t, getResp.Err) + // team_obs has RoleObserver in Team 1, and an RoleAdmin in Team 2 + // so it should be able to see the secrets in Team 1 + for _, secret := range getResp.Team.Secrets { + if u.ID == team_obs.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[0].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[1].ID) + } + + if u.ID == team_obs_plus.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[1].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[0].ID) + } + } + + var secResp teamEnrollSecretsResponse + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/teams/%d/secrets", team.ID), nil, http.StatusOK, &secResp) + + require.Len(t, secResp.Secrets, 1) + require.NoError(t, secResp.Err) + for _, secret := range secResp.Secrets { + if u.ID == team_obs.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[0].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[1].ID) + } + + if u.ID == team_obs_plus.ID { + require.Equal(t, fleet.MaskedPassword == secret.Secret, team.ID == teams[1].ID) + require.Equal(t, fleet.MaskedPassword != secret.Secret, team.ID == teams[0].ID) + } + } + } + } +} + func (s *integrationEnterpriseTestSuite) TestExternalIntegrationsTeamConfig() { t := s.T()