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

Adds KindIdentityCenter umbrella resource kind #48730

Merged
merged 6 commits into from
Nov 12, 2024
Merged

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Nov 11, 2024

The Identity Center integration manages several resource types,
and specifying individal condition statements for each kind is
both unwieldy and unnecessary - anyone that can manipulate one
of these resources should be able to manilpate them all in the
same way.

In order to simplify things, this patch introduces an umbrella
KindIdentityCenter that will represent any KindIdentityCenter*
resource in Role conditions and RBAC checks.

The Identity Center integration manages several resource types,
and specifying individal condition statements for each kind is
both unwieldy and unnecessary - anyone that can manipulate one
of these resources should be able to manilpate them all in the
same way.

In order to simplify things, this patch introduces an umbrella
`KindIdentityCenter` that will represent _any_ `KindIdentityCenter*`
resource in Role conditions and RBAC checks.
@tcsc tcsc added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Nov 11, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48730.d3pp5qlev8mo18.amplifyapp.com

Updates the default implicit rules to refrence the new `KindIdentityCenter`
resource kind. Also updates comments on the covered  `KindIdentityCenter*`
kinds with a reminder to user `KindIdentityCenter` in RBAC checks.
// KindIdentityCenterAccount describes an Identity-Center managed AWS Account
// DO NOT USE THIS KIND IN RBAC CHECKS: use KindIdentityCenter instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems error prone.

What if we made these constants the SubKind?

Copy link
Collaborator

@r0mant r0mant Nov 11, 2024

Choose a reason for hiding this comment

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

I don't know if we need to make these sub-kinds (that would probably complicate existing resources/caching quite a bit) but @tcsc what we usually do in these cases is in the ACL layer you check whether identity has either specific permission or the more broad one so e.g. in the service methods that deal with IC accounts, you would see check KindIdentityCenterAccount or KindIdentityCenter. We do this with e.g. connectors - there are kinds for each specific connector type and the kind that covers all auth connectors.

So I would do that, and remove these "DO NOT USE ..." comments because they do look error prone. I also don't see where this new kind is being used, only that it's added to the default roles - is that PR coming to teleport.e next?

@tcsc tcsc enabled auto-merge November 12, 2024 11:57
@tcsc tcsc added this pull request to the merge queue Nov 12, 2024
Merged via the queue into master with commit 32b18a4 Nov 12, 2024
41 checks passed
@tcsc tcsc deleted the tcsc/idc-umbrella-kind branch November 12, 2024 12:20
@public-teleport-github-review-bot

@tcsc See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants