fix(ecr): grants for cross-account principals result in failed deploy…

…ments

packages/@aws-cdk/aws-ecr/jest.config.js

@@ -5,6 +5,7 @@ module.exports = {
     global: {
       ...baseConfig.coverageThreshold.global,
       branches: 70,
+      statements: 79,
     },
   },
 };

packages/@aws-cdk/aws-ecr/lib/repository.ts

@@ -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.
+
+    // 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)) {
+      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)) {

packages/@aws-cdk/aws-ecr/test/repository.test.ts

@@ -444,6 +444,36 @@ describe('repository', () => {
     }).toThrow('encryptionKey is specified, so \'encryption\' must be set to KMS (value: AES256)');
   });
 
+  test('removal policy is "Retain" by default', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    // WHEN
+    new ecr.Repository(stack, 'Repo');
+
+    // THEN
+    Template.fromStack(stack).hasResource('AWS::ECR::Repository', {
+      'Type': 'AWS::ECR::Repository',
+      'DeletionPolicy': 'Retain',
+    });
+  });
+
+  test('"Delete" removal policy can be set explicitly', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    // WHEN
+    new ecr.Repository(stack, 'Repo', {
+      removalPolicy: cdk.RemovalPolicy.DESTROY,
+    });
+
+    // THEN
+    Template.fromStack(stack).hasResource('AWS::ECR::Repository', {
+      'Type': 'AWS::ECR::Repository',
+      'DeletionPolicy': 'Delete',
+    });
+  });
+
   describe('events', () => {
     test('onImagePushed without imageTag creates the correct event', () => {
       const stack = new cdk.Stack();
@@ -701,43 +731,43 @@ describe('repository', () => {
         repositoryName: 'a//a-a',
       })).toThrow(/must follow the specified pattern/);
     });
+  });
 
-    test('return value addToResourcePolicy', () => {
-      // GIVEN
-      const stack = new cdk.Stack();
-      const policyStmt1 = new iam.PolicyStatement({
-        actions: ['*'],
-        principals: [new iam.AnyPrincipal()],
-      });
-      const policyStmt2 = new iam.PolicyStatement({
-        effect: iam.Effect.DENY,
-        actions: ['ecr:BatchGetImage', 'ecr:GetDownloadUrlForLayer'],
-        principals: [new iam.AnyPrincipal()],
-      });
-      const policyText1 = '{"Statement": [{"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}], "Version": "2012-10-17"}';
-      const policyText2 = `{"Statement": [
-        {"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}},
-        {"Action": ["ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"], "Effect": "Deny", "Principal": {"AWS": "*"}}
-      ], "Version": "2012-10-17"}`;
+  test('return value addToResourcePolicy', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const policyStmt1 = new iam.PolicyStatement({
+      actions: ['*'],
+      principals: [new iam.AnyPrincipal()],
+    });
+    const policyStmt2 = new iam.PolicyStatement({
+      effect: iam.Effect.DENY,
+      actions: ['ecr:BatchGetImage', 'ecr:GetDownloadUrlForLayer'],
+      principals: [new iam.AnyPrincipal()],
+    });
+    const policyText1 = '{"Statement": [{"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}], "Version": "2012-10-17"}';
+    const policyText2 = `{"Statement": [
+      {"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}},
+      {"Action": ["ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"], "Effect": "Deny", "Principal": {"AWS": "*"}}
+    ], "Version": "2012-10-17"}`;
 
-      // WHEN
-      const artifact1 = new ecr.Repository(stack, 'Repo1').addToResourcePolicy(policyStmt1);
-      const repo = new ecr.Repository(stack, 'Repo2');
-      repo.addToResourcePolicy(policyStmt1);
-      const artifact2 =repo.addToResourcePolicy(policyStmt2);
+    // WHEN
+    const artifact1 = new ecr.Repository(stack, 'Repo1').addToResourcePolicy(policyStmt1);
+    const repo = new ecr.Repository(stack, 'Repo2');
+    repo.addToResourcePolicy(policyStmt1);
+    const artifact2 =repo.addToResourcePolicy(policyStmt2);
 
-      // THEN
-      expect(stack.resolve(artifact1.statementAdded)).toEqual(true);
-      expect(stack.resolve(artifact1.policyDependable)).toEqual(JSON.parse(policyText1));
-      Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', {
-        RepositoryPolicyText: JSON.parse(policyText1),
-      });
+    // THEN
+    expect(stack.resolve(artifact1.statementAdded)).toEqual(true);
+    expect(stack.resolve(artifact1.policyDependable)).toEqual(JSON.parse(policyText1));
+    Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', {
+      RepositoryPolicyText: JSON.parse(policyText1),
+    });
 
-      expect(stack.resolve(artifact2.statementAdded)).toEqual(true);
-      expect(stack.resolve(artifact2.policyDependable)).toEqual(JSON.parse(policyText2));
-      Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', {
-        RepositoryPolicyText: JSON.parse(policyText2),
-      });
+    expect(stack.resolve(artifact2.statementAdded)).toEqual(true);
+    expect(stack.resolve(artifact2.policyDependable)).toEqual(JSON.parse(policyText2));
+    Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', {
+      RepositoryPolicyText: JSON.parse(policyText2),
     });
   });
 });

packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts

@@ -77,52 +77,39 @@ describe('tag parameter container image', () => {
                 'Fn::Join': ['', [
                   'arn:',
                   { Ref: 'AWS::Partition' },
-                  ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4',
+                  ':iam::service-account:root',
                 ]],
               },
             },
           }],
         },
       });
-      Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Role', {
-        RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4',
-      });
-      Template.fromStack(serviceStack).hasResourceProperties('AWS::ECS::TaskDefinition', {
-        ContainerDefinitions: [
-          Match.objectLike({
-            Image: {
-              'Fn::Join': ['', [
-                {
-                  'Fn::Select': [4, {
-                    'Fn::Split': [':', {
-                      'Fn::Join': ['', [
-                        'arn:',
-                        { Ref: 'AWS::Partition' },
-                        `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`,
-                      ]],
-                    }],
-                  }],
-                },
-                '.dkr.ecr.',
-                {
-                  'Fn::Select': [3, {
-                    'Fn::Split': [':', {
-                      'Fn::Join': ['', [
-                        'arn:',
-                        { Ref: 'AWS::Partition' },
-                        `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`,
-                      ]],
-                    }],
-                  }],
-                },
-                '.',
-                { Ref: 'AWS::URLSuffix' },
-                `/${repositoryName}:`,
-                { Ref: 'ServiceTaskDefinitionContainerImageTagParamCEC9D0BA' },
-              ]],
-            },
-          }),
-        ],
+
+      Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Policy', {
+        PolicyDocument: Match.objectLike({
+          Statement: Match.arrayWith([
+            Match.objectLike({
+              Action: [
+                'ecr:BatchCheckLayerAvailability',
+                'ecr:GetDownloadUrlForLayer',
+                'ecr:BatchGetImage',
+              ],
+              Effect: 'Allow',
+              Resource: {
+                'Fn::Join': ['', [
+                  'arn:',
+                  { Ref: 'AWS::Partition' },
+                  `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`,
+                ]],
+              },
+            }),
+            Match.objectLike({
+              Action: 'ecr:GetAuthorizationToken',
+              Effect: 'Allow',
+              Resource: '*',
+            }),
+          ]),
+        }),
       });
     });
   });

packages/@aws-cdk/aws-iam/lib/group.ts

@@ -6,8 +6,8 @@ import { IManagedPolicy } from './managed-policy';
 import { Policy } from './policy';
 import { PolicyStatement } from './policy-statement';
 import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
+import { AttachedPolicies } from './private/util';
 import { IUser } from './user';
-import { AttachedPolicies } from './util';
 
 /**
  * Represents an IAM Group.

packages/@aws-cdk/aws-iam/lib/index.ts

@@ -14,6 +14,7 @@ export * from './oidc-provider';
 export * from './permissions-boundary';
 export * from './saml-provider';
 export * from './access-key';
+export * from './utils';
 
 // AWS::IAM CloudFormation Resources:
 export * from './iam.generated';

packages/@aws-cdk/aws-iam/lib/managed-policy.ts

@@ -4,9 +4,9 @@ import { IGroup } from './group';
 import { CfnManagedPolicy } from './iam.generated';
 import { PolicyDocument } from './policy-document';
 import { PolicyStatement } from './policy-statement';
+import { undefinedIfEmpty } from './private/util';
 import { IRole } from './role';
 import { IUser } from './user';
-import { undefinedIfEmpty } from './util';
 
 /**
  * A managed policy

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

@@ -6,7 +6,7 @@ import {
   FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, validateConditionObject,
 } from './principals';
 import { normalizeStatement } from './private/postprocess-policy-document';
-import { LITERAL_STRING_KEY, mergePrincipal, sum } from './util';
+import { LITERAL_STRING_KEY, mergePrincipal, sum } from './private/util';
 
 const ensureArrayOrUndefined = (field: any) => {
   if (field === undefined) {

packages/@aws-cdk/aws-iam/lib/policy.ts

@@ -4,9 +4,9 @@ import { IGroup } from './group';
 import { CfnPolicy } from './iam.generated';
 import { PolicyDocument } from './policy-document';
 import { PolicyStatement } from './policy-statement';
+import { generatePolicyName, undefinedIfEmpty } from './private/util';
 import { IRole } from './role';
 import { IUser } from './user';
-import { generatePolicyName, undefinedIfEmpty } from './util';
 
 /**
  * Represents an IAM Policy

packages/@aws-cdk/aws-iam/lib/principals.ts

@@ -5,8 +5,8 @@ import { IOpenIdConnectProvider } from './oidc-provider';
 import { PolicyDocument } from './policy-document';
 import { Condition, Conditions, PolicyStatement } from './policy-statement';
 import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy';
+import { LITERAL_STRING_KEY, mergePrincipal } from './private/util';
 import { ISamlProvider } from './saml-provider';
-import { LITERAL_STRING_KEY, mergePrincipal } from './util';
 
 /**
  * Any object that has an associated principal that a permission can be granted to