feat(iam): validate policies for missing resources/principals (#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*

packages/@aws-cdk/aws-ecr/lib/repository.ts

@@ -473,6 +473,12 @@ export class Repository extends RepositoryBase {
     return { statementAdded: false, policyDependable: this.policyDocument };
   }
 
+  protected validate(): string[] {
+    const errors = super.validate();
+    errors.push(...this.policyDocument?.validateForResourcePolicy() || []);
+    return errors;
+  }
+
   /**
    * Add a life cycle rule to the repository
    *

packages/@aws-cdk/aws-ecr/test/test.repository.ts

@@ -284,7 +284,10 @@ export = {
     const repo = new ecr.Repository(stack, 'Repo');
 
     // WHEN
-    repo.addToResourcePolicy(new iam.PolicyStatement({ actions: ['*'] }));
+    repo.addToResourcePolicy(new iam.PolicyStatement({
+      actions: ['*'],
+      principals: [new iam.AnyPrincipal()],
+    }));
 
     // THEN
     expect(stack).to(haveResource('AWS::ECR::Repository', {
@@ -293,6 +296,7 @@ export = {
           {
             Action: '*',
             Effect: 'Allow',
+            Principal: '*',
           },
         ],
         Version: '2012-10-17',
@@ -302,6 +306,40 @@ export = {
     test.done();
   },
 
+  'fails if repository policy has no actions'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'my-stack');
+    const repo = new ecr.Repository(stack, 'Repo');
+
+    // WHEN
+    repo.addToResourcePolicy(new iam.PolicyStatement({
+      resources: ['*'],
+      principals: [new iam.ArnPrincipal('arn')],
+    }));
+
+    // THEN
+    test.throws(() => app.synth(), /A PolicyStatement must specify at least one \'action\' or \'notAction\'/);
+    test.done();
+  },
+
+  'fails if repository policy has no IAM principals'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'my-stack');
+    const repo = new ecr.Repository(stack, 'Repo');
+
+    // WHEN
+    repo.addToResourcePolicy(new iam.PolicyStatement({
+      resources: ['*'],
+      actions: ['ecr:*'],
+    }));
+
+    // THEN
+    test.throws(() => app.synth(), /A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
+    test.done();
+  },
+
   'events': {
     'onImagePushed without imageTag creates the correct event'(test: Test) {
       const stack = new cdk.Stack();

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

@@ -313,3 +313,6 @@ const principal = new iam.OpenIdConnectPrincipal(provider);
  * Policy name uniqueness is enforced. If two policies by the same name are attached to the same
    principal, the attachment will fail.
  * Policy names are not required - the CDK logical ID will be used and ensured to be unique.
+ * Policies are validated during synthesis to ensure that they have actions, and that policies
+   attached to IAM principals specify relevant resources, while policies attached to resources
+   specify which IAM principals they apply to.

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

@@ -294,6 +294,8 @@ export class ManagedPolicy extends Resource implements IManagedPolicy {
       result.push('Managed Policy is empty. You must add statements to the policy');
     }
 
+    result.push(...this.document.validateForIdentityPolicy());
+
     return result;
   }
 }

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

@@ -98,6 +98,48 @@ export class PolicyDocument implements cdk.IResolvable {
     return this.render();
   }
 
+  /**
+   * Validate that all policy statements in the policy document satisfies the
+   * requirements for any policy.
+   *
+   * @see https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html#access_policies-json
+   */
+  public validateForAnyPolicy(): string[] {
+    const errors = new Array<string>();
+    for (const statement of this.statements) {
+      errors.push(...statement.validateForAnyPolicy());
+    }
+    return errors;
+  }
+
+  /**
+   * Validate that all policy statements in the policy document satisfies the
+   * requirements for a resource-based policy.
+   *
+   * @see https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html#access_policies-json
+   */
+  public validateForResourcePolicy(): string[] {
+    const errors = new Array<string>();
+    for (const statement of this.statements) {
+      errors.push(...statement.validateForResourcePolicy());
+    }
+    return errors;
+  }
+
+  /**
+   * Validate that all policy statements in the policy document satisfies the
+   * requirements for an identity-based policy.
+   *
+   * @see https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html#access_policies-json
+   */
+  public validateForIdentityPolicy(): string[] {
+    const errors = new Array<string>();
+    for (const statement of this.statements) {
+      errors.push(...statement.validateForIdentityPolicy());
+    }
+    return errors;
+  }
+
   private render(): any {
     if (this.isEmpty) {
       return undefined;

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

@@ -408,6 +408,42 @@ export class PolicyStatement {
     }
     this.addConditions(conditions);
   }
+
+  /**
+   * Validate that the policy statement satisfies base requirements for a policy.
+   */
+  public validateForAnyPolicy(): string[] {
+    const errors = new Array<string>();
+    if (this.action.length === 0 && this.notAction.length === 0) {
+      errors.push('A PolicyStatement must specify at least one \'action\' or \'notAction\'.');
+    }
+    return errors;
+  }
+
+  /**
+   * Validate that the policy statement satisfies all requirements for a resource-based policy.
+   */
+  public validateForResourcePolicy(): string[] {
+    const errors = this.validateForAnyPolicy();
+    if (Object.keys(this.principal).length === 0 && Object.keys(this.notPrincipal).length === 0) {
+      errors.push('A PolicyStatement used in a resource-based policy must specify at least one IAM principal.');
+    }
+    return errors;
+  }
+
+  /**
+   * Validate that the policy statement satisfies all requirements for an identity-based policy.
+   */
+  public validateForIdentityPolicy(): string[] {
+    const errors = this.validateForAnyPolicy();
+    if (Object.keys(this.principal).length > 0 || Object.keys(this.notPrincipal).length > 0) {
+      errors.push('A PolicyStatement used in an identity-based policy cannot specify any IAM principals.');
+    }
+    if (Object.keys(this.resource).length === 0 && Object.keys(this.notResource).length === 0) {
+      errors.push('A PolicyStatement used in an identity-based policy must specify at least one resource.');
+    }
+    return errors;
+  }
 }
 
 /**

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

@@ -232,6 +232,8 @@ export class Policy extends Resource implements IPolicy {
       }
     }
 
+    result.push(...this.document.validateForIdentityPolicy());
+
     return result;
   }
 

packages/@aws-cdk/aws-iam/lib/role.ts

@@ -292,6 +292,7 @@ export class Role extends Resource implements IRole {
   private defaultPolicy?: Policy;
   private readonly managedPolicies: IManagedPolicy[] = [];
   private readonly attachedPolicies = new AttachedPolicies();
+  private readonly inlinePolicies: { [name: string]: PolicyDocument };
   private immutableRole?: IRole;
 
   constructor(scope: Construct, id: string, props: RoleProps) {
@@ -306,6 +307,7 @@ export class Role extends Resource implements IRole {
 
     this.assumeRolePolicy = createAssumeRolePolicy(props.assumedBy, externalIds);
     this.managedPolicies.push(...props.managedPolicies || []);
+    this.inlinePolicies = props.inlinePolicies || {};
     this.permissionsBoundary = props.permissionsBoundary;
     const maxSessionDuration = props.maxSessionDuration && props.maxSessionDuration.toSeconds();
     validateMaxSessionDuration(maxSessionDuration);
@@ -318,7 +320,7 @@ export class Role extends Resource implements IRole {
     const role = new CfnRole(this, 'Resource', {
       assumeRolePolicyDocument: this.assumeRolePolicy as any,
       managedPolicyArns: Lazy.listValue({ produce: () => this.managedPolicies.map(p => p.managedPolicyArn) }, { omitEmpty: true }),
-      policies: _flatten(props.inlinePolicies),
+      policies: _flatten(this.inlinePolicies),
       path: props.path,
       permissionsBoundary: this.permissionsBoundary ? this.permissionsBoundary.managedPolicyArn : undefined,
       roleName: this.physicalName,
@@ -420,6 +422,15 @@ export class Role extends Resource implements IRole {
 
     return this.immutableRole;
   }
+
+  protected validate(): string[] {
+    const errors = super.validate();
+    errors.push(...this.assumeRolePolicy?.validateForResourcePolicy() || []);
+    for (const policy of Object.values(this.inlinePolicies)) {
+      errors.push(...policy.validateForIdentityPolicy());
+    }
+    return errors;
+  }
 }
 
 /**

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

@@ -547,12 +547,30 @@ describe('managed policy', () => {
     });
   });
 
+  test('fails if policy document does not specify resources', () => {
+    new ManagedPolicy(stack, 'MyManagedPolicy', { statements: [
+      new PolicyStatement({ actions: ['*'] }),
+    ] });
+
+    expect(() => app.synth()).toThrow(/A PolicyStatement used in an identity-based policy must specify at least one resource/);
+  });
+
+
+  test('fails if policy document specifies principals', () => {
+    new ManagedPolicy(stack, 'MyManagedPolicy', { statements: [
+      new PolicyStatement({ actions: ['*'], resources: ['*'], principals: [new ServicePrincipal('test.service')] }),
+    ] });
+
+    expect(() => app.synth()).toThrow(/A PolicyStatement used in an identity-based policy cannot specify any IAM principals/);
+  });
+
   test('cross-stack hard-name contains the right resource type', () => {
     const mp = new ManagedPolicy(stack, 'Policy', {
       managedPolicyName: cdk.PhysicalName.GENERATE_IF_NEEDED,
     });
     mp.addStatements(new PolicyStatement({
       actions: ['a:abc'],
+      resources: ['*'],
     }));
 
     const stack2 = new cdk.Stack(app, 'Stack2', { env: { account: '5678', region: 'us-east-1' }});

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

@@ -725,4 +725,25 @@ describe('IAM policy document', () => {
       Condition: {StringEquals: {'kms:ViaService': 'service', 'sts:ExternalId': '12221121221'}},
     });
   });
+
+  test('validation error if policy statement has no actions', () => {
+    const policyStatement = new PolicyStatement({
+      principals: [new AnyPrincipal()],
+    });
+
+    // THEN
+    const validationErrorsForResourcePolicy: string[] = policyStatement.validateForResourcePolicy();
+    // const validationErrorsForIdentityPolicy: string[] = policyStatement.validateForIdentityPolicy();
+    expect(validationErrorsForResourcePolicy).toEqual(['A PolicyStatement must specify at least one \'action\' or \'notAction\'.']);
+  });
+
+  test('validation error if policy statement for resource-based policy has no principals specified', () => {
+    const policyStatement = new PolicyStatement({
+      actions: ['*'],
+    });
+
+    // THEN
+    const validationErrors: string[] = policyStatement.validateForResourcePolicy();
+    expect(validationErrors).toEqual(['A PolicyStatement used in a resource-based policy must specify at least one IAM principal.']);
+  });
 });

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

@@ -310,6 +310,17 @@ describe('IAM policy', () => {
 
     expect(() => app.synth()).toThrow(/must contain at least one statement/);
   });
+
+  test('fails if policy document is invalid', () => {
+    new Policy(stack, 'MyRole', {
+      statements: [new PolicyStatement({
+        actions: ['*'],
+        principals: [new ServicePrincipal('test.service')],
+      })],
+    });
+
+    expect(() => app.synth()).toThrow(/A PolicyStatement used in an identity-based policy cannot specify any IAM principals/);
+  });
 });
 
 function createPolicyWithLogicalId(stack: Stack, logicalId: string): void {