search

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.47k stars 3.83k forks source link

bucketNotificationDestination: SnsDestination does not give S3 service principal KMS access when Topic is encrypted under KMS key #29511

Open vcattoir opened 5 months ago

vcattoir commented 5 months ago

Describe the bug

In https://github.com/aws/aws-cdk/blob/v2.133.0/packages/aws-cdk-lib/aws-s3-notifications/lib/sns.ts, creating SnsDestination sets up topic access policy for S3 Service principal to publish to SNS topic.

But if topic has KMS key encryption, nothing is added and S3 cannot verify the notification destination configuration is valid. Similar to what is done in SqsDestination https://github.com/aws/aws-cdk/blob/v2.133.0/packages/aws-cdk-lib/aws-s3-notifications/lib/sqs.ts#L27-L37

Expected Behavior

I was expecting the KMS key access to be given automatically when creating Sns notification destination.

Current Behavior

The SNS does not receive any notification as S3 is not able to verify destination is valid when SNS has KMS encryption.

Reproduction Steps

Creating an SNS topic using KSM encryption. Creating an S3 bucket. Adding SNS notification destination for S3 bucket events.

Possible Solution

Do similar as what is done in SqsDestination https://github.com/aws/aws-cdk/blob/v2.133.0/packages/aws-cdk-lib/aws-s3-notifications/lib/sqs.ts#L27-L37

Additional Information/Context

No response

CDK CLI Version

v2.133

Framework Version

No response

Node.js Version

18

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

msambol commented 5 months ago

I'll take this.

msambol commented 5 months ago

This one is tricky because with SQS, if you specify KMS and don't specify a key, CDK will create a new KMS key and can update the policy. However, if you specify the KMS key, CDK cannot update the key policy. See these lines.

With SNS, CDK does not create the KMS key for you. You must specify the KMS key. And we can't update the key policy of an imported key. I suppose the options are:

  1. Update SNS to create a KMS if you don't specify one.
  2. Throw a warning that the KMS key policy cannot be updated for an imported KMS key.

I will wait for import from the team to proceed.

pahud commented 5 months ago

Thank you @msambol we probably can do both but we need the input from the maintainers.

vcattoir commented 5 months ago

@msambol I have a question about this

This one is tricky because with SQS, if you specify KMS and don't specify a key, CDK will create a new KMS key and can update the policy. However, if you specify the KMS key, CDK cannot update the key policy. See these lines.

Is this a documented behavior ? I do have examples where a specific KMS key was given to an SQS and the key policy was updated when creating an SNS subscription to the SQS ( code adding to the key here). My example was with both resources being in same account and same stack. I would have imagined the error message mentioned there should not exclude any imported KMS but those owned by another account or stack which it obviously cannot update.

paulhcsun commented 3 months ago

Hey @msambol, since the queue.encryptionMasterKey is a kms.IKey the same as the sns.masterKey is, we can implement this the same way for SNS as is done in SQS by both updating SNS to create a KMS if you don't specify one and also throwing a warning that the KMS key policy cannot be updated for an imported KMS key.

Thanks!