Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Manage IAM permissions for (some) CFN CodePipeline actions #843

Merged
merged 5 commits into from
Oct 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ export class PipelineExecuteChangeSetAction extends PipelineCloudFormationAction
ActionMode: 'CHANGE_SET_EXECUTE',
ChangeSetName: props.changeSetName,
});

const stackArn = cdk.ArnUtils.fromComponents({ service: 'cloudformation', resource: 'stack', resourceName: `${props.stackName}/*` });
props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement()
.addAction('cloudformation:ExecuteChangeSet')
.addResource(stackArn)
.addCondition('StringEquals', { 'cloudformation:ChangeSetName': props.changeSetName }));
}
}

Expand Down Expand Up @@ -243,6 +249,24 @@ export class PipelineCreateReplaceChangeSetAction extends PipelineCloudFormation
});

this.addInputArtifact(props.templatePath.artifact);
if (props.templateConfiguration && props.templateConfiguration.artifact.name !== props.templatePath.artifact.name) {
this.addInputArtifact(props.templateConfiguration.artifact);
}

const stackArn = cdk.ArnUtils.fromComponents({ service: 'cloudformation', resource: 'stack', resourceName: `${props.stackName}/*` });
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
// Allow the pipeline to check for Stack & ChangeSet existence
props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement()
.addActions('cloudformation:DescribeChangeSet', 'cloudformation:DescribeStacks')
.addResource(stackArn));
// Allow the pipeline to create & delete the specified ChangeSet
props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement()
.addActions('cloudformation:CreateChangeSet', 'cloudformation:DeleteChangeSet')
.addResource(stackArn)
.addCondition('StringEquals', { 'cloudformation:ChangeSetName': props.changeSetName }));
// Allow the pipeline to pass this actions' role to CloudFormation
props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement()
.addAction('iam:PassRole')
.addResource(this.role.roleArn));
}
}

Expand Down
144 changes: 144 additions & 0 deletions packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import cpapi = require('@aws-cdk/aws-codepipeline-api');
import iam = require('@aws-cdk/aws-iam');
import cdk = require('@aws-cdk/cdk');
import nodeunit = require('nodeunit');
import util = require('util');
import cloudformation = require('../lib');

export = nodeunit.testCase({
CreateReplaceChangeSet: {
works(test: nodeunit.Test) {
const stack = new cdk.Stack();
const statements = new Array<PolicyStatementJson>();
const pipelineRole = {
addToPolicy: statement => { statements.push(statement.toJson()); }
} as iam.Role;
const actions = new Array<cpapi.Action>();
const stage = {
_attachAction: act => { actions.push(act); },
pipelineRole
} as cpapi.IStage;
const artifact = new cpapi.Artifact(stack as any, 'TestArtifact');
const action = new cloudformation.PipelineCreateReplaceChangeSetAction(stack, 'Action', {
stage,
changeSetName: 'MyChangeSet',
stackName: 'MyStack',
templatePath: artifact.atPath('path/to/file')
});

test.ok(_grantsPermission(statements, 'iam:PassRole', action.role.roleArn),
'The pipelineRole was given permissions to iam:PassRole to the action');

const stackArn = cdk.ArnUtils.fromComponents({
service: 'cloudformation',
resource: 'stack',
resourceName: 'MyStack/*'
});
test.ok(_grantsPermission(statements, 'cloudformation:DescribeStacks', stackArn)
&& _grantsPermission(statements, 'cloudformation:DescribeChangeSet', stackArn),
'The pipelineRole was given permissions to describe the stack & it\'s ChangeSets');
test.ok(_grantsPermission(statements, 'cloudformation:CreateChangeSet', stackArn, {
StringEquals: { 'cloudformation:ChangeSetName': 'MyChangeSet' }
}),
'The pipelineRole was given permissions to create the desired ChangeSet');
test.ok(_grantsPermission(statements, 'cloudformation:DeleteChangeSet', stackArn, {
StringEquals: { 'cloudformation:ChangeSetName': 'MyChangeSet' }
}),
'The pipelineRole was given permissions to delete the desired ChangeSet');

test.deepEqual(action.inputArtifacts, [artifact],
'The inputArtifact was correctly registered');

test.ok(_hasAction(actions, 'AWS', 'CloudFormation', 'Deploy', {
ActionMode: 'CHANGE_SET_CREATE_REPLACE',
StackName: 'MyStack',
ChangeSetName: 'MyChangeSet'
}));

test.done();
}
},
ExecuteChangeSet: {
works(test: nodeunit.Test) {
const stack = new cdk.Stack();
const statements = new Array<PolicyStatementJson>();
const pipelineRole = {
addToPolicy: statement => { statements.push(statement.toJson()); }
} as iam.Role;
const actions = new Array<cpapi.Action>();
const stage = {
_attachAction: act => { actions.push(act); },
pipelineRole
} as cpapi.IStage;
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
new cloudformation.PipelineExecuteChangeSetAction(stack, 'Action', {
stage,
changeSetName: 'MyChangeSet',
stackName: 'MyStack',
});

const stackArn = cdk.ArnUtils.fromComponents({
service: 'cloudformation',
resource: 'stack',
resourceName: 'MyStack/*'
});
test.ok(_grantsPermission(statements, 'cloudformation:ExecuteChangeSet', stackArn, {
StringEquals: { 'cloudformation:ChangeSetName': 'MyChangeSet' }
}),
'The pipelineRole was given permissions to execute the desired ChangeSet');

test.ok(_hasAction(actions, 'AWS', 'CloudFormation', 'Deploy', {
ActionMode: 'CHANGE_SET_EXECUTE',
StackName: 'MyStack',
ChangeSetName: 'MyChangeSet'
}));

test.done();
}
}
});

interface PolicyStatementJson {
Effect: 'Allow' | 'Deny';
Action: string | string[];
Resource: string | string[];
Condition: any;
}

function _hasAction(actions: cpapi.Action[], owner: string, provider: string, category: string, configuration?: { [key: string]: any}) {
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
for (const action of actions) {
if (action.owner !== owner) { continue; }
if (action.provider !== provider) { continue; }
if (action.category !== category) { continue; }
if (configuration && !action.configuration) { continue; }
if (configuration) {
for (const key of Object.keys(configuration)) {
if (!util.isDeepStrictEqual(cdk.resolve(action.configuration[key]), cdk.resolve(configuration[key]))) {
continue;
}
}
}
return true;
}
return false;
}

function _grantsPermission(statements: PolicyStatementJson[], action: string, resource: string, conditions?: any) {
for (const statement of statements.filter(s => s.Effect === 'Allow')) {
if (!_isOrContains(statement.Action, action)) { continue; }
if (!_isOrContains(statement.Resource, resource)) { continue; }
if (conditions && !_isOrContains(statement.Condition, conditions)) { continue; }
return true;
}
return false;
}

function _isOrContains(entity: string | string[], value: string): boolean {
const resolvedValue = cdk.resolve(value);
const resolvedEntity = cdk.resolve(entity);
if (util.isDeepStrictEqual(resolvedEntity, resolvedValue)) { return true; }
if (!Array.isArray(resolvedEntity)) { return false; }
for (const tested of entity) {
if (util.isDeepStrictEqual(tested, resolvedValue)) { return true; }
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...

Integ test instead? 🙃

Copy link
Contributor Author

@RomainMuller RomainMuller Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My perception here (prove me wrong if you can :D) is that those are super brittle, because they involve almost as many, if not more, parts that are not part of the tested module than parts that I want to test. It is also not possible for me to integ-test from @aws-cdk/aws-cloudformation without incurring dependency cycles, and moving the test to somewhere else (aws-cdk/aws-codepipeline) feels wrong & would mis-represent the test coverage of the CFN L2, which I think is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's possible to substitute some of these helper functions with our expect haveResource helpers from cdk-assert? I'm worried that the failures these produce will be pretty much impossible to diagnose (for example, when you return false from _hasAction).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what we have stack traces for, but I hear you. The problem will be that expect / haveResource operate on synthesized stacks, and that I am precisely unable to synthesize a stack here because it requires me to pull in many unneeded things. Instead, I propose to make assertion-style helpers that will actually format the actual in the message, so it is more actionable.

}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codepipeline-api/lib/artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export class Artifact extends Construct {
* Output is in the form "<artifact-name>::<file-name>"
* @param fileName The name of the file
*/
public subartifact(fileName: string) {
public atPath(fileName: string) {
return new ArtifactPath(this, fileName);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ new cfn.PipelineCreateReplaceChangeSetAction(prodStage, 'PrepareChanges', {
stackName,
changeSetName,
fullPermissions: true,
templatePath: source.artifact.subartifact('template.yaml'),
templatePath: source.artifact.atPath('template.yaml'),
});

new codepipeline.ManualApprovalAction(stack, 'ApproveChanges', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export = {
new PipelineCreateUpdateStackAction(stack.deployStage, 'CreateUpdate', {
stage: stack.deployStage,
stackName: 'MyStack',
templatePath: stack.source.artifact.subartifact('template.yaml'),
templatePath: stack.source.artifact.atPath('template.yaml'),
fullPermissions: true,
});

Expand Down Expand Up @@ -256,7 +256,7 @@ export = {
new PipelineCreateUpdateStackAction(stack, 'CreateUpdate', {
stage: stack.deployStage,
stackName: 'MyStack',
templatePath: stack.source.artifact.subartifact('template.yaml'),
templatePath: stack.source.artifact.atPath('template.yaml'),
outputFileName: 'CreateResponse.json',
});

Expand Down Expand Up @@ -287,7 +287,7 @@ export = {
new PipelineCreateUpdateStackAction(stack, 'CreateUpdate', {
stage: stack.deployStage,
stackName: 'MyStack',
templatePath: stack.source.artifact.subartifact('template.yaml'),
templatePath: stack.source.artifact.atPath('template.yaml'),
replaceOnFailure: true,
});

Expand Down Expand Up @@ -320,7 +320,7 @@ export = {
new PipelineCreateUpdateStackAction(stack, 'CreateUpdate', {
stage: stack.deployStage,
stackName: 'MyStack',
templatePath: stack.source.artifact.subartifact('template.yaml'),
templatePath: stack.source.artifact.atPath('template.yaml'),
parameterOverrides: {
RepoName: stack.repo.repositoryName
}
Expand Down