Exposes Identity Center accounts as Apps in Unified Resource Cache #49301

tcsc · 2024-11-21T00:29:26Z

For the purposes of the UI, Identity Center accounts and account
assignments are treated like special Apps. This patch exposes
Account Assignments to the UI via the Unified Resource Cache.

Includes:

Generating an App resource from an Identity Center Account resource
General plumbing from backend through to cache and UI

r0mant

I think this would benefit from a review from someone on the scale team as well who is familiar with unified resources cache. @rosstimothy Who would be best to take a look?

@tcsc Also, I think we're getting overly excited about generics and iterators and should dial back. It is my strong opinion that they hurt readability a lot and we should be using them very sparingly and only if there's a really good reason. I think it took me 3x more time to review this than it should have because of trying to comprehend what all these templated types do and why are they needed and I strongly suspect we do not need any of them.

api/types/role.go

api/utils/slices.go

lib/auth/auth_with_roles.go

lib/services/unified_resource.go

lib/services/unified_resource_adapter.go

lib/web/ui/app.go

rosstimothy · 2024-11-22T15:46:04Z

lib/services/unified_resource.go

@@ -470,6 +480,7 @@ func (c *UnifiedResourceCache) getResourcesAndUpdateCurrent(ctx context.Context)
 	putResources[types.KubeServer](c, newKubes)
 	putResources[types.SAMLIdPServiceProvider](c, newSAMLApps)
 	putResources[types.WindowsDesktop](c, newDesktops)
+	putResources[UnifiedResource153Adapter[IdentityCenterAccount]](c, newICAccounts)


How is this RFD 153 style resource going to work with the ListUnifiedResources RPC which requires a legacy gogo proto style resource? I thought when we discussed this in the past we decided that these should just be applications?

Accounts gets converted to an App on read

Why can't they be inserted into the unified resource cache as an app? Wouldn't that reduce/eliminate 90% of the complexity introduced in this PR?

Mainly so they are governed by the RBAC conditions for Identity Center resources, rather than app resources.

What can't RBAC be performed against the identity center resource if it detects the appropriate subkind?

lib/services/unified_resource_adapter.go

lib/web/ui/app.go

lib/auth/auth_with_roles.go

rosstimothy

I'd suggest splitting this up into more focused PRs. There is a lot of what feels like glued on functionality that isn't directly related to unified resource caching being included here.

rosstimothy · 2024-12-02T14:07:07Z

api/proto/teleport/identitycenter/v1/identitycenter.proto

+  // AssignmentName is the name of an account assignment representing this
+  // permission set being assigned to the enclosing account.
+  string assignment_name = 4;


Move this out into it's own PR and leave this PR solely dedicated to unified resource caching

api/types/constants.go

api/types/resource.go

api/types/resource_153.go

rosstimothy · 2024-12-02T14:15:26Z

api/types/resource_153.go

+	if cloner, ok := r.inner.(interface{ CloneResource() Resource153 }); ok {
+		return &Resource153ToLegacyAdapter{inner: cloner.CloneResource().(Resource153)}
+	}
+	panic("interface Resource153 does not implement CloneResource for the wrapped type")


Should we really panic here?

I'm not sure what the correct alternative is for resources that don't implement CloneResource, short of reflect.DeepCopy.

An alternative approach would be to validate this during the LegacyToResource153() call when the adapter is created. This would allow you to return an error if the object does not fulfill the required interfaces for cloning.

We can create a new private adapter like toLegacyResourceWithLabels to avoid aligning already exiting code what uses LegacyToResource153

This is why I had a separate implementation in the first place.

rosstimothy · 2024-12-02T14:16:56Z

api/types/resource_153.go

+func (r *Resource153ToLegacyAdapter) MatchSearch(searchValues []string) bool {
+	fieldVals := append(utils.MapToStrings(r.GetAllLabels()), r.GetName())
+	return MatchSearch(fieldVals, searchValues, nil)
+}


This might work now, but I can imagine that it will be too limiting for a resource that wants to customize this behavior in the future.

