aws-ecr: RepositoryPolicyText should error if includes Resource

[ahammond]

Describe the feature

When adding a resource policy to an ECR repo, if your PolicyStatment includes a Resource, it will successfully synth, but then fail to deploy.

CDK should detect the presence of the Resource key and issue an Error.

Use Case

I spent an entire day staring at a perfectly legal looking resource policy. Only to find out that my bug was this illegal resource key. Error message from Cfn wasn't very helpful:

Resource handler returned message: "Invalid parameter at 'PolicyText' failed to satisfy constraint: 'Invalid repository policy provided' (Service: Ecr, Status Code: 400,  ...

Proposed Solution

When calling myRepo.addToResourcePolicy(), check the input PolicyStatement to confirm it doesn't have a resources: key.

Other Information

No response

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

2.27.0

Environment details (OS name and version, etc.)

MacOS, pretty recent. Node 16

[khushail]

Thanks for reaching out @ahammond . It would be really helpful if you could share the code for reproducing this error

[ahammond]

Write literally any PolicyStatement that includes the traditional

resources: ['*']

And you'll see it. I can provide a snippet if necessary, but this is pretty easy to repro.

[ahammond]

import { App, Stack, StackProps } from 'aws-cdk-lib';
import { Repository } from 'aws-cdk-lib/aws-ecr';
import { AnyPrincipal, PolicyStatement } from 'aws-cdk-lib/aws-iam';
import { Construct } from 'constructs';

export class EcrStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const r = new Repository(this, 'MyEcrRepo');
    r.addToResourcePolicy(new PolicyStatement({
      actions: ['ecr:GetDownloadUrlForLayer'],
      principals: [new AnyPrincipal()],
      // Every other resource policy I've ever written requires a resource.
      // They're really only good for limiting access within a resource.
      // I understand why they're allowed, but not why they're required.
      // Strangely, not only does Cfn for ECR not allow
      // us to specify a resource policy for a specific image,
      // it will straight up fail if a resource section is present at all.
      resources: ['*'], // This will break, so CDK should warn. At least until Cfn (and possibly ECR) is fixed.
    }));
  }
}

const devEnv = {
  account: process.env.CDK_DEFAULT_ACCOUNT,
  region: process.env.CDK_DEFAULT_REGION,
};

const app = new App();
new EcrStack(app, 'MyEcrStack',  { env: devEnv });

app.synth();

[pahud]

@ahammond

I just checked the doc for ECR resource policy and yes 'resource' is not required while in other resource types such as cloudwatch logs resource policy or s3 bucket policy, the resource attribute is required.
https://docs.aws.amazon.com/AmazonECR/latest/userguide/repository-policy-examples.html

Yes I think we should add a check to avoid this error. I am making it as a p2 feature request and any PR submission will be highly appreciated.

[TheRealAmazonKendra]

The implementation of addToResourcePolicy here just plain shouldn't allow for providing resources. This is a bug. We'll likely need to deprecate the function and write a new one. Because there is a workaround, however, this is still a p2 and we will not have capacity to implement the fix. We welcome community contributions on this, though.

[ahammond]

@TheRealAmazonKendra I agree it shouldn't allow providing a resource. Deprecating and replacing the function is probably a medium, but... adding a Warn on the existing function would save your customers hours of very frustrating debugging time. It's an easy fix for what it otherwise a terrible UX. And it's a very, very, small.

[TheRealAmazonKendra]

You're totally right and we'd certainly accept a PR with that as a stopgap. Since this is a P2, however, we don't have the capacity to make the change.

[jbrathwa]

Is there any workaround to this?

[ahammond]

Is there any workaround to this?

What I did was

• Write a reasonable resource policy, including a resources entry. Same as I've done for countless other AWS resources.
• Got an obscure error message.
• Googled the error message, found nothing.
• Found and read https://docs.aws.amazon.com/AmazonECR/latest/userguide/repository-policies.html but still didn't see the problem.
• Opened an AWS support ticket. Chatted with the agent. They thought my policy was reasonable and correct.
• Agent escalated internally, they also thought policy was reasonable.
• +1hr of trying different things and agent asked me to try removing the resource.

So... sure. The workaround is finding this issue and knowing to not put a resource in your policy statement. Which... is a pretty bad user experience.

[paulhcsun]

Closed by #24401

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.