Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

fix: Add SNS encryption for FhirWorksAlarm #462

Merged
merged 4 commits into from
Sep 30, 2021
Merged

Conversation

nguyen102
Copy link
Contributor

@nguyen102 nguyen102 commented Sep 28, 2021

Issue #, if available:

Description of changes:
Add SNS encryption for FhirWorksAlarm. As best practices Amazon recommends encrypting SNS topics.

References
https://aws.amazon.com/blogs/compute/encrypting-messages-published-to-amazon-sns-with-aws-kms/
https://stackoverflow.com/a/58849754
https://aws.amazon.com/premiumsupport/knowledge-center/cloudwatch-receive-sns-for-alarm-trigger/

Checklist:

  • Have you successfully deployed to an AWS account with your changes?
  • Have you written new tests for your core changes, as applicable?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nguyen102 nguyen102 marked this pull request as ready for review September 28, 2021 21:04
@nguyen102
Copy link
Contributor Author

@Zambonilli , As best practice SNS topics should be encrypted at rest with a KMS key. I wanted to confirm that you're ok with this change, and this wouldn't break anything on your side.

AWS: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:root'
Action: 'kms:*'
Resource: '*'
- Sid: 'Allow Cloudwatch to use this Key Policy'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had to have sns.amazonaws.com service principal w/kms:GenerateDataKey* and kms:Decrypt permissions in this policy as well. iirc, that was what allowed sns to encrypt and decrypt when sending to the subscribers.

@Zambonilli
Copy link
Contributor

ugh, thanks. I translated our terraform definition for the sns topic here incorrectly and forgot the kms key. This matches what we have in terraform minus the comment about adding sns.amazonaws.com service principal to the key's policy.

@nguyen102
Copy link
Contributor Author

@Zambonilli thanks for double checking. Just to confirm, does this mean the PR as is matches what is currently in your terraform definition?

@Zambonilli
Copy link
Contributor

This PR is close to matching our terraform. It's just missing the following Sid in the KMS key's inline policy:

- Sid: 'Allow Cloudwatch to use this Key Policy'
            Effect: Allow
            Principal:
              Service: sns.amazonaws.com
            Action:
              - kms:Encrypt
              - kms:Decrypt
              - kms:GenerateDataKey*
            Resource: '*'

@nguyen102
Copy link
Contributor Author

Good catch, after further examining the JSON at this blog post, I see they include an SID for SNS as well. I've updated the PR.

@nguyen102 nguyen102 changed the title chore: Add SNS encryption for FhirWorksAlarm fix: Add SNS encryption for FhirWorksAlarm Sep 29, 2021
@nguyen102 nguyen102 merged commit 9809087 into develop Sep 30, 2021
@nguyen102 nguyen102 deleted the encrypt-sns-topic branch September 30, 2021 14:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants