From e44dc185e1d1a8b56e68b8ab56299845dcd6908d Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Thu, 16 Apr 2020 13:31:51 -0700 Subject: [PATCH] Add account and region to (I)Resource. --- .../test/s3/test.s3-deploy-action.ts | 34 ++- .../@aws-cdk/aws-codepipeline/lib/pipeline.ts | 22 +- .../aws-dynamodb/test/dynamodb.test.ts | 2 +- packages/@aws-cdk/aws-ec2/lib/bastion-host.ts | 7 +- packages/@aws-cdk/aws-ec2/package.json | 1 + .../@aws-cdk/aws-ec2/test/userdata.test.ts | 4 +- .../integ.load-balanced-fargate-service.ts | 10 +- .../test.load-balanced-fargate-service.ts | 17 +- .../aws-eks-legacy/lib/kubectl-layer.ts | 8 +- .../@aws-cdk/aws-eks/lib/kubectl-layer.ts | 8 +- packages/@aws-cdk/aws-iam/lib/grant.ts | 21 +- packages/@aws-cdk/aws-iam/lib/group.ts | 5 +- packages/@aws-cdk/aws-iam/lib/lazy-role.ts | 1 + .../@aws-cdk/aws-iam/lib/oidc-provider.ts | 7 +- packages/@aws-cdk/aws-iam/lib/principals.ts | 10 + .../aws-iam/lib/private/immutable-role.ts | 12 +- packages/@aws-cdk/aws-iam/lib/role.ts | 42 ++-- packages/@aws-cdk/aws-iam/lib/user.ts | 4 +- packages/@aws-cdk/aws-iam/package.json | 1 + .../aws-iam/test/cross-account.test.ts | 233 ++++++++++++++++++ packages/@aws-cdk/aws-iam/test/grant.test.ts | 13 +- .../@aws-cdk/aws-lambda/lib/function-base.ts | 2 + packages/@aws-cdk/aws-lambda/lib/function.ts | 14 +- .../@aws-cdk/aws-route53/test/test.util.ts | 29 +-- packages/@aws-cdk/aws-s3/lib/bucket.ts | 53 ++-- packages/@aws-cdk/aws-s3/test/test.util.ts | 35 +-- packages/@aws-cdk/core/lib/arn.ts | 10 +- packages/@aws-cdk/core/lib/cfn-pseudo.ts | 7 +- .../core/lib/environment-component.ts | 130 ++++++++++ packages/@aws-cdk/core/lib/index.ts | 1 + packages/@aws-cdk/core/lib/resource.ts | 32 +++ packages/@aws-cdk/core/test/test.arn.ts | 21 +- 32 files changed, 623 insertions(+), 173 deletions(-) create mode 100644 packages/@aws-cdk/aws-iam/test/cross-account.test.ts create mode 100644 packages/@aws-cdk/core/lib/environment-component.ts diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/s3/test.s3-deploy-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/s3/test.s3-deploy-action.ts index 01ebb43dca605..6ecca100f33ef 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/s3/test.s3-deploy-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/s3/test.s3-deploy-action.ts @@ -1,7 +1,7 @@ import { expect, haveResourceLike } from '@aws-cdk/assert'; import * as codepipeline from '@aws-cdk/aws-codepipeline'; import * as s3 from '@aws-cdk/aws-s3'; -import { Duration, SecretValue, Stack } from '@aws-cdk/core'; +import { App, Duration, SecretValue, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import * as cpactions from '../../lib'; @@ -153,6 +153,38 @@ export = { test.done(); }, + + 'correctly makes the action cross-region for a Bucket imported with a different region'(test: Test) { + const app = new App(); + const stack = new Stack(app, 'PipelineStack', { + env: { account: '123456789012', region: 'us-west-2' }, + }); + const deployBucket = s3.Bucket.fromBucketAttributes(stack, 'DeployBucket', { + bucketName: 'my-deploy-bucket', + region: 'ap-southeast-1', + }); + + minimalPipeline(stack, { + bucket: deployBucket, + }); + + expect(stack).to(haveResourceLike('AWS::CodePipeline::Pipeline', { + Stages: [ + {}, + { + Name: 'Deploy', + Actions: [ + { + Name: 'CopyFiles', + Region: 'ap-southeast-1', + }, + ], + }, + ], + })); + + test.done(); + }, }, }; diff --git a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts index b498c20945f83..bdff88fddc95b 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts @@ -3,7 +3,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; import * as s3 from '@aws-cdk/aws-s3'; import { - App, BootstraplessSynthesizer, Construct, DefaultStackSynthesizer, + App, BootstraplessSynthesizer, Construct, DefaultStackSynthesizer, EnvComponentComparison, IStackSynthesizer, Lazy, PhysicalName, RemovalPolicy, Resource, Stack, Token, } from '@aws-cdk/core'; import { ActionCategory, IAction, IPipeline, IStage } from './action'; @@ -400,12 +400,20 @@ export class Pipeline extends PipelineBase { const actionResource = action.actionProperties.resource; if (actionResource) { - const actionResourceStack = Stack.of(actionResource); - if (pipelineStack.region !== actionResourceStack.region) { - actionRegion = actionResourceStack.region; + const pipelineAndActionRegionComparisonResult = this.environment.region.compare(actionResource.environment.region); + const pipelineAndActionInDifferentRegions = pipelineAndActionRegionComparisonResult === EnvComponentComparison.ONE_UNRESOLVED || + pipelineAndActionRegionComparisonResult === EnvComponentComparison.DIFFERENT; + if (pipelineAndActionInDifferentRegions) { + actionRegion = actionResource.environment.region.toString(); + // if the resource is from a different stack in another region but the same account, // use that stack as home for the cross-region support resources - if (pipelineStack.account === actionResourceStack.account) { + const actionResourceStack = Stack.of(actionResource); + const actionResourceAndItsStackRegionComparisonResult = actionResource.environment.region.compareToString(actionResourceStack.region); + const actionResourceInSameRegionAsItsStack = actionResourceAndItsStackRegionComparisonResult === EnvComponentComparison.SAME || + actionResourceAndItsStackRegionComparisonResult === EnvComponentComparison.BOTH_UNRESOLVED; + if (pipelineStack.account === actionResourceStack.account && + actionResourceInSameRegionAsItsStack) { otherStack = actionResourceStack; } } @@ -850,7 +858,7 @@ export class Pipeline extends PipelineBase { } private requireRegion(): string { - const region = Stack.of(this).region; + const region = this.environment.region.toString(); if (Token.isUnresolved(region)) { throw new Error('Pipeline stack which uses cross-environment actions must have an explicitly set region'); } @@ -932,4 +940,4 @@ class PipelineLocation { // runOrders are 1-based, so make the stageIndex also 1-based otherwise it's going to be confusing. return `Stage ${this.stageIndex + 1} Action ${this.action.runOrder} ('${this.stageName}'/'${this.actionName}')`; } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts b/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts index 754a30150ace5..4b6b7e6c17593 100644 --- a/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts +++ b/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts @@ -2106,7 +2106,7 @@ describe('import', () => { 'Roles': [{ 'Ref': 'NewRole99763075' }], }); - expect(table.tableArn).toBe('arn:${Token[AWS::Partition.3]}:dynamodb:${Token[AWS::Region.4]}:${Token[AWS::AccountId.0]}:table/MyTable'); + expect(table.tableArn).toBe('arn:${Token[AWS.Partition.3]}:dynamodb:${Token[AWS.Region.4]}:${Token[AWS.AccountId.0]}:table/MyTable'); expect(stack.resolve(table.tableName)).toBe(tableName); }); diff --git a/packages/@aws-cdk/aws-ec2/lib/bastion-host.ts b/packages/@aws-cdk/aws-ec2/lib/bastion-host.ts index 00b6cd396ddf9..9795551abf5a4 100644 --- a/packages/@aws-cdk/aws-ec2/lib/bastion-host.ts +++ b/packages/@aws-cdk/aws-ec2/lib/bastion-host.ts @@ -1,5 +1,5 @@ import { IPrincipal, IRole, PolicyStatement } from '@aws-cdk/aws-iam'; -import { CfnOutput, Construct, Stack } from '@aws-cdk/core'; +import { CfnOutput, Construct, Resource, Stack } from '@aws-cdk/core'; import { AmazonLinuxGeneration, InstanceClass, InstanceSize, InstanceType } from '.'; import { Connections } from './connections'; import { IInstance, Instance } from './instance'; @@ -90,8 +90,9 @@ export interface BastionHostLinuxProps { * You can also configure this bastion host to allow connections via SSH * * @experimental + * @resource AWS::EC2::Instance */ -export class BastionHostLinux extends Construct implements IInstance { +export class BastionHostLinux extends Resource implements IInstance { public readonly stack: Stack; /** @@ -192,4 +193,4 @@ export class BastionHostLinux extends Construct implements IInstance { this.connections.allowFrom(p, Port.tcp(22), 'SSH access'); }); } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ec2/package.json b/packages/@aws-cdk/aws-ec2/package.json index ad6075cc5ec77..0a48f2620252d 100644 --- a/packages/@aws-cdk/aws-ec2/package.json +++ b/packages/@aws-cdk/aws-ec2/package.json @@ -117,6 +117,7 @@ "props-physical-name:@aws-cdk/aws-ec2.InterfaceVpcEndpointProps", "from-method:@aws-cdk/aws-ec2.Instance", "from-method:@aws-cdk/aws-ec2.VpcEndpointService", + "attribute-tag:@aws-cdk/aws-ec2.BastionHostLinux.instance", "attribute-tag:@aws-cdk/aws-ec2.Instance.instance", "from-signature:@aws-cdk/aws-ec2.SecurityGroup.fromSecurityGroupId", "docs-public-apis:@aws-cdk/aws-ec2.WindowsVersion.WINDOWS_SERVER_2016_ENGLISH_DEEP_LEARNING", diff --git a/packages/@aws-cdk/aws-ec2/test/userdata.test.ts b/packages/@aws-cdk/aws-ec2/test/userdata.test.ts index 5be98eb41668c..bda3c6cc71ddc 100644 --- a/packages/@aws-cdk/aws-ec2/test/userdata.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/userdata.test.ts @@ -52,7 +52,7 @@ nodeunitShim({ test.equals(rendered, 'trap {\n' + '$success=($PSItem.Exception.Message -eq "Success")\n' + - 'cfn-signal --stack Default --resource RESOURCE1989552F --region ${Token[AWS::Region.4]} --success ($success.ToString().ToLower())\n' + + 'cfn-signal --stack Default --resource RESOURCE1989552F --region ${Token[AWS.Region.4]} --success ($success.ToString().ToLower())\n' + 'break\n' + '}\n' + 'command1\n' + @@ -157,7 +157,7 @@ nodeunitShim({ test.equals(rendered, '#!/bin/bash\n' + 'function exitTrap(){\n' + 'exitCode=$?\n' + - '/opt/aws/bin/cfn-signal --stack Default --resource RESOURCE1989552F --region ${Token[AWS::Region.4]} -e $exitCode || echo \'Failed to send Cloudformation Signal\'\n' + + '/opt/aws/bin/cfn-signal --stack Default --resource RESOURCE1989552F --region ${Token[AWS.Region.4]} -e $exitCode || echo \'Failed to send Cloudformation Signal\'\n' + '}\n' + 'trap exitTrap EXIT\n' + 'command1'); diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.load-balanced-fargate-service.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.load-balanced-fargate-service.ts index f4ecc0f0a5616..322d5f684c6dc 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.load-balanced-fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.load-balanced-fargate-service.ts @@ -1,6 +1,7 @@ import { Vpc } from '@aws-cdk/aws-ec2'; import { Cluster, ContainerImage } from '@aws-cdk/aws-ecs'; import { ApplicationProtocol } from '@aws-cdk/aws-elasticloadbalancingv2'; +import * as route53 from '@aws-cdk/aws-route53'; import { App, Stack } from '@aws-cdk/core'; import { ApplicationLoadBalancedFargateService } from '../../lib'; @@ -20,13 +21,10 @@ new ApplicationLoadBalancedFargateService(stack, 'myService', { protocol: ApplicationProtocol.HTTPS, enableECSManagedTags: true, domainName: 'test.example.com', - domainZone: { + domainZone: route53.HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'example.com.', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }, + }), }); -app.synth(); \ No newline at end of file +app.synth(); diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts index 4b30dfa721480..0c4c0587eb081 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts @@ -3,6 +3,7 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import * as ecs from '@aws-cdk/aws-ecs'; import { ApplicationLoadBalancer, ApplicationProtocol, NetworkLoadBalancer } from '@aws-cdk/aws-elasticloadbalancingv2'; import * as iam from '@aws-cdk/aws-iam'; +import * as route53 from '@aws-cdk/aws-route53'; import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; import * as ecsPatterns from '../../lib'; @@ -370,13 +371,10 @@ export = { cluster, protocol: ApplicationProtocol.HTTPS, domainName: 'domain.com', - domainZone: { + domainZone: route53.HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'domain.com', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }, + }), taskImageOptions: { containerPort: 2015, image: ecs.ContainerImage.fromRegistry('abiosoft/caddy'), @@ -408,13 +406,10 @@ export = { cluster, protocol: ApplicationProtocol.HTTPS, domainName: 'test.domain.com', - domainZone: { + domainZone: route53.HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'domain.com.', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }, + }), taskImageOptions: { containerPort: 2015, image: ecs.ContainerImage.fromRegistry('abiosoft/caddy'), @@ -642,4 +637,4 @@ export = { test.done(); }, -}; \ No newline at end of file +}; diff --git a/packages/@aws-cdk/aws-eks-legacy/lib/kubectl-layer.ts b/packages/@aws-cdk/aws-eks-legacy/lib/kubectl-layer.ts index 73168622e4ff4..03b3fda0324f2 100644 --- a/packages/@aws-cdk/aws-eks-legacy/lib/kubectl-layer.ts +++ b/packages/@aws-cdk/aws-eks-legacy/lib/kubectl-layer.ts @@ -1,5 +1,5 @@ import * as lambda from '@aws-cdk/aws-lambda'; -import { CfnResource, Construct, Stack, Token } from '@aws-cdk/core'; +import { CfnResource, Construct, Resource, Stack, Token } from '@aws-cdk/core'; import * as crypto from 'crypto'; const KUBECTL_APP_ARN = 'arn:aws:serverlessrepo:us-east-1:903779448426:applications/lambda-layer-kubectl'; @@ -19,7 +19,7 @@ export interface KubectlLayerProps { * * @see https://github.com/aws-samples/aws-lambda-layer-kubectl */ -export class KubectlLayer extends Construct implements lambda.ILayerVersion { +export class KubectlLayer extends Resource implements lambda.ILayerVersion { /** * Gets or create a singleton instance of this construct. @@ -68,10 +68,6 @@ export class KubectlLayer extends Construct implements lambda.ILayerVersion { this.layerVersionArn = Token.asString(resource.getAtt('Outputs.LayerVersionArn')); } - public get stack() { - return Stack.of(this); - } - public addPermission(_id: string, _permission: lambda.LayerVersionPermission): void { return; } diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts b/packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts index 84efa5325854a..e58773989e1c9 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts @@ -1,5 +1,5 @@ import * as lambda from '@aws-cdk/aws-lambda'; -import { CfnResource, Construct, Stack, Token } from '@aws-cdk/core'; +import { CfnResource, Construct, Resource, Stack, Token } from '@aws-cdk/core'; import * as crypto from 'crypto'; const KUBECTL_APP_ARN = 'arn:aws:serverlessrepo:us-east-1:903779448426:applications/lambda-layer-kubectl'; @@ -21,7 +21,7 @@ export interface KubectlLayerProps { * * @see https://github.com/aws-samples/aws-lambda-layer-kubectl */ -export class KubectlLayer extends Construct implements lambda.ILayerVersion { +export class KubectlLayer extends Resource implements lambda.ILayerVersion { /** * Gets or create a singleton instance of this construct. @@ -70,10 +70,6 @@ export class KubectlLayer extends Construct implements lambda.ILayerVersion { this.layerVersionArn = Token.asString(resource.getAtt('Outputs.LayerVersionArn')); } - public get stack() { - return Stack.of(this); - } - public addPermission(_id: string, _permission: lambda.LayerVersionPermission): void { return; } diff --git a/packages/@aws-cdk/aws-iam/lib/grant.ts b/packages/@aws-cdk/aws-iam/lib/grant.ts index c0cb065f7ee2c..55d1408b6dfe5 100644 --- a/packages/@aws-cdk/aws-iam/lib/grant.ts +++ b/packages/@aws-cdk/aws-iam/lib/grant.ts @@ -121,7 +121,22 @@ export class Grant implements cdk.IDependable { scope: options.resource, }); - if (result.success) { return result; } + const compareResult = options.grantee.grantPrincipal.principalAccount + ? options.resource.environment.account.compareToString(options.grantee.grantPrincipal.principalAccount) + : undefined; + // if both accounts are tokens, we assume here they are the same + const equalOrBothUnresolved = compareResult === cdk.EnvComponentComparison.SAME + || compareResult == cdk.EnvComponentComparison.BOTH_UNRESOLVED; + const sameAccount: boolean = compareResult + ? equalOrBothUnresolved + // if the principal doesn't have an account (for example, a service principal), + // we shouldn't modify the resource policy for it + : true; + // If we added to the principal AND we're in the same account, then we're done. + // If not, it's a different account and we must also add a trust policy on the resource. + if (result.success && sameAccount) { + return result; + } const statement = new PolicyStatement({ actions: options.actions, @@ -292,7 +307,7 @@ interface GrantProps { /** * A resource with a resource policy that can be added to */ -export interface IResourceWithPolicy extends cdk.IConstruct { +export interface IResourceWithPolicy extends cdk.IResource { /** * Add a statement to the resource's resource policy */ @@ -332,4 +347,4 @@ export class CompositeDependable implements cdk.IDependable { }, }); } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index 08ef30cba9203..38e706ae7c247 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -72,6 +72,7 @@ abstract class GroupBase extends Resource implements IGroup { public abstract readonly groupArn: string; public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount: string | undefined = this.environment.account.toString(); public readonly assumeRoleAction: string = 'sts:AssumeRole'; private readonly attachedPolicies = new AttachedPolicies(); @@ -143,10 +144,12 @@ export class Group extends GroupBase { * @param groupArn the ARN of the group to import (e.g. `arn:aws:iam::account-id:group/group-name`) */ public static fromGroupArn(scope: Construct, id: string, groupArn: string): IGroup { - const groupName = Stack.of(scope).parseArn(groupArn).resourceName!; + const arnComponents = Stack.of(scope).parseArn(groupArn); + const groupName = arnComponents.resourceName!; class Import extends GroupBase { public groupName = groupName; public groupArn = groupArn; + public principalAccount = arnComponents.account; } return new Import(scope, id); diff --git a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts index febb6372d25e6..748e805d39740 100644 --- a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts @@ -26,6 +26,7 @@ export interface LazyRoleProps extends RoleProps { */ export class LazyRole extends cdk.Resource implements IRole { public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount: string | undefined = this.environment.account.toString(); public readonly assumeRoleAction: string = 'sts:AssumeRole'; private role?: Role; diff --git a/packages/@aws-cdk/aws-iam/lib/oidc-provider.ts b/packages/@aws-cdk/aws-iam/lib/oidc-provider.ts index b8345827699d1..352c6a7bf70b0 100644 --- a/packages/@aws-cdk/aws-iam/lib/oidc-provider.ts +++ b/packages/@aws-cdk/aws-iam/lib/oidc-provider.ts @@ -1,5 +1,5 @@ import * as path from 'path'; -import { Construct, CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, IResource, Resource, Stack, Token } from '@aws-cdk/core'; +import { Construct, CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, IResource, Resource, Token } from '@aws-cdk/core'; const RESOURCE_TYPE = 'Custom::AWSCDKOpenIdConnectProvider'; @@ -88,8 +88,9 @@ export interface OpenIdConnectProviderProps { * @see https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_providers_oidc.html * * @experimental + * @resource AWS::CloudFormation::CustomResource */ -export class OpenIdConnectProvider extends Construct implements IOpenIdConnectProvider { +export class OpenIdConnectProvider extends Resource implements IOpenIdConnectProvider { /** * Imports an Open ID connect provider from an ARN. * @param scope The definition scope @@ -130,8 +131,6 @@ export class OpenIdConnectProvider extends Construct implements IOpenIdConnectPr this.openIdConnectProviderArn = Token.asString(resource.ref); } - public get stack() { return Stack.of(this); } - private getOrCreateProvider() { return CustomResourceProvider.getOrCreate(this, RESOURCE_TYPE, { codeDirectory: path.join(__dirname, 'oidc-provider'), diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 53e7aca1968d7..ee4cdf0c43e27 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -42,6 +42,15 @@ export interface IPrincipal extends IGrantable { */ readonly policyFragment: PrincipalPolicyFragment; + /** + * The AWS account ID of this principal. + * Can be undefined when the account is not known + * (for example, for service principals). + * Can be a Token - in that case, + * it's assumed to be AWS::AccountId. + */ + readonly principalAccount?: string; + /** * Add to the policy of this principal. * @@ -83,6 +92,7 @@ export interface AddToPrincipalPolicyResult { */ export abstract class PrincipalBase implements IPrincipal { public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount: string | undefined = undefined; /** * Return the policy fragment that identifies this principal in a Policy. diff --git a/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts b/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts index 92aed748514d5..7479d48b65d04 100644 --- a/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts @@ -1,4 +1,4 @@ -import { ConcreteDependable, Construct, DependableTrait } from '@aws-cdk/core'; +import { ConcreteDependable, Construct, DependableTrait, Resource } from '@aws-cdk/core'; import { Grant } from '../grant'; import { IManagedPolicy } from '../managed-policy'; import { Policy } from '../policy'; @@ -19,16 +19,20 @@ import { IRole } from '../role'; * which was imported into the CDK with {@link Role.fromRoleArn}, you don't have to use this class - * simply pass the property mutable = false when calling {@link Role.fromRoleArn}. */ -export class ImmutableRole extends Construct implements IRole { +export class ImmutableRole extends Resource implements IRole { public readonly assumeRoleAction = this.role.assumeRoleAction; public readonly policyFragment = this.role.policyFragment; public readonly grantPrincipal = this; + public readonly principalAccount = this.role.principalAccount; public readonly roleArn = this.role.roleArn; public readonly roleName = this.role.roleName; public readonly stack = this.role.stack; constructor(scope: Construct, id: string, private readonly role: IRole) { - super(scope, id); + super(scope, id, { + account: role.environment.account.toString(), + region: role.environment.region.toString(), + }); // implement IDependable privately DependableTrait.implement(this, { @@ -60,4 +64,4 @@ export class ImmutableRole extends Construct implements IRole { public grantPassRole(grantee: IPrincipal): Grant { return this.role.grantPassRole(grantee); } -} \ 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 8b62425be50c6..dbac3bd407b91 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -1,4 +1,4 @@ -import { Construct, Duration, Lazy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Construct, Duration, EnvComponentComparison, Lazy, Resource, Stack } from '@aws-cdk/core'; import { Grant } from './grant'; import { CfnRole } from './iam.generated'; import { IIdentity } from './identity-base'; @@ -176,6 +176,7 @@ export class Role extends Resource implements IRole { const scopeStack = Stack.of(scope); const parsedArn = scopeStack.parseArn(roleArn); const resourceName = parsedArn.resourceName!; + const roleAccount = parsedArn.account; // service roles have an ARN like 'arn:aws:iam:::role/service-role/' // or 'arn:aws:iam:::role/service-role/servicename.amazonaws.com/service-role/' // we want to support these as well, so we just use the element after the last slash as role name @@ -183,6 +184,7 @@ export class Role extends Resource implements IRole { class Import extends Resource implements IRole { public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount = roleAccount; public readonly assumeRoleAction: string = 'sts:AssumeRole'; public readonly policyFragment = new ArnPrincipal(roleArn).policyFragment; public readonly roleArn = roleArn; @@ -190,6 +192,12 @@ export class Role extends Resource implements IRole { private readonly attachedPolicies = new AttachedPolicies(); private defaultPolicy?: Policy; + constructor(_scope: Construct, _id: string) { + super(_scope, _id, { + account: roleAccount, + }); + } + public addToPolicy(statement: PolicyStatement): boolean { return this.addToPrincipalPolicy(statement).statementAdded; } @@ -204,9 +212,11 @@ export class Role extends Resource implements IRole { } public attachInlinePolicy(policy: Policy): void { - const policyAccount = Stack.of(policy).account; - - if (accountsAreEqualOrOneIsUnresolved(policyAccount, roleAccount)) { + const comparisonResult = this.environment.account.compare(policy.environment.account); + const equalOrAnyUnresolved = comparisonResult === EnvComponentComparison.SAME || + comparisonResult === EnvComponentComparison.BOTH_UNRESOLVED || + comparisonResult === EnvComponentComparison.ONE_UNRESOLVED; + if (equalOrAnyUnresolved) { this.attachedPolicies.attach(policy); policy.attachToRole(this); } @@ -236,23 +246,19 @@ export class Role extends Resource implements IRole { } } - const roleAccount = parsedArn.account; - - const scopeAccount = scopeStack.account; - - return options.mutable !== false && accountsAreEqualOrOneIsUnresolved(scopeAccount, roleAccount) - ? new Import(scope, id) - : new ImmutableRole(scope, `ImmutableRole${id}`, new Import(scope, id)); - - function accountsAreEqualOrOneIsUnresolved( - account1: string | undefined, - account2: string | undefined): boolean { - return Token.isUnresolved(account1) || Token.isUnresolved(account2) || - account1 === account2; - } + const importedRole = new Import(scope, id); + const comparisonResult = importedRole.environment.account.compareToString(scopeStack.account); + const equalOrAnyUnresolved = comparisonResult === EnvComponentComparison.SAME || + comparisonResult === EnvComponentComparison.BOTH_UNRESOLVED || + comparisonResult === EnvComponentComparison.ONE_UNRESOLVED; + // we only return an immutable Role if both accounts were explicitly provided, and different + return options.mutable !== false && equalOrAnyUnresolved + ? importedRole + : new ImmutableRole(scope, `ImmutableRole${id}`, importedRole); } public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount: string | undefined = this.environment.account.toString(); public readonly assumeRoleAction: string = 'sts:AssumeRole'; diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 886937fcf3cb8..816cfb96090ad 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -1,4 +1,4 @@ -import { Construct, Lazy, Resource, SecretValue, Stack } from '@aws-cdk/core'; +import { Aws, Construct, Lazy, Resource, SecretValue, Stack } from '@aws-cdk/core'; import { IGroup } from './group'; import { CfnUser } from './iam.generated'; import { IIdentity } from './identity-base'; @@ -139,6 +139,7 @@ export class User extends Resource implements IIdentity, IUser { class Import extends Resource implements IUser { public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount = Aws.ACCOUNT_ID; public readonly userName: string = userName; public readonly userArn: string = arn; public readonly assumeRoleAction: string = 'sts:AssumeRole'; @@ -175,6 +176,7 @@ export class User extends Resource implements IIdentity, IUser { } public readonly grantPrincipal: IPrincipal = this; + public readonly principalAccount: string | undefined = this.environment.account.toString(); public readonly assumeRoleAction: string = 'sts:AssumeRole'; /** diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json index a0dcc893ca6da..0f37067cedab6 100644 --- a/packages/@aws-cdk/aws-iam/package.json +++ b/packages/@aws-cdk/aws-iam/package.json @@ -89,6 +89,7 @@ "exclude": [ "from-signature:@aws-cdk/aws-iam.Role.fromRoleArn", "construct-interface-extends-iconstruct:@aws-cdk/aws-iam.IManagedPolicy", + "props-physical-name:@aws-cdk/aws-iam.OpenIdConnectProviderProps", "resource-interface-extends-resource:@aws-cdk/aws-iam.IManagedPolicy", "docs-public-apis:@aws-cdk/aws-iam.IUser" ] diff --git a/packages/@aws-cdk/aws-iam/test/cross-account.test.ts b/packages/@aws-cdk/aws-iam/test/cross-account.test.ts new file mode 100644 index 0000000000000..899747f98fd04 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/test/cross-account.test.ts @@ -0,0 +1,233 @@ +import '@aws-cdk/assert/jest'; +import * as cdk from '@aws-cdk/core'; +import * as iam from '../lib'; + +// Test cross-account grant scenario's for principals +// +// When doing a grant on a resource with a resource policy: +// +// - Permissions are added to the identity if possible. +// - Trust is added to the resource if necessary (identity is in +// a different account than the resource). + +let app: cdk.App; +const stack1Account = '1234'; +let stack1: cdk.Stack; +const stack2Account = '5678'; +let stack2: cdk.Stack; +const thirdAccount = '123456789012'; + +beforeEach(() => { + app = new cdk.App(); + stack1 = new cdk.Stack(app, 'Stack1', { env: { account: stack1Account, region: 'us-bla-5' }}); + stack2 = new cdk.Stack(app, 'Stack2', { env: { account: stack2Account, region: 'us-bla-5' }}); +}); + +test('cross-account Role grant creates permissions AND trust', () => { + // GIVEN + const role = new iam.Role(stack1, 'Role', { + roleName: cdk.PhysicalName.GENERATE_IF_NEEDED, + assumedBy: new iam.ServicePrincipal('some.service'), + }); + const resource = new FakeResource(stack2, 'Resource'); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack1); + assertTrustCreated(stack2, { AWS: { + 'Fn::Join': [ '', [ + 'arn:', + { Ref: 'AWS::Partition' }, + `:iam::${stack1Account}:role/stack1stack1rolef3c14260253562f428b7`, + ]], + }}); +}); + +test('Service Principal grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + + // WHEN + doGrant(resource, new iam.ServicePrincipal('service.amazonaws.com')); + + // THEN + assertTrustCreated(stack2, { Service: 'service.amazonaws.com' }); +}); + +test('Imported Role with definitely different account grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:aws:iam::${thirdAccount}:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + noPolicyCreated(stack2); + assertTrustCreated(stack2, { + AWS: `arn:aws:iam::${thirdAccount}:role/S3Access`, + }); +}); + +test('Imported Role with partition token in ARN (definitely different account) grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:${stack2.partition}:iam::${thirdAccount}:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + noPolicyCreated(stack2); + assertTrustCreated(stack2, { + AWS: { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + `:iam::${thirdAccount}:role/S3Access`, + ]], + }, + }); +}); + +test('Imported Role with definitely same account grant does not create trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:aws:iam::${stack2Account}:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack2); + noTrustCreated(stack2); +}); + +test('Imported Role with partition token and definitely same account grant does not create trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:${stack2.partition}:iam::5678:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack2); + noTrustCreated(stack2); +}); + +test('Agnostic stack with concrete imported role adds trust', () => { + // GIVEN + const stack = new cdk.Stack(app, 'AgStack'); + const resource = new FakeResource(stack, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', 'arn:aws:iam::5678:role/S3Access', { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertTrustCreated(stack, { AWS: 'arn:aws:iam::5678:role/S3Access' }); + noPolicyCreated(stack); +}); + +test('Agnostic stack with agnostic imported role does not add trust', () => { + // GIVEN + const stack = new cdk.Stack(app, 'AgStack'); + const resource = new FakeResource(stack, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:aws:iam::${cdk.Aws.ACCOUNT_ID}:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack2); + noTrustCreated(stack); +}); + +test('Immutable role in same account adds no policy and no trust', () => { + // GIVEN + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:aws:iam::${stack2Account}:role/S3Access`, { mutable: false }); + + // require('util').inspect.defaultOptions.customInspect = false; // ? + + // WHEN + doGrant(resource, role); + + // THEN + noTrustCreated(stack2); + noPolicyCreated(stack2); + +}); + +class FakeResource extends cdk.Resource implements iam.IResourceWithPolicy { + public readonly arn = 'arn:aws:resource'; + private readonly policy = new iam.PolicyDocument(); + + constructor(scope: cdk.Construct, id: string) { + super(scope, id); + + new cdk.CfnResource(this, 'Default', { + type: 'Test::Fake::Resource', + properties: { + ResourcePolicy: cdk.Lazy.anyValue({ produce: () => this.policy }), + }, + }); + } + + public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { + this.policy.addStatements(statement); + return { statementAdded: true, policyDependable: this.policy }; + } +} + +function doGrant(resource: FakeResource, principal: iam.IPrincipal) { + iam.Grant.addToPrincipalOrResource({ + actions: ['some:action'], + grantee: principal, + resourceArns: [resource.arn], + resource, + }); +} + +function assertTrustCreated(stack: cdk.Stack, principal: any) { + expect(stack).toHaveResource('Test::Fake::Resource', { + ResourcePolicy: { + Statement: [ + { + Action: 'some:action', + Effect: 'Allow', + Resource: 'arn:aws:resource', + Principal: principal, + }, + ], + Version: '2012-10-17', + }, + }); +} + +function noTrustCreated(stack: cdk.Stack) { + expect(stack).not.toHaveResourceLike('Test::Fake::Resource', { + ResourcePolicy: { + Statement: [ + {}, + ], + }, + }); +} + +function assertPolicyCreated(stack: cdk.Stack) { + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: 'some:action', + Effect: 'Allow', + Resource: 'arn:aws:resource', + }, + ], + Version: '2012-10-17', + }, + }); +} + +function noPolicyCreated(stack: cdk.Stack) { + expect(stack).not.toHaveResource('AWS::IAM::Policy'); +} diff --git a/packages/@aws-cdk/aws-iam/test/grant.test.ts b/packages/@aws-cdk/aws-iam/test/grant.test.ts index 93143f07c8125..3a1d7d580fcb0 100644 --- a/packages/@aws-cdk/aws-iam/test/grant.test.ts +++ b/packages/@aws-cdk/aws-iam/test/grant.test.ts @@ -1,6 +1,6 @@ import { ResourcePart } from '@aws-cdk/assert'; import '@aws-cdk/assert/jest'; -import { CfnResource, Construct, Stack } from '@aws-cdk/core'; +import { CfnResource, Construct, Resource, Stack } from '@aws-cdk/core'; import * as iam from '../lib'; let stack: Stack; @@ -113,13 +113,12 @@ function expectDependencyOn(id: string) { }, ResourcePart.CompleteDefinition); } -class FakeResourceWithPolicy extends CfnResource implements iam.IResourceWithPolicy { - private policy: CfnResource; +class FakeResourceWithPolicy extends Resource implements iam.IResourceWithPolicy { + private readonly policy: CfnResource; constructor(scope: Construct, id: string) { - super(scope, id, { - type: 'CDK::Test::Buckaroo', - }); + super(scope, id); + this.policy = new CfnResource(this, 'Policy', { type: 'CDK::Test::BuckarooPolicy', }); @@ -128,4 +127,4 @@ class FakeResourceWithPolicy extends CfnResource implements iam.IResourceWithPol public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { return { statementAdded: true, policyDependable: this.policy }; } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-lambda/lib/function-base.ts b/packages/@aws-cdk/aws-lambda/lib/function-base.ts index 39ad7d665fcb7..52b0c65c29aac 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function-base.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function-base.ts @@ -287,6 +287,8 @@ export abstract class FunctionBase extends Resource implements IFunction { return { statementAdded: true, policyDependable: this._functionNode().findChild(identifier) } as iam.AddToResourcePolicyResult; }, node: this.node, + stack: this.stack, + environment: this.environment, }, }); } diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index 92b6d64774751..aae8da849eb40 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -504,7 +504,7 @@ export class Function extends FunctionBase { /** * Environment variables for this function */ - private readonly environment: { [key: string]: string }; + private readonly environmentVariables: { [key: string]: string }; private readonly currentVersionOptions?: VersionOptions; private _currentVersion?: Version; @@ -569,7 +569,7 @@ export class Function extends FunctionBase { }; } - this.environment = { ...profilingGroupEnvironmentVariables, ...(props.environment || {}) }; + this.environmentVariables = { ...profilingGroupEnvironmentVariables, ...(props.environment || {}) }; this.deadLetterQueue = this.buildDeadLetterQueue(props); @@ -664,7 +664,7 @@ export class Function extends FunctionBase { * @param value The environment variable's value. */ public addEnvironment(key: string, value: string): this { - this.environment[key] = value; + this.environmentVariables[key] = value; return this; } @@ -752,7 +752,7 @@ export class Function extends FunctionBase { } private renderEnvironment() { - if (!this.environment || Object.keys(this.environment).length === 0) { + if (!this.environmentVariables || Object.keys(this.environmentVariables).length === 0) { return undefined; } @@ -762,7 +762,7 @@ export class Function extends FunctionBase { // stacks. if (!this._currentVersion) { return { - variables: this.environment, + variables: this.environmentVariables, }; } @@ -770,8 +770,8 @@ export class Function extends FunctionBase { // `currentVersion` is not affected by key order (this is how lambda does // it). const variables: { [key: string]: string } = {}; - for (const key of Object.keys(this.environment).sort()) { - variables[key] = this.environment[key]; + for (const key of Object.keys(this.environmentVariables).sort()) { + variables[key] = this.environmentVariables[key]; } return { variables }; diff --git a/packages/@aws-cdk/aws-route53/test/test.util.ts b/packages/@aws-cdk/aws-route53/test/test.util.ts index 006527d3ad869..d589b058e40cc 100644 --- a/packages/@aws-cdk/aws-route53/test/test.util.ts +++ b/packages/@aws-cdk/aws-route53/test/test.util.ts @@ -1,5 +1,6 @@ import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; +import { HostedZone } from '../lib'; import * as util from '../lib/util'; export = { @@ -20,13 +21,10 @@ export = { // WHEN const providedName = 'test.domain.com.'; - const qualified = util.determineFullyQualifiedDomainName(providedName, { + const qualified = util.determineFullyQualifiedDomainName(providedName, HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'ignored', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }); + })); // THEN test.equal(qualified, 'test.domain.com.'); @@ -39,13 +37,10 @@ export = { // WHEN const providedName = 'test.domain.com'; - const qualified = util.determineFullyQualifiedDomainName(providedName, { + const qualified = util.determineFullyQualifiedDomainName(providedName, HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'test.domain.com.', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }); + })); // THEN test.equal(qualified, 'test.domain.com.'); @@ -58,13 +53,10 @@ export = { // WHEN const providedName = 'test.domain.com'; - const qualified = util.determineFullyQualifiedDomainName(providedName, { + const qualified = util.determineFullyQualifiedDomainName(providedName, HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'domain.com.', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }); + })); // THEN test.equal(qualified, 'test.domain.com.'); @@ -77,13 +69,10 @@ export = { // WHEN const providedName = 'test'; - const qualified = util.determineFullyQualifiedDomainName(providedName, { + const qualified = util.determineFullyQualifiedDomainName(providedName, HostedZone.fromHostedZoneAttributes(stack, 'HostedZone', { hostedZoneId: 'fakeId', zoneName: 'domain.com.', - hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId', - stack, - node: stack.node, - }); + })); // THEN test.equal(qualified, 'test.domain.com.'); diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 29327a1685bf6..d4b095385a514 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -291,6 +291,20 @@ export interface BucketAttributes { * @default false */ readonly isWebsite?: boolean; + + /** + * The account this existing bucket belongs to. + * + * @default - it's assumed the bucket belongs to the same account as the scope it's being imported into + */ + readonly account?: string; + + /** + * The region this existing bucket is in. + * + * @default - it's assumed the bucket is in the same region as the scope it's being imported into + */ + readonly region?: string; } /** @@ -627,25 +641,12 @@ abstract class BucketBase extends Resource implements IBucket { resourceArn: string, ...otherResourceArns: string[]) { const resources = [ resourceArn, ...otherResourceArns ]; - const crossAccountAccess = this.isGranteeFromAnotherAccount(grantee); - let ret: iam.Grant; - if (crossAccountAccess) { - // if the access is cross-account, we need to trust the accessing principal in the bucket's policy - ret = iam.Grant.addToPrincipalAndResource({ - grantee, - actions: bucketActions, - resourceArns: resources, - resource: this, - }); - } else { - // if not, we don't need to modify the resource policy if the grantee is an identity principal - ret = iam.Grant.addToPrincipalOrResource({ - grantee, - actions: bucketActions, - resourceArns: resources, - resource: this, - }); - } + const ret = iam.Grant.addToPrincipalOrResource({ + grantee, + actions: bucketActions, + resourceArns: resources, + resource: this, + }); if (this.encryptionKey && keyActions && keyActions.length !== 0) { this.encryptionKey.grant(grantee, ...keyActions); @@ -653,15 +654,6 @@ abstract class BucketBase extends Resource implements IBucket { return ret; } - - private isGranteeFromAnotherAccount(grantee: iam.IGrantable): boolean { - if (!(Construct.isConstruct(grantee))) { - return false; - } - const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); - return bucketStack.account !== identityStack.account; - } } export interface BlockPublicAccessOptions { @@ -1166,7 +1158,10 @@ export class Bucket extends BucketBase { } } - return new Import(scope, id); + return new Import(scope, id, { + account: attrs.account, + region: attrs.region, + }); } public readonly bucketArn: string; diff --git a/packages/@aws-cdk/aws-s3/test/test.util.ts b/packages/@aws-cdk/aws-s3/test/test.util.ts index dfa612da7d335..96276f681d499 100644 --- a/packages/@aws-cdk/aws-s3/test/test.util.ts +++ b/packages/@aws-cdk/aws-s3/test/test.util.ts @@ -51,37 +51,10 @@ export = { const stack = new cdk.Stack(); const bucketArn = `arn:aws:s3:::${cdk.Token.asString({ Ref: 'my-bucket' })}`; - test.deepEqual(stack.resolve(parseBucketName(stack, { bucketArn })), { - 'Fn::Select': [ - 0, - { - 'Fn::Split': [ - '/', - { - 'Fn::Select': [ - 5, - { - 'Fn::Split': [ - ':', - { - 'Fn::Join': [ - '', - [ - 'arn:aws:s3:::', - { - Ref: 'my-bucket', - }, - ], - ], - }, - ], - }, - ], - }, - ], - }, - ], - }); + test.deepEqual( + stack.resolve(parseBucketName(stack, { bucketArn })), + { Ref: 'my-bucket' }, + ); test.done(); }, diff --git a/packages/@aws-cdk/core/lib/arn.ts b/packages/@aws-cdk/core/lib/arn.ts index 966832c9e4786..d92ddbf9a0aeb 100644 --- a/packages/@aws-cdk/core/lib/arn.ts +++ b/packages/@aws-cdk/core/lib/arn.ts @@ -135,12 +135,16 @@ export class Arn { * components of the ARN. */ public static parse(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents { - if (Token.isUnresolved(arn)) { + const components = arn.split(':') as Array; + const looksLikeArn = arn.startsWith('arn:') && components.length >= 6 && components.length <= 7; + + // If the ARN merely contains Tokens, but otherwise *looks* mostly like an ARN, + // it's a string of the form 'arn:${partition}:service:us-west-1:${account}:abc/xyz'. Parse fields + // out to the best of our ability. Tokens won't contain ":" so this won't break them. + if (Token.isUnresolved(arn) && !looksLikeArn) { return parseToken(arn, sepIfToken, hasName); } - const components = arn.split(':') as Array; - if (components.length < 6) { throw new Error('ARNs must have at least 6 components: ' + arn); } diff --git a/packages/@aws-cdk/core/lib/cfn-pseudo.ts b/packages/@aws-cdk/core/lib/cfn-pseudo.ts index dc1848aa49ca4..7144b7edc2af1 100644 --- a/packages/@aws-cdk/core/lib/cfn-pseudo.ts +++ b/packages/@aws-cdk/core/lib/cfn-pseudo.ts @@ -77,5 +77,10 @@ export class ScopedAws { } function pseudoString(name: string): string { - return Token.asString({ Ref: name }, { displayHint: name }); + // we don't want any ':' in the serialized form, + // as ':' is the ARN separator, + // and so we don't want ARN components + // (which these CFN references like AWS::Partition certainly can be) + // to contain ':'s themselves + return Token.asString({ Ref: name }, { displayHint: name.replace('::', '.') }); } diff --git a/packages/@aws-cdk/core/lib/environment-component.ts b/packages/@aws-cdk/core/lib/environment-component.ts new file mode 100644 index 0000000000000..eaf443eeedaa8 --- /dev/null +++ b/packages/@aws-cdk/core/lib/environment-component.ts @@ -0,0 +1,130 @@ +import { Token } from './token'; + +/** + * Interface representing the environment a given resource lives in. + * Used as the return value for the IResource.environment property. + */ +export interface ResourceEnvironment { + /** The AWS account ID that this resource belongs to. */ + readonly account: EnvComponent; + + /** The AWS region that this resource belongs to. */ + readonly region: EnvComponent; +} + +/** + * An enum-like class that represents the result of comparing either two {@link EnvComponent}s, + * or a {@link EnvComponent}, and a string. + */ +export class EnvComponentComparison { + /** + * This means we're certain the two components are NOT + * Tokens, and identical. + */ + public static readonly SAME = new EnvComponentComparison(); + + /** + * This means we're certain the two components are NOT + * Tokens, and different. + */ + public static readonly DIFFERENT = new EnvComponentComparison(); + + /** This means exactly one of the components is a Token. */ + public static readonly ONE_UNRESOLVED = new EnvComponentComparison(); + + /** This means both components are Tokens. */ + public static readonly BOTH_UNRESOLVED = new EnvComponentComparison(); + + private constructor() { + } + + /** + * Returns true if: + * - Neither component was a Token, and they were equal to each other. + * Or: + * - Both component were Tokens. + * + * @returns true if `this` is either SAME or BOTH_UNRESOLVED, + * false if `this` is either DIFFERENT or ONE_UNRESOLVED + */ + // public equalOrBothUnresolved(): boolean { + // return this === EnvComponentComparison.SAME || + // this === EnvComponentComparison.BOTH_UNRESOLVED; + // } + + /** + * Returns true if: + * - Neither component was a Token, and they were equal to each other. + * Or: + * - Either component (or both!) was a Token. + * + * @returns true if `this` is either SAME or ONE_UNRESOLVED or BOTH_UNRESOLVED, + * false if `this` is DIFFERENT + */ + // public equalOrAnyUnresolved(): boolean { + // return this !== EnvComponentComparison.DIFFERENT; + // } + + /** + * Returns true if: + * - Neither component was a Token, and they were not equal to each other. + * Or: + * - Exactly one of the components was a Token. + * + * @returns true if `this` is either DIFFERENT or ONE_UNRESOLVED, + * false if `this` is either SAME or BOTH_UNRESOLVED + */ + // public unEqual(): boolean { + // return !this.equalOrBothUnresolved(); + // } +} + +/** + * Represents a component of an Environment, + * like an AWS account ID, + * or the code name of an AWS region. + * Since they can be Tokens representing deploy-time values like AWS::AccountId and AWS::Region, + * their comparison logic is slightly more complicated than just comparing strings for equality. + */ +export class EnvComponent { + private readonly component: string; + + public constructor(component: string) { + this.component = component; + } + + /** + * Compare two components with each other. + */ + public compare(envComponent: EnvComponent): EnvComponentComparison { + return this.compareToString(envComponent.toString()); + } + + /** + * Compare this component to a string representation. + */ + public compareToString(thatComponent: string): EnvComponentComparison { + const firstIsUnresolved = Token.isUnresolved(this.component); + const secondIsUnresolved = Token.isUnresolved(thatComponent); + + if (firstIsUnresolved && secondIsUnresolved) { + return EnvComponentComparison.BOTH_UNRESOLVED; + } + if (firstIsUnresolved || secondIsUnresolved) { + return EnvComponentComparison.ONE_UNRESOLVED; + } + + return this.component === thatComponent + ? EnvComponentComparison.SAME + : EnvComponentComparison.DIFFERENT; + } + + /** + * Returns a string representation of this component. + * Useful when passing it to places that expect strings, + * like Environment. + */ + public toString(): string { + return this.component; + } +} diff --git a/packages/@aws-cdk/core/lib/index.ts b/packages/@aws-cdk/core/lib/index.ts index 6c54a222901d6..558a2dff110ee 100644 --- a/packages/@aws-cdk/core/lib/index.ts +++ b/packages/@aws-cdk/core/lib/index.ts @@ -37,6 +37,7 @@ export * from './stack-trace'; export * from './app'; export * from './context-provider'; export * from './environment'; +export * from './environment-component'; export * from './runtime'; export * from './secret-value'; diff --git a/packages/@aws-cdk/core/lib/resource.ts b/packages/@aws-cdk/core/lib/resource.ts index 6574dbe331da0..a2d6d5f0d8ed2 100644 --- a/packages/@aws-cdk/core/lib/resource.ts +++ b/packages/@aws-cdk/core/lib/resource.ts @@ -1,5 +1,6 @@ import { ArnComponents } from './arn'; import { Construct, IConstruct } from './construct-compat'; +import { EnvComponent, ResourceEnvironment } from './environment-component'; import { Lazy } from './lazy'; import { generatePhysicalName, isGeneratedWhenNeededMarker } from './private/physical-name-generator'; import { IResolveContext } from './resolvable'; @@ -14,6 +15,17 @@ export interface IResource extends IConstruct { * The stack in which this resource is defined. */ readonly stack: Stack; + + /** + * The environment this resource belongs to. + * For resources that are created and managed by the CDK + * (generally, those created by creating new class instances like Role, Bucket, etc.) + * this is always the same as the environment of the stack they belong to; + * however, for imported resources + * (those obtained from static methods like fromRoleArn, fromBucketName, etc.) + * that might be different than the stack they were imported into. + */ + readonly environment: ResourceEnvironment; } /** @@ -32,6 +44,20 @@ export interface ResourceProps { * @default - The physical name will be allocated by CloudFormation at deployment time */ readonly physicalName?: string; + + /** + * The AWS account ID this resource belongs to. + * + * @default - the resource is in the same account as the stack it belongs to + */ + readonly account?: string; + + /** + * The AWS region this resource belongs to. + * + * @default - the resource is in the same region as the stack it belongs to + */ + readonly region?: string; } /** @@ -39,6 +65,7 @@ export interface ResourceProps { */ export abstract class Resource extends Construct implements IResource { public readonly stack: Stack; + public readonly environment: ResourceEnvironment; /** * Returns a string-encoded token that resolves to the physical name that @@ -59,7 +86,12 @@ export abstract class Resource extends Construct implements IResource { constructor(scope: Construct, id: string, props: ResourceProps = {}) { super(scope, id); + this.stack = Stack.of(this); + this.environment = { + account: new EnvComponent(props.account ?? this.stack.account), + region: new EnvComponent(props.region ?? this.stack.region), + }; let physicalName = props.physicalName; diff --git a/packages/@aws-cdk/core/test/test.arn.ts b/packages/@aws-cdk/core/test/test.arn.ts index 0fd85741508e7..3307ba203acc6 100644 --- a/packages/@aws-cdk/core/test/test.arn.ts +++ b/packages/@aws-cdk/core/test/test.arn.ts @@ -1,5 +1,5 @@ import { Test } from 'nodeunit'; -import { ArnComponents, CfnOutput, ScopedAws, Stack } from '../lib'; +import { ArnComponents, Aws, CfnOutput, ScopedAws, Stack } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { toCloudFormation } from './util'; @@ -256,4 +256,23 @@ export = { test.done(); }, + + 'parse other fields if only some are tokens'(test: Test) { + // GIVEN + const stack = new Stack(); + + // WHEN + const parsed = stack.parseArn(`arn:${Aws.PARTITION}:iam::123456789012:role/S3Access`); + + // THEN + test.deepEqual(stack.resolve(parsed.partition), { Ref: 'AWS::Partition' }); + test.deepEqual(stack.resolve(parsed.service), 'iam'); + test.equal(stack.resolve(parsed.region), ''); + test.deepEqual(stack.resolve(parsed.account), '123456789012'); + test.deepEqual(stack.resolve(parsed.resource), 'role'); + test.deepEqual(stack.resolve(parsed.resourceName), 'S3Access'); + test.equal(parsed.sep, '/'); + + test.done(); + }, };