From 69bff3d28687f7db1fb65ef4c967f8cce3b554ec Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 10 Sep 2019 17:15:30 -0700 Subject: [PATCH] feat(codepipeline): cross-environment (account+region) actions (#3694) This changes the scaffolding stack logic for the cross-region CodePipelines to include a KMS key and alias as part of it, which are required if an action is simultaneously cross-region and cross-account. We also change to use the KMS key ID instead of the key ARN when rendering the ArtifactStores property. We also add an alias to the default pipeline artifact bucket. This required a bunch of changes to the KMS and S3 modules: * Alias now implements IKey * Added the keyId property to IKey * Added removalPolicy property to Alias * Granting permissions to a key works if the principal belongs to a stack that is a dependent of the key stack * Allow specifying a key when importing a bucket Fixes #52 Concerns #1584 Fixes #2517 Fixes #2569 Concerns #3275 Fixes #3138 Fixes #3388 --- ...g.cfn-template-from-repo.lit.expected.json | 19 +- ...yed-through-codepipeline.lit.expected.json | 19 +- .../test/integ.lambda-pipeline.expected.json | 19 +- .../integ.pipeline-alexa-deploy.expected.json | 19 +- .../test/integ.pipeline-cfn.expected.json | 19 +- ...g.pipeline-code-commit-build.expected.json | 19 +- .../integ.pipeline-code-commit.expected.json | 19 +- .../test/integ.pipeline-events.expected.json | 19 +- .../integ.pipeline-s3-deploy.expected.json | 19 +- packages/@aws-cdk/aws-codepipeline/README.md | 58 ++++- .../lib/cross-region-support-stack.ts | 8 + .../@aws-cdk/aws-codepipeline/lib/pipeline.ts | 55 +++-- .../test/fake-build-action.ts | 2 + .../aws-codepipeline/test/test.pipeline.ts | 216 +++++++++++++++++- .../integ.pipeline-event-target.expected.json | 19 +- packages/@aws-cdk/aws-iam/lib/grant.ts | 11 +- packages/@aws-cdk/aws-kms/lib/alias.ts | 73 +++++- packages/@aws-cdk/aws-kms/lib/key.ts | 92 +++++++- packages/@aws-cdk/aws-kms/test/test.alias.ts | 73 +++++- packages/@aws-cdk/aws-kms/test/test.key.ts | 154 ++++++++++--- packages/@aws-cdk/aws-s3/lib/bucket.ts | 17 +- packages/@aws-cdk/core/lib/resource.ts | 6 +- .../test/__snapshots__/synth.test.js.snap | 19 +- 23 files changed, 838 insertions(+), 136 deletions(-) diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json index dcc7e89566439..ce1f5d554bd45 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json @@ -131,6 +131,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinecloudformationpipeline7dbde619", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -379,10 +393,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json index 52a0ab9813f1a..a9cd3292bc0df 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json @@ -182,6 +182,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-pipelinestackpipeline9db740af", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -504,10 +518,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json index 9d4c58d45426c..d1e562e2204e0 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json @@ -107,6 +107,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinelambdapipeline87a4b3d3", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -282,10 +296,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json index 1471e9f0443f9..4f6d52c4c1829 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json @@ -117,6 +117,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinealexadeploypipeline961107f5", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -281,10 +295,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json index dd5b6c6f849ba..44990099303a5 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json @@ -140,6 +140,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinecloudformationpipeline7dbde619", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -367,10 +381,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json index 3609e47f5bf10..c18c56d5f9e4f 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json @@ -355,6 +355,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinecodecommitcodebuildpipeline9540e1f5", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "UpdateReplacePolicy": "Retain", + "DeletionPolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -579,10 +593,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit.expected.json index 6dcd3a65d6c81..5c53fa46db4ef 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit.expected.json @@ -180,6 +180,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelinecodecommitpipelinef780ca18", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -353,10 +367,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-events.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-events.expected.json index eb4ea22cafcf1..8ab452d92da02 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-events.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-events.expected.json @@ -128,6 +128,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "MyPipelineArtifactsBucketEncryptionKeyAlias9D4F8C59": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkpipelineeventtargetmypipeline4ae5d407", + "TargetKeyId": { + "Fn::GetAtt": [ + "MyPipelineArtifactsBucketEncryptionKey8BF0A7F3", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "MyPipelineRoleC0D47CA4": { "Type": "AWS::IAM::Role", "Properties": { @@ -316,10 +330,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "MyPipelineArtifactsBucketEncryptionKey8BF0A7F3", - "Arn" - ] + "Ref": "MyPipelineArtifactsBucketEncryptionKey8BF0A7F3" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json index ee6a7452567f6..59590ad989c44 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json @@ -138,6 +138,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-awscdkcodepipelines3deploypipeline907bf1e7", + "TargetKeyId": { + "Fn::GetAtt": [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "PipelineRoleD68726F7": { "Type": "AWS::IAM::Role", "Properties": { @@ -318,10 +332,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn" - ] + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-codepipeline/README.md b/packages/@aws-cdk/aws-codepipeline/README.md index 40dab2d19072a..4e439c665aa90 100644 --- a/packages/@aws-cdk/aws-codepipeline/README.md +++ b/packages/@aws-cdk/aws-codepipeline/README.md @@ -88,13 +88,18 @@ into a different region than your Pipeline is in. It works like this: -```ts +```typescript const pipeline = new codepipeline.Pipeline(this, 'MyFirstPipeline', { // ... crossRegionReplicationBuckets: { // note that a physical name of the replication Bucket must be known at synthesis time - 'us-west-1': s3.Bucket.fromBucketName(this, 'UsWest1ReplicationBucket', - 'my-us-west-1-replication-bucket'), + 'us-west-1': s3.Bucket.fromBucketAttributes(this, 'UsWest1ReplicationBucket', { + bucketName: 'my-us-west-1-replication-bucket', + // optional KMS key + encryptionKey: kms.Key.fromKeyArn(this, 'UsWest1ReplicationKey', + 'arn:aws:kms:us-west-1:123456789012:key/1234-5678-9012' + ), + }), }, }); @@ -128,6 +133,53 @@ $ cdk deploy MyMainStack See [the AWS docs here](https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-create-cross-region.html) for more information on cross-region CodePipelines. +#### Creating an encrypted replication bucket + +If you're passing a replication bucket created in a different stack, +like this: + +```typescript +const replicationStack = new Stack(app, 'ReplicationStack', { + env: { + region: 'us-west-1', + }, +}); +const key = new kms.Key(replicationStack, 'ReplicationKey'); +const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', { + // like was said above - replication buckets need a set physical name + bucketName: PhysicalName.GENERATE_IF_NEEDED, + encryptionKey: key, // does not work! +}); + +// later... +new codepipeline.Pipeline(pipelineStack, 'Pipeline', { + crossRegionReplicationBuckets: { + 'us-west-1': replicationBucket, + }, +}); +``` + +When trying to encrypt it +(and note that if any of the cross-region actions happen to be cross-account as well, +the bucket *has to* be encrypted - otherwise the pipeline will fail at runtime), +you cannot use a key directly - KMS keys don't have physical names, +and so you can't reference them across environments. + +In this case, you need to use an alias in place of the key when creating the bucket: + +```typescript +const key = new kms.Key(replicationStack, 'ReplicationKey'); +const alias = new kms.Alias(replicationStack, 'ReplicationAlias', { + // aliasName is required + aliasName: PhysicalName.GENERATE_IF_NEEDED, + targetKey: key, +}); +const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', { + bucketName: PhysicalName.GENERATE_IF_NEEDED, + encryptionKey: alias, +}); +``` + ### Events #### Using a pipeline as an event target diff --git a/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts b/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts index e916924d3ac24..f87d6655fa41f 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts @@ -1,3 +1,4 @@ +import kms = require('@aws-cdk/aws-kms'); import s3 = require('@aws-cdk/aws-s3'); import cdk = require('@aws-cdk/core'); @@ -44,8 +45,15 @@ export class CrossRegionSupportStack extends cdk.Stack { }, }); + const encryptionKey = new kms.Key(this, 'CrossRegionCodePipelineReplicationBucketEncryptionKey'); + const encryptionAlias = new kms.Alias(this, 'CrossRegionCodePipelineReplicationBucketEncryptionAlias', { + targetKey: encryptionKey, + aliasName: cdk.PhysicalName.GENERATE_IF_NEEDED, + removalPolicy: cdk.RemovalPolicy.RETAIN, + }); this.replicationBucket = new s3.Bucket(this, 'CrossRegionCodePipelineReplicationBucket', { bucketName: cdk.PhysicalName.GENERATE_IF_NEEDED, + encryptionKey: encryptionAlias, }); } } diff --git a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts index 3cdc89d84c075..c3f73a12dafab 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts @@ -231,6 +231,12 @@ export class Pipeline extends PipelineBase { encryption: s3.BucketEncryption.KMS, removalPolicy: RemovalPolicy.RETAIN }); + // add an alias to make finding the key in the console easier + new kms.Alias(this, 'ArtifactsBucketEncryptionKeyAlias', { + aliasName: this.generateNameForDefaultBucketKeyAlias(), + targetKey: encryptionKey, + removalPolicy: RemovalPolicy.RETAIN, // alias should be retained, like the key + }); } this.artifactBucket = propsBucket; @@ -240,8 +246,8 @@ export class Pipeline extends PipelineBase { }); const codePipeline = new CfnPipeline(this, 'Resource', { - artifactStore: Lazy.anyValue({ produce: () => this.renderArtifactStore() }), - artifactStores: Lazy.anyValue({ produce: () => this.renderArtifactStores() }), + artifactStore: Lazy.anyValue({ produce: () => this.renderArtifactStoreProperty() }), + artifactStores: Lazy.anyValue({ produce: () => this.renderArtifactStoresProperty() }), stages: Lazy.anyValue({ produce: () => this.renderStages() }), roleArn: this.role.roleArn, restartExecutionOnUpdate: props && props.restartExecutionOnUpdate, @@ -367,9 +373,10 @@ export class Pipeline extends PipelineBase { return this.artifactBucket; } + const pipelineStack = Stack.of(this); + let otherStack: Stack; let replicationBucket = this.crossRegionReplicationBuckets[region]; if (!replicationBucket) { - const pipelineStack = Stack.of(this); const pipelineAccount = pipelineStack.account; if (Token.isUnresolved(pipelineAccount)) { throw new Error("You need to specify an explicit account when using CodePipeline's cross-region support"); @@ -382,23 +389,34 @@ export class Pipeline extends PipelineBase { account: pipelineAccount, }); replicationBucket = crossRegionScaffoldStack.replicationBucket; - pipelineStack.addDependency(crossRegionScaffoldStack); this._crossRegionSupport[region] = { stack: crossRegionScaffoldStack, replicationBucket, }; this.crossRegionReplicationBuckets[region] = replicationBucket; + otherStack = crossRegionScaffoldStack; + } else { + otherStack = Stack.of(replicationBucket); } + + // the stack containing the replication bucket must be deployed before the pipeline + pipelineStack.addDependency(otherStack); replicationBucket.grantReadWrite(this.role); - this.artifactStores[region] = { - location: replicationBucket.bucketName, - type: 'S3', - }; + this.artifactStores[region] = this.renderArtifactStore(replicationBucket); return replicationBucket; } + private generateNameForDefaultBucketKeyAlias(): string { + const prefix = 'alias/codepipeline-'; + const maxAliasLength = 256; + const uniqueId = this.node.uniqueId; + // take the last 256 - (prefix length) characters of uniqueId + const startIndex = Math.max(0, uniqueId.length - (maxAliasLength - prefix.length)); + return prefix + uniqueId.substring(startIndex).toLowerCase(); + } + /** * Gets the role used for this action, * including handling the case when the action is supposed to be cross-account. @@ -644,7 +662,7 @@ export class Pipeline extends PipelineBase { return ret; } - private renderArtifactStores(): CfnPipeline.ArtifactStoreMapProperty[] | undefined { + private renderArtifactStoresProperty(): CfnPipeline.ArtifactStoreMapProperty[] | undefined { if (!this.crossRegion) { return undefined; } // add the Pipeline's artifact store @@ -657,30 +675,29 @@ export class Pipeline extends PipelineBase { })); } - private renderArtifactStore(): CfnPipeline.ArtifactStoreProperty | undefined { + private renderArtifactStoreProperty(): CfnPipeline.ArtifactStoreProperty | undefined { if (this.crossRegion) { return undefined; } return this.renderPrimaryArtifactStore(); } private renderPrimaryArtifactStore(): CfnPipeline.ArtifactStoreProperty { + return this.renderArtifactStore(this.artifactBucket); + } + + private renderArtifactStore(bucket: s3.IBucket): CfnPipeline.ArtifactStoreProperty { let encryptionKey: CfnPipeline.EncryptionKeyProperty | undefined; - const bucketKey = this.artifactBucket.encryptionKey; + const bucketKey = bucket.encryptionKey; if (bucketKey) { encryptionKey = { type: 'KMS', - id: bucketKey.keyArn, + id: bucketKey.keyId, }; } - const bucketName = this.artifactBucket.bucketName; - if (!bucketName) { - throw new Error('Artifacts bucket must have a bucket name'); - } - return { type: 'S3', - location: bucketName, - encryptionKey + location: bucket.bucketName, + encryptionKey, }; } diff --git a/packages/@aws-cdk/aws-codepipeline/test/fake-build-action.ts b/packages/@aws-cdk/aws-codepipeline/test/fake-build-action.ts index 5e29ffc05ef0d..20045461296d3 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/fake-build-action.ts +++ b/packages/@aws-cdk/aws-codepipeline/test/fake-build-action.ts @@ -15,6 +15,8 @@ export interface FakeBuildActionProps extends codepipeline.CommonActionProps { role?: iam.IRole; account?: string; + + region?: string; } export class FakeBuildAction implements codepipeline.IAction { diff --git a/packages/@aws-cdk/aws-codepipeline/test/test.pipeline.ts b/packages/@aws-cdk/aws-codepipeline/test/test.pipeline.ts index 275596de6c826..c918c9dfcd6b2 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/test.pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline/test/test.pipeline.ts @@ -1,5 +1,7 @@ -import { expect, haveResourceLike } from '@aws-cdk/assert'; +import { expect, haveResourceLike, ResourcePart } from '@aws-cdk/assert'; import iam = require('@aws-cdk/aws-iam'); +import kms = require('@aws-cdk/aws-kms'); +import s3 = require('@aws-cdk/aws-s3'); import cdk = require('@aws-cdk/core'); import { Test } from 'nodeunit'; import codepipeline = require('../lib'); @@ -43,6 +45,218 @@ export = { test.done(); }, + 'that is cross-region': { + 'allows passing an Alias in place of the KMS Key in the replication Bucket'(test: Test) { + const app = new cdk.App(); + + const replicationRegion = 'us-west-1'; + const replicationStack = new cdk.Stack(app, 'ReplicationStack', { + env: { region: replicationRegion, account: '123456789012' }, + }); + const replicationKey = new kms.Key(replicationStack, 'ReplicationKey'); + const replicationAlias = replicationKey.addAlias('alias/my-replication-alias'); + const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', { + encryptionKey: replicationAlias, + bucketName: cdk.PhysicalName.GENERATE_IF_NEEDED, + }); + + const pipelineRegion = 'us-west-2'; + const pipelineStack = new cdk.Stack(app, 'PipelineStack', { + env: { region: pipelineRegion, account: '123456789012' }, + }); + const sourceOutput = new codepipeline.Artifact(); + new codepipeline.Pipeline(pipelineStack, 'Pipeline', { + crossRegionReplicationBuckets: { + [replicationRegion]: replicationBucket, + }, + stages: [ + { + stageName: 'Source', + actions: [new FakeSourceAction({ + actionName: 'Source', + output: sourceOutput, + })], + }, + { + stageName: 'Build', + actions: [new FakeBuildAction({ + actionName: 'Build', + input: sourceOutput, + region: replicationRegion, + })], + }, + ], + }); + + expect(pipelineStack).to(haveResourceLike('AWS::CodePipeline::Pipeline', { + "ArtifactStores": [ + { + "Region": replicationRegion, + "ArtifactStore": { + "Type": "S3", + "EncryptionKey": { + "Type": "KMS", + "Id": "alias/my-replication-alias", + }, + }, + }, + { + "Region": pipelineRegion, + }, + ], + })); + + expect(replicationStack).to(haveResourceLike('AWS::KMS::Key', { + "KeyPolicy": { + "Statement": [ + { + // owning account management permissions - we don't care about them in this test + }, + { + // KMS verifies whether the principal given in its key policy exists when creating that key. + // Since the replication bucket must be deployed before the pipeline, + // we cannot put the pipeline role as the principal here - + // hence, we put the account itself + "Action": [ + "kms:Decrypt", + "kms:DescribeKey", + "kms:Encrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + ], + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": ["", [ + "arn:", + { "Ref": "AWS::Partition" }, + ":iam::123456789012:root", + ]], + }, + }, + "Resource": "*", + }, + ], + }, + })); + + test.done(); + }, + + "generates ArtifactStores with the alias' name as the KeyID"(test: Test) { + const app = new cdk.App(); + const replicationRegion = 'us-west-1'; + + const pipelineRegion = 'us-west-2'; + const pipelineStack = new cdk.Stack(app, 'MyStack', { + env: { region: pipelineRegion, account: '123456789012' }, + }); + const sourceOutput = new codepipeline.Artifact(); + const pipeline = new codepipeline.Pipeline(pipelineStack, 'Pipeline', { + stages: [ + { + stageName: 'Source', + actions: [new FakeSourceAction({ + actionName: 'Source', + output: sourceOutput, + })], + }, + { + stageName: 'Build', + actions: [new FakeBuildAction({ + actionName: 'Build', + input: sourceOutput, + region: replicationRegion, + })], + }, + ], + }); + + expect(pipelineStack).to(haveResourceLike('AWS::CodePipeline::Pipeline', { + "ArtifactStores": [ + { + "Region": replicationRegion, + "ArtifactStore": { + "Type": "S3", + "EncryptionKey": { + "Type": "KMS", + "Id": "alias/mystack-support-us-west-1tencryptionalias9b344b2b8e6825cb1f7d", + }, + }, + }, + { + "Region": pipelineRegion, + }, + ], + })); + + expect(pipeline.crossRegionSupport[replicationRegion].stack).to(haveResourceLike('AWS::KMS::Alias', { + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain", + }, ResourcePart.CompleteDefinition)); + + test.done(); + }, + + 'allows passing an imported Bucket and Key for the replication Bucket'(test: Test) { + const replicationRegion = 'us-west-1'; + + const pipelineRegion = 'us-west-2'; + const pipelineStack = new cdk.Stack(undefined, undefined, { + env: { region: pipelineRegion }, + }); + const sourceOutput = new codepipeline.Artifact(); + new codepipeline.Pipeline(pipelineStack, 'Pipeline', { + crossRegionReplicationBuckets: { + [replicationRegion]: s3.Bucket.fromBucketAttributes(pipelineStack, 'ReplicationBucket', { + bucketArn: 'arn:aws:s3:::my-us-west-1-replication-bucket', + encryptionKey: kms.Key.fromKeyArn(pipelineStack, 'ReplicationKey', + `arn:aws:kms:${replicationRegion}:123456789012:key/1234-5678-9012` + ), + }), + }, + stages: [ + { + stageName: 'Source', + actions: [new FakeSourceAction({ + actionName: 'Source', + output: sourceOutput, + })], + }, + { + stageName: 'Build', + actions: [new FakeBuildAction({ + actionName: 'Build', + input: sourceOutput, + region: replicationRegion, + })], + }, + ], + }); + + expect(pipelineStack).to(haveResourceLike('AWS::CodePipeline::Pipeline', { + "ArtifactStores": [ + { + "Region": replicationRegion, + "ArtifactStore": { + "Type": "S3", + "Location": "my-us-west-1-replication-bucket", + "EncryptionKey": { + "Type": "KMS", + "Id": "1234-5678-9012", + }, + }, + }, + { + "Region": pipelineRegion, + }, + ], + })); + + test.done(); + }, + }, + 'that is cross-account': { 'does not allow passing a dynamic value in the Action account property'(test: Test) { const app = new cdk.App(); diff --git a/packages/@aws-cdk/aws-events-targets/test/codepipeline/integ.pipeline-event-target.expected.json b/packages/@aws-cdk/aws-events-targets/test/codepipeline/integ.pipeline-event-target.expected.json index 9912dc13b4b66..fe5590840de9e 100644 --- a/packages/@aws-cdk/aws-events-targets/test/codepipeline/integ.pipeline-event-target.expected.json +++ b/packages/@aws-cdk/aws-events-targets/test/codepipeline/integ.pipeline-event-target.expected.json @@ -96,6 +96,20 @@ "DeletionPolicy": "Retain", "UpdateReplacePolicy": "Retain" }, + "pipelinePipeline22F2A91DArtifactsBucketEncryptionKeyAlias9530209A": { + "Type": "AWS::KMS::Alias", + "Properties": { + "AliasName": "alias/codepipeline-pipelineeventspipelinepipeline22f2a91dfbb66895", + "TargetKeyId": { + "Fn::GetAtt": [ + "pipelinePipeline22F2A91DArtifactsBucketEncryptionKey87C796D2", + "Arn" + ] + } + }, + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" + }, "pipelinePipeline22F2A91DRole58B7B05E": { "Type": "AWS::IAM::Role", "Properties": { @@ -268,10 +282,7 @@ "ArtifactStore": { "EncryptionKey": { "Id": { - "Fn::GetAtt": [ - "pipelinePipeline22F2A91DArtifactsBucketEncryptionKey87C796D2", - "Arn" - ] + "Ref": "pipelinePipeline22F2A91DArtifactsBucketEncryptionKey87C796D2" }, "Type": "KMS" }, diff --git a/packages/@aws-cdk/aws-iam/lib/grant.ts b/packages/@aws-cdk/aws-iam/lib/grant.ts index 04c060dfc0420..ccda8a5eff3e5 100644 --- a/packages/@aws-cdk/aws-iam/lib/grant.ts +++ b/packages/@aws-cdk/aws-iam/lib/grant.ts @@ -1,6 +1,6 @@ import cdk = require('@aws-cdk/core'); import { PolicyStatement } from "./policy-statement"; -import { IGrantable } from "./principals"; +import { IGrantable, IPrincipal } from "./principals"; /** * Basic options for a grant operation @@ -83,6 +83,13 @@ export interface GrantOnPrincipalAndResourceOptions extends CommonGrantOptions { * @default Same as regular resource ARNs */ readonly resourceSelfArns?: string[]; + + /** + * The principal to use in the statement for the resource policy. + * + * @default - the principal of the grantee will be used + */ + readonly resourcePolicyPrincipal?: IPrincipal; } /** @@ -160,7 +167,7 @@ export class Grant { const statement = new PolicyStatement({ actions: options.actions, resources: (options.resourceSelfArns || options.resourceArns), - principals: [options.grantee!.grantPrincipal] + principals: [options.resourcePolicyPrincipal || options.grantee!.grantPrincipal] }); options.resource.addToResourcePolicy(statement); diff --git a/packages/@aws-cdk/aws-kms/lib/alias.ts b/packages/@aws-cdk/aws-kms/lib/alias.ts index bf1dd240ecea9..41c10132750e2 100644 --- a/packages/@aws-cdk/aws-kms/lib/alias.ts +++ b/packages/@aws-cdk/aws-kms/lib/alias.ts @@ -1,4 +1,5 @@ -import { Construct, IResource, Resource, Token } from '@aws-cdk/core'; +import iam = require('@aws-cdk/aws-iam'); +import { Construct, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; import { IKey } from './key'; import { CfnAlias } from './kms.generated'; @@ -7,8 +8,9 @@ const DISALLOWED_PREFIX = REQUIRED_ALIAS_PREFIX + 'aws/'; /** * A KMS Key alias. + * An alias can be used in all places that expect a key. */ -export interface IAlias extends IResource { +export interface IAlias extends IKey { /** * The name of the alias. * @@ -41,12 +43,55 @@ export interface AliasProps { * specify another alias. */ readonly targetKey: IKey; + + /** + * Policy to apply when the alias is removed from this stack. + * + * @default - The alias will be deleted + */ + readonly removalPolicy?: RemovalPolicy; } abstract class AliasBase extends Resource implements IAlias { public abstract readonly aliasName: string; public abstract readonly aliasTargetKey: IKey; + + public get keyArn(): string { + return Stack.of(this).formatArn({ + service: 'kms', + // aliasName already contains the '/' + resource: this.aliasName, + }); + } + + public get keyId(): string { + return this.aliasName; + } + + public addAlias(alias: string): Alias { + return this.aliasTargetKey.addAlias(alias); + } + + public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp?: boolean): void { + this.aliasTargetKey.addToResourcePolicy(statement, allowNoOp); + } + + public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant { + return this.aliasTargetKey.grant(grantee, ...actions); + } + + public grantDecrypt(grantee: iam.IGrantable): iam.Grant { + return this.aliasTargetKey.grantDecrypt(grantee); + } + + public grantEncrypt(grantee: iam.IGrantable): iam.Grant { + return this.aliasTargetKey.grantEncrypt(grantee); + } + + public grantEncryptDecrypt(grantee: iam.IGrantable): iam.Grant { + return this.aliasTargetKey.grantEncryptDecrypt(grantee); + } } export interface AliasAttributes { @@ -79,8 +124,6 @@ export class Alias extends AliasBase { public readonly aliasTargetKey: IKey; constructor(scope: Construct, id: string, props: AliasProps) { - super(scope, id); - let aliasName = props.aliasName; if (!Token.isUnresolved(aliasName)) { @@ -92,7 +135,7 @@ export class Alias extends AliasBase { throw new Error(`Alias must include a value after "${REQUIRED_ALIAS_PREFIX}": ${aliasName}`); } - if (aliasName.startsWith(DISALLOWED_PREFIX)) { + if (aliasName.toLocaleLowerCase().startsWith(DISALLOWED_PREFIX)) { throw new Error(`Alias cannot start with ${DISALLOWED_PREFIX}: ${aliasName}`); } @@ -101,11 +144,25 @@ export class Alias extends AliasBase { } } + super(scope, id, { + physicalName: aliasName, + }); + + this.aliasTargetKey = props.targetKey; + const resource = new CfnAlias(this, 'Resource', { - aliasName, - targetKeyId: props.targetKey.keyArn + aliasName: this.physicalName, + targetKeyId: this.aliasTargetKey.keyArn }); - this.aliasName = resource.aliasName; + this.aliasName = this.getResourceNameAttribute(resource.aliasName); + + if (props.removalPolicy) { + resource.applyRemovalPolicy(props.removalPolicy); + } + } + + protected generatePhysicalName(): string { + return REQUIRED_ALIAS_PREFIX + super.generatePhysicalName(); } } diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 2d6cb9cc27e51..c8406e1429c04 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -15,6 +15,14 @@ export interface IKey extends IResource { */ readonly keyArn: string; + /** + * The ID of the key + * (the part that looks something like: 1234abcd-12ab-34cd-56ef-1234567890ab). + * + * @attribute + */ + readonly keyId: string; + /** * Defines a new alias for the key. */ @@ -56,6 +64,8 @@ abstract class KeyBase extends Resource implements IKey { */ public abstract readonly keyArn: string; + public abstract readonly keyId: string; + /** * Optional policy document that represents the resource policy of this key. * @@ -109,12 +119,30 @@ abstract class KeyBase extends Resource implements IKey { * must not be empty and so default grants won't work. */ public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant { + // KMS verifies whether the principals included in its key policy actually exist. + // This is a problem if the stack the grantee is part of depends on the key stack + // (as it won't exist before the key policy is attempted to be created). + // In that case, make the account the resource policy principal + const granteeStackDependsOnKeyStack = this.granteeStackDependsOnKeyStack(grantee); + const principal = granteeStackDependsOnKeyStack + ? new iam.AccountPrincipal(granteeStackDependsOnKeyStack) + : grantee.grantPrincipal; + + const crossAccountAccess = this.isGranteeFromAnotherAccount(grantee); + const crossRegionAccess = this.isGranteeFromAnotherRegion(grantee); + const crossEnvironment = crossAccountAccess || crossRegionAccess; return iam.Grant.addToPrincipalAndResource({ grantee, actions, - resourceArns: [this.keyArn], resource: this, - resourceSelfArns: ['*'] + resourcePolicyPrincipal: principal, + + // if the key is used in a cross-environment matter, + // we can't access the Key ARN (they don't have physical names), + // so fall back to using '*'. ToDo we need to make this better... somehow + resourceArns: crossEnvironment ? ['*'] : [this.keyArn], + + resourceSelfArns: crossEnvironment ? undefined : ['*'], }); } @@ -149,6 +177,46 @@ abstract class KeyBase extends Resource implements IKey { 'kms:GenerateDataKey*' ); } + + /** + * Checks whether the grantee belongs to a stack that will be deployed + * after the stack containing this key. + * + * @param grantee the grantee to give permissions to + * @returns the account ID of the grantee stack if its stack does depend on this stack, + * undefined otherwise + */ + private granteeStackDependsOnKeyStack(grantee: iam.IGrantable): string | undefined { + if (!(Construct.isConstruct(grantee))) { + return undefined; + } + const keyStack = Stack.of(this); + const granteeStack = Stack.of(grantee); + if (keyStack === granteeStack) { + return undefined; + } + return granteeStack.dependencies.includes(keyStack) + ? granteeStack.account + : undefined; + } + + private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean { + if (!(Construct.isConstruct(grantee))) { + return false; + } + const bucketStack = Stack.of(this); + const identityStack = Stack.of(grantee); + return bucketStack.region !== identityStack.region; + } + + 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; + } } /** @@ -218,14 +286,27 @@ export class Key extends KeyBase { */ public static fromKeyArn(scope: Construct, id: string, keyArn: string): IKey { class Import extends KeyBase { - public keyArn = keyArn; - protected policy?: iam.PolicyDocument | undefined = undefined; + public readonly keyArn = keyArn; + public readonly keyId: string; + protected readonly policy?: iam.PolicyDocument | undefined = undefined; + + constructor(keyId: string) { + super(scope, id); + + this.keyId = keyId; + } + } + + const keyResourceName = Stack.of(scope).parseArn(keyArn).resourceName; + if (!keyResourceName) { + throw new Error(`KMS key ARN must be in the format 'arn:aws:kms:::key/', got: '${keyArn}'`); } - return new Import(scope, id); + return new Import(keyResourceName); } public readonly keyArn: string; + public readonly keyId: string; protected readonly policy?: PolicyDocument; constructor(scope: Construct, id: string, props: KeyProps = {}) { @@ -246,6 +327,7 @@ export class Key extends KeyBase { }); this.keyArn = resource.attrArn; + this.keyId = resource.ref; resource.applyRemovalPolicy(props.removalPolicy); if (props.alias !== undefined) { diff --git a/packages/@aws-cdk/aws-kms/test/test.alias.ts b/packages/@aws-cdk/aws-kms/test/test.alias.ts index dc2011a561dd9..5a71481745658 100644 --- a/packages/@aws-cdk/aws-kms/test/test.alias.ts +++ b/packages/@aws-cdk/aws-kms/test/test.alias.ts @@ -1,8 +1,10 @@ -import { expect, haveResource } from '@aws-cdk/assert'; -import { App, Stack } from '@aws-cdk/core'; +import { expect, haveResource, SynthUtils } from '@aws-cdk/assert'; +import { App, CfnOutput, Construct, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; -import { Key } from '../lib'; import { Alias } from '../lib/alias'; +import { IKey, Key } from '../lib/key'; + +// tslint:disable:object-literal-key-quotes export = { 'default alias'(test: Test) { @@ -103,21 +105,72 @@ export = { enabled: false }); - test.throws(() => new Alias(stack, 'Alias', { + test.throws(() => new Alias(stack, 'Alias1', { aliasName: 'alias/aws/', targetKey: key - })); + }), /Alias cannot start with alias\/aws\/: alias\/aws\//); - test.throws(() => new Alias(stack, 'Alias', { + test.throws(() => new Alias(stack, 'Alias2', { aliasName: 'alias/aws/Awesome', targetKey: key - })); + }), /Alias cannot start with alias\/aws\/: alias\/aws\/Awesome/); - test.throws(() => new Alias(stack, 'Alias', { + test.throws(() => new Alias(stack, 'Alias3', { aliasName: 'alias/AWS/awesome', targetKey: key - })); + }), /Alias cannot start with alias\/aws\/: alias\/AWS\/awesome/); test.done(); - } + }, + + 'can be used wherever a key is expected'(test: Test) { + const stack = new Stack(); + + const myKey = new Key(stack, 'MyKey', { + enableKeyRotation: true, + enabled: false + }); + const myAlias = new Alias(stack, 'MyAlias', { + targetKey: myKey, + aliasName: 'alias/myAlias', + }); + + class MyConstruct extends Construct { + constructor(scope: Construct, id: string, key: IKey) { + super(scope, id); + + new CfnOutput(stack, 'OutId', { + value: key.keyId, + }); + new CfnOutput(stack, 'OutArn', { + value: key.keyArn, + }); + } + } + + new MyConstruct(stack, 'MyConstruct', myAlias); + + const template = SynthUtils.synthesize(stack).template.Outputs; + + test.deepEqual(template, { + "OutId": { + "Value": "alias/myAlias", + }, + "OutArn": { + "Value": { + "Fn::Join": ["", [ + "arn:", + { Ref: "AWS::Partition" }, + ":kms:", + { Ref: "AWS::Region" }, + ":", + { Ref: "AWS::AccountId" }, + ":alias/myAlias", + ]], + }, + }, + }); + + test.done(); + }, }; diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index a8e6dcd4341fb..ed10ac6a7b010 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -1,9 +1,19 @@ -import { exactlyMatchTemplate, expect, haveResource, ResourcePart } from '@aws-cdk/assert'; +import { + exactlyMatchTemplate, + expect, + haveResource, + haveResourceLike, + ResourcePart, + SynthUtils +} from '@aws-cdk/assert'; import { PolicyStatement, User } from '@aws-cdk/aws-iam'; -import { App, RemovalPolicy, Stack, Tag } from '@aws-cdk/core'; +import iam = require('@aws-cdk/aws-iam'); +import { App, CfnOutput, RemovalPolicy, Stack, Tag } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import { Key } from '../lib'; +// tslint:disable:object-literal-key-quotes + export = { 'default key'(test: Test) { const stack = new Stack(); @@ -475,51 +485,135 @@ export = { test.done(); }, - 'import/export can be used to bring in an existing key'(test: Test) { - const stack2 = new Stack(); - const myKeyImported = Key.fromKeyArn(stack2, 'MyKeyImported', 'arn:of:key'); + 'grant for a principal in a dependent stack works correctly'(test: Test) { + const app = new App(); - // addAlias can be called on imported keys. - myKeyImported.addAlias('alias/hello'); + const principalStack = new Stack(app, 'PrincipalStack'); + const principal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AnyPrincipal(), + }); - expect(stack2).toMatch({ - Resources: { - MyKeyImportedAliasB1C5269F: { - Type: "AWS::KMS::Alias", - Properties: { - AliasName: "alias/hello", - TargetKeyId: 'arn:of:key' - } - } - } + const keyStack = new Stack(app, 'KeyStack'); + const key = new Key(keyStack, 'Key'); + + principalStack.addDependency(keyStack); + + key.grantEncrypt(principal); + + expect(keyStack).to(haveResourceLike('AWS::KMS::Key', { + "KeyPolicy": { + "Statement": [ + { + // owning account management permissions - we don't care about them in this test + }, + { + "Action": [ + "kms:Encrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + ], + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": ["", [ + "arn:", + { "Ref": "AWS::Partition" }, + ":iam::", + { "Ref": "AWS::AccountId" }, + ":root", + ]], + }, + }, + "Resource": "*", + }, + ], + }, + })); + + test.done(); + }, + + 'keyId resolves to a Ref'(test: Test) { + const stack = new Stack(); + const key = new Key(stack, 'MyKey'); + + new CfnOutput(stack, 'Out', { + value: key.keyId, + }); + + const template = SynthUtils.synthesize(stack).template.Outputs; + + test.deepEqual(template, { + "Out": { + "Value": { + "Ref": "MyKey6AB29FA6", + }, + }, }); test.done(); }, - 'addToResourcePolicy allowNoOp and there is no policy': { - 'succeed if set to true (default)'(test: Test) { + 'imported keys': { + 'throw an error when providing something that is not a valid key ARN'(test: Test) { const stack = new Stack(); - const key = Key.fromKeyArn(stack, 'Imported', 'foo/bar'); - - key.addToResourcePolicy(new PolicyStatement({ resources: ['*'], actions: ['*'] })); + test.throws(() => { + Key.fromKeyArn(stack, 'Imported', 'arn:aws:kms:us-east-1:123456789012:key'); + }, /KMS key ARN must be in the format 'arn:aws:kms:::key\/', got: 'arn:aws:kms:us-east-1:123456789012:key'/); test.done(); }, - 'fails if set to false'(test: Test) { + 'can have aliases added to them'(test: Test) { + const stack2 = new Stack(); + const myKeyImported = Key.fromKeyArn(stack2, 'MyKeyImported', + 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); - const stack = new Stack(); + // addAlias can be called on imported keys. + myKeyImported.addAlias('alias/hello'); - const key = Key.fromKeyArn(stack, 'Imported', 'foo/bar'); + test.equal(myKeyImported.keyId, '12345678-1234-1234-1234-123456789012'); - test.throws(() => - key.addToResourcePolicy(new PolicyStatement({ resources: ['*'], actions: ['*'] }), /* allowNoOp */ false), - 'Unable to add statement to IAM resource policy for KMS key: "foo/bar"'); + expect(stack2).toMatch({ + Resources: { + MyKeyImportedAliasB1C5269F: { + Type: "AWS::KMS::Alias", + Properties: { + AliasName: "alias/hello", + TargetKeyId: "arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012" + } + } + } + }); test.done(); + }, + + 'addToResourcePolicy allowNoOp and there is no policy': { + 'succeed if set to true (default)'(test: Test) { + const stack = new Stack(); + + const key = Key.fromKeyArn(stack, 'Imported', + 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); + + key.addToResourcePolicy(new PolicyStatement({ resources: ['*'], actions: ['*'] })); + + test.done(); + }, - } - } + 'fails if set to false'(test: Test) { + const stack = new Stack(); + + const key = Key.fromKeyArn(stack, 'Imported', + 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); + + test.throws(() => { + key.addToResourcePolicy(new PolicyStatement({ resources: ['*'], actions: ['*'] }), /* allowNoOp */ false); + }, 'Unable to add statement to IAM resource policy for KMS key: "foo/bar"'); + + test.done(); + }, + }, + }, }; diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 98d14fe55b46a..120dfe3263697 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -240,6 +240,8 @@ export interface BucketAttributes { * @default false */ readonly bucketWebsiteNewUrlFormat?: boolean; + + readonly encryptionKey?: kms.IKey; } /** @@ -529,18 +531,7 @@ abstract class BucketBase extends Resource implements IBucket { } if (this.encryptionKey) { - if (crossAccountAccess) { - // we can't access the Key ARN (they don't have physical names), - // so fall back on using '*'. ToDo we need to make this better... somehow - iam.Grant.addToPrincipalAndResource({ - actions: keyActions, - grantee, - resourceArns: ['*'], - resource: this.encryptionKey, - }); - } else { - this.encryptionKey.grant(grantee, ...keyActions); - } + this.encryptionKey.grant(grantee, ...keyActions); } return ret; @@ -897,7 +888,7 @@ export class Bucket extends BucketBase { public readonly bucketRegionalDomainName = attrs.bucketRegionalDomainName || `${bucketName}.s3.${region}.${urlSuffix}`; public readonly bucketDualStackDomainName = attrs.bucketDualStackDomainName || `${bucketName}.s3.dualstack.${region}.${urlSuffix}`; public readonly bucketWebsiteNewUrlFormat = newUrlFormat; - public readonly encryptionKey?: kms.IKey; + public readonly encryptionKey = attrs.encryptionKey; public policy?: BucketPolicy = undefined; protected autoCreatePolicy = false; protected disallowPublicAccess = false; diff --git a/packages/@aws-cdk/core/lib/resource.ts b/packages/@aws-cdk/core/lib/resource.ts index e3c618046dc3f..99885d5b04a62 100644 --- a/packages/@aws-cdk/core/lib/resource.ts +++ b/packages/@aws-cdk/core/lib/resource.ts @@ -101,10 +101,14 @@ export abstract class Resource extends Construct implements IResource { } if (!this._physicalName) { - this._physicalName = generatePhysicalName(this); + this._physicalName = this.generatePhysicalName(); } } + protected generatePhysicalName(): string { + return generatePhysicalName(this); + } + /** * Returns an environment-sensitive token that should be used for the * resource's "name" attribute (e.g. `bucket.bucketName`). diff --git a/packages/decdk/test/__snapshots__/synth.test.js.snap b/packages/decdk/test/__snapshots__/synth.test.js.snap index b725565815377..3f3370f18a219 100644 --- a/packages/decdk/test/__snapshots__/synth.test.js.snap +++ b/packages/decdk/test/__snapshots__/synth.test.js.snap @@ -2012,6 +2012,20 @@ Object { "Type": "AWS::KMS::Key", "UpdateReplacePolicy": "Retain", }, + "PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": Object { + "DeletionPolicy": "Retain", + "Properties": Object { + "AliasName": "alias/codepipeline-pipelinepipeline22f2a91d", + "TargetKeyId": Object { + "Fn::GetAtt": Array [ + "PipelineArtifactsBucketEncryptionKey01D58D69", + "Arn", + ], + }, + }, + "Type": "AWS::KMS::Alias", + "UpdateReplacePolicy": "Retain", + }, "PipelineBuildCodePipelineActionRoleD77A08E6": Object { "Properties": Object { "AssumeRolePolicyDocument": Object { @@ -2083,10 +2097,7 @@ Object { "ArtifactStore": Object { "EncryptionKey": Object { "Id": Object { - "Fn::GetAtt": Array [ - "PipelineArtifactsBucketEncryptionKey01D58D69", - "Arn", - ], + "Ref": "PipelineArtifactsBucketEncryptionKey01D58D69", }, "Type": "KMS", },