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.

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

(aws-s3-notifications): add s3 trust to key for SNS event subscription #16271

Open
SamStephens opened this issue Aug 30, 2021 · 4 comments
Open
Labels
@aws-cdk/aws-s3-notifications feature-request A feature should be added or improved. p1

Comments

@SamStephens
Copy link
Contributor

Automatically add the required policy to the KMS key when an encrypted SNS topic is subscribed to S3 event.

Equivalent logic is present when subscribing an SQS queue to an S3 event.

Use Case

When configuring S3 notifications, I do not have to manually configure KMS trust for S3 when subscribing an encrypted SNS topic.

Proposed Solution

Add similar logic to encrypted SNS notification subscriptions as is present for encrypted SQS notification subscriptions.

Other

I had trouble finding where the logic doing this lives in the CDK currently, but here is the test showing this is done for SQS subscriptions, and here is the logic in an old version of the CDK, as referenced in #2504.


This is a 🚀 Feature Request

@SamStephens SamStephens added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 30, 2021
@github-actions github-actions bot added @aws-cdk/aws-s3 Related to Amazon S3 @aws-cdk/aws-sns Related to Amazon Simple Notification Service labels Aug 30, 2021
@otaviomacedo otaviomacedo changed the title (aws-cdk.aws-sns): add s3 trust to key for SNS event subscription (aws-s3-notifications): add s3 trust to key for SNS event subscription Sep 10, 2021
@otaviomacedo otaviomacedo added bug This issue is a bug. p1 and removed @aws-cdk/aws-s3 Related to Amazon S3 @aws-cdk/aws-sns Related to Amazon Simple Notification Service needs-triage This issue or PR still needs to be triaged. bug This issue is a bug. labels Sep 10, 2021
@otaviomacedo
Copy link
Contributor

Thanks for raising this, @SamStephens. The challenge I see here is that the ITopic interface (passed as a constructor parameter to SnsDestination) does not expose any information about encryption. And, in fact, it can't expose that information, since we can get an ITopic using Topic.fromTopicArn() to reference an existing topic.

We can't work on a design for this immediately, but we welcome community contributions! If you are able, we encourage you to contribute a bug fix or new feature to the CDK. If you decide to contribute, please start an engineering discussion in this issue to ensure there is a commonly understood design before submitting code. This will minimize the number of review cycles and get your code merged faster.

@otaviomacedo otaviomacedo removed their assignment Sep 10, 2021
@SamStephens
Copy link
Contributor Author

You should be able to mirror exactly what is done for an SQS queue, as I say; in this context an SQS queue and an SNS topic are equivalent.

IQueue exposes encryption information. They deal with the import case by simply assuming there's no encryption in fromQueueArn. If you want to import a queue that has encryption, and you need that encryption information in your stack, you need to use fromQueueAttributes. Topics should be able to parallel all of these constructs; indeed, there's probably room for refactoring to reflect this commonality.

Once the encryption information is available, the S3 notification logic for SQS encryption could then be applied for SNS notifications.

Is it possible to contribute this as a potential design to implement, even though I may not end up being the one to implement it (I'm not sure I'll have the bandwidth)?

@github-actions
Copy link

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

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 11, 2022
@SamStephens
Copy link
Contributor Author

Still relevant, commenting to avoid auto-close

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3-notifications feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests

3 participants