Skip to content

Commit

Permalink
fix(pipelines): Secrets Manager permissions not added to asset projec…
Browse files Browse the repository at this point in the history 
…ts (#15718)

We used to use an immutable singleton role with `*` permissions for the assets projects, because if there were many different destinations in a pipeline, and each asset build had to publish to each destination, the policy could grow too long and exceed the maximum policy size.

However, this also disabled the automatic policy wrangling that CodeBuild would do for us, like automatically adding permissions to bind to a VPC, and adding permissions to read Secrets Manager secrets. This especially becoming relevant since that now in the modern API, it's possible to modify build the environment in a way that normally automatically adds SecretsManager permission, but now won't (and it's not possible to fix either).

Replace the immutable singleton role with a mutable singleton role, but in such a way that it won't add permissions statements for which it already has a `*` statement (to cut down on duplication), and have the CB project do the automatic VPC bind permissions again.

Fixes #15628.


----

*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
rix0rrr authored Jul 23, 2021
1 parent e133bca commit 7668400
Show file tree
Hide file tree
Showing 9 changed files with 2,730 additions and 211 deletions.
46 changes: 29 additions & 17 deletions packages/@aws-cdk/aws-codebuild/lib/project.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { CODEPIPELINE_SOURCE_ARTIFACTS_TYPE, NO_SOURCE_TYPE } from './source-typ
// eslint-disable-next-line
import { Construct as CoreConstruct } from '@aws-cdk/core';

const VPC_POLICY_SYM = Symbol.for('@aws-cdk/aws-codebuild.roleVpcPolicy');

/**
* The type returned from {@link IProject#enableBatchBuilds}.
*/
Expand Down Expand Up @@ -1437,23 +1439,33 @@ export class Project extends ProjectBase {
},
}));

const policy = new iam.Policy(this, 'PolicyDocument', {
statements: [
new iam.PolicyStatement({
resources: ['*'],
actions: [
'ec2:CreateNetworkInterface',
'ec2:DescribeNetworkInterfaces',
'ec2:DeleteNetworkInterface',
'ec2:DescribeSubnets',
'ec2:DescribeSecurityGroups',
'ec2:DescribeDhcpOptions',
'ec2:DescribeVpcs',
],
}),
],
});
this.role.attachInlinePolicy(policy);
// If the same Role is used for multiple Projects, always creating a new `iam.Policy`
// will attach the same policy multiple times, probably exceeding the maximum size of the
// Role policy. Make sure we only do it once for the same role.
//
// This deduplication could be a feature of the Role itself, but that feels risky and
// is hard to implement (what with Tokens and all). Safer to fix it locally for now.
let policy: iam.Policy | undefined = (this.role as any)[VPC_POLICY_SYM];
if (!policy) {
policy = new iam.Policy(this, 'PolicyDocument', {
statements: [
new iam.PolicyStatement({
resources: ['*'],
actions: [
'ec2:CreateNetworkInterface',
'ec2:DescribeNetworkInterfaces',
'ec2:DeleteNetworkInterface',
'ec2:DescribeSubnets',
'ec2:DescribeSecurityGroups',
'ec2:DescribeDhcpOptions',
'ec2:DescribeVpcs',
],
}),
],
});
this.role.attachInlinePolicy(policy);
(this.role as any)[VPC_POLICY_SYM] = policy;
}

// add an explicit dependency between the EC2 Policy and this Project -
// otherwise, creating the Project fails, as it requires these permissions
Expand Down
35 changes: 35 additions & 0 deletions packages/@aws-cdk/aws-codebuild/test/test.project.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,41 @@ export = {
test.done();
},

'if a role is shared between projects in a VPC, the VPC Policy is only attached once'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Vpc');
const role = new iam.Role(stack, 'Role', {
assumedBy: new iam.ServicePrincipal('codebuild.amazonaws.com'),
});
const source = codebuild.Source.gitHubEnterprise({
httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo',
});

// WHEN
new codebuild.Project(stack, 'Project1', { source, role, vpc, projectName: 'P1' });
new codebuild.Project(stack, 'Project2', { source, role, vpc, projectName: 'P2' });

