encryption: memoize KEK unwraps across startup (Stage 9C-3)

[bootjp]

Stacked on #1223 (Stage 9C-2). Targets design/encryption-9c2-sidecar-kek-metrics; I'll retarget to main once that merges.

How this was found

Auditing *_implemented_* design docs for deferred follow-ups — my earlier inventory only counted docs by filename marker, which hides work that a shipped doc explicitly parked. 2026_05_25_implemented_6d6c2_production_storage_envelope_wiring.md §5 says:

Redundant KEK unwrap at startup is deferred to Stage 9. … For the file-mode KEK (the only provider today) the unwrap is a local AES operation, so the cost is negligible. Once Stage 9 lands KMS providers (network round-trips per unwrap) this doubles startup KMS calls

Stage 9B landed those providers (AWS KMS, GCP KMS, Vault Transit). The condition the deferral was waiting on has already arrived, so a node with N wrapped DEKs has been making 2N network round-trips to boot ever since.

What

encryption.StartupUnwrapCache memoizes wrapped → DEK across the startup phase, collapsing the §9.1 guard pass and HydrateKeystoreFromSidecar back to one provider call per DEK. Wired at the single KEK load site, so both hydration call sites benefit with no plumbing change.

Decisions worth reviewing

The constructor returns the kek.Wrapper interface, not *StartupUnwrapCache. Returning a typed nil pointer produces a non-nil interface holding nil — and startup decides whether encryption mutators may run from kekWrapper != nil. A node with no KEK configured would have reported one. I hit this while wiring it and caught it before pushing.

That test needed three attempts to be real. require.Nil is reflection-based and accepts a typed nil; so does got == nil when got is the concrete pointer type. Both passed with the bug present. The property only shows through an interface conversion, so the test routes the value through a kek.Wrapper-typed parameter — and now fails when the return type is reverted.

Failures are not cached. A transient KMS timeout must not become a permanent startup refusal.

Wrap is not memoized. Providers may add fresh randomness per call, so a cache there would be wrong rather than merely wasteful.

The cache sits OUTSIDE the §9.2 latency decorator (cache(timed(raw))), so a cache hit is not recorded as a zero-duration KMS round-trip and cannot flatten elastickv_encryption_kek_unwrap_seconds.

Reset zeroes and drops the entries. The keystore already holds every unretired DEK for the process lifetime, so this adds no new class of exposure — but there is no reason to keep a second copy of the plaintext alive past hydration.

Behavior change / risk

Startup makes fewer KMS calls; nothing else changes. wrapped → DEK is deterministic, so memoizing cannot alter a result. Errors are wrapped with errors.Wrapf, which preserves Is/As, so the §9.1 guards still match ErrKEKMismatch through the decorator — covered by the existing guard tests, which pass unchanged.

Test evidence

• go test . ./internal/encryption/... -race -count=1 — all pass
• golangci-lint run (full repo) — 0 issues; no //nolint in the new source (the two wrapcheck hits were resolved by wrapping properly, per the repo convention)
• Revert-checked (restores byte-exact):
  1. concrete return type → TestStartupUnwrapCacheReturnsANilInterfaceWithoutAKEK FAILs
  2. cache lookup removed → TestStartupUnwrapCacheCollapsesTheDuplicateStartupUnwrap FAILs
  3. failures cached → TestStartupUnwrapCacheDoesNotCacheFailures FAILs

6 tests: duplicate-unwrap collapse, typed-nil, failure non-caching, caller-isolation (a caller zeroing its DEK must not corrupt the cache), Reset, and Wrap/Name delegation.

Self-review (five passes)

1. Data loss — none; no storage or apply path touched. Key material is copied in and out so no caller shares the cached backing array.
2. Concurrency / distributed failures — all access is mutex-guarded; race-clean. Cached DEKs are copied on both store and load, so concurrent callers cannot observe each other's mutations.
3. Performance — this is the performance fix: halves startup KMS round-trips. Not on any request hot path.
4. Data consistency — wrapped → DEK is deterministic and stable, so the cache is semantically transparent. Errors preserve Is/As for the startup guards.
5. Test coverage — as above, three revert-checks. The typed-nil test is the one that matters most and is the one that took three tries to actually pin.

