diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts index 711c3399b59c4..f41ef46812a92 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts @@ -189,7 +189,7 @@ export class PolicyDocument implements cdk.IResolvable { */ public _maybeMergeStatements(scope: IConstruct): void { if (this.shouldMerge(scope)) { - const result = mergeStatements(scope, this.statements, false); + const result = mergeStatements(this.statements, { scope }); this.statements.splice(0, this.statements.length, ...result.mergedStatements); } } @@ -213,11 +213,17 @@ export class PolicyDocument implements cdk.IResolvable { // Maps final statements to original statements let statementsToOriginals = new Map(this.statements.map(s => [s, [s]])); - if (this.shouldMerge(scope)) { - const result = mergeStatements(scope, this.statements, true); - this.statements.splice(0, this.statements.length, ...result.mergedStatements); - statementsToOriginals = result.originsMap; - } + + // We always run 'mergeStatements' to minimize the policy before splitting. + // However, we only 'merge' when the feature flag is on. If the flag is not + // on, we only combine statements that are *exactly* the same. We must do + // this before splitting, otherwise we may end up with the statement set [X, + // X, X, X, X] being split off into [[X, X, X], [X, X]] before being reduced + // to [[X], [X]] (but should have been just [[X]]). + const doActualMerging = this.shouldMerge(scope); + const result = mergeStatements(this.statements, { scope, limitSize: true, mergeIfCombinable: doActualMerging }); + this.statements.splice(0, this.statements.length, ...result.mergedStatements); + statementsToOriginals = result.originsMap; const sizeOptions = deriveEstimateSizeOptions(scope); diff --git a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts index b9a92d6a76d0f..42a60adbc9841 100644 --- a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts @@ -18,6 +18,32 @@ import { partitionPrincipals } from './comparable-principal'; */ const MAX_MERGE_SIZE = 2000; +/** + * Options for the mergeStatement command + */ +export interface MergeStatementOptions { + /** + * Scope to derive configuration flags from + */ + readonly scope: IConstruct; + + /** + * Do not merge statements if the result would be bigger than MAX_MERGE_SIZE + * + * @default false + */ + readonly limitSize?: boolean; + + /** + * Merge statements if they can be combined to produce the same effects. + * + * If false, statements are only merged if they are exactly equal. + * + * @default true + */ + readonly mergeIfCombinable?: boolean; +} + /** * Merge as many statements as possible to shrink the total policy doc, modifying the input array in place * @@ -26,9 +52,10 @@ const MAX_MERGE_SIZE = 2000; * Good Enough(tm). If it merges anything, it's at least going to produce a smaller output * than the input. */ -export function mergeStatements(scope: IConstruct, statements: PolicyStatement[], limitSize: boolean): MergeStatementResult { - const sizeOptions = deriveEstimateSizeOptions(scope); +export function mergeStatements(statements: PolicyStatement[], options: MergeStatementOptions): MergeStatementResult { + const sizeOptions = deriveEstimateSizeOptions(options.scope); const compStatements = statements.map(makeComparable); + const mergeFn = options?.mergeIfCombinable ?? true ? mergeIfCombinable : mergeIfEqual; // Keep trying until nothing changes anymore while (onePass()) { /* again */ } @@ -50,7 +77,7 @@ export function mergeStatements(scope: IConstruct, statements: PolicyStatement[] for (let i = 0; i < compStatements.length; i++) { let j = i + 1; while (j < compStatements.length) { - const merged = tryMerge(compStatements[i], compStatements[j], limitSize, sizeOptions); + const merged = mergeFn(compStatements[i], compStatements[j], !!options.limitSize, sizeOptions); if (merged) { compStatements[i] = merged; @@ -90,7 +117,12 @@ export interface MergeStatementResult { * - From their Action, Resource and Principal sets, 2 are subsets of each other * (empty sets are fine). */ -function tryMerge(a: ComparableStatement, b: ComparableStatement, limitSize: boolean, options: EstimateSizeOptions): ComparableStatement | undefined { +function mergeIfCombinable( + a: ComparableStatement, + b: ComparableStatement, + limitSize: boolean, + options: EstimateSizeOptions, +): ComparableStatement | undefined { // Effects must be the same if (a.statement.effect !== b.statement.effect) { return; } // We don't merge Sids (for now) @@ -128,6 +160,35 @@ function tryMerge(a: ComparableStatement, b: ComparableStatement, limitSize: boo }; } +/** + * We merge two statements only if they are exactly the same + */ +function mergeIfEqual(a: ComparableStatement, b: ComparableStatement): ComparableStatement | undefined { + if (a.statement.effect !== b.statement.effect) { return; } + if (a.statement.sid !== b.statement.sid) { return; } + if (a.conditionString !== b.conditionString) { return; } + if ( + !setEqual(a.statement.notActions, b.statement.notActions) || + !setEqual(a.statement.notResources, b.statement.notResources) || + !setEqualPrincipals(a.statement.notPrincipals, b.statement.notPrincipals) + ) { + return; + } + if ( + !setEqual(a.statement.actions, b.statement.actions) || + !setEqual(a.statement.resources, b.statement.resources) || + !setEqualPrincipals(a.statement.principals, b.statement.principals) + ) { + return; + } + + return { + originals: [...a.originals, ...b.originals], + statement: a.statement, + conditionString: a.conditionString, + }; +} + /** * Calculate and return cached string set representation of the statement elements * diff --git a/packages/@aws-cdk/aws-iam/test/role.test.ts b/packages/@aws-cdk/aws-iam/test/role.test.ts index e40a4bc8a11d7..7651fea51e403 100644 --- a/packages/@aws-cdk/aws-iam/test/role.test.ts +++ b/packages/@aws-cdk/aws-iam/test/role.test.ts @@ -721,6 +721,27 @@ describe('role with too large inline policy', () => { }); }); +test('many copies of the same statement do not result in overflow policies', () => { + const N = 100; + + const app = new App(); + const stack = new Stack(app, 'my-stack'); + const role = new Role(stack, 'MyRole', { + assumedBy: new ServicePrincipal('service.amazonaws.com'), + }); + + for (let i = 0; i < N; i++) { + role.addToPrincipalPolicy(new PolicyStatement({ + actions: ['aws:DoAThing'], + resources: ['arn:aws:service:us-east-1:111122223333:someResource/SomeSpecificResource'], + })); + } + + // THEN + const template = Template.fromStack(stack); + template.resourceCountIs('AWS::IAM::ManagedPolicy', 0); +}); + test('cross-env role ARNs include path', () => { const app = new App(); const roleStack = new Stack(app, 'role-stack', { env: { account: '123456789012', region: 'us-east-1' } });