Skip to content

Commit

Permalink
[v17] Filter IC Permission Sets based on access (#50191)
Browse files Browse the repository at this point in the history 
* [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 <[email protected]>

* fix bad cherrypick

---------

Co-authored-by: Marek Smoliński <[email protected]>
  • Loading branch information
@tcsc @smallinsky
tcsc and smallinsky authored Dec 13, 2024
1 parent 94a872a commit d7face0
Show file tree
Hide file tree
Showing 5 changed files with 306 additions and 33 deletions.
2 changes: 1 addition & 1 deletion api/types/app.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
78 changes: 75 additions & 3 deletions lib/auth/auth_with_roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
}
}

Expand Down
203 changes: 203 additions & 0 deletions lib/auth/auth_with_roles_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
4 changes: 2 additions & 2 deletions lib/services/local/identitycenter_events.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit d7face0

Please sign in to comment.