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

[sns] add sns service trust to key when subscribing to an encrypted queue #2504

Closed
skinny85 opened this issue May 8, 2019 · 5 comments · Fixed by #14960
Closed

[sns] add sns service trust to key when subscribing to an encrypted queue #2504

skinny85 opened this issue May 8, 2019 · 5 comments · Fixed by #14960
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@skinny85
Copy link
Contributor

skinny85 commented May 8, 2019

When using Bucket Notifications with SQS (so, something like bucket.onObjectCreated(queue)), in the case the Queue is encrypted with a KMS Key, we correctly trust the S3 Service Principal in the Key's Resource Policy.

We need to do something similar in the case of an SNS subscription to an encrypted SQS Queue (so, topic.subscribeQueue(queue)) - the Key should trust the SNS Service Principal in that case.

@skinny85 skinny85 added @aws-cdk/aws-sns Related to Amazon Simple Notification Service gap labels May 8, 2019
@garnaat garnaat self-assigned this Aug 12, 2019
@nija-at nija-at assigned nija-at and unassigned garnaat Sep 3, 2019
@SomayaB SomayaB added feature-request A feature should be added or improved. and removed gap labels Nov 11, 2019
@nija-at nija-at changed the title sns.Topic.subscribeQueue() for a Queue with a KMS Key should change the Key's Resource Policy [sns] add sns service trust to kns when subscribing to an encrypted queue Nov 28, 2019
@nija-at nija-at changed the title [sns] add sns service trust to kns when subscribing to an encrypted queue [sns] add sns service trust to key when subscribing to an encrypted queue Nov 28, 2019
@nija-at nija-at assigned MrArnoldPalmer and unassigned nija-at Jan 23, 2020
@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort p1 labels Mar 14, 2020
@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 Mar 14, 2021
@MrArnoldPalmer
Copy link
Contributor

Keeping open 👍🏻

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 16, 2021
@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@mergify mergify bot closed this as completed in #14960 Jun 24, 2021
mergify bot pushed a commit that referenced this issue Jun 24, 2021
…tions (#14960)

Add SNS service trust to KMS key when creating an SNS subscription for an encrypted SQS queue.

Closes #2504
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…tions (aws#14960)

Add SNS service trust to KMS key when creating an SNS subscription for an encrypted SQS queue.

Closes aws#2504
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@HansFalkenberg-Visma
Copy link

HansFalkenberg-Visma commented May 13, 2022

I have spent some time looking into this and will summarize my findings here for ease of discovery (this was my first google hit).

There are two duplicates of this issue:
#11122
#12120

Related issue to warn that the KMS key cannot get the necessary policy if it is an AWS managed key:
#19796

This issue (#2504) was
implemented by #14960
merged in ccc2e30 and
released in https://github.com/aws/aws-cdk/releases/tag/v1.111.0
on July 2nd 2021.
All v2 releases (except pre-releases v2.0.0-rc.11 and older) have the fix.

There is also a duplicate attempted implementation:
#12146

That PR has some interesting comments about restricting CMK access by SourceArn, which is the reason I started looking into this. I had defined my CMK policy correctly with a condition on the subscribe SNS Topic ARN being the source, and was surprised when the L3 magic added a more permissive statement allowing any SNS service in the world access to my CMK.

Apparently this is because CMK access couldn't be restricted when #14960 was implemented. Now it can, and it should be a trivial change for which I created this issue:
#20339

@HansFalkenberg-Visma
Copy link

HansFalkenberg-Visma commented May 13, 2022

Also useful to know:
If your SNS Topic messages aren't reaching their destination (SQS in this case), you can enable delivery status logging and get a clue as to the cause in a CloudWatch log group. See https://docs.aws.amazon.com/sns/latest/dg/sns-topic-attributes.html#topics-attrib

CloudFormation (and thus CDK) does not support enabling delivery status logging, but it's coming: aws-cloudformation/cloudformation-coverage-roadmap#66

The error when CMK policy isn't right is

{
  "ErrorCode": "KMS.AccessDeniedException",
  "ErrorMessage": "null (Service: AWSKMS; Status Code: 400; Error Code: AccessDeniedException; Request ID: 187caab5-970a-49d9-836f-a12cb1e94796; Proxy: null)",
  "sqsRequestId": "Unrecoverable"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants