From b8f306662b6a72359cd3bb2ebc15c317bc95b05e Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Sat, 28 Dec 2019 23:35:14 -0800 Subject: [PATCH 1/8] feat(kms): add property to enable IAM policy key administration --- packages/@aws-cdk/aws-kms/lib/key.ts | 46 +++++++++++++--------- packages/@aws-cdk/aws-kms/test/test.key.ts | 27 +++++++++++++ 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 44f7c267027c0..1870ec61f0acc 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -268,6 +268,13 @@ export interface KeyProps { * @default RemovalPolicy.Retain */ readonly removalPolicy?: RemovalPolicy; + + /** + * Whether the key usage can be granted by IAM policies + * + * @default false + */ + readonly enablePolicyControl?: boolean; } /** @@ -315,7 +322,7 @@ export class Key extends KeyBase { this.policy = props.policy; } else { this.policy = new iam.PolicyDocument(); - this.allowAccountToAdmin(); + this.allowAccountToAdmin(props.enablePolicyControl || false); } const resource = new CfnKey(this, 'Resource', { @@ -335,25 +342,28 @@ export class Key extends KeyBase { } /** - * Let users from this account admin this key. + * Let users or IAM policies from this account admin this key. + * @link https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default * @link https://aws.amazon.com/premiumsupport/knowledge-center/update-key-policy-future/ */ - private allowAccountToAdmin() { - const actions = [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ]; + private allowAccountToAdmin(enablePolicyControl: boolean) { + const actions = enablePolicyControl ? + ['kms:*'] : + [ + "kms:Create*", + "kms:Describe*", + "kms:Enable*", + "kms:List*", + "kms:Put*", + "kms:Update*", + "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" + ]; this.addToResourcePolicy(new iam.PolicyStatement({ resources: ['*'], diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index a8833a61a9513..6556a0d0bdd72 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -552,6 +552,33 @@ export = { test.done(); }, + 'enablePolicyControl changes key policy to allow IAM control'(test: Test) { + const stack = new Stack(); + new Key(stack, 'MyKey', {enablePolicyControl: true}); + expect(stack).to(haveResourceLike('AWS::KMS::Key', { + "KeyPolicy": { + "Statement": [ + { + "Action": "kms:*", + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": ["", [ + "arn:", + { "Ref": "AWS::Partition" }, + ":iam::", + { "Ref": "AWS::AccountId" }, + ":root", + ]], + }, + }, + "Resource": "*", + }, + ], + }, + })); + test.done(); + }, 'imported keys': { 'throw an error when providing something that is not a valid key ARN'(test: Test) { From 0232fc16d362cd752634e6df5c53ba6cf32cea87 Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Sun, 19 Jan 2020 23:50:57 -0800 Subject: [PATCH 2/8] fix: update key policies to include admin functions, rename to trustAccountIdentities for identity based kms key control --- packages/@aws-cdk/aws-kms/lib/key.ts | 98 +++++-- .../test/integ.key-sharing.lit.expected.json | 16 +- .../aws-kms/test/integ.key.expected.json | 16 +- packages/@aws-cdk/aws-kms/test/test.key.ts | 276 ++++++------------ 4 files changed, 180 insertions(+), 226 deletions(-) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 1870ec61f0acc..165b44d563745 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -73,6 +73,14 @@ abstract class KeyBase extends Resource implements IKey { */ protected abstract readonly policy?: iam.PolicyDocument; + /** + * Optional property to control trusting account identities. + * + * If specified grants will default identity policies instead of to both + * resource and identity policies. + */ + protected abstract readonly trustAccountIdentities: boolean; + /** * Collection of aliases added to the key * @@ -130,19 +138,25 @@ abstract class KeyBase extends Resource implements IKey { const crossAccountAccess = this.isGranteeFromAnotherAccount(grantee); const crossRegionAccess = this.isGranteeFromAnotherRegion(grantee); const crossEnvironment = crossAccountAccess || crossRegionAccess; - return iam.Grant.addToPrincipalAndResource({ + const grantOptions: iam.GrantWithResourceOptions = { grantee, actions, resource: this, - 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], - + resourceArns: [this.keyArn], resourceSelfArns: crossEnvironment ? undefined : ['*'], - }); + }; + if (this.trustAccountIdentities) { + return iam.Grant.addToPrincipalOrResource(grantOptions); + } else { + return iam.Grant.addToPrincipalAndResource({ + ...grantOptions, + // 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], + resourcePolicyPrincipal: principal, + }); + } } /** @@ -272,9 +286,13 @@ export interface KeyProps { /** * Whether the key usage can be granted by IAM policies * + * Setting this to true adds a default statement which delegates key + * access control completely to the identity's IAM policy (similar + * to how it works for other AWS resources). + * * @default false */ - readonly enablePolicyControl?: boolean; + readonly trustAccountIdentities?: boolean; } /** @@ -295,6 +313,10 @@ export class Key extends KeyBase { public readonly keyArn = keyArn; public readonly keyId: string; protected readonly policy?: iam.PolicyDocument | undefined = undefined; + // defaulting true: if we are importing they key the key policy is + // undefined and impossible to change here; this means updating identity + // policies is really the only option + protected readonly trustAccountIdentities: boolean = true; constructor(keyId: string) { super(scope, id); @@ -314,15 +336,17 @@ export class Key extends KeyBase { public readonly keyArn: string; public readonly keyId: string; protected readonly policy?: iam.PolicyDocument; + protected readonly trustAccountIdentities: boolean; constructor(scope: Construct, id: string, props: KeyProps = {}) { super(scope, id); - if (props.policy) { - this.policy = props.policy; + this.policy = props.policy || new iam.PolicyDocument(); + this.trustAccountIdentities = props.trustAccountIdentities || false; + if (this.trustAccountIdentities) { + this.allowAccountIdentitiesToControl(); } else { - this.policy = new iam.PolicyDocument(); - this.allowAccountToAdmin(props.enablePolicyControl || false); + this.allowAccountToAdmin(); } const resource = new CfnKey(this, 'Resource', { @@ -341,29 +365,39 @@ export class Key extends KeyBase { } } + private allowAccountIdentitiesToControl() { + this.addToResourcePolicy(new iam.PolicyStatement({ + resources: ['*'], + actions: ['kms:*'], + principals: [new iam.AccountRootPrincipal()] + })); + + } /** * Let users or IAM policies from this account admin this key. * @link https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default * @link https://aws.amazon.com/premiumsupport/knowledge-center/update-key-policy-future/ */ - private allowAccountToAdmin(enablePolicyControl: boolean) { - const actions = enablePolicyControl ? - ['kms:*'] : - [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ]; + private allowAccountToAdmin() { + const actions = [ + "kms:CancelKeyDeletion", + "kms:Create*", + "kms:Delete*", + "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", + "kms:Enable*", + "kms:GenerateDataKey", // this is not an admin action; leaving for backwards compatibility + "kms:Get*", + "kms:ImportKeyMaterial", + "kms:List*", + "kms:Put*", + "kms:Revoke*", + "kms:ScheduleKeyDeletion", + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", + ]; this.addToResourcePolicy(new iam.PolicyStatement({ resources: ['*'], diff --git a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json index 8d410c358bd61..acebd96fbcafb 100644 --- a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json +++ b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json @@ -8,19 +8,23 @@ "Statement": [ { "Action": [ + "kms:CancelKeyDeletion", "kms:Create*", + "kms:Delete*", "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", - "kms:Update*", "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:TagResource", + "kms:UntagResource", + "kms:Update*" ], "Effect": "Allow", "Principal": { diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json index 1620e693e2dfc..18ce6d4b8cde8 100644 --- a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json +++ b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json @@ -7,19 +7,23 @@ "Statement": [ { "Action": [ + "kms:CancelKeyDeletion", "kms:Create*", + "kms:Delete*", "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", - "kms:Update*", "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:TagResource", + "kms:UntagResource", + "kms:Update*" ], "Effect": "Allow", "Principal": { diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index 6556a0d0bdd72..2f92d65828a97 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -1,4 +1,5 @@ import { + countResources, exactlyMatchTemplate, expect, haveResource, @@ -27,21 +28,25 @@ export = { KeyPolicy: { Statement: [ { - Action: [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ], + Action: [ + "kms:CancelKeyDeletion", + "kms:Create*", + "kms:Delete*", + "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", + "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", + "kms:List*", + "kms:Put*", + "kms:Revoke*", + "kms:ScheduleKeyDeletion", + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", + ], Effect: "Allow", Principal: { AWS: { @@ -103,19 +108,23 @@ export = { Statement: [ { Action: [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:CancelKeyDeletion", + "kms:Create*", + "kms:Delete*", + "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", + "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", + "kms:List*", + "kms:Put*", + "kms:Revoke*", + "kms:ScheduleKeyDeletion", + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", ], Effect: "Allow", Principal: { @@ -183,19 +192,23 @@ export = { Statement: [ { Action: [ + "kms:CancelKeyDeletion", "kms:Create*", + "kms:Delete*", "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", - "kms:Update*", "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", ], Effect: "Allow", Principal: { @@ -267,74 +280,16 @@ export = { const alias = key.addAlias('alias/xoo'); test.ok(alias.aliasName); - expect(stack).toMatch({ - Resources: { - MyKey6AB29FA6: { - Type: "AWS::KMS::Key", - Properties: { - EnableKeyRotation: true, - Enabled: false, - KeyPolicy: { - Statement: [ - { - Action: [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ], - Effect: "Allow", - Principal: { - AWS: { - "Fn::Join": [ - "", - [ - "arn:", - { - Ref: "AWS::Partition" - }, - ":iam::", - { - Ref: "AWS::AccountId" - }, - ":root" - ] - ] - } - }, - Resource: "*" - } - ], - Version: "2012-10-17" - } - }, - DeletionPolicy: "Retain", - UpdateReplacePolicy: "Retain", - }, - MyKeyAlias1B45D9DA: { - Type: "AWS::KMS::Alias", - Properties: { - AliasName: "alias/xoo", - TargetKeyId: { - "Fn::GetAtt": [ - "MyKey6AB29FA6", - "Arn" - ] - } - } - } + expect(stack).to(countResources('AWS::KMS::Alias', 1)); + expect(stack).to(haveResource('AWS::KMS::Alias', { + AliasName: "alias/xoo", + TargetKeyId: { + "Fn::GetAtt": [ + "MyKey6AB29FA6", + "Arn" + ] } - }); - + })); test.done(); }, @@ -352,86 +307,25 @@ export = { test.ok(alias1.aliasName); test.ok(alias2.aliasName); - expect(stack).toMatch({ - Resources: { - MyKey6AB29FA6: { - Type: "AWS::KMS::Key", - Properties: { - EnableKeyRotation: true, - Enabled: false, - KeyPolicy: { - Statement: [ - { - Action: [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ], - Effect: "Allow", - Principal: { - AWS: { - "Fn::Join": [ - "", - [ - "arn:", - { - Ref: "AWS::Partition" - }, - ":iam::", - { - Ref: "AWS::AccountId" - }, - ":root" - ] - ] - } - }, - Resource: "*" - } - ], - Version: "2012-10-17" - } - }, - DeletionPolicy: "Retain", - UpdateReplacePolicy: "Retain", - }, - MyKeyAlias1B45D9DA: { - Type: "AWS::KMS::Alias", - Properties: { - AliasName: "alias/alias1", - TargetKeyId: { - "Fn::GetAtt": [ - "MyKey6AB29FA6", - "Arn" - ] - } - } - }, - MyKeyAliasaliasalias2EC56BD3E: { - Type: "AWS::KMS::Alias", - Properties: { - AliasName: "alias/alias2", - TargetKeyId: { - "Fn::GetAtt": [ - "MyKey6AB29FA6", - "Arn" - ] - } - } - } + expect(stack).to(countResources('AWS::KMS::Alias', 2)); + expect(stack).to(haveResource('AWS::KMS::Alias', { + AliasName: 'alias/alias1', + TargetKeyId: { + "Fn::GetAtt": [ + "MyKey6AB29FA6", + "Arn" + ] } - }); - + })); + expect(stack).to(haveResource('AWS::KMS::Alias', { + AliasName: 'alias/alias2', + TargetKeyId: { + "Fn::GetAtt": [ + "MyKey6AB29FA6", + "Arn" + ] + } + })); test.done(); }, @@ -451,7 +345,25 @@ export = { // This one is there by default { // tslint:disable-next-line:max-line-length - Action: [ "kms:Create*", "kms:Describe*", "kms:Enable*", "kms:List*", "kms:Put*", "kms:Update*", "kms:Revoke*", "kms:Disable*", "kms:Get*", "kms:Delete*", "kms:ScheduleKeyDeletion", "kms:CancelKeyDeletion", "kms:GenerateDataKey" ], + Action: [ + "kms:CancelKeyDeletion", + "kms:Create*", + "kms:Delete*", + "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", + "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", + "kms:List*", + "kms:Put*", + "kms:Revoke*", + "kms:ScheduleKeyDeletion", + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", + ], Effect: "Allow", Principal: { AWS: { "Fn::Join": [ "", [ "arn:", { Ref: "AWS::Partition" }, ":iam::", { Ref: "AWS::AccountId" }, ":root" ] ] } }, Resource: "*" @@ -554,7 +466,7 @@ export = { }, 'enablePolicyControl changes key policy to allow IAM control'(test: Test) { const stack = new Stack(); - new Key(stack, 'MyKey', {enablePolicyControl: true}); + new Key(stack, 'MyKey', {trustAccountIdentities: true}); expect(stack).to(haveResourceLike('AWS::KMS::Key', { "KeyPolicy": { "Statement": [ From 0a145c3be411325dc92c677cd321bd8f2faf7dbc Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Mon, 20 Jan 2020 09:06:27 -0800 Subject: [PATCH 3/8] fix(s3): updating bucket tests for new default kms key policy --- .../aws-s3/test/integ.bucket.expected.json | 34 ++++++------ packages/@aws-cdk/aws-s3/test/test.bucket.ts | 54 ++++++++----------- 2 files changed, 41 insertions(+), 47 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json index e24cd29bc29e5..07b9130f39f04 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json @@ -7,19 +7,23 @@ "Statement": [ { "Action": [ + "kms:CancelKeyDeletion", "kms:Create*", + "kms:Delete*", "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", - "kms:Update*", "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:TagResource", + "kms:UntagResource", + "kms:Update*" ], "Effect": "Allow", "Principal": { @@ -66,13 +70,11 @@ }, "Description": "Created by aws-cdk-s3/MyBucket" }, - "DeletionPolicy": "Retain", - "UpdateReplacePolicy": "Retain" + "UpdateReplacePolicy": "Retain", + "DeletionPolicy": "Retain" }, "MyBucketF68F3FF0": { "Type": "AWS::S3::Bucket", - "DeletionPolicy": "Delete", - "UpdateReplacePolicy": "Delete", "Properties": { "BucketEncryption": { "ServerSideEncryptionConfiguration": [ @@ -89,12 +91,12 @@ } ] } - } + }, + "UpdateReplacePolicy": "Delete", + "DeletionPolicy": "Delete" }, "MyOtherBucket543F3540": { "Type": "AWS::S3::Bucket", - "DeletionPolicy": "Delete", - "UpdateReplacePolicy": "Delete", "Properties": { "BucketEncryption": { "ServerSideEncryptionConfiguration": [ @@ -105,7 +107,9 @@ } ] } - } + }, + "UpdateReplacePolicy": "Delete", + "DeletionPolicy": "Delete" }, "MyUserDC45028B": { "Type": "AWS::IAM::User" @@ -206,4 +210,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index a5e05f4b6e56a..296eb2569c999 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -9,6 +9,25 @@ import * as s3 from '../lib'; // to make it easy to copy & paste from output: // tslint:disable:object-literal-key-quotes +const stdKeyPolicyActions = [ + "kms:CancelKeyDeletion", + "kms:Create*", + "kms:Delete*", + "kms:Describe*", + "kms:Disable*", + "kms:DisconnectCustomKeyStore", + "kms:Enable*", + "kms:GenerateDataKey", + "kms:Get*", + "kms:ImportKeyMaterial", + "kms:List*", + "kms:Put*", + "kms:Revoke*", + "kms:ScheduleKeyDeletion", + "kms:TagResource", + "kms:UntagResource", + "kms:Update*", +]; export = { 'default bucket'(test: Test) { const stack = new cdk.Stack(); @@ -265,21 +284,7 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ], + "Action": stdKeyPolicyActions, "Effect": "Allow", "Principal": { "AWS": { @@ -826,8 +831,7 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": ["kms:Create*", "kms:Describe*", "kms:Enable*", "kms:List*", "kms:Put*", "kms:Update*", - "kms:Revoke*", "kms:Disable*", "kms:Get*", "kms:Delete*", "kms:ScheduleKeyDeletion", "kms:CancelKeyDeletion", "kms:GenerateDataKey"], + "Action": stdKeyPolicyActions, "Effect": "Allow", "Principal": { "AWS": { @@ -869,21 +873,7 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": [ - "kms:Create*", - "kms:Describe*", - "kms:Enable*", - "kms:List*", - "kms:Put*", - "kms:Update*", - "kms:Revoke*", - "kms:Disable*", - "kms:Get*", - "kms:Delete*", - "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion", - "kms:GenerateDataKey" - ], + "Action": stdKeyPolicyActions, "Effect": "Allow", "Principal": { "AWS": { From 8350874c6e6cca83b0dbdbbfd98381120e682511 Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Sat, 1 Feb 2020 22:02:00 -0800 Subject: [PATCH 4/8] fix: revert kms key policy changes --- packages/@aws-cdk/aws-kms/lib/key.ts | 16 +- .../test/integ.key-sharing.lit.expected.json | 16 +- .../aws-kms/test/integ.key.expected.json | 16 +- packages/@aws-cdk/aws-kms/test/test.key.ts | 228 +++++++----------- .../aws-s3/test/integ.bucket.expected.json | 34 ++- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 54 +++-- 6 files changed, 150 insertions(+), 214 deletions(-) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 165b44d563745..65be3ea355b61 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -380,23 +380,19 @@ export class Key extends KeyBase { */ private allowAccountToAdmin() { const actions = [ - "kms:CancelKeyDeletion", "kms:Create*", - "kms:Delete*", "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", "kms:Enable*", - "kms:GenerateDataKey", // this is not an admin action; leaving for backwards compatibility - "kms:Get*", - "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", + "kms:Update*", "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" ]; this.addToResourcePolicy(new iam.PolicyStatement({ diff --git a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json index acebd96fbcafb..8d410c358bd61 100644 --- a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json +++ b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json @@ -8,23 +8,19 @@ "Statement": [ { "Action": [ - "kms:CancelKeyDeletion", "kms:Create*", - "kms:Delete*", "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", + "kms:Update*", "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*" + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" ], "Effect": "Allow", "Principal": { diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json index 18ce6d4b8cde8..1620e693e2dfc 100644 --- a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json +++ b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json @@ -7,23 +7,19 @@ "Statement": [ { "Action": [ - "kms:CancelKeyDeletion", "kms:Create*", - "kms:Delete*", "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", + "kms:Update*", "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*" + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" ], "Effect": "Allow", "Principal": { diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index 2f92d65828a97..35c44e0b34d33 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -13,6 +13,21 @@ import { Test } from 'nodeunit'; import { Key } from '../lib'; // tslint:disable:object-literal-key-quotes +const ACTIONS: string[] = [ + "kms:Create*", + "kms:Describe*", + "kms:Enable*", + "kms:List*", + "kms:Put*", + "kms:Update*", + "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" +]; export = { 'default key'(test: Test) { @@ -28,53 +43,35 @@ export = { KeyPolicy: { Statement: [ { - Action: [ - "kms:CancelKeyDeletion", - "kms:Create*", - "kms:Delete*", - "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", - "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", - "kms:List*", - "kms:Put*", - "kms:Revoke*", - "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", + Action: ACTIONS, + Effect: "Allow", + Principal: { + AWS: { + "Fn::Join": [ + "", + [ + "arn:", + { + Ref: "AWS::Partition" + }, + ":iam::", + { + Ref: "AWS::AccountId" + }, + ":root" + ] + ] + } + }, + Resource: "*" + } ], - Effect: "Allow", - Principal: { - AWS: { - "Fn::Join": [ - "", - [ - "arn:", - { - Ref: "AWS::Partition" - }, - ":iam::", - { - Ref: "AWS::AccountId" - }, - ":root" - ] - ] + Version: "2012-10-17" } - }, - Resource: "*" - } - ], - Version: "2012-10-17" + }, + DeletionPolicy: "Retain", + UpdateReplacePolicy: "Retain" } - }, - DeletionPolicy: "Retain", - UpdateReplacePolicy: "Retain" - } } })); test.done(); @@ -102,68 +99,50 @@ export = { expect(stack).to(exactlyMatchTemplate({ Resources: { MyKey6AB29FA6: { - Type: "AWS::KMS::Key", - Properties: { - KeyPolicy: { - Statement: [ - { - Action: [ - "kms:CancelKeyDeletion", - "kms:Create*", - "kms:Delete*", - "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", - "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", - "kms:List*", - "kms:Put*", - "kms:Revoke*", - "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", - ], - Effect: "Allow", - Principal: { - AWS: { - "Fn::Join": [ - "", - [ - "arn:", + Type: "AWS::KMS::Key", + Properties: { + KeyPolicy: { + Statement: [ { - Ref: "AWS::Partition" + Action: ACTIONS, + Effect: "Allow", + Principal: { + AWS: { + "Fn::Join": [ + "", + [ + "arn:", + { + Ref: "AWS::Partition" + }, + ":iam::", + { + Ref: "AWS::AccountId" + }, + ":root" + ] + ] + } + }, + Resource: '*' }, - ":iam::", { - Ref: "AWS::AccountId" - }, - ":root" - ] - ] - } - }, - Resource: '*' - }, - { - Action: "kms:encrypt", - Effect: "Allow", - Principal: { - AWS: "arn" - }, - Resource: "*" + Action: "kms:encrypt", + Effect: "Allow", + Principal: { + AWS: "arn" + }, + Resource: "*" + } + ], + Version: "2012-10-17" } - ], - Version: "2012-10-17" - } - }, - DeletionPolicy: "Retain", - UpdateReplacePolicy: "Retain", + }, + DeletionPolicy: "Retain", + UpdateReplacePolicy: "Retain", } } - })); + })); test.done(); }, @@ -191,25 +170,7 @@ export = { KeyPolicy: { Statement: [ { - Action: [ - "kms:CancelKeyDeletion", - "kms:Create*", - "kms:Delete*", - "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", - "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", - "kms:List*", - "kms:Put*", - "kms:Revoke*", - "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", - ], + Action: ACTIONS, Effect: "Allow", Principal: { AWS: { @@ -344,35 +305,16 @@ export = { Statement: [ // This one is there by default { - // tslint:disable-next-line:max-line-length - Action: [ - "kms:CancelKeyDeletion", - "kms:Create*", - "kms:Delete*", - "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", - "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", - "kms:List*", - "kms:Put*", - "kms:Revoke*", - "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", - ], + Action: ACTIONS, Effect: "Allow", - Principal: { AWS: { "Fn::Join": [ "", [ "arn:", { Ref: "AWS::Partition" }, ":iam::", { Ref: "AWS::AccountId" }, ":root" ] ] } }, + Principal: { AWS: { "Fn::Join": ["", ["arn:", { Ref: "AWS::Partition" }, ":iam::", { Ref: "AWS::AccountId" }, ":root"]] } }, Resource: "*" }, // This is the interesting one { Action: "kms:Decrypt", Effect: "Allow", - Principal: { AWS: { "Fn::GetAtt": [ "User00B015A1", "Arn" ] } }, + Principal: { AWS: { "Fn::GetAtt": ["User00B015A1", "Arn"] } }, Resource: "*" } ], @@ -386,7 +328,7 @@ export = { { Action: "kms:Decrypt", Effect: "Allow", - Resource: { "Fn::GetAtt": [ "Key961B73FD", "Arn" ] } + Resource: { "Fn::GetAtt": ["Key961B73FD", "Arn"] } } ], Version: "2012-10-17" @@ -466,7 +408,7 @@ export = { }, 'enablePolicyControl changes key policy to allow IAM control'(test: Test) { const stack = new Stack(); - new Key(stack, 'MyKey', {trustAccountIdentities: true}); + new Key(stack, 'MyKey', { trustAccountIdentities: true }); expect(stack).to(haveResourceLike('AWS::KMS::Key', { "KeyPolicy": { "Statement": [ diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json index 07b9130f39f04..e24cd29bc29e5 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json @@ -7,23 +7,19 @@ "Statement": [ { "Action": [ - "kms:CancelKeyDeletion", "kms:Create*", - "kms:Delete*", "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", "kms:List*", "kms:Put*", + "kms:Update*", "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*" + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" ], "Effect": "Allow", "Principal": { @@ -70,11 +66,13 @@ }, "Description": "Created by aws-cdk-s3/MyBucket" }, - "UpdateReplacePolicy": "Retain", - "DeletionPolicy": "Retain" + "DeletionPolicy": "Retain", + "UpdateReplacePolicy": "Retain" }, "MyBucketF68F3FF0": { "Type": "AWS::S3::Bucket", + "DeletionPolicy": "Delete", + "UpdateReplacePolicy": "Delete", "Properties": { "BucketEncryption": { "ServerSideEncryptionConfiguration": [ @@ -91,12 +89,12 @@ } ] } - }, - "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + } }, "MyOtherBucket543F3540": { "Type": "AWS::S3::Bucket", + "DeletionPolicy": "Delete", + "UpdateReplacePolicy": "Delete", "Properties": { "BucketEncryption": { "ServerSideEncryptionConfiguration": [ @@ -107,9 +105,7 @@ } ] } - }, - "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + } }, "MyUserDC45028B": { "Type": "AWS::IAM::User" @@ -210,4 +206,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 296eb2569c999..a5e05f4b6e56a 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -9,25 +9,6 @@ import * as s3 from '../lib'; // to make it easy to copy & paste from output: // tslint:disable:object-literal-key-quotes -const stdKeyPolicyActions = [ - "kms:CancelKeyDeletion", - "kms:Create*", - "kms:Delete*", - "kms:Describe*", - "kms:Disable*", - "kms:DisconnectCustomKeyStore", - "kms:Enable*", - "kms:GenerateDataKey", - "kms:Get*", - "kms:ImportKeyMaterial", - "kms:List*", - "kms:Put*", - "kms:Revoke*", - "kms:ScheduleKeyDeletion", - "kms:TagResource", - "kms:UntagResource", - "kms:Update*", -]; export = { 'default bucket'(test: Test) { const stack = new cdk.Stack(); @@ -284,7 +265,21 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": stdKeyPolicyActions, + "Action": [ + "kms:Create*", + "kms:Describe*", + "kms:Enable*", + "kms:List*", + "kms:Put*", + "kms:Update*", + "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" + ], "Effect": "Allow", "Principal": { "AWS": { @@ -831,7 +826,8 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": stdKeyPolicyActions, + "Action": ["kms:Create*", "kms:Describe*", "kms:Enable*", "kms:List*", "kms:Put*", "kms:Update*", + "kms:Revoke*", "kms:Disable*", "kms:Get*", "kms:Delete*", "kms:ScheduleKeyDeletion", "kms:CancelKeyDeletion", "kms:GenerateDataKey"], "Effect": "Allow", "Principal": { "AWS": { @@ -873,7 +869,21 @@ export = { "KeyPolicy": { "Statement": [ { - "Action": stdKeyPolicyActions, + "Action": [ + "kms:Create*", + "kms:Describe*", + "kms:Enable*", + "kms:List*", + "kms:Put*", + "kms:Update*", + "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" + ], "Effect": "Allow", "Principal": { "AWS": { From 6e8dd2f00a82d5ad708d2fd66853ec313d76af1e Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Sun, 2 Feb 2020 14:26:14 -0800 Subject: [PATCH 5/8] refactor: update comment --- packages/@aws-cdk/aws-kms/lib/key.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 65be3ea355b61..5b577ef9d6bec 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -313,7 +313,7 @@ export class Key extends KeyBase { public readonly keyArn = keyArn; public readonly keyId: string; protected readonly policy?: iam.PolicyDocument | undefined = undefined; - // defaulting true: if we are importing they key the key policy is + // defaulting true: if we are importing the key the key policy is // undefined and impossible to change here; this means updating identity // policies is really the only option protected readonly trustAccountIdentities: boolean = true; From 177eafc3cab7081995b43aad7ec703f745845330 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 4 Feb 2020 10:25:28 +0100 Subject: [PATCH 6/8] Update key.ts --- packages/@aws-cdk/aws-kms/lib/key.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 5b577ef9d6bec..4bf01796cd5cb 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -291,6 +291,7 @@ export interface KeyProps { * to how it works for other AWS resources). * * @default false + * @see https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam */ readonly trustAccountIdentities?: boolean; } From 2835826e125d1745929fbc37b7ceb9fb4cfa3658 Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Fri, 14 Feb 2020 12:49:40 -0800 Subject: [PATCH 7/8] test: fix from permission changes --- packages/@aws-cdk/aws-kms/test/test.key.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index 35c44e0b34d33..a80a5b8ad839a 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -26,7 +26,9 @@ const ACTIONS: string[] = [ "kms:Delete*", "kms:ScheduleKeyDeletion", "kms:CancelKeyDeletion", - "kms:GenerateDataKey" + "kms:GenerateDataKey", + "kms:TagResource", + "kms:UntagResource", ]; export = { From 50073b363ea7c53cca4d1c3ee038ede45f17465a Mon Sep 17 00:00:00 2001 From: Mike Cowgill Date: Fri, 14 Feb 2020 13:50:32 -0800 Subject: [PATCH 8/8] chore: update kms readme --- packages/@aws-cdk/aws-kms/README.md | 73 +++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/packages/@aws-cdk/aws-kms/README.md b/packages/@aws-cdk/aws-kms/README.md index 548960524c803..c03f4e40a7032 100644 --- a/packages/@aws-cdk/aws-kms/README.md +++ b/packages/@aws-cdk/aws-kms/README.md @@ -29,6 +29,8 @@ key.addAlias('alias/bar'); ### Sharing keys between stacks +> see Trust Account Identities for additional details + To use a KMS key in a different stack in the same CDK application, pass the construct to the other stack: @@ -37,6 +39,8 @@ pass the construct to the other stack: ### Importing existing keys +> see Trust Account Identities for additional details + To use a KMS key that is not defined in this CDK app, but is created through other means, use `Key.fromKeyArn(parent, name, ref)`: @@ -50,3 +54,72 @@ myKeyImported.addAlias('alias/foo'); Note that a call to `.addToPolicy(statement)` on `myKeyImported` will not have an affect on the key's policy because it is not owned by your stack. The call will be a no-op. + +### Trust Account Identities + +KMS keys can be created to trust IAM policies. This is the default behavior in +the console and is described +[here](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html). +This same behavior can be enabled by: + +```ts +new Key(stack, 'MyKey', { trustAccountIdentities: true }); +``` + +Using `trustAccountIdentities` solves many issues around cyclic dependencies +between stacks. The most common use case is creating an S3 Bucket with CMK +default encryption which is later accessed by IAM roles in other stacks. + +stack-1 (bucket and key created) + +```ts +// ... snip +const myKmsKey = new kms.Key(this, 'MyKey', { trustAccountIdentities: true }); + +const bucket = new Bucket(this, 'MyEncryptedBucket', { + bucketName: 'myEncryptedBucket', + encryption: BucketEncryption.KMS, + encryptionKey: myKmsKey +}); +``` + +stack-2 (lambda that operates on bucket and key) + +```ts +// ... snip + +const fn = new lambda.Function(this, 'MyFunction', { + runtime: lambda.Runtime.NODEJS_10_X, + handler: 'index.handler', + code: lambda.Code.fromAsset(path.join(__dirname, 'lambda-handler')), +}); + +const bucket = s3.Bucket.fromBucketName(this, 'BucketId', 'myEncryptedBucket'); + +const key = kms.Key.fromKeyArn(this, 'KeyId', 'arn:aws:...'); // key ARN passed via stack props + +bucket.grantReadWrite(fn); +key.grantEncryptDecrypt(fn); +``` + +The challenge in this scenario is the KMS key policy behavior. The simple way to understand +this, is IAM policies for account entities can only grant the permissions granted to the +account root principle in the key policy. When `trustAccountIdentities` is true, +the following policy statement is added: + +```json +{ + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": {"AWS": "arn:aws:iam::111122223333:root"}, + "Action": "kms:*", + "Resource": "*" +} +``` + +As the name suggests this trusts IAM policies to control access to the key. +If account root does not have permissions to the specific actions, then the key +policy and the IAM policy for the entity (e.g. Lambda) both need to grant +permission. + +