aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.48k stars 3.83k forks source link

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

Open SamStephens opened 3 years ago

SamStephens commented 3 years ago

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 https://github.com/aws/aws-cdk/issues/2504.


This is a :rocket: Feature Request

otaviomacedo commented 2 years ago

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.

SamStephens commented 2 years ago

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[bot] commented 1 year ago

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.

SamStephens commented 1 year ago

Still relevant, commenting to avoid auto-close