From e72c3538280dc2056a8097d9e4483229d0bc0f42 Mon Sep 17 00:00:00 2001
From: Rico Huijbers <rix0rrr@gmail.com>
Date: Tue, 5 May 2020 13:10:07 +0200
Subject: [PATCH] fix(iam): principal with implicit conditions overwrite each
 other

Principals can come with implicit conditions, which is useful for
representing principals that can't be expressed with a single field
in IAM.

However, you should not be allowed to use principals with mixed
conditions in a single statement, because IAM can't properly
express the implied OR there.

Fixes #3227.
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 32 ++++++++++-
 .../@aws-cdk/aws-iam/test/principals.test.ts  | 53 +++++++++++++++++++
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index ece5ae6b31a0d..24d99386a2391 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -56,6 +56,7 @@ export class PolicyStatement {
   private readonly resource = new Array<any>();
   private readonly notResource = new Array<any>();
   private readonly condition: { [key: string]: any } = { };
+  private principalConditionsJson?: string;
 
   constructor(props: PolicyStatementProps = {}) {
     // Validate actions
@@ -137,7 +138,7 @@ export class PolicyStatement {
     for (const principal of principals) {
       const fragment = principal.policyFragment;
       mergePrincipal(this.principal, fragment.principalJson);
-      this.addConditions(fragment.conditions);
+      this.addPrincipalConditions(fragment.conditions);
     }
   }
 
@@ -156,7 +157,7 @@ export class PolicyStatement {
     for (const notPrincipal of notPrincipals) {
       const fragment = notPrincipal.policyFragment;
       mergePrincipal(this.notPrincipal, fragment.principalJson);
-      this.addConditions(fragment.conditions);
+      this.addPrincipalConditions(fragment.conditions);
     }
   }
 
@@ -380,6 +381,33 @@ export class PolicyStatement {
   public toJSON() {
     return this.toStatementJson();
   }
+
+  /**
+   * Add a principal's conditions
+   *
+   * For convenience, principals have been modeled as both a principal
+   * and a set of conditions. This makes it possible to have a single
+   * object represent e.g. an "SNS Topic" (SNS service principal + aws:SourcArn
+   * condition) or an Organization member (* + aws:OrgId condition).
+   *
+   * However, when using multiple principals in the same policy statement,
+   * they must all have the same conditions or the OR samentics
+   * implied by a list of principals cannot be guaranteed (user needs to
+   * add multiple statements in that case).
+   */
+  private addPrincipalConditions(conditions: Conditions) {
+    // Stringifying the conditions is an easy way to do deep equality
+    const theseConditions = JSON.stringify(conditions);
+    if (this.principalConditionsJson === undefined) {
+      // First principal, anything goes
+      this.principalConditionsJson = theseConditions;
+    } else {
+      if (this.principalConditionsJson !== theseConditions) {
+        throw new Error(`All principals in a PolicyStatement must have the same Conditions (got '${this.principalConditionsJson}' and '${theseConditions}'). Use multiple statements instead.`);
+      }
+    }
+    this.addConditions(conditions);
+  }
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-iam/test/principals.test.ts b/packages/@aws-cdk/aws-iam/test/principals.test.ts
index 0f4d398eea44f..a78d31bf9cea9 100644
--- a/packages/@aws-cdk/aws-iam/test/principals.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/principals.test.ts
@@ -49,3 +49,56 @@ test('use of cross-stack role reference does not lead to URLSuffix being exporte
   },
   );
 });
+
+test('cannot have multiple principals with different conditions in the same statement', () => {
+  const stack = new Stack(undefined, 'First');
+  const user = new iam.User(stack, 'User');
+
+  expect(() => {
+    user.addToPolicy(new iam.PolicyStatement({
+      principals: [
+        new iam.ServicePrincipal('myService.amazon.com', {
+          conditions: {
+            StringEquals: {
+              hairColor: 'blond',
+            },
+          },
+        }),
+        new iam.ServicePrincipal('yourservice.amazon.com', {
+          conditions: {
+            StringEquals: {
+              hairColor: 'black',
+            },
+          },
+        }),
+      ],
+    }));
+  }).toThrow(/All principals in a PolicyStatement must have the same Conditions/);
+});
+
+test('can have multiple principals the same conditions in the same statement', () => {
+  const stack = new Stack(undefined, 'First');
+  const user = new iam.User(stack, 'User');
+
+  user.addToPolicy(new iam.PolicyStatement({
+    principals: [
+      new iam.ServicePrincipal('myService.amazon.com'),
+      new iam.ServicePrincipal('yourservice.amazon.com'),
+    ],
+  }));
+
+  user.addToPolicy(new iam.PolicyStatement({
+    principals: [
+      new iam.ServicePrincipal('myService.amazon.com', {
+        conditions: {
+          StringEquals: { hairColor: 'blond' },
+        },
+      }),
+      new iam.ServicePrincipal('yourservice.amazon.com', {
+        conditions: {
+          StringEquals: { hairColor: 'blond' },
+        },
+      }),
+    ],
+  }));
+});
\ No newline at end of file