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

fix(ecr): grants for cross-account principals result in failed deployments #16376

Merged
merged 8 commits into from
Dec 9, 2022

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Sep 4, 2021

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

@skinny85 skinny85 self-assigned this Sep 4, 2021
@gitpod-io
Copy link

gitpod-io bot commented Sep 4, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 4, 2021
rix0rrr
rix0rrr previously requested changes Sep 8, 2021
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with this approach. We're actually now doing something different than what the user asked for, and they could equally well just do what we're doing, but explicitly. So we can add a warning for it, and/or add it to the docs. Or tell the user to do authz based on tags.

Either that, or we potentially add the stack dependency automatically.

But trusting the whole account "just because" it happens to work out, although it does something different than what the user requested... I don't think that's a good idea.


// condition #3
const principalStack = Stack.of(principal);
if (this.stack.dependencies.includes(principalStack)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is safe or even likely to be true. Might as well leave this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we already do this for KMS Keys, so I think it's not the worst idea.

@rix0rrr rix0rrr added bug This issue is a bug. p1 and removed contribution/core This is a PR that came from AWS. labels Mar 4, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 4, 2022
@skinny85
Copy link
Contributor Author

@rix0rrr I want to resurrect this old PR, if you don't mind 🙂.

I'm not sure I agree with this approach. We're actually now doing something different than what the user asked for, and they could equally well just do what we're doing, but explicitly. So we can add a warning for it, and/or add it to the docs. Or tell the user to do authz based on tags.

Either that, or we potentially add the stack dependency automatically.

But trusting the whole account "just because" it happens to work out, although it does something different than what the user requested... I don't think that's a good idea.

I'm not sure there's something else we can do. If we don't change the logic inside the ECR Repository, then, regardless of what the customer tries to do "manually" themselves, we already add the Role from the Service Stack (which doesn't exist yet) to the resource policy of the ECR repo that lives in the pipeline Stack. Which means, regardless of what else the customer wants to do there (trust the entire account, use tags for permissions, etc.), the problem is already there, and the repository will fail to deploy - so, no amount of tinkering (short of something really low-level, like escape hatches) can fix that.

@skinny85 skinny85 assigned rix0rrr and unassigned skinny85 May 5, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the base branch from v1-main to main July 6, 2022 22:27
@TheRealAmazonKendra TheRealAmazonKendra removed the contribution/core This is a PR that came from AWS. label Jul 6, 2022
@github-actions github-actions bot added the effort/small Small work item – less than a day of effort label Jul 6, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 6, 2022
@mrgrain mrgrain removed the contribution/core This is a PR that came from AWS. label Aug 8, 2022
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@skinny85 skinny85 requested a review from rix0rrr August 9, 2022 04:44
@skinny85
Copy link
Contributor Author

skinny85 commented Aug 9, 2022

@rix0rrr you want to reply here, before the PR gets closed? 🙂 #16376 (comment)

@TheRealAmazonKendra
Copy link
Contributor

Psssst: If you resolve the conflicts, it resets the timer.

@mergify mergify bot dismissed rix0rrr’s stale review August 12, 2022 06:36

Pull request has been modified.

@skinny85 skinny85 force-pushed the fix/tag-parameter-image-cross-account branch from 74f3ed8 to c8251d4 Compare August 12, 2022 06:38
@skinny85 skinny85 force-pushed the fix/tag-parameter-image-cross-account branch from c8251d4 to e3bf9e6 Compare August 12, 2022 06:42
@skinny85
Copy link
Contributor Author

Psssst: If you resolve the conflicts, it resets the timer.

Thanks, I really hope so, because solving these conflicts was super irritating 😃.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still scared of opening up to an entire account.

Can we do something like the following:

class Role {

  public crossAccountSafeGrantable(): IGrantable {
    if (!this.roleTag) {
      this.roleTag = `${Stack.of(this).stackName}${this.node.addr}`; 
      Tags.of(this).add(this.roleTag, 'aws-cdk:id', this.roleTag);
    }

    return new AccountRootPrincipal(Stack.of(this.account), {
      conditions: {
        StringEquals: { 'aws:PrincipalTag/aws-cdk:id', this.roleTag },
      },
    });
  }
}

And use that as the IGrantable instead?

* if we cannot put a reference to the principal itself there,
* and 'undefined' in case we can.
*/
private principalCannotBeSafelyAddedToResourcePolicy(grantee: iam.IGrantable): string | undefined {
Copy link
Contributor

@rix0rrr rix0rrr Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this return the correct IGrantable instead?

private correctGrantable(grantee: IGrantable): IGrantable {
  if (isRoleInDifferentStack(grantee)) {
    return grantee.crossAccountSafeGrantable();
  }

  return grantee;
}

Copy link
Contributor Author

@skinny85 skinny85 Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the code you suggested:

    return new AccountRootPrincipal(Stack.of(this.account), {
      conditions: {
        StringEquals: { 'aws:PrincipalTag/aws-cdk:id', this.roleTag },
      },
    });

doesn't work (conditions are attached to Policy Statements, not to Principals).

However, added the tag to the Role in a different way, let me know if that works!

Comment on lines 354 to 356
// 1. The principal is from a different account.
// 2. The principal is a new resource (meaning, not imported).
// 3. The Stack this repo belongs to doesn't depend on the Stack the principal belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these combine with AND or OR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And. Clarified in the comments.

packages/@aws-cdk/aws-iam/lib/utils.ts Outdated Show resolved Hide resolved

// condition #3
const principalStack = Stack.of(principal);
if (this.stack.dependencies.includes(principalStack)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't really know this. Dependencies probably get resolve really late, especially implicit ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's better to make this check than not (even if it doesn't work 100% of the time, it might work some of the time).

@skinny85
Copy link
Contributor Author

skinny85 commented Nov 7, 2022

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

Yep. The validation error is because of missing integration tests:

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

But, this is a cross-account scenario, and CDK integration tests are meant to run in a single account.

The functionality has unit tests that confirm the cross-account scenario works as expected.

@skinny85
Copy link
Contributor Author

skinny85 commented Nov 7, 2022

@rix0rrr incorporated your comments, would appreciate another review!

@skinny85 skinny85 requested review from rix0rrr and removed request for madeline-k November 7, 2022 08:12
@Naumel Naumel added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 7, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 7, 2022 09:35

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@skinny85
Copy link
Contributor Author

Is the build for main broken right now? This is what I get:

aws-cdk: In package package.json
aws-cdk: - [@aws-cdk/node-bundle => outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)
aws-cdk: Error: Some package.json files had errors
aws-cdk:     at main (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:30:15)
aws-cdk:     at Object.<anonymous> (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:33:1)
aws-cdk:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
aws-cdk:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
aws-cdk:     at Module.load (internal/modules/cjs/loader.js:950:32)
aws-cdk:     at Function.Module._load (internal/modules/cjs/loader.js:790:12)
aws-cdk:     at Module.require (internal/modules/cjs/loader.js:974:19)
aws-cdk:     at require (internal/modules/cjs/helpers.js:101:18)
aws-cdk:     at Object.<anonymous> (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint:2:1)
aws-cdk:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
aws-cdk: Error: pkglint exited with error code 1
aws-cdk: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
aws-cdk: error Command failed with exit code 1.
aws-cdk: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
aws-cdk: error Command failed with exit code 1.
aws-cdk: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build+test exited 1 in 'aws-cdk'

I haven't touched the CLI in this PR, but I also had to update the third-party license attribution for two other packages to make them build, so I suspect something is up with main.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@skinny85
Copy link
Contributor Author

OK, looks like main was fixed with regard to the 3rd-party license issues, and the build is passing.

@rix0rrr this is ready for another round of reviews!

rix0rrr
rix0rrr previously approved these changes Dec 8, 2022
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 8, 2022

(Sorry for the late reply 😓 )

@mergify mergify bot dismissed rix0rrr’s stale review December 9, 2022 06:49

Pull request has been modified.

@skinny85
Copy link
Contributor Author

skinny85 commented Dec 9, 2022

(Sorry for the late reply 😓 )

It's all good brother ❤️. I needed a few months to address your comments, so I don't blame you at all for taking a few days to review this one!

@skinny85
Copy link
Contributor Author

skinny85 commented Dec 9, 2022

(BTW, I had to merge from main, as Mergify didn't want to add the PR to the merge queue, as it claimed there was a difference in a GitHub Actions workflow - I guess one of the workflows changed on main in the meantime... so if you could approve again, I would appreciate it!)

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0ba193b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Dec 9, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 84e20f8 into aws:main Dec 9, 2022
@skinny85 skinny85 deleted the fix/tag-parameter-image-cross-account branch December 9, 2022 07:26
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request 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 pull request 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 pull request 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
bug This issue is a bug. effort/small Small work item – less than a day of effort p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-codepipeline-actions): TagParameterContainerImage unusable cross-account
7 participants