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

fix(codebuild): allow passing the ARN of the Secret in environment variables #13706

Merged
merged 2 commits into from
Mar 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
72 changes: 54 additions & 18 deletions packages/@aws-cdk/aws-codebuild/lib/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, SecretValue, Stack, Tokenization } from '@aws-cdk/core';
import { ArnComponents, Aws, Duration, IResource, Lazy, Names, PhysicalName, Resource, SecretValue, Stack, Token, Tokenization } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { IArtifacts } from './artifacts';
import { BuildSpec } from './build-spec';
Expand Down Expand Up @@ -707,14 +707,15 @@ export class Project extends ProjectBase {
validateNoPlainTextSecrets: boolean = false, principal?: iam.IGrantable): CfnProject.EnvironmentVariableProperty[] {

const ret = new Array<CfnProject.EnvironmentVariableProperty>();
const ssmVariables = new Array<string>();
const secretsManagerSecrets = new Array<string>();
const ssmIamResources = new Array<string>();
const secretsManagerIamResources = new Array<string>();

for (const [name, envVariable] of Object.entries(environmentVariables)) {
const envVariableValue = envVariable.value?.toString();
const cfnEnvVariable: CfnProject.EnvironmentVariableProperty = {
name,
type: envVariable.type || BuildEnvironmentVariableType.PLAINTEXT,
value: envVariable.value?.toString(),
value: envVariableValue,
};
ret.push(cfnEnvVariable);

Expand All @@ -733,10 +734,11 @@ export class Project extends ProjectBase {
}

if (principal) {
const stack = Stack.of(principal);

// save the SSM env variables
if (envVariable.type === BuildEnvironmentVariableType.PARAMETER_STORE) {
const envVariableValue = envVariable.value.toString();
ssmVariables.push(Stack.of(principal).formatArn({
ssmIamResources.push(stack.formatArn({
service: 'ssm',
resource: 'parameter',
// If the parameter name starts with / the resource name is not separated with a double '/'
Expand All @@ -749,27 +751,58 @@ export class Project extends ProjectBase {

// save SecretsManager env variables
if (envVariable.type === BuildEnvironmentVariableType.SECRETS_MANAGER) {
secretsManagerSecrets.push(Stack.of(principal).formatArn({
service: 'secretsmanager',
resource: 'secret',
// we don't know the exact ARN of the Secret just from its name, but we can get close
resourceName: `${envVariable.value}-??????`,
sep: ':',
}));
if (Token.isUnresolved(envVariableValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for this condition? I'm not seeing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - those are the can be provided as the ARN attribute of a new Secret and can be provided as the ARN attribute of a new Secret, followed by a JSON key tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess I was looking for a test where a Fn:Split call was expected, as we might be splitting a token. Now I see that we're assuming the input is partially an unresolved token (e.g., ${secretArn}:jsonKey).

If I understand correctly, you're supporting something like the above, but not a "full" token'ed value. So if I create a custom resource which returns back my-secret-name:jsonKey as a Token, then we will be passing the Token as a whole to secretsManagerIamResources, and the grant won't work. Am I reading that correctly now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're reading it correctly.

What's the behavior that you would want to see here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Frankly, I'm not sure. I don't know that there's a good solution here. I'm hoping this is a relatively edge-of-the-corner case. :D

// the value of the property can be a complex string, separated by ':';
// see https://docs.aws.amazon.com/codebuild/latest/userguide/build-spec-ref.html#build-spec.env.secrets-manager
const secretArn = envVariableValue.split(':')[0];

// if we are passed a Token, we should assume it's the ARN of the Secret
// (as the name would not work anyway, because it would be the full name, which CodeBuild does not support)
secretsManagerIamResources.push(secretArn);
} else {
// check if the provided value is a full ARN of the Secret
let parsedArn: ArnComponents | undefined;
try {
parsedArn = stack.parseArn(envVariableValue, ':');
} catch (e) {}
const secretSpecifier: string = parsedArn ? parsedArn.resourceName : envVariableValue;

// the value of the property can be a complex string, separated by ':';
// see https://docs.aws.amazon.com/codebuild/latest/userguide/build-spec-ref.html#build-spec.env.secrets-manager
const secretName = secretSpecifier.split(':')[0];
const secretIamResourceName = parsedArn
// If we were given an ARN, we don't' know whether the name is full, or partial,
// as CodeBuild supports both ARN forms.
// Because of that, follow the name with a '*', which works for both
? `${secretName}*`
// If we were given just a name, it must be partial, as CodeBuild doesn't support providing full names.
// In this case, we need to accommodate for the generated suffix in the IAM resource name
: `${secretName}-??????`;
secretsManagerIamResources.push(Stack.of(principal).formatArn({
service: 'secretsmanager',
resource: 'secret',
resourceName: secretIamResourceName,
sep: ':',
// if we were given an ARN, we need to use the provided partition/account/region
partition: parsedArn?.partition,
account: parsedArn?.account,
region: parsedArn?.region,
}));
}
}
}
}

if (ssmVariables.length !== 0) {
if (ssmIamResources.length !== 0) {
principal?.grantPrincipal.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['ssm:GetParameters'],
resources: ssmVariables,
resources: ssmIamResources,
}));
}
if (secretsManagerSecrets.length !== 0) {
if (secretsManagerIamResources.length !== 0) {
principal?.grantPrincipal.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['secretsmanager:GetSecretValue'],
resources: secretsManagerSecrets,
resources: secretsManagerIamResources,
}));
}

Expand Down Expand Up @@ -1831,7 +1864,10 @@ export interface BuildEnvironmentVariable {
* The value of the environment variable.
* For plain-text variables (the default), this is the literal value of variable.
* For SSM parameter variables, pass the name of the parameter here (`parameterName` property of `IParameter`).
* For SecretsManager variables secrets, pass the secret name here (`secretName` property of `ISecret`).
* For SecretsManager variables secrets, pass either the secret name (`secretName` property of `ISecret`)
* or the secret ARN (`secretArn` property of `ISecret`) here,
* along with optional SecretsManager qualifiers separated by ':', like the JSON key, or the version or stage
* (see https://docs.aws.amazon.com/codebuild/latest/userguide/build-spec-ref.html#build-spec.env.secrets-manager for details).
*/
readonly value: any;
}
Expand Down
Loading