https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 12ceae85-f67b-4f18-8038-ac51200a4f99

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[bootjp]

@codex review

[bootjp]

@claude review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-08T08:22:14.527384Z	c7d5361	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude encountered an error after 3m 7s —— View job

---

Claude finished @bootjp's review request

• Gather context (diff, changed files, design doc)
• Read internal/encryption/kek_startup_cache.go and its test
• Trace how the returned kek.Wrapper is actually wired through main.go
• Check design doc updates
• Post review feedback

Review

The typed-nil handling, the "don't cache Wrap / don't cache failures" decisions, and the placement outside the §9.2 timing decorator are all correct and the tests genuinely pin them (I checked the typed-nil test does fail on a reverted return type via the asKEKWrapper conversion — that's a real trap and a good catch).

One finding that I think undercuts the PR's stated rationale:

Reset() is never called anywhere in production code — the cache lives for the whole process, not "across the startup phase."

• kek_startup_cache.go:132-134 says: "Call it once startup has hydrated the keystore; later unwraps (rotation applies) go straight to the provider."
• But loadKEKAndRunStartupGuards (main.go:1846-1879) returns the still-cache-wrapped kekWrapper and never calls .Reset() on it.
• That same value flows into buildShardGroupsWithEncryptionWiring → encryptionApplierOptionsFor → encryption.WithKEK(kekWrapper) → encryption.NewApplier(...) at main.go:1684, which stores it as a.kek on the Applier for the life of the process.
• a.kek.Unwrap(...) is called from ApplyBootstrap/ApplyRotation (internal/encryption/applier.go:725,729,1019) — i.e. on every future rotation an operator triggers, potentially days or months after boot, not just at startup.
• I grepped the whole tree for .Reset()/StartupUnwrapCache outside the two new files and the test file — there's no call site (main.go:1866 is the only production reference, and it's the constructor call, not Reset).

Net effect: the cache doesn't just memoize "across the startup phase" as documented — it silently accumulates one extra plaintext copy of every DEK ever unwrapped by this KEK for the entire process lifetime, which is exactly what the PR body's own reasoning says there's "no reason" to do ("there is no reason to keep a second copy of the plaintext alive past hydration"). It also means the doc comment on Reset describing the intended post-startup behavior doesn't match what actually ships.

Practical severity is moderate rather than critical — growth is bounded by the number of distinct wrapped blobs ever unwrapped (roughly one per rotation event), not attacker-controllable — but it's a real gap between the stated design and the wiring, and no test (unit or the main_encryption_*_test.go wiring tests) currently catches it, since the cache is only exercised in isolation.

Suggested fix direction: after the guard + hydration passes complete (i.e., once installEncryptionRotateOnStartup's startup-time rotation has also run, since that still needs the cache), either call Reset() on the cache and stop handing the cache-decorated wrapper to encryption.NewApplier — pass the inner/timed wrapper for the process-lifetime WithKEK wiring instead, so post-startup rotations "go straight to the provider" as the comment claims. Per CLAUDE.md's review convention, this would want a regression test first (e.g., an integration-style test asserting the applier's KEK path bypasses the cache after startup, or asserting cache size stays at 0 post-hydration) before the fix.

Everything else — error wrapping via errors.Wrapf preserving Is/As for ErrKEKMismatch, the copy-in/copy-out isolation between cache and caller, Wrap correctly staying unmemoized, crypto/subtle.ConstantTimeCopy for zeroing — checks out against the code as written.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7d5361946

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Stop caching unwraps after startup

When an encryption-enabled process reaches normal serving, this wrapper is retained by every applier, but production never calls Reset; moreover, Reset only clears the map and does not disable future caching. Consequently the sidecar DEKs and random KEK-preflight DEK remain as extra plaintext copies for the process lifetime, and each runtime rotation adds another cached DEK, contrary to the startup-only contract. End memoization after both startup hydration passes and bypass the cache thereafter before handing the wrapper to runtime code.

Useful? React with 👍 / 👎.