fix(pipelines): pipeline asset role trust policy has account root principal #30084
Conversation
There was a problem hiding this comment.
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.
xazhao
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
xazhao
added
p1
and removed
p2
pr-linter/exempt-integ-test
aws-cdk-automation
added
the
pr/needs-maintainer-review
| Remove the root account principal from the generated PipelineAssetsFileRole trust policy, | ||
| because it's only assumed by the codebuild. | ||
| Use a feature flag to make sure existing customers who might be relying | ||
| on the overly-broad trust policy are not broken. |
There was a problem hiding this comment.
Instead of explaining why we add a feature flag here, can we explain the behaviour of the flag when it's true vs false, i.e. When this feature flag is enabled, the it will not add root account principal ...
packages/aws-cdk-lib/pipelines/test/codepipeline/codepipeline.test.ts
Outdated
Show resolved
Hide resolved
| Action: 'sts:AssumeRole', | ||
| Effect: 'Allow', | ||
| Principal: { | ||
| AWS: { | ||
| 'Fn::Join': ['', [ | ||
| 'arn:', { Ref: 'AWS::Partition' }, `:iam::${PIPELINE_ENV.account}:root`, | ||
| ]], | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Why does this test change when no feature flag is set?
There was a problem hiding this comment.
Since the behavior has been tested with feature flag in the other test, we can use this to test default value.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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
removed
the
pr/needs-maintainer-review
|
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. |
Note
This may be a breaking change for some users. If the
AssetFileRoleis being assumed by other service/Users/Roles, they will not be able to assume the role after this change by default.To revert back to the old behavior, setting the following feature flag to false in the
contextsection ofcdk.jsonThe is enabled by default to address the overly broad trust policy of
AssetFileRoleReason for this change
CDK Pipeline will create a
AssetFileRolewhich has trust policy including the root account principal. The root account principal is not needed in this use case and should be removed to scope down trust policy.Description of changes
Adding a new feature flag
PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPEwith default valuetrue.When the feature flag is enabled, remove the root account principal from the trust policy.
When the feature flag is disabled, keep the old behavior.
Using the feature flag here in case of customers are using the root account principal and it will allow them to turn off this change.
Description of how you validated changes
Unit test/Integration Test
Manually tested in cross-account pipeline
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license