From c40ab7917b4e927b8736b2fa37e1c76fbb02a9d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Thu, 18 Oct 2018 14:13:05 +0200 Subject: [PATCH 1/6] fix(iam): Merge multiple principals correctly When adidng multiple principals, an array of principals was created, but IAM documents expect an object with different keys, for which values may be arrays. This corrects the merging process so it produces valid statements. This restores the fix for #924 that was introduced in #916 and reverted in #958 due to an other feature part of the same commit. --- .../@aws-cdk/aws-iam/lib/policy-document.ts | 36 +++++++++++++++---- .../aws-iam/test/test.policy-document.ts | 16 +++++++++ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts index dba222ab5817b..4cffb206a3aee 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts @@ -65,7 +65,7 @@ export abstract class PolicyPrincipal { */ export class PrincipalPolicyFragment { constructor( - public readonly principalJson: any, + public readonly principalJson: { [key: string]: any }, public readonly conditions: {[key: string]: any} = {}) { } } @@ -155,7 +155,7 @@ export class Anyone extends PolicyPrincipal { public readonly accountId = '*'; public policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment('*'); + return new PrincipalPolicyFragment({ AWS: this.accountId }); } } @@ -164,7 +164,7 @@ export class Anyone extends PolicyPrincipal { */ export class PolicyStatement extends Token { private action = new Array(); - private principal = new Array(); + private principal: { [key: string]: any[] } = {}; private resource = new Array(); private condition: { [key: string]: any } = { }; private effect?: PolicyStatementEffect; @@ -197,12 +197,20 @@ export class PolicyStatement extends Token { * Indicates if this permission has a "Principal" section. */ public get hasPrincipal() { - return this.principal && this.principal.length > 0; + return Object.keys(this.principal).length > 0; } public addPrincipal(principal: PolicyPrincipal): PolicyStatement { const fragment = principal.policyFragment(); - this.principal.push(fragment.principalJson); + for (const key of Object.keys(fragment.principalJson)) { + this.principal[key] = this.principal[key] || []; + const value = fragment.principalJson[key]; + if (Array.isArray(value)) { + this.principal[key].push(...value); + } else { + this.principal[key].push(value); + } + } this.addConditions(fragment.conditions); return this; } @@ -330,7 +338,7 @@ export class PolicyStatement extends Token { Action: _norm(this.action), Condition: _norm(this.condition), Effect: _norm(this.effect), - Principal: _norm(this.principal), + Principal: _normPrincipal(this.principal), Resource: _norm(this.resource), Sid: _norm(this.sid), }; @@ -361,6 +369,22 @@ export class PolicyStatement extends Token { return values; } + + function _normPrincipal(principal: { [key: string]: any[] }) { + const keys = Object.keys(principal); + if (keys.length === 0) { return undefined; } + const result: any = {}; + for (const key of keys) { + const normVal = _norm(principal[key]); + if (normVal) { + result[key] = normVal; + } + } + if (Object.keys(result).length === 1 && result.AWS === '*') { + return '*'; + } + return result; + } } } diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index 77c6036d97c2d..5888ae7636bac 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -143,6 +143,22 @@ export = { test.done(); }, + 'addAwsAccountPrincipal can be used multiple times'(test: Test) { + const p = new PolicyStatement(); + p.addAwsAccountPrincipal('1234'); + p.addAwsAccountPrincipal('5678'); + test.deepEqual(resolve(p), { + Effect: 'Allow', + Principal: { + AWS: [ + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::1234:root']] }, + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::5678:root']] } + ] + } + }); + test.done(); + }, + 'hasResource': { 'false if there are no resources'(test: Test) { test.equal(new PolicyStatement().hasResource, false, 'hasResource should be false for an empty permission'); From 096ee9e00026709e83103f7d97c6e5c58dce0690 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 23 Oct 2018 10:54:47 +0200 Subject: [PATCH 2/6] PR comments addressed --- .../@aws-cdk/aws-iam/lib/policy-document.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts index 4cffb206a3aee..530593881cb68 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts @@ -144,18 +144,9 @@ export class AccountRootPrincipal extends AccountPrincipal { /** * A principal representing all identities in all accounts */ -export class Anyone extends PolicyPrincipal { - /** - * Interface compatibility with AccountPrincipal for the purposes of the Lambda library - * - * The Lambda's addPermission() call works differently from regular - * statements, and will use the value of this property directly if present - * (which leads to the correct statement ultimately). - */ - public readonly accountId = '*'; - - public policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment({ AWS: this.accountId }); +export class Anyone extends ArnPrincipal { + constructor() { + super('*'); } } @@ -203,6 +194,9 @@ export class PolicyStatement extends Token { public addPrincipal(principal: PolicyPrincipal): PolicyStatement { const fragment = principal.policyFragment(); for (const key of Object.keys(fragment.principalJson)) { + if (Object.keys(this.principal).length > 0 && !(key in this.principal)) { + throw new Error(`Attempted to add principal key ${key} in principal of type ${Object.keys(this.principal)[0]}`); + } this.principal[key] = this.principal[key] || []; const value = fragment.principalJson[key]; if (Array.isArray(value)) { From 1a06df446268eda9a6a867dc12569016ba1c3a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 23 Oct 2018 11:19:06 +0200 Subject: [PATCH 3/6] Add test for behavior of 'AWS:*' principal --- packages/@aws-cdk/aws-iam/test/test.policy-document.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index 5888ae7636bac..711488e964441 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -1,6 +1,6 @@ import { FnConcat, resolve } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; -import { CanonicalUserPrincipal, PolicyDocument, PolicyStatement } from '../lib'; +import { CanonicalUserPrincipal, PolicyDocument, PolicyStatement, Anyone } from '../lib'; export = { 'the Permission class is a programming model for iam'(test: Test) { @@ -204,5 +204,11 @@ export = { p.addStatement(new PolicyStatement()); test.equal(p.statementCount, 2); test.done(); + }, + + 'the { AWS: "*" } principal is represented as "*"'(test: Test) { + const p = new PolicyDocument().addStatement(new PolicyStatement().addPrincipal(new Anyone())); + test.deepEqual(resolve(p), { Statement: [{ Effect: 'Allow', Principal: '*' }], Version: '2012-10-17' }); + test.done(); } }; From 537e768b08d27e8cdf225d926361bb4e3732146e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 23 Oct 2018 11:25:25 +0200 Subject: [PATCH 4/6] Add test for prventing mixing of principal types --- packages/@aws-cdk/aws-iam/test/test.policy-document.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index 711488e964441..df0a9832fdd40 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -210,5 +210,12 @@ export = { const p = new PolicyDocument().addStatement(new PolicyStatement().addPrincipal(new Anyone())); test.deepEqual(resolve(p), { Statement: [{ Effect: 'Allow', Principal: '*' }], Version: '2012-10-17' }); test.done(); + }, + + 'addPrincipal prohibits mixing principal types'(test: Test) { + const s = new PolicyStatement().addAccountRootPrincipal(); + test.throws(() => { s.addServicePrincipal('rds.amazonaws.com'); }); + test.throws(() => { s.addFederatedPrincipal('federation', { ConditionOp: { ConditionKey: 'ConditionValue' } }); }); + test.done(); } }; From dfeb706f23c4311e6e2457ed5f378b81fc78627e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 23 Oct 2018 17:11:09 +0200 Subject: [PATCH 5/6] Add error message expectations --- packages/@aws-cdk/aws-iam/test/test.policy-document.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index df0a9832fdd40..098e2a0c5820f 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -1,6 +1,6 @@ import { FnConcat, resolve } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; -import { CanonicalUserPrincipal, PolicyDocument, PolicyStatement, Anyone } from '../lib'; +import { Anyone, CanonicalUserPrincipal, PolicyDocument, PolicyStatement } from '../lib'; export = { 'the Permission class is a programming model for iam'(test: Test) { @@ -214,8 +214,10 @@ export = { 'addPrincipal prohibits mixing principal types'(test: Test) { const s = new PolicyStatement().addAccountRootPrincipal(); - test.throws(() => { s.addServicePrincipal('rds.amazonaws.com'); }); - test.throws(() => { s.addFederatedPrincipal('federation', { ConditionOp: { ConditionKey: 'ConditionValue' } }); }); + test.throws(() => { s.addServicePrincipal('rds.amazonaws.com'); }, + /Attempted to add principal key Service/); + test.throws(() => { s.addFederatedPrincipal('federation', { ConditionOp: { ConditionKey: 'ConditionValue' } }); }, + /Attempted to add principal key Federated/); test.done(); } }; From 8fc2442da88706cff08f6cdee58bc51527fa595d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 23 Oct 2018 18:32:23 +0200 Subject: [PATCH 6/6] Improve test coverage --- .../aws-iam/test/test.policy-document.ts | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index 098e2a0c5820f..2e5041afd5fcf 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -1,6 +1,6 @@ import { FnConcat, resolve } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; -import { Anyone, CanonicalUserPrincipal, PolicyDocument, PolicyStatement } from '../lib'; +import { Anyone, CanonicalUserPrincipal, PolicyDocument, PolicyPrincipal, PolicyStatement, PrincipalPolicyFragment } from '../lib'; export = { 'the Permission class is a programming model for iam'(test: Test) { @@ -219,5 +219,24 @@ export = { test.throws(() => { s.addFederatedPrincipal('federation', { ConditionOp: { ConditionKey: 'ConditionValue' } }); }, /Attempted to add principal key Federated/); test.done(); - } + }, + + 'addPrincipal correctly merges array in'(test: Test) { + const arrayPrincipal: PolicyPrincipal = { + assumeRoleAction: 'sts:AssumeRole', + policyFragment: () => new PrincipalPolicyFragment({ AWS: ['foo', 'bar'] }), + }; + const s = new PolicyStatement().addAccountRootPrincipal() + .addPrincipal(arrayPrincipal); + test.deepEqual(resolve(s), { + Effect: 'Allow', + Principal: { + AWS: [ + { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + 'foo', 'bar' + ] + } + }); + test.done(); + }, };