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
Changes from 3 commits
e3bf9e6
3fc764c
7376f5e
87b1505
9a63bf0
1841739
c3400e2
0ba193b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ module.exports = { | |
global: { | ||
...baseConfig.coverageThreshold.global, | ||
branches: 70, | ||
statements: 79, | ||
}, | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { EOL } from 'os'; | |
import * as events from '@aws-cdk/aws-events'; | ||
import * as iam from '@aws-cdk/aws-iam'; | ||
import * as kms from '@aws-cdk/aws-kms'; | ||
import { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; | ||
import { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core'; | ||
import { IConstruct, Construct } from 'constructs'; | ||
import { CfnRepository } from './ecr.generated'; | ||
import { LifecycleRule, TagStatus } from './lifecycle'; | ||
|
@@ -279,17 +279,41 @@ export abstract class RepositoryBase extends Resource implements IRepository { | |
rule.addTarget(options.target); | ||
return rule; | ||
} | ||
|
||
/** | ||
* Grant the given principal identity permissions to perform the actions on this repository | ||
*/ | ||
public grant(grantee: iam.IGrantable, ...actions: string[]) { | ||
return iam.Grant.addToPrincipalOrResource({ | ||
grantee, | ||
actions, | ||
resourceArns: [this.repositoryArn], | ||
resourceSelfArns: [], | ||
resource: this, | ||
}); | ||
const accountToTrust = this.principalCannotBeSafelyAddedToResourcePolicy(grantee); | ||
if (accountToTrust) { | ||
// If the principal is from a different account, | ||
// that means addToPrincipalOrResource() will update the Resource Policy of this repo to trust that principal. | ||
// However, ECR verifies that the principal used in the Policy exists, | ||
// and will error out if it doesn't. | ||
// Because of that, if the principal is a newly created resource, | ||
// and there is not a dependency relationship between the Stacks of this repo and the principal, | ||
// trust the entire account of the principal instead | ||
// (otherwise, deploying this repo will fail) | ||
this.addToResourcePolicy(new iam.PolicyStatement({ | ||
actions, | ||
principals: [new iam.AccountPrincipal(accountToTrust)], | ||
})); | ||
|
||
return iam.Grant.addToPrincipal({ | ||
grantee, | ||
actions, | ||
resourceArns: [this.repositoryArn], | ||
scope: this, | ||
}); | ||
} else { | ||
return iam.Grant.addToPrincipalOrResource({ | ||
grantee, | ||
actions, | ||
resourceArns: [this.repositoryArn], | ||
resourceSelfArns: [], | ||
resource: this, | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -319,6 +343,43 @@ export abstract class RepositoryBase extends Resource implements IRepository { | |
'ecr:UploadLayerPart', | ||
'ecr:CompleteLayerUpload'); | ||
} | ||
|
||
/** | ||
* Returns the account ID of the account to put into the Resource Policy | ||
* if we cannot put a reference to the principal itself there, | ||
* and 'undefined' in case we can. | ||
*/ | ||
private principalCannotBeSafelyAddedToResourcePolicy(grantee: iam.IGrantable): string | undefined { | ||
// A principal cannot be safely added to the Resource Policy of this repo if: | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these combine with AND or OR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And. Clarified in the comments. |
||
|
||
// condition #1 | ||
const principal = grantee.grantPrincipal; | ||
const principalAccount = principal.principalAccount; | ||
if (!principalAccount) { | ||
return undefined; | ||
} | ||
const repoAndPrincipalAccountCompare = Token.compareStrings(this.env.account, principalAccount); | ||
if (repoAndPrincipalAccountCompare === TokenComparison.BOTH_UNRESOLVED || | ||
repoAndPrincipalAccountCompare === TokenComparison.SAME) { | ||
return undefined; | ||
} | ||
|
||
// condition #2 | ||
if (!iam.principalIsOwnedResource(principal)) { | ||
return undefined; | ||
} | ||
|
||
// condition #3 | ||
const principalStack = Stack.of(principal); | ||
if (this.stack.dependencies.includes(principalStack)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
return undefined; | ||
} | ||
|
||
return principalAccount; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -491,7 +552,6 @@ export class Repository extends RepositoryBase { | |
}); | ||
} | ||
|
||
|
||
private static validateRepositoryName(physicalName: string) { | ||
const repositoryName = physicalName; | ||
if (!repositoryName || Token.isUnresolved(repositoryName)) { | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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:
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!