Skip to content

Commit

Permalink
fix(iam): can't use OrganizationPrincipal for assuming Role
Browse files Browse the repository at this point in the history
`Principal: "*"` supposedly works to allow any Principal to assume
a Role (restricted by `Conditions`, of course), but doesn't work in
practice. The IAM API rejects it as a MalformedPolicyDocument.

In order to not generate a large diff on existing policies, disable
simplification of `Principal: { AWS: * }` to `Principal: *` only
for AssumeRole policy documents.

Fixes #5732.
  • Loading branch information
rix0rrr committed Jan 10, 2020
1 parent 01220cf commit b7f36a9
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 69 deletions.
78 changes: 11 additions & 67 deletions packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -1,6 +1,7 @@
import * as cdk from '@aws-cdk/core';
import { AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, CanonicalUserPrincipal,
FederatedPrincipal, IPrincipal, ServicePrincipal, ServicePrincipalOpts } from './principals';
import { normalizePolicyField, normalizePolicyPrincipal, noUndefined } from './private/policy-language';
import { mergePrincipal } from './util';

/**
Expand Down Expand Up @@ -187,64 +188,17 @@ export class PolicyStatement {
}

public toStatementJson(): any {
return noUndef({
Action: _norm(this.action),
NotAction: _norm(this.notAction),
Condition: _norm(this.condition),
Effect: _norm(this.effect),
Principal: _normPrincipal(this.principal),
NotPrincipal: _normPrincipal(this.notPrincipal),
Resource: _norm(this.resource),
NotResource: _norm(this.notResource),
Sid: _norm(this.sid),
return noUndefined({
Action: normalizePolicyField(this.action),
NotAction: normalizePolicyField(this.notAction),
Condition: normalizePolicyField(this.condition),
Effect: normalizePolicyField(this.effect),
Principal: normalizePolicyPrincipal(this.principal),
NotPrincipal: normalizePolicyPrincipal(this.notPrincipal),
Resource: normalizePolicyField(this.resource),
NotResource: normalizePolicyField(this.notResource),
Sid: normalizePolicyField(this.sid),
});

function _norm(values: any) {

if (typeof(values) === 'undefined') {
return undefined;
}

if (cdk.Token.isUnresolved(values)) {
return values;
}

if (Array.isArray(values)) {
if (!values || values.length === 0) {
return undefined;
}

if (values.length === 1) {
return values[0];
}

return values;
}

if (typeof(values) === 'object') {
if (Object.keys(values).length === 0) {
return undefined;
}
}

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

public toString() {
Expand Down Expand Up @@ -328,13 +282,3 @@ export interface PolicyStatementProps {
*/
readonly effect?: Effect;
}

function noUndef(x: any): any {
const ret: any = {};
for (const [key, value] of Object.entries(x)) {
if (value !== undefined) {
ret[key] = value;
}
}
return ret;
}
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-iam/test/integ.role.expected.json
Expand Up @@ -95,6 +95,28 @@
"Version": "2012-10-17"
}
}
},
"TestRole3C1F30727": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Condition": {
"StringEquals": {
"aws:PrincipalOrgID": "o-1234"
}
},
"Effect": "Allow",
"Principal": {
"AWS": "*"
}
}
],
"Version": "2012-10-17"
}
}
}
}
}
7 changes: 6 additions & 1 deletion packages/@aws-cdk/aws-iam/test/integ.role.ts
@@ -1,5 +1,5 @@
import { App, Stack } from "@aws-cdk/core";
import { AccountRootPrincipal, Policy, PolicyStatement, Role, ServicePrincipal } from "../lib";
import { AccountRootPrincipal, OrganizationPrincipal, Policy, PolicyStatement, Role, ServicePrincipal } from "../lib";

const app = new App();

Expand All @@ -21,4 +21,9 @@ new Role(stack, 'TestRole2', {
externalIds: ['supply-me'],
});

// Role with an org
new Role(stack, 'TestRole3', {
assumedBy: new OrganizationPrincipal('o-1234'),
});

app.synth();
28 changes: 27 additions & 1 deletion packages/@aws-cdk/aws-iam/test/role.test.ts
@@ -1,6 +1,6 @@
import '@aws-cdk/assert/jest';
import { Duration, Stack } from '@aws-cdk/core';
import { ArnPrincipal, CompositePrincipal, FederatedPrincipal, ManagedPolicy, PolicyStatement, Role, ServicePrincipal, User } from '../lib';
import { AnyPrincipal, ArnPrincipal, CompositePrincipal, FederatedPrincipal, ManagedPolicy, PolicyStatement, Role, ServicePrincipal, User } from '../lib';

describe('IAM role', () => {
test('default role', () => {
Expand Down Expand Up @@ -319,4 +319,30 @@ describe('IAM role', () => {
}
});
});

test('Principal-* in an AssumeRolePolicyDocument gets translated to { "AWS": "*" }', () => {
// The docs say that "Principal: *" and "Principal: { AWS: * }" are equivalent
// (https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html)
// but in practice CreateRole errors out if you use "Principal: *" in an AssumeRolePolicyDocument:
// An error occurred (MalformedPolicyDocument) when calling the CreateRole operation: AssumeRolepolicy contained an invalid principal: "STAR":"*".

// Make sure that we handle this case specially.
const stack = new Stack();
new Role(stack, 'Role', {
assumedBy: new AnyPrincipal(),
});

expect(stack).toHaveResource('AWS::IAM::Role', {
AssumeRolePolicyDocument: {
Statement: [
{
Action: "sts:AssumeRole",
Effect: "Allow",
Principal: { AWS: "*" },
}
],
Version: "2012-10-17"
}
});
});
});

0 comments on commit b7f36a9

Please sign in to comment.