fix(ecr): policytext errors when includes resource (#24401)

ECR does not allow resource to be included in private repository resource policies.
CFN largely swallows the error message.
Most resources require or at least allow a resource in their policies, so we should at least warn.

See issue #24314

Author: ahammond

packages/@aws-cdk-testing/framework-integ/test/aws-ecr/test/integ.basic.js.snapshot/aws-ecr-integ-stack.template.json

@@ -5,7 +5,19 @@
    "Properties": {
     "LifecyclePolicy": {
      "LifecyclePolicyText": "{\"rules\":[{\"rulePriority\":1,\"selection\":{\"tagStatus\":\"any\",\"countType\":\"imageCountMoreThan\",\"countNumber\":5},\"action\":{\"type\":\"expire\"}}]}"
-    }
+    },
+    "RepositoryPolicyText": {
+      "Statement": [
+       {
+        "Action": "ecr:GetDownloadUrlForLayer",
+        "Effect": "Allow",
+        "Principal": {
+         "AWS": "*"
+        }
+       }
+      ],
+      "Version": "2012-10-17"
+     }
    },
    "UpdateReplacePolicy": "Retain",
    "DeletionPolicy": "Retain"

packages/@aws-cdk-testing/framework-integ/test/aws-ecr/test/integ.basic.ts

@@ -1,12 +1,17 @@
 import * as cdk from 'aws-cdk-lib';
 import { IntegTest } from '@aws-cdk/integ-tests-alpha';
 import * as ecr from 'aws-cdk-lib/aws-ecr';
+import * as iam from 'aws-cdk-lib/aws-iam';
 
 const app = new cdk.App();
 const stack = new cdk.Stack(app, 'aws-ecr-integ-stack');
 
 const repo = new ecr.Repository(stack, 'Repo');
 repo.addLifecycleRule({ maxImageCount: 5 });
+repo.addToResourcePolicy(new iam.PolicyStatement({
+  actions: ['ecr:GetDownloadUrlForLayer'],
+  principals: [new iam.AnyPrincipal()],
+}));
 
 new cdk.CfnOutput(stack, 'RepositoryURI', {
   value: repo.repositoryUri,

packages/aws-cdk-lib/aws-ecr/README.md

@@ -126,3 +126,18 @@ const repository = new Repository(this, 'MyTempRepo', {
   autoDeleteImages: true,
 });
 ```
+
+## Managing the Resource Policy
+
+You can add statements to the resource policy of the repository using the
+`addToResourcePolicy` method. However, be advised that you must not include
+a `resources` section in the `PolicyStatement`.
+
+```ts
+declare const repository: ecr.Repository;
+repository.addToResourcePolicy(new iam.PolicyStatement({
+  actions: ['ecr:GetDownloadUrlForLayer'],
+  // resources: ['*'], // not currently allowed!
+  principals: [new iam.AnyPrincipal()],
+}));
+```

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

@@ -4,6 +4,7 @@ import * as events from '../../aws-events';
 import * as iam from '../../aws-iam';
 import * as kms from '../../aws-kms';
 import {
+  Annotations,
   ArnFormat,
   IResource,
   Lazy,
@@ -424,7 +425,7 @@ export interface OnCloudTrailImagePushedOptions extends events.OnEventOptions {
  */
 export interface OnImageScanCompletedOptions extends events.OnEventOptions {
   /**
-   * Only watch changes to the image tags spedified.
+   * Only watch changes to the image tags specified.
    * Leave it undefined to watch the full repository.
    *
    * @default - Watch the changes to the repository with all image tags
@@ -660,7 +661,17 @@ export class Repository extends RepositoryBase {
     this.node.addValidation({ validate: () => this.policyDocument?.validateForResourcePolicy() ?? [] });
   }
 
+  /**
+   * Add a policy statement to the repository's resource policy.
+   *
+   * While other resources policies in AWS either require or accept a resource section,
+   * Cfn for ECR does not allow us to specify a resource policy.
+   * It will fail if a resource section is present at all.
+   */
   public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
+    if (statement.resources) {
+      Annotations.of(this).addWarning('ECR resource policy does not allow resource statements.');
+    }
     if (this.policyDocument === undefined) {
       this.policyDocument = new iam.PolicyDocument();
     }

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

@@ -1,5 +1,5 @@
 import { EOL } from 'os';
-import { Template } from '../../assertions';
+import { Annotations, Template } from '../../assertions';
 import * as iam from '../../aws-iam';
 import * as kms from '../../aws-kms';
 import * as cdk from '../../core';
@@ -347,7 +347,6 @@ describe('repository', () => {
 
     // WHEN
     repo.addToResourcePolicy(new iam.PolicyStatement({
-      resources: ['*'],
       principals: [new iam.ArnPrincipal('arn')],
     }));
 
@@ -363,14 +362,30 @@ describe('repository', () => {
 
     // WHEN
     repo.addToResourcePolicy(new iam.PolicyStatement({
-      resources: ['*'],
       actions: ['ecr:*'],
     }));
 
     // THEN
     expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
   });
 
+  test('warns if repository policy has resources', () => {
+    // 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:*'],
+      principals: [new iam.AnyPrincipal()],
+    }));
+
+    // THEN
+    Annotations.fromStack(stack).hasWarning('*', 'ECR resource policy does not allow resource statements.');
+  });
+
   test('default encryption configuration', () => {
     // GIVEN
     const app = new cdk.App();