Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(iam): Validate the policy statements when synthesizing policy #7897

Closed
wants to merge 9 commits into from
Closed

feat(iam): Validate the policy statements when synthesizing policy #7897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7647d98
Validate the policy statements
shreyasdamle May 10, 2020
ae838a1
Fix PR linter error
shreyasdamle May 10, 2020
e4470b2
Add runtime check to validate PolicyStatementProps
May 19, 2020
507f493
Fix policy statement in aws-logs for 'add policy to destination' test
May 19, 2020
8befbe9
Fix policy statement in aws-sns for unit test
May 19, 2020
8ba9a3b
Fix policy statement in aws-lambda unit test
May 19, 2020
d0081f9
Fix policy statement in CF web-distributions
May 19, 2020
4e743ed
Fix policy statement in CF tests
May 19, 2020
f37637b
Merge branch 'master' into feat-7615
shreyasdamle May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@ export class CloudFrontWebDistribution extends cdk.Construct implements IDistrib
if (a.lambdaFunction.role && a.lambdaFunction.role instanceof iam.Role && a.lambdaFunction.role.assumeRolePolicy) {
a.lambdaFunction.role.assumeRolePolicy.addStatements(new iam.PolicyStatement({
actions: [ 'sts:AssumeRole' ],
resources: ['*'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah as you mentioned yourself, this is incorrect. This * doesn't go here.

principals: [ new iam.ServicePrincipal('edgelambda.amazonaws.com') ],
}));
}
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-cloudfront/test/test.basic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,7 @@ export = {
'Principal': {
'Service': 'edgelambda.amazonaws.com',
},
'Resource': '*',
},
],
'Version': '2012-10-17',
Expand Down
12 changes: 12 additions & 0 deletions packages/@aws-cdk/aws-iam/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,18 @@ const newPolicyDocument = PolicyDocument.fromJson(policyDocument);

