fix(iam): IAM Policies are too large to deploy

packages/@aws-cdk/aws-iam/docs/policy-merging.als

@@ -0,0 +1,116 @@
+/*
+Alloy model to confirm the logic behind merging IAM Statements.
+
+This proves that merging two statements based on the following conditions:
+
+- Effects are the same, and one of:
+  - Both have some Actions and the rest of the statements is the same
+  - Both have some Resources and the rest of the statements is the same
+  - Both have some Principals and the rest of the statements is the same
+
+Is sound, as the model doesn't find any examples of where the meaning
+of statements is changed by merging.
+
+Find Alloy at https://alloytools.org/.
+*/
+
+---------------------------------------------------------
+-- Base Statement definitions
+enum Effect { Allow, Deny }
+enum Resource { ResourceA, ResourceB }
+enum Action { ActionA, ActionB }
+enum Principal { PrincipalA, PrincipalB }
+
+sig Statement {
+  effect: Effect,
+  principal: set Principal,
+  notPrincipal: set Principal,
+  action: set Action,
+  notAction: set Action,
+  resource: set Resource,
+  notResource: set Resource,
+} {
+  // Exactly one of Xxx and notXxx is non-empty
+  (some principal) iff not (some notPrincipal)
+  (some action) iff not (some notAction)
+  (some resource) iff not (some notResource)
+}
+
+---------------------------------------------------------
+-- Requests and evaluations
+sig Request {
+  principal: Principal,
+  resource: Resource,
+  action: Action,
+}
+
+// Whether the statement applies to the given request
+pred applies[s: Statement, req: Request] {
+  req.principal in s.principal or req.principal not in s.notPrincipal
+  req.action in s.action or req.action not in s.notAction
+  req.resource in s.resource or req.resource not in s.notResource
+}
+
+// Whether or not to allow the given request according to the given statements
+//
+// A request is allowed if there's at least one statement allowing it and
+// no statements denying it.
+pred allow[req: Request, ss: some Statement] {
+  some s: ss | applies[s, req] and s.effect = Allow
+  no s: ss | applies[s, req] and s.effect = Deny
+}
+
+---------------------------------------------------------
+-- Statement merging
+
+// Helpers to assert that certain fields are the same
+let sameAction[a, b] = { a.action = b.action and a.notAction = b.notAction }
+let sameResource[a, b] = { a.resource = b.resource and a.notResource = b.notResource }
+let samePrincipal[a, b] = { a.principal = b.principal and a.notPrincipal = b.notPrincipal }
+
+// Assert that m is the merged version of a and b
+//
+// This encodes the important logic: the rules of merging.
+pred merged[a: Statement, b: Statement, m: Statement] {
+  m.effect = a.effect and m.effect = b.effect 
+  {
+    // Writing 'some a.action', 'some b.action' here is critical
+    // This asserts we are dealing with a POSITIVE action and not
+    // a notAction (that's excluded by the fact on Statement that only
+    // one of the two can be set.
+    //
+    // We can show that we have a bug if you uncomment the next line.
+    some a.action and some b.action // Excludes notAction
+
+    // The constraint on notAction here is not necessary, because
+    // in practice it must always the empty set, but it is necessary to
+    // correctly show the bug in action if the previous line is commented out.
+    m.action = a.action + b.action and m.notAction = a.notAction + b.notAction
+    sameResource[a, b] and sameResource[b, m]
+    samePrincipal[a, b] and samePrincipal[b, m]
+  } or {
+    some a.resource and some b.resource // Excludes notResource
+    m.resource = a.resource + b.resource and m.notResource = a.notResource + b.notResource
+    sameAction[a, b] and sameAction[b, m]
+    samePrincipal[a, b] and samePrincipal[b, m]
+  } or {
+    some a.principal and some b.principal // Excludes notPrincipal
+    m.principal = a.principal + b.principal and m.notPrincipal = a.notPrincipal + b.notPrincipal
+    sameAction[a, b] and sameAction[b, m]
+    sameResource[a, b] and sameResource[b, m]
+  }
+}
+
+// Maximum merging of all statements in orig
+pred maxMerged[orig: some Statement, merged: some Statement] {
+}
+
+// There are no statements so that if they are merged, 
+// the evaluation of the separate statements is different than the evaluation
+// of the merged statement
+check merging_does_not_change_evaluation {
+  no a: Statement, b: Statement, m: Statement, r: Request {
+    merged[a, b, m]
+    allow[r, a + b] iff not allow[r, m]
+  }
+} for 5

packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts

@@ -1,25 +1,12 @@
 const LENGTH_CACHE_SYM = Symbol();
 
