Skip to content

Commit

Permalink
fix(iam): principal with implicit conditions overwrite each other
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rix0rrr authored May 5, 2020
1 parent 668a6d7 commit e72c353
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 2 deletions.
32 changes: 30 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/policy-statement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}

/**
Expand Down
53 changes: 53 additions & 0 deletions packages/@aws-cdk/aws-iam/test/principals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
},
}),
],
}));
});

0 comments on commit e72c353

Please sign in to comment.