From 611e001bb5ec9caabf1a826be94f00b7a0d55f47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 21 Jun 2019 10:10:58 +0200 Subject: [PATCH 1/3] fix(core): Prevent volatile physical name generation The PhysicalName generation strategy for cross-account/region use-cases could generate names that are subject to collisions/sniping when the account and region were not specified; and those names would also have changed if a stack went from re-locatable to account/region specific, even if the target environment would not have changed. The generator will now throw errors in case the account ID or region is blank or a Token. --- .../cdk/lib/private/physical-name-generator.ts | 16 ++++++++-------- .../cdk/test/test.cross-environment-token.ts | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/@aws-cdk/cdk/lib/private/physical-name-generator.ts b/packages/@aws-cdk/cdk/lib/private/physical-name-generator.ts index 21aa333ff7f36..0ac00d8d72e2f 100644 --- a/packages/@aws-cdk/cdk/lib/private/physical-name-generator.ts +++ b/packages/@aws-cdk/cdk/lib/private/physical-name-generator.ts @@ -8,14 +8,14 @@ export function generatePhysicalName(resource: IResource): string { const stackPart = new PrefixNamePart(stack.stackName, 25); const idPart = new SuffixNamePart(resource.node.uniqueId, 24); - let region: string | undefined = stack.region; - if (Token.isUnresolved(region)) { - region = undefined; + const region: string = stack.region; + if (Token.isUnresolved(region) || !region) { + throw new Error(`Cannot generate a physical name for ${resource.node.path}, because the region is un-resolved or missing`); } - let account: string | undefined = stack.account; - if (Token.isUnresolved(account)) { - account = undefined; + const account: string = stack.account; + if (Token.isUnresolved(account) || !account) { + throw new Error(`Cannot generate a physical name for ${resource.node.path}, because the account is un-resolved or missing`); } const parts = [stackPart, idPart] @@ -25,8 +25,8 @@ export function generatePhysicalName(resource: IResource): string { const sha256 = crypto.createHash('sha256') .update(stackPart.bareStr) .update(idPart.bareStr) - .update(region || '') - .update(account || ''); + .update(region) + .update(account); const hash = sha256.digest('hex').slice(0, hashLength); const ret = [...parts, hash].join(''); diff --git a/packages/@aws-cdk/cdk/test/test.cross-environment-token.ts b/packages/@aws-cdk/cdk/test/test.cross-environment-token.ts index 2a30d6883052a..bd6b32442232f 100644 --- a/packages/@aws-cdk/cdk/test/test.cross-environment-token.ts +++ b/packages/@aws-cdk/cdk/test/test.cross-environment-token.ts @@ -12,6 +12,7 @@ export = { const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', + region: 'bermuda-triangle-1337', }, }); const myResource = new MyResource(stack1, 'MyResource', PhysicalName.of('PhysicalName')); @@ -19,6 +20,7 @@ export = { const stack2 = new Stack(app, 'Stack2', { env: { account: '234567890123', + region: 'bermuda-triangle-42', }, }); @@ -56,11 +58,13 @@ export = { const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', + region: 'bermuda-triangle-1337', }, }); const stack2 = new Stack(app, 'Stack2', { env: { account: '234567890123', + region: 'bermuda-triangle-42', }, }); @@ -88,6 +92,7 @@ export = { const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', + region: 'bermuda-triangle-1337', }, }); const myResource = new MyResource(stack1, 'MyResource', PhysicalName.auto({ crossEnvironment: true })); @@ -95,6 +100,7 @@ export = { const stack2 = new Stack(app, 'Stack2', { env: { account: '234567890123', + region: 'bermuda-triangle-42', }, }); @@ -115,7 +121,7 @@ export = { { Ref: 'AWS::Partition', }, - ':myservice:::my-resource/stack1stack1myresourcec54ced43dab875fcfa49', + ':myservice:::my-resource/stack1stack1myresourcec54ced43683ebf9a3c4c', ], ], }, @@ -132,11 +138,13 @@ export = { const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', + region: 'bermuda-triangle-1337', }, }); const stack2 = new Stack(app, 'Stack2', { env: { account: '234567890123', + region: 'bermuda-triangle-42', }, }); @@ -150,7 +158,7 @@ export = { test.deepEqual(toCloudFormation(stack2), { Outputs: { Output: { - Value: 'stack1stack1myresourcec54ced43dab875fcfa49', + Value: 'stack1stack1myresourcec54ced43683ebf9a3c4c', }, }, }); @@ -165,11 +173,13 @@ export = { const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', + region: 'bermuda-triangle-1337', }, }); const stack2 = new Stack(app, 'Stack2', { env: { account: '234567890123', + region: 'bermuda-triangle-42', }, }); From 9ff4304af7c1f2d5cf505ac2334fa96da835934a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 24 Jun 2019 11:13:08 +0200 Subject: [PATCH 2/3] Add test --- .../private/test.physical-name-generator.ts | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 packages/@aws-cdk/core/test/private/test.physical-name-generator.ts diff --git a/packages/@aws-cdk/core/test/private/test.physical-name-generator.ts b/packages/@aws-cdk/core/test/private/test.physical-name-generator.ts new file mode 100644 index 0000000000000..75cc5af30216b --- /dev/null +++ b/packages/@aws-cdk/core/test/private/test.physical-name-generator.ts @@ -0,0 +1,92 @@ +import nodeunit = require('nodeunit'); +import { App, Aws, Resource, Stack } from '../../lib'; +import { generatePhysicalName } from '../../lib/private/physical-name-generator'; + +export = nodeunit.testCase({ + 'generates correct physical names'(test: nodeunit.Test) { + const app = new App(); + const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } }); + + const testResourceA = new TestResource(stack, 'A'); + const testResourceB = new TestResource(testResourceA, 'B'); + + test.equal(generatePhysicalName(testResourceA), 'teststackteststackaa164c141d59b37c1b663'); + test.equal(generatePhysicalName(testResourceB), 'teststackteststackab27595cd34d8188283a1f'); + + test.done(); + }, + + 'generates different names in different accounts'(test: nodeunit.Test) { + const appA = new App(); + const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } }); + const resourceA = new TestResource(stackA, 'Resource'); + + const appB = new App(); + const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678913', region: 'bermuda-triangle-1' } }); + const resourceB = new TestResource(stackB, 'Resource'); + + test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB)); + + test.done(); + }, + + 'generates different names in different regions'(test: nodeunit.Test) { + const appA = new App(); + const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } }); + const resourceA = new TestResource(stackA, 'Resource'); + + const appB = new App(); + const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-2' } }); + const resourceB = new TestResource(stackB, 'Resource'); + + test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB)); + + test.done(); + }, + + 'fails when the region is an unresolved token'(test: nodeunit.Test) { + const app = new App(); + const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: Aws.REGION } }); + const testResource = new TestResource(stack, 'A'); + + test.throws(() => generatePhysicalName(testResource), + /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/); + + test.done(); + }, + + 'fails when the region is not provided'(test: nodeunit.Test) { + const app = new App(); + const stack = new Stack(app, 'TestStack', { env: { account: '012345678912' } }); + const testResource = new TestResource(stack, 'A'); + + test.throws(() => generatePhysicalName(testResource), + /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/); + + test.done(); + }, + + 'fails when the account is an unresolved token'(test: nodeunit.Test) { + const app = new App(); + const stack = new Stack(app, 'TestStack', { env: { account: Aws.ACCOUNT_ID, region: 'bermuda-triangle-1' } }); + const testResource = new TestResource(stack, 'A'); + + test.throws(() => generatePhysicalName(testResource), + /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/); + + test.done(); + }, + + 'fails when the account is not provided'(test: nodeunit.Test) { + const app = new App(); + const stack = new Stack(app, 'TestStack', { env: { region: 'bermuda-triangle-1' } }); + const testResource = new TestResource(stack, 'A'); + + test.throws(() => generatePhysicalName(testResource), + /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/); + + test.done(); + }, +}); + +class TestResource extends Resource {} From e1afab6c68db950f5f428337144de74c40908924 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 24 Jun 2019 12:30:39 +0200 Subject: [PATCH 3/3] Fix test --- .../aws-codepipeline-actions/test/test.pipeline.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts index 82709290c4418..6d372f45d9252 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts @@ -756,8 +756,9 @@ export = { const app = new App(); const buildAccount = '901234567890'; + const buildRegion = 'bermuda-triangle-1'; const buildStack = new Stack(app, 'BuildStack', { - env: { account: buildAccount }, + env: { account: buildAccount, region: buildRegion }, }); const rolePhysicalName = 'ProjectRolePhysicalName'; const projectRole = new iam.Role(buildStack, 'ProjectRole', { @@ -771,7 +772,7 @@ export = { }); const pipelineStack = new Stack(app, 'PipelineStack', { - env: { account: '123456789012' }, + env: { account: '123456789012', region: 'bermuda-triangle-42' }, }); const sourceBucket = new s3.Bucket(pipelineStack, 'ArtifactBucket', { bucketName: 'source-bucket', @@ -826,7 +827,7 @@ export = { { "Ref": "AWS::Partition", }, - `:iam::${buildAccount}:role/buildstackebuildactionrole166c75d145cdaa010350`, + `:iam::${buildAccount}:role/buildstackebuildactionrole166c75d1d8be701b1ad8`, ], ], }, @@ -861,7 +862,7 @@ export = { { "Ref": "AWS::Partition", }, - `:s3:::pipelinestackeartifactsbucket5409dc8418216ab8debe`, + ':s3:::pipelinestackeartifactsbucket5409dc84ec8d21c5e28c', ], ], }, @@ -873,7 +874,7 @@ export = { { "Ref": "AWS::Partition", }, - `:s3:::pipelinestackeartifactsbucket5409dc8418216ab8debe/*`, + ':s3:::pipelinestackeartifactsbucket5409dc84ec8d21c5e28c/*', ], ], },