From 063d1bcb6798ab1d436bf6e6bd84ac23172b4b2e Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 8 Dec 2020 14:12:04 -0800 Subject: [PATCH] feat(codebuild): prevent using Secrets in plain-text environment variables If you use a Secret in an environment variable of the default type `BuildEnvironmentVariableType.PLAINTEXT`, it will be visible in plain text in the AWS Console. Add validation that checks for this common mistake, along with a flag that allows you to opt out of it. --- .../@aws-cdk/aws-codebuild/lib/project.ts | 61 +++++++++++--- .../aws-codebuild/test/test.project.ts | 35 ++++++++ .../lib/codebuild/build-action.ts | 14 +++- .../test/codebuild/test.codebuild-action.ts | 80 ++++++++++++++++++- 4 files changed, 177 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index 597a816ca8fc1..e2bfb73932fcc 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -7,7 +7,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; import * as s3 from '@aws-cdk/aws-s3'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; -import { Aws, Duration, IResource, Lazy, Names, PhysicalName, Resource, Stack } from '@aws-cdk/core'; +import { Aws, Duration, IResource, Lazy, Names, PhysicalName, Resource, SecretValue, Stack, Tokenization } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { IArtifacts } from './artifacts'; import { BuildSpec } from './build-spec'; @@ -465,6 +465,17 @@ export interface CommonProjectProps { */ readonly environmentVariables?: { [name: string]: BuildEnvironmentVariable }; + /** + * Whether to check for the presence of any secrets in the environment variables of the default type, BuildEnvironmentVariableType.PLAINTEXT. + * Since using a secret for the value of that kind of variable would result in it being displayed in plain text in the AWS Console, + * the construct will throw an exception if it detects a secret was passed there. + * Pass this property as false if you want to skip this validation, + * and keep using a secret in a plain text environment variable. + * + * @default true + */ + readonly checkSecretsInPlainTextEnvVariables?: boolean; + /** * The physical, human-readable name of the CodeBuild Project. * @@ -659,15 +670,39 @@ export class Project extends ProjectBase { * which is the representation of environment variables in CloudFormation. * * @param environmentVariables the map of string to environment variables + * @param validateNoPlainTextSecrets whether to throw an exception + * if any of the plain text environment variables contain secrets, defaults to 'false' * @returns an array of {@link CfnProject.EnvironmentVariableProperty} instances */ - public static serializeEnvVariables(environmentVariables: { [name: string]: BuildEnvironmentVariable }): - CfnProject.EnvironmentVariableProperty[] { - return Object.keys(environmentVariables).map(name => ({ - name, - type: environmentVariables[name].type || BuildEnvironmentVariableType.PLAINTEXT, - value: environmentVariables[name].value, - })); + public static serializeEnvVariables(environmentVariables: { [name: string]: BuildEnvironmentVariable }, + validateNoPlainTextSecrets: boolean = false): CfnProject.EnvironmentVariableProperty[] { + + const ret = new Array(); + + for (const [name, envVariable] of Object.entries(environmentVariables)) { + const cfnEnvVariable: CfnProject.EnvironmentVariableProperty = { + name, + type: envVariable.type || BuildEnvironmentVariableType.PLAINTEXT, + value: envVariable.value?.toString(), + }; + ret.push(cfnEnvVariable); + + // validate that the plain-text environment variables don't contain any secrets in them + if (validateNoPlainTextSecrets && cfnEnvVariable.type === BuildEnvironmentVariableType.PLAINTEXT) { + const fragments = Tokenization.reverseString(cfnEnvVariable.value); + for (const token of fragments.tokens) { + if (token instanceof SecretValue) { + throw new Error(`Plaintext environment variable '${name}' contains a secret value! ` + + 'This means the value of this variable will be visible in plain text in the AWS Console. ' + + "Please consider using CodeBuild's SecretsManager environment variables feature instead. " + + "If you'd like to continue with having this secret in the plaintext environment variables, " + + 'please set the checkSecretsInPlainTextEnvVariables property to false'); + } + } + } + } + + return ret; } public readonly grantPrincipal: iam.IPrincipal; @@ -761,7 +796,7 @@ export class Project extends ProjectBase { }, artifacts: artifactsConfig.artifactsProperty, serviceRole: this.role.roleArn, - environment: this.renderEnvironment(props.environment, environmentVariables), + environment: this.renderEnvironment(props, environmentVariables), fileSystemLocations: Lazy.any({ produce: () => this.renderFileSystemLocations() }), // lazy, because we have a setter for it in setEncryptionKey // The 'alias/aws/s3' default is necessary because leaving the `encryptionKey` field @@ -952,8 +987,10 @@ export class Project extends ProjectBase { } private renderEnvironment( - env: BuildEnvironment = {}, + props: ProjectProps, projectVars: { [name: string]: BuildEnvironmentVariable } = {}): CfnProject.EnvironmentProperty { + + const env = props.environment ?? {}; const vars: { [name: string]: BuildEnvironmentVariable } = {}; const containerVars = env.environmentVariables || {}; @@ -1008,7 +1045,9 @@ export class Project extends ProjectBase { : undefined, privilegedMode: env.privileged || false, computeType: env.computeType || this.buildImage.defaultComputeType, - environmentVariables: hasEnvironmentVars ? Project.serializeEnvVariables(vars) : undefined, + environmentVariables: hasEnvironmentVars + ? Project.serializeEnvVariables(vars, props.checkSecretsInPlainTextEnvVariables ?? true) + : undefined, }; } diff --git a/packages/@aws-cdk/aws-codebuild/test/test.project.ts b/packages/@aws-cdk/aws-codebuild/test/test.project.ts index 81a09bd3b96f8..f3101557a691b 100644 --- a/packages/@aws-cdk/aws-codebuild/test/test.project.ts +++ b/packages/@aws-cdk/aws-codebuild/test/test.project.ts @@ -889,5 +889,40 @@ export = { test.done(); }, + + 'should fail creating when using a secret value in a plaintext variable'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // THEN + test.throws(() => { + new codebuild.PipelineProject(stack, 'Project', { + environmentVariables: { + 'a': { + value: `a_${cdk.SecretValue.secretsManager('my-secret')}_b`, + }, + }, + }); + }, /Plaintext environment variable 'a' contains a secret value!/); + + test.done(); + }, + + "should allow opting out of the 'secret value in a plaintext variable' validation"(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // THEN + new codebuild.PipelineProject(stack, 'Project', { + environmentVariables: { + 'b': { + value: cdk.SecretValue.secretsManager('my-secret'), + }, + }, + checkSecretsInPlainTextEnvVariables: false, + }); + + test.done(); + }, }, }; diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts index e81095b746eee..c833832fa565f 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts @@ -78,6 +78,17 @@ export interface CodeBuildActionProps extends codepipeline.CommonAwsActionProps */ readonly environmentVariables?: { [name: string]: codebuild.BuildEnvironmentVariable }; + /** + * Whether to check for the presence of any secrets in the environment variables of the default type, BuildEnvironmentVariableType.PLAINTEXT. + * Since using a secret for the value of that kind of variable would result in it being displayed in plain text in the AWS Console, + * the construct will throw an exception if it detects a secret was passed there. + * Pass this property as false if you want to skip this validation, + * and keep using a secret in a plain text environment variable. + * + * @default true + */ + readonly checkSecretsInPlainTextEnvVariables?: boolean; + /** * Trigger a batch build. * @@ -177,7 +188,8 @@ export class CodeBuildAction extends Action { const configuration: any = { ProjectName: this.props.project.projectName, EnvironmentVariables: this.props.environmentVariables && - cdk.Stack.of(scope).toJsonString(codebuild.Project.serializeEnvVariables(this.props.environmentVariables)), + cdk.Stack.of(scope).toJsonString(codebuild.Project.serializeEnvVariables(this.props.environmentVariables, + this.props.checkSecretsInPlainTextEnvVariables ?? true)), }; if ((this.actionProperties.inputs || []).length > 1) { // lazy, because the Artifact name might be generated lazily diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/codebuild/test.codebuild-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/codebuild/test.codebuild-action.ts index f5d5abaeb4934..85c661f2c9894 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/codebuild/test.codebuild-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/codebuild/test.codebuild-action.ts @@ -4,7 +4,7 @@ import * as codecommit from '@aws-cdk/aws-codecommit'; import * as codepipeline from '@aws-cdk/aws-codepipeline'; import * as s3 from '@aws-cdk/aws-s3'; import * as sns from '@aws-cdk/aws-sns'; -import { App, Stack } from '@aws-cdk/core'; +import { App, SecretValue, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import * as cpactions from '../../lib'; @@ -257,5 +257,83 @@ export = { test.done(); }, + + 'environment variables': { + 'should fail by default when added to a Pipeline while using a secret value in a plaintext variable'(test: Test) { + const stack = new Stack(); + + const sourceOutput = new codepipeline.Artifact(); + const pipeline = new codepipeline.Pipeline(stack, 'Pipeline', { + stages: [ + { + stageName: 'Source', + actions: [new cpactions.CodeCommitSourceAction({ + actionName: 'source', + repository: new codecommit.Repository(stack, 'CodeCommitRepo', { + repositoryName: 'my-repo', + }), + output: sourceOutput, + })], + }, + ], + }); + + const buildStage = pipeline.addStage({ + stageName: 'Build', + }); + const codeBuildProject = new codebuild.PipelineProject(stack, 'CodeBuild'); + const buildAction = new cpactions.CodeBuildAction({ + actionName: 'Build', + project: codeBuildProject, + input: sourceOutput, + environmentVariables: { + 'X': { + value: SecretValue.secretsManager('my-secret'), + }, + }, + }); + + test.throws(() => { + buildStage.addAction(buildAction); + }, /Plaintext environment variable 'X' contains a secret value!/); + test.done(); + }, + + "should allow opting out of the 'secret value in a plaintext variable' validation"(test: Test) { + const stack = new Stack(); + + const sourceOutput = new codepipeline.Artifact(); + new codepipeline.Pipeline(stack, 'Pipeline', { + stages: [ + { + stageName: 'Source', + actions: [new cpactions.CodeCommitSourceAction({ + actionName: 'source', + repository: new codecommit.Repository(stack, 'CodeCommitRepo', { + repositoryName: 'my-repo', + }), + output: sourceOutput, + })], + }, + { + stageName: 'Build', + actions: [new cpactions.CodeBuildAction({ + actionName: 'build', + project: new codebuild.PipelineProject(stack, 'CodeBuild'), + input: sourceOutput, + environmentVariables: { + 'X': { + value: SecretValue.secretsManager('my-secret'), + }, + }, + checkSecretsInPlainTextEnvVariables: false, + })], + }, + ], + }); + + test.done(); + }, + }, }, };