From 29974336c8efc7d222c492ad2ff285f9a2877c78 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jun 2022 10:45:06 -0700 Subject: [PATCH] fix(iam): duplicate PolicyStatements lead to too many overflow policies Historically, policy statement deduplicating was done during policy rendering. There could be a 100 statements in a policy, but if they were all the same it would only render as 1 statement. When we introduced policy splitting, those 100 equal statements would *first* be divided into Overflow policies before being deduplicated (potentially leading to too many policies being created, hitting limits). Especially CDK Pipelines takes heavy advantage of the deduplicating behavior, and adds the same policies over and over again, leading to overflows. When the `minimizePolicies` feature flag is turned on, duplicate elimitation is implicitly performed (because equal statements count as mergeable), but when the feature flag is off we hit the bad behavior. As a fix, do deduplication even if merging is turned off. --- .../@aws-cdk/aws-iam/lib/policy-document.ts | 18 +++-- .../aws-iam/lib/private/merge-statements.ts | 69 +++++++++++++++++-- packages/@aws-cdk/aws-iam/test/role.test.ts | 21 ++++++ 3 files changed, 98 insertions(+), 10 deletions(-) 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' } });