From d7face000e16795b2535208371fbad573324c249 Mon Sep 17 00:00:00 2001 From: Trent Clarke Date: Sat, 14 Dec 2024 01:38:18 +1100 Subject: [PATCH] [v17] Filter IC Permission Sets based on access (#50191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [v17] Filter IC Permission Sets based on access Backports #49936 If a user is granted access to an IC Permission Set via an access request, the UI treats the referenced account as if all Permission Sets are granted. This hides the requestable Permission Sets from the user, making it impossible for a user to requests more Permission Sets on that account. This patch filters the Permission Sets delivered to the UI based on whether the user already has access the Permission Sets and marks any requestable Permission Sets on the account. This provides the UI with enough information to decide how to display the Permission Set list in order to get around the issue. --------- Co-authored-by: Marek Smoliński * fix bad cherrypick --------- Co-authored-by: Marek Smoliński --- api/types/app.go | 2 +- lib/auth/auth_with_roles.go | 78 +++++++- lib/auth/auth_with_roles_test.go | 203 ++++++++++++++++++++ lib/services/local/identitycenter_events.go | 4 +- lib/services/unified_resource.go | 52 +++-- 5 files changed, 306 insertions(+), 33 deletions(-) diff --git a/api/types/app.go b/api/types/app.go index 60ac4cafc67fd..3805224b9a7fe 100644 --- a/api/types/app.go +++ b/api/types/app.go @@ -421,7 +421,7 @@ func (a *AppV3) GetDisplayName() string { if a.Spec.IdentityCenter == nil { return "" } - return a.GetName() + return a.Metadata.Description } // IsEqual determines if two application resources are equivalent to one another. diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 5df568395a0c2..e522843c6daa7 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -40,6 +40,8 @@ import ( "github.com/gravitational/teleport/api/constants" apidefaults "github.com/gravitational/teleport/api/defaults" auditlogpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/auditlog/v1" + headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1" + identitycenterv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/identitycenter/v1" mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/internalutils/stream" "github.com/gravitational/teleport/api/metadata" @@ -1301,7 +1303,7 @@ func (c *resourceAccess) checkAccess(resource types.ResourceWithLabels, filter s return true, nil } - // check access normally if base checker doesnt exist + // check access normally if base checker doesn't exist if c.baseAuthChecker == nil { if err := c.accessChecker.CanAccess(resource); err != nil { if trace.IsAccessDenied(err) { @@ -1503,6 +1505,16 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L } r.Logins = logins } else if d := r.GetAppServer(); d != nil { + // Apps representing an Identity Center Account have a collection of Permission Sets + // that can be thought of as individually-addressable sub-resources. To present a consitent + // view of the account we check access for each Permission Set, filter out those that have + // no access and treat the whole app as requiring an access request if _any_ of the contained + // permission sets require one. + if err := a.filterICPermissionSets(r, d.GetApp(), resourceAccess); err != nil { + log.WithError(err).WithField("resource", d.GetApp().GetName()).Warn("Unable to filter ") + continue + } + logins, err := checker.GetAllowedLoginsForResource(d.GetApp()) if err != nil { log.WithError(err).WithField("resource", d.GetApp().GetName()).Warn("Unable to determine logins for app") @@ -1519,6 +1531,56 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L }, nil } +func (a *ServerWithRoles) filterICPermissionSets(r *proto.PaginatedResource, app types.Application, checker *resourceAccess) error { + appV3, ok := app.(*types.AppV3) + if !ok { + return trace.BadParameter("resource must be an app") + } + + pss := appV3.Spec.IdentityCenter.GetPermissionSets() + if pss == nil { + return nil + } + + assignment := services.IdentityCenterAccountAssignment{ + AccountAssignment: &identitycenterv1.AccountAssignment{ + Kind: types.KindIdentityCenterAccountAssignment, + Version: types.V1, + Metadata: &headerv1.Metadata{}, + Spec: &identitycenterv1.AccountAssignmentSpec{ + AccountId: appV3.GetName(), + PermissionSet: &identitycenterv1.PermissionSetInfo{}, + }, + }, + } + permissionSetQuery := assignment.Spec.PermissionSet + checkable := types.Resource153ToResourceWithLabels(assignment) + + var output []*types.IdentityCenterPermissionSet + for _, ps := range pss { + assignment.Metadata.Name = ps.AssignmentID + permissionSetQuery.Arn = ps.ARN + + hasAccess, err := checker.checkAccess(checkable, services.MatchResourceFilter{ + ResourceKind: types.KindIdentityCenterAccountAssignment, + }) + if err != nil { + return trace.Wrap(err) + } + + if !hasAccess { + continue + } + output = append(output, ps) + if _, requestable := checker.requestableMap[ps.AssignmentID]; requestable { + r.RequiresRequest = true + } + } + appV3.Spec.IdentityCenter.PermissionSets = output + + return nil +} + func (a *ServerWithRoles) GetNodes(ctx context.Context, namespace string) ([]types.Server, error) { if err := a.action(namespace, types.KindNode, types.VerbList); err != nil { return nil, trace.Wrap(err) @@ -1860,8 +1922,18 @@ func (r resourceChecker) CanAccess(resource types.Resource) error { return r.CheckAccess(rr, state) case types.Resource153Unwrapper: - if checkable, ok := rr.(services.AccessCheckable); ok { - return r.CheckAccess(checkable, state) + checkable, isCheckable := rr.(services.AccessCheckable) + if isCheckable { + switch unwrapped := rr.Unwrap().(type) { + case services.IdentityCenterAccount: + return r.CheckAccess(checkable, state, services.NewIdentityCenterAccountMatcher(unwrapped)) + + case services.IdentityCenterAccountAssignment: + return r.CheckAccess(checkable, state, services.NewIdentityCenterAccountAssignmentMatcher(unwrapped)) + + default: + return r.CheckAccess(checkable, state) + } } } diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index 458787a2288b9..fdf1b644d4de0 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -9627,3 +9627,206 @@ func TestRoleRequestReasonModeValidation(t *testing.T) { }) } } + +func testUserName(testName string) string { + return strings.ReplaceAll(testName, " ", "_") +} + +func TestFilterIdentityCenterPermissionSets(t *testing.T) { + const ( + allAccessRoleName = "all-access" + accountID = "1234567890" + permissionSetArnPrefix = "aws:awn:test:permission:set:" + ) + + // GIVEN a test cluster... + ctx := context.Background() + srv := newTestTLSServer(t) + s := newTestServerWithRoles(t, srv.AuthServer, types.RoleAdmin) + + // GIVEN an Identity Center Account with some associated Permission Set + // resources + permissionSets := []*identitycenterv1.PermissionSetInfo{ + { + Name: "PS One", + Arn: permissionSetArnPrefix + "one", + AssignmentId: accountID + "-" + "ps_one", + }, + { + Name: "PS Two", + Arn: permissionSetArnPrefix + "two", + AssignmentId: accountID + "-" + "ps_two", + }, + { + Name: "PS Three", + Arn: permissionSetArnPrefix + "ps_three", + AssignmentId: accountID + "-" + "ps_three", + }, + } + + _, err := s.authServer.CreateIdentityCenterAccount(ctx, + services.IdentityCenterAccount{ + Account: &identitycenterv1.Account{ + Kind: types.KindIdentityCenterAccount, + Version: types.V1, + Metadata: &headerv1.Metadata{ + Name: accountID, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + }, + Spec: &identitycenterv1.AccountSpec{ + Id: accountID, + Arn: "aws:arn:test:account", + Name: "Test Account", + Description: "An account for testing", + PermissionSetInfo: permissionSets, + }, + }, + }) + require.NoError(t, err) + + // GIVEN a role that allows access to all permission sets on the target + // Identity Center account + roleAccessAll, err := types.NewRole(allAccessRoleName, types.RoleSpecV6{ + Allow: types.RoleConditions{ + AccountAssignments: []types.IdentityCenterAccountAssignment{ + { + Account: accountID, + PermissionSet: types.Wildcard, + }, + }, + }, + }) + require.NoError(t, err, "Constructing role should succeed") + _, err = srv.Auth().CreateRole(ctx, roleAccessAll) + require.NoError(t, err, "Cretaing role should succeed") + + withRequesterRole := WithRoleMutator(func(role types.Role) { + r := role.(*types.RoleV6) + r.Spec.Allow.Request = &types.AccessRequestConditions{ + SearchAsRoles: []string{allAccessRoleName}, + } + }) + + // EXPECT that the IC Account has made it to the cache + inlineEventually(t, + func() bool { + testAssignments, _, err := srv.Auth().ListIdentityCenterAccounts( + ctx, 100, &pagination.PageRequestToken{}) + require.NoError(t, err) + return len(testAssignments) == 1 + }, + 5*time.Second, 200*time.Millisecond, + "Target resource missing from cache") + + testCases := []struct { + name string + roleModifiers []CreateUserAndRoleOption + includeRequestable bool + expectedPSs []*types.IdentityCenterPermissionSet + expectedRequireRequest require.BoolAssertionFunc + }{ + { + name: "basic access", + roleModifiers: []CreateUserAndRoleOption{ + withAccountAssignment(types.Allow, accountID, permissionSets[0].Arn), + withAccountAssignment(types.Allow, accountID, permissionSets[1].Arn), + }, + expectedPSs: []*types.IdentityCenterPermissionSet{ + paginatedAppPermissionSet(permissionSets[0]), + paginatedAppPermissionSet(permissionSets[1]), + }, + expectedRequireRequest: require.False, + }, + { + name: "ignore search as roles when disabled", + roleModifiers: []CreateUserAndRoleOption{ + withAccountAssignment(types.Allow, accountID, permissionSets[1].Arn), + withRequesterRole, + }, + includeRequestable: false, + expectedPSs: []*types.IdentityCenterPermissionSet{ + paginatedAppPermissionSet(permissionSets[1]), + }, + expectedRequireRequest: require.False, + }, + { + name: "requestable access", + roleModifiers: []CreateUserAndRoleOption{ + withAccountAssignment(types.Allow, accountID, permissionSets[1].Arn), + withRequesterRole, + }, + includeRequestable: true, + expectedPSs: []*types.IdentityCenterPermissionSet{ + paginatedAppPermissionSet(permissionSets[0]), + paginatedAppPermissionSet(permissionSets[1]), + paginatedAppPermissionSet(permissionSets[2]), + }, + expectedRequireRequest: require.True, + }, + { + name: "no access", + roleModifiers: []CreateUserAndRoleOption{ + withAccountAssignment(types.Allow, accountID, "some-non-existent-ps"), + }, + expectedRequireRequest: require.False, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + // GIVEN a user who has a role that allows a test-defined level of + // Identity Center access + user, _, err := CreateUserAndRole(srv.Auth(), testUserName(test.name), + nil, nil, test.roleModifiers...) + require.NoError(t, err) + + // GIVEN an auth client using the above user + identity := TestUser(user.GetName()) + clt, err := srv.NewClient(identity) + require.NoError(t, err) + t.Cleanup(func() { clt.Close() }) + + // WHEN I list the unified resources, with a filter specifically for + // the account resource defined above... + resp, err := clt.ListUnifiedResources(ctx, &proto.ListUnifiedResourcesRequest{ + Kinds: []string{types.KindApp}, + Labels: map[string]string{ + types.OriginLabel: apicommon.OriginAWSIdentityCenter, + }, + UseSearchAsRoles: test.includeRequestable, + IncludeRequestable: test.includeRequestable, + IncludeLogins: true, + SortBy: types.SortBy{IsDesc: true, Field: types.ResourceMetadataName}, + }) + + // EXPECT that the listing succeeds and returns a single resource + require.NoError(t, err) + require.Len(t, resp.Resources, 1, "Must return exactly one resource") + + // EXPECT that the contained resource has the test-defined value for + // the RequiresRequest flag + resource := resp.Resources[0] + test.expectedRequireRequest(t, resource.RequiresRequest) + + // EXPECT that the returned resource is an App + appServer := resp.Resources[0].GetAppServer() + require.NotNil(t, appServer, "Expected resource to be an app") + app := appServer.GetApp() + + // EXPECT that the app PermissionSets are filtered to the test-defined + // list + require.ElementsMatch(t, + test.expectedPSs, app.GetIdentityCenter().PermissionSets) + }) + } +} + +func paginatedAppPermissionSet(src *identitycenterv1.PermissionSetInfo) *types.IdentityCenterPermissionSet { + return &types.IdentityCenterPermissionSet{ + ARN: src.Arn, + Name: src.Name, + AssignmentID: src.AssignmentId, + } +} diff --git a/lib/services/local/identitycenter_events.go b/lib/services/local/identitycenter_events.go index b8cc8933b200b..967db5ae731bc 100644 --- a/lib/services/local/identitycenter_events.go +++ b/lib/services/local/identitycenter_events.go @@ -65,7 +65,7 @@ func (p *identityCenterAccountParser) parse(event backend.Event) (types.Resource if err != nil { return nil, trace.Wrap(err) } - return types.Resource153ToLegacy(services.IdentityCenterAccount{Account: r}), nil + return types.Resource153ToResourceWithLabels(services.IdentityCenterAccount{Account: r}), nil default: return nil, trace.BadParameter("event %v is not supported", event.Type) } @@ -109,7 +109,7 @@ func (p *identityCenterPrincipalAssignmentParser) parse(event backend.Event) (ty if err != nil { return nil, trace.Wrap(err) } - return types.Resource153ToLegacy(r), nil + return types.Resource153ToResourceWithLabels(r), nil default: return nil, trace.BadParameter("event %v is not supported", event.Type) diff --git a/lib/services/unified_resource.go b/lib/services/unified_resource.go index 31582e9dbc3ea..2cbb267caee22 100644 --- a/lib/services/unified_resource.go +++ b/lib/services/unified_resource.go @@ -20,7 +20,6 @@ package services import ( "context" - "maps" "strings" "sync" "time" @@ -1015,37 +1014,36 @@ func makePaginatedIdentityCenterAccount(resourceKind string, resource types.Reso } } - protoResource := &proto.PaginatedResource{ - Resource: &proto.PaginatedResource_AppServer{ - AppServer: &types.AppServerV3{ - Kind: types.KindAppServer, + appServer := &types.AppServerV3{ + Kind: types.KindAppServer, + Version: types.V3, + Metadata: resource.GetMetadata(), + Spec: types.AppServerSpecV3{ + App: &types.AppV3{ + Kind: types.KindApp, + SubKind: types.KindIdentityCenterAccount, Version: types.V3, - Metadata: resource.GetMetadata(), - Spec: types.AppServerSpecV3{ - App: &types.AppV3{ - Kind: types.KindApp, - SubKind: types.KindIdentityCenterAccount, - Version: types.V3, - Metadata: types.Metadata{ - Name: acct.Spec.Name, - Description: acct.Spec.Description, - Labels: maps.Clone(acct.Metadata.Labels), - }, - Spec: types.AppSpecV3{ - URI: acct.Spec.StartUrl, - PublicAddr: acct.Spec.StartUrl, - AWS: &types.AppAWS{ - ExternalID: acct.Spec.Id, - }, - IdentityCenter: &types.AppIdentityCenter{ - AccountID: acct.Spec.Id, - PermissionSets: pss, - }, - }, + Metadata: types.Metadata153ToLegacy(acct.Metadata), + Spec: types.AppSpecV3{ + URI: acct.Spec.StartUrl, + PublicAddr: acct.Spec.StartUrl, + AWS: &types.AppAWS{ + ExternalID: acct.Spec.Id, + }, + IdentityCenter: &types.AppIdentityCenter{ + AccountID: acct.Spec.Id, + PermissionSets: pss, }, }, }, }, + } + appServer.Metadata.Description = acct.Spec.Name + + protoResource := &proto.PaginatedResource{ + Resource: &proto.PaginatedResource_AppServer{ + AppServer: appServer, + }, RequiresRequest: requiresRequest, }