feat(stepfunctions-tasks): add awsSdkCredentials to CallAwsServiceCrossRegion for cross-account scenarios#35472
feat(stepfunctions-tasks): add awsSdkCredentials to CallAwsServiceCrossRegion for cross-account scenarios#35472
Conversation
79472e3 to
a1e4f8f
Compare
…sServiceCrossRegion
…region-cross-account
a1e4f8f to
eac302a
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…ependencies" This reverts commit d9881d2.
…cies to dependencies
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Thank you for your contributions! I'm not sure this is the right solution here. Adding this field makes this a leaky abstraction. The end user shouldn't have to know or care that a lambda is being used under the hood. I think that the solution here needs to be purely under the hood.
Please correct me if my understanding of this issue isn't quite correct but it seems that the state machine shouldn't have access to the credentials at all (except to pass them on) and that we're giving it permissions it should not have. That's concerning aside from the problem that the construct simply isn't working for your use case.
Fixing this may be a breaking change but I do think we need to correct where the credentials are going so we're not giving unintended permissions.
| const account = process.env.CDK_INTEG_ACCOUNT || '123456789012'; | ||
| const crossAccount = process.env.CDK_INTEG_CROSS_ACCOUNT || '234567890123'; |
There was a problem hiding this comment.
Can you please remove the fake account numbers here? I know it's obvious to any human reading this that these are dummy values but having a fallback fake account can sometimes cause issues both with security scanning and with running tests when running the whole suite (and therefore not actually reading the code)
There was a problem hiding this comment.
Thank you for the feedback!
I followed the existing pattern from other cross-account integration tests, such as integ.cross-account-pipeline-cfn-action.ts, which also uses fallback dummy account IDs ('123456789012' and '234567890123').
Are you suggesting that all existing cross-account integration tests should also remove
their fallback dummy account IDs? I found several tests following this same pattern
throughout the codebase.
Additionally, specifying fallback account IDs in the test code helps maintain consistency
of account IDs included in snapshots, avoiding snapshot diffs when different developers run
the tests with different account configurations.
Should I align with the existing pattern, or would you prefer to establish a new standard
across all cross-account integration tests?
|
@TheRealAmazonKendra Thank you for the review and feedback! I implemented the current approach with the Regarding "users shouldn't need to know that Lambda is used under the hood" - I agree with this principle in general. However, in cases where users need to minimize trust policies, they might need to understand the internal architecture to properly configure cross-account role trust relationships. The Lambda execution role would need to be trusted by the cross-account role for the assume role operation to succeed. |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
Pull request has been modified.
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
Issue # (if applicable)
Closes #35317.
Reason for this change
When using
CallAwsServiceCrossRegionwith the credentials property for cross-account scenarios, the StateMachine execution role assumes the specified credentials role and attempts to execute the auto-generatedLambda function with that role. This causes Lambda execution failures because the cross-account role lacks lambda:InvokeFunction permissions for the Lambda function in the original account.
Description of changes
Added a new
awsSdkCredentialsproperty toCallAwsServiceCrossRegionthat allows specifying IAM credentials exclusively for AWS SDK calls within the Lambda function, without affecting Lambda execution itself.Key changes:
awsSdkCredentialsproperty toCallAwsServiceCrossRegionOptionsinterfaceassumeRoleArnparameter and use STS AssumeRole for AWS SDK client initializationsts:AssumeRoleIAM permission whenawsSdkCredentialsis specifiedDesign decisions:
@aws-sdk/client-stsfor assume role functionalityDescribe any new or updated permissions being added
When
awsSdkCredentialsis specified, the Lambda execution role automatically receives:sts:AssumeRolepermission on all resources (*) to assume the specified cross-account roleDescription of how you validated changes
integ.call-aws-service-cross-region-cross-account.ts) that validates cross-account and cross-region DynamoDB API callsChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license