// THEN
// - 1 is for `ec2:CreateNetworkInterfacePermission`, deduplicated as they're part of a single policy
// - 1 is for `ec2:CreateNetworkInterface`, this is the separate Policy we're deduplicating
// We would have found 3 if the deduplication didn't work.
expect(stack).to(countResources('AWS::IAM::Policy', 2));

// THEN - both Projects have a DependsOn on the same policy
expect(stack).to(haveResourceLike('AWS::CodeBuild::Project', {
Properties: { Name: 'P1' },
DependsOn: ['Project1PolicyDocumentF9761562'],
}, ResourcePart.CompleteDefinition));

expect(stack).to(haveResourceLike('AWS::CodeBuild::Project', {
Properties: { Name: 'P1' },
DependsOn: ['Project1PolicyDocumentF9761562'],
}, ResourcePart.CompleteDefinition));

test.done();
},

'can use an imported Role for a Project within a VPC'(test: Test) {
const stack = new cdk.Stack();

Expand Down
113 changes: 9 additions & 104 deletions packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ import * as cp from '@aws-cdk/aws-codepipeline';
import * as cpa from '@aws-cdk/aws-codepipeline-actions';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { Aws, Fn, IDependable, Lazy, PhysicalName, Stack } from '@aws-cdk/core';
import { Aws, Fn, Lazy, PhysicalName, Stack } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct, Node } from 'constructs';
import { AssetType, FileSet, IFileSetProducer, ManualApprovalStep, ShellStep, StackAsset, StackDeployment, Step } from '../blueprint';
import { DockerCredential, dockerCredentialsInstallCommands, DockerCredentialUsage } from '../docker-credentials';
import { GraphNode, GraphNodeCollection, isGraph, AGraphNode, PipelineGraph } from '../helpers-internal';
import { PipelineBase } from '../main';
import { AssetSingletonRole } from '../private/asset-singleton-role';
import { appOf, assemblyBuilderOf, embeddedAsmPath, obtainScope } from '../private/construct-internals';
import { toPosixPath } from '../private/fs';
import { enumerate, flatten, maybeSuffix, noUndefined } from '../private/javascript';
Expand Down Expand Up @@ -254,11 +255,6 @@ export class CodePipeline extends PipelineBase {
*/
private readonly assetCodeBuildRoles: Record<string, iam.IRole> = {};

/**
* Policies created for the build projects that they have to depend on
*/
private readonly assetAttachedPolicies: Record<string, iam.Policy> = {};

/**
* Per asset type, the target role ARNs that need to be assumed
*/
Expand Down Expand Up @@ -635,7 +631,7 @@ export class CodePipeline extends PipelineBase {
}
}

const assetBuildConfig = this.obtainAssetCodeBuildRole(assets[0].assetType);
const role = this.obtainAssetCodeBuildRole(assets[0].assetType);

