fix(sns-subscriptions): SQS queue encrypted by AWS managed KMS key is allowed to be specified as subscription and dead-letter queue by tam0ri · Pull Request #26110 · aws/aws-cdk

tam0ri · 2023-06-25T15:07:29Z

To send message to SQS queues encrypted by KMS from SNS, we need to grant SNS service-principal access to the key by key policy. From this reason, we need to use customer managed key because we can't edit key policy for AWS managed key. However, CDK makes it easy to create such a non-functional subscription.

To prevent CDK from making such a subscription, I added the validation which throw an error when SQS queue encrypted by AWS managed KMS key is specified as subscription or dead-letter queue.

Closes #19796

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

… allowed to be specified as subscription and dead-letter queue.

gitpod-io · 2023-06-25T15:07:38Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

tam0ri · 2023-06-25T15:13:50Z

Exemption Request because this change includes only additional validations. Please let me know if adding integration test is needed.

corymhall

Looks good! Just 1 comment

corymhall · 2023-06-27T10:42:51Z

packages/aws-cdk-lib/aws-sns-subscriptions/lib/sqs.ts


+    // if the queue is encrypted by AWS managed KMS key (alias/aws/sqs),
+    // throw error message
+    if ((this.queue.node.defaultChild as sqs.CfnQueue).kmsMasterKeyId == 'alias/aws/sqs') {


We also need to handle the case where the queue is imported IQueue in which
case there will not be a default child.

I think we may want to add a new attribute to the IQueue, something like
encryptionType that holds the type of encryption.

Hi @corymhall, thank you for your review! I understand your concern.

If users import the queue via fromQueueArn method, CDK cannot determine whether the queue is encrypted by AWS managed KMS key or not.

aws-cdk/packages/aws-cdk-lib/aws-sqs/lib/queue.ts

Lines 268 to 287 in f8a94d8

public static fromQueueArn(scope: Construct, id: string, queueArn: string): IQueue {

return Queue.fromQueueAttributes(scope, id, { queueArn });

}

/**

* Import an existing queue

*/

public static fromQueueAttributes(scope: Construct, id: string, attrs: QueueAttributes): IQueue {

const stack = Stack.of(scope);

const parsedArn = stack.splitArn(attrs.queueArn, ArnFormat.NO_RESOURCE_NAME);

const queueName = attrs.queueName || parsedArn.resource;

const queueUrl = attrs.queueUrl || `https://sqs.${parsedArn.region}.${stack.urlSuffix}/${parsedArn.account}/${queueName}`;

class Import extends QueueBase {

public readonly queueArn = attrs.queueArn; // arn:aws:sqs:us-east-1:123456789012:queue1

public readonly queueUrl = queueUrl;

public readonly queueName = queueName;

public readonly encryptionMasterKey = attrs.keyArn

? kms.Key.fromKeyArn(this, 'Key', attrs.keyArn)

: undefined;

So in my understanding, we can't validate it completely in that case. I think one possible solution is skipping the validation if encryptionMasterKey is undefined to prevent error due to missing default child. Does it make sense?

I believe users can provide the key in fromQueueAttributes so if the encryptionType is undefined then we don't do the validation.

Please let me clarify. In you assumption, what type of value encryptionType has? Is the same value as QueueEncryption enum?
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sqs.QueueEncryption.html

Yeah I think so.

corymhall · 2023-06-27T10:43:09Z

packages/aws-cdk-lib/aws-sns-subscriptions/lib/sqs.ts

+
+    // if the dead-letter queue is encrypted by AWS managed KMS key (alias/aws/sqs),
+    // throw error message
+    if (this.props.deadLetterQueue && (this.props.deadLetterQueue.node.defaultChild as sqs.CfnQueue).kmsMasterKeyId == 'alias/aws/sqs') {


Same comment as above

mrgrain · 2023-07-10T12:55:33Z

@tam0ri Are you still able to address the outstanding comments?

tam0ri · 2023-07-11T13:44:55Z

@mrgrain
Sorry for delay. I still have a motivation to address this. I develop in my spare time, but I have not been able to take a time recently. I'll try reserving a time for this in this week. Can I take some additional time?

mrgrain · 2023-07-12T10:59:28Z

@mrgrain
Sorry for delay. I still have a motivation to address this. I develop in my spare time, but I have not been able to take a time recently. I'll try reserving a time for this in this week. Can I take some additional time?

Yes of course! Take as much time you need. If the automation closes this PR, just open a new one and feel free to tag me in it. 😃

aws-cdk-automation · 2023-07-19T00:13:01Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

tam0ri · 2023-07-19T15:27:33Z

@corymhall Sorry for delay, I made some changes per your suggestion. Could you review it again?

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2023-07-24T12:20:52Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…anaged-key

aws-cdk-automation · 2023-07-24T12:59:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: fff3d75
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-07-24T12:59:44Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

… allowed to be specified as subscription and dead-letter queue (aws#26110) To send message to SQS queues encrypted by KMS from SNS, we need to grant SNS service-principal access to the key by key policy. From this reason, we need to use customer managed key because we can't edit key policy for AWS managed key. However, CDK makes it easy to create such a non-functional subscription. To prevent CDK from making such a subscription, I added the validation which throw an error when SQS queue encrypted by AWS managed KMS key is specified as subscription or dead-letter queue. Closes aws#19796 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…S key is allowed to be specified as subscription and dead-letter queue (#26110)" This reverts commit 0531492.

fix(sns-subscriptions): SQS queue encrypted by AWS managed KMS key is…

9516f66

… allowed to be specified as subscription and dead-letter queue.

aws-cdk-automation requested a review from a team June 25, 2023 15:07

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jun 25, 2023

aws-cdk-automation previously requested changes Jun 25, 2023

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jun 25, 2023

corymhall suggested changes Jun 27, 2023

View reviewed changes

corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 27, 2023

corymhall self-assigned this Jun 27, 2023

Add encryptionType property.

04ad98e

tam0ri requested a review from corymhall July 18, 2023 15:00

corymhall approved these changes Jul 24, 2023

View reviewed changes

corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 24, 2023

Merge branch 'main' into fix/validate-sqs-subscription-encrypted-by-m…

fff3d75

…anaged-key

mergify bot merged commit 0531492 into aws:main Jul 24, 2023

github-actions bot mentioned this pull request Jul 26, 2023

Monthly PRs metrics report - June 2023 #26516

Closed

mrgrain added a commit that referenced this pull request Aug 24, 2023

Revert "fix(sns-subscriptions): SQS queue encrypted by AWS managed KM…

dde7212

…S key is allowed to be specified as subscription and dead-letter queue (#26110)" This reverts commit 0531492.

mrgrain mentioned this pull request Aug 24, 2023

fix(sns-subscriptions): revert "fix(sns-subscriptions): SQS queue encrypted by AWS managed KMS key is allowed to be specified as subscription and dead-letter queue" #26869

Closed

	public static fromQueueArn(scope: Construct, id: string, queueArn: string): IQueue {
	return Queue.fromQueueAttributes(scope, id, { queueArn });
	}

	/**
	* Import an existing queue
	*/
	public static fromQueueAttributes(scope: Construct, id: string, attrs: QueueAttributes): IQueue {
	const stack = Stack.of(scope);
	const parsedArn = stack.splitArn(attrs.queueArn, ArnFormat.NO_RESOURCE_NAME);
	const queueName = attrs.queueName \|\| parsedArn.resource;
	const queueUrl = attrs.queueUrl \|\| `https://sqs.${parsedArn.region}.${stack.urlSuffix}/${parsedArn.account}/${queueName}`;

	class Import extends QueueBase {
	public readonly queueArn = attrs.queueArn; // arn:aws:sqs:us-east-1:123456789012:queue1
	public readonly queueUrl = queueUrl;
	public readonly queueName = queueName;
	public readonly encryptionMasterKey = attrs.keyArn
	? kms.Key.fromKeyArn(this, 'Key', attrs.keyArn)
	: undefined;

Conversation

tam0ri commented Jun 25, 2023

Uh oh!

gitpod-io bot commented Jun 25, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

tam0ri commented Jun 25, 2023

Uh oh!

corymhall left a comment

Choose a reason for hiding this comment

Uh oh!

corymhall Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

tam0ri Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corymhall Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

tam0ri Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

corymhall Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

corymhall Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain commented Jul 10, 2023

Uh oh!

tam0ri commented Jul 11, 2023

Uh oh!

mrgrain commented Jul 12, 2023

Uh oh!

aws-cdk-automation commented Jul 19, 2023

Uh oh!

tam0ri commented Jul 19, 2023

Uh oh!

mergify bot commented Jul 24, 2023

Uh oh!

aws-cdk-automation commented Jul 24, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tam0ri Jun 27, 2023 •

edited

Loading