sns_subscriptions: Breaking change from issue 19796 where an exception was added for code that works

[MurraySpeight]

Describe the bug

Merging of change from issue #19796 has caused a breaking change in my CDK project. Creating new issue to create attention to the problem and quoting comment I put on the closed issue below:

@NGL321 @bmoffatt it looks like this change is preventing my originally working CDK on v2.88 to no longer synth on v2.89.

I am currently using an SSE enabled queue subscribed to a topic and with the necessary IAM privileges mentioned here https://docs.aws.amazon.com/sns/latest/dg/sns-key-management.html#sns-what-permissions-for-sse the CDK was synthing and deploying in CloudFormation without issue. Now that I have upgraded CDK, the code is refusing to synth with this new exception!

Here is what the code I am using looks like:

import { RemovalPolicy } from "aws-cdk-lib";
import { Queue, QueueEncryption } from "aws-cdk-lib/aws-sqs";
import { Construct } from "constructs";
import { Topic } from "aws-cdk-lib/aws-sns";
import { SqsSubscription } from "aws-cdk-lib/aws-sns-subscriptions";
import { Key } from "aws-cdk-lib/aws-kms";
import { Effect, PolicyStatement, ServicePrincipal } from "aws-cdk-lib/aws-iam";
import { StageProps } from "@ros-aws-coop/shared-constructs";

interface SubscriptionQueueProps {
  stageName: string;
}

export class SubscriptionQueue extends Construct {
  public readonly dlq;
  public readonly subDlq;
  public readonly queue;

  constructor(
    scope: Construct,
    id: string,
    props: StageProps<SubscriptionQueueProps>
  ) {
    super(scope, id);

    const key = new Key(this, "Key");
    const keyAlias = key.addAlias(`my-sqs-${props.stageName}`);
    keyAlias.applyRemovalPolicy(RemovalPolicy.DESTROY);

    this.dlq = new Queue(scope, "SqsDlq", {
      encryption: QueueEncryption.KMS_MANAGED,
      encryptionMasterKey: keyAlias,
      enforceSSL: true
    });

    this.subDlq = new Queue(scope, "SubDlq", {
      encryption: QueueEncryption.KMS_MANAGED,
      encryptionMasterKey: keyAlias,
      enforceSSL: true
    });

    this.queue = new Queue(scope, "Queue", {
      encryption: QueueEncryption.KMS_MANAGED,
      encryptionMasterKey: keyAlias,
      enforceSSL: true,
      deadLetterQueue: {
        queue: this.dlq,
        maxReceiveCount: 1
      }
    });

    const someTopic = Topic.fromTopicArn(
      this,
      "TopicInAnotherAccount",
      "arn:aws:sns:us-east-1:12345678910:topicinanotheraccount"
    );
    someTopic.addSubscription(
      new SqsSubscription(this.queue, {
        rawMessageDelivery: true,
        deadLetterQueue: this.subDlq
      })
    );

    // Allow SNS topics to write into the queue
    keyAlias.addToResourcePolicy(
      new PolicyStatement({
        sid: "sns-allow",
        effect: Effect.ALLOW,
        resources: [someTopic.topicArn],
        principals: [new ServicePrincipal("sns")],
        actions: ["kms:Decrypt", "kms:GenerateDataKey"]
      })
    );
  }
}

Expected Behavior

Calling addSubscription method on a topic with a subscription to an SSE SQS queue using a customer key to work without exception.

Current Behavior

SQS queue encrypted by AWS managed KMS key cannot be used as SNS subscription exception message thrown when calling addSubscription method on a topic with a subscription to an SSE SQS queue using a customer key.

Reproduction Steps

Code in main description. Specifically:

someTopic.addSubscription(
      new SqsSubscription(this.queue, {
        rawMessageDelivery: true,
        deadLetterQueue: this.subDlq
      })
    );

Possible Solution

Back out change made on issue #19796

Additional Information/Context

No response

CDK CLI Version

2.89

Framework Version

No response

Node.js Version

18

OS

Linux

Language

Typescript

Language Version

No response

Other information

CDK versions before 2.89 do not have this issue.

[peterwoodworth]

Thanks for reporting this @MurraySpeight,

It appears to me that the AWS managed key for SQS has the proper permissions. Here's the policy for the one in my console (since I can't find the docs for this):

{
    "Version": "2012-10-17",
    "Id": "auto-sqs-1",
    "Statement": [
        {
            "Sid": "Allow access through Simple Queue Service (SQS) for all principals in the account that are authorized to use SQS",
            "Effect": "Allow",
            "Principal": {
                "AWS": "*"
            },
            "Action": [
                "kms:Encrypt",
                "kms:Decrypt",
                "kms:ReEncrypt*",
                "kms:GenerateDataKey*",
                "kms:CreateGrant",
                "kms:DescribeKey"
            ],
            "Resource": "*",
            "Condition": {
                "StringEquals": {
                    "kms:CallerAccount": "123456789012",
                    "kms:ViaService": "sqs.us-east-1.amazonaws.com"
                }
            }
        },
        {
            "Sid": "Allow direct access to key metadata to the account",
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::123456789012:root"
            },
            "Action": [
                "kms:Describe*",
                "kms:Get*",
                "kms:List*",
                "kms:RevokeGrant"
            ],
            "Resource": "*"
        }
    ]
}

[peterwoodworth]

Have you verified that this setup actually works, or just that it deploys? I can get it to deploy, but I'm not sure if the redrive functionality will work (I'm not sure exactly how to force that to occur to test). I see this warning in the console on my subscription

[github-actions]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[MurraySpeight]

Hi @peterwoodworth, yes this deploys and works as expected. My SQS queue is subscribed to the topic and is receiving events. The DLQ redrive functionality also works and I do not get the message in the console you see - but DLQs aren't relevant to this problem because the exception occurs with just an SQS trying to subscribe to a topic without DLQs configured.

[lixx]

We also see the same issue after upgrading to 2.89.0

import sns = require('aws-cdk-lib/aws-sns');
import sqs = require('aws-cdk-lib/aws-sqs');
// ...
const failureQueue = new sqs.Queue(this, failureQueueName, {
    queueName: failureQueueName,
    encryption: sqs.QueueEncryption.KMS_MANAGED,
    retentionPeriod: Duration.days(14),
    visibilityTimeout: Duration.seconds(60),
});

failureTopic.addSubscription(
    new SqsSubscription(failureQueue, {
        rawMessageDelivery: true,
    })
);

FYI: Before the upgrade, the SQS queue was deployed successfully with SSE enabled using SSE-KMS (with master key alias/aws/sqs).

[mrgrain]

Hi @peterwoodworth, yes this deploys and works as expected. My SQS queue is subscribed to the topic and is receiving events. The DLQ redrive functionality also works and I do not get the message in the console you see - but DLQs aren't relevant to this problem because the exception occurs with just an SQS trying to subscribe to a topic without DLQs configured.

Hey @MurraySpeight Thanks for providing the example! When I deploy your code, it does work. However the queue is actually encrypted with a customer managed key.

This is because we silently do this check at the moment:

      if (encryption !== QueueEncryption.KMS && props.encryptionMasterKey) {
        encryption = QueueEncryption.KMS; // KMS is implied by specifying an encryption key
      }

Can you please confirm for me:

• firstly, if you are queues are also encrypted with a Customer Managed Key, or actually with a KMS managed key.
• secondly, if your app works again if you change the encryption property in your code to:

encryption: QueueEncryption.KMS,

Basically I think being able to specify encryption: QueueEncryption.KMS_MANAGED and a encryptionMasterKey at the same time should error, or at least warn.

We also see the same issue after upgrading to 2.89.0

Hi @lixx Thanks for providing these details. I cannot get your example to work.
Could you double check for me the actual encryption configuration on the queue?

[mrgrain]

It appears to me that the AWS managed key for SQS has the proper permissions. Here's the policy for the one in my console (since I can't find the docs for this):

@peterwoodworth I think the policy is maybe missing permissions for SNS. Which explains why @MurraySpeight's code works - it does add permissions to the key for new ServicePrincipal("sns"). The default policy does this weird "kms:ViaService": "sqs.us-east-1.amazonaws.com" thing.

Edit: It tested various policies, and it's the "kms:CallerAccount": "123456789012" condition that breaks it.

[mrgrain]

@lixx I did some more resource (see post above) and I don't believe this does work. The code snippet you've provided does synth and deploy, but the subscription does not actually work. If you can provide me with more details, I'm happy to investigate further. Until then, I am assuming you've run into a similar situation as @MurraySpeight and will consider the issue fixed with the change in #26884

[MurraySpeight]

@mrgrain the queues are encrypted with a customer managed key as I believe this is the only way to encrypt when subscribing to a cross-account SNS topic. I have made your suggested change of changing the QueueEncryption from KMS_MANAGED to KMS and upgraded to the latest CDK. This has cleared the exception when synthing and resulted in the same CloudFormation template being generated. I have gone a step further and removed the QueueEncryption parameter altogether because as you have stated, the value of KMS is assumed by the presence of the encryptionMasterKey parameter and CDK sets this automatically to KMS.

So my understanding of this issue from your above description is I was incorrectly configuring the queue but pre-v2.89 CDK was silently fixing my mistake by overriding to the KMS value since I had the encryptionMasterKey set. This silent fixing made me think I had set the correct value because it worked but it only worked because CDK corrected it for me.

I think a warning here would be massively helpful e.g. IF encryptionMasterKey has value AND encryption is not QueueEncryption.KMS THEN throw warning. The exception message is not taking into account that CDK is changing the QueueEncryption for the ignorant developer and is therefore misleading.

Thanks for your help resolving this!

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.