-type IamValue = unknown | unknown[];
-
-/**
- * Only the parts of the policy schema we're interested in
- */
-interface StatementSchema {
-  readonly Sid?: string;
-  readonly Effect?: string;
-  readonly Principal?: IamValue | Record<string, IamValue>;
-  readonly Resource?: IamValue;
-  readonly Action?: IamValue;
-}
-
 /**
  * Merge as many statements as possible to shrink the total policy doc, modifying the input array in place
  */
 export function mergeStatements(statements: StatementSchema[]): StatementSchema[] {
   let merges = findMerges(statements);
   while (merges.length > 0) {
-    merges.sort((a, b) => a.sizeDelta - b.sizeDelta); // Most negative number first
+    merges.sort((a, b) => a.sizeDelta - b.sizeDelta); // Biggest reduction first
     if (merges[0].sizeDelta >= 0) { break; } // Nothing more to be gained
 
     // Apply all merges, order biggest to smallest gain
@@ -28,7 +15,12 @@ export function mergeStatements(statements: StatementSchema[]): StatementSchema[
       statements[merge.i0] = merge.combined;
       statements.splice(merge.i1, 1);
 
-      // This invalidates all merges involving i0 or i1, and adjusts all indices > i1
+      // Optimization: we can still merges that are for other pairs in this same
+      // go-around (which saves us recalculating all merge pairs again). We will
+      // need to adjust indices because we just changed the input array.
+      //
+      // The following removes all merges involving i0 or i1 (no longer valid,
+      // these have been used up), and shifts all indices > i1 left by 1.
       // Assumes i0 < i1.
       const invalidIndices = [merge.i0, merge.i1];
       let j = 0;
@@ -48,6 +40,9 @@ export function mergeStatements(statements: StatementSchema[]): StatementSchema[
   return statements;
 }
 
+/**
+ * Return all possible merges between all pairs of statements in the input array
+ */
 function findMerges(statements: StatementSchema[]): StatementMerge[] {
   const ret = new Array<StatementMerge>();
   for (let i0 = 0; i0 < statements.length; i0++) {
@@ -62,7 +57,9 @@ function tryMerge(statements: StatementSchema[], i0: number, i1: number, into: S
   const a = statements[i0];
   const b = statements[i1];
 
-  if (a.Effect !== 'Allow' || b.Effect !== 'Allow') { return; }
+  // Effects must be the same
+  if (a.Effect !== b.Effect) { return; }
+  // We don't merge Sids (for now)
   if (a.Sid || b.Sid) { return; }
 
   const beforeLen = jsonLength(a) + jsonLength(b);
@@ -175,6 +172,19 @@ function normalizedArray(...xs: unknown[]) {
   return Array.from(new Set(xs)).sort();
 }
 
+type IamValue = unknown | unknown[];
+
+/**
+ * Only the parts of the policy schema we're interested in
+ */
+interface StatementSchema {
+  readonly Sid?: string;
+  readonly Effect?: string;
+  readonly Principal?: IamValue | Record<string, IamValue>;
+  readonly Resource?: IamValue;
+  readonly Action?: IamValue;
+}
+
 interface StatementMerge {
   /**
    * Index of statement 0

packages/@aws-cdk/aws-iam/test/merge-statements.test.ts

@@ -7,23 +7,6 @@ const principal = new iam.ArnPrincipal(PRINCIPAL_ARN);
 const PRINCIPAL_ARN2 = 'arn:aws:iam::111111111:role/role-name';
 const principal2 = new iam.ArnPrincipal(PRINCIPAL_ARN2);
 
-test("don't merge Deny statements", () => {
-  assertNoMerge([
-    new iam.PolicyStatement({
-      effect: iam.Effect.DENY,
-      resources: ['a'],
-      actions: ['service:Action'],
-      principals: [principal],
-    }),
-    new iam.PolicyStatement({
-      effect: iam.Effect.DENY,
-      resources: ['b'],
-      actions: ['service:Action'],
-      principals: [principal],
-    }),
-  ]);
-});
-
 test.each([
   ['resources', true],
   ['notResources', false],
@@ -199,6 +182,30 @@ test('if conditions are the smae, statements are merged', () => {
   ]);
 });
 
+test('also merge Deny statements', () => {
+  assertMerged([
+    new iam.PolicyStatement({
+      effect: iam.Effect.DENY,
+      resources: ['a'],
+      actions: ['service:Action'],
+      principals: [principal],
+    }),
+    new iam.PolicyStatement({
+      effect: iam.Effect.DENY,
+      resources: ['b'],
+      actions: ['service:Action'],
+      principals: [principal],
+    }),
+  ], [
+    {
+      Effect: 'Deny',
+      Resource: ['a', 'b'],
+      Action: 'service:Action',
+      Principal: { AWS: PRINCIPAL_ARN },
+    },
+  ]);
+});
+
 test('merges 3 statements in multiple steps', () => {
   assertMerged([
     new iam.PolicyStatement({
@@ -227,6 +234,45 @@ test('merges 3 statements in multiple steps', () => {
   ]);
 });
 
+test('merges 2 pairs separately', () => {
+  // Merges pairs (0,2) and (1,3)
+  assertMerged([
+    new iam.PolicyStatement({
+      resources: ['a'],
+      actions: ['service:Action'],
+      principals: [principal],
+    }),
+    new iam.PolicyStatement({
+      resources: ['c'],
+      actions: ['service:Action1'],
+      principals: [principal],
+    }),
+    new iam.PolicyStatement({
+      resources: ['b'],
+      actions: ['service:Action'],
+      principals: [principal],
+    }),
+    new iam.PolicyStatement({
+      resources: ['c'],
+      actions: ['service:Action2'],
+      principals: [principal],
+    }),
+  ], [
+    {
+      Effect: 'Allow',
+      Resource: ['a', 'b'],
+      Action: 'service:Action',
+      Principal: { AWS: PRINCIPAL_ARN },
+    },
+    {
+      Effect: 'Allow',
+      Resource: 'c',
+      Action: ['service:Action1', 'service:Action2'],
+      Principal: { AWS: PRINCIPAL_ARN },
+    },
+  ]);
+});
+
 test('do not deep-merge info Refs and GetAtts', () => {
   const stack = new Stack();
   const user1 = new iam.User(stack, 'User1');