diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c1351384b..f58b21253 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5085,6 +5085,7 @@ impl Connection { } } Frame::ReachOut(reach_out) => { + let ipv6 = self.is_ipv6(); let server_state = match self.iroh_hp.server_side_mut() { Ok(state) => state, Err(err) => { @@ -5094,7 +5095,7 @@ impl Connection { } }; - if let Err(err) = server_state.handle_reach_out(reach_out) { + if let Err(err) = server_state.handle_reach_out(reach_out, ipv6) { return Err(TransportError::PROTOCOL_VIOLATION(format!( "Nat traversal(REACH_OUT): {err}" ))); @@ -6420,7 +6421,17 @@ impl Connection { } } - /// Add addresses the local endpoint considers are reachable for nat traversal + /// Returns whether this connection has a socket that supports IPv6. + /// + /// TODO(matheus23): This is related to quinn endpoint state's `ipv6` bool. We should move that info + /// here instead of trying to hack around not knowing it exactly. + fn is_ipv6(&self) -> bool { + self.paths + .values() + .any(|p| p.data.network_path.remote.is_ipv6()) + } + + /// Add addresses the local endpoint considers are reachable for nat traversal. pub fn add_nat_traversal_address(&mut self, address: SocketAddr) -> Result<(), iroh_hp::Error> { if let Some(added) = self.iroh_hp.add_local_address(address)? { self.spaces[SpaceId::Data].pending.add_address.insert(added); @@ -6459,30 +6470,17 @@ impl Connection { /// Attempts to open a path for nat traversal. /// - /// `ipv6` indicates if the path should be opened using an IPV6 remote. If the address is - /// ignored, it will return `None`. - /// /// On success returns the [`PathId`] and remote address of the path. fn open_nat_traversal_path( &mut self, now: Instant, - (ip, port): (IpAddr, u16), - ipv6: bool, + ip_port: (IpAddr, u16), ) -> Result, PathError> { - // If this endpoint is an IPv6 endpoint we use IPv6 addresses for all remotes. - let remote = match ip { - IpAddr::V4(addr) if ipv6 => SocketAddr::new(addr.to_ipv6_mapped().into(), port), - IpAddr::V4(addr) => SocketAddr::new(addr.into(), port), - IpAddr::V6(_) if ipv6 => SocketAddr::new(ip, port), - IpAddr::V6(_) => { - trace!("not using IPv6 nat candidate for IPv4 socket"); - return Ok(None); - } - }; + let remote = ip_port.into(); // TODO(matheus23): Probe the correct 4-tuple, instead of only a remote address? - // By specifying None, we do two things: 1. open_path_ensure won't generate two - // paths to the same remote and 2. we let the OS choose which interface to use for - // sending on that path. + // By specifying None for `local_ip`, we do two things: 1. open_path_ensure won't + // generate two paths to the same remote and 2. we let the OS choose which + // interface to use for sending on that path. let network_path = FourTuple { remote, local_ip: None, @@ -6518,13 +6516,14 @@ impl Connection { return Err(iroh_hp::Error::Closed); } + let ipv6 = self.is_ipv6(); let client_state = self.iroh_hp.client_side_mut()?; let iroh_hp::NatTraversalRound { new_round, reach_out_at, addresses_to_probe, prev_round_path_ids, - } = client_state.initiate_nat_traversal_round()?; + } = client_state.initiate_nat_traversal_round(ipv6)?; self.spaces[SpaceId::Data].pending.reach_out = Some((new_round, reach_out_at)); @@ -6540,9 +6539,7 @@ impl Connection { // And we only close paths that we don't want to probe anyways. if !addresses_to_probe .iter() - .any(|(_, (probe_ip, probe_port))| { - *probe_port == port && probe_ip.to_canonical() == ip.to_canonical() - }) + .any(|(_, probe)| *probe == (ip, port)) && !path.validated && !self.abandoned_paths.contains(&path_id) { @@ -6559,13 +6556,9 @@ impl Connection { let mut path_ids = Vec::with_capacity(addresses_to_probe.len()); let mut probed_addresses = Vec::with_capacity(addresses_to_probe.len()); - let ipv6 = self - .paths - .values() - .any(|p| p.data.network_path.remote.is_ipv6()); for (id, address) in addresses_to_probe { - match self.open_nat_traversal_path(now, address, ipv6) { + match self.open_nat_traversal_path(now, address) { Ok(None) => {} Ok(Some((path_id, remote))) => { path_ids.push(path_id); @@ -6601,13 +6594,10 @@ impl Connection { /// If there was nothing to do, it returns `None`. Otherwise it returns whether the path was /// successfully open. fn continue_nat_traversal_round(&mut self, now: Instant) -> Option { + let ipv6 = self.is_ipv6(); let client_state = self.iroh_hp.client_side_mut().ok()?; - let (id, address) = client_state.continue_nat_traversal_round()?; - let ipv6 = self - .paths - .values() - .any(|p| p.data.network_path.remote.is_ipv6()); - let open_result = self.open_nat_traversal_path(now, address, ipv6); + let (id, address) = client_state.continue_nat_traversal_round(ipv6)?; + let open_result = self.open_nat_traversal_path(now, address); let client_state = self.iroh_hp.client_side_mut().expect("validated"); match open_result { Ok(None) => Some(true), diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index f9b051716..109bf7516 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -48,6 +48,8 @@ pub(crate) struct NatTraversalRound { /// /// The addresses include their Id, so that it can be used to signal these should be returned /// in a nat traversal continuation by calling [`ClientState::report_in_continuation`]. + /// + /// These are filtered and mapped to the IP family the local socket supports. pub(crate) addresses_to_probe: Vec<(VarInt, IpPort)>, /// [`PathId`]s of the cancelled round. pub(crate) prev_round_path_ids: Vec, @@ -131,7 +133,15 @@ impl ClientState { /// frames, and initiating probing of the known remote addresses. When a new round is /// initiated, the previous one is cancelled, and paths that have not been opened should be /// closed. - pub(crate) fn initiate_nat_traversal_round(&mut self) -> Result { + /// + /// `ipv6` indicates if the connection runs on a socket that supports IPv6. If so, then all + /// addresses returned in [`NatTraversalRound`] will be IPv6 addresses (and IPv4-mapped IPv6 + /// addresses if necessary). Otherwise they're all IPv4 addresses. + /// See also [`map_to_local_socket_family`]. + pub(crate) fn initiate_nat_traversal_round( + &mut self, + ipv6: bool, + ) -> Result { if self.local_addresses.is_empty() { return Err(Error::NotEnoughAddresses); } @@ -139,9 +149,14 @@ impl ClientState { let prev_round_path_ids = std::mem::take(&mut self.round_path_ids); self.round = self.round.saturating_add(1u8); let mut addresses_to_probe = Vec::with_capacity(self.remote_addresses.len()); - for (id, (address, report_in_continuation)) in self.remote_addresses.iter_mut() { - addresses_to_probe.push((*id, *address)); + for (id, ((ip, port), report_in_continuation)) in self.remote_addresses.iter_mut() { *report_in_continuation = false; + + if let Some(ip) = map_to_local_socket_family(*ip, ipv6) { + addresses_to_probe.push((*id, (ip, *port))); + } else { + trace!(?ip, "not using IPv6 nat candidate for IPv4 socket"); + } } Ok(NatTraversalRound { @@ -172,14 +187,28 @@ impl ClientState { /// /// The address will not be returned twice unless marked as such again with /// [`Self::report_in_continuation`]. - pub(crate) fn continue_nat_traversal_round(&mut self) -> Option<(VarInt, IpPort)> { + /// + /// `ipv6` indicates if the connection runs on a socket that supports IPv6. If so, then all + /// addresses returned in [`NatTraversalRound`] will be IPv6 addresses (and IPv4-mapped IPv6 + /// addresses if necessary). Otherwise they're all IPv4 addresses. + /// See also [`map_to_local_socket_family`]. + pub(crate) fn continue_nat_traversal_round(&mut self, ipv6: bool) -> Option<(VarInt, IpPort)> { // this being random depends on iteration not returning always on the same order let (id, (address, report_in_continuation)) = self .remote_addresses .iter_mut() - .find(|(_id, (_addr, report))| *report)?; + .filter(|(_id, (_addr, report))| *report) + .filter_map(|(id, ((ip, port), report))| { + // only continue with addresses we can send on our local socket + let Some(ip) = map_to_local_socket_family(*ip, ipv6) else { + trace!(?ip, "not using IPv6 nat candidate for IPv4 socket"); + return None; + }; + Some((*id, ((ip, *port), report))) + }) + .next()?; *report_in_continuation = false; - Some((*id, *address)) + Some((id, address)) } /// Add a [`PathId`] as part of the current attempts to create paths based on the server's @@ -314,17 +343,23 @@ impl ServerState { /// Handles a received [`ReachOut`]. /// - /// It returns the token that should be sent in response to this frame as a challenge, and - /// whether this starts a new nat traversal round. - /// - /// If this frame was ignored, it returns `None`. - pub(crate) fn handle_reach_out(&mut self, reach_out: ReachOut) -> Result<(), Error> { + /// This might ignore the reach out frame if it belongs to an older round or if + /// the reach out can't be handled by an ipv4-only local socket. + pub(crate) fn handle_reach_out( + &mut self, + reach_out: ReachOut, + ipv6: bool, + ) -> Result<(), Error> { let ReachOut { round, ip, port } = reach_out; if round < self.round { trace!(current_round=%self.round, "ignoring REACH_OUT for previous round"); return Ok(()); } + 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(()); + }; if round > self.round { self.round = round; @@ -421,15 +456,14 @@ impl State { &mut self, address: SocketAddr, ) -> Result, Error> { + let ip_port = IpPort::from((address.ip(), address.port())); match self { Self::NotNegotiated => Err(Error::ExtensionNotNegotiated), Self::ClientSide(client_state) => { - client_state.add_local_address((address.ip(), address.port()))?; + client_state.add_local_address(ip_port)?; Ok(None) } - Self::ServerSide(server_state) => { - server_state.add_local_address((address.ip(), address.port())) - } + Self::ServerSide(server_state) => server_state.add_local_address(ip_port), } } @@ -443,14 +477,14 @@ impl State { &mut self, address: SocketAddr, ) -> Result, Error> { - let address = &(address.ip(), address.port()); + let address = IpPort::from((address.ip(), address.port())); match self { Self::NotNegotiated => Err(Error::ExtensionNotNegotiated), Self::ClientSide(client_state) => { - client_state.remove_local_address(address); + client_state.remove_local_address(&address); Ok(None) } - Self::ServerSide(server_state) => Ok(server_state.remove_local_address(address)), + Self::ServerSide(server_state) => Ok(server_state.remove_local_address(&address)), } } @@ -473,6 +507,22 @@ impl State { } } +/// Returns the given address as canonicalized IP address. +/// +/// This checks that the address family is supported by our local socket. +/// If it is supported, then the address is mapped to the respective IP address. +/// If the given address is an IPv6 address, but our local socket doesn't support +/// IPv6, then this returns `None`. +pub(crate) fn map_to_local_socket_family(address: IpAddr, ipv6: bool) -> Option { + let ip = match address { + IpAddr::V4(addr) if ipv6 => IpAddr::V6(addr.to_ipv6_mapped()), + IpAddr::V4(_) => address, + IpAddr::V6(_) if ipv6 => address, + IpAddr::V6(addr) => IpAddr::V4(addr.to_ipv4_mapped()?), + }; + Some(ip) +} + #[cfg(test)] mod tests { use super::*; @@ -482,19 +532,25 @@ mod tests { let mut state = ServerState::new(2, 2); state - .handle_reach_out(ReachOut { - round: 1u32.into(), - ip: std::net::Ipv4Addr::LOCALHOST.into(), - port: 1, - }) + .handle_reach_out( + ReachOut { + round: 1u32.into(), + ip: std::net::Ipv4Addr::LOCALHOST.into(), + port: 1, + }, + true, + ) .unwrap(); state - .handle_reach_out(ReachOut { - round: 1u32.into(), - ip: "1.1.1.1".parse().unwrap(), //std::net::Ipv4Addr::LOCALHOST.into(), - port: 2, - }) + .handle_reach_out( + ReachOut { + round: 1u32.into(), + ip: "1.1.1.1".parse().unwrap(), //std::net::Ipv4Addr::LOCALHOST.into(), + port: 2, + }, + true, + ) .unwrap(); dbg!(&state); @@ -510,4 +566,28 @@ mod tests { assert_eq!(state.pending_probes.len(), 0); assert_eq!(state.active_probes.len(), 2); } + + #[test] + fn test_map_to_local_socket() { + assert_eq!( + map_to_local_socket_family("1.1.1.1".parse().unwrap(), false), + Some("1.1.1.1".parse().unwrap()) + ); + assert_eq!( + map_to_local_socket_family("1.1.1.1".parse().unwrap(), true), + Some("::ffff:1.1.1.1".parse().unwrap()) + ); + assert_eq!( + map_to_local_socket_family("::1".parse().unwrap(), true), + Some("::1".parse().unwrap()) + ); + assert_eq!( + map_to_local_socket_family("::1".parse().unwrap(), false), + None + ); + assert_eq!( + map_to_local_socket_family("::ffff:1.1.1.1".parse().unwrap(), false), + Some("1.1.1.1".parse().unwrap()) + ) + } }