diff --git a/CHANGELOG.md b/CHANGELOG.md index ea316156..a9298f1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -977,6 +977,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 is an emission change only: an unmodified peer parses the frame exactly as before. Which of the two signals is emitted still discloses whether the entry exists. +- A reactive `MtuExceeded` is now believed only when this node has actually + sent a frame larger than the bottleneck it reports. The signal is + unauthenticated: the admission gate narrows which destination may be named + but cannot say who named it, so a value at the floor was a legal value from + anyone, and one datagram drove a bound session's path MTU to 256 and pinned + the address-keyed entry the SYN-time MSS clamp reads, with recovery costing + three consecutive higher notifications across two notification intervals. + Each session now carries the largest frame this node has put on the wire + toward it since the last accepted decrease, and a report is refused unless it + names something smaller. Honest path-MTU discovery satisfies that by + construction, because the report exists only because a frame we sent did not + fit; a forgery has to wait for us to emit something bigger than the value it + wants to claim, which bounds every accepted claim from below by our own + traffic. The evidence is cleared on each accepted decrease and on release, so + one large send early in a session cannot vouch for the rest of it. + + The guard sits ahead of both effects rather than between them, which is also + where the existing floor check moved to: the floor previously ran after the + session's own path MTU had already been changed and so governed only the + lookup table. The reactive carrier now names its own floor constant, held + equal to the actionable floor so no hop legitimately configured with a small + transport MTU loses its feedback; corroboration, not the floor's value, is + what stops a legal-but-forged claim. A separate counter, rendered on the + fipstop Routing tab, distinguishes an uncorroborated refusal from a + below-floor one. + +- The path-MTU release a `PathBroken` drives is now rate limited per + destination on a budget of its own. That signal is unauthenticated too, and + the release discards a bottleneck this node learned by having a packet + dropped, so repeating the claim discarded a genuine value as fast as it could + be relearned. The limiter is a separate instance rather than the one the + coordinate warmup send already uses: a budget another signal can spend is not + a bound. Deferring a release is the safe direction, since the value kept is + the tighter one. - 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 diff --git a/src/bin/fipstop/ui/routing.rs b/src/bin/fipstop/ui/routing.rs index 19014763..3afe933e 100644 --- a/src/bin/fipstop/ui/routing.rs +++ b/src/bin/fipstop/ui/routing.rs @@ -304,6 +304,10 @@ fn draw_routing_stats( ("Emit Over Peer Budget", err("emit_over_peer_budget")), ("Emit Over Dest Interval", err("emit_over_dest_interval")), ("Emit Limiter At Capacity", err("emit_limiter_at_capacity")), + ( + "MTU Exceeded Uncorroborated", + err("mtu_exceeded_uncorroborated"), + ), ], )); right.push(Line::from("")); diff --git a/src/bin/fipstop/ui/snapshots.rs b/src/bin/fipstop/ui/snapshots.rs index a91611b4..5aa93f72 100644 --- a/src/bin/fipstop/ui/snapshots.rs +++ b/src/bin/fipstop/ui/snapshots.rs @@ -1151,7 +1151,7 @@ fn routing_focused_pane_scrolls() { // column is the taller of the two, so scrolling fully to the bottom would // over-scroll the right column past Congestion; this offset lands the // Congestion region inside the short window instead. - app1.scroll_offsets.insert((Tab::Routing, 2), 31); + app1.scroll_offsets.insert((Tab::Routing, 2), 32); let buf1 = testkit::render(100, 20, |frame, area| { super::routing::draw(frame, &app1, area); }); diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 95cbb229..efbdf1f0 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -42,6 +42,7 @@ "lookup_resp_mtu_below_floor": 0, "mtu_exceeded": 0, "mtu_exceeded_below_floor": 0, + "mtu_exceeded_uncorroborated": 0, "path_broken": 0, "path_mtu_notif_below_floor": 0, "unbound_broken": 0, diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index c8e11bcb..3601dd71 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -41,6 +41,29 @@ use crate::upper::icmp::FIPS_OVERHEAD; use secp256k1::PublicKey; use tracing::{debug, info, trace, warn}; +/// Minimum interval between path-MTU releases driven by `PathBroken` for one +/// destination. +/// +/// `PathBroken` is unauthenticated, so a release is a remote party's claim +/// that the path a tightened MTU described is gone. Without an interval the +/// claim can be repeated at line rate, discarding a genuinely learned +/// bottleneck as fast as it is relearned. Raising it defers a legitimate +/// release after a second real break, which costs throughput on the new path +/// but never a blackhole, since the deferred value is the tighter one. +pub(in crate::node) const PATH_MTU_RELEASE_MIN_INTERVAL: std::time::Duration = + std::time::Duration::from_millis(1000); + +/// Bytes the link layer adds to an encoded `SessionDatagram` on its way to the +/// wire: the established FMP header, the 4-byte session-relative timestamp and +/// the AEAD tag. Mirrors the buffer `send_encrypted_link_message_with_ce` +/// builds. +const LINK_FRAME_OVERHEAD: usize = ESTABLISHED_HEADER_SIZE + 4 + crate::noise::TAG_SIZE; + +/// Wire size of an encoded `SessionDatagram` of `encoded_len` bytes. +fn link_wire_len(encoded_len: usize) -> usize { + encoded_len + LINK_FRAME_OVERHEAD +} + /// Inputs to `try_send_session_data_pipelined` — the FSP+FMP pipelined /// fast path that hands both AEAD operations to the encrypt worker /// in a single dispatch. @@ -1560,8 +1583,17 @@ impl Node { 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); + // it rather than carrying it onto whatever path replaces it. Rate + // limited per destination on its own budget: PathBroken is + // unauthenticated, and an unlimited release discards a genuinely + // learned bottleneck as fast as it is relearned. The budget is not + // shared with any other signal, so nothing else can spend it. + if self.path_mtu_release_limiter.should_send(&msg.dest_addr) { + self.path_mtu_lookup_release(&msg.dest_addr); + } else { + trace!(dest = %msg.dest_addr, + "PathBroken path MTU release rate-limited, keeping the stored value"); + } // Trigger re-discovery to get fresh coordinates, but only if we have // the target's identity cached — otherwise we can't verify the @@ -1628,6 +1660,55 @@ impl Node { "MtuExceeded: transit router reports oversized packet" ); + // Both effects below — the session's own path MTU and the + // FipsAddress-keyed lookup the TUN MSS clamp reads — are refused from + // here, so one return covers both. The guards sit ahead of the apply + // rather than between the two effects, which is what makes the floor + // govern `current_mtu` and not only the lookup table. + + // Refuse 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. The reactive carrier is unauthenticated, so it has its own + // floor constant, currently equal to the actionable one. + if msg.mtu < crate::upper::icmp::MIN_REACTIVE_PATH_MTU { + warn!( + dest = %peer_name, + reporter = %msg.reporter, + bottleneck_mtu = msg.mtu, + floor = crate::upper::icmp::MIN_REACTIVE_PATH_MTU, + "MtuExceeded reports a path MTU below the actionable floor; ignoring" + ); + self.metrics().errors.mtu_exceeded_below_floor.inc(); + return; + } + + // Corroboration. The admission gate narrows which destination may be + // named; it cannot authenticate the reporter, so a legal value is a + // legal value from anyone and the floor alone only sets the outcome of + // a forgery rather than preventing it. An honest report exists only + // because a frame this node emitted did not fit some hop, so require + // that this node has actually sent something larger than the value + // being claimed since the last accepted decrease. Honest path-MTU + // discovery satisfies this by construction; a forgery has to wait for + // us to emit a frame bigger than the value it wants to claim, which + // bounds every accepted claim from below by our own traffic. + let sent_wire_len = self + .sessions + .get(&msg.dest_addr) + .map(|e| e.max_sent_wire_len()) + .unwrap_or(0); + if msg.mtu >= sent_wire_len { + debug!( + dest = %peer_name, + reporter = %msg.reporter, + bottleneck_mtu = msg.mtu, + max_sent_wire_len = sent_wire_len, + "MtuExceeded reports a bottleneck no smaller than anything this node has sent; ignoring" + ); + self.metrics().errors.mtu_exceeded_uncorroborated.inc(); + return; + } + // Apply to PathMtuState: immediate decrease via apply_notification() if let Some(entry) = self.sessions.get_mut(&msg.dest_addr) && let Some(mmp) = entry.mmp_mut() @@ -1646,20 +1727,12 @@ 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; + // Spent: the evidence vouched for this decrease and does not vouch for + // the next one. An initiating session has no `mmp` and so reaches this + // with the apply above skipped; the reset belongs to the acceptance, + // not to the apply. + if let Some(entry) = self.sessions.get_mut(&msg.dest_addr) { + entry.clear_sent_wire_len(); } // Mirror the bottleneck into the FipsAddress-keyed lookup used by @@ -2192,6 +2265,7 @@ impl Node { if let Some(entry) = self.sessions.get_mut(dest_addr) { entry.record_sent(send.payload.len()); + entry.record_sent_wire_len(wire_capacity); if let Some(mmp) = entry.mmp_mut() { mmp.sender.record_sent( fsp_counter, @@ -2467,6 +2541,13 @@ impl Node { self.send_encrypted_link_message(&next_hop_addr, &encoded) .await?; self.metrics().forwarding.record_originated(encoded.len()); + + // Evidence for the reactive path-MTU carrier. A transit hop + // re-encapsulates what it forwards, so the frame that overflows a + // downstream link is the size this frame is here. + if let Some(entry) = self.sessions.get_mut(&datagram.dest_addr) { + entry.record_sent_wire_len(link_wire_len(encoded.len())); + } Ok(()) } diff --git a/src/node/metrics.rs b/src/node/metrics.rs index c32dfcef..896a3621 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -515,6 +515,11 @@ pub struct ErrorMetrics { /// count means a forwarder on the reverse path is mangling the unsigned /// annotation. pub lookup_resp_mtu_below_floor: Counter, + /// `MtuExceeded` signals ignored because this node has not sent a frame + /// larger than the bottleneck they report since the last accepted + /// decrease. An honest report cannot arise without such a frame, so a + /// rising count is a forged or stale reactive signal. + pub mtu_exceeded_uncorroborated: Counter, pub unbound: UnboundSignals, /// Routing errors this node declined to emit because the authenticated /// link peer that induced them had spent its budget. A rising count is @@ -548,6 +553,7 @@ impl ErrorMetrics { emit_over_peer_budget: self.emit_over_peer_budget.get(), emit_over_dest_interval: self.emit_over_dest_interval.get(), emit_limiter_at_capacity: self.emit_limiter_at_capacity.get(), + mtu_exceeded_uncorroborated: self.mtu_exceeded_uncorroborated.get(), } } } diff --git a/src/node/mod.rs b/src/node/mod.rs index 35b4a600..c18c610e 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -521,6 +521,10 @@ pub struct Node { peer_error_budget: PeerErrorBudget, /// Rate limiter for source-side CoordsRequired/PathBroken responses. coords_response_rate_limiter: RoutingErrorRateLimiter, + /// Rate limiter for PathBroken-driven path-MTU releases, per destination. + /// Deliberately its own instance: a budget another signal can spend is + /// not a bound on this one. + path_mtu_release_limiter: RoutingErrorRateLimiter, /// Backoff for failed discovery lookups (originator-side). discovery_backoff: DiscoveryBackoff, /// Rate limiter for forwarded discovery requests (transit-side). @@ -821,6 +825,9 @@ impl Node { coords_response_rate_limiter: RoutingErrorRateLimiter::with_interval( std::time::Duration::from_millis(coords_response_interval_ms), ), + path_mtu_release_limiter: RoutingErrorRateLimiter::with_interval( + crate::node::handlers::session::PATH_MTU_RELEASE_MIN_INTERVAL, + ), discovery_backoff: DiscoveryBackoff::with_params(backoff_base_secs, backoff_max_secs), discovery_forward_limiter: DiscoveryForwardRateLimiter::with_interval( std::time::Duration::from_secs(forward_min_interval_secs), @@ -984,6 +991,9 @@ impl Node { coords_response_rate_limiter: RoutingErrorRateLimiter::with_interval( std::time::Duration::from_millis(coords_response_interval_ms), ), + path_mtu_release_limiter: RoutingErrorRateLimiter::with_interval( + crate::node::handlers::session::PATH_MTU_RELEASE_MIN_INTERVAL, + ), discovery_backoff: DiscoveryBackoff::new(), discovery_forward_limiter: DiscoveryForwardRateLimiter::new(), discovery_sign_limiter: LookupSignRateLimiter::new(), @@ -2662,6 +2672,12 @@ impl Node { /// `FipsAddress`-keyed map the TCP MSS clamp reads, and the session's own /// source-side path MTU estimate. fn path_mtu_lookup_release(&mut self, addr: &NodeAddr) { + // The evidence that corroborates a reactive MtuExceeded described the + // path being released, so it does not vouch for whatever replaces it. + if let Some(entry) = self.sessions.get_mut(addr) { + entry.clear_sent_wire_len(); + } + // The session's own source-side estimate described the same dead path, // and the increase ladder is the only thing that would ever raise it // again. Reset it here so the two halves of "this path is gone" stay diff --git a/src/node/session.rs b/src/node/session.rs index 4c036e59..29f9ac07 100644 --- a/src/node/session.rs +++ b/src/node/session.rs @@ -102,6 +102,15 @@ pub(crate) struct SessionEntry { /// Whether this node initiated the Noise handshake. /// Used for spin bit role assignment in session-layer MMP. is_initiator: bool, + /// Largest on-the-wire frame this node has sent toward the remote since + /// the last accepted path-MTU decrease or release, in bytes. + /// + /// Corroborates a reactive `MtuExceeded`, which is unauthenticated: an + /// honest report exists only because a frame this node emitted did not + /// fit some hop, so an honest report always names a value below this. + /// Reset on each accepted decrease and on release so one historical + /// large send cannot vouch for a session's whole lifetime. + max_sent_wire_len: u16, /// Session-layer MMP state. Initialized on Established transition. mmp: Option, @@ -205,6 +214,7 @@ impl SessionEntry { session_start_ms: 0, coords_warmup_remaining: 0, is_initiator, + max_sent_wire_len: 0, mmp: None, packets_sent: 0, packets_recv: 0, @@ -308,6 +318,27 @@ impl SessionEntry { self.coords_warmup_remaining = value; } + /// Largest wire frame sent toward the remote since the last accepted + /// path-MTU decrease or release. + pub(crate) fn max_sent_wire_len(&self) -> u16 { + self.max_sent_wire_len + } + + /// Note a frame of `wire_len` bytes sent toward the remote, keeping the + /// largest. Frames beyond `u16::MAX` saturate, which only ever makes the + /// corroboration more permissive and cannot exceed what a path MTU can + /// name. + pub(crate) fn record_sent_wire_len(&mut self, wire_len: usize) { + let wire_len = u16::try_from(wire_len).unwrap_or(u16::MAX); + self.max_sent_wire_len = self.max_sent_wire_len.max(wire_len); + } + + /// Forget what has been sent, so the next reactive report needs fresh + /// evidence of its own. + pub(crate) fn clear_sent_wire_len(&mut self) { + self.max_sent_wire_len = 0; + } + /// Mark the session as started (transition to Established). /// /// Records the current time as the session start for computing diff --git a/src/node/stats.rs b/src/node/stats.rs index 957371df..52d0f399 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -427,6 +427,7 @@ pub struct ErrorSignalStatsSnapshot { pub emit_over_peer_budget: u64, pub emit_over_dest_interval: u64, pub emit_limiter_at_capacity: u64, + pub mtu_exceeded_uncorroborated: u64, } #[derive(Clone, Debug, Default, Serialize)] diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 44f800c7..2fe5eedd 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -2228,6 +2228,17 @@ fn install_halfopen(node: &mut Node, claimed: NodeAddr) { node.sessions.insert(claimed, entry); } +/// Record that this node put a frame of `wire_len` bytes on the wire toward +/// `dest`, which is what corroborates a reactive `MtuExceeded` reporting a +/// smaller bottleneck. Honest path-MTU discovery produces this by sending; +/// a handler test that installs a session without sending has to state it. +fn note_sent_wire_len(node: &mut Node, dest: &NodeAddr, wire_len: usize) { + node.sessions + .get_mut(dest) + .expect("session must exist to corroborate a report") + .record_sent_wire_len(wire_len); +} + /// Install the entry `initiate_session` creates: an address this node chose /// itself, with the handshake still in flight and MMP not yet initialized. fn install_initiating(node: &mut Node, remote: &Identity) { @@ -2263,6 +2274,7 @@ async fn test_handle_mtu_exceeded_writes_path_mtu_lookup_when_empty() { "lookup should start empty for this destination" ); + note_sent_wire_len(&mut tn.node, &dest, 1400); let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); tn.node.handle_mtu_exceeded(&reporter, &inner).await; @@ -2289,6 +2301,7 @@ async fn test_handle_mtu_exceeded_tightens_existing_path_mtu_lookup() { // response that didn't reflect the forward-path bottleneck). tn.node.path_mtu_lookup_insert(dest_fips, 1500); + note_sent_wire_len(&mut tn.node, &dest, 1400); let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); tn.node.handle_mtu_exceeded(&reporter, &inner).await; @@ -2420,8 +2433,9 @@ async fn test_handle_mtu_exceeded_at_the_floor_still_writes_path_mtu_lookup() { let dest = *remote.node_addr(); 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 floor = crate::upper::icmp::MIN_REACTIVE_PATH_MTU; + note_sent_wire_len(&mut tn.node, &dest, 1400); let inner = build_mtu_exceeded_inner(&dest, &reporter, floor); tn.node.handle_mtu_exceeded(&reporter, &inner).await; @@ -2944,6 +2958,7 @@ async fn test_mtu_exceeded_for_a_session_we_initiated_seeds_path_mtu_lookup_befo let reporter = NodeAddr::from_bytes([0xBB; 16]); let dest_fips = crate::FipsAddress::from_node_addr(&dest); + note_sent_wire_len(&mut node, &dest, 1400); let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); node.handle_mtu_exceeded(&reporter, &inner).await; @@ -2970,6 +2985,7 @@ async fn test_mtu_exceeded_from_a_third_party_forwarder_still_tightens_an_active let reporter = NodeAddr::from_bytes([0xBB; 16]); let dest_fips = crate::FipsAddress::from_node_addr(&dest); + note_sent_wire_len(&mut node, &dest, 1400); let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); node.handle_mtu_exceeded(&reporter, &inner).await; @@ -4959,3 +4975,226 @@ async fn test_peer_restart_reestablishes_through_a_pending_session_that_waited_o cleanup_nodes(&mut nodes).await; } + +// --------------------------------------------------------------------------- +// Reactive MtuExceeded: corroboration against what this node actually sent +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn a_reactive_mtu_exceeded_at_the_floor_no_longer_pins_a_session_this_node_has_not_overfilled() + { + // The defect itself. A report of exactly the floor is a legal value, and + // the admission gate cannot tell an honest forwarder from anyone else, so + // one packet drove a bound session's path MTU to the floor and pinned the + // FipsAddress-keyed entry the SYN-time MSS clamp reads. Nothing this node + // sent could have overflowed a hop at that size, so no honest report of it + // exists. + let mut node = make_node(); + + let remote = Identity::generate(); + install_established_session_with_mmp(&mut node, &remote); + let dest = *remote.node_addr(); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + let before = node + .sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()); + + let inner = + build_mtu_exceeded_inner(&dest, &reporter, crate::upper::icmp::MIN_REACTIVE_PATH_MTU); + node.handle_mtu_exceeded(&reporter, &inner).await; + + assert_eq!( + node.sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()), + before, + "an uncorroborated report must leave the session path MTU alone" + ); + assert_eq!( + node.path_mtu_lookup_get(&dest_fips), + None, + "an uncorroborated report must leave no clamp entry behind" + ); + assert_eq!( + node.metrics().errors.mtu_exceeded_uncorroborated.get(), + 1, + "the refusal must be counted apart from the below-floor refusal" + ); + assert_eq!( + node.metrics().errors.mtu_exceeded_below_floor.get(), + 0, + "the floor is not what refused this; the value is exactly at it" + ); +} + +#[tokio::test] +async fn an_initiating_session_refuses_an_uncorroborated_report_and_accepts_a_corroborated_one() { + // The lookup write is the effect that survives on an initiating session, + // which has no MMP state at all, so this branch needs its own coverage: + // a guard placed on the apply rather than ahead of it would miss it. + let mut node = make_node(); + + let remote = Identity::generate(); + install_initiating(&mut node, &remote); + let dest = *remote.node_addr(); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + let inner = build_mtu_exceeded_inner(&dest, &reporter, 800); + node.handle_mtu_exceeded(&reporter, &inner).await; + assert_eq!( + node.path_mtu_lookup_get(&dest_fips), + None, + "nothing this node sent could have overflowed a hop at 800 bytes" + ); + + // A SessionSetup can itself be the datagram that overflows a hop, so an + // initiating session must still be able to act on a real report. + note_sent_wire_len(&mut node, &dest, 1400); + node.handle_mtu_exceeded(&reporter, &inner).await; + assert_eq!( + node.path_mtu_lookup_get(&dest_fips), + Some(800), + "a report corroborated by an oversized send must still be applied" + ); +} + +#[tokio::test] +async fn a_second_reactive_decrease_needs_its_own_corroborating_send() { + // The evidence is spent on the decrease it vouched for. Otherwise one + // large send early in a session would vouch for every forged report for + // the rest of that session's life. + let mut node = make_node(); + + let remote = Identity::generate(); + install_established_session_with_mmp(&mut node, &remote); + let dest = *remote.node_addr(); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + + note_sent_wire_len(&mut node, &dest, 1400); + let first = build_mtu_exceeded_inner(&dest, &reporter, 1200); + node.handle_mtu_exceeded(&reporter, &first).await; + assert_eq!( + node.sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()), + Some(1200), + "the corroborated first decrease is accepted" + ); + + let second = build_mtu_exceeded_inner(&dest, &reporter, 600); + node.handle_mtu_exceeded(&reporter, &second).await; + assert_eq!( + node.sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()), + Some(1200), + "a further decrease needs evidence of its own" + ); + + // A genuine re-route onto a smaller hop is preceded by a send that hop + // drops, so the honest sequence still converges. + note_sent_wire_len(&mut node, &dest, 900); + node.handle_mtu_exceeded(&reporter, &second).await; + assert_eq!( + node.sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()), + Some(600), + "once this node has again sent something that does not fit, the report applies" + ); +} + +#[tokio::test] +async fn a_corroborated_report_below_the_reactive_floor_is_still_refused() { + // Corroboration and the floor are independent refusals. A hop that really + // is tiny still cannot drive the clamp into the band where the derived + // MSS degenerates. + let mut node = make_node(); + + let remote = Identity::generate(); + install_established_session_with_mmp(&mut node, &remote); + let dest = *remote.node_addr(); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + note_sent_wire_len(&mut node, &dest, 1400); + let inner = build_mtu_exceeded_inner( + &dest, + &reporter, + crate::upper::icmp::MIN_REACTIVE_PATH_MTU - 1, + ); + node.handle_mtu_exceeded(&reporter, &inner).await; + + assert_eq!(node.path_mtu_lookup_get(&dest_fips), None); + assert_eq!(node.metrics().errors.mtu_exceeded_below_floor.get(), 1); + assert_eq!(node.metrics().errors.mtu_exceeded_uncorroborated.get(), 0); +} + +#[tokio::test] +async fn the_authenticated_path_mtu_notification_still_applies_at_the_actionable_floor() { + // The reactive guards must not leak onto the carrier that arrives inside + // an established session on the decrypted path, which is authenticated and + // needs no corroboration. + let mut node = make_node(); + + let remote = Identity::generate(); + install_established_session_with_mmp(&mut node, &remote); + let dest = *remote.node_addr(); + + let floor = crate::upper::icmp::MIN_ACTIONABLE_PATH_MTU; + let body = build_path_mtu_notification_body(floor); + node.handle_session_path_mtu_notification(&dest, &body); + + assert_eq!( + node.sessions + .get(&dest) + .and_then(|e| e.mmp()) + .map(|m| m.path_mtu.current_mtu()), + Some(floor), + "the authenticated carrier still applies a value at the actionable floor" + ); +} + +#[tokio::test] +async fn a_path_broken_flood_releases_the_stored_path_mtu_only_once_per_interval() { + use crate::protocol::PathBroken; + + // PathBroken is unauthenticated and its release discards a bottleneck this + // node learned the hard way. Unlimited, the claim can be repeated as fast + // as it can be sent, so a genuinely learned value never survives. + let mut node = make_node(); + + let remote = Identity::generate(); + install_initiating(&mut node, &remote); + let dest = *remote.node_addr(); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + let encoded = PathBroken::new(dest, reporter).encode(); + let inner = &encoded[5..]; + + node.path_mtu_lookup_insert(dest_fips, 700); + node.handle_path_broken(&reporter, inner).await; + assert_eq!( + node.path_mtu_lookup_get(&dest_fips), + None, + "the first PathBroken still releases" + ); + + node.path_mtu_lookup_insert(dest_fips, 700); + node.handle_path_broken(&reporter, inner).await; + assert_eq!( + node.path_mtu_lookup_get(&dest_fips), + Some(700), + "a second release for the same destination inside the interval is refused" + ); +} diff --git a/src/upper/icmp.rs b/src/upper/icmp.rs index f866867d..d4f6d30e 100644 --- a/src/upper/icmp.rs +++ b/src/upper/icmp.rs @@ -122,6 +122,22 @@ pub const FIPS_IPV6_OVERHEAD: u16 = 77; /// small and refuses only the zero cliff, which no provenance makes usable. pub const MIN_ACTIONABLE_PATH_MTU: u16 = 256; +/// Smallest path MTU this node will act on when the claim arrives on the +/// unauthenticated reactive carrier, `MtuExceeded`. +/// +/// Held equal to [`MIN_ACTIONABLE_PATH_MTU`] so no hop legitimately configured +/// with a small transport MTU loses reactive feedback. It is a separate +/// constant because the two carriers differ in what they prove: the +/// authenticated `PathMtuNotification` and the proof-carrying discovery +/// response come from a party this node has verified, whereas this one comes +/// from whoever could route a datagram here. What keeps a legal-but-forged +/// claim from pinning a session is corroboration against what this node has +/// actually sent, not this floor. Raising it (576 is the value the original +/// path-MTU floor design proposed, and derives an inner IPv6 MTU of 499) +/// bounds the outcome of an uncorroborated claim further, at the cost of +/// ignoring an honest report from any hop configured between the two values. +pub const MIN_REACTIVE_PATH_MTU: u16 = MIN_ACTIONABLE_PATH_MTU; + /// Calculate the effective IPv6 MTU for FIPS-encapsulated traffic. /// /// Given a transport MTU (e.g., UDP payload size), returns the maximum