From e6da9aff22c1793378ce6e6224f6fd79d10d7094 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Wed, 16 Jun 2021 17:54:50 -0700 Subject: [PATCH] fix(core): parsing an ARN with a slash after a colon in the resource part fails New-style ARNs are of the form 'arn:aws:s4:us-west-1:12345:/resource-type/resource-name'. We didn't handle that correctly in parseArn(), and instead returned an `undefined` resource, which funnily enough should never happen according to our types. Spotted in https://github.com/aws/aws-cdk/pull/15140/files#r653112073 --- packages/@aws-cdk/core/lib/arn.ts | 196 ++++++++++++++++++------ packages/@aws-cdk/core/lib/stack.ts | 18 ++- packages/@aws-cdk/core/test/arn.test.ts | 62 +++++++- tools/nodeunit-shim/index.ts | 32 ++-- 4 files changed, 239 insertions(+), 69 deletions(-) diff --git a/packages/@aws-cdk/core/lib/arn.ts b/packages/@aws-cdk/core/lib/arn.ts index fe6b5b12b934b..37e992e5f114b 100644 --- a/packages/@aws-cdk/core/lib/arn.ts +++ b/packages/@aws-cdk/core/lib/arn.ts @@ -3,6 +3,50 @@ import { Stack } from './stack'; import { Token } from './token'; import { filterUndefined } from './util'; +/** + * An enum representing the various ARN formats that different services use. + */ +export enum ArnFormat { + /** + * This represents a format where there is no 'resourceName' part. + * This format is used for S3 resources, + * like 'arn:aws:s3:::bucket'. + * Everything after the last colon is considered the 'resource', + * even if it contains slashes, + * like in 'arn:aws:s3:::bucket/object.zip'. + */ + NO_RESOURCE_NAME = 'arn:aws:service:region:account:resource', + + /** + * This represents a format where the 'resource' and 'resourceName' + * parts are separated with a colon. + * Like in: 'arn:aws:service:region:account:resource:resourceName'. + * Everything after the last colon is considered the 'resourceName', + * even if it contains slashes, + * like in 'arn:aws:apigateway:region:account:resource:/test/mydemoresource/*'. + */ + COLON_RESOURCE_NAME = 'arn:aws:service:region:account:resource:resourceName', + + /** + * This represents a format where the 'resource' and 'resourceName' + * parts are separated with a slash. + * Like in: 'arn:aws:service:region:account:resource/resourceName'. + * Everything after the separating slash is considered the 'resourceName', + * even if it contains colons, + * like in 'arn:aws:cognito-sync:region:account:identitypool/us-east-1:1a1a1a1a-ffff-1111-9999-12345678:bla'. + */ + SLASH_RESOURCE_NAME = 'arn:aws:service:region:account:resource/resourceName', + + /** + * This represents a format where the 'resource' and 'resourceName' + * parts are seperated with a slash, + * but there is also an additional slash after the colon separating 'account' from 'resource'. + * Like in: 'arn:aws:service:region:account:/resource/resourceName'. + * Note that the leading slash is _not_ included in the parsed 'resource' part. + */ + SLASH_RESOURCE_NAME_AND_RESOURCE = 'arn:aws:service:region:account:/resource/resourceName', +} + export interface ArnComponents { /** * The partition that the resource is in. For standard AWS regions, the @@ -56,6 +100,13 @@ export interface ArnComponents { * a wildcard such as ``"*"``. This is service-dependent. */ readonly resourceName?: string; + + /** + * The specific ARN format to use for this ARN value. + * + * @default - for parsing, the format is required; for formatting, sep will be used + */ + readonly arnFormat?: ArnFormat; } export class Arn { @@ -80,9 +131,13 @@ export class Arn { const partition = components.partition ?? stack.partition; const region = components.region ?? stack.region; const account = components.account ?? stack.account; - const sep = components.sep ?? '/'; + const sep = components.sep ?? (components.arnFormat === ArnFormat.COLON_RESOURCE_NAME ? ':' : '/'); - const values = ['arn', ':', partition, ':', components.service, ':', region, ':', account, ':', components.resource]; + const values = [ + 'arn', ':', partition, ':', components.service, ':', region, ':', account, ':', + ...(components.arnFormat === ArnFormat.SLASH_RESOURCE_NAME_AND_RESOURCE ? ['/'] : []), + components.resource, + ]; if (sep !== '/' && sep !== ':' && sep !== '') { throw new Error('resourcePathSep may only be ":", "/" or an empty string'); @@ -133,11 +188,33 @@ export class Arn { * * @returns an ArnComponents object which allows access to the various * components of the ARN. + * + * @deprecated use split instead */ public static parse(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents { + let arnFormat: ArnFormat; + if (!hasName) { + arnFormat = ArnFormat.NO_RESOURCE_NAME; + } else { + arnFormat = sepIfToken === '/' ? ArnFormat.SLASH_RESOURCE_NAME : ArnFormat.COLON_RESOURCE_NAME; + } + return this.split(arn, arnFormat); + } + + /** + * Splits the provided ARN into its components. + * Works both if 'arn' is a string like 'arn:aws:s3:::bucket', + * and a Token representing a dynamic CloudFormation expression + * (in which case the returned components will also be dynamic CloudFormation expressions, + * encoded as Tokens). + * + * @param arn the ARN to split into its components + * @param arnFormat the expected format of 'arn' - depends on what format the service 'arn' represents uses + */ + public static split(arn: string, arnFormat: ArnFormat): ArnComponents { const components = parseArnShape(arn); if (components === 'token') { - return parseToken(arn, sepIfToken, hasName); + return parseTokenArn(arn, arnFormat); } const [, partition, service, region, account, resourceTypeOrName, ...rest] = components; @@ -145,18 +222,41 @@ export class Arn { let resource: string; let resourceName: string | undefined; let sep: string | undefined; - - let sepIndex = resourceTypeOrName.indexOf('/'); - if (sepIndex !== -1) { - sep = '/'; + let resourcePartStartIndex = 0; + let detectedArnFormat: ArnFormat; + + let slashIndex = resourceTypeOrName.indexOf('/'); + if (slashIndex === 0) { + // new-style ARNs are of the form 'arn:aws:s4:us-west-1:12345:/resource-type/resource-name' + slashIndex = resourceTypeOrName.indexOf('/', 1); + resourcePartStartIndex = 1; + detectedArnFormat = ArnFormat.SLASH_RESOURCE_NAME_AND_RESOURCE; + } + if (slashIndex !== -1) { + // the slash is only a separator if ArnFormat is not NO_RESOURCE_NAME + if (arnFormat === ArnFormat.NO_RESOURCE_NAME) { + sep = undefined; + slashIndex = -1; + detectedArnFormat = ArnFormat.NO_RESOURCE_NAME; + } else { + sep = '/'; + detectedArnFormat = resourcePartStartIndex === 0 + ? ArnFormat.SLASH_RESOURCE_NAME + // need to repeat this here, as otherwise the compiler thinks 'detectedArnFormat' is not initialized in all paths + : ArnFormat.SLASH_RESOURCE_NAME_AND_RESOURCE; + } } else if (rest.length > 0) { sep = ':'; - sepIndex = -1; + slashIndex = -1; + detectedArnFormat = ArnFormat.COLON_RESOURCE_NAME; + } else { + sep = undefined; + detectedArnFormat = ArnFormat.NO_RESOURCE_NAME; } - if (sepIndex !== -1) { - resource = resourceTypeOrName.substr(0, sepIndex); - resourceName = resourceTypeOrName.substr(sepIndex + 1); + if (slashIndex !== -1) { + resource = resourceTypeOrName.substring(resourcePartStartIndex, slashIndex); + resourceName = resourceTypeOrName.substring(slashIndex + 1); } else { resource = resourceTypeOrName; } @@ -182,6 +282,7 @@ export class Arn { account, resourceName, sep, + arnFormat: detectedArnFormat, }); } @@ -232,32 +333,18 @@ export class Arn { * subexpressions of the ARN, not string literals. * * WARNING: this function cannot properly parse the complete final - * resourceName (path) out of ARNs that use '/' to both separate the - * 'resource' from the 'resourceName' AND to subdivide the resourceName - * further. For example, in S3 ARNs: - * - * arn:aws:s3:::my_corporate_bucket/path/to/exampleobject.png - * - * After parsing the resourceName will not contain 'path/to/exampleobject.png' - * but simply 'path'. This is a limitation because there is no slicing - * functionality in CloudFormation templates. + * 'resourceName' part if it contains colons, + * like 'arn:aws:cognito-sync:region:account:identitypool/us-east-1:1a1a1a1a-ffff-1111-9999-12345678:bla'. * * @param arnToken The input token that contains an ARN - * @param sep The separator used to separate resource from resourceName - * @param hasName Whether there is a name component in the ARN at all. - * For example, SNS Topics ARNs have the 'resource' component contain the - * topic name, and no 'resourceName' component. - * @returns an ArnComponents object which allows access to the various - * components of the ARN. + * @param arnFormat the expected format of 'arn' - depends on what format the service the ARN represents uses */ -function parseToken(arnToken: string, sep: string = '/', hasName: boolean = true): ArnComponents { - // Arn ARN looks like: - // arn:partition:service:region:account-id:resource - // arn:partition:service:region:account-id:resourcetype/resource - // arn:partition:service:region:account-id:resourcetype:resource - - // We need the 'hasName' argument because {Fn::Select}ing a nonexistent field - // throws an error. +function parseTokenArn(arnToken: string, arnFormat: ArnFormat): ArnComponents { + // ARN looks like: + // arn:partition:service:region:account:resource + // arn:partition:service:region:account:resource:resourceName + // arn:partition:service:region:account:resource/resourceName + // arn:partition:service:region:account:/resource/resourceName const components = Fn.split(':', arnToken); @@ -265,22 +352,39 @@ function parseToken(arnToken: string, sep: string = '/', hasName: boolean = true const service = Fn.select(2, components).toString(); const region = Fn.select(3, components).toString(); const account = Fn.select(4, components).toString(); - - if (sep === ':') { - const resource = Fn.select(5, components).toString(); - const resourceName = hasName ? Fn.select(6, components).toString() : undefined; - - return { partition, service, region, account, resource, resourceName, sep }; + let resource: string; + let resourceName: string | undefined; + let sep: string | undefined; + + if (arnFormat === ArnFormat.NO_RESOURCE_NAME || arnFormat === ArnFormat.COLON_RESOURCE_NAME) { + // we know that the 'resource' part will always be the 6th segment in this case + resource = Fn.select(5, components); + if (arnFormat === ArnFormat.COLON_RESOURCE_NAME) { + resourceName = Fn.select(6, components); + sep = ':'; + } else { + resourceName = undefined; + sep = undefined; + } } else { - const lastComponents = Fn.split(sep, Fn.select(5, components)); + // we know that the 'resource' and 'resourceName' parts are separated by slash here, + // so we split the 6th segment from the colon-separated ones with a slash + const lastComponents = Fn.split('/', Fn.select(5, components)); - const resource = Fn.select(0, lastComponents).toString(); - const resourceName = hasName ? Fn.select(1, lastComponents).toString() : undefined; - - return { partition, service, region, account, resource, resourceName, sep }; + if (arnFormat === ArnFormat.SLASH_RESOURCE_NAME) { + resource = Fn.select(0, lastComponents); + resourceName = Fn.select(1, lastComponents); + } else { + // arnFormat is ArnFormat.SLASH_RESOURCE_NAME_AND_RESOURCE, + // which means there's an extra slash there at the beginning that we need to skip + resource = Fn.select(1, lastComponents); + resourceName = Fn.select(2, lastComponents); + } + sep = '/'; } -} + return { partition, service, region, account, resource, resourceName, sep, arnFormat }; +} /** * Validate that a string is either unparseable or looks mostly like an ARN diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index b32095233b80d..a74a82e55ccd6 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -5,7 +5,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import { IConstruct, Construct, Node } from 'constructs'; import { Annotations } from './annotations'; import { App } from './app'; -import { Arn, ArnComponents } from './arn'; +import { Arn, ArnComponents, ArnFormat } from './arn'; import { DockerImageAssetLocation, DockerImageAssetSource, FileAssetLocation, FileAssetSource } from './assets'; import { CfnElement } from './cfn-element'; import { Fn } from './cfn-fn'; @@ -609,11 +609,27 @@ export class Stack extends CoreConstruct implements ITaggable { * * @returns an ArnComponents object which allows access to the various * components of the ARN. + * + * @deprecated use splitArn instead */ public parseArn(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents { return Arn.parse(arn, sepIfToken, hasName); } + /** + * Splits the provided ARN into its components. + * Works both if 'arn' is a string like 'arn:aws:s3:::bucket', + * and a Token representing a dynamic CloudFormation expression + * (in which case the returned components will also be dynamic CloudFormation expressions, + * encoded as Tokens). + * + * @param arn the ARN to split into its components + * @param arnFormat the expected format of 'arn' - depends on what format the service 'arn' represents uses + */ + public splitArn(arn: string, arnFormat: ArnFormat): ArnComponents { + return Arn.split(arn, arnFormat); + } + /** * Returns the list of AZs that are available in the AWS environment * (account/region) associated with this stack. diff --git a/packages/@aws-cdk/core/test/arn.test.ts b/packages/@aws-cdk/core/test/arn.test.ts index febbcc407c2b7..f713d09aa16f5 100644 --- a/packages/@aws-cdk/core/test/arn.test.ts +++ b/packages/@aws-cdk/core/test/arn.test.ts @@ -1,5 +1,5 @@ import { nodeunitShim, Test } from 'nodeunit-shim'; -import { Arn, ArnComponents, Aws, CfnOutput, ScopedAws, Stack, Token } from '../lib'; +import { Arn, ArnComponents, ArnFormat, Aws, CfnOutput, ScopedAws, Stack, Token } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { evaluateCFN } from './evaluate-cfn'; import { toCloudFormation } from './util'; @@ -128,8 +128,13 @@ nodeunitShim({ }, 'various successful parses'(test: Test) { + interface TestArnComponents extends ArnComponents { + /** @default true */ + checkCfnEncoding?: boolean; + } + const stack = new Stack(); - const tests: { [arn: string]: ArnComponents } = { + const tests: { [arn: string]: TestArnComponents } = { 'arn:aws:a4b:region:accountid:resourcetype/resource': { partition: 'aws', service: 'a4b', @@ -138,6 +143,7 @@ nodeunitShim({ resource: 'resourcetype', resourceName: 'resource', sep: '/', + arnFormat: ArnFormat.SLASH_RESOURCE_NAME, }, 'arn:aws:apigateway:us-east-1::a123456789012bc3de45678901f23a45:/test/mydemoresource/*': { partition: 'aws', @@ -147,6 +153,7 @@ nodeunitShim({ resource: 'a123456789012bc3de45678901f23a45', sep: ':', resourceName: '/test/mydemoresource/*', + arnFormat: ArnFormat.COLON_RESOURCE_NAME, }, 'arn:aws-cn:cloud9::123456789012:environment:81e900317347585a0601e04c8d52eaEX': { partition: 'aws-cn', @@ -156,6 +163,7 @@ nodeunitShim({ resource: 'environment', resourceName: '81e900317347585a0601e04c8d52eaEX', sep: ':', + arnFormat: ArnFormat.COLON_RESOURCE_NAME, }, 'arn:aws:cognito-sync:::identitypool/us-east-1:1a1a1a1a-ffff-1111-9999-12345678:bla': { service: 'cognito-sync', @@ -165,6 +173,19 @@ nodeunitShim({ resource: 'identitypool', resourceName: 'us-east-1:1a1a1a1a-ffff-1111-9999-12345678:bla', sep: '/', + arnFormat: ArnFormat.SLASH_RESOURCE_NAME, + // cannot be successfully parsed with CFN functions because of the colons in resourceName + checkCfnEncoding: false, + }, + 'arn:aws:servicecatalog:us-east-1:123456789012:/applications/0aqmvxvgmry0ecc4mjhwypun6i': { + resource: 'applications', + resourceName: '0aqmvxvgmry0ecc4mjhwypun6i', + sep: '/', + service: 'servicecatalog', + region: 'us-east-1', + account: '123456789012', + partition: 'aws', + arnFormat: ArnFormat.SLASH_RESOURCE_NAME_AND_RESOURCE, }, 'arn:aws:s3:::my_corporate_bucket': { partition: 'aws', @@ -172,13 +193,41 @@ nodeunitShim({ region: '', account: '', resource: 'my_corporate_bucket', + arnFormat: ArnFormat.NO_RESOURCE_NAME, + }, + 'arn:aws:s3:::my_corporate_bucket/object.zip': { + partition: 'aws', + service: 's3', + region: '', + account: '', + resource: 'my_corporate_bucket/object.zip', + arnFormat: ArnFormat.NO_RESOURCE_NAME, }, }; - Object.keys(tests).forEach(arn => { - const expected = tests[arn]; - test.deepEqual(stack.parseArn(arn), expected, arn); - }); + for (const [arn, expectedComponents] of Object.entries(tests)) { + const skipCheckingCfnEncoding = expectedComponents.checkCfnEncoding === false; + // delete the extra field so it doesn't screw up the equality comparison + delete expectedComponents.checkCfnEncoding; + + // test the basic case + const parsedComponents = stack.splitArn(arn, expectedComponents.arnFormat!); + test.deepEqual(parsedComponents, expectedComponents, arn); + + // test the round-trip + test.equal(stack.formatArn(parsedComponents), arn); + + // test that the CloudFormation functions we generate evaluate to the correct value + if (skipCheckingCfnEncoding) { + continue; + } + const tokenArnComponents = stack.splitArn( + Token.asString(new Intrinsic({ Ref: 'TheArn' })), + parsedComponents.arnFormat!); + const cfnArnComponents = stack.resolve(tokenArnComponents); + const evaluatedArnComponents = evaluateCFN(cfnArnComponents, { TheArn: arn }); + test.deepEqual(evaluatedArnComponents, parsedComponents); + } test.done(); }, @@ -250,6 +299,7 @@ nodeunitShim({ resource: 'role', resourceName: 'abc123', sep: '/', + arnFormat: ArnFormat.SLASH_RESOURCE_NAME, }; test.deepEqual(stack.parseArn(arn), expected, arn); diff --git a/tools/nodeunit-shim/index.ts b/tools/nodeunit-shim/index.ts index 8ba50bedefefd..ced735d9e6897 100644 --- a/tools/nodeunit-shim/index.ts +++ b/tools/nodeunit-shim/index.ts @@ -11,36 +11,36 @@ export class Test { constructor(private readonly cb: () => void) { } - public equal(a: any, b: any, _message?: string) { - expect(a).toEqual(b); + public equal(actual: any, expected: any, _message?: string) { + expect(actual).toEqual(expected); } - public notEqual(a: any, b: any, _message?: string) { - expect(a).not.toEqual(b); + public notEqual(actual: any, expected: any, _message?: string) { + expect(actual).not.toEqual(expected); } - public equals(a: any, b: any, _message?: string) { - expect(a).toEqual(b); + public equals(actual: any, expected: any, _message?: string) { + expect(actual).toEqual(expected); } - public strictEqual(a: any, b: any, _message?: string) { - expect(a).toEqual(b); + public strictEqual(actual: any, expected: any, _message?: string) { + expect(actual).toEqual(expected); } - public deepEqual(a: any, b: any, _message?: string) { - expect(a).toEqual(b); + public deepEqual(actual: any, expected: any, _message?: string) { + expect(actual).toEqual(expected); } - public notDeepEqual(a: any, b: any, _message?: string) { - expect(a).not.toEqual(b); + public notDeepEqual(actual: any, expected: any, _message?: string) { + expect(actual).not.toEqual(expected); } - public ok(a: any, _message?: string) { - expect(a).toBeTruthy(); + public ok(actual: any, _message?: string) { + expect(actual).toBeTruthy(); } - public same(a: any, b: any) { - expect(a).toBe(b); + public same(actual: any, expected: any) { + expect(actual).toBe(expected); } public throws(block: () => any, error?: string | RegExp | ErrorConstructor, _message?: string) {