gsadmin/noq
1
0
Fork 0
mirror of https://github.com/n0-computer/noq.git synced 2026-09-24 12:13:05 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(proto): Extract address canonicalization and local socket family detection into fns (#346)

Browse Source 
* fix(proto): Don't close paths from previous HP round we're still interested in

* Use local/remote instead of src/dst in 4-tuple logs

* Don't clear all path timers in `close_path`.

* Also check for `!self.abandoned_paths.contains(&path_id)`

* refactor(proto): Canonicalize IP addrs early for HP state

* Spellcheck

* Revert canonicalization to the right places again.

We don't want to canonicalize when adding local addresses, since there might be NAT46 between us and the peer.
We also don't want to canonicalize right when we receive ADD_ADDRESS frames, since that has an effect on maximum number of addresses calculations.

* Preserve address mapping invariant in `iroh_hp::NatTraversalRound`

* Update doc comments

* Map ipv6-mapped IPv4 addresses to IPv4 if possible/necessary

* Don't convert `::1` to `127.0.0.1` in `map_to_local_socket_family`

Also: add a test case

* Prefer `Ipv4Addr::to_ipv4_mapped`
This commit is contained in:
Philipp Krüger
2026-01-26 16:40:10 +01:00
committed by GitHub
parent 2cc515adbc
commit c38be740eb
2 changed files with 133 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files
quinn-proto/src/connection/mod.rs
+25 -35
View File
@@ -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<Option<(PathId, SocketAddr)>, 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<bool> {
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),
quinn-proto/src/iroh_hp.rs
+108 -28
View File
@@ -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<PathId>,
@@ -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<NatTraversalRound, Error> {
///
/// `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<NatTraversalRound, Error> {
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<Option<AddAddress>, 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<Option<RemoveAddress>, 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<IpAddr> {
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())
)
}
}