fix(iam): adding addPrincipalsToAssumedBy() to allow adding moreprincipals to TrustRelationship of Role to overcome limitation of grantAssumeRole() (#22550)

[stm29]

Issue # (if applicable)

Closes #22550

Reason for this change

• We need to rely on assumeRolePolicy property of a Role to add more trust relationships to a role, as grantAssumeRole() has a limitation mentioned in aws-iam: Make setting trust on roles more clear in overview and function descriptions #22550 (comment)
• This Following PR - fix(iam): grantAssumeRole silently fails with service and account principals #29452 added,
  • How to use assumeRolePolicy in README.md
  • Silently Fails, when Service / Account Principal is used with grantAssumeRole()
  • We still can't add ArnPrincipal() using grantAssumeRole(), (Doing this does nothing) as mentioned on below issues,
    • [aws-iam]: Should it warn or fail if using grantAssumeRole on imported role? #23090
    • (aws-iam): grantAssumeRole does nothing #24507
    • aws-iam: Make setting trust on roles more clear in overview and function descriptions #22550
• It would be better to have similar function like grantAssumeRole() to add Principals directly.

Description of changes

• Added addPrincipalsToAssumedBy() function
• Removed Optional property on assumeRolePolicy (as I guess, it will never be undefined)
• Changed README.md respectively
• Added Integration Tests

Description of how you validated changes

• Unit Tests
• Integ Tests

• $ yarn integ --directory test/aws-iam/test --update-on-failed
  

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 14f0889
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[shikha372]

i think we can do it currently with the help of role.assumeRolePolicy? .

[stm29]

Correct, but I personally fell rather than relying role.assumeRolePolicy, it is better to have similar function like grant(), grantAssume(). Having function of similar kind addPrincipalsToAssumedBy() to give better helper value to users (though it is documented well).
Consider this following scenario, grant() is sufficient enough to add a PolicyDocument, but we do have helper functions like grantSendMessage() for sqs, grantEncryptDepcrypt() for kms to service for specific purposes right?, so this will also serve similar value is my thought.

Would like you hear from you..

[shikha372]

grant methods are encapsulation for policy statements, i don't think we're encapsulating any policies here(just the assume role), we are using the same object and just modifying it in the function, i'll put this into perspective with the help of dx to make it more clear and to be sure i'm not missing something here:

Current method:

role.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
  actions: ['sts:AssumeRole'],
  principals: [
    new iam.AccountPrincipal('123456789'),
    new iam.ServicePrincipal('beep-boop.amazonaws.com')
    ],
}));

Proposed:

role.addPrincipalsToAssumedBy([
  new iam.AccountPrincipal('123456789012'),
  new iam.ServicePrincipal('beep-boop.amazonaws.com')
]);

[stm29]

Exactly, you got my point. that sts:AssumeRole is the one i am talking about in terms of easing usage of utilities.
For Example see here how it is used in other utility functions in current scenario,

aws-cdk/packages/aws-cdk-lib/aws-kms/lib/key.ts

Lines 201 to 203 in 975df1f

	public grantDecrypt(grantee: iam.IGrantable): iam.Grant {
	return this.grant(grantee, ...perms.DECRYPT_ACTIONS);
	}

aws-cdk/packages/aws-cdk-lib/aws-kms/lib/private/perms.ts

Lines 26 to 28 in 975df1f

	export const DECRYPT_ACTIONS = [
	'kms:Decrypt',
	];

This just adds kms:Decrypt to the provided Grantee.

[shikha372]

Thanks @stm29 for submitting this PR, we're aware of the current limitations of grantAssumeRole method and it is documented in README as well. While we value our community contributions, the functionality that we're trying to add here can easily be done already with role.assumeRole, i don't see how doing the same thing by two different methods could help.

[stm29]

Hi @shikha372 , is there any comments, pending from my end to be addressed?

[shikha372]

Hi @stm29,
Thank you for contributing to this, aligned with team as well that this function is not adding any value to the current usage. Hence, will be closing this PR. If we wish to modify this behaviour, right place to address it could be under assumeRole function where current behaviour is to throw an error.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.