mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
fix(node): scope the medium-change reaction to the peers that moved
The reaction dropped every peer's connected socket and heartbeated every connectionless peer, whatever the change actually named. That was defensible while the fingerprint was host-wide and could not say which peering had moved. Keying it on peers removed that excuse, and introduced a reason to care. `probe_target` is the observed source address of the last authentic packet a peer sent, updated with no throttle. So the trigger is now within reach of a remote party for the first time: a peer alternating between two of its own addresses that leave this host by different interfaces moves the fingerprint at will. Node-wide, that one peer could drive every other peering's socket teardown and heartbeat, repeatedly, bounded only by the poll interval — up to `max_peers` drain threads torn down and respawned per period. Scoped, the only peer in the set is the roamer itself, whose connected socket the data plane has already cleared on the address change. The lever closes by construction rather than by a rate limit. Nothing is left stranded by the narrowing. A peer absent from the set is one whose local source address the kernel still resolves to the same place, and that is the entire content of the fingerprint: a peer that did not move is a peer whose socket is not stale. To be clear about what this was worth: nothing black-holed before it. The sockets reinstall on a later tick and sends continue over the wildcard socket meanwhile, so the cost was internal teardown and respawn work rather than an outage. It is done here because this change is what created the lever, not because it was urgent. The test holds two peers, moves one, and asserts the other keeps both its socket and its heartbeat timestamp. The medium-change lab still passes 17/17, which is the end-to-end check that the peer whose route really did move is still in the set and still repaired.
This commit is contained in:
committed by
Johnathan Corgan
parent
0264c9e275
commit
10b4e6ea60
@@ -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
|
||||
|
||||
+40
-34
@@ -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<NodeAddr> = 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<NodeAddr> = self
|
||||
.peers
|
||||
fn drop_connected_sockets_after_net_change(&mut self, moved: &[NodeAddr]) -> usize {
|
||||
let pinned: Vec<NodeAddr> = 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<NodeAddr> = self
|
||||
.peers
|
||||
let targets: Vec<NodeAddr> = 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;
|
||||
|
||||
+27
-4
@@ -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<PeerSourceMove> = 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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user