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.

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

iam: Validate the policy statements when synthesizing policy #7615

Closed
zxkane opened this issue Apr 27, 2020 · 1 comment · Fixed by #9269
Closed

iam: Validate the policy statements when synthesizing policy #7615

zxkane opened this issue Apr 27, 2020 · 1 comment · Fixed by #9269
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.

Comments

@zxkane
Copy link
Contributor

zxkane commented Apr 27, 2020

Currently we can create policy with statements without actions and resources. However the deployment of Cloudformation stack will fail due to the 400 error of IAM API.

Per the doc of IAM policy grammar, the action and resource blocks are mandatory.

It would be nice to have a CDK validation to prevent malform statements submitted to Cloudformation to have a runtime failure.

Reproduction Steps

// create a malform policy like below,

const policy = new Policy(stack, 'Pol', { force: false });
    policy.addStatements(new PolicyStatement({
      resources: ['*'],
    }));

Error Log

Policy statement must contain actions. (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: 903150f5-cfd6-4db7-92ff-b8ae27065fa3)

Environment

  • CLI Version : 1.35.0
  • Framework Version:
  • OS : macosx
  • Language : ts

Other


This is 🐛 Bug Report

@zxkane zxkane added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 27, 2020
@SomayaB SomayaB added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Apr 29, 2020
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md and removed bug This issue is a bug. labels May 4, 2020
@SomayaB SomayaB added in-progress This issue is being actively worked on. and removed needs-triage This issue or PR still needs to be triaged. labels May 11, 2020
@shreyasdamle
Copy link
Contributor

I tried to implement this feature and then noticed that resource block isn't allowed in trust relationship policy. So, I'm not sure if we can write a generic condition to make resource block mandatory for PolicyStatement without first checking the trust relationship. For example, in the following code, resource block isn't allowed.

      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') ],
          }));
        }
      }

@mergify mergify bot closed this as completed in #9269 Aug 11, 2020
mergify bot pushed a commit that referenced this issue Aug 11, 2020
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.
Projects
None yet
4 participants