Skip to content

fix(sync): ingest replayed quarantined grants + guard Prime's legacy custody upgrade (post-#33 review hardening)#34

Merged
Reederey87 merged 2 commits into
mainfrom
fix/p6-sec-01-replay-ingest
Jul 2, 2026
Merged

fix(sync): ingest replayed quarantined grants + guard Prime's legacy custody upgrade (post-#33 review hardening)#34
Reederey87 merged 2 commits into
mainfrom
fix/p6-sec-01-replay-ingest

Conversation

@Reederey87

Copy link
Copy Markdown
Owner

Summary

Hardening follow-up to #33 from the post-merge gpt-5.5 (Codex) security review. Four findings triaged: two real gaps fixed here, two are the already-documented residuals (P4-SEC-04, P6-SEC-03) now annotated in the specs.

Fix 1 — replay-time grant ingestion (availability, High)

A device.key.granted carrier that arrives before its author is approved locally is quarantined (event_verification_failure) and the pull cursor advances past it (PR #30 semantics). EncryptedHub.Pull is the only path that ingests a grant's WCK — and it never re-pulls that carrier. So the approve-time replay applied the event's audit row but the granted (epoch, kid) was permanently lost: every fleet event sealed under it would defer ("awaiting workspace key grant") forever.

replayQuarantinedEvents (both devices approve and devices enroll --approve) now unmarshals a replayed grant and calls Keyring.IngestGrant — recipient filtering, kid-mismatch rejection, and the no-overwrite guarantees all apply unchanged. Pinned by TestReplayIngestsQuarantinedGrant: grant quarantined under the fail-closed regime → approve → key metadata, custody slot, and decrypt candidates all held, conflict resolved.

Fix 2 — Prime legacy-upgrade custody guard (Medium)

Keyring.Prime's legacy kid-less backfill was the one surviving unconditional custody write: it computed the kid from the legacy slot and called StoreWCK even if the kid-aware target slot already held different bytes (local corruption/tampering). It now byte-compares and refuses a mismatch — the same P6-SEC-01b defense IngestGrant ships. Pinned by TestPrimeRefusesLegacyUpgradeOverDifferentBytes.

Documented residuals (spec notes only)

  • P4-SEC-04 (spec/15): pre-enrollment bootstrap grants gain 'grant' origin and hence push preference — the residual's full extent is now stated; closing it is the out-of-band fingerprint work.
  • P6-SEC-03 (spec/07): the permanent-truncate wedge extends to a hostile hub forging an unheld kid at a held epoch; the truncate-vs-skip trade is deliberate (skipping would lose legitimately-decryptable-later fleet events), the fix remains the bounded grace window.

Validation

  • gofmt -w cmd internal — clean
  • golangci-lint run (v2.12.0) — 0 issues
  • go run ./cmd/spec-drift --base origin/main --head HEAD — passed (20 specs, 8 changed files)
  • GOCACHE=/tmp/devstrap-gocache go test -race ./... — all pass

🤖 Generated with Claude Code

…custody upgrade (post-#33 review)

Two real findings from the gpt-5.5 review of merged #33:

1. A quarantined device.key.granted carrier is only WCK-ingested by
   EncryptedHub.Pull, which already advanced past it and never re-pulls.
   replayQuarantinedEvents (devices approve / enroll --approve) now ingests a
   replayed grant into the keyring, so the granted (epoch, kid) is recovered
   instead of permanently lost (fleet events sealed under it would otherwise
   defer forever).

2. Keyring.Prime's legacy kid-less backfill was the one surviving
   unconditional custody write; it now byte-compares an existing kid-aware
   slot and refuses a mismatch, matching IngestGrant's P6-SEC-01b defense.

The review's other two findings are the documented residuals P4-SEC-04
(bootstrap-window grants gain 'grant' origin and push preference) and
P6-SEC-03 (the truncate wedge extends to forged kids) - spec notes added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fbea2426-751b-4a0d-aef5-41e05f8565e2

📥 Commits

Reviewing files that changed from the base of the PR and between f5f527e and 5fbc503.

📒 Files selected for processing (7)
  • internal/sync/encryptedhub.go
  • internal/sync/encryptedhub_test.go
  • internal/sync/eventcrypt.go
  • internal/workspacekeys/keyring.go
  • spec/07_NAMESPACE_AND_SYNC_MODEL.md
  • spec/15_SECURITY_THREAT_MODEL.md
  • spec/18_WORK_LOG.md
👮 Files not reviewed due to content moderation or server errors (7)
  • spec/18_WORK_LOG.md
  • internal/sync/encryptedhub_test.go
  • spec/15_SECURITY_THREAT_MODEL.md
  • internal/sync/eventcrypt.go
  • internal/sync/encryptedhub.go
  • internal/workspacekeys/keyring.go
  • spec/07_NAMESPACE_AND_SYNC_MODEL.md

📝 Walkthrough

[!WARNING]

Walkthrough skipped

File diffs could not be summarized.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/p6-sec-01-replay-ingest

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

…a filter (review: relabeled kid must not wedge)

fable-5 review Major on #33: the kid rides outside the AAD until enc.v2, so
a hostile hub could relabel a genuinely decryptable event's kid to an unheld
value and wedge the device forever on the truncate path. Pull now tries the
exact-kid match first, then every held key at the epoch; truncate (defer)
only when the kid is unheld AND nothing decrypts (P6-SEC-02 fleet-key defer
preserved), skip only when the named key is held (or legacy kid-less) and
all candidates fail auth. Also corrects the overclaiming kid comment,
documents the kid-strip residual (enc.v2/P6-SYNC-04) in spec/15 and the
split-brain double-founding caveat in spec/07.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Reederey87

Copy link
Copy Markdown
Owner Author

Second review pass (fable-5 adversarial review of the merged #33 diff) added one more fix in 5fbc503:

Major — relabeled-kid wedge. The envelope kid is outside the AAD until enc.v2, so a hostile hub could take a genuinely decryptable event, relabel its kid to any unheld 64-hex value, and wedge the device forever on the truncate path — even though it held the decrypting key. Pull now treats the kid strictly as a candidate-ordering hint: exact match first, then every held key at the epoch; truncate (defer) only when the kid is unheld and nothing at the epoch decrypts (the P6-SEC-02 fleet-key defer is preserved), skip only when the named key is held (or the envelope is legacy kid-less) and all candidates fail auth. Pinned by TestEncryptedHubRelabeledKidStillDecrypts; all prior poison/truncate pins pass unchanged.

Also from that review: the kid-STRIP availability residual (targeted event drop for a not-yet-granted joiner; closed by the enc.v2 AAD binding, P6-SYNC-04) is now documented in spec/15, the overclaiming comment in eventcrypt.go is corrected, and the split-brain double-founding caveat (init --join on second machines) is documented in spec/07. Verdict items not fixed here are the two scheduled residuals (P4-SEC-04, P6-SYNC-04) plus two nits addressed as comments.

@Reederey87 Reederey87 merged commit 8b10be9 into main Jul 2, 2026
11 checks passed
@Reederey87 Reederey87 deleted the fix/p6-sec-01-replay-ingest branch July 2, 2026 11:25
Reederey87 added a commit that referenced this pull request Jul 2, 2026
…te time (P6-DATA-01) (#35)

* fix(data): record the origin's draft_snapshots row atomically at create time (P6-DATA-01)

The device that ran `draft snapshot create` inserted the
draft.snapshot.created event but never wrote its own draft_snapshots row
(the apply path skips already-present events), so routine sync local GC
and `hub gc` deleted the only copy of the origin's draft bundle blob.

- Extract Store.InsertLocalEvent's stamping body into the exported
  Store.InsertLocalEventTx; InsertLocalEvent is now a thin WithTx wrapper.
- `draft snapshot create` and the revoke-rewrap superseding path insert
  the event and the origin's draft_snapshots row in one transaction
  (InsertLocalEventTx + tx.RecordDraftSnapshotTx).
- DraftSnapshotRef carries NamespaceID so the rewrap path can record the
  superseding row.
- Ledger reconciliation: P6-SYNC-01/P6-SEC-01/P6-SEC-02 (PRs #30-#34) and
  P6-DATA-01 moved to Recently shipped; remaining P1s are P6-HUB-01 and
  P6-GIT-01.

Pinned by TestInsertLocalEventTxMatchesInsertLocalEvent,
TestDraftSnapshotCreateRecordsOriginSnapshotRow,
TestRewrapDraftBlobRecordsOriginSupersedingSnapshot, and the e2e
draft_snapshot_gc_retains_origin.txtar (create -> sync -> hub gc on the
origin -> blob survives locally and on the hub).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs(gc): note the intentional duplicate origin row after rewrap (review nit)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant