diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 7bd8a102..6e1964b9 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -250,6 +250,14 @@ on the same LAN, reached by its subnet route rather than the default route, is covered as well as one across the internet, and so is a more specific route moving under a single peer. +The converse is the residual. The probe answers for the peer's *current* +address, and that address is the source of the last authentic packet it sent, +so a peer that roams between two of its own addresses which leave this host by +different interfaces is indistinguishable from a local path move. The reaction +is scoped to the peers named in the change, so such a peer moves nothing but +its own send path — but it is the peer, not this host, that decided the +fingerprint changed. + Peers appearing and leaving are ignored on their own — that is ordinary node behaviour and says nothing about the medium. A peer seen for the first time is the one exception, and it is not judged against history but against its own diff --git a/src/node/handlers/netmon.rs b/src/node/handlers/netmon.rs index de6f551d..42a8912b 100644 --- a/src/node/handlers/netmon.rs +++ b/src/node/handlers/netmon.rs @@ -53,22 +53,22 @@ //! remains the backstop for a peer that genuinely cannot be reached on the new //! medium. //! -//! # Why the reaction is still node-wide +//! # Why the reaction is scoped to the peers that moved //! -//! [`NetChange`] now names the peers whose local source address moved, because -//! the fingerprint is keyed on them. The reaction deliberately does not use -//! that yet: it drops every connected socket and heartbeats every -//! connectionless peer, exactly as it did when the detector could only say -//! "something about this host moved". +//! [`NetChange`] names them, and the reaction acts on exactly that set. It is +//! not an optimisation: keying the sample on peers put the trigger within +//! reach of a remote party for the first time. `probe_target` is the observed +//! source address of every authentic packet, updated with no throttle, so a +//! peer alternating between two addresses that resolve to different local +//! sources can move the fingerprint at will. Node-wide, that peer could drive +//! every other peering's socket teardown, bounded only by the poll interval. +//! Scoped, the only peer in the set is the roamer itself — whose connected +//! socket `dataplane::encrypted` has already cleared on the address change. //! -//! That is over-broad and known to be. It is left node-wide here because -//! narrowing it is a behavioural change with its own failure mode — a peer -//! left un-rebound because it was absent from the moved set is stranded for -//! `link_dead_timeout_secs`, which is the bug this subsystem exists to close — -//! and it wants its own tests rather than a free ride on a change to the -//! fingerprint. The cost of staying broad is now small: a change is only -//! reported when a peering's own path moved, so the fan-out no longer fires -//! for a container bridge appearing. +//! Nothing is left stranded by the narrowing, because a peer absent from the +//! set is one whose local source address the kernel still resolves to the same +//! place. That is the whole content of the fingerprint: a peer that did not +//! move is a peer whose socket is not stale. use std::time::Instant; @@ -80,22 +80,21 @@ use crate::node::netmon::NetChange; use crate::proto::link::LinkMessageType; impl Node { - /// React to a settled transport-medium change. - /// - /// `change.summary` names the peers that moved; see the module docs for - /// why the reaction is node-wide regardless. + /// React to a settled transport-medium change, on the peers it names. pub(in crate::node) async fn handle_net_change(&mut self, change: NetChange) { + let moved: Vec = change.summary.moved.iter().map(|m| m.peer).collect(); let peers = self.peers.len(); // Before the heartbeats: they must go out over a socket that resolves // the route now, not one still pinned to the interface just left. - let sockets_rebound = self.drop_connected_sockets_after_net_change(); + let sockets_rebound = self.drop_connected_sockets_after_net_change(&moved); - let heartbeated = self.heartbeat_all_peers_after_net_change().await; + let heartbeated = self.heartbeat_moved_peers_after_net_change(&moved).await; info!( generation = change.generation, change = %change.summary, peers, + moved = moved.len(), sockets_rebound, heartbeated, "Transport medium changed; rebinding sends and re-pinning peers" @@ -109,12 +108,15 @@ impl Node { /// source address to the interface that carried the route at connect time, /// and never re-evaluates it. #[cfg(any(target_os = "linux", target_os = "macos"))] - fn drop_connected_sockets_after_net_change(&mut self) -> usize { - let pinned: Vec = self - .peers + fn drop_connected_sockets_after_net_change(&mut self, moved: &[NodeAddr]) -> usize { + let pinned: Vec = moved .iter() - .filter(|(_, peer)| peer.connected_udp().is_some()) - .map(|(addr, _)| *addr) + .filter(|addr| { + self.peers + .get(*addr) + .is_some_and(|peer| peer.connected_udp().is_some()) + }) + .copied() .collect(); for addr in &pinned { self.clear_connected_udp_for_peer(addr); @@ -124,7 +126,7 @@ impl Node { /// No per-peer connected sockets on this platform, so nothing to rebind. #[cfg(not(any(target_os = "linux", target_os = "macos")))] - fn drop_connected_sockets_after_net_change(&mut self) -> usize { + fn drop_connected_sockets_after_net_change(&mut self, _moved: &[NodeAddr]) -> usize { 0 } @@ -164,18 +166,22 @@ impl Node { /// dropping the stale connection /// rather than writing into it, which is a different change with a real /// cost behind it — a Tor peer pays a fresh circuit — and is not this one. - pub(in crate::node) async fn heartbeat_all_peers_after_net_change(&mut self) -> usize { + pub(in crate::node) async fn heartbeat_moved_peers_after_net_change( + &mut self, + moved: &[NodeAddr], + ) -> usize { let now = Instant::now(); let heartbeat = [LinkMessageType::Heartbeat.to_byte()]; - let targets: Vec = self - .peers + let targets: Vec = moved .iter() - .filter(|(_, peer)| { - peer.transport_id() - .and_then(|id| self.transports.get(&id)) - .is_some_and(|t| !t.transport_type().connection_oriented) + .filter(|addr| { + self.peers.get(*addr).is_some_and(|peer| { + peer.transport_id() + .and_then(|id| self.transports.get(&id)) + .is_some_and(|t| !t.transport_type().connection_oriented) + }) }) - .map(|(addr, _)| *addr) + .copied() .collect(); let mut sent = 0usize; diff --git a/src/node/netmon/mod.rs b/src/node/netmon/mod.rs index 839234d3..79e78c65 100644 --- a/src/node/netmon/mod.rs +++ b/src/node/netmon/mod.rs @@ -460,10 +460,9 @@ pub(crate) struct NetChange { } impl NetChange { - /// A synthetic change, for tests that exercise the node's *reaction* to a - /// medium change rather than its detection. The summary is empty because - /// the handler does not read it — it re-evaluates every peer regardless of - /// which peer's source address moved. + /// A synthetic change naming no peers, for tests that assert the node does + /// *nothing* — the reaction is scoped to the peers the summary names, so an + /// empty summary must move nothing. #[cfg(test)] pub(crate) fn for_test(generation: u64) -> Self { Self { @@ -474,6 +473,30 @@ impl NetChange { }, } } + + /// A synthetic change naming `peers` as having moved, for tests that + /// exercise the node's *reaction* rather than its detection. + /// + /// The addresses are placeholders: the handler reads only which peers + /// moved, not where from or to. + #[cfg(test)] + pub(crate) fn for_test_moved(generation: u64, peers: &[NodeAddr]) -> Self { + let moved: Vec = peers + .iter() + .map(|peer| PeerSourceMove { + peer: *peer, + before: Some(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 10))), + after: Some(IpAddr::V4(Ipv4Addr::new(10, 40, 0, 7))), + }) + .collect(); + Self { + generation, + summary: NetChangeSummary { + probed: moved.len(), + moved, + }, + } + } } /// What tells the detector it is worth taking another sample. diff --git a/src/node/tests/netmon.rs b/src/node/tests/netmon.rs index 72658238..0bd9cc6d 100644 --- a/src/node/tests/netmon.rs +++ b/src/node/tests/netmon.rs @@ -102,7 +102,7 @@ async fn a_medium_change_drops_connected_sockets_pinned_to_the_old_path() { nodes[0] .node - .handle_net_change(NetChange::for_test(1)) + .handle_net_change(NetChange::for_test_moved(1, &[addr_1])) .await; assert!( @@ -116,6 +116,69 @@ async fn a_medium_change_drops_connected_sockets_pinned_to_the_old_path() { ); } +/// **A peer the change did not name keeps its socket.** +/// +/// The reaction is scoped to `change.summary.moved`, and that is not an +/// optimisation. Keying the sample on peers put the trigger within reach of a +/// remote party: `probe_target` is the observed source of every authentic +/// packet, updated with no throttle, so a peer alternating between two +/// addresses can move the fingerprint at will. Node-wide, that peer could tear +/// down every other peering's send path on repeat. If this test starts failing +/// because the untouched peer lost its socket, that lever is back. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[tokio::test] +async fn a_peer_the_change_did_not_name_keeps_its_socket() { + let mut nodes = run_tree_test(3, &[(0, 1), (0, 2)], false).await; + verify_tree_convergence(&nodes); + + let moved = *nodes[1].node.node_addr(); + let untouched = *nodes[2].node.node_addr(); + let transport_id = nodes[0].transport_id; + install_connected_udp(&mut nodes[0].node, &moved, transport_id); + install_connected_udp(&mut nodes[0].node, &untouched, transport_id); + + let heartbeat_before = nodes[0] + .node + .get_peer(&untouched) + .unwrap() + .last_heartbeat_sent(); + + nodes[0] + .node + .handle_net_change(NetChange::for_test_moved(1, &[moved])) + .await; + + assert!( + nodes[0] + .node + .get_peer(&moved) + .unwrap() + .connected_udp() + .is_none(), + "the peer that moved must lose its pinned socket" + ); + assert!( + nodes[0] + .node + .get_peer(&untouched) + .unwrap() + .connected_udp() + .is_some(), + "a peer whose source address did not move must keep its socket" + ); + assert_eq!( + nodes[0] + .node + .get_peer(&untouched) + .unwrap() + .last_heartbeat_sent(), + heartbeat_before, + "and must not be heartbeated for another peer's move" + ); + + cleanup_nodes(&mut nodes).await; +} + /// The rebind must not cost the peering. Everything above the socket — the /// Noise session, the tree position, the routes — is unaffected by which local /// address the node sends from, so a medium change that tore peers down would @@ -132,7 +195,7 @@ async fn a_medium_change_keeps_every_peering_intact() { nodes[0] .node - .handle_net_change(NetChange::for_test(1)) + .handle_net_change(NetChange::for_test_moved(1, &[addr_1])) .await; let peer = nodes[0] @@ -169,7 +232,7 @@ async fn every_peer_is_heartbeated_so_the_far_side_re_pins() { nodes[0] .node - .handle_net_change(NetChange::for_test(1)) + .handle_net_change(NetChange::for_test_moved(1, &[addr_1])) .await; let after = nodes[0] @@ -234,7 +297,7 @@ async fn a_peer_on_a_connection_oriented_transport_is_left_to_the_periodic_heart nodes[0] .node - .handle_net_change(NetChange::for_test(1)) + .handle_net_change(NetChange::for_test_moved(1, &[addr_1])) .await; let after = nodes[0] @@ -486,7 +549,10 @@ async fn a_heartbeat_that_failed_is_not_counted_and_does_not_suppress_the_next() .unwrap() .last_heartbeat_sent(); - let sent = nodes[0].node.heartbeat_all_peers_after_net_change().await; + let sent = nodes[0] + .node + .heartbeat_moved_peers_after_net_change(&[addr_1]) + .await; assert_eq!( sent, 0,