encryption: add storage-envelope metrics (Stage 9C-1, §9.2) - #1221
Conversation
Implements the three §9.2 metrics that the storage envelope path can
actually emit, and wires them from the store through main.go:
elastickv_encryption_decrypt_failures_total{reason}
elastickv_encryption_writes_per_dek{key_id}
elastickv_encryption_value_overhead_bytes
The reason label set is closed (tag_mismatch, unknown_key_id,
truncated, bad_version, plus an `unknown` catch-all so a failure is
never silently dropped). Classification lives beside the sentinels it
maps, in internal/encryption, so a new decrypt-path error and its
label are edited in one place; monitoring re-normalizes on the way in
because the label is derived from attacker-influenceable on-disk bytes
and must not mint unbounded series.
The rebadge guard's trial-decrypt loop is deliberately NOT observed: it
decrypts a cleartext body against every loaded DEK, so a tag mismatch
there is the expected outcome and counting it would fire the
paging-grade counter on essentially every cleartext read.
The other four §9.2 metrics are owned by the rotation / sidecar / KEK
subsystems and are left unregistered rather than registered-and-zero,
so an operator cannot mistake an always-zero series for health.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (15)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @bootjp's task in 3m 29s —— View job Review
Note: Overall this is a clean, well-scoped addition — the classifier, the observer interface split (store declares its own One thing worth a second look:
Everything else checked out:
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What
Implements Stage 9C-1 of
docs/design/2026_04_29_partial_data_at_rest_encryption.md— the three §9.2 metrics that the storage-envelope path can actually emit, wired end-to-end fromstorethroughmonitoring.Registrytomain.go.elastickv_encryption_decrypt_failures_total{reason}elastickv_encryption_writes_per_dek{key_id}elastickv_encryption_value_overhead_byteslen(stored payload) - len(plaintext)Design decisions worth reviewing
Classification lives with the sentinels.
encryption.DecryptFailureReasonmaps errors → the doc's closed reason set, ininternal/encryptionnext to the errors it classifies, so adding a decrypt-path error and its label is a single-file edit.monitoringre-normalizes on the way in: the label is derived from on-disk bytes an attacker can influence, so an unexpected string must collapse intounknownrather than mint a series.A failure is never dropped.
DecryptFailureReasonreturnsunknownrather than "no label" for an unclassified error — a paging-grade counter that silently skips what it doesn't recognize is worse than one with anunknownbucket. It returnsok=falseonly fornil, so a success can never be counted.The rebadge guard is deliberately unobserved.
rejectRebadgedEnvelopetrial-decrypts a cleartext body against every loaded DEK; a tag mismatch there is the healthy, expected result. Counting it would increment the paging-grade counter on essentially every cleartext read. This is pinned by a test and a comment at the site.The other four §9.2 metrics are left unregistered.
active_dek_id,last_proposed_index_per_raft_dek,kek_unwrap_seconds, andsidecar_raft_indexare owned by the rotation / sidecar / KEK subsystems. Registering them now would expose always-zero series an operator could mistake for health, so they land with their milestones. The doc's §9.2 list is annotated to say exactly which three are live.Hot-path cost. The per-
key_idchild counter is memoized behind an RWMutex, so the write path does notstrconv-format the key_id on every write. Nil observer (metrics disabled) costs one nil check.Behavior change / risk
Metrics-only. Nothing in the storage path reads an observation back, so a dropped or nil observation cannot change stored bytes, MVCC visibility, or apply determinism.
WithEncryptionObserver(nil)is a no-op.Two existing assertions on
pebbleOptions()length moved 4 → 5.Test evidence
go test . ./store/ ./monitoring/ ./internal/encryption/... -race -count=1— all passgolangci-lint run(full repo) — 0 issues, no//nolintaddeddiff -q):ObserveEncryptionWrite→TestEncryptionObserverCountsEveryEmittedEnvelopeFAILsTestEncryptionObserverCountsTagMismatchOnTamperedEnvelopeFAILsTestEncryptionObserverIgnoresRebadgeGuardTrialDecryptsFAILsWithEncryptionObserverfrom productionpebbleOptions()→TestEncryptionObserverReachesStoreThroughProductionWiringFAILsThe store-level tests build their own option list, so they would stay green if production never wired the observer.
TestEncryptionObserverReachesStoreThroughProductionWiringdrives the realbuildEncryptionWriteWiring → pebbleOptions() → NewPebbleStoretopology through Bootstrap → cutover → Put, and is the test that catches that case (revert #4 above). A compile-time assertion also pins thatmonitoring's implementation satisfiesstore.EncryptionObserver.Self-review (five passes)
env.Encode()succeeds and is discarded on any error path.-race. No new locks are taken while holding a store lock; the observer never blocks.key_idcounter memoized to avoid a per-writestrconvallocation; the histogram is unlabelled. Nil observer costs one nil check.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE