Skip to content

Commit

Permalink
fix(iam): Groups are erroneously accepted as the Principal of a policy (
Browse files Browse the repository at this point in the history
aws#11479) (aws#12549)

closes aws#11479
----
Hello, picking this up as my first issue to learn about how CDK works:

[AWS mentions](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html) that iam groups can't be principals in policies. If a developer doesn't know this, they get an error during deployment. This change will throw an error to help them catch the issue during synthesis time.

Note that this doesn't handle the code path where an arn string is specified as the principal, but the method already documents that a group would be invalid.

Let me know if there's a cleaner way to fix this.
  • Loading branch information
swar8080 authored and Mohan Rajendran committed Jan 24, 2021
1 parent ed6777b commit 7461d16
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 1 deletion.
9 changes: 9 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/policy-statement.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as cdk from '@aws-cdk/core';
import { Group } from './group';
import {
AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, CanonicalUserPrincipal,
FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts,
Expand Down Expand Up @@ -138,6 +139,7 @@ export class PolicyStatement {
throw new Error('Cannot add \'Principals\' to policy statement if \'NotPrincipals\' have been added');
}
for (const principal of principals) {
this.validatePolicyPrincipal(principal);
const fragment = principal.policyFragment;
mergePrincipal(this.principal, fragment.principalJson);
this.addPrincipalConditions(fragment.conditions);
Expand All @@ -157,12 +159,19 @@ export class PolicyStatement {
throw new Error('Cannot add \'NotPrincipals\' to policy statement if \'Principals\' have been added');
}
for (const notPrincipal of notPrincipals) {
this.validatePolicyPrincipal(notPrincipal);
const fragment = notPrincipal.policyFragment;
mergePrincipal(this.notPrincipal, fragment.principalJson);
this.addPrincipalConditions(fragment.conditions);
}
}

private validatePolicyPrincipal(principal: IPrincipal) {
if (principal instanceof Group) {
throw new Error('Cannot use an IAM Group as the \'Principal\' or \'NotPrincipal\' in an IAM Policy');
}
}

/**
* Specify AWS account ID as the principal entity to the "Principal" section of a policy statement.
*/
Expand Down
12 changes: 11 additions & 1 deletion packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import '@aws-cdk/assert/jest';
import { Stack } from '@aws-cdk/core';
import { AnyPrincipal, PolicyDocument, PolicyStatement } from '../lib';
import { AnyPrincipal, Group, PolicyDocument, PolicyStatement } from '../lib';

describe('IAM policy statement', () => {

Expand Down Expand Up @@ -180,4 +180,14 @@ describe('IAM policy statement', () => {
});
});

test('throws error when group is specified for \'Principal\' or \'NotPrincipal\'', () => {
const stack = new Stack();
const group = new Group(stack, 'groupId');
const policyStatement = new PolicyStatement();

expect(() => policyStatement.addPrincipals(group))
.toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/);
expect(() => policyStatement.addNotPrincipals(group))
.toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/);
});
});

0 comments on commit 7461d16

Please sign in to comment.