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

PolicyPrincipal assumeRoleAction should support multiple actions #2041

Closed
reillykw opened this issue Mar 18, 2019 · 5 comments · Fixed by #17689
Closed

PolicyPrincipal assumeRoleAction should support multiple actions #2041

reillykw opened this issue Mar 18, 2019 · 5 comments · Fixed by #17689
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@reillykw
Copy link
Contributor

currently, assumeRoleAction for PolicyPrincipals in the iam package doesn't support multiple actions and is defined as a string. Some orgs require assumeRole and assumeRoleWithSAML and this currently isn't possible. This type should support string | Array<string>

@SomayaB SomayaB added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Sep 9, 2019
@rix0rrr rix0rrr removed the needs-triage This issue or PR still needs to be triaged. label Sep 11, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 30, 2019

Still relevant.

@reillykw
Copy link
Contributor Author

reillykw commented Nov 7, 2019

So I submitted a PR months ago and it was rejected due to L2 construct attribute rules. Without modifying that attribute, I'm uncertain how this would be achieved. Any tips would be helpful.

Some ideas:

  • Make the attribute an Array only.

Would I need to modify any L1 classes or attributes?

@reillykw
Copy link
Contributor Author

reillykw commented Nov 7, 2019

So I was looking at the CompositePrincipal class and it's still limited because it inherits from PrincipalBase where the assumeRoleAction is a string type. Wouldn't accepting an array for the action type allow multiple actions on a single principal and at least close this issue out? I'm trying to catch up on how all of these objects interact with the synthesizer (how they actually get translated into json cloudformation) as well, so if you could direct me to where that takes place I can read up on the code. I'm digging through the repo now.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Nov 8, 2019
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Feb 28, 2020
@rix0rrr rix0rrr added the p1 label Aug 12, 2020
@ericzbeard ericzbeard added the feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. label Apr 20, 2021
@rix0rrr rix0rrr removed their assignment Jun 18, 2021
@miekassu
Copy link

still relevant

rix0rrr added a commit that referenced this issue Nov 24, 2021
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes #15908, closes #16725, fixes #2041, fixes #1578.
@mergify mergify bot closed this as completed in #17689 Dec 16, 2021
mergify bot pushed a commit that referenced this issue Dec 16, 2021
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes #15908, closes #16725, fixes #2041, fixes #1578.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes aws#15908, closes aws#16725, fixes aws#2041, fixes aws#1578.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
5 participants