Skip to content

Commit

Permalink
fix(pipelines): pipeline asset role trust policy has account root pri…
Browse files Browse the repository at this point in the history
…ncipal (#30084)

### Reason for this change

CDK Pipeline will create a `AssetFileRole` which 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_SCOPE` with default value `true`.
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
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
xazhao committed May 8, 2024
1 parent 2c53cf9 commit 3928eae
Show file tree
Hide file tree
Showing 13 changed files with 114 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const codebuild = require("aws-cdk-lib/aws-codebuild");
const ec2 = require("aws-cdk-lib/aws-ec2");
const s3 = require("aws-cdk-lib/aws-s3");
const s3_assets = require("aws-cdk-lib/aws-s3-assets");
const cx_api_1 = require("aws-cdk-lib/cx-api");
const aws_cdk_lib_1 = require("aws-cdk-lib");
const integ = require("@aws-cdk/integ-tests-alpha");
const pipelines = require("aws-cdk-lib/pipelines");
Expand Down Expand Up @@ -54,6 +55,7 @@ const app = new aws_cdk_lib_1.App({
postCliContext: {
'@aws-cdk/core:newStyleStackSynthesis': '1',
'@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2': false,
[cx_api_1.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: true,
},
});
const stack = new TestStack(app, 'PipelinesFileSystemLocations');
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -1504,22 +1504,6 @@
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::",
{
"Ref": "AWS::AccountId"
},
":root"
]
]
},
"Service": "codebuild.amazonaws.com"
}
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as codebuild from 'aws-cdk-lib/aws-codebuild';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as s3_assets from 'aws-cdk-lib/aws-s3-assets';
import { PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE } from 'aws-cdk-lib/cx-api';
import { App, Stack, StackProps, Stage, StageProps, Aws, RemovalPolicy, DefaultStackSynthesizer } from 'aws-cdk-lib';
import * as integ from '@aws-cdk/integ-tests-alpha';
import { Construct } from 'constructs';
Expand Down Expand Up @@ -61,6 +62,7 @@ const app = new App({
postCliContext: {
'@aws-cdk/core:newStyleStackSynthesis': '1',
'@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2': false,
[PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: true,
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as sqs from 'aws-cdk-lib/aws-sqs';
import { App, Stack, StackProps, Stage, StageProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as pipelines from 'aws-cdk-lib/pipelines';
import { PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE } from 'aws-cdk-lib/cx-api';

class PipelineStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
Expand Down Expand Up @@ -50,6 +51,7 @@ const app = new App({
postCliContext: {
'@aws-cdk/core:newStyleStackSynthesis': '1',
'@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2': false,
[PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: false,
},
});
new PipelineStack(app, 'PipelineStack');
Expand Down
21 changes: 20 additions & 1 deletion packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Flags come in three types:
| [@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope](#aws-cdkaws-kmsreducecrossaccountregionpolicyscope) | When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only. | 2.134.0 | (fix) |
| [@aws-cdk/aws-eks:nodegroupNameAttribute](#aws-cdkaws-eksnodegroupnameattribute) | When enabled, nodegroupName attribute of the provisioned EKS NodeGroup will not have the cluster name prefix. | 2.139.0 | (fix) |
| [@aws-cdk/aws-ec2:ebsDefaultGp3Volume](#aws-cdkaws-ec2ebsdefaultgp3volume) | When enabled, the default volume type of the EBS volume will be GP3 | 2.140.0 | (default) |
| [@aws-cdk/pipelines:reduceAssetRoleTrustScope](#aws-cdkpipelinesreduceassetroletrustscope) | Remove the root account principal from PipelineAssetsFileRole trust policy | V2NEXT | (default) |

<!-- END table -->

Expand Down Expand Up @@ -171,6 +172,7 @@ are migrating a v1 CDK project to v2, explicitly set any of these flags which do
| [@aws-cdk/aws-apigateway:usagePlanKeyOrderInsensitiveId](#aws-cdkaws-apigatewayusageplankeyorderinsensitiveid) | Allow adding/removing multiple UsagePlanKeys independently | (fix) | 1.98.0 | `false` | `true` |
| [@aws-cdk/aws-lambda:recognizeVersionProps](#aws-cdkaws-lambdarecognizeversionprops) | Enable this feature flag to opt in to the updated logical id calculation for Lambda Version created using the `fn.currentVersion`. | (fix) | 1.106.0 | `false` | `true` |
| [@aws-cdk/aws-cloudfront:defaultSecurityPolicyTLSv1.2\_2021](#aws-cdkaws-cloudfrontdefaultsecuritypolicytlsv12_2021) | Enable this feature flag to have cloudfront distributions use the security policy TLSv1.2_2021 by default. | (fix) | 1.117.0 | `false` | `true` |
| [@aws-cdk/pipelines:reduceAssetRoleTrustScope](#aws-cdkpipelinesreduceassetroletrustscope) | Remove the root account principal from PipelineAssetsFileRole trust policy | (default) | | `false` | `true` |

<!-- END diff -->

Expand All @@ -185,7 +187,8 @@ Here is an example of a `cdk.json` file that restores v1 behavior for these flag
"@aws-cdk/aws-rds:lowercaseDbIdentifier": false,
"@aws-cdk/aws-apigateway:usagePlanKeyOrderInsensitiveId": false,
"@aws-cdk/aws-lambda:recognizeVersionProps": false,
"@aws-cdk/aws-cloudfront:defaultSecurityPolicyTLSv1.2_2021": false
"@aws-cdk/aws-cloudfront:defaultSecurityPolicyTLSv1.2_2021": false,
"@aws-cdk/pipelines:reduceAssetRoleTrustScope": false
}
}
```
Expand Down Expand Up @@ -1298,4 +1301,20 @@ When this featuer flag is enabled, the default volume type of the EBS volume wil
**Compatibility with old behavior:** Pass `volumeType: EbsDeviceVolumeType.GENERAL_PURPOSE_SSD` to `Volume` construct to restore the previous behavior.


### @aws-cdk/pipelines:reduceAssetRoleTrustScope

*Remove the root account principal from PipelineAssetsFileRole trust policy* (default)

When this feature flag is enabled, the root account principal will not be added to the trust policy of asset role.
When this feature flag is disabled, it will keep the root account principal in the trust policy.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `true` | `true` |

**Compatibility with old behavior:** Disable the feature flag to add the root account principal back


<!-- END details -->
15 changes: 15 additions & 0 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export const LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION = '@aws-cdk/aws-clou
export const CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE = '@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse';
export const CODEPIPELINE_DEFAULT_PIPELINE_TYPE_TO_V2 = '@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2';
export const KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE = '@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope';
export const PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE = '@aws-cdk/pipelines:reduceAssetRoleTrustScope';
export const EKS_NODEGROUP_NAME = '@aws-cdk/aws-eks:nodegroupNameAttribute';
export const EBS_DEFAULT_GP3 = '@aws-cdk/aws-ec2:ebsDefaultGp3Volume';

Expand Down Expand Up @@ -1037,6 +1038,20 @@ export const FLAGS: Record<string, FlagInfo> = {
recommendedValue: true,
},

//////////////////////////////////////////////////////////////////////
[PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: {
type: FlagType.ApiDefault,
summary: 'Remove the root account principal from PipelineAssetsFileRole trust policy',
detailsMd: `
When this feature flag is enabled, the root account principal will not be added to the trust policy of asset role.
When this feature flag is disabled, it will keep the root account principal in the trust policy.
`,
introducedIn: { v2: 'V2NEXT' },
defaults: { v2: true },
recommendedValue: true,
compatibilityWithOldBehaviorMd: 'Disable the feature flag to add the root account principal back',
},

//////////////////////////////////////////////////////////////////////
[EKS_NODEGROUP_NAME]: {
type: FlagType.BugFix,
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/cx-api/test/features.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ test('feature flag defaults may not be changed anymore', () => {
[feats.LAMBDA_RECOGNIZE_VERSION_PROPS]: true,
[feats.CLOUDFRONT_DEFAULT_SECURITY_POLICY_TLS_V1_2_2021]: true,
// Add new disabling feature flags below this line
// ...
[feats.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: true,

});
});
Expand Down
19 changes: 13 additions & 6 deletions packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import * as cpa from '../../../aws-codepipeline-actions';
import * as ec2 from '../../../aws-ec2';
import * as iam from '../../../aws-iam';
import * as s3 from '../../../aws-s3';
import { Aws, CfnCapabilities, Duration, PhysicalName, Stack, Names } from '../../../core';
import { Aws, CfnCapabilities, Duration, PhysicalName, Stack, Names, FeatureFlags } from '../../../core';
import * as cxapi from '../../../cx-api';
import { AssetType, FileSet, IFileSetProducer, ManualApprovalStep, ShellStep, StackAsset, StackDeployment, Step } from '../blueprint';
import { DockerCredential, dockerCredentialsInstallCommands, DockerCredentialUsage } from '../docker-credentials';
Expand Down Expand Up @@ -984,13 +984,20 @@ export class CodePipeline extends PipelineBase {

const stack = Stack.of(this);

const rolePrefix = assetType === AssetType.DOCKER_IMAGE ? 'Docker' : 'File';
const assetRole = new AssetSingletonRole(this.assetsScope, `${rolePrefix}Role`, {
roleName: PhysicalName.GENERATE_IF_NEEDED,
assumedBy: new iam.CompositePrincipal(
const removeRootPrincipal = FeatureFlags.of(this).isEnabled(cxapi.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE);

const assumePrincipal = removeRootPrincipal ? new iam.CompositePrincipal(
new iam.ServicePrincipal('codebuild.amazonaws.com'),
) :
new iam.CompositePrincipal(
new iam.ServicePrincipal('codebuild.amazonaws.com'),
new iam.AccountPrincipal(stack.account),
),
);

const rolePrefix = assetType === AssetType.DOCKER_IMAGE ? 'Docker' : 'File';
let assetRole = new AssetSingletonRole(this.assetsScope, `${rolePrefix}Role`, {
roleName: PhysicalName.GENERATE_IF_NEEDED,
assumedBy: assumePrincipal,
});

// Grant pull access for any ECR registries and secrets that exist
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as s3 from '../../../aws-s3';
import * as sqs from '../../../aws-sqs';
import * as cdk from '../../../core';
import { Stack } from '../../../core';
import * as cxapi from '../../../cx-api';
import * as cdkp from '../../lib';
import { CodePipeline } from '../../lib';
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, TwoStackApp, StageWithStackOutput } from '../testhelpers';
Expand Down Expand Up @@ -197,6 +198,61 @@ test('CodeBuild action role has the right AssumeRolePolicyDocument', () => {
});
});

test('CodeBuild asset role has the right Principal with the feature enabled', () => {
const stack = new cdk.Stack();
stack.node.setContext(cxapi.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE, true);
const pipelineStack = new cdk.Stack(stack, 'PipelineStack', { env: PIPELINE_ENV });
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk');
pipeline.addStage(new FileAssetApp(pipelineStack, 'App', {}));;
const template = Template.fromStack(pipelineStack);
const assetRole = template.toJSON().Resources.CdkAssetsFileRole6BE17A07;
const statementLength = assetRole.Properties.AssumeRolePolicyDocument.Statement;
expect(statementLength).toStrictEqual(
[
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: 'codebuild.amazonaws.com',
},
},
],
);
});

