From f0fe9708093de5f60dbd547d68a4fae2248c3c7e Mon Sep 17 00:00:00 2001 From: Cryptskii <47649969+cryptskii@users.noreply.github.com> Date: Thu, 6 Aug 2026 03:51:05 -0400 Subject: [PATCH 1/4] fix(storage): verify the ML-KEM identity binding before persisting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/api/identity/device_api.rs | 54 ++- .../tests/kyber_binding_enforced.rs | 310 ++++++++++++++++++ 2 files changed, 361 insertions(+), 3 deletions(-) create mode 100644 dsm_storage_node/tests/kyber_binding_enforced.rs diff --git a/dsm_storage_node/src/api/identity/device_api.rs b/dsm_storage_node/src/api/identity/device_api.rs index e1404736..deb8ccff 100644 --- a/dsm_storage_node/src/api/identity/device_api.rs +++ b/dsm_storage_node/src/api/identity/device_api.rs @@ -36,6 +36,10 @@ pub enum RegisterError { InvalidGenesisHash, InvalidKyberKey, InvalidKyberBinding, + /// The binding is well-formed but does not verify against the device's AK. + /// Distinct from `InvalidKyberBinding` (absent/malformed) so a forgery is + /// never reported as a formatting problem. + KyberBindingDoesNotVerify, DeviceAlreadyExists, DeviceNotFound, DatabaseError(String), @@ -53,6 +57,10 @@ impl IntoResponse for RegisterError { StatusCode::BAD_REQUEST, "Invalid or missing kyber_public_key (ML-KEM-768, 1184 bytes required)", ), + RegisterError::KyberBindingDoesNotVerify => ( + StatusCode::BAD_REQUEST, + "kyber_binding_sig does not bind this Kyber key to (device_id, genesis_hash) under the device's AK", + ), RegisterError::InvalidKyberBinding => ( StatusCode::BAD_REQUEST, "Invalid or missing kyber_binding_sig", @@ -94,9 +102,7 @@ pub async fn register_device( return Err(RegisterError::InvalidGenesisHash); } - // Validate Kyber material — MANDATORY (DSM beta has no legacy path). The node - // is a dumb indexer: it enforces length/presence only; the cryptographic - // identity binding is verified client-side against the peer's AK. + // Validate Kyber material — MANDATORY (DSM beta has no legacy path). if req.kyber_public_key.len() != 1184 { return Err(RegisterError::InvalidKyberKey); } @@ -104,6 +110,48 @@ pub async fn register_device( return Err(RegisterError::InvalidKyberBinding); } + // VERIFY BEFORE PERSISTENCE. + // + // This block used to read: "the node is a dumb indexer: it enforces + // length/presence only; the cryptographic identity binding is verified + // client-side against the peer's AK." That client-side verification did not + // exist — `verify_kyber_identity_binding` had ZERO callers anywhere in the + // repository. The node stored a signature nothing ever checked, so an + // ML-KEM key was bound to a device identity purely by assertion, and a + // substituted key would have been served to every peer that fetched it. + // + // 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 at all. + // + // `verify_kyber_identity_binding` re-derives + // `H(domain ‖ device_id ‖ genesis_hash ‖ kyber_pubkey)` and requires + // `Ok(true)` from `sphincs_verify` — an `Err` and an `Ok(false)` are both + // refusals, and neither reaches the database. + let device_id_arr: [u8; 32] = req + .device_id + .as_slice() + .try_into() + .map_err(|_| RegisterError::InvalidDeviceId)?; + let genesis_arr: [u8; 32] = req + .genesis_hash + .as_slice() + .try_into() + .map_err(|_| RegisterError::InvalidGenesisHash)?; + if let Err(e) = dsm_sdk::sdk::kyber_identity::verify_kyber_identity_binding( + &device_id_arr, + &genesis_arr, + &req.kyber_public_key, + &req.kyber_binding_sig, + &req.pubkey, + ) { + log::warn!( + "device registration refused: kyber identity binding failed to verify for {}: {e}", + text_id::encode_base32_crockford(&req.device_id) + ); + return Err(RegisterError::KyberBindingDoesNotVerify); + } + // Convert to Base32 for DB storage (DB uses string device_id) let device_id_b32 = text_id::encode_base32_crockford(&req.device_id); diff --git a/dsm_storage_node/tests/kyber_binding_enforced.rs b/dsm_storage_node/tests/kyber_binding_enforced.rs new file mode 100644 index 00000000..abde122b --- /dev/null +++ b/dsm_storage_node/tests/kyber_binding_enforced.rs @@ -0,0 +1,310 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 + +//! The node must VERIFY the ML-KEM identity binding before persisting it. +//! +//! Until this branch it did not. `register_device` checked only length and +//! presence, under a comment asserting that "the cryptographic identity binding +//! is verified client-side against the peer's AK". +//! +//! That client-side verification is PARTIAL, not absent. `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, deliberately before repair +//! overwrites `public_key`. But that is ONE repair path, not the general fetch +//! path, so the node was still persisting bindings that nothing had checked at +//! the time of writing. A device could bind any ML-KEM key to its identity by +//! assertion and the node would store it and serve it. +//! +//! These tests drive the REAL handler against a real (in-memory) database and +//! then read the persisted rows back. They deliberately do not test the +//! standalone verifier: the verifier was already correct, and testing it was +//! exactly the thing that failed to notice nothing called it. +//! +//! No skip path. The pre-existing `device_api::tests` return early unless +//! `DSM_RUN_DB_TESTS=1`, so they are vacuous in CI and could not have caught +//! this. + +#![cfg(feature = "local-dev")] +#![allow(clippy::disallowed_methods)] + +use axum::body::Bytes; +use axum::Extension; +use dsm::crypto::signatures::SignatureKeyPair; +use dsm::types::proto as pb; +use dsm_sdk::util::text_id; +use dsm_storage_node::{ + api::identity::device_api::register_device, + db, + replication::{ReplicationConfig, ReplicationManager}, + AppState, +}; +use prost::Message; +use std::sync::Arc; + +async fn make_state() -> AppState { + let pool = db::create_pool(":memory:", true).expect("create_pool"); + db::init_db(&pool).await.expect("init_db"); + let replication_config = ReplicationConfig { + replication_factor: 3, + gossip_interval_ticks: 100, + failure_timeout_ticks: 300, + gossip_fanout: 3, + max_concurrent_jobs: 10, + }; + let replication_manager = Arc::new( + ReplicationManager::new_for_tests( + replication_config, + "test-node".to_string(), + "http://localhost:8080".to_string(), + ) + .expect("ReplicationManager::new_for_tests"), + ); + AppState::new( + "test-node".to_string(), + "http://localhost:8080", + None, + Arc::new(pool), + replication_manager, + ) +} + +const KYBER_PK_LEN: usize = 1184; + +/// The canonical binding digest, rebuilt here independently of the SDK so this +/// suite does not simply echo the implementation it is gating. +fn canonical_binding_digest(device_id: &[u8; 32], genesis: &[u8; 32], kyber_pk: &[u8]) -> [u8; 32] { + let mut h = blake3::Hasher::new(); + h.update(b"DSM/kyber-identity-binding"); + h.update(&[0u8]); // the encoder's one delimiter + h.update(device_id); + h.update(genesis); + h.update(kyber_pk); + *h.finalize().as_bytes() +} + +struct Device { + id: [u8; 32], + genesis: [u8; 32], + kyber_pk: Vec, + ak: SignatureKeyPair, +} + +fn device(seed: u8) -> Device { + Device { + id: [seed; 32], + genesis: [seed.wrapping_add(1); 32], + kyber_pk: vec![seed.wrapping_add(2); KYBER_PK_LEN], + ak: SignatureKeyPair::generate_from_entropy( + format!("DSM/test/kyber-enforce/{seed}").as_bytes(), + ) + .expect("AK"), + } +} + +fn request(d: &Device, kyber_pk: &[u8], sig: Vec) -> Bytes { + let req = pb::RegisterDeviceRequest { + device_id: d.id.to_vec(), + pubkey: d.ak.public_key().to_vec(), + genesis_hash: d.genesis.to_vec(), + kyber_public_key: kyber_pk.to_vec(), + kyber_binding_sig: sig, + }; + let mut buf = Vec::new(); + req.encode(&mut buf).expect("encode"); + Bytes::from(buf) +} + +fn valid_sig(d: &Device) -> Vec { + let digest = canonical_binding_digest(&d.id, &d.genesis, &d.kyber_pk); + d.ak.sign(&digest).expect("sign binding") +} + +async fn stored(state: &AppState, d: &Device) -> Option<(Vec, Vec, Vec, Vec)> { + db::get_device(&state.db_pool, &text_id::encode_base32_crockford(&d.id)) + .await + .expect("get_device") +} + +/// ANTI-VACUITY, and the acceptance half of the gate: a correctly generated +/// canonical binding is accepted AND persisted. Without this, a handler that +/// refused everything would pass every rejection test below. +#[tokio::test] +async fn a_canonical_binding_is_accepted_and_persisted() { + let state = make_state().await; + let d = device(0x10); + + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, valid_sig(&d)), + ) + .await; + assert!(res.is_ok(), "a valid canonical binding must register"); + + let row = stored(&state, &d).await.expect("device row must exist"); + assert_eq!( + row.2, d.kyber_pk, + "the Kyber key must be persisted verbatim" + ); + assert!(!row.3.is_empty(), "the binding signature must be persisted"); +} + +/// A FORGERY: a well-formed signature by the right key over the wrong message. +/// `sphincs_verify` answers `Ok(false)` here, not `Err` — so a handler that +/// tested "no error" rather than `Ok(true)` would accept it. +#[tokio::test] +async fn a_forged_binding_is_refused_and_nothing_is_persisted() { + let state = make_state().await; + let d = device(0x20); + + let forged = d.ak.sign(b"a different message entirely").expect("sign"); + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, forged), + ) + .await; + + assert!(res.is_err(), "a forged binding must be refused"); + assert!( + stored(&state, &d).await.is_none(), + "a refused registration left a row behind — verification must happen \ + BEFORE persistence, not alongside it" + ); +} + +/// KEY SUBSTITUTION — the attack the binding exists to stop. A valid signature +/// over key A is replayed with key B under the same device identity. +#[tokio::test] +async fn a_substituted_kyber_key_is_refused_and_nothing_is_persisted() { + let state = make_state().await; + let d = device(0x30); + + // Signature is over d.kyber_pk; the request carries a different key. + let other_pk = vec![0xEEu8; KYBER_PK_LEN]; + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &other_pk, valid_sig(&d)), + ) + .await; + + assert!(res.is_err(), "a substituted Kyber key must be refused"); + assert!( + stored(&state, &d).await.is_none(), + "a substituted key was persisted — the node would serve it to peers" + ); +} + +/// OLD-DOMAIN artifact: a binding signed under the pre-cut double-NUL digest +/// (impact-table row B4). It must fail, with no compatibility path. +#[tokio::test] +async fn an_old_domain_binding_is_refused() { + let state = make_state().await; + let d = device(0x40); + + let mut old = blake3::Hasher::new(); + old.update(b"DSM/kyber-identity-binding\0"); // literal carried its own NUL + old.update(&[0u8]); // and the helper appended another + old.update(&d.id); + old.update(&d.genesis); + old.update(&d.kyber_pk); + let old_digest = *old.finalize().as_bytes(); + let stale = d.ak.sign(&old_digest).expect("sign old-domain"); + + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, stale), + ) + .await; + + assert!( + res.is_err(), + "a binding signed under the pre-cut domain still registers — there is a \ + compatibility verifier that must not exist" + ); + assert!(stored(&state, &d).await.is_none()); +} + +/// MALFORMED: a signature of the wrong length. `sphincs::verify` fails closed on +/// a length mismatch rather than erroring, so this also exercises the +/// `Ok(false)` path rather than the `Err` path. +#[tokio::test] +async fn a_malformed_binding_is_refused() { + let state = make_state().await; + let d = device(0x50); + + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, vec![0u8; 7]), + ) + .await; + + assert!( + res.is_err(), + "a truncated binding signature must be refused" + ); + assert!(stored(&state, &d).await.is_none()); +} + +/// A binding signed by a DIFFERENT AK than the one presented as `pubkey`. +#[tokio::test] +async fn a_binding_signed_by_another_key_is_refused() { + let state = make_state().await; + let d = device(0x60); + let impostor = device(0x61); + + let digest = canonical_binding_digest(&d.id, &d.genesis, &d.kyber_pk); + let wrong_signer = impostor.ak.sign(&digest).expect("sign"); + + let res = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, wrong_signer), + ) + .await; + + assert!( + res.is_err(), + "a binding signed by another AK must be refused" + ); + assert!(stored(&state, &d).await.is_none()); +} + +/// RELOAD does not bypass verification. A row that was accepted stays readable +/// across a fresh read, and — critically — a rejected registration cannot be +/// "completed" by retrying without a valid binding. +#[tokio::test] +async fn a_rejected_registration_cannot_be_completed_by_retrying() { + let state = make_state().await; + let d = device(0x70); + + // Rejected. + let bad = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, vec![0xABu8; 64]), + ) + .await; + assert!(bad.is_err()); + assert!(stored(&state, &d).await.is_none()); + + // Retrying with the same invalid binding is still refused — no partial row + // from the first attempt makes the second one succeed. + let again = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, vec![0xABu8; 64]), + ) + .await; + assert!(again.is_err()); + assert!(stored(&state, &d).await.is_none()); + + // A correct binding then registers cleanly, and reads back. + let good = register_device( + Extension(Arc::new(state.clone())), + request(&d, &d.kyber_pk, valid_sig(&d)), + ) + .await; + assert!( + good.is_ok(), + "a valid binding must register after rejections" + ); + + let row = stored(&state, &d).await.expect("row after success"); + assert_eq!(row.2, d.kyber_pk); +} From 392c3aedbdd395ddb721748a4d25c185c9a229a0 Mon Sep 17 00:00:00 2001 From: Cryptskii <47649969+cryptskii@users.noreply.github.com> Date: Fri, 7 Aug 2026 01:25:31 -0400 Subject: [PATCH 2/4] fix(sdk): node-derived contact repair may not re-root the pairing-established AK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../dsm_sdk/src/handlers/app_router_impl.rs | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs index 7fe6070c..c6c52325 100644 --- a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs +++ b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs @@ -312,17 +312,28 @@ impl AppRouterImpl { mut contact_record: crate::storage::client_db::ContactRecord, authoritative: &QuorumDeviceIdentity, ) -> Result { - // The AK established in-person via the QR (the contact's signing key) is - // what verifies the recipient's registry-carried ML-KEM identity binding. - // Capture it before any repair overwrites `public_key`. + // The AK established in-person via the QR/BLE pairing (the contact's signing key) is the + // trust root: it verifies the recipient's registry-carried ML-KEM identity binding, and it + // is NEVER overwritten by a node/quorum-served value. Capture it before any repair. let contact_ak = contact_record.public_key.clone(); - let public_key_matches = authoritative.public_key.is_empty() - || contact_record.public_key == authoritative.public_key; + // Trust-root invariant: a non-empty authoritative (node/quorum) AK that DIFFERS from the + // pinned contact AK is a substitution attempt. Reject and leave the pinned contact untouched + // — node-derived repair may refresh genesis/Kyber material, but MAY NOT re-root the AK. A + // genuinely rotated AK is a new identity that must be re-established in person (no implicit + // TOFU). An empty authoritative AK carries no AK claim to check (the Kyber binding below is + // still verified against the pinned `contact_ak`, so forged material fails closed regardless). + if !authoritative_ak_permits_repair(&contact_ak, &authoritative.public_key) { + return Err( + "authoritative device AK differs from the pairing-established contact AK — \ + rejecting (possible substitution/equivocation)" + .to_string(), + ); + } + let kyber_matches = !authoritative.kyber_public_key.is_empty() && contact_record.kyber_public_key == authoritative.kyber_public_key; if contact_record.genesis_hash.as_slice() == authoritative.genesis_hash.as_slice() - && public_key_matches && kyber_matches { return Ok(contact_record); @@ -340,9 +351,8 @@ impl AppRouterImpl { ); contact_record.genesis_hash = authoritative.genesis_hash.to_vec(); - if !authoritative.public_key.is_empty() { - contact_record.public_key = authoritative.public_key.clone(); - } + // The AK (`public_key`) is the pairing-established trust root and is NEVER overwritten from a + // node/quorum value — the guard above already required any authoritative AK to equal it. // MANDATORY recipient ML-KEM identity binding (DSM beta, no legacy path): // the quorum-agreed Kyber key must be bound to (device_id, genesis) under @@ -2786,6 +2796,42 @@ fn select_quorum_device_identity( None } +/// Whether a node/quorum-served authoritative AK may participate in a contact-identity repair. +/// The pairing-established contact AK is the trust root: a non-empty authoritative AK that DIFFERS +/// from it is a substitution attempt and is rejected; an empty or matching one carries no +/// re-rooting claim. Either way the AK itself is NEVER overwritten (the Kyber binding is verified +/// against the pinned contact AK), so a permitted repair can only refresh genesis/Kyber material. +fn authoritative_ak_permits_repair(contact_ak: &[u8], authoritative_ak: &[u8]) -> bool { + authoritative_ak.is_empty() || authoritative_ak == contact_ak +} + +#[cfg(test)] +mod ak_trust_root_tests { + use super::authoritative_ak_permits_repair; + + /// Read-side trust-root invariant: node-derived repair may never re-root the AK. A node serving a + /// DIFFERENT AK than the QR/BLE-pinned one is rejected (substitution); an absent or matching node + /// AK is permitted — and even then the AK is never overwritten by the repair. + #[test] + fn node_substituted_ak_is_rejected_matching_or_absent_is_permitted() { + let contact_ak = vec![0xA1u8; 64]; // pairing-established trust root + assert!( + authoritative_ak_permits_repair(&contact_ak, &[]), + "an absent node AK carries no claim → permitted (Kyber still verified against contact_ak)" + ); + assert!( + authoritative_ak_permits_repair(&contact_ak, &contact_ak), + "a node AK equal to the pinned AK is permitted" + ); + assert!( + !authoritative_ak_permits_repair(&contact_ak, &vec![0xE5u8; 64]), + "a node AK differing from the pinned AK is a substitution → rejected" + ); + // A shorter/off-by-one AK must not accidentally match. + assert!(!authoritative_ak_permits_repair(&contact_ak, &vec![0xA1u8; 63])); + } +} + pub(crate) async fn fetch_quorum_device_identity( storage_endpoints: &[String], device_id: [u8; 32], From 018f0dd358eaf8c7705448745b85afe350ea2913 Mon Sep 17 00:00:00 2001 From: Cryptskii <47649969+cryptskii@users.noreply.github.com> Date: Fri, 7 Aug 2026 01:58:06 -0400 Subject: [PATCH 3/4] test(sdk): drive the AK trust-root guard through the real repair path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../dsm_sdk/src/handlers/app_router_impl.rs | 306 +++++++++++++----- .../dsm_sdk/src/sdk/kyber_identity.rs | 6 +- 2 files changed, 234 insertions(+), 78 deletions(-) diff --git a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs index c6c52325..a36cc73d 100644 --- a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs +++ b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs @@ -309,88 +309,43 @@ struct OnlineSendArtifacts { impl AppRouterImpl { async fn repair_contact_identity_from_quorum( &self, - mut contact_record: crate::storage::client_db::ContactRecord, + contact_record: crate::storage::client_db::ContactRecord, authoritative: &QuorumDeviceIdentity, ) -> Result { - // The AK established in-person via the QR/BLE pairing (the contact's signing key) is the - // trust root: it verifies the recipient's registry-carried ML-KEM identity binding, and it - // is NEVER overwritten by a node/quorum-served value. Capture it before any repair. - let contact_ak = contact_record.public_key.clone(); - - // Trust-root invariant: a non-empty authoritative (node/quorum) AK that DIFFERS from the - // pinned contact AK is a substitution attempt. Reject and leave the pinned contact untouched - // — node-derived repair may refresh genesis/Kyber material, but MAY NOT re-root the AK. A - // genuinely rotated AK is a new identity that must be re-established in person (no implicit - // TOFU). An empty authoritative AK carries no AK claim to check (the Kyber binding below is - // still verified against the pinned `contact_ak`, so forged material fails closed regardless). - if !authoritative_ak_permits_repair(&contact_ak, &authoritative.public_key) { - return Err( - "authoritative device AK differs from the pairing-established contact AK — \ - rejecting (possible substitution/equivocation)" - .to_string(), - ); - } - - let kyber_matches = !authoritative.kyber_public_key.is_empty() - && contact_record.kyber_public_key == authoritative.kyber_public_key; - if contact_record.genesis_hash.as_slice() == authoritative.genesis_hash.as_slice() - && kyber_matches - { - return Ok(contact_record); - } - - log::warn!( - "[wallet.send] repairing stale contact identity alias={} genesis_old={} genesis_new={}", - contact_record.alias, - crate::util::text_id::encode_base32_crockford(&contact_record.genesis_hash) - .get(..8) - .unwrap_or("?"), - crate::util::text_id::encode_base32_crockford(&authoritative.genesis_hash) - .get(..8) - .unwrap_or("?"), - ); - - contact_record.genesis_hash = authoritative.genesis_hash.to_vec(); - // The AK (`public_key`) is the pairing-established trust root and is NEVER overwritten from a - // node/quorum value — the guard above already required any authoritative AK to equal it. - - // MANDATORY recipient ML-KEM identity binding (DSM beta, no legacy path): - // the quorum-agreed Kyber key must be bound to (device_id, genesis) under - // the QR-established AK before we persist it to the contact. Fail-closed on - // missing or unbound (substituted/equivocating) material — an online - // per-step-EK send has no legacy fallback. - if authoritative.kyber_public_key.is_empty() { - return Err("recipient identity quorum returned no Kyber public key".to_string()); - } - let recipient_device_id: [u8; 32] = contact_record - .device_id - .as_slice() - .try_into() - .map_err(|_| "contact device_id is not 32 bytes".to_string())?; - crate::sdk::kyber_identity::verify_kyber_identity_binding( - &recipient_device_id, - &authoritative.genesis_hash, - &authoritative.kyber_public_key, - &authoritative.kyber_binding_sig, - &contact_ak, - ) - .map_err(|e| format!("recipient Kyber identity binding invalid: {e}"))?; - contact_record.kyber_public_key = authoritative.kyber_public_key.clone(); + let alias = contact_record.alias.clone(); + let old_genesis = contact_record.genesis_hash.clone(); + + // The trust decision — AK guard, ML-KEM binding verification, and record synthesis — is a + // pure, I/O-free function. `store_contact` below is UNREACHABLE unless it returns + // `Repaired`, so a future refactor cannot slip a persist ahead of the guard/verify: the only + // way to reach persistence is for the decision to have already accepted the material. + match repair_contact_decision(contact_record, authoritative)? { + ContactRepair::Unchanged(record) => Ok(record), + ContactRepair::Repaired(record) => { + log::warn!( + "[wallet.send] repairing stale contact identity alias={} genesis_old={} genesis_new={}", + alias, + crate::util::text_id::encode_base32_crockford(&old_genesis) + .get(..8) + .unwrap_or("?"), + crate::util::text_id::encode_base32_crockford(&record.genesis_hash) + .get(..8) + .unwrap_or("?"), + ); - contact_record.verified = true; - contact_record.needs_online_reconcile = false; + crate::storage::client_db::store_contact(&record) + .map_err(|e| format!("failed to persist repaired contact identity: {e}"))?; - crate::storage::client_db::store_contact(&contact_record) - .map_err(|e| format!("failed to persist repaired contact identity: {e}"))?; + if let Some(verified) = record.to_verified_contact() { + let mut cm = self.contact_manager.clone(); + cm.restore_contact_from_storage(verified) + .await + .map_err(|e| format!("failed to refresh in-memory contact identity: {e}"))?; + } - if let Some(verified) = contact_record.to_verified_contact() { - let mut cm = self.contact_manager.clone(); - cm.restore_contact_from_storage(verified) - .await - .map_err(|e| format!("failed to refresh in-memory contact identity: {e}"))?; + Ok(record) + } } - - Ok(contact_record) } pub fn new(config: SdkConfig) -> Result { @@ -2805,9 +2760,90 @@ fn authoritative_ak_permits_repair(contact_ak: &[u8], authoritative_ak: &[u8]) - authoritative_ak.is_empty() || authoritative_ak == contact_ak } +/// Outcome of the pure contact-repair trust decision. +enum ContactRepair { + /// The pinned contact already matches the authoritative identity — nothing to persist. + Unchanged(crate::storage::client_db::ContactRecord), + /// Genesis/Kyber material was refreshed under the pinned AK — the caller must persist this. + Repaired(crate::storage::client_db::ContactRecord), +} + +/// Reconcile a node/quorum-served identity against a pinned contact, WITHOUT any I/O. +/// +/// The pairing-established AK (`contact_record.public_key`) is the trust root: it verifies the +/// recipient's registry-carried ML-KEM identity binding and is NEVER overwritten by a +/// node/quorum-served value. A non-empty authoritative AK that differs from it is a substitution +/// attempt and is rejected here (no implicit TOFU) — the caller's persistence step is unreachable on +/// this path, so a rejected repair cannot touch the stored contact. A permitted repair may refresh +/// only genesis/Kyber material, and only after the ML-KEM binding verifies under the pinned AK. +fn repair_contact_decision( + mut contact_record: crate::storage::client_db::ContactRecord, + authoritative: &QuorumDeviceIdentity, +) -> Result { + // Capture the pinned AK before touching anything; it is the binding-verification trust root. + let contact_ak = contact_record.public_key.clone(); + + // Trust-root invariant: a non-empty authoritative (node/quorum) AK that DIFFERS from the pinned + // contact AK is a substitution attempt. Reject and leave the pinned contact untouched — a + // genuinely rotated AK is a new identity that must be re-established in person. An empty + // authoritative AK carries no AK claim (the Kyber binding is still verified against the pinned + // `contact_ak`, so forged material fails closed regardless). + if !authoritative_ak_permits_repair(&contact_ak, &authoritative.public_key) { + return Err( + "authoritative device AK differs from the pairing-established contact AK — \ + rejecting (possible substitution/equivocation)" + .to_string(), + ); + } + + let kyber_matches = !authoritative.kyber_public_key.is_empty() + && contact_record.kyber_public_key == authoritative.kyber_public_key; + if contact_record.genesis_hash.as_slice() == authoritative.genesis_hash.as_slice() + && kyber_matches + { + return Ok(ContactRepair::Unchanged(contact_record)); + } + + contact_record.genesis_hash = authoritative.genesis_hash.to_vec(); + // The AK (`public_key`) is the pairing-established trust root and is NEVER overwritten from a + // node/quorum value — the guard above already required any authoritative AK to equal it. + + // MANDATORY recipient ML-KEM identity binding (DSM beta, no legacy path): the quorum-agreed + // Kyber key must be bound to (device_id, genesis) under the QR-established AK before it can be + // persisted. Fail-closed on missing or unbound (substituted/equivocating) material. + if authoritative.kyber_public_key.is_empty() { + return Err("recipient identity quorum returned no Kyber public key".to_string()); + } + let recipient_device_id: [u8; 32] = contact_record + .device_id + .as_slice() + .try_into() + .map_err(|_| "contact device_id is not 32 bytes".to_string())?; + crate::sdk::kyber_identity::verify_kyber_identity_binding( + &recipient_device_id, + &authoritative.genesis_hash, + &authoritative.kyber_public_key, + &authoritative.kyber_binding_sig, + &contact_ak, + ) + .map_err(|e| format!("recipient Kyber identity binding invalid: {e}"))?; + contact_record.kyber_public_key = authoritative.kyber_public_key.clone(); + + contact_record.verified = true; + contact_record.needs_online_reconcile = false; + + Ok(ContactRepair::Repaired(contact_record)) +} + #[cfg(test)] mod ak_trust_root_tests { - use super::authoritative_ak_permits_repair; + use super::{ + authoritative_ak_permits_repair, repair_contact_decision, ContactRepair, + QuorumDeviceIdentity, + }; + use crate::sdk::kyber_identity::binding_digest; + use crate::storage::client_db::ContactRecord; + use dsm::crypto::{kyber, sphincs}; /// Read-side trust-root invariant: node-derived repair may never re-root the AK. A node serving a /// DIFFERENT AK than the QR/BLE-pinned one is rejected (substitution); an absent or matching node @@ -2830,6 +2866,122 @@ mod ak_trust_root_tests { // A shorter/off-by-one AK must not accidentally match. assert!(!authoritative_ak_permits_repair(&contact_ak, &vec![0xA1u8; 63])); } + + /// Minimal contact row pinned to `ak` (the pairing-established AK) with the given genesis/Kyber. + fn pinned_contact( + device_id: [u8; 32], + ak: &[u8], + genesis: [u8; 32], + kyber_public_key: Vec, + ) -> ContactRecord { + ContactRecord { + contact_id: "peer".to_string(), + device_id: device_id.to_vec(), + alias: "peer".to_string(), + genesis_hash: genesis.to_vec(), + public_key: ak.to_vec(), + kyber_public_key, + current_chain_tip: None, + added_at: 0, + verified: true, + verification_proof: None, + metadata: std::collections::HashMap::new(), + ble_address: None, + status: "active".to_string(), + needs_online_reconcile: false, + last_seen_online_counter: 0, + last_seen_ble_counter: 0, + previous_chain_tip: None, + } + } + + /// Real repair-path proof (not just the pure guard helper): a node that serves a Kyber binding + /// that is genuinely VALID under the pinned AK, but ALSO substitutes a different device AK, is + /// rejected by `repair_contact_decision` — it never yields a `Repaired` record, so the caller's + /// `store_contact` is never reached and the pinned contact row is left byte-for-byte unchanged. + /// + /// The binding is valid on purpose: it isolates the AK guard. A malformed binding would be + /// caught by the Kyber verify regardless, proving nothing about the guard. This case passes the + /// Kyber verify (binding signed by the pinned AK) so ONLY the guard can reject it — which is why + /// disabling `authoritative_ak_permits_repair` flips this to `Repaired` and turns the test red. + #[test] + fn repair_rejects_ak_substitution_even_with_a_binding_valid_under_the_pinned_ak() { + let device_id = [0x11u8; 32]; + let (pinned_ak, pinned_sk) = sphincs::generate_sphincs_keypair().expect("keypair"); + let genesis = [0x22u8; 32]; + let kyber_public_key = vec![0x9Au8; kyber::public_key_bytes()]; + + // Binding genuinely valid under the PINNED AK. + let digest = binding_digest(&device_id, &genesis, &kyber_public_key); + let binding_sig = sphincs::sphincs_sign(&pinned_sk, &digest).expect("sign"); + + let contact = pinned_contact(device_id, &pinned_ak, genesis, Vec::new()); + + // The node substitutes a DIFFERENT AK (equivocation) while carrying the valid binding. + let substituted_ak = vec![0xE5u8; pinned_ak.len()]; + assert_ne!(substituted_ak, pinned_ak); + let authoritative = QuorumDeviceIdentity { + device_id, + genesis_hash: genesis, + public_key: substituted_ak, + kyber_public_key, + kyber_binding_sig: binding_sig, + }; + + let outcome = repair_contact_decision(contact, &authoritative); + assert!( + matches!(outcome, Err(_)), + "AK substitution must be rejected even when the Kyber binding is valid under the pinned AK; \ + got a non-error outcome (guard bypassed → store_contact would run)" + ); + } + + /// Real repair-path proof of the permitted case: a node AK that MATCHES the pinned AK, carrying a + /// valid canonical binding for a refreshed genesis/Kyber key, yields `Repaired` with the genesis + /// and Kyber fields updated — while `public_key` remains EXACTLY the pinned AK (never re-rooted). + #[test] + fn repair_refreshes_genesis_and_kyber_but_preserves_the_pinned_ak() { + let device_id = [0x44u8; 32]; + let (pinned_ak, pinned_sk) = sphincs::generate_sphincs_keypair().expect("keypair"); + let old_genesis = [0x55u8; 32]; + let new_genesis = [0x66u8; 32]; + let new_kyber = vec![0x7Bu8; kyber::public_key_bytes()]; + + // Canonical binding for the refreshed (genesis, Kyber) under the pinned AK. + let digest = binding_digest(&device_id, &new_genesis, &new_kyber); + let binding_sig = sphincs::sphincs_sign(&pinned_sk, &digest).expect("sign"); + + // Stale contact: old genesis, no Kyber yet. + let contact = pinned_contact(device_id, &pinned_ak, old_genesis, Vec::new()); + + let authoritative = QuorumDeviceIdentity { + device_id, + genesis_hash: new_genesis, + public_key: pinned_ak.clone(), // node AK matches the pinned trust root + kyber_public_key: new_kyber.clone(), + kyber_binding_sig: binding_sig, + }; + + match repair_contact_decision(contact, &authoritative) { + Ok(ContactRepair::Repaired(record)) => { + assert_eq!(record.genesis_hash, new_genesis.to_vec(), "genesis refreshed"); + assert_eq!(record.kyber_public_key, new_kyber, "Kyber key refreshed"); + assert_eq!( + record.public_key, pinned_ak, + "the pairing-established AK MUST be preserved exactly (never re-rooted)" + ); + assert!(record.verified, "a verified repair marks the contact verified"); + } + other => panic!( + "expected Repaired with the pinned AK preserved; got {}", + match other { + Ok(ContactRepair::Unchanged(_)) => "Unchanged", + Ok(ContactRepair::Repaired(_)) => unreachable!(), + Err(_) => "Err", + } + ), + } + } } pub(crate) async fn fetch_quorum_device_identity( diff --git a/dsm_client/deterministic_state_machine/dsm_sdk/src/sdk/kyber_identity.rs b/dsm_client/deterministic_state_machine/dsm_sdk/src/sdk/kyber_identity.rs index 3b22b91a..c1a9c5f5 100644 --- a/dsm_client/deterministic_state_machine/dsm_sdk/src/sdk/kyber_identity.rs +++ b/dsm_client/deterministic_state_machine/dsm_sdk/src/sdk/kyber_identity.rs @@ -32,7 +32,11 @@ pub const KYBER_IDENTITY_BINDING_TAG: dsm::crypto::domain::TaggedHashDomain<'sta /// Canonical binding digest over `device_id || genesis_hash || kyber_pubkey`, /// domain-separated by [`KYBER_IDENTITY_BINDING_TAG`]. This is the message the /// device AK signs and a verifier re-derives. -fn binding_digest(device_id: &[u8; 32], genesis_hash: &[u8; 32], kyber_pubkey: &[u8]) -> [u8; 32] { +pub(crate) fn binding_digest( + device_id: &[u8; 32], + genesis_hash: &[u8; 32], + kyber_pubkey: &[u8], +) -> [u8; 32] { let mut preimage = Vec::with_capacity(64 + kyber_pubkey.len()); preimage.extend_from_slice(device_id); preimage.extend_from_slice(genesis_hash); From fce2a482f7ced0a43ca713b9a09968c871f5615a Mon Sep 17 00:00:00 2001 From: Cryptskii <47649969+cryptskii@users.noreply.github.com> Date: Fri, 7 Aug 2026 02:18:22 -0400 Subject: [PATCH 4/4] style(sdk): satisfy `make lint` (rustfmt + clippy --all-targets) on the trust-root tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../dsm_sdk/src/handlers/app_router_impl.rs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs index a36cc73d..e3fee114 100644 --- a/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs +++ b/dsm_client/deterministic_state_machine/dsm_sdk/src/handlers/app_router_impl.rs @@ -340,7 +340,9 @@ impl AppRouterImpl { let mut cm = self.contact_manager.clone(); cm.restore_contact_from_storage(verified) .await - .map_err(|e| format!("failed to refresh in-memory contact identity: {e}"))?; + .map_err(|e| { + format!("failed to refresh in-memory contact identity: {e}") + })?; } Ok(record) @@ -2860,11 +2862,11 @@ mod ak_trust_root_tests { "a node AK equal to the pinned AK is permitted" ); assert!( - !authoritative_ak_permits_repair(&contact_ak, &vec![0xE5u8; 64]), + !authoritative_ak_permits_repair(&contact_ak, &[0xE5u8; 64]), "a node AK differing from the pinned AK is a substitution → rejected" ); // A shorter/off-by-one AK must not accidentally match. - assert!(!authoritative_ak_permits_repair(&contact_ak, &vec![0xA1u8; 63])); + assert!(!authoritative_ak_permits_repair(&contact_ak, &[0xA1u8; 63])); } /// Minimal contact row pinned to `ak` (the pairing-established AK) with the given genesis/Kyber. @@ -2930,7 +2932,7 @@ mod ak_trust_root_tests { let outcome = repair_contact_decision(contact, &authoritative); assert!( - matches!(outcome, Err(_)), + outcome.is_err(), "AK substitution must be rejected even when the Kyber binding is valid under the pinned AK; \ got a non-error outcome (guard bypassed → store_contact would run)" ); @@ -2964,13 +2966,20 @@ mod ak_trust_root_tests { match repair_contact_decision(contact, &authoritative) { Ok(ContactRepair::Repaired(record)) => { - assert_eq!(record.genesis_hash, new_genesis.to_vec(), "genesis refreshed"); + assert_eq!( + record.genesis_hash, + new_genesis.to_vec(), + "genesis refreshed" + ); assert_eq!(record.kyber_public_key, new_kyber, "Kyber key refreshed"); assert_eq!( record.public_key, pinned_ak, "the pairing-established AK MUST be preserved exactly (never re-rooted)" ); - assert!(record.verified, "a verified repair marks the contact verified"); + assert!( + record.verified, + "a verified repair marks the contact verified" + ); } other => panic!( "expected Repaired with the pinned AK preserved; got {}",