fix(sns): for SSE topics, add KMS permissions in grantPublish #32794

lightningboltemoji · 2025-01-08T07:11:11Z

Issue # (if applicable)

Pre-requisite for #16271, #29511

Reason for this change

For SNS topics with SSE enabled, the grants added by grantPublish are insufficient, since they don't include any KMS actions.

The SNS docs discuss what's required to publish to an encrypted topic here (sns:Publish, kms:Decrypt, kms:GenerateKeyData*).

Description of changes

I used the SQS queue implementation as a reference, since it's configured similarly, etc.

Have Topic#grantPublish grant kms:Decrypt + kms:GenerateKeyData*
- This is least-privilege, but slightly inconsistent with SQS queues, which need these same actions and use grantEncryptDecrypt (but I have no preference -- just let me know what's best)
Exposes masterKey as a property of ITopic so callers can access it after creation
- Enables this, for example, and in general makes it consistent with SQS queues

Describe any new or updated permissions being added

(Discussed above)

Description of how you validated changes

Unit/integration tests
- yarn integ test/aws-sns/test/integ.sns.js --update-on-failed

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

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.

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

lpizzinidev

Thanks 👍 Left comments for minor adjustments

lpizzinidev · 2025-01-10T08:41:23Z

packages/aws-cdk-lib/aws-sns/lib/topic-base.ts

      grantee,
      actions: ['sns:Publish'],
      resourceArns: [this.topicArn],
      resource: this,
    });
+    if (this.masterKey) {
+      this.masterKey.grantEncryptDecrypt(grantee);


Suggested change

this.masterKey.grantEncryptDecrypt(grantee);

this.masterKey.grant(grantee, 'kms:Decrypt', 'kms:GenerateDataKey*')

We should be fine without kms:Encrypt and kms:ReEncrypt.

Sounds good, thanks!

lpizzinidev · 2025-01-10T08:58:08Z

packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns.ts

+    importedTopic2.grantPublish(publishRole);
+
+    // Can import encrypted topic by attributes
+    const topic3 = new Topic(this, 'MyTopic3', {


Couldn't we import topic2 instead?

Sorry, not sure I understand the suggestion, do you mean topic? I can try to provide some of my thinking here:

The existing test is showing a non-functional example. importedTopic isn't aware there's a KMS key so grantPublish doesn't work.

I unencrypted topic2, so that importing with fromTopicArn and calling grantPublish does work.

That left me wanting to import an encrypted topic, so I created topic3 and used fromTopicAttributes, testing the same basic flow.

topic is encrypted and could be used for importing... it's just created up higher, and it felt like we'd moved on from it flow-wise.

I'm not totally sure what the norm is here, so your guidance is appreciated!

Thanks for clarifying 👍 Your implementation makes sense as is

lpizzinidev · 2025-01-10T09:00:41Z

packages/aws-cdk-lib/aws-sns/lib/topic-base.ts

@@ -25,6 +26,13 @@ export interface ITopic extends IResource, notifications.INotificationRuleTarget
   */
  readonly topicName: string;

+  /**
+   * A KMS Key, either managed by this CDK app, or imported.
+   *


Suggested change

*

*

* This property applies only to server-side encryption.

*

* @see https://docs.aws.amazon.com/sns/latest/dg/sns-server-side-encryption.html

Added, thanks!

aws-cdk-automation · 2025-01-11T02:58:24Z

AWS CodeBuild CI Report

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

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

lpizzinidev

👍

lpizzinidev · 2025-01-11T16:03:53Z

packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns.ts

+    importedTopic2.grantPublish(publishRole);
+
+    // Can import encrypted topic by attributes
+    const topic3 = new Topic(this, 'MyTopic3', {


Thanks for clarifying 👍 Your implementation makes sense as is

codecov · 2025-01-12T12:21:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.38%. Comparing base (49eb9d3) to head (469a884).
Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32794   +/-   ##
=======================================
  Coverage   81.38%   81.38%           
=======================================
  Files         222      222           
  Lines       13695    13695           
  Branches     2412     2412           
=======================================
  Hits        11145    11145           
  Misses       2271     2271           
  Partials      279      279

Flag	Coverage Δ
suite.unit	`81.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.69% <ø> (ø)`
packages/aws-cdk-lib/core	`82.09% <ø> (ø)`

aws-cdk-automation requested a review from a team January 8, 2025 07:11

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jan 8, 2025

aws-cdk-automation previously requested changes Jan 8, 2025

View reviewed changes

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort and removed feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort labels Jan 8, 2025

lightningboltemoji force-pushed the sns-kms branch 4 times, most recently from a45469a to 24f6412 Compare January 9, 2025 06:52

lightningboltemoji marked this pull request as ready for review January 9, 2025 06:57

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 9, 2025

lpizzinidev suggested changes Jan 10, 2025

View reviewed changes

lightningboltemoji force-pushed the sns-kms branch from 24f6412 to e0ee009 Compare January 11, 2025 02:23

fix(sns): for SSE topics, add KMS permissions in grantPublish

469a884

lightningboltemoji force-pushed the sns-kms branch from e0ee009 to 469a884 Compare January 11, 2025 02:26

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 11, 2025

lpizzinidev approved these changes Jan 11, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 12, 2025

moelasmar added the needs-security-review Related to feature or issues that needs security review label Jan 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sns): for SSE topics, add KMS permissions in grantPublish #32794

fix(sns): for SSE topics, add KMS permissions in grantPublish #32794

lightningboltemoji commented Jan 8, 2025 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev left a comment

lpizzinidev Jan 10, 2025

lightningboltemoji Jan 11, 2025

lpizzinidev Jan 10, 2025

lightningboltemoji Jan 11, 2025

lpizzinidev Jan 11, 2025

lpizzinidev Jan 10, 2025

lightningboltemoji Jan 11, 2025

aws-cdk-automation commented Jan 11, 2025

lpizzinidev left a comment

lpizzinidev Jan 11, 2025

codecov bot commented Jan 12, 2025

	this.masterKey.grantEncryptDecrypt(grantee);
	this.masterKey.grant(grantee, 'kms:Decrypt', 'kms:GenerateDataKey*')

-   *
+   *
+   * This property applies only to server-side encryption.
+   *
+   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-server-side-encryption.html

fix(sns): for SSE topics, add KMS permissions in grantPublish #32794

Are you sure you want to change the base?

fix(sns): for SSE topics, add KMS permissions in grantPublish #32794

Conversation

lightningboltemoji commented Jan 8, 2025 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 11, 2025

AWS CodeBuild CI Report

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2025

Codecov Report

lightningboltemoji commented Jan 8, 2025 •

edited

Loading