api/types/resource_153.go

lib/services/identitycenter.go

lib/auth/auth_with_roles.go

api/proto/teleport/legacy/types/types.proto

flyinghermit · 2024-12-03T17:36:45Z

lib/services/unified_resource.go

+							URI:        apiconstants.AWSConsoleURL,
+							PublicAddr: apiconstants.AWSConsoleURL,


The value of apiconstants.AWSConsoleURL is https://console.aws.amazon.com, which sends user to the root AWS console that lets them pick account with permission set. Since we want to launch users directly to AWS console with specific account+permission set, we need URL value that contains Identity Center instance ID and account ID.

In the POC, I used StartUrl field to contain such value https://github.com/gravitational/teleport.e/blob/sshah/idc-dev/lib/aws/identitycenter/accounts.go#L156. But in our current version, we have the StartUrl field defined but we missed to enrich the field with instance ID and account ID on initial account import.

tldr; We need to update the newIdentityCenterAccount func to enrich StartUrl field and the use the value here.

Updated the references here. Will update the account constructor in a later PR.

flyinghermit · 2024-12-03T17:39:25Z

lib/services/unified_resource.go

+							},
+							IdentityCenter: &types.AppIdentityCenter{
+								AccountID:      acct.Spec.Id,
+								PermissionSets: pss,


Given your another PR #49580 and that we use account assignment resource list to let user select desired permission set for access request, why do we need to include PermissionSets field and its value?

tcsc · 2024-12-04T12:15:13Z

Label matching removed and replaces with custom role matchers.

smallinsky · 2024-12-04T13:23:58Z

lib/auth/auth_with_roles.go

+		actionChecker := a.action
+		if kind == types.KindIdentityCenterAccount {
+			actionChecker = a.identityCenterAction
+		}


Could you lave a comment explaining the custom behavior for the types.KindIdentityCenterAccount kind ?

smallinsky · 2024-12-04T13:32:50Z

lib/services/unified_resource.go

+							URI:        acct.Spec.StartUrl,
+							PublicAddr: acct.Spec.StartUrl,


In current state the StartUrl is never set in our backend code:

e git:(tcsc/idc-ui) rg 'StartUrl' web/teleport/src/services/upgradeWindow/upgradeWindow.ts 16: .get(cfg.getWindowUpgradeStartUrl(clusterId)) 31: .post(cfg.getWindowUpgradeStartUrl(clusterId), req) web/teleport/src/config.ts 182: getWindowUpgradeStartUrl(clusterId: string) { lib/aws/identitycenter/equal/equal_test.go 145: StartUrl: "https://example.com/start", 164: StartUrl: "https://example.com/start", lib/aws/identitycenter/equal/account.go 21: s1.GetStartUrl() == s2.GetStartUrl() &&

This is coming in a change discussed with @flyinghermit

smallinsky · 2024-12-04T13:41:53Z

lib/web/ui/app.go

+	// AssignmentName is the assignment resource that will provision an Account
+	// Assignment for this Permission Set on the enclosing account
+	AssignmentName  string `json:"accountAssignment,omitempty"`
+	RequiresRequest bool   `json:"requiresRequest,omitempty"`


Right now the RequiresRequest field is never set:

teleport/lib/web/ui/app.go

Line 188 in 4bdc809

AssignmentName: srcPS.AssignmentName,

This will be used when we filter the resources based on access.

Could you add a go docs ?

lib/web/ui/app.go

smallinsky · 2024-12-04T13:44:30Z

lib/web/ui/app.go

@@ -66,6 +69,9 @@ type App struct {
 	// Integration is the integration name that must be used to access this Application.
 	// Only applicable to AWS App Access.
 	Integration string `json:"integration,omitempty"`
+	// PermissionSets holds the permission sets that this app grants access to.
+	// Only valid for Identity Center Account apps
+	PermissionSets []IdentityCenterPermissionSet `json:"permission_sets,omitempty"`


The json:camel_cases seems to be inconsistent with current json style for that uses PascalCase

https://github.com/gravitational/teleport/pull/49301/files#diff-f635fca1ca174ba271760ef6e8dbe4aa703df1ba9acde1ae8828b86fb8992631R68

smallinsky · 2024-12-04T14:09:03Z

lib/services/identitycenter.go

+			return true, nil
+		}
+
+		psMatches, err := m.matchExpression(*(m.permissionSetARN), asmt.PermissionSet)


The naming conventions are quite vague - the asmt.PermissionSet is passed to matchExpression as accountExpression:
m.matchExpression(*(m.permissionSetARN), asmt.PermissionSet)

However, the naming of the second parameter in matchExpression suggests that it should represent an accountExpression:

func (m *IdentityCenterMatcher) matchExpression(target, accountExpression string) (bool, error) {

This inconsistency in parameter naming can be confusing and might lead to misunderstandings about the purpose of the method nad match logic.

smallinsky · 2024-12-04T14:20:51Z

lib/services/identitycenter.go

+
+// NewIdentityCenterAccountAssignmentMatcher creates a new [IdentityCenterMatcher]
+// configured to match the supplied [IdentityCenterAccountAssignment].
+func NewIdentityCenterAccountAssignmentMatcher(account IdentityCenterAccountAssignment) *IdentityCenterMatcher {


The AWS Account assignment in the business domain is defined as an accountID and permissionSetARN pair. How is it possible that an Account Assignment is missing the permissionSetARN element?

This creates confusion for the reader and demonstrates tight coupling in the current implementation. The matcher logic tries to couple two distinct objects.

I recommend splitting the matcher into separate matchers for each object. This would also help avoid logic like m.permissionSetARN == nil, which is unclear without a broader understanding of the context.

For example, it is not immediately obvious why the permissionSetARN can be empty when matching IdentityCenterAccountAssignments in current code

for _, asmt := range role.GetIdentityCenterAccountAssignments(condition) { accountMatches, err := m.matchExpression(m.accountID, asmt.Account) if !accountMatches { continue } // Why permissionSetARN is nil during GetIdentityCenterAccountAssignment match check ? if m.permissionSetARN == nil { return true, nil } psMatches, err := m.matchExpression(*(m.permissionSetARN), asmt.PermissionSet) }

By decoupling the logic into separate matchers, the design would become clearer and easier to maintain and reason about business domain. It would also reduce the need for special-case-if logic.

smallinsky · 2024-12-04T14:22:14Z

lib/auth/auth_with_roles_test.go

+	})
+
+}


Suggested change

})

}

})

}

