-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(iam): Assume role actions #16725
feat(iam): Assume role actions #16725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of the unnecessary prettier
-induced changes. They obscure what's going on to me.
Can you please revert those so we can get back to the essence of this change?
ba0868b
to
2ac6cb3
Compare
@@ -141,6 +151,25 @@ export abstract class PrincipalBase implements IPrincipal { | |||
public withConditions(conditions: Conditions): IPrincipal { | |||
return new PrincipalWithConditions(this, conditions); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts here are that it would be very difficult to change the public API of all the Principals.
However, that might be a better long term solution.
Additionally another approach would be to not attempt to support extension at all, and just create a new class e.g. PrincipalWithAssumeRoleActions
.
Please give some advise 👍
@@ -486,7 +486,12 @@ export interface IRole extends IIdentity { | |||
function createAssumeRolePolicy(principal: IPrincipal, externalIds: string[]) { | |||
const statement = new AwsStarStatement(); | |||
statement.addPrincipals(principal); | |||
statement.addActions(principal.assumeRoleAction); | |||
|
|||
if (Array.isArray(principal.assumeRoleActions) && principal.assumeRoleActions.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we have assumeRoleActions, which includes more than just the base assumeRoleAction
then we would favour the full set of actions from the array, instead of the single action.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Hi @simonireilly, thanks for the submission. I got inspired while reviewing this and figured it's long past time to lift some old limitations. I decided to try and tackle this differently. Have a look at this and see if this would solve the problem for you: #17689 |
Awesome! Let's close this draft, I look forward to using this feature, thanks @rix0rrr |
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*
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*
fixes #15908
Description
Assume role policies can have a string array of actions.
To support this, if the principal has the optional attribute:
Then it is used as the actions, in the created AssumeRolePolicy.
The default behaviour; of a string being assigned, is not changed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license