```

### Policy Statement

As per the doc of [IAM policy grammar](https://docs.aws.amazon.com/en_us/IAM/latest/UserGuide/reference_policies_grammar.html#policies-grammar-bnf), the action and resource blocks are mandatory. Hence, the new policy stament using `PolicyStatementProps` can be created as:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this README section is helping a lot. What kind of information do you think people are missing?

Or did you write this section just to appease the linter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just added it to appease the linter. Is there any better way to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, yes, we can add a tag to silence it.


```ts
const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' });
policy.addStatements(new PolicyStatement({
resources: ['*'],
actions: ['sqs:SendMessage']
}));
```

### OpenID Connect Providers

OIDC identity providers are entities in IAM that describe an external identity
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/policy-statement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export class PolicyStatement {
if (props.conditions !== undefined) {
this.addConditions(props.conditions);
}
this.validateProps(props);
}

//
Expand Down Expand Up @@ -408,6 +409,27 @@ export class PolicyStatement {
}
this.addConditions(conditions);
}

/**
* Validate PolicyStatementProps
*
* As per [IAM policy grammar](https://docs.aws.amazon.com/en_us/IAM/latest/UserGuide/reference_policies_grammar.html#policies-grammar-bnf),
* the action and resource blocks are mandatory.
*
* @param props
*/
private validateProps(props: PolicyStatementProps) {
if (props.conditions || props.effect || props.notPrincipals || props.notResources || props.principals || props.resources || props.sid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get this additional guarding if. Is action always required, or not? Where are you running into trouble if you don't add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional if is to guard against PolicyStatement class instantiated without any props.

const p = new PolicyStatement();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the old usage pattern. Good catch. Could you rewrite it a little though to make that more self-evident for the future?

Maybe to the following:

const anyConstructorParamsSupplied = Object.values(props).filter(x => x !== undefined).length > 0;

if (anyConstructorParamsSupplied && !(props.actions || props.notActions)) {
  throw new Error('...');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey but while writing that, that reminds me...

We can force either a full policy statement in the constructor, or an empty policy statement in the constructor and then using the fluent API... but why would we prevent a mix?

I think it's actually a breaking change to do that.

People could have written:

const s = new PolicyStatement({ resources: ['*'] });
s.addPrincipal(...):

And this change would break them.

The only place where we can do proper validation is at the end, when we are rendering the policy statement out.

if (!props.actions && !props.notActions) {
throw new Error('Action block is mandatory. Either `actions` or `notActions` prop must be specified');
}
}
if (props.actions || props.conditions || props.effect || props.notActions || props.notPrincipals || props.principals || props.sid) {
if (!props.resources && !props.notResources) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have a question on IAM policy grammar. It says the action and resource blocks are mandatory. However, for certain policies (example: trust relationship policy) resource block isn't allowed. Here's the instance of such case:

 for (const a of input.lambdaFunctionAssociations) {
        if (a.lambdaFunction.role && a.lambdaFunction.role instanceof iam.Role && a.lambdaFunction.role.assumeRolePolicy) {
          a.lambdaFunction.role.assumeRolePolicy.addStatements(new iam.PolicyStatement({
            actions: [ 'sts:AssumeRole' ],
            principals: [ new iam.ServicePrincipal('edgelambda.amazonaws.com') ],
          }));
        }
      }

Please let me know if I'm missing something. I will move this PR to draft in the meantime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs aren't super clear about this, but there is a distinction between:

  • Identity policies (what permissions an actor is allowed to perform, generally called IAM policies)
  • Resource policies (what actions does the resource allow to be performed TO IT, you can think of these as "trust" policies although that is not a name that is in general use).

The syntax between these two is quite similar, except:

  • An identity policy generally does not need to indicate the principal, as it is always about this principal.
  • A resource policy generally does not need to indicate the resource, as it is always about this resource (of course there are exceptions: some resources like Queues require a *, and some resources like Buckets allow you to use ARNs to identify objects in the bucket).

Now here's the catch:

An AssumeRolePolicy is actually a resource policy on an identity object.

You can see that that's true, because the AssumeRolePolicy defines what actions are allowed to performed on the role (very specifically, who is allowed to Assume it).

And so therefore, it does not take a Resource. The resource is implicitly: this role.

throw new Error('Resource block is mandatory. Either `resources` or `notResources` prop must be specified');
}
}
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-iam/test/managed-policy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ describe('managed policy', () => {
const mp = new ManagedPolicy(stack, 'Policy');
mp.addStatements(new PolicyStatement({
actions: ['a:abc'],
resources: ['*'],
}));

expect(stack.resolve(mp.managedPolicyName)).toEqual({
Expand All @@ -553,6 +554,7 @@ describe('managed policy', () => {
});
mp.addStatements(new PolicyStatement({
actions: ['a:abc'],
resources: ['*'],
}));

const stack2 = new cdk.Stack(app, 'Stack2', { env: { account: '5678', region: 'us-east-1' }});
Expand Down
52 changes: 45 additions & 7 deletions packages/@aws-cdk/aws-iam/test/policy-document.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ describe('IAM policy document', () => {
new PolicyStatement({
actions: ['abc:def'],
notActions: ['abc:def'],
resources: ['*'],
});
}).toThrow(/Cannot add 'NotActions' to policy statement if 'Actions' have been added/);
});
Expand All @@ -83,6 +84,7 @@ describe('IAM policy document', () => {
expect(() => {
new PolicyStatement({
actions: ['service:action', '*', 'service:acti*', 'in:val:id'],
resources: ['*'],
});
}).toThrow(/Action 'in:val:id' is invalid/);
});
Expand All @@ -91,13 +93,32 @@ describe('IAM policy document', () => {
expect(() => {
new PolicyStatement({
notActions: ['service:action', '*', 'service:acti*', 'in:val:id'],
resources: ['*'],
});
}).toThrow(/Action 'in:val:id' is invalid/);
});

test('Throws with actions and notActions both undefined', () => {
expect(() => {
new PolicyStatement({
principals: [new CanonicalUserPrincipal('abc')],
resources: ['*'],
});
}).toThrow(/Action block is mandatory. Either `actions` or `notActions` prop must be specified/);
});

test('Throws with resources and notResources both undefined', () => {
expect(() => {
new PolicyStatement({
actions: ['abc:def'],
});
}).toThrow(/Resource block is mandatory. Either `resources` or `notResources` prop must be specified/);
});

test('Cannot combine Resources and NotResources', () => {
expect(() => {
new PolicyStatement({
actions: ['abc:def'],
resources: ['abc'],
notResources: ['def'],
});
Expand All @@ -107,6 +128,8 @@ describe('IAM policy document', () => {
test('Cannot add NotPrincipals when Principals exist', () => {
const stmt = new PolicyStatement({
principals: [new CanonicalUserPrincipal('abc')],
actions: ['abc:def'],
resources: ['*'],
});
expect(() => {
stmt.addNotPrincipals(new CanonicalUserPrincipal('def'));
Expand All @@ -116,12 +139,27 @@ describe('IAM policy document', () => {
test('Cannot add Principals when NotPrincipals exist', () => {
const stmt = new PolicyStatement({
notPrincipals: [new CanonicalUserPrincipal('abc')],
actions: ['abc:def'],
resources: ['*'],
});
expect(() => {
stmt.addPrincipals(new CanonicalUserPrincipal('def'));
}).toThrow(/Cannot add 'Principals' to policy statement if 'NotPrincipals' have been added/);
});

test('combine NotActions and NotResources', () => {
const stack = new Stack();
const statement = new PolicyStatement({
notActions: ['abc:def'],
notResources: ['def'],
});
expect(stack.resolve(statement.toStatementJson())).toEqual({
Effect: 'Allow',
NotAction: 'abc:def',
NotResource: 'def',
});
});

test('Permission allows specifying multiple actions upon construction', () => {
const stack = new Stack();
const perm = new PolicyStatement();
Expand Down Expand Up @@ -215,7 +253,7 @@ describe('IAM policy document', () => {
});

test('true if there is one resource', () => {
expect(new PolicyStatement({ resources: ['one-resource'] }).hasResource).toEqual(true);
expect(new PolicyStatement({ resources: ['one-resource'], actions: ['abc:def'] }).hasResource).toEqual(true);
});

test('true for multiple resources', () => {
Expand Down Expand Up @@ -247,9 +285,9 @@ describe('IAM policy document', () => {
test('statementCount returns the number of statement in the policy document', () => {
const p = new PolicyDocument();
expect(p.statementCount).toEqual(0);
p.addStatements(new PolicyStatement({ actions: ['service:action1'] }));
p.addStatements(new PolicyStatement({ actions: ['service:action1'], resources: ['*'] }));
expect(p.statementCount).toEqual(1);
p.addStatements(new PolicyStatement({ actions: ['service:action2'] }));
p.addStatements(new PolicyStatement({ actions: ['service:action2'], resources: ['*'] }));
expect(p.statementCount).toEqual(2);
});

Expand All @@ -258,11 +296,11 @@ describe('IAM policy document', () => {
const stack = new Stack();
const p = new PolicyDocument();

p.addStatements(new PolicyStatement({ principals: [new Anyone()] }));
p.addStatements(new PolicyStatement({ principals: [new Anyone()], actions: ['abc:def'], resources: ['*'] }));

expect(stack.resolve(p)).toEqual({
Statement: [
{ Effect: 'Allow', Principal: '*' },
{ Action: 'abc:def', Effect: 'Allow', Principal: '*', Resource: '*' },
],
Version: '2012-10-17',
});
Expand All @@ -272,11 +310,11 @@ describe('IAM policy document', () => {
const stack = new Stack();
const p = new PolicyDocument();

p.addStatements(new PolicyStatement({ principals: [new AnyPrincipal()] }));
p.addStatements(new PolicyStatement({ principals: [new AnyPrincipal()], actions: ['abc:def'], resources: ['*']}));

expect(stack.resolve(p)).toEqual({
Statement: [
{ Effect: 'Allow', Principal: '*' },
{ Action: 'abc:def', Effect: 'Allow', Principal: '*', Resource: '*' },
],
Version: '2012-10-17',
});
Expand Down
7 changes: 6 additions & 1 deletion packages/@aws-cdk/aws-iam/test/principals.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ test('cannot have multiple principals with different conditions in the same stat
},
}),
],
actions: ['abc:def'],
resources: ['*'],
}));
}).toThrow(/All principals in a PolicyStatement must have the same Conditions/);
});
Expand All @@ -85,6 +87,8 @@ test('can have multiple principals the same conditions in the same statement', (
new iam.ServicePrincipal('myService.amazon.com'),
new iam.ServicePrincipal('yourservice.amazon.com'),
],
actions: ['abc:def'],
resources: ['*'],
}));

user.addToPolicy(new iam.PolicyStatement({
Expand All @@ -100,9 +104,10 @@ test('can have multiple principals the same conditions in the same statement', (
},
}),
],
actions: ['abc:def'],
resources: ['*'],
}));
});

test('use Web Identity principal', () => {
// GIVEN
const stack = new Stack();
Expand Down
12 changes: 6 additions & 6 deletions packages/@aws-cdk/aws-lambda/test/test.lambda.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export = {
const role = new iam.Role(stack, 'SomeRole', {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
});
role.addToPolicy(new iam.PolicyStatement({ actions: ['confirm:itsthesame'] }));
role.addToPolicy(new iam.PolicyStatement({ actions: ['confirm:itsthesame'], resources: ['*'] }));

// WHEN
const fn = new lambda.Function(stack, 'Function', {
Expand All @@ -246,20 +246,20 @@ export = {
handler: 'index.test',
role,
initialPolicy: [
new iam.PolicyStatement({ actions: ['inline:inline'] }),
new iam.PolicyStatement({ actions: ['inline:inline'], resources: ['*'] }),
],
});

fn.addToRolePolicy(new iam.PolicyStatement({ actions: ['explicit:explicit'] }));
fn.addToRolePolicy(new iam.PolicyStatement({ actions: ['explicit:explicit'], resources: ['*'] }));

// THEN
expect(stack).to(haveResource('AWS::IAM::Policy', {
'PolicyDocument': {
'Version': '2012-10-17',
'Statement': [
{ 'Action': 'confirm:itsthesame', 'Effect': 'Allow' },
{ 'Action': 'inline:inline', 'Effect': 'Allow' },
{ 'Action': 'explicit:explicit', 'Effect': 'Allow' },
{ 'Action': 'confirm:itsthesame', 'Effect': 'Allow', 'Resource': '*' },
{ 'Action': 'inline:inline', 'Effect': 'Allow', 'Resource': '*' },
{ 'Action': 'explicit:explicit', 'Effect': 'Allow', 'Resource': '*' },
],
},
}));
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-logs/test/test.destination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export = {
// WHEN
dest.addToPolicy(new iam.PolicyStatement({
actions: ['logs:TalkToMe'],
resources: ['*'],
}));

// THEN
Expand Down
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-sns/test/test.sns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ export = {

const topic = new sns.Topic(stack, 'MyTopic');

topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement0'] }));
topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement1'] }));
topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement0'], resources: ['*'] }));
topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement1'], resources: ['*'] }));

expect(stack).toMatch({
'Resources': {
Expand All @@ -181,11 +181,13 @@ export = {
{
'Action': 'service:statement0',
'Effect': 'Allow',
'Resource': '*',
'Sid': '0',
},
{
'Action': 'service:statement1',
'Effect': 'Allow',
'Resource': '*',
'Sid': '1',
},
],
Expand Down