Skip to content

Commit

Permalink
fix(iam): cannot use the same Role for multiple Config Rules (aws#12724)
Browse files Browse the repository at this point in the history
This, or any other construct that adds Managed Policies to a Role
by default; Managed Policy ARNs need to be deduplicated, otherwise
CloudFormation will throw an error upon creation.

Slightly more complicated than you'd expect in order to deal with
Tokens.

Fixes aws#12714.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Jan 27, 2021
1 parent 126a693 commit 2f6521a
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 4 deletions.
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Duration, Lazy, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core';
import { Duration, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core';
import { Construct, Node } from 'constructs';
import { Grant } from './grant';
import { CfnRole } from './iam.generated';
Expand All @@ -9,7 +9,7 @@ import { PolicyDocument } from './policy-document';
import { PolicyStatement } from './policy-statement';
import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
import { ImmutableRole } from './private/immutable-role';
import { AttachedPolicies } from './util';
import { AttachedPolicies, UniqueStringSet } from './util';

/**
* Properties for defining an IAM Role
Expand Down Expand Up @@ -326,7 +326,7 @@ export class Role extends Resource implements IRole {

const role = new CfnRole(this, 'Resource', {
assumeRolePolicyDocument: this.assumeRolePolicy as any,
managedPolicyArns: Lazy.list({ produce: () => this.managedPolicies.map(p => p.managedPolicyArn) }, { omitEmpty: true }),
managedPolicyArns: UniqueStringSet.from(() => this.managedPolicies.map(p => p.managedPolicyArn)),
policies: _flatten(this.inlinePolicies),
path: props.path,
permissionsBoundary: this.permissionsBoundary ? this.permissionsBoundary.managedPolicyArn : undefined,
Expand Down
44 changes: 43 additions & 1 deletion packages/@aws-cdk/aws-iam/lib/util.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DefaultTokenResolver, Lazy, StringConcat, Tokenization } from '@aws-cdk/core';
import { captureStackTrace, DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, Lazy, StringConcat, Token, Tokenization } from '@aws-cdk/core';
import { IConstruct } from 'constructs';
import { IPolicy } from './policy';

Expand Down Expand Up @@ -82,3 +82,45 @@ export function mergePrincipal(target: { [key: string]: string[] }, source: { [k

return target;
}

/**
* Lazy string set token that dedupes entries
*
* Needs to operate post-resolve, because the inputs could be
* `[ '${Token[TOKEN.9]}', '${Token[TOKEN.10]}', '${Token[TOKEN.20]}' ]`, which
* still all resolve to the same string value.
*
* Needs to JSON.stringify() results because strings could resolve to literal
* strings but could also resolve to `{ Fn::Join: [...] }`.
*/
export class UniqueStringSet implements IResolvable, IPostProcessor {
public static from(fn: () => string[]) {
return Token.asList(new UniqueStringSet(fn));
}

public readonly creationStack: string[];

private constructor(private readonly fn: () => string[]) {
this.creationStack = captureStackTrace();
}

public resolve(context: IResolveContext) {
context.registerPostProcessor(this);
return this.fn();
}

public postProcess(input: any, _context: IResolveContext) {
if (!Array.isArray(input)) { return input; }
if (input.length === 0) { return undefined; }

const uniq: Record<string, any> = {};
for (const el of input) {
uniq[JSON.stringify(el)] = el;
}
return Object.values(uniq);
}

public toString(): string {
return Token.asString(this);
}
}
28 changes: 28 additions & 0 deletions packages/@aws-cdk/aws-iam/test/role.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -535,3 +535,31 @@ describe('IAM role', () => {
expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
});
});

test('managed policy ARNs are deduplicated', () => {
const app = new App();
const stack = new Stack(app, 'my-stack');
const role = new Role(stack, 'MyRole', {
assumedBy: new ServicePrincipal('sns.amazonaws.com'),
managedPolicies: [
ManagedPolicy.fromAwsManagedPolicyName('SuperDeveloper'),
ManagedPolicy.fromAwsManagedPolicyName('SuperDeveloper'),
],
});
role.addManagedPolicy(ManagedPolicy.fromAwsManagedPolicyName('SuperDeveloper'));

expect(stack).toHaveResource('AWS::IAM::Role', {
ManagedPolicyArns: [
{
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/SuperDeveloper',
],
],
},
],
});
});

0 comments on commit 2f6521a

Please sign in to comment.