Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter IC Permission Sets based on access #49936

Merged
merged 5 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ func (a *AppV3) GetDisplayName() string {
if a.Spec.IdentityCenter == nil {
return ""
}
return a.GetName()
return a.Metadata.Description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not related to "Filter IC Permission Sets based on access" right ?

Just want to make sure that this is not leftover from local stash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to change the name value of the "app" to be the account ID - for two reasons:

  1. so I can pull it out to build the dummy Account Assignment that we use to query access
  2. to avoid the uniqueness issues we discussed yesterday

The knock-on effect of this is that the friendly name is now pulled from the app 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
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 @@ -1272,7 +1274,7 @@ func (c *resourceAccess) checkAccess(resource types.ResourceWithLabels, filter s
return false, 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 @@ -1474,6 +1476,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 {
tcsc marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -1490,6 +1502,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the assignment.Metadata.Name needs to be updated. Does the checker.checkAccess RBAC check depends on the resource name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - when it writes to the requestable map

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 {
Copy link
Contributor

@smallinsky smallinsky Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the checker.requestableMap map is updated here? This filterICPermissionSets function is called after the checker.requestableMap map is used in

paginatedResources, err := services.MakePaginatedResources(ctx, types.KindUnifiedResource, unifiedResources, resourceAccess.requestableMap)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The access check for the AccountAssignment on line 1530 modifies the requestableMap as a side effect, and that's how we pull out whether the assignment requires an Access Request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Thanks. I had in mind the previous logic model but for this flow where the checker.checkAccess is reused that makes sens.

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.actionNamespace(namespace, types.KindNode, types.VerbList); err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -1831,8 +1893,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
Original file line number Diff line number Diff line change
Expand Up @@ -9628,3 +9628,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
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
Loading