From d427585907b36e325e1fe2d4920cc1ed75c3aee4 Mon Sep 17 00:00:00 2001 From: James Parker Date: Mon, 11 Aug 2025 23:10:54 -0400 Subject: [PATCH 1/6] Add TCP for nat_pmp --- portmapper/src/nat_pmp/protocol.rs | 4 ++ portmapper/src/nat_pmp/protocol/request.rs | 51 +++++++++++++-------- portmapper/src/nat_pmp/protocol/response.rs | 38 +++++++++------ 3 files changed, 62 insertions(+), 31 deletions(-) diff --git a/portmapper/src/nat_pmp/protocol.rs b/portmapper/src/nat_pmp/protocol.rs index 5e4e3b43..15b45db9 100644 --- a/portmapper/src/nat_pmp/protocol.rs +++ b/portmapper/src/nat_pmp/protocol.rs @@ -32,4 +32,8 @@ pub enum Opcode { /// /// See [RFC 6886 Requesting a Mapping](https://datatracker.ietf.org/doc/html/rfc6886#section-3.3). MapUdp = 1, + /// Get a TCP Mapping. + /// + /// See [RFC 6886 Requesting a Mapping](https://datatracker.ietf.org/doc/html/rfc6886#section-3.3). + MapTcp = 2, } diff --git a/portmapper/src/nat_pmp/protocol/request.rs b/portmapper/src/nat_pmp/protocol/request.rs index 6483b899..af20e198 100644 --- a/portmapper/src/nat_pmp/protocol/request.rs +++ b/portmapper/src/nat_pmp/protocol/request.rs @@ -23,12 +23,13 @@ pub enum Request { } /// Protocol for which a port mapping is requested. -// NOTE: spec defines TCP as well, which we don't need. #[derive(Debug, Clone, Copy, PartialEq, Eq, IntoPrimitive, TryFromPrimitive)] #[repr(u8)] pub enum MapProtocol { /// UDP mapping. Udp = 1, + /// TCP mapping. + Tcp = 2, } impl Request { @@ -47,6 +48,7 @@ impl Request { } => { let opcode = match proto { MapProtocol::Udp => Opcode::MapUdp, + MapProtocol::Tcp => Opcode::MapTcp, }; let mut buf = vec![Version::NatPmp.into(), opcode.into()]; buf.push(0); // reserved @@ -69,35 +71,48 @@ impl Request { external_port: rng.random(), lifetime_seconds: rng.random(), }, + Opcode::MapTcp => Request::Mapping { + proto: MapProtocol::Tcp, + local_port: rng.random(), + external_port: rng.random(), + lifetime_seconds: rng.random(), + } } } #[cfg(test)] #[track_caller] fn decode(buf: &[u8]) -> Self { + fn decode_map(buf: &[u8], proto: MapProtocol) -> Request { + // buf[2] reserved + // buf[3] reserved + + let local_port_bytes = buf[4..6].try_into().expect("slice has the right size"); + let local_port = u16::from_be_bytes(local_port_bytes); + + let external_port_bytes = buf[6..8].try_into().expect("slice has the right size"); + let external_port = u16::from_be_bytes(external_port_bytes); + + let lifetime_bytes: [u8; 4] = buf[8..12].try_into().unwrap(); + let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); + Request::Mapping { + proto, + local_port, + external_port, + lifetime_seconds, + } + } + let _version: Version = buf[0].try_into().unwrap(); let opcode: super::Opcode = buf[1].try_into().unwrap(); // check if this is a mapping request, or an external address request match opcode { Opcode::DetermineExternalAddress => Request::ExternalAddress, Opcode::MapUdp => { - // buf[2] reserved - // buf[3] reserved - - let local_port_bytes = buf[4..6].try_into().expect("slice has the right size"); - let local_port = u16::from_be_bytes(local_port_bytes); - - let external_port_bytes = buf[6..8].try_into().expect("slice has the right size"); - let external_port = u16::from_be_bytes(external_port_bytes); - - let lifetime_bytes: [u8; 4] = buf[8..12].try_into().unwrap(); - let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); - Request::Mapping { - proto: MapProtocol::Udp, - local_port, - external_port, - lifetime_seconds, - } + decode_map(buf, MapProtocol::Udp) + } + Opcode::MapTcp => { + decode_map(buf, MapProtocol::Tcp) } } } diff --git a/portmapper/src/nat_pmp/protocol/response.rs b/portmapper/src/nat_pmp/protocol/response.rs index f5bdadcd..bd31e03a 100644 --- a/portmapper/src/nat_pmp/protocol/response.rs +++ b/portmapper/src/nat_pmp/protocol/response.rs @@ -143,19 +143,7 @@ impl Response { ResultCode::UnsupportedOpcode => Err(UnsupportedOpcodeSnafu.build()), }?; - let response = match opcode { - Opcode::DetermineExternalAddress => { - let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); - let epoch_time = u32::from_be_bytes(epoch_bytes); - let ip_bytes: [u8; 4] = buf[8..12].try_into().expect("slice has the right len"); - Response::PublicAddress { - epoch_time, - public_ip: ip_bytes.into(), - } - } - Opcode::MapUdp => { - let proto = MapProtocol::Udp; - + fn decode_map(buf: &[u8], proto: MapProtocol) -> Response { let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); let epoch_time = u32::from_be_bytes(epoch_bytes); @@ -175,6 +163,23 @@ impl Response { external_port, lifetime_seconds, } + } + + let response = match opcode { + Opcode::DetermineExternalAddress => { + let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); + let epoch_time = u32::from_be_bytes(epoch_bytes); + let ip_bytes: [u8; 4] = buf[8..12].try_into().expect("slice has the right len"); + Response::PublicAddress { + epoch_time, + public_ip: ip_bytes.into(), + } + } + Opcode::MapUdp => { + decode_map(buf, MapProtocol::Udp) + } + Opcode::MapTcp => { + decode_map(buf, MapProtocol::Tcp) } }; @@ -198,6 +203,13 @@ impl Response { external_port: rng.random(), lifetime_seconds: rng.random(), }, + Opcode::MapTcp => Response::PortMap { + proto: MapProtocol::Tcp, + epoch_time: rng.random(), + private_port: rng.random(), + external_port: rng.random(), + lifetime_seconds: rng.random(), + }, } } From e91b95f88cd1ac291a094e15202acf2d038fc9f8 Mon Sep 17 00:00:00 2001 From: James Parker Date: Mon, 11 Aug 2025 23:32:40 -0400 Subject: [PATCH 2/6] Add protocol config for UDP or TCP --- portmapper/src/lib.rs | 20 +++++++++++++++++--- portmapper/src/mapping.rs | 4 ++++ portmapper/src/nat_pmp.rs | 11 ++++++++--- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/portmapper/src/lib.rs b/portmapper/src/lib.rs index 990e2f63..a2855fd9 100644 --- a/portmapper/src/lib.rs +++ b/portmapper/src/lib.rs @@ -108,6 +108,15 @@ enum Message { }, } +/// Configuration for UDP or TCP network protocol. +#[derive(Debug, Clone, Copy)] +pub enum Protocol { + /// UDP protocol. + Udp, + /// TCP protocol. + Tcp, +} + /// Configures which port mapping protocols are enabled in the [`Service`]. #[derive(Debug, Clone)] pub struct Config { @@ -117,15 +126,18 @@ pub struct Config { pub enable_pcp: bool, /// Whether PMP is enabled. pub enable_nat_pmp: bool, + /// Whether to use UDP or TCP. + pub protocol: Protocol, } impl Default for Config { - /// By default all port mapping protocols are enabled. + /// By default all port mapping protocols are enabled for UDP. fn default() -> Self { Config { enable_upnp: true, enable_pcp: true, enable_nat_pmp: true, + protocol: Protocol::Udp, } } } @@ -281,6 +293,7 @@ impl Probe { enable_upnp, enable_pcp, enable_nat_pmp, + protocol: _, } = config; let mut upnp_probing_task = util::MaybeFuture { inner: (enable_upnp && !upnp).then(|| { @@ -633,6 +646,7 @@ impl Service { debug!("getting a port mapping for {local_ip}:{local_port} -> {external_addr:?}"); let recently_probed = self.full_probe.last_probe + UNAVAILABILITY_TRUST_DURATION > Instant::now(); + let protocol = self.config.protocol; // strategy: // 1. check the available services and prefer pcp, then nat_pmp then upnp since it's // the most unreliable, but possibly the most deployed one @@ -647,7 +661,7 @@ impl Service { } else if nat_pmp { // next nat_pmp if available let task = - mapping::Mapping::new_nat_pmp(local_ip, local_port, gateway, external_addr); + mapping::Mapping::new_nat_pmp(protocol, local_ip, local_port, gateway, external_addr); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pmp")), ))) @@ -675,7 +689,7 @@ impl Service { } else if !recently_probed && self.config.enable_nat_pmp { // finally try nat_pmp if enabled let task = - mapping::Mapping::new_nat_pmp(local_ip, local_port, gateway, external_addr); + mapping::Mapping::new_nat_pmp(protocol, local_ip, local_port, gateway, external_addr); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pmp")), ))) diff --git a/portmapper/src/mapping.rs b/portmapper/src/mapping.rs index e90c4e6e..aae66d8a 100644 --- a/portmapper/src/mapping.rs +++ b/portmapper/src/mapping.rs @@ -5,6 +5,8 @@ use std::{net::Ipv4Addr, num::NonZeroU16, time::Duration}; use nested_enum_utils::common_fields; use snafu::{Backtrace, ResultExt, Snafu}; +use crate::Protocol; + use super::{nat_pmp, pcp, upnp}; pub(super) trait PortMapped: std::fmt::Debug + Unpin { @@ -56,12 +58,14 @@ impl Mapping { /// Create a new NAT-PMP mapping. pub(crate) async fn new_nat_pmp( + protocol: Protocol, local_ip: Ipv4Addr, local_port: NonZeroU16, gateway: Ipv4Addr, external_addr: Option<(Ipv4Addr, NonZeroU16)>, ) -> Result { nat_pmp::Mapping::new( + protocol, local_ip, local_port, gateway, diff --git a/portmapper/src/nat_pmp.rs b/portmapper/src/nat_pmp.rs index a1dd6a2b..f131eee4 100644 --- a/portmapper/src/nat_pmp.rs +++ b/portmapper/src/nat_pmp.rs @@ -8,7 +8,7 @@ use snafu::{Backtrace, Snafu}; use tracing::{debug, trace}; use self::protocol::{MapProtocol, Request, Response}; -use crate::defaults::NAT_PMP_RECV_TIMEOUT as RECV_TIMEOUT; +use crate::{defaults::NAT_PMP_RECV_TIMEOUT as RECV_TIMEOUT, Protocol}; mod protocol; @@ -63,6 +63,7 @@ impl super::mapping::PortMapped for Mapping { impl Mapping { /// Attempt to register a new mapping with the NAT-PMP server on the provided gateway. pub async fn new( + protocol: Protocol, local_ip: Ipv4Addr, local_port: NonZeroU16, gateway: Ipv4Addr, @@ -72,8 +73,12 @@ impl Mapping { let socket = UdpSocket::bind_full((local_ip, 0))?; socket.connect((gateway, protocol::SERVER_PORT).into())?; + let proto = match protocol { + Protocol::Udp => MapProtocol::Udp, + Protocol::Tcp => MapProtocol::Tcp, + }; let req = Request::Mapping { - proto: MapProtocol::Udp, + proto, local_port: local_port.into(), external_port: external_port.map(Into::into).unwrap_or_default(), lifetime_seconds: MAPPING_REQUESTED_LIFETIME_SECONDS, @@ -92,7 +97,7 @@ impl Mapping { let (external_port, lifetime_seconds) = match response { Response::PortMap { - proto: MapProtocol::Udp, + proto: _, epoch_time: _, private_port, external_port, From 62aa93634fdd6d2ceddb04c5c8590685ec35f7d2 Mon Sep 17 00:00:00 2001 From: James Parker Date: Mon, 11 Aug 2025 23:49:02 -0400 Subject: [PATCH 3/6] Add TCP for pcp --- portmapper/src/lib.rs | 4 ++-- portmapper/src/mapping.rs | 3 ++- portmapper/src/pcp.rs | 20 +++++++++++++++----- portmapper/src/pcp/protocol/opcode_data.rs | 1 + portmapper/src/pcp/protocol/request.rs | 3 ++- 5 files changed, 22 insertions(+), 9 deletions(-) diff --git a/portmapper/src/lib.rs b/portmapper/src/lib.rs index a2855fd9..e27e7c99 100644 --- a/portmapper/src/lib.rs +++ b/portmapper/src/lib.rs @@ -654,7 +654,7 @@ impl Service { // nat_pmp self.mapping_task = if pcp { // try pcp if available first - let task = mapping::Mapping::new_pcp(local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_pcp(protocol, local_ip, local_port, gateway, external_addr); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pcp")), ))) @@ -681,7 +681,7 @@ impl Service { } else if !recently_probed && self.config.enable_pcp { // if no service is available and the default fallback (upnp) is disabled, try pcp // first - let task = mapping::Mapping::new_pcp(local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_pcp(protocol, local_ip, local_port, gateway, external_addr); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pcp")), diff --git a/portmapper/src/mapping.rs b/portmapper/src/mapping.rs index aae66d8a..f2626dca 100644 --- a/portmapper/src/mapping.rs +++ b/portmapper/src/mapping.rs @@ -45,12 +45,13 @@ pub enum Error { impl Mapping { /// Create a new PCP mapping. pub(crate) async fn new_pcp( + protocol: Protocol, local_ip: Ipv4Addr, local_port: NonZeroU16, gateway: Ipv4Addr, external_addr: Option<(Ipv4Addr, NonZeroU16)>, ) -> Result { - pcp::Mapping::new(local_ip, local_port, gateway, external_addr) + pcp::Mapping::new(protocol, local_ip, local_port, gateway, external_addr) .await .map(Self::Pcp) .context(PcpSnafu) diff --git a/portmapper/src/pcp.rs b/portmapper/src/pcp.rs index 0c8951de..b9074c41 100644 --- a/portmapper/src/pcp.rs +++ b/portmapper/src/pcp.rs @@ -8,7 +8,7 @@ use rand::RngCore; use snafu::{Backtrace, ResultExt, Snafu}; use tracing::{debug, trace}; -use crate::defaults::PCP_RECV_TIMEOUT as RECV_TIMEOUT; +use crate::{defaults::PCP_RECV_TIMEOUT as RECV_TIMEOUT, Protocol}; mod protocol; @@ -19,6 +19,8 @@ const MAPPING_REQUESTED_LIFETIME_SECONDS: u32 = 60 * 60; /// A mapping successfully registered with a PCP server. #[derive(Debug)] pub struct Mapping { + /// Protocol for this mapping. + protocol: protocol::MapProtocol, /// Local ip used to create this mapping. local_ip: Ipv4Addr, /// Local port used to create this mapping. @@ -45,7 +47,7 @@ pub struct Mapping { pub enum Error { #[snafu(display("received nonce does not match sent request"))] NonceMissmatch {}, - #[snafu(display("received mapping is not for UDP"))] + #[snafu(display("received mapping does not match the requested protocol"))] ProtocolMissmatch {}, #[snafu(display("received mapping is for a local port that does not match the requested one"))] PortMissmatch {}, @@ -74,6 +76,7 @@ impl super::mapping::PortMapped for Mapping { impl Mapping { /// Attempt to registered a new mapping with the PCP server on the provided gateway. pub async fn new( + protocol: Protocol, local_ip: Ipv4Addr, local_port: NonZeroU16, gateway: Ipv4Addr, @@ -93,8 +96,13 @@ impl Mapping { None => (None, None), }; + let protocol = match protocol { + Protocol::Udp => protocol::MapProtocol::Udp, + Protocol::Tcp => protocol::MapProtocol::Tcp, + }; let req = protocol::Request::mapping( nonce, + protocol, local_port.into(), local_ip, requested_port, @@ -126,7 +134,7 @@ impl Mapping { protocol::OpcodeData::MapData(map_data) => { let protocol::MapData { nonce: received_nonce, - protocol, + protocol: received_protocol, local_port: received_local_port, external_port, external_address, @@ -136,7 +144,7 @@ impl Mapping { return Err(NonceMissmatchSnafu.build()); } - if protocol != protocol::MapProtocol::Udp { + if received_protocol != protocol { return Err(ProtocolMissmatchSnafu.build()); } @@ -153,6 +161,7 @@ impl Mapping { .ok_or(NotIpv4Snafu.build())?; Ok(Mapping { + protocol: received_protocol, external_port, external_address, lifetime_seconds, @@ -168,6 +177,7 @@ impl Mapping { pub async fn release(self) -> Result<(), Error> { let Mapping { + protocol, nonce, local_ip, local_port, @@ -182,7 +192,7 @@ impl Mapping { .context(IoSnafu)?; let local_port = local_port.into(); - let req = protocol::Request::mapping(nonce, local_port, local_ip, None, None, 0); + let req = protocol::Request::mapping(nonce, protocol, local_port, local_ip, None, None, 0); socket.send(&req.encode()).await.context(IoSnafu)?; diff --git a/portmapper/src/pcp/protocol/opcode_data.rs b/portmapper/src/pcp/protocol/opcode_data.rs index adaf60c3..5f80305b 100644 --- a/portmapper/src/pcp/protocol/opcode_data.rs +++ b/portmapper/src/pcp/protocol/opcode_data.rs @@ -37,6 +37,7 @@ pub struct MapData { #[repr(u8)] pub enum MapProtocol { Udp = 17, + Tcp = 6, } /// Generic error returned when decoding [`OpcodeData`] fails. diff --git a/portmapper/src/pcp/protocol/request.rs b/portmapper/src/pcp/protocol/request.rs index 4ed35456..30dd3e24 100644 --- a/portmapper/src/pcp/protocol/request.rs +++ b/portmapper/src/pcp/protocol/request.rs @@ -76,6 +76,7 @@ impl Request { /// Create a mapping request. pub fn mapping( nonce: [u8; 12], + protocol: MapProtocol, local_port: u16, local_ip: Ipv4Addr, preferred_external_port: Option, @@ -88,7 +89,7 @@ impl Request { client_addr: local_ip.to_ipv6_mapped(), opcode_data: OpcodeData::MapData(MapData { nonce, - protocol: MapProtocol::Udp, + protocol, local_port, // if the pcp client does not know the external port, or does not have a // preference, it must use 0. From e84c5ae9dd97d88d06fa1b808405bddb8960a94b Mon Sep 17 00:00:00 2001 From: James Parker Date: Mon, 11 Aug 2025 23:55:21 -0400 Subject: [PATCH 4/6] Add TCP for upnp --- portmapper/src/lib.rs | 2 +- portmapper/src/mapping.rs | 3 ++- portmapper/src/upnp.rs | 19 +++++++++++++++---- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/portmapper/src/lib.rs b/portmapper/src/lib.rs index e27e7c99..72751dd3 100644 --- a/portmapper/src/lib.rs +++ b/portmapper/src/lib.rs @@ -673,7 +673,7 @@ impl Service { .last_upnp_gateway_addr .as_ref() .map(|(gateway, _last_seen)| gateway.clone()); - let task = mapping::Mapping::new_upnp(local_ip, local_port, gateway, external_port); + let task = mapping::Mapping::new_upnp(protocol, local_ip, local_port, gateway, external_port); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("upnp")), diff --git a/portmapper/src/mapping.rs b/portmapper/src/mapping.rs index f2626dca..5abd3e30 100644 --- a/portmapper/src/mapping.rs +++ b/portmapper/src/mapping.rs @@ -79,12 +79,13 @@ impl Mapping { /// Create a new UPnP mapping. pub(crate) async fn new_upnp( + protocol: Protocol, local_ip: Ipv4Addr, local_port: NonZeroU16, gateway: Option, external_port: Option, ) -> Result { - upnp::Mapping::new(local_ip, local_port, gateway, external_port) + upnp::Mapping::new(protocol, local_ip, local_port, gateway, external_port) .await .map(Self::Upnp) .context(UpnpSnafu) diff --git a/portmapper/src/upnp.rs b/portmapper/src/upnp.rs index 3c4db560..349d0e6b 100644 --- a/portmapper/src/upnp.rs +++ b/portmapper/src/upnp.rs @@ -14,7 +14,7 @@ use super::Metrics; pub type Gateway = aigd::Gateway; -use crate::defaults::UPNP_SEARCH_TIMEOUT as SEARCH_TIMEOUT; +use crate::{defaults::UPNP_SEARCH_TIMEOUT as SEARCH_TIMEOUT, Protocol}; /// Seconds we ask the router to maintain the port mapping. 0 means infinite. const PORT_MAPPING_LEASE_DURATION_SECONDS: u32 = 0; @@ -28,6 +28,8 @@ const PORT_MAPPING_DESCRIPTION: &str = "iroh-portmap"; #[derive(derive_more::Debug, Clone)] pub struct Mapping { + /// Protocol for this mapping. + protocol: igd_next::PortMappingProtocol, /// The internet Gateway device (router) used to create this mapping. #[debug("{}", gateway)] gateway: Gateway, @@ -62,6 +64,7 @@ pub enum Error { impl Mapping { pub(crate) async fn new( + protocol: Protocol, local_addr: Ipv4Addr, port: NonZeroU16, gateway: Option, @@ -97,12 +100,17 @@ impl Mapping { return Err(NotIpv4Snafu.build()); }; + let protocol = match protocol { + Protocol::Udp => igd_next::PortMappingProtocol::UDP, + Protocol::Tcp => igd_next::PortMappingProtocol::TCP, + }; + // if we are trying to get a specific external port, try this first. If this fails, default // to try to get any port if let Some(external_port) = preferred_port { if gateway .add_port( - igd_next::PortMappingProtocol::UDP, + protocol, external_port.into(), local_addr.into(), PORT_MAPPING_LEASE_DURATION_SECONDS, @@ -112,6 +120,7 @@ impl Mapping { .is_ok() { return Ok(Mapping { + protocol, gateway, external_ip, external_port, @@ -121,7 +130,7 @@ impl Mapping { let external_port = gateway .add_any_port( - igd_next::PortMappingProtocol::UDP, + protocol, local_addr.into(), PORT_MAPPING_LEASE_DURATION_SECONDS, PORT_MAPPING_DESCRIPTION, @@ -132,6 +141,7 @@ impl Mapping { .map_err(|_| ZeroExternalPortSnafu.build())?; Ok(Mapping { + protocol, gateway, external_ip, external_port, @@ -147,10 +157,11 @@ impl Mapping { let Mapping { gateway, external_port, + protocol, .. } = self; gateway - .remove_port(igd_next::PortMappingProtocol::UDP, external_port.into()) + .remove_port(protocol, external_port.into()) .await .context(RemovePortSnafu)?; Ok(()) From 259622c7761ea56204865fe0a34633ee410e2f8e Mon Sep 17 00:00:00 2001 From: James Parker Date: Fri, 15 Aug 2025 16:20:47 -0400 Subject: [PATCH 5/6] Format --- portmapper/src/lib.rs | 42 +++++++++++++++++---- portmapper/src/nat_pmp.rs | 2 +- portmapper/src/nat_pmp/protocol/request.rs | 10 ++--- portmapper/src/nat_pmp/protocol/response.rs | 38 +++++++++---------- portmapper/src/pcp.rs | 2 +- portmapper/src/upnp.rs | 2 +- 6 files changed, 58 insertions(+), 38 deletions(-) diff --git a/portmapper/src/lib.rs b/portmapper/src/lib.rs index 72751dd3..64b9a76b 100644 --- a/portmapper/src/lib.rs +++ b/portmapper/src/lib.rs @@ -654,14 +654,25 @@ impl Service { // nat_pmp self.mapping_task = if pcp { // try pcp if available first - let task = mapping::Mapping::new_pcp(protocol, local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_pcp( + protocol, + local_ip, + local_port, + gateway, + external_addr, + ); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pcp")), ))) } else if nat_pmp { // next nat_pmp if available - let task = - mapping::Mapping::new_nat_pmp(protocol, local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_nat_pmp( + protocol, + local_ip, + local_port, + gateway, + external_addr, + ); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pmp")), ))) @@ -673,7 +684,13 @@ impl Service { .last_upnp_gateway_addr .as_ref() .map(|(gateway, _last_seen)| gateway.clone()); - let task = mapping::Mapping::new_upnp(protocol, local_ip, local_port, gateway, external_port); + let task = mapping::Mapping::new_upnp( + protocol, + local_ip, + local_port, + gateway, + external_port, + ); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("upnp")), @@ -681,15 +698,26 @@ impl Service { } else if !recently_probed && self.config.enable_pcp { // if no service is available and the default fallback (upnp) is disabled, try pcp // first - let task = mapping::Mapping::new_pcp(protocol, local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_pcp( + protocol, + local_ip, + local_port, + gateway, + external_addr, + ); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pcp")), ))) } else if !recently_probed && self.config.enable_nat_pmp { // finally try nat_pmp if enabled - let task = - mapping::Mapping::new_nat_pmp(protocol, local_ip, local_port, gateway, external_addr); + let task = mapping::Mapping::new_nat_pmp( + protocol, + local_ip, + local_port, + gateway, + external_addr, + ); Some(AbortOnDropHandle::new(tokio::spawn( task.instrument(info_span!("pmp")), ))) diff --git a/portmapper/src/nat_pmp.rs b/portmapper/src/nat_pmp.rs index f131eee4..020826e5 100644 --- a/portmapper/src/nat_pmp.rs +++ b/portmapper/src/nat_pmp.rs @@ -8,7 +8,7 @@ use snafu::{Backtrace, Snafu}; use tracing::{debug, trace}; use self::protocol::{MapProtocol, Request, Response}; -use crate::{defaults::NAT_PMP_RECV_TIMEOUT as RECV_TIMEOUT, Protocol}; +use crate::{Protocol, defaults::NAT_PMP_RECV_TIMEOUT as RECV_TIMEOUT}; mod protocol; diff --git a/portmapper/src/nat_pmp/protocol/request.rs b/portmapper/src/nat_pmp/protocol/request.rs index af20e198..8e688415 100644 --- a/portmapper/src/nat_pmp/protocol/request.rs +++ b/portmapper/src/nat_pmp/protocol/request.rs @@ -76,7 +76,7 @@ impl Request { local_port: rng.random(), external_port: rng.random(), lifetime_seconds: rng.random(), - } + }, } } @@ -108,12 +108,8 @@ impl Request { // check if this is a mapping request, or an external address request match opcode { Opcode::DetermineExternalAddress => Request::ExternalAddress, - Opcode::MapUdp => { - decode_map(buf, MapProtocol::Udp) - } - Opcode::MapTcp => { - decode_map(buf, MapProtocol::Tcp) - } + Opcode::MapUdp => decode_map(buf, MapProtocol::Udp), + Opcode::MapTcp => decode_map(buf, MapProtocol::Tcp), } } } diff --git a/portmapper/src/nat_pmp/protocol/response.rs b/portmapper/src/nat_pmp/protocol/response.rs index bd31e03a..271982a6 100644 --- a/portmapper/src/nat_pmp/protocol/response.rs +++ b/portmapper/src/nat_pmp/protocol/response.rs @@ -144,25 +144,25 @@ impl Response { }?; fn decode_map(buf: &[u8], proto: MapProtocol) -> Response { - let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); - let epoch_time = u32::from_be_bytes(epoch_bytes); + let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); + let epoch_time = u32::from_be_bytes(epoch_bytes); - let private_port_bytes = buf[8..10].try_into().expect("slice has the right len"); - let private_port = u16::from_be_bytes(private_port_bytes); + let private_port_bytes = buf[8..10].try_into().expect("slice has the right len"); + let private_port = u16::from_be_bytes(private_port_bytes); - let external_port_bytes = buf[10..12].try_into().expect("slice has the right len"); - let external_port = u16::from_be_bytes(external_port_bytes); + let external_port_bytes = buf[10..12].try_into().expect("slice has the right len"); + let external_port = u16::from_be_bytes(external_port_bytes); - let lifetime_bytes = buf[12..16].try_into().expect("slice has the right len"); - let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); + let lifetime_bytes = buf[12..16].try_into().expect("slice has the right len"); + let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); - Response::PortMap { - proto, - epoch_time, - private_port, - external_port, - lifetime_seconds, - } + Response::PortMap { + proto, + epoch_time, + private_port, + external_port, + lifetime_seconds, + } } let response = match opcode { @@ -175,12 +175,8 @@ impl Response { public_ip: ip_bytes.into(), } } - Opcode::MapUdp => { - decode_map(buf, MapProtocol::Udp) - } - Opcode::MapTcp => { - decode_map(buf, MapProtocol::Tcp) - } + Opcode::MapUdp => decode_map(buf, MapProtocol::Udp), + Opcode::MapTcp => decode_map(buf, MapProtocol::Tcp), }; Ok(response) diff --git a/portmapper/src/pcp.rs b/portmapper/src/pcp.rs index b9074c41..5d762c3a 100644 --- a/portmapper/src/pcp.rs +++ b/portmapper/src/pcp.rs @@ -8,7 +8,7 @@ use rand::RngCore; use snafu::{Backtrace, ResultExt, Snafu}; use tracing::{debug, trace}; -use crate::{defaults::PCP_RECV_TIMEOUT as RECV_TIMEOUT, Protocol}; +use crate::{Protocol, defaults::PCP_RECV_TIMEOUT as RECV_TIMEOUT}; mod protocol; diff --git a/portmapper/src/upnp.rs b/portmapper/src/upnp.rs index 349d0e6b..eec16ce3 100644 --- a/portmapper/src/upnp.rs +++ b/portmapper/src/upnp.rs @@ -14,7 +14,7 @@ use super::Metrics; pub type Gateway = aigd::Gateway; -use crate::{defaults::UPNP_SEARCH_TIMEOUT as SEARCH_TIMEOUT, Protocol}; +use crate::{Protocol, defaults::UPNP_SEARCH_TIMEOUT as SEARCH_TIMEOUT}; /// Seconds we ask the router to maintain the port mapping. 0 means infinite. const PORT_MAPPING_LEASE_DURATION_SECONDS: u32 = 0; From 2b647f9991a79e094afb8627ba74e47d29e16d07 Mon Sep 17 00:00:00 2001 From: James Parker Date: Mon, 18 Aug 2025 02:07:37 -0400 Subject: [PATCH 6/6] Address feedback - Extract functions to decode map requests - Check that the response protocol matches requested protocol --- portmapper/src/mapping.rs | 3 +- portmapper/src/nat_pmp.rs | 6 ++- portmapper/src/nat_pmp/protocol/request.rs | 46 ++++++++++--------- portmapper/src/nat_pmp/protocol/response.rs | 49 +++++++++++---------- 4 files changed, 54 insertions(+), 50 deletions(-) diff --git a/portmapper/src/mapping.rs b/portmapper/src/mapping.rs index 5abd3e30..91a58789 100644 --- a/portmapper/src/mapping.rs +++ b/portmapper/src/mapping.rs @@ -5,9 +5,8 @@ use std::{net::Ipv4Addr, num::NonZeroU16, time::Duration}; use nested_enum_utils::common_fields; use snafu::{Backtrace, ResultExt, Snafu}; -use crate::Protocol; - use super::{nat_pmp, pcp, upnp}; +use crate::Protocol; pub(super) trait PortMapped: std::fmt::Debug + Unpin { fn external(&self) -> (Ipv4Addr, NonZeroU16); diff --git a/portmapper/src/nat_pmp.rs b/portmapper/src/nat_pmp.rs index 020826e5..fe7847c2 100644 --- a/portmapper/src/nat_pmp.rs +++ b/portmapper/src/nat_pmp.rs @@ -97,12 +97,14 @@ impl Mapping { let (external_port, lifetime_seconds) = match response { Response::PortMap { - proto: _, + proto: proto_rcvd, epoch_time: _, private_port, external_port, lifetime_seconds, - } if private_port == Into::::into(local_port) => (external_port, lifetime_seconds), + } if private_port == Into::::into(local_port) && proto == proto_rcvd => { + (external_port, lifetime_seconds) + } _ => return Err(UnexpectedServerResponseSnafu.build()), }; diff --git a/portmapper/src/nat_pmp/protocol/request.rs b/portmapper/src/nat_pmp/protocol/request.rs index 8e688415..bb8a58b8 100644 --- a/portmapper/src/nat_pmp/protocol/request.rs +++ b/portmapper/src/nat_pmp/protocol/request.rs @@ -81,35 +81,37 @@ impl Request { } #[cfg(test)] - #[track_caller] - fn decode(buf: &[u8]) -> Self { - fn decode_map(buf: &[u8], proto: MapProtocol) -> Request { - // buf[2] reserved - // buf[3] reserved - - let local_port_bytes = buf[4..6].try_into().expect("slice has the right size"); - let local_port = u16::from_be_bytes(local_port_bytes); - - let external_port_bytes = buf[6..8].try_into().expect("slice has the right size"); - let external_port = u16::from_be_bytes(external_port_bytes); - - let lifetime_bytes: [u8; 4] = buf[8..12].try_into().unwrap(); - let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); - Request::Mapping { - proto, - local_port, - external_port, - lifetime_seconds, - } + /// Decode a map request. + fn decode_map(buf: &[u8], proto: MapProtocol) -> Request { + // buf[2] reserved + // buf[3] reserved + + let local_port_bytes = buf[4..6].try_into().expect("slice has the right size"); + let local_port = u16::from_be_bytes(local_port_bytes); + + let external_port_bytes = buf[6..8].try_into().expect("slice has the right size"); + let external_port = u16::from_be_bytes(external_port_bytes); + + let lifetime_bytes: [u8; 4] = buf[8..12].try_into().unwrap(); + let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); + Request::Mapping { + proto, + local_port, + external_port, + lifetime_seconds, } + } + #[cfg(test)] + #[track_caller] + fn decode(buf: &[u8]) -> Self { let _version: Version = buf[0].try_into().unwrap(); let opcode: super::Opcode = buf[1].try_into().unwrap(); // check if this is a mapping request, or an external address request match opcode { Opcode::DetermineExternalAddress => Request::ExternalAddress, - Opcode::MapUdp => decode_map(buf, MapProtocol::Udp), - Opcode::MapTcp => decode_map(buf, MapProtocol::Tcp), + Opcode::MapUdp => Self::decode_map(buf, MapProtocol::Udp), + Opcode::MapTcp => Self::decode_map(buf, MapProtocol::Tcp), } } } diff --git a/portmapper/src/nat_pmp/protocol/response.rs b/portmapper/src/nat_pmp/protocol/response.rs index 271982a6..2ea27e93 100644 --- a/portmapper/src/nat_pmp/protocol/response.rs +++ b/portmapper/src/nat_pmp/protocol/response.rs @@ -114,6 +114,29 @@ impl Response { /// Indicator ORd into the [`Opcode`] to indicate a response packet. pub const RESPONSE_INDICATOR: u8 = 1u8 << 7; + /// Decode a map response. + fn decode_map(buf: &[u8], proto: MapProtocol) -> Response { + let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); + let epoch_time = u32::from_be_bytes(epoch_bytes); + + let private_port_bytes = buf[8..10].try_into().expect("slice has the right len"); + let private_port = u16::from_be_bytes(private_port_bytes); + + let external_port_bytes = buf[10..12].try_into().expect("slice has the right len"); + let external_port = u16::from_be_bytes(external_port_bytes); + + let lifetime_bytes = buf[12..16].try_into().expect("slice has the right len"); + let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); + + Response::PortMap { + proto, + epoch_time, + private_port, + external_port, + lifetime_seconds, + } + } + /// Decode a response. pub fn decode(buf: &[u8]) -> Result { if buf.len() < Self::MIN_SIZE || buf.len() > Self::MAX_SIZE { @@ -143,28 +166,6 @@ impl Response { ResultCode::UnsupportedOpcode => Err(UnsupportedOpcodeSnafu.build()), }?; - fn decode_map(buf: &[u8], proto: MapProtocol) -> Response { - let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); - let epoch_time = u32::from_be_bytes(epoch_bytes); - - let private_port_bytes = buf[8..10].try_into().expect("slice has the right len"); - let private_port = u16::from_be_bytes(private_port_bytes); - - let external_port_bytes = buf[10..12].try_into().expect("slice has the right len"); - let external_port = u16::from_be_bytes(external_port_bytes); - - let lifetime_bytes = buf[12..16].try_into().expect("slice has the right len"); - let lifetime_seconds = u32::from_be_bytes(lifetime_bytes); - - Response::PortMap { - proto, - epoch_time, - private_port, - external_port, - lifetime_seconds, - } - } - let response = match opcode { Opcode::DetermineExternalAddress => { let epoch_bytes = buf[4..8].try_into().expect("slice has the right len"); @@ -175,8 +176,8 @@ impl Response { public_ip: ip_bytes.into(), } } - Opcode::MapUdp => decode_map(buf, MapProtocol::Udp), - Opcode::MapTcp => decode_map(buf, MapProtocol::Tcp), + Opcode::MapUdp => Self::decode_map(buf, MapProtocol::Udp), + Opcode::MapTcp => Self::decode_map(buf, MapProtocol::Tcp), }; Ok(response)