-
-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add KMS encryption support for CloudTrail logs #89
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
feat: add KMS encryption support for CloudTrail logs #89
Conversation
… logs This change addresses Drata compliance requirements by adding KMS encryption to the CloudTrail logs created by the datadog-logs-archive component. The implementation provides flexible configuration: encryption is enabled by default with automatic key creation, but users can also bring their own KMS key or disable encryption if needed. The KMS resources are organized in a dedicated kms.tf file for better maintainability. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
WalkthroughThis PR adds CloudTrail KMS encryption support: new Terraform variables and outputs, a conditional KMS key, alias and IAM policy (in Changes
Sequence DiagramsequenceDiagram
participant User
participant Config as Input Variables
participant Locals as Local Resolution
participant KMS as KMS Resources
participant CloudTrail as CloudTrail Module
User->>Config: Provide encryption settings (enable, ARN, create, rotation)
Config->>Locals: Provide values for local resolution
rect rgb(230, 245, 230)
note right of Locals: If enabled && create && no external ARN
Locals->>KMS: Request creation (policy, key, alias)
KMS->>Locals: Return created key ARN/ID/alias
end
alt External ARN provided
Locals->>CloudTrail: Use provided `cloudtrail_kms_key_arn`
else Created key used
Locals->>CloudTrail: Use created key ARN
end
Locals->>CloudTrail: Pass `kms_key_arn`
CloudTrail->>CloudTrail: Configure CloudTrail with KMS encryption
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (4)**/*.tf📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/README.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{yaml,yml,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
README.{yaml,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Important Do not edit the Please update the Could you fix it @johncblandii? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/variables.tf (1)
111-115: Clarify variable description to reflect full conditional logic.The description for
cloudtrail_create_kms_key(line 113) states it's "Only used if cloudtrail_kms_key_arn is not provided and cloudtrail_enable_kms_encryption is true," but it omits a key dependency: the variable is also ignored ifcloudtrail_enable_kms_encryptionis false. The description should clarify this cascading condition.Suggested revision:
- description = "Create a new KMS key for CloudTrail encryption. Only used if cloudtrail_kms_key_arn is not provided and cloudtrail_enable_kms_encryption is true" + description = "Create a new KMS key for CloudTrail encryption. Only used if cloudtrail_enable_kms_encryption is true and cloudtrail_kms_key_arn is not provided"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/README.md(1 hunks)src/kms.tf(1 hunks)src/main.tf(3 hunks)src/outputs.tf(1 hunks)src/variables.tf(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/README.md
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate src/README.md using atmos docs generate readme-simple; do not edit src/README.md manually
Files:
src/README.md
**/*.{yaml,yml,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation for YAML and Markdown files
Files:
src/README.md
src/{main,variables,outputs,providers,versions,context}.tf
📄 CodeRabbit inference engine (AGENTS.md)
Keep the Terraform component’s source of truth in src/ with canonical files: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, and context.tf
Files:
src/variables.tfsrc/main.tfsrc/outputs.tf
**/*.tf
📄 CodeRabbit inference engine (AGENTS.md)
**/*.tf: Use 2-space indentation for all Terraform files
In Terraform, use lower_snake_case for variables and locals; keep resource/data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and adhere to TFLint rules defined in .tflint.hcl; do not commit formatting or lint violations
Files:
src/variables.tfsrc/main.tfsrc/outputs.tfsrc/kms.tf
🪛 markdownlint-cli2 (0.18.1)
src/README.md
19-19: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (5)
src/outputs.tf (1)
46-59: Outputs correctly guard against null references and match resource creation conditions.The three new outputs are well-structured:
- Line 47:
cloudtrail_kms_key_arnexposes the derived or provided ARN without additional conditioning, returning null when encryption is disabled—appropriate for downstream modules.- Lines 52 & 57:
cloudtrail_kms_key_idandcloudtrail_kms_key_aliasuse a consistent 4-part AND condition that precisely matches the condition inkms.tf(lines 3, 109, 130), ensuring outputs only expose created resources.Indentation and naming are correct throughout.
src/main.tf (2)
34-40: KMS ARN resolution logic is sound.The ternary chain correctly handles all paths:
- Encryption disabled → null
- Explicit ARN provided → use it
- Create key && module enabled → resolve created key ARN via
try()- Otherwise → null
The use of
try()at line 38 is the correct pattern for safely referencing a resource that may not exist due to conditional count.
315-315: Verify Terraform dependency behavior with conditional resource count.At line 315,
aws_kms_key.cloudtrailis added todepends_on, but this resource usescountand may not be created (count = 0 when encryption disabled or using external key).Terraform should gracefully handle this—dependencies to non-existent resources are typically ignored. However, please verify this behavior in your environment by running
terraform planand confirming that:
- When encryption is disabled (
cloudtrail_enable_kms_encryption = false), no KMS key is created and CloudTrail still provisions without errors.- The depends_on to the non-existent KMS key does not cause a validation error.
Also applies to: 325-325
src/kms.tf (2)
108-126: KMS key resource is well-configured.The
aws_kms_keyresource properly:
- Uses user-provided variables for deletion window (line 112) and rotation (line 113).
- Includes a descriptive key policy (line 114).
- Applies appropriate tags and naming conventions (lines 116–125).
The conditional count ensures the key is created only when encryption is enabled, no external ARN is provided, and creation is requested.
2-41: KMS policy statements for CloudTrail encryption are comprehensive.The policy document includes all required permissions:
- Account root can manage the key and create aliases (statements 1 & 5).
- CloudTrail service can encrypt logs and describe the key (statements 2 & 3).
- Account principals can decrypt logs for access (statement 4).
EncryptionContext and SourceArn conditions properly scope access. However, review the
ViaServicecondition in statement 5 (line 102) as noted separately.
Signed-off-by: John C. Bland II <[email protected]>
The ViaService condition restricting alias creation to EC2 would prevent Terraform from creating the KMS alias. Keeping only the CallerAccount condition allows principals in the account to create aliases while maintaining proper account scoping. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
/terratest |
|
Thanks @johncblandii for creating this pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. While you wait, make sure to review our contributor guidelines. Tip Need help or want to ask for a PR review to be expedited?Join us on Slack in the |
2ecefc5
|
These changes were released in v1.537.2. |
what
Add KMS encryption support to the datadog-logs-archive component to address Drata compliance requirements. CloudTrail logs are now encrypted with SSE-KMS by default, with flexible configuration options to use an existing key or disable encryption if needed.
why
kms.tffile with KMS key, policy, and alias resourcesSummary by CodeRabbit
New Features
Documentation