Skip to content

Commit 50dbeeb

Browse files
committed
fix(encryption): PR782 r3 codex P2 — narrow IsEncryptionRelevantOpcode to exclude OpRegistration
PR #782 round-3 codex P2 finding (Exclude registration opcode from sidecar gap predicate). ## codex r3 P2 — IsEncryptionRelevantOpcode false positive on registration The previous predicate range [OpEncryptionMin, OpEncryptionMax] included OpRegistration (0x03). But ApplyRegistration in applier.go only mutates writer-registry rows via SetRegistryRow; it NEVER calls WriteSidecar. So a gap containing OpRegistration entries does not actually need to refuse — the sidecar IS consistent because the sidecar's raft_applied_index is not advanced by registration applies (only WriteSidecar moves it). The design's §5.5 enumeration agrees: the listed sidecar-relevant opcodes are `rotate-dek`, `rewrap-deks`, `bootstrap-encryption`, `enable-storage-envelope`, `enable-raft-envelope`, `retire-dek` — notably NOT `register-writer`. The predicate range was wrong. Concrete impact of the bug: every restart after the first RegisterEncryptionWriter would fire ErrSidecarBehindRaftLog and refuse to boot, because: - registration entries bump engine.AppliedIndex but not sidecar.RaftAppliedIndex - the gap therefore exists and contains OpRegistration - the previous predicate classified that as relevant - the guard would fire → cluster boot loop Fix: narrow the range to [OpBootstrap, OpEncryptionMax] = [0x04, 0x07]. OpRegistration (0x03) is explicitly excluded with a clear IMPORTANT block in the godoc explaining why and pointing at ApplyRegistration as the authoritative reference. ## Semantic change — caller audit IsEncryptionRelevantOpcode's return value changes for opcode=0x03 (true → false). Per the cron directive, audited all callers: - audit.go (definition, updated) - audit_test.go (tests, updated to assert the new behavior: OpRegistration explicitly classified NOT relevant) - errors.go (docstring on ErrSidecarBehindRaftLog updated to reflect the narrowed range AND explicitly note that 0x03 OpRegistration is excluded) No production caller exists yet (the predicate is consumed by GuardSidecarBehindRaftLog whose only caller is 6C-2c, not yet landed). The 6C-2c scanner author will see the corrected predicate from day one. ## Verification - go test -race -timeout=60s ./internal/encryption/... — PASS (including the new TestIsEncryptionRelevantOpcode_KnownRanges sub-test "OpRegistration_0x03_NOT_relevant" that pins the exclusion explicitly) - golangci-lint run ./internal/encryption/... — 0 issues ## Test additions - TestIsEncryptionRelevantOpcode_AllRangeMembers now asserts OpRegistration is NOT relevant before iterating the sidecar-mutating range. - TestIsEncryptionRelevantOpcode_KnownRanges adds a dedicated "OpRegistration_0x03_NOT_relevant" sub-test for grep-able regression anchoring.
1 parent 8945b77 commit 50dbeeb

3 files changed

Lines changed: 83 additions & 31 deletions

File tree

internal/encryption/audit.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,31 @@ import (
66
)
77

