From f444457270d9b9a8f860573bc9f402c305c35a27 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Thu, 24 Sep 2026 14:12:26 +0000 Subject: [PATCH] Move ICMPv6 error sending toward the host into ipv6tun Building a Destination Unreachable or Packet Too Big and writing it to the TUN is host-side IPv6 work, but the send logic lived in the node's session handler. Move it to ipv6tun::icmp behind IcmpContext, which borrows the TUN channel, our address and the Packet Too Big rate limiter for the length of one use. Borrowing rather than holding a clone of the TUN sender keeps teardown able to close the channel. The logic is unchanged. Packet Too Big still consults the per-source limiter before anything else, and Destination Unreachable is still unlimited. The discovery lookup timeout now hands its queued packets to the context as one no-route report instead of sending one reply per packet itself. Node keeps send_icmpv6_dest_unreachable and send_icmpv6_packet_too_big as wrappers over the context, so the outbound handler and the tests that drive it are unchanged; the Destination Unreachable wrapper now takes &mut self because the context borrows the limiter. The two Packet Too Big debug lines now log under fips::ipv6tun::icmp instead of fips::node::handlers::session. Add that target to the test harness filters that relied on fips::node=debug or the session trace filter to show them, and note the rename in the changelog. --- CHANGELOG.md | 10 ++- src/ipv6tun/icmp.rs | 97 ++++++++++++++++++++++++ src/node/handlers/lookup.rs | 4 +- src/node/handlers/session.rs | 67 ++++------------ testing/acl-allowlist/README.md | 2 +- testing/acl-allowlist/docker-compose.yml | 2 +- testing/firewall/docker-compose.yml | 2 +- testing/iface-binding/docker-compose.yml | 2 +- testing/mesh-lab/compose-trace.yml | 5 +- 9 files changed, 128 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 774f3f42..95989d04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -332,9 +332,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `fips::upper::*` to `fips::ipv6tun::*`, because the module they log from is now `ipv6tun` (for example `fips::upper::tun` becomes `fips::ipv6tun::tun`). The hosts-file loader and reloader log as `fips::hosts` rather than - `fips::upper::hosts`, since the hosts file is now a top-level module. An - existing `RUST_LOG` filter naming an old target still parses and simply - stops matching, so the symptom is missing log lines rather than an error. + `fips::upper::hosts`, since the hosts file is now a top-level module. The + ICMPv6 Packet Too Big debug lines ("Sending ICMP Packet Too Big", "Rate + limiting ICMP Packet Too Big") log as `fips::ipv6tun::icmp` rather than + `fips::node::handlers::session`, so a `fips::node=debug` filter no longer + shows them. An existing `RUST_LOG` filter naming an old target still parses + and simply stops matching, so the symptom is missing log lines rather than an + error. Update `RUST_LOG` filters, journal-watch recipes and any log-scraping alert accordingly. The library path `fips::upper` still resolves. diff --git a/src/ipv6tun/icmp.rs b/src/ipv6tun/icmp.rs index 080e2534..963a54b9 100644 --- a/src/ipv6tun/icmp.rs +++ b/src/ipv6tun/icmp.rs @@ -4,7 +4,10 @@ //! Currently supports Destination Unreachable (Type 1) for //! packets that cannot be routed. +use super::icmp_rate_limit::IcmpRateLimiter; +use super::tun::TunTx; use std::net::Ipv6Addr; +use tracing::debug; /// ICMPv6 message types. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -370,6 +373,100 @@ pub fn build_packet_too_big( Some(response) } +/// Sends ICMPv6 errors back to the host through the TUN writer. +/// +/// Holds what both replies need: the channel to the TUN writer, this +/// node's address, and the per-source Packet Too Big limiter. It borrows +/// all three for the duration of one use, so it never keeps the TUN +/// channel open past the adapter's teardown. With no TUN channel the +/// replies are built and dropped. +pub(crate) struct IcmpContext<'a> { + /// Channel to the TUN writer, `None` when no TUN is up. + tun_tx: Option<&'a TunTx>, + /// This node's address, the source of Destination Unreachable replies. + our_addr: Ipv6Addr, + /// Per-source limiter, applied to Packet Too Big only. + limiter: &'a mut IcmpRateLimiter, +} + +impl<'a> IcmpContext<'a> { + /// Borrow the TUN channel and limiter, with our address as `our_addr`. + pub(crate) fn new( + tun_tx: Option<&'a TunTx>, + our_addr: Ipv6Addr, + limiter: &'a mut IcmpRateLimiter, + ) -> Self { + Self { + tun_tx, + our_addr, + limiter, + } + } + + /// Send ICMPv6 Destination Unreachable back through TUN. + /// + /// Not rate limited: only Packet Too Big goes through the limiter. + pub(crate) fn dest_unreachable(&self, original_packet: &[u8]) { + if !should_send_icmp_error(original_packet) { + return; + } + + if let Some(response) = + build_dest_unreachable(original_packet, DestUnreachableCode::NoRoute, self.our_addr) + && let Some(tun_tx) = self.tun_tx + { + let _ = tun_tx.send(response); + } + } + + /// Answer packets that were queued for a destination found to have no + /// route, with one Destination Unreachable each. + pub(crate) fn no_route>(&self, packets: impl IntoIterator) { + for packet in packets { + self.dest_unreachable(packet.as_ref()); + } + } + + /// Send ICMPv6 Packet Too Big back through TUN. + /// + /// Rate-limited per source address to prevent ICMP floods from + /// misconfigured applications sending repeated oversized packets. + pub(crate) fn packet_too_big(&mut self, original_packet: &[u8], mtu: u32) { + // Extract source address for rate limiting + if original_packet.len() < 40 { + return; + } + let src_addr = Ipv6Addr::from(<[u8; 16]>::try_from(&original_packet[8..24]).unwrap()); + + // Rate limit ICMP PTB messages per source + if !self.limiter.should_send(src_addr) { + debug!( + src = %src_addr, + "Rate limiting ICMP Packet Too Big" + ); + return; + } + + // Use the original packet's *destination* as the ICMP source so the + // kernel sees the PTB coming from a remote router, not from itself. + // Linux ignores PTBs whose source matches a local address, which + // causes a PMTUD blackhole when both src and ICMP-src are local. + let dest_addr = Ipv6Addr::from(<[u8; 16]>::try_from(&original_packet[24..40]).unwrap()); + if let Some(response) = build_packet_too_big(original_packet, mtu, dest_addr) + && let Some(tun_tx) = self.tun_tx + { + debug!( + original_src = %src_addr, + original_dst = %dest_addr, + packet_size = original_packet.len(), + reported_mtu = mtu, + "Sending ICMP Packet Too Big" + ); + let _ = tun_tx.send(response); + } + } +} + /// Calculate ICMPv6 checksum per RFC 4443. /// /// The checksum is calculated over a pseudo-header plus the ICMPv6 message. diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 90a45208..ad49fc09 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -793,9 +793,7 @@ impl Node { "Discovery lookup timed out, destination unreachable" ); if let Some(packets) = queued { - for pkt in &packets { - self.send_icmpv6_dest_unreachable(pkt); - } + self.host_icmp().no_route(&packets); } } } diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 15aad913..ce65171e 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -6,6 +6,7 @@ //! encrypted data, and error signals (CoordsRequired, PathBroken). use crate::NodeAddr; +use crate::ipv6tun::icmp::IcmpContext; use crate::node::handlers::mmp::format_throughput; use crate::node::rate_limit::Msg1Class; use crate::node::reject::{RejectReason, SessionReject}; @@ -3143,24 +3144,20 @@ impl Node { self.queue_pending_packet(dest_addr, ipv6_packet); } + /// Borrow the host-facing ICMPv6 sender: the TUN channel, our address + /// and the Packet Too Big rate limiter. + pub(in crate::node) fn host_icmp(&mut self) -> IcmpContext<'_> { + let our_ipv6 = crate::FipsAddress::from_node_addr(self.node_addr()).to_ipv6(); + IcmpContext::new( + self.supervisor.tun_tx.as_ref(), + our_ipv6, + &mut self.icmp_rate_limiter, + ) + } + /// Send ICMPv6 Destination Unreachable back through TUN. - pub(in crate::node) fn send_icmpv6_dest_unreachable(&self, original_packet: &[u8]) { - use crate::FipsAddress; - use crate::upper::icmp::{ - DestUnreachableCode, build_dest_unreachable, should_send_icmp_error, - }; - - if !should_send_icmp_error(original_packet) { - return; - } - - let our_ipv6 = FipsAddress::from_node_addr(self.node_addr()).to_ipv6(); - if let Some(response) = - build_dest_unreachable(original_packet, DestUnreachableCode::NoRoute, our_ipv6) - && let Some(tun_tx) = &self.supervisor.tun_tx - { - let _ = tun_tx.send(response); - } + pub(in crate::node) fn send_icmpv6_dest_unreachable(&mut self, original_packet: &[u8]) { + self.host_icmp().dest_unreachable(original_packet); } /// Send ICMPv6 Packet Too Big back through TUN. @@ -3168,41 +3165,7 @@ impl Node { /// Rate-limited per source address to prevent ICMP floods from /// misconfigured applications sending repeated oversized packets. pub(in crate::node) fn send_icmpv6_packet_too_big(&mut self, original_packet: &[u8], mtu: u32) { - use crate::upper::icmp::build_packet_too_big; - use std::net::Ipv6Addr; - - // Extract source address for rate limiting - if original_packet.len() < 40 { - return; - } - let src_addr = Ipv6Addr::from(<[u8; 16]>::try_from(&original_packet[8..24]).unwrap()); - - // Rate limit ICMP PTB messages per source - if !self.icmp_rate_limiter.should_send(src_addr) { - debug!( - src = %src_addr, - "Rate limiting ICMP Packet Too Big" - ); - return; - } - - // Use the original packet's *destination* as the ICMP source so the - // kernel sees the PTB coming from a remote router, not from itself. - // Linux ignores PTBs whose source matches a local address, which - // causes a PMTUD blackhole when both src and ICMP-src are local. - let dest_addr = Ipv6Addr::from(<[u8; 16]>::try_from(&original_packet[24..40]).unwrap()); - if let Some(response) = build_packet_too_big(original_packet, mtu, dest_addr) - && let Some(tun_tx) = &self.supervisor.tun_tx - { - debug!( - original_src = %src_addr, - original_dst = %dest_addr, - packet_size = original_packet.len(), - reported_mtu = mtu, - "Sending ICMP Packet Too Big" - ); - let _ = tun_tx.send(response); - } + self.host_icmp().packet_too_big(original_packet, mtu); } /// Queue a packet while waiting for session establishment. diff --git a/testing/acl-allowlist/README.md b/testing/acl-allowlist/README.md index 95d0f2ee..866861d8 100644 --- a/testing/acl-allowlist/README.md +++ b/testing/acl-allowlist/README.md @@ -157,7 +157,7 @@ Rejected peer by ACL ... context=inbound_handshake decision=denylist match ``` Those messages are now emitted at debug level. This harness enables -`RUST_LOG=info,fips::node=debug` so the ACL rejection details stay visible in +`fips::node=debug` in `RUST_LOG` so the ACL rejection details stay visible in test logs, and operators can temporarily raise log level the same way when diagnosing ACL issues locally. diff --git a/testing/acl-allowlist/docker-compose.yml b/testing/acl-allowlist/docker-compose.yml index a20428a0..0bee0f2e 100644 --- a/testing/acl-allowlist/docker-compose.yml +++ b/testing/acl-allowlist/docker-compose.yml @@ -35,7 +35,7 @@ x-fips-common: &fips-common restart: "no" environment: - FIPS_TEST_MODE=default - - RUST_LOG=info,fips::node=debug + - RUST_LOG=info,fips::node=debug,fips::ipv6tun::icmp=debug volumes: - ../docker/resolv.conf:/etc/resolv.conf:ro diff --git a/testing/firewall/docker-compose.yml b/testing/firewall/docker-compose.yml index fe657732..13498b18 100644 --- a/testing/firewall/docker-compose.yml +++ b/testing/firewall/docker-compose.yml @@ -36,7 +36,7 @@ x-fips-common: &fips-common restart: "no" environment: - FIPS_TEST_MODE=default - - RUST_LOG=info,fips::node=debug + - RUST_LOG=info,fips::node=debug,fips::ipv6tun::icmp=debug services: service-a: diff --git a/testing/iface-binding/docker-compose.yml b/testing/iface-binding/docker-compose.yml index fd87ba6b..1d631ca2 100644 --- a/testing/iface-binding/docker-compose.yml +++ b/testing/iface-binding/docker-compose.yml @@ -33,7 +33,7 @@ x-fips-common: &fips-common # the daemon, which is precisely the workaround this mechanism retires. The # daemon must do its own waiting here or the suite proves nothing. - FIPS_TEST_MODE=default - - RUST_LOG=info,fips::transport::ethernet=debug,fips::node=debug + - RUST_LOG=info,fips::transport::ethernet=debug,fips::node=debug,fips::ipv6tun::icmp=debug networks: - ifb-net diff --git a/testing/mesh-lab/compose-trace.yml b/testing/mesh-lab/compose-trace.yml index feb0f7ae..dbf7a69b 100644 --- a/testing/mesh-lab/compose-trace.yml +++ b/testing/mesh-lab/compose-trace.yml @@ -9,6 +9,9 @@ # - fips::node::handlers::session — FSP K-bit cutover, drain # - fips::node::dataplane::encrypted — FMP K-bit flip detection # - fips::node::handlers::mmp — link liveness, SRTT, ETX +# - fips::ipv6tun::icmp — ICMPv6 Packet Too Big toward +# the host (formerly logged +# under handlers::session) # # Other modules stay at info to keep log volume manageable. The base # docker-compose.yml's per-service RUST_LOG values (currently @@ -33,7 +36,7 @@ # environment variable FIPS_MESH_LAB_TRACE=1 is set. x-trace-rust-log: &trace-rust-log - RUST_LOG: "info,fips::node::handlers::rekey=trace,fips::node::handlers::handshake=trace,fips::node::dataplane::forwarding=trace,fips::node::handlers::session=trace,fips::node::dataplane::encrypted=trace,fips::node::handlers::mmp=trace" + RUST_LOG: "info,fips::node::handlers::rekey=trace,fips::node::handlers::handshake=trace,fips::node::dataplane::forwarding=trace,fips::node::handlers::session=trace,fips::node::dataplane::encrypted=trace,fips::node::handlers::mmp=trace,fips::ipv6tun::icmp=trace" services: # rekey profile