Skip to content

Commit

Permalink
fix(pipelines): too many CodeBuild steps inflate policy size (#20396)
Browse files Browse the repository at this point in the history
(This change has been split off from #20189 because that PR was growing
too big)

Collapse CodeBuild action Roles: each CodeBuild step used to create a
fresh Role to run the CodeBuild action. Change to use one Role for all
CodeBuild actions. This saves a lot of resources and policy space when
using many CodeBuild steps, and doesn't appreciably change the
security posture of the Pipeline (note: this is not about the
Execution Role of the CodeBuild projects, this is about the Role
assumed by the Pipeline to initiate execution of the Project).

Relates to #19276, #19939, #19835.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored May 24, 2022
1 parent de027e2 commit f334060
Show file tree
Hide file tree
Showing 106 changed files with 1,797 additions and 2,672 deletions.
18 changes: 7 additions & 11 deletions packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ export class Pipeline extends PipelineBase {
private readonly crossAccountKeys: boolean;
private readonly enableKeyRotation?: boolean;
private readonly reuseCrossRegionSupportStacks: boolean;
private readonly codePipeline: CfnPipeline;

constructor(scope: Construct, id: string, props: PipelineProps = {}) {
super(scope, id, {
Expand Down Expand Up @@ -426,7 +427,7 @@ export class Pipeline extends PipelineBase {
assumedBy: new iam.ServicePrincipal('codepipeline.amazonaws.com'),
});

const codePipeline = new CfnPipeline(this, 'Resource', {
this.codePipeline = new CfnPipeline(this, 'Resource', {
artifactStore: Lazy.any({ produce: () => this.renderArtifactStoreProperty() }),
artifactStores: Lazy.any({ produce: () => this.renderArtifactStoresProperty() }),
stages: Lazy.any({ produce: () => this.renderStages() }),
Expand All @@ -437,11 +438,11 @@ export class Pipeline extends PipelineBase {
});

// this will produce a DependsOn for both the role and the policy resources.
codePipeline.node.addDependency(this.role);
this.codePipeline.node.addDependency(this.role);

this.artifactBucket.grantReadWrite(this.role);
this.pipelineName = this.getResourceNameAttribute(codePipeline.ref);
this.pipelineVersion = codePipeline.attrVersion;
this.pipelineName = this.getResourceNameAttribute(this.codePipeline.ref);
this.pipelineVersion = this.codePipeline.attrVersion;
this.crossRegionBucketsPassed = !!props.crossRegionReplicationBuckets;

for (const [region, replicationBucket] of Object.entries(props.crossRegionReplicationBuckets || {})) {
Expand Down Expand Up @@ -724,13 +725,8 @@ export class Pipeline extends PipelineBase {
}

// the pipeline role needs assumeRole permissions to the action role
if (actionRole) {
this.role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['sts:AssumeRole'],
resources: [actionRole.roleArn],
}));
}

const grant = actionRole?.grantAssumeRole(this.role);
grant?.applyBefore(this.codePipeline);
return actionRole;
}

Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/lazy-role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ export class LazyRole extends cdk.Resource implements IRole {
return this.instantiate().grantPassRole(identity);
}

/**
* Grant permissions to the given principal to assume this role.
*/
public grantAssumeRole(identity: IPrincipal): Grant {
return this.instantiate().grantAssumeRole(identity);
}

private instantiate(): Role {
if (!this.role) {
const role = new Role(this, 'Default', this.props);
Expand Down
15 changes: 15 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/principals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,21 @@ export interface ServicePrincipalOpts {
* An IAM principal that represents an AWS service (i.e. sqs.amazonaws.com).
*/
export class ServicePrincipal extends PrincipalBase {
/**
* Translate the given service principal name based on the region it's used in.
*
* For example, for Chinese regions this may (depending on whether that's necessary
* for the given service principal) append `.cn` to the name.
*
* The `region-info` module is used to obtain this information.
*
* @example
* const principalName = iam.ServicePrincipal.servicePrincipalName('ec2.amazonaws.com');
*/
public static servicePrincipalName(service: string): string {
return new ServicePrincipalToken(service, {}).toString();
}

/**
*
* @param service AWS service (i.e. sqs.amazonaws.com)
Expand Down
4 changes: 4 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,8 @@ export class ImmutableRole extends Resource implements IRole {
public grantPassRole(grantee: IPrincipal): Grant {
return this.role.grantPassRole(grantee);
}

public grantAssumeRole(identity: IPrincipal): Grant {
return this.role.grantAssumeRole(identity);
}
}
20 changes: 20 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ export class Role extends Resource implements IRole {
return this.grant(identity, 'iam:PassRole');
}

/**
* Grant permissions to the given principal to pass this role.
*/
public grantAssumeRole(identity: IPrincipal): Grant {
return this.grant(identity, 'sts:AssumeRole');
}

/**
* Grant the actions defined in actions to the identity Principal on this resource.
*/
Expand Down Expand Up @@ -447,6 +454,14 @@ export class Role extends Resource implements IRole {
return this.grant(identity, 'iam:PassRole');
}

/**
* Grant permissions to the given principal to assume this role.
*/
public grantAssumeRole(identity: IPrincipal) {
return this.grant(identity, 'sts:AssumeRole');
}


/**
* Return a copy of this Role object whose Policies will not be updated
*
Expand Down Expand Up @@ -502,6 +517,11 @@ export interface IRole extends IIdentity {
* Grant permissions to the given principal to pass this role.
*/
grantPassRole(grantee: IPrincipal): Grant;

/**
* Grant permissions to the given principal to assume this role.
*/
grantAssumeRole(grantee: IPrincipal): Grant;
}

function createAssumeRolePolicy(principal: IPrincipal, externalIds: string[]) {
Expand Down
10 changes: 10 additions & 0 deletions packages/@aws-cdk/aws-iam/test/principals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,16 @@ test('AccountPrincipal can specify an organization', () => {
});
});

test('ServicePrincipalName returns just a string representing the principal', () => {
// GIVEN
const usEastStack = new Stack(undefined, undefined, { env: { region: 'us-east-1' } });
const afSouthStack = new Stack(undefined, undefined, { env: { region: 'af-south-1' } });
const principalName = iam.ServicePrincipal.servicePrincipalName('ssm.amazonaws.com');

expect(usEastStack.resolve(principalName)).toEqual('ssm.amazonaws.com');
expect(afSouthStack.resolve(principalName)).toEqual('ssm.af-south-1.amazonaws.com');
});

test('Passing non-string as accountId parameter in AccountPrincipal constructor should throw error', () => {
expect(() => new iam.AccountPrincipal(1234)).toThrowError('accountId should be of type string');
});
Expand Down
24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-iam/test/role.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,30 @@ describe('IAM role', () => {
});
});

test('a role can grant AssumeRole permissions', () => {
// GIVEN
const stack = new Stack();
const role = new Role(stack, 'Role', { assumedBy: new ServicePrincipal('henk.amazonaws.com') });
const user = new User(stack, 'User');

// WHEN
role.grantAssumeRole(user);

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Resource: { 'Fn::GetAtt': ['Role1ABCC5F0', 'Arn'] },
},
],
Version: '2012-10-17',
},
});
});

testDeprecated('can supply externalId', () => {
// GIVEN
const stack = new Stack();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,15 +323,27 @@ export class CodeBuildFactory implements ICodePipelineActionFactory {
? { _PROJECT_CONFIG_HASH: projectConfigHash }
: {};


// Start all CodeBuild projects from a single (shared) Action Role, so that we don't have to generate an Action Role for each
// individual CodeBuild Project and blow out the pipeline policy size (and potentially # of resources in the stack).
const actionRoleCid = 'CodeBuildActionRole';
const actionRole = this.props.actionRole
?? options.pipeline.node.tryFindChild(actionRoleCid) as iam.IRole
?? new iam.Role(options.pipeline, actionRoleCid, {
assumedBy: new iam.PrincipalWithConditions(new iam.AccountRootPrincipal(), {
Bool: { 'aws:ViaAWSService': iam.ServicePrincipal.servicePrincipalName('codepipeline.amazonaws.com') },
}),
});

stage.addAction(new codepipeline_actions.CodeBuildAction({
actionName: actionName,
input: inputArtifact,
extraInputs: extraInputArtifacts,
outputs: outputArtifacts,
project,
runOrder: options.runOrder,
role: this.props.actionRole,
variablesNamespace: options.variablesNamespace,
role: actionRole,

// Inclusion of the hash here will lead to the pipeline structure for any changes
// made the config of the underlying CodeBuild Project.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* eslint-disable import/no-extraneous-dependencies */
import '@aws-cdk/assert-internal/jest';
import * as cdkp from '../../../lib';
import { ManualApprovalStep, Step } from '../../../lib';
import { Graph, GraphNode, PipelineGraph } from '../../../lib/helpers-internal';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* eslint-disable import/no-extraneous-dependencies */
import '@aws-cdk/assert-internal/jest';
import * as cdkp from '../../../lib';
import { PipelineQueries } from '../../../lib/helpers-internal/pipeline-queries';
import { AppWithOutput, TestApp } from '../../testhelpers/test-app';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ test('role passed it used for project and code build action', () => {
});

// THEN
Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodeBuild::Project', {
const tpl = Template.fromStack(pipelineStack);
tpl.hasResourceProperties('AWS::CodeBuild::Project', {
ServiceRole: {
'Fn::GetAtt': [
'ProjectRole5B707505',
Expand All @@ -181,7 +182,7 @@ test('role passed it used for project and code build action', () => {
},
});

expect(pipelineStack).toHaveResourceLike('AWS::CodePipeline::Pipeline', {
tpl.hasResourceProperties('AWS::CodePipeline::Pipeline', {
Stages: [
// source stage
{},
Expand All @@ -203,6 +204,8 @@ test('role passed it used for project and code build action', () => {
},
],
},
// Self-update
{},
],
});
});
Expand Down
39 changes: 32 additions & 7 deletions packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Template } from '@aws-cdk/assertions';
import * as ccommit from '@aws-cdk/aws-codecommit';
import * as sqs from '@aws-cdk/aws-sqs';
import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';
import * as cdkp from '../../lib';
import { PIPELINE_ENV, TestApp } from '../testhelpers';
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline } from '../testhelpers';

let app: TestApp;

Expand Down Expand Up @@ -52,24 +53,48 @@ describe('CodePipeline support stack reuse', () => {
const supportStackAArtifact = assembly.getStackByName(`PipelineStackA-support-${testStageEnv.region}`);
const supportStackBArtifact = assembly.getStackByName(`PipelineStackB-support-${testStageEnv.region}`);

const supportStackATemplate = supportStackAArtifact.template;
expect(supportStackATemplate).toHaveResourceLike('AWS::S3::Bucket', {
const supportStackATemplate = Template.fromJSON(supportStackAArtifact.template);
supportStackATemplate.hasResourceProperties('AWS::S3::Bucket', {
BucketName: 'pipelinestacka-support-useplicationbucket80db3753a0ebbf052279',
});
expect(supportStackATemplate).toHaveResourceLike('AWS::KMS::Alias', {
supportStackATemplate.hasResourceProperties('AWS::KMS::Alias', {
AliasName: 'alias/pport-ustencryptionalias5cad45754e1ff088476b',
});

const supportStackBTemplate = supportStackBArtifact.template;
expect(supportStackBTemplate).toHaveResourceLike('AWS::S3::Bucket', {
const supportStackBTemplate = Template.fromJSON(supportStackBArtifact.template);
supportStackBTemplate.hasResourceProperties('AWS::S3::Bucket', {
BucketName: 'pipelinestackb-support-useplicationbucket1d556ec7f959b336abf8',
});
expect(supportStackBTemplate).toHaveResourceLike('AWS::KMS::Alias', {
supportStackBTemplate.hasResourceProperties('AWS::KMS::Alias', {
AliasName: 'alias/pport-ustencryptionalias668c7ffd0de17c9867b0',
});
});
});

test('CodeBuild action role has the right AssumeRolePolicyDocument', () => {
const pipelineStack = new cdk.Stack(app, 'PipelineStack', { env: PIPELINE_ENV });
new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk');

const template = Template.fromStack(pipelineStack);
template.hasResourceProperties('AWS::IAM::Role', {
AssumeRolePolicyDocument: {
Statement: [
{
Action: 'sts:AssumeRole',
Principal: {
AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::123pipeline:root']] },
},
Condition: {
Bool: {
'aws:ViaAWSService': 'codepipeline.amazonaws.com',
},
},
},
],
},
});
});

interface ReuseCodePipelineStackProps extends cdk.StackProps {
reuseCrossRegionSupportStacks?: boolean;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import * as fs from 'fs';
import * as path from 'path';
import { Capture, Match, Template } from '@aws-cdk/assertions';
import '@aws-cdk/assert-internal/jest';
import { Stack, Stage, StageProps, Tags } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { behavior, LegacyTestGitHubNpmPipeline, OneStackApp, BucketStack, PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, stringLike } from '../testhelpers';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "17.0.0",
"version": "19.0.0",
"files": {
"9220951fe280727c8695ca9cd06f929712e6c44319ad69591a87f7ce6a931b6b": {
"09ed6a107711fc77b4417fe759eedb1920ea48ea07d68490b9973255f017840d": {
"source": {
"path": "PipelineStack.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "9220951fe280727c8695ca9cd06f929712e6c44319ad69591a87f7ce6a931b6b.json",
"objectKey": "09ed6a107711fc77b4417fe759eedb1920ea48ea07d68490b9973255f017840d.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
Loading

0 comments on commit f334060

Please sign in to comment.