From ca15ac09fb8b59c9bd56cc724089b8d331f76a79 Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Sat, 11 Apr 2020 12:09:12 +0200 Subject: [PATCH 1/5] fix(ecs): using secret JSON field with fargate task does not fail For tasks that use the Fargate launch type, it is only supported to inject the full contents of a secret as an environment variable. Specifying a specific JSON key or version is not supported at this time. Also clean up/refactor a bit. See https://docs.aws.amazon.com/AmazonECS/latest/userguide/specifying-sensitive-data-secrets.html#secrets-considerations Closes #7272 --- .../aws-ecs/lib/base/task-definition.ts | 2 +- .../aws-ecs/lib/container-definition.ts | 49 ++++++++++++++----- packages/@aws-cdk/aws-ecs/package.json | 2 - .../integ.secret-json-field.expected.json} | 9 ++-- .../integ.secret-json-field.ts} | 8 ++- .../aws-ecs/test/test.container-definition.ts | 23 +++++++++ 6 files changed, 68 insertions(+), 25 deletions(-) rename packages/@aws-cdk/aws-ecs/test/{fargate/integ.secret-json-key.expected.json => ec2/integ.secret-json-field.expected.json} (94%) rename packages/@aws-cdk/aws-ecs/test/{fargate/integ.secret-json-key.ts => ec2/integ.secret-json-field.ts} (75%) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts b/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts index 00c14d4b47e9e..94c70c44748c9 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts @@ -477,7 +477,7 @@ export class TaskDefinition extends TaskDefinitionBase { } } - return this.containers.map(x => x.renderContainerDefinition(this)); + return this.containers.map(x => x.renderContainerDefinition()); } } diff --git a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts index e876f31fc730e..f080d7845e2ea 100644 --- a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts @@ -36,11 +36,24 @@ export abstract class Secret { public static fromSecretsManager(secret: secretsmanager.ISecret, field?: string): Secret { return { arn: field ? `${secret.secretArn}:${field}::` : secret.secretArn, + hasField: true, grantRead: grantee => secret.grantRead(grantee), }; } + /** + * The ARN of the secret + */ public abstract readonly arn: string; + + /** + * Whether this secret uses a specific JSON field + */ + public abstract readonly hasField?: boolean; + + /** + * Grants reading the secret to a principal + */ public abstract grantRead(grantee: iam.IGrantable): iam.Grant; } @@ -519,9 +532,9 @@ export class ContainerDefinition extends cdk.Construct { /** * Render this container definition to a CloudFormation object * - * @param taskDefinition [disable-awslint:ref-via-interface] (made optional to avoid breaking change) + * @param _taskDefinition [disable-awslint:ref-via-interface] (unused but kept to avoid breaking change) */ - public renderContainerDefinition(taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty { + public renderContainerDefinition(_taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty { return { command: this.props.command, cpu: this.props.cpu, @@ -551,16 +564,7 @@ export class ContainerDefinition extends cdk.Construct { workingDirectory: this.props.workingDirectory, logConfiguration: this.logDriverConfig, environment: this.props.environment && renderKV(this.props.environment, 'name', 'value'), - secrets: this.props.secrets && Object.entries(this.props.secrets) - .map(([k, v]) => { - if (taskDefinition) { - v.grantRead(taskDefinition.obtainExecutionRole()); - } - return { - name: k, - valueFrom: v.arn - }; - }), + secrets: this.renderSecrets(), extraHosts: this.props.extraHosts && renderKV(this.props.extraHosts, 'hostname', 'ipAddress'), healthCheck: this.props.healthCheck && renderHealthCheck(this.props.healthCheck), links: cdk.Lazy.listValue({ produce: () => this.links }, { omitEmpty: true }), @@ -568,6 +572,27 @@ export class ContainerDefinition extends cdk.Construct { resourceRequirements: (this.props.gpuCount !== undefined) ? renderResourceRequirements(this.props.gpuCount) : undefined, }; } + + private renderSecrets(): CfnTaskDefinition.SecretProperty[] | undefined { + if (!this.props.secrets) { + return undefined; + } + + const secrets: CfnTaskDefinition.SecretProperty[] = []; + + for (const [k, v] of Object.entries(this.props.secrets)) { + if (this.taskDefinition.isFargateCompatible && v.hasField) { + throw new Error(`Cannot specify secret JSON field for a task using the FARGATE launch type: '${k}' in container '${this.node.id}'`); + } + v.grantRead(this.taskDefinition.obtainExecutionRole()); + secrets.push({ + name: k, + valueFrom: v.arn + }); + } + + return secrets; + } } /** diff --git a/packages/@aws-cdk/aws-ecs/package.json b/packages/@aws-cdk/aws-ecs/package.json index 7fd8d3bb13b82..c1d156ede68ee 100644 --- a/packages/@aws-cdk/aws-ecs/package.json +++ b/packages/@aws-cdk/aws-ecs/package.json @@ -137,8 +137,6 @@ "docs-public-apis:@aws-cdk/aws-ecs.GelfCompressionType.GZIP", "docs-public-apis:@aws-cdk/aws-ecs.WindowsOptimizedVersion.SERVER_2016", "docs-public-apis:@aws-cdk/aws-ecs.WindowsOptimizedVersion.SERVER_2019", - "docs-public-apis:@aws-cdk/aws-ecs.Secret.arn", - "docs-public-apis:@aws-cdk/aws-ecs.Secret.grantRead", "props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.egressIgnoredIPs", "props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.egressIgnoredPorts", "props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.ignoredGID", diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.expected.json similarity index 94% rename from packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.expected.json rename to packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.expected.json index 1a8e791bff7e1..5378fdbb03212 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.expected.json @@ -33,6 +33,7 @@ { "Essential": true, "Image": "amazon/amazon-ecs-sample", + "Memory": 256, "Name": "web", "Secrets": [ { @@ -52,18 +53,16 @@ ] } ], - "Cpu": "512", "ExecutionRoleArn": { "Fn::GetAtt": [ "TaskDefExecutionRoleB4775C97", "Arn" ] }, - "Family": "awsecsintegsecretjsonkeyTaskDefC01C0E99", - "Memory": "1024", - "NetworkMode": "awsvpc", + "Family": "awsecsintegsecretjsonfieldTaskDef1C2EE990", + "NetworkMode": "bridge", "RequiresCompatibilities": [ - "FARGATE" + "EC2" ], "TaskRoleArn": { "Fn::GetAtt": [ diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.ts b/packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.ts similarity index 75% rename from packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.ts rename to packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.ts index c7f7073512b98..a3bd8acd3d741 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret-json-key.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.secret-json-field.ts @@ -3,7 +3,7 @@ import * as cdk from '@aws-cdk/core'; import * as ecs from '../../lib'; const app = new cdk.App(); -const stack = new cdk.Stack(app, 'aws-ecs-integ-secret-json-key'); +const stack = new cdk.Stack(app, 'aws-ecs-integ-secret-json-field'); const secret = new secretsmanager.Secret(stack, 'Secret', { generateSecretString: { @@ -12,13 +12,11 @@ const secret = new secretsmanager.Secret(stack, 'Secret', { } }); -const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef', { - memoryLimitMiB: 1024, - cpu: 512 -}); +const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef'); taskDefinition.addContainer('web', { image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 256, secrets: { PASSWORD: ecs.Secret.fromSecretsManager(secret, 'password') } diff --git a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts index 69809a149d6fd..62d8f819ebd1b 100644 --- a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts @@ -843,6 +843,29 @@ export = { }, + 'throws when using a specific secret JSON field as environment variable for a Fargate task'(test: Test) { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'Stack'); + const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef'); + + const secret = new secretsmanager.Secret(stack, 'Secret'); + + // WHEN + taskDefinition.addContainer('cont', { + image: ecs.ContainerImage.fromRegistry('test'), + memoryLimitMiB: 1024, + secrets: { + SECRET_KEY: ecs.Secret.fromSecretsManager(secret, 'specificKey'), + } + }); + + // THEN + test.throws(() => app.synth(), /Cannot specify secret JSON field for a task using the FARGATE launch type/); + + test.done(); + }, + 'can add AWS logging to container definition'(test: Test) { // GIVEN const stack = new cdk.Stack(); From 9ad6215d696ebd4ce843c0e3fb7b469150f0522b Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Sat, 11 Apr 2020 12:25:54 +0200 Subject: [PATCH 2/5] git add . -u -pfield --- packages/@aws-cdk/aws-ecs/lib/container-definition.ts | 2 +- packages/@aws-cdk/aws-ecs/test/test.container-definition.ts | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts index f080d7845e2ea..73b73f1ee3f4c 100644 --- a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts @@ -36,7 +36,7 @@ export abstract class Secret { public static fromSecretsManager(secret: secretsmanager.ISecret, field?: string): Secret { return { arn: field ? `${secret.secretArn}:${field}::` : secret.secretArn, - hasField: true, + hasField: !!field, grantRead: grantee => secret.grantRead(grantee), }; } diff --git a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts index 62d8f819ebd1b..126bba6176d80 100644 --- a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts @@ -795,6 +795,9 @@ export = { } })); + test.equal(ecs.Secret.fromSecretsManager(secret).hasField, false); + test.equal(ecs.Secret.fromSsmParameter(parameter).hasField, undefined); + test.done(); }, From ea4609f38b2bbd44afd8964fe7fb6e70f77cb200 Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Sat, 11 Apr 2020 12:53:15 +0200 Subject: [PATCH 3/5] extra integ test --- .../test/fargate/integ.secret.expected.json | 109 ++++++++++++++++++ .../aws-ecs/test/fargate/integ.secret.ts | 24 ++++ 2 files changed, 133 insertions(+) create mode 100644 packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.expected.json create mode 100644 packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.ts diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.expected.json b/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.expected.json new file mode 100644 index 0000000000000..919ea2bbf03d8 --- /dev/null +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.expected.json @@ -0,0 +1,109 @@ +{ + "Resources": { + "SecretA720EF05": { + "Type": "AWS::SecretsManager::Secret", + "Properties": { + "GenerateSecretString": { + "GenerateStringKey": "password", + "SecretStringTemplate": "{\"username\":\"user\"}" + } + } + }, + "TaskDefTaskRole1EDB4A67": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Statement": [ + { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": { + "Service": "ecs-tasks.amazonaws.com" + } + } + ], + "Version": "2012-10-17" + } + } + }, + "TaskDef54694570": { + "Type": "AWS::ECS::TaskDefinition", + "Properties": { + "ContainerDefinitions": [ + { + "Essential": true, + "Image": "amazon/amazon-ecs-sample", + "Name": "web", + "Secrets": [ + { + "Name": "SECRET", + "ValueFrom": { + "Ref": "SecretA720EF05" + } + } + ] + } + ], + "Cpu": "256", + "ExecutionRoleArn": { + "Fn::GetAtt": [ + "TaskDefExecutionRoleB4775C97", + "Arn" + ] + }, + "Family": "awsecsintegsecretTaskDef58AA207D", + "Memory": "512", + "NetworkMode": "awsvpc", + "RequiresCompatibilities": [ + "FARGATE" + ], + "TaskRoleArn": { + "Fn::GetAtt": [ + "TaskDefTaskRole1EDB4A67", + "Arn" + ] + } + } + }, + "TaskDefExecutionRoleB4775C97": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Statement": [ + { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": { + "Service": "ecs-tasks.amazonaws.com" + } + } + ], + "Version": "2012-10-17" + } + } + }, + "TaskDefExecutionRoleDefaultPolicy0DBB737A": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": "secretsmanager:GetSecretValue", + "Effect": "Allow", + "Resource": { + "Ref": "SecretA720EF05" + } + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "TaskDefExecutionRoleDefaultPolicy0DBB737A", + "Roles": [ + { + "Ref": "TaskDefExecutionRoleB4775C97" + } + ] + } + } + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.ts b/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.ts new file mode 100644 index 0000000000000..7ef33a197889a --- /dev/null +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.ts @@ -0,0 +1,24 @@ +import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; +import * as cdk from '@aws-cdk/core'; +import * as ecs from '../../lib'; + +const app = new cdk.App(); +const stack = new cdk.Stack(app, 'aws-ecs-integ-secret'); + +const secret = new secretsmanager.Secret(stack, 'Secret', { + generateSecretString: { + generateStringKey: 'password', + secretStringTemplate: JSON.stringify({ username: 'user' }) + } +}); + +const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef'); + +taskDefinition.addContainer('web', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + secrets: { + SECRET: ecs.Secret.fromSecretsManager(secret) + } +}); + +app.synth(); From 1f19bff95b7401116c33e3c6624bd77bc87ab5ee Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Sat, 11 Apr 2020 22:19:17 +0200 Subject: [PATCH 4/5] useless taskDefinition --- packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts b/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts index 5d1e9aec1f2b6..f507625f0ff82 100644 --- a/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts +++ b/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts @@ -236,9 +236,9 @@ export class FirelensLogRouter extends ContainerDefinition { /** * Render this container definition to a CloudFormation object */ - public renderContainerDefinition(taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty { + public renderContainerDefinition(_taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty { return { - ...(super.renderContainerDefinition(taskDefinition)), + ...(super.renderContainerDefinition()), firelensConfiguration: this.firelensConfig && renderFirelensConfig(this.firelensConfig), }; } From 11fd607e874b4fe6287b8fc62406cc96cea59ecf Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Wed, 29 Apr 2020 11:03:12 +0200 Subject: [PATCH 5/5] move to constructor --- .../aws-ecs/lib/container-definition.ts | 39 ++++++++----------- .../aws-ecs/test/test.container-definition.ts | 15 ++----- 2 files changed, 21 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts index e1e03a62ea879..6e2a4f2868192 100644 --- a/packages/@aws-cdk/aws-ecs/lib/container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/lib/container-definition.ts @@ -361,6 +361,8 @@ export class ContainerDefinition extends cdk.Construct { private readonly imageConfig: ContainerImageConfig; + private readonly secrets?: CfnTaskDefinition.SecretProperty[]; + /** * Constructs a new instance of the ContainerDefinition class. */ @@ -382,6 +384,20 @@ export class ContainerDefinition extends cdk.Construct { this.logDriverConfig = props.logging.bind(this, this); } props.taskDefinition._linkContainer(this); + + if (props.secrets) { + this.secrets = []; + for (const [name, secret] of Object.entries(props.secrets)) { + if (this.taskDefinition.isFargateCompatible && secret.hasField) { + throw new Error(`Cannot specify secret JSON field for a task using the FARGATE launch type: '${name}' in container '${this.node.id}'`); + } + secret.grantRead(this.taskDefinition.obtainExecutionRole()); + this.secrets.push({ + name, + valueFrom: secret.arn, + }); + } + } } /** @@ -564,7 +580,7 @@ export class ContainerDefinition extends cdk.Construct { workingDirectory: this.props.workingDirectory, logConfiguration: this.logDriverConfig, environment: this.props.environment && renderKV(this.props.environment, 'name', 'value'), - secrets: this.renderSecrets(), + secrets: this.secrets, extraHosts: this.props.extraHosts && renderKV(this.props.extraHosts, 'hostname', 'ipAddress'), healthCheck: this.props.healthCheck && renderHealthCheck(this.props.healthCheck), links: cdk.Lazy.listValue({ produce: () => this.links }, { omitEmpty: true }), @@ -572,27 +588,6 @@ export class ContainerDefinition extends cdk.Construct { resourceRequirements: (this.props.gpuCount !== undefined) ? renderResourceRequirements(this.props.gpuCount) : undefined, }; } - - private renderSecrets(): CfnTaskDefinition.SecretProperty[] | undefined { - if (!this.props.secrets) { - return undefined; - } - - const secrets: CfnTaskDefinition.SecretProperty[] = []; - - for (const [k, v] of Object.entries(this.props.secrets)) { - if (this.taskDefinition.isFargateCompatible && v.hasField) { - throw new Error(`Cannot specify secret JSON field for a task using the FARGATE launch type: '${k}' in container '${this.node.id}'`); - } - v.grantRead(this.taskDefinition.obtainExecutionRole()); - secrets.push({ - name: k, - valueFrom: v.arn, - }); - } - - return secrets; - } } /** diff --git a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts index 28e9747bb5eec..cccb2e9efdefb 100644 --- a/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts +++ b/packages/@aws-cdk/aws-ecs/test/test.container-definition.ts @@ -795,9 +795,6 @@ export = { }, })); - test.equal(ecs.Secret.fromSecretsManager(secret).hasField, false); - test.equal(ecs.Secret.fromSsmParameter(parameter).hasField, undefined); - test.done(); }, @@ -848,23 +845,19 @@ export = { 'throws when using a specific secret JSON field as environment variable for a Fargate task'(test: Test) { // GIVEN - const app = new cdk.App(); - const stack = new cdk.Stack(app, 'Stack'); + const stack = new cdk.Stack(); const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef'); const secret = new secretsmanager.Secret(stack, 'Secret'); - // WHEN - taskDefinition.addContainer('cont', { + // THEN + test.throws(() => taskDefinition.addContainer('cont', { image: ecs.ContainerImage.fromRegistry('test'), memoryLimitMiB: 1024, secrets: { SECRET_KEY: ecs.Secret.fromSecretsManager(secret, 'specificKey'), }, - }); - - // THEN - test.throws(() => app.synth(), /Cannot specify secret JSON field for a task using the FARGATE launch type/); + }), /Cannot specify secret JSON field for a task using the FARGATE launch type/); test.done(); },