-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 all 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: 78, | ||
}, | ||
}, | ||
}; |
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, Tags, Token, TokenComparison } from '@aws-cdk/core'; | ||
import { IConstruct, Construct } from 'constructs'; | ||
import { CfnRepository } from './ecr.generated'; | ||
import { LifecycleRule, TagStatus } from './lifecycle'; | ||
|
@@ -279,17 +279,49 @@ 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 crossAccountPrincipal = this.unsafeCrossAccountResourcePolicyPrincipal(grantee); | ||
if (crossAccountPrincipal) { | ||
// 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). | ||
// To scope down the permissions as much as possible, | ||
// only trust principals from that account with a specific tag | ||
const crossAccountPrincipalStack = Stack.of(crossAccountPrincipal); | ||
const roleTag = `${crossAccountPrincipalStack.stackName}_${crossAccountPrincipal.node.addr}`; | ||
Tags.of(crossAccountPrincipal).add('aws-cdk:id', roleTag); | ||
this.addToResourcePolicy(new iam.PolicyStatement({ | ||
actions, | ||
principals: [new iam.AccountPrincipal(crossAccountPrincipalStack.account)], | ||
conditions: { | ||
StringEquals: { 'aws:PrincipalTag/aws-cdk:id': roleTag }, | ||
}, | ||
})); | ||
|
||
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 +351,43 @@ export abstract class RepositoryBase extends Resource implements IRepository { | |
'ecr:UploadLayerPart', | ||
'ecr:CompleteLayerUpload'); | ||
} | ||
|
||
/** | ||
* Returns the resource that backs the given IAM grantee if we cannot put a direct reference | ||
* to the grantee in the resource policy of this ECR repository, | ||
* and 'undefined' in case we can. | ||
*/ | ||
private unsafeCrossAccountResourcePolicyPrincipal(grantee: iam.IGrantable): IConstruct | undefined { | ||
// A principal cannot be safely added to the Resource Policy of this ECR repository, if: | ||
// 1. The principal is from a different account, and | ||
// 2. The principal is a new resource (meaning, not just referenced), and | ||
// 3. The Stack this repo belongs to doesn't depend on the Stack the principal belongs to. | ||
|
||
// 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. 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 principal; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -491,7 +560,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.
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 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.