feat: resolve webhook secrets from SSM and Secrets Manager#49
feat: resolve webhook secrets from SSM and Secrets Manager#49mtweeman wants to merge 1 commit intocloudposse-terraform-components:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as "Terraform Config"
participant Loc as "locals.webhooks"
participant SSM as "AWS SSM Data Source\n(aws_ssm_parameter)"
participant ASM as "AWS Secrets Manager Data Source\n(aws_secretsmanager_secret_version)"
participant Mod as "module.repository"
Dev->>Loc: Evaluate webhooks map
alt secret starts with ssm://
Loc->>SSM: lookup parameter key
SSM-->>Loc: return value
else secret starts with asm://
Loc->>ASM: lookup secret version
ASM-->>Loc: return secret_string
else secret is plain or null
Loc-->>Loc: passthrough or null
end
Loc->>Mod: pass normalized webhooks (secrets resolved/nonsensitive-wrapped)
Mod-->>Dev: module configured with resolved webhooks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.tf`:
- Around line 111-123: The webhook secret resolution currently uses
nonsensitive() to unwrap SSM/ASM values (in the webhooks mapping for
var.webhooks), which exposes secrets; change it to wrap the entire resolution in
sensitive() instead. Locate the webhooks block and the secret expression
referencing data.aws_ssm_parameter.default[...] and
data.aws_secretsmanager_secret_version.default[...] and replace the
nonsensitive(...) calls with sensitive(...) so the resolved SSM/ASM values
remain marked sensitive (preserving var.webhooks sensitive = true semantics) and
return null when v.secret is null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66a20ed3-a41c-46b6-a4fa-2aecaeaebfbf
📒 Files selected for processing (2)
src/main.tfsrc/variables.tf
| webhooks = { | ||
| for k, v in coalesce(var.webhooks, {}) : k => { | ||
| active = v.active | ||
| events = v.events | ||
| url = v.url | ||
| content_type = v.content_type | ||
| insecure_ssl = v.insecure_ssl | ||
| secret = v.secret != null ? ( | ||
| startswith(v.secret, "ssm://") ? nonsensitive(data.aws_ssm_parameter.default[v.secret].value) : | ||
| startswith(v.secret, "asm://") ? nonsensitive(data.aws_secretsmanager_secret_version.default[v.secret].secret_string) : v.secret | ||
| ) : null | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Terraform's nonsensitive() function permanently remove the sensitive mark from aws_ssm_parameter value and aws_secretsmanager_secret_version secret_string when passed downstream to a module input?
💡 Result:
Yes, Terraform's nonsensitive function permanently removes the sensitive marking from the specified value, including when that value is passed downstream as a module input. The resulting value is treated as non-sensitive throughout the configuration graph, including in the child module where it becomes a regular (non-sensitive) variable value.
Citations:
- 1: https://developer.hashicorp.com/terraform/language/functions/nonsensitive
- 2: https://www.terraform.io/language/functions/nonsensitive
- 3: https://docs.hashicorp.com/terraform/language/functions/nonsensitive
- 4: [Bug]: data ssm_parameter.value should not be sensitive hashicorp/terraform-provider-aws#31590
- 5: https://stackoverflow.com/questions/76463908/terraform-data-source-aws-ssm-parameter-causing-enygmatic-type-errors
🏁 Script executed:
#!/bin/bash
# Verify the code in src/main.tf and check the local.secrets pattern
echo "=== Lines 75-80 (local.secrets pattern) ==="
sed -n '75,80p' src/main.tf
echo ""
echo "=== Lines 111-123 (webhook secrets block) ==="
sed -n '111,123p' src/main.tf
echo ""
echo "=== Check variables.tf for webhooks definition ==="
grep -A 20 "variable.*webhooks" src/variables.tf | head -30Repository: cloudposse-terraform-components/aws-github-repository
Length of output: 1725
Wrap webhook secret field in sensitive() instead of nonsensitive().
The webhooks block (lines 111–123) uses nonsensitive() to strip the sensitive marker from resolved SSM and ASM values. This contradicts the sensitive = true marking on var.webhooks in variables.tf and exposes resolved secrets in plan output and logs.
The local.secrets block above (lines 75–80) demonstrates the correct pattern: wrap the entire resolution in sensitive() instead of using nonsensitive(). Apply the same approach to the webhook secret field:
Fix
webhooks = {
for k, v in coalesce(var.webhooks, {}) : k => {
active = v.active
events = v.events
url = v.url
content_type = v.content_type
insecure_ssl = v.insecure_ssl
- secret = v.secret != null ? (
- startswith(v.secret, "ssm://") ? nonsensitive(data.aws_ssm_parameter.default[v.secret].value) :
- startswith(v.secret, "asm://") ? nonsensitive(data.aws_secretsmanager_secret_version.default[v.secret].secret_string) : v.secret
- ) : null
+ secret = v.secret != null ? sensitive(
+ startswith(v.secret, "ssm://") ? data.aws_ssm_parameter.default[v.secret].value :
+ startswith(v.secret, "asm://") ? data.aws_secretsmanager_secret_version.default[v.secret].secret_string : v.secret
+ ) : null
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| webhooks = { | |
| for k, v in coalesce(var.webhooks, {}) : k => { | |
| active = v.active | |
| events = v.events | |
| url = v.url | |
| content_type = v.content_type | |
| insecure_ssl = v.insecure_ssl | |
| secret = v.secret != null ? ( | |
| startswith(v.secret, "ssm://") ? nonsensitive(data.aws_ssm_parameter.default[v.secret].value) : | |
| startswith(v.secret, "asm://") ? nonsensitive(data.aws_secretsmanager_secret_version.default[v.secret].secret_string) : v.secret | |
| ) : null | |
| } | |
| } | |
| webhooks = { | |
| for k, v in coalesce(var.webhooks, {}) : k => { | |
| active = v.active | |
| events = v.events | |
| url = v.url | |
| content_type = v.content_type | |
| insecure_ssl = v.insecure_ssl | |
| secret = v.secret != null ? sensitive( | |
| startswith(v.secret, "ssm://") ? data.aws_ssm_parameter.default[v.secret].value : | |
| startswith(v.secret, "asm://") ? data.aws_secretsmanager_secret_version.default[v.secret].secret_string : v.secret | |
| ) : null | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.tf` around lines 111 - 123, The webhook secret resolution currently
uses nonsensitive() to unwrap SSM/ASM values (in the webhooks mapping for
var.webhooks), which exposes secrets; change it to wrap the entire resolution in
sensitive() instead. Locate the webhooks block and the secret expression
referencing data.aws_ssm_parameter.default[...] and
data.aws_secretsmanager_secret_version.default[...] and replace the
nonsensitive(...) calls with sensitive(...) so the resolved SSM/ASM values
remain marked sensitive (preserving var.webhooks sensitive = true semantics) and
return null when v.secret is null.
mtweeman
force-pushed
the
feat/webhooks-secrets
branch
from
262cf63 to
5bf315b
Compare
1 participant
what
Handling webhook secret as sensitive data.
why
Extending the existing
ssm:///asm://prefix resolution, so that thesecretfield ofvar.webhooksis fetched from Parameter Store or Secrets Manager, matching the handling already used for variables, secrets and environments.Summary by CodeRabbit
New Features
Security