Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jul 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9516f66
fix(sns-subscriptions): SQS queue encrypted by AWS managed KMS key is…
tam0ri Jun 25, 2023
04ad98e
Add encryptionType property.
tam0ri Jul 18, 2023
fff3d75
Merge branch 'main' into fix/validate-sqs-subscription-encrypted-by-m…
mergify[bot] Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/aws-cdk-lib/aws-sns-subscriptions/lib/sqs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,18 @@ export class SqsSubscription implements sns.ITopicSubscription {
}
const snsServicePrincipal = new iam.ServicePrincipal('sns.amazonaws.com');

// 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tam0ri tam0ri Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think so.

throw new Error('SQS queue encrypted by AWS managed KMS key cannot be used as SNS subscription');
}

// 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

throw new Error('SQS queue encrypted by AWS managed KMS key cannot be used as dead-letter queue');
}

// add a statement to the queue resource policy which allows this topic
// to send messages to the queue.
const queuePolicyDependable = this.queue.addToResourcePolicy(new iam.PolicyStatement({
Expand Down
25 changes: 25 additions & 0 deletions packages/aws-cdk-lib/aws-sns-subscriptions/test/subs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,31 @@ describe('Restrict sqs decryption feature flag', () => {
});
});

test('throws an error when a queue is encrypted by AWS managed KMS kye for queue subscription', () => {
// WHEN
const queue = new sqs.Queue(stack, 'MyQueue', {
encryption: sqs.QueueEncryption.KMS_MANAGED,
});

// THEN
expect(() => topic.addSubscription(new subs.SqsSubscription(queue)))
.toThrowError(/SQS queue encrypted by AWS managed KMS key cannot be used as SNS subscription/);
});

test('throws an error when a dead-letter queue is encrypted by AWS managed KMS kye for queue subscription', () => {
// WHEN
const queue = new sqs.Queue(stack, 'MyQueue');
const dlq = new sqs.Queue(stack, 'MyDLQ', {
encryption: sqs.QueueEncryption.KMS_MANAGED,
});

// THEN
expect(() => topic.addSubscription(new subs.SqsSubscription(queue, {
deadLetterQueue: dlq,
})))
.toThrowError(/SQS queue encrypted by AWS managed KMS key cannot be used as dead-letter queue/);
});

test('lambda subscription', () => {
const func = new lambda.Function(stack, 'MyFunc', {
runtime: lambda.Runtime.NODEJS_14_X,
Expand Down
Loading