Stop trimming binary HMAC secrets before use

[jhrozek]

Summary

loadHMACSecrets (embedded vMCP OAuth authorization server) ran
bytes.TrimSpace on raw HMAC secret file bytes before using them as key
material. HMAC secrets are documented (HMACSecretRefs on
MCPExternalAuthConfig) as raw binary, "at least 32 bytes and
cryptographically random" — not text or base64. TrimSpace strips any
leading/trailing byte that happens to be ASCII whitespace
(0x09 0x0A 0x0B 0x0C 0x0D 0x20), which corrupts (shortens) a random
32-byte secret with probability ≈4.6% (1-(250/256)^2, empirically
confirmed at 4.74% over 200k trials). When that happens the secret drops
below the 32-byte minimum, Config.Validate() rejects it with a generic
"HMAC secret must be at least 32 bytes" error, and the safe
ephemeral-secret fallback never runs because it only triggers when
HMACSecrets == nil, not "non-nil but short". Net effect: roughly 1 in 21
embedded-auth-server pods fail to boot on a perfectly valid secret, with an
error that gives no hint the real cause is byte-trimming.

Found while investigating an intermittent e2e failure ("Redis-Backed
Session Sharing" spec) that looked like a CI flake but was root-caused to
this deterministic, probabilistic production bug.

What changed:

• loadHMACSecrets no longer trims — file bytes are used verbatim for
  both the current and rotated secrets.
• Added explicit length checks that name the file path and actual/required
  byte counts, so a genuinely short secret fails loudly and clearly instead
  of via a generic error three layers away.
• Deleted an unused, independent, identically-buggy duplicate loader
  (loadHMACSecretFile / LoadHMACSecrets in server/crypto/keys.go) —
  confirmed zero callers anywhere in the repo besides its own test.
• Added regression tests covering binary secrets with whitespace-valued
  edge bytes (current and rotated paths) and the new short-secret error
  messages; flipped the one existing test that had encoded the old
  (buggy) trimming behavior as correct.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Ran go test ./pkg/authserver/... (all packages ok) and a fresh
-count=1 -v -run TestLoadHMACSecrets pass confirming all 10 subtests in
pkg/authserver/runner succeed, including the 4 new regression cases, and
that the dead duplicate test in pkg/authserver/server/crypto is fully
gone ("no tests to run").

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File	Change
pkg/authserver/runner/embeddedauthserver.go	Remove both bytes.TrimSpace calls (current + rotated secrets); add explicit length checks with descriptive errors
pkg/authserver/runner/embeddedauthserver_test.go	Flip the old "trims whitespace" test; add 4 regression subtests
pkg/authserver/server/crypto/keys.go	Delete unused, identically-buggy duplicate loader (loadHMACSecretFile, LoadHMACSecrets)
pkg/authserver/server/crypto/keys_test.go	Delete the matching dead test and helper

Does this introduce a user-facing change?

Yes. The embedded auth server's HMACSecretRefs-based HMAC secret files
are now read verbatim instead of being whitespace-trimmed. Practical
impact: secrets exactly at the documented 32-byte minimum that previously
crash-looped the pod (the bug this PR fixes) now boot correctly. The
narrower case of an already-deployed secret longer than 32 bytes with an
incidental leading/trailing whitespace byte will pick up a different
(correct, full) key on upgrade — any auth codes/refresh tokens issued
before the upgrade become invalid, a one-time and harmless side effect
(short-lived tokens, no data loss).

Implementation plan

Approved implementation plan

Fix: loadHMACSecrets corrupts binary HMAC secrets via bytes.TrimSpace

Context

The embedded vMCP OAuth authorization server reads HMAC signing secrets from
mounted Kubernetes Secret files. loadHMACSecrets in
pkg/authserver/runner/embeddedauthserver.go runs the raw file bytes through
bytes.TrimSpace before using them as key material. HMAC secrets are
documented (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:383-390,
the HMACSecretRefs doc comment) as raw binary, "at least 32 bytes and
cryptographically random" — not text or base64. TrimSpace strips any
leading/trailing byte that happens to fall in the 6-byte ASCII-whitespace set
(0x09 0x0A 0x0B 0x0C 0x0D 0x20), which happens to a random 32-byte secret
with probability ≈4.6% (1-(250/256)^2, empirically confirmed at 4.74% over
200k trials). When that happens, the secret shrinks below the 32-byte minimum,
Config.Validate() (pkg/authserver/config.go:709-711) rejects it with "HMAC
secret must be at least 32 bytes", and the safe ephemeral-secret fallback in
applyDefaults() (config.go:1010-1018) never runs because it only triggers
on HMACSecrets == nil, not "non-nil but short". Net effect: roughly 1 in 21
vMCP embedded-auth-server pods fail to boot on a perfectly valid secret, with
an error that gives no hint the cause is byte-trimming.

This was found while investigating an intermittent e2e failure in PR #6056
(test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go, "Redis-
Backed Session Sharing" spec) and confirmed as a real production bug, not a
test artifact — the handover doc and follow-up research verified every step
of the chain against the actual code (call order, validation gate, fallback
bypass) and found a second, independent, identically-buggy implementation
sitting unused in the tree.

Decisions confirmed with the user:

• Remove trimming entirely (no heuristic trim of any kind) rather than a
  narrower TrimSuffix("\n") — any byte-level heuristic still has residual
  corruption risk on raw binary data; the CRD contract already documents
  these as opaque bytes with no encoding, so verbatim use is the only
  approach with zero residual risk. This is a pure runtime-behavior fix, no
  CRD/schema change. Practical impact: already-deployed secrets exactly at
  the 32-byte minimum with an edge whitespace byte currently crash-loop and
  will now boot correctly (pure fix, nothing to invalidate); the narrower case
  of secrets deployed longer than 32 bytes with such an edge byte will get
  a new (correct) key on upgrade, invalidating any auth codes/refresh tokens
  issued before the upgrade — one-time, harmless, worth a one-line PR note.
• Delete the dead duplicate in pkg/authserver/server/crypto/keys.go
  (loadHMACSecretFile, exported LoadHMACSecrets) in this same PR — same
  root-cause bug, confirmed zero callers anywhere in the repo (only its own
  definition and its own test reference it), and leaving it risks a future
  contributor wiring it up and reintroducing the exact bug just fixed.

Changes

1. pkg/authserver/runner/embeddedauthserver.go — the fix

In loadHMACSecrets (~lines 366-410):

• Delete current = bytes.TrimSpace(current) (current-secret path).
• Delete the bytes.TrimSpace(...) wrapper around secret in the
  files[1:] rotated-secrets loop — keep the bytes as read.
• Replace the now-wrong comment ("Trim whitespace (Kubernetes Secret mounts
  may include trailing newlines)") with one explaining why trimming is
  wrong: these are raw binary secrets per HMACSecretRefs's documented
  contract, and any byte — including whitespace-valued ones — may be
  genuine key material.
• Add an explicit length check right after each os.ReadFile, before the
  bytes are used, so a too-short secret fails with a message naming the
  file and the actual byte count instead of relying on the generic error
  from Config.Validate() three layers away:
  • Current: fmt.Errorf("HMAC secret in %s is %d bytes, but must be at least %d bytes", files[0], len(current), servercrypto.MinSecretLength).
  • Rotated: same pattern, worded for "rotated HMAC secret in %s".
• Leave Config.Validate()'s existing length check untouched — it remains
  valid defense-in-depth for any caller that builds Config.HMACSecrets
  directly without going through loadHMACSecrets.
• The bytes import stays — still used by the unrelated resolveSecret
  (~line 666), which trims a text-based OAuth client secret and is
  correctly out of scope.

2. pkg/authserver/runner/embeddedauthserver_test.go — regression coverage

• Flip the existing "trims whitespace from secrets" subtest (~lines
  170-185): it currently asserts trimming happens for a text secret with
  leading/trailing spaces/newlines — that's the old, now-wrong behavior.
  Rename to something like "preserves leading and trailing whitespace bytes in secrets" and assert the loaded value equals the full,
  untrimmed input bytes.
• Add a pathological-binary-bytes subtest: build a 32-byte slice with
  secret[0] = 0x20 and secret[31] = 0x0A (fixed non-whitespace filler in
  between), write it raw via os.WriteFile, load via loadHMACSecrets, and
  assert the result is exactly 32 bytes and byte-for-byte equal to the
  original. This is the direct regression pin for the corruption bug.
• Cover the rotated-secret path the same way — a second file with the
  same pathological edge bytes used as a rotated secret, asserting
  secrets.Rotated[0] is untouched.
• Add short-secret error-message subtests for both current and rotated
  paths: write a 31-byte file, assert the returned error mentions the file
  path and both the actual (31) and required (32) byte counts.
• Leave the other existing subtests (single file, multiple files, empty
  paths skipped, missing file, empty file) unchanged — none exercise
  whitespace bytes.

3. Delete dead duplicate in pkg/authserver/server/crypto/keys.go

• Remove loadHMACSecretFile and exported LoadHMACSecrets (same
  strings.TrimSpace-on-raw-bytes bug, zero production callers repo-wide).
• Remove the now-unused "strings" import (confirmed its only use in this
  file is inside the deleted function).

4. pkg/authserver/server/crypto/keys_test.go — drop the matching test

• Remove TestLoadHMACSecrets and the writeFileNamed helper (used only by
  that test).
• Remove the now-unused "strings" import (confirmed its only uses —
  strings.Repeat, three call sites — are inside the deleted test). Keep
  writePEM and everything else in the file untouched.

Out of scope (confirmed, don't touch)

• The sibling signing-key file path (servercrypto.LoadSigningKey) — reads
  raw bytes straight into pem.Decode with no trimming; PEM is a text
  format tolerant of surrounding whitespace by design, no bug there.
• cmd/thv/app/auth_flags.go:35 and cmd/thv/app/secret.go:429 — trim
  interactively-typed/CLI-flag secret input, a different trust context
  (human-typed text, not file-mounted binary key material).
• test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go — its
  existing rand.Read(32) raw-bytes setup is already the most adversarial
  realistic input; once the production fix lands this path is safe by
  construction, and the new deterministic unit test is a stronger, cheaper
  regression pin than re-running a probabilistic e2e spec. No test change
  needed there.

Verification

• task test scoped to the two touched packages (or the full suite if
  path-filtering isn't convenient): pkg/authserver/runner and
  pkg/authserver/server/crypto. The new pathological-byte subtest is
  deterministic, so a single green run is real proof (unlike the e2e spec,
  which passes ~95% of the time even when broken).
• task lint-fix before submitting — also catches any missed unused-import
  cleanup.
• No e2e re-run needed for this fix specifically (see "Out of scope" above).

PR

Single PR, one logical change ("fix HMAC secret file loading corruption"),
net deletion overall (~+20/-8 in the two fixed files, ~-190 in the two
dead-code files) — 4 files touched, comfortably within the 400-line/10-file
guideline. Mention the one-time-token-reissue side effect (see Context) in
the PR description as a one-line note, not a code change.

Special notes for reviewers

• Public API removal: LoadHMACSecrets in pkg/authserver/server/crypto
  was exported but had zero callers anywhere in this repo (confirmed via
  full-repo grep — only its own definition and its own test referenced it).
  It shared the exact same TrimSpace-on-raw-bytes bug as the code being
  fixed. Deleted rather than fixed in place to avoid leaving a second,
  dormant copy of the same defect. Flagging in case any external consumer
  of this module depends on it (none found in-tree).
• No issue was filed for this bug before starting — it was root-caused
  while investigating an unrelated e2e failure in Add Redis-backed mixed-era vMCP e2e coverage #6056; happy to file one
  retroactively if preferred for tracking.
• No file overlap with Deflake CI: TUF warn mode, readiness diagnostics, test races #6063 (in-flight CI-deflake PR) — checked directly.

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.19%. Comparing base (a099fa0) to head (31e028d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6067      +/-   ##
==========================================
- Coverage   72.20%   72.19%   -0.01%     
==========================================
  Files         721      721              
  Lines       75062    75060       -2     
==========================================
- Hits        54198    54191       -7     
- Misses      17003    17006       +3     
- Partials     3861     3863       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ChrisJBurns]

Multi-Agent Consensus Review

Agents consulted: security-hmac-reviewer, pr-reviewer-test-coverage, code-quality-agent

Overall

This is a correct, principled fix for a probabilistic-but-deterministic production defect. bytes.TrimSpace on raw binary HMAC key material corrupts secrets with ≈4.6% probability whenever an edge byte falls in the six-byte ASCII-whitespace set — causing ≈1-in-21 embedded auth-server pods to crash on startup with a misleading "HMAC secret must be at least 32 bytes" error. Removing the trim calls entirely (verbatim binary reads) is the right call: any byte-level heuristic retains residual corruption risk on raw binary data, and the CRD contract already documents these as opaque bytes with no encoding.

The dead code deletion is well-scoped: crypto.LoadHMACSecrets / loadHMACSecretFile have zero in-tree callers (confirmed via grep across the full repo), carry the identical strings.TrimSpace-on-raw-bytes bug, and deleting them now prevents a future contributor from wiring them up and reintroducing the exact defect just fixed. The new explicit length checks — naming the file path and actual/required byte counts — are a meaningful improvement over the generic error from Config.Validate() three layers away. Regression tests are deterministic, cover both the current and rotated secret paths, and include the exact pathological byte values (0x20, 0x0A) that triggered the bug.

---

Generated with Claude Code