From 4abb0f4c8df64114a19e00babc975a4f365ea385 Mon Sep 17 00:00:00 2001 From: Trent Clarke Date: Tue, 10 Dec 2024 22:40:51 +1100 Subject: [PATCH] Fixes IC Account Assignment role matcher (#49977) * Fixes IC Account Assignment role matcher Fixes issue where the IC Account Assignment role matcher constructor created the wrong type of Role Matcher, meaning that the any Account Assignment would match as long as the account matched, regardless of the Permission set. * tests --- lib/auth/auth_with_roles_test.go | 166 ++++++++++++++++++++++++++----- lib/services/identitycenter.go | 9 +- 2 files changed, 145 insertions(+), 30 deletions(-) diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index ee7fa9158c035..dc4ba4ee2e647 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -5778,7 +5778,28 @@ func TestListUnifiedResources_WithPredicate(t *testing.T) { require.Error(t, err) } +func withAccountAssignment(condition types.RoleConditionType, accountID, permissionSet string) CreateUserAndRoleOption { + return WithRoleMutator(func(role types.Role) { + r := role.(*types.RoleV6) + cond := &r.Spec.Deny + if condition == types.Allow { + cond = &r.Spec.Allow + } + cond.AccountAssignments = append( + cond.AccountAssignments, + types.IdentityCenterAccountAssignment{ + Account: accountID, + PermissionSet: permissionSet, + }) + }) +} + func TestUnifiedResources_IdentityCenter(t *testing.T) { + const ( + validAccountID = "11111111" + validPermissionSetARN = "some:ps:arn" + ) + ctx := context.Background() srv := newTestTLSServer(t, withCacheEnabled(true)) @@ -5786,16 +5807,15 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { return srv.Auth().UnifiedResourceCache.IsInitialized() }, 5*time.Second, 200*time.Millisecond, "unified resource watcher never initialized") - setAccountAssignment := func(role types.Role) { - r := role.(*types.RoleV6) - r.Spec.Allow.AccountAssignments = []types.IdentityCenterAccountAssignment{ - { - Account: "11111111", - PermissionSet: "some:arn", - }, - } + allowByGenericKind := []types.Rule{ + types.NewRule(types.KindIdentityCenter, services.RO()), } + // adds a Rule ALLOW condition for the valid account ID and Permission set + // pair + withMatchingAccountAssignment := withAccountAssignment(types.Allow, + validAccountID, validPermissionSetARN) + testCases := []struct { name string kind string @@ -5816,8 +5836,8 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { }, }, Spec: &identitycenterv1.AccountSpec{ - Id: "11111111", - Arn: "some:arn", + Id: validAccountID, + Arn: "some:account:arn", Name: "Test Account", }, }, @@ -5854,11 +5874,11 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { }, }, Spec: &identitycenterv1.AccountAssignmentSpec{ - AccountId: "11111111", + AccountId: validAccountID, Display: "Test Account Assignment", PermissionSet: &identitycenterv1.PermissionSetInfo{ - Arn: "some:arn", - Name: "Test Account", + Arn: validPermissionSetARN, + Name: "Test permission set", AssignmentId: "Test Assignment on Test Account", }, }, @@ -5887,6 +5907,10 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { t.Run(test.name, func(t *testing.T) { test.init(t) + allowBySpecificKind := []types.Rule{ + types.NewRule(test.kind, services.RO()), + } + t.Run("no access", func(t *testing.T) { userNoAccess, _, err := CreateUserAndRole(srv.Auth(), "no-access", nil, nil) require.NoError(t, err) @@ -5906,12 +5930,53 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { require.Empty(t, resp.Resources) }) + t.Run("no access via no matching account condition ", func(t *testing.T) { + userNoAccess, _, err := CreateUserAndRole(srv.Auth(), "no-access-account-mismatch", nil, + allowByGenericKind, + withAccountAssignment(types.Allow, "22222222", validPermissionSetARN)) + require.NoError(t, err) + + identity := TestUser(userNoAccess.GetName()) + clt, err := srv.NewClient(identity) + require.NoError(t, err) + defer clt.Close() + + resp, err := clt.ListResources(ctx, proto.ListResourcesRequest{ + ResourceType: test.kind, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + }) + require.NoError(t, err) + require.Empty(t, resp.Resources) + }) + + t.Run("access denied by account deny condition", func(t *testing.T) { + userNoAccess, _, err := CreateUserAndRole(srv.Auth(), "no-access-account-mismatch", nil, + allowBySpecificKind, + withMatchingAccountAssignment, + withAccountAssignment(types.Deny, validAccountID, "*")) + require.NoError(t, err) + + identity := TestUser(userNoAccess.GetName()) + clt, err := srv.NewClient(identity) + require.NoError(t, err) + defer clt.Close() + + resp, err := clt.ListResources(ctx, proto.ListResourcesRequest{ + ResourceType: test.kind, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + }) + require.NoError(t, err) + require.Empty(t, resp.Resources) + }) + t.Run("access via generic kind", func(t *testing.T) { user, _, err := CreateUserAndRole(srv.Auth(), "read-generic", nil, - []types.Rule{ - types.NewRule(types.KindIdentityCenter, services.RO()), - }, - WithRoleMutator(setAccountAssignment)) + allowByGenericKind, + withMatchingAccountAssignment) require.NoError(t, err) identity := TestUser(user.GetName()) @@ -5931,10 +5996,8 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { t.Run("access via specific kind", func(t *testing.T) { user, _, err := CreateUserAndRole(srv.Auth(), "read-specific", nil, - []types.Rule{ - types.NewRule(test.kind, services.RO()), - }, - WithRoleMutator(setAccountAssignment)) + allowBySpecificKind, + withMatchingAccountAssignment) require.NoError(t, err) identity := TestUser(user.GetName()) @@ -5949,13 +6012,11 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { require.Len(t, resp.Resources, 1) }) - t.Run("denied via specific kind beats allow via generic kind", func(t *testing.T) { + t.Run("access denied via specific kind beats allow via generic kind", func(t *testing.T) { user, _, err := CreateUserAndRole(srv.Auth(), "specific-beats-generic", nil, - []types.Rule{ - types.NewRule(types.KindIdentityCenter, services.RO()), - }, + allowByGenericKind, + withMatchingAccountAssignment, WithRoleMutator(func(r types.Role) { - setAccountAssignment(r) r.SetRules(types.Deny, []types.Rule{ types.NewRule(test.kind, services.RO()), }) @@ -5973,6 +6034,59 @@ func TestUnifiedResources_IdentityCenter(t *testing.T) { require.True(t, trace.IsAccessDenied(err), "Expected Access Denied, got %v", err) }) + + // The tests below this point are only applicable to Identity Center + // Account assignments + if test.kind == types.KindIdentityCenterAccount { + return + } + + // Asserts that a role ALLOW condition with a matching Account ID but + // nonmatching PermissionSet ARN does not allow access + t.Run("no access via no matching allow permission set condition", func(t *testing.T) { + userNoAccess, _, err := CreateUserAndRole(srv.Auth(), "no-access-allow-ps-mismatch", nil, + allowByGenericKind, + withAccountAssignment(types.Allow, validAccountID, "some:other:ps:arn")) + require.NoError(t, err) + + identity := TestUser(userNoAccess.GetName()) + clt, err := srv.NewClient(identity) + require.NoError(t, err) + defer clt.Close() + + resp, err := clt.ListResources(ctx, proto.ListResourcesRequest{ + ResourceType: test.kind, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + }) + require.NoError(t, err) + require.Empty(t, resp.Resources) + }) + + // Asserts that a role DENY condition with a matching Account ID but + // nonmatching PermissionSet ARN does not block access + t.Run("access via no matching deny permission set condition", func(t *testing.T) { + userNoAccess, _, err := CreateUserAndRole(srv.Auth(), "access-deny-ps-mismatch", nil, + allowByGenericKind, + withMatchingAccountAssignment, + withAccountAssignment(types.Deny, "*", "some:other:ps")) + require.NoError(t, err) + + identity := TestUser(userNoAccess.GetName()) + clt, err := srv.NewClient(identity) + require.NoError(t, err) + defer clt.Close() + + resp, err := clt.ListResources(ctx, proto.ListResourcesRequest{ + ResourceType: test.kind, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + }) + require.NoError(t, err) + require.Len(t, resp.Resources, 1) + }) }) } } diff --git a/lib/services/identitycenter.go b/lib/services/identitycenter.go index 7418dedf6be40..5bac0349b804b 100644 --- a/lib/services/identitycenter.go +++ b/lib/services/identitycenter.go @@ -244,7 +244,7 @@ type IdentityCenter interface { // NewIdentityCenterAccountMatcher creates a new [RoleMatcher] configured to // match the supplied [IdentityCenterAccount]. -func NewIdentityCenterAccountMatcher(account IdentityCenterAccount) RoleMatcher { +func NewIdentityCenterAccountMatcher(account IdentityCenterAccount) *IdentityCenterAccountMatcher { return &IdentityCenterAccountMatcher{ accountID: account.GetSpec().GetId(), } @@ -280,9 +280,10 @@ func (m *IdentityCenterAccountMatcher) String() string { // NewIdentityCenterAccountAssignmentMatcher creates a new [IdentityCenterAccountAssignmentMatcher] // configured to match the supplied [IdentityCenterAccountAssignment]. -func NewIdentityCenterAccountAssignmentMatcher(account IdentityCenterAccountAssignment) RoleMatcher { - return &IdentityCenterAccountMatcher{ - accountID: account.GetSpec().GetAccountId(), +func NewIdentityCenterAccountAssignmentMatcher(assignment IdentityCenterAccountAssignment) *IdentityCenterAccountAssignmentMatcher { + return &IdentityCenterAccountAssignmentMatcher{ + accountID: assignment.GetSpec().GetAccountId(), + permissionSetARN: assignment.GetSpec().GetPermissionSet().Arn, } }