From 9404ea3d5160242c18dc78157dc2034450b6a955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Thu, 30 Apr 2026 12:11:35 +0200 Subject: [PATCH 1/4] refactor(proto): Introduce `CanonicalIpPort` and fix canonicalization issues --- noq-proto/src/connection/mod.rs | 5 +- noq-proto/src/n0_nat_traversal.rs | 216 ++++++++++++++++++++++-------- 2 files changed, 164 insertions(+), 57 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 208d152c3..2758b289c 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2094,15 +2094,14 @@ impl Connection { builder.finish(self, now); // Mark as sent after packet build succeeds. - self.n0_nat_traversal - .mark_probe_sent((remote.ip(), remote.port()), token); + self.n0_nat_traversal.mark_probe_sent(remote, token); let size = buf.len(); self.path_stats.for_path(path_id).udp_tx.on_sent(1, size); trace!(dst = ?remote, len = buf.len(), "sending off-path NAT probe"); Some(Transmit { - destination: remote, + destination: remote.to_addr(), size, ecn: None, segment_size: None, diff --git a/noq-proto/src/n0_nat_traversal.rs b/noq-proto/src/n0_nat_traversal.rs index d238f8ea6..5cf166fb9 100644 --- a/noq-proto/src/n0_nat_traversal.rs +++ b/noq-proto/src/n0_nat_traversal.rs @@ -2,6 +2,7 @@ use std::{ collections::hash_map::Entry, + fmt::Display, net::{IpAddr, SocketAddr}, }; @@ -16,6 +17,105 @@ use crate::{ type IpPort = (IpAddr, u16); +/// An IP & port in canonical form. +/// +/// Avoids using ipv6-mapped ipv4 addresses. +/// This is the primary type used to send ip addresses around remotely +/// and the primary type used to canonicalize received addresses. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(crate) struct CanonicalIpPort { + canonical_ip: IpAddr, + port: u16, +} + +impl CanonicalIpPort { + pub(crate) fn ip(&self) -> IpAddr { + self.canonical_ip + } + + pub(crate) fn port(&self) -> u16 { + self.port + } + + pub(crate) fn to_local_socket_family(&self, ipv6: bool) -> Option { + SocketIpPort::new(self.canonical_ip, self.port, ipv6) + } + + pub(crate) fn to_addr(&self) -> SocketAddr { + SocketAddr::new(self.canonical_ip, self.port) + } +} + +impl Display for CanonicalIpPort { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.to_addr().fmt(f) + } +} + +impl From for CanonicalIpPort { + fn from(addr: SocketAddr) -> Self { + Self { + canonical_ip: addr.ip().to_canonical(), + port: addr.port(), + } + } +} + +impl From for CanonicalIpPort { + fn from((ip, port): IpPort) -> Self { + Self { + canonical_ip: ip.to_canonical(), + port, + } + } +} + +impl From for CanonicalIpPort { + fn from(addr: SocketIpPort) -> Self { + Self::from((addr.ip(), addr.port())) + } +} + +/// An IP & port in socket-canonical form. +/// +/// In noq, whenever the local socket supports ipv6, all addresses used with +/// the socket are ipv6-mapped ipv4 or ipv6 addresses. +/// +/// When the local socket only supports ipv4, then this will only ever be ipv4 +/// addresses. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(crate) struct SocketIpPort { + ip: IpAddr, + port: u16, +} + +impl SocketIpPort { + pub(crate) fn ip(&self) -> IpAddr { + self.ip + } + + pub(crate) fn port(&self) -> u16 { + self.port + } + + pub(crate) fn new(ip: IpAddr, port: u16, ipv6: bool) -> Option { + Some(Self { + ip: map_to_local_socket_family(ip, ipv6)?, + port, + }) + } + + pub(crate) fn to_addr(&self) -> SocketAddr { + SocketAddr::new(self.ip, self.port) + } +} + +impl Display for SocketIpPort { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.to_addr().fmt(f) + } +} + /// Errors that the nat traversal state might encounter. #[derive(Debug, thiserror::Error)] pub enum Error { @@ -150,14 +250,12 @@ impl State { Self::ClientSide(client_state) => Ok(client_state .local_addresses .iter() - .copied() - .map(Into::into) + .map(CanonicalIpPort::to_addr) .collect()), Self::ServerSide(server_state) => Ok(server_state .local_addresses .keys() - .copied() - .map(Into::into) + .map(CanonicalIpPort::to_addr) .collect()), } } @@ -165,7 +263,7 @@ impl State { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - pub(crate) fn next_probe_addr(&self) -> Option { + pub(crate) fn next_probe_addr(&self) -> Option { match self { Self::NotNegotiated => None, Self::ClientSide(state) => state.next_probe_addr(), @@ -174,7 +272,7 @@ impl State { } /// Marks a probe as sent to the address with the challenge. - pub(crate) fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { + pub(crate) fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { match self { Self::NotNegotiated => (), Self::ClientSide(state) => state.mark_probe_sent(remote, challenge), @@ -226,7 +324,7 @@ pub(crate) struct ClientState { /// They are indexed by their ADD_ADDRESS sequence id and stored in **canonical /// form**. Not in the socket-native form as usual. This because we need to store them /// so we have the correct sequence IDs. - remote_addresses: FxHashMap, + remote_addresses: FxHashMap, /// Candidate addresses for the local endpoint. /// /// These are addresses on which we are potentially reachable, to use for NAT traversal @@ -234,7 +332,7 @@ pub(crate) struct ClientState { /// /// They are stored in **canonical form**, not in socket-native form as usual. We may /// nave a reflexive address that is IPv6 even if our local socket can only handle IPv4. - local_addresses: FxHashSet, + local_addresses: FxHashSet, /// Current nat traversal round. round: VarInt, /// The data of PATH_CHALLENGE frames sent in probes. @@ -243,14 +341,14 @@ pub(crate) struct ClientState { /// have no effect. /// /// They are stored in the usual socket-native form. - sent_challenges: FxHashMap, + sent_challenges: FxHashMap, /// Queued probes to be sent in the next [`poll_transmit`] call. /// /// [`poll_transmit`]: crate::connection::Connection::poll_transmit /// /// They are stored in the usual socket-native form. Probes to address families not /// addressable by the family are never inserted. - pending_probes: FxHashSet, + pending_probes: FxHashSet, /// Network paths that were successfully probed but not yet opened. /// /// When we do not have enough CIDs or free path IDs we might not have been able to open @@ -275,7 +373,7 @@ impl ClientState { } fn add_local_address(&mut self, address: SocketAddr) -> Result<(), Error> { - let address = (address.ip().to_canonical(), address.port()); + let address = CanonicalIpPort::from(address); if self.local_addresses.len() < self.max_local_addresses { self.local_addresses.insert(address); Ok(()) @@ -289,7 +387,7 @@ impl ClientState { } fn remove_local_address(&mut self, address: &IpPort) { - let address = (address.0.to_canonical(), address.1); + let address = CanonicalIpPort::from(*address); self.local_addresses.remove(&address); } @@ -327,30 +425,29 @@ impl ClientState { // Enqueue the NAT probes to known remote addresses. self.remote_addresses .values_mut() - .for_each(|((ip, port), state)| { - if let Some(ip) = map_to_local_socket_family(*ip, ipv6) { - self.pending_probes.insert((ip, *port)); + .for_each(|(ip_port, state)| { + if let Some(ip_port) = ip_port.to_local_socket_family(ipv6) { + self.pending_probes.insert(ip_port); *state = ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS - 1); } else { - trace!(?ip, "not using IPv6 NAT candidate for IPv4 socket"); + trace!(%ip_port, "not using IPv6 NAT candidate for IPv4 socket"); *state = ProbeState::Active(0); } }); let probed_addrs: Vec = self .pending_probes .iter() - .copied() - .map(Into::into) + .map(SocketIpPort::to_addr) .collect(); // Build the REACH_OUT frames. let reach_out_frames: PendingReachOutFrames = self .local_addresses .iter() - .map(|&(ip, port)| ReachOut { + .map(|ip_port| ReachOut { round: self.round, - ip, - port, + ip: ip_port.ip(), + port: ip_port.port(), }) .collect(); @@ -372,13 +469,13 @@ impl ClientState { pub(crate) fn queue_retries(&mut self, ipv6: bool) -> bool { self.remote_addresses .values_mut() - .for_each(|(addr, state)| match state { + .for_each(|(ip_port, state)| match state { ProbeState::Active(remaining) if *remaining > 0 => { *remaining -= 1; - if let Some(ip) = map_to_local_socket_family(addr.0, ipv6) { - self.pending_probes.insert((ip, addr.1)); + if let Some(ip_port) = ip_port.to_local_socket_family(ipv6) { + self.pending_probes.insert(ip_port); } else { - trace!(?addr, "skipping IPv6 NAT candidate for IPv4 socket"); + trace!(%ip_port, "skipping IPv6 NAT candidate for IPv4 socket"); *remaining = 0; } } @@ -390,17 +487,17 @@ impl ClientState { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - fn next_probe_addr(&self) -> Option { - self.pending_probes.iter().next().map(|addr| (*addr).into()) + fn next_probe_addr(&self) -> Option { + self.pending_probes.iter().next().cloned() } /// Marks a probe as sent to the address with the challenge. - fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { + fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { self.pending_probes.remove(&remote); self.sent_challenges.insert(challenge, remote); } - /// Adds an address to the remote set + /// Adds an address to the remote set. /// /// On success returns the address if it was new to the set. It will error when the set /// has no capacity for the address. @@ -416,7 +513,7 @@ impl ClientState { add_addr: AddAddress, ) -> Result, Error> { let AddAddress { seq_no, ip, port } = add_addr; - let address = (ip.to_canonical(), port); + let address = CanonicalIpPort::from((ip, port)); let allow_new = self.remote_addresses.len() < self.max_remote_addresses; match self.remote_addresses.entry(seq_no) { Entry::Occupied(mut occupied_entry) => { @@ -426,11 +523,11 @@ impl ClientState { } // The value might be different. This should not happen, but we assume that the new // address is more recent than the previous, and thus worth updating - Ok(is_update.then_some(address.into())) + Ok(is_update.then_some(address.to_addr())) } Entry::Vacant(vacant_entry) if allow_new => { vacant_entry.insert((address, ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS))); - Ok(Some(address.into())) + Ok(Some(address.to_addr())) } _ => Err(Error::TooManyAddresses), } @@ -445,7 +542,7 @@ impl ClientState { ) -> Option { self.remote_addresses .remove(&remove_addr.seq_no) - .map(|(address, _)| address.into()) + .map(|(address, _)| address.to_addr()) } /// Checks that a received remote address is valid. @@ -454,14 +551,14 @@ impl ClientState { pub(crate) fn check_remote_address(&self, add_addr: &AddAddress) -> bool { match self.remote_addresses.get(&add_addr.seq_no) { None => true, - Some((existing, _)) => existing == &add_addr.ip_port(), + Some((existing, _)) => *existing == CanonicalIpPort::from(add_addr.ip_port()), } } pub(crate) fn get_remote_nat_traversal_addresses(&self) -> Vec { self.remote_addresses .values() - .map(|(address, _)| (*address).into()) + .map(|(address, _)| (*address).to_addr()) .collect() } @@ -471,12 +568,16 @@ impl ClientState { /// [`Self::pop_pending_path_open`] should be called to open the next path. fn handle_path_response(&mut self, network_path: FourTuple, challenge: u64) -> bool { if let Entry::Occupied(entry) = self.sent_challenges.entry(challenge) { - let remote = (network_path.remote().ip(), network_path.remote().port()); + let remote = SocketIpPort { + // TODO(matheus23): Look at this again + ip: network_path.remote().ip(), + port: network_path.remote().port(), + }; if *entry.get() == remote { entry.remove(); // self.remote_addresses is stored in canonical form. - let remote = (remote.0.to_canonical(), remote.1); + let remote = CanonicalIpPort::from(remote); // TODO: linear search is sad. if let Some(seq) = self .remote_addresses @@ -505,7 +606,7 @@ impl ClientState { ?network_path.remote, expected_remote = ?entry.get(), challenge = %display(format_args!("0x{challenge:x}")), - "PATH_RESPONSE matched a NAT traversal probe but mismatching addr XXXX", + "PATH_RESPONSE matched a NAT traversal probe but mismatching addr", ) } } @@ -570,7 +671,7 @@ pub(crate) struct ServerState { /// /// They are stored in **canonical form**, not in socket-native form as usual. We may /// nave a reflexive address that is IPv6 even if our local socket can only handle IPv4. - local_addresses: FxHashMap, + local_addresses: FxHashMap, /// The next id to use for local addresses sent to the client. next_local_addr_id: VarInt, /// Current nat traversal round @@ -583,19 +684,19 @@ pub(crate) struct ServerState { /// The set is cleared when a new round starts. /// /// These are stored in the usual local-socket native form. - remotes: FxHashMap, + remotes: FxHashMap, /// The data of PATH_CHALLENGE frames sent in probes. /// /// These are cleared when a new round starts, so any late-arriving PATH_RESPONSEs will /// have no effect. - sent_challenges: FxHashMap, + sent_challenges: FxHashMap, /// Queued probes to be sent in the next [`poll_transmit`] call. /// /// At the beginning of a round this is populated from REACH_OUT frames and at every /// retry this is populated from [`Self::remotes`]. /// /// [`poll_transmit`]: crate::connection::Connection::poll_transmit - pending_probes: FxHashSet, + pending_probes: FxHashSet, } impl ServerState { @@ -613,7 +714,7 @@ impl ServerState { } fn add_local_address(&mut self, address: SocketAddr) -> Result, Error> { - let address = (address.ip().to_canonical(), address.port()); + let address = CanonicalIpPort::from(address); let allow_new = self.local_addresses.len() < self.max_local_addresses; match self.local_addresses.entry(address) { Entry::Occupied(_) => Ok(None), @@ -621,14 +722,17 @@ impl ServerState { let id = self.next_local_addr_id; self.next_local_addr_id = self.next_local_addr_id.saturating_add(1u8); vacant_entry.insert(id); - Ok(Some(AddAddress::new(address, id))) + Ok(Some(AddAddress::new((address.ip(), address.port()), id))) } _ => Err(Error::TooManyAddresses), } } fn remove_local_address(&mut self, address: &IpPort) -> Option { - self.local_addresses.remove(address).map(RemoveAddress::new) + let address = CanonicalIpPort::from(*address); + self.local_addresses + .remove(&address) + .map(RemoveAddress::new) } /// Returns the current NAT traversal round number. @@ -653,7 +757,7 @@ impl ServerState { trace!(current_round=%self.round, "ignoring REACH_OUT for previous round"); return Ok(()); } - let Some(ip) = map_to_local_socket_family(ip, ipv6) else { + let Some(address) = SocketIpPort::new(ip, port, ipv6) else { trace!("Ignoring IPv6 REACH_OUT frame due to not supporting IPv6 locally"); return Ok(()); }; @@ -663,16 +767,16 @@ impl ServerState { self.remotes.clear(); self.sent_challenges.clear(); self.pending_probes.clear(); - } else if self.remotes.contains_key(&(ip, port)) { + } else if self.remotes.contains_key(&address) { // Retransmitted frame. return Ok(()); } else if self.remotes.len() >= self.max_remote_addresses { return Err(Error::TooManyAddresses); } self.remotes - .entry((ip, port)) + .entry(address) .or_insert(ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS - 1)); - self.pending_probes.insert((ip, port)); + self.pending_probes.insert(address); Ok(()) } @@ -696,12 +800,12 @@ impl ServerState { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - fn next_probe_addr(&self) -> Option { - self.pending_probes.iter().next().map(|addr| (*addr).into()) + fn next_probe_addr(&self) -> Option { + self.pending_probes.iter().next().cloned() } /// Marks a probe as sent to the address with the challenge. - fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { + fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { self.pending_probes.remove(&remote); self.sent_challenges.insert(challenge, remote); } @@ -711,7 +815,11 @@ impl ServerState { /// Returns `true` if it was a response to one of the NAT traversal probes. fn handle_path_response(&mut self, src: FourTuple, challenge: u64) -> bool { if let Entry::Occupied(entry) = self.sent_challenges.entry(challenge) { - let remote = (src.remote().ip(), src.remote().port()); + let remote = SocketIpPort { + // TODO(matheus23): Look at this again + ip: src.remote().ip(), + port: src.remote().port(), + }; if *entry.get() == remote { entry.remove(); self.remotes.insert(remote, ProbeState::Succeeded); @@ -782,7 +890,7 @@ mod tests { let mut send_probe = |state: &mut ServerState| { let remote = state.next_probe_addr().unwrap(); challenge += 1; - state.mark_probe_sent((remote.ip(), remote.port()), challenge); + state.mark_probe_sent(remote, challenge); }; send_probe(&mut state); From 015682af6bdc19a4940d7ce03cd0d35ef0ceaf59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Thu, 30 Apr 2026 12:35:32 +0200 Subject: [PATCH 2/4] Remove `SocketIpPort` again --- noq-proto/src/connection/mod.rs | 2 +- noq-proto/src/n0_nat_traversal.rs | 123 ++++++++++-------------------- 2 files changed, 42 insertions(+), 83 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 2758b289c..b604d5838 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2101,7 +2101,7 @@ impl Connection { trace!(dst = ?remote, len = buf.len(), "sending off-path NAT probe"); Some(Transmit { - destination: remote.to_addr(), + destination: remote.into(), size, ecn: None, segment_size: None, diff --git a/noq-proto/src/n0_nat_traversal.rs b/noq-proto/src/n0_nat_traversal.rs index 5cf166fb9..583a70ecf 100644 --- a/noq-proto/src/n0_nat_traversal.rs +++ b/noq-proto/src/n0_nat_traversal.rs @@ -15,6 +15,15 @@ use crate::{ frame::{AddAddress, ReachOut, RemoveAddress}, }; +/// An IP & port. +/// +/// Invariant: This value should always be in the ip family that the local +/// socket operates in. +/// E.g. if the local socket is ipv4, then all `IpPort`s should only have +/// IPv4 addresses, and if the socket supports ipv6, then all `IpPort`s +/// should be IPv6 addresses or IPv6-mapped IPv4 addresses. +/// +/// See also [`map_to_local_socket_family`], which powers this conversion. type IpPort = (IpAddr, u16); /// An IP & port in canonical form. @@ -37,18 +46,21 @@ impl CanonicalIpPort { self.port } - pub(crate) fn to_local_socket_family(&self, ipv6: bool) -> Option { - SocketIpPort::new(self.canonical_ip, self.port, ipv6) + pub(crate) fn to_local_socket_family(&self, ipv6: bool) -> Option { + Some(( + map_to_local_socket_family(self.canonical_ip, ipv6)?, + self.port, + )) } - pub(crate) fn to_addr(&self) -> SocketAddr { + pub(crate) fn as_canonical_addr(&self) -> SocketAddr { SocketAddr::new(self.canonical_ip, self.port) } } impl Display for CanonicalIpPort { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.to_addr().fmt(f) + self.as_canonical_addr().fmt(f) } } @@ -70,52 +82,6 @@ impl From for CanonicalIpPort { } } -impl From for CanonicalIpPort { - fn from(addr: SocketIpPort) -> Self { - Self::from((addr.ip(), addr.port())) - } -} - -/// An IP & port in socket-canonical form. -/// -/// In noq, whenever the local socket supports ipv6, all addresses used with -/// the socket are ipv6-mapped ipv4 or ipv6 addresses. -/// -/// When the local socket only supports ipv4, then this will only ever be ipv4 -/// addresses. -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub(crate) struct SocketIpPort { - ip: IpAddr, - port: u16, -} - -impl SocketIpPort { - pub(crate) fn ip(&self) -> IpAddr { - self.ip - } - - pub(crate) fn port(&self) -> u16 { - self.port - } - - pub(crate) fn new(ip: IpAddr, port: u16, ipv6: bool) -> Option { - Some(Self { - ip: map_to_local_socket_family(ip, ipv6)?, - port, - }) - } - - pub(crate) fn to_addr(&self) -> SocketAddr { - SocketAddr::new(self.ip, self.port) - } -} - -impl Display for SocketIpPort { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.to_addr().fmt(f) - } -} - /// Errors that the nat traversal state might encounter. #[derive(Debug, thiserror::Error)] pub enum Error { @@ -250,12 +216,12 @@ impl State { Self::ClientSide(client_state) => Ok(client_state .local_addresses .iter() - .map(CanonicalIpPort::to_addr) + .map(CanonicalIpPort::as_canonical_addr) .collect()), Self::ServerSide(server_state) => Ok(server_state .local_addresses .keys() - .map(CanonicalIpPort::to_addr) + .map(CanonicalIpPort::as_canonical_addr) .collect()), } } @@ -263,7 +229,7 @@ impl State { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - pub(crate) fn next_probe_addr(&self) -> Option { + pub(crate) fn next_probe_addr(&self) -> Option { match self { Self::NotNegotiated => None, Self::ClientSide(state) => state.next_probe_addr(), @@ -272,7 +238,7 @@ impl State { } /// Marks a probe as sent to the address with the challenge. - pub(crate) fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { + pub(crate) fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { match self { Self::NotNegotiated => (), Self::ClientSide(state) => state.mark_probe_sent(remote, challenge), @@ -341,14 +307,14 @@ pub(crate) struct ClientState { /// have no effect. /// /// They are stored in the usual socket-native form. - sent_challenges: FxHashMap, + sent_challenges: FxHashMap, /// Queued probes to be sent in the next [`poll_transmit`] call. /// /// [`poll_transmit`]: crate::connection::Connection::poll_transmit /// /// They are stored in the usual socket-native form. Probes to address families not /// addressable by the family are never inserted. - pending_probes: FxHashSet, + pending_probes: FxHashSet, /// Network paths that were successfully probed but not yet opened. /// /// When we do not have enough CIDs or free path IDs we might not have been able to open @@ -437,7 +403,8 @@ impl ClientState { let probed_addrs: Vec = self .pending_probes .iter() - .map(SocketIpPort::to_addr) + .cloned() + .map(Into::into) .collect(); // Build the REACH_OUT frames. @@ -487,12 +454,12 @@ impl ClientState { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - fn next_probe_addr(&self) -> Option { + fn next_probe_addr(&self) -> Option { self.pending_probes.iter().next().cloned() } /// Marks a probe as sent to the address with the challenge. - fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { + fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { self.pending_probes.remove(&remote); self.sent_challenges.insert(challenge, remote); } @@ -523,11 +490,11 @@ impl ClientState { } // The value might be different. This should not happen, but we assume that the new // address is more recent than the previous, and thus worth updating - Ok(is_update.then_some(address.to_addr())) + Ok(is_update.then_some(address.as_canonical_addr())) } Entry::Vacant(vacant_entry) if allow_new => { vacant_entry.insert((address, ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS))); - Ok(Some(address.to_addr())) + Ok(Some(address.as_canonical_addr())) } _ => Err(Error::TooManyAddresses), } @@ -542,7 +509,7 @@ impl ClientState { ) -> Option { self.remote_addresses .remove(&remove_addr.seq_no) - .map(|(address, _)| address.to_addr()) + .map(|(address, _)| address.as_canonical_addr()) } /// Checks that a received remote address is valid. @@ -558,7 +525,7 @@ impl ClientState { pub(crate) fn get_remote_nat_traversal_addresses(&self) -> Vec { self.remote_addresses .values() - .map(|(address, _)| (*address).to_addr()) + .map(|(address, _)| (*address).as_canonical_addr()) .collect() } @@ -568,11 +535,7 @@ impl ClientState { /// [`Self::pop_pending_path_open`] should be called to open the next path. fn handle_path_response(&mut self, network_path: FourTuple, challenge: u64) -> bool { if let Entry::Occupied(entry) = self.sent_challenges.entry(challenge) { - let remote = SocketIpPort { - // TODO(matheus23): Look at this again - ip: network_path.remote().ip(), - port: network_path.remote().port(), - }; + let remote = (network_path.remote().ip(), network_path.remote().port()); if *entry.get() == remote { entry.remove(); @@ -684,19 +647,19 @@ pub(crate) struct ServerState { /// The set is cleared when a new round starts. /// /// These are stored in the usual local-socket native form. - remotes: FxHashMap, + remotes: FxHashMap, /// The data of PATH_CHALLENGE frames sent in probes. /// /// These are cleared when a new round starts, so any late-arriving PATH_RESPONSEs will /// have no effect. - sent_challenges: FxHashMap, + sent_challenges: FxHashMap, /// Queued probes to be sent in the next [`poll_transmit`] call. /// /// At the beginning of a round this is populated from REACH_OUT frames and at every /// retry this is populated from [`Self::remotes`]. /// /// [`poll_transmit`]: crate::connection::Connection::poll_transmit - pending_probes: FxHashSet, + pending_probes: FxHashSet, } impl ServerState { @@ -757,7 +720,7 @@ impl ServerState { trace!(current_round=%self.round, "ignoring REACH_OUT for previous round"); return Ok(()); } - let Some(address) = SocketIpPort::new(ip, port, ipv6) else { + let Some(ip) = map_to_local_socket_family(ip, ipv6) else { trace!("Ignoring IPv6 REACH_OUT frame due to not supporting IPv6 locally"); return Ok(()); }; @@ -767,16 +730,16 @@ impl ServerState { self.remotes.clear(); self.sent_challenges.clear(); self.pending_probes.clear(); - } else if self.remotes.contains_key(&address) { + } else if self.remotes.contains_key(&(ip, port)) { // Retransmitted frame. return Ok(()); } else if self.remotes.len() >= self.max_remote_addresses { return Err(Error::TooManyAddresses); } self.remotes - .entry(address) + .entry((ip, port)) .or_insert(ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS - 1)); - self.pending_probes.insert(address); + self.pending_probes.insert((ip, port)); Ok(()) } @@ -800,12 +763,12 @@ impl ServerState { /// Returns the next ready probe's address. /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. - fn next_probe_addr(&self) -> Option { + fn next_probe_addr(&self) -> Option { self.pending_probes.iter().next().cloned() } /// Marks a probe as sent to the address with the challenge. - fn mark_probe_sent(&mut self, remote: SocketIpPort, challenge: u64) { + fn mark_probe_sent(&mut self, remote: IpPort, challenge: u64) { self.pending_probes.remove(&remote); self.sent_challenges.insert(challenge, remote); } @@ -815,11 +778,7 @@ impl ServerState { /// Returns `true` if it was a response to one of the NAT traversal probes. fn handle_path_response(&mut self, src: FourTuple, challenge: u64) -> bool { if let Entry::Occupied(entry) = self.sent_challenges.entry(challenge) { - let remote = SocketIpPort { - // TODO(matheus23): Look at this again - ip: src.remote().ip(), - port: src.remote().port(), - }; + let remote = (src.remote().ip(), src.remote().port()); if *entry.get() == remote { entry.remove(); self.remotes.insert(remote, ProbeState::Succeeded); From 8eb1bb1889d01f2f46cd876e1d1387facb560fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Thu, 30 Apr 2026 12:52:16 +0200 Subject: [PATCH 3/4] Add documentation, reduce diff --- noq-proto/src/n0_nat_traversal.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/noq-proto/src/n0_nat_traversal.rs b/noq-proto/src/n0_nat_traversal.rs index 583a70ecf..3500ddb09 100644 --- a/noq-proto/src/n0_nat_traversal.rs +++ b/noq-proto/src/n0_nat_traversal.rs @@ -46,6 +46,16 @@ impl CanonicalIpPort { self.port } + /// Converts this into a local-socket-family-mapped IP & port. + /// + /// Instead of using ipv4 and ipv6 addresses, this tries to match `ipv6`, which + /// should indicate whether the local socket supports ipv6 or not. + /// + /// If ipv6 is supported, all ipv4 addresses are mapped using ipv6-mapped ipv4 + /// addresses. + /// If ipv6 is not supported, then this returns `None` for ipv6 addresses. + /// + /// See also [`map_to_local_socket_family`]. pub(crate) fn to_local_socket_family(&self, ipv6: bool) -> Option { Some(( map_to_local_socket_family(self.canonical_ip, ipv6)?, @@ -53,6 +63,7 @@ impl CanonicalIpPort { )) } + /// Returns this address as-is with the canonical IP used in a `SocketAddr`. pub(crate) fn as_canonical_addr(&self) -> SocketAddr { SocketAddr::new(self.canonical_ip, self.port) } @@ -403,7 +414,7 @@ impl ClientState { let probed_addrs: Vec = self .pending_probes .iter() - .cloned() + .copied() .map(Into::into) .collect(); @@ -455,7 +466,7 @@ impl ClientState { /// /// If this is actually sent you must call [`Self::mark_probe_sent`]. fn next_probe_addr(&self) -> Option { - self.pending_probes.iter().next().cloned() + self.pending_probes.iter().next().copied() } /// Marks a probe as sent to the address with the challenge. From f001f5ed8902b4d233a639b72189e373f71f0e76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Thu, 30 Apr 2026 14:37:33 +0200 Subject: [PATCH 4/4] Make clippy happy --- noq-proto/src/n0_nat_traversal.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/noq-proto/src/n0_nat_traversal.rs b/noq-proto/src/n0_nat_traversal.rs index 3500ddb09..bc55cc6eb 100644 --- a/noq-proto/src/n0_nat_traversal.rs +++ b/noq-proto/src/n0_nat_traversal.rs @@ -56,7 +56,7 @@ impl CanonicalIpPort { /// If ipv6 is not supported, then this returns `None` for ipv6 addresses. /// /// See also [`map_to_local_socket_family`]. - pub(crate) fn to_local_socket_family(&self, ipv6: bool) -> Option { + pub(crate) fn as_local_socket_family(&self, ipv6: bool) -> Option { Some(( map_to_local_socket_family(self.canonical_ip, ipv6)?, self.port, @@ -403,7 +403,7 @@ impl ClientState { self.remote_addresses .values_mut() .for_each(|(ip_port, state)| { - if let Some(ip_port) = ip_port.to_local_socket_family(ipv6) { + if let Some(ip_port) = ip_port.as_local_socket_family(ipv6) { self.pending_probes.insert(ip_port); *state = ProbeState::Active(MAX_NAT_PROBE_ATTEMPTS - 1); } else { @@ -450,7 +450,7 @@ impl ClientState { .for_each(|(ip_port, state)| match state { ProbeState::Active(remaining) if *remaining > 0 => { *remaining -= 1; - if let Some(ip_port) = ip_port.to_local_socket_family(ipv6) { + if let Some(ip_port) = ip_port.as_local_socket_family(ipv6) { self.pending_probes.insert(ip_port); } else { trace!(%ip_port, "skipping IPv6 NAT candidate for IPv4 socket");