From a3578ff2f3fbb6b5cf7b1d47f1fdbb13da5794d9 Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 4 Aug 2026 11:11:36 +0900 Subject: [PATCH 1/3] fix(replication): scope round-2 audit openings to the proved subtree Round 2 of the storage-commitment audit authorised an opening if its key was anywhere in the pinned commitment, rather than in the subtree that round 1 actually proved. Round 1 proves one subtree of roughly the square root of the commitment's key count, so a caller could complete a cheap round 1 over small records and then request openings against large records elsewhere in the same commitment. The work round 2 costs was therefore not bounded by the work the caller had already paid for in round 1, and the per-peer cooldown that would otherwise limit repetition is escapable by presenting a different peer identity. Openings are now checked against the round-1 subtree, and a challenge naming any key outside it is refused whole before a single chunk is read. This matches how an over-broad challenge is already handled a few lines earlier. The authorised set is recomputed from the pinned tree and the audit nonce rather than stored in the round-1 session. The subtree is a pure function of those two, and both are already pinned, so recomputing costs one tree walk with no chunk reads. Storing the key list instead would have cost up to a full subtree of keys for every live session, which is the wrong trade when sessions are bounded by count rather than by size. An honest auditor cannot trip the new check: it samples only the leaves round 1 returned, and without that leaf's committed block-tree root and content length it has nothing to verify an answer against, so an opening outside the subtree could tell it nothing even if it were served. A failure to rebuild the subtree is reported as transient rather than as a protocol error, because the tree belongs to this node and round 1 already walked it successfully, so an inconsistency there is local and should not brand the node with a confirmed audit failure. The accompanying test drives one live round 1 and then asserts both halves from its result, so they cannot drift apart: a leaf inside the proved subtree is still served, and a key that is genuinely committed and whose bytes are present on disk is refused solely for being outside the subtree. Verified to fail without the change, where the responder serves a complete block opening for a key round 1 never proved. --- src/replication/storage_commitment_audit.rs | 64 +++++++++--- tests/poc_audit_handler_live.rs | 107 ++++++++++++++++++++ 2 files changed, 159 insertions(+), 12 deletions(-) diff --git a/src/replication/storage_commitment_audit.rs b/src/replication/storage_commitment_audit.rs index 745a49db..481272a0 100644 --- a/src/replication/storage_commitment_audit.rs +++ b/src/replication/storage_commitment_audit.rs @@ -1538,10 +1538,13 @@ fn build_slice_items_for_key( /// produce the bytes it returns [`SubtreeSliceItem::Absent`], which the auditor /// counts as a provable failure. /// -/// A key the responder never committed to (not in the pinned tree) is also -/// returned `Absent`: the auditor only ever samples keys it saw in round 1, so -/// in practice this guards against a malformed/forged challenge rather than an -/// honest mismatch. +/// Openings are authorised against the subtree round 1 actually proved, not +/// merely against the pinned commitment. A challenge naming any key outside that +/// subtree is refused whole, before any chunk is read, so the work this round can +/// cost is bounded by the work the caller already paid for in round 1. An honest +/// auditor cannot trip this: it samples only the leaves round 1 returned, and +/// without that leaf's `nonced_root` and `content_len` it has nothing to verify +/// an answer against. pub async fn handle_subtree_slice_challenge( challenge: &SubtreeSliceChallenge, storage: &LmdbStorage, @@ -1631,17 +1634,54 @@ pub async fn handle_subtree_slice_challenge( }; } + // Round 2 may open ONLY the leaves round 1 actually proved, not the whole + // pinned commitment. Membership of the pinned tree is the weaker property: + // it would let a cheap round 1 over small records authorise openings against + // large records elsewhere in the commitment, so the work this round costs + // would not be bounded by the work the caller paid for in round 1. + // + // The authorised set is recomputed from `(tree, nonce)` rather than carried + // in the round-1 session. The subtree is a pure function of those two, both + // of which are already pinned — the nonce was matched against the session + // before this handler ran — so recomputing costs one tree walk with no chunk + // reads, while storing the key list would cost up to a full subtree of keys + // per live session. + let plan = match subtree_plan(built.tree(), &challenge.nonce) { + Ok(plan) => plan, + // The tree is ours and round 1 already walked it, so a failure here is + // local inconsistency rather than anything the caller did. Transient + // routes the auditor to the graced timeout lane instead of branding this + // node with a confirmed failure it did not earn. + Err(e) => { + warn!("Subtree slice audit: cannot rebuild the round-1 subtree plan: {e:?}"); + return SubtreeSliceResponse::Rejected { + challenge_id: challenge.challenge_id, + kind: RejectKind::Transient, + reason: "cannot rebuild the audited subtree".to_string(), + }; + } + }; + let authorised: HashSet = plan.leaf_keys.iter().copied().collect(); + + // Refuse the whole challenge before touching storage, matching how an + // over-broad challenge is handled above. An honest auditor cannot reach this: + // it samples only the leaves round 1 returned, and it has no `nonced_root` or + // `content_len` to verify an answer for anything else against, so an opening + // outside the subtree could not tell it anything even if served. + if let Some(outside) = key_order.iter().find(|key| !authorised.contains(*key)) { + return SubtreeSliceResponse::Rejected { + challenge_id: challenge.challenge_id, + kind: RejectKind::Protocol, + reason: format!( + "slice challenge opens {} which is outside the audited subtree", + hex::encode(outside) + ), + }; + } + let mut items = Vec::with_capacity(challenge.openings.len()); for key in key_order { let indices = indices_by_key.remove(&key).unwrap_or_default(); - // Open ONLY keys committed under this pin. A key not in the pinned tree - // is `Absent` — never served from local storage just because we happen to - // hold it (§15: serving an uncommitted-but-held key would let a forged - // challenge harvest data and muddy the possession proof for THIS commit). - if built.proof_for(&key).is_none() { - items.push(SubtreeSliceItem::Absent { key }); - continue; - } match serve_committed_key_openings(challenge, storage, key, indices).await { KeyServe::Items(mut built_items) => items.append(&mut built_items), KeyServe::Absent => items.push(SubtreeSliceItem::Absent { key }), diff --git a/tests/poc_audit_handler_live.rs b/tests/poc_audit_handler_live.rs index 103f40e0..9b988dc9 100644 --- a/tests/poc_audit_handler_live.rs +++ b/tests/poc_audit_handler_live.rs @@ -894,3 +894,110 @@ async fn slice_challenge_with_too_many_distinct_keys_is_rejected() { other => panic!("expected Rejected(distinct keys), got {other:?}"), } } + +/// Round 2 may open only the leaves round 1 proved, not any key in the pinned +/// commitment. Otherwise a cheap round 1 over a small subtree would authorise +/// openings against arbitrary other records, so the work round 2 costs would not +/// be bounded by the work the caller paid for in round 1. +/// +/// Both halves are asserted from one live round 1 so they cannot drift: a key +/// inside the proved subtree is served, and a key that is genuinely committed +/// and whose bytes are present on disk is refused purely for being outside it. +/// +/// FLIPS IF: the responder goes back to authorising round 2 against the whole +/// pinned commitment (a `contains_key`/`proof_for` membership test) instead of +/// against the round-1 subtree. +#[tokio::test] +async fn slice_challenge_outside_the_audited_subtree_is_refused() { + let (storage, _t) = test_storage().await; + // 64 committed leaves select a strictly smaller subtree, so committed keys + // outside it definitely exist. + let indices: Vec = (1..=64u8).collect(); + let r = Responder::new(&storage, &indices).await; + let pin = r.current_hash(); + let nonce = [0x51u8; 32]; + + // Round 1 decides the authorised subtree; take its leaves as ground truth + // rather than recomputing the selection in the test. + let proof = match handle_subtree_challenge( + &challenge_for(&r, pin, nonce), + &storage, + &r.peer_id, + false, + Some(&r.state), + ) + .await + { + SubtreeAuditResponse::Proof { proof, .. } => proof, + other => panic!("expected Proof, got {other:?}"), + }; + let inside: Vec<[u8; 32]> = proof.leaves.iter().map(|l| l.key).collect(); + assert!(!inside.is_empty(), "round 1 must prove at least one leaf"); + + // A committed key that round 1 did NOT prove, whose bytes are on disk — so + // serving it would succeed, and a refusal can only be the scope check. + let outside = indices + .iter() + .map(|i| Responder::address(*i)) + .find(|addr| !inside.contains(addr)) + .expect("64 leaves must leave a committed key outside the subtree"); + assert!( + storage.get_raw(&outside).await.expect("read").is_some(), + "the out-of-subtree key's bytes must be present, so the refusal is about \ + scope and not about missing data" + ); + + let slice_challenge = |key: [u8; 32]| SubtreeSliceChallenge { + challenge_id: 51, + nonce, + challenged_peer_id: r.peer_id_bytes, + expected_commitment_hash: pin, + openings: vec![SubtreeSliceOpening { + key, + block_index: 0, + }], + }; + + // Inside the proved subtree: served. + let in_resp = handle_subtree_slice_challenge( + &slice_challenge(inside[0]), + &storage, + &r.peer_id, + false, + Some(&r.state), + ) + .await; + match in_resp { + SubtreeSliceResponse::Items { items, .. } => { + assert!( + matches!(items.as_slice(), [SubtreeSliceItem::Present { .. }]), + "a leaf round 1 proved must still be openable, got {items:?}" + ); + } + other => panic!("expected Items for an in-subtree key, got {other:?}"), + } + + // Outside it: refused whole, and NOT as `Absent`. `Absent` is what a read + // that found nothing would produce, so a `Rejected` here is what shows the + // challenge was turned away before storage was consulted. + let out_resp = handle_subtree_slice_challenge( + &slice_challenge(outside), + &storage, + &r.peer_id, + false, + Some(&r.state), + ) + .await; + match out_resp { + SubtreeSliceResponse::Rejected { reason, .. } => { + assert!( + reason.contains("outside the audited subtree"), + "expected an out-of-subtree rejection, got: {reason}" + ); + } + other => panic!( + "a committed-but-unproved key must be refused, not served or reported \ + absent; got {other:?}" + ), + } +} From c9f287070fd78a8103ea505b33dedf5f2e20355f Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 4 Aug 2026 11:16:50 +0900 Subject: [PATCH 2/3] fix(replication): apply the audit wire ceiling on encode and to unknown variants Two gaps in how the replication wire-size ceiling was selected, both of them places where the rule "a body that cannot be classified must not be treated generously" was not being applied. The audit message families take a much tighter ceiling than core replication, because a core body legitimately carries a whole chunk while an audit body never does. That ceiling was enforced when decoding but not when encoding, where only the core limit applied. An audit body that outgrew the audit ceiling would therefore serialise successfully and then be dropped, before decoding, by every peer that received it. Because that drop is silent, the sender would be scored for the resulting non-answer instead of being told its message was unsendable. Encoding now measures against the same family ceiling the decoder uses, so the failure surfaces locally, at the point the oversized body was built. No legitimate body reaches the limit today: the largest an honest audit can produce is a round-1 proof at the commitment key-count cap, which an existing test pins under the ceiling with headroom. Separately, the family lookup answered "core" for any index outside the declared variants, by way of a catch-all arm. An index no variant declares cannot be a legitimate message of any family, so treating it as core handed it the larger allowance on both the encode and decode paths. The lookup now returns no family for such an index, and every caller gives that the strict ceiling, matching how a prefix too malformed to parse is already handled. Postcard rejects an unknown outer discriminant before decoding any trailing collection, so this closes the invariant rather than a reachable hole; the value is that a variant added later without a family entry fails loudly instead of quietly inheriting the larger limit. Two existing tests built oversized audit payloads by calling the encoder, which now refuses them. They exercise the receive path, so they construct their bytes by serialising directly instead. That is also a more faithful reproduction: a hostile peer does not run this encoder, and a test of what arrives should not depend on the sender being willing to produce it. --- src/replication/protocol.rs | 209 +++++++++++++++++++++++++++++--- tests/poc_audit_handler_live.rs | 151 +++++++++++++++++++++-- 2 files changed, 335 insertions(+), 25 deletions(-) diff --git a/src/replication/protocol.rs b/src/replication/protocol.rs index 2ebe24d6..9005bb08 100644 --- a/src/replication/protocol.rs +++ b/src/replication/protocol.rs @@ -49,10 +49,28 @@ impl ReplicationMessage { let bytes = postcard::to_stdvec(self) .map_err(|e| ReplicationProtocolError::SerializationFailed(e.to_string()))?; - if bytes.len() > MAX_REPLICATION_MESSAGE_SIZE { + // The same family ceiling the decoder applies, from the same table and + // with the same arms, including the unclassified case. Every receiver + // drops a subtree-audit body over that ceiling before decoding it, so + // encoding one and putting it on the wire produces a message nobody can + // read, and the responder is then scored for the resulting silence. + // Failing here turns that into a local, attributable error at the point + // the oversized body was built. + // + // Matching `decode` exactly also means a body added later without a + // family entry is measured against the same limit at both ends, so it + // cannot become traffic that encodes here and is dropped there. It fails + // loudly only if it exceeds the strict ceiling; a small unclassified body + // still encodes, which is fine, because the receiver applies the same + // ceiling and will accept it too. No legitimate body reaches the ceiling: + // the largest is a round-1 proof at the commitment + // key-count cap, pinned under it with headroom by + // `max_round1_proof_fits_the_audit_family_ceiling`. + let max_size = ceiling_for(family_of_variant(self.body.variant_index())); + if bytes.len() > max_size { return Err(ReplicationProtocolError::MessageTooLarge { size: bytes.len(), - max_size: MAX_REPLICATION_MESSAGE_SIZE, + max_size, }); } @@ -85,10 +103,10 @@ impl ReplicationMessage { // A prefix too malformed to classify takes the strict ceiling too: it // cannot be a legitimate message of any family, so it is not one to // decode generously. - let max_size = match peek_variant_index(data).map(family_of_variant) { - Some(family) if !family.is_audit() => MAX_REPLICATION_MESSAGE_SIZE, - _ => MAX_SUBTREE_AUDIT_MESSAGE_SIZE, - }; + // `and_then`, not `map`: an index no variant declares classifies as + // nothing, and `ceiling_for` gives nothing the strict ceiling exactly + // like an unparseable prefix rather than the core allowance. + let max_size = ceiling_for(peek_variant_index(data).and_then(family_of_variant)); if data.len() > max_size { return Err(ReplicationProtocolError::MessageTooLarge { size: data.len(), @@ -266,7 +284,7 @@ impl ReplicationMessageBody { /// [`REPLICATION_PROTOCOL_ID`]: crate::replication::config::REPLICATION_PROTOCOL_ID #[must_use] pub fn is_subtree_audit(&self) -> bool { - family_of_variant(self.variant_index()) == BodyFamily::SubtreeAudit + family_of_variant(self.variant_index()) == Some(BodyFamily::SubtreeAudit) } } @@ -305,11 +323,38 @@ impl BodyFamily { /// /// Indices match [`ReplicationMessageBody::variant_index`], which is declaration /// order and postcard-stable. +/// +/// `None` for an index that is not a variant of this enum. That is not the same +/// as "core": an index nothing declares cannot be a legitimate message of any +/// family, so callers selecting a wire ceiling must give it the strict audit one +/// rather than the generous core allowance. Postcard rejects an unknown outer +/// discriminant before decoding any trailing collection, so this is closing the +/// invariant rather than a demonstrated hole — but the rule "failing to classify +/// means do not decode generously" should hold everywhere, not just for a prefix +/// too malformed to parse. #[must_use] -pub(crate) fn family_of_variant(index: usize) -> BodyFamily { +pub(crate) fn family_of_variant(index: usize) -> Option { match index { - 11..=14 => BodyFamily::SubtreeAudit, - _ => BodyFamily::Core, + 11..=14 => Some(BodyFamily::SubtreeAudit), + 0..=10 | 15 | 16 => Some(BodyFamily::Core), + _ => None, + } +} + +/// The wire ceiling a body of this family takes. +/// +/// One definition for both directions: `encode` measures what it produced +/// against it, `decode` measures what arrived before allocating anything. They +/// were duplicated matches that had to be kept in step by hand, which is the +/// kind of pair that silently drifts. +/// +/// `None` — an index no declared variant uses — takes the STRICT ceiling. +/// Failing to classify must never mean "decode generously". +#[must_use] +pub(crate) fn ceiling_for(family: Option) -> usize { + match family { + Some(family) if !family.is_audit() => MAX_REPLICATION_MESSAGE_SIZE, + _ => MAX_SUBTREE_AUDIT_MESSAGE_SIZE, } } @@ -1439,15 +1484,22 @@ mod tests { }, }), }; - let encoded = msg - .encode() - .expect("worst-case round-1 proof must fit the wire cap"); + // Measure without going through `encode`, which now enforces this very + // ceiling: encoding first would turn a regression into an opaque + // `MessageTooLarge` from the guard instead of the explicit, diagnosable + // assertion below. + let encoded = postcard::to_stdvec(&msg).expect("serialize"); assert!( encoded.len() <= MAX_SUBTREE_AUDIT_MESSAGE_SIZE, "worst legitimate round-1 proof is {} bytes, over the audit ceiling of \ {MAX_SUBTREE_AUDIT_MESSAGE_SIZE}", encoded.len() ); + // And it must genuinely pass the guard, not merely fit the number. + assert!( + msg.encode().is_ok(), + "the worst legitimate round-1 proof must still be encodable" + ); } /// Every body variant, in declaration order, so a test can walk the whole @@ -1592,12 +1644,26 @@ mod tests { }) .collect(), }; - let encoded = ReplicationMessage { + // Serialised directly rather than through `encode`, which now refuses to + // emit an audit body over the audit ceiling. That guard protects honest + // senders; it says nothing about what arrives, since a hostile peer does + // not run our encoder. Building the bytes the way an attacker would is + // what keeps this a test of the RECEIVE path. + let encoded = postcard::to_stdvec(&ReplicationMessage { request_id: 1, body: ReplicationMessageBody::SubtreeSliceChallenge(challenge), - } - .encode() - .expect("encode"); + }) + .expect("serialize"); + // Pin BOTH bounds. Going through `encode` used to imply the upper one; + // asserting it keeps the reproduction meaningful, since a body over the + // core ceiling would be refused by size alone and would no longer + // demonstrate that classification is what catches it. + assert!( + encoded.len() < crate::replication::config::MAX_REPLICATION_MESSAGE_SIZE, + "the reproduction must sit UNDER the core ceiling, or it proves \ + nothing about family classification; got {} bytes", + encoded.len() + ); assert!( encoded.len() > crate::replication::config::MAX_SUBTREE_AUDIT_MESSAGE_SIZE, @@ -1605,7 +1671,7 @@ mod tests { encoded.len() ); // The classification the receive path performs, before any decode. - let family = peek_variant_index(&encoded).map(family_of_variant); + let family = peek_variant_index(&encoded).and_then(family_of_variant); assert_eq!(family, Some(BodyFamily::SubtreeAudit)); assert!( family.map_or(true, BodyFamily::is_audit), @@ -1620,11 +1686,118 @@ mod tests { fn family_table_agrees_with_the_body_predicates() { for body in all_bodies() { let family = family_of_variant(body.variant_index()); + // Every DECLARED variant must classify. `None` is reserved for an + // index no variant uses; a real body landing there would silently + // take the strict ceiling on both encode and decode. + let family = family.expect("every declared variant must have a family"); assert_eq!(body.is_subtree_audit(), family == BodyFamily::SubtreeAudit); assert_eq!(family.is_audit(), body.is_subtree_audit()); } } + /// The encoder applies the same family ceiling the decoder does. Without + /// this, an audit body that outgrew the audit ceiling would serialise + /// happily and then be dropped pre-decode by every peer that received it — + /// and since the drop is silent, the sender would be scored for the + /// resulting non-answer rather than told its message was unsendable. + /// + /// No legitimate body reaches the ceiling today; this is about where the + /// failure surfaces if one ever does. + /// + /// FLIPS IF: `encode` goes back to checking only the core ceiling. + #[test] + fn the_encoder_refuses_an_audit_body_over_the_audit_ceiling() { + let z = [0u8; 32]; + let oversized = ReplicationMessage { + request_id: 1, + body: ReplicationMessageBody::SubtreeSliceChallenge(SubtreeSliceChallenge { + challenge_id: 1, + nonce: z, + challenged_peer_id: z, + expected_commitment_hash: z, + openings: (0..200_000u32) + .map(|i| SubtreeSliceOpening { + key: z, + block_index: i, + }) + .collect(), + }), + }; + // Sits under the core ceiling, so only the family-aware check can catch + // it — this is the exact gap, not merely an enormous message. + let raw = postcard::to_stdvec(&oversized).expect("serialize"); + assert!(raw.len() > MAX_SUBTREE_AUDIT_MESSAGE_SIZE); + assert!(raw.len() < MAX_REPLICATION_MESSAGE_SIZE); + + match oversized.encode() { + Err(ReplicationProtocolError::MessageTooLarge { max_size, .. }) => { + assert_eq!( + max_size, MAX_SUBTREE_AUDIT_MESSAGE_SIZE, + "an audit body must be measured against the audit ceiling" + ); + } + // Report only the length on the success arm: debug-printing the body + // would dump megabytes of payload into the failure output. + other => panic!( + "expected the audit ceiling to refuse this, got {:?}", + other.map(|bytes| bytes.len()) + ), + } + + // A core body of the same size still encodes: the tighter ceiling is + // scoped to the audit families and must not shrink core traffic, which + // legitimately carries a whole chunk. + let core = ReplicationMessage { + request_id: 1, + body: ReplicationMessageBody::FetchResponse(FetchResponse::Success { + key: z, + data: vec![0u8; MAX_SUBTREE_AUDIT_MESSAGE_SIZE * 2], + }), + }; + assert!(core.encode().is_ok(), "core bodies keep the core allowance"); + } + + /// An index no variant declares classifies as nothing, and "nothing" takes + /// the strict audit ceiling rather than the generous core allowance on both + /// sides of the wire. Postcard rejects an unknown outer discriminant before + /// decoding any trailing collection, so this closes the invariant rather than + /// a demonstrated hole — but the rule has to hold uniformly, or a later + /// variant added without a family entry would quietly inherit 10 MiB. + /// + /// FLIPS IF: the family table regains a catch-all that answers `Core`. + #[test] + fn an_undeclared_variant_index_classifies_as_nothing() { + let declared = all_bodies().len(); + assert_eq!( + declared, 17, + "update this test's bounds when a variant is added or removed" + ); + for index in [declared, declared + 1, 99, usize::MAX] { + assert_eq!( + family_of_variant(index), + None, + "index {index} is not a declared variant and must not classify" + ); + } + // Call the REAL selector both wire paths use. Restating its arms here, or + // asserting a property that `None` satisfies vacuously, would pass no + // matter how the production selection changed. + assert_eq!( + ceiling_for(family_of_variant(declared)), + MAX_SUBTREE_AUDIT_MESSAGE_SIZE, + "an unclassifiable variant must take the strict audit ceiling, not \ + the generous core allowance" + ); + // And a declared core body still takes the generous one, so the + // assertion above is about being unclassifiable rather than about the + // selector returning one answer for everything. + assert_eq!( + ceiling_for(family_of_variant(0)), + MAX_REPLICATION_MESSAGE_SIZE, + "core bodies keep the core allowance" + ); + } + // `is_subtree_audit()` classifies exactly the four subtree-audit variants // (both rounds), and NOTHING else — crucially not the digest-based // `AuditChallenge`/`AuditResponse` pair, which remains on the core protocol. diff --git a/tests/poc_audit_handler_live.rs b/tests/poc_audit_handler_live.rs index 9b988dc9..03e865b7 100644 --- a/tests/poc_audit_handler_live.rs +++ b/tests/poc_audit_handler_live.rs @@ -19,7 +19,12 @@ clippy::expect_used, clippy::panic, clippy::missing_panics_doc, - clippy::cast_possible_truncation + clippy::cast_possible_truncation, + // These drive a live responder end to end: build a store, run round 1, then + // assert on round 2. Splitting one scenario to satisfy a line count would + // mean re-running the setup and losing the guarantee that both halves see + // the SAME proof. + clippy::too_many_lines )] use std::sync::Arc; @@ -111,6 +116,19 @@ impl Responder { } } +/// Describe a slice response by variant only, for failure messages. +/// +/// A served response carries a full Bao slice per opening, so debug-printing one +/// dumps kilobytes here and megabytes at the opening cap over full-size chunks. +/// Assertions that can fail on a SERVED response report through this. +fn response_kind(resp: &SubtreeSliceResponse) -> String { + match resp { + SubtreeSliceResponse::Rejected { reason, .. } => format!("Rejected({reason})"), + SubtreeSliceResponse::Items { items, .. } => format!("Items({} entries)", items.len()), + SubtreeSliceResponse::Bootstrapping { .. } => "Bootstrapping".to_string(), + } +} + fn challenge_for(responder: &Responder, pin: [u8; 32], nonce: [u8; 32]) -> SubtreeAuditChallenge { SubtreeAuditChallenge { challenge_id: 42, @@ -958,9 +976,11 @@ async fn slice_challenge_outside_the_audited_subtree_is_refused() { }], }; + let first_inside = *inside.first().expect("round 1 proved at least one leaf"); + // Inside the proved subtree: served. let in_resp = handle_subtree_slice_challenge( - &slice_challenge(inside[0]), + &slice_challenge(first_inside), &storage, &r.peer_id, false, @@ -974,12 +994,16 @@ async fn slice_challenge_outside_the_audited_subtree_is_refused() { "a leaf round 1 proved must still be openable, got {items:?}" ); } - other => panic!("expected Items for an in-subtree key, got {other:?}"), + other => panic!( + "expected Items for an in-subtree key, got {}", + response_kind(&other) + ), } - // Outside it: refused whole, and NOT as `Absent`. `Absent` is what a read - // that found nothing would produce, so a `Rejected` here is what shows the - // challenge was turned away before storage was consulted. + // Outside it: refused whole, and NOT as `Absent`. Here the bytes are + // present, so serving would have succeeded; the sibling test repeats this + // with the bytes deleted and still expects `Rejected`, which together pin + // that the refusal is decided by scope and not by storage contents. let out_resp = handle_subtree_slice_challenge( &slice_challenge(outside), &storage, @@ -997,7 +1021,120 @@ async fn slice_challenge_outside_the_audited_subtree_is_refused() { } other => panic!( "a committed-but-unproved key must be refused, not served or reported \ - absent; got {other:?}" + absent; got {}", + response_kind(&other) ), } + + // A challenge MIXING an authorised key with an unauthorised one must be + // refused whole. Without this, an implementation that served the inside key + // and marked the outside one `Absent` would satisfy the two cases above + // while still doing the work the scope check exists to prevent. + let mixed = SubtreeSliceChallenge { + challenge_id: 52, + nonce, + challenged_peer_id: r.peer_id_bytes, + expected_commitment_hash: pin, + openings: vec![ + SubtreeSliceOpening { + key: first_inside, + block_index: 0, + }, + SubtreeSliceOpening { + key: outside, + block_index: 0, + }, + ], + }; + let mixed_resp = + handle_subtree_slice_challenge(&mixed, &storage, &r.peer_id, false, Some(&r.state)).await; + assert!( + matches!(mixed_resp, SubtreeSliceResponse::Rejected { .. }), + "one unauthorised opening must refuse the whole challenge, not just its \ + own entry; got {mixed_resp:?}" + ); +} + +/// The scope refusal must not depend on what storage holds, which is what makes +/// it a work bound rather than only an authorisation rule. +/// +/// The key here is committed, outside the proved subtree, and its bytes are +/// DELETED. An implementation that consulted storage and reported what it found +/// would answer `Absent`; this one still answers `Rejected`, the same as when the +/// bytes are present in the sibling test above. So the refusal is decided +/// entirely by scope. +/// +/// Stated precisely, because the stronger claim is tempting and wrong: this +/// cannot prove no read occurred. An implementation that read the bytes and then +/// rejected anyway would also pass. What it pins is that the ANSWER does not vary +/// with storage contents, which is the property the two tests together establish +/// and the one a per-key `Absent` degradation would break. +/// +/// FLIPS IF: the scope check degrades to per-key `Absent` handling, or the +/// refusal starts depending on what storage holds. +#[tokio::test] +async fn an_out_of_subtree_key_is_refused_without_consulting_storage() { + let (storage, _t) = test_storage().await; + let indices: Vec = (1..=64u8).collect(); + let r = Responder::new(&storage, &indices).await; + let pin = r.current_hash(); + let nonce = [0x53u8; 32]; + + let proof = match handle_subtree_challenge( + &challenge_for(&r, pin, nonce), + &storage, + &r.peer_id, + false, + Some(&r.state), + ) + .await + { + SubtreeAuditResponse::Proof { proof, .. } => proof, + other => panic!("expected Proof, got {other:?}"), + }; + let inside: Vec<[u8; 32]> = proof.leaves.iter().map(|l| l.key).collect(); + + // Committed, outside the proved subtree, and its bytes are then DELETED. + let outside = indices + .iter() + .map(|i| Responder::address(*i)) + .find(|addr| !inside.contains(addr)) + .expect("a committed key outside the subtree must exist"); + storage.delete(&outside).await.expect("delete bytes"); + assert!( + storage.get_raw(&outside).await.expect("read").is_none(), + "the bytes must really be gone for this test to discriminate" + ); + + let challenge = SubtreeSliceChallenge { + challenge_id: 53, + nonce, + challenged_peer_id: r.peer_id_bytes, + expected_commitment_hash: pin, + openings: vec![SubtreeSliceOpening { + key: outside, + block_index: 0, + }], + }; + let resp = + handle_subtree_slice_challenge(&challenge, &storage, &r.peer_id, false, Some(&r.state)) + .await; + + match resp { + SubtreeSliceResponse::Rejected { reason, .. } => { + assert!( + reason.contains("outside the audited subtree"), + "expected the scope refusal, got: {reason}" + ); + } + // `Absent` here would mean the refusal reported what storage holds rather + // than that the key is out of scope. + other => { + panic!( + "the refusal must not depend on storage contents; expected \ + Rejected as with the bytes present, got {}", + response_kind(&other) + ) + } + } } From 339c13eb5a7aaee4d8e91db622fa69a96df9c833 Mon Sep 17 00:00:00 2001 From: grumbach Date: Thu, 6 Aug 2026 13:25:37 +0900 Subject: [PATCH 3/3] docs(replication): record the round-2 scope bound and correct the is_held contract ADR-0009 enumerates the subtree audit's responder work bounds and its wire ceilings. Both of the fixes in this branch add to those lists, so leaving them out would leave the ADR describing a responder that no longer exists. The ADR is still Proposed, so governance asks for an amendment here rather than a superseding decision record: nothing it decided has changed. The amendment records that round 2 may open only the leaves round 1 proved rather than any key in the pinned commitment, why membership of the commitment is the weaker property, and why an honest auditor cannot reach the refusal. It records that one selector now chooses the wire ceiling for both directions, so an oversized audit body fails at the sender instead of being dropped without diagnosis by every receiver, and that a discriminant no declared variant uses takes the strict ceiling on both paths. It also records what the new tests pin. Separately, `is_held` claimed to be the SAME predicate the round-2 responder applies, and after the scope change that is no longer true: the responder now authorises only the nonce-selected subtree of the pinned tree. The relationship that survives is the one the pruner actually needs, so the comment now states that instead. `is_held` is a strict upper bound on what can be opened, which means a key it answers false for can never be asked for. Narrowing it toward the responder's predicate would be unsound, because the pruner runs without knowing which nonce a future challenge will carry, so every committed key is reachable. The test comment asserting the old equality is corrected the same way. Comments and documentation only; no behaviour change. --- ...audit-proof-shape-and-protocol-families.md | 26 ++++++++++++++- src/replication/commitment_state.rs | 32 ++++++++++++------- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/docs/adr/ADR-0009-audit-proof-shape-and-protocol-families.md b/docs/adr/ADR-0009-audit-proof-shape-and-protocol-families.md index 1b735dec..c47df85a 100644 --- a/docs/adr/ADR-0009-audit-proof-shape-and-protocol-families.md +++ b/docs/adr/ADR-0009-audit-proof-shape-and-protocol-families.md @@ -111,6 +111,17 @@ The work budget carries debt, so an expensive proof is admitted proportionally less often than a cheap one. Its refill and burst sizes are set above measured honest demand at the maximum supported commitment size. +Round two may open only the leaves round one proved, not any key in the pinned +commitment. Membership of the commitment is the weaker property: it would let a +cheap round one over small records authorise openings against large records +elsewhere, so round two's cost would not be bounded by the work the caller had +already paid for. The authorised set is recomputed from the pinned tree and the +audit nonce, both of which the session already fixes, so it costs one tree walk +with no chunk reads. A challenge naming any key outside the proved subtree is +refused whole, before any chunk is read. An honest auditor cannot reach that +case: it samples only the leaves round one returned, and it holds no committed +block-tree root or content length to verify an answer for anything else against. + ### Wire and decode bounds Subtree family messages take a tighter wire ceiling than core replication. The @@ -118,6 +129,14 @@ limit is selected from the encoded body discriminant before deserializing attacker-controlled collections and is sized above the largest legitimate round-one proof at the commitment key-count cap. +One selector serves both directions. Encoding measures the body it produced +against the same family ceiling the decoder applies, so an audit body that +outgrew that ceiling fails at the sender rather than being dropped, before +decode and without diagnosis, by every peer that receives it. A discriminant no +declared variant uses classifies as no family at all, and no family takes the +strict ceiling on both paths, matching how a prefix too malformed to parse is +already treated. Failing to classify never means decoding generously. + Core messages retain the existing core ceiling. In particular, `AuditChallenge` and `AuditResponse` remain core messages and are not reclassified by this ADR. @@ -175,7 +194,12 @@ reclassified by this ADR. - Routing tests assert that subtree bodies are accepted only on the subtree id while core and digest-audit bodies remain on the core id. - Size tests prove the largest legitimate subtree response fits its ceiling and - oversized subtree collections are rejected before allocation. + oversized subtree collections are rejected before allocation, on the encode + path as well as the decode path. +- Live-responder tests drive one round one and then assert both halves of the + round-two scope rule from its result: a proved leaf is still served, and a + committed key outside the proved subtree is refused whether or not its bytes + are on disk. - Re-validation is required for changes to subtree key derivation, sampling, round-one selection, session rules, or the subtree protocol family. diff --git a/src/replication/commitment_state.rs b/src/replication/commitment_state.rs index 3d8e4c29..8c7b2840 100644 --- a/src/replication/commitment_state.rs +++ b/src/replication/commitment_state.rs @@ -592,15 +592,22 @@ impl ResponderCommitmentState { /// could still pin a recently gossiped root and open this key's blocks in a /// round-2 slice challenge. /// - /// This is the SAME predicate the round-2 responder uses to decide a key is - /// "committed" (`handle_subtree_slice_challenge` calls `built.proof_for(key)` - /// on the pinned slot, which is committed iff `contains_key`), folded over - /// every retained slot. The pruner consults it before deleting an - /// out-of-range key, so "the pruner will not delete it" and "the responder - /// still owes an answer for it" are provably the same boolean and cannot - /// drift. `slots` holds at most `RETAINED_GOSSIPED_COMMITMENTS` + 1 - /// commitments, and `contains_key` is an allocation-free binary search, so - /// this is a short, allocation-free read. + /// This is `contains_key` on the pinned slot, folded over every retained + /// slot, and it is a strict UPPER bound on what the round-2 responder can be + /// asked to open. `handle_subtree_slice_challenge` authorises an opening only + /// if its key is in the subtree round 1 actually proved, which is one + /// nonce-selected block of the pinned tree rather than all of it. The bound + /// holds in the direction the pruner needs: every key the responder may owe + /// an answer for is committed under some retained slot, so a key this returns + /// `false` for can never be opened. The pruner consults it before deleting an + /// out-of-range key, so it keeps at least what is still answerable, and over + /// a given nonce it keeps more. Narrowing this predicate toward the + /// responder's would be unsound: the nonce is not known when the pruner runs, + /// so every committed key is reachable by some future challenge. + /// + /// `slots` holds at most `RETAINED_GOSSIPED_COMMITMENTS` + 1 commitments, and + /// `contains_key` is an allocation-free binary search, so this is a short, + /// allocation-free read. #[must_use] pub fn is_held(&self, key: &XorName) -> bool { self.inner.read().slots.iter().any(|c| c.contains_key(key)) @@ -1202,9 +1209,10 @@ mod tests { // ANY retained slot (current + any root gossiped within the TTL) must // read held. It stops reading held once its commitment ages out BY TTL — // a bounded reprieve, not a permanent pin (the time-based age-out is - // covered by the synthetic-clock prune_slots tests). This mirrors the - // round-2 responder's `built.proof_for(key).is_some()` check folded over - // the slots, so "pruner won't delete" == "responder owes an answer". + // covered by the synthetic-clock prune_slots tests). It is the upper + // bound on what the round-2 responder can be asked to open: the responder + // authorises only the nonce-selected subtree of the pinned slot, so + // "pruner won't delete" covers every key it could still owe an answer for. let (pk, sk) = keypair(); let pk_bytes = pk.to_bytes(); let state = ResponderCommitmentState::new();