Skip to content

Commit

Permalink
fix(sqs): encryptionType is incorrect when encryptionMasterKey is pro…
Browse files Browse the repository at this point in the history
…vided (#26886)

In #19796 we added additional validation to sns subscription.
For that purpose the Queue `encryptionType` was exposed as a public property.
However the PR forgot to take into account that the provided `encryption` property is automatically changed when a `encryptionMasterKey` is provided.

This PR ensures that the public `encryptionType` has the correct value.

Additionally, adds a warning for an incorrect configuration scenario where `encryptionMasterKey` is provided together with an `encryption` other than QueueEncryption.KMS.
This feature was supposed to allow users to simply provide an encryption key and have the encryption type being selected automatically.
However it now unintentionally allows for wrong configurations that are silently fixed, e.g. setting QueueEncryption.UNENCRYPTED and providing an encryption key.
The warning keeps backwards compatibility, but instructs users to fix their configuration.

Closes #26719

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
mrgrain committed Aug 25, 2023
1 parent d152d61 commit bf441fa
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 5 deletions.
31 changes: 31 additions & 0 deletions packages/aws-cdk-lib/aws-sns-subscriptions/test/sqs.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Template } from '../../assertions';
import * as kms from '../../aws-kms';
import * as sns from '../../aws-sns';
import * as sqs from '../../aws-sqs';
import { Stack } from '../../core';
import * as subscriptions from '../lib';

test('can add subscription to queue that has encryptionType auto changed', () => {
// GIVEN
const stack = new Stack();
const key = new kms.Key(stack, 'CustomKey');
const queue = new sqs.Queue(stack, 'Queue', {
encryption: sqs.QueueEncryption.KMS_MANAGED,
encryptionMasterKey: key,
});

const someTopic = new sns.Topic(stack, 'Topic');
someTopic.addSubscription(
new subscriptions.SqsSubscription(queue, {
rawMessageDelivery: true,
}),
);

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::SNS::Subscription', {
Endpoint: {
'Fn::GetAtt': ['Queue4A7E3555', 'Arn'],
},
Protocol: 'sqs',
});
});
25 changes: 20 additions & 5 deletions packages/aws-cdk-lib/aws-sqs/lib/queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CfnQueue } from './sqs.generated';
import { validateProps } from './validate-props';
import * as iam from '../../aws-iam';
import * as kms from '../../aws-kms';
import { Duration, RemovalPolicy, Stack, Token, ArnFormat } from '../../core';
import { Duration, RemovalPolicy, Stack, Token, ArnFormat, Annotations } from '../../core';

/**
* Properties for creating a new Queue
Expand Down Expand Up @@ -336,7 +336,7 @@ export class Queue extends QueueBase {
}
: undefined;

const { encryptionMasterKey, encryptionProps } = _determineEncryptionProps.call(this);
const { encryptionMasterKey, encryptionProps, encryptionType } = _determineEncryptionProps.call(this);

const fifoProps = this.determineFifoProps(props);
this.fifo = fifoProps.fifoQueue || false;
Expand All @@ -362,25 +362,37 @@ export class Queue extends QueueBase {
this.encryptionMasterKey = encryptionMasterKey;
this.queueUrl = queue.ref;
this.deadLetterQueue = props.deadLetterQueue;
this.encryptionType = props.encryption;
this.encryptionType = encryptionType;

function _determineEncryptionProps(this: Queue): { encryptionProps: EncryptionProps, encryptionMasterKey?: kms.IKey } {
function _determineEncryptionProps(this: Queue): {
encryptionProps: EncryptionProps,
encryptionMasterKey?: kms.IKey,
encryptionType: QueueEncryption | undefined
} {
let encryption = props.encryption;

if (encryption === QueueEncryption.SQS_MANAGED && props.encryptionMasterKey) {
throw new Error("'encryptionMasterKey' is not supported if encryption type 'SQS_MANAGED' is used");
}

if (encryption !== QueueEncryption.KMS && props.encryptionMasterKey) {
if (encryption !== undefined) {
Annotations.of(this).addWarningV2('@aws-cdk/aws-sqs:queueEncryptionChangedToKMS', [
`encryption: Automatically changed to QueueEncryption.KMS, was: QueueEncryption.${Object.keys(QueueEncryption)[Object.values(QueueEncryption).indexOf(encryption)]}`,
'When encryptionMasterKey is provided, always set `encryption: QueueEncryption.KMS`',
].join('\n'));
}

encryption = QueueEncryption.KMS; // KMS is implied by specifying an encryption key
}

if (!encryption) {
return { encryptionProps: {} };
return { encryptionProps: {}, encryptionType: encryption };
}

if (encryption === QueueEncryption.UNENCRYPTED) {
return {
encryptionType: encryption,
encryptionProps: {
sqsManagedSseEnabled: false,
},
Expand All @@ -389,6 +401,7 @@ export class Queue extends QueueBase {

if (encryption === QueueEncryption.KMS_MANAGED) {
return {
encryptionType: encryption,
encryptionProps: {
kmsMasterKeyId: 'alias/aws/sqs',
kmsDataKeyReusePeriodSeconds: props.dataKeyReuse && props.dataKeyReuse.toSeconds(),
Expand All @@ -402,6 +415,7 @@ export class Queue extends QueueBase {
});

return {
encryptionType: encryption,
encryptionMasterKey: masterKey,
encryptionProps: {
kmsMasterKeyId: masterKey.keyArn,
Expand All @@ -412,6 +426,7 @@ export class Queue extends QueueBase {

if (encryption === QueueEncryption.SQS_MANAGED) {
return {
encryptionType: encryption,
encryptionProps: {
sqsManagedSseEnabled: true,
},
Expand Down
14 changes: 14 additions & 0 deletions packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ describe('queue encryption', () => {
const queue = new sqs.Queue(stack, 'Queue', { encryptionMasterKey: key });

expect(queue.encryptionMasterKey).toEqual(key);
expect(queue.encryptionType).toEqual(sqs.QueueEncryption.KMS);
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
'KmsMasterKeyId': { 'Fn::GetAtt': ['CustomKey1E6D0D07', 'Arn'] },
});
Expand Down Expand Up @@ -492,6 +493,19 @@ describe('queue encryption', () => {
encryptionMasterKey: key,
})).toThrow(/'encryptionMasterKey' is not supported if encryption type 'SQS_MANAGED' is used/);
});

test('encryptionType is always KMS, when an encryptionMasterKey is provided', () => {
// GIVEN
const stack = new Stack();
const key = new kms.Key(stack, 'CustomKey');
const queue = new sqs.Queue(stack, 'Queue', {
encryption: sqs.QueueEncryption.KMS_MANAGED,
encryptionMasterKey: key,
});

// THEN
expect(queue.encryptionType).toBe(sqs.QueueEncryption.KMS);
});
});

describe('encryption in transit', () => {
Expand Down

0 comments on commit bf441fa

Please sign in to comment.