From ee29152fb97c2cb06967f0d0431ff1f98c91cdfc Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Fri, 3 Sep 2021 18:31:49 -0700 Subject: [PATCH] fix(ecr): grants for cross-account principals result in failed deployments 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 --- packages/@aws-cdk/aws-ecr/lib/repository.ts | 78 ++++- .../@aws-cdk/aws-ecr/test/repository.test.ts | 322 ++++++++++++++---- .../tag-parameter-container-image.test.ts | 71 ++-- packages/@aws-cdk/aws-iam/lib/group.ts | 2 +- packages/@aws-cdk/aws-iam/lib/index.ts | 1 + .../@aws-cdk/aws-iam/lib/managed-policy.ts | 2 +- .../@aws-cdk/aws-iam/lib/policy-statement.ts | 2 +- packages/@aws-cdk/aws-iam/lib/policy.ts | 2 +- packages/@aws-cdk/aws-iam/lib/principals.ts | 2 +- .../aws-iam/lib/{ => private}/util.ts | 4 +- packages/@aws-cdk/aws-iam/lib/role.ts | 4 +- packages/@aws-cdk/aws-iam/lib/user.ts | 2 +- packages/@aws-cdk/aws-iam/lib/utils.ts | 44 +++ packages/@aws-cdk/aws-kms/lib/key.ts | 41 +-- 14 files changed, 422 insertions(+), 155 deletions(-) rename packages/@aws-cdk/aws-iam/lib/{ => private}/util.ts (98%) create mode 100644 packages/@aws-cdk/aws-iam/lib/utils.ts diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts index af8547ba5edaf..d9e9df57c2d74 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository.ts +++ b/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'; @@ -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, + }); + } } /** @@ -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; + } } /** @@ -441,7 +502,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 index 37cc58a8485eb..c4eeedef17493 100644 --- a/packages/@aws-cdk/aws-ecr/test/repository.test.ts +++ b/packages/@aws-cdk/aws-ecr/test/repository.test.ts @@ -1,5 +1,5 @@ import { EOL } from 'os'; -import { expect as expectCDK, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert-internal'; +import { arrayWith, expect as expectCDK, haveResource, haveResourceLike, objectLike, ResourcePart } from '@aws-cdk/assert-internal'; import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import * as ecr from '../lib'; @@ -330,6 +330,36 @@ describe('repository', () => { expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/); }); + test('removal policy is "Retain" by default', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo'); + + // THEN + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Retain', + }, ResourcePart.CompleteDefinition)); + }); + + 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 + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Delete', + }, ResourcePart.CompleteDefinition)); + }); + describe('events', () => { test('onImagePushed without imageTag creates the correct event', () => { const stack = new cdk.Stack(); @@ -462,63 +492,6 @@ describe('repository', () => { 'State': 'ENABLED', })); }); - - test('removal policy is "Retain" by default', () => { - // GIVEN - const stack = new cdk.Stack(); - - // WHEN - new ecr.Repository(stack, 'Repo'); - - // THEN - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'Type': 'AWS::ECR::Repository', - 'DeletionPolicy': 'Retain', - }, ResourcePart.CompleteDefinition)); - }); - - 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 - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'Type': 'AWS::ECR::Repository', - 'DeletionPolicy': 'Delete', - }, ResourcePart.CompleteDefinition)); - }); - - test('grant adds appropriate resource-*', () => { - // GIVEN - const stack = new cdk.Stack(); - const repo = new ecr.Repository(stack, 'TestHarnessRepo'); - - // WHEN - repo.grantPull(new iam.AnyPrincipal()); - - // THEN - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'RepositoryPolicyText': { - 'Statement': [ - { - 'Action': [ - 'ecr:BatchCheckLayerAvailability', - 'ecr:GetDownloadUrlForLayer', - 'ecr:BatchGetImage', - ], - 'Effect': 'Allow', - 'Principal': { AWS: '*' }, - }, - ], - 'Version': '2012-10-17', - }, - })); - }); }); describe('repository name validation', () => { @@ -588,4 +561,237 @@ describe('repository', () => { })).toThrow(/must follow the specified pattern/); }); }); + + describe('permission grants', () => { + test('grant adds appropriate resource-*', () => { + // GIVEN + const stack = new cdk.Stack(); + const repo = new ecr.Repository(stack, 'TestHarnessRepo'); + + // WHEN + repo.grantPull(new iam.AnyPrincipal()); + + // THEN + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': { + 'Statement': [ + { + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { AWS: '*' }, + }, + ], + 'Version': '2012-10-17', + }, + })); + }); + + describe('across accounts', () => { + let repoStack: cdk.Stack; + let repo: ecr.IRepository; + + let principalStack: cdk.Stack; + let newPrincipal: iam.IGrantable; + let existingPrincipal: iam.IGrantable; + + beforeEach(() => { + const app = new cdk.App(); + + repoStack = new cdk.Stack(app, 'RepoStack', { + env: { + account: 'repo-account', + region: 'us-west-2', + }, + }); + repo = new ecr.Repository(repoStack, 'TestHarnessRepo', { + repositoryName: cdk.PhysicalName.GENERATE_IF_NEEDED, + }); + + principalStack = new cdk.Stack(app, 'PrincipalStack', { + env: { + account: 'principal-account', + region: 'us-west-2', + }, + }); + newPrincipal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AccountRootPrincipal(), + roleName: cdk.PhysicalName.GENERATE_IF_NEEDED, + }); + existingPrincipal = iam.Role.fromRoleArn(principalStack, 'ImportedRole', + 'arn:aws:iam::principal-account:role/my-principal-role'); + }); + + test("puts the new principal in the repo's resource policy if the principal Stack's is a dependency of its Stack", () => { + // GIVEN + repoStack.addDependency(principalStack); + + // WHEN + repo.grantPull(newPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':iam::principal-account:role/principalstackincipalstackrole7c6dfa81516e80f323e3', + ]], + }, + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + + test("puts the new principal's account in the repo's resource policy if the principal Stack's is _not_ a dependency of its Stack", () => { + // GIVEN + // we do _not_ add a dependency on principalStack to repoStack + + // WHEN + repo.grantPull(newPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':iam::principal-account:root', + ]], + }, + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + + test("puts the existing principal in the repo's resource policy, even if the principal Stack's is not a dependency of its Stack", () => { + // GIVEN + // we do _not_ add a dependency on principalStack to repoStack + + // WHEN + repo.grantPull(existingPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': 'arn:aws:iam::principal-account:role/my-principal-role', + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + }); + }); }); 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 index 3f409739d76ed..ce047e5043fc7 100644 --- 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 @@ -1,5 +1,5 @@ import '@aws-cdk/assert-internal/jest'; -import { SynthUtils } from '@aws-cdk/assert-internal'; +import { arrayWith, objectLike, SynthUtils } from '@aws-cdk/assert-internal'; import * as ecr from '@aws-cdk/aws-ecr'; import * as cdk from '@aws-cdk/core'; import * as ecs from '../../lib'; @@ -82,55 +82,40 @@ describe('tag parameter container image', () => { 'Fn::Join': ['', [ 'arn:', { Ref: 'AWS::Partition' }, - ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4', + ':iam::service-account:root', ]], }, }, }], }, }); - expect(serviceStack).toHaveResourceLike('AWS::IAM::Role', { - RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4', - }); - expect(serviceStack).toHaveResourceLike('AWS::ECS::TaskDefinition', { - ContainerDefinitions: [ - { - 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' }, - ]], - }, - }, - ], - }); - + expect(serviceStack).toHaveResourceLike('AWS::IAM::Policy', { + PolicyDocument: objectLike({ + Statement: arrayWith( + 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}`, + ]], + }, + }), + objectLike({ + Action: 'ecr:GetAuthorizationToken', + Effect: 'Allow', + Resource: '*', + }), + ), + }), + }); }); }); }); diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index eca266f6975dd..0c70bc9be739b 100644 --- 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 index 06c2a9bb6cdcd..ce737fbd2f670 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -13,6 +13,7 @@ export * from './unknown-principal'; export * from './oidc-provider'; export * from './permissions-boundary'; export * from './saml-provider'; +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 index cc8bddc0e8a17..db5f5316b02f6 100644 --- a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts +++ b/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 diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 14ca172de5506..62175666b3f7e 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -4,7 +4,7 @@ import { AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, CanonicalUserPrincipal, FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, } from './principals'; -import { mergePrincipal } from './util'; +import { mergePrincipal } 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 index 60862dca07c56..8278b472f35fd 100644 --- 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 index 2c89f96749324..151d135830f52 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -2,8 +2,8 @@ import * as cdk from '@aws-cdk/core'; import { Default, RegionInfo } from '@aws-cdk/region-info'; import { IOpenIdConnectProvider } from './oidc-provider'; import { Condition, Conditions, PolicyStatement } from './policy-statement'; +import { mergePrincipal } from './private/util'; import { ISamlProvider } from './saml-provider'; -import { 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/util.ts b/packages/@aws-cdk/aws-iam/lib/private/util.ts similarity index 98% rename from packages/@aws-cdk/aws-iam/lib/util.ts rename to packages/@aws-cdk/aws-iam/lib/private/util.ts index 19fcbffe09639..929848837ffd0 100644 --- a/packages/@aws-cdk/aws-iam/lib/util.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/util.ts @@ -1,6 +1,6 @@ import { captureStackTrace, DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, Lazy, StringConcat, Token, Tokenization } from '@aws-cdk/core'; import { IConstruct } from 'constructs'; -import { IPolicy } from './policy'; +import { IPolicy } from '../policy'; const MAX_POLICY_NAME_LEN = 128; @@ -123,4 +123,4 @@ export class UniqueStringSet implements IResolvable, IPostProcessor { public toString(): string { return Token.asString(this); } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index f142940b2b3ef..3d2155fa0675a 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -9,7 +9,7 @@ import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; import { ImmutableRole } from './private/immutable-role'; -import { AttachedPolicies, UniqueStringSet } from './util'; +import { AttachedPolicies, UniqueStringSet } from './private/util'; /** * Properties for defining an IAM Role @@ -540,4 +540,4 @@ export interface WithoutPolicyUpdatesOptions { * @default false */ readonly addGrantsToResources?: boolean; -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 4874e84f791df..975b9d9a8aed4 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -7,7 +7,7 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; -import { AttachedPolicies, undefinedIfEmpty } from './util'; +import { AttachedPolicies, undefinedIfEmpty } from './private/util'; /** * Represents an IAM user diff --git a/packages/@aws-cdk/aws-iam/lib/utils.ts b/packages/@aws-cdk/aws-iam/lib/utils.ts new file mode 100644 index 0000000000000..a35481ba5f931 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/utils.ts @@ -0,0 +1,44 @@ +import { Construct, IConstruct } from 'constructs'; +import { Group } from './group'; +import { IPrincipal } from './principals'; +import { Role } from './role'; +import { User } from './user'; + +/** + * Determines whether the given Principal is a newly created resource managed by the CDK, + * or if it's only an imported existing resource. + * + * @param principal the Principal to check + * @returns true if the Principal is a newly created resource, false otherwise. + * Additionally, the type of the principal will now also be IConstruct + * (because a newly created resource cannot possibly not be a construct) + */ +export function principalIsANewlyCreatedResource(principal: IPrincipal): principal is IPrincipal & IConstruct { + // a newly created resource will for sure be a construct + if (!isConstruct(principal)) { + return false; + } + + // yes, this sucks + // this is just a temporary stopgap to stem the bleeding while we work on a proper fix + return principal instanceof Role || + principal instanceof User || + principal instanceof Group; +} + +/** + * Whether the given object is a Construct + * + * Normally we'd do `x instanceof Construct`, but that is not robust against + * multiple copies of the `constructs` library on disk. This can happen + * when upgrading and downgrading between v2 and v1, and in the use of CDK + * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in + * a cross-environment fashion", which is very confusing. + */ +function isConstruct(x: any): x is IConstruct { + const sym = Symbol.for('constructs.Construct.node'); + return (typeof x === 'object' && x && + (x instanceof Construct // happy fast case + || !!(x as any).node // constructs v10 + || !!(x as any)[sym])); // constructs v3 +} diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index aae71efb460a7..2e434364e59db 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -1,7 +1,7 @@ import * as iam from '@aws-cdk/aws-iam'; import { FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Duration } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; -import { IConstruct, Construct } from 'constructs'; +import { Construct } from 'constructs'; import { Alias } from './alias'; import { CfnKey } from './kms.generated'; import * as perms from './private/perms'; @@ -201,15 +201,11 @@ abstract class KeyBase extends Resource implements IKey { */ private granteeStackDependsOnKeyStack(grantee: iam.IGrantable): string | undefined { const grantPrincipal = grantee.grantPrincipal; - if (!isConstruct(grantPrincipal)) { - return undefined; - } // this logic should only apply to newly created // (= not imported) resources - if (!this.principalIsANewlyCreatedResource(grantPrincipal)) { + if (!iam.principalIsANewlyCreatedResource(grantPrincipal)) { return undefined; } - // return undefined; const keyStack = Stack.of(this); const granteeStack = Stack.of(grantPrincipal); if (keyStack === granteeStack) { @@ -220,29 +216,21 @@ abstract class KeyBase extends Resource implements IKey { : undefined; } - private principalIsANewlyCreatedResource(principal: IConstruct): boolean { - // yes, this sucks - // this is just a temporary stopgap to stem the bleeding while we work on a proper fix - return principal instanceof iam.Role || - principal instanceof iam.User || - principal instanceof iam.Group; - } - private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsANewlyCreatedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.region !== identityStack.region; } private isGranteeFromAnotherAccount(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsANewlyCreatedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.account !== identityStack.account; } } @@ -675,20 +663,3 @@ export class Key extends KeyBase { })); } } - -/** - * Whether the given object is a Construct - * - * Normally we'd do `x instanceof Construct`, but that is not robust against - * multiple copies of the `constructs` library on disk. This can happen - * when upgrading and downgrading between v2 and v1, and in the use of CDK - * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in - * a cross-environment fashion", which is very confusing. - */ -function isConstruct(x: any): x is Construct { - const sym = Symbol.for('constructs.Construct.node'); - return (typeof x === 'object' && x && - (x instanceof Construct // happy fast case - || !!(x as any).node // constructs v10 - || !!(x as any)[sym])); // constructs v3 -} \ No newline at end of file