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

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 allow or deny action/);
+    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 allow or deny action.');
+    }
+    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

@@ -219,6 +219,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 policyName of Object.keys(this.inlinePolicies)) {
+      errors.push(...this.inlinePolicies[policyName].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 allow or deny action.']);
+  });
+
+  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 {