Skip to content

Commit

Permalink
fix(ecr): grants for cross-account principals result in failed deploy…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
skinny85 committed Sep 4, 2021
1 parent 6e55c95 commit ee29152
Show file tree
Hide file tree
Showing 14 changed files with 422 additions and 155 deletions.
78 changes: 69 additions & 9 deletions packages/@aws-cdk/aws-ecr/lib/repository.ts
@@ -1,7 +1,7 @@
import { EOL } from 'os';
import * as events from '@aws-cdk/aws-events';
import * as iam from '@aws-cdk/aws-iam';
import { IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core';
import { 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';
Expand Down Expand Up @@ -252,17 +252,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,
});
}
}

/**
Expand Down Expand Up @@ -292,6 +316,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.

// 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.principalIsANewlyCreatedResource(principal)) {
return undefined;
}

// condition #3
const principalStack = Stack.of(principal);
if (this.stack.dependencies.includes(principalStack)) {
return undefined;
}

return principalAccount;
}
}

/**
Expand Down Expand Up @@ -441,7 +502,6 @@ export class Repository extends RepositoryBase {
});
}


private static validateRepositoryName(physicalName: string) {
const repositoryName = physicalName;
if (!repositoryName || Token.isUnresolved(repositoryName)) {
Expand Down

0 comments on commit ee29152

Please sign in to comment.