From 348d28fda39f30e94e5eefc742ae100aacc8f0c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 2 Jul 2019 13:50:26 +0200 Subject: [PATCH 1/4] fix(sqs): Do not emit grants to the AWS-managed encryption key Grants on the `alias/aws/sqs` KMS key alias are not necessary since the key will implicitly allow for it's intended usage to be fulfilled (as opposed to how you have to manage grants yourself when using a user-managed key instead). This removes the statement that was generated using an invalid resource entry. Fixes #2794 --- packages/@aws-cdk/aws-sqs/lib/queue.ts | 3 - packages/@aws-cdk/aws-sqs/package.json | 1 + .../aws-sqs/test/integ.sqs.expected.json | 175 +++++++++++++++++- packages/@aws-cdk/aws-sqs/test/integ.sqs.ts | 21 ++- 4 files changed, 191 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/aws-sqs/lib/queue.ts b/packages/@aws-cdk/aws-sqs/lib/queue.ts index 9247c156ef188..c1718eb1eaa45 100644 --- a/packages/@aws-cdk/aws-sqs/lib/queue.ts +++ b/packages/@aws-cdk/aws-sqs/lib/queue.ts @@ -277,10 +277,7 @@ export class Queue extends QueueBase { } if (encryption === QueueEncryption.KMS_MANAGED) { - const masterKey = kms.Key.fromKeyArn(this, 'Key', 'alias/aws/sqs'); - return { - encryptionMasterKey: masterKey, encryptionProps: { kmsMasterKeyId: 'alias/aws/sqs', kmsDataKeyReusePeriodSeconds: props.dataKeyReuse && props.dataKeyReuse.toSeconds() diff --git a/packages/@aws-cdk/aws-sqs/package.json b/packages/@aws-cdk/aws-sqs/package.json index 99df00cbba6ad..975d50b1978bc 100644 --- a/packages/@aws-cdk/aws-sqs/package.json +++ b/packages/@aws-cdk/aws-sqs/package.json @@ -66,6 +66,7 @@ "license": "Apache-2.0", "devDependencies": { "@aws-cdk/assert": "^0.36.1", + "@aws-cdk/aws-kms": "^0.36.1", "@aws-cdk/aws-s3": "^0.36.1", "aws-sdk": "^2.438.0", "cdk-build-tools": "^0.36.1", diff --git a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json index 8f53d9d476299..c827d2a7aef50 100644 --- a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json +++ b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json @@ -6,6 +6,7 @@ "Queue4A7E3555": { "Type": "AWS::SQS::Queue", "Properties": { + "KmsMasterKeyId": "alias/aws/sqs", "RedrivePolicy": { "deadLetterTargetArn": { "Fn::GetAtt": [ @@ -17,10 +18,182 @@ } } }, + "EncryptionKey1B843E66": { + "Type": "AWS::KMS::Key", + "Properties": { + "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" + ], + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":iam::", + { + "Ref": "AWS::AccountId" + }, + ":root" + ] + ] + } + }, + "Resource": "*" + }, + { + "Action": "kms:Decrypt", + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::GetAtt": [ + "Role1ABCC5F0", + "Arn" + ] + } + }, + "Resource": "*" + } + ], + "Version": "2012-10-17" + } + }, + "DeletionPolicy": "Delete" + }, "FifoQueueE5FF7273": { "Type": "AWS::SQS::Queue", "Properties": { - "FifoQueue": true + "FifoQueue": true, + "KmsMasterKeyId": { + "Fn::GetAtt": [ + "EncryptionKey1B843E66", + "Arn" + ] + } + } + }, + "Role1ABCC5F0": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Statement": [ + { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":iam::", + { + "Ref": "AWS::AccountId" + }, + ":root" + ] + ] + } + } + } + ], + "Version": "2012-10-17" + } + } + }, + "RoleDefaultPolicy5FFB7DAB": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "sqs:ReceiveMessage", + "sqs:ChangeMessageVisibility", + "sqs:GetQueueUrl", + "sqs:DeleteMessage", + "sqs:GetQueueAttributes" + ], + "Effect": "Allow", + "Resource": { + "Fn::GetAtt": [ + "DeadLetterQueue9F481546", + "Arn" + ] + } + }, + { + "Action": [ + "sqs:ReceiveMessage", + "sqs:ChangeMessageVisibility", + "sqs:GetQueueUrl", + "sqs:DeleteMessage", + "sqs:GetQueueAttributes" + ], + "Effect": "Allow", + "Resource": { + "Fn::GetAtt": [ + "Queue4A7E3555", + "Arn" + ] + } + }, + { + "Action": [ + "sqs:ReceiveMessage", + "sqs:ChangeMessageVisibility", + "sqs:GetQueueUrl", + "sqs:DeleteMessage", + "sqs:GetQueueAttributes" + ], + "Effect": "Allow", + "Resource": { + "Fn::GetAtt": [ + "FifoQueueE5FF7273", + "Arn" + ] + } + }, + { + "Action": "kms:Decrypt", + "Effect": "Allow", + "Resource": { + "Fn::GetAtt": [ + "EncryptionKey1B843E66", + "Arn" + ] + } + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "RoleDefaultPolicy5FFB7DAB", + "Roles": [ + { + "Ref": "Role1ABCC5F0" + } + ] } } }, diff --git a/packages/@aws-cdk/aws-sqs/test/integ.sqs.ts b/packages/@aws-cdk/aws-sqs/test/integ.sqs.ts index c17b3f423dbea..5e0136e750ede 100644 --- a/packages/@aws-cdk/aws-sqs/test/integ.sqs.ts +++ b/packages/@aws-cdk/aws-sqs/test/integ.sqs.ts @@ -1,5 +1,7 @@ -import { App, CfnOutput, Stack } from '@aws-cdk/core'; -import { Queue } from '../lib'; +import { AccountRootPrincipal, Role } from '@aws-cdk/aws-iam'; +import { Key } from '@aws-cdk/aws-kms'; +import { App, CfnOutput, RemovalPolicy, Stack } from '@aws-cdk/core'; +import { Queue, QueueEncryption } from '../lib'; const app = new App(); @@ -7,13 +9,22 @@ const stack = new Stack(app, 'aws-cdk-sqs'); const dlq = new Queue(stack, 'DeadLetterQueue'); const queue = new Queue(stack, 'Queue', { - deadLetterQueue: { queue: dlq, maxReceiveCount: 5 } + deadLetterQueue: { queue: dlq, maxReceiveCount: 5 }, + encryption: QueueEncryption.KMS_MANAGED, +}); +const fifo = new Queue(stack, 'FifoQueue', { + fifo: true, + encryptionMasterKey: new Key(stack, 'EncryptionKey', { removalPolicy: RemovalPolicy.DESTROY }) }); -new Queue(stack, 'FifoQueue', { - fifo: true +const role = new Role(stack, 'Role', { + assumedBy: new AccountRootPrincipal(), }); +dlq.grantConsumeMessages(role); +queue.grantConsumeMessages(role); +fifo.grantConsumeMessages(role); + new CfnOutput(stack, 'QueueUrl', { value: queue.queueUrl }); app.synth(); From 58c040ffc6c8823106d6100042b32eb2def0a74a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 8 Jul 2019 10:29:29 +0200 Subject: [PATCH 2/4] remove test that is bound to fail --- .../@aws-cdk/aws-s3-notifications/test/queue.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/@aws-cdk/aws-s3-notifications/test/queue.test.ts b/packages/@aws-cdk/aws-s3-notifications/test/queue.test.ts index fa17bc698c883..a7df8db8fddd3 100644 --- a/packages/@aws-cdk/aws-s3-notifications/test/queue.test.ts +++ b/packages/@aws-cdk/aws-s3-notifications/test/queue.test.ts @@ -139,12 +139,3 @@ test('if the queue is encrypted with a custom kms key, the key resource policy i Description: "Created by Queue" }); }); - -test('fails if trying to subscribe to a queue with managed kms encryption', () => { - const stack = new Stack(); - const queue = new sqs.Queue(stack, 'Queue', { encryption: sqs.QueueEncryption.KMS_MANAGED }); - const bucket = new s3.Bucket(stack, 'Bucket'); - expect(() => { - bucket.addObjectRemovedNotification(new notif.SqsDestination(queue)); - }).toThrow('Unable to add statement to IAM resource policy for KMS key: "alias/aws/sqs"'); -}); From 09a29e15cf7ffa9ff58d9e2843dd89ea70145e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 5 Aug 2019 08:12:26 -0700 Subject: [PATCH 3/4] Fixup test outcomes --- packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json index c827d2a7aef50..29a6d24e4bffd 100644 --- a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json +++ b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json @@ -76,6 +76,7 @@ "Version": "2012-10-17" } }, + "UpdateReplacePolicy": "Delete", "DeletionPolicy": "Delete" }, "FifoQueueE5FF7273": { From afa843176d982e96abe76b3a577a2271cf3723cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 5 Aug 2019 09:04:59 -0700 Subject: [PATCH 4/4] fix test expectations --- packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json index 29a6d24e4bffd..ae5da0f4d0525 100644 --- a/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json +++ b/packages/@aws-cdk/aws-sqs/test/integ.sqs.expected.json @@ -36,7 +36,8 @@ "kms:Get*", "kms:Delete*", "kms:ScheduleKeyDeletion", - "kms:CancelKeyDeletion" + "kms:CancelKeyDeletion", + "kms:GenerateDataKey" ], "Effect": "Allow", "Principal": {