api/types/role.go

smallinsky · 2024-12-04T14:27:42Z

api/types/resource_153.go

+	if cloner, ok := r.inner.(interface{ CloneResource() Resource153 }); ok {
+		return &Resource153ToLegacyAdapter{inner: cloner.CloneResource().(Resource153)}
+	}
+	panic("interface Resource153 does not implement CloneResource for the wrapped type")


An alternative approach would be to validate this during the LegacyToResource153() call when the adapter is created. This would allow you to return an error if the object does not fulfill the required interfaces for cloning.

We can create a new private adapter like toLegacyResourceWithLabels to avoid aligning already exiting code what uses LegacyToResource153

smallinsky

I have left some comment/suggestion

Also and wonder about:
#49301 (comment)

it feels like it’s attempting to introduce multiple changes at once—such as RBAC > checks, backend updates, and adding new resources.

To make the review process more efficient and focused it might be helpful to break > these changes into separate smaller PRs his way each part can be reviewed and > discussed independently.

For example - the PR description mentions: Exposes Identity Center accounts as Apps > in Unified Resource Cache but it’s not immediately clear how or why RBAC alignment > backend changes ** adding new resources** are tied to this goal.

Splitting the changes could make the purpose of each adjustment clearer and help reviewers better understand the overall intent.

I think that this is worth to consider as it could significantly improve the review process and the overall quality of the PR.

r0mant

This makes much more sense than the previous iteration, lgtm once remaining feedback has been addressed.

r0mant · 2024-12-04T22:37:48Z

api/types/app.go

+
+// GetDisplayName fetches a human-readable display name for the App.
+func (a *AppV3) GetDisplayName() string {
+	// Only Identity Center apps have a display name at this point. Returning


Technically Okta apps also have a different display name because their actual name is an internal Okta ID. What do we use for them?

r0mant · 2024-12-04T22:42:49Z

lib/auth/auth_with_roles.go

+// umbrella `KindIdentityCenter` resource kind. This means that if access to the
+// target resource is not explicitly denied, then the user has a second chance
+// to get access via the generic resource kind.
+func (a *ServerWithRoles) identityCenterActionNamespace(namespace string, resource string, verbs ...string) error {


Nit: We literally never use a namespace other than "default", do we really need a separate identityCenterActionNamespace method?

I would consider just keeping identityCenterAction and hardcoding apidefaults.Namespace to keep things a little simpler.

r0mant · 2024-12-04T22:50:56Z

lib/services/identitycenter.go

+// NewIdentityCenterAccountAssignmentMatcher creates a new [IdentityCenterMatcher]
+// configured to match the supplied [IdentityCenterAccountAssignment].
+func NewIdentityCenterAccountAssignmentMatcher(account IdentityCenterAccountAssignment) *IdentityCenterMatcher {
+	psARN := account.GetSpec().GetPermissionSet().GetArn()


Can any of these be nil?

lib/services/identitycenter.go

lib/services/role.go

r0mant · 2024-12-04T22:56:27Z

lib/services/role.go

+		if requiresLabelMatching {
+			matchLabels, labelsMessage, err := checkRoleLabelsMatch(types.Deny, role, traits, r, isDebugEnabled)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			if matchLabels {
+				debugf("Access to %v %q denied, deny rule in role %q matched; match(namespace=%v, %s)",
+					r.GetKind(), r.GetName(), role.GetName(), namespaceMessage, labelsMessage)
+				return trace.AccessDenied("access to %v denied. User does not have permissions. %v",
+					r.GetKind(), additionalDeniedMessage)
+			}


This has been here before though and I think "allow" and "deny" code paths are slightly different so may be tricky to factor out. I think it's ok to keep as-is.

tcsc · 2024-12-05T05:06:11Z

Moved RBAC in to another PR

For the purposes of the UI, Identity Center accounts and account assignments are treated like special Apps. This patch exposes Account Assignments to the UI via the Unified Resource Cache. Includes: - Generating an App resource from an Identity Center Account resource - General plumbing from backend through to cache and UI

…' into tcsc/idc-expose-accounts-as-apps

public-teleport-github-review-bot · 2024-12-05T12:45:11Z

@tcsc See the table below for backport results.

Branch	Result
branch/v17	Failed

Backports #49301 For the purposes of the UI, Identity Center accounts and account assignments are treated like special Apps. This patch exposes Account Assignments to the UI via the Unified Resource Cache. Includes: - Generating an App resource from an Identity Center Account resource - General plumbing from backend through to cache and UI

…che (#49858) Backports #49301 For the purposes of the UI, Identity Center accounts and account assignments are treated like special Apps. This patch exposes Account Assignments to the UI via the Unified Resource Cache. Includes: - Generating an App resource from an Identity Center Account resource - General plumbing from backend through to cache and UI

tcsc added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Nov 21, 2024

github-actions bot requested review from Joerger and ryanclark November 21, 2024 00:29

github-actions bot added the size/md label Nov 21, 2024

tcsc requested review from r0mant and smallinsky November 21, 2024 04:26

r0mant requested changes Nov 21, 2024

View reviewed changes

r0mant requested review from rosstimothy and removed request for ryanclark and Joerger November 21, 2024 22:51

rosstimothy reviewed Nov 22, 2024

View reviewed changes

r0mant mentioned this pull request Nov 25, 2024

GitHub Proxy part 2: git_server resource, service, and RBAC #49393

Merged

smallinsky reviewed Nov 26, 2024

View reviewed changes

lib/services/unified_resource_adapter.go Outdated Show resolved Hide resolved

lib/web/ui/app.go Outdated Show resolved Hide resolved

smallinsky reviewed Nov 26, 2024

View reviewed changes

lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch from 72ece0d to 9d1bdad Compare November 27, 2024 08:56

tcsc requested a review from r0mant November 27, 2024 08:56

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch from eff54e2 to 34ea806 Compare November 28, 2024 03:05

tcsc added the aws-iam-identity-center label Nov 29, 2024

rosstimothy reviewed Dec 2, 2024

View reviewed changes

smallinsky reviewed Dec 2, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

flyinghermit reviewed Dec 2, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

flyinghermit reviewed Dec 3, 2024

View reviewed changes

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch from e4d174c to 1ca7aec Compare December 4, 2024 11:46

tcsc changed the base branch from master to tcsc/idc-add-assignments-to-apps December 4, 2024 11:47

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch 2 times, most recently from 8ff81be to 82f58ee Compare December 4, 2024 12:13

tcsc requested review from flyinghermit, smallinsky and rosstimothy December 4, 2024 12:13

smallinsky reviewed Dec 4, 2024

View reviewed changes

r0mant approved these changes Dec 4, 2024

View reviewed changes

tcsc force-pushed the tcsc/idc-add-assignments-to-apps branch 2 times, most recently from 3cc204d to a3c8cdb Compare December 5, 2024 00:21

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch 2 times, most recently from 512945c to 8ecf2cd Compare December 5, 2024 04:58

tcsc requested a review from smallinsky December 5, 2024 05:05

tcsc force-pushed the tcsc/idc-add-assignments-to-apps branch from f25e964 to 1e8f8bc Compare December 5, 2024 05:52

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch from 7bed34c to ba6f65d Compare December 5, 2024 06:32

Base automatically changed from tcsc/idc-add-assignments-to-apps to master December 5, 2024 06:38

tcsc force-pushed the tcsc/idc-expose-accounts-as-apps branch from ba6f65d to aa1132c Compare December 5, 2024 07:06

smallinsky approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'master' into tcsc/idc-expose-accounts-as-apps

211433d

tcsc enabled auto-merge December 5, 2024 10:54

tcsc added 3 commits December 5, 2024 22:41

lint

4fe5330

Merge remote-tracking branch 'origin/tcsc/idc-expose-accounts-as-apps…

a4b4272

…' into tcsc/idc-expose-accounts-as-apps

Merge branch 'master' into tcsc/idc-expose-accounts-as-apps

43478be

tcsc added this pull request to the merge queue Dec 5, 2024

Merged via the queue into master with commit e1be633 Dec 5, 2024
41 checks passed

tcsc deleted the tcsc/idc-expose-accounts-as-apps branch December 5, 2024 12:43

tcsc mentioned this pull request Dec 6, 2024

[v17] Exposes Identity Center accounts as Apps in Unified Resource Cache #49858

Merged

flyinghermit mentioned this pull request Dec 6, 2024

UI changes for Identity Center Resource Access Requests #49621

Merged

		URI: apiconstants.AWSConsoleURL,
		PublicAddr: apiconstants.AWSConsoleURL,

-	})
-}
+	})
+}

Exposes Identity Center accounts as Apps in Unified Resource Cache #49301

Exposes Identity Center accounts as Apps in Unified Resource Cache #49301

Conversation

tcsc commented Nov 21, 2024 • edited Loading

r0mant left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcsc commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky left a comment • edited Loading

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcsc commented Dec 5, 2024

public-teleport-github-review-bot bot commented Dec 5, 2024

tcsc commented Nov 21, 2024 •

edited

Loading

r0mant left a comment •

edited

Loading

rosstimothy Nov 27, 2024 •

edited

Loading

smallinsky Dec 4, 2024 •

edited

Loading

smallinsky Dec 4, 2024 •

edited

Loading

smallinsky left a comment •

edited

Loading