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

…es (#20767)

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.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

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