From 8f4c2abea5d0f3b5595292b561c7bf97326c958f Mon Sep 17 00:00:00 2001 From: Romain Marcadier-Muller Date: Mon, 29 Oct 2018 14:36:10 +0100 Subject: [PATCH] feat(codepipeline/cfn): Use fewer statements for pipeline permissions (#1009) When trying to make minimal-permission IAM policies, it can be necessary to ensure the policy remains as compact as possible. In certain cases, the same permissions will be extended to multiple resources separately, and those can be represented using a single statement, instead of one per each resource. This feature uses a role-local singleton construct to ensure only one statement is created for a given permission template, so as to minimize the size of the resulting policy. The feature is being used in order to avoid creating extremely large policy documents when adding CodePipeline actions to deploy a number of CloudFormation stacks using the same ChangeSet name (using a single statement instead of one per stack). --- .../lib/pipeline-actions.ts | 164 +++++++++++++----- .../test/test.pipeline-actions.ts | 109 +++++++++++- ...g.cfn-template-from-repo.lit.expected.json | 29 +--- .../test/integ.lambda-pipeline.expected.json | 2 +- .../test/integ.pipeline-cfn.expected.json | 29 +--- ...g.pipeline-code-commit-build.expected.json | 2 +- .../integ.pipeline-code-commit.expected.json | 2 +- .../integ.pipeline-code-deploy.expected.json | 2 +- .../test/integ.pipeline-events.expected.json | 2 +- packages/@aws-cdk/aws-iam/lib/role.ts | 24 ++- 10 files changed, 256 insertions(+), 109 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts b/packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts index 776d3fa694afa..74c3b453d1256 100644 --- a/packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts +++ b/packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts @@ -92,10 +92,8 @@ export class PipelineExecuteChangeSetAction extends PipelineCloudFormationAction ChangeSetName: props.changeSetName, }); - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addAction('cloudformation:ExecuteChangeSet') - .addResource(stackArnFromName(props.stackName)) - .addCondition('StringEquals', { 'cloudformation:ChangeSetName': props.changeSetName })); + SingletonPolicy.forRole(props.stage.pipelineRole) + .grantExecuteChangeSet(props); } } @@ -212,11 +210,7 @@ export abstract class PipelineCloudFormationDeployAction extends PipelineCloudFo } } - // Allow the pipeline to pass this actions' role to CloudFormation - // Required by all Actions that perform CFN deployments - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addAction('iam:PassRole') - .addResource(this.role.roleArn)); + SingletonPolicy.forRole(props.stage.pipelineRole).grantPassRole(this.role); } /** @@ -261,16 +255,7 @@ export class PipelineCreateReplaceChangeSetAction extends PipelineCloudFormation this.addInputArtifact(props.templateConfiguration.artifact); } - const stackArn = stackArnFromName(props.stackName); - // Allow the pipeline to check for Stack & ChangeSet existence - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addAction('cloudformation:DescribeStacks') - .addResource(stackArn)); - // Allow the pipeline to create & delete the specified ChangeSet - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addActions('cloudformation:CreateChangeSet', 'cloudformation:DeleteChangeSet', 'cloudformation:DescribeChangeSet') - .addResource(stackArn) - .addCondition('StringEquals', { 'cloudformation:ChangeSetName': props.changeSetName })); + SingletonPolicy.forRole(props.stage.pipelineRole).grantCreateReplaceChangeSet(props); } } @@ -325,22 +310,7 @@ export class PipelineCreateUpdateStackAction extends PipelineCloudFormationDeplo this.addInputArtifact(props.templateConfiguration.artifact); } - // permissions are based on best-guess from - // https://docs.aws.amazon.com/codepipeline/latest/userguide/how-to-custom-role.html - // and https://docs.aws.amazon.com/IAM/latest/UserGuide/list_awscloudformation.html - const stackArn = stackArnFromName(props.stackName); - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addActions( - 'cloudformation:DescribeStack*', - 'cloudformation:CreateStack', - 'cloudformation:UpdateStack', - 'cloudformation:DeleteStack', // needed when props.replaceOnFailure is true - 'cloudformation:GetTemplate*', - 'cloudformation:ValidateTemplate', - 'cloudformation:GetStackPolicy', - 'cloudformation:SetStackPolicy', - ) - .addResource(stackArn)); + SingletonPolicy.forRole(props.stage.pipelineRole).grantCreateUpdateStack(props); } } @@ -362,13 +332,7 @@ export class PipelineDeleteStackAction extends PipelineCloudFormationDeployActio super(parent, id, props, { ActionMode: 'DELETE_ONLY', }); - const stackArn = stackArnFromName(props.stackName); - props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement() - .addActions( - 'cloudformation:DescribeStack*', - 'cloudformation:DeleteStack', - ) - .addResource(stackArn)); + SingletonPolicy.forRole(props.stage.pipelineRole).grantDeleteStack(props); } } @@ -401,3 +365,119 @@ function stackArnFromName(stackName: string): string { resourceName: `${stackName}/*` }); } + +/** + * Manages a bunch of singleton-y statements on the policy of an IAM Role. + * Dedicated methods can be used to add specific permissions to the role policy + * using as few statements as possible (adding resources to existing compatible + * statements instead of adding new statements whenever possible). + * + * Statements created outside of this class are not considered when adding new + * permissions. + */ +class SingletonPolicy extends cdk.Construct { + /** + * Obtain a SingletonPolicy for a given role. + * @param role the Role this policy is bound to. + * @returns the SingletonPolicy for this role. + */ + public static forRole(role: iam.Role): SingletonPolicy { + const found = role.tryFindChild(SingletonPolicy.UUID); + return (found as SingletonPolicy) || new SingletonPolicy(role); + } + + private static readonly UUID = '8389e75f-0810-4838-bf64-d6f85a95cf83'; + + private statements: { [key: string]: iam.PolicyStatement } = {}; + + private constructor(private readonly role: iam.Role) { + super(role, SingletonPolicy.UUID); + } + + public grantCreateUpdateStack(props: { stackName: string, replaceOnFailure?: boolean }): void { + const actions = [ + 'cloudformation:DescribeStack*', + 'cloudformation:CreateStack', + 'cloudformation:UpdateStack', + 'cloudformation:GetTemplate*', + 'cloudformation:ValidateTemplate', + 'cloudformation:GetStackPolicy', + 'cloudformation:SetStackPolicy', + ]; + if (props.replaceOnFailure) { + actions.push('cloudformation:DeleteStack'); + } + this.statementFor({ actions }).addResource(stackArnFromName(props.stackName)); + } + + public grantCreateReplaceChangeSet(props: { stackName: string, changeSetName: string }): void { + this.statementFor({ + actions: [ + 'cloudformation:CreateChangeSet', + 'cloudformation:DeleteChangeSet', + 'cloudformation:DescribeChangeSet', + 'cloudformation:DescribeStacks', + ], + conditions: { StringEqualsIfExists: { 'cloudformation:ChangeSetName': props.changeSetName } }, + }).addResource(stackArnFromName(props.stackName)); + } + + public grantExecuteChangeSet(props: { stackName: string, changeSetName: string }): void { + this.statementFor({ + actions: ['cloudformation:ExecuteChangeSet'], + conditions: { StringEquals: { 'cloudformation:ChangeSetName': props.changeSetName } }, + }).addResource(stackArnFromName(props.stackName)); + } + + public grantDeleteStack(props: { stackName: string }): void { + this.statementFor({ + actions: [ + 'cloudformation:DescribeStack*', + 'cloudformation:DeleteStack', + ] + }).addResource(stackArnFromName(props.stackName)); + } + + public grantPassRole(role: iam.Role): void { + this.statementFor({ actions: ['iam:PassRole'] }).addResource(role.roleArn); + } + + private statementFor(template: StatementTemplate): iam.PolicyStatement { + const key = keyFor(template); + if (!(key in this.statements)) { + this.statements[key] = new iam.PolicyStatement().addActions(...template.actions); + if (template.conditions) { + this.statements[key].addConditions(template.conditions); + } + this.role.addToPolicy(this.statements[key]); + } + return this.statements[key]; + + function keyFor(props: StatementTemplate): string { + const actions = `${props.actions.sort().join('\x1F')}`; + const conditions = formatConditions(props.conditions); + return `${actions}\x1D${conditions}`; + + function formatConditions(cond?: StatementCondition): string { + if (cond == null) { return ''; } + let result = ''; + for (const op of Object.keys(cond).sort()) { + result += `${op}\x1E`; + const condition = cond[op]; + for (const attribute of Object.keys(condition).sort()) { + const value = condition[attribute]; + result += `${value}\x1F`; + } + } + return result; + } + } + } +} + +interface StatementTemplate { + actions: string[]; + conditions?: StatementCondition; +} + +type StatementCondition = { [op: string]: { [attribute: string]: string } }; diff --git a/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts b/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts index 071e80fc76253..11db184b50963 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts @@ -7,7 +7,7 @@ import cloudformation = require('../lib'); export = nodeunit.testCase({ 'CreateReplaceChangeSet': { - works(test: nodeunit.Test) { + 'works'(test: nodeunit.Test) { const stack = new cdk.Stack(); const pipelineRole = new RoleDouble(stack, 'PipelineRole'); const stage = new StageDouble({ pipelineRole }); @@ -22,8 +22,8 @@ export = nodeunit.testCase({ _assertPermissionGranted(test, pipelineRole.statements, 'iam:PassRole', action.role.roleArn); const stackArn = _stackArn('MyStack'); - const changeSetCondition = { StringEquals: { 'cloudformation:ChangeSetName': 'MyChangeSet' } }; - _assertPermissionGranted(test, pipelineRole.statements, 'cloudformation:DescribeStacks', stackArn); + const changeSetCondition = { StringEqualsIfExists: { 'cloudformation:ChangeSetName': 'MyChangeSet' } }; + _assertPermissionGranted(test, pipelineRole.statements, 'cloudformation:DescribeStacks', stackArn, changeSetCondition); _assertPermissionGranted(test, pipelineRole.statements, 'cloudformation:DescribeChangeSet', stackArn, changeSetCondition); _assertPermissionGranted(test, pipelineRole.statements, 'cloudformation:CreateChangeSet', stackArn, changeSetCondition); _assertPermissionGranted(test, pipelineRole.statements, 'cloudformation:DeleteChangeSet', stackArn, changeSetCondition); @@ -37,11 +37,64 @@ export = nodeunit.testCase({ ChangeSetName: 'MyChangeSet' }); + test.done(); + }, + + 'uses a single permission statement if the same ChangeSet name is used'(test: nodeunit.Test) { + const stack = new cdk.Stack(); + const pipelineRole = new RoleDouble(stack, 'PipelineRole'); + const stage = new StageDouble({ pipelineRole }); + const artifact = new cpapi.Artifact(stack as any, 'TestArtifact'); + new cloudformation.PipelineCreateReplaceChangeSetAction(stack, 'ActionA', { + stage, + changeSetName: 'MyChangeSet', + stackName: 'StackA', + templatePath: artifact.atPath('path/to/file') + }); + + new cloudformation.PipelineCreateReplaceChangeSetAction(stack, 'ActionB', { + stage, + changeSetName: 'MyChangeSet', + stackName: 'StackB', + templatePath: artifact.atPath('path/to/other/file') + }); + + test.deepEqual( + cdk.resolve(pipelineRole.statements), + [ + { + Action: 'iam:PassRole', + Effect: 'Allow', + Resource: [ + { 'Fn::GetAtt': [ 'ActionARole72759154', 'Arn' ] }, + { 'Fn::GetAtt': [ 'ActionBRole6A2F6804', 'Arn' ] } + ], + }, + { + Action: [ + 'cloudformation:CreateChangeSet', + 'cloudformation:DeleteChangeSet', + 'cloudformation:DescribeChangeSet', + 'cloudformation:DescribeStacks' + ], + Condition: { StringEqualsIfExists: { 'cloudformation:ChangeSetName': 'MyChangeSet' } }, + Effect: 'Allow', + Resource: [ + // tslint:disable-next-line:max-line-length + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':cloudformation:', { Ref: 'AWS::Region' }, ':', { Ref: 'AWS::AccountId' }, ':stack/StackA/*' ] ] }, + // tslint:disable-next-line:max-line-length + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':cloudformation:', { Ref: 'AWS::Region' }, ':', { Ref: 'AWS::AccountId' }, ':stack/StackB/*' ] ] } + ], + } + ] + ); + test.done(); } }, + 'ExecuteChangeSet': { - works(test: nodeunit.Test) { + 'works'(test: nodeunit.Test) { const stack = new cdk.Stack(); const pipelineRole = new RoleDouble(stack, 'PipelineRole'); const stage = new StageDouble({ pipelineRole }); @@ -61,6 +114,42 @@ export = nodeunit.testCase({ ChangeSetName: 'MyChangeSet' }); + test.done(); + }, + + 'uses a single permission statement if the same ChangeSet name is used'(test: nodeunit.Test) { + const stack = new cdk.Stack(); + const pipelineRole = new RoleDouble(stack, 'PipelineRole'); + const stage = new StageDouble({ pipelineRole }); + new cloudformation.PipelineExecuteChangeSetAction(stack, 'ActionA', { + stage, + changeSetName: 'MyChangeSet', + stackName: 'StackA', + }); + + new cloudformation.PipelineExecuteChangeSetAction(stack, 'ActionB', { + stage, + changeSetName: 'MyChangeSet', + stackName: 'StackB', + }); + + test.deepEqual( + cdk.resolve(pipelineRole.statements), + [ + { + Action: 'cloudformation:ExecuteChangeSet', + Condition: { StringEquals: { 'cloudformation:ChangeSetName': 'MyChangeSet' } }, + Effect: 'Allow', + Resource: [ + // tslint:disable-next-line:max-line-length + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':cloudformation:', { Ref: 'AWS::Region' }, ':', { Ref: 'AWS::AccountId' }, ':stack/StackA/*' ] ] }, + // tslint:disable-next-line:max-line-length + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':cloudformation:', { Ref: 'AWS::Region' }, ':', { Ref: 'AWS::AccountId' }, ':stack/StackB/*' ] ] } + ], + } + ] + ); + test.done(); } }, @@ -72,6 +161,7 @@ export = nodeunit.testCase({ stage: new StageDouble({ pipelineRole }), templatePath: new cpapi.Artifact(stack as any, 'TestArtifact').atPath('some/file'), stackName: 'MyStack', + replaceOnFailure: true, }); const stackArn = _stackArn('MyStack'); @@ -144,12 +234,13 @@ function _hasAction(actions: cpapi.Action[], owner: string, provider: string, ca return false; } -function _assertPermissionGranted(test: nodeunit.Test, statements: PolicyStatementJson[], action: string, resource: string, conditions?: any) { +function _assertPermissionGranted(test: nodeunit.Test, statements: iam.PolicyStatement[], action: string, resource: string, conditions?: any) { const conditionStr = conditions ? ` with condition(s) ${JSON.stringify(cdk.resolve(conditions))}` : ''; - const statementsStr = JSON.stringify(cdk.resolve(statements), null, 2); - test.ok(_grantsPermission(statements, action, resource, conditions), + const resolvedStatements = cdk.resolve(statements); + const statementsStr = JSON.stringify(resolvedStatements, null, 2); + test.ok(_grantsPermission(resolvedStatements, action, resource, conditions), `Expected to find a statement granting ${action} on ${JSON.stringify(cdk.resolve(resource))}${conditionStr}, found:\n${statementsStr}`); } @@ -218,7 +309,7 @@ class StageDouble implements cpapi.IStage, cpapi.IInternalStage { } class RoleDouble extends iam.Role { - public readonly statements = new Array(); + public readonly statements = new Array(); constructor(parent: cdk.Construct, id: string, props: iam.RoleProps = { assumedBy: new iam.ServicePrincipal('test') }) { super(parent, id, props); @@ -226,6 +317,6 @@ class RoleDouble extends iam.Role { public addToPolicy(statement: iam.PolicyStatement) { super.addToPolicy(statement); - this.statements.push(statement.toJson()); + this.statements.push(statement); } } diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json index 2b78411b8811d..552b0ee5903aa 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json @@ -85,38 +85,15 @@ ] } }, - { - "Action": "cloudformation:DescribeStacks", - "Effect": "Allow", - "Resource": { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition" - }, - ":cloudformation:", - { - "Ref": "AWS::Region" - }, - ":", - { - "Ref": "AWS::AccountId" - }, - ":stack/OurStack/*" - ] - ] - } - }, { "Action": [ "cloudformation:CreateChangeSet", "cloudformation:DeleteChangeSet", - "cloudformation:DescribeChangeSet" + "cloudformation:DescribeChangeSet", + "cloudformation:DescribeStacks" ], "Condition": { - "StringEquals": { + "StringEqualsIfExists": { "cloudformation:ChangeSetName": "StagedChangeSet" } }, diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json index c4eba9bf3313a..c8b43657dfa5c 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json @@ -272,4 +272,4 @@ ] } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json index ac56d1f1bbb6a..2481e51936f1f 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json @@ -99,38 +99,15 @@ ] } }, - { - "Action": "cloudformation:DescribeStacks", - "Effect": "Allow", - "Resource": { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition" - }, - ":cloudformation:", - { - "Ref": "AWS::Region" - }, - ":", - { - "Ref": "AWS::AccountId" - }, - ":stack/IntegTest-TestActionStack/*" - ] - ] - } - }, { "Action": [ "cloudformation:CreateChangeSet", "cloudformation:DeleteChangeSet", - "cloudformation:DescribeChangeSet" + "cloudformation:DescribeChangeSet", + "cloudformation:DescribeStacks" ], "Condition": { - "StringEquals": { + "StringEqualsIfExists": { "cloudformation:ChangeSetName": "ChangeSetIntegTest" } }, diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json index 5ff0eed026a50..c0083e6923c60 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json @@ -403,4 +403,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json index f00592f0ee917..52f99453d0776 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json @@ -165,4 +165,4 @@ ] } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-deploy.expected.json index 4dc1147ef370d..85cde39cd5645 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-deploy.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-deploy.expected.json @@ -319,4 +319,4 @@ ] } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json index 4667b873e5679..e9e1f3be44016 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json @@ -531,4 +531,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 22b4d14bd22b3..65da3d1fe9cae 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -21,6 +21,15 @@ export interface RoleProps { */ managedPolicyArns?: string[]; + /** + * A list of named policies to inline into this role. These policies will be + * created with the role, whereas those added by ``addToPolicy`` are added + * using a separate CloudFormation resource (allowing a way around circular + * dependencies that could otherwise be introduced). + * @default No policy is inlined in the Role resource. + */ + inlinePolicies?: { [name: string]: PolicyDocument }; + /** * The path associated with this role. For information about IAM paths, see * Friendly Names and Paths in IAM User Guide. @@ -119,15 +128,28 @@ export class Role extends Construct implements IRole { const role = new cloudformation.RoleResource(this, 'Resource', { assumeRolePolicyDocument: this.assumeRolePolicy as any, managedPolicyArns: undefinedIfEmpty(() => this.managedPolicyArns), + policies: _flatten(props.inlinePolicies), path: props.path, roleName: props.roleName, - maxSessionDuration: props.maxSessionDurationSec + maxSessionDuration: props.maxSessionDurationSec, }); this.roleArn = role.roleArn; this.principal = new ArnPrincipal(this.roleArn); this.roleName = role.roleName; this.dependencyElements = [ role ]; + + function _flatten(policies?: { [name: string]: PolicyDocument }) { + if (policies == null || Object.keys(policies).length === 0) { + return undefined; + } + const result = new Array(); + for (const policyName of Object.keys(policies)) { + const policyDocument = policies[policyName]; + result.push({ policyName, policyDocument }); + } + return result; + } } /**