From 9043e14aea5de75bf6b9c3c118e2d3d4182063ab Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Mon, 1 Sep 2025 00:47:24 +0200 Subject: [PATCH 01/12] feat(net_report): extend probes for nat detection --- iroh-relay/src/quic.rs | 128 ++++++++++-- iroh-relay/src/server.rs | 1 + iroh/src/net_report.rs | 153 ++++++++++++++- iroh/src/net_report/probes.rs | 6 + iroh/src/net_report/report.rs | 324 ++++++++++++++++++++++++++++++- iroh/src/net_report/reportgen.rs | 16 +- 6 files changed, 605 insertions(+), 23 deletions(-) diff --git a/iroh-relay/src/quic.rs b/iroh-relay/src/quic.rs index b634802dda0..8f3b7783d2d 100644 --- a/iroh-relay/src/quic.rs +++ b/iroh-relay/src/quic.rs @@ -103,7 +103,7 @@ pub(crate) mod server { pub(crate) fn spawn(mut quic_config: QuicConfig) -> Result { quic_config.server_config.alpn_protocols = vec![crate::quic::ALPN_QUIC_ADDR_DISC.to_vec()]; - let server_config = QuicServerConfig::try_from(quic_config.server_config)?; + let server_config = QuicServerConfig::try_from(quic_config.server_config.clone())?; let mut server_config = quinn::ServerConfig::with_crypto(Arc::new(server_config)); let transport_config = Arc::get_mut(&mut server_config.transport).expect("not used yet"); @@ -113,19 +113,57 @@ pub(crate) mod server { // enable sending quic address discovery frames .send_observed_address_reports(true); - let endpoint = quinn::Endpoint::server(server_config, quic_config.bind_addr) + // Spawn main QUIC server + let endpoint = quinn::Endpoint::server(server_config.clone(), quic_config.bind_addr) .context(EndpointServerSnafu)?; let bind_addr = endpoint.local_addr().context(LocalAddrSnafu)?; + // Spawn port variation server (port + 1) for NAT testing + let port_variation_addr = match quic_config.bind_addr { + SocketAddr::V4(addr) => Some(SocketAddr::V4(std::net::SocketAddrV4::new( + *addr.ip(), + addr.port() + 1, + ))), + SocketAddr::V6(addr) => Some(SocketAddr::V6(std::net::SocketAddrV6::new( + *addr.ip(), + addr.port() + 1, + 0, + 0, + ))), + }; + + let port_variation_endpoint = if let Some(port_variation_addr) = port_variation_addr { + match quinn::Endpoint::server(server_config.clone(), port_variation_addr) { + Ok(endpoint) => { + info!( + ?port_variation_addr, + "QUIC port variation server listening on" + ); + Some(endpoint) + } + Err(err) => { + // Log the error but don't fail - port variation is optional + tracing::warn!( + ?port_variation_addr, + ?err, + "Failed to bind QUIC port variation server, continuing without it" + ); + None + } + } + } else { + None + }; + info!(?bind_addr, "QUIC server listening on"); let cancel = CancellationToken::new(); let cancel_accept_loop = cancel.clone(); - let task = tokio::task::spawn( + let main_task = tokio::task::spawn( async move { let mut set = JoinSet::new(); - debug!("waiting for connections..."); + debug!("waiting for connections on main port..."); loop { tokio::select! { biased; @@ -143,13 +181,16 @@ pub(crate) mod server { } res = endpoint.accept() => match res { Some(conn) => { - debug!("accepting connection"); + debug!("accepting connection on main port"); let remote_addr = conn.remote_address(); - set.spawn( - handle_connection(conn).instrument(info_span!("qad-conn", %remote_addr)) - ); } + set.spawn(async move { + if let Err(err) = handle_connection(conn).await { + debug!("connection error: {err:#?}"); + } + }.instrument(info_span!("qad-conn", %remote_addr))); + } None => { - debug!("endpoint closed"); + debug!("main endpoint closed"); break; } } @@ -164,17 +205,82 @@ pub(crate) mod server { // all connections, but await to ensure they are finished. set.abort_all(); while !set.is_empty() { - _ = set.join_next().await; + set.join_next().await; } debug!("quic endpoint has been shutdown."); } .instrument(info_span!("quic-endpoint")), ); + + // Spawn separate task for port variation endpoint if it exists + let port_var_task = if let Some(port_var_endpoint) = port_variation_endpoint { + let port_var_cancel = cancel.child_token(); + Some(tokio::task::spawn( + async move { + let mut set = JoinSet::new(); + debug!("waiting for connections on port variation port..."); + loop { + tokio::select! { + biased; + _ = port_var_cancel.cancelled() => { + break; + } + Some(res) = set.join_next() => { + if let Err(err) = res { + if err.is_panic() { + panic!("port variation task panicked: {err:#?}"); + } else { + debug!("error accepting port variation connection: {err:#?}"); + } + } + } + res = port_var_endpoint.accept() => match res { + Some(conn) => { + debug!("accepting connection on port variation port"); + let remote_addr = conn.remote_address(); + set.spawn(async move { + if let Err(err) = handle_connection(conn).await { + debug!("port variation connection error: {err:#?}"); + } + }.instrument(info_span!("qad-conn-port-var", %remote_addr))); + } + None => { + debug!("port variation endpoint closed"); + break; + } + } + } + } + port_var_endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); + port_var_endpoint.wait_idle().await; + + set.abort_all(); + while !set.is_empty() { + set.join_next().await; + } + + debug!("port variation quic endpoint has been shutdown."); + } + .instrument(info_span!("quic-endpoint-port-var")), + )) + } else { + None + }; + + // Combine both tasks into a single handle + let combined_task = tokio::task::spawn(async move { + if let Some(port_var_task) = port_var_task { + let _ = tokio::try_join!(main_task, port_var_task); + } else { + let _ = main_task.await; + } + }); + Ok(Self { bind_addr, cancel, - handle: AbortOnDropHandle::new(task), + handle: AbortOnDropHandle::new(combined_task), }) } diff --git a/iroh-relay/src/server.rs b/iroh-relay/src/server.rs index e4d0e5c7985..eb9c15a2539 100644 --- a/iroh-relay/src/server.rs +++ b/iroh-relay/src/server.rs @@ -166,6 +166,7 @@ pub struct QuicConfig { /// The socket address on which the QUIC server should bind. /// /// Normally you'd chose port `7842`, see [`crate::defaults::DEFAULT_RELAY_QUIC_PORT`]. + /// A second server will automatically be spawned on port + 1 for NAT port mapping variation testing. pub bind_addr: SocketAddr, /// The TLS server configuration for the QUIC server. /// diff --git a/iroh/src/net_report.rs b/iroh/src/net_report.rs index e3ab4f5bb45..97044ba9a67 100644 --- a/iroh/src/net_report.rs +++ b/iroh/src/net_report.rs @@ -443,15 +443,47 @@ impl Client { let dns_resolver = self.socket_state.dns_resolver.clone(); let quic_client = quic_client.clone(); let relay_url = relay_node.url.clone(); + + // Regular QAD probe v4_buf.spawn( cancel_v4 .child_token() .run_until_cancelled_owned(time::timeout( PROBES_TIMEOUT, - run_probe_v4(ip_mapped_addrs, relay_node, quic_client, dns_resolver), + run_probe_v4( + ip_mapped_addrs.clone(), + relay_node.clone(), + quic_client.clone(), + dns_resolver.clone(), + None, + ), )) .instrument(info_span!("QAD-IPv4", %relay_url)), ); + + // Port variation probe (QUIC port + 1) + if let Ok(relay_addr) = + reportgen::get_relay_addr_ipv4(&dns_resolver, &relay_node).await + { + let port_variation = relay_addr.port() + 1; + v4_buf.spawn( + cancel_v4 + .child_token() + .run_until_cancelled_owned(time::timeout( + PROBES_TIMEOUT, + run_probe_v4( + ip_mapped_addrs, + relay_node, + quic_client, + dns_resolver, + Some(port_variation), + ), + )) + .instrument( + info_span!("QAD-IPv4-PortVar", %relay_url, port = port_variation), + ), + ); + } } if if_state.have_v6 { @@ -461,15 +493,47 @@ impl Client { let dns_resolver = self.socket_state.dns_resolver.clone(); let quic_client = quic_client.clone(); let relay_url = relay_node.url.clone(); + + // Regular QAD probe v6_buf.spawn( cancel_v6 .child_token() .run_until_cancelled_owned(time::timeout( PROBES_TIMEOUT, - run_probe_v6(ip_mapped_addrs, relay_node, quic_client, dns_resolver), + run_probe_v6( + ip_mapped_addrs.clone(), + relay_node.clone(), + quic_client.clone(), + dns_resolver.clone(), + None, + ), )) .instrument(info_span!("QAD-IPv6", %relay_url)), ); + + // Port variation probe (QUIC port + 1) + if let Ok(relay_addr) = + reportgen::get_relay_addr_ipv6(&dns_resolver, &relay_node).await + { + let port_variation = relay_addr.port() + 1; + v6_buf.spawn( + cancel_v6 + .child_token() + .run_until_cancelled_owned(time::timeout( + PROBES_TIMEOUT, + run_probe_v6( + ip_mapped_addrs, + relay_node, + quic_client, + dns_resolver, + Some(port_variation), + ), + )) + .instrument( + info_span!("QAD-IPv6-PortVar", %relay_url, port = port_variation), + ), + ); + } } } @@ -686,12 +750,18 @@ async fn run_probe_v4( relay_node: Arc, quic_client: QuicClient, dns_resolver: DnsResolver, + port_override: Option, ) -> n0_snafu::Result<(QadProbeReport, QadConn)> { use n0_snafu::ResultExt; let relay_addr_orig = reportgen::get_relay_addr_ipv4(&dns_resolver, &relay_node).await?; + let relay_addr_with_port = if let Some(port) = port_override { + std::net::SocketAddrV4::new(*relay_addr_orig.ip(), port) + } else { + relay_addr_orig + }; let relay_addr = - reportgen::maybe_to_mapped_addr(ip_mapped_addrs.as_ref(), relay_addr_orig.into()); + reportgen::maybe_to_mapped_addr(ip_mapped_addrs.as_ref(), relay_addr_with_port.into()); debug!(?relay_addr_orig, ?relay_addr, "relay addr v4"); let host = relay_node.url.host_str().context("missing host url")?; @@ -704,10 +774,20 @@ async fn run_probe_v4( .await .context("receiver dropped")? .expect("known"); + + // Test hairpinning by attempting to connect to our discovered external address. + // This is done once per probe (not in the continuous observer loop) since hairpinning + // behavior is a static NAT/firewall property that doesn't change during a session. + // Testing once is sufficient to determine if the NAT supports hairpinning. + let external_addr = SocketAddr::new(addr.ip().to_canonical(), addr.port()); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + let report = QadProbeReport { node: relay_node.url.clone(), - addr: SocketAddr::new(addr.ip().to_canonical(), addr.port()), + addr: external_addr, latency: conn.rtt(), + dest_port: relay_addr.port(), + hairpinning_works: Some(hairpinning_result), }; let observer = Watchable::new(None); @@ -728,6 +808,8 @@ async fn run_probe_v4( node: node.clone(), addr, latency, + dest_port: relay_addr.port(), + hairpinning_works: None, // TODO: Implement hairpinning test })) .ok(); if receiver.changed().await.is_err() { @@ -754,11 +836,17 @@ async fn run_probe_v6( relay_node: Arc, quic_client: QuicClient, dns_resolver: DnsResolver, + port_override: Option, ) -> n0_snafu::Result<(QadProbeReport, QadConn)> { use n0_snafu::ResultExt; let relay_addr_orig = reportgen::get_relay_addr_ipv6(&dns_resolver, &relay_node).await?; + let relay_addr_with_port = if let Some(port) = port_override { + std::net::SocketAddrV6::new(*relay_addr_orig.ip(), port, 0, 0) + } else { + relay_addr_orig + }; let relay_addr = - reportgen::maybe_to_mapped_addr(ip_mapped_addrs.as_ref(), relay_addr_orig.into()); + reportgen::maybe_to_mapped_addr(ip_mapped_addrs.as_ref(), relay_addr_with_port.into()); debug!(?relay_addr_orig, ?relay_addr, "relay addr v6"); let host = relay_node.url.host_str().context("missing host url")?; @@ -771,10 +859,20 @@ async fn run_probe_v6( .await .context("receiver dropped")? .expect("known"); + + // Test hairpinning by attempting to connect to our discovered external address. + // This is done once per probe (not in the continuous observer loop) since hairpinning + // behavior is a static NAT/firewall property that doesn't change during a session. + // Testing once is sufficient to determine if the NAT supports hairpinning. + let external_addr = SocketAddr::new(addr.ip().to_canonical(), addr.port()); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + let report = QadProbeReport { node: relay_node.url.clone(), - addr: SocketAddr::new(addr.ip().to_canonical(), addr.port()), + addr: external_addr, latency: conn.rtt(), + dest_port: relay_addr.port(), + hairpinning_works: Some(hairpinning_result), }; let observer = Watchable::new(None); @@ -795,6 +893,8 @@ async fn run_probe_v6( node: node.clone(), addr, latency, + dest_port: relay_addr.port(), + hairpinning_works: None, // TODO: Implement hairpinning test })) .ok(); if receiver.changed().await.is_err() { @@ -815,6 +915,47 @@ async fn run_probe_v6( )) } +#[cfg(not(wasm_browser))] +async fn test_hairpinning( + external_addr: SocketAddr, + quic_client: &QuicClient, + relay_node: &RelayNode, +) -> bool { + let host = match relay_node.url.host_str() { + Some(host) => host, + None => { + debug!("No host available for hairpinning test"); + return false; + } + }; + + debug!(?external_addr, "attempting hairpinning test"); + + match time::timeout( + Duration::from_millis(2000), // Short timeout for hairpinning test + quic_client.create_conn(external_addr, host), + ) + .await + { + Ok(Ok(_conn)) => { + debug!(?external_addr, "hairpinning test succeeded"); + true + } + Ok(Err(err)) => { + debug!( + ?external_addr, + ?err, + "hairpinning test failed - connection error" + ); + false + } + Err(_) => { + debug!(?external_addr, "hairpinning test failed - timeout"); + false + } + } +} + #[cfg(test)] mod test_utils { //! Creates a relay server against which to perform tests diff --git a/iroh/src/net_report/probes.rs b/iroh/src/net_report/probes.rs index f25f7053a31..e1d505ea7b7 100644 --- a/iroh/src/net_report/probes.rs +++ b/iroh/src/net_report/probes.rs @@ -30,6 +30,12 @@ pub enum Probe { /// QUIC Address Discovery Ipv6 #[cfg(not(wasm_browser))] QadIpv6, + /// QUIC Address Discovery IPv4 with port mapping variation test (QUIC port + 1) + #[cfg(not(wasm_browser))] + QadIpv4PortVariation, + /// QUIC Address Discovery IPv6 with port mapping variation test (QUIC port + 1) + #[cfg(not(wasm_browser))] + QadIpv6PortVariation, } /// A probe set is a sequence of similar [`Probe`]s with delays between them. diff --git a/iroh/src/net_report/report.rs b/iroh/src/net_report/report.rs index 19d3b22d5c8..e363d886c61 100644 --- a/iroh/src/net_report/report.rs +++ b/iroh/src/net_report/report.rs @@ -21,6 +21,14 @@ pub struct Report { pub mapping_varies_by_dest_ipv4: Option, /// Whether the reported public address differs when probing different servers (on IPv6). pub mapping_varies_by_dest_ipv6: Option, + /// Whether the reported public port differs when probing different destination ports (on IPv4). + pub mapping_varies_by_dest_port_ipv4: Option, + /// Whether the reported public port differs when probing different destination ports (on IPv6). + pub mapping_varies_by_dest_port_ipv6: Option, + /// Whether hairpinning (sending to own external address) works for IPv4. + pub hairpinning_ipv4: Option, + /// Whether hairpinning (sending to own external address) works for IPv6. + pub hairpinning_ipv6: Option, /// Probe indicating the presence of port mapping protocols on the LAN. /// `None` for unknown pub preferred_relay: Option, @@ -33,6 +41,12 @@ pub struct Report { /// CaptivePortal is set when we think there's a captive portal that is /// intercepting HTTP traffic. pub captive_portal: Option, + + // Internal tracking for port mapping variation detection + /// Tracks IPv4 public ports seen from different destination ports (dest_port -> public_port) + ipv4_port_mappings: BTreeMap, + /// Tracks IPv6 public ports seen from different destination ports (dest_port -> public_port) + ipv6_port_mappings: BTreeMap, } impl fmt::Display for Report { @@ -60,6 +74,29 @@ impl Report { } } + /// Whether the reported public port differs when probing different destination ports. + pub fn mapping_varies_by_dest_port(&self) -> Option { + match ( + self.mapping_varies_by_dest_port_ipv4, + self.mapping_varies_by_dest_port_ipv6, + ) { + (Some(v4), Some(v6)) => Some(v4 || v6), + (None, Some(v6)) => Some(v6), + (Some(v4), None) => Some(v4), + (None, None) => None, + } + } + + /// Whether hairpinning (sending to own external address) works. + pub fn hairpinning(&self) -> Option { + match (self.hairpinning_ipv4, self.hairpinning_ipv6) { + (Some(v4), Some(v6)) => Some(v4 || v6), + (None, Some(v6)) => Some(v6), + (Some(v4), None) => Some(v4), + (None, None) => None, + } + } + /// Updates a net_report [`Report`] with a new [`ProbeReport`]. pub(super) fn update(&mut self, report: &ProbeReport) { match report { @@ -94,6 +131,43 @@ impl Report { } else { self.global_v4 = Some(ipp); } + + let public_port = ipp.port(); + let dest_port = report.dest_port; + + if let Some(&existing_public_port) = self.ipv4_port_mappings.get(&dest_port) { + if existing_public_port != public_port { + self.mapping_varies_by_dest_port_ipv4 = Some(true); + warn!( + "IPv4 public port varies by destination port: dest {} -> public {} (was {})", + dest_port, public_port, existing_public_port + ); + } else if self.mapping_varies_by_dest_port_ipv4.is_none() { + let has_different_mappings = self + .ipv4_port_mappings + .values() + .any(|&port| port != public_port); + self.mapping_varies_by_dest_port_ipv4 = Some(has_different_mappings); + } + } else { + self.ipv4_port_mappings.insert(dest_port, public_port); + + if self.ipv4_port_mappings.len() > 1 { + let has_different_mappings = self + .ipv4_port_mappings + .values() + .any(|&port| port != public_port); + if has_different_mappings { + self.mapping_varies_by_dest_port_ipv4 = Some(true); + } else if self.mapping_varies_by_dest_port_ipv4.is_none() { + self.mapping_varies_by_dest_port_ipv4 = Some(false); + } + } + } + + if let Some(hairpinning_result) = report.hairpinning_works { + self.hairpinning_ipv4 = Some(hairpinning_result); + } } #[cfg(not(wasm_browser))] ProbeReport::QadIpv6(report) => { @@ -121,6 +195,43 @@ impl Report { } else { self.global_v6 = Some(ipp); } + + let public_port = ipp.port(); + let dest_port = report.dest_port; + + if let Some(&existing_public_port) = self.ipv6_port_mappings.get(&dest_port) { + if existing_public_port != public_port { + self.mapping_varies_by_dest_port_ipv6 = Some(true); + warn!( + "IPv6 public port varies by destination port: dest {} -> public {} (was {})", + dest_port, public_port, existing_public_port + ); + } else if self.mapping_varies_by_dest_port_ipv6.is_none() { + let has_different_mappings = self + .ipv6_port_mappings + .values() + .any(|&port| port != public_port); + self.mapping_varies_by_dest_port_ipv6 = Some(has_different_mappings); + } + } else { + self.ipv6_port_mappings.insert(dest_port, public_port); + + if self.ipv6_port_mappings.len() > 1 { + let has_different_mappings = self + .ipv6_port_mappings + .values() + .any(|&port| port != public_port); + if has_different_mappings { + self.mapping_varies_by_dest_port_ipv6 = Some(true); + } else if self.mapping_varies_by_dest_port_ipv6.is_none() { + self.mapping_varies_by_dest_port_ipv6 = Some(false); + } + } + } + + if let Some(hairpinning_result) = report.hairpinning_works { + self.hairpinning_ipv6 = Some(hairpinning_result); + } } } } @@ -142,9 +253,9 @@ impl RelayLatencies { let list = match probe { Probe::Https => &mut self.https, #[cfg(not(wasm_browser))] - Probe::QadIpv4 => &mut self.ipv4, + Probe::QadIpv4 | Probe::QadIpv4PortVariation => &mut self.ipv4, #[cfg(not(wasm_browser))] - Probe::QadIpv6 => &mut self.ipv6, + Probe::QadIpv6 | Probe::QadIpv6PortVariation => &mut self.ipv6, }; let old_latency = list.entry(url).or_insert(latency); if latency < *old_latency { @@ -212,3 +323,212 @@ impl RelayLatencies { list.into_iter().min() } } + +#[cfg(test)] +mod tests { + use std::{net::SocketAddr, time::Duration}; + + use iroh_base::RelayUrl; + + use super::*; + use crate::net_report::reportgen::{ProbeReport, QadProbeReport}; + + #[test] + fn test_port_mapping_variation_detection_ipv4() { + let mut report = Report::default(); + let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); + + // Test Case 1: Same public port for different destination ports -> false + let probe_port_7842 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(50), + addr: "203.0.113.42:54321".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: None, + }); + + let probe_port_7843 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(52), + addr: "203.0.113.42:54321".parse::().unwrap(), // Same public port + dest_port: 7843, + hairpinning_works: None, + }); + + report.update(&probe_port_7842); + assert_eq!( + report.mapping_varies_by_dest_port_ipv4, None, + "Should be None with only one probe" + ); + + report.update(&probe_port_7843); + assert_eq!( + report.mapping_varies_by_dest_port_ipv4, + Some(false), + "Should be false when public ports are the same" + ); + + // Test Case 2: Different public port for third destination port -> true + let probe_port_7844 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(48), + addr: "203.0.113.42:54322".parse::().unwrap(), // Different public port + dest_port: 7844, + hairpinning_works: None, + }); + + report.update(&probe_port_7844); + assert_eq!( + report.mapping_varies_by_dest_port_ipv4, + Some(true), + "Should be true when public ports vary" + ); + + // Verify the mappings are tracked correctly + assert_eq!(report.ipv4_port_mappings.get(&7842), Some(&54321)); + assert_eq!(report.ipv4_port_mappings.get(&7843), Some(&54321)); + assert_eq!(report.ipv4_port_mappings.get(&7844), Some(&54322)); + } + + #[test] + fn test_port_mapping_variation_detection_ipv6() { + let mut report = Report::default(); + let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); + + // Test IPv6 with port mapping variation + let probe_v6_7842 = ProbeReport::QadIpv6(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(45), + addr: "[2001:db8::1]:54321".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: None, + }); + + let probe_v6_7843 = ProbeReport::QadIpv6(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(47), + addr: "[2001:db8::1]:54399".parse::().unwrap(), // Different public port + dest_port: 7843, + hairpinning_works: None, + }); + + report.update(&probe_v6_7842); + assert_eq!(report.mapping_varies_by_dest_port_ipv6, None); + + report.update(&probe_v6_7843); + assert_eq!( + report.mapping_varies_by_dest_port_ipv6, + Some(true), + "Should detect IPv6 port variation" + ); + + // Verify IPv6 mappings + assert_eq!(report.ipv6_port_mappings.get(&7842), Some(&54321)); + assert_eq!(report.ipv6_port_mappings.get(&7843), Some(&54399)); + } + + #[test] + fn test_port_mapping_same_dest_port_different_public_ports() { + let mut report = Report::default(); + let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); + + // Test same destination port reporting different public ports (unstable NAT) + let probe1 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(50), + addr: "203.0.113.42:54321".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: None, + }); + + let probe2 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(52), + addr: "203.0.113.42:54322".parse::().unwrap(), // Different public port for same dest port + dest_port: 7842, + hairpinning_works: None, + }); + + report.update(&probe1); + assert_eq!(report.mapping_varies_by_dest_port_ipv4, None); + + report.update(&probe2); + assert_eq!( + report.mapping_varies_by_dest_port_ipv4, + Some(true), + "Should detect unstable mapping for same dest port" + ); + } + + #[test] + fn test_hairpinning_detection() { + let mut report = Report::default(); + let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); + + // Test hairpinning works + let probe_hairpin_works = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(50), + addr: "203.0.113.42:54321".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: Some(true), + }); + + report.update(&probe_hairpin_works); + assert_eq!(report.hairpinning_ipv4, Some(true)); + + // Test hairpinning doesn't work (should override previous value) + let probe_hairpin_fails = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(55), + addr: "203.0.113.42:54322".parse::().unwrap(), + dest_port: 7843, + hairpinning_works: Some(false), + }); + + report.update(&probe_hairpin_fails); + assert_eq!( + report.hairpinning_ipv4, + Some(false), + "Should update hairpinning result" + ); + } + + #[test] + fn test_combined_functionality() { + let mut report = Report::default(); + let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); + + // Test that the combined helper functions work correctly + let probe1 = ProbeReport::QadIpv4(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(50), + addr: "203.0.113.42:54321".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: Some(true), + }); + + let probe2 = ProbeReport::QadIpv6(QadProbeReport { + node: relay_url.clone(), + latency: Duration::from_millis(45), + addr: "[2001:db8::1]:54399".parse::().unwrap(), + dest_port: 7842, + hairpinning_works: Some(false), + }); + + report.update(&probe1); + report.update(&probe2); + + // Test the combined helper functions + assert_eq!( + report.mapping_varies_by_dest_port(), + None, + "Should be None when only one dest port tested per IP version" + ); + assert_eq!( + report.hairpinning(), + Some(true), + "Should be true if any IP version supports hairpinning" + ); + } +} diff --git a/iroh/src/net_report/reportgen.rs b/iroh/src/net_report/reportgen.rs index 4c26bbea32d..6a7a26cb5ff 100644 --- a/iroh/src/net_report/reportgen.rs +++ b/iroh/src/net_report/reportgen.rs @@ -434,6 +434,10 @@ pub(super) struct QadProbeReport { pub(super) latency: Duration, /// The discovered public address. pub(super) addr: SocketAddr, + /// The destination port that was used for this probe + pub(super) dest_port: u16, + /// Whether hairpinning test succeeded (if attempted) + pub(super) hairpinning_works: Option, } #[derive(Debug, Clone)] @@ -513,7 +517,10 @@ impl Probe { } } #[cfg(not(wasm_browser))] - Probe::QadIpv4 | Probe::QadIpv6 => unreachable!("must not be used"), + Probe::QadIpv4 + | Probe::QadIpv6 + | Probe::QadIpv4PortVariation + | Probe::QadIpv6PortVariation => unreachable!("must not be used"), } } } @@ -874,9 +881,10 @@ mod tests { let quic_client = iroh_relay::quic::QuicClient::new(ep.clone(), client_config); let dns_resolver = DnsResolver::default(); - let (report, conn) = super::super::run_probe_v4(None, relay, quic_client, dns_resolver) - .await - .unwrap(); + let (report, conn) = + super::super::run_probe_v4(None, relay, quic_client, dns_resolver, None) + .await + .unwrap(); assert_eq!(report.addr, client_addr); drop(conn); From 4231b41295adf62c3b47bc2346ade6d3e96841fb Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Mon, 1 Sep 2025 01:34:56 +0200 Subject: [PATCH 02/12] serde --- iroh/src/net_report/report.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/iroh/src/net_report/report.rs b/iroh/src/net_report/report.rs index e363d886c61..f2b6e2e7475 100644 --- a/iroh/src/net_report/report.rs +++ b/iroh/src/net_report/report.rs @@ -6,12 +6,13 @@ use std::{ }; use iroh_base::RelayUrl; +use serde::{Deserialize, Serialize}; use tracing::warn; use super::{ProbeReport, probes::Probe}; /// A net_report report. -#[derive(Default, Debug, PartialEq, Eq, Clone)] +#[derive(Default, Debug, Serialize, Deserialize, PartialEq, Eq, Clone)] pub struct Report { /// A QAD IPv4 round trip completed. pub udp_v4: bool, From c93a98787daf609ab16020759777080c9fe88bd1 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Mon, 1 Sep 2025 01:39:10 +0200 Subject: [PATCH 03/12] serde --- iroh/src/net_report/report.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iroh/src/net_report/report.rs b/iroh/src/net_report/report.rs index f2b6e2e7475..eb6efb55ffe 100644 --- a/iroh/src/net_report/report.rs +++ b/iroh/src/net_report/report.rs @@ -239,7 +239,7 @@ impl Report { } /// Latencies per relay node. -#[derive(Debug, Default, PartialEq, Eq, Clone)] +#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq, Clone)] pub struct RelayLatencies { #[cfg(not(wasm_browser))] ipv4: BTreeMap, From be38c51a05ba04f34134a46ce5c841c91b10eb13 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Mon, 1 Sep 2025 14:39:50 +0200 Subject: [PATCH 04/12] bump --- iroh/src/net_report/probes.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/iroh/src/net_report/probes.rs b/iroh/src/net_report/probes.rs index e1d505ea7b7..87ab91f3d82 100644 --- a/iroh/src/net_report/probes.rs +++ b/iroh/src/net_report/probes.rs @@ -194,7 +194,13 @@ mod tests { } fn default_protocols() -> BTreeSet { - BTreeSet::from([Probe::QadIpv4, Probe::QadIpv6, Probe::Https]) + BTreeSet::from([ + Probe::QadIpv4, + Probe::QadIpv6, + Probe::QadIpv4PortVariation, + Probe::QadIpv6PortVariation, + Probe::Https + ]) } #[tokio::test] From 2228d1e26d0b812193f7d0c2d940ec1ee7eaa725 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Mon, 1 Sep 2025 22:54:30 +0200 Subject: [PATCH 05/12] maybe fix --- iroh/src/net_report.rs | 46 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/iroh/src/net_report.rs b/iroh/src/net_report.rs index 97044ba9a67..65bdf8d9896 100644 --- a/iroh/src/net_report.rs +++ b/iroh/src/net_report.rs @@ -779,8 +779,27 @@ async fn run_probe_v4( // This is done once per probe (not in the continuous observer loop) since hairpinning // behavior is a static NAT/firewall property that doesn't change during a session. // Testing once is sufficient to determine if the NAT supports hairpinning. - let external_addr = SocketAddr::new(addr.ip().to_canonical(), addr.port()); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + let (external_addr, hairpinning_result) = match addr { + SocketAddr::V4(addr_v4) => { + // Use pure IPv4 address for hairpinning test + let external_addr = SocketAddr::V4(addr_v4); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } + SocketAddr::V6(addr_v6) => { + // For IPv6, check if it's an IPv4-mapped address and extract the IPv4 part + if let Some(ipv4) = addr_v6.ip().to_ipv4_mapped() { + let external_addr = SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } else { + // Pure IPv6 address + let external_addr = SocketAddr::V6(addr_v6); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } + } + }; let report = QadProbeReport { node: relay_node.url.clone(), @@ -864,8 +883,27 @@ async fn run_probe_v6( // This is done once per probe (not in the continuous observer loop) since hairpinning // behavior is a static NAT/firewall property that doesn't change during a session. // Testing once is sufficient to determine if the NAT supports hairpinning. - let external_addr = SocketAddr::new(addr.ip().to_canonical(), addr.port()); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + let (external_addr, hairpinning_result) = match addr { + SocketAddr::V4(addr_v4) => { + // Use pure IPv4 address for hairpinning test + let external_addr = SocketAddr::V4(addr_v4); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } + SocketAddr::V6(addr_v6) => { + // For IPv6, check if it's an IPv4-mapped address and extract the IPv4 part + if let Some(ipv4) = addr_v6.ip().to_ipv4_mapped() { + let external_addr = SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } else { + // Pure IPv6 address + let external_addr = SocketAddr::V6(addr_v6); + let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; + (external_addr, hairpinning_result) + } + } + }; let report = QadProbeReport { node: relay_node.url.clone(), From 50582e49ba3ddb655a456494380021e0c5913b5d Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Tue, 2 Sep 2025 13:31:19 +0200 Subject: [PATCH 06/12] remove hairpinning --- iroh/src/net_report.rs | 87 +++----------------------- iroh/src/net_report/probes.rs | 6 +- iroh/src/net_report/report.rs | 101 ------------------------------- iroh/src/net_report/reportgen.rs | 2 - 4 files changed, 11 insertions(+), 185 deletions(-) diff --git a/iroh/src/net_report.rs b/iroh/src/net_report.rs index 65bdf8d9896..d46e4c59140 100644 --- a/iroh/src/net_report.rs +++ b/iroh/src/net_report.rs @@ -775,28 +775,15 @@ async fn run_probe_v4( .context("receiver dropped")? .expect("known"); - // Test hairpinning by attempting to connect to our discovered external address. - // This is done once per probe (not in the continuous observer loop) since hairpinning - // behavior is a static NAT/firewall property that doesn't change during a session. - // Testing once is sufficient to determine if the NAT supports hairpinning. - let (external_addr, hairpinning_result) = match addr { - SocketAddr::V4(addr_v4) => { - // Use pure IPv4 address for hairpinning test - let external_addr = SocketAddr::V4(addr_v4); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) - } + let external_addr = match addr { + SocketAddr::V4(addr_v4) => SocketAddr::V4(addr_v4), SocketAddr::V6(addr_v6) => { // For IPv6, check if it's an IPv4-mapped address and extract the IPv4 part if let Some(ipv4) = addr_v6.ip().to_ipv4_mapped() { - let external_addr = SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) + SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())) } else { // Pure IPv6 address - let external_addr = SocketAddr::V6(addr_v6); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) + SocketAddr::V6(addr_v6) } } }; @@ -806,7 +793,6 @@ async fn run_probe_v4( addr: external_addr, latency: conn.rtt(), dest_port: relay_addr.port(), - hairpinning_works: Some(hairpinning_result), }; let observer = Watchable::new(None); @@ -828,7 +814,6 @@ async fn run_probe_v4( addr, latency, dest_port: relay_addr.port(), - hairpinning_works: None, // TODO: Implement hairpinning test })) .ok(); if receiver.changed().await.is_err() { @@ -879,28 +864,15 @@ async fn run_probe_v6( .context("receiver dropped")? .expect("known"); - // Test hairpinning by attempting to connect to our discovered external address. - // This is done once per probe (not in the continuous observer loop) since hairpinning - // behavior is a static NAT/firewall property that doesn't change during a session. - // Testing once is sufficient to determine if the NAT supports hairpinning. - let (external_addr, hairpinning_result) = match addr { - SocketAddr::V4(addr_v4) => { - // Use pure IPv4 address for hairpinning test - let external_addr = SocketAddr::V4(addr_v4); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) - } + let external_addr = match addr { + SocketAddr::V4(addr_v4) => SocketAddr::V4(addr_v4), SocketAddr::V6(addr_v6) => { // For IPv6, check if it's an IPv4-mapped address and extract the IPv4 part if let Some(ipv4) = addr_v6.ip().to_ipv4_mapped() { - let external_addr = SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) + SocketAddr::V4(std::net::SocketAddrV4::new(ipv4, addr_v6.port())) } else { // Pure IPv6 address - let external_addr = SocketAddr::V6(addr_v6); - let hairpinning_result = test_hairpinning(external_addr, &quic_client, &relay_node).await; - (external_addr, hairpinning_result) + SocketAddr::V6(addr_v6) } } }; @@ -910,7 +882,6 @@ async fn run_probe_v6( addr: external_addr, latency: conn.rtt(), dest_port: relay_addr.port(), - hairpinning_works: Some(hairpinning_result), }; let observer = Watchable::new(None); @@ -932,7 +903,6 @@ async fn run_probe_v6( addr, latency, dest_port: relay_addr.port(), - hairpinning_works: None, // TODO: Implement hairpinning test })) .ok(); if receiver.changed().await.is_err() { @@ -953,47 +923,6 @@ async fn run_probe_v6( )) } -#[cfg(not(wasm_browser))] -async fn test_hairpinning( - external_addr: SocketAddr, - quic_client: &QuicClient, - relay_node: &RelayNode, -) -> bool { - let host = match relay_node.url.host_str() { - Some(host) => host, - None => { - debug!("No host available for hairpinning test"); - return false; - } - }; - - debug!(?external_addr, "attempting hairpinning test"); - - match time::timeout( - Duration::from_millis(2000), // Short timeout for hairpinning test - quic_client.create_conn(external_addr, host), - ) - .await - { - Ok(Ok(_conn)) => { - debug!(?external_addr, "hairpinning test succeeded"); - true - } - Ok(Err(err)) => { - debug!( - ?external_addr, - ?err, - "hairpinning test failed - connection error" - ); - false - } - Err(_) => { - debug!(?external_addr, "hairpinning test failed - timeout"); - false - } - } -} - #[cfg(test)] mod test_utils { //! Creates a relay server against which to perform tests diff --git a/iroh/src/net_report/probes.rs b/iroh/src/net_report/probes.rs index 87ab91f3d82..00174a2e31b 100644 --- a/iroh/src/net_report/probes.rs +++ b/iroh/src/net_report/probes.rs @@ -195,11 +195,11 @@ mod tests { fn default_protocols() -> BTreeSet { BTreeSet::from([ - Probe::QadIpv4, - Probe::QadIpv6, + Probe::QadIpv4, + Probe::QadIpv6, Probe::QadIpv4PortVariation, Probe::QadIpv6PortVariation, - Probe::Https + Probe::Https, ]) } diff --git a/iroh/src/net_report/report.rs b/iroh/src/net_report/report.rs index eb6efb55ffe..06d6498c733 100644 --- a/iroh/src/net_report/report.rs +++ b/iroh/src/net_report/report.rs @@ -26,10 +26,6 @@ pub struct Report { pub mapping_varies_by_dest_port_ipv4: Option, /// Whether the reported public port differs when probing different destination ports (on IPv6). pub mapping_varies_by_dest_port_ipv6: Option, - /// Whether hairpinning (sending to own external address) works for IPv4. - pub hairpinning_ipv4: Option, - /// Whether hairpinning (sending to own external address) works for IPv6. - pub hairpinning_ipv6: Option, /// Probe indicating the presence of port mapping protocols on the LAN. /// `None` for unknown pub preferred_relay: Option, @@ -88,16 +84,6 @@ impl Report { } } - /// Whether hairpinning (sending to own external address) works. - pub fn hairpinning(&self) -> Option { - match (self.hairpinning_ipv4, self.hairpinning_ipv6) { - (Some(v4), Some(v6)) => Some(v4 || v6), - (None, Some(v6)) => Some(v6), - (Some(v4), None) => Some(v4), - (None, None) => None, - } - } - /// Updates a net_report [`Report`] with a new [`ProbeReport`]. pub(super) fn update(&mut self, report: &ProbeReport) { match report { @@ -165,10 +151,6 @@ impl Report { } } } - - if let Some(hairpinning_result) = report.hairpinning_works { - self.hairpinning_ipv4 = Some(hairpinning_result); - } } #[cfg(not(wasm_browser))] ProbeReport::QadIpv6(report) => { @@ -229,10 +211,6 @@ impl Report { } } } - - if let Some(hairpinning_result) = report.hairpinning_works { - self.hairpinning_ipv6 = Some(hairpinning_result); - } } } } @@ -345,7 +323,6 @@ mod tests { latency: Duration::from_millis(50), addr: "203.0.113.42:54321".parse::().unwrap(), dest_port: 7842, - hairpinning_works: None, }); let probe_port_7843 = ProbeReport::QadIpv4(QadProbeReport { @@ -353,7 +330,6 @@ mod tests { latency: Duration::from_millis(52), addr: "203.0.113.42:54321".parse::().unwrap(), // Same public port dest_port: 7843, - hairpinning_works: None, }); report.update(&probe_port_7842); @@ -375,7 +351,6 @@ mod tests { latency: Duration::from_millis(48), addr: "203.0.113.42:54322".parse::().unwrap(), // Different public port dest_port: 7844, - hairpinning_works: None, }); report.update(&probe_port_7844); @@ -402,7 +377,6 @@ mod tests { latency: Duration::from_millis(45), addr: "[2001:db8::1]:54321".parse::().unwrap(), dest_port: 7842, - hairpinning_works: None, }); let probe_v6_7843 = ProbeReport::QadIpv6(QadProbeReport { @@ -410,7 +384,6 @@ mod tests { latency: Duration::from_millis(47), addr: "[2001:db8::1]:54399".parse::().unwrap(), // Different public port dest_port: 7843, - hairpinning_works: None, }); report.update(&probe_v6_7842); @@ -439,7 +412,6 @@ mod tests { latency: Duration::from_millis(50), addr: "203.0.113.42:54321".parse::().unwrap(), dest_port: 7842, - hairpinning_works: None, }); let probe2 = ProbeReport::QadIpv4(QadProbeReport { @@ -447,7 +419,6 @@ mod tests { latency: Duration::from_millis(52), addr: "203.0.113.42:54322".parse::().unwrap(), // Different public port for same dest port dest_port: 7842, - hairpinning_works: None, }); report.update(&probe1); @@ -460,76 +431,4 @@ mod tests { "Should detect unstable mapping for same dest port" ); } - - #[test] - fn test_hairpinning_detection() { - let mut report = Report::default(); - let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); - - // Test hairpinning works - let probe_hairpin_works = ProbeReport::QadIpv4(QadProbeReport { - node: relay_url.clone(), - latency: Duration::from_millis(50), - addr: "203.0.113.42:54321".parse::().unwrap(), - dest_port: 7842, - hairpinning_works: Some(true), - }); - - report.update(&probe_hairpin_works); - assert_eq!(report.hairpinning_ipv4, Some(true)); - - // Test hairpinning doesn't work (should override previous value) - let probe_hairpin_fails = ProbeReport::QadIpv4(QadProbeReport { - node: relay_url.clone(), - latency: Duration::from_millis(55), - addr: "203.0.113.42:54322".parse::().unwrap(), - dest_port: 7843, - hairpinning_works: Some(false), - }); - - report.update(&probe_hairpin_fails); - assert_eq!( - report.hairpinning_ipv4, - Some(false), - "Should update hairpinning result" - ); - } - - #[test] - fn test_combined_functionality() { - let mut report = Report::default(); - let relay_url = RelayUrl::from(url::Url::parse("https://relay.example.com").unwrap()); - - // Test that the combined helper functions work correctly - let probe1 = ProbeReport::QadIpv4(QadProbeReport { - node: relay_url.clone(), - latency: Duration::from_millis(50), - addr: "203.0.113.42:54321".parse::().unwrap(), - dest_port: 7842, - hairpinning_works: Some(true), - }); - - let probe2 = ProbeReport::QadIpv6(QadProbeReport { - node: relay_url.clone(), - latency: Duration::from_millis(45), - addr: "[2001:db8::1]:54399".parse::().unwrap(), - dest_port: 7842, - hairpinning_works: Some(false), - }); - - report.update(&probe1); - report.update(&probe2); - - // Test the combined helper functions - assert_eq!( - report.mapping_varies_by_dest_port(), - None, - "Should be None when only one dest port tested per IP version" - ); - assert_eq!( - report.hairpinning(), - Some(true), - "Should be true if any IP version supports hairpinning" - ); - } } diff --git a/iroh/src/net_report/reportgen.rs b/iroh/src/net_report/reportgen.rs index 6a7a26cb5ff..1a3d3556fce 100644 --- a/iroh/src/net_report/reportgen.rs +++ b/iroh/src/net_report/reportgen.rs @@ -436,8 +436,6 @@ pub(super) struct QadProbeReport { pub(super) addr: SocketAddr, /// The destination port that was used for this probe pub(super) dest_port: u16, - /// Whether hairpinning test succeeded (if attempted) - pub(super) hairpinning_works: Option, } #[derive(Debug, Clone)] From 88144c280c4becacd41cfc68cd7e9b8a3dda6309 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Tue, 2 Sep 2025 14:31:18 +0200 Subject: [PATCH 07/12] fix port variation probing --- iroh/src/net_report.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/iroh/src/net_report.rs b/iroh/src/net_report.rs index d46e4c59140..4339739b6d1 100644 --- a/iroh/src/net_report.rs +++ b/iroh/src/net_report.rs @@ -792,11 +792,12 @@ async fn run_probe_v4( node: relay_node.url.clone(), addr: external_addr, latency: conn.rtt(), - dest_port: relay_addr.port(), + dest_port: relay_addr_with_port.port(), }; let observer = Watchable::new(None); let node = relay_node.url.clone(); + let dest_port = relay_addr_with_port.port(); let handle = task::spawn({ let conn = conn.clone(); let observer = observer.clone(); @@ -813,7 +814,7 @@ async fn run_probe_v4( node: node.clone(), addr, latency, - dest_port: relay_addr.port(), + dest_port, })) .ok(); if receiver.changed().await.is_err() { @@ -881,11 +882,12 @@ async fn run_probe_v6( node: relay_node.url.clone(), addr: external_addr, latency: conn.rtt(), - dest_port: relay_addr.port(), + dest_port: relay_addr_with_port.port(), }; let observer = Watchable::new(None); let node = relay_node.url.clone(); + let dest_port = relay_addr_with_port.port(); let handle = task::spawn({ let observer = observer.clone(); let conn = conn.clone(); @@ -902,7 +904,7 @@ async fn run_probe_v6( node: node.clone(), addr, latency, - dest_port: relay_addr.port(), + dest_port, })) .ok(); if receiver.changed().await.is_err() { From 8b3f1e00ad9fe7e834dd18015c31b363ad18296b Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Tue, 2 Sep 2025 14:36:23 +0200 Subject: [PATCH 08/12] bump tracing subscriber to comply with cargo deny --- Cargo.lock | 48 +++++++++++++----------------------------------- 1 file changed, 13 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 424b2818aa3..6e1605368a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2766,11 +2766,11 @@ dependencies = [ [[package]] name = "matchers" -version = "0.1.0" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8263075bb86c5a1b1427b5ae862e8889656f126e9f77c484496e8b47cf5c5558" +checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9" dependencies = [ - "regex-automata 0.1.10", + "regex-automata", ] [[package]] @@ -3092,12 +3092,11 @@ dependencies = [ [[package]] name = "nu-ansi-term" -version = "0.46.0" +version = "0.50.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77a8165726e8236064dbb45459242600304b42a5ea24ee2948e18e023bf7ba84" +checksum = "d4a28e057d01f97e61255210fcff094d74ed0466038633e95017f5beb68e4399" dependencies = [ - "overload", - "winapi", + "windows-sys 0.52.0", ] [[package]] @@ -3217,12 +3216,6 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d05e27ee213611ffe7d6348b942e8f942b37114c00cc03cec254295a4a17852e" -[[package]] -name = "overload" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" - [[package]] name = "parking" version = "2.2.1" @@ -3670,7 +3663,7 @@ dependencies = [ "rand 0.9.1", "rand_chacha 0.9.0", "rand_xorshift", - "regex-syntax 0.8.5", + "regex-syntax", "rusty-fork", "tempfile", "unarray", @@ -3937,17 +3930,8 @@ checksum = "b544ef1b4eac5dc2db33ea63606ae9ffcfac26c1416a2806ae0bf5f56b201191" dependencies = [ "aho-corasick", "memchr", - "regex-automata 0.4.9", - "regex-syntax 0.8.5", -] - -[[package]] -name = "regex-automata" -version = "0.1.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6c230d73fb8d8c1b9c0b3135c5142a8acee3a0558fb8db5cf1cb65f8d7862132" -dependencies = [ - "regex-syntax 0.6.29", + "regex-automata", + "regex-syntax", ] [[package]] @@ -3958,7 +3942,7 @@ checksum = "809e8dc61f6de73b46c85f4c96486310fe304c434cfa43669d7b40f711150908" dependencies = [ "aho-corasick", "memchr", - "regex-syntax 0.8.5", + "regex-syntax", ] [[package]] @@ -3967,12 +3951,6 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "53a49587ad06b26609c52e423de037e7f57f20d53535d66e08c695f347df952a" -[[package]] -name = "regex-syntax" -version = "0.6.29" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f162c6dd7b008981e4d40210aca20b4bd0f9b60ca9271061b07f78537722f2e1" - [[package]] name = "regex-syntax" version = "0.8.5" @@ -5248,14 +5226,14 @@ dependencies = [ [[package]] name = "tracing-subscriber" -version = "0.3.19" +version = "0.3.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8189decb5ac0fa7bc8b96b7cb9b2701d60d48805aca84a238004d665fcc4008" +checksum = "2054a14f5307d601f88daf0553e1cbf472acc4f2c51afab632431cdcd72124d5" dependencies = [ "matchers", "nu-ansi-term", "once_cell", - "regex", + "regex-automata", "sharded-slab", "smallvec", "thread_local", From 2f4d4f014ed3aedd7ecad4faa95ff64cb8f57593 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Tue, 2 Sep 2025 14:45:25 +0200 Subject: [PATCH 09/12] make the port_variation_addr not an Option --- iroh-relay/src/quic.rs | 143 +++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 84 deletions(-) diff --git a/iroh-relay/src/quic.rs b/iroh-relay/src/quic.rs index 8f3b7783d2d..f499d04eae7 100644 --- a/iroh-relay/src/quic.rs +++ b/iroh-relay/src/quic.rs @@ -17,6 +17,8 @@ pub const QUIC_ADDR_DISC_CLOSE_REASON: &[u8] = b"finished"; #[cfg(feature = "server")] pub(crate) mod server { + use std::net::{SocketAddrV4, SocketAddrV6}; + use quinn::{ ApplicationClose, ConnectionError, crypto::rustls::{NoInitialCipherSuite, QuicServerConfig}, @@ -120,41 +122,22 @@ pub(crate) mod server { // Spawn port variation server (port + 1) for NAT testing let port_variation_addr = match quic_config.bind_addr { - SocketAddr::V4(addr) => Some(SocketAddr::V4(std::net::SocketAddrV4::new( - *addr.ip(), - addr.port() + 1, - ))), - SocketAddr::V6(addr) => Some(SocketAddr::V6(std::net::SocketAddrV6::new( - *addr.ip(), - addr.port() + 1, - 0, - 0, - ))), - }; - - let port_variation_endpoint = if let Some(port_variation_addr) = port_variation_addr { - match quinn::Endpoint::server(server_config.clone(), port_variation_addr) { - Ok(endpoint) => { - info!( - ?port_variation_addr, - "QUIC port variation server listening on" - ); - Some(endpoint) - } - Err(err) => { - // Log the error but don't fail - port variation is optional - tracing::warn!( - ?port_variation_addr, - ?err, - "Failed to bind QUIC port variation server, continuing without it" - ); - None - } + SocketAddr::V4(addr) => { + SocketAddr::V4(SocketAddrV4::new(*addr.ip(), addr.port() + 1)) + } + SocketAddr::V6(addr) => { + SocketAddr::V6(SocketAddrV6::new(*addr.ip(), addr.port() + 1, 0, 0)) } - } else { - None }; + let port_variation_endpoint = + quinn::Endpoint::server(server_config.clone(), port_variation_addr) + .context(EndpointServerSnafu)?; + info!( + ?port_variation_addr, + "QUIC port variation server listening on" + ); + info!(?bind_addr, "QUIC server listening on"); let cancel = CancellationToken::new(); @@ -213,68 +196,60 @@ pub(crate) mod server { .instrument(info_span!("quic-endpoint")), ); - // Spawn separate task for port variation endpoint if it exists - let port_var_task = if let Some(port_var_endpoint) = port_variation_endpoint { - let port_var_cancel = cancel.child_token(); - Some(tokio::task::spawn( - async move { - let mut set = JoinSet::new(); - debug!("waiting for connections on port variation port..."); - loop { - tokio::select! { - biased; - _ = port_var_cancel.cancelled() => { - break; - } - Some(res) = set.join_next() => { - if let Err(err) = res { - if err.is_panic() { - panic!("port variation task panicked: {err:#?}"); - } else { - debug!("error accepting port variation connection: {err:#?}"); - } + // Spawn separate task for port variation endpoint + let port_var_cancel = cancel.child_token(); + let port_var_task = tokio::task::spawn( + async move { + let mut set = JoinSet::new(); + debug!("waiting for connections on port variation port..."); + loop { + tokio::select! { + biased; + _ = port_var_cancel.cancelled() => { + break; + } + Some(res) = set.join_next() => { + if let Err(err) = res { + if err.is_panic() { + panic!("port variation task panicked: {err:#?}"); + } else { + debug!("error accepting port variation connection: {err:#?}"); } } - res = port_var_endpoint.accept() => match res { - Some(conn) => { - debug!("accepting connection on port variation port"); - let remote_addr = conn.remote_address(); - set.spawn(async move { - if let Err(err) = handle_connection(conn).await { - debug!("port variation connection error: {err:#?}"); - } - }.instrument(info_span!("qad-conn-port-var", %remote_addr))); - } - None => { - debug!("port variation endpoint closed"); - break; - } + } + res = port_variation_endpoint.accept() => match res { + Some(conn) => { + debug!("accepting connection on port variation port"); + let remote_addr = conn.remote_address(); + set.spawn(async move { + if let Err(err) = handle_connection(conn).await { + debug!("port variation connection error: {err:#?}"); + } + }.instrument(info_span!("qad-conn-port-var", %remote_addr))); + } + None => { + debug!("port variation endpoint closed"); + break; } } } - port_var_endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); - port_var_endpoint.wait_idle().await; - - set.abort_all(); - while !set.is_empty() { - set.join_next().await; - } + } + port_variation_endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); + port_variation_endpoint.wait_idle().await; - debug!("port variation quic endpoint has been shutdown."); + set.abort_all(); + while !set.is_empty() { + set.join_next().await; } - .instrument(info_span!("quic-endpoint-port-var")), - )) - } else { - None - }; + + debug!("port variation quic endpoint has been shutdown."); + } + .instrument(info_span!("quic-endpoint-port-var")), + ); // Combine both tasks into a single handle let combined_task = tokio::task::spawn(async move { - if let Some(port_var_task) = port_var_task { - let _ = tokio::try_join!(main_task, port_var_task); - } else { - let _ = main_task.await; - } + let _ = tokio::try_join!(main_task, port_var_task); }); Ok(Self { From 578a4cc731c9d94f99c92aa93643453787ded045 Mon Sep 17 00:00:00 2001 From: Frando Date: Wed, 3 Sep 2025 13:05:08 +0200 Subject: [PATCH 10/12] refactor: add config for alternate port, make endpoint loop DRY --- iroh-relay/src/main.rs | 1 + iroh-relay/src/quic.rs | 188 +++++++++++++------------------ iroh-relay/src/server.rs | 30 +++++ iroh-relay/src/server/testing.rs | 1 + iroh/src/test_utils.rs | 1 + 5 files changed, 109 insertions(+), 112 deletions(-) diff --git a/iroh-relay/src/main.rs b/iroh-relay/src/main.rs index df9424b5d9c..e26ae8774ae 100644 --- a/iroh-relay/src/main.rs +++ b/iroh-relay/src/main.rs @@ -654,6 +654,7 @@ async fn build_relay_config(cfg: Config) -> Result None, + Some(addr) => { + let endpoint = quinn::Endpoint::server(server_config.clone(), addr) + .context(EndpointServerSnafu)?; + info!( + ?port_variation_addr, + "QUIC port variation server listening on" + ); + Some(endpoint) + } + }; info!(?bind_addr, "QUIC server listening on"); let cancel = CancellationToken::new(); - let cancel_accept_loop = cancel.clone(); - - let main_task = tokio::task::spawn( - async move { - let mut set = JoinSet::new(); - debug!("waiting for connections on main port..."); - loop { - tokio::select! { - biased; - _ = cancel_accept_loop.cancelled() => { - break; - } - Some(res) = set.join_next() => { - if let Err(err) = res { - if err.is_panic() { - panic!("task panicked: {err:#?}"); - } else { - debug!("error accepting incoming connection: {err:#?}"); - } - } - } - res = endpoint.accept() => match res { - Some(conn) => { - debug!("accepting connection on main port"); - let remote_addr = conn.remote_address(); - set.spawn(async move { - if let Err(err) = handle_connection(conn).await { - debug!("connection error: {err:#?}"); - } - }.instrument(info_span!("qad-conn", %remote_addr))); - } - None => { - debug!("main endpoint closed"); - break; - } - } - } - } - // close all connections and wait until they have all grace - // fully closed. - endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); - endpoint.wait_idle().await; - - // all tasks should be closed, since the endpoint has shutdown - // all connections, but await to ensure they are finished. - set.abort_all(); - while !set.is_empty() { - set.join_next().await; - } - - debug!("quic endpoint has been shutdown."); - } - .instrument(info_span!("quic-endpoint")), - ); - // Spawn separate task for port variation endpoint - let port_var_cancel = cancel.child_token(); - let port_var_task = tokio::task::spawn( + let task = tokio::task::spawn({ + let cancel = cancel.child_token(); async move { - let mut set = JoinSet::new(); - debug!("waiting for connections on port variation port..."); - loop { - tokio::select! { - biased; - _ = port_var_cancel.cancelled() => { - break; - } - Some(res) = set.join_next() => { - if let Err(err) = res { - if err.is_panic() { - panic!("port variation task panicked: {err:#?}"); - } else { - debug!("error accepting port variation connection: {err:#?}"); - } - } - } - res = port_variation_endpoint.accept() => match res { - Some(conn) => { - debug!("accepting connection on port variation port"); - let remote_addr = conn.remote_address(); - set.spawn(async move { - if let Err(err) = handle_connection(conn).await { - debug!("port variation connection error: {err:#?}"); - } - }.instrument(info_span!("qad-conn-port-var", %remote_addr))); - } - None => { - debug!("port variation endpoint closed"); - break; - } - } + let main_fut = accept_loop(endpoint, cancel.clone()) + .instrument(info_span!("quic-endpoint")); + let port_variation_fut = async move { + if let Some(endpoint) = port_variation_endpoint { + accept_loop(endpoint, cancel).await } } - port_variation_endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); - port_variation_endpoint.wait_idle().await; - - set.abort_all(); - while !set.is_empty() { - set.join_next().await; - } - - debug!("port variation quic endpoint has been shutdown."); + .instrument(info_span!("quic-endpoint-port-var")); + let _ = tokio::join!(main_fut, port_variation_fut); } - .instrument(info_span!("quic-endpoint-port-var")), - ); - - // Combine both tasks into a single handle - let combined_task = tokio::task::spawn(async move { - let _ = tokio::try_join!(main_task, port_var_task); }); Ok(Self { bind_addr, cancel, - handle: AbortOnDropHandle::new(combined_task), + handle: AbortOnDropHandle::new(task), }) } @@ -271,6 +184,56 @@ pub(crate) mod server { } } + async fn accept_loop(endpoint: quinn::Endpoint, cancel_accept_loop: CancellationToken) { + let mut set = JoinSet::new(); + debug!("waiting for connections on main port..."); + loop { + tokio::select! { + biased; + _ = cancel_accept_loop.cancelled() => { + break; + } + Some(res) = set.join_next() => { + if let Err(err) = res { + if err.is_panic() { + panic!("task panicked: {err:#?}"); + } else { + debug!("error accepting incoming connection: {err:#?}"); + } + } + } + res = endpoint.accept() => match res { + Some(conn) => { + debug!("accepting connection on main port"); + let remote_addr = conn.remote_address(); + set.spawn(async move { + if let Err(err) = handle_connection(conn).await { + debug!("connection error: {err:#?}"); + } + }.instrument(info_span!("qad-conn", %remote_addr))); + } + None => { + debug!("main endpoint closed"); + break; + } + } + } + } + // close all connections and wait until they have all grace + // fully closed. + endpoint.close(QUIC_ADDR_DISC_CLOSE_CODE, QUIC_ADDR_DISC_CLOSE_REASON); + endpoint.wait_idle().await; + + // all tasks should be closed, since the endpoint has shutdown + // all connections, but await to ensure they are finished. + set.abort_all(); + while !set.is_empty() { + set.join_next().await; + } + + debug!("quic endpoint has been shutdown."); + } + /// A handle for the Server side of QUIC address discovery. /// /// This does not allow access to the task but can communicate with it. @@ -464,6 +427,7 @@ mod tests { let quic_server = QuicServer::spawn(QuicConfig { server_config, bind_addr, + alternate_port: Default::default(), })?; // create a client-side endpoint diff --git a/iroh-relay/src/server.rs b/iroh-relay/src/server.rs index eb9c15a2539..b8d4a2aa1c6 100644 --- a/iroh-relay/src/server.rs +++ b/iroh-relay/src/server.rs @@ -173,6 +173,36 @@ pub struct QuicConfig { /// If this [`rustls::ServerConfig`] does not support TLS 1.3, the QUIC server will fail /// to spawn. pub server_config: rustls::ServerConfig, + + /// Select an additional port to listen on. + /// + /// This may be used by nodes to check for NAT port binding variation. + pub alternate_port: AlternatePortConfig, +} + +/// Configuration for the alternate listening port of the QUIC server. +#[derive(Debug, Default, Copy, Clone)] +pub enum AlternatePortConfig { + /// Don't listen on an additional port. + Disabled, + /// Use the port of the bind address + 1. + #[default] + MainPortPlusOne, + /// Use the specified port. + Port(u16), +} + +impl AlternatePortConfig { + /// Returns the bind address for the alternate endpoint, or None if disabled. + pub(crate) fn get_bind_addr(self, main_addr: SocketAddr) -> Option { + match self { + AlternatePortConfig::Disabled => None, + AlternatePortConfig::MainPortPlusOne => { + Some(SocketAddr::from((main_addr.ip(), main_addr.port() + 1))) + } + AlternatePortConfig::Port(port) => Some(SocketAddr::from((main_addr.ip(), port))), + } + } } /// TLS configuration for Relay server. diff --git a/iroh-relay/src/server/testing.rs b/iroh-relay/src/server/testing.rs index 6a30f303e62..83d569f4b01 100644 --- a/iroh-relay/src/server/testing.rs +++ b/iroh-relay/src/server/testing.rs @@ -71,6 +71,7 @@ pub fn quic_config() -> QuicConfig { QuicConfig { bind_addr: (Ipv4Addr::UNSPECIFIED, 0).into(), server_config, + alternate_port: Default::default(), } } diff --git a/iroh/src/test_utils.rs b/iroh/src/test_utils.rs index bfd045f949e..ab18cfe7239 100644 --- a/iroh/src/test_utils.rs +++ b/iroh/src/test_utils.rs @@ -49,6 +49,7 @@ pub async fn run_relay_server_with(quic: bool) -> Result<(RelayMap, RelayUrl, Se Some(QuicConfig { server_config: tls.server_config.clone(), bind_addr: tls.quic_bind_addr, + alternate_port: Default::default(), }) } else { None From 097168c3d7477ac7fff6a3a7ec3b77e8cd16ab92 Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Thu, 4 Sep 2025 18:03:11 +0200 Subject: [PATCH 11/12] disable alt port on tests --- iroh-relay/src/quic.rs | 2 +- iroh-relay/src/server/testing.rs | 5 +++-- iroh/src/test_utils.rs | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/iroh-relay/src/quic.rs b/iroh-relay/src/quic.rs index d874d2aec6d..9530c0210a4 100644 --- a/iroh-relay/src/quic.rs +++ b/iroh-relay/src/quic.rs @@ -427,7 +427,7 @@ mod tests { let quic_server = QuicServer::spawn(QuicConfig { server_config, bind_addr, - alternate_port: Default::default(), + alternate_port: crate::server::AlternatePortConfig::Disabled, })?; // create a client-side endpoint diff --git a/iroh-relay/src/server/testing.rs b/iroh-relay/src/server/testing.rs index 83d569f4b01..856ea4167ec 100644 --- a/iroh-relay/src/server/testing.rs +++ b/iroh-relay/src/server/testing.rs @@ -1,7 +1,7 @@ //! Exposes functions to quickly configure a server suitable for testing. use std::net::Ipv4Addr; -use super::{AccessConfig, CertConfig, QuicConfig, RelayConfig, ServerConfig, TlsConfig}; +use super::{AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, ServerConfig, TlsConfig}; /// Creates a [`rustls::ServerConfig`] and certificates suitable for testing. /// @@ -65,13 +65,14 @@ pub fn relay_config() -> RelayConfig<()> { /// Creates a [`QuicConfig`] suitable for testing. /// /// - Binds to an OS assigned port on ipv4 +/// - Disables alternate port to avoid conflicts in parallel tests /// - Uses [`self_signed_tls_certs_and_config`] to create tls certificates pub fn quic_config() -> QuicConfig { let (_, server_config) = self_signed_tls_certs_and_config(); QuicConfig { bind_addr: (Ipv4Addr::UNSPECIFIED, 0).into(), server_config, - alternate_port: Default::default(), + alternate_port: AlternatePortConfig::Disabled, // Explicitly disable for tests } } diff --git a/iroh/src/test_utils.rs b/iroh/src/test_utils.rs index ab18cfe7239..1a7fd858575 100644 --- a/iroh/src/test_utils.rs +++ b/iroh/src/test_utils.rs @@ -6,7 +6,7 @@ use iroh_base::RelayUrl; use iroh_relay::{ RelayMap, RelayNode, RelayQuicConfig, server::{ - AccessConfig, CertConfig, QuicConfig, RelayConfig, Server, ServerConfig, SpawnError, + AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, Server, ServerConfig, SpawnError, TlsConfig, }, }; @@ -49,7 +49,7 @@ pub async fn run_relay_server_with(quic: bool) -> Result<(RelayMap, RelayUrl, Se Some(QuicConfig { server_config: tls.server_config.clone(), bind_addr: tls.quic_bind_addr, - alternate_port: Default::default(), + alternate_port: AlternatePortConfig::Disabled, // Disable to avoid port conflicts in tests }) } else { None From f40397ef724491609b053e37c19554b9fb575d1a Mon Sep 17 00:00:00 2001 From: Asmir Avdicevic Date: Fri, 5 Sep 2025 16:01:15 +0200 Subject: [PATCH 12/12] simplifying --- iroh-relay/src/quic.rs | 31 +++++-------- iroh-relay/src/server.rs | 7 +-- iroh-relay/src/server/testing.rs | 19 +++++++- iroh/src/net_report.rs | 46 +++++++++++++------ iroh/src/net_report/reportgen.rs | 78 ++++++++++++++++++++++++++++++++ iroh/src/test_utils.rs | 4 +- 6 files changed, 142 insertions(+), 43 deletions(-) diff --git a/iroh-relay/src/quic.rs b/iroh-relay/src/quic.rs index 9530c0210a4..fda4dd7c737 100644 --- a/iroh-relay/src/quic.rs +++ b/iroh-relay/src/quic.rs @@ -17,7 +17,6 @@ pub const QUIC_ADDR_DISC_CLOSE_REASON: &[u8] = b"finished"; #[cfg(feature = "server")] pub(crate) mod server { - use std::net::{SocketAddrV4, SocketAddrV6}; use quinn::{ ApplicationClose, ConnectionError, @@ -120,28 +119,17 @@ pub(crate) mod server { .context(EndpointServerSnafu)?; let bind_addr = endpoint.local_addr().context(LocalAddrSnafu)?; - // Spawn port variation server (port + 1) for NAT testing - let port_variation_addr = match quic_config.bind_addr { - SocketAddr::V4(addr) => { - SocketAddr::V4(SocketAddrV4::new(*addr.ip(), addr.port() + 1)) - } - SocketAddr::V6(addr) => { - SocketAddr::V6(SocketAddrV6::new(*addr.ip(), addr.port() + 1, 0, 0)) - } - }; - + // Spawn port variation server for NAT testing based on configuration let port_variation_addr = quic_config .alternate_port .get_bind_addr(quic_config.bind_addr); let port_variation_endpoint = match port_variation_addr { None => None, Some(addr) => { - let endpoint = quinn::Endpoint::server(server_config.clone(), addr) + let endpoint = quinn::Endpoint::server(server_config, addr) .context(EndpointServerSnafu)?; - info!( - ?port_variation_addr, - "QUIC port variation server listening on" - ); + let actual_addr = endpoint.local_addr().context(LocalAddrSnafu)?; + info!(?actual_addr, "QUIC port variation server listening on"); Some(endpoint) } }; @@ -160,7 +148,7 @@ pub(crate) mod server { accept_loop(endpoint, cancel).await } } - .instrument(info_span!("quic-endpoint-port-var")); + .instrument(info_span!("quic-endpoint-alt-port")); let _ = tokio::join!(main_fut, port_variation_fut); } }); @@ -186,7 +174,10 @@ pub(crate) mod server { async fn accept_loop(endpoint: quinn::Endpoint, cancel_accept_loop: CancellationToken) { let mut set = JoinSet::new(); - debug!("waiting for connections on main port..."); + let local_addr = endpoint + .local_addr() + .unwrap_or_else(|_| "unknown".parse().unwrap()); + debug!("waiting for connections on {local_addr}..."); loop { tokio::select! { biased; @@ -204,7 +195,7 @@ pub(crate) mod server { } res = endpoint.accept() => match res { Some(conn) => { - debug!("accepting connection on main port"); + debug!("accepting connection on {local_addr}"); let remote_addr = conn.remote_address(); set.spawn(async move { if let Err(err) = handle_connection(conn).await { @@ -213,7 +204,7 @@ pub(crate) mod server { }.instrument(info_span!("qad-conn", %remote_addr))); } None => { - debug!("main endpoint closed"); + debug!("endpoint closed"); break; } } diff --git a/iroh-relay/src/server.rs b/iroh-relay/src/server.rs index b8d4a2aa1c6..60192c10e2b 100644 --- a/iroh-relay/src/server.rs +++ b/iroh-relay/src/server.rs @@ -187,9 +187,7 @@ pub enum AlternatePortConfig { Disabled, /// Use the port of the bind address + 1. #[default] - MainPortPlusOne, - /// Use the specified port. - Port(u16), + Enabled, } impl AlternatePortConfig { @@ -197,10 +195,9 @@ impl AlternatePortConfig { pub(crate) fn get_bind_addr(self, main_addr: SocketAddr) -> Option { match self { AlternatePortConfig::Disabled => None, - AlternatePortConfig::MainPortPlusOne => { + AlternatePortConfig::Enabled => { Some(SocketAddr::from((main_addr.ip(), main_addr.port() + 1))) } - AlternatePortConfig::Port(port) => Some(SocketAddr::from((main_addr.ip(), port))), } } } diff --git a/iroh-relay/src/server/testing.rs b/iroh-relay/src/server/testing.rs index 856ea4167ec..fcc6135ddc2 100644 --- a/iroh-relay/src/server/testing.rs +++ b/iroh-relay/src/server/testing.rs @@ -1,7 +1,9 @@ //! Exposes functions to quickly configure a server suitable for testing. use std::net::Ipv4Addr; -use super::{AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, ServerConfig, TlsConfig}; +use super::{ + AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, ServerConfig, TlsConfig, +}; /// Creates a [`rustls::ServerConfig`] and certificates suitable for testing. /// @@ -76,10 +78,23 @@ pub fn quic_config() -> QuicConfig { } } +/// Creates a [`QuicConfig`] with port variation enabled for testing port detection logic. +/// +/// - Same as regular quic_config but with alternate port enabled +/// - Uses OS-assigned port (0) with alternate port enabled (main_port + 1) +pub fn quic_config_with_port_variation() -> QuicConfig { + let (_, server_config) = self_signed_tls_certs_and_config(); + QuicConfig { + bind_addr: (Ipv4Addr::UNSPECIFIED, 0).into(), // Let OS assign port + server_config, + alternate_port: AlternatePortConfig::Enabled, // Enable alternate port (main + 1) + } +} + /// Creates a [`ServerConfig`] suitable for testing. /// /// - Relaying is enabled using [`relay_config`] -/// - QUIC addr discovery is disabled. +/// - QUIC addr discovery is enabled with disabled alternate port to avoid conflicts. /// - Metrics are not enabled. pub fn server_config() -> ServerConfig<()> { ServerConfig { diff --git a/iroh/src/net_report.rs b/iroh/src/net_report.rs index 4339739b6d1..56ed40523fc 100644 --- a/iroh/src/net_report.rs +++ b/iroh/src/net_report.rs @@ -461,11 +461,9 @@ impl Client { .instrument(info_span!("QAD-IPv4", %relay_url)), ); - // Port variation probe (QUIC port + 1) - if let Ok(relay_addr) = - reportgen::get_relay_addr_ipv4(&dns_resolver, &relay_node).await - { - let port_variation = relay_addr.port() + 1; + // Port variation probe + if let Some(quic_config) = &relay_node.quic { + let alternate_port = quic_config.port + 1; v4_buf.spawn( cancel_v4 .child_token() @@ -476,11 +474,11 @@ impl Client { relay_node, quic_client, dns_resolver, - Some(port_variation), + Some(alternate_port), ), )) .instrument( - info_span!("QAD-IPv4-PortVar", %relay_url, port = port_variation), + info_span!("QAD-IPv4-PortVar", %relay_url, port = alternate_port), ), ); } @@ -511,11 +509,9 @@ impl Client { .instrument(info_span!("QAD-IPv6", %relay_url)), ); - // Port variation probe (QUIC port + 1) - if let Ok(relay_addr) = - reportgen::get_relay_addr_ipv6(&dns_resolver, &relay_node).await - { - let port_variation = relay_addr.port() + 1; + // Port variation probe + if let Some(quic_config) = &relay_node.quic { + let alternate_port = quic_config.port + 1; v6_buf.spawn( cancel_v6 .child_token() @@ -526,11 +522,11 @@ impl Client { relay_node, quic_client, dns_resolver, - Some(port_variation), + Some(alternate_port), ), )) .instrument( - info_span!("QAD-IPv6-PortVar", %relay_url, port = port_variation), + info_span!("QAD-IPv6-PortVar", %relay_url, port = alternate_port), ), ); } @@ -946,6 +942,28 @@ mod test_utils { (server, node_desc) } + /// Create a relay server with port variation explicitly enabled for testing + pub(crate) async fn relay_with_port_variation() -> (server::Server, RelayNode) { + let quic_config = server::testing::quic_config_with_port_variation(); + let mut server_config = server::testing::server_config(); + + server_config.quic = Some(quic_config); + + let server = server::Server::spawn(server_config) + .await + .expect("should serve relay with port variation"); + + let quic = Some(RelayQuicConfig { + port: server.quic_addr().expect("server should run quic").port(), + }); + let node_desc = RelayNode { + url: server.https_url().expect("should work as relay"), + quic, + }; + + (server, node_desc) + } + /// Create a [`crate::RelayMap`] of the given size. /// /// This function uses [`relay`]. Note that the returned map uses internal order that will diff --git a/iroh/src/net_report/reportgen.rs b/iroh/src/net_report/reportgen.rs index 1a3d3556fce..11379bc57c8 100644 --- a/iroh/src/net_report/reportgen.rs +++ b/iroh/src/net_report/reportgen.rs @@ -890,4 +890,82 @@ mod tests { server.shutdown().await?; Ok(()) } + + mod run_in_isolation { + use n0_snafu::ResultExt; + + use super::*; + use crate::net_report::run_probe_v4; + + #[tokio::test] + #[traced_test] + async fn test_port_variation_detection() -> Result { + use crate::net_report::test_utils; + + let (server, relay_node) = test_utils::relay_with_port_variation().await; + let relay = Arc::new(relay_node); + let client_config = iroh_relay::client::make_dangerous_client_config(); + let ep = quinn::Endpoint::client(SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 0)).e()?; + let client_addr = ep.local_addr().e()?; + + let quic_client = iroh_relay::quic::QuicClient::new(ep.clone(), client_config); + let dns_resolver = DnsResolver::default(); + + let (regular_report, regular_conn) = run_probe_v4( + None, + relay.clone(), + quic_client.clone(), + dns_resolver.clone(), + None, + ) + .await + .unwrap(); + + // Run port variation probe (main port + 1) + let main_port = relay.quic.as_ref().unwrap().port; + let alternate_port = main_port + 1; + let (variation_report, variation_conn) = run_probe_v4( + None, + relay.clone(), + quic_client.clone(), + dns_resolver.clone(), + Some(alternate_port), + ) + .await + .unwrap(); + + assert_eq!( + regular_report.addr, client_addr, + "Regular probe should get correct address" + ); + assert_eq!( + variation_report.addr, client_addr, + "Port variation probe should get correct address" + ); + assert_eq!( + regular_report.dest_port, main_port, + "Regular probe should target main port" + ); + assert_eq!( + variation_report.dest_port, alternate_port, + "Variation probe should target alternate port" + ); + + let mut report = super::Report::default(); + report.update(&ProbeReport::QadIpv4(regular_report)); + report.update(&ProbeReport::QadIpv4(variation_report)); + + assert_eq!( + report.mapping_varies_by_dest_port_ipv4, + Some(false), + "Should detect no port variation when same external address is used" + ); + + drop(regular_conn); + drop(variation_conn); + ep.wait_idle().await; + server.shutdown().await?; + Ok(()) + } + } } diff --git a/iroh/src/test_utils.rs b/iroh/src/test_utils.rs index 1a7fd858575..1bee0bdd9e1 100644 --- a/iroh/src/test_utils.rs +++ b/iroh/src/test_utils.rs @@ -6,8 +6,8 @@ use iroh_base::RelayUrl; use iroh_relay::{ RelayMap, RelayNode, RelayQuicConfig, server::{ - AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, Server, ServerConfig, SpawnError, - TlsConfig, + AccessConfig, AlternatePortConfig, CertConfig, QuicConfig, RelayConfig, Server, + ServerConfig, SpawnError, TlsConfig, }, }; use tokio::sync::oneshot;