fix(iam): duplicate PolicyStatements lead to too many overflow policies

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);
 

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
  *

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' } });