Skip to content

Commit

Permalink
fix(iam): Role import doesn't fail when forgetting the region in the …
Browse files Browse the repository at this point in the history
…ARN (#13821)

If you forget the region part of the ARN for an IAM resource like a Role
(and considering the region is never provided for IAM resources,
it's easy to forget, as you have to provide it as `::`),
the IAM library doesn't catch this error,
and you only find out about it at deployment time,
with a very confusing `Fn::Select  cannot select nonexistent value at index 5`
message from CloudFormation.

Re-factor the ARN parsing code in core a little bit to allow us to catch this common error.

Fixes #13812

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
skinny85 committed Mar 30, 2021
1 parent 2384cdd commit 560a853
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 14 deletions.
17 changes: 16 additions & 1 deletion packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import '@aws-cdk/assert/jest';
import { App, CfnElement, Lazy, Stack } from '@aws-cdk/core';
import { App, Aws, CfnElement, Lazy, Stack } from '@aws-cdk/core';
import { AnyPrincipal, ArnPrincipal, IRole, Policy, PolicyStatement, Role } from '../lib';

/* eslint-disable quote-props */
Expand Down Expand Up @@ -519,6 +519,21 @@ describe('IAM Role.fromRoleArn', () => {
});
});
});

describe('for an incorrect ARN', () => {
beforeEach(() => {
roleStack = new Stack(app, 'RoleStack');
});

describe("that accidentally skipped the 'region' fragment of the ARN", () => {
test('throws an exception, indicating that error', () => {
expect(() => {
Role.fromRoleArn(roleStack, 'Role',
`arn:${Aws.PARTITION}:iam:${Aws.ACCOUNT_ID}:role/AwsCicd-${Aws.REGION}-CodeBuildRole`);
}).toThrow(/The `resource` component \(6th component\) of an ARN is required:/);
});
});
});
});

function somePolicyStatement() {
Expand Down
28 changes: 18 additions & 10 deletions packages/@aws-cdk/core/lib/arn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,36 +286,44 @@ function parseToken(arnToken: string, sep: string = '/', hasName: boolean = true
* Validate that a string is either unparseable or looks mostly like an ARN
*/
function parseArnShape(arn: string): 'token' | string[] {
const components = arn.split(':');
const looksLikeArn = arn.startsWith('arn:') && components.length >= 6;
// assume anything that starts with 'arn:' is an ARN,
// so we can report better errors
const looksLikeArn = arn.startsWith('arn:');

if (!looksLikeArn) {
if (Token.isUnresolved(arn)) { return 'token'; }
throw new Error(`ARNs must start with "arn:" and have at least 6 components: ${arn}`);
if (Token.isUnresolved(arn)) {
return 'token';
} else {
throw new Error(`ARNs must start with "arn:" and have at least 6 components: ${arn}`);
}
}

// If the ARN merely contains Tokens, but otherwise *looks* mostly like an ARN,
// it's a string of the form 'arn:${partition}:service:${region}:${account}:abc/xyz'.
// it's a string of the form 'arn:${partition}:service:${region}:${account}:resource/xyz'.
// Parse fields out to the best of our ability.
// Tokens won't contain ":", so this won't break them.
const components = arn.split(':');

const [/* arn */, partition, service, /* region */ , /* account */ , resource] = components;
// const [/* arn */, partition, service, /* region */ , /* account */ , resource] = components;

const partition = components.length > 1 ? components[1] : undefined;
if (!partition) {
throw new Error('The `partition` component (2nd component) is required: ' + arn);
throw new Error('The `partition` component (2nd component) of an ARN is required: ' + arn);
}

const service = components.length > 2 ? components[2] : undefined;
if (!service) {
throw new Error('The `service` component (3rd component) is required: ' + arn);
throw new Error('The `service` component (3rd component) of an ARN is required: ' + arn);
}

const resource = components.length > 5 ? components[5] : undefined;
if (!resource) {
throw new Error('The `resource` component (6th component) is required: ' + arn);
throw new Error('The `resource` component (6th component) of an ARN is required: ' + arn);
}

// Region can be missing in global ARNs (such as used by IAM)

// Account can be missing in some ARN types (such as used for S3 buckets)

return components;
}
}
6 changes: 3 additions & 3 deletions packages/@aws-cdk/core/test/arn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,19 +110,19 @@ nodeunitShim({

'if the ARN doesnt have enough components'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:is:too:short'), /ARNs must.*have at least 6 components.*arn:is:too:short/);
test.throws(() => stack.parseArn('arn:is:too:short'), /The `resource` component \(6th component\) of an ARN is required/);
test.done();
},

'if "service" is not specified'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:aws::4:5:6'), /The `service` component \(3rd component\) is required/);
test.throws(() => stack.parseArn('arn:aws::4:5:6'), /The `service` component \(3rd component\) of an ARN is required/);
test.done();
},

'if "resource" is not specified'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:aws:service:::'), /The `resource` component \(6th component\) is required/);
test.throws(() => stack.parseArn('arn:aws:service:::'), /The `resource` component \(6th component\) of an ARN is required/);
test.done();
},
},
Expand Down

0 comments on commit 560a853

Please sign in to comment.