From e4504e893468ffda47e9541a834f23d5d7dcae87 Mon Sep 17 00:00:00 2001 From: Mike Wilson Date: Fri, 22 Sep 2023 16:23:27 -0400 Subject: [PATCH 1/2] Prevent duplicate Access List owners. Duplicate access list owners are now prevented. In the event that duplicate access list owners are currently in the backend, this commit will deduplicate them and choose the first owner with the same name when getting the access list to maintain backwards compatibility. --- api/types/accesslist/accesslist.go | 25 ++++++++--- api/types/accesslist/accesslist_test.go | 58 +++++++++++++++++++++++++ lib/services/local/access_list.go | 7 +++ lib/services/local/access_list_test.go | 36 +++++++++++++++ 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/api/types/accesslist/accesslist.go b/api/types/accesslist/accesslist.go index 690643d2481a0..0cf7e07cdb988 100644 --- a/api/types/accesslist/accesslist.go +++ b/api/types/accesslist/accesslist.go @@ -157,12 +157,6 @@ func (a *AccessList) CheckAndSetDefaults() error { return trace.BadParameter("owners are missing") } - for _, owner := range a.Spec.Owners { - if owner.Name == "" { - return trace.BadParameter("owner name is missing") - } - } - if a.Spec.Audit.Frequency == 0 { return trace.BadParameter("audit frequency must be greater than 0") } @@ -173,6 +167,25 @@ func (a *AccessList) CheckAndSetDefaults() error { return trace.BadParameter("grants must specify at least one role or trait") } + // Deduplicate owners. The backend will currently prevent this, but it's possible that access lists + // were created with duplicated owners before the backend checked for duplicate owners. In order to + // ensure that these access lists are backwards compatible, we'll deduplicate them here. + ownerMap := map[string]bool{} + deduplicatedOwners := []Owner{} + for _, owner := range a.Spec.Owners { + if owner.Name == "" { + return trace.BadParameter("owner name is missing") + } + + if ownerMap[owner.Name] { + continue + } + + ownerMap[owner.Name] = true + deduplicatedOwners = append(deduplicatedOwners, owner) + } + a.Spec.Owners = deduplicatedOwners + return nil } diff --git a/api/types/accesslist/accesslist_test.go b/api/types/accesslist/accesslist_test.go index b0f58300c8338..b8ea9b4738693 100644 --- a/api/types/accesslist/accesslist_test.go +++ b/api/types/accesslist/accesslist_test.go @@ -21,9 +21,67 @@ import ( "testing" "time" + "github.com/gravitational/teleport/api/types/header" "github.com/stretchr/testify/require" ) +func TestDeduplicateOwners(t *testing.T) { + accessList, err := NewAccessList( + header.Metadata{ + Name: "duplicate test", + }, + Spec{ + Title: "title", + Description: "test access list", + Owners: []Owner{ + { + Name: "test-user1", + Description: "test user 1", + }, + { + Name: "test-user2", + Description: "test user 2", + }, + { + Name: "test-user2", + Description: "duplicate", + }, + }, + Audit: Audit{ + Frequency: time.Hour, + }, + MembershipRequires: Requires{ + Roles: []string{"mrole1", "mrole2"}, + Traits: map[string][]string{ + "mtrait1": {"mvalue1", "mvalue2"}, + "mtrait2": {"mvalue3", "mvalue4"}, + }, + }, + OwnershipRequires: Requires{ + Roles: []string{"orole1", "orole2"}, + Traits: map[string][]string{ + "otrait1": {"ovalue1", "ovalue2"}, + "otrait2": {"ovalue3", "ovalue4"}, + }, + }, + Grants: Grants{ + Roles: []string{"grole1", "grole2"}, + Traits: map[string][]string{ + "gtrait1": {"gvalue1", "gvalue2"}, + "gtrait2": {"gvalue3", "gvalue4"}, + }, + }, + }, + ) + require.NoError(t, err) + + require.Len(t, accessList.Spec.Owners, 2) + require.Equal(t, "test-user1", accessList.Spec.Owners[0].Name) + require.Equal(t, "test user 1", accessList.Spec.Owners[0].Description) + require.Equal(t, "test-user2", accessList.Spec.Owners[1].Name) + require.Equal(t, "test user 2", accessList.Spec.Owners[1].Description) +} + func TestAuditMarshaling(t *testing.T) { audit := Audit{ Frequency: time.Hour, diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index 312dabe45ae55..06ae53e2ab993 100644 --- a/lib/services/local/access_list.go +++ b/lib/services/local/access_list.go @@ -112,6 +112,13 @@ func (a *AccessListService) GetAccessList(ctx context.Context, name string) (*ac // UpsertAccessList creates or updates an access list resource. func (a *AccessListService) UpsertAccessList(ctx context.Context, accessList *accesslist.AccessList) (*accesslist.AccessList, error) { err := a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { + ownerMap := map[string]bool{} + for _, owner := range accessList.Spec.Owners { + if ownerMap[owner.Name] { + return trace.AlreadyExists("owner %s already exists in the owner list", owner.Name) + } + ownerMap[owner.Name] = true + } return trace.Wrap(a.service.UpsertResource(ctx, accessList)) }) if err != nil { diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index ff39a128bf5c8..aaee4487e76a2 100644 --- a/lib/services/local/access_list_test.go +++ b/lib/services/local/access_list_test.go @@ -124,6 +124,42 @@ func TestAccessListCRUD(t *testing.T) { out, err = service.GetAccessLists(ctx) require.NoError(t, err) require.Empty(t, out) + + // Try to create an access list with duplicate owners. + accessListDuplicateOwners := newAccessList(t, "accessListDuplicateOwners") + accessListDuplicateOwners.Spec.Owners = append(accessListDuplicateOwners.Spec.Owners, accessListDuplicateOwners.Spec.Owners[0]) + + _, err = service.UpsertAccessList(ctx, accessListDuplicateOwners) + require.True(t, trace.IsAlreadyExists(err)) +} + +func TestAccessListDedupeOwnersBackwardsCompat(t *testing.T) { + ctx := context.Background() + clock := clockwork.NewFakeClock() + + mem, err := memory.New(memory.Config{ + Context: ctx, + Clock: clock, + }) + require.NoError(t, err) + + service, err := NewAccessListService(backend.NewSanitizer(mem), clock) + require.NoError(t, err) + + // Put an unduplicated owners access list in the backend. + accessListDuplicateOwners := newAccessList(t, "accessListDuplicateOwners") + accessListDuplicateOwners.Spec.Owners = append(accessListDuplicateOwners.Spec.Owners, accessListDuplicateOwners.Spec.Owners[0]) + require.Len(t, accessListDuplicateOwners.Spec.Owners, 3) + + item, err := service.service.MakeBackendItem(accessListDuplicateOwners, accessListDuplicateOwners.GetName()) + require.NoError(t, err) + _, err = mem.Put(ctx, item) + require.NoError(t, err) + + accessList, err := service.GetAccessList(ctx, accessListDuplicateOwners.GetName()) + require.NoError(t, err) + + require.Len(t, accessList.Spec.Owners, 2) } func TestAccessListUpsertWithMembers(t *testing.T) { From 61c5cdbe7f2cbd414760980929fdd58cc6d0593a Mon Sep 17 00:00:00 2001 From: Mike Wilson Date: Mon, 25 Sep 2023 09:06:56 -0400 Subject: [PATCH 2/2] Use map struct, run GCI. --- api/types/accesslist/accesslist.go | 6 +++--- api/types/accesslist/accesslist_test.go | 3 ++- lib/services/local/access_list.go | 6 +++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/types/accesslist/accesslist.go b/api/types/accesslist/accesslist.go index 0cf7e07cdb988..ace9f67a0c8f6 100644 --- a/api/types/accesslist/accesslist.go +++ b/api/types/accesslist/accesslist.go @@ -170,18 +170,18 @@ func (a *AccessList) CheckAndSetDefaults() error { // Deduplicate owners. The backend will currently prevent this, but it's possible that access lists // were created with duplicated owners before the backend checked for duplicate owners. In order to // ensure that these access lists are backwards compatible, we'll deduplicate them here. - ownerMap := map[string]bool{} + ownerMap := make(map[string]struct{}, len(a.Spec.Owners)) deduplicatedOwners := []Owner{} for _, owner := range a.Spec.Owners { if owner.Name == "" { return trace.BadParameter("owner name is missing") } - if ownerMap[owner.Name] { + if _, ok := ownerMap[owner.Name]; ok { continue } - ownerMap[owner.Name] = true + ownerMap[owner.Name] = struct{}{} deduplicatedOwners = append(deduplicatedOwners, owner) } a.Spec.Owners = deduplicatedOwners diff --git a/api/types/accesslist/accesslist_test.go b/api/types/accesslist/accesslist_test.go index b8ea9b4738693..4f2e92594a139 100644 --- a/api/types/accesslist/accesslist_test.go +++ b/api/types/accesslist/accesslist_test.go @@ -21,8 +21,9 @@ import ( "testing" "time" - "github.com/gravitational/teleport/api/types/header" "github.com/stretchr/testify/require" + + "github.com/gravitational/teleport/api/types/header" ) func TestDeduplicateOwners(t *testing.T) { diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index 06ae53e2ab993..6a73bab7fe3a0 100644 --- a/lib/services/local/access_list.go +++ b/lib/services/local/access_list.go @@ -112,12 +112,12 @@ func (a *AccessListService) GetAccessList(ctx context.Context, name string) (*ac // UpsertAccessList creates or updates an access list resource. func (a *AccessListService) UpsertAccessList(ctx context.Context, accessList *accesslist.AccessList) (*accesslist.AccessList, error) { err := a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - ownerMap := map[string]bool{} + ownerMap := make(map[string]struct{}, len(accessList.Spec.Owners)) for _, owner := range accessList.Spec.Owners { - if ownerMap[owner.Name] { + if _, ok := ownerMap[owner.Name]; ok { return trace.AlreadyExists("owner %s already exists in the owner list", owner.Name) } - ownerMap[owner.Name] = true + ownerMap[owner.Name] = struct{}{} } return trace.Wrap(a.service.UpsertResource(ctx, accessList)) })