diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index b12e7c2ab6..47c0b51a10 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3,7 +3,7 @@ use std::{ collections::{BTreeMap, VecDeque, btree_map}, convert::TryFrom, fmt, io, mem, - net::SocketAddr, + net::{IpAddr, SocketAddr, SocketAddrV6}, num::{NonZeroU32, NonZeroUsize}, sync::Arc, }; @@ -171,6 +171,15 @@ pub struct Connection { /// deterministically select the next PathId to send on. // TODO(flub): well does it really? But deterministic is nice for now. paths: BTreeMap, + /// Fixed at connection establishment from the initial path's remote family. + /// + /// This does NOT adapt if `Endpoint::rebind()` later changes the underlying socket's + /// address family mid-connection: `noq-proto::Connection` currently has no signal for + /// that. The proto-level [`ConnectionEventInner`] only carries datagrams and issued + /// connection identifiers, while the `noq` wrapper's own `ConnectionEvent::Rebind` + /// never reaches here. Extending that signal is out of scope for this fix and should be + /// flagged for maintainers. + established_ipv6: bool, /// Counter to uniquely identify every [`PathData`] created in this connection. /// /// Each [`PathData`] gets a [`PathData::generation`] that is unique among all @@ -324,6 +333,9 @@ impl Connection { side_args: SideArgs, qlog: QlogSink, ) -> Self { + let established_ipv6 = network_path.remote.is_ipv6(); + let network_path = + Self::normalize_network_path_local_ip_for_family(network_path, established_ipv6); let pref_addr_cid = side_args.pref_addr_cid(); let path_validated = side_args.path_validated(); let connection_side = ConnectionSide::from(side_args); @@ -366,6 +378,7 @@ impl Connection { handshake_cid: local_cid, remote_handshake_cid: remote_cid, local_cid_state, + established_ipv6: false, paths: BTreeMap::from_iter([( PathId::ZERO, PathState { @@ -430,6 +443,7 @@ impl Connection { n0_nat_traversal: Default::default(), qlog, }; + this.set_socket_family(established_ipv6); if path_validated { this.on_path_validated(PathId::ZERO); } @@ -537,6 +551,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result<(PathId, bool), PathError> { + let network_path = self.normalize_network_path(network_path)?; let existing_open_path = self.paths.iter().find(|(id, path)| { network_path.is_probably_same_path(&path.data.network_path) && !self.abandoned_paths.contains(id) @@ -558,6 +573,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result { + let network_path = self.normalize_network_path(network_path)?; let Some(max_path_id) = self.max_path_id() else { return Err(PathError::MultipathNotNegotiated); }; @@ -2220,6 +2236,12 @@ impl Connection { first_decode, remaining, }) => { + // `network_path` comes from the endpoint's UDP receive metadata (`RecvMeta` in the + // `noq` crate), so this is a `Connection` entry boundary for OS-observed local + // addresses. Normalize `local_ip` before it can update `PathData.network_path`; + // `remote` is intentionally preserved because it also drives + // `Transmit::destination`. + let network_path = self.normalize_network_path_local_ip(network_path); let span = trace_span!("pkt", %path_id); let _guard = span.enter(); @@ -2339,7 +2361,7 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && known_path.network_path.local_ip != network_path.local_ip + && network_path.local_ip != known_path.network_path.local_ip && !local_ip_may_migrate { trace!( @@ -3912,6 +3934,7 @@ impl Connection { packet: InitialPacket, remaining: Option, ) -> Result<(), ConnectionError> { + let network_path = self.normalize_network_path_local_ip(network_path); let span = trace_span!("first recv"); let _guard = span.enter(); debug_assert!(self.side.is_server()); @@ -5563,10 +5586,8 @@ impl Connection { && let Some(new_local_ip) = network_path.local_ip { let path_data = self.path_data_mut(path_id); - if path_data - .network_path - .local_ip - .is_some_and(|ip| ip != new_local_ip) + if path_data.network_path.local_ip.is_some() + && path_data.network_path.local_ip != network_path.local_ip { debug!( %path_id, @@ -5582,7 +5603,7 @@ impl Connection { if self.peer_may_migrate() && (migrate_on_any_packet || !is_probing_packet) && is_largest_received_pn - && network_path.remote != self.path_data(path_id).network_path.remote + && self.path_data(path_id).network_path.remote != network_path.remote { self.migrate(path_id, now, network_path, migration_observed_addr); // Break linkability, if possible @@ -7063,14 +7084,101 @@ impl Connection { } } - /// Returns whether this connection has a socket that supports IPv6. + /// Returns whether this connection was established on an IPv6-family socket. + pub fn is_ipv6(&self) -> bool { + self.established_ipv6 + } + + /// Sets the fixed socket family captured when the connection is established. /// - /// TODO(matheus23): This is related to noq endpoint state's `ipv6` bool. We should move that - /// info here instead of trying to hack around not knowing it exactly. - pub(crate) fn is_ipv6(&self) -> bool { - self.paths - .values() - .any(|p| p.data.network_path.remote.is_ipv6()) + /// This has the same rebind limitation documented on [`Self::established_ipv6`]. + fn set_socket_family(&mut self, ipv6: bool) { + self.established_ipv6 = ipv6; + } + + /// Normalizes caller-supplied path addresses at `open_path` entry boundaries. + /// + /// `open_path` and `open_path_ensure` receive caller-supplied `remote` addresses, which may + /// use either plain IPv4 or IPv4-mapped-IPv6 representation. Normalize both `remote` and + /// `local_ip` to the connection's established socket family before storing or comparing them + /// (#738, #784). + fn normalize_network_path(&self, network_path: FourTuple) -> Result { + let ipv6 = self.is_ipv6(); + let remote = Self::normalize_remote_to_socket_family(network_path.remote, ipv6)?; + let local_ip = network_path + .local_ip + .map(|ip| Self::normalize_local_ip(ip, ipv6)); + Ok(FourTuple::new(remote, local_ip)) + } + + fn normalize_remote_to_socket_family( + remote: SocketAddr, + ipv6: bool, + ) -> Result { + if ipv6 { + Ok(SocketAddr::V6(Self::ensure_ipv6(remote))) + } else { + match remote { + SocketAddr::V4(_) => Ok(remote), + SocketAddr::V6(v6) => v6 + .ip() + .to_ipv4_mapped() + .map(|ip| SocketAddr::new(IpAddr::V4(ip), v6.port())) + .ok_or(PathError::InvalidRemoteAddress(remote)), + } + } + } + + fn ensure_ipv6(remote: SocketAddr) -> SocketAddrV6 { + match remote { + SocketAddr::V6(v6) => v6, + SocketAddr::V4(v4) => SocketAddrV6::new(v4.ip().to_ipv6_mapped(), v4.port(), 0, 0), + } + } + + /// Normalizes OS-supplied local IPs at `Connection` entry boundaries. + /// + /// `PathData.network_path.local_ip` is now normalized before it enters + /// connection-owned state, so plain `.local_ip ==` comparisons are safe inside + /// `noq-proto` (#738, #784). This normalization is intentionally based on the + /// connection's established socket family: on a dual-stack socket, a plain IPv4 `local_ip` is + /// mapped to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is folded to plain IPv4. + /// + /// This does change what is handed to the OS. `PathData.network_path.local_ip` becomes + /// `Transmit::src_ip`, and the UDP backend sends different control messages for + /// `IpAddr::V4` and `IpAddr::V6` (`IP_PKTINFO` vs `IPV6_PKTINFO` on Unix). That is the + /// intended behavior for a dual-stack socket: sending IPv4 packet info while the destination + /// sockaddr is IPv6-family is a plausible reason the OS would ignore or mishandle source + /// selection for #738, though this root-cause hypothesis has not been verified on real + /// multi-interface hardware. + /// + /// `remote` is normalized by [`Self::normalize_network_path`] for caller-supplied + /// `open_path`/`open_path_ensure` inputs. It is intentionally preserved here because incoming + /// datagrams come from the OS receive metadata, which is already consistent for a single bound + /// socket, and because `PathData.network_path.remote` drives `Transmit::destination`. + fn normalize_network_path_local_ip(&self, network_path: FourTuple) -> FourTuple { + Self::normalize_network_path_local_ip_for_family(network_path, self.is_ipv6()) + } + + fn normalize_network_path_local_ip_for_family( + network_path: FourTuple, + ipv6: bool, + ) -> FourTuple { + let local_ip = network_path + .local_ip + .map(|ip| Self::normalize_local_ip(ip, ipv6)); + FourTuple::new(network_path.remote, local_ip) + } + + fn normalize_local_ip(ip: IpAddr, ipv6: bool) -> IpAddr { + if ipv6 { + match ip { + IpAddr::V4(v4) => IpAddr::V6(v4.to_ipv6_mapped()), + IpAddr::V6(_) => ip, + } + } else { + ip.to_canonical() + } } /// Add addresses the local endpoint considers are reachable for nat traversal. diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index efe71812f6..0375a504d2 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -894,10 +894,11 @@ impl PathResponses { pub(crate) fn pop_off_path(&mut self, network_path: FourTuple) -> Option<(u64, FourTuple)> { let response = *self.pending.last()?; - // We use an exact comparison here, because once we've received for the first time, - // we really should either already have a local_ip, or we will never get one - // (because our OS doesn't support it). And even if we get it wrong we are only - // slightly less efficient and would not include other on-path data in the packet. + // We use an exact comparison here. By the time connection-owned path state and + // received PATH_CHALLENGE metadata reach this queue, `Connection` has normalized + // `remote` and `local_ip` to the established socket family. Exact matching here remains + // the right on-path/off-path split because different `local_ip`s can be different + // interfaces. if response.network_path == network_path { // We don't bother searching further because we expect that the on-path response will // get drained in the immediate future by a call to `pop_on_path` @@ -1202,4 +1203,19 @@ mod tests { // outside range saturates assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX) } + + /// `PATH_CHALLENGE`s from the same remote must coalesce into one pending response. + #[test] + fn push_coalesces_same_remote() { + let remote = "1.2.3.4:443".parse().unwrap(); + let path = FourTuple::from_remote(remote); + let alternate_local = FourTuple::new(remote, Some("192.0.2.1".parse().unwrap())); + + let mut responses = PathResponses::default(); + responses.push(1, 0xaaaa, path); + responses.push(2, 0xbbbb, alternate_local); + + assert_eq!(responses.pending.len(), 1); + assert_eq!(responses.pending[0].token, 0xbbbb); + } } diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 61f26e7ed9..8b9b399596 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -367,6 +367,11 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// paths exist with the same remote, but different local IP interfaces. /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. +/// +/// `noq-proto::Connection` normalizes caller-supplied `remote` addresses at +/// `open_path`/`open_path_ensure` and `local_ip` at every entry boundary before storing a +/// `FourTuple` in connection-owned state, so plain structural comparisons are safe there (#738, +/// #784). #[derive(Hash, Eq, PartialEq, Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. @@ -464,3 +469,54 @@ impl From for FourTuple { Self::from_remote(value) } } + +#[cfg(test)] +mod four_tuple_tests { + use super::*; + + /// Link-local `FourTuple`s differing only in `scope_id` describe different + /// interfaces and must stay distinct. + #[test] + fn new_preserves_link_local_scope_id() { + let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); + assert_ne!(iface_a, iface_b); + } + + /// For a global IPv6 address, `FourTuple::new()` zeroes `scope_id` on construction. + #[test] + fn new_zeroes_scope_id_for_global_v6() { + let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); + let b = FourTuple::from_remote("[2001:db8::1%5]:443".parse().unwrap()); + assert_eq!(a, b); + } + + /// Multicast `FourTuple`s differing only in `scope_id` describe different interfaces and must + /// stay distinct. + #[test] + fn new_preserves_multicast_scope_id() { + let iface_a = FourTuple::from_remote("[ff02::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[ff02::1%5]:443".parse().unwrap()); + assert_ne!(iface_a, iface_b); + } + + /// Representation differences are not hidden here. `Connection` normalizes path addresses at + /// its entry boundaries before storing them. + #[test] + fn is_probably_same_path_uses_structural_equality() { + let remote = "9.9.9.9:443".parse().unwrap(); + let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); + let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); + assert!(!mapped.is_probably_same_path(&plain)); + assert!(!plain.is_probably_same_path(&mapped)); + } + + /// `is_probably_same_path` still distinguishes scoped link-local remotes. + #[test] + fn is_probably_same_path_distinguishes_link_local_scope_id() { + let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); + assert!(!iface_a.is_probably_same_path(&iface_b)); + assert!(!iface_b.is_probably_same_path(&iface_a)); + } +} diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 2b00fd542c..8415a146e8 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1,6 +1,6 @@ //! Tests for multipath -use std::net::SocketAddr; +use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::num::NonZeroU32; use std::sync::Arc; use std::time::Duration; @@ -384,6 +384,230 @@ fn open_path() -> TestResult { Ok(()) } +/// Confirms the proto-level `Connection::open_path` and `open_path_ensure` APIs +/// normalize `local_ip` before matching or storing paths, even when callers bypass +/// the `noq` wrapper crate. +#[test] +fn open_path_normalizes_local_ip_for_dual_stack_connection() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + let server_addr = pair.routes.public_server_addr(); + let plain_local_ip = Ipv4Addr::new(192, 0, 2, 1); + let mapped_local_ip = IpAddr::V6(plain_local_ip.to_ipv6_mapped()); + + let opened_with_mapped = FourTuple::new(server_addr, Some(mapped_local_ip)); + let path_id = pair.open_path(Client, opened_with_mapped, PathStatus::Available)?; + assert_eq!( + pair.network_path(Client, path_id)?.local_ip(), + Some(mapped_local_ip) + ); + + let requested_with_plain = FourTuple::new(server_addr, Some(IpAddr::V4(plain_local_ip))); + let (same_path_id, existed) = + pair.open_path_ensure(Client, requested_with_plain, PathStatus::Available)?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.local_ip(), + Some(mapped_local_ip) + ); + + Ok(()) +} + +#[test] +fn open_path_normalizes_remote_for_dual_stack_connection() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + assert!(pair.conn(Client).is_ipv6()); + + let remote_ip = Ipv4Addr::new(192, 0, 2, 2); + let plain_remote = SocketAddr::new(remote_ip.into(), 4433); + let mapped_remote = SocketAddr::new(remote_ip.to_ipv6_mapped().into(), 4433); + let path_id = pair.open_path( + Client, + FourTuple::from_remote(plain_remote), + PathStatus::Available, + )?; + + assert_eq!(pair.network_path(Client, path_id)?.remote(), mapped_remote); + + let (same_path_id, existed) = pair.open_path_ensure( + Client, + FourTuple::from_remote(mapped_remote), + PathStatus::Available, + )?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.remote(), + mapped_remote + ); + + Ok(()) +} + +#[test] +fn open_path_normalizes_remote_for_ipv4_connection() -> TestResult { + let _guard = subscribe(); + let transport = Arc::new(TransportConfig { + max_concurrent_multipath_paths: NonZeroU32::new(MAX_PATHS), + ..TransportConfig::default() + }); + let server_cfg = ServerConfig { + transport: transport.clone(), + ..server_config() + }; + let client_cfg = ClientConfig { + transport, + ..client_config() + }; + let server = Endpoint::new(Default::default(), Some(Arc::new(server_cfg)), false); + let client = Endpoint::new(Default::default(), None, false); + let mut pair = Pair::new_from_endpoint(client, server); + pair.routes = + ManyToManyRouting::from_routes([("1.1.1.1:44433", 0)], [("2.2.2.1:4433", 0)]).into(); + let (client_ch, server_ch) = pair.connect_with(client_cfg); + let mut pair = ConnPair::new(pair, client_ch, server_ch); + + assert!(!pair.conn(Client).is_ipv6()); + + let remote_ip = Ipv4Addr::new(192, 0, 2, 2); + let plain_remote = SocketAddr::new(remote_ip.into(), 4433); + let mapped_remote = SocketAddr::new(remote_ip.to_ipv6_mapped().into(), 4433); + let path_id = pair.open_path( + Client, + FourTuple::from_remote(mapped_remote), + PathStatus::Available, + )?; + + assert_eq!(pair.network_path(Client, path_id)?.remote(), plain_remote); + + let (same_path_id, existed) = pair.open_path_ensure( + Client, + FourTuple::from_remote(plain_remote), + PathStatus::Available, + )?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.remote(), + plain_remote + ); + + Ok(()) +} + +#[test] +fn connection_socket_family_is_fixed_at_establishment() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + assert!(pair.conn(Client).is_ipv6()); + + let server_addr_v4 = "192.0.2.2:4433".parse()?; + let client_ip_v4 = Ipv4Addr::new(192, 0, 2, 1); + let path_id = pair.open_path( + Client, + FourTuple::new(server_addr_v4, Some(IpAddr::V4(client_ip_v4))), + PathStatus::Available, + )?; + + assert!(pair.conn(Client).is_ipv6()); + assert_eq!( + pair.network_path(Client, path_id)?.local_ip(), + Some(IpAddr::V6(client_ip_v4.to_ipv6_mapped())) + ); + + Ok(()) +} + +#[test] +fn open_path_with_plain_remote_validates_when_response_uses_mapped_ipv4() -> TestResult { + let _guard = subscribe(); + let client_addr_0 = "[::ffff:1.1.1.0]:44433".parse::()?; + let server_addr_0 = "[::ffff:2.2.2.0]:4433".parse::()?; + let client_addr_1 = "[::ffff:1.1.1.1]:44433".parse::()?; + let server_addr_1_mapped = "[::ffff:2.2.2.1]:4433".parse::()?; + let server_addr_1_plain = "2.2.2.1:4433".parse::()?; + let mut pair = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .with_routes(ManyToManyRouting::from_routes( + [(client_addr_0, 0), (client_addr_1, 2)], + [ + (server_addr_0, 0), + (server_addr_1_plain, 1), + (server_addr_1_mapped, 1), + ], + )) + .connect(); + + let path_id = pair.open_path( + Client, + FourTuple::from_remote(server_addr_1_plain), + PathStatus::Available, + )?; + + assert!( + !pair.drive_bounded(100), + "path validation should become idle" + ); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + + Ok(()) +} + +#[test] +fn open_path_with_plain_local_ip_validates_on_mapped_ipv4_route() -> TestResult { + let _guard = subscribe(); + let client_addr_0 = "[::ffff:1.1.1.0]:44433".parse::()?; + let server_addr_0 = "[::ffff:2.2.2.0]:4433".parse::()?; + let client_addr_1 = "[::ffff:1.1.1.1]:44433".parse::()?; + let server_addr_1 = "[::ffff:2.2.2.1]:4433".parse::()?; + let mut pair = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .with_routes(ManyToManyRouting::from_routes( + [(client_addr_0, 0), (client_addr_1, 1)], + [(server_addr_0, 0), (server_addr_1, 1)], + )) + .connect(); + + let client_ip_plain = client_addr_1.ip().to_canonical(); + let path_id = pair.open_path( + Client, + FourTuple::new(server_addr_1, Some(client_ip_plain)), + PathStatus::Available, + )?; + + pair.drive(); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + + Ok(()) +} + #[test] fn open_path_key_update() -> TestResult { let _guard = subscribe(); diff --git a/noq/src/connection.rs b/noq/src/connection.rs index fd1c909555..9c6765dedf 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -952,33 +952,39 @@ impl Connection { /// Normalizes a [`FourTuple`] against the connection's address family. /// -/// If the connection already uses IPv6 paths, the remote is canonicalised via -/// [`ensure_ipv6`]. If it uses IPv4 and the requested remote is IPv6, this returns -/// [`PathError::InvalidRemoteAddress`]. +/// If the connection already uses IPv6 paths, both `remote` and `local_ip` are +/// canonicalised via [`ensure_ipv6`]/[`std::net::Ipv4Addr::to_ipv6_mapped`]. If it uses IPv4 and +/// the requested remote is IPv6, this returns [`PathError::InvalidRemoteAddress`]. +/// +/// `local_ip` used to be passed through unchanged in the IPv6 branch, while `remote` +/// was canonicalised (#738). This is the one place where addresses supplied by the +/// caller (as opposed to observed from the OS) enter [`Connection`], so keeping +/// `local_ip` in the same representation as `remote` here avoids relying on every +/// downstream comparison to canonicalize it individually. The proto layer now also normalizes +/// caller-supplied `remote` addresses at `open_path`/`open_path_ensure`, closing the same +/// representation gap there (#784). Note: unlike the proto-level storage normalization for #738 +/// (which was verified against the actual reported failure on real Android hardware with physical +/// Wi-Fi/cellular interfaces), this wrapper-level `local_ip` normalization was not independently +/// verified to fix #738's reported symptom on its own -- see the doc comment on the `noq` crate's +/// `open_path_with_explicit_ipv4_local_ip_on_dualstack_socket` test for what was and wasn't +/// reproducible in a loopback-only environment. It is included because it closes a real +/// inconsistency with `remote`'s handling in this same function, independent of whether it's also +/// part of #738's root cause. fn normalize_network_path( network_path: FourTuple, conn: &proto::Connection, ) -> Result { - // If endpoint::State::ipv6 is true we want to keep all our IP addresses as IPv6. - // If not, we do not support IPv6. We can not access endpoint::State from here - // however, but either all our paths use an IPv6 address, or all our paths use an - // IPv4 address. So we can use that information. - let ipv6 = conn - .paths() - .iter() - .filter_map(|id| { - conn.network_path(*id) - .map(|addrs| addrs.remote().is_ipv6()) - .ok() - }) - .next() - .unwrap_or_default(); + let ipv6 = conn.is_ipv6(); let remote = network_path.remote(); if remote.is_ipv6() && !ipv6 { Err(PathError::InvalidRemoteAddress(remote)) } else if ipv6 { let remote = SocketAddr::V6(ensure_ipv6(remote)); - Ok(FourTuple::new(remote, network_path.local_ip())) + let local_ip = network_path.local_ip().map(|ip| match ip { + IpAddr::V4(v4) => IpAddr::V6(v4.to_ipv6_mapped()), + IpAddr::V6(_) => ip, + }); + Ok(FourTuple::new(remote, local_ip)) } else { Ok(network_path) } diff --git a/noq/src/tests.rs b/noq/src/tests.rs index adcd75682e..5ecb8b736a 100755 --- a/noq/src/tests.rs +++ b/noq/src/tests.rs @@ -336,6 +336,25 @@ impl EndpointFactory { endpoint } + + /// Like [`Self::endpoint_with_config`], but for a client-only endpoint bound to + /// `addr` instead of the default IPv4 loopback address. Used by tests that need + /// a specific bind address (e.g. a dual-stack wildcard `[::]:0`) rather than + /// self-connecting IPv4 loopback endpoints. + fn client_endpoint_with_config( + &self, + addr: SocketAddr, + transport_config: TransportConfig, + ) -> Endpoint { + let transport_config = Arc::new(transport_config); + let mut roots = RootCertStore::empty(); + roots.add(self.cert.cert.der().clone()).unwrap(); + let endpoint = Endpoint::client(addr).unwrap(); + let mut client_config = ClientConfig::with_root_certificates(Arc::new(roots)).unwrap(); + client_config.transport_config(transport_config); + endpoint.set_default_client_config(client_config); + endpoint + } } #[tokio::test] @@ -1115,6 +1134,106 @@ async fn test_multipath_observed_address() { tokio::join!(server_task, client_task); } +/// Coverage for `normalize_network_path`'s `local_ip` canonicalization (#738). +/// +/// A client bound to a dual-stack wildcard socket (`[::]:0`) opens an additional +/// path with an explicit *plain* IPv4 `local_ip` -- standing in for an application +/// that enumerated its own network interfaces and passed one of their addresses +/// straight through, the way `open_path`'s docs describe. The path must validate, +/// and the established `Path::local_ip()` must reflect the address that was +/// actually requested (not silently fall back to a different one). +/// +/// Note this is *not* a regression test that fails on unpatched `main`: on this +/// loopback setup the mismatch this fix targets (an application-supplied plain +/// IPv4 `local_ip` vs. the representation [`noq_udp::RecvMeta::dst_ip`] reports for +/// datagrams received on a dual-stack socket, see +/// [`noq_proto::FourTuple`](proto::FourTuple)'s docs) does not by itself block path +/// validation here, for reasons not fully tracked down -- most likely because +/// `early_discard_packet`'s defending comparison only gates already-established +/// paths, not the initial `PATH_CHALLENGE`/`PATH_RESPONSE` handshake. The #738 +/// report's real symptom was only reproduced on real Android hardware with +/// physical Wi-Fi/cellular interfaces; this environment has no equivalent, so +/// this test only guards against future regressions in the normalization itself, +/// not against #738 recurring. +#[tokio::test] +async fn open_path_with_explicit_ipv4_local_ip_on_dualstack_socket() -> TestResult { + let _logging = subscribe(); + let factory = EndpointFactory::new(); + + let mut transport_config = TransportConfig::default(); + transport_config.max_concurrent_multipath_paths(2); + let server = factory.endpoint_with_config("server", transport_config.clone()); + let server_addr = server.local_addr()?; + assert_eq!( + server_addr.ip(), + IpAddr::V4(Ipv4Addr::LOCALHOST), + "test assumes the server is plain IPv4 loopback" + ); + + let server_task = async move { + let conn = server.accept().await.ok_or("closed conn?")?.await?; + conn.closed().await; + TestResult::Ok(()) + } + .instrument(info_span!("server")); + + let client = factory.client_endpoint_with_config( + SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 0), + transport_config, + ); + + let client_task = async move { + let conn = client.connect(server_addr, "localhost")?.await?; + assert!(conn.is_multipath_enabled()); + // small synchronization step necessary to allow the server to set remote CIDs, + // see the same comment in `test_multipath_observed_address` above. + tokio::time::sleep(Duration::from_millis(200)).await; + + // A second, distinct loopback address standing in for a real second network + // interface's address. Deliberately plain IPv4, not pre-mapped -- the same + // representation an app would get from enumerating its own interfaces. + let second_local_ip = IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)); + let open = async { + loop { + match conn + .open_path( + FourTuple::new(server_addr, Some(second_local_ip)), + PathStatus::Available, + ) + .await + { + Ok(path) => break Ok(path), + Err(proto::PathError::RemoteCidsExhausted) => { + tokio::time::sleep(Duration::from_millis(20)).await; + } + Err(err) => break Err(err), + } + } + }; + let path = tokio::time::timeout(Duration::from_secs(10), open) + .await + .map_err(|_| "path with explicit local_ip timed out instead of establishing")??; + // The client is dual-stack, so the established path's remote is normalized + // to IPv4-mapped-IPv6 (see `normalize_network_path`); compare canonically. + let remote = path.remote_address()?; + assert_eq!(remote.ip().to_canonical(), server_addr.ip()); + assert_eq!(remote.port(), server_addr.port()); + assert_eq!( + path.local_ip()?.map(|ip| ip.to_canonical()), + Some(second_local_ip), + "path claims to use a different local_ip than requested -- \ + the source address selection silently fell back instead of erroring" + ); + TestResult::Ok(()) + } + .instrument(info_span!("client")); + + let (server_res, client_res) = tokio::join!(server_task, client_task); + server_res?; + client_res?; + Ok(()) +} + #[tokio::test] async fn on_closed() { let _guard = subscribe();