From bb623193c5ff87a42dc110ebd852d7422b05581f Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 30 Jul 2019 13:35:18 -0700 Subject: [PATCH] fix(iam): correctly limit the default PolicyName to 128 characters Our logic for trimming the length of the default IAM policy name was not working, as it wasn't updated when logicalId became a Token rather than a literate string, and so it was never actually triggered (it just checked that the display name of the Token was less than 128 characters, which it always is). The fix is to resolve the logical ID Token before applying the trimming logic. Fixes #3402 --- packages/@aws-cdk/aws-iam/lib/policy.ts | 4 +- packages/@aws-cdk/aws-iam/lib/util.ts | 25 ++++++++-- packages/@aws-cdk/aws-iam/test/test.policy.ts | 48 ++++++++++++++----- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts index 0617aacf6a5a1..564b33a649532 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy.ts @@ -1,4 +1,4 @@ -import { Construct, IResource, Lazy, Resource } from '@aws-cdk/core'; +import { Construct, IResource, Lazy, Resource} from '@aws-cdk/core'; import { IGroup } from './group'; import { CfnPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; @@ -96,7 +96,7 @@ export class Policy extends Resource implements IPolicy { // generatePolicyName will take the last 128 characters of the logical id since // policy names are limited to 128. the last 8 chars are a stack-unique hash, so // that shouod be sufficient to ensure uniqueness within a principal. - Lazy.stringValue({ produce: () => generatePolicyName(resource.logicalId) }) + Lazy.stringValue({ produce: () => generatePolicyName(scope, resource.logicalId) }) }); const resource = new CfnPolicy(this, 'Resource', { diff --git a/packages/@aws-cdk/aws-iam/lib/util.ts b/packages/@aws-cdk/aws-iam/lib/util.ts index 9203c0efbb6f7..cd1a29cd7d7c3 100644 --- a/packages/@aws-cdk/aws-iam/lib/util.ts +++ b/packages/@aws-cdk/aws-iam/lib/util.ts @@ -1,4 +1,4 @@ -import { Lazy } from '@aws-cdk/core'; +import { DefaultTokenResolver, IConstruct, Lazy, StringConcat, Tokenization } from '@aws-cdk/core'; import { IPolicy } from './policy'; const MAX_POLICY_NAME_LEN = 128; @@ -16,8 +16,25 @@ export function undefinedIfEmpty(f: () => string[]): string[] { * 128 characters, so we take the last 128 characters (in order to make sure the hash * is there). */ -export function generatePolicyName(logicalId: string) { - return logicalId.substring(Math.max(logicalId.length - MAX_POLICY_NAME_LEN, 0), logicalId.length); +export function generatePolicyName(scope: IConstruct, logicalId: string): string { + // as logicalId is itself a Token, resolve it first + const resolvedLogicalId = Tokenization.resolve(logicalId, { + scope, + resolver: new DefaultTokenResolver(new StringConcat()), + }); + return lastNCharacters(resolvedLogicalId, MAX_POLICY_NAME_LEN); +} + +/** + * Returns a string composed of the last n characters of str. + * If str is shorter than n, returns str. + * + * @param str the string to return the last n characters of + * @param n how many characters to return + */ +function lastNCharacters(str: string, n: number) { + const startIndex = Math.max(str.length - n, 0); + return str.substring(startIndex, str.length); } /** @@ -61,4 +78,4 @@ export function mergePrincipal(target: { [key: string]: string[] }, source: { [k } return target; -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/test/test.policy.ts b/packages/@aws-cdk/aws-iam/test/test.policy.ts index 7fe0fb17affb6..93a84a932b59d 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy.ts @@ -1,8 +1,9 @@ -import { expect } from '@aws-cdk/assert'; +import { expect, haveResourceLike } from '@aws-cdk/assert'; import { App, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; -import { Group, Policy, PolicyStatement, Role, ServicePrincipal, User } from '../lib'; -import { generatePolicyName } from '../lib/util'; +import { AnyPrincipal, CfnPolicy, Group, Policy, PolicyStatement, Role, ServicePrincipal, User } from '../lib'; + +// tslint:disable:object-literal-key-quotes export = { 'fails when policy is empty'(test: Test) { @@ -254,17 +255,29 @@ export = { test.done(); }, + "generated policy name is the same as the logical id if it's shorter than 128 characters"(test: Test) { + const stack = new Stack(); + + createPolicyWithLogicalId(stack, 'Foo'); + + expect(stack).to(haveResourceLike('AWS::IAM::Policy', { + "PolicyName": 'Foo', + })); + + test.done(); + }, + 'generated policy name only uses the last 128 characters of the logical id'(test: Test) { - test.equal(generatePolicyName('Foo'), 'Foo'); + const stack = new Stack(); - const logicalId50 = '[' + dup(50 - 2) + ']'; - test.equal(generatePolicyName(logicalId50), logicalId50); + const logicalId128 = 'a' + dup(128 - 2) + 'a'; + const logicalIdOver128 = 'PREFIX' + logicalId128; - const logicalId128 = '[' + dup(128 - 2) + ']'; - test.equal(generatePolicyName(logicalId128), logicalId128); + createPolicyWithLogicalId(stack, logicalIdOver128); - const withPrefix = 'PREFIX' + logicalId128; - test.equal(generatePolicyName(withPrefix), logicalId128, 'ensure prefix is omitted'); + expect(stack).to(haveResourceLike('AWS::IAM::Policy', { + "PolicyName": logicalId128, + })); test.done(); @@ -275,5 +288,18 @@ export = { } return r; } - } + }, }; + +function createPolicyWithLogicalId(stack: Stack, logicalId: string): void { + const policy = new Policy(stack, logicalId); + const cfnPolicy = policy.node.defaultChild as CfnPolicy; + cfnPolicy.overrideLogicalId(logicalId); // force a particular logical ID + + // add statements & principal to satisfy validation + policy.addStatements(new PolicyStatement({ + actions: ['*'], + resources: ['*'], + })); + policy.attachToRole(new Role(stack, 'Role', { assumedBy: new AnyPrincipal() })); +}