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

[Chriscbr]

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 that can be constructed in the CDK.

Based on this 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

• Policy documents added through the "inlinePolicies" prop of iam.Role do not get validated.
• Add blurb about feature to 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

[rix0rrr]

Thanks for the hard work you've been putting in here.

A couple of small tweaks and I'm happy to ship this!

[rix0rrr]

Yes, however, there is also effect: Allow | Deny, so I wonder if this might not be confusing what the actual check is.

Allow NotActions is not the same as Deny Actions.

Suggested change

	errors.push('A PolicyStatement must specify at least one allow or deny action.');
	errors.push('A PolicyStatement must specify at least one \'action\' or \'notAction\'.');

[Chriscbr]

Good catch - I forgot about this distinction!

[rix0rrr]

You can iterate over Object.values() instead.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 030da8d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).