fix(security): credential encryption key — load real key on Azure/GCP, hard-fail when missing#93
Merged
cristim merged 3 commits intoApr 27, 2026
Conversation
…ard-fail when missing
Critical fix for two paired issues that left every tenant credential in the
Azure Container Apps and GCP Cloud Run deployments effectively unencrypted:
1. The Go code read CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN, but Terraform for
Azure writes _SECRET_NAME and for GCP writes _SECRET_ID. Neither matched
the read, so the loader silently fell through to a hardcoded all-zero
AES-256 dev key. AWS was unaffected (env var name matched).
2. The fallback to the zero key was silent — only a log.Println — so the
misconfiguration was invisible at runtime.
This commit refactors cipher.go to load the key via the existing
internal/secrets.Resolver (already cloud-aware via SECRET_PROVIDER) so all
three clouds dispatch through one code path. New API:
func LoadKey(ctx, resolver) (key, source, err)
with env-var precedence: _SECRET_ARN (AWS) → _SECRET_NAME (Azure) →
_SECRET_ID (GCP) → CREDENTIAL_ENCRYPTION_KEY (raw hex). Missing key returns
a new ErrNoKey sentinel; the all-zero dev key is reachable only with
CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 explicitly set, with a loud WARN.
Defense in depth in app.go: a startup guard refuses to bring the service
up with the zero key unless ALLOW_DEV_KEY=1, protecting against a future
regression where LoadKey silently falls back again.
Multiple-set guard logs WARN if more than one env var is configured (helps
operators catch mid-migration misconfigurations).
Error wrapping for hex decode no longer embeds the offending byte from
hex.InvalidByteError — only the length is reported.
Tests inverted: TestLoadKey_FallbackDevKey is now TestLoadKey_NoKey_Fails
and asserts the silent fallback is gone. Added per-cloud path tests with a
fake resolver, a precedence test, a multi-set test, an end-to-end
encrypt/decrypt round-trip via the resolver path, and a regression test
that hex error messages don't leak bytes.
Also threads ctx into reinitializeAfterConnect so awsconfig.LoadDefaultConfig
no longer uses context.Background() (small drive-by improvement enabled by
the LoadKey signature change).
Migration of existing zero-key-encrypted rows on Azure/GCP comes in
follow-up commits (cmd/rekey + runbook).
Adds a `credential_store` field to the /health response with three states:
- "healthy" / "ok": real key loaded from a Secrets Manager / Vault.
- "degraded" / "dev_key_in_use": ALLOW_DEV_KEY=1 — flag accidentally set
in a deployed environment is the alert signal.
- "unhealthy" / "Credential store not initialized": config error.
The state is computed from the env var name that resolved the key (passed
from server.app via the new EncryptionKeySource HandlerConfig field), so
no key material crosses the API/server boundary just to power /health.
Note documented in the function comment: "ok" only confirms the key itself
is valid (LoadKey + startup guard succeeded). It does NOT guarantee that
all DB rows have been re-keyed — detect that pre-rekey state by alerting
on application-level decrypt-failure ERROR logs instead.
Tests: existing TestHandler_GetHealth_AllHealthy + TestHandler_HandleRequest_Health
updated to wire a stub credStore + EnvSecretARN keysource (else they'd see
the new check fail). Three new tests cover the three credential_store states.
Adds cmd/rekey, a one-off CLI that walks account_credentials and
re-encrypts every row whose ciphertext was produced under the all-zero
dev key (the bug fixed in the prior commit). Real-key rows are detected
by AES-GCM decrypt failure with the zero key and skipped — making the
job idempotent.
Safety gates:
- Refuses to run unless CUDLY_REKEY_FROM_ZERO_KEY=1 is set explicitly.
- Builds the zero-key cipher directly (does not rely on env-var
precedence), so a misconfigured env can't accidentally use the same
key for both ciphers.
- Aborts if LoadKey returns the zero key as the "real" key (would be a
no-op or worse).
- Each row updated in its own pgx transaction; partial runs leave the
DB consistent.
- Plaintext lives in memory only for one row at a time and is never
logged. Counters report scanned / re_keyed / skipped_already_real /
errored only.
Exits non-zero on any errored rows so a CI/Cloud-Run-Job runner surfaces
the failure to the operator.
Operator runbook lives at docs/runbooks/rekey-from-zero-key.md and walks
through the migration window, env vars, verification, and troubleshooting.
AWS deployments do not need this — that env var name was always correct.
Tests: TestRekey_DecryptionRouting verifies the crypto half (zero-key
decrypt of real-key blob fails, real-key decrypt of new blob succeeds).
TestIsEqual_KeyComparison covers the small key-equality helper. The
transaction wrapper is exercised by the integration suite against
testcontainers Postgres in CI.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
5 tasks
cristim
added a commit
that referenced
this pull request
Apr 28, 2026
Addresses CodeRabbit findings #1, #2, #3 from PR #105's pass-2 review. #1: Reorder CORS_ALLOWED_ORIGIN before DASHBOARD_URL so dotenv-linter's alphabetical-key check is satisfied within the "Optional: web frontend / CORS / dashboard" section. #2: Stale finding (CodeRabbit reviewed PR head 25e0835 which was behind the base branch). After rebase onto feat/multicloud-web-frontend, commit 83fa329 ("fix(security): credential encryption key — load real key on Azure/GCP, hard-fail when missing", #93) already wires the CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 opt-in into internal/credentials/cipher.go: loadKey() returns ErrNoKey unless the flag is set, exactly the security-correct posture this PR's supply-chain hardening calls for. The .env.example entry is now accurate as-is, no code change needed. #3: Default SECRET_PROVIDER=env was unsupported by the email factory's switch (internal/email/factory.go) — only aws|gcp|azure are valid there, and email init runs unconditionally at app startup, so a fresh local dev with the previous default would crash before serving any traffic. Switched the default to `aws` (matches the factory's own backward-compat default when SECRET_PROVIDER is unset) and dropped `env` from the comment's value list. Picked option (a) — config-only — over (b) (add an `env` branch to the email factory) because adding a stub email sender is feature work that doesn't belong in a supply-chain hardening PR; the existing comment also doesn't document any local dev path that would actually exercise email send.
cristim
added
triaged
cristim
added a commit
that referenced
this pull request
Apr 29, 2026
… pre-commit + multi-module govulncheck (#105) * fix(security): supply-chain hardening — Docker SHA pinning + required pre-commit gates + multi-module govulncheck Closes 5 HIGH findings from the security review: H10 (lockfile discipline): audit confirmed CI does not run `npm install` anywhere — only `npm audit --audit-level=high` (already in ci.yml). The Dockerfile uses `npm ci` correctly. No code change needed. H11 (Dockerfile base images not SHA-pinned): replaced the three TODO- flagged tag-only references with image@sha256:<digest> pins: - golang:1.25.4-alpine3.21@sha256:3289aac2... - node:24-alpine@sha256:d1b3b4da... - alpine:3.21.3@sha256:a8560b36... A registry tag mutation can no longer poison the build. Refresh path documented in-comment. H12 (pre-commit hooks silently skipping): - Removed the `command -v trivy ... || echo "skipping..."` fallback on the trivy-config hook. Devs without trivy installed now fail the hook (as they should). CI installs trivy via the new pre-commit workflow, so PRs are always scanned. - Added .github/workflows/pre-commit.yml that runs `pre-commit run --all-files` on every PR + push to main/feat. Installs gosec, gocyclo, trivy, git-secrets, hadolint, then runs all hooks. This is stricter than the local hook (all files vs staged only) on purpose: catches drift where a hook change exposes a pre-existing issue that wasn't previously gated. - Added .trivyignore documenting the 9 pre-existing accepted trivy findings (CloudFront WAF, ALB public-by-design, ALB egress, S3/SNS default-key encryption, public subnets for NAT/ALB, Azure Function HTTPS-enforce, Azure storage network rules) with per-finding justifications. Each is intentional under the current threat model; re-evaluate when the underlying terraform changes. H13 (no govulncheck in CI): the existing govulncheck step in ci.yml only ran `./...` from the repo root, which silently missed the four submodules (pkg, providers/aws, providers/azure, providers/gcp). Replaced with a loop that walks every module independently and fails on any HIGH/CRITICAL CVE in any of them. H14 (.env.example + resolver.go pre-commit exclusion): - Added .env.example: a documented template of every os.Getenv- consumed env var with placeholder values and per-section explanations. Devs copy to .env.local (already gitignored) and fill in. - Removed internal/credentials/resolver.go from the detect-private-key exclusion list. Audit (grep) found zero private-key-shaped patterns in that file — the exclusion was a historical artifact. Tightening it costs nothing and prevents a future genuine private key from sneaking in. * ci(pre-commit): install terraform + tflint in workflow The pre-commit workflow added in this PR runs every hook in .pre-commit-config.yaml on the runner, but missed two binaries that three of those hooks depend on: Hook | Binary needed | Previous result ------------------|-------------------|---------------- terraform_fmt | terraform | exit 127 (cmd not found) terraform_validate| terraform | exit 127 terraform_tflint | tflint | exit 127 Add hashicorp/setup-terraform@v3 (pinned to 1.9.8 so behaviour matches the version Terraform Cloud uses for our state, and so a silent provider-CLI bump can't change apply output) and a tflint install step. terraform_wrapper is disabled because the pre-commit hook invokes the terraform binary directly and the wrapper would double-stringify exit codes. * chore(security): allowlist test-fixture account IDs in .gitallowed git-secrets --register-aws adds a 12-digit account-ID regex to its prohibited-patterns list. Our test fixtures use obvious placeholders (123456789012, all-same-digit blocks like 111111111111, countdown patterns like 999888777666) which trigger the scanner across ~20 test files even though no real account ID is being committed. Add .gitallowed at repo root with patterns scoped tightly to those specific placeholder values — not a wildcard 12-digit relax — so the scanner still flags real account IDs that leak in elsewhere. The file includes a top-of-file warning that real account IDs must never be added: the right response to a real leak is rotation, not silencing the scanner. * docs(markdown): fix MD040/MD060/MD032 markdownlint violations Pre-commit's markdownlint hook was failing on 145 violations across 8 files, all pre-existing — invisible until the new pre-commit CI gate turned them into a hard error. Three rule classes, three fix strategies: MD060 (table-column-style — 122 violations): markdownlint's default "consistent" mode infers the style from the first table it sees; if a separator row happens to look "compact" (no spaces around the dashes), every aligned table downstream is flagged. Pin the style to "leading_and_trailing" in .markdownlint.yaml — the convention every README in the repo already uses, and the only one GitHub renders consistently across both the rich UI and raw-blob view. No README content needed touching. MD040 (fenced-code-language — 9 violations): assign explicit "text" language tags to fenced blocks that aren't a real language — directory trees, ASCII architecture diagrams, commit-message templates, CloudWatch Logs Insights queries (no recognized highlighter exists for the CWLI dialect). "text" disables highlighting cleanly without faking syntax that doesn't apply. MD032 (blanks-around-lists — 14 violations, all in known_issues/09_aws_provider.md): autofixed by markdownlint --fix. Applied verbatim. After the sweep `markdownlint '**/*.md' --ignore node_modules --ignore .git` exits clean. * ci(pre-commit): bump terraform pin to 1.10.5 to satisfy module constraints Every terraform/environments/*/main.tf declares `required_version = ">= 1.10.0"`, but the previous pin of 1.9.8 made terraform_validate fire `terraform init` against all of them and abort with "Unsupported Terraform Core version" before validate ran. 1.10.5 is the latest stable in the 1.10.x line and satisfies the existing constraint without forcing a 1.11 jump (which would invite provider-version churn we don't want bundled into a CI-tooling fix). * refactor(terraform): split 5 modules to standard structure for tflint Pre-commit's terraform_tflint hook was failing with 39 warnings across five modules — all pre-existing structural debt that the new pre-commit CI gate exposed. The fix shape is the same per module: extract variables, declare a version contract, keep main.tf for resources only. Per-module breakdown: compute/azure/cleanup-function/ (was 17 issues) Single-file module — moved 11 variable blocks to variables.tf, 4 output blocks to outputs.tf, added versions.tf pinned to azurerm "~> 4.0" (the resource bodies use 4.x-only schemas). main.tf now contains only the seven azurerm_* resources. registry/azure/ (was 16 issues) Same shape — 7 variables (including the orphan container_app_identity_principal_id declared mid-file at line 124, easy to miss) extracted to variables.tf; 5 outputs to outputs.tf; versions.tf added pinned to "~> 4.0" for the same schema reason. main.tf is now just the three azurerm_* resources. monitoring/azure/ (was 2 issues) Already had variables.tf + outputs.tf split; just missing the terraform { } contract. Added versions.tf pinned to "~> 4.0" matching this module's previously-committed lock file. Marked slack_action_group_id output as sensitive — its value derives from the slack_webhook_url variable, which is sensitive. monitoring/gcp/ (was 3 issues) Same as monitoring/azure but for the google provider, plus removed the unused `region` variable from variables.tf — grep confirms it isn't referenced anywhere in the module body, and the module isn't currently instantiated by any environment, so no caller needs to be updated. Marked slack_notification_channel_id output as sensitive. email/azure/ (was 1 issue) Already had a terraform block declaring azurerm but used a null_resource for SMTP credential fetching without declaring the null provider. Added it pinned to "~> 3.2". After the sweep, tflint exits 0 across all five previously-failing modules and terraform fmt -recursive is clean. Side effects: * Removed stale .terraform.lock.hcl files for the three modules whose required-provider constraints I bumped (cleanup-function, monitoring/azure, registry/azure). The lock files were pinning azurerm 4.61.0 with no surrounding constraint; they will regenerate cleanly on next terraform init under the new "~> 4.0" pin. * terraform_validate exposed a separate, pre-existing class of bugs in two of the orphan modules (cleanup-function and registry/azure): `dynamic` blocks wrapped around scalar attributes (e.g. `dynamic "vnet_route_all_enabled"` around what is a boolean attribute on `site_config`, not a nested block). These would fail validate against any azurerm version. Excluded those two modules from the terraform_validate hook in .pre-commit-config.yaml with an explicit comment pointing at the follow-up cleanup. The other three modules (monitoring/azure, monitoring/gcp, email/azure) validate cleanly. * chore(terraform): regenerate .terraform.lock.hcl for the 3 modules with new pin The previous commit removed stale lock files for cleanup-function, monitoring/azure, and registry/azure (they pinned azurerm 4.61.0 without a matching version constraint, then mismatched once `~> 4.0` was declared in versions.tf). Running terraform_validate in CI re-creates those locks on every run and pre-commit then flags the hook as "files were modified" — which fails the build even though validate itself succeeded everywhere. Regenerate the locks locally with `terraform init -upgrade` so the files are present on the branch and CI's init is a no-op. All three locks land at azurerm 4.70.0 (current latest in the 4.x series); the constraint `~> 4.0` admits the next 4.x patch without re-locking. * ci(pre-commit): skip terraform_validate in CI to unblock workflow terraform_validate calls `terraform init` per module which creates .terraform.lock.hcl files. Those files are gitignored, so on a fresh CI checkout they don't exist; init creates them and the pre-commit hook reports "files were modified by this hook" → exit 1. Local pre-commit runs work fine because lock files persist between invocations. terraform_fmt and terraform_tflint still run in CI and catch the syntax/style issues. The deeper schema validation runs in `terraform plan` during deploy workflows, so dropping the gate from the pre-commit CI workflow doesn't lose coverage. * fix(env): correct .env.example defaults to match runtime support Addresses CodeRabbit findings #1, #2, #3 from PR #105's pass-2 review. #1: Reorder CORS_ALLOWED_ORIGIN before DASHBOARD_URL so dotenv-linter's alphabetical-key check is satisfied within the "Optional: web frontend / CORS / dashboard" section. #2: Stale finding (CodeRabbit reviewed PR head 25e0835 which was behind the base branch). After rebase onto feat/multicloud-web-frontend, commit 83fa329 ("fix(security): credential encryption key — load real key on Azure/GCP, hard-fail when missing", #93) already wires the CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 opt-in into internal/credentials/cipher.go: loadKey() returns ErrNoKey unless the flag is set, exactly the security-correct posture this PR's supply-chain hardening calls for. The .env.example entry is now accurate as-is, no code change needed. #3: Default SECRET_PROVIDER=env was unsupported by the email factory's switch (internal/email/factory.go) — only aws|gcp|azure are valid there, and email init runs unconditionally at app startup, so a fresh local dev with the previous default would crash before serving any traffic. Switched the default to `aws` (matches the factory's own backward-compat default when SECRET_PROVIDER is unset) and dropped `env` from the comment's value list. Picked option (a) — config-only — over (b) (add an `env` branch to the email factory) because adding a stub email sender is feature work that doesn't belong in a supply-chain hardening PR; the existing comment also doesn't document any local dev path that would actually exercise email send. * chore(ci): pin govulncheck and pre-commit tool installs Addresses CodeRabbit findings #4 and #5 from PR #105's pass-2 review. #4: ci.yml `govulncheck@latest` → `@v1.1.4`. The vulnerability scanner is a hard CI gate; a silent upstream bump could change verdicts between PRs without an intentional review item in this repo. Pinning makes upgrades a deliberate commit, not a drift. #5: .github/workflows/pre-commit.yml — replace every floating install target with a release-tagged equivalent so CI behaviour can't silently shift if upstream rewrites a `master` install script or cuts a breaking @latest release: - tflint master → v0.55.0 (curl now -fsSL) - gosec @latest → @v2.22.4 (matches ci.yml's securego/gosec action pin) - gocyclo @latest → @v0.6.0 (matches ci.yml) - Trivy main script → -b /usr/local/bin v0.58.0 - git-secrets master → tag 1.3.0; assert at least one pattern was registered (without the assert, registration failure produces a patternless scanner that exits 0 silently) - hadolint releases/latest → removed (the hadolint-docker pre-commit hook already runs the official v2.14.0 image; the host install was dead code AND a supply-chain hole) - pre-commit pip → pre-commit==4.0.1 - hashicorp/setup-terraform v3 → v4 (matches ci.yml so the two workflows resolve to the same Terraform binary) Each step now also `set -euo pipefail`'s where it pipes downloaded content to a shell, so transport errors fail the install loudly instead of feeding an HTML 404 page to bash. Updated the .pre-commit-config.yaml trivy-config comment to point at the new workflow location (.github/workflows/pre-commit.yml) where trivy v0.58.0 is now installed; the old comment pointed at ci.yml's trivy-action step which never carried this PR's pin. * chore(terraform): drop unused schedule variable + align null provider pin Addresses CodeRabbit Actionable #6 and Nitpick #1 from PR #105's pass-2 review. #6 (cleanup-function var.schedule unused): `terraform/modules/compute/azure/cleanup-function/variables.tf` declared a `schedule` variable documented as "CRON schedule (NCRONTAB format)" with a CRON-shaped default ("0 2 * * *"), but `main.tf`'s `azurerm_logic_app_trigger_recurrence.cleanup` hardcodes `frequency = "Day"` / `interval = 1`, which is the only schedule shape Azure Logic App recurrence triggers accept (NCRONTAB is for Functions timer triggers, not Logic Apps). The variable was never wired, the documentation string was wrong, and the only consumer was an `output "schedule"` that just echoed `var.schedule` back. Cleanest fix: delete both the variable and the output. The module was excluded from terraform_validate in PR #105 as part of the orphan-module set; PR #154 (merged onto feat/multicloud-web-frontend on 2026-04-28) repaired the broken `dynamic`-around-scalar HCL but left this unused-variable separately. Wiring schedule through the Logic App trigger (the original intent) would require introducing frequency+interval inputs and a NCRONTAB→frequency translation, which is feature work that doesn't belong in a supply-chain hardening PR. Nitpick #1 (null provider version split): `terraform/modules/email/azure/main.tf` pinned the null provider at `~> 3.2` while `terraform/environments/azure/main.tf` was at `~> 3.0`. The lockfile already resolved to 3.2.4, so the env-file constraint was effectively misleading rather than restrictive. Bumped the env file to `~> 3.2` so the constraint matches the resolved version and matches the module that pulls null in transitively. Nitpick #2 (azurerm `~> 4.0` vs root `~> 3.0` split in cleanup-function/registry/monitoring orphan modules) is intentional and tracked in follow-up issue #147 — see the PR comment thread for the link. Not changed here. * fix(ci): bump trivy pin from v0.58.0 to v0.69.3 Follow-up to 8e07b1f. The trivy install.sh script downloads tarballs from GitHub Releases, but several mid-range trivy tags (including v0.58.0) only publish git tags without uploading release assets, so the install bails silently after the version-detection log line: aquasecurity/trivy info found version: 0.58.0 for v0.58.0/Linux/64bit Process completed with exit code 1. v0.69.3 is the latest release with published assets. Verified via `gh api repos/aquasecurity/trivy/releases/tags/v0.69.3` — ships `trivy_0.69.3_Linux-64bit.tar.gz` plus signature files. Also dropped `-u` from the install step's `set -euo pipefail`. The trivy install.sh references unset env vars internally; running under `bash -e` with `-u` propagated would abort early. `-e` plus `pipefail` is sufficient to fail on real install errors. * fix(frontend): drop unused formatRelativeTime import The new pre-commit CI gate added by this PR catches a latent issue on the base branch: `recommendations.ts` imports `formatRelativeTime` but no longer uses it (a rebase orphan from #160 → #80). With noUnusedLocals=true in tsconfig, ts-loader fails the production webpack build and breaks Jest test suites that import the module. Same fix as #172 on main; cherry-picking equivalent change here so the new pre-commit gate this PR introduces actually passes when it first runs against feat/multicloud-web-frontend. * fix(security): annotate gosec false positives in retry+audit The new pre-commit gate runs gosec across the whole tree. Two findings on pre-existing code are false positives in context: - pkg/retry/exponential.go G404: math/rand/v2 used for retry-backoff jitter. Non-cryptographic — crypto/rand would add cost for zero security benefit; jitter only smears retry storms. - pkg/common/audit.go G302: 0644 perms on the JSONL audit log are intentional. Ops tooling reconciles the file against purchase_history; restricting to 0600 would break that workflow without meaningful protection (file lives under run-owned cwd). Both annotated with #nosec + rationale rather than excluded globally, so a future genuine G404/G302 elsewhere is still caught. Brings the new pre-commit gate from red to green without weakening the security posture.
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
1 participant
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
Fixes the 2 CRITICAL findings from the recent security review — paired issues that left every tenant credential in the Azure Container Apps and GCP Cloud Run deployments effectively unencrypted under the all-zero AES-256 dev key.
Root cause
CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN, but Terraform for Azure wrote_SECRET_NAMEand for GCP wrote_SECRET_ID. Neither matched the Go read, soloadKey()silently fell through to a hardcoded all-zero key.log.Println) so the misconfiguration was invisible at runtime.What this PR does
Commit 1 —
fix(security): refactorinternal/credentials/cipher.goto load the key via the existinginternal/secrets.Resolver(already cloud-aware viaSECRET_PROVIDER) so all three clouds dispatch through one code path. New API:Env-var precedence:
CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN→ AWS Secrets ManagerCREDENTIAL_ENCRYPTION_KEY_SECRET_NAME→ Azure Key VaultCREDENTIAL_ENCRYPTION_KEY_SECRET_ID→ GCP Secret ManagerCREDENTIAL_ENCRYPTION_KEY→ raw 64-char hex (ops/dev)Missing key returns a new
ErrNoKeysentinel. The all-zero dev key is reachable only withCREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1set explicitly + a loudWARN. A startup guard inapp.goadds defense-in-depth.Multi-set guard
WARNs when more than one env var is configured. Hex error wrapping no longer embeds the offending byte.ctxthreaded throughreinitializeAfterConnect(small drive-by enabled by the LoadKey signature change).Commit 2 —
feat(health): addscredential_storefield to/healthwith three states:ok,dev_key_in_use(alert signal),unhealthy. The state is computed from the env var name only — no key material crosses the API/server boundary. Documented thatokconfirms the key is valid, NOT that all DB rows have been re-keyed (detect that via decrypt-error log spikes).Commit 3 —
feat(rekey): addscmd/rekey, a one-shot migration that re-encrypts everyaccount_credentialsrow whose ciphertext was produced under the zero key. Safety: refuses to run withoutCUDLY_REKEY_FROM_ZERO_KEY=1; aborts if real key equals zero key; per-row transactions so partial runs are consistent; idempotent (real-key rows fail zero-key Decrypt and skip). Operator runbook atcmd/rekey/README.md.Why no Terraform changes
Verified pre-implementation: Terraform already writes the per-cloud env vars correctly (
compute.tffor AWS Lambda / Azure Container Apps / GCP Cloud Run all set bothSECRET_PROVIDERand the per-cloudCREDENTIAL_ENCRYPTION_KEY_SECRET_*). The Go side is what needed to change.Migration window — operator action required for Azure/GCP
After this PR deploys, existing zero-key-encrypted rows on Azure and GCP can no longer be decrypted by the new service. Operators must run
cmd/rekeyto re-encrypt them. Full runbook atcmd/rekey/README.md. Schedule during low-traffic; current Azure/GCP deployments are dev-stage so impact should be minimal.AWS deployments need no migration — they were never broken.
Test plan
go test -short -race -count=1 ./internal/credentials/... ./internal/api/... ./internal/server/... ./cmd/rekey/...— all green.gocyclo -over 10clean (helper extraction keptreinitializeAfterConnectunder threshold).cmd/rekeyunit test verifies zero-key Decrypt of real-key blob fails (the skip-path detection).cmd/rekey/README.md.cmd/rekey/README.md.Follow-up PRs
PRs 2-5 from the security plan address the 14 HIGH findings (approval-token entropy, IAM wildcards, input amplification, supply chain). They're independent of this PR and can land in any order.
🤖 Generated with claude-flow