From 2eef1833f82062febfe3ef6a2825fbd37b3d2a9a Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 28 May 2019 14:24:01 -0700 Subject: [PATCH] fix(codebuild): correctly handle permissions for Projects inside VPC. A CodeBuild Project needs to have appropriate EC2 permissions on creation when it uses a VPC. However, the default Policy that a Project Role has depends on the Project itself (for CloudWatch Logs permissions). Because of that, add a dependency between the Policy containing the EC2 permissions and the Project. Also correctly handle the case when the Project's Role is imported. Fixes #2651 Fixes #2652 --- .../@aws-cdk/aws-codebuild/lib/project.ts | 81 ++++++++++++------- .../test/integ.project-vpc.expected.json | 7 +- .../aws-codebuild/test/test.project.ts | 24 ++++++ 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index b7535f8af7bb2..087f0acefedd6 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -6,7 +6,7 @@ import ecr = require('@aws-cdk/aws-ecr'); import events = require('@aws-cdk/aws-events'); import iam = require('@aws-cdk/aws-iam'); import kms = require('@aws-cdk/aws-kms'); -import { Aws, Construct, IResource, Resource, Token } from '@aws-cdk/cdk'; +import { Aws, CfnResource, Construct, IResource, Resource, Token } from '@aws-cdk/cdk'; import { BuildArtifacts, CodePipelineBuildArtifacts, NoBuildArtifacts } from './artifacts'; import { Cache } from './cache'; import { CfnProject } from './codebuild.generated'; @@ -707,6 +707,8 @@ export class Project extends ProjectBase { vpcConfig: this.configureVpc(props), }); + this.addVpcRequiredPermissions(props); + this.projectArn = resource.projectArn; this.projectName = resource.projectName; @@ -727,20 +729,6 @@ export class Project extends ProjectBase { } } - /** - * Add a permission only if there's a policy attached. - * @param statement The permissions statement to add - */ - public addToRoleInlinePolicy(statement: iam.PolicyStatement) { - if (this.role) { - const policy = new iam.Policy(this, 'PolicyDocument', { - policyName: 'CodeBuildEC2Policy', - statements: [statement] - }); - this.role.attachInlinePolicy(policy); - } - } - /** * Adds a secondary source to the Project. * @@ -882,7 +870,29 @@ export class Project extends ProjectBase { }); this._securityGroups = [securityGroup]; } - this.addToRoleInlinePolicy(new iam.PolicyStatement() + + return { + vpcId: props.vpc.vpcId, + subnets: props.vpc.selectSubnets(props.subnetSelection).subnetIds, + securityGroupIds: this._securityGroups.map(s => s.securityGroupId) + }; + } + + private addVpcRequiredPermissions(props: ProjectProps): void { + if (!props.vpc) { + return; + } + + this.addToRolePolicy(new iam.PolicyStatement() + .addResource(`arn:aws:ec2:${Aws.region}:${Aws.accountId}:network-interface/*`) + .addAction('ec2:CreateNetworkInterfacePermission') + .addCondition('StringEquals', { + 'ec2:Subnet': props.vpc + .selectSubnets(props.subnetSelection).subnetIds + .map(si => `arn:aws:ec2:${Aws.region}:${Aws.accountId}:subnet/${si}`), + 'ec2:AuthorizedService': "codebuild.amazonaws.com", + })); + this.addInlineEc2Policy(new iam.PolicyStatement() .addAllResources() .addActions( 'ec2:CreateNetworkInterface', @@ -891,22 +901,31 @@ export class Project extends ProjectBase { 'ec2:DescribeSubnets', 'ec2:DescribeSecurityGroups', 'ec2:DescribeDhcpOptions', - 'ec2:DescribeVpcs' + 'ec2:DescribeVpcs', )); - this.addToRolePolicy(new iam.PolicyStatement() - .addResource(`arn:aws:ec2:${Aws.region}:${Aws.accountId}:network-interface/*`) - .addCondition('StringEquals', { - "ec2:Subnet": props.vpc - .selectSubnets(props.subnetSelection).subnetIds - .map(si => `arn:aws:ec2:${Aws.region}:${Aws.accountId}:subnet/${si}`), - "ec2:AuthorizedService": "codebuild.amazonaws.com" - }) - .addAction('ec2:CreateNetworkInterfacePermission')); - return { - vpcId: props.vpc.vpcId, - subnets: props.vpc.selectSubnets(props.subnetSelection).subnetIds, - securityGroupIds: this._securityGroups.map(s => s.securityGroupId) - }; + } + + private addInlineEc2Policy(statement: iam.PolicyStatement) { + if (this.role) { + // If the Role is imported, the Policy will be left dangling, + // not attached to any IAM entity, which is invalid in CFN. + // Because of that, check for that case before creating the Policy + const cfnRole = this.role.node.tryFindChild('Resource') as CfnResource; + if (cfnRole) { + const policy = new iam.Policy(this, 'PolicyDocument', { + policyName: 'CodeBuildEC2Policy', + statements: [statement] + }); + this.role.attachInlinePolicy(policy); + + // add an explicit dependency between the EC2 Policy and this Project - + // otherwise, creating the Project fails, + // as it requires these permissions to be already attached to the Project's Role + const cfnPolicy = policy.node.findChild('Resource') as CfnResource; + const cfnProject = this.node.findChild('Resource') as CfnResource; + cfnProject.addDependsOn(cfnPolicy); + } + } } private parseArtifacts(props: ProjectProps) { diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json index 35f355bfeb053..0ebb733a366b0 100644 --- a/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json +++ b/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json @@ -524,7 +524,10 @@ "Ref": "MyVPCAFB07A31" } } - } + }, + "DependsOn": [ + "MyProjectPolicyDocument646EE0F2" + ] } }, "Parameters": { @@ -541,4 +544,4 @@ "Description": "Artifact hash for asset \"aws-cdk-codebuild-project-vpc/Bundle\"" } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-codebuild/test/test.project.ts b/packages/@aws-cdk/aws-codebuild/test/test.project.ts index 174ce65be4eae..590497c780eea 100644 --- a/packages/@aws-cdk/aws-codebuild/test/test.project.ts +++ b/packages/@aws-cdk/aws-codebuild/test/test.project.ts @@ -1,5 +1,7 @@ import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert'; import assets = require('@aws-cdk/assets'); +import ec2 = require('@aws-cdk/aws-ec2'); +import iam = require('@aws-cdk/aws-iam'); import { Bucket } from '@aws-cdk/aws-s3'; import cdk = require('@aws-cdk/cdk'); import { Test } from 'nodeunit'; @@ -239,4 +241,26 @@ export = { test.done(); }, + + 'can use an imported Role for a Project within a VPC'(test: Test) { + const stack = new cdk.Stack(); + + const importedRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::1234567890:role/service-role/codebuild-bruiser-service-role'); + const vpc = new ec2.Vpc(stack, 'Vpc'); + + new codebuild.Project(stack, 'Project', { + source: new codebuild.GitHubEnterpriseSource({ + httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo', + }), + artifacts: new codebuild.NoBuildArtifacts(), + role: importedRole, + vpc, + }); + + expect(stack).to(haveResourceLike('AWS::CodeBuild::Project', { + // no need to do any assertions + })); + + test.done(); + }, };