test('CodeBuild asset role has the right Principal with the feature disabled', () => {
const stack = new cdk.Stack();
stack.node.setContext(cxapi.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE, false);
const pipelineStack = new cdk.Stack(stack, 'PipelineStack', { env: PIPELINE_ENV });
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk');
pipeline.addStage(new FileAssetApp(pipelineStack, 'App', {}));;
const template = Template.fromStack(pipelineStack);
const assetRole = template.toJSON().Resources.CdkAssetsFileRole6BE17A07;
const statementLength = assetRole.Properties.AssumeRolePolicyDocument.Statement;
expect(statementLength).toStrictEqual(
[
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: 'codebuild.amazonaws.com',
},
},
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
AWS: {
'Fn::Join': ['', [
'arn:', { Ref: 'AWS::Partition' }, `:iam::${PIPELINE_ENV.account}:root`,
]],
},
},
},
],
);
});

test('CodePipeline throws when key rotation is enabled without enabling cross account keys', ()=>{
const pipelineStack = new cdk.Stack(app, 'PipelineStack', { env: PIPELINE_ENV });
const repo = new ccommit.Repository(pipelineStack, 'Repo', {
Expand Down
22 changes: 0 additions & 22 deletions packages/aws-cdk-lib/pipelines/test/compliance/assets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -396,17 +396,6 @@ describe('basic pipeline', () => {
Service: 'codebuild.amazonaws.com',
},
},
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
AWS: {
'Fn::Join': ['', [
'arn:', { Ref: 'AWS::Partition' }, `:iam::${PIPELINE_ENV.account}:root`,
]],
},
},
},
],
},
});
Expand Down Expand Up @@ -510,17 +499,6 @@ describe('basic pipeline', () => {
Service: 'codebuild.amazonaws.com',
},
},
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
AWS: {
'Fn::Join': ['', [
'arn:', { Ref: 'AWS::Partition' }, `:iam::${PIPELINE_ENV.account}:root`,
]],
},
},
},
],
},
});
Expand Down

0 comments on commit 3928eae

Please sign in to comment.