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

shreyasdamle · 2020-05-10T17:58:19Z

Commit Message

Added compile time validation for the policy statements. PolicyStatement props will compile only with the following condition:

export type PolicyStatementProps =  (PolicyStatementActions | PolicyStatementNotActions) & (PolicyStatementResources | PolicyStatementNotResources);

Fixes #7615

End Commit Message

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation · 2020-05-10T18:04:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e41f22e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-10T18:25:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c5aa2b2
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2020-05-14T11:59:11Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

@@ -33,8 +33,8 @@ export class PolicyStatement {
      resources: ensureArrayOrUndefined(obj.Resource),
      conditions: obj.Condition,
      effect: obj.Effect,
-      notActions: ensureArrayOrUndefined(obj.NotAction),
-      notResources: ensureArrayOrUndefined(obj.NotResource),
+      notActions: ensureArrayOrUndefined(obj.NotAction) ?? [],


Why this change?

Why this change for notActions and notResources, and not for actions and resources?

rix0rrr · 2020-05-14T12:00:59Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+  readonly notResources: string[];
+}
+
+export type PolicyStatementProps =  (PolicyStatementActions | PolicyStatementNotActions) & (PolicyStatementResources | PolicyStatementNotResources);


We can't support type declarations like this in jsii.

Your build should have yelled at you.

rix0rrr · 2020-05-14T12:02:43Z

packages/@aws-cdk/aws-iam/test/policy-document.test.ts

        resources: ['abc'],
-        notResources: ['def'],
+        notResources: ['abc'],


You should not have needed to change this line. If you have, something is wrong.

Pull request has been modified.

aws-cdk-automation · 2020-05-19T01:54:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e4470b2
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T01:54:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8c520a9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T02:47:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 806062c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T02:47:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 507f493
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T03:28:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b552ba7
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T03:30:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8befbe9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T04:18:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8ba9a3b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T04:22:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: aac74dc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T04:55:20Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d0081f9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T04:57:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b70dacd
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T20:58:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f37637b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T20:58:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4e743ed
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2020-05-21T11:22:51Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+   * @param props
+   */
+  private validateProps(props: PolicyStatementProps) {
+    if (props.conditions || props.effect || props.notPrincipals || props.notResources || props.principals || props.resources || props.sid) {


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?

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

const p = new PolicyStatement();

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('...'); }

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.

rix0rrr · 2020-05-21T11:24:19Z

packages/@aws-cdk/aws-iam/README.md

@@ -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:


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?

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

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

shreyasdamle · 2020-05-21T18:47:35Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+      }
+    }
+    if (props.actions || props.conditions || props.effect || props.notActions || props.notPrincipals || props.principals || props.sid) {
+      if (!props.resources && !props.notResources) {


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.

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.

rix0rrr · 2020-06-19T08:37:52Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+   * @param props
+   */
+  private validateProps(props: PolicyStatementProps) {
+    if (props.conditions || props.effect || props.notPrincipals || props.notResources || props.principals || props.resources || props.sid) {


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('...'); }

rix0rrr · 2020-06-19T08:39:36Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+   * @param props
+   */
+  private validateProps(props: PolicyStatementProps) {
+    if (props.conditions || props.effect || props.notPrincipals || props.notResources || props.principals || props.resources || props.sid) {


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.

rix0rrr · 2020-06-19T08:40:08Z

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

@@ -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: ['*'],


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

rix0rrr · 2020-06-19T08:44:25Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+      }
+    }
+    if (props.actions || props.conditions || props.effect || props.notActions || props.notPrincipals || props.principals || props.sid) {
+      if (!props.resources && !props.notResources) {


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.

rix0rrr · 2020-08-10T13:24:59Z

Seems to have been replaced by #9269

Adds policy statement validation checks for all constructs that contain IAM policies. Fixes #7615 ---- Note: sensitive module (requires 2 PR approvers). I am not sure if the changes presented here are considered breaking or not. I'm looking to get early feedback for this commit! I'm not an IAM subject matter expert, so this is my best interpretation of the AWS docs I've read so far - but I'm still trying to figure out what details and edge cases I'm missing if any. 😅 ## Problem To recap what rix0rrr mentions in <#7897>, my understanding is that there are two main kinds of IAM policies we are interested in checking: - identity-based policy: gets **attached to an IAM principal**, but **specifies the resources** it applies to - resource-based policy gets **attached to a resource**, but **specifies the IAM principal(s)** it applies to I am not aware of any other kinds of policies mentioned in the AWS docs [here](https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html) that can be constructed in the CDK. Based on [this](https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html#access_policies-json) IAM docs page, it seems to me that there are at least four types of distinct policy statements errors that we could feasibly try to check right before synthesis time: 1. a resource-based policy and no principal/notPrincipal are specified 2. a identity-based policy and principal/notPrincipal are specified 3. a identity-based policy and no resources/notResources are specified 4. any kind of policy and no actions/notActions are specified The reason we need to perform these checks right before synthesis is because it can't be done at constructor time, since CDK users can add actions, principals, etc. after creation / out of order. ## Design Since we want the validation logic to happen at synthesis time, the best option is to override the validate() method for the appropriate Construct child subclasses that we want these rules to apply to. The following is the list of constructs I found that directly contain policy documents: - Role - ManagedPolicy - Policy - BucketPolicy - TopicPolicy - QueuePolicy - Repository (ECR) - Secret - Key - Alias (KMS) All other constructs that have some kind of role/policy statement functions (e.g. iam.User, lambda.Function) automatically get the validations transitively through one of the constructs above. In my commits, I've added appropriate methods to PolicyStatement and PolicyDocument to perform validation of various kinds, and then called these methods in the appropriate construct's validate() methods. ### Other considerations: It's also possible we could add calls to the PolicyStatement validation methods inside other methods such as addToResourcePolicy() found within several classes - but I think this could make the library less flexible, since a statement can still be modified after it has been added to a PolicyDocument (correct me if wrong). An alternative design I considered for the case of Resource constructs was extending the interface of IResourceWithPolicy with a validatePolicy() method, but it doesn't make a lot of sense to make the method public (which would have been required by TypeScript), and in the end all I want to do is overwrite the protected Construct#validate() method anyway. Since IResourceWithPolicy is just an interface (and not a class), I don't see any way to cleanly enforce that all Resources with policies will perform the policy validation, but I think the current solution is adequate. ## Examples These are examples of the four errors presented above that you can easily verify will fail during deployment (but aren't caught at compile or synth time), and which I've tested fail with my added changes. ``` // 1 const bucket = new s3.Bucket(this, 'TestBucket'); bucket.addToResourcePolicy(new iam.PolicyStatement({ actions: ['*'] })); // 2 const role = new iam.Role(this, 'TestRole', { assumedBy: new iam.AnyPrincipal(), }) role.attachInlinePolicy(new iam.Policy(this, 'MyPolicy', { statements: [new iam.PolicyStatement({ resources: ['*'], actions: ['*'], principals: [new iam.AnyPrincipal()] })] })); // 3 const role = new iam.Role(this, 'TestRole', { assumedBy: new iam.AccountPrincipal("457310432007"), }); role.attachInlinePolicy(new iam.Policy(this, 'MyPolicy', { statements: [new iam.PolicyStatement({ actions: ['*'], })] })); // 4 const bucket = new s3.Bucket(this, 'TestBucket') bucket.addToResourcePolicy(new iam.PolicyStatement({ principals: [new iam.AnyPrincipal()] })); ``` ## Issues - [x] Policy documents added through the "inlinePolicies" prop of iam.Role do not get validated. - [x] Add blurb about feature to [README.md](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-iam/README.md) to silence PR linter ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

SomayaB assigned rix0rrr May 11, 2020

rix0rrr previously requested changes May 14, 2020

View reviewed changes

shreyasdamle and others added 3 commits May 18, 2020 15:38

Validate the policy statements

7647d98

Fix PR linter error

ae838a1

Add runtime check to validate PolicyStatementProps

e4470b2

shreyasdamle force-pushed the feat-7615 branch from c5aa2b2 to e4470b2 Compare May 19, 2020 01:46

Fix policy statement in aws-logs for 'add policy to destination' test

507f493

shreyasdamle force-pushed the feat-7615 branch from 8c520a9 to 507f493 Compare May 19, 2020 02:39

Fix policy statement in aws-sns for unit test

8befbe9

shreyasdamle force-pushed the feat-7615 branch from 806062c to 8befbe9 Compare May 19, 2020 03:19

Fix policy statement in aws-lambda unit test

8ba9a3b

shreyasdamle force-pushed the feat-7615 branch from b552ba7 to 8ba9a3b Compare May 19, 2020 04:07

Fix policy statement in CF web-distributions

d0081f9

shreyasdamle force-pushed the feat-7615 branch from aac74dc to d0081f9 Compare May 19, 2020 04:45

Fix policy statement in CF tests

4e743ed

shreyasdamle force-pushed the feat-7615 branch from b70dacd to 4e743ed Compare May 19, 2020 20:49

Merge branch 'master' into feat-7615

f37637b

rix0rrr reviewed May 21, 2020

View reviewed changes

shreyasdamle commented May 21, 2020

View reviewed changes

shreyasdamle marked this pull request as draft June 10, 2020 20:35

rix0rrr added the pr-linter/exempt-readme The PR linter will not require README changes label Jun 19, 2020

rix0rrr reviewed Jun 19, 2020

View reviewed changes

Chriscbr mentioned this pull request Jul 27, 2020

feat(iam): validate policies for missing resources/principals #9269

Merged

2 tasks

rix0rrr closed this Aug 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shreyasdamle commented May 10, 2020

aws-cdk-automation commented May 10, 2020

aws-cdk-automation commented May 10, 2020

rix0rrr May 14, 2020

rix0rrr May 14, 2020

rix0rrr May 14, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

rix0rrr May 21, 2020

shreyasdamle May 21, 2020

rix0rrr Jun 19, 2020

rix0rrr Jun 19, 2020

rix0rrr May 21, 2020

shreyasdamle May 21, 2020

rix0rrr Jun 19, 2020

shreyasdamle May 21, 2020

rix0rrr Jun 19, 2020

rix0rrr Jun 19, 2020

rix0rrr Jun 19, 2020

rix0rrr Jun 19, 2020

rix0rrr Jun 19, 2020

rix0rrr commented Aug 10, 2020

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

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

Conversation

shreyasdamle commented May 10, 2020

Commit Message

End Commit Message

aws-cdk-automation commented May 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 10, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Aug 10, 2020