Fix two misleading claims in the sns_topic_kms_key_arn docs - #10
Merged
Merged
Conversation
…llow-up) Code review on PR #9 caught two gaps in the sns_topic_kms_key_arn documentation: 1. failover.tf grants the Lambda role kms:GenerateDataKey*/kms:Decrypt via IAM policy, but KMS key policies take precedence over IAM policies (confirmed against AWS's KMS key-policy docs) -- a restrictive customer-managed key policy that doesn't name this role or retain the default IAM-delegation statement leaves sns:Publish failing regardless of the IAM grant. The docs implied the IAM grant alone was sufficient. 2. "has no effect if the topic uses the default alias/aws/sns key" was misleading: AWS requires a customer-managed key for any AWS service (CloudWatch, Auto Scaling) to publish to an encrypted SNS topic at all (confirmed against AWS's SNS KMS docs), so the Spot-fallback alarm, backstop alarms, and ASG notification cannot use the default key regardless of this variable -- only the Lambda's own publish works with either key type. Documented both constraints explicitly instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #9. Code review caught two doc gaps:
failover.tfgrants the failover Lambda's rolekms:GenerateDataKey*/kms:Decryptvia IAM policy, but KMS key policies take precedence over IAM policies -- confirmed against AWS's KMS key-policy docs: "Unless the key policy explicitly allows it, you cannot use IAM policies to allow access to a KMS key... Without permission from the key policy, IAM policies that allow permissions have no effect." A restrictive customer-managed key policy that doesn't name this role or retain the default IAM-delegation statement leavessns:Publishfailing withKMSAccessDeniedExceptionregardless of the IAM grant. The docs previously implied the IAM grant alone was sufficient."has no effect if the topic uses the default alias/aws/sns key"was misleading: AWS's SNS KMS docs require a customer-managed key for any AWS service (CloudWatch, Auto Scaling) to publish to an encrypted SNS topic at all -- so the Spot-fallback alarm, backstop alarms, and ASG notification cannot use the default key regardless of this variable. Only the Lambda's own publish works with either key type.Both constraints are now documented explicitly in the
sns_topic_kms_key_arnREADME row and thefailover.tfcomment.Test plan
terraform fmt -recursive -check -diff(clean)terraform validate(module root andexamples/basic)