From b2ad24363f2e7cd6a499d48b2e1ec1414c98a9f6 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 06:37:21 +0000 Subject: [PATCH 01/17] Remove unresolvable internal references from source comments Fifteen comments in src/ cited internal identifiers that a reader of the published source cannot resolve. Each now states the thing the identifier stood for, or drops the citation where the surrounding text already carries the meaning. The MMP report group needed more than a rewording. It claimed the payload content was undefined and both variants were stubs, and neither is true: the link dispatcher routes 0x01 and 0x02 to real handlers, and both report types implement encode and decode. The group comment now names the types, and the two stale "(stub)" doc comments below it are corrected with it. --- src/bin/fips-gateway.rs | 2 +- src/config/gateway.rs | 2 +- src/config/mod.rs | 9 ++++----- src/config/transport.rs | 2 +- src/discovery/nostr/runtime.rs | 3 +-- src/gateway/nat.rs | 4 ++-- src/node/mod.rs | 2 -- src/node/tests/handshake.rs | 5 +++-- src/node/tests/unit.rs | 4 ++-- src/protocol/link.rs | 6 +++--- src/transport/udp/mod.rs | 4 ++-- 11 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index 02bfbf0e..ad4ea187 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -299,7 +299,7 @@ async fn main() { } }; - // Install inbound port-forward rules (TASK-2026-0061). + // Install inbound port-forward rules. if let Err(e) = nat_mgr.set_port_forwards(&gw_config.port_forwards) { error!(error = %e, "Failed to install port-forward rules"); let _ = nat_mgr.cleanup(); diff --git a/src/config/gateway.rs b/src/config/gateway.rs index d524176d..b9a96330 100644 --- a/src/config/gateway.rs +++ b/src/config/gateway.rs @@ -76,7 +76,7 @@ pub struct GatewayConfig { #[serde(default)] pub conntrack: ConntrackConfig, - /// Inbound mesh port forwarding rules. See TASK-2026-0061. + /// Inbound mesh port forwarding rules. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub port_forwards: Vec, } diff --git a/src/config/mod.rs b/src/config/mod.rs index 995ee4de..10cfb0c4 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -68,8 +68,7 @@ const PUB_FILENAME: &str = "fips.pub"; /// Recognizes IPv4 `127.x.x.x`, IPv6 `::1` (with or without brackets), and /// the literal string `localhost`. Hostnames are conservatively assumed to /// be non-loopback. Used by `Config::validate()` to reject misconfigured -/// loopback UDP binds combined with non-loopback peer addresses (see -/// ISSUE-2026-0005). +/// loopback UDP binds combined with non-loopback peer addresses. fn is_loopback_addr_str(addr: &str) -> bool { // Bracketed IPv6: `[::1]:port` if let Some(rest) = addr.strip_prefix('[') @@ -896,9 +895,9 @@ impl Config { // Reject loopback UDP bind combined with non-loopback peer addresses. // Linux pins the source IP to a loopback-bound socket, so packets // sent from such a socket to external peers are dropped at the - // routing layer with no clear error in the daemon log. See - // ISSUE-2026-0005. Outbound-only mode is exempt because it - // overrides bind_addr to 0.0.0.0:0 (kernel-picked source). + // routing layer with no clear error in the daemon log. + // Outbound-only mode is exempt because it overrides bind_addr to + // 0.0.0.0:0 (kernel-picked source). for (name, cfg) in self.transports.udp.iter() { if cfg.outbound_only() { continue; diff --git a/src/config/transport.rs b/src/config/transport.rs index 1c73cc08..f911ad48 100644 --- a/src/config/transport.rs +++ b/src/config/transport.rs @@ -103,7 +103,7 @@ pub struct UdpConfig { /// unfamiliar addresses. The Node-level gate at /// `src/node/handlers/handshake.rs` carves out msg1 from peers /// already established on this transport (so rekey continues to - /// work) — see ISSUE-2026-0004. + /// work). #[serde(default, skip_serializing_if = "Option::is_none")] pub accept_connections: Option, } diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index e71e5646..af8c68f8 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -128,8 +128,7 @@ fn log_refusals(tally: &PunchTargetTally, peer: &str, session: &str) { /// `BootstrapEvent::Established` events and `adopt_established_traversal` keeps /// only the first on a non-deterministic race; when the two nodes' independent /// races resolve to mismatched sessions, each side's Noise msg1 lands on a peer -/// port the peer already stopped draining and both handshakes stall (root cause -/// of ISSUE-2026-0031). +/// port the peer already stopped draining and both handshakes stall. /// /// To collapse the four-socket dance to a single, guaranteed-matching socket /// pair, both nodes deterministically keep the session **initiated by the diff --git a/src/gateway/nat.rs b/src/gateway/nat.rs index b908e3cc..b1859f61 100644 --- a/src/gateway/nat.rs +++ b/src/gateway/nat.rs @@ -66,7 +66,7 @@ pub struct NatManager { lan_interface: String, /// Active mappings keyed by virtual IP. mappings: HashMap, - /// Inbound port-forward rules (TASK-2026-0061). + /// Inbound port-forward rules. port_forwards: Vec, } @@ -235,7 +235,7 @@ impl NatManager { batch.add(&snat_rule, MsgType::Add); } - // Inbound port-forward rules (TASK-2026-0061). Each forward is + // Inbound port-forward rules. Each forward is // one DNAT rule in prerouting keyed on (iif fips0, nfproto ipv6, // l4proto, th dport). When any forwards are configured, emit a // single LAN-side masquerade in postrouting so the LAN target diff --git a/src/node/mod.rs b/src/node/mod.rs index 458fda71..8e9f8b78 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1322,8 +1322,6 @@ impl Node { /// Returning the smallest (rather than the first-iterated, which used /// to vary across HashMap iteration order + async-startup race) makes /// the clamp deterministic across daemon restarts. - /// - /// See `ISSUE-2026-0011` for the empirical investigation. pub fn transport_mtu(&self) -> u16 { let min_operational = self .transports diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 996bdc2d..f6ea31b3 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -997,7 +997,7 @@ async fn test_should_admit_msg1_rejects_fresh_when_accept_off() { assert!(!node.should_admit_msg1(transport_id, &addr)); } -/// ISSUE-2026-0004 regression test: `should_admit_msg1` admits rekey/restart +/// Regression test: `should_admit_msg1` admits rekey/restart /// msg1 from a peer with an existing link even when the transport has /// accept_connections=false. Without this, the dual-init tie-breaker /// deadlocks (the larger-NodeAddr side drops the winner's rekey msg1). @@ -1069,7 +1069,8 @@ async fn test_should_admit_msg1_admits_rekey_when_udp_accept_off() { } /// Regression test for the udp.outbound_only rekey loop observed in -/// production 2026-04-30 (parallel to ISSUE-2026-0004). +/// production 2026-04-30 (parallel to the rekey/restart admission case +/// above). /// /// Production scenario: nomad runs `udp.outbound_only=true` with peer /// core-vm configured by hostname (`core-vm.tail65015.ts.net:2121`). diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 5467b57e..509aef5c 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1404,7 +1404,7 @@ async fn test_initiate_peer_connections_schedules_retry_on_no_transport() { } // ============================================================================ -// transport_mtu() — ISSUE-2026-0011 regression coverage +// transport_mtu() — minimum-across-transports regression coverage // ============================================================================ /// Helper: spawn a UdpTransport with the given mtu, started and operational. @@ -1429,7 +1429,7 @@ async fn make_udp_transport_with_mtu(id: u32, mtu: u16) -> TransportHandle { async fn test_transport_mtu_returns_min_across_operational() { // Multiple operational transports with varied MTUs. The picker must // return the smallest, deterministically, regardless of HashMap - // iteration order. This is the core ISSUE-2026-0011 regression test. + // iteration order. This is the core regression test for that. let mut node = make_node(); let (packet_tx, packet_rx) = packet_channel(64); node.packet_tx = Some(packet_tx); diff --git a/src/protocol/link.rs b/src/protocol/link.rs index 0dd652bb..2e0a22cb 100644 --- a/src/protocol/link.rs +++ b/src/protocol/link.rs @@ -73,10 +73,10 @@ pub enum LinkMessageType { /// Payload is opaque to intermediate nodes (end-to-end encrypted). SessionDatagram = 0x00, - // MMP reports (0x01-0x02) — content defined in TASK-2026-0006 - /// Sender-side MMP report (stub). + // MMP reports (0x01-0x02) — payload is an encoded SenderReport or ReceiverReport + /// Sender-side MMP report. SenderReport = 0x01, - /// Receiver-side MMP report (stub). + /// Receiver-side MMP report. ReceiverReport = 0x02, // Tree protocol (0x10-0x1F) diff --git a/src/transport/udp/mod.rs b/src/transport/udp/mod.rs index 3728fb90..83de32df 100644 --- a/src/transport/udp/mod.rs +++ b/src/transport/udp/mod.rs @@ -419,8 +419,8 @@ impl Transport for UdpTransport { /// Whether the transport accepts inbound handshake initiations. /// `outbound_only` mode forces this to false; otherwise reflects the /// `accept_connections` config field (default: true). Note that the - /// hard gate is at the Node level (see ISSUE-2026-0004 fix in - /// `src/node/handlers/handshake.rs`); this method is what that gate + /// hard gate is at the Node level (in `src/node/handlers/handshake.rs`); + /// this method is what that gate /// consults for transports that lack runtime-state-based filtering. fn accept_connections(&self) -> bool { if self.config.outbound_only() { From 005942f1413c0a750fbb5b7ccf9bff44cf9c5a3b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 07:08:40 +0000 Subject: [PATCH 02/17] Stop citing a design document that is not in the repository Three comments in the control snapshot module pointed readers at a fast-path design note that exists only in my working notes and is not a blob at any branch. Each now states the thing the document was cited for, so the comment stands on its own. The fourth citation, in the read handle's module doc, stays for now: the sentence around it is also wrong about when queries read the handle, and correcting the path alone would commit the false half. That whole block is replaced together in a later change. --- src/control/snapshot.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/control/snapshot.rs b/src/control/snapshot.rs index 8f5f4378..c7132202 100644 --- a/src/control/snapshot.rs +++ b/src/control/snapshot.rs @@ -1,8 +1,7 @@ //! Read-side state snapshots published from the node's natural mutators so //! pure-snapshot `show_*` queries render off the rx_loop hot path. //! -//! [`StatsSnapshot`] is the R2 reference implementation of the canonical -//! snapshot pattern (see `design/fast-path-refactoring-r0-read-handle.md`): a +//! [`StatsSnapshot`] is the reference implementation of the canonical snapshot pattern: a //! read-only data bundle published via `ArcSwap` from the tick after //! `StatsHistory::tick()`. It carries //! @@ -145,8 +144,8 @@ fn empty_acl_status() -> PeerAclStatus { /// the pure-snapshot `show_tree` / `show_bloom` / `show_cache` / `show_routing` /// / `show_identity_cache` queries render. Published via `ArcSwap`. /// -/// The R0 stub (`design/fast-path-refactoring-r0-read-handle.md`) names a -/// single combined `ArcSwap` for R3. This is that cell: one +/// This is the single combined `ArcSwap` cell for the routing +/// subsystems: one /// cohesive routing view holding the four subsystems (tree / bloom / coord /// cache / identity cache) plus the F-queue summary scalars. /// @@ -405,10 +404,8 @@ pub(crate) struct IdentityRow { /// `show_connections` / `show_transports` / `show_mmp` queries render. /// Published via `ArcSwap`. /// -/// The R0 stub (`design/fast-path-refactoring-r0-read-handle.md`) pre-scopes -/// R4 as `entities — ArcSwap: peers / sessions / links / -/// connections / transports, published per-entity with `Vec>` -/// structural sharing`. This is that cell. +/// This is the `entities` cell: peers / sessions / links / connections / +/// transports, published per-entity with `Vec>` structural sharing. /// /// **Structural sharing (the umbrella mandate).** Every entity table is a /// `Vec>`, so a republish in which only one row changed re-allocates From 32822d77fecef0a0f05b1c1286b607b6c3f97c69 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 12:05:20 +0000 Subject: [PATCH 03/17] Remove unresolvable internal references from master-only comments These sites have no counterpart on the maintenance branch: the lookup origination comment, three labels in the traversal machine, a stale note in the MMP wire module, and nine lines in the node module left over from an earlier partial cleanup. The nine node-module lines matter more than they look. The same comments exist on the maintenance branch, so both sides must reach identical text for the merge to apply as one modification instead of a conflict. This commit writes the final text; the maintenance branch copies it. --- src/node/mod.rs | 20 ++++++++++---------- src/nostr/traversal_machine.rs | 6 +++--- src/proto/lookup/core.rs | 4 ++-- src/proto/mmp/wire.rs | 2 -- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index fa4b93f9..7a78aa0b 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -440,19 +440,19 @@ pub struct Node { /// live mutable `stats_history` above stays on the tick. stats_snapshot: std::sync::Arc>, - /// Read-side snapshot of the Category-D derived/routing/cache subsystems + /// Read-side snapshot of the derived/routing/cache subsystems /// (tree / bloom / coord cache / identity cache + F-queue scalars) that the /// `show_tree` / `show_bloom` / `show_cache` / `show_routing` / /// `show_identity_cache` queries render off the rx_loop. Published from the - /// tick (see [`Self::publish_routing_snapshot`] for the Q1 rationale). + /// tick (see [`Self::publish_routing_snapshot`] for the rationale). routing_snapshot: std::sync::Arc>, - /// Read-side snapshot of the Category-E per-entity tables (peers / sessions + /// Read-side snapshot of the per-entity tables (peers / sessions /// / links / connections / transports + mmp) that the `show_peers` / /// `show_sessions` / `show_links` / `show_connections` / `show_transports` /// / `show_mmp` queries render off the rx_loop. Published from the tick with /// `Vec>` structural sharing (unchanged rows reused by pointer); - /// see [`Self::publish_entities_snapshot`] for the Q1 rationale. + /// see [`Self::publish_entities_snapshot`] for the rationale. entities_snapshot: std::sync::Arc>, // === TUN Interface === @@ -1600,11 +1600,11 @@ impl Node { }; self.stats_snapshot.store(std::sync::Arc::new(snapshot)); - // Publish the Category-D routing read view alongside the stats + // Publish the routing read view alongside the stats // snapshot, from the same tick. self.publish_routing_snapshot(); - // Publish the Category-E per-entity read view from the same tick, with + // Publish the per-entity read view from the same tick, with // `Vec>` structural sharing against the previous snapshot. self.publish_entities_snapshot(); } @@ -1631,7 +1631,7 @@ impl Node { None } - /// Project the Category-D derived/routing/cache state into a + /// Project the derived/routing/cache state into a /// [`RoutingSnapshot`](crate::control::snapshot::RoutingSnapshot) and /// publish it via `ArcSwap`, so `show_tree` / `show_bloom` / `show_cache` /// / `show_routing` / `show_identity_cache` render off the rx_loop. @@ -1832,7 +1832,7 @@ impl Node { self.routing_snapshot.store(std::sync::Arc::new(snapshot)); } - /// Project the Category-E per-entity tables (peers / sessions / links / + /// Project the per-entity tables (peers / sessions / links / /// connections / transports + mmp) into an /// [`EntitySnapshot`](crate::control::snapshot::EntitySnapshot) and publish /// it via `ArcSwap`, so `show_peers` / `show_sessions` / `show_links` / @@ -1858,8 +1858,8 @@ impl Node { /// `Arc` is reused (kept by pointer) whenever it matches the prior row by /// identity and compares equal by value, so a tick in which only one /// peer/session changed re-allocates only that one row, not the whole table. - /// This is what keeps the publish cost off the hot path at scale (the exact - /// thing the umbrella warns a naive per-tick rebuild would violate). + /// This is what keeps the publish cost off the hot path at scale, which a + /// naive whole-table rebuild on every tick would not. fn publish_entities_snapshot(&self) { use crate::control::snapshot as snap; diff --git a/src/nostr/traversal_machine.rs b/src/nostr/traversal_machine.rs index 844c0af6..6725ccf6 100644 --- a/src/nostr/traversal_machine.rs +++ b/src/nostr/traversal_machine.rs @@ -275,7 +275,7 @@ mod tests { #[test] fn replay_first_then_repeat() { - // R1: first id Fresh; same id within window Replay. + // First id Fresh; same id within window Replay. let m = machine(); assert_eq!( m.note_session_seen("s1", 1000), @@ -286,7 +286,7 @@ mod tests { #[test] fn replay_prunes_expired() { - // R2: an entry past its expiry is pruned, so re-seeing it is Fresh. + // An entry past its expiry is pruned, so re-seeing it is Fresh. let m = machine(); // replay_window_ms = 1_000_000 assert_eq!( m.note_session_seen("s1", 1000), @@ -302,7 +302,7 @@ mod tests { #[test] fn replay_cap_evicts_oldest_by_expiry() { - // R3: cap overflow evicts oldest-by-expiry, returns (evicted, retained). + // Cap overflow evicts oldest-by-expiry, returns (evicted, retained). let m = machine(); // cap = 3, window huge so nothing expires here assert_eq!( m.note_session_seen("s1", 1), diff --git a/src/proto/lookup/core.rs b/src/proto/lookup/core.rs index 2602fac7..8967707d 100644 --- a/src/proto/lookup/core.rs +++ b/src/proto/lookup/core.rs @@ -123,8 +123,8 @@ pub(crate) fn plan_forward(request: &mut LookupRequest, rv: &impl RoutingView) - /// NOTE: unlike [`plan_forward`], this does NOT fall back to non-tree /// (cross-link) bloom-matching peers. That asymmetry is preserved verbatim from /// the pre-sans-IO `initiate_lookup` to keep this extraction behavior-neutral; -/// it is a known origination gap (ISSUE-2026-0059) whose fix adds the fallback -/// branch as a separate, behavior-changing change. +/// it is a known origination gap whose fix adds the fallback branch as a +/// separate, behavior-changing change. pub(crate) fn plan_initiate(request: &LookupRequest, rv: &impl RoutingView) -> Vec { let targets: Vec = rv .peers_reaching(&request.target) diff --git a/src/proto/mmp/wire.rs b/src/proto/mmp/wire.rs index 282f8fa1..59fd7143 100644 --- a/src/proto/mmp/wire.rs +++ b/src/proto/mmp/wire.rs @@ -78,8 +78,6 @@ pub struct ReceiverReport { pub interval_bytes_recv: u32, } -// Encode/decode will be implemented in Step 2. - impl SenderReport { /// Encode to wire format (48 bytes: msg_type + 3 reserved + 44 payload). pub fn encode(&self) -> Vec { From 8de50bd22b775291927e2f4e00cb5147066d16e7 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 12:19:28 +0000 Subject: [PATCH 04/17] Remove unresolvable internal references from files outside the source tree Six comments in CI configuration, packaging and test scripts cited internal identifiers, or named the private tracker in prose. Each now states what the identifier stood for. One of these ships. The DNS setup helper installs to /usr/lib/fips on every packaging path, so its comment reached users. That edit removes the identifier and the words naming the tracker; the two lines above it already state the whole mechanism, so nothing is lost. The retry-policy comment in the rekey test now says the loss rate is what the policy is sized for, rather than what the phases run under. No runner injects loss, so the stronger claim was not true. --- .github/workflows/ci.yml | 5 +++-- packaging/common/fips-dns-setup | 3 +-- testing/chaos/scenarios/bloom-storm.yaml | 2 +- testing/interop/interop-test.sh | 2 +- testing/mesh-lab/compose-trace-nat.yml | 2 +- testing/static/scripts/rekey-test.sh | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a271b7e4..711b5dcc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -466,8 +466,9 @@ jobs: # NM+dnsmasq, dns-delegate, no-resolver) across five distros, # plus end-to-end scenarios that boot a real fips daemon with a # real TUN and assert `dig @127.0.0.53 AAAA .fips` - # returns AAAA. Pins the production DNS bind path that - # ISSUE-2026-0002 lived in. Single matrix entry runs all 13 + # returns AAAA. Pins the production DNS bind path where a + # loopback-delivered query was once misattributed to the mesh + # interface and dropped. Single matrix entry runs all 13 # scenarios sequentially; ~7-12 min warm, ~12-15 min cold. - suite: dns-resolver type: dns-resolver diff --git a/packaging/common/fips-dns-setup b/packaging/common/fips-dns-setup index 2a6678b2..6617060f 100755 --- a/packaging/common/fips-dns-setup +++ b/packaging/common/fips-dns-setup @@ -117,8 +117,7 @@ EOF # - The IPV6_PKTINFO ifindex attribution behaviour where Linux reports # a packet to fips0's own IPv6 address as arriving on fips0 (despite # loopback delivery), which would otherwise be silently dropped by -# the daemon's mesh-interface filter — see ISSUE-2026-0002 in the -# project tracker. +# the daemon's mesh-interface filter. # # This backend is the recommended path on every systemd-resolved host # that doesn't have native dns-delegate support (systemd >= 258). diff --git a/testing/chaos/scenarios/bloom-storm.yaml b/testing/chaos/scenarios/bloom-storm.yaml index 9a017512..ca94eba9 100644 --- a/testing/chaos/scenarios/bloom-storm.yaml +++ b/testing/chaos/scenarios/bloom-storm.yaml @@ -72,7 +72,7 @@ netem: # `interval_secs`, the policies on the two listed edges are swapped. # This drives n04 to alternate parents between n02 and n03 each # round. The 4s cadence and 5ms-vs-100ms delta come from the -# original ISSUE-2026-0019 reproduction harness — they are +# original reproduction harness for this flap — they are # calibrated to produce a parent switch per round under the # zero-hysteresis FIPS overrides below. link_swap: diff --git a/testing/interop/interop-test.sh b/testing/interop/interop-test.sh index ab27b3c1..83c1ac9f 100755 --- a/testing/interop/interop-test.sh +++ b/testing/interop/interop-test.sh @@ -949,7 +949,7 @@ echo "" # versions. A mixed-version bloom/tree-encoding divergence shows up as a # node that never produces an in-band estimate (or returns null). Strict # band = [0.75N, 1.25N]; polled up to MESH_SIZE_TIMEOUT (the estimate -# converges over minutes — see ISSUE-2026-0046 on its transient jitter). +# converges over minutes and is transiently jittery). echo "Phase 7: Mesh-size estimate convergence (strict ±25% of true N=$NUM_NODES)" PASSED=0; FAILED=0 ms_lo="$(awk -v n="$NUM_NODES" 'BEGIN{printf "%.2f", 0.75*n}')" diff --git a/testing/mesh-lab/compose-trace-nat.yml b/testing/mesh-lab/compose-trace-nat.yml index 63680d19..cd172a16 100644 --- a/testing/mesh-lab/compose-trace-nat.yml +++ b/testing/mesh-lab/compose-trace-nat.yml @@ -1,6 +1,6 @@ # Compose override that bumps RUST_LOG to trace level on the modules # relevant to NAT-traversal handshake-completion flake evidence -# collection (ISSUE-2026-0027): +# collection: # # - fips::discovery::nostr — overlay advert publish/consume # (where the cross-init race begins) diff --git a/testing/static/scripts/rekey-test.sh b/testing/static/scripts/rekey-test.sh index bc78113b..4a4d39c2 100755 --- a/testing/static/scripts/rekey-test.sh +++ b/testing/static/scripts/rekey-test.sh @@ -176,7 +176,7 @@ RECONVERGE_STALL=10 TIMEOUT=5 CONVERGENCE_PING_TIMEOUT=1 # Strict-ping retry policy for the per-phase ping_all asserts. Under 1% -# i.i.d. packet loss (the lab condition surfaced by ISSUE-2026-0028) a +# i.i.d. packet loss (the lab condition this retry policy is sized for) a # single-shot ping fails at roughly 2% per directed pair, so a 20-pair # strict assert misses with probability ~1 - (0.98)^20 ≈ 33%, which is # below the per-pair loss-math floor but well above the routing-state From 09215db909eae4f1300fb80465a06740a223f8cf Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 12:45:38 +0000 Subject: [PATCH 05/17] Correct control-plane comments that were wrong about the code These comments did not merely use private vocabulary; they described behaviour the code does not have, so a reader who believed them was misled. The control module said the snapshot dispatch always returns nothing and that no query was served from it. It serves about twenty commands. The read handle's module doc said no query reads the handle yet, and that two of the three snapshot cells are published from their own mutators. Nineteen query functions read them today, and all three cells publish from the periodic tick. The publisher paragraph is rewritten rather than trimmed, because its central claim was also wrong: it said the cells are never published by the rx_loop task the handle exists to bypass. The tick is one arm of that task's select, so publishing costs the rx_loop. What the handle removes is the read-side round trip. It now also says a projection is a point-in-time copy, since the entity tables are mutated on the packet path between ticks. The report module's note about encoding arriving later goes too, matching the deletion already made on the branch above. --- src/control/mod.rs | 4 ++-- src/control/read_handle.rs | 46 +++++++++++++++++++++----------------- src/mmp/report.rs | 2 -- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/control/mod.rs b/src/control/mod.rs index 5e770df8..e26de0c1 100644 --- a/src/control/mod.rs +++ b/src/control/mod.rs @@ -78,8 +78,8 @@ where match serde_json::from_str::(line.trim()) { Ok(request) => { // First try to serve the request entirely off-loop from the - // read handle. In R0 this always returns None (no query is - // cut over yet); R1+ adds the per-command snapshot branches. + // read handle. It returns None for any command with no snapshot + // branch, and those fall through to the rx_loop path below. match snapshot_dispatch(&request, &read_handle) { Some(resp) => resp, None => { diff --git a/src/control/read_handle.rs b/src/control/read_handle.rs index 22660f1d..030dc83e 100644 --- a/src/control/read_handle.rs +++ b/src/control/read_handle.rs @@ -2,27 +2,33 @@ //! queries can render off the rx_loop hot path instead of round-tripping the //! mpsc → rx_loop oneshot. //! -//! This is the stable seam of the control read-isolation milestone -//! (TASK-2026-0152, phase R0). The handle bundles the state that is already -//! independently shareable, and grows one `ArcSwap` snapshot cell per phase as -//! each subsystem's read state is published from its natural mutator: +//! The handle bundles the node state that is independently shareable, plus one +//! `ArcSwap` snapshot cell per read subsystem: //! -//! - `context` / `metrics` — already `Arc`-shared (refactor steps B/C). -//! - `stats` (R2) — `ArcSwap`: stats_history dual-ring + the -//! scalar gauges `show_status` needs, published from the tick. -//! - `routing` (R3) — `ArcSwap`: tree / bloom / coord / -//! identity, published from their announce / discovery mutators. -//! - `entities` (R4) — `ArcSwap`: peers / sessions / links / -//! connections / transports, published per-entity with `Vec>` +//! - `context` / `metrics` — already `Arc`-shared. +//! - `stats` — `ArcSwap`: stats_history dual-ring + the scalar +//! gauges `show_status` needs, published from the tick. +//! - `routing` — `ArcSwap`: tree / bloom / coord / identity, +//! published from the tick. +//! - `entities` — `ArcSwap`: peers / sessions / links / +//! connections / transports, published from the tick with `Vec>` //! structural sharing. //! -//! Publisher placement follows the Q1 rules in -//! `design/fast-path-refactoring-r0-read-handle.md`: every snapshot is -//! published at its state's natural mutation site (on-change), never by the -//! contended rx_loop task it is meant to bypass. +//! Publisher placement: all three snapshot cells are published from the +//! periodic tick, which runs as one arm of the rx_loop's `select!`. Publishing +//! therefore costs the rx_loop; what the handle removes is the read-side round +//! trip out to the rx_loop and back, not the cost of publishing. The +//! `publish_routing_snapshot` and `publish_entities_snapshot` doc comments on +//! `Node` carry the reasoning for the two projections that need coherent +//! `&Node` access across subsystems. //! -//! R0 ships only the type and the dispatch seam ([`snapshot_dispatch`]); no -//! query reads the handle yet. Cutover begins in R1. +//! A projection is a point-in-time copy, not a live view. The entity tables in +//! particular are mutated on the packet path between ticks, so a reader sees +//! the state as of the last publish. +//! +//! [`snapshot_dispatch`] is the seam: it serves the commands in its match arms +//! directly from the handle and returns `None` for everything else, so the +//! caller falls back to the mpsc → rx_loop path. use std::sync::Arc; @@ -37,9 +43,9 @@ use super::snapshot::{EntitySnapshot, RoutingSnapshot, StatsSnapshot}; /// Cloneable read-only view of node state for off-loop control serving. /// /// All fields are `Arc` / `ArcSwap` handles, so cloning is cheap and a clone -/// can be held by every accepted control connection. Fields are consumed -/// starting R1 as `show_*` queries cut over to off-loop rendering; until then -/// they are wired but unread. +/// can be held by every accepted control connection. The snapshot cells are +/// read by the `*_from_handle` query functions that [`snapshot_dispatch`] +/// routes to. #[derive(Clone)] pub(crate) struct ControlReadHandle { /// Effectively-immutable node context (config, identity, limits). diff --git a/src/mmp/report.rs b/src/mmp/report.rs index 8d0289f9..463992d9 100644 --- a/src/mmp/report.rs +++ b/src/mmp/report.rs @@ -74,8 +74,6 @@ pub struct ReceiverReport { pub interval_bytes_recv: u32, } -// Encode/decode will be implemented in Step 2. - impl SenderReport { /// Encode to wire format (48 bytes: msg_type + 3 reserved + 44 payload). pub fn encode(&self) -> Vec { From 95a8b21a5f85e3d8a10b5d298620c54fa1f573aa Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 12:05:28 +0000 Subject: [PATCH 06/17] Pin the three dormant peer-machine constants to the config defaults The rekey and liveness constants carried bare literals with no tie to the configuration they were copied from. Each now has a test asserting it against the config default function, so a change to the default reds the build instead of leaving the constant silently stale. The constants stay compile-time values rather than expressions over the default, because that default is an ordinary Default implementation and not a const function, so it cannot appear in a const initialiser. The comments say what these are: placeholders pinned to today's defaults. Nothing reads the configuration to produce them, and wiring that up is separate work. --- src/peer/machine.rs | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 115aeef2..59f524cf 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -88,10 +88,19 @@ const RESEND_BACKOFF: f64 = 2.0; const REKEY_CADENCE_INTERVAL_MS: u64 = 60_000; const REKEY_RESEND_INTERVAL_MS: u64 = 1_000; const REKEY_MAX_RESENDS: u32 = 5; -const REKEY_AFTER_SECS: u64 = 3_600; -const REKEY_AFTER_MESSAGES: u64 = 1_000_000; +// `REKEY_AFTER_SECS`, `REKEY_AFTER_MESSAGES` and `LIVENESS_INTERVAL_MS` below +// are placeholders pinned to today's `RekeyConfig` and `NodeConfig` defaults. +// They are not a wiring to the config: nothing here reads a config value, so +// an operator override is not tracked. They are what the machine falls back to +// until it is wired to config. The tie to the defaults is asserted by +// `rekey_constants_match_the_rekey_config_defaults` and +// `liveness_interval_matches_the_heartbeat_config_default` rather than stated +// in these declarations, because `Default for NodeConfig` is an ordinary impl +// and cannot be called from a `const` initializer. +const REKEY_AFTER_SECS: u64 = 120; +const REKEY_AFTER_MESSAGES: u64 = 65_536; const DRAIN_WINDOW_MS: u64 = 5_000; -const LIVENESS_INTERVAL_MS: u64 = 15_000; +const LIVENESS_INTERVAL_MS: u64 = 10_000; const REKEY_DAMPEN_MS: u64 = 30_000; const CLOSED_BACKOFF_MS: u64 = 5_000; @@ -3386,6 +3395,28 @@ mod tests { .is_err() ); } + + /// `LIVENESS_INTERVAL_MS` stays pinned to `NodeConfig`'s heartbeat default. + /// + /// The expectation is read from the default rather than repeated as a + /// literal, so raising or lowering `heartbeat_interval_secs` without + /// re-pinning the constant reds here instead of drifting unnoticed. + #[test] + fn liveness_interval_matches_the_heartbeat_config_default() { + assert_eq!( + LIVENESS_INTERVAL_MS, + crate::config::NodeConfig::default().heartbeat_interval_secs * 1_000 + ); + } + + /// `REKEY_AFTER_SECS` and `REKEY_AFTER_MESSAGES` stay pinned to + /// `RekeyConfig`'s defaults, read from the impl for the same reason. + #[test] + fn rekey_constants_match_the_rekey_config_defaults() { + let defaults = crate::config::RekeyConfig::default(); + assert_eq!(REKEY_AFTER_SECS, defaults.after_secs); + assert_eq!(REKEY_AFTER_MESSAGES, defaults.after_messages); + } } /// T-SANSIO: the action vocabulary must stay plain, comparable data. From 1e95d34152af0297165dc67622997159e6df3a41 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 12:30:39 +0000 Subject: [PATCH 07/17] Source the drain window from the value that governs the live drain The drain deadline armed at rekey cutover carried its own literal, five seconds, while the session layer's drain window is ten. The constant is now an expression over that limit, so the two cannot disagree. This is a stored-value correction rather than a timing change: nothing fires the armed deadline today, because the timer driver has no arm for it. Two tests come with it. One drives a cutover and asserts the deadline against a literal, so a change to the session-layer limit reds it; that is the drift this exists to catch, and it is measured, not assumed. The other states the sourcing directly and is labelled at the site as a tautology, kept because it says what the declaration means and not because it covers anything. --- src/peer/machine.rs | 65 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 59f524cf..316af9d6 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -99,7 +99,12 @@ const REKEY_MAX_RESENDS: u32 = 5; // and cannot be called from a `const` initializer. const REKEY_AFTER_SECS: u64 = 120; const REKEY_AFTER_MESSAGES: u64 = 65_536; -const DRAIN_WINDOW_MS: u64 = 5_000; +/// Drain-window deadline armed at rekey cutover. Sourced from the value that +/// actually governs the live drain so the two cannot drift; the armed timer +/// is currently stored and never fired (`drive_peer_timers` has no +/// `DrainExpiry` arm), so this is a stored-value correction, not a live +/// timing change. +const DRAIN_WINDOW_MS: u64 = crate::proto::fsp::limits::DRAIN_WINDOW_SECS * 1_000; const LIVENESS_INTERVAL_MS: u64 = 10_000; const REKEY_DAMPEN_MS: u64 = 30_000; const CLOSED_BACKOFF_MS: u64 = 5_000; @@ -3417,6 +3422,64 @@ mod tests { assert_eq!(REKEY_AFTER_SECS, defaults.after_secs); assert_eq!(REKEY_AFTER_MESSAGES, defaults.after_messages); } + + /// A rekey cutover arms the drain timer for the drain window FSP uses. + /// + /// The expected offset is the literal `10_000`: `DRAIN_WINDOW_SECS` in + /// `src/proto/fsp/limits.rs` is 10 seconds, and that is the value this + /// deadline is meant to carry. Writing it out rather than reusing + /// `DRAIN_WINDOW_MS` is what keeps the assertion able to fail; expressed + /// in terms of the constant under test it would move with any re-pointing + /// of that constant and assert nothing. + #[test] + fn drain_expiry_deadline_is_the_configured_drain_window() { + let mut alloc = IndexAllocator::new(); + let id = peer_identity(); + let addr = *id.node_addr(); + let mut m = PeerMachine::new_outbound(LinkId::new(1), id, 0); + m.state = PeerState::Maintaining { + addr, + kind: MaintainKind::Rekey(RekeyPhase::PendingCutover), + }; + m.rekey_our_index = Some(SessionIndex::new(0x2222)); + m.conn.set_our_index(SessionIndex::new(0x1111)); + m.remote_epoch = Some([9u8; 8]); + m.session_established_at_ms = 0; + + let actions = m.step( + PeerEvent::Timeout { + kind: TimerKind::RekeyCadence, + }, + 7_000, + &mut alloc, + ); + + let deadline = actions + .iter() + .find_map(|a| match a { + PeerAction::SetTimer { + kind: TimerKind::DrainExpiry, + at_ms, + } => Some(*at_ms), + _ => None, + }) + .expect("the cutover must arm a DrainExpiry timer"); + assert_eq!(deadline, 7_000 + 10_000); + } + + /// `DRAIN_WINDOW_MS` is the FSP drain limit in milliseconds. + /// + /// This is a tautology as the constant is now declared, and is not + /// coverage: it is an executable statement of where the value comes from. + /// It reds only if a later edit replaces the const expression with a + /// literal that disagrees with the limit. + #[test] + fn drain_window_ms_is_sourced_from_the_fsp_limit() { + assert_eq!( + DRAIN_WINDOW_MS, + crate::proto::fsp::limits::DRAIN_WINDOW_SECS * 1_000 + ); + } } /// T-SANSIO: the action vocabulary must stay plain, comparable data. From 2970bd07eb1021884f53b1bb0add916b82e4a7fa Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 13:08:41 +0000 Subject: [PATCH 08/17] Drop refactor-stage labels from the control-plane comments Eighty-one comment lines named stages of the refactor that moved the control-plane read path off the event loop: R0 through R5, category letters, cut-over steps. Those names existed only in my planning notes, so a reader of the published source could not resolve any of them. Each comment keeps what it says about the code and loses the stage it happened during. Five blocks in the node module are copied from the branch above rather than rewritten here. That branch had already been partly de-jargoned upstream, and its rewrite re-wrapped lines that carry no label at all, so deriving the text independently would have produced something plausible and different, and the merge would have conflicted. The copied text is byte-identical, checked by hash on both sides. Three bare commit hashes are deliberately left alone. None is reachable from any branch, but one sits inside the text above, so removing it would break that identity. --- src/control/queries.rs | 45 +++++++++++++------------ src/control/read_handle.rs | 30 ++++++++--------- src/control/snapshot.rs | 68 ++++++++++++++++++------------------- src/node/mod.rs | 69 +++++++++++++++++++------------------- 4 files changed, 107 insertions(+), 105 deletions(-) diff --git a/src/control/queries.rs b/src/control/queries.rs index 3efd27ec..58c148b8 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -2680,8 +2680,8 @@ mod tests { assert_snapshot("show_stats_history_all_peers", &render_response(resp)); } - /// The five Category-D queries cut over to off-loop serving in R3. Served - /// via `snapshot_dispatch`; coverage asserted in + /// The five derived/routing/cache queries served off-loop via + /// `snapshot_dispatch`; coverage asserted in /// `snapshot_dispatch_serves_category_d_queries` below. const OFF_LOOP_CATEGORY_D: &[&str] = &[ "show_tree", @@ -2691,7 +2691,7 @@ mod tests { "show_identity_cache", ]; - /// The six Category-E queries cut over to off-loop serving in R4. Served via + /// The six per-entity table queries served off-loop via /// `snapshot_dispatch`; coverage asserted in /// `snapshot_dispatch_serves_category_e_queries`. const OFF_LOOP_CATEGORY_E: &[&str] = &[ @@ -2703,7 +2703,7 @@ mod tests { "show_mmp", ]; - /// Milestone-completion contract: every pure-read `show_*` query is served + /// Contract: every pure-read `show_*` query is served /// off-loop via `snapshot_dispatch`, and the rx_loop control path carries no /// `show_*` arm at all — only the mutating COMMAND handlers (`connect` / /// `disconnect`) reach it. This test enumerates the full read surface and @@ -2777,8 +2777,8 @@ mod tests { /// the rx_loop source carries no `queries::dispatch` call and no /// `starts_with("show_")` routing branch. Reads the committed source of /// `src/node/handlers/rx_loop.rs` and asserts both markers are absent. This - /// is the milestone's "remove `show_*` from the data-plane dispatch path" - /// invariant, guarded against regression. + /// guards the "no `show_*` on the data-plane dispatch path" invariant + /// against regression. #[test] fn rx_loop_has_no_show_dispatch() { let src = include_str!("../node/handlers/rx_loop.rs"); @@ -2841,10 +2841,10 @@ mod tests { } } - /// The R1/R2 scalar-and-series queries are served off-loop via + /// The scalar-and-series queries are served off-loop via /// `snapshot_dispatch`; mutations return `None` and take the rx_loop COMMAND /// path. (`show_stats_peers` / `show_stats_history_all_peers`, formerly - /// asserted on-loop here, were cut over in R5 — see + /// asserted on-loop here, are now served off-loop too — see /// `snapshot_dispatch_serves_every_read_query` for the full read surface.) #[test] fn snapshot_dispatch_serves_scalar_and_series_queries() { @@ -2879,7 +2879,7 @@ mod tests { "show_stats_all_history", Some(json!({ "window": "10s", "granularity": "1s" })), ), - // R3 Category-D cutover. + // Derived/routing/cache queries, served off-loop. ("show_tree", None), ("show_bloom", None), ("show_cache", None), @@ -2901,7 +2901,7 @@ mod tests { } } - /// R5 cutover + byte-identity: after a `record_stats_history()` tick the + /// Byte-identity: after a `record_stats_history()` tick the /// off-loop `show_acl` / `show_stats_peers` / `show_stats_history_all_peers` /// renders each equal their on-loop oracle byte-for-byte, and all three are /// served off-loop via `snapshot_dispatch`. @@ -2987,7 +2987,7 @@ mod tests { assert_eq!(snap.connection_count, node.connection_count()); assert_eq!(snap.estimated_mesh_size, node.estimated_mesh_size()); assert_eq!(snap.effective_ipv6_mtu, node.effective_ipv6_mtu()); - // R5: the ACL status projection matches the node's live ACL status. + // The ACL status projection matches the node's live ACL status. assert_eq!(snap.acl_status, node.peer_acl_status()); // Off-loop render must equal the on-loop render byte-for-byte. @@ -2999,9 +2999,9 @@ mod tests { ); } - /// The five Category-D queries are served off-loop via `snapshot_dispatch` - /// (return `Some` with status ok); everything not cut over stays on the - /// rx_loop path (`None`). + /// The five derived/routing/cache queries are served off-loop via + /// `snapshot_dispatch` (return `Some` with status ok); everything not cut + /// over stays on the rx_loop path (`None`). #[test] fn snapshot_dispatch_serves_category_d_queries() { use super::super::protocol::Request; @@ -3024,7 +3024,7 @@ mod tests { } // Mutations take the rx_loop COMMAND path. (Every read query, including - // the per-peer stats-series queries, is served off-loop as of R5.) + // the per-peer stats-series queries, is served off-loop.) for cmd in ["connect", "disconnect"] { assert!( snapshot_dispatch(&req(cmd), &handle).is_none(), @@ -3034,7 +3034,7 @@ mod tests { } /// The tick-published `RoutingSnapshot` reflects node state, and each - /// off-loop Category-D render equals its on-loop render byte-for-byte + /// off-loop routing render equals its on-loop render byte-for-byte /// (modulo the volatile-key redaction the wire-schema tests already apply). #[test] fn routing_snapshot_matches_on_loop_after_tick() { @@ -3093,10 +3093,11 @@ mod tests { ); } - // ---- R4 Category-E coverage ------------------------------------------ + // ---- per-entity table coverage --------------------------------------- - /// The six Category-E queries are served off-loop via `snapshot_dispatch` - /// (return `Some` with status ok); mutations take the rx_loop COMMAND path. + /// The six per-entity table queries are served off-loop via + /// `snapshot_dispatch` (return `Some` with status ok); mutations take the + /// rx_loop COMMAND path. #[test] fn snapshot_dispatch_serves_category_e_queries() { use super::super::protocol::Request; @@ -3119,7 +3120,7 @@ mod tests { } // Mutations take the rx_loop COMMAND path. (Every read query is served - // off-loop as of R5.) + // off-loop.) for cmd in ["connect", "disconnect"] { assert!( snapshot_dispatch(&req(cmd), &handle).is_none(), @@ -3129,7 +3130,7 @@ mod tests { } /// Freshness + fidelity: after a `record_stats_history()` tick (the entity - /// publisher site) each off-loop Category-E render equals its on-loop render + /// publisher site) each off-loop per-entity render equals its on-loop render /// byte-for-byte, and the seeded snapshot is empty before the first tick. #[test] fn entity_snapshot_matches_on_loop_after_tick() { @@ -3179,7 +3180,7 @@ mod tests { ); } - /// Structural sharing (the R4 umbrella mandate): a republish in which only + /// Structural sharing: a republish in which only /// one row changed re-allocates only that one `Arc` — every unchanged /// row is reused by pointer (`Arc::ptr_eq`). Exercises /// [`reconcile_rows`](super::super::snapshot::reconcile_rows), the diff --git a/src/control/read_handle.rs b/src/control/read_handle.rs index 030dc83e..acca38eb 100644 --- a/src/control/read_handle.rs +++ b/src/control/read_handle.rs @@ -53,14 +53,14 @@ pub(crate) struct ControlReadHandle { /// Metrics registry (counters / gauges) for `show_stats_*`. metrics: Arc, /// stats_history dual-ring read copy + the scalar gauges/counts - /// `show_status` needs, published from the tick (R2, Q1-b). + /// `show_status` needs, published from the tick. stats: Arc>, - /// Category-D derived/routing/cache read view (tree / bloom / coord / - /// identity + F-queue scalars), published from the tick (R3). + /// Derived/routing/cache read view (tree / bloom / coord / + /// identity + F-queue scalars), published from the tick. routing: Arc>, - /// Category-E per-entity table read view (peers / sessions / links / + /// Per-entity table read view (peers / sessions / links / /// connections / transports + mmp), published from the tick with - /// `Vec>` structural sharing (R4). + /// `Vec>` structural sharing. entities: Arc>, } @@ -96,19 +96,19 @@ impl ControlReadHandle { } /// Load the latest published stats snapshot (the freshest available by - /// construction; no IO_TIMEOUT staleness gate, per Q1-e). + /// construction; no IO_TIMEOUT staleness gate). pub(crate) fn stats(&self) -> arc_swap::Guard> { self.stats.load() } - /// Load the latest published Category-D routing snapshot (freshest - /// available by construction; no staleness gate, per Q1-e). + /// Load the latest published routing snapshot (freshest + /// available by construction; no staleness gate). pub(crate) fn routing(&self) -> arc_swap::Guard> { self.routing.load() } - /// Load the latest published Category-E entity snapshot (freshest available - /// by construction; no staleness gate, per Q1-e). + /// Load the latest published entity snapshot (freshest available + /// by construction; no staleness gate). pub(crate) fn entities(&self) -> arc_swap::Guard> { self.entities.load() } @@ -133,18 +133,18 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - )), "show_stats_list" => Some(Response::ok(queries::show_stats_list())), "show_metrics" => Some(Response::ok(queries::show_metrics_from_handle(handle))), - // R5: peer-ACL status, served from the tick-published `StatsSnapshot`. + // Peer-ACL status, served from the tick-published `StatsSnapshot`. // The ACL is an `arc_swap::ArcSwap` reloaded only on the tick; // its status projection is captured at the same tick. "show_acl" => Some(Response::ok(queries::show_acl_from_handle(handle))), - // R2: served from the tick-published `StatsSnapshot` (rings + scalar + // Served from the tick-published `StatsSnapshot` (rings + scalar // gauges/counts). `show_status` and the two node-level/per-peer series // queries carry enough data in the snapshot to render faithfully // off-loop, including the parameterized series selectors (the snapshot // holds the full rings, so any metric / window / granularity is // satisfiable). // - // R5 closes out the per-peer stats queries: `show_stats_peers` and + // The per-peer stats queries: `show_stats_peers` and // `show_stats_history_all_peers` now read the snapshot's per-peer // `peer_meta` (live `is_active`, resolved npub / display name, captured // at publish time) joined against the `history` rings, so they no longer @@ -163,7 +163,7 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - handle, request.params.as_ref(), )), - // R3: served from the tick-published `RoutingSnapshot` (tree / bloom / + // Served from the tick-published `RoutingSnapshot` (tree / bloom / // coord cache / identity cache + F-queue scalars). Display names are // resolved at publish time, so these render entirely off-loop. The // counter-family `stats` blocks come from the `MetricsRegistry` (also @@ -175,7 +175,7 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - "show_identity_cache" => Some(Response::ok(queries::show_identity_cache_from_handle( handle, ))), - // R4: served from the tick-published `EntitySnapshot` (per-entity + // Served from the tick-published `EntitySnapshot` (per-entity // `Vec>` tables with structural sharing). Display names, // tree-relationship flags, and Nostr-traversal state are resolved at // publish time, so these render entirely off-loop. All six are diff --git a/src/control/snapshot.rs b/src/control/snapshot.rs index c7132202..042f79b3 100644 --- a/src/control/snapshot.rs +++ b/src/control/snapshot.rs @@ -12,10 +12,10 @@ //! peer / session / link / connection / transport counts), plus //! `peer_aliases` (effectively immutable after construction). //! -//! The snapshot holds *data*, not rendered `Response` envelopes (Q1-d): +//! The snapshot holds *data*, not rendered `Response` envelopes: //! rendering happens in the control task off the rx_loop. Staleness is bounded //! by the tick interval and is never staler than the underlying data, which -//! also advances only on the tick (Q1-b). +//! also advances only on the tick. use std::collections::HashMap; use std::sync::Arc; @@ -27,7 +27,7 @@ use crate::node::stats_history::StatsHistory; use crate::upper::tun::TunState; /// Read-only snapshot of the stats-history rings plus the scalar gauges and -/// counts `show_status` reports. Published from the tick (Q1-b). +/// counts `show_status` reports. Published from the tick. #[derive(Clone)] pub(crate) struct StatsSnapshot { /// Cloned read copy of the history rings (the dual-ring read side). @@ -65,14 +65,14 @@ pub(crate) struct StatsSnapshot { pub peer_aliases: Arc>, /// Loaded peer-ACL status (`show_acl`). The ACL itself is an /// `arc_swap::ArcSwap` mutated only by the tick's `reload_peer_acl`; - /// the human-readable status is a cheap projection of it (R5). + /// the human-readable status is a cheap projection of it. pub acl_status: PeerAclStatus, /// Per-stats-history-peer metadata resolved against the live peer/session /// tables and host map at publish time (`show_stats_peers` / /// `show_stats_history_all_peers`), keyed by `NodeAddr`. The lifecycle /// timestamps and per-peer metric rings stay in `history`; this map carries /// only the cross-subsystem fields a renderer can't derive from the rings - /// alone (`is_active`, resolved `npub`, resolved `display_name`) (R5). + /// alone (`is_active`, resolved `npub`, resolved `display_name`). pub peer_meta: Arc>, } @@ -137,10 +137,10 @@ fn empty_acl_status() -> PeerAclStatus { } // ===================================================================== -// RoutingSnapshot (R3 — Category-D derived/routing/cache read view) +// RoutingSnapshot (derived/routing/cache read view) // ===================================================================== -/// Read-only snapshot of the Category-D derived/routing/cache subsystems that +/// Read-only snapshot of the derived/routing/cache subsystems that /// the pure-snapshot `show_tree` / `show_bloom` / `show_cache` / `show_routing` /// / `show_identity_cache` queries render. Published via `ArcSwap`. /// @@ -149,22 +149,22 @@ fn empty_acl_status() -> PeerAclStatus { /// cohesive routing view holding the four subsystems (tree / bloom / coord /// cache / identity cache) plus the F-queue summary scalars. /// -/// **Publisher placement (Q1).** The four subsystems mutate at many scattered +/// **Publisher placement.** The four subsystems mutate at many scattered /// handler sites (28 `coord_cache_mut` call sites, 16 `tree_state_mut`, ~32 /// identity-cache touches), and every projected row needs a *display name* -/// resolved against the live peer/session tables and host map — Category-E +/// resolved against the live peer/session tables and host map — per-entity /// state reachable only with `&Node`. Wiring an on-change `publish_*` at each /// mutation site would be large, error-prone surgery, and each call would still /// need `&Node` to resolve names across subsystem boundaries. So this snapshot -/// is published from the **tick** (Q1-b acceptable-at-mutator / the documented -/// interim the spec permits, mirroring R2's stats publish): the tick is the one -/// site with coherent `&Node` access to resolve all display names together. A +/// is published from the **tick**, the same placement the stats snapshot +/// above uses: the tick is the one site with coherent `&Node` access to +/// resolve all display names together. A /// single combined cell is the natural shape because there is exactly one /// publisher — the multi-mutator "rebuild the whole snapshot N times" hazard -/// that Q1-c warns against does not arise. +/// does not arise. /// /// The snapshot holds *data* (typed rows + scalars), not rendered `Response` -/// envelopes (Q1-d); rendering happens off the rx_loop in the control task. The +/// envelopes; rendering happens off the rx_loop in the control task. The /// counter-family `stats` blocks the queries also emit come from the /// `MetricsRegistry` (already `Arc`-shared in the handle) at render time, not /// from this snapshot. @@ -173,9 +173,9 @@ fn empty_acl_status() -> PeerAclStatus { /// the captured absolute timestamps, so the rendered age stays fresh relative /// to the read, exactly as the on-loop queries computed it. /// -/// Forward-compat: when step 5 structurally extracts the Category-D subsystems -/// into typed types, these projections become thin views over them without -/// changing the read-handle interface or this publisher placement. +/// Forward-compat: if the derived/routing/cache subsystems are later +/// extracted into typed types, these projections become thin views over them +/// without changing the read-handle interface or this publisher placement. #[derive(Clone)] pub(crate) struct RoutingSnapshot { /// Spanning-tree read view (`show_tree`). @@ -396,10 +396,10 @@ pub(crate) struct IdentityRow { } // ===================================================================== -// EntitySnapshot (R4 — Category-E per-entity table read views) +// EntitySnapshot (per-entity table read views) // ===================================================================== -/// Read-only snapshot of the Category-E per-entity tables that the +/// Read-only snapshot of the per-entity tables that the /// pure-snapshot `show_peers` / `show_sessions` / `show_links` / /// `show_connections` / `show_transports` / `show_mmp` queries render. /// Published via `ArcSwap`. @@ -407,7 +407,7 @@ pub(crate) struct IdentityRow { /// This is the `entities` cell: peers / sessions / links / connections / /// transports, published per-entity with `Vec>` structural sharing. /// -/// **Structural sharing (the umbrella mandate).** Every entity table is a +/// **Structural sharing.** Every entity table is a /// `Vec>`, so a republish in which only one row changed re-allocates /// only that one `Arc` — the unchanged rows are reused by pointer from the /// previous snapshot (`Arc::ptr_eq`-stable). The publisher diffs each freshly @@ -415,10 +415,11 @@ pub(crate) struct IdentityRow { /// keeps the old `Arc` when they are equal. A clone of the snapshot for each /// accepted control connection is then a vector of cheap pointer clones, not a /// deep table copy. This is what keeps the per-tick publish cost off the hot -/// path at scale, as the umbrella requires for R4. +/// path at scale. /// -/// **Publisher placement (Q1).** Like R3, this is published from the **tick**, -/// not per-mutator. Two reasons, both stronger than for R3: +/// **Publisher placement.** Like the routing snapshot, this is published from +/// the **tick**, not per-mutator. Two reasons, both stronger here than for the +/// routing snapshot: /// /// 1. Every projected row needs a *display name* resolved against the live /// peer/session tables and host map (`&Node`), and `show_peers` additionally @@ -429,23 +430,22 @@ pub(crate) struct IdentityRow { /// metrics, `last_seen`, noise counters, replay/decrypt counters) are /// mutated continuously on the **data plane / rx_loop**, not at the discrete /// peer/session/link lifecycle mutators. Per-lifecycle-mutator publication -/// (Q1-a) would therefore not even capture freshness for those fields; the +/// would therefore not even capture freshness for those fields; the /// tick is the natural cadence at which this read view advances. /// -/// The diff-and-reuse therefore satisfies the structural-sharing goal the -/// umbrella mandates (only changed rows re-allocate) while keeping a single -/// coherent `&Node` publisher — the "no monolithic per-tick *re-allocation* of -/// every row" warning is honored because unchanged rows are reused, not rebuilt. -/// This is the documented acceptable interim (the spec's tick-publish-with- -/// Arc-reuse fallback), consistent with R3. +/// The diff-and-reuse therefore satisfies the structural-sharing goal (only +/// changed rows re-allocate) while keeping a single coherent `&Node` +/// publisher: there is no monolithic per-tick *re-allocation* of every row, +/// because unchanged rows are reused rather than rebuilt. This is the same +/// tick publish placement the routing snapshot uses, for the same reason. /// -/// The snapshot holds typed rows (Q1-d data, not rendered `Response` +/// The snapshot holds typed rows (data, not rendered `Response` /// envelopes). Time-relative fields (`idle_ms`) are derived at render time from /// captured absolute timestamps, so the rendered age stays fresh relative to /// the read, exactly as the on-loop queries computed it. /// -/// Forward-compat: step 10 later extracts the session table into a typed -/// `(transport_id, our_index)`-indexed type; these projections then become thin +/// Forward-compat: if the session table is later extracted into a typed +/// `(transport_id, our_index)`-indexed type, these projections become thin /// views over it without changing the read-handle interface or this publisher /// placement. #[derive(Clone)] @@ -733,7 +733,7 @@ pub(crate) struct MmpSessionRow { /// one, preserving structural sharing: an `Arc` from `prev` is reused /// (kept by pointer) whenever a new row matches an old row by identity `key` /// **and** compares equal by value, so only changed/new rows allocate a fresh -/// `Arc`. This is the `Vec>` discipline the R4 umbrella mandates — a +/// `Arc`. This is the `Vec>` structural-sharing discipline — a /// single-row change re-allocates one row, not the whole table, keeping the /// per-tick publish cost off the hot path at scale. /// diff --git a/src/node/mod.rs b/src/node/mod.rs index 8e9f8b78..e5efad54 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -440,19 +440,19 @@ pub struct Node { /// live mutable `stats_history` above stays on the tick. stats_snapshot: std::sync::Arc>, - /// Read-side snapshot of the Category-D derived/routing/cache subsystems + /// Read-side snapshot of the derived/routing/cache subsystems /// (tree / bloom / coord cache / identity cache + F-queue scalars) that the /// `show_tree` / `show_bloom` / `show_cache` / `show_routing` / /// `show_identity_cache` queries render off the rx_loop. Published from the - /// tick (see [`Self::publish_routing_snapshot`] for the Q1 rationale). + /// tick (see [`Self::publish_routing_snapshot`] for the rationale). routing_snapshot: std::sync::Arc>, - /// Read-side snapshot of the Category-E per-entity tables (peers / sessions + /// Read-side snapshot of the per-entity tables (peers / sessions /// / links / connections / transports + mmp) that the `show_peers` / /// `show_sessions` / `show_links` / `show_connections` / `show_transports` /// / `show_mmp` queries render off the rx_loop. Published from the tick with /// `Vec>` structural sharing (unchanged rows reused by pointer); - /// see [`Self::publish_entities_snapshot`] for the Q1 rationale. + /// see [`Self::publish_entities_snapshot`] for the rationale. entities_snapshot: std::sync::Arc>, // === TUN Interface === @@ -1596,14 +1596,14 @@ impl Node { self.stats_history.tick(now, &snap, &peer_snaps); - // Publish the read-side snapshot (R2 dual-ring, Q1-b). The tick is the + // Publish the read copy of the dual-ring stats snapshot. The tick is the // natural and sole mutator of `stats_history`, so publishing here can // never produce false staleness: the snapshot and the underlying data - // advance together. This is data, not a rendered response (Q1-d), and - // it is published only here, not in a monolithic per-tick rebuild of - // every query (Q1-c). It also is not gated behind any slow I/O on the - // tick the way the abandoned 2edc8a1 republish was. - // Per-stats-history-peer metadata (R5). `show_stats_peers` / + // advance together. What is published is data, not a rendered response, + // and it is published only here, rather than as a monolithic per-tick + // rebuild of every query's result. It also is not gated behind any slow + // I/O on the tick the way the abandoned 2edc8a1 republish was. + // Per-stats-history-peer metadata. `show_stats_peers` / // `show_stats_history_all_peers` need each tracked peer's live // membership (`is_active`), resolved npub, and display name — all // cross-subsystem reads against the live peer table and host map, @@ -1671,11 +1671,11 @@ impl Node { }; self.stats_snapshot.store(std::sync::Arc::new(snapshot)); - // Publish the Category-D routing read view alongside the stats + // Publish the routing read view alongside the stats // snapshot, from the same tick. self.publish_routing_snapshot(); - // Publish the Category-E per-entity read view from the same tick, with + // Publish the per-entity read view from the same tick, with // `Vec>` structural sharing against the previous snapshot. self.publish_entities_snapshot(); } @@ -1702,26 +1702,26 @@ impl Node { None } - /// Project the Category-D derived/routing/cache state into a + /// Project the derived/routing/cache state into a /// [`RoutingSnapshot`](crate::control::snapshot::RoutingSnapshot) and /// publish it via `ArcSwap`, so `show_tree` / `show_bloom` / `show_cache` /// / `show_routing` / `show_identity_cache` render off the rx_loop. /// - /// **Q1 publisher placement.** The four projected subsystems (tree / bloom + /// **Publisher placement.** The four projected subsystems (tree / bloom /// / coord cache / identity cache) mutate at dozens of scattered handler /// sites, and every projected row carries a *display name* resolved against /// the live peer/session tables and host map — state reachable only with - /// `&Node`. Per-mutator on-change publication (Q1-a) would therefore be - /// large, error-prone surgery, and each call would still need `&Node` to - /// resolve names across subsystem boundaries. So this projection is - /// published from the tick — the documented acceptable interim (the spec's - /// "publish from the tick" allowance, mirroring R2's stats publish). The - /// tick is the one site with coherent `&Node` access to resolve every - /// display name together. A single combined cell is the natural shape - /// because there is exactly one publisher, so the multi-mutator - /// whole-snapshot-rebuild hazard Q1-c warns against does not arise. + /// `&Node`. Publishing on change from each individual mutator would + /// therefore be large, error-prone surgery, and each call would still need + /// `&Node` to resolve names across subsystem boundaries. So this projection + /// is published from the tick instead, the same placement the stats + /// snapshot above uses. The tick is the one site with coherent `&Node` + /// access to resolve every display name together. A single combined cell is + /// the natural shape because there is exactly one publisher, so the + /// whole-snapshot-rebuild hazard that afflicts multi-mutator designs does + /// not arise. /// - /// The snapshot holds typed rows + scalars (Q1-d data, not rendered + /// The snapshot holds typed rows + scalars (data, not rendered /// responses); the counter-family `stats` blocks the queries also emit are /// served from the `MetricsRegistry` (already `Arc`-shared) at render time. fn publish_routing_snapshot(&self) { @@ -1903,33 +1903,34 @@ impl Node { self.routing_snapshot.store(std::sync::Arc::new(snapshot)); } - /// Project the Category-E per-entity tables (peers / sessions / links / + /// Project the per-entity tables (peers / sessions / links / /// connections / transports + mmp) into an /// [`EntitySnapshot`](crate::control::snapshot::EntitySnapshot) and publish /// it via `ArcSwap`, so `show_peers` / `show_sessions` / `show_links` / /// `show_connections` / `show_transports` / `show_mmp` render off the /// rx_loop. /// - /// **Q1 publisher placement (tick, like R3).** Every projected row needs a - /// display name resolved against the live peer/session tables and host map + /// **Publisher placement (from the tick, as with the routing snapshot + /// above).** Every projected row needs a display name resolved against the + /// live peer/session tables and host map /// (`&Node`); `show_peers` additionally needs the live tree state to derive /// `is_parent` / `is_child` plus the Nostr-discovery failure-state map — /// cross-subsystem reads available only with `&Node`. And most fields /// (link/session traffic counters, MMP metrics, `last_seen`, noise counters) /// mutate continuously on the data plane, not at the discrete entity - /// lifecycle mutators, so per-lifecycle-mutator publication (Q1-a) would not - /// capture their freshness anyway. The tick is the natural cadence with - /// coherent `&Node` access. + /// lifecycle mutators, so publishing on change from each lifecycle mutator + /// would not capture their freshness anyway. The tick is the natural + /// cadence with coherent `&Node` access. /// - /// **Structural sharing (the R4 umbrella mandate).** Each table is a - /// `Vec>`. The freshly-projected rows are reconciled against the + /// **Structural sharing.** Each table is a `Vec>`. + /// The freshly-projected rows are reconciled against the /// previously published snapshot via /// [`reconcile_rows`](crate::control::snapshot::reconcile_rows): a row's /// `Arc` is reused (kept by pointer) whenever it matches the prior row by /// identity and compares equal by value, so a tick in which only one /// peer/session changed re-allocates only that one row, not the whole table. - /// This is what keeps the publish cost off the hot path at scale (the exact - /// thing the umbrella warns a naive per-tick rebuild would violate). + /// This is what keeps the publish cost off the hot path at scale, which a + /// naive whole-table rebuild on every tick would not. fn publish_entities_snapshot(&self) { use crate::control::snapshot as snap; From 4e73cb76c2eeabb9dd8669607f4db59e9ead6894 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 13:12:16 +0000 Subject: [PATCH 09/17] Correct the dispatch doc, which was wrong about what it serves The doc on the snapshot dispatch said the queries it serves read only the node context and the metrics registry, and that anything parameterized takes the slow path back through the event loop. Both are false. Those queries read all five bundled cells, counted across the query module, and the stats-history query is parameterized and served here. The distinction that actually decides it is whether a query needs live node state the snapshot does not carry. The doc now says that, and points at the two modules that gather the host facts rather than naming them loosely. Found while sweeping stage labels: stripping the label from this block would have left a narrower claim that was still false, so it belongs here rather than in that sweep. --- src/control/read_handle.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/control/read_handle.rs b/src/control/read_handle.rs index acca38eb..0be64c30 100644 --- a/src/control/read_handle.rs +++ b/src/control/read_handle.rs @@ -116,14 +116,16 @@ impl ControlReadHandle { /// Attempt to serve a request entirely from the read handle, off the rx_loop. /// -/// Returns `Some(response)` when the command is a pure-snapshot query that has -/// been cut over to off-loop rendering, or `None` when it must take the -/// mpsc → rx_loop path (parameterized queries, mutations, and any query not -/// yet cut over). +/// Returns `Some(response)` when the command can be rendered from the bundled +/// snapshot cells, or `None` when it must take the mpsc → rx_loop path. /// -/// Cutover queries (R1) read only `NodeContext` / `MetricsRegistry` (the state -/// the read handle already bundles) plus host-OS facts (`/proc`, nftables), so -/// they render entirely in the control task without touching `Node`. +/// The queries served here read any of the cells the handle bundles — +/// `context`, `metrics`, `stats`, `routing`, `entities` — plus host-OS facts +/// gathered in [`super::listening`] and [`super::firewall_state`], so they +/// render in the control task without touching `Node`. Taking a parameter is +/// not what decides it: `show_stats_history` is parameterized and is served +/// here. What falls back is a query needing live `Node` state the snapshot does +/// not carry, and every mutation. pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) -> Option { use crate::control::queries; From 9b977f8359c9f59f72e9fadd9af0159183833761 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 14:28:28 +0000 Subject: [PATCH 10/17] Gate every source comment reference so the sweep cannot silently undo A hand-run search closes the population that exists the day it runs. This closes it for every commit after. Three checks over the committed tree: no local item identifier anywhere in the repository, no source comment citing a document that is not in the tree, and no refactor-stage vocabulary in the source. The identifier check is repository-wide rather than source-only, because the packaging and workflow files are at least as public as the source, and one of them installs to users' machines. The vocabulary check shares its pattern verbatim with the sweep that produced the clean tree, so the two cannot drift apart. Three exit states, not two. Zero means every reference resolves, one means some do not and every offender is printed, and two means the check could not look at all: not in a work tree, wrong directory, or an empty pathspec. A search that found nothing because it searched the wrong place returns the same empty result as a healthy tree, so that case is an error rather than a pass. The coverage gaps that remain are listed in the script itself. --- .github/workflows/ci.yml | 2 + testing/check-comment-refs.sh | 181 ++++++++++++++++++++++++++++++++++ testing/ci-local.sh | 13 +++ 3 files changed, 196 insertions(+) create mode 100755 testing/check-comment-refs.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 711b5dcc..939b80b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,6 +69,8 @@ jobs: run: bash testing/check-image-scoping.sh - name: Check every action is pinned to a commit SHA run: bash testing/check-action-pins.sh + - name: Check every source comment resolves in-repo + run: bash testing/check-comment-refs.sh # Hermetic: synthetic ping functions, no containers, ~45s. Lives beside # the other two so both runners gate on it identically — putting it in # only one would create exactly the drift check-ci-parity.sh exists to diff --git a/testing/check-comment-refs.sh b/testing/check-comment-refs.sh new file mode 100755 index 00000000..118e6560 --- /dev/null +++ b/testing/check-comment-refs.sh @@ -0,0 +1,181 @@ +#!/bin/bash +# ── Source comment reference guard ────────────────────────────────────────── +# Every reference a source comment makes must resolve for a reader who has +# only this repository. +# +# A comment that names a private planning artifact — by identifier, by file +# path, or in programme vocabulary that has no in-repo referent — is dead text +# to everyone outside the workspace that holds the artifact, and it publishes +# the existence and shape of that workspace to everyone else. A hand-run grep +# closes the population that exists on the day it runs; this closes it for +# every commit after. +# +# Three checks, each a `git grep` at HEAD rather than over the working tree, +# because the thing being gated is what is committed: +# +# 1. identifiers, repo-wide. A local item identifier anywhere in the tree. +# Repo-wide because packaging/, testing/ and the workflow files are as +# public as src/ — more so in the packaging case, which ships to users. +# 2. document paths, scoped to src/. A comment under src/ citing an *.md +# path that does not exist in the tree at the checked commit. This is a +# resolvability rule rather than a denylist, so it catches private +# documents nobody has thought of. Scoped to src/ because resolving the +# whole tree's relative documentation links against the repository root +# is a different checker with its own population to triage first. +# 3. programme vocabulary, scoped to src/. Phase, rung, milestone and step +# labels that name a plan a reader cannot open. The pattern is shared +# verbatim with the sweep that produced today's clean tree, so the two +# cannot drift apart. +# +# Known coverage gaps, recorded rather than discovered: +# - a bare "milestone" in some other phrasing (the narrow alternatives keep +# the Tor bootstrap loop in src/transport/tor/mod.rs out of check 3); +# - the CATEGORY_D / CATEGORY_E code identifiers and test names, which +# check 3's case-sensitive Category-[A-Z] deliberately does not match; +# - programme vocabulary, or a document path, outside src/; +# - a reference to a private artifact made in free prose with no marker at +# all, at any scope: no check here matches it; +# - a pathspec typo introduced after this file lands. git grep returns 1 +# with empty output for a pathspec that matches nothing, which is +# indistinguishable from health. The non-empty-population assertion below +# narrows that for the src/-scoped checks and closes nothing for check 1. +# +# Exit 0 = every reference resolves. Exit 1 = one or more do not; every hit is +# printed, not just the first. Exit 2 = the check could not look (not in a work +# tree, wrong directory, empty pathspec, or a git-level failure); never treated +# as a pass. +# ───────────────────────────────────────────────────────────────────────────── +set -uo pipefail + +# A pathspec is resolved against the current directory even when a rev is +# supplied, and a run from the wrong directory returns rc 1 with empty output +# on every check — the same shape as a clean tree. Assert the directory, then +# assert the post-condition rather than trusting the cd: `cd ""` succeeds and +# does not move, so a one-line form silently leaves the script where it began +# whenever the command substitution comes back empty. +ROOT=$(git rev-parse --show-toplevel 2>/dev/null) \ + || { echo "check-comment-refs: not inside a git work tree" >&2; exit 2; } +[[ -n "$ROOT" ]] \ + || { echo "check-comment-refs: empty work-tree root" >&2; exit 2; } +cd "$ROOT" || exit 2 +[[ -z "$(git rev-parse --show-prefix)" ]] \ + || { echo "check-comment-refs: not at the work-tree root" >&2; exit 2; } + +# The src/ pathspec must select something. wc -l is deliberate where the rest +# of this script preserves exit statuses: it always exits 0, so the decision is +# made on n and never on a status, and a failing git ls-tree yields n=0 and +# fires the assertion. This covers checks 2 and 3 only; check 1's pathspec is +# "." and stays non-empty from anywhere, so its wrong-directory case is covered +# by the --show-prefix assertion above and its mistyped-pathspec case by +# nothing. +n=$(git ls-tree -r --name-only HEAD -- src/ | wc -l) +(( n > 0 )) || { echo "check-comment-refs: pathspec src/ matched no files" >&2; exit 2; } + +# Deliberately a superset of the sweep's own acceptance regex: the year group +# is optional, so the three-digit form is caught as well, and the separator is +# optional so underscores and spaces are caught alongside hyphens. Narrowing +# this is how a whole class goes unguarded while every break-check still +# passes. +ID_PAT='(TASK|ISSUE|IDEA|QUICK|RECUR)[-_ ]?(20[0-9]{2}[-_ ])?[0-9]{3,4}' + +# Verbatim from the sweep's enumerating pattern. Do not edit one without the +# other. If check 3's scope is ever widened beyond src/, this text matches +# itself through six of its alternatives and the widening must add +# ':(exclude)testing/check-comment-refs.sh' — never an exclusion of testing/, +# which holds real check-1 hits a directory-wide exclusion would drop. +VOCAB_PAT='\b[RQ][0-9]\b|Category-[A-Z]|\bumbrella\b|refactor steps?|\bpre-scopes\b|R0 stub|read-isolation|cut over yet|Cutover begins|in Step [0-9]|(the|this) milestone|Milestone-' + +MD_PAT='[A-Za-z0-9_./-]+\.md\b' + +FAILED=0 + +# ── Check 1: local item identifiers, repo-wide ────────────────────────────── +# The capture preserves git grep's status instead of flattening it with +# `|| true`: a legitimate no-hit run exits 1, but so would a malformed +# pathspec magic, an unresolvable rev or a bad regex exit 128, and collapsing +# both into "clean" is the single likeliest way this script reports green +# while checking nothing. The hit decision is made on the output; the +# could-not-look decision is made on the status. +rc=0 +out=$(git grep -nEI "$ID_PAT" HEAD -- .) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 1 grep failed (rc=$rc)" >&2 + exit 2 +fi +if [[ -n "$out" ]]; then + printf '%s\n' "$out" \ + | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a local work item: |' + FAILED=1 +fi + +# ── Check 2: document paths under src/ must resolve in-repo ───────────────── +rc=0 +md_lines=$(git grep -nI '\.md' HEAD -- src/) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 2 grep failed (rc=$rc)" >&2 + exit 2 +fi + +# Drop any line carrying a URL before extracting a token from it. Keying the +# skip on the extracted token cannot work: ':' is outside the token character +# class, so the scheme is stripped first and what survives does not begin with +# "http". Inert on today's tree, and kept so a future URL citation does not red. +cited=$(printf '%s\n' "$md_lines" | grep -vE 'https?://') + +declare -A CITES=() +while IFS= read -r line; do + [[ -n "$line" ]] || continue + loc=${line#HEAD:} + loc=$(printf '%s' "$loc" | sed -E 's|^([^:]*):([0-9]+):.*|\1:\2|') + content=$(printf '%s' "$line" | sed -E 's|^HEAD:[^:]*:[0-9]+:||') + while IFS= read -r tok; do + [[ -n "$tok" ]] || continue + CITES["$tok"]+="$loc " + done < <(printf '%s\n' "$content" | grep -oE "$MD_PAT") +done <<< "$cited" + +if (( ${#CITES[@]} > 0 )); then + while IFS= read -r tok; do + [[ -n "$tok" ]] || continue + lrc=0 + lsout=$(git ls-tree HEAD -- "$tok" 2>/dev/null) || lrc=$? + # A '../'-prefixed or absolute token makes git ls-tree exit 128 with + # "is outside repository". That is neither a hit nor clean: the check + # could not look, and reporting it as a hit would red a legitimate + # citation while reporting it as clean would hide one. + if (( lrc != 0 )); then + echo "check-comment-refs: could not resolve '$tok' (git ls-tree rc=$lrc)" >&2 + exit 2 + fi + if [[ -z "$lsout" ]]; then + for loc in ${CITES["$tok"]}; do + printf '%s: cites %s, which does not exist in this repository\n' \ + "$loc" "$tok" + done + FAILED=1 + fi + done < <(printf '%s\n' "${!CITES[@]}" | sort) +fi + +# ── Check 3: programme vocabulary under src/ ──────────────────────────────── +rc=0 +out=$(git grep -nEI "$VOCAB_PAT" HEAD -- src/) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 3 grep failed (rc=$rc)" >&2 + exit 2 +fi +if [[ -n "$out" ]]; then + printf '%s\n' "$out" \ + | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a plan this repository does not carry: |' + FAILED=1 +fi + +if (( FAILED != 0 )); then + echo "" >&2 + echo "check-comment-refs: a source comment references something a reader holding" >&2 + echo "only this repository cannot resolve. Rewrite the comment to say what the" >&2 + echo "code does, citing nothing outside the tree." >&2 + exit 1 +fi + +exit 0 diff --git a/testing/ci-local.sh b/testing/ci-local.sh index f3df3ab7..9610266b 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -1328,6 +1328,18 @@ run_action_pins() { record "action-pins" $rc } +# Every reference a source comment makes must resolve for a reader who has only +# this repository. A comment naming a private planning artifact is dead text to +# everyone outside the workspace holding it, and publishes that workspace's +# shape to everyone else. A hand-run grep closes the population that exists on +# the day it runs; this closes it for every commit after. +run_comment_refs() { + local rc=0 + info "[comment-refs] Checking that every source comment resolves in-repo" + "$SCRIPT_DIR/check-comment-refs.sh" || rc=$? + record "comment-refs" $rc +} + # Every daemon log string a test matches on must still be emitted by src/. # A stale one does not fail — it stops observing, and an expect-zero assertion # built on it then passes for the wrong reason. @@ -1383,6 +1395,7 @@ main() { run_trailing_log run_image_scoping run_action_pins + run_comment_refs run_wait_converge if [[ "$TEST_ONLY" == true ]]; then From c9abe10f3c123dc19fa900c4e8327e05568ba335 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 06:37:27 +0000 Subject: [PATCH 11/17] Stop claiming the Noise handshake hash provides transcript binding The published crypto tables named the bare Noise pattern strings without recording that this construction passes an empty associated-data field where standard Noise feeds the handshake hash. The security reference now carries a short deviation subsection stating what that choice does and does not buy: domain separation and DH binding survive through the chaining key, while transcript binding is the property actually absent. The comment on the hash field called it transcript binding and four getters called it channel binding. Nothing in production reads the value, so all five overstated it. They now describe what the field is, and the field comment records that anything built on it will silently not work until the associated data carries the hash. --- docs/design/fips-session-layer.md | 5 ++++- docs/reference/security.md | 24 +++++++++++++++++++++--- src/noise/handshake.rs | 12 +++++++++--- src/noise/session.rs | 4 ++-- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/docs/design/fips-session-layer.md b/docs/design/fips-session-layer.md index faa37ee9..93becd62 100644 --- a/docs/design/fips-session-layer.md +++ b/docs/design/fips-session-layer.md @@ -223,7 +223,10 @@ than network addresses. A session survives: FSP uses Noise XK for session encryption, distinct from the Noise IK pattern used at the link layer. The full Noise descriptor is -`Noise_XK_secp256k1_ChaChaPoly_SHA256`. +`Noise_XK_secp256k1_ChaChaPoly_SHA256`, with one deviation recorded in +[the security reference](../reference/security.md): the handshake AEAD +uses an empty associated-data field where standard Noise +`EncryptAndHash` uses the handshake hash `h`. The XK pattern (pre-message: `← s`): diff --git a/docs/reference/security.md b/docs/reference/security.md index a439b0c3..ed2c46b0 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -58,16 +58,34 @@ idempotent). | Curve | secp256k1 | FMP IK, FSP XK, Schnorr signatures | | Diffie-Hellman | ECDH on secp256k1 (x-only normalized) | Noise IK, Noise XK | | AEAD | ChaCha20-Poly1305 | FMP link encryption, FSP session encryption | -| Hash | SHA-256 | NodeAddr derivation, Noise transcript | +| Hash | SHA-256 | NodeAddr derivation, Noise key schedule | | Key derivation | HKDF-SHA256 | Noise key schedule | | Signatures | secp256k1 Schnorr | TreeAnnounce, LookupResponse proof, Nostr adverts | -| Noise pattern (link) | `Noise_IK_secp256k1_ChaChaPoly_SHA256` | FMP link layer (IK with epoch payload) | -| Noise pattern (session) | `Noise_XK_secp256k1_ChaChaPoly_SHA256` | FSP session layer (XK with epoch payload) | +| Noise pattern (link) | `Noise_IK_secp256k1_ChaChaPoly_SHA256`, with the deviation below | FMP link layer (IK with epoch payload) | +| Noise pattern (session) | `Noise_XK_secp256k1_ChaChaPoly_SHA256`, with the deviation below | FSP session layer (XK with epoch payload) | These choices align with the Nostr cryptographic stack (secp256k1 + ChaCha20-Poly1305 + SHA-256) and the NIP-44 encrypted messaging standard. +### Deviation: Empty Associated Data in the Handshake AEAD + +Both Noise patterns above deviate from the standard construction in one +respect. The handshake AEAD uses an empty associated-data field where +standard Noise `EncryptAndHash` uses the handshake hash `h`. + +The choice was deliberate. Using secp256k1 rather than 25519 already put the +construction outside standard Noise, so no standard-Noise peer could be +confused with it, and the transcript hash bought no distinguishing value. + +That argument is about domain separation, and on those grounds it holds. It +does not cover transcript binding, which is the property actually absent. +Domain separation and DH binding survive through the chaining key `ck`, which +`mix_key` chains from `ck = h`, seeded from the protocol name in +`SymmetricState::initialize` (`src/noise/handshake.rs`). The handshake hash +`h` is maintained at every step and is never fed to the AEAD, so it binds +nothing. + ## Rekey Defaults Both link-layer and session-layer Noise sessions rekey under one of diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index a7de40bc..db9f4000 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -21,7 +21,13 @@ use std::fmt; struct SymmetricState { /// Chaining key for key derivation. ck: [u8; 32], - /// Handshake hash for transcript binding. + /// Running SHA-256 accumulator over the handshake transcript. + /// + /// Maintained by `mix_hash` at every step, but never fed to the AEAD: + /// `encrypt_and_hash` passes an empty associated-data field. Nothing in + /// production reads it, so it provides no transcript binding today, and + /// anything built on `handshake_hash()` (channel binding, an exporter, + /// cookie binding) will silently not work until the AAD carries `h`. h: [u8; 32], /// Current cipher state for encrypting handshake payloads. cipher: CipherState, @@ -101,7 +107,7 @@ impl SymmetricState { (CipherState::new(k1), CipherState::new(k2)) } - /// Get the handshake hash (for channel binding). + /// Get the handshake hash. fn handshake_hash(&self) -> [u8; 32] { self.h } @@ -916,7 +922,7 @@ impl HandshakeState { )) } - /// Get the handshake hash (for channel binding, available after complete). + /// Get the handshake hash (available after complete). pub fn handshake_hash(&self) -> [u8; 32] { self.symmetric.handshake_hash() } diff --git a/src/noise/session.rs b/src/noise/session.rs index 0df0aaef..92a8a059 100644 --- a/src/noise/session.rs +++ b/src/noise/session.rs @@ -14,7 +14,7 @@ pub struct NoiseSession { send_cipher: CipherState, /// Cipher for receiving. recv_cipher: CipherState, - /// Handshake hash for channel binding. + /// Handshake hash. handshake_hash: [u8; 32], /// Remote peer's static public key. remote_static: PublicKey, @@ -190,7 +190,7 @@ impl NoiseSession { self.replay_window.reset(); } - /// Get the handshake hash for channel binding. + /// Get the handshake hash. pub fn handshake_hash(&self) -> &[u8; 32] { &self.handshake_hash } From ae787b9bbd9619b25c9984e1d7d11118c99dd6e3 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 07:59:04 +0000 Subject: [PATCH 12/17] Clear the symmetric key material we can reach when it goes out of scope Adds the zeroize dependency and clears the retained ChaCha20-Poly1305 key on CipherState, the chaining key and handshake hash on SymmetricState, and the 64-byte HKDF outputs in mix_key and split along with the two session keys split derives. The key mix_key derives for the handshake cipher is cleared too: it is passed by value into a Copy parameter, so the caller's copy survives the call and is the same class of residue as the ones beside it. Three fields are deliberately left out. The cached LessSafeKey holds its material behind ring's API and cannot be cleared from here, and the nonce and the has-key flag are not secret. Also corrects the comments around this code, which were wrong in ways that would have made the change harder to reason about later. ring's LessSafeKey does implement Clone; UnboundKey is the one that does not, and two comments said otherwise while a third said the opposite. The key bytes are retained for Clone and cipher_clone, not for initialize_key, which builds from its own argument. Caching the keyed cipher does not skip Poly1305 key derivation, which is nonce-dependent and happens per message. And the construction it does skip is a length check and a word conversion, not a constant-time check. The HKDF state itself, the ephemeral private key, and the per-handshake DH outputs are still uncleared; those are separate changes. --- Cargo.lock | 15 +++++++++++++++ Cargo.toml | 1 + src/node/encrypt_worker.rs | 6 ++++-- src/noise/handshake.rs | 19 +++++++++++++++++-- src/noise/mod.rs | 35 +++++++++++++++++++++++++++-------- src/noise/tests.rs | 20 ++++++++++++++++++++ 6 files changed, 84 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 85a6aa1d..2a0a98e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1126,6 +1126,7 @@ dependencies = [ "tun", "windows-service", "wintun", + "zeroize", ] [[package]] @@ -4288,6 +4289,20 @@ name = "zeroize" version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c50655cbb0fe3fc43170059e702f1ce5e19b84cec58dc87b037a09935c2f328" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] [[package]] name = "zerotrie" diff --git a/Cargo.toml b/Cargo.toml index 7d8573b7..d5b1db2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,6 +17,7 @@ secp256k1 = { version = "0.30", features = ["rand", "global-context"] } sha2 = "0.10" hkdf = "0.12" ring = "0.17" +zeroize = { version = "1.9", features = ["zeroize_derive"] } rand = "0.10.1" crossbeam-channel = "0.5" thiserror = "2.0" diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index d27ebfb8..c12b27f7 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -93,8 +93,10 @@ use tracing::{debug, trace, warn}; /// inside the worker. That second alloc + ~1.5 KB memcpy per packet at /// line rate cost ~150 MB/sec of memory bandwidth on the hot worker.) pub(crate) struct FmpSendJob { - /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`) - /// — the clone is just a refcount bump on the inner key material. + /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`), but + /// the clone is not a refcount bump: `ring` stores the ChaCha20 key + /// inline as `[u32; 8]`, so cloning copies the key material outright and + /// leaves a second copy that nothing outside `ring` can clear. pub cipher: LessSafeKey, /// Pre-reserved monotonic counter (via `take_send_counter`). pub counter: u64, diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index db9f4000..b7dbb881 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -9,6 +9,7 @@ use rand::Rng; use secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey, ecdh::shared_secret_point}; use sha2::{Digest, Sha256}; use std::fmt; +use zeroize::{Zeroize, ZeroizeOnDrop}; /// Symmetric state during handshake. /// @@ -17,7 +18,11 @@ use std::fmt; /// `Clone` exists for [`HandshakeState::try_read_xk_message_2`], which has to /// put the pre-read state back after a message that mixed material in before /// failing to authenticate. -#[derive(Clone)] +/// +/// `ck` and `h` are cleared on drop, including on the clone above once it +/// goes out of scope. `cipher` is skipped because [`CipherState`] clears its +/// own retained key. +#[derive(Clone, Zeroize, ZeroizeOnDrop)] struct SymmetricState { /// Chaining key for key derivation. ck: [u8; 32], @@ -30,6 +35,7 @@ struct SymmetricState { /// cookie binding) will silently not work until the AAD carries `h`. h: [u8; 32], /// Current cipher state for encrypting handshake payloads. + #[zeroize(skip)] cipher: CipherState, } @@ -76,6 +82,9 @@ impl SymmetricState { let mut key = [0u8; 32]; key.copy_from_slice(&output[32..64]); self.cipher.initialize_key(key); + key.zeroize(); + + output.zeroize(); } /// Encrypt and mix into hash. @@ -104,7 +113,13 @@ impl SymmetricState { k1.copy_from_slice(&output[..32]); k2.copy_from_slice(&output[32..64]); - (CipherState::new(k1), CipherState::new(k2)) + let ciphers = (CipherState::new(k1), CipherState::new(k2)); + + output.zeroize(); + k1.zeroize(); + k2.zeroize(); + + ciphers } /// Get the handshake hash. diff --git a/src/noise/mod.rs b/src/noise/mod.rs index a3d656c4..e2c21be0 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -42,6 +42,7 @@ mod session; use ring::aead::{Aad, CHACHA20_POLY1305, LessSafeKey, Nonce, UnboundKey}; use std::fmt; use thiserror::Error; +use zeroize::{Zeroize, ZeroizeOnDrop}; pub use handshake::HandshakeState; pub use replay::ReplayWindow; @@ -181,21 +182,32 @@ impl fmt::Display for HandshakeProgress { /// AEAD is `ring`'s ChaCha20-Poly1305 (BoringSSL backend), which dispatches /// to NEON on aarch64 and AVX2/AVX-512 on x86_64. The 32-byte key is /// retained alongside a cached `LessSafeKey` so the per-packet AEAD skips -/// the keyed-cipher construction (key copy + Poly1305 key derivation). -/// `LessSafeKey` itself doesn't implement `Clone` (deliberate, for safety), -/// so `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from the -/// retained key bytes — cheap for ChaCha20-Poly1305 since the construction -/// is essentially a key copy plus a constant-time check. +/// rebuilding the keyed cipher. (It does not skip Poly1305 key derivation, +/// which is nonce-dependent and happens per message inside seal and open.) +/// `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from those +/// retained bytes, and so does `cipher_clone` for each off-task AEAD worker; +/// those two are why the bytes are kept. The rebuild is cheap for +/// ChaCha20-Poly1305: a length check and a conversion of the 32 key bytes +/// into little-endian words. +/// +/// Because the key bytes are retained, every clone leaves a second copy in +/// memory; each copy is cleared when it goes out of scope. Only `key` is +/// zeroized. `cipher` is skipped because `LessSafeKey` +/// holds its material behind `ring`'s own API and cannot be cleared from +/// here; `nonce` and `has_key` are skipped because they are not secret. +#[derive(Zeroize, ZeroizeOnDrop)] pub struct CipherState { - /// Encryption key (32 bytes). Retained so we can rebuild the keyed - /// AEAD on `Clone` and on `initialize_key` (ring's `UnboundKey` / - /// `LessSafeKey` do not implement `Clone`). + /// Encryption key (32 bytes). Retained because `Clone` and + /// `cipher_clone` rebuild the keyed AEAD from these bytes. key: [u8; 32], /// Cached keyed AEAD, valid iff `has_key`. None for an un-keyed state. + #[zeroize(skip)] cipher: Option, /// Nonce counter (8 bytes used, 4 bytes zero prefix). + #[zeroize(skip)] pub(super) nonce: u64, /// Whether this cipher has a valid key. + #[zeroize(skip)] has_key: bool, } @@ -395,6 +407,13 @@ impl CipherState { self.has_key } + /// Copy out the retained key bytes, so a test can observe zeroization + /// without reading freed memory. + #[cfg(test)] + fn key_bytes(&self) -> [u8; 32] { + self.key + } + /// Clone the underlying keyed AEAD, for off-task AEAD workers. /// /// Returns `None` if no key. The cloned `LessSafeKey` pairs with diff --git a/src/noise/tests.rs b/src/noise/tests.rs index f452f8bc..c03a0ef8 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -201,6 +201,26 @@ fn test_cipher_state_nonce_sequence() { assert_eq!(cipher.nonce(), 2); } +#[test] +fn cipher_state_drop_impl_clears_the_retained_key() { + // Reading the bytes back out of a dropped `CipherState` would mean + // reading freed memory, so the drop behaviour is asserted through two + // observable proxies instead: that `CipherState` implements + // `ZeroizeOnDrop`, and that an explicit `zeroize()` leaves `key` + // all-zero while `has_key` is untouched. + fn assert_zeroize_on_drop() {} + assert_zeroize_on_drop::(); + + let mut cipher = CipherState::new([7u8; 32]); + assert_eq!(cipher.key_bytes(), [7u8; 32]); + assert!(cipher.has_key()); + + cipher.zeroize(); + + assert_eq!(cipher.key_bytes(), [0u8; 32]); + assert!(cipher.has_key()); +} + #[test] fn test_session_remote_static() { let keypair1 = generate_keypair(); From ee1c624cef145a643326ec59f4218c23f93f9ba3 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 13:28:37 +0000 Subject: [PATCH 13/17] Clear every copy of private key material the crate can reach The earlier change cleared the symmetric keys. This one covers the rest, and covers it by enumerating where key bytes actually live rather than by pattern, because three successive passes each cleared one place and missed another. The handshake state and the identity now clear their keypairs on drop. That matters more than the stack copies already handled: the ephemeral key was being wiped in two short-lived locals and then stored in a field that outlived both. These types are ordinary structs, so they can carry a drop even though the keypair inside them cannot. Also cleared: the temporary each of the fourteen elliptic-curve calls makes from a keypair, the by-value keypair parameters, the identity generation and parsing paths, the encoded secret strings, and the private key as it passes through configuration. The config file text is treated as secret for as long as it is held, since the key can be written straight into it. Two places assign over the configured key rather than dropping the struct that holds it. Assignment frees the old string without running the drop, so both now clear it first. What is deliberately not cleared, and why: the hash and key-derivation states, and the cached cipher keys inside ring. None of those crates offers a clearing route at the versions we pin, which I checked in their sources rather than assuming, and reaching for unsafe here was not worth it for a residue that needs local memory access to read. One limitation is worth stating plainly. These key types are copyable, so the compiler may duplicate them where we cannot see. This clears the copies the crate owns, not every copy that ever existed. The drops also have no test: reading a dropped struct's bytes means reading freed memory. --- src/bin/fips.rs | 13 +++- src/bin/fipsctl.rs | 12 ++- src/config/mod.rs | 92 ++++++++++++++++++---- src/discovery/nostr/runtime.rs | 11 ++- src/identity/encoding.rs | 20 ++++- src/identity/local.rs | 89 ++++++++++++++++++--- src/identity/mod.rs | 1 + src/node/handlers/handshake.rs | 10 ++- src/node/handlers/rekey.rs | 10 ++- src/node/handlers/session.rs | 21 ++++- src/node/lifecycle.rs | 28 ++++--- src/noise/handshake.rs | 137 ++++++++++++++++++++++++++++----- src/noise/mod.rs | 17 +++- src/noise/tests.rs | 6 +- src/peer/connection.rs | 22 +++++- 15 files changed, 405 insertions(+), 84 deletions(-) diff --git a/src/bin/fips.rs b/src/bin/fips.rs index b43f5282..afc09ca2 100644 --- a/src/bin/fips.rs +++ b/src/bin/fips.rs @@ -10,6 +10,7 @@ use fips::{Config, Node}; use std::path::PathBuf; use tracing::{debug, error, info, warn}; use tracing_subscriber::{EnvFilter, fmt}; +use zeroize::Zeroize; /// FIPS mesh network daemon #[derive(Parser, Debug)] @@ -126,7 +127,7 @@ async fn run_daemon( fips::node::warn_on_legacy_config_paths(); // Identity provisioning: config nsec > key file > generate ephemeral - let resolved = match resolve_identity(&config, &loaded_paths) { + let mut resolved = match resolve_identity(&config, &loaded_paths) { Ok(r) => r, Err(e) => { error!("Failed to resolve identity: {}", e); @@ -146,7 +147,15 @@ async fn run_daemon( // Create node with resolved identity let mut config = config; - config.node.identity.nsec = Some(resolved.nsec); + // Take the nsec rather than move it: `ResolvedIdentity` clears its copy + // on drop, so it cannot be left partially moved. Clear whatever the field + // already held first — assigning over it drops the old `String` in place, + // which does not run `Drop for IdentityConfig`, so a key that came from + // the config file would be freed uncleared. + if let Some(mut old) = config.node.identity.nsec.take() { + old.zeroize(); + } + config.node.identity.nsec = Some(std::mem::take(&mut resolved.nsec)); debug!("Creating node"); let mut node = match Node::new(config) { Ok(node) => node, diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index 15543044..5cf7701e 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -15,6 +15,7 @@ use std::io::{BufRead, BufReader, Write}; use std::net::{Ipv6Addr, SocketAddrV6}; use std::path::{Path, PathBuf}; use std::time::Duration; +use zeroize::Zeroizing; /// FIPS control client #[derive(Parser, Debug)] @@ -382,11 +383,18 @@ fn main() { // Commands that don't require a running daemon if let Commands::Keygen { dir, force, stdout } = &cli.command { let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. The + // nsec is that same key in another encoding, so it is guarded too. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = Zeroizing::new(encode_nsec(&secret_key)); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if *stdout { - println!("{nsec}"); + println!("{}", nsec.as_str()); println!("{npub}"); return; } diff --git a/src/config/mod.rs b/src/config/mod.rs index 10cfb0c4..bab1ec9a 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -32,6 +32,7 @@ use crate::{Identity, IdentityError}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use thiserror::Error; +use zeroize::{Zeroize, Zeroizing}; #[cfg(target_os = "linux")] pub use gateway::{ConntrackConfig, GatewayConfig, GatewayDnsConfig, PortForward, Proto}; @@ -217,11 +218,16 @@ pub fn default_gateway_path() -> PathBuf { } /// Read a bare bech32 nsec from a key file. +/// +/// The file contents are the private key, and trimming copies it into a +/// second string, so the read buffer is cleared on every exit path rather +/// than dropped as it stands. The returned nsec is the caller's. pub fn read_key_file(path: &Path) -> Result { let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { path: path.to_path_buf(), source: e, })?; + let contents = Zeroizing::new(contents); let nsec = contents.trim().to_string(); if nsec.is_empty() { return Err(ConfigError::EmptyKeyFile { @@ -442,7 +448,9 @@ pub fn resolve_identity( if config.node.identity.persistent { // Persistent mode: load existing key file or generate-and-persist if key_path.exists() { - let nsec = read_key_file(&key_path)?; + // Held in a guard, not a bare `String`: if the parse below fails, + // the `?` returns and a bare local would be freed uncleared. + let nsec = Zeroizing::new(read_key_file(&key_path)?); let identity = Identity::from_secret_str(&nsec)?; warn_unmanaged_key_file(&key_path); if let Err(e) = write_pub_file(&pub_path, &identity.npub()) { @@ -453,7 +461,7 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(key_path), }); } @@ -467,7 +475,8 @@ pub fn resolve_identity( Path::new(SYSTEM_CONFIG_DIR), Path::new(LEGACY_SYSTEM_CONFIG_DIR), ) { - let nsec = read_key_file(&legacy)?; + // Guarded for the same reason as the current-path read above. + let nsec = Zeroizing::new(read_key_file(&legacy)?); let identity = Identity::from_secret_str(&nsec)?; tracing::warn!( legacy = %legacy.display(), @@ -484,14 +493,20 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(legacy), }); } // No key file anywhere — generate and persist let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -529,7 +544,13 @@ pub fn resolve_identity( // Ephemeral mode (default): fresh keypair every start, write key files // for operator visibility let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -571,6 +592,14 @@ pub fn resolve_identity( } /// Result of identity resolution. +/// +/// `nsec` is the node's private key in plaintext. Every local that carries it +/// through [`resolve_identity`] either moves into this struct or is held in a +/// guard that clears it, so this is where the surviving string lives and where +/// clearing it belongs. +/// A caller that wants the value out should take it with [`Option::take`] or +/// `std::mem::take` rather than moving the field, which the `Drop` below +/// forbids. pub struct ResolvedIdentity { /// The nsec string (bech32 or hex) for creating an Identity. pub nsec: String, @@ -578,6 +607,14 @@ pub struct ResolvedIdentity { pub source: IdentitySource, } +impl Drop for ResolvedIdentity { + /// Clear the plaintext private key rather than dropping the allocation + /// with the key still in it. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Where a resolved identity originated. pub enum IdentitySource { /// From explicit nsec in config file. @@ -639,6 +676,20 @@ pub struct IdentityConfig { pub persistent: bool, } +impl Drop for IdentityConfig { + /// Clear the plaintext private key. + /// + /// This field holds the node's private key for the whole process + /// lifetime, which is the longest any secret lives in this crate, so + /// leaving the allocation to be freed with the key still in it is the + /// largest residue the crate can reach. A caller that needs the value out + /// should take it with [`Option::take`]; moving the field is what the + /// `Drop` forbids. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Root configuration structure. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Config { @@ -709,10 +760,16 @@ impl Config { /// Load configuration from a single file. pub fn load_file(path: &Path) -> Result { - let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { - path: path.to_path_buf(), - source: e, - })?; + // The config file is the highest-priority home of a plaintext key: + // `node.identity.nsec` is read straight out of it, so the whole file + // text is treated as secret for as long as it is held. + let contents = + Zeroizing::new( + std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { + path: path.to_path_buf(), + source: e, + })?, + ); serde_yaml::from_str(&contents).map_err(|e| ConfigError::ParseYaml { path: path.to_path_buf(), @@ -755,10 +812,19 @@ impl Config { /// Merge another configuration into this one. /// /// Values from `other` override values in `self` when present. - pub fn merge(&mut self, other: Config) { - // Merge node.identity section + pub fn merge(&mut self, mut other: Config) { + // Merge node.identity section. The nsec is taken rather than moved + // out of `other.node.identity`, which clears its private key on drop + // and so cannot be left partially moved. if other.node.identity.nsec.is_some() { - self.node.identity.nsec = other.node.identity.nsec; + // Clear whatever this field already held before overwriting it. + // Assigning over the field drops the old `String` in place, which + // does not run `Drop for IdentityConfig` and would free a + // plaintext key uncleared when two config files both carry one. + if let Some(mut old) = self.node.identity.nsec.take() { + old.zeroize(); + } + self.node.identity.nsec = other.node.identity.nsec.take(); } if other.node.identity.persistent { self.node.identity.persistent = true; diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index af8c68f8..41704467 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -15,6 +15,7 @@ use serde::Serialize; use tokio::sync::{Mutex, Notify, RwLock, broadcast, mpsc, oneshot}; use tokio::task::JoinHandle; use tracing::{debug, info, trace, warn}; +use zeroize::{Zeroize, Zeroizing}; use super::failure_state::FailureState; use super::offer_admission::{AdmissionReject, OfferAdmission}; @@ -273,7 +274,15 @@ impl NostrDiscovery { return Err(BootstrapError::Disabled); } - let keys = nostr::Keys::parse(&hex::encode(identity.keypair().secret_bytes())) + // Three copies of the private key are made to reach `Keys::parse`: + // the keypair, its raw bytes, and the hex string. Each is bound and + // cleared here; `nostr::Keys` clears its own on drop. + let mut our_keypair = identity.keypair(); + let mut secret_bytes = our_keypair.secret_bytes(); + let secret_hex = Zeroizing::new(hex::encode(secret_bytes)); + secret_bytes.zeroize(); + our_keypair.non_secure_erase(); + let keys = nostr::Keys::parse(secret_hex.as_str()) .map_err(|e| BootstrapError::Nostr(e.to_string()))?; let client = Client::builder() .signer(keys.clone()) diff --git a/src/identity/encoding.rs b/src/identity/encoding.rs index d81f9aa6..772a635f 100644 --- a/src/identity/encoding.rs +++ b/src/identity/encoding.rs @@ -2,6 +2,7 @@ use bech32::{Bech32, Hrp}; use secp256k1::{SecretKey, XOnlyPublicKey}; +use zeroize::{Zeroize, Zeroizing}; use super::IdentityError; @@ -33,14 +34,25 @@ pub fn decode_npub(npub: &str) -> Result { } /// Encode a secret key as a bech32 nsec string (NIP-19). +/// +/// The returned string is the private key in another encoding, so it is the +/// caller's to clear. What this function clears is the raw byte copy +/// `secret_bytes` hands back, which would otherwise sit in an unnamed +/// temporary until the end of the statement. pub fn encode_nsec(secret_key: &SecretKey) -> String { - bech32::encode::(NSEC_HRP, &secret_key.secret_bytes()) - .expect("nsec encoding cannot fail") + let mut secret_bytes = secret_key.secret_bytes(); + let nsec = + bech32::encode::(NSEC_HRP, &secret_bytes).expect("nsec encoding cannot fail"); + secret_bytes.zeroize(); + nsec } /// Decode an nsec string to a secret key. pub fn decode_nsec(nsec: &str) -> Result { let (hrp, data) = bech32::decode(nsec)?; + // `data` is the raw private key. The guard clears it on every exit path, + // including the two length and prefix rejections below. + let data = Zeroizing::new(data); if hrp != NSEC_HRP { return Err(IdentityError::InvalidNsecPrefix(hrp.to_string())); @@ -59,7 +71,9 @@ pub fn decode_secret(s: &str) -> Result { if s.starts_with("nsec1") { decode_nsec(s) } else { - let bytes = hex::decode(s)?; + // `bytes` is the raw private key; the guard clears it on both the + // length rejection and the normal return. + let bytes = Zeroizing::new(hex::decode(s)?); if bytes.len() != 32 { return Err(IdentityError::InvalidNsecLength(bytes.len())); } diff --git a/src/identity/local.rs b/src/identity/local.rs index 26db8e17..7a494369 100644 --- a/src/identity/local.rs +++ b/src/identity/local.rs @@ -2,6 +2,7 @@ use secp256k1::{Keypair, PublicKey, SecretKey, XOnlyPublicKey}; use std::fmt; +use zeroize::Zeroize; use super::auth::{AuthResponse, auth_challenge_digest}; use super::encoding::{decode_secret, encode_npub}; @@ -11,6 +12,13 @@ use super::{FipsAddress, IdentityError, NodeAddr, sha256}; /// /// The identity holds the secp256k1 keypair and provides methods for signing /// and verifying protocol messages. +/// +/// The keypair is the node's long-term private key. It is erased when the +/// identity is dropped, and every constructor below erases the intermediate +/// secret it built the identity from. All of that clears the copies this +/// crate owns, not every copy that ever existed: `secp256k1` names its erase +/// non-secure because the compiler may duplicate or move the bytes to places +/// no code here can name. #[derive(Clone)] pub struct Identity { keypair: Keypair, @@ -23,39 +31,51 @@ impl Identity { pub fn generate() -> Self { let mut secret_bytes = [0u8; 32]; rand::Rng::fill_bytes(&mut rand::rng(), &mut secret_bytes); - let secret_key = + let mut secret_key = SecretKey::from_slice(&secret_bytes).expect("32 random bytes is a valid secret key"); - Self::from_secret_key(secret_key) + let identity = Self::from_secret_key(secret_key); + secret_bytes.zeroize(); + secret_key.non_secure_erase(); + identity } /// Create an identity from an existing keypair. - pub fn from_keypair(keypair: Keypair) -> Self { + pub fn from_keypair(mut keypair: Keypair) -> Self { let (pubkey, _parity) = keypair.x_only_public_key(); let node_addr = NodeAddr::from_pubkey(&pubkey); let address = FipsAddress::from_node_addr(&node_addr); - Self { + let identity = Self { keypair, node_addr, address, - } + }; + keypair.non_secure_erase(); + identity } /// Create an identity from a secret key. - pub fn from_secret_key(secret_key: SecretKey) -> Self { - let keypair = Keypair::from_secret_key(&super::SECP, &secret_key); - Self::from_keypair(keypair) + pub fn from_secret_key(mut secret_key: SecretKey) -> Self { + let mut keypair = Keypair::from_secret_key(&super::SECP, &secret_key); + let identity = Self::from_keypair(keypair); + keypair.non_secure_erase(); + secret_key.non_secure_erase(); + identity } /// Create an identity from secret key bytes. pub fn from_secret_bytes(bytes: &[u8; 32]) -> Result { - let secret_key = SecretKey::from_slice(bytes)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = SecretKey::from_slice(bytes)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Create an identity from an nsec string (bech32) or hex-encoded secret. pub fn from_secret_str(s: &str) -> Result { - let secret_key = decode_secret(s)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = decode_secret(s)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Return the underlying keypair. @@ -110,6 +130,51 @@ impl Identity { } } +impl Drop for Identity { + /// Erase the long-term private key this identity owns. + /// + /// `Keypair` is `Copy` and so cannot clear itself on drop; `Identity` is + /// not, so it does it for the copy it holds. See the type's own + /// documentation for what that does and does not reach. + fn drop(&mut self) { + self.keypair.non_secure_erase(); + } +} + +/// A `Keypair` copy that is erased when it goes out of scope. +/// +/// `Keypair` is `Copy` and so cannot clear itself on drop. A frame that holds +/// a copy of the node's long-term private key across several exit paths — +/// early error returns, `?`, a normal return — would otherwise need an erase +/// written at each one, and a missed path is invisible. Holding the copy here +/// instead makes the clearing structural. +/// +/// This clears the copy this guard owns, not every copy that ever existed: +/// `secp256k1` names its erase non-secure because the compiler may duplicate +/// or move the bytes to places no code here can name. +pub(crate) struct ErasingKeypair(Keypair); + +impl ErasingKeypair { + /// Take a copy of `source` into the guard and erase `source` in place, so + /// the caller's own binding does not outlive the move. + pub(crate) fn take(source: &mut Keypair) -> Self { + let guarded = Self(*source); + source.non_secure_erase(); + guarded + } + + /// Borrow the guarded keypair. + pub(crate) fn get(&self) -> &Keypair { + &self.0 + } +} + +impl Drop for ErasingKeypair { + fn drop(&mut self) { + self.0.non_secure_erase(); + } +} + impl fmt::Debug for Identity { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Identity") diff --git a/src/identity/mod.rs b/src/identity/mod.rs index 6093427d..b260f8ce 100644 --- a/src/identity/mod.rs +++ b/src/identity/mod.rs @@ -20,6 +20,7 @@ use thiserror::Error; pub use address::FipsAddress; pub use auth::{AuthChallenge, AuthResponse}; pub use encoding::{decode_npub, decode_nsec, decode_secret, encode_npub, encode_nsec}; +pub(crate) use local::ErasingKeypair; pub use local::Identity; pub use node_addr::NodeAddr; pub use peer::PeerIdentity; diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 4d5d9384..cc73a1ea 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -226,14 +226,18 @@ impl Node { packet.timestamp_ms, ); - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let noise_msg1 = &packet.data[header.noise_msg1_offset..]; - let msg2_response = match conn.receive_handshake_init( + let init_result = conn.receive_handshake_init( our_keypair, self.startup_epoch(), noise_msg1, packet.timestamp_ms, - ) { + ); + our_keypair.non_secure_erase(); + let msg2_response = match init_result { Ok(m) => m, Err(e) => { debug!( diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index fc2c22fb..bac07b8a 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -194,8 +194,11 @@ impl Node { }; // Create IK initiator handshake directly (no PeerConnection) - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut hs = HandshakeState::new_initiator(our_keypair, peer_pubkey); + our_keypair.non_secure_erase(); hs.set_local_epoch(self.startup_epoch()); let noise_msg1 = match hs.write_message_1() { @@ -605,8 +608,11 @@ impl Node { let dest_pubkey = *entry.remote_pubkey(); // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = match handshake.write_xk_message_1() { diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 2e892075..c8e11bcb 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -626,8 +626,11 @@ impl Node { .record_reject(RejectReason::Session(SessionReject::RekeyPending)); return; } - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; + // the handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -681,8 +684,11 @@ impl Node { } // Create XK responder handshake and process msg1 - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -720,7 +726,11 @@ impl Node { // Store session entry in AwaitingMsg3 state with ack payload for potential resend. // Use a dummy pubkey since we don't know the initiator's identity yet. // We use our own pubkey as placeholder; it will be replaced in handle_session_msg3. - let placeholder_pubkey = self.identity().keypair().public_key(); + // `keypair()` hands back a copy of the long-term private key, so the + // temporary is bound and erased rather than left to the statement end. + let mut our_keypair = self.identity().keypair(); + let placeholder_pubkey = our_keypair.public_key(); + our_keypair.non_secure_erase(); let now_ms = Self::now_ms(); let resend_interval = self.config().node.rate_limit.handshake_resend_interval_ms; let mut entry = SessionEntry::new( @@ -1728,8 +1738,11 @@ impl Node { } // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = handshake .write_xk_message_1() diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index e2523976..e8a7b385 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -489,18 +489,22 @@ impl Node { }; // Start the Noise handshake and get message 1 - let our_keypair = self.identity().keypair(); - let noise_msg1 = - match connection.start_handshake(our_keypair, self.startup_epoch(), current_time_ms) { - Ok(msg) => msg, - Err(e) => { - // Clean up the index and link - let _ = self.index_allocator.free(our_index); - self.links.remove(&link_id); - self.addr_to_link.remove(&(transport_id, remote_addr)); - return Err(NodeError::HandshakeFailed(e.to_string())); - } - }; + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); + let start_result = + connection.start_handshake(our_keypair, self.startup_epoch(), current_time_ms); + our_keypair.non_secure_erase(); + let noise_msg1 = match start_result { + Ok(msg) => msg, + Err(e) => { + // Clean up the index and link + let _ = self.index_allocator.free(our_index); + self.links.remove(&link_id); + self.addr_to_link.remove(&(transport_id, remote_addr)); + return Err(NodeError::HandshakeFailed(e.to_string())); + } + }; // Set index and transport info on the connection connection.set_our_index(our_index); diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index b7dbb881..5116707f 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -179,7 +179,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key and will send first. /// Used by FMP (link layer). - pub fn new_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -201,6 +201,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -208,7 +212,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 1. Used by FMP (link layer). - pub fn new_responder(static_keypair: Keypair) -> Self { + pub fn new_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -229,6 +233,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -236,7 +244,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key. XK defers the /// initiator's static key reveal to msg3. Used by FSP (session layer). - pub fn new_xk_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_xk_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -256,6 +264,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -263,7 +275,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 3. Used by FSP (session layer). - pub fn new_xk_responder(static_keypair: Keypair) -> Self { + pub fn new_xk_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -283,6 +295,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -322,9 +338,15 @@ impl HandshakeState { let mut secret_bytes = [0u8; 32]; rng.fill_bytes(&mut secret_bytes); - let secret_key = + let mut secret_key = SecretKey::from_slice(&secret_bytes).expect("32 random bytes is valid secret key"); self.ephemeral_keypair = Some(Keypair::from_secret_key(&self.secp, &secret_key)); + secret_bytes.zeroize(); + // The erase is called non-secure because the private key may sit in + // further copies this function cannot name. It still clears the copy + // this frame owns; the keypair the key was just stored in is cleared + // by `Drop for HandshakeState`. + secret_key.non_secure_erase(); } /// Perform ECDH between our secret and their public key. @@ -335,15 +357,26 @@ impl HandshakeState { /// may have the wrong parity for the responder's static key. Since P and /// -P produce ECDH result points with the same x-coordinate, hashing /// only x ensures both sides derive the same shared secret. + /// + /// `our_secret` is borrowed, so this frame makes no copy of it. Every + /// caller below binds the value `Keypair::secret_key` hands back and + /// erases that binding once the DH is done, because the returned + /// `SecretKey` is a whole private key rather than a handle to one. Those + /// erases clear the copies this crate owns; `secp256k1` names its erase + /// non-secure because the compiler may hold further copies that no code + /// here can name. fn ecdh(&self, our_secret: &SecretKey, their_public: &PublicKey) -> [u8; 32] { // Get raw (x, y) coordinates (64 bytes) without any hashing - let point = shared_secret_point(their_public, our_secret); + let mut point = shared_secret_point(their_public, our_secret); // Hash only the x-coordinate (first 32 bytes), ignoring y/parity let mut hasher = Sha256::new(); hasher.update(&point[..32]); - let hash = hasher.finalize(); + let mut hash = hasher.finalize(); let mut result = [0u8; 32]; result.copy_from_slice(&hash); + hash.as_mut_slice().zeroize(); + point.zeroize(); + // `result` is moved out, so clearing it belongs to the caller. result } @@ -388,8 +421,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: encrypt our static and send let our_static = self.static_keypair.public_key().serialize(); @@ -397,8 +433,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &remote_static); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -441,8 +480,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: decrypt initiator's static let encrypted_static_end = PUBKEY_SIZE + PUBKEY_SIZE + super::TAG_SIZE; @@ -453,8 +495,11 @@ impl HandshakeState { self.remote_static = Some(rs); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &rs); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &rs); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -508,12 +553,18 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -556,14 +607,20 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(e, rs), mix into key // (initiator uses their ephemeral with responder's static) let rs = self.remote_static.expect("initiator has remote static"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -620,8 +677,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -660,8 +720,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -707,8 +770,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -752,8 +818,11 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -839,8 +908,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -890,8 +962,11 @@ impl HandshakeState { .ephemeral_keypair .as_ref() .expect("should have ephemeral after msg2"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -943,6 +1018,26 @@ impl HandshakeState { } } +impl Drop for HandshakeState { + /// Erase the two private keys this state holds. + /// + /// `static_keypair` is the node's long-term private key and + /// `ephemeral_keypair` is the handshake's own. Both live here for the + /// whole handshake, which is longer than in any other frame, so this is + /// where clearing them matters most. `Keypair` is `Copy` and so cannot + /// clear itself on drop; `HandshakeState` is not, so it does it for both. + /// + /// This clears the copies this crate owns, not every copy that ever + /// existed: `secp256k1` names its erase non-secure because the compiler + /// may duplicate or move the bytes to places no code here can name. + fn drop(&mut self) { + self.static_keypair.non_secure_erase(); + if let Some(ephemeral) = self.ephemeral_keypair.as_mut() { + ephemeral.non_secure_erase(); + } + } +} + impl fmt::Debug for HandshakeState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("HandshakeState") diff --git a/src/noise/mod.rs b/src/noise/mod.rs index e2c21be0..85366e4f 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -229,14 +229,19 @@ impl Clone for CipherState { impl CipherState { /// Create a new cipher state with the given key. - pub(crate) fn new(key: [u8; 32]) -> Self { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(crate) fn new(mut key: [u8; 32]) -> Self { let cipher = Self::build_cipher(&key); - Self { + let state = Self { key, cipher, nonce: 0, has_key: true, - } + }; + key.zeroize(); + state } /// Create an empty cipher state (no key yet). @@ -250,11 +255,15 @@ impl CipherState { } /// Initialize with a key. - pub(super) fn initialize_key(&mut self, key: [u8; 32]) { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(super) fn initialize_key(&mut self, mut key: [u8; 32]) { self.key = key; self.cipher = Self::build_cipher(&key); self.nonce = 0; self.has_key = true; + key.zeroize(); } /// Build a ring `LessSafeKey` from raw key bytes. Centralized so the diff --git a/src/noise/tests.rs b/src/noise/tests.rs index c03a0ef8..82cd7a93 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -212,7 +212,11 @@ fn cipher_state_drop_impl_clears_the_retained_key() { assert_zeroize_on_drop::(); let mut cipher = CipherState::new([7u8; 32]); - assert_eq!(cipher.key_bytes(), [7u8; 32]); + // `key_bytes` hands back a copy of live key material, so the observation + // is bound and cleared rather than left in an unnamed temporary. + let mut observed = cipher.key_bytes(); + assert_eq!(observed, [7u8; 32]); + observed.zeroize(); assert!(cipher.has_key()); cipher.zeroize(); diff --git a/src/peer/connection.rs b/src/peer/connection.rs index bc6c6e21..8ba399cf 100644 --- a/src/peer/connection.rs +++ b/src/peer/connection.rs @@ -5,6 +5,7 @@ //! ActivePeer upon successful authentication. use crate::PeerIdentity; +use crate::identity::ErasingKeypair; use crate::noise::{self, NoiseError, NoiseSession}; use crate::transport::{LinkDirection, LinkId, LinkStats, TransportAddr, TransportId}; use crate::utils::index::SessionIndex; @@ -402,10 +403,15 @@ impl PeerConnection { /// The epoch is our startup epoch, encrypted into msg1 for restart detection. pub fn start_handshake( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], current_time_ms: u64, ) -> Result, NoiseError> { + // The parameter is this frame's own copy of the node's long-term + // private key, and the state checks below return before it is used. + // The guard clears it on every exit path. + let our_keypair = ErasingKeypair::take(&mut our_keypair); + if self.direction != LinkDirection::Outbound { return Err(NoiseError::WrongState { expected: "outbound connection".to_string(), @@ -426,7 +432,9 @@ impl PeerConnection { .expect("outbound must have expected identity") .pubkey_full(); - let mut hs = noise::HandshakeState::new_initiator(our_keypair, remote_static); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_initiator(kp, remote_static); + kp.non_secure_erase(); hs.set_local_epoch(epoch); let msg1 = hs.write_message_1()?; @@ -443,11 +451,15 @@ impl PeerConnection { /// The epoch is our startup epoch, encrypted into msg2 for restart detection. pub fn receive_handshake_init( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], message: &[u8], current_time_ms: u64, ) -> Result, NoiseError> { + // Same as `start_handshake`: the parameter copy outlives two early + // returns, so the guard owns it rather than an erase per exit path. + let our_keypair = ErasingKeypair::take(&mut our_keypair); + if self.direction != LinkDirection::Inbound { return Err(NoiseError::WrongState { expected: "inbound connection".to_string(), @@ -462,7 +474,9 @@ impl PeerConnection { }); } - let mut hs = noise::HandshakeState::new_responder(our_keypair); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_responder(kp); + kp.non_secure_erase(); hs.set_local_epoch(epoch); // Process message 1 (this reveals the initiator's identity and epoch) From 2d3ddf71885f275fe51e2d89b894cfa82f3e2714 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 14:42:48 +0000 Subject: [PATCH 14/17] Add the frame length validator, without calling it yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The received frame header carries a payload length that nothing reads. This adds the function that says what that field should hold for a given frame kind, so the next change can compare the two and drop a frame whose header disagrees with what actually arrived. No call site yet, deliberately: this commit changes no behaviour, and the function carries a dead-code annotation that the change adding the call removes with it. I checked that the annotation is doing real work rather than assuming so — removing it fails the lint build. Each frame kind gets its own arm because there is no shared convention. The handshake frames count everything after the common prefix; the established frame counts only the inner plaintext. An unrecognised phase returns nothing rather than a guess, so a caller skips the frame instead of rejecting it. Three tests come with it, each broken to confirm it can fail. One catches the wrong convention specifically, reproducing the value a validator written from the field's looser description would have computed. --- src/node/wire.rs | 73 +++++++++++++++++++++++++++++++++++++ src/transport/tcp/stream.rs | 21 +++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/node/wire.rs b/src/node/wire.rs index 21bc149f..cb8c7208 100644 --- a/src/node/wire.rs +++ b/src/node/wire.rs @@ -66,6 +66,43 @@ pub const FLAG_CE: u8 = 0x02; /// Spin bit for RTT measurement. pub const FLAG_SP: u8 = 0x04; +// ============================================================================ +// Wire Length Validation +// ============================================================================ + +/// Expected `payload_len` for a packet of `phase` and total wire length +/// `total`, or `None` if the phase carries no fixed relationship. +/// +/// Each frame kind states its own relationship; there is no single shared +/// convention. The handshake frames count everything after the 4-byte common +/// prefix. The established frame counts only the inner plaintext, excluding +/// both the 16-byte header and the AEAD tag, which is what +/// [`CommonPrefix::payload_len`] documents. +/// +/// The handshake arms are built from the same constants the encoders use; the +/// established arm is not, since no encoder reads `ENCRYPTED_MIN_SIZE`. Neither +/// shape stops a one-sided edit from making the two disagree, which is what the +/// tests below exist to catch. +/// +/// A `None` from the established arm means "no fixed relationship", so a caller +/// skips such a frame rather than rejecting it. That is the safe direction: a +/// truncating cast here would produce a false rejection instead. +// Nothing calls this yet. The dispatch point that calls it arrives in the +// following commit, which removes this annotation with it. `CommonPrefix::flags` +// and `EncryptedHeader::payload_len` below carry the annotation for the same +// reason. +#[allow(dead_code)] +pub fn expected_payload_len(phase: u8, total: usize) -> Option { + match phase { + PHASE_MSG1 => Some((MSG1_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16), + PHASE_MSG2 => Some((MSG2_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16), + PHASE_ESTABLISHED => total + .checked_sub(ENCRYPTED_MIN_SIZE) + .and_then(|n| u16::try_from(n).ok()), + _ => None, + } +} + // ============================================================================ // Common Prefix // ============================================================================ @@ -623,4 +660,40 @@ mod tests { // payload_len = sender_idx(4) + receiver_idx(4) + noise_msg2(57) = 65 assert_eq!(prefix.payload_len, 65); } + + #[test] + fn expected_payload_len_for_an_established_frame_excludes_header_and_tag() { + let header = build_established_header(SessionIndex::new(7), 0, 0, 40); + let frame = build_encrypted(&header, &[0u8; 40 + TAG_SIZE]); + + // 16 header + 40 plaintext + 16 tag = 72 on the wire, declaring 40. + assert_eq!(frame.len(), ESTABLISHED_HEADER_SIZE + 40 + TAG_SIZE); + assert_eq!( + expected_payload_len(PHASE_ESTABLISHED, frame.len()), + Some(40) + ); + } + + #[test] + fn expected_payload_len_matches_what_build_msg1_and_build_msg2_actually_emit() { + // The Noise buffers are sized with literals rather than + // HANDSHAKE_MSG1_SIZE / HANDSHAKE_MSG2_SIZE, unlike the two tests + // above. With the constant on both sides, changing it would move the + // encoder's payload_len and the validator's expectation together and + // leave this test green. Pinning the input keeps the two sides + // independent so a one-sided change is caught. + let msg1 = build_msg1(SessionIndex::new(1), &[0u8; 106]); + let prefix = CommonPrefix::parse(&msg1).unwrap(); + assert_eq!( + expected_payload_len(PHASE_MSG1, msg1.len()), + Some(prefix.payload_len) + ); + + let msg2 = build_msg2(SessionIndex::new(1), SessionIndex::new(2), &[0u8; 57]); + let prefix = CommonPrefix::parse(&msg2).unwrap(); + assert_eq!( + expected_payload_len(PHASE_MSG2, msg2.len()), + Some(prefix.payload_len) + ); + } } diff --git a/src/transport/tcp/stream.rs b/src/transport/tcp/stream.rs index 4d5e03c0..1d499275 100644 --- a/src/transport/tcp/stream.rs +++ b/src/transport/tcp/stream.rs @@ -229,6 +229,27 @@ mod tests { frame } + /// The wire sizes above are written as literals, independently of the FMP + /// wire module, which derives the same values from the Noise message sizes. + /// Keeping them independent is deliberate: this module takes no dependency + /// on `crate::node`, and the import below exists only under `cfg(test)`. + /// The cost is that the two can drift. A wrong literal on this side also + /// breaks the TCP integration tests, since they move real frames through + /// this reader; a wrong value on the wire-module side does not reach here + /// at all, and this test is what catches that direction. + #[test] + fn stream_reader_constants_agree_with_the_fmp_wire_module() { + use crate::node::wire; + + assert_eq!(MSG1_WIRE_SIZE, wire::MSG1_WIRE_SIZE); + assert_eq!(MSG2_WIRE_SIZE, wire::MSG2_WIRE_SIZE); + assert_eq!(PREFIX_SIZE, wire::COMMON_PREFIX_SIZE); + assert_eq!( + ESTABLISHED_REMAINING_HEADER + PREFIX_SIZE, + wire::ESTABLISHED_HEADER_SIZE + ); + } + #[tokio::test] async fn test_read_established_frame() { let payload_len = 64u16; From cfee2ccde55dffd61525a5b56de7b2dfa93261da Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 15:22:03 +0000 Subject: [PATCH 15/17] Drop frames whose header disagrees with the frame that arrived The received frame header declares a payload length that nothing read. It now gets compared against the frame the transport actually delivered, at the one dispatch point every transport converges on, before that field can be used as a parsing input. This changes behaviour on a deployed line, so it is worth being exact about what it drops. The stream transports read their frame boundary out of this same field, so the comparison holds by construction and never fires for them. The datagram transports deliver one whole frame per packet, where the length is known exactly and nothing checked it before. A short read there is a truncated frame, which already failed the tag or the exact-size parse; this changes which reason it is dropped for, not whether it is dropped. An unrecognised phase carries no fixed relationship, so it is left alone and reaches the dispatch as before, rather than being rejected on a guess. The drop gets its own rejection reason and its own counter. Reusing the admission reason would have mis-attributed it: this is a framing rejection decided before the phase dispatch, and it applies to data frames as well as handshakes. The dispatch function is now visible to the rest of the node module so a test can drive one packet through it, which is the reach the handshake handlers beside it already had. --- src/node/handlers/rx_loop.rs | 41 ++++++++++- src/node/reject.rs | 29 ++++++-- src/node/stats.rs | 32 +++++++++ src/node/tests/handshake.rs | 134 +++++++++++++++++++++++++++++++++++ src/node/wire.rs | 5 -- src/transport/ble/io.rs | 9 +++ 6 files changed, 240 insertions(+), 10 deletions(-) diff --git a/src/node/handlers/rx_loop.rs b/src/node/handlers/rx_loop.rs index 6ca0112e..4e54b002 100644 --- a/src/node/handlers/rx_loop.rs +++ b/src/node/handlers/rx_loop.rs @@ -1,8 +1,10 @@ //! RX event loop and packet dispatch. use crate::control::{ControlSocket, commands}; +use crate::node::reject::{RejectReason, TransportReject}; use crate::node::wire::{ COMMON_PREFIX_SIZE, CommonPrefix, FMP_VERSION, PHASE_ESTABLISHED, PHASE_MSG1, PHASE_MSG2, + expected_payload_len, }; use crate::node::{Node, NodeError}; use crate::transport::ReceivedPacket; @@ -299,7 +301,11 @@ impl Node { /// Process a single received packet. /// /// Dispatches based on the phase field in the 4-byte common prefix. - async fn process_packet(&mut self, packet: ReceivedPacket) { + /// + /// Visible to the rest of `crate::node` so tests can drive a single + /// packet through the dispatch, the same reach `handle_msg1` and + /// `handle_msg2` already have. + pub(in crate::node) async fn process_packet(&mut self, packet: ReceivedPacket) { if packet.data.len() < COMMON_PREFIX_SIZE { return; // Drop packets too short for common prefix } @@ -346,6 +352,39 @@ impl Node { return; } + // Drop a frame whose declared payload length disagrees with the + // frame that arrived, before that field can be used as a parsing + // input. + // + // Every transport's packets converge here, but the two families + // reach this line differently. TCP, Tor and Nym read their frame + // boundary out of this same field, so for them the comparison holds + // by construction and never fires. UDP, Ethernet and BLE deliver one + // whole frame per packet, where the arrived length is known exactly + // and nothing compares the two today. A short read on those + // transports is a truncated frame, which fails the AEAD tag or the + // exact-size handshake parse already; this changes which reason it + // is dropped for, not whether it is dropped. + // + // A `None` means the phase carries no fixed relationship and the + // frame is left alone rather than rejected, so an unrecognised phase + // still reaches the dispatch below and is handled there. + if let Some(expected) = expected_payload_len(prefix.phase, packet.data.len()) + && prefix.payload_len != expected + { + debug!( + phase = prefix.phase, + declared = prefix.payload_len, + expected, + len = packet.data.len(), + transport_id = %packet.transport_id, + "FMP payload_len disagrees with frame length, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Transport(TransportReject::PayloadLenMismatch)); + return; + } + match prefix.phase { PHASE_ESTABLISHED => { self.handle_encrypted_frame(packet).await; diff --git a/src/node/reject.rs b/src/node/reject.rs index 13c6cb58..c270c637 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -298,10 +298,10 @@ pub enum ForwardingReject { /// Transport-layer rejection reasons. /// -/// Currently covers the admission cap-hit path at the TCP and Tor -/// accept loops. Additional transport-side rejection variants -/// (framing errors, connection failures wired through to the node -/// stats path) can be added incrementally. +/// Covers the admission cap-hit path at the TCP and Tor accept loops +/// and the frame-length check at the receive dispatch point. Additional +/// transport-side rejection variants (connection failures wired through +/// to the node stats path) can be added incrementally. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum TransportReject { @@ -310,6 +310,13 @@ pub enum TransportReject { /// (`max_inbound_connections`) was already reached. Tracked via /// [`TransportStats::inbound_cap_exceeded`](crate::node::stats::TransportStats). InboundCapExceeded, + /// Inbound FMP frame dropped because the payload length its header + /// declares does not match the frame the transport delivered. This + /// is a framing rejection, not an admission one: it applies to + /// established data frames as well as to handshake frames, and it + /// is decided before the phase dispatch. Tracked via + /// [`TransportStats::payload_len_mismatch`](crate::node::stats::TransportStats). + PayloadLenMismatch, } #[cfg(test)] @@ -403,4 +410,18 @@ mod tests { RejectReason::Transport(TransportReject::InboundCapExceeded) )); } + + #[test] + fn transport_reject_payload_len_mismatch_round_trips() { + let r = RejectReason::Transport(TransportReject::PayloadLenMismatch); + assert!(matches!( + r, + RejectReason::Transport(TransportReject::PayloadLenMismatch) + )); + assert_ne!( + r, + RejectReason::Transport(TransportReject::InboundCapExceeded), + "a framing drop must not compare equal to an admission rejection" + ); + } } diff --git a/src/node/stats.rs b/src/node/stats.rs index ecaa830f..acf5f7c9 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -202,6 +202,10 @@ impl MmpStats { /// typed-rejection enum stays the canonical entry point and so a /// future transport-to-node bridge (event or sampling) has a /// well-known destination. +/// +/// `payload_len_mismatch` is different in kind: it counts a framing +/// drop the node itself performs at the receive dispatch point, so it +/// has a live writer and can be non-zero on a running node. #[derive(Default)] pub struct TransportStats { /// Reserved for node-side inbound-cap-exceeded admission rejection @@ -209,18 +213,24 @@ pub struct TransportStats { /// on the transport-level stats (`TcpStats::connections_rejected`, /// `TorStats::connections_rejected`) directly. pub inbound_cap_exceeded: u64, + /// Inbound FMP frames dropped because the payload length declared + /// in the common prefix did not match the frame the transport + /// delivered. + pub payload_len_mismatch: u64, } impl TransportStats { pub fn snapshot(&self) -> TransportStatsSnapshot { TransportStatsSnapshot { inbound_cap_exceeded: self.inbound_cap_exceeded, + payload_len_mismatch: self.payload_len_mismatch, } } pub(super) fn record_reject(&mut self, reason: TransportReject) { match reason { TransportReject::InboundCapExceeded => self.inbound_cap_exceeded += 1, + TransportReject::PayloadLenMismatch => self.payload_len_mismatch += 1, } } } @@ -396,6 +406,7 @@ pub struct MmpStatsSnapshot { #[derive(Clone, Debug, Default, Serialize)] pub struct TransportStatsSnapshot { pub inbound_cap_exceeded: u64, + pub payload_len_mismatch: u64, } #[derive(Clone, Debug, Default, Serialize)] @@ -577,4 +588,25 @@ mod tests { stats.record_reject(RejectReason::Transport(TransportReject::InboundCapExceeded)); assert_eq!(stats.transport.inbound_cap_exceeded, 1); } + + /// Records both transport reasons so a swapped or shared arm shows up + /// as a mis-attributed counter rather than as a plausible total. + #[test] + fn transport_stats_record_reject_keeps_the_two_reasons_on_separate_counters() { + let mut s = TransportStats::default(); + s.record_reject(TransportReject::PayloadLenMismatch); + s.record_reject(TransportReject::PayloadLenMismatch); + s.record_reject(TransportReject::InboundCapExceeded); + assert_eq!(s.payload_len_mismatch, 2); + assert_eq!(s.inbound_cap_exceeded, 1); + } + + #[test] + fn node_stats_record_reject_dispatches_payload_len_mismatch_to_transport() { + let mut stats = NodeStats::new(); + stats.record_reject(RejectReason::Transport(TransportReject::PayloadLenMismatch)); + assert_eq!(stats.transport.payload_len_mismatch, 1); + assert_eq!(stats.transport.inbound_cap_exceeded, 0); + assert_eq!(stats.handshake.bad_state, 0); + } } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index f6ea31b3..04be27f4 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -1151,3 +1151,137 @@ async fn test_should_admit_msg1_admits_rekey_when_addr_form_differs() { assert!(node.is_established_link_msg1(transport_id, &numeric_addr)); assert!(!node.is_established_link_msg1(transport_id, &stranger_addr)); } + +// ============================================================================ +// Frame-length validation at the dispatch point +// ============================================================================ + +/// Build a promoted peer and return the node, the peer's address, and the +/// session index inbound frames must name to reach it. +/// +/// `handle_encrypted_frame` looks a frame up by `(transport_id, receiver_idx)` +/// in `peers_by_index`, so a frame carrying this index reaches the decrypt and +/// bumps the peer's failure counter. That counter is how the tests below tell +/// "the frame reached its handler" apart from "the frame was dropped before +/// the dispatch": an unknown session is dropped silently and counts nothing. +fn node_with_promoted_peer(transport_id: TransportId) -> (Node, NodeAddr, SessionIndex) { + let mut node = make_node(); + let link_id = LinkId::new(1); + let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1_000); + let node_addr = *identity.node_addr(); + node.add_connection(conn).unwrap(); + node.promote_connection(link_id, identity, 2_000).unwrap(); + let our_index = node + .get_peer(&node_addr) + .and_then(|p| p.our_index()) + .expect("promoted peer must have our_index"); + (node, node_addr, our_index) +} + +/// A well-formed established frame carrying 40 bytes of inner plaintext. +/// +/// 16-byte header + 40 + 16-byte tag = 72 bytes on the wire, declaring 40. +/// The ciphertext is filler: these tests are about the length check, and +/// every one of them stops before or at the AEAD. +fn established_frame_declaring_40(receiver_idx: SessionIndex) -> Vec { + use crate::node::wire::{build_encrypted, build_established_header}; + use crate::noise::TAG_SIZE; + + let header = build_established_header(receiver_idx, 0, 0, 40); + build_encrypted(&header, &[0u8; 40 + TAG_SIZE]) +} + +#[tokio::test] +async fn an_established_frame_whose_declared_payload_len_disagrees_with_its_length_is_dropped() { + let transport_id = TransportId::new(1); + let (mut node, node_addr, our_index) = node_with_promoted_peer(transport_id); + + let mut frame = established_frame_declaring_40(our_index); + // Bytes 2-3 are the little-endian payload_len. 68 is what a validator + // written to the field's looser description would compute for this frame + // (72 on the wire minus the 4-byte common prefix), so it is both a wrong + // value and the specific wrong value worth naming. + frame[2..4].copy_from_slice(&68u16.to_le_bytes()); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 1, + "the frame must be counted as a framing drop" + ); + assert_eq!( + node.get_peer(&node_addr) + .expect("peer must survive a dropped frame") + .consecutive_decrypt_failures(), + 0, + "the frame must be dropped before the phase dispatch, so the \ + encrypted-frame handler never sees it" + ); +} + +#[tokio::test] +async fn an_established_frame_with_a_correct_payload_len_is_not_dropped() { + let transport_id = TransportId::new(1); + let (mut node, node_addr, our_index) = node_with_promoted_peer(transport_id); + + let frame = established_frame_declaring_40(our_index); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 0, + "a frame whose header agrees with its length must not be dropped" + ); + assert_eq!( + node.get_peer(&node_addr) + .expect("peer must survive a failed decrypt below the threshold") + .consecutive_decrypt_failures(), + 1, + "the frame must reach the encrypted-frame handler, where the filler \ + ciphertext fails the AEAD tag" + ); +} + +#[tokio::test] +async fn a_msg1_with_a_correct_payload_len_is_not_dropped() { + use crate::node::wire::build_msg1; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + // A real-shaped msg1 with filler Noise bytes: `build_msg1` writes the + // only payload_len the msg1 arm accepts, and the handshake fails one + // step later at the DH, which is the observable that it got there. + let frame = build_msg1(SessionIndex::new(1), &[0u8; 106]); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 0, + "a msg1 built by the encoder must not be dropped by the length check" + ); + assert_eq!( + node.stats().handshake.bad_state, + 1, + "the msg1 must reach handle_msg1, which rejects the filler Noise \ + payload at the DH" + ); +} diff --git a/src/node/wire.rs b/src/node/wire.rs index cb8c7208..9ae59af4 100644 --- a/src/node/wire.rs +++ b/src/node/wire.rs @@ -87,11 +87,6 @@ pub const FLAG_SP: u8 = 0x04; /// A `None` from the established arm means "no fixed relationship", so a caller /// skips such a frame rather than rejecting it. That is the safe direction: a /// truncating cast here would produce a false rejection instead. -// Nothing calls this yet. The dispatch point that calls it arrives in the -// following commit, which removes this annotation with it. `CommonPrefix::flags` -// and `EncryptedHeader::payload_len` below carry the annotation for the same -// reason. -#[allow(dead_code)] pub fn expected_payload_len(phase: u8, total: usize) -> Option { match phase { PHASE_MSG1 => Some((MSG1_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16), diff --git a/src/transport/ble/io.rs b/src/transport/ble/io.rs index 178a8a2f..6b053492 100644 --- a/src/transport/ble/io.rs +++ b/src/transport/ble/io.rs @@ -23,6 +23,15 @@ pub trait BleStream: Send + Sync { /// Receive data from the L2CAP connection. /// /// Returns the number of bytes read into `buf`. + /// + /// A single call must never return bytes drawn from more than one SDU. + /// The receive loop emits what one call returns as one FMP frame. A + /// concatenation is caught by the frame-length check in the node's + /// dispatch only when the leading frame is an established one; where it + /// is a handshake frame, that check passes and the buffer is dropped one + /// step later by that frame kind's exact-size parse. Returning less than + /// a whole SDU is allowed: a truncated frame fails the AEAD tag or the + /// exact-size parse regardless. fn recv( &self, buf: &mut [u8], From f7085f2ed73216aff85be3ab703a158f2c8b5d3f Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 16:03:23 +0000 Subject: [PATCH 16/17] Confirm the peer identity after the handshake, not just the address An inbound setup message from the address of an established peer is admitted even when the node is configured to refuse inbound connections. That carve-out exists so a peer that re-handshakes is not locked out, but it decided purely on the address, so an off-path party sourcing from that address was admitted too. The waiver now classifies into three outcomes rather than two: no waiver was needed, a waiver was used and an owning identity is known, or a waiver was used and no identity owns the link. The third rejects after the key exchange. An earlier shape returned nothing for that case, which silently skipped the check for an ordinary population, which is the defect this change exists to close. The address lookup deliberately falls through rather than returning when it finds no owning identity. The reverse lookup can name a link that no longer exists, because removal clears it only under the key rebuilt from the link's own address, while the cross-connection path inserts a second key from the observed source address. Returning there would refuse a peer the address scan can still attribute, and refuse it permanently: the confirmation returns above the insert that repairs the map, so nothing downstream would ever fix it. Six tests, each broken to prove it can fail. The refusing transport is a real bound socket rather than the loopback handle, which has no refuse override, so "no response was sent" is an observation on a wire rather than a property of a transport that was never started. --- src/node/handlers/handshake.rs | 136 +++++++++ src/node/handlers/mod.rs | 2 +- src/node/tests/handshake.rs | 526 +++++++++++++++++++++++++++++++++ 3 files changed, 663 insertions(+), 1 deletion(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index cc73a1ea..f5d4ce52 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -1,5 +1,6 @@ //! Handshake handlers and connection promotion. +use crate::NodeAddr; use crate::PeerIdentity; use crate::node::acl::PeerAclContext; use crate::node::rate_limit::Msg1Class; @@ -11,6 +12,28 @@ use crate::transport::{Link, LinkDirection, LinkId, ReceivedPacket}; use std::time::Duration; use tracing::{debug, info, warn}; +/// Why an inbound msg1 got past the `accept_connections` gate, and against +/// what identity the post-DH confirmation must check it. +/// +/// Three outcomes, not two: an `Option` would conflate "no waiver was needed" +/// with "the waiver was used and nobody owns the matched address", and the +/// second of those is the case that must reject. +#[derive(Debug, PartialEq, Eq)] +pub(in crate::node) enum Msg1Waiver { + /// The transport accepts fresh inbound handshakes (or no transport is + /// registered), so the address carve-out did not admit this msg1 and + /// there is nothing to confirm. + NotNeeded, + /// The carve-out is what admitted this msg1, and the matched address + /// belongs to this identity: either a promoted peer, or a handshake + /// already in flight on the matched link whose identity is expected + /// (outbound dial) or already learned (inbound msg1). + Expect(NodeAddr), + /// The carve-out is what admitted this msg1, and no identity can be + /// attributed to the matched address. Fail closed: reject after the DH. + Unattributed, +} + impl Node { /// Returns true if an inbound msg1's source matches an established /// link, i.e. it is rekey/restart maintenance traffic rather than a @@ -63,6 +86,80 @@ impl Node { false } + /// Classify the msg1 waiver for a source that `should_admit_msg1` + /// admitted, so the post-DH confirmation knows whether it has an + /// identity to check against and what to do when it has none. + /// + /// `established` is the caller's already-computed + /// `is_established_link_msg1(...)`, so the O(peers) scan is not repeated + /// on the refusal path. + /// + /// The two attribution limbs are composed the same way + /// `is_established_link_msg1` composes its own: as an OR, not as an + /// if/else. An `addr_to_link` entry that yields no identity must not + /// short-circuit the address scan, because the two keys can be different + /// forms of the same peer's address (the hostname-vs-numeric case that + /// predicate 2 exists for) and the entry can outlive the link it named. + pub(in crate::node) fn msg1_waiver( + &self, + established: bool, + transport_id: crate::transport::TransportId, + remote_addr: &crate::transport::TransportAddr, + ) -> Msg1Waiver { + // The carve-out only admits anything when the gate would otherwise + // refuse, so an accepting transport has nothing to confirm. + if self + .transports + .get(&transport_id) + .is_none_or(|t| t.accept_connections()) + { + return Msg1Waiver::NotNeeded; + } + if !established { + // `should_admit_msg1` refused this msg1 and the caller returned, + // so this arm is unreachable from the one call site. Fail closed + // rather than skipping the check, so a second caller cannot + // reintroduce the hole this classifier exists to close. + return Msg1Waiver::Unattributed; + } + + // Predicate 1: the reverse-address lookup. + if let Some(&link_id) = self.addr_to_link.get(&(transport_id, remote_addr.clone())) { + if let Some(peer) = self.peers.values().find(|p| p.link_id() == link_id) { + return Msg1Waiver::Expect(*peer.node_addr()); + } + // A link with no promoted peer: a dial in progress or an inbound + // handshake in flight. Both register a connection carrying the + // expected (outbound) or learned (inbound) identity. + if let Some(id) = self + .connections + .get(&link_id) + .and_then(|c| c.expected_identity()) + { + return Msg1Waiver::Expect(*id.node_addr()); + } + // Deliberately fall through instead of returning. The entry can + // name a link that no longer exists — `remove_link` clears the + // reverse lookup only under the key it rebuilds from the link's + // own remote address, so an entry inserted under a second + // address form for that link survives its removal. Rejecting + // here would refuse a peer predicate 2 can still attribute, and + // would refuse it permanently: this classifier's caller returns + // above the insert that overwrites the stale entry, so nothing + // downstream would ever repair the map. + } + + // Predicate 2: the address scan over promoted peers, which always + // yields an identity when it matches. + self.peers + .values() + .find(|p| { + p.transport_id() == Some(transport_id) && p.current_addr() == Some(remote_addr) + }) + .map(|p| Msg1Waiver::Expect(*p.node_addr())) + .unwrap_or(Msg1Waiver::Unattributed) + } + /// Returns true if an inbound msg1 should be admitted past the /// `accept_connections` gate. /// @@ -135,6 +232,12 @@ impl Node { return; } + // Snapshot which identity, if any, the address carve-out attributed + // this source to. Taken here rather than after the DH so the answer + // is the one the gate acted on. On an accepting transport this is one + // map lookup and a return. + let waiver = self.msg1_waiver(established, packet.transport_id, &packet.remote_addr); + // Parse header let header = match Msg1Header::parse(&packet.data) { Some(h) => h, @@ -263,6 +366,39 @@ impl Node { let peer_node_addr = *peer_identity.node_addr(); + // The address carve-out admitted this msg1 past a refusing gate on + // the strength of the source address alone. Now that the DH has + // revealed the initiator's static, confirm it belongs to the party + // that address is attributed to; an off-path party sourcing from an + // established peer's address gets no further than here. Cheap + // rejection is unchanged: a stranger under accept_connections=false + // is still refused above, having paid nothing. + match waiver { + Msg1Waiver::NotNeeded => {} + Msg1Waiver::Expect(expected) if expected == peer_node_addr => {} + Msg1Waiver::Expect(expected) => { + warn!( + expected = %self.peer_display_name(&expected), + actual = %self.peer_display_name(&peer_node_addr), + transport_id = %packet.transport_id, + "Msg1 admitted by the established-address waiver carries a different identity, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + return; + } + Msg1Waiver::Unattributed => { + warn!( + actual = %self.peer_display_name(&peer_node_addr), + transport_id = %packet.transport_id, + "Msg1 admitted by the established-address waiver, but no identity owns that address, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + return; + } + } + // Identity-based restart/rekey detection: if the peer is already // active but addr_to_link didn't match (different source address, e.g., // TCP from a different port), we still need to check for restart/rekey. diff --git a/src/node/handlers/mod.rs b/src/node/handlers/mod.rs index b5d2e829..5270cad3 100644 --- a/src/node/handlers/mod.rs +++ b/src/node/handlers/mod.rs @@ -6,7 +6,7 @@ pub(crate) mod discovery; mod dispatch; mod encrypted; mod forwarding; -mod handshake; +pub(in crate::node) mod handshake; mod mmp; mod rekey; mod rx_loop; diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 04be27f4..78a8675c 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -1285,3 +1285,529 @@ async fn a_msg1_with_a_correct_payload_len_is_not_dropped() { payload at the DH" ); } + +// ============================================================================ +// Identity confirmation after the DH (the address-keyed msg1 carve-out) +// ============================================================================ + +/// A node whose only transport refuses fresh inbound handshakes, paired with +/// a second started UDP socket standing in for the far end. +/// +/// Returns the node, the far end's address, the far end's receive channel, +/// and the far end's transport, which the caller must keep alive for its +/// receive loop to go on running. +/// +/// Both transports are started on purpose. A msg2 the node decides to send +/// then really leaves it and really arrives on the returned channel, which is +/// what makes "no msg2 was sent" an observation about the confirmation rather +/// than a property of the fixture: on an unstarted transport every send +/// fails, and the send-failure arm records the same reject the confirmation +/// records, so the two worlds would be indistinguishable. +async fn node_refusing_inbound( + transport_id: TransportId, +) -> ( + Node, + TransportAddr, + crate::transport::PacketRx, + crate::transport::udp::UdpTransport, +) { + use crate::config::UdpConfig; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + + let refusing = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let mut udp = UdpTransport::new(transport_id, None, refusing, tx); + udp.start_async().await.expect("node transport must bind"); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + let far_end_config = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }; + let (far_tx, far_rx) = packet_channel(64); + let mut far_end = UdpTransport::new(TransportId::new(200), None, far_end_config, far_tx); + far_end.start_async().await.expect("far end must bind"); + let far_addr = TransportAddr::from_string( + &far_end + .local_addr() + .expect("a started transport has a local address") + .to_string(), + ); + + (node, far_addr, far_rx, far_end) +} + +/// A real, cryptographically valid msg1 from `initiator` addressed to +/// `responder`'s static key. +/// +/// It has to be valid under our static, or `receive_handshake_init` refuses +/// it for the wrong reason and the test passes without ever reaching the +/// confirmation. +fn genuine_msg1(initiator: &Node, responder: &Node) -> Vec { + use crate::node::wire::build_msg1; + + let responder_identity = PeerIdentity::from_pubkey_full(responder.identity().pubkey_full()); + let mut conn = PeerConnection::outbound(LinkId::new(9_999), responder_identity, 1_000); + let noise_msg1 = conn + .start_handshake( + initiator.identity().keypair(), + initiator.startup_epoch(), + 1_000, + ) + .expect("the initiator side of a real msg1 must build"); + build_msg1(SessionIndex::new(7), &noise_msg1) +} + +/// The node address a `Node`'s own identity presents to its peers. +fn node_addr_of(node: &Node) -> NodeAddr { + *PeerIdentity::from_pubkey_full(node.identity().pubkey_full()).node_addr() +} + +/// The FMP phase byte of a packet, for telling a msg1 from a msg2 on the wire. +fn wire_phase(data: &[u8]) -> Option { + crate::node::wire::CommonPrefix::parse(data).map(|p| p.phase) +} + +/// Assert that nothing arrives on `rx` within a window long enough for a +/// localhost datagram to have been delivered had one been sent. +async fn assert_nothing_sent(rx: &mut crate::transport::PacketRx, why: &str) { + let arrival = tokio::time::timeout(std::time::Duration::from_millis(250), rx.recv()).await; + assert!(arrival.is_err(), "{}", why); +} + +#[tokio::test] +async fn a_msg1_spoofed_from_an_established_peers_address_is_dropped_after_the_dh_reveals_a_different_identity() + { + use crate::peer::ActivePeer; + + let transport_id = TransportId::new(1); + let (mut node, victim_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // The victim: a promoted peer at the address the spoofed msg1 will be + // sourced from, so the carve-out admits the msg1 past the refusing gate. + let victim = make_node(); + let victim_identity = PeerIdentity::from_pubkey_full(victim.identity().pubkey_full()); + let victim_node_addr = *victim_identity.node_addr(); + let victim_link = node.allocate_link_id(); + let mut victim_peer = ActivePeer::new(victim_identity, victim_link, 1_000); + victim_peer.set_current_addr(transport_id, victim_addr.clone()); + node.peers.insert(victim_node_addr, victim_peer); + node.addr_to_link + .insert((transport_id, victim_addr.clone()), victim_link); + + // The off-path party: a genuine msg1 under our static, built with a + // different identity's keypair, sourced from the victim's address. + let attacker = make_node(); + let attacker_node_addr = node_addr_of(&attacker); + let wire_msg1 = genuine_msg1(&attacker, &node); + + let bad_state_before = node.stats().handshake.bad_state; + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + victim_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + assert!( + node.get_peer(&attacker_node_addr).is_none(), + "the identity the DH revealed does not own the address the waiver \ + matched, so it must not become a peer" + ); + assert_eq!( + node.peer_count(), + 1, + "only the victim may remain a peer after the spoofed msg1" + ); + assert_eq!( + node.connection_count(), + 0, + "the rejected msg1 must leave no connection behind" + ); + assert_eq!( + node.link_count(), + 0, + "the rejected msg1 must leave no link behind" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); + assert_nothing_sent( + &mut far_rx, + "no msg2 may reach the victim's address: the responder answers only \ + after the confirmation, and this msg1 must not get that far", + ) + .await; +} + +#[tokio::test] +async fn a_msg1_admitted_by_a_link_no_identity_owns_is_dropped_after_the_dh() { + let transport_id = TransportId::new(1); + let (mut node, source_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // The fixture `test_should_admit_msg1_admits_rekey_when_accept_off` uses: + // a reverse-address entry with no peer and no connection behind it. It is + // enough to waive the refusing gate, and it attributes the address to + // nobody. + let link_id = node.allocate_link_id(); + node.addr_to_link + .insert((transport_id, source_addr.clone()), link_id); + + assert!( + node.should_admit_msg1(transport_id, &source_addr), + "the fixture must exercise the carve-out, not the gate" + ); + + let initiator = make_node(); + let initiator_node_addr = node_addr_of(&initiator); + let wire_msg1 = genuine_msg1(&initiator, &node); + + let bad_state_before = node.stats().handshake.bad_state; + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + source_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + assert!( + node.get_peer(&initiator_node_addr).is_none(), + "a msg1 the waiver could attribute to no identity must not promote \ + the identity the DH revealed" + ); + assert_eq!(node.peer_count(), 0, "no peer may be created"); + assert_eq!( + node.connection_count(), + 0, + "the rejected msg1 must leave no connection behind" + ); + assert_eq!( + node.link_count(), + 0, + "the rejected msg1 must leave no link behind" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); + assert_nothing_sent( + &mut far_rx, + "no msg2 may leave the node for an address no identity owns", + ) + .await; +} + +#[tokio::test] +async fn a_msg1_from_a_link_whose_dial_expects_this_identity_is_admitted() { + let transport_id = TransportId::new(1); + let (mut node, peer_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // UDP is connectionless, so `initiate_connection` runs `start_handshake` + // in the same synchronous stretch: the reverse-address entry and the + // connection carrying the dialled identity land together, and the + // classifier can attribute the address. Do not substitute a + // connection-oriented transport here; that arm defers `start_handshake` + // and is the window the next test is about. + let peer = make_node(); + let peer_identity = PeerIdentity::from_pubkey_full(peer.identity().pubkey_full()); + node.initiate_connection(transport_id, peer_addr.clone(), peer_identity) + .await + .expect("the dial must register the link and the connection"); + + // Our own dial's msg1 went out first; drain it so the assertion below is + // about the answer to the crossing msg1. + let ours = tokio::time::timeout(std::time::Duration::from_secs(1), far_rx.recv()) + .await + .expect("our dial's msg1 must arrive") + .expect("the far end's channel must be open"); + assert_eq!( + wire_phase(&ours.data), + Some(crate::node::wire::PHASE_MSG1), + "the dial's own packet is a msg1" + ); + + let bad_state_before = node.stats().handshake.bad_state; + let wire_msg1 = genuine_msg1(&peer, &node); + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + peer_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + let answer = tokio::time::timeout(std::time::Duration::from_secs(1), far_rx.recv()) + .await + .expect("the crossing msg1 must be answered") + .expect("the far end's channel must be open"); + assert_eq!( + wire_phase(&answer.data), + Some(crate::node::wire::PHASE_MSG2), + "a simultaneous open with the peer we dialled must still be answered" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 0, + "a crossing msg1 from the identity the dial expects must not be \ + rejected" + ); +} + +#[tokio::test] +async fn a_crossing_msg1_in_the_connection_oriented_dial_window_is_rejected() { + use crate::config::TcpConfig; + use crate::transport::tcp::TcpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + // bind_addr=None makes accept_connections() false, the idiom + // `test_should_admit_msg1_rejects_fresh_when_accept_off` uses. + let cfg = TcpConfig { + bind_addr: None, + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let tcp = TcpTransport::new(transport_id, None, cfg, tx); + node.transports + .insert(transport_id, TransportHandle::Tcp(tcp)); + + let peer = make_node(); + let peer_identity = PeerIdentity::from_pubkey_full(peer.identity().pubkey_full()); + let addr = TransportAddr::from_string("10.0.0.2:2121"); + + // The state `initiate_connection`'s connection-oriented arm leaves behind + // while the transport connect is outstanding: a link, a reverse-address + // entry, a pending connect, and no connection yet. Built directly rather + // than by driving a connect, which would need a reachable peer. + let link_id = node.allocate_link_id(); + let link = Link::new( + link_id, + transport_id, + addr.clone(), + LinkDirection::Outbound, + Duration::from_millis(100), + ); + node.links.insert(link_id, link); + node.addr_to_link + .insert((transport_id, addr.clone()), link_id); + node.pending_connects.push(PendingConnect { + link_id, + transport_id, + remote_addr: addr.clone(), + peer_identity, + }); + + let bad_state_before = node.stats().handshake.bad_state; + let wire_msg1 = genuine_msg1(&peer, &node); + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + // This is the assertion that separates the two worlds, and it is the one + // to read first when the test reds. After the change the reject returns + // above every registry mutation, so the dial's own entry is untouched. + // Without it, `handle_msg1` allocates a fresh link id, writes it over + // this entry, and then removes the entry outright when the msg2 send + // fails on the unstarted transport. + assert_eq!( + node.addr_to_link.get(&(transport_id, addr.clone())), + Some(&link_id), + "the dial's reverse-address entry must still name the dial's own link" + ); + // The remaining three state the shape of the outcome. They hold either + // way for this fixture, whose unstarted TCP transport cannot send, so + // they are not what makes this test able to fail. + assert_eq!(node.peer_count(), 0, "no peer may be created"); + assert_eq!(node.connection_count(), 0, "no connection may be created"); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); +} + +#[tokio::test] +async fn msg1_waiver_classifies_all_three_outcomes() { + use crate::config::UdpConfig; + use crate::node::handlers::handshake::Msg1Waiver; + use crate::peer::ActivePeer; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + + // A registered accepting transport, not an unregistered id, so the + // NotNeeded assertion exercises the `accept_connections()` limb of the + // guard rather than its absent-transport fallback. + let accepting_id = TransportId::new(1); + let (accept_tx, _accept_rx) = packet_channel(64); + let accepting = UdpTransport::new( + accepting_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(true), + ..Default::default() + }, + accept_tx, + ); + node.transports + .insert(accepting_id, TransportHandle::Udp(accepting)); + + let refusing_id = TransportId::new(2); + let (refuse_tx, _refuse_rx) = packet_channel(64); + let refusing = UdpTransport::new( + refusing_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }, + refuse_tx, + ); + node.transports + .insert(refusing_id, TransportHandle::Udp(refusing)); + + let classify = |node: &Node, transport_id: TransportId, addr: &TransportAddr| { + node.msg1_waiver( + node.is_established_link_msg1(transport_id, addr), + transport_id, + addr, + ) + }; + + // NotNeeded: the gate would have admitted this msg1 anyway, so nothing + // was waived and there is nothing to confirm. + let fresh = TransportAddr::from_string("10.0.0.2:2121"); + assert_eq!( + classify(&node, accepting_id, &fresh), + Msg1Waiver::NotNeeded, + "an accepting transport waives nothing" + ); + + // Unattributed: the carve-out admits on a bare reverse-address entry + // that names neither a promoted peer nor a carrier. + let bare = TransportAddr::from_string("10.0.0.3:2121"); + let bare_link = node.allocate_link_id(); + node.addr_to_link + .insert((refusing_id, bare.clone()), bare_link); + assert_eq!( + classify(&node, refusing_id, &bare), + Msg1Waiver::Unattributed, + "a link no identity owns must fail closed" + ); + + // Expect, by promoted peer. `ActivePeer::new` sets neither transport_id + // nor current_addr, so this peer is reached through the reverse-address + // entry that names its link. + let promoted_addr = TransportAddr::from_string("10.0.0.4:2121"); + let promoted_link = node.allocate_link_id(); + let promoted = make_peer_identity(); + let promoted_node_addr = *promoted.node_addr(); + node.peers.insert( + promoted_node_addr, + ActivePeer::new(promoted, promoted_link, 1_000), + ); + node.addr_to_link + .insert((refusing_id, promoted_addr.clone()), promoted_link); + assert_eq!( + classify(&node, refusing_id, &promoted_addr), + Msg1Waiver::Expect(promoted_node_addr), + "an address whose link a promoted peer owns is attributed to that peer" + ); + + // Expect, by carrier: a link with no promoted peer yet, whose connection + // carries the identity the dial expects. + let carrier_addr = TransportAddr::from_string("10.0.0.5:2121"); + let carrier_link = node.allocate_link_id(); + let dialled = make_peer_identity(); + let dialled_node_addr = *dialled.node_addr(); + node.connections.insert( + carrier_link, + PeerConnection::outbound(carrier_link, dialled, 1_000), + ); + node.addr_to_link + .insert((refusing_id, carrier_addr.clone()), carrier_link); + assert_eq!( + classify(&node, refusing_id, &carrier_addr), + Msg1Waiver::Expect(dialled_node_addr), + "an address whose link carries a dialled identity is attributed to it" + ); +} + +#[tokio::test] +async fn a_stale_reverse_address_entry_does_not_hide_a_peer_reachable_by_address() { + use crate::config::UdpConfig; + use crate::node::handlers::handshake::Msg1Waiver; + use crate::peer::ActivePeer; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + let (tx, _rx) = packet_channel(64); + let udp = UdpTransport::new( + transport_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }, + tx, + ); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + // A live peer whose current_addr is the numeric form inbound packets + // carry, which is the second predicate's whole reason for existing. + let numeric = TransportAddr::from_string("100.64.0.5:2121"); + let peer_link = node.allocate_link_id(); + let peer_identity = make_peer_identity(); + let peer_node_addr = *peer_identity.node_addr(); + let mut peer = ActivePeer::new(peer_identity, peer_link, 1_000); + peer.set_current_addr(transport_id, numeric.clone()); + node.peers.insert(peer_node_addr, peer); + + // A reverse-address entry at that same numeric address naming a link + // that no longer exists. `remove_link` clears the reverse lookup only + // under the key it rebuilds from the removed link's own remote address, + // so an entry inserted for that link under a second address form outlives + // it, and link ids are never reused. + let dead_link = node.allocate_link_id(); + assert_ne!( + dead_link, peer_link, + "the stale entry names a different link" + ); + node.addr_to_link + .insert((transport_id, numeric.clone()), dead_link); + + assert_eq!( + node.msg1_waiver( + node.is_established_link_msg1(transport_id, &numeric), + transport_id, + &numeric + ), + Msg1Waiver::Expect(peer_node_addr), + "the stale entry must not hide the peer the address scan finds: \ + classifying this Unattributed would reject the peer's rekey msg1 \ + after the DH, and would go on rejecting it, because the reject \ + returns above the insert that would overwrite the stale entry" + ); +} From f07480ecb5f7d477eb643affa5917298390fb178 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 16 Aug 2026 17:41:58 +0000 Subject: [PATCH 17/17] Source the stream reader's framing constants from the FMP wire module The reader restated ten wire facts as literals that the FMP wire module already derives from the Noise message sizes. The two sets could drift, and only one direction of drift was caught. Re-sourced: the three phase values, the prefix size, the established remaining header, the AEAD tag size, and both handshake wire sizes. The two payload-length constants derive from the local wire sizes and become correct without being touched. The version gate compared against a bare zero and now names FMP_VERSION. The AEAD tag size comes from the Noise module's TAG_SIZE rather than by subtracting the established header size from the encrypted minimum, which would round-trip through a definition to recover a value that can be named directly. This is done here and not on the maintenance line. There the only place holding these values is the node layer, which sits above transport, so the same edit would create the first dependency from transport up into node. Here the values live in a protocol module that already imports from transport, so re-sourcing adds no new layering that the tree does not already have. The constants cross-check becomes a tautology as a result: every assertion now compares a constant with itself. It is relabelled to say that it asserts sourcing, not agreement, and to record that the drift coverage it used to provide is gone. What still catches a wrong value is the TCP suite, which moves real frames through this reader against separately written literals. Four test-only duplicates of the same wire facts are deliberately left alone, in the Tor and Nym handshake tests, in this module's own frame builders, and in the TCP established-frame arithmetic. Folding them onto the shared builders would delete the independent coverage named above. --- src/transport/framing.rs | 49 ++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/src/transport/framing.rs b/src/transport/framing.rs index 6e317b5e..859c8974 100644 --- a/src/transport/framing.rs +++ b/src/transport/framing.rs @@ -6,22 +6,22 @@ //! This module is deliberately separate from any single transport so it can //! be shared by the stream-oriented transports (TCP, Tor, Nym). +use crate::noise::TAG_SIZE; +use crate::proto::fmp::wire; use tokio::io::{AsyncRead, AsyncReadExt}; /// FMP phase values (low nibble of byte 0). -const PHASE_ESTABLISHED: u8 = 0x0; -const PHASE_MSG1: u8 = 0x1; -const PHASE_MSG2: u8 = 0x2; +const PHASE_ESTABLISHED: u8 = wire::PHASE_ESTABLISHED; +const PHASE_MSG1: u8 = wire::PHASE_MSG1; +const PHASE_MSG2: u8 = wire::PHASE_MSG2; /// Size of the FMP common prefix. -const PREFIX_SIZE: usize = 4; +const PREFIX_SIZE: usize = wire::COMMON_PREFIX_SIZE; -/// Overhead for established frames: 12 bytes remaining header + 16 bytes AEAD tag. -/// The full established header is 16 bytes (PREFIX_SIZE + 12), so after reading -/// the 4-byte prefix, 12 more header bytes remain. Then payload_len bytes of -/// ciphertext, then 16 bytes of AEAD tag. -const ESTABLISHED_REMAINING_HEADER: usize = 12; -const AEAD_TAG_SIZE: usize = 16; +/// Overhead for established frames: the header bytes left after the prefix, +/// then payload_len bytes of ciphertext, then the AEAD tag. +const ESTABLISHED_REMAINING_HEADER: usize = wire::ESTABLISHED_HEADER_SIZE - PREFIX_SIZE; +const AEAD_TAG_SIZE: usize = TAG_SIZE; /// Errors from the FMP stream reader. #[derive(Debug)] @@ -83,8 +83,8 @@ impl From for StreamError { /// Known wire sizes for handshake messages. /// msg1: 4 (prefix) + 4 (sender_idx) + 106 (noise_msg1) = 114 bytes /// msg2: 4 (prefix) + 4 (sender_idx) + 4 (receiver_idx) + 57 (noise_msg2) = 69 bytes -const MSG1_WIRE_SIZE: usize = 114; -const MSG2_WIRE_SIZE: usize = 69; +const MSG1_WIRE_SIZE: usize = wire::MSG1_WIRE_SIZE; +const MSG2_WIRE_SIZE: usize = wire::MSG2_WIRE_SIZE; /// Expected payload_len for msg1: sender_idx(4) + noise_msg1(106) = 110. const MSG1_PAYLOAD_LEN: u16 = (MSG1_WIRE_SIZE - PREFIX_SIZE) as u16; @@ -120,7 +120,7 @@ pub async fn read_fmp_packet( let version = prefix[0] >> 4; let phase = prefix[0] & 0x0F; - if version != 0 { + if version != wire::FMP_VERSION { return Err(StreamError::UnknownVersion(version)); } @@ -229,19 +229,18 @@ mod tests { frame } - /// The wire sizes above are written as literals, independently of the FMP - /// wire module, which derives the same values from the Noise message sizes. - /// Keeping them independent is deliberate: this module takes no dependency - /// on `crate::proto::fmp`, and the import below exists only under - /// `cfg(test)`. - /// The cost is that the two can drift. A wrong literal on this side also - /// breaks the TCP integration tests, since they move real frames through - /// this reader; a wrong value on the wire-module side does not reach here - /// at all, and this test is what catches that direction. + /// States in executable form that the constants above are sourced from the + /// FMP wire module rather than restated here. + /// + /// These assertions no longer discriminate: each side of every one is now + /// the same constant, so this test cannot fail while the sourcing holds, + /// and it is here to fail loudly if someone re-inlines a literal. It + /// replaces a genuine drift check that was meaningful only while the two + /// sets of values were written independently. What still catches a wrong + /// value is the TCP suite in `super::tcp`, which moves real frames through + /// this reader against separately written literals. #[test] - fn stream_reader_constants_agree_with_the_fmp_wire_module() { - use crate::proto::fmp::wire; - + fn stream_reader_constants_are_sourced_from_the_fmp_wire_module() { assert_eq!(MSG1_WIRE_SIZE, wire::MSG1_WIRE_SIZE); assert_eq!(MSG2_WIRE_SIZE, wire::MSG2_WIRE_SIZE); assert_eq!(PREFIX_SIZE, wire::COMMON_PREFIX_SIZE);