diff --git a/CHANGELOG.md b/CHANGELOG.md index 47b3a1a4..321a0ec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -228,6 +228,45 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- The influence a remote party has over path MTU is now bounded, and the + per-destination path MTU cache has a way back. The `path_mtu` field is an + unsigned per-hop transit annotation carried outside the signed proof, and the + `MtuExceeded` and `PathBroken` signals arrive unencrypted with no sender + check, so any forwarder — or anyone who can reach the node — could lower it, + and it was accepted with no minimum. A single `MtuExceeded` carrying a very + small value drove a session's path MTU to zero, after which every packet to + that destination was answered with an ICMPv6 Packet Too Big instead of being + sent: a blackhole that lasted until the daemon restarted. The same value + reached the SYN-time TCP MSS clamp, where anything at or below 137 saturates + to a segment size of zero and the band just above it yields single digits. + Values below an actionable minimum are now ignored rather than applied or + stored, at the three places a remote value is acted on: the path MTU state + machine, the reactive `MtuExceeded` write, and the discovery response, whose + coordinates are still cached so refusing the annotation cannot become a way + to deny discovery. The MSS clamp additionally refuses to write a zero. Each + of the three refusals logs a warning and increments its own counter in the + error-signal family, so an operator can tell them apart without scraping + logs: they carry different meanings, one being an authenticated peer inside + an established session, one an unencrypted signal anyone able to reach the + node can send at will, and one a verified discovery response whose unsigned + annotation a forwarder on the reverse path rewrote. Because those three + refusals are the only way a remote value reaches the per-destination store, + the SYN-time clamp does not apply the minimum a second time when it reads + that store: a small value there is one the node derived from its own outgoing + link, which is exact rather than suspect, and BLE in particular negotiates a + link MTU per connection that lands under the minimum routinely. The clamp + refuses only a stored value admitting no TCP payload byte at all, at 137 or + below, where the segment size saturates to zero and the clamp would be + skipped entirely; it logs that at trace rather than warn, since it sits on + the per-packet path, and the peer's link promotion reports it once instead. + A stored per-destination path MTU is released when the path is invalidated by + a `PathBroken` report, by session idle expiry, or by handshake timeout, and + the link MTU read from the local transport is reseeded in its place, so a + directly connected peer does not lose its own measurement along with the + remote claim. Locally derived MTUs are not subject to the minimum, at the + seed or at the clamp. Legitimate narrow paths are unaffected: adaptation to + hops well below the IPv6 minimum, which the mesh does use, continues to work. + - The FSP session address is now bound to the peer key the Noise handshake authenticated, on both the initial and the rekey path. The responder recorded a session under the source address carried in the datagram without ever diff --git a/src/bin/fipstop/ui/routing.rs b/src/bin/fipstop/ui/routing.rs index e3b9a7b1..10dd5492 100644 --- a/src/bin/fipstop/ui/routing.rs +++ b/src/bin/fipstop/ui/routing.rs @@ -291,6 +291,9 @@ fn draw_routing_stats( ("Coords Required", err("coords_required")), ("Path Broken", err("path_broken")), ("MTU Exceeded", err("mtu_exceeded")), + ("PMTU Notif < Floor", err("path_mtu_notif_below_floor")), + ("MTU Exceeded < Floor", err("mtu_exceeded_below_floor")), + ("Lookup PMTU < Floor", err("lookup_resp_mtu_below_floor")), ], )); right.push(Line::from("")); diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 83be9d2d..437701a2 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -33,8 +33,11 @@ }, "error_signals": { "coords_required": 0, + "lookup_resp_mtu_below_floor": 0, "mtu_exceeded": 0, - "path_broken": 0 + "mtu_exceeded_below_floor": 0, + "path_broken": 0, + "path_mtu_notif_below_floor": 0 }, "forwarding": { "decode_error_bytes": 0, diff --git a/src/mmp/mod.rs b/src/mmp/mod.rs index b8940626..6340a862 100644 --- a/src/mmp/mod.rs +++ b/src/mmp/mod.rs @@ -435,7 +435,20 @@ impl PathMtuState { /// value, spanning at least 2 * notification_interval. /// /// Returns `true` if the effective MTU changed. + /// + /// A reported value below [`MIN_ACTIONABLE_PATH_MTU`] is ignored entirely. + /// The notification carries a remote party's claim about the path, and + /// below that floor the claim cannot describe a usable path: acting on it + /// drives the send gate into answering every packet with an ICMPv6 Packet + /// Too Big instead of sending it. Returning `false` leaves whatever the + /// local seed established and correctly reports "no change". + /// + /// [`MIN_ACTIONABLE_PATH_MTU`]: crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU pub fn apply_notification(&mut self, reported_mtu: u16, now: Instant) -> bool { + if reported_mtu < crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU { + return false; + } + if reported_mtu < self.current_mtu { // Decrease: immediate self.current_mtu = reported_mtu; @@ -447,7 +460,7 @@ impl PathMtuState { if reported_mtu > self.current_mtu { // Increase: track consecutive notifications if reported_mtu == self.pending_increase_mtu { - self.consecutive_increase_count += 1; + self.consecutive_increase_count = self.consecutive_increase_count.saturating_add(1); } else { // Different value: reset sequence self.pending_increase_mtu = reported_mtu; @@ -552,4 +565,65 @@ owd_window_size: 48 assert_eq!(config.log_interval_secs, DEFAULT_LOG_INTERVAL_SECS); assert_eq!(config.owd_window_size, DEFAULT_OWD_WINDOW_SIZE); } + + #[test] + fn apply_notification_ignores_a_decrease_below_the_actionable_floor() { + // The reported value comes from a remote party over an unauthenticated + // signal. Driving current_mtu into this band turns the TUN send gate + // into a blackhole: every packet is answered with a Packet Too Big + // instead of being sent. Sub-floor values are ignored outright, not + // clamped, so the locally seeded value survives untouched. + for reported in [0u16, 1, 137, 138, 200, 255] { + let mut state = PathMtuState::new(); + state.seed_source_mtu(1400); + + let changed = state.apply_notification(reported, Instant::now()); + + assert!( + !changed, + "reported path MTU {reported} is below the floor and must report no change" + ); + assert_eq!( + state.current_mtu(), + 1400, + "reported path MTU {reported} must leave the seeded value intact" + ); + } + } + + #[test] + fn apply_notification_accepts_a_decrease_at_the_actionable_floor() { + // The floor must not swallow the smallest value the node does act on, + // nor the legitimately narrow hops the mesh actually carries. + for reported in [crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU, 576, 800] { + let mut state = PathMtuState::new(); + state.seed_source_mtu(1400); + + let changed = state.apply_notification(reported, Instant::now()); + + assert!(changed, "reported path MTU {reported} must be applied"); + assert_eq!(state.current_mtu(), reported); + } + } + + #[test] + fn apply_notification_increase_counter_does_not_overflow_on_a_repeated_value() { + // The counter is a u8 and resets only when the value changes or the + // increase is accepted. Acceptance additionally requires the sequence + // to span two notification intervals, so a peer repeating one higher + // value fast enough stays in the increase branch indefinitely. + let mut state = PathMtuState::new(); + state.seed_source_mtu(1000); + + let now = Instant::now(); + for _ in 0..600 { + state.apply_notification(1200, now); + } + + assert_eq!( + state.current_mtu(), + 1000, + "the increase is not yet due, so the effective MTU must be unchanged" + ); + } } diff --git a/src/node/handlers/discovery.rs b/src/node/handlers/discovery.rs index cf688c5d..42bc304e 100644 --- a/src/node/handlers/discovery.rs +++ b/src/node/handlers/discovery.rs @@ -233,47 +233,73 @@ impl Node { "Discovery succeeded, proof verified, route cached" ); - self.coord_cache - .insert_with_path_mtu(target, response.target_coords, now_ms, path_mtu); + // The annotation is unsigned and accumulates hop by hop, so any + // forwarder on the reverse path can lower it. A value below the + // actionable floor cannot describe a usable path, so treat it as + // absent: cache the coordinates, which are what the proof covers, + // and store no path MTU from this response at all. + let path_mtu_actionable = path_mtu >= crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU; + if path_mtu_actionable { + self.coord_cache.insert_with_path_mtu( + target, + response.target_coords, + now_ms, + path_mtu, + ); + } else { + warn!( + request_id = response.request_id, + target = %self.peer_display_name(&target), + path_mtu = path_mtu, + floor = crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU, + "LookupResponse carries a path MTU below the actionable floor; \ + caching coordinates without it" + ); + self.metrics().errors.lookup_resp_mtu_below_floor.inc(); + self.coord_cache + .insert(target, response.target_coords, now_ms); + } // Mirror path_mtu into the FipsAddress-keyed read-only lookup // map used by the TUN reader/writer at TCP MSS clamp time. let fips_addr = crate::FipsAddress::from_node_addr(&target); - match self.path_mtu_lookup.write() { - Ok(mut map) => match map.get(&fips_addr).copied() { - Some(existing) if existing <= path_mtu => { - // Keep the tighter learned value; never loosen the - // clamp. A reactive MtuExceeded or PathMtuNotification - // tighten takes precedence over a looser discovery - // estimate (cross-carrier keep-tighter). - debug!( + if path_mtu_actionable { + match self.path_mtu_lookup.write() { + Ok(mut map) => match map.get(&fips_addr).copied() { + Some(existing) if existing <= path_mtu => { + // Keep the tighter learned value; never loosen the + // clamp. A reactive MtuExceeded or PathMtuNotification + // tighten takes precedence over a looser discovery + // estimate (cross-carrier keep-tighter). + debug!( + target = %self.peer_display_name(&target), + fips_addr = %fips_addr, + path_mtu = path_mtu, + existing = existing, + "LookupResponse: keeping tighter existing path_mtu_lookup value" + ); + } + other => { + map.insert(fips_addr, path_mtu); + debug!( + target = %self.peer_display_name(&target), + fips_addr = %fips_addr, + path_mtu = path_mtu, + prior = ?other, + map_len = map.len(), + "Wrote path_mtu_lookup from discovery LookupResponse" + ); + } + }, + Err(e) => { + warn!( target = %self.peer_display_name(&target), fips_addr = %fips_addr, path_mtu = path_mtu, - existing = existing, - "LookupResponse: keeping tighter existing path_mtu_lookup value" + error = %e, + "path_mtu_lookup write lock poisoned; clamp will not see this update" ); } - other => { - map.insert(fips_addr, path_mtu); - debug!( - target = %self.peer_display_name(&target), - fips_addr = %fips_addr, - path_mtu = path_mtu, - prior = ?other, - map_len = map.len(), - "Wrote path_mtu_lookup from discovery LookupResponse" - ); - } - }, - Err(e) => { - warn!( - target = %self.peer_display_name(&target), - fips_addr = %fips_addr, - path_mtu = path_mtu, - error = %e, - "path_mtu_lookup write lock poisoned; clamp will not see this update" - ); } } @@ -698,6 +724,22 @@ impl Node { return; }; let link_mtu = transport.link_mtu(addr); + // A locally derived MTU is deliberately exempt from the actionable + // floor, so this seeds the value either way, and a narrow link is not + // by itself worth reporting: BLE negotiates its MTU per connection and + // lands below the floor routinely, where the tight clamp the seed + // produces is exactly what the flow needs. Warn only where the link + // admits no TCP payload byte at all, since there the SYN-time clamp + // has nothing usable to derive and drops the peer onto the + // conservative fallback ceiling for as long as the link stands. + if crate::upper::icmp::mss_ceiling(link_mtu) == 0 { + warn!( + peer = %self.peer_display_name(peer_addr), + link_mtu = link_mtu, + "Link MTU leaves no room for a TCP payload byte; TCP to this peer \ + will not work until the link or the transport's mtu setting changes" + ); + } let fips_addr = crate::FipsAddress::from_node_addr(peer_addr); let Ok(mut map) = self.path_mtu_lookup.write() else { warn!( diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 16dc005e..497fe561 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -1081,6 +1081,23 @@ impl Node { return; }; + // `apply_notification` refuses a sub-floor value, but it returns the + // same `false` it returns for the ordinary "no change" case, which is + // the common one. Test the floor here so the refusal is visible: this + // arrives on the decrypted service-payload path, so a value this low + // means an authenticated peer we hold a session with is sending + // something unusable. + if notif.path_mtu < crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU { + warn!( + src = %peer_name, + reported_mtu = notif.path_mtu, + floor = crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU, + "PathMtuNotification reports a path MTU below the actionable floor; ignoring" + ); + self.metrics.errors.path_mtu_notif_below_floor.inc(); + return; + } + let old_mtu = mmp.path_mtu.current_mtu(); let now = std::time::Instant::now(); let changed = mmp.path_mtu.apply_notification(notif.path_mtu, now); @@ -1206,7 +1223,7 @@ impl Node { /// The router has coordinates but still can't route to the destination. /// Send a standalone CoordsWarmup immediately (rate-limited), invalidate /// cached coordinates, trigger re-discovery, and reset the warmup counter. - async fn handle_path_broken(&mut self, inner: &[u8]) { + pub(in crate::node) async fn handle_path_broken(&mut self, inner: &[u8]) { self.metrics().errors.path_broken.inc(); let msg = match PathBroken::decode(inner) { @@ -1243,6 +1260,10 @@ impl Node { // Invalidate stale cached coordinates self.coord_cache.remove(&msg.dest_addr); + // The path this destination's stored MTU described is gone, so release + // it rather than carrying it onto whatever path replaces it. + self.path_mtu_lookup_release(&msg.dest_addr); + // Trigger re-discovery to get fresh coordinates, but only if we have // the target's identity cached — otherwise we can't verify the // LookupResponse proof. This avoids a race when the XK responder @@ -1309,6 +1330,22 @@ impl Node { } } + // The lookup write below is not gated on a session existing, so an + // unencrypted MtuExceeded from anyone reaches it. Refuse to store a + // bottleneck too small to describe a usable path; a stored value that + // low drives the SYN-time MSS clamp into single digits or zero. + if msg.mtu < crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU { + warn!( + dest = %peer_name, + reporter = %msg.reporter, + bottleneck_mtu = msg.mtu, + floor = crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU, + "MtuExceeded reports a path MTU below the actionable floor; ignoring" + ); + self.metrics().errors.mtu_exceeded_below_floor.inc(); + return; + } + // Mirror the bottleneck into the FipsAddress-keyed lookup used by // the TUN reader/writer at TCP MSS clamp time. Discovery's reverse- // path response can carry a value too generous for the actual diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index 27437cf2..c03bc507 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -208,6 +208,7 @@ impl Node { info!(dest = %name, "Session handshake timed out, removing"); self.sessions.remove(addr); self.pending_tun_packets.remove(addr); + self.path_mtu_lookup_release(addr); } // Second pass: collect resend candidates @@ -285,6 +286,7 @@ impl Node { } self.sessions.remove(&addr); self.pending_tun_packets.remove(&addr); + self.path_mtu_lookup_release(&addr); debug!( dest = %name, idle_secs = timeout_ms / 1000, diff --git a/src/node/metrics.rs b/src/node/metrics.rs index e2eb813a..415266d4 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -459,6 +459,21 @@ pub struct ErrorMetrics { pub coords_required: Counter, pub path_broken: Counter, pub mtu_exceeded: Counter, + /// `PathMtuNotification`s ignored for carrying a path MTU below the + /// actionable floor. This signal arrives inside an established session + /// on the decrypted path, so a rising count means an authenticated peer + /// is misconfigured or misbehaving. + pub path_mtu_notif_below_floor: Counter, + /// `MtuExceeded` signals whose bottleneck was ignored for falling below + /// the actionable floor. The signal is unencrypted, unauthenticated and + /// unmetered, so a rising count on its own is the forged-signal + /// signature; `mtu_exceeded` counts the whole population. + pub mtu_exceeded_below_floor: Counter, + /// `LookupResponse` path MTU annotations ignored for falling below the + /// actionable floor. The response carried a verified proof, so a rising + /// count means a forwarder on the reverse path is mangling the unsigned + /// annotation. + pub lookup_resp_mtu_below_floor: Counter, } impl ErrorMetrics { @@ -468,6 +483,9 @@ impl ErrorMetrics { coords_required: self.coords_required.get(), path_broken: self.path_broken.get(), mtu_exceeded: self.mtu_exceeded.get(), + path_mtu_notif_below_floor: self.path_mtu_notif_below_floor.get(), + mtu_exceeded_below_floor: self.mtu_exceeded_below_floor.get(), + lookup_resp_mtu_below_floor: self.lookup_resp_mtu_below_floor.get(), } } } diff --git a/src/node/mod.rs b/src/node/mod.rs index 7842d335..7a944c6c 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -2545,6 +2545,50 @@ impl Node { } } + /// Drop the remote-learned path MTU for a destination whose path is no + /// longer valid, then restore what is known locally. + /// + /// Entries in `path_mtu_lookup` come from two sources: values a remote + /// party supplied (discovery responses, `MtuExceeded`, path MTU + /// notifications) and the link MTU this node reads from its own transport + /// configuration for a directly connected peer. When the path is declared + /// broken or the session goes away, the remote-supplied value describes a + /// path that no longer exists and must not outlive it, but the locally + /// derived one is still true. Removing the entry and then re-running the + /// link-peer seed keeps the second while discarding the first; a plain + /// removal would silently drop a direct peer back to the conservative + /// ceiling until its link re-handshakes. + fn path_mtu_lookup_release(&self, addr: &NodeAddr) { + let fips_addr = crate::FipsAddress::from_node_addr(addr); + match self.path_mtu_lookup.write() { + Ok(mut map) => { + if map.remove(&fips_addr).is_some() { + tracing::debug!( + dest = %self.peer_display_name(addr), + fips_addr = %fips_addr, + "Released path_mtu_lookup entry for an invalidated path" + ); + } + } + Err(e) => { + tracing::warn!( + fips_addr = %fips_addr, + error = %e, + "path_mtu_lookup write lock poisoned; entry not released" + ); + return; + } + } + // The write guard above must be dropped before the seed runs: it takes + // the same lock, and `std::sync::RwLock` is not re-entrant. + if let Some(peer) = self.peers.get(addr) + && let Some(transport_id) = peer.transport_id() + && let Some(transport_addr) = peer.current_addr().cloned() + { + self.seed_path_mtu_for_link_peer(addr, transport_id, &transport_addr); + } + } + /// Number of end-to-end sessions. pub fn session_count(&self) -> usize { self.sessions.len() diff --git a/src/node/reloadable.rs b/src/node/reloadable.rs index 60131107..4d81acec 100644 --- a/src/node/reloadable.rs +++ b/src/node/reloadable.rs @@ -38,7 +38,11 @@ //! //! - `path_mtu_lookup` is an event-driven cache (`Arc>`) //! populated from observed path-MTU discovery traffic, not loaded from a -//! file. There is nothing to poll. (Its read side could adopt the same +//! file. There is nothing to poll. Release is event-driven for the same +//! reason: an entry is dropped when the path it describes is declared +//! invalid (a `PathBroken` report, session idle expiry, or handshake +//! timeout) and the locally derived link MTU is reseeded in its place, so +//! there is no expiry sweep either. (Its read side could adopt the same //! lock-free `ArcSwap` shape in the future, but that is an optimization, not //! a reload.) //! - `nostr_discovery` is an async spawned subsystem, not a snapshot of disk diff --git a/src/node/stats.rs b/src/node/stats.rs index 1d29c6f3..a88154c4 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -343,6 +343,9 @@ pub struct ErrorSignalStatsSnapshot { pub coords_required: u64, pub path_broken: u64, pub mtu_exceeded: u64, + pub path_mtu_notif_below_floor: u64, + pub mtu_exceeded_below_floor: u64, + pub lookup_resp_mtu_below_floor: u64, } #[derive(Clone, Debug, Default, Serialize)] diff --git a/src/node/tests/discovery.rs b/src/node/tests/discovery.rs index 3503f729..45da022f 100644 --- a/src/node/tests/discovery.rs +++ b/src/node/tests/discovery.rs @@ -912,6 +912,95 @@ async fn test_originator_stores_path_mtu_in_cache() { ); } +#[tokio::test] +async fn test_originator_ignores_sub_floor_path_mtu_but_still_caches_coords() { + // The path_mtu annotation accumulates hop by hop outside the signed proof, + // so any forwarder on the reverse path can lower it. A value below the + // actionable floor must be treated as absent rather than stored — but the + // coordinates it travelled with are proof-covered and must still land, + // otherwise a value-poisoning vector becomes a discovery-denial one. + let mut node = make_node(); + let from = make_node_addr(0xAA); + + let target_identity = Identity::generate(); + let target = *target_identity.node_addr(); + let target_fips = crate::FipsAddress::from_node_addr(&target); + let root = make_node_addr(0xF0); + let coords = TreeCoordinate::from_addrs(vec![target, root]).unwrap(); + + node.register_identity(target, target_identity.pubkey_full()); + + let proof_data = LookupResponse::proof_bytes(801, &target, &coords); + let proof = target_identity.sign(&proof_data); + + let mut response = LookupResponse::new(801, target, coords.clone(), proof); + response.path_mtu = 64; + + let payload = &response.encode()[1..]; + node.handle_lookup_response(&from, payload).await; + + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0); + + assert!( + node.coord_cache().contains(&target, now_ms), + "coordinates must still be cached; the proof covers them" + ); + assert_eq!( + node.coord_cache().get_entry(&target).unwrap().path_mtu(), + None, + "a sub-floor annotation must not reach the coordinate cache" + ); + assert_eq!( + node.path_mtu_lookup_get(&target_fips), + None, + "a sub-floor annotation must not reach the MSS clamp lookup" + ); + assert_eq!( + node.metrics().errors.lookup_resp_mtu_below_floor.get(), + 1, + "refusing the annotation must be visible on a counter, not only in a log" + ); +} + +#[tokio::test] +async fn test_actionable_lookup_response_path_mtu_does_not_bump_below_floor_counter() { + // Discriminating half of the sub-floor counter check: the refusal counter + // is only useful if an ordinary verified response leaves it alone. + let mut node = make_node(); + let from = make_node_addr(0xAA); + + let target_identity = Identity::generate(); + let target = *target_identity.node_addr(); + let target_fips = crate::FipsAddress::from_node_addr(&target); + let root = make_node_addr(0xF0); + let coords = TreeCoordinate::from_addrs(vec![target, root]).unwrap(); + + node.register_identity(target, target_identity.pubkey_full()); + + let proof_data = LookupResponse::proof_bytes(802, &target, &coords); + let proof = target_identity.sign(&proof_data); + + let mut response = LookupResponse::new(802, target, coords.clone(), proof); + response.path_mtu = 1280; + + let payload = &response.encode()[1..]; + node.handle_lookup_response(&from, payload).await; + + assert_eq!( + node.path_mtu_lookup_get(&target_fips), + Some(1280), + "an actionable annotation must reach the MSS clamp lookup" + ); + assert_eq!( + node.metrics().errors.lookup_resp_mtu_below_floor.get(), + 0, + "an actionable annotation must not bump the below-floor counter" + ); +} + #[tokio::test] async fn test_originator_lookup_response_keeps_tighter_path_mtu_lookup() { // Regression: a LookupResponse carrying a looser (larger) path_mtu must diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 5f5b1cb0..3c0bf00e 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -2272,6 +2272,282 @@ async fn test_handle_mtu_exceeded_keeps_tighter_existing_path_mtu_lookup() { ); } +#[tokio::test] +async fn test_handle_mtu_exceeded_below_floor_leaves_path_mtu_lookup_untouched() { + use crate::node::tests::spanning_tree::make_test_node; + + // MtuExceeded is an unencrypted signal and the lookup write below it is + // not gated on a session existing, so anyone can reach it. A bottleneck + // this small cannot describe a real path; storing it would drive the + // SYN-time MSS clamp to a single-digit or zero segment size. + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + let inner = build_mtu_exceeded_inner(&dest, &reporter, 100); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + None, + "a sub-floor MtuExceeded must leave no path_mtu_lookup entry behind" + ); +} + +#[tokio::test] +async fn test_sub_floor_mtu_exceeded_is_counted_separately_from_all_mtu_exceeded() { + // `mtu_exceeded` counts every MtuExceeded regardless of value, so the + // sub-floor subset is not separable from it. The signal is unencrypted, + // unauthenticated and unmetered, so that subset climbing on its own is + // the forged-signal signature and needs its own counter. + use crate::node::tests::spanning_tree::make_test_node; + + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + + assert_eq!( + tn.node.metrics().errors.mtu_exceeded_below_floor.get(), + 0, + "counter starts at zero on a fresh node" + ); + + let inner = build_mtu_exceeded_inner( + &dest, + &reporter, + crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU - 1, + ); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.metrics().errors.mtu_exceeded_below_floor.get(), + 1, + "a sub-floor MtuExceeded must bump the below-floor counter" + ); + + // The counter must discriminate: an actionable bottleneck is stored and + // must bump only the all-signals counter. + let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.metrics().errors.mtu_exceeded_below_floor.get(), + 1, + "an actionable MtuExceeded must not bump the below-floor counter" + ); + assert_eq!( + tn.node.metrics().errors.mtu_exceeded.get(), + 2, + "the all-signals counter must count both, sub-floor and actionable" + ); +} + +#[tokio::test] +async fn test_handle_mtu_exceeded_at_the_floor_still_writes_path_mtu_lookup() { + use crate::node::tests::spanning_tree::make_test_node; + + // The guard must reject only what is below the floor. Without this the + // floor could be widened arbitrarily and the test above would not notice. + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCD; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + let floor = crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU; + + let inner = build_mtu_exceeded_inner(&dest, &reporter, floor); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + Some(floor), + "a bottleneck exactly at the floor is actionable and must be stored" + ); +} + +#[tokio::test] +async fn test_forged_mtu_exceeded_of_zero_does_not_blackhole_the_session() { + // The security property itself. MtuExceeded arrives unencrypted with no + // sender check, so anyone who can reach this node can inject one. Applied + // unfiltered, a reported MTU of zero drives the session's path MTU to + // zero, and from then on the TUN send gate answers every packet with an + // ICMPv6 Packet Too Big instead of sending it: a total blackhole for that + // destination that survives until the daemon restarts. + let edges = vec![(0, 1)]; + let mut nodes = run_tree_test(2, &edges, false).await; + verify_tree_convergence(&nodes); + populate_all_coord_caches(&mut nodes); + + let node0_addr = *nodes[0].node.node_addr(); + let node1_addr = *nodes[1].node.node_addr(); + let node1_pubkey = nodes[1].node.identity().pubkey_full(); + + let src_fips = crate::FipsAddress::from_node_addr(&node0_addr); + let dst_fips = crate::FipsAddress::from_node_addr(&node1_addr); + + nodes[0] + .node + .initiate_session(node1_addr, node1_pubkey) + .await + .unwrap(); + for _ in 0..3 { + tokio::time::sleep(Duration::from_millis(20)).await; + process_available_packets(&mut nodes).await; + } + assert!( + nodes[0] + .node + .get_session(&node1_addr) + .unwrap() + .state() + .is_established() + ); + + // Forge the signal: an MtuExceeded claiming the path to node 1 carries + // nothing at all, reported by a node that is not on the path. + let reporter = NodeAddr::from_bytes([0xEE; 16]); + let inner = build_mtu_exceeded_inner(&node1_addr, &reporter, 0); + nodes[0].node.handle_mtu_exceeded(&inner).await; + + let (tun_tx, tun_rx) = std::sync::mpsc::channel(); + nodes[0].node.tun_tx = Some(tun_tx); + + let payload = vec![0u8; 560]; + let ipv6_packet = build_ipv6_packet(&src_fips, &dst_fips, &payload); + assert_eq!(ipv6_packet.len(), 600); + assert!( + ipv6_packet.len() <= nodes[0].node.effective_ipv6_mtu() as usize, + "the packet must fit the local MTU, so any PTB comes from the forged signal" + ); + + nodes[0].node.handle_tun_outbound(ipv6_packet).await; + + let tun_messages: Vec> = std::iter::from_fn(|| tun_rx.try_recv().ok()).collect(); + assert!( + tun_messages.is_empty(), + "a forged MtuExceeded of zero must not turn ordinary packets into \ + ICMPv6 Packet Too Big; got {} message(s)", + tun_messages.len() + ); + + cleanup_nodes(&mut nodes).await; +} + +#[tokio::test] +async fn test_path_broken_releases_path_mtu_lookup_entry() { + use crate::node::tests::spanning_tree::make_test_node; + use crate::protocol::PathBroken; + + // A PathBroken report declares the path to a destination gone. The stored + // path MTU described that path, so it must not be carried onto whatever + // path replaces it — otherwise a value learned once (or injected once) + // outlives every route change until the daemon restarts. + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + tn.node.path_mtu_lookup_insert(dest_fips, 700); + assert_eq!(tn.node.path_mtu_lookup_get(&dest_fips), Some(700)); + + // Build the body the dispatcher would hand the handler: encode() prepends + // a 4-byte FSP prefix and a msg_type byte, both already consumed there. + let encoded = PathBroken::new(dest, reporter).encode(); + let inner = &encoded[5..]; + assert!( + PathBroken::decode(inner).is_ok(), + "the test body must decode, or the handler returns early and the \ + assertion below observes nothing" + ); + + tn.node.handle_path_broken(inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + None, + "PathBroken must release the stored path MTU for the dead path" + ); +} + +#[tokio::test] +async fn test_idle_session_purge_keeps_link_peer_path_mtu_seed() { + use crate::peer::ActivePeer; + use crate::transport::udp::UdpTransport; + use crate::transport::{TransportHandle, packet_channel}; + + // Releasing on idle expiry must not throw away what local configuration + // knows. Idle expiry removes an end-to-end session; the FMP link to a + // directly connected peer stays up, and its link MTU is seeded only on + // link promotion. A blanket removal here would drop that peer to the + // conservative ceiling for every later flow until the link re-handshakes. + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let (transport_packet_tx, _transport_packet_rx) = packet_channel(64); + let transport_id = TransportId::new(1); + let mut udp = UdpTransport::new( + transport_id, + Some("udp1".to_string()), + crate::config::UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + mtu: Some(1452), + ..Default::default() + }, + transport_packet_tx, + ); + udp.start_async().await.unwrap(); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + // A directly connected peer, seeded from its link MTU the way FMP + // promotion seeds it, with an end-to-end session on top. + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + let transport_addr = TransportAddr::from_string("127.0.0.1:2121"); + + let peer_identity = PeerIdentity::from_pubkey_full(remote.pubkey_full()); + let mut peer = ActivePeer::new(peer_identity, LinkId::new(7), 0); + peer.set_current_addr(transport_id, transport_addr.clone()); + node.peers.insert(remote_addr, peer); + + node.seed_path_mtu_for_link_peer(&remote_addr, transport_id, &transport_addr); + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1452), + "precondition: the direct-link seed is in place" + ); + + let session = make_noise_session(node.identity(), &remote); + let entry = crate::node::session::SessionEntry::new( + remote_addr, + remote.pubkey_full(), + EndToEndState::Established(session), + 1000, + true, + ); + node.sessions.insert(remote_addr, entry); + + node.purge_idle_sessions(1000 + 92_000); + assert_eq!(node.session_count(), 0, "precondition: the session expired"); + + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1452), + "idle expiry must leave the locally derived link MTU in place" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + // ============================================================================ // Proactive PathMtuNotification → path_mtu_lookup focused unit tests // @@ -2392,6 +2668,57 @@ fn test_handle_path_mtu_notification_no_session_no_op() { ); } +#[test] +fn test_sub_floor_path_mtu_notification_is_ignored_and_counted() { + // The state machine returns the same `false` for a sub-floor refusal as + // for an ordinary no-change, so without a counter at the caller the + // refusal is indistinguishable from the common case. This arrives on the + // decrypted path, so a rising count means an authenticated peer is + // sending unusable values. + let mut node = make_node(); + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + + install_established_session_with_mmp(&mut node, &remote); + + assert_eq!( + node.metrics().errors.path_mtu_notif_below_floor.get(), + 0, + "counter starts at zero on a fresh node" + ); + + let body = build_path_mtu_notification_body(crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU - 1); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert_eq!( + node.metrics().errors.path_mtu_notif_below_floor.get(), + 1, + "a sub-floor PathMtuNotification must bump the below-floor counter" + ); + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + None, + "a sub-floor PathMtuNotification must leave no path_mtu_lookup entry" + ); + + // The counter must discriminate: an actionable value is applied and must + // not bump it. + let body = build_path_mtu_notification_body(1280); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert_eq!( + node.metrics().errors.path_mtu_notif_below_floor.get(), + 1, + "an actionable PathMtuNotification must not bump the below-floor counter" + ); + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1280), + "the actionable value must still be applied after a refused one" + ); +} + // ============================================================================ // Session identity binding: XK msg3 source address / static key // ============================================================================ diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 66494a68..02d064f0 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1521,6 +1521,53 @@ async fn test_seed_path_mtu_inserts_when_empty() { } } +#[tokio::test] +async fn test_seeded_narrow_link_mtu_reaches_the_clamp_as_a_tight_ceiling() { + // The seed and the SYN-time MSS clamp are two halves of one mechanism: the + // seed writes the node's own outgoing link MTU, the clamp reads it. A + // narrow link is the case that matters, because BLE negotiates its MTU per + // connection and lands below the remote-value floor routinely, and a + // direct link has no forwarder to answer an over-large segment with + // MtuExceeded. Driving the real seed rather than inserting into the map + // pins that the clamp honours what the seed actually stores. + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp = make_udp_transport_with_mtu(1, 240).await; + node.transports.insert(TransportId::new(1), udp); + + let peer_addr = make_node_addr(0xEE); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let transport_addr = TransportAddr::from_string("10.0.0.6:2121"); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &transport_addr); + + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .copied(), + Some(240), + "the seed stores a narrow link MTU unchanged" + ); + // 240 - 77 encap - 40 IPv6 - 20 TCP = 103. A clamp that discarded the + // seeded value would advertise the 1143 conservative ceiling instead, and + // every full-size segment would be refused by the transport with no + // feedback to the TCP stack. + assert_eq!( + crate::upper::tun::per_flow_max_mss(&node.path_mtu_lookup, fips_addr.as_bytes(), 1360), + 103, + "the clamp must honour the seeded link MTU, not fall back to 1143" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + #[tokio::test] async fn test_seed_path_mtu_keeps_tighter_existing_value() { let mut node = make_node(); diff --git a/src/upper/icmp.rs b/src/upper/icmp.rs index bf71ced5..f866867d 100644 --- a/src/upper/icmp.rs +++ b/src/upper/icmp.rs @@ -103,6 +103,25 @@ pub const FIPS_OVERHEAD: u16 = 16 + 16 + 5 + 35 + 12 + 6 + 16; // 106 bytes /// ``` pub const FIPS_IPV6_OVERHEAD: u16 = 77; +/// Smallest remote-supplied transport path MTU this node will act on. +/// +/// The `path_mtu` field is an unsigned per-hop transit annotation carried +/// outside `proof_bytes`, and the `MtuExceeded` and `PathBroken` signals +/// arrive unencrypted, so any forwarder on the path can lower it. Below this +/// value the quantities derived from it degenerate: at a transport MTU of 137 +/// or less, [`mss_ceiling`] saturates to a TCP MSS of zero, at 138 it is a +/// single byte, and the derived MSS stays under a hundred all the way to 236. +/// At the floor itself the derived inner IPv6 MTU is 179 and the TCP MSS is +/// 119, clear of both the zero cliff and that band. +/// +/// A candidate below the floor is ignored — treated as no information at all, +/// never applied and never stored — rather than clamped, because clamping +/// would fabricate an estimate the node has no basis for. Locally derived link +/// MTUs are not subject to the floor; it applies only to values a remote party +/// supplied. A local value is exact, so the SYN-time clamp honours it however +/// small and refuses only the zero cliff, which no provenance makes usable. +pub const MIN_ACTIONABLE_PATH_MTU: u16 = 256; + /// Calculate the effective IPv6 MTU for FIPS-encapsulated traffic. /// /// Given a transport MTU (e.g., UDP payload size), returns the maximum @@ -112,6 +131,21 @@ pub fn effective_ipv6_mtu(transport_mtu: u16) -> u16 { transport_mtu.saturating_sub(FIPS_IPV6_OVERHEAD) } +/// Largest TCP segment size a FIPS-encapsulated path of `transport_mtu` +/// bytes on the wire admits: the effective inner IPv6 MTU less the 40-byte +/// IPv6 header and the 20-byte TCP header. +/// +/// Zero means the path has no room for even one payload byte, so no TCP +/// segment fits and no clamp derived from it carries information. That is +/// the one condition the SYN-time clamp treats as unusable regardless of +/// where the MTU came from, and it is why the seed site warns; both read it +/// from here so they cannot disagree about where the cliff is. +pub fn mss_ceiling(transport_mtu: u16) -> u16 { + effective_ipv6_mtu(transport_mtu) + .saturating_sub(40) + .saturating_sub(20) +} + /// Check if we should send an ICMPv6 error for this packet. /// /// Returns false if the packet is: diff --git a/src/upper/tcp_mss.rs b/src/upper/tcp_mss.rs index 6fd9776c..6ff3ae33 100644 --- a/src/upper/tcp_mss.rs +++ b/src/upper/tcp_mss.rs @@ -52,6 +52,12 @@ pub fn clamp_tcp_mss(ipv6_packet: &mut [u8], max_mss: u16) -> bool { return false; } + // A ceiling of zero carries no information and an MSS option of zero is + // not a legal segment size. Refuse the clamp rather than write it. + if max_mss == 0 { + return false; + } + // Get TCP header start let tcp_start = 40; if ipv6_packet.len() < tcp_start + TCP_HEADER_MIN_LEN { @@ -236,6 +242,24 @@ mod tests { assert_eq!(mss, 1200); } + #[test] + fn clamp_tcp_mss_with_zero_ceiling_leaves_mss_option_untouched() { + // A ceiling of zero reaches here only when something upstream + // degenerated. Writing it would put an MSS of 0 in the SYN and wedge + // the flow, so the clamp must refuse and report that it did nothing. + let src = [0xfd, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]; + let dst = [0xfd, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2]; + let mut packet = make_tcp_syn_packet(src, dst, 1460); + + let modified = clamp_tcp_mss(&mut packet, 0); + + assert!(!modified, "a zero ceiling must not count as a clamp"); + + let tcp_start = 40; + let mss = u16::from_be_bytes([packet[tcp_start + 22], packet[tcp_start + 23]]); + assert_eq!(mss, 1460, "MSS option must be left exactly as it arrived"); + } + #[test] fn test_clamp_tcp_mss_leaves_small_mss_unchanged() { let src = [0xfd, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]; diff --git a/src/upper/tun.rs b/src/upper/tun.rs index bc302b83..b18982ca 100644 --- a/src/upper/tun.rs +++ b/src/upper/tun.rs @@ -71,15 +71,13 @@ pub(crate) fn per_flow_max_mss( addr_bytes: &[u8], global_max_mss: u16, ) -> u16 { - use super::icmp::effective_ipv6_mtu; + use super::icmp::mss_ceiling; // RFC 8200 IPv6-minimum MTU (1280) → effective FIPS-encapsulated // payload (1203) → TCP segment after IPv6+TCP headers (1143). // Used as the conservative ceiling for empty-lookup destinations. const IPV6_MIN_MTU: u16 = 1280; - let conservative_max_mss = effective_ipv6_mtu(IPV6_MIN_MTU) - .saturating_sub(40) - .saturating_sub(20); + let conservative_max_mss = mss_ceiling(IPV6_MIN_MTU); let empty_lookup_ceiling = std::cmp::min(global_max_mss, conservative_max_mss); if addr_bytes.len() != 16 { @@ -118,9 +116,39 @@ pub(crate) fn per_flow_max_mss( ); return empty_lookup_ceiling; }; - let path_max_mss = effective_ipv6_mtu(path_mtu) - .saturating_sub(40) - .saturating_sub(20); + let path_max_mss = mss_ceiling(path_mtu); + // The actionable floor deliberately does not apply here. Every value a + // remote party supplies is refused before it can reach this map, at the + // path MTU state machine, the reactive `MtuExceeded` write and the + // discovery response, so a small stored value is one the node derived + // from its own outgoing link: a configured transport MTU, or the MTU a + // BLE connection negotiated, which on that transport is routinely well + // under the floor. Such a value is exact rather than suspect, and the + // tight clamp it yields is the reason it is stored: discarding it would + // advertise the conservative ceiling on a link that cannot carry it, and + // a direct link has no forwarder to answer with `MtuExceeded`, so the + // flow would stall with no feedback. + // + // What no provenance rescues is the arithmetic degenerating. At a stored + // MTU of 137 or less not one payload byte fits alongside the IPv6 and TCP + // headers, and `clamp_tcp_mss` refuses a ceiling of zero, which would + // leave the SYN carrying the kernel-natural MSS instead. Fall back to the + // conservative ceiling there; any positive result is by construction the + // largest segment the stored MTU admits. + // + // `trace!`, not `warn!`, because this runs on every packet rather than + // only on SYNs: one degenerate stored value would otherwise emit a WARN + // per packet indefinitely and bury every other warning on the node. The + // link promotion path warns once instead. + if path_max_mss == 0 { + trace!( + fips_addr = %fips_addr, + path_mtu, + empty_lookup_ceiling, + "per_flow_max_mss: stored path_mtu leaves no room for a TCP payload byte, using conservative ceiling" + ); + return empty_lookup_ceiling; + } let result = std::cmp::min(global_max_mss, path_max_mss); trace!( fips_addr = %fips_addr, @@ -1513,6 +1541,77 @@ mod tests { assert_eq!(per_flow_max_mss(&lookup, addr.as_bytes(), 1360), 1315); } + #[test] + fn per_flow_stored_mtu_admitting_no_payload_byte_falls_back_to_conservative_ceiling() { + // A stored MTU of 137 or less leaves nothing after the 77 bytes of + // FIPS encapsulation and the 40 + 20 bytes of IPv6 and TCP header, so + // the MSS arithmetic saturates to zero. Returning that zero would be + // worse than the fallback: `clamp_tcp_mss` refuses a ceiling of zero, + // so the SYN would go out at the kernel-natural MSS, unclamped. + for stored in [0u16, 1, 100, 137] { + let lookup = empty_lookup(); + let addr = fips_addr_with_node_byte(0x42); + lookup.write().unwrap().insert(addr, stored); + assert_eq!( + per_flow_max_mss(&lookup, addr.as_bytes(), 1360), + 1143, + "stored path_mtu {stored} admits no payload byte and must be ignored" + ); + } + } + + #[test] + fn per_flow_honors_a_locally_seeded_sub_floor_mtu_instead_of_loosening_to_the_ceiling() { + // Only `seed_path_mtu_for_link_peer` can put a sub-floor value in this + // map: every remote-supplied path MTU is refused at ingress, at the + // path MTU state machine, the reactive `MtuExceeded` write and the + // discovery response. A seeded value is therefore the node's own link + // measurement, and BLE negotiates one per connection that lands in + // this band routinely. + // + // Applying the remote-value floor here discarded it and advertised + // 1143 instead, which a link this narrow cannot carry: every full-size + // segment is refused by the transport, a direct link has no forwarder + // to answer with `MtuExceeded`, and the flow stalls with no feedback. + // The tight clamp is the whole reason the seed exists. + // + // 138 is the first MTU admitting a payload byte; 240 is a plausible + // negotiated BLE value; 255 is one below the remote-value floor. The + // whole table is evaluated before asserting, so a regression names + // every band it broke rather than only the first. + let want = [(138u16, 1u16), (240, 103), (255, 118)]; + let got: Vec<(u16, u16)> = want + .iter() + .map(|&(stored, _)| { + let lookup = empty_lookup(); + let addr = fips_addr_with_node_byte(0x42); + lookup.write().unwrap().insert(addr, stored); + (stored, per_flow_max_mss(&lookup, addr.as_bytes(), 1360)) + }) + .collect(); + assert_eq!( + got, + want.to_vec(), + "each locally seeded path_mtu must clamp tight, not fall back to 1143" + ); + } + + #[test] + fn per_flow_stored_mtu_at_the_actionable_floor_is_still_honored() { + // The smallest value the node will accept from a remote party still + // clamps to its own arithmetic and nothing coarser: 256 - 77 - 40 - 20 + // = 119. Reintroducing the remote-value floor as a clamp-time guard + // would leave this case passing, so it is pinned separately from the + // sub-floor table above. + let lookup = empty_lookup(); + let addr = fips_addr_with_node_byte(0x42); + lookup + .write() + .unwrap() + .insert(addr, super::super::icmp::MIN_ACTIONABLE_PATH_MTU); + assert_eq!(per_flow_max_mss(&lookup, addr.as_bytes(), 1360), 119); + } + #[test] fn per_flow_returns_conservative_ceiling_for_non_fips_addr() { // Non-fips IPv6 (e.g. fe80::/10 link-local) takes the empty-