fix(helm): require API_KEY_HMAC_SECRET instead of deriving it from SECRET_KEY - #226
Merged
Merged
Conversation
…CRET_KEY secret.yaml silently derived API_KEY_HMAC_SECRET from secretKey when env.secret.apiKeyHmacSecret was left blank, so a chart-rendered Secret always looked "explicitly set" to core.config.api_key_hmac_secret and its fail-closed check never fired. That reopened the exact risk A5 was meant to close: a leaked SECRET_KEY would also expose API-key hashing. apiKeyHmacSecret now follows the same fail() policy as secretKey. Updated every helm-template caller (golden tests, chart-release.yml, scan-values.yaml, install/README docs) to pass an explicit value, and added a security-reviewer follow-up guard test (test_helm_api_key_hmac_secret_wiring.py) that renders all four workloads and confirms the render fails cleanly when the value is unset -- the Helm-side equivalent of test_prod_failclosed_secrets_compose_wiring.py. Ref: concurrency-scaling-tracker.md §2, A5 (27) Medium + Low follow-up.
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
security-reviewer follow-up on the A5 PR (#221, hash-migration to keyed
HMAC-SHA256 for API keys) flagged that
charts/trustedoss/templates/secret.yamlsilently derived
API_KEY_HMAC_SECRETfromsecretKey(
sha256sum "trustedoss-api-key-hmac-secret-v1:" + secretKey) instead offailing the render, unlike
secretKeyitself, which already fails via{{- fail ... -}}when unset. A derived value always looks "explicitly set"to
core.config.api_key_hmac_secret'sAPP_ENV != devfail-closed check, sothat check could never fire against a chart-rendered Secret — silently
reopening the "leaked SECRET_KEY also exposes API-key hashing" risk A5 set
out to close.
secret.yaml:apiKeyHmacSecretnow fails the render (same policy assecretKey) when unset andexistingSecretis not set.helm template/helm lint/helm installcaller that previouslyonly set
secretKeynow also setsapiKeyHmacSecret: the four goldentest files,
chart-release.yml's smoke render,ci/scan-values.yaml(Trivy IaC scan), and the installation docs (EN + KO mirror).
test_helm_api_key_hmac_secret_wiring.py: confirmsAPI_KEY_HMAC_SECRETis wired viasecretKeyRefinto all fourworkloads (backend, worker-scan, worker-default, beat) when set, that the
render fails with a clear message when unset, and that
existingSecretstill bypasses the requirement (mirrors
secretKey's existing behavior).Same defect class as
test_prod_failclosed_secrets_compose_wiring.py,which explicitly scoped itself out of Helm.
values.yamlandREADME.md(chart + docs-site, EN/KO) upgrade notesfor operators relying on the old silent derivation.
Test plan
helm lint --strict charts/trustedosswith explicit values, and with-f ci/scan-values.yamlhelm templaterenders fail with the expected message whenapiKeyHmacSecretis unset (full chart and--show-only)ruff check/mypyclean (test-only change, no production Pythontouched — coverage-gate has nothing to diff)
node tools/em-dash/lint.mjs --base origin/maincleannode tools/ko-style/lint.mjs --all --fail-on S2clean (0 findings)node tools/docs-uat/extract.mjs --lintclean, andnode tools/docs-uat/run.mjs --tier=nightly --doc=installation/helm.mdpasses with the updated command
actionlint .github/workflows/chart-release.ymlclean54 passed
tests/unit(5613 passed) andtests/integration(1672passed) — remaining local-only failures are pre-existing disk-pressure
and missing-local-worker artifacts on this laptop, none touching a file
in this diff