// The base commands that need to be run
const script = new CodeBuildStep(node.id, {
Expand All @@ -647,13 +643,12 @@ export class CodePipeline extends PipelineBase {
buildEnvironment: {
privileged: assets.some(asset => asset.assetType === AssetType.DOCKER_IMAGE),
},
role: assetBuildConfig.role,
role,
});

// Customizations that are not accessible to regular users
return CodeBuildFactory.fromCodeBuildStep(node.id, script, {
additionalConstructLevel: false,
additionalDependable: assetBuildConfig.dependable,

// If we use a single publisher, pass buildspec via file otherwise it'll
// grow too big.
Expand Down Expand Up @@ -775,64 +770,22 @@ export class CodePipeline extends PipelineBase {
* Modeled after the CodePipeline role and 'CodePipelineActionRole' roles.
* Generates one role per asset type to separate file and Docker/image-based permissions.
*/
private obtainAssetCodeBuildRole(assetType: AssetType): AssetCodeBuildRole {
private obtainAssetCodeBuildRole(assetType: AssetType): iam.IRole {
if (this.assetCodeBuildRoles[assetType]) {
return {
role: this.assetCodeBuildRoles[assetType],
dependable: this.assetAttachedPolicies[assetType],
};
return this.assetCodeBuildRoles[assetType];
}

const stack = Stack.of(this);

const rolePrefix = assetType === AssetType.DOCKER_IMAGE ? 'Docker' : 'File';
const assetRole = new iam.Role(this.assetsScope, `${rolePrefix}Role`, {
const assetRole = new AssetSingletonRole(this.assetsScope, `${rolePrefix}Role`, {
roleName: PhysicalName.GENERATE_IF_NEEDED,
assumedBy: new iam.CompositePrincipal(
new iam.ServicePrincipal('codebuild.amazonaws.com'),
new iam.AccountPrincipal(stack.account),
),
});

// Logging permissions
const logGroupArn = stack.formatArn({
service: 'logs',
resource: 'log-group',
sep: ':',
resourceName: '/aws/codebuild/*',
});
assetRole.addToPolicy(new iam.PolicyStatement({
resources: [logGroupArn],
actions: ['logs:CreateLogGroup', 'logs:CreateLogStream', 'logs:PutLogEvents'],
}));

// CodeBuild report groups
const codeBuildArn = stack.formatArn({
service: 'codebuild',
resource: 'report-group',
resourceName: '*',
});
assetRole.addToPolicy(new iam.PolicyStatement({
actions: [
'codebuild:CreateReportGroup',
'codebuild:CreateReport',
'codebuild:UpdateReport',
'codebuild:BatchPutTestCases',
'codebuild:BatchPutCodeCoverages',
],
resources: [codeBuildArn],
}));

// CodeBuild start/stop
assetRole.addToPolicy(new iam.PolicyStatement({
resources: ['*'],
actions: [
'codebuild:BatchGetBuilds',
'codebuild:StartBuild',
'codebuild:StopBuild',
],
}));

// Publishing role access
// The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted.
// Lazy-evaluated so all asset publishing roles are included.
Expand All @@ -846,51 +799,8 @@ export class CodePipeline extends PipelineBase {
this.dockerCredentials.forEach(reg => reg.grantRead(assetRole, DockerCredentialUsage.ASSET_PUBLISHING));
}

// Artifact access
this.pipeline.artifactBucket.grantRead(assetRole);

// VPC permissions required for CodeBuild
// Normally CodeBuild itself takes care of this but we're creating a singleton role so now
// we need to do this.
const assetCodeBuildOptions = this.codeBuildDefaultsFor(CodeBuildProjectType.ASSETS);
if (assetCodeBuildOptions?.vpc) {
const vpcPolicy = new iam.Policy(assetRole, 'VpcPolicy', {
statements: [
new iam.PolicyStatement({
resources: [`arn:${Aws.PARTITION}:ec2:${Aws.REGION}:${Aws.ACCOUNT_ID}:network-interface/*`],
actions: ['ec2:CreateNetworkInterfacePermission'],
conditions: {
StringEquals: {
'ec2:Subnet': assetCodeBuildOptions.vpc
.selectSubnets(assetCodeBuildOptions.subnetSelection).subnetIds
.map(si => `arn:${Aws.PARTITION}:ec2:${Aws.REGION}:${Aws.ACCOUNT_ID}:subnet/${si}`),
'ec2:AuthorizedService': 'codebuild.amazonaws.com',
},
},
}),
new iam.PolicyStatement({
resources: ['*'],
actions: [
'ec2:CreateNetworkInterface',
'ec2:DescribeNetworkInterfaces',
'ec2:DeleteNetworkInterface',
'ec2:DescribeSubnets',
'ec2:DescribeSecurityGroups',
'ec2:DescribeDhcpOptions',
'ec2:DescribeVpcs',
],
}),
],
});
assetRole.attachInlinePolicy(vpcPolicy);
this.assetAttachedPolicies[assetType] = vpcPolicy;
}

this.assetCodeBuildRoles[assetType] = assetRole.withoutPolicyUpdates();
return {
role: this.assetCodeBuildRoles[assetType],
dependable: this.assetAttachedPolicies[assetType],
};
this.assetCodeBuildRoles[assetType] = assetRole;
return assetRole;
}
}

Expand All @@ -903,11 +813,6 @@ function dockerUsageFromCodeBuild(cbt: CodeBuildProjectType): DockerCredentialUs
}
}

interface AssetCodeBuildRole {
readonly role: iam.IRole;
readonly dependable?: IDependable;
}

enum CodeBuildProjectType {
SYNTH = 'SYNTH',
ASSETS = 'ASSETS',
Expand Down
Loading

0 comments on commit 7668400

Please sign in to comment.