fix(iam): grantAssumeRole silently fails with service and account principals #29452

kishiel · 2024-03-11T20:45:06Z

Issue #24507

Reason for this change

grantAssumeRole silently fails if a Service Principal or Account Principal is used which led me to a false assumption about the correctness of a role's permission scope

Description of changes

This change will throw an error if a Service Principal is used. I was unable to find a way to accomplish the same behavior for Account Principals.

Documentation was updated to help guide a user to the appropriate function usage for Service and Account Principals.

Description of how you validated changes

Added a unit test
This change required me to re-run two unrelated snapshot tests which were throwing errors outside of the scope of this change.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

msambol · 2024-03-20T03:29:27Z

packages/aws-cdk-lib/aws-iam/README.md

@@ -369,6 +369,37 @@ new iam.Role(this, 'Role', {
 });
 ```

+### Granting assume role permission from a role
+
+Principals can be granted permission to assume a role using `grantAssumeRole`.  Note that this does not apply to Service Principals or Account Principals as they must be added to the role Trust Policy.


Nit: lowercase service principals [1]
Nit: lowercase account principals [2]

Also move the note to a newline.

[1] https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-services
[2] https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-accounts

msambol · 2024-03-20T03:33:10Z

packages/aws-cdk-lib/aws-iam/README.md

@@ -369,6 +369,37 @@ new iam.Role(this, 'Role', {
 });
 ```

+### Granting assume role permission from a role


"From a role" is confusing here. Can you change this to "Granting principals permission to assume a role"

msambol · 2024-03-20T03:33:28Z

packages/aws-cdk-lib/aws-iam/README.md

+role.grantAssumeRole(user);
+```
+
+### Giving Service Principals and Account Principals assume role permission from a role


"Granting service and account principals permission to assume a role"

msambol · 2024-03-20T03:33:31Z

packages/aws-cdk-lib/aws-iam/README.md

+
+### Giving Service Principals and Account Principals assume role permission from a role
+
+Service Principals and Account Principals can be granted permission to assume a role using `assumeRolePolicy` which modifies the role Trust Policy.


Trust Policy should be lowercase too.

msambol · 2024-03-20T03:41:53Z

packages/aws-cdk-lib/aws-iam/lib/role.ts

+    // Service Principals must use assumeRolePolicy
+    if (identity.policyFragment.principalJson.Service) {
+      throw new Error('Cannot use a Service Principal with grantAssumeRole, use assumeRolePolicy instead.');
+    }


I think this would work?

if (identity instanceof ServicePrincipal || identity instanceof AccountPrincipal) { throw new Error('Cannot use a service or account principal with grantAssumeRole, use assumeRolePolicy instead.'); }

Good call, this appears to have worked. Added another test for account principal.

msambol · 2024-03-20T03:43:44Z

Can you change the title? It should reflect the bug. Maybe:

fix(iam): grantAssumeRole silently fails with service and account principals

msambol

See inline. Great start!

…el/eks-ipv6 into iam_grantassumerole_filter

kishiel · 2024-03-20T15:52:20Z

Incorporated all feedback and updated title

msambol · 2024-03-20T15:54:28Z

packages/aws-cdk-lib/aws-iam/README.md

@@ -369,6 +369,39 @@ new iam.Role(this, 'Role', {
 });
 ```

+### Granting an identity permission to assume a role


Can you change this to "Granting a principal...". That's the more common verbiage.

Ah, I forgot I'd modified this. I changed this to identity based on this section of the readme but I think I'm just misreading it. Will change

msambol · 2024-03-20T15:54:46Z

packages/aws-cdk-lib/aws-iam/README.md

@@ -369,6 +369,39 @@ new iam.Role(this, 'Role', {
 });
 ```

+### Granting an identity permission to assume a role
+
+An identity can be granted permission to assume a role using `grantAssumeRole`.


Likewise here, "a principal."

msambol · 2024-03-20T15:55:11Z

packages/aws-cdk-lib/aws-iam/README.md

+
+An identity can be granted permission to assume a role using `grantAssumeRole`.
+
+Note that this does not apply to service principals or account principals as they must be added to the role trust policy.


Can you add via assumeRolePolicy.

msambol

Few more comments then we're good to go.

kishiel · 2024-03-20T16:02:01Z

Incorporated feedback

msambol

Looks good. Thanks!

GavinZZ

Generally LGTM, just a question for clarification before approving.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.ts

GavinZZ · 2024-03-28T22:59:24Z

packages/aws-cdk-lib/aws-iam/README.md

@@ -369,6 +369,39 @@ new iam.Role(this, 'Role', {
 });
 ```

+### Granting a principal permission to assume a role


Thanks for this clear documentation update.

mergify · 2024-04-02T20:19:33Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2024-04-02T20:47:35Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-04-02T21:15:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 18771f4
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-04-02T21:16:20Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

kishiel added 4 commits March 11, 2024 18:50

grantAssume errors when a service principal is used

93c4e4f

update snapshots complaining about rules unrelated to this change

a5ced68

add in account principals, update readme

dd88e30

Remove account principal filter from grantAssumeRole

c312e0a

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Mar 11, 2024

aws-cdk-automation requested a review from a team March 11, 2024 20:45

aws-cdk-automation previously requested changes Mar 11, 2024

View reviewed changes

kishiel changed the title ~~fix(iam): Throw error if grantAssumeRole uses a Service Principal~~ fix(iam): throw error if grantAssumeRole uses a Service Principal Mar 11, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 11, 2024

Merge branch 'main' into iam_grantassumerole_filter

d838904

github-actions bot mentioned this pull request Mar 18, 2024

Weekly PR metrics report #29527

Closed

msambol reviewed Mar 20, 2024

View reviewed changes

msambol suggested changes Mar 20, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 20, 2024

kishiel added 2 commits March 20, 2024 15:46

error on account principals, readme fixes

55987f3

Merge branch 'iam_grantassumerole_filter' of https://github.com/kishi…

a9c2523

…el/eks-ipv6 into iam_grantassumerole_filter

kishiel changed the title ~~fix(iam): throw error if grantAssumeRole uses a Service Principal~~ fix(iam): grantAssumeRole silently fails with service and account principals Mar 20, 2024

msambol reviewed Mar 20, 2024

View reviewed changes

correct identity to principal

0562d44

msambol approved these changes Mar 20, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 20, 2024

GavinZZ reviewed Mar 28, 2024

View reviewed changes

kishiel added 2 commits March 29, 2024 10:33

revert snapshot change

fc0a305

fix typo in readme

8e5abe9

github-actions bot mentioned this pull request Apr 1, 2024

Monthly PR metrics report #29668

Closed

GavinZZ previously approved these changes Apr 2, 2024

View reviewed changes

Merge branch 'main' into iam_grantassumerole_filter

9e972a9

GavinZZ approved these changes Apr 2, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 2, 2024

Merge branch 'main' into iam_grantassumerole_filter

18771f4

mergify bot merged commit 36fd79d into aws:main Apr 2, 2024
9 checks passed

vmx-kbabenko mentioned this pull request Apr 10, 2024

aws_iam: Cannot use a service or account principal with grantAssumeRole, use assumeRolePolicy instead. #29782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iam): grantAssumeRole silently fails with service and account principals #29452

fix(iam): grantAssumeRole silently fails with service and account principals #29452

kishiel commented Mar 11, 2024

aws-cdk-automation left a comment

msambol Mar 20, 2024 •

edited

msambol Mar 20, 2024

msambol Mar 20, 2024

msambol Mar 20, 2024

msambol Mar 20, 2024

msambol Mar 20, 2024

kishiel Mar 20, 2024

msambol commented Mar 20, 2024

msambol left a comment

kishiel commented Mar 20, 2024

msambol Mar 20, 2024

kishiel Mar 20, 2024

msambol Mar 20, 2024

msambol Mar 20, 2024

msambol left a comment

kishiel commented Mar 20, 2024

msambol left a comment

GavinZZ left a comment

GavinZZ Mar 28, 2024

mergify bot commented Apr 2, 2024

mergify bot commented Apr 2, 2024

aws-cdk-automation commented Apr 2, 2024

mergify bot commented Apr 2, 2024


		### Giving Service Principals and Account Principals assume role permission from a role

		Service Principals and Account Principals can be granted permission to assume a role using `assumeRolePolicy` which modifies the role Trust Policy.


		An identity can be granted permission to assume a role using `grantAssumeRole`.

		Note that this does not apply to service principals or account principals as they must be added to the role trust policy.

fix(iam): grantAssumeRole silently fails with service and account principals #29452

fix(iam): grantAssumeRole silently fails with service and account principals #29452

Conversation

kishiel commented Mar 11, 2024

Issue #24507

Reason for this change

Description of changes

Description of how you validated changes

aws-cdk-automation left a comment

Choose a reason for hiding this comment

msambol Mar 20, 2024 • edited

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

msambol commented Mar 20, 2024

msambol left a comment

Choose a reason for hiding this comment

kishiel commented Mar 20, 2024

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

msambol left a comment

Choose a reason for hiding this comment

kishiel commented Mar 20, 2024

msambol left a comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Apr 2, 2024

mergify bot commented Apr 2, 2024

aws-cdk-automation commented Apr 2, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 2, 2024

msambol Mar 20, 2024 •

edited