diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 0e1dad20..1a0b3e5b 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -51,6 +51,8 @@ "unbound_mtu": 0 }, "forwarding": { + "coord_warm_foreign_root": 0, + "coord_warm_key_mismatch": 0, "decode_error_bytes": 0, "decode_error_packets": 0, "delivered_bytes": 0, diff --git a/src/control/snapshots/show_status.json b/src/control/snapshots/show_status.json index fd2efe5f..3f8b1550 100644 --- a/src/control/snapshots/show_status.json +++ b/src/control/snapshots/show_status.json @@ -6,6 +6,8 @@ "estimated_mesh_size": null, "exe_path": "", "forwarding": { + "coord_warm_foreign_root": 0, + "coord_warm_key_mismatch": 0, "decode_error_bytes": 0, "decode_error_packets": 0, "delivered_bytes": 0, diff --git a/src/node/dataplane/forwarding.rs b/src/node/dataplane/forwarding.rs index 725a8de5..d7436b2b 100644 --- a/src/node/dataplane/forwarding.rs +++ b/src/node/dataplane/forwarding.rs @@ -17,8 +17,9 @@ use crate::proto::fsp::wire::{ use crate::proto::fsp::{SessionAck, SessionSetup}; use crate::proto::link::{SessionDatagram, SessionDatagramRef}; use crate::proto::routing::{DropReason, LimitVerdict, NextHop, RouteAction, RouteOutcome}; +use crate::proto::stp::TreeCoordinate; use std::time::{Duration, Instant}; -use tracing::{debug, warn}; +use tracing::{debug, trace, warn}; impl Node { /// Handle an incoming SessionDatagram from a peer. @@ -226,6 +227,45 @@ impl Node { /// reconstructed from the header size so the malformed-frame byte counter /// measures the same population as its siblings — which are charged the /// outer slice — instead of the inner FSP payload. + /// Warm one coordinate-cache entry from a plaintext session header, after + /// the two write-side sanity checks. + /// + /// The key and the value both come off the wire unauthenticated, so this + /// is the only place a warm write can be filtered at all. Two checks, and + /// they are deliberately of different strengths: + /// + /// **Foreign root: refused.** A coordinate under a root other than ours + /// can never route. `StpState::find_next_hop` returns `None` outright on a + /// root mismatch, and the bloom fallback compares against a `my_distance` + /// of `usize::MAX`, so no candidate is ever strictly closer. Caching one + /// therefore buys nothing and costs something real: the entry's presence + /// is what `synth_routing_error` reads to choose `PathBroken` over + /// `CoordsRequired`, so a foreign-root plant turns this node into a + /// one-packet reflector aimed at whatever source the datagram claimed. + /// `CoordCache::invalidate_other_roots` already applies this same + /// invariant whenever our own tree position moves; this applies it at + /// write time instead of waiting for the next move. + /// + /// **Key mismatch: counted only.** A coordinate whose first element is not + /// the address it is filed under is wrong, but refusing it here would also + /// refuse a write honest nodes make: a sender whose own cache missed puts + /// its *own* coordinates in `SessionSetup.dest_coords`, by way of + /// `get_dest_coords`. What that costs a transit node on first contact is + /// not established, so this counts and does not refuse. It is **not** a + /// security check either way — an attacker satisfies it by naming the + /// victim as its own child, which is the forgery worth making. + fn warm_coord(&mut self, key: NodeAddr, coords: TreeCoordinate, now_ms: u64) { + if coords.root_id() != self.tree_state.my_coords().root_id() { + self.metrics().forwarding.record_warm_foreign_root(); + trace!(addr = %key, "Warm write names a foreign root; not caching"); + return; + } + if *coords.node_addr() != key { + self.metrics().forwarding.record_warm_key_mismatch(); + } + self.coord_cache_mut().insert(key, coords, now_ms); + } + fn try_warm_coord_cache_ref(&mut self, datagram: &SessionDatagramRef<'_>, outer_len: usize) { let prefix = match FspCommonPrefix::parse(datagram.payload) { Some(p) => p, @@ -242,10 +282,8 @@ impl Node { match prefix.phase { FSP_PHASE_MSG1 => match SessionSetup::decode(inner) { Ok(setup) => { - self.coord_cache_mut() - .insert(datagram.src_addr, setup.src_coords, now_ms); - self.coord_cache_mut() - .insert(datagram.dest_addr, setup.dest_coords, now_ms); + self.warm_coord(datagram.src_addr, setup.src_coords, now_ms); + self.warm_coord(datagram.dest_addr, setup.dest_coords, now_ms); debug!( src = %datagram.src_addr, dest = %datagram.dest_addr, @@ -258,10 +296,8 @@ impl Node { }, FSP_PHASE_MSG2 => match SessionAck::decode(inner) { Ok(ack) => { - self.coord_cache_mut() - .insert(datagram.src_addr, ack.src_coords, now_ms); - self.coord_cache_mut() - .insert(datagram.dest_addr, ack.dest_coords, now_ms); + self.warm_coord(datagram.src_addr, ack.src_coords, now_ms); + self.warm_coord(datagram.dest_addr, ack.dest_coords, now_ms); debug!( src = %datagram.src_addr, dest = %datagram.dest_addr, @@ -297,12 +333,10 @@ impl Node { match parse_encrypted_coords(coord_data) { Ok((src_coords, dest_coords, _bytes_consumed)) => { if let Some(coords) = src_coords { - self.coord_cache_mut() - .insert(datagram.src_addr, coords, now_ms); + self.warm_coord(datagram.src_addr, coords, now_ms); } if let Some(coords) = dest_coords { - self.coord_cache_mut() - .insert(datagram.dest_addr, coords, now_ms); + self.warm_coord(datagram.dest_addr, coords, now_ms); } debug!( src = %datagram.src_addr, diff --git a/src/node/metrics.rs b/src/node/metrics.rs index 231780c8..45f2f5b8 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -69,6 +69,18 @@ pub struct ForwardingMetrics { pub decode_error_bytes: Counter, pub warm_malformed_packets: Counter, pub warm_malformed_bytes: Counter, + /// Coordinate-cache warm writes refused because the coordinate names a + /// root other than this node's. Such an entry can never route — both + /// selectors reject a foreign root — so a write of one is either tree + /// churn or a plant, and the counter is the only place the difference + /// shows. + pub coord_warm_foreign_root: Counter, + /// Coordinate-cache warm writes whose coordinate does not name the address + /// it is filed under. Counted, not refused. **This is not a security + /// signal**: an attacker forges a passing coordinate by naming the victim + /// as its own child. It counts a defect honest nodes make, where a sender + /// whose own cache missed sends its own coordinates as the destination's. + pub coord_warm_key_mismatch: Counter, pub ttl_exhausted_packets: Counter, pub ttl_exhausted_bytes: Counter, pub delivered_packets: Counter, @@ -134,6 +146,16 @@ impl ForwardingMetrics { self.warm_malformed_bytes.add(bytes as u64); } + /// Record a warm write refused for naming a foreign root. + pub fn record_warm_foreign_root(&self) { + self.coord_warm_foreign_root.inc(); + } + + /// Record a warm write whose coordinate does not name its own key. + pub fn record_warm_key_mismatch(&self) { + self.coord_warm_key_mismatch.inc(); + } + /// Record a forwarded (transit) packet of `bytes` payload. #[inline] pub fn record_forwarded(&self, bytes: usize) { @@ -202,6 +224,8 @@ impl ForwardingMetrics { decode_error_bytes: self.decode_error_bytes.get(), warm_malformed_packets: self.warm_malformed_packets.get(), warm_malformed_bytes: self.warm_malformed_bytes.get(), + coord_warm_foreign_root: self.coord_warm_foreign_root.get(), + coord_warm_key_mismatch: self.coord_warm_key_mismatch.get(), ttl_exhausted_packets: self.ttl_exhausted_packets.get(), ttl_exhausted_bytes: self.ttl_exhausted_bytes.get(), delivered_packets: self.delivered_packets.get(), diff --git a/src/node/stats.rs b/src/node/stats.rs index 74fe5ed3..23ab304f 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -303,6 +303,8 @@ pub struct ForwardingStatsSnapshot { pub decode_error_bytes: u64, pub warm_malformed_packets: u64, pub warm_malformed_bytes: u64, + pub coord_warm_foreign_root: u64, + pub coord_warm_key_mismatch: u64, pub ttl_exhausted_packets: u64, pub ttl_exhausted_bytes: u64, pub delivered_packets: u64, diff --git a/src/node/tests/forwarding.rs b/src/node/tests/forwarding.rs index fe19f636..eaccba35 100644 --- a/src/node/tests/forwarding.rs +++ b/src/node/tests/forwarding.rs @@ -204,13 +204,103 @@ async fn test_forwarding_direct_peer() { // Coordinate Cache Warming Tests // ============================================================================ +#[tokio::test] +async fn warming_refuses_a_coordinate_rooted_in_a_tree_this_node_is_not_in() { + let mut node = make_node(); + let from = make_node_addr(0xAA); + let src_addr = make_node_addr(0x01); + let dest_addr = make_node_addr(0x02); + // Deliberately NOT this node's root. Such an entry can never route: both + // selectors reject a foreign root, so caching it only occupies a slot and + // flips the error-PDU choice in `synth_routing_error` from CoordsRequired + // to PathBroken, which is the primitive this guard removes. + let foreign_root = make_node_addr(0xF0); + assert_ne!( + &foreign_root, + node.tree_state.my_coords().root_id(), + "fixture must not accidentally share the node's root" + ); + + let src_coords = TreeCoordinate::from_addrs(vec![src_addr, foreign_root]).unwrap(); + let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, foreign_root]).unwrap(); + let setup_payload = SessionSetup::new(src_coords, dest_coords).encode(); + let encoded = SessionDatagram::new(src_addr, dest_addr, setup_payload).encode(); + + let before = node.metrics().forwarding.coord_warm_foreign_root.get(); + node.handle_session_datagram(&from, &encoded[1..], false) + .await; + + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64; + assert!( + node.coord_cache().get(&src_addr, now_ms).is_none(), + "a foreign-root src coordinate was cached" + ); + assert!( + node.coord_cache().get(&dest_addr, now_ms).is_none(), + "a foreign-root dest coordinate was cached" + ); + assert_eq!( + node.metrics().forwarding.coord_warm_foreign_root.get(), + before + 2, + "both refusals should be counted" + ); +} + +#[tokio::test] +async fn warming_counts_but_still_caches_a_coordinate_that_does_not_name_its_own_key() { + let mut node = make_node(); + let from = make_node_addr(0xAA); + let src_addr = make_node_addr(0x01); + let dest_addr = make_node_addr(0x02); + let root_addr = *node.tree_state.my_coords().root_id(); + let someone_else = make_node_addr(0x09); + + // dest_coords names 0x09, not the 0x02 it will be filed under. This is the + // shape an honest sender produces when its own cache missed and + // `get_dest_coords` fell back to the sender's own coordinates, so it is + // counted and NOT refused. + let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); + let dest_coords = TreeCoordinate::from_addrs(vec![someone_else, root_addr]).unwrap(); + let setup_payload = SessionSetup::new(src_coords, dest_coords).encode(); + let encoded = SessionDatagram::new(src_addr, dest_addr, setup_payload).encode(); + + let before = node.metrics().forwarding.coord_warm_key_mismatch.get(); + node.handle_session_datagram(&from, &encoded[1..], false) + .await; + + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64; + assert!( + node.coord_cache().get(&dest_addr, now_ms).is_some(), + "the mismatching entry should still be cached; this check counts only" + ); + assert_eq!( + node.metrics().forwarding.coord_warm_key_mismatch.get(), + before + 1, + "the mismatch should be counted exactly once" + ); + assert_eq!( + node.metrics().forwarding.coord_warm_key_mismatch.get() - before, + 1, + "the well-formed src coordinate must not be counted as a mismatch" + ); +} + #[tokio::test] async fn test_coord_cache_warming_session_setup() { let mut node = make_node(); let from = make_node_addr(0xAA); let src_addr = make_node_addr(0x01); let dest_addr = make_node_addr(0x02); - let root_addr = make_node_addr(0xF0); + // The warming path refuses a coordinate under a root other than this + // node's, so a fixture that wants the write to land has to share the + // node's root. A fresh node is its own root. + let root_addr = *node.tree_state.my_coords().root_id(); let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap(); @@ -254,7 +344,10 @@ async fn test_coord_cache_warming_session_ack() { let from = make_node_addr(0xAA); let src_addr = make_node_addr(0x01); let dest_addr = make_node_addr(0x02); - let root_addr = make_node_addr(0xF0); + // The warming path refuses a coordinate under a root other than this + // node's, so a fixture that wants the write to land has to share the + // node's root. A fresh node is its own root. + let root_addr = *node.tree_state.my_coords().root_id(); let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap(); @@ -298,7 +391,10 @@ async fn test_coord_cache_warming_encrypted_msg_with_coords() { let from = make_node_addr(0xAA); let src_addr = make_node_addr(0x01); let dest_addr = make_node_addr(0x02); - let root_addr = make_node_addr(0xF0); + // The warming path refuses a coordinate under a root other than this + // node's, so a fixture that wants the write to land has to share the + // node's root. A fresh node is its own root. + let root_addr = *node.tree_state.my_coords().root_id(); let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap(); @@ -392,7 +488,10 @@ async fn test_coord_cache_warming_ttl_zero_local_delivery() { let from = make_node_addr(0xAA); let my_addr = *node.node_addr(); let src_addr = make_node_addr(0x01); - let root_addr = make_node_addr(0xF0); + // The warming path refuses a coordinate under a root other than this + // node's, so a fixture that wants the write to land has to share the + // node's root. A fresh node is its own root. + let root_addr = *node.tree_state.my_coords().root_id(); let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); let dest_coords = TreeCoordinate::from_addrs(vec![my_addr, root_addr]).unwrap(); @@ -438,7 +537,10 @@ async fn test_coord_cache_warming_ttl_zero_transit_drop() { let from = make_node_addr(0xAA); let src_addr = make_node_addr(0x01); let dest_addr = make_node_addr(0x02); - let root_addr = make_node_addr(0xF0); + // The warming path refuses a coordinate under a root other than this + // node's, so a fixture that wants the write to land has to share the + // node's root. A fresh node is its own root. + let root_addr = *node.tree_state.my_coords().root_id(); let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap(); let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap();