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

skinny85 · web-flow · commit 84e20f8da37c · 2022-12-09T07:26:34.000Z
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*
diff --git a/packages/@aws-cdk/aws-ecr/jest.config.js b/packages/@aws-cdk/aws-ecr/jest.config.js
@@ -5,6 +5,7 @@ module.exports = {
     global: {
       ...baseConfig.coverageThreshold.global,
       branches: 70,
+      statements: 78,
     },
   },
 };
diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/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, 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)) {
+      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)) {
diff --git a/packages/@aws-cdk/aws-ecr/test/repository.test.ts b/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();
diff --git a/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts b/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts
@@ -77,51 +77,52 @@ describe('tag parameter container image', () => {
                 'Fn::Join': ['', [
                   'arn:',
                   { Ref: 'AWS::Partition' },
-                  ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4',
+                  ':iam::service-account:root',
                 ]],
               },
             },
+            Condition: {
+              StringEquals: {
+                'aws:PrincipalTag/aws-cdk:id': 'ServiceStack_c8a38b9d3ed0e8d960dd0d679c0bab1612dafa96f5',
+              },
+            },
           }],
         },
       });
-      Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Role', {
-        RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4',
+
+      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: '*',
+            }),
+          ]),
+        }),
       });
-      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::Role', {
+        Tags: [
+          {
+            Key: 'aws-cdk:id',
+            Value: 'ServiceStack_c8a38b9d3ed0e8d960dd0d679c0bab1612dafa96f5',
+          },
         ],
       });
     });
diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/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.
diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/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';
diff --git a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts b/packages/@aws-cdk/aws-iam/lib/managed-policy.ts
@@ -5,9 +5,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
diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/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) {
diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/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
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -6,8 +6,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
diff --git a/packages/@aws-cdk/aws-iam/lib/private/util.ts b/packages/@aws-cdk/aws-iam/lib/private/util.ts
diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts
diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts
diff --git a/packages/@aws-cdk/aws-iam/lib/utils.ts b/packages/@aws-cdk/aws-iam/lib/utils.ts
diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts
