Verify the ML-KEM identity binding before persisting it; preserve the pairing-established AK on repair - #625
Merged
Merged
Conversation
The storage node stored `kyber_binding_sig` and never checked it. `register_device` validated length and presence only, under a comment asserting that "the node is a dumb indexer: it enforces length/presence only; the cryptographic identity binding is verified client-side against the peer's AK." Client-side verification is PARTIAL, not absent — and an earlier version of this message said "absent", which was wrong. `dsm_sdk`'s `repair_contact_identity_from_quorum` (handlers/app_router_impl.rs:360) does call `verify_kyber_identity_binding`, and does it correctly: against the QR-established `contact_ak`, captured at :318 with an explicit comment about doing so BEFORE repair overwrites `public_key`. Someone had already reasoned about the substitution problem on that path. But that is one repair path, not the general fetch path, and it is on the client. The node itself checked nothing. So a device could bind any ML-KEM key to its own identity by assertion, and the node would persist it and serve it to every peer that looked the device up — including peers with no contact record and therefore no repair path to protect them. Being a dumb indexer is about not interpreting CONTENT. It was never a reason to persist an identity claim without checking the signature that makes it a claim. `register_device` now calls the verifier BEFORE the insert. It re-derives `H(domain ‖ device_id ‖ genesis_hash ‖ kyber_pubkey)` and requires `Ok(true)` from `sphincs_verify`, so `Err` and `Ok(false)` are both refusals and neither reaches the database. Refusal is a new `KyberBindingDoesNotVerify` variant, kept distinct from `InvalidKyberBinding` so a forgery is never reported as a formatting problem. Tests drive the REAL handler against an in-memory database and read the persisted rows back. They deliberately do not test the standalone verifier: a_canonical_binding_is_accepted_and_persisted (anti-vacuity) a_forged_binding_is_refused_and_nothing_is_persisted a_substituted_kyber_key_is_refused_and_nothing_is_persisted an_old_domain_binding_is_refused (impact-table B4) a_malformed_binding_is_refused a_binding_signed_by_another_key_is_refused a_rejected_registration_cannot_be_completed_by_retrying Each rejection asserts NO ROW EXISTS afterwards, proving verification happens before persistence rather than alongside it. The digest is rebuilt independently in the test file so the suite does not echo the implementation it gates. Discarding the verification result turns six of the seven red; the acceptance test stays green, which is how you can tell it is not the one carrying the proof. WHY THE EXISTING TESTS MISSED IT: `device_api::tests` return early unless `DSM_RUN_DB_TESTS=1`, so all three are vacuous in CI — passing in 0.00s with a 64-byte dummy signature. NOT FIXED HERE, and worth naming because it is a separate trust-root defect found while correcting the above: `app_router_impl.rs:344` sets `contact_record.public_key = authoritative.public_key`, overwriting a QR/BLE -established AK with a node-supplied one. The verification just above it is safe because `contact_ak` was captured first, but every LATER reader of `contacts.public_key` may now be reading node-derived material. Filed separately. Provenance audit of every `contacts.public_key` writer, for the record: BLE update_contact_public_key <- bilateral_ble_handler.rs:2315, :3095 QR contact_sdk.rs:338, :648 <- resolve_counterparty_via_transport(&qr) clean export.rs:255 writes an empty key, deferred to BLE NODE-DERIVED app_router_impl.rs:344 <- the defect above test-only contact_sdk.rs:1247, transactions.rs:576, ~24 test sites
cryptskii
force-pushed
the
fix/kyber-binding-verification
branch
from
August 6, 2026 18:05
7ac85f5 to
f0fe970
Compare
…ablished AK The read-side trust root for a contact is the AK established in person via QR/BLE pairing. `repair_contact_identity_from_quorum` verified the recipient's ML-KEM identity binding against that pinned `contact_ak` (correct) — but then OVERWROTE `contact_record.public_key` with the node/quorum-served `authoritative.public_key` whenever they differed. So a storage node could substitute its own AK into a contact after one repair, and a subsequent repair would then verify future Kyber bindings against the node's AK: silent identity substitution. Fix: a non-empty authoritative AK that differs from the pinned `contact_ak` is now REJECTED (fail-closed, prior contact untouched, no persist); the AK is NEVER overwritten. Node-derived repair may still refresh genesis/Kyber material, but only under the pinned AK — the Kyber binding is verified against `contact_ak` exactly as before, so forged material fails closed regardless. A genuinely rotated AK is a new identity that must be re-established in person (no implicit TOFU). Invariants held (unchanged callers): the send path already rejects an unknown peer (must be an added contact) and aborts on repair Err with nothing persisted; the read-side hydrate path (`hydrate_missing_sender_kyber_capability`) already requires a pinned AK, verifies against it, and binds the Kyber key only if absent — never overwriting the AK. The AK-trust decision is extracted to the pure `authoritative_ak_permits_repair` and pinned by a mutation-style test (`ak_trust_root_tests`): a node-substituted AK is rejected; absent/matching is permitted; disabling the gate turns the test red.
The AK-substitution guard was covered only by a test of the pure
`authoritative_ak_permits_repair` comparator. That proves the comparator, but not
that the repair handler can't be refactored into persisting before the guard, or
into bypassing the helper entirely.
Extract the trust decision into a pure, I/O-free `repair_contact_decision`
returning `ContactRepair::{Unchanged, Repaired}`. The async
`repair_contact_identity_from_quorum` now persists only on `Repaired`, so
`store_contact` is textually unreachable until the AK guard AND the ML-KEM binding
verification have both passed — the persist-before-verify ordering cannot regress.
Two tests exercise `repair_contact_decision` itself (the exact decision the handler
runs), not just the comparator:
- a node that serves a binding VALID under the pinned AK but substitutes a
different device AK is rejected (Err, no Repaired record -> store_contact never
reached, pinned row untouched);
- a node AK that matches the pinned AK plus a valid canonical binding yields
Repaired with genesis/Kyber refreshed and `public_key` preserved exactly as the
pinned AK.
The rejection test carries a genuinely valid binding on purpose so that only the
AK guard can reject it; disabling `authoritative_ak_permits_repair` flips it to
Repaired and turns it red (mutation-verified, then restored).
`binding_digest` is made `pub(crate)` so the test can construct a real
SPHINCS+-signed ML-KEM binding under a test AK.
…he trust-root tests CI's Rust job Lint step is `make lint`: `cargo fmt --all -- --check` then `cargo clippy --all-targets -- -D warnings`. `--all-targets` lints cfg(test) code, which `ci/production_safety_checks.sh` does not — so two fixed-size `&vec![..; N]` in a test (clippy::useless_vec) and one `matches!(outcome, Err(_))` (clippy::redundant_pattern_matching) were hard errors under -D warnings, and rustfmt wanted the new assert!/map_err wrapping reflowed. No behavior change: `outcome.is_err()` is identical to `matches!(outcome, Err(_))` and `&[0xE5u8; 64]` to `&vec![0xE5u8; 64]`. The three ak_trust_root_tests still pass; fmt --check and clippy --all-targets -- -D warnings are both green locally.
cryptskii
added a commit
that referenced
this pull request
Aug 7, 2026
…ber-trust-boundary Restore the Kyber trust boundary: storage node non-authoritative (revert #625's node gate) + preserve pinned AK on BLE prepare (ADR 0002)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Node-side: verify before persist
The storage node stored
kyber_binding_sigand never checked it.register_devicevalidated length and presence only, under a comment asserting that "the node is a dumb indexer: it enforces length/presence only; the cryptographic identity binding is verified client-side against the peer's AK."Correction to an earlier version of this description
An earlier version of this PR said that client-side verification did not exist and that
verify_kyber_identity_bindinghad "zero callers anywhere in the repository." That was wrong.repair_contact_identity_from_quorum(handlers/app_router_impl.rs:360) does call it, and does it correctly — against the QR-establishedcontact_ak, captured at:318with an explicit comment about doing so before repair overwritespublic_key. Someone had already reasoned about the substitution problem on that path.The cause of the error was a truncated
grep … | head -12whose visible output I treated as complete — the same mistake that earlier undercounted the domain-cut breaking set as four when it was six.Client-side verification is therefore PARTIAL, not absent.
Node-side verification is admission hardening, NOT the trust boundary
The node previously checked nothing — it stored
kyber_binding_sigand served it back unverified. Verifying it before persistence is worth doing (reject internally-inconsistent submissions, reduce poisoning and storage abuse), and this PR adds it. But it must be read for exactly what it is:req.pubkey— the AK the registrant itself supplied. That proves the submission is internally consistent: the registrant holds the private key for the AK it claims, and the Kyber key is signed under that AK. It does not establish that the AK is the identity any peer trusts — a device can still bind any Kyber key to its own self-chosen AK and register it.The trust boundary is the client. It verifies the binding against the peer AK obtained out-of-band via QR/BLE contact state (
verify_kyber_identity_binding(…, contact_ak)) and only then uses/caches the Kyber key. A peer with no contact record has no trusted AK, so it must not trust node-served identity at all — no implicit TOFU — and node-side verification does nothing to protect it, because the served AK is self-asserted. That client read-side trust-provenance is the actual security requirement, and it is what the second half of this PR (392c3aed+ tests) enforces.Being a dumb indexer is about not interpreting content. Verify-before-persist is hygiene against garbage and poisoning; it was never — and this PR does not make it — a substitute for the client checking the signature against an AK it trusts out-of-band.
The change
register_devicecalls the verifier before the insert. It re-derivesH(domain ‖ device_id ‖ genesis_hash ‖ kyber_pubkey)and requiresOk(true)fromsphincs_verifyagainst the registrant-supplied AK (req.pubkey) —ErrandOk(false)are both refusals and neither reaches the database. Refusal uses a newKyberBindingDoesNotVerifyvariant, distinct fromInvalidKyberBinding, so a forgery is never reported as a formatting problem. Note the verification key is the registrant's own AK: this is the internal-consistency check of the admission-hardening note above, not an identity trust decision.Tests drive the real handler
7 tests against an in-memory database, reading persisted rows back. They deliberately do not test the standalone verifier — testing the verifier is what failed to notice its coverage was partial.
a_canonical_binding_is_accepted_and_persisteda_forged_binding_is_refused_and_nothing_is_persistedOk(false), notErra_substituted_kyber_key_is_refused_and_nothing_is_persistedan_old_domain_binding_is_refuseda_malformed_binding_is_refuseda_binding_signed_by_another_key_is_refuseda_rejected_registration_cannot_be_completed_by_retryingEvery rejection asserts no row exists afterwards, proving verification precedes persistence. The digest is rebuilt independently in the test file so the suite does not echo the implementation it gates.
Mutation-checked: discarding the verification result turns 6 of 7 red; the acceptance test stays green, which is how you can tell it is not the one carrying the proof.
device_api::testsreturn early unlessDSM_RUN_DB_TESTS=1— all three pass in 0.00s with a 64-byte dummy signature. Vacuous in CI. This suite is gated tolocal-devwith no skip path.The client-side trust-root defect — now fixed in this PR
This branch now also carries the client repair-path fix (previously deferred to its own branch).
repair_contact_identity_from_quorumused to setcontact_record.public_key = authoritative.public_key, overwriting a QR/BLE-established AK with a node-supplied one whenever they differed. The verification immediately above was safe becausecontact_akwas captured first — but every later reader ofcontacts.public_keywas then reading node-derived material, and the next repair would verify future Kyber bindings against the node's AK: silent identity substitution.Fix (commit
392c3aed+ test hardening): a non-empty authoritative AK that differs from the pinnedcontact_akis now REJECTED — fail-closed, prior contact untouched, nothing persisted. The AK is never overwritten. Node-derived repair may still refresh genesis/Kyber material, but only under the pinned AK; the Kyber binding is verified againstcontact_akexactly as before, so forged material fails closed regardless. A genuinely rotated AK is a new identity that must be re-established in person — no implicit TOFU.The trust decision is extracted to a pure, I/O-free
repair_contact_decision, sostore_contactis textually unreachable until the AK guard and the ML-KEM binding verification have both passed — a future refactor cannot slip a persist ahead of the guard.Repair-path tests drive the real handler, not just the guard
Two tests exercise
repair_contact_decision(the exact decision the async handler runs), not only the pure AK comparator:repair_rejects_ak_substitution_even_with_a_binding_valid_under_the_pinned_akRepairedrecord, sostore_contactis never reached and the pinned row is byte-for-byte untouchedrepair_refreshes_genesis_and_kyber_but_preserves_the_pinned_ak(genesis, Kyber)→ Repaired, genesis and Kyber updated,public_keyremains exactly the pinned AKThe rejection test carries a valid binding on purpose: that isolates the AK guard (a malformed binding would be caught by the Kyber verify regardless and prove nothing about the guard). Mutation-checked: disabling
authoritative_ak_permits_repairflips the rejection test toRepairedand turns it red — the guard is the sole line of defense on that case.Provenance audit — every writer of
contacts.public_keyupdate_contact_public_key←bilateral_ble_handler.rs:2315, :3095contact_sdk.rs:338, :648←resolve_counterparty_via_transport(&qr)export.rs:255— writes an empty key, deferred to BLEapp_router_impl.rsrepair path ←authoritative.public_keycontact_sdk.rs:1247,transactions.rs:576, ~24 othersAlso not fixed here
reissue_tokendoes not re-verify — it re-checks stored pubkey and genesis but never rewrites the Kyber columns, so there is nothing to re-verify today. If it ever does, it needs the same gate.Gates (local, tip
fce2a482)cargo test --locked --workspace --exclude dsm_storage_node,make lint.cargo test -p dsm_sdk— 1843 passed / 0 failed;ci/production_safety_checks.sh— production clippy (-D warnings) + TLA+ both green.make lintlocally —cargo fmt --all -- --checkgreen andcargo clippy --all-targets -- -D warningsgreen (this step lintscfg(test)code, whichproduction_safety_checks.shdoes not).authoritative_ak_permits_repairturnsrepair_rejects_ak_substitution_even_with_a_binding_valid_under_the_pinned_akred (guard is the sole defense on that case), then restored to green.Full CI 12-check matrix runs on
fce2a482.