feat(encryption): raft envelope + engine pre-apply hook (Stage 3)#744
feat(encryption): raft envelope + engine pre-apply hook (Stage 3)#744
Conversation
Stage 3 of the encryption rollout (design doc §4.2 + §6.3). Adds
the AES-256-GCM raft envelope used to wrap Raft proposal payloads
end-to-end through the WAL, plus the engine-side pre-apply hook
that unwraps committed entries past a configurable cutover index.
Wire format. Reuses the §4.1 storage envelope layout but with a
distinct AAD: 'R' (0x52) ‖ envelope_version ‖ key_id BE32. No
pebble_key — the raft envelope is location-independent; the engine
identifies entries by raftpb.Entry.Index. Storage and raft envelopes
therefore cannot be cross-replayed: a storage ciphertext fed to
UnwrapRaftPayload (or vice versa) fails GCM tag verification on
the AAD prefix.
Engine pre-apply hook. internal/raftengine/etcd/engine.go gains
two optional OpenConfig fields (RaftCipher, RaftCutoverIndex) plus
matching Engine struct fields. applyNormalEntry now decodes the
proposal envelope FIRST (preserving the leading
0x01 proposalEnvelopeVersion byte that resolveProposal needs), then
— iff cipher!=nil AND entry.Index > cutoverIndex() — unwraps the
inner fsm payload through the raft DEK before handing the
cleartext to fsm.Apply. The FSM contract is unchanged at
Apply(data []byte) any. resolveProposal continues to receive the
original entry.Data, so per-proposal id resolution and response
delivery are untouched.
Fail-closed. applyNormalEntry now returns (any, error). On unwrap
failure (GCM tag mismatch / unknown DEK / malformed envelope)
applyCommitted propagates ErrRaftUnwrapFailed up to runLoop and
DOES NOT advance setApplied — the next restart must replay the
same entry, not skip it. Silent skip would let the local FSM
diverge from peers that successfully unwrapped, defeating the
integrity property the tag was added to detect (design §6.3).
applyCommitted refactor. Each entry-type arm extracted into its
own helper (applyNormalCommitted, applyConfChangeCommitted,
applyConfChangeV2Committed) so the dispatch loop stays under the
cyclomatic budget while the new (any, error) signature is woven
through.
Coordinator seam. kv/raft_payload_wrapper.go introduces the
RaftPayloadWrapper closure type and a wrappedProposer adapter that
runs the wrapper transparently before forwarding to a
raftengine.Proposer. Stage 3 ships only the seam: a nil wrapper is
returned verbatim by newWrappedProposer, preserving the existing
zero-encryption behaviour for every coordinator path. Stage 6
installs the active wrapper (cipher + active raft DEK + nonce
factory) from the sidecar.
Stage 3 is INERT in production. With cipher=nil and cutover=
^uint64(0) (the OpenConfig defaults from orInertCutover), every
existing engine_test continues to pass byte-for-byte; the new
behaviour only fires when Stage 5/6 wires the cipher and cutover.
Test coverage:
- internal/encryption/raft_envelope_test.go: round-trip across 4
sizes, deterministic-nonce determinism, tag tamper, key_id
tamper, retired DEK, short-input, nil cipher, AAD layout pin
(TestBuildRaftAAD), layer-confusion (storage envelope rejected
by UnwrapRaftPayload), no-plaintext-leak.
- internal/raftengine/etcd/encryption_test.go: cipher=nil pass-
through, below-cutover pass-through (incl. boundary index ==
cutover), above-cutover Unwrap, unwrap-failure halts apply
loop without advancing setApplied, proposal id still
resolvable after unwrap, OpenConfig nil-cutover default.
- kv/raft_payload_wrapper_test.go: nil wrapper is identity,
wrapper invoked exactly once per Propose, propagates wrap-side
errors without touching the inner proposer, end-to-end round-
trip through a real Cipher.
Self-review (CLAUDE.md 5 passes):
1. Data loss: tampered raft entries fail closed via
ErrRaftUnwrapFailed; setApplied never advances past a failing
entry; runLoop's existing fatal-error path takes the process
down for operator-supervised restart.
2. Concurrency: raftCipher / raftCutoverIndex are set once at
Open and only read from the run-loop apply goroutine — same
single-writer discipline as fsm. The wrapper closure is
documented as concurrent-safe; Stage 3's pass-through default
avoids the question entirely.
3. Performance: cipher=nil hot path is one nil branch + one
uint64 compare per applied entry. With cipher set, the
Unwrap path adds one DecodeEnvelope + one AEAD.Open per
above-cutover entry; AES key expansion is amortised in the
keystore's pre-initialised AEAD cache.
4. Consistency: AAD purpose byte 'R' rules out storage↔raft
layer confusion; key_id participates in AAD so on-disk
rewrite fails GCM; cutover boundary is strict greater-than
so the enable-flag entry itself stays cleartext (design
§7.1).
5. Test coverage: 30+ unit tests across three packages plus the
existing raftengine_test.go which continues to pass with no
changes (cipher=nil legacy path).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Stage 3 of the Raft-level encryption design, introducing the core logic for wrapping and unwrapping Raft payloads using AES-GCM. It adds a new raft_envelope package for handling encrypted envelopes with distinct AAD, integrates an unwrap hook into the etcd-based Raft engine with cutover index support, and provides a coordinator-side wrapper for transparently encrypting proposals. The implementation ensures that decryption failures are fatal and prevent the advancement of the applied index to maintain consistency. I have no feedback to provide.
|
@claude review Stage 3 raft envelope + engine pre-apply hook の review trigger です。 主要変更点:
Stage 3 は production で inert (cipher=nil + cutover=^uint64(0) の OpenConfig default で従来動作 100% 維持)。Stage 5/6 で sidecar wiring 完了時に initial-active になります。 caller audit:
特に確認したい点:
|
|
Claude finished @bootjp's task in 3m 3s —— View job Stage 3 Raft Envelope ReviewTasks:
Layer-Confusion Verdict (§4.2 vs §4.1)Sound. The discriminant is byte 0 of the AAD:
Any storage ciphertext fed to One nuance worth documenting in the design doc: the
|
Two non-blocking documentation follow-ups from the Stage 3 review: 1. applyNormalCommitted: explicitly note that resolveProposal is intentionally NOT called on the unwrap-failure path. The originating coordinator's done channel is left dangling and reaped by process exit (§6.3 process-fatal contract). A graceful resolveProposal would deliver a misleading "committed-but-failed" signal that the coordinator could mis-interpret as a recoverable apply error. 2. applyNormalEntry: document the FSM payload aliasing asymmetry between cipher OFF (payload is a slice into entry.Data) and cipher ON (fresh allocation from Decrypt). FSM implementations that retain `data` past Apply's return without copying would see different ownership semantics depending on cipher state. Note that Stage 6 plans a defensive copy at the apply boundary to make the contract uniform. No code change. Tests / lint green.
|
@claude review PR744 round-2: claude[bot] round-1 review の 2 件の non-blocking follow-up を反映しました (commit 3292ced)。
コード変更なし、test/lint green。 |
|
Claude finished @bootjp's task in 1m 9s —— View job Stage 3 Raft Envelope — Round 2 ReviewTasks:
Both round-1 non-blocking follow-up items are correctly and fully resolved by commit 3292ced. No new issues found. Item 1 —
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3292cedb7e
ℹ️ 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".
| if e.raftCipher != nil && entry.Index > e.raftCutoverIndex() { | ||
| plain, err := unwrapRaftPayload(e.raftCipher, payload) | ||
| if err != nil { |
There was a problem hiding this comment.
Once the cluster starts submitting wrapped raft payloads (indexes above the cutover), this branch silently skips UnwrapRaftPayload whenever raftCipher is nil and sends ciphertext bytes directly to fsm.Apply, then setApplied advances as if apply succeeded. A node that starts with RaftCutoverIndex wired but RaftCipher missing (e.g., sidecar/init race or misconfiguration) will permanently diverge state instead of halting on an unwrap error.
Useful? React with 👍 / 👎.
…R744 codex P1)
The previous applyNormalEntry hook gated on `raftCipher != nil &&
entry.Index > cutover()`. When `raftCipher == nil` AND
`raftCutoverIndex` returned a real cutover (any value other than
the inert MaxUint64 default), an above-cutover entry would silently
fall through to fsm.Apply with raw envelope bytes. The cluster
would then advance setApplied as if the entry succeeded, while
peers that DID unwrap applied a different (cleartext) payload —
permanent FSM divergence from a sidecar/init race or operator
misconfig, exactly the silent-skip failure mode the integrity tag
was added to detect.
Restructure the gate as: `if entry.Index > cutover() { ... }`,
and inside the branch, if the cipher is nil, log + return
ErrRaftUnwrapFailed wrapped with a "cutover active but no raft
cipher wired" message. applyCommitted's existing fail-closed
machinery then halts the apply loop without advancing setApplied,
the same as it does for any other unwrap failure.
The default OpenConfig (no cutover, no cipher) still uses
RaftCutoverIndex = MaxUint64 via orInertCutover, so no entry
satisfies entry.Index > cutover and the new branch never fires.
The existing TestApplyNormalEntry_NoCipher_PassThrough test
exercises this path verbatim and continues to pass.
Caller audit (semantic change: fail-closed on a previously-silent
misconfig path):
- applyNormalEntry's only caller is applyNormalCommitted
(engine.go:2027), which propagates errors through to
applyCommitted's halt-without-setApplied path. New
ErrRaftUnwrapFailed surface is handled identically to the
existing tampered-envelope case.
- No other code path constructs an Engine with cutover wired
and cipher missing today (Stage 3 OpenConfig wiring is the
only seam, and it accepts both fields together). Stage 6's
cluster-flag pipeline must continue to wire both atomically
or expect this engine refusal.
New regression test
TestApplyNormalEntry_CutoverActive_NoCipher_FailsClosed pins both
sides of the contract: above-cutover with no cipher returns
ErrRaftUnwrapFailed and the FSM is NOT called; below-cutover with
no cipher continues to pass through (legacy cleartext entries
remain readable).
|
@claude review PR744 round-3: codex P1 Gap: 修正: gate を caller audit:
新規テスト
make test/lint green。 |
|
Claude finished @bootjp's task in 1m 49s —— View job Stage 3 Raft Envelope — Round 3 ReviewTasks:
P1 Fix VerdictCorrect. The gap was real and the fix is the minimal, right restructuring. The original gate: if e.raftCipher != nil && entry.Index > e.raftCutoverIndex()had the AND-short-circuit flaw: The fix correctly inverts the decomposition: if entry.Index > e.raftCutoverIndex() { // outer: "is this a post-cutover entry?"
if e.raftCipher == nil { // inner: "can we actually unwrap it?"
// fail closed
}
// normal unwrap
}The outer predicate answers "is decryption required for this entry?" and the inner answers "can we do it?" — which is the correct structure. Error Wrapping ConsistencyThe nil-cipher fail-closed path uses
|
1 participant
Summary
Stage 3 of the data-at-rest encryption rollout (design doc:
docs/design/2026_04_29_proposed_data_at_rest_encryption.md, §4.2 + §6.3). Wraps Raft proposal payloads in an authenticated AES-256-GCM envelope so the etcd/raft WAL on disk holds ciphertext only, and adds the engine pre-apply hook that unwraps committed entries past the §7.1 Phase 2 cutover index. Stage 3 is inert in production: with the newOpenConfigfields nil/unset, the apply hook never fires and every existing engine test continues to pass byte-for-byte.'R' ‖ envelope_version ‖ key_id) so storage↔raft layer-confusion replay fails GCM.applyNormalEntrybecomes(any, error); on unwrap failureapplyCommittedhalts without advancingsetApplied, mapping the entry-replay invariant directly into the runLoop's fatal-error path. Silent skip is rejected by design (§6.3).applyCommittedarms refactored intoapplyNormalCommitted/applyConfChangeCommitted/applyConfChangeV2Committedto stay under the cyclomatic budget while threading the new error.RaftPayloadWrapper+wrappedProposeradapter — Stage 3 ships only the seam (nil wrapper = identity); Stage 6 will install the active wrapper from the sidecar.Out of scope (deferred)
raftEncodeEncryptionRegistration/Bootstrap/Rotationopcodes)Test plan
go test -race ./internal/encryption/...— round-trip, tag/keyID tamper, retired DEK, short input, layer-confusion, no-leak, AAD layout pingo test -race ./internal/raftengine/etcd/...— cipher=nil pass-through, cutover boundary (strict greater-than), above-cutover Unwrap, unwrap-failure halts apply loop without advancing setApplied, proposal-id still resolvable after unwrap, OpenConfig nil-cutover defaultgo test -race ./kv/...— wrapper nil = identity, wrapper invoked exactly once, propagates wrap-side errors, end-to-end round-trip with real Ciphergolangci-lint run ./internal/raftengine/... ./internal/encryption/... ./kv/...cleanmake testfull sweep (CI verifies)Self-review (CLAUDE.md 5 passes)
ErrRaftUnwrapFailed;setAppliednever advances past a failing entry; runLoop's existing fatal-error path takes the process down for operator-supervised restart. The replay-not-skip invariant is the load-bearing safety property and is locked down byTestApplyCommitted_UnwrapFailure_DoesNotAdvanceApplied.raftCipher/raftCutoverIndexare set once atOpenand only read from the run-loop apply goroutine (same single-writer discipline asfsm). The wrapper closure type is documented as concurrent-safe; Stage 3's pass-through default sidesteps the question.cipher=nilhot path is one nil branch + one uint64 compare per applied entry. With cipher set, the Unwrap path adds oneDecodeEnvelope+ oneAEAD.Openper above-cutover entry; AES key expansion is amortised in the keystore's pre-initialised AEAD cache.'R'rules out storage↔raft layer confusion;key_idparticipates in AAD so on-disk rewrite fails GCM; cutover boundary is strict>so the enable-flag entry itself stays cleartext (design §7.1).raftengine_test.gocontinues to pass untouched.