Enable KMS encryption for S3 buckets #6
Conversation
- Switch from SSE-S3 (AES256) to SSE-KMS (aws:kms) encryption - Add kms_key_arn variable to S3 module - Configure KMS keys for dev and prod environments - Provides additional encryption key control and audit logging
WalkthroughThese changes introduce KMS-based encryption for S3 buckets across dev and prod environments. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/s3/main.tf (2)
70-79: Critical: Verify policy exception approval for user_content KMS encryption.This encryption change is subject to the same mandatory S3 Encryption Standards policy violation as the static_content bucket. SSE-KMS requires CISO-approved exception before implementation.
Additionally, consider enabling S3 Bucket Keys to reduce KMS API costs:
resource "aws_s3_bucket_server_side_encryption_configuration" "user_content" { bucket = aws_s3_bucket.user_content.id rule { apply_server_side_encryption_by_default { sse_algorithm = "aws:kms" kms_master_key_id = var.kms_key_arn } + bucket_key_enabled = true } }Based on external tools context.
19-28: Critical: CloudFront OAI does not support SSE-KMS encryption—migrate to Origin Access Control (OAC).The current configuration combines
aws_cloudfront_origin_access_identitywith SSE-KMS encryption, which is incompatible. CloudFront's legacy OAI does not natively support KMS-encrypted S3 objects and requires complex workarounds.Required changes:
- Migrate from OAI to Origin Access Control (OAC)—the modern AWS solution that natively supports SSE-KMS.
- Update the KMS key policy to allow the CloudFront service principal (
cloudfront.amazonaws.com) withkms:Decryptandkms:GenerateDataKey*permissions, restricted byaws:SourceArnto your CloudFront distribution ARN.- Update the S3 bucket policy to work with the OAC principal instead of the OAI IAM ARN.
Additional optimization:
Addbucket_key_enabled = trueto reduce KMS API request costs (typically from $0.03 per 10,000 requests down significantly with S3 Bucket Keys).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
dev/s3/terragrunt.hcl(1 hunks)modules/s3/main.tf(2 hunks)modules/s3/variables.tf(1 hunks)prod/s3/terragrunt.hcl(1 hunks)
🧰 Additional context used
🔍 Remote MCP Exa, Linear, Notion
Excellent! I found critical information. Let me compile the final summary with all the relevant context gathered.
Summary of Additional Context for PR Review
Based on comprehensive searches across Exa, Linear, and Notion, I found significant conflicting information that must be addressed before merging this PR:
🚨 CRITICAL FINDING: Organizational Security Policy Conflict discovered a mandatory S3 Encryption Standards document (Last Updated: October 2024, Status: Mandatory) that explicitly prohibits the changes being made in this PR:
Current Policy (Mandatory):
- Approved: SSE-S3 (AES256) is the ONLY approved server-side encryption method
- Explicitly Prohibited: SSE-KMS (aws:kms) for S3 bucket encryption
Stated Reasons for KMS Prohibition:
- Cross-account complexity requiring KMS key policies for each accessing account
- CloudFront issues requiring Lambda@Edge for signed URLs with KMS-encrypted objects
- Cost implications ($0.03 per 10,000 KMS API requests)
- Performance impact from additional decryption latency
- Key management overhead (rotation policies, access management)
Exception Process Required: The policy explicitly states that SSE-KMS requires:
- Documentation of regulatory requirement with compliance reference
- Exception request submission to [email protected]
- Security architecture review
- CISO approval
AWS Best Practices Documentation confirms that KMS encryption for S3 is technically valid and supported by AWS, with best practices including:
- Automatic key rotation (recommended annually)
- CloudTrail logging for audit trails
- Key policy enforcement for granular access control
- Appropriate for scenarios requiring enhanced control and audit capabilities
Related Technical Issues in Linear shows related encryption work:
- ENG-2449: "Encrypt agent outputs and intermediary steps instead of plain JSON in file storage" (In Progress) - indicates organizational focus on encryption improvements
- ENG-2159: "Migrate encryption password and salt" (Done) - completed encryption migration work
- RR-610: "Unexpected AWS CLI Commands During CodeRabbit Analysis" (Medium priority) - shows security concerns with infrastructure changes
Key Implementation Details confirms the Terraform configuration is technically correct:
- The KMS key ARN reference via
var.kms_key_arnis properly structured - Terraform registry examples show identical pattern for
aws:kmsalgorithm withkms_master_key_id
Recommendation for Reviewer
Do NOT approve this PR without explicit written exception approval from the security team. The PR requires:
- ✅ Confirmation that a formal SSE-KMS exception has been approved (referencing CISO approval)
- ✅ Evidence that the exception is documented with compliance justification
- ✅ Verification that security architecture review was completed
- ✅ Confirmation that both dev and prod environments meet the approved exception criteria
The technical implementation appears sound, but organizational policy compliance is mandatory before merge.
🔇 Additional comments (1)
modules/s3/variables.tf (1)
29-32: Remove this review comment; it is based on unverified external policy claims.The mandatory "S3 Encryption Standards" policy (October 2024) referenced in this comment cannot be found or substantiated within the repository. Additionally, the claims do not align with the current codebase state:
- KMS encryption is already enabled in production (
prod/s3/terragrunt.hclline 19) and development (dev/s3/terragrunt.hclline 19) environments with valid ARN values, indicating this is not a breaking change.- No CISO approval documentation, security exceptions, or regulatory compliance references exist in the repository.
- No policy documentation mandating SSE-S3 over SSE-KMS is present in the codebase.
- The
kms_key_arnvariable is already in active use across both environments.If a legitimate security policy exists requiring exception approval, that documentation should be referenced with specific links or tickets. Otherwise, this review should be dismissed.
Minor note: Adding ARN format validation to the variable would be a reasonable enhancement, but this is not critical given the module is already production-deployed.
Likely an incorrect or invalid review comment.
| # KMS encryption | ||
| kms_key_arn = "arn:aws:kms:us-east-1:123456789012:key/87654321-4321-4321-4321-210987654321" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS account ID 123456789012 placeholder documentation
💡 Result:
AWS documentation commonly uses the 12-digit number 123456789012 as an example/placeholder AWS account ID (not a real account). Examples appear throughout official docs (ARN and API examples, CloudFormation return values, etc.). [1][2][3]
Sources:
- IAM identifiers (ARN examples). [1]
- AWS CloudFormation (AccountId example). [2]
- AWS Account Management API examples (GetAccountInformation). [3]
🌐 Web query:
hardcoding KMS ARN in code security best practices
💡 Result:
Short answer
Don’t hardcode KMS key ARNs in application source. Use identity/metadata (IAM roles), configuration/secret stores, and aliases so keys aren’t embedded in code and you can rotate/repoint keys without code changes. (docs.aws.amazon.com)
What to do instead (concise)
- Use IAM roles/instance profiles or STS-assumed roles so the runtime gets credentials and permissions from AWS, not from code. (aws.amazon.com)
- Put configuration (key identifiers or alias names) in a secure store (AWS Secrets Manager, Systems Manager Parameter Store, or environment config injected at deploy), not in source control. (docs.aws.amazon.com)
- Refer to KMS aliases or key IDs where appropriate instead of embedding a specific ARN in many places; keep the authoritative mapping in config or IaC (CloudFormation/Terraform) so you can change keys centrally. (docs.aws.amazon.com)
- Apply least-privilege IAM policies that specify which KMS key ARNs a principal may use; manage that in IAM/key policies, not in app code. (docs.aws.amazon.com)
Operational controls
- Rotate keys and secrets; automate rotation via Secrets Manager or KMS/automation. (v1.maturitymodel.security.aws.dev)
- Audit access to keys with CloudTrail and alert on abnormal usage. (docs.aws.amazon.com)
If you want, I can:
- show a short example pattern for storing a KMS alias in Parameter Store + using an IAM role in Lambda or EC2, or
- review a snippet of your code / IaC and suggest concretely how to remove a hardcoded ARN.
Citations:
- 1: https://docs.aws.amazon.com/kms/latest/developerguide/iam-policies-best-practices.html?utm_source=openai
- 2: https://aws.amazon.com/blogs/security/guidelines-for-protecting-your-aws-account-while-using-programmatic-access/?utm_source=openai
- 3: https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars-encryption.html?utm_source=openai
- 4: https://docs.aws.amazon.com/kms/latest/developerguide/iam-policies-best-practices.html?utm_source=openai
- 5: https://docs.aws.amazon.com/kms/latest/developerguide/iam-policies-best-practices.html?utm_source=openai
- 6: https://v1.maturitymodel.security.aws.dev/en/2.-foundational/dont-store-secrets-in-code/?utm_source=openai
- 7: https://docs.aws.amazon.com/prescriptive-guidance/latest/aws-kms-best-practices/access.html?utm_source=openai
🌐 Web query:
S3 Encryption Standards policy CISO approval requirements
💡 Result:
Short answer — who/what the CISO should approve for an S3 Encryption Standards policy:
- Formal approval of the encryption standard/policy itself (scope, data classifications that require encryption, approved algorithms and key types, KMS usage and key ownership). [1][3]
- Authority to accept or reject exceptions and any compensating controls (CISO sign‑off on documented exceptions; annual re‑review). [1][2]
- Approval of key management model (AWS KMS vs. customer‑managed keys vs. HSM/externally‑managed keys), key rotation and destruction rules, and access/usage controls for keys. [1][3]
- Approval of enforcement and monitoring controls (bucket-level default encryption, IAM/Security Hub/Config rules, alerts and remediation workflows). [3][4]
- Approval of any residual risk decisions (business/line requests to avoid encryption for specific flows) and schedule for periodic policy review (at least annually). [1][2]
Sources:
- AWS Prescriptive Guidance — Encryption policy guidance (roles & approval expectations). [1]
- AWS Security Blog — CISO approval/compensating‑control guidance (regulatory requirements such as NYDFS). [2]
- Amazon S3 security & best practices (bucket encryption, KMS, enforcement and deprecation notes). [3]
- AWS Config / Security Hub operational controls (enforcement/monitoring recommendations). [4]
🌐 Web query:
KMS key policy requirements S3 service principal encryption
💡 Result:
Summary — what a KMS key policy must grant for S3 to perform SSE‑KMS encryption/decryption
- Principals and actions
- Grant the appropriate S3 service principal permission to use the CMK. For general S3 object SSE‑KMS use you typically allow the S3 service principal (s3.amazonaws.com) or specific S3 service principals used by features (for S3 Tables use maintenance.s3tables.amazonaws.com or metadata.s3.amazonaws.com). Allow at least: kms:GenerateDataKey (and kms:GenerateDataKeyWithoutPlaintext if needed) and kms:Decrypt. [1][3]
- Conditions to scope access (recommended)
- Use kms:ViaService to restrict calls to those that come via the S3 service in the region (e.g. "kms:ViaService":"s3.us‑east‑1.amazonaws.com").
- Use AWS:SourceAccount and/or AWS:SourceArn to restrict the service principal to requests for a specific account/bucket or delivery channel. These condition keys are commonly used in AWS examples to limit scope. [2][4]
- Placement and format
- The permission must be in the CMK key policy (not only an IAM policy). S3 checks the CMK key policy when performing SSE‑KMS operations, so the key policy must allow the S3 principal to use the key for the target bucket/objects. [3][1]
Minimal example key‑policy statement (replace placeholders)
{
"Sid": "AllowS3ToUseKeyForSSEKMS",
"Effect": "Allow",
"Principal": { "Service": "s3.amazonaws.com" },
"Action": [
"kms:GenerateDataKey",
"kms:Decrypt"
],
"Resource": "",
"Condition": {
"StringEquals": {
"kms:ViaService": "s3.us-east-1.amazonaws.com",
"AWS:SourceAccount": "111122223333"
},
"ArnLike": {
"AWS:SourceArn": "arn:aws:s3:::example-bucket/"
}
}
}
Notes / references
- S3 Tables (table buckets) require the maintenance/metadata S3 service principals to have kms:GenerateDataKey and kms:Decrypt on the CMK. If the maintenance principal lacks access, table creation will fail. [1]
- Use kms:ViaService and other KMS condition keys to restrict use to S3 in a particular region/account. [2]
- S3 SSE‑KMS docs describe the need for the key policy to match the KMS key ID/ARN used by S3 and how S3 enforces it. [3]
- Example of using a service principal + SourceAccount condition is shown in other AWS service docs (e.g., AWS Config). [4]
Sources: [1] AWS S3 — Permission requirements for S3 Tables SSE‑KMS encryption; [2] AWS KMS — condition keys (kms:ViaService); [3] AWS S3 — Using server‑side encryption with AWS KMS keys (SSE‑KMS); [4] AWS Config KMS key policy example.
Replace placeholder account ID and remove hardcoded KMS ARN from version control.
This configuration has several issues:
-
Placeholder Account ID: The account ID
123456789012is a standard AWS documentation placeholder—replace it with the actual AWS account ID. -
Hardcoded ARN in Version Control: Per AWS KMS best practices, avoid hardcoding KMS key ARNs in code or infrastructure files. Store the key identifier (ARN, key ID, or alias) in a secure configuration store such as AWS Secrets Manager, Systems Manager Parameter Store, or infrastructure-as-code variables that aren't checked into version control. This allows key rotation and repointing without code changes.
-
KMS Key Policy: Ensure the key policy grants the S3 service principal (
s3.amazonaws.com) the following actions:kms:GenerateDataKeykms:Decrypt
Restrict these permissions using conditions like
kms:ViaService(e.g.,s3.us-east-1.amazonaws.com) andAWS:SourceAccountto scope access appropriately. Verify policy is set on the KMS key itself, not only in IAM policies.
🤖 Prompt for AI Agents
In dev/s3/terragrunt.hcl around lines 18-19, the kms_key_arn is using a
placeholder account ID and a hardcoded KMS ARN which must not be checked into
version control; replace the hardcoded ARN with a reference to a secure,
non-checked-in source (e.g., Terragrunt/Terraform variable populated from AWS
Secrets Manager, SSM Parameter Store, or an environment-specific non-committed
vars file) and update the module/stack to read that variable instead of a
literal; additionally, ensure the KMS key’s policy (configured on the key
resource, not only via IAM) grants s3.amazonaws.com kms:GenerateDataKey and
kms:Decrypt scoped by kms:ViaService (s3.<region>.amazonaws.com) and
AWS:SourceAccount to the correct account ID so S3 can use the key.
| # KMS encryption | ||
| kms_key_arn = "arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the terragrunt.hcl file and verify it exists
find . -name "terragrunt.hcl" -o -name "*terragrunt*" | head -20Repository: coderabbitai/rabbits-playground
Length of output: 468
🏁 Script executed:
# Look for policy documentation or S3 encryption standards in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.json" \) | xargs rg -l "S3.*policy|encryption.*standard|CISO" 2>/dev/null | head -20Repository: coderabbitai/rabbits-playground
Length of output: 57
🏁 Script executed:
# Check for AWS account ID patterns and environment configs
rg "123456789012|account.*id|aws.*account" -t hcl -t json -A 2 | head -40Repository: coderabbitai/rabbits-playground
Length of output: 102
🏁 Script executed:
# Read the actual prod/s3/terragrunt.hcl file
cat -n ./prod/s3/terragrunt.hclRepository: coderabbitai/rabbits-playground
Length of output: 806
🏁 Script executed:
# Search for policy, compliance, or CISO-related documentation
rg "policy|compliance|CISO|encryption.*standard|SSE-KMS" -i --type-list | head -5
rg "policy|compliance|CISO|encryption.*standard|SSE-KMS" -i . 2>/dev/null | head -30Repository: coderabbitai/rabbits-playground
Length of output: 2645
🏁 Script executed:
# Check dev environment S3 config to compare account IDs
cat -n ./dev/s3/terragrunt.hclRepository: coderabbitai/rabbits-playground
Length of output: 798
🏁 Script executed:
# Search for any documentation or policy files more thoroughly
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" -o -name "README*" \) | xargs cat 2>/dev/null | grep -i "policy\|compliance\|ciso\|encryption.*standard" -A 2 -B 2 2>/dev/null | head -50Repository: coderabbitai/rabbits-playground
Length of output: 57
🏁 Script executed:
# Check the S3 module implementation to see how KMS is configured
cat -n ./modules/s3/main.tf | head -80Repository: coderabbitai/rabbits-playground
Length of output: 2599
🏁 Script executed:
# Look for variables and validation rules in S3 module
cat -n ./modules/s3/variables.tf 2>/dev/null || echo "variables.tf not found"Repository: coderabbitai/rabbits-playground
Length of output: 989
🏁 Script executed:
# Check root terragrunt config for environment-specific policies
cat -n ./terragrunt.hcl ./prod/terragrunt.hcl 2>/dev/null | head -60Repository: coderabbitai/rabbits-playground
Length of output: 2063
Replace placeholder KMS ARN with actual production key before deployment.
The KMS ARN contains placeholder values (account ID 123456789012 and key ID 12345678-1234-1234-1234-123456789012) that must be replaced with the actual production KMS key ARN. The root terragrunt.hcl shows account IDs are managed via the AWS_ACCOUNT_ID environment variable, so ensure this is properly set during deployment.
Note: The suggestion to use AWS Systems Manager Parameter Store for the KMS ARN is optional—passing the ARN directly as a Terraform variable is standard practice.
🤖 Prompt for AI Agents
In prod/s3/terragrunt.hcl around lines 18-19, the kms_key_arn is set to a
placeholder ARN with a fake account and key ID; replace this placeholder with
the actual production KMS key ARN (or pass it in as a Terraform variable) and
ensure the AWS_ACCOUNT_ID environment variable used by root terragrunt.hcl is
set to the production account during deployment; optionally, if you prefer
secret management, read the ARN from SSM Parameter Store and inject it into
terragrunt/terraform rather than hardcoding.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.