88
// IsEncryptionRelevantOpcode reports whether the supplied FSM
9-
// opcode byte is one of the §5.5 encryption-relevant opcodes:
10-
// 0x03 OpRegistration, 0x04 OpBootstrap, 0x05 OpRotation, and
11-
// the reserved 0x06 / 0x07 slots in the fsmwire OpEncryption
12-
// range.
9+
// opcode byte is one of the §5.5 SIDECAR-MUTATING opcodes —
10+
// the set of entry types whose apply path writes the §5.1
11+
// keys.json sidecar and therefore matters for the
12+
// ErrSidecarBehindRaftLog gap-coverage check:
13+
//
14+
// - 0x04 OpBootstrap (bootstrap-encryption — §5.6)
15+
// - 0x05 OpRotation (rotate-dek + future sub-tags
16+
// rewrap-deks / retire-dek / enable-storage-envelope /
17+
// enable-raft-envelope — §5.2 + §7.1)
18+
// - 0x06 / 0x07 (reserved slots in the fsmwire
19+
// OpEncryption range for Stage 6E wire extensions; whether
20+
// they end up sidecar-mutating is design-stage-defined,
21+
// but conservatively treating them as relevant matches the
22+
// §5.5 enumeration and forces a future ABI extension to be
23+
// explicit about adding a NOT-relevant opcode).
24+
//
25+
// IMPORTANT — 0x03 OpRegistration is EXCLUDED. ApplyRegistration
26+
// in internal/encryption/applier.go only mutates writer-registry
27+
// rows via SetRegistryRow; it never calls WriteSidecar. Treating
28+
// it as sidecar-relevant would fire ErrSidecarBehindRaftLog on
29+
// EVERY startup after the first registration, since the sidecar's
30+
// raft_applied_index is not advanced by registration applies.
31+
// The design's §5.5 enumeration matches this exclusion (rotate-
32+
// dek, rewrap-deks, bootstrap-encryption, enable-storage-envelope,
33+
// enable-raft-envelope, retire-dek — no register-writer).
1334
//
1435
// IMPORTANT — wire-format contract: the opcode byte is `data[0]`
1536
// of the FSM entry payload (the LEADING opcode tag), NOT the
@@ -24,17 +45,17 @@ import (
2445
//
2546
// The predicate is the §9.1 ErrSidecarBehindRaftLog guard's
2647
// gap-coverage check: an unapplied entry in the sidecar/engine
27-
// gap matters iff its `data[0]` is in this range. Non-encryption-
28-
// relevant entries (writes, transactions, control-plane RPCs)
29-
// in the gap do not affect the encryption sidecar and are safe
30-
// to ignore.
48+
// gap matters iff its `data[0]` is in this range. Non-sidecar-
49+
// mutating entries (writes, transactions, control-plane RPCs,
50+
// AND OpRegistration) in the gap do not affect the encryption
51+
// sidecar and are safe to ignore.
3152
//
3253
// Defined here (rather than in fsmwire) so the encryption
3354
// package owns its semantic-level predicate; the fsmwire
34-
// package owns the wire-level constants (OpEncryptionMin /
55+
// package owns the wire-level constants (OpBootstrap /
3556
// OpEncryptionMax) that this function reads.
3657
func IsEncryptionRelevantOpcode(opcode byte) bool {
37-
return opcode >= fsmwire.OpEncryptionMin && opcode <= fsmwire.OpEncryptionMax
58+
return opcode >= fsmwire.OpBootstrap && opcode <= fsmwire.OpEncryptionMax
3859
}
3960

4061
// EncryptionRelevantScanner is the cross-package contract that

internal/encryption/audit_test.go

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,33 +10,45 @@ import (
1010
)
1111

1212
// TestIsEncryptionRelevantOpcode_AllRangeMembers pins the
13-
// §5.5 predicate against every byte in the [OpEncryptionMin,
14-
// OpEncryptionMax] range AND a sampling of out-of-range bytes
15-
// (including the boundaries one below and one above the range).
13+
// §5.5 predicate against every byte in the sidecar-mutating
14+
// range [OpBootstrap, OpEncryptionMax] AND a sampling of
15+
// out-of-range bytes (including OpRegistration directly below
16+
// the lower bound, since that is the most likely regression to
17+
// reintroduce false-positive refusals).
1618
// The predicate is the load-bearing input to the
1719
// ErrSidecarBehindRaftLog guard; a regression that widens or
1820
// narrows the range silently changes the security guarantee.
1921
func TestIsEncryptionRelevantOpcode_AllRangeMembers(t *testing.T) {
20-
// Every byte in the OpEncryption range MUST be relevant.
22+
// Every byte in the sidecar-mutating range MUST be relevant.
2123
//
2224
// Iterate via int so the loop terminates even if a future
2325
// stage sets OpEncryptionMax = 0xFF; a byte-typed loop
2426
// counter would wrap to 0x00 on opcode++ at 0xFF and the
2527
// loop would never exit. The design reserves 0x06 / 0x07
2628
// for Stage 6E, so the range is expected to grow and a
2729
// byte-typed loop is a latent overflow risk.
28-
for i := int(fsmwire.OpEncryptionMin); i <= int(fsmwire.OpEncryptionMax); i++ {
30+
for i := int(fsmwire.OpBootstrap); i <= int(fsmwire.OpEncryptionMax); i++ {
2931
opcode := byte(i)
3032
if !encryption.IsEncryptionRelevantOpcode(opcode) {
3133
t.Errorf("opcode 0x%02X in [0x%02X, 0x%02X] must be encryption-relevant",
32-
opcode, fsmwire.OpEncryptionMin, fsmwire.OpEncryptionMax)
34+
opcode, fsmwire.OpBootstrap, fsmwire.OpEncryptionMax)
3335
}
3436
}
37+
// OpRegistration sits just below the sidecar-mutating range
38+
// (0x03 < 0x04) and MUST NOT be relevant. ApplyRegistration
39+
// in applier.go only mutates writer-registry rows and never
40+
// calls WriteSidecar, so registration-only gaps would be
41+
// false-positive refusals if this byte were classified as
42+
// relevant.
43+
if encryption.IsEncryptionRelevantOpcode(fsmwire.OpRegistration) {
44+
t.Errorf("OpRegistration (0x%02X) must NOT be encryption-relevant (writer-registry only, never WriteSidecar)",
45+
fsmwire.OpRegistration)
46+
}
3547
// The byte one below the range must NOT be relevant.
36-
if fsmwire.OpEncryptionMin > 0 {
37-
below := fsmwire.OpEncryptionMin - 1
48+
if fsmwire.OpBootstrap > 0 {
49+
below := fsmwire.OpBootstrap - 1
3850
if encryption.IsEncryptionRelevantOpcode(below) {
39-
t.Errorf("opcode 0x%02X (one below OpEncryptionMin) must NOT be encryption-relevant", below)
51+
t.Errorf("opcode 0x%02X (one below OpBootstrap) must NOT be encryption-relevant", below)
4052
}
4153
}
4254
// The byte one above the range must NOT be relevant.
@@ -49,37 +61,52 @@ func TestIsEncryptionRelevantOpcode_AllRangeMembers(t *testing.T) {
4961
}
5062

5163
// TestIsEncryptionRelevantOpcode_KnownRanges pins the explicit
52-
// opcode values mentioned in §5.5: 0x03 (registration), 0x04
53-
// (bootstrap), 0x05 (rotation), plus 0x06/0x07 reserved slots.
54-
// Spelling them out by name catches a regression where someone
55-
// changes the constants but leaves the predicate range alone.
64+
// opcode values mentioned in §5.5: 0x04 (bootstrap), 0x05
65+
// (rotation), plus 0x06/0x07 reserved slots. Spelling them out
66+
// by name catches a regression where someone changes the
67+
// constants but leaves the predicate range alone.
68+
//
69+
// 0x03 OpRegistration is EXCLUDED — see the per-opcode comment
70+
// below and the IMPORTANT block in IsEncryptionRelevantOpcode's
71+
// godoc.
5672
func TestIsEncryptionRelevantOpcode_KnownRanges(t *testing.T) {
5773
for _, tc := range []struct {
5874
name string
5975
opcode byte
6076
}{
61-
{"OpRegistration_0x03", fsmwire.OpRegistration},
6277
{"OpBootstrap_0x04", fsmwire.OpBootstrap},
6378
{"OpRotation_0x05", fsmwire.OpRotation},
6479
// Reserved slots in the OpEncryption range. No named
6580
// constant yet (Stage 6E will assign them); pinning the
6681
// raw bytes here ensures the predicate still treats them
6782
// as relevant when 6E lands the wire-format extension
6883
// and a future reader is reminded the range is reserved-
69-
// not-just-currently-3-opcodes.
84+
// not-just-currently-2-opcodes.
7085
{"Reserved_0x06", 0x06},
7186
{"Reserved_0x07", 0x07},
7287
} {
7388
t.Run(tc.name, func(t *testing.T) {
7489
if !encryption.IsEncryptionRelevantOpcode(tc.opcode) {
75-
t.Errorf("known encryption opcode 0x%02X must be relevant", tc.opcode)
90+
t.Errorf("known sidecar-mutating opcode 0x%02X must be relevant", tc.opcode)
7691
}
7792
})
7893
}
94+
// OpRegistration (0x03) is in the OpEncryption opcode range
95+
// but is NOT sidecar-mutating — ApplyRegistration only writes
96+
// writer-registry rows via SetRegistryRow, never WriteSidecar.
97+
// Including it in the predicate would force startup refusal
98+
// on every restart after the first registration, since the
99+
// sidecar's raft_applied_index is not advanced by registration
100+
// applies.
101+
t.Run("OpRegistration_0x03_NOT_relevant", func(t *testing.T) {
102+
if encryption.IsEncryptionRelevantOpcode(fsmwire.OpRegistration) {
103+
t.Errorf("OpRegistration (0x%02X) must NOT be relevant (writer-registry only)", fsmwire.OpRegistration)
104+
}
105+
})
79106
// 0x00, 0x01, 0x02 (non-encryption FSM opcodes) MUST NOT be
80107
// relevant. The exact non-encryption opcode space is project-
81108
// specific; what matters here is that bytes below 0x03 are
82-
// never in the encryption range.
109+
// never in the sidecar-mutating range.
83110
for _, op := range []byte{0x00, 0x01, 0x02} {
84111
if encryption.IsEncryptionRelevantOpcode(op) {
85112
t.Errorf("non-encryption opcode 0x%02X must NOT be relevant", op)

internal/encryption/errors.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,14 @@ var (
178178
// ErrSidecarBehindRaftLog is the §9.1 startup-refusal guard
179179
// raised when the sidecar's raft_applied_index is behind the
180180
// raftengine's persisted applied index AND the gap covers any
181-
// encryption-relevant Raft entry (per §5.5's
182-
// IsEncryptionRelevantOpcode predicate: 0x03 OpRegistration,
183-
// 0x04 OpBootstrap, 0x05 OpRotation, plus the reserved opcodes
184-
// 0x06 / 0x07 from the fsmwire OpEncryption range).
181+
// SIDECAR-MUTATING Raft entry (per §5.5's
182+
// IsEncryptionRelevantOpcode predicate: 0x04 OpBootstrap,
183+
// 0x05 OpRotation, plus the reserved opcodes 0x06 / 0x07
184+
// from the fsmwire OpEncryption range). 0x03 OpRegistration
185+
// is in the OpEncryption range but is NOT sidecar-mutating
186+
// — its apply path writes writer-registry rows only and
187+
// never WriteSidecar, so registration-only gaps would be
188+
// spurious refusals.
185189
//
186190
// The classic scenario this catches is a partial-write crash:
187191
// an encryption-relevant entry was applied to the engine and

0 commit comments

Comments
 (0)