Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-codepipeline-actions): TagParameterContainerImage unusable cross-account #15070

Closed
danwiltshire opened this issue Jun 10, 2021 · 15 comments · Fixed by #15073 or #16376
Closed

(aws-codepipeline-actions): TagParameterContainerImage unusable cross-account #15070

danwiltshire opened this issue Jun 10, 2021 · 15 comments · Fixed by #15073 or #16376
Labels
@aws-cdk/aws-codepipeline-actions bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1

Comments

@danwiltshire
Copy link
Contributor

danwiltshire commented Jun 10, 2021

Using TagParameterContainerImage as a property for a stack in a different account causes a resolution error.

Reproduction Steps

const myPipeline = new MyPipeline(app, 'my-pipeline', {
  env: nonprod
}

new MyFargateApp(app, 'my-app', {
  env: prod
  image: myPipeline.tagParameterContainerImage
}

What did you expect to happen?

Stack my-app should reference the ECR repo from my-pipeline.

What actually happened?

Error: Resolution error: Resolution error: Resolution error: Resolution error: Resolution error: Cannot use resource 'my-app/FargateService/TaskDef/ExecutionRole' in a cross-environment fashion, the resource's physical name must be explicit set or use 'PhysicalName.GENERATE_IF_NEEDED'.

Environment

  • CDK CLI Version : 1.107.0 (build 52c4434)
  • Framework Version: 1.107.0
  • Node.js Version: 14.16.0
  • OS : WSL2 Ubuntu 20.04.02 LTS on Windows 10 1909
  • Language (Version): TypeScript (3.9.9)

Other details

I'm using ApplicationLoadBalancedFargateService.

This is 🐛 Bug Report

@danwiltshire danwiltshire added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 10, 2021
@skinny85
Copy link
Contributor

Thanks for reporting @danwiltshire. Confirming I was able to reproduce it. I'm working on a fix.

In the meantime, you should be able to unblock yourself by passing the executionRole of the TaskDefinition explicitly with a Role that has the name set, like so:

      const fargateTaskDefinition = new ecs.FargateTaskDefinition(this, 'ServiceTaskDefinition', {
        executionRole: new iam.Role(serviceStack, 'ExecutionRole', {
          assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
          roleName: 'my-name',
        }),
      });

If you don't want to set a name yourself, you can pass roleName as cdk.PhysicalName.GENERATE_IF_NEEDED, and the CDK will generate it for you automatically.

@skinny85 skinny85 added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 10, 2021
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jun 10, 2021
When TagParameterContainerImage is used from a different acocunt than the service itself is in,
it fails resolution, because the Task's execution Role does not have a physical name set by default.
Create it with PhysicalName.GENERATE_IF_NEEDED, so that it assigns a name when accessed from a cross-account context.

Fixes aws#15070
@danwiltshire
Copy link
Contributor Author

danwiltshire commented Jun 11, 2021

Hey @skinny85 thanks, this got me a bit further. I'm now getting another issue where the ECR policy principal is not valid.

Error: Invalid parameter at 'PolicyText' failed to satisfy constraint: 'Invalid repository policy provided'

Cause: The Principal field must contain an asterisk when a full role ARN is used.

Solutions:

  • Use an asterisk in the ARN
  • Use the AWS root reference (as shown in Working policy below).

Synthed CFN:

    "FargatePipelineEcsDeployRepository70287658": {
      "Type": "AWS::ECR::Repository",
      "Properties": {
        "RepositoryName": "<EXPLICIT_REPO_NAME>",
        "RepositoryPolicyText": {
          "Statement": [
            {
              "Action": [
                "ecr:BatchCheckLayerAvailability",
                "ecr:GetDownloadUrlForLayer",
                "ecr:BatchGetImage"
              ],
              "Effect": "Allow",
              "Principal": {
                "AWS": {
                  "Fn::Join": [
                    "",
                    [
                      "arn:",
                      {
                        "Ref": "AWS::Partition"
                      },
                      ":iam::<ACCOUNT_NUMBER>:role/prod-appappexecutionrole0e3f44e5e1548be0860e"
                    ]
                  ]
                }
              }
            }
          ],
          "Version": "2012-10-17"
        },
      },
      "UpdateReplacePolicy": "Retain",
      "DeletionPolicy": "Retain",
      "Metadata": {
        "aws:cdk:path": ".../FargatePipeline/EcsDeployRepository/Resource"
      }
    },

Working policy:

{
  "Version": "2008-10-17",
  "Statement": [
    {
      "Sid": "new statement",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::<ACCOUNT_NUMBER>:root"
      },
      "Action": [
        "ecr:BatchCheckLayerAvailability",
        "ecr:BatchGetImage",
        "ecr:GetDownloadUrlForLayer"
      ]
    }
  ]
}

Other notes:

  • The ECR permission GUI also states an asterisk is needed: Statement[0].Principal must match the following: "/\*/" @ Statement[0].Principal
  • I've unblocked myself by using an escape hatch.

Workaround

        const cfnRepository = appEcrRepo.node.defaultChild as ecr.CfnRepository;

        cfnRepository.repositoryPolicyText = {
            "Version": "2008-10-17",
            "Statement": [
              {
                "Effect": "Allow",
                "Principal": {
                  "AWS": "arn:aws:iam::<ACCOUNT_NUMBER>:root"
                },
                "Action": [
                  "ecr:BatchCheckLayerAvailability",
                  "ecr:BatchGetImage",
                  "ecr:GetDownloadUrlForLayer"
                ]
              }
            ]
          }

If you need this in a new issue I'm happy to spin one up.

@skinny85
Copy link
Contributor

Hmm, but the error you get and the resolution don't match each other...?

{
  "Version": "2008-10-17",
  "Statement": [
    {
      "Sid": "new statement",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::<ACCOUNT_NUMBER>:root"
      },
      "Action": [
        "ecr:BatchCheckLayerAvailability",
        "ecr:BatchGetImage",
        "ecr:GetDownloadUrlForLayer"
      ]
    }
  ]
}

This does not contain a * in the Principal of the Statement...?

@danwiltshire
Copy link
Contributor Author

Correct, that seems to be the second available solution.

If you were to take a role ARN and put an asterisk in, that works to.

@skinny85
Copy link
Contributor

But an asterisk where? 🤔 Just tack it on randomly at the end of the Role ARN?

@danwiltshire
Copy link
Contributor Author

Yeah pretty much. I wonder if ECR just doesn't allow granting access to roles? Looking at the AWS docs granting a role isn't listed: https://docs.aws.amazon.com/AmazonECR/latest/userguide/repository-policy-examples.html

@mergify mergify bot closed this as completed in #15073 Jun 15, 2021
mergify bot pushed a commit that referenced this issue Jun 15, 2021
…15073)

When TagParameterContainerImage is used from a different acocunt than the service itself is in,
it fails resolution, because the Task's execution Role does not have a physical name set by default.
Create it with PhysicalName.GENERATE_IF_NEEDED, so that it assigns a name when accessed from a cross-account context.

Fixes #15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

@skinny85 skinny85 reopened this Jun 18, 2021
@skinny85
Copy link
Contributor

@danwiltshire confirming I was able to reproduce this error. I'm working on a fix.

@skinny85
Copy link
Contributor

I can confirm just adding a * at the end of the Role ARN does not solve the problem.

@skinny85
Copy link
Contributor

skinny85 commented Jul 13, 2021

Hmm, so I think I know what the problem is.

The issue is that, when using TagParameterContainerImage across accounts, the execution Role of the Task in the Stack is added to the resource policy of the ECR Repository. However, that Role doesn't exist at the time the Repository is deployed! (Because it belongs to the service Stack which will only be deployed by the Pipeline) And apparently ECR validates that.

@skinny85
Copy link
Contributor

skinny85 commented Jul 13, 2021

If you provide a Role that does exist in the Repository's resource policy, the deployment is successful. So ECR definitely does support Role-based resource policies.

@danwiltshire
Copy link
Contributor Author

Yeah you're right, I just tried specifying a nonexistent role in a different account and it failed. As soon as I specify a role that exists, it works.

@skinny85
Copy link
Contributor

This is quite the conundrum. Not sure exactly how to handle this from the CDK side.

@danwiltshire
Copy link
Contributor Author

There is the option of granting access at the account level e.g. arn:aws:iam::01234567890:root... although maybe not ideal for the security concious.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…ws#15073)

When TagParameterContainerImage is used from a different acocunt than the service itself is in,
it fails resolution, because the Task's execution Role does not have a physical name set by default.
Create it with PhysicalName.GENERATE_IF_NEEDED, so that it assigns a name when accessed from a cross-account context.

Fixes aws#15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Sep 4, 2021
…ments

When performing grants in ECR's Repository class for principals from other accounts,
we put the ARN of the principal inside the Resource Policy of the Repository.
However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository,
so if this cross-account principal was not created before the Repository,
its deployment would fail.

Detect that situation in the Repository class,
and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the `TagParameterContainerImage` class.

Fixes aws#15070
@skinny85 skinny85 added the in-progress This issue is being actively worked on. label May 4, 2022
@skinny85 skinny85 removed their assignment May 4, 2022
@mergify mergify bot closed this as completed in #16376 Dec 9, 2022
mergify bot pushed a commit that referenced this issue Dec 9, 2022
…ments (#16376)

When performing grants in ECR's Repository class for principals from other accounts,
we put the ARN of the principal inside the Resource Policy of the Repository.
However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository,
so if this cross-account principal was not created before the Repository,
its deployment would fail.

Detect that situation in the Repository class,
and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the `TagParameterContainerImage` class.

Fixes #15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

⚠️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.

brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Dec 9, 2022
…ments (aws#16376)

When performing grants in ECR's Repository class for principals from other accounts,
we put the ARN of the principal inside the Resource Policy of the Repository.
However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository,
so if this cross-account principal was not created before the Repository,
its deployment would fail.

Detect that situation in the Repository class,
and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the `TagParameterContainerImage` class.

Fixes aws#15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Jan 20, 2023
…ments (aws#16376)

When performing grants in ECR's Repository class for principals from other accounts,
we put the ARN of the principal inside the Resource Policy of the Repository.
However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository,
so if this cross-account principal was not created before the Repository,
its deployment would fail.

Detect that situation in the Repository class,
and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the `TagParameterContainerImage` class.

Fixes aws#15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Feb 22, 2023
…ments (aws#16376)

When performing grants in ECR's Repository class for principals from other accounts,
we put the ARN of the principal inside the Resource Policy of the Repository.
However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository,
so if this cross-account principal was not created before the Repository,
its deployment would fail.

Detect that situation in the Repository class,
and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the `TagParameterContainerImage` class.

Fixes aws#15070

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codepipeline-actions bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1
Projects
None yet
2 participants