fix(sns): for SSE topics, add KMS permissions in grantPublish by lightningboltemoji · Pull Request #32794 · aws/aws-cdk

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

      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

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

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 80.83%. Comparing base (bc82f57) to head (41a5f94).
Report is 17 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32794   +/-   ##
=======================================
  Coverage   80.83%   80.83%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11522    11522           
  Misses       2446     2446           
  Partials      285      285

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

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

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

Dismissing as this would require security review.

GavinZZ · 2025-02-05T18:50:08Z

I think the changes look good to me. I'll bring it up to the internal security team for review next week.

GavinZZ

An update on the security review: please wait patiently, I've raised this PR to the security engineer and pending an response from him.

GavinZZ

Setting this PR to changes requested to meet the security engineer's requirement to add assertion tests. @lightningboltemoji let me know if the comments make sense to you.

GavinZZ · 2025-02-25T17:46:40Z

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

+    topic3.masterKey!.addToResourcePolicy(new PolicyStatement({
+      principals: [new ServicePrincipal('s3.amazonaws.com')],
+      actions: ['kms:GenerateDataKey*', 'kms:Decrypt'],
+      resources: ['*'],
+    }));


Curious what is this code block for?

What I meant by:

// Can reference KMS key after creation

is that referring to the topic's KMS key after topic creation is new functionality. This is testing that masterKey refers to the key we passed in, and that we're able to make changes to it.

I switched this to a more generic unit test

GavinZZ · 2025-02-25T17:49:30Z

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

+      topicArn: topic3.topicArn,
+      keyArn: key.keyArn,
+    });
+    importedTopic3.grantPublish(publishRole);


One of the security engineer's feedback is to test functionality instead of simply deployability. Can you use assertions to test publishing a message to the SNS topic to see if it will be successful? https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md#assertions

OK. I'll explore that tonight.

I added an integration test, but I had to use assertions.invokeFunction to accomplish it... I first tried to implement it along the lines of:

const publish = testCase.assertions.awsApiCall('sns', 'Publish', { TopicArn: ..., Message: ... });

but I couldn't find a way to combine this with grantPublish (i.e. we can't access the role associated with this custom resource).

Let me know what you think.

Awesome, this works as well!

Pull request has been modified.

GavinZZ

LGTM, approving this PR as assertions has been added which is the only requirement from Security guardian. Thank you for the patience and addressing the comments quickly!

mergify · 2025-02-26T21:36:33Z

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).

aws-cdk-automation · 2025-02-26T22:07:42Z

AWS CodeBuild CI Report

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

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

mergify · 2025-02-26T22:08:01Z

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).

github-actions · 2025-02-26T22:08:17Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

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 2 times, most recently 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

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

GavinZZ previously approved these changes Feb 5, 2025

View reviewed changes

GavinZZ self-assigned this Feb 5, 2025

lightningboltemoji force-pushed the sns-kms branch from 469a884 to 41a5f94 Compare February 6, 2025 05:27

GavinZZ reviewed Feb 19, 2025

View reviewed changes

GavinZZ previously requested changes Feb 25, 2025

View reviewed changes

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

b13671d

lightningboltemoji force-pushed the sns-kms branch from 41a5f94 to b13671d Compare February 26, 2025 07:39

GavinZZ approved these changes Feb 26, 2025

View reviewed changes

Merge branch 'main' into sns-kms

a98a3d6

mergify bot merged commit f1c0926 into aws:main Feb 26, 2025
10 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 26, 2025

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

Conversation

lightningboltemoji commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GavinZZ commented Feb 5, 2025

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

GavinZZ left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinZZ left a comment • 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!

mergify bot commented Feb 26, 2025

Uh oh!

aws-cdk-automation commented Feb 26, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 26, 2025

Uh oh!

Uh oh!

github-actions bot commented Feb 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lightningboltemoji commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading

GavinZZ left a comment •

edited

Loading

GavinZZ left a comment •

edited

Loading