Masquerading clean-ups

nat/src/lib.rs

@@ -43,8 +43,18 @@ struct NatTranslationData {
 }
 impl NatTranslationData {
     #[must_use]
-    pub(crate) fn new() -> Self {
-        Self::default()
+    pub(crate) fn new(
+        src_addr: Option<IpAddr>,
+        dst_addr: Option<IpAddr>,
+        src_port: Option<NatPort>,
+        dst_port: Option<NatPort>,
+    ) -> Self {
+        Self {
+            src_addr,
+            dst_addr,
+            src_port,
+            dst_port,
+        }
     }
     #[must_use]
     pub(crate) fn src_addr(mut self, address: IpAddr) -> Self {

nat/src/portfw/icmp_handling.rs

@@ -51,10 +51,10 @@ use tracing::debug;
 // Build `NatTranslationData` from `PortFwState` to translate the packet embedded in the ICMP error
 fn as_nat_translation(pfw_state: &PortFwState) -> NatTranslationData {
     match pfw_state.action {
-        PortFwAction::SrcNat => NatTranslationData::new()
+        PortFwAction::SrcNat => NatTranslationData::default()
             .dst_addr(pfw_state.use_ip().inner())
             .dst_port(NatPort::Port(pfw_state.use_port())),
-        PortFwAction::DstNat => NatTranslationData::new()
+        PortFwAction::DstNat => NatTranslationData::default()
             .src_addr(pfw_state.use_ip().inner())
             .src_port(NatPort::Port(pfw_state.use_port())),
     }

nat/src/stateful/allocator.rs

@@ -3,10 +3,12 @@
 
 //! NAT allocator trait: a trait to build allocators to manage IP addresses and ports for stateful NAT.
 
+use crate::NatPort;
 use crate::port::NatPortError;
 use net::ExtendedFlowKey;
 use net::ip::NextHeader;
 use std::fmt::{Debug, Display};
+use std::net::IpAddr;
 use std::time::Duration;
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, thiserror::Error)]
@@ -37,60 +39,40 @@ pub enum AllocatorError {
 
 /// `AllocationResult` is a struct to represent the result of an allocation.
 ///
-/// It contains the allocated IP addresses and ports for both source and destination NAT for the
-/// packet forwarded. In addition, it "reserves" IP addresses and ports for packets on the return
-/// path for this flow, and returns them so that the stateful NAT pipeline stage can update the flow
-/// table to prepare for the reply. It is necessary to "reserve" the IP and ports at this stage, to
-/// limit the risk of another flow accidentally getting the same resources assigned.
+/// It contains the allocated IP addresses and ports for source NAT for the packet forwarded. In
+/// addition, it "reserves" IP addresses and ports for packets on the return path for this flow, and
+/// returns them so that the stateful NAT pipeline stage can update the flow table to prepare for
+/// the reply. It is necessary to "reserve" the IP and ports at this stage, to limit the risk of
+/// another flow accidentally getting the same resources assigned.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct AllocationResult<T: Debug> {
     pub src: Option<T>,
-    pub dst: Option<T>,
-    pub return_src: Option<T>,
-    pub return_dst: Option<T>,
-    pub src_flow_idle_timeout: Option<Duration>,
-    pub dst_flow_idle_timeout: Option<Duration>,
+    pub return_dst: Option<(IpAddr, NatPort)>,
+    pub idle_timeout: Option<Duration>,
 }
 
 impl<T: Debug> AllocationResult<T> {
     /// Returns the idle timeout for the flow.
-    ///
-    /// # Returns
-    ///
-    /// * `Some(Duration)` if at least one of `src_flow_idle_timeout` or `dst_flow_idle_timeout` is set.
-    /// * `None` if both `src_flow_idle_timeout` and `dst_flow_idle_timeout` are `None`.
     #[must_use]
     pub fn idle_timeout(&self) -> Option<Duration> {
-        // Use the minimum of the two timeouts (source/destination).
-        //
-        // FIXME: We shouldn't use just one of the two timeouts, but doing otherwise will require
-        //        uncoupling entry creation for source and destination NAT.
-        match (self.src_flow_idle_timeout, self.dst_flow_idle_timeout) {
-            (Some(src), Some(dst)) => Some(src.min(dst)),
-            (Some(src), None) => Some(src),
-            (None, Some(dst)) => Some(dst),
-            // Given that at least one of alloc.src or alloc.dst is set, we should always have at
-            // least one timeout set.
-            (None, None) => None,
-        }
+        self.idle_timeout
     }
 }
 
 impl<T: Debug + Display> Display for AllocationResult<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
             f,
-            "src: {}, dst: {}, return_src: {}, return_dst: {}, src_flow_idle_timeout: {:?}, dst_flow_idle_timeout: {:?}",
+            "src: {}, return_dst: {}, idle_timeout: {:?}",
             self.src.as_ref().map_or("None".to_string(), T::to_string),
-            self.dst.as_ref().map_or("None".to_string(), T::to_string),
-            self.return_src
-                .as_ref()
-                .map_or("None".to_string(), T::to_string),
             self.return_dst
                 .as_ref()
-                .map_or("None".to_string(), T::to_string),
-            self.src_flow_idle_timeout,
-            self.dst_flow_idle_timeout
+                .map_or("None".to_string(), |(ip, port)| format!(
+                    "{}:{}",
+                    ip,
+                    port.as_u16()
+                )),
+            self.idle_timeout,
         )
     }
 }

nat/src/stateful/apalloc/display.rs

@@ -37,10 +37,6 @@ impl Display for NatDefaultAllocator {
         writeln!(with_indent!(f), "{}", self.pools_src44)?;
         writeln!(f, "source pools (IPv6):")?;
         writeln!(with_indent!(f), "{}", self.pools_src66)?;
-        writeln!(f, "destination pools (IPv4):")?;
-        writeln!(with_indent!(f), "{}", self.pools_dst44)?;
-        writeln!(f, "destination pools (IPv6):")?;
-        writeln!(with_indent!(f), "{}", self.pools_dst66)?;
         Ok(())
     }
 }

nat/src/stateful/apalloc/mod.rs

@@ -76,7 +76,7 @@ use net::packet::VpcDiscriminant;
 use net::{ExtendedFlowKey, FlowKey};
 use std::collections::BTreeMap;
 use std::fmt::Display;
-use std::net::{Ipv4Addr, Ipv6Addr};
+use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
 use tracing::error;
 
 mod alloc;
@@ -160,7 +160,6 @@ impl<I: NatIpWithBitmap, J: NatIpWithBitmap> PoolTable<I, J> {
 
 /// [`AllocatedIpPort`] is the public type for the object returned by our allocator.
 pub type AllocatedIpPort<I> = port_alloc::AllocatedPort<I>;
-type AllocationMapping<I> = (Option<AllocatedIpPort<I>>, Option<AllocatedIpPort<I>>);
 
 impl<I: NatIpWithBitmap> Display for AllocatedIpPort<I> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
@@ -180,9 +179,7 @@ impl<I: NatIpWithBitmap> Display for AllocatedIpPort<I> {
 #[derive(Debug)]
 pub struct NatDefaultAllocator {
     pools_src44: PoolTable<Ipv4Addr, Ipv4Addr>,
-    pools_dst44: PoolTable<Ipv4Addr, Ipv4Addr>,
     pools_src66: PoolTable<Ipv6Addr, Ipv6Addr>,
-    pools_dst66: PoolTable<Ipv6Addr, Ipv6Addr>,
     #[cfg(test)]
     disable_randomness: bool,
 }
@@ -191,9 +188,7 @@ impl NatAllocator<AllocatedIpPort<Ipv4Addr>, AllocatedIpPort<Ipv6Addr>> for NatD
     fn new() -> Self {
         Self {
             pools_src44: PoolTable::new(),
-            pools_dst44: PoolTable::new(),
             pools_src66: PoolTable::new(),
-            pools_dst66: PoolTable::new(),
             #[cfg(test)]
             disable_randomness: false,
         }
@@ -203,39 +198,30 @@ impl NatAllocator<AllocatedIpPort<Ipv4Addr>, AllocatedIpPort<Ipv6Addr>> for NatD
         &self,
         eflow_key: &ExtendedFlowKey,
     ) -> Result<AllocationResult<AllocatedIpPort<Ipv4Addr>>, AllocatorError> {
-        Self::allocate_from_tables(
-            eflow_key,
-            &self.pools_src44,
-            &self.pools_dst44,
-            self.must_disable_randomness(),
-        )
+        Self::allocate_from_tables(eflow_key, &self.pools_src44, self.must_disable_randomness())
     }
 
     fn allocate_v6(
         &self,
         eflow_key: &ExtendedFlowKey,
     ) -> Result<AllocationResult<AllocatedIpPort<Ipv6Addr>>, AllocatorError> {
-        Self::allocate_from_tables(
-            eflow_key,
-            &self.pools_src66,
-            &self.pools_dst66,
-            self.must_disable_randomness(),
-        )
+        Self::allocate_from_tables(eflow_key, &self.pools_src66, self.must_disable_randomness())
     }
 }
 
 impl NatDefaultAllocator {
     fn allocate_from_tables<I: NatIpWithBitmap>(
         eflow_key: &ExtendedFlowKey,
         pools_src: &PoolTable<I, I>,
-        pools_dst: &PoolTable<I, I>,
         disable_randomness: bool,
     ) -> Result<AllocationResult<AllocatedIpPort<I>>, AllocatorError> {
         // get flow key from extended flow key
         let flow_key = eflow_key.flow_key();
         let next_header = Self::get_next_header(flow_key);
         Self::check_proto(next_header)?;
-        let (src_vpc_id, dst_vpc_id) = Self::get_vpc_discriminants(eflow_key)?;
+        let dst_vpc_id = eflow_key
+            .dst_vpcd()
+            .ok_or(AllocatorError::MissingDiscriminant)?;
 
         // Get address pools for source
         let pool_src_opt = pools_src.get_entry(
@@ -261,52 +247,15 @@ impl NatDefaultAllocator {
             return Err(AllocatorError::Denied);
         }
 
-        // Get address pools for destination
-        let pool_dst_opt = pools_dst.get_entry(
-            next_header,
-            dst_vpc_id,
-            NatIp::try_from_addr(*flow_key.data().dst_ip()).map_err(|()| {
-                AllocatorError::InternalIssue(
-                    "Failed to convert IP address to Ipv4Addr".to_string(),
-                )
-            })?,
-        );
-
         // Allocate IP and ports from pools, for source and destination NAT
         let allow_null = matches!(flow_key.data().proto_key_info(), IpProtoKey::Icmp(_));
-        let (src_mapping, dst_mapping) =
-            Self::get_mapping(pool_src_opt, pool_dst_opt, allow_null, disable_randomness)?;
-
-        // Now based on the previous allocation, we need to "reserve" IP and ports for the reverse
-        // path for the flow. First retrieve the relevant address pools.
-
-        let reverse_pool_src_opt = if let Some(mapping) = &dst_mapping {
-            pools_src.get_entry(next_header, src_vpc_id, mapping.ip())
-        } else {
-            None
-        };
-
-        let reverse_pool_dst_opt = if let Some(mapping) = &src_mapping {
-            pools_dst.get_entry(next_header, src_vpc_id, mapping.ip())
-        } else {
-            None
-        };
-
-        // Reserve IP and ports for the reverse path for the flow.
-        let (reverse_src_mapping, reverse_dst_mapping) = Self::get_reverse_mapping(
-            flow_key,
-            reverse_pool_src_opt,
-            reverse_pool_dst_opt,
-            disable_randomness,
-        )?;
+        let src_mapping = Self::get_mapping(pool_src_opt, allow_null, disable_randomness)?;
+        let reverse_dst_mapping = Self::get_reverse_mapping(flow_key)?;
 
         Ok(AllocationResult {
             src: src_mapping,
-            dst: dst_mapping,
-            return_src: reverse_src_mapping,
             return_dst: reverse_dst_mapping,
-            src_flow_idle_timeout: pool_src_opt.and_then(IpAllocator::idle_timeout),
-            dst_flow_idle_timeout: pool_dst_opt.and_then(IpAllocator::idle_timeout),
+            idle_timeout: pool_src_opt.and_then(IpAllocator::idle_timeout),
         })
     }
 
@@ -335,27 +284,11 @@ impl NatDefaultAllocator {
         }
     }
 
-    fn get_vpc_discriminants(
-        eflow_key: &ExtendedFlowKey,
-    ) -> Result<(VpcDiscriminant, VpcDiscriminant), AllocatorError> {
-        let src_vpc_id = eflow_key
-            .flow_key()
-            .data()
-            .src_vpcd()
-            .ok_or(AllocatorError::MissingDiscriminant)?;
-
-        let dst_vpc_id = eflow_key
-            .dst_vpcd()
-            .ok_or(AllocatorError::MissingDiscriminant)?;
-        Ok((src_vpc_id, dst_vpc_id))
-    }
-
     fn get_mapping<I: NatIpWithBitmap>(
         pool_src_opt: Option<&alloc::IpAllocator<I>>,
-        pool_dst_opt: Option<&alloc::IpAllocator<I>>,
         allow_null: bool,
         disable_randomness: bool,
-    ) -> Result<AllocationMapping<I>, AllocatorError> {
+    ) -> Result<Option<AllocatedIpPort<I>>, AllocatorError> {
         // Allocate IP and ports for source and destination NAT.
         //
         // In the case of ICMP Query messages, use dst_mapping to hold an allocated identifier
@@ -372,69 +305,19 @@ impl NatDefaultAllocator {
             None => None,
         };
 
-        let dst_mapping = match pool_dst_opt {
-            Some(pool_dst) => Some(pool_dst.allocate(allow_null, disable_randomness)?),
-            None => None,
-        };
-
-        Ok((src_mapping, dst_mapping))
+        Ok(src_mapping)
     }
 
-    fn get_reverse_mapping<I: NatIpWithBitmap>(
+    fn get_reverse_mapping(
         flow_key: &FlowKey,
-        reverse_pool_src_opt: Option<&alloc::IpAllocator<I>>,
-        reverse_pool_dst_opt: Option<&alloc::IpAllocator<I>>,
-        disable_randomness: bool,
-    ) -> Result<AllocationMapping<I>, AllocatorError> {
-        let reverse_src_mapping = match reverse_pool_src_opt {
-            Some(pool_src) => {
-                let reservation_src_port_number = match flow_key.data().proto_key_info() {
-                    IpProtoKey::Tcp(tcp) => tcp.dst_port.into(),
-                    IpProtoKey::Udp(udp) => udp.dst_port.into(),
-                    // FIXME: We're doing a useless port reservation here, but without reserving a
-                    // "port" (or an ID for ICMP) we can't reserve an IP, given the current
-                    // architecture of the allocator. The ID will be overwritten by the ID for the
-                    // destination mapping. Note: this does not mean we're exhausting allocatable
-                    // identifiers sooner, because we allocate from a ports/identifier pool we don't
-                    // need.
-                    IpProtoKey::Icmp(icmp) => NatPort::Identifier(Self::get_icmp_query_id(icmp)?),
-                };
-
-                Some(pool_src.reserve(
-                    NatIp::try_from_addr(*flow_key.data().dst_ip()).map_err(|()| {
-                        AllocatorError::InternalIssue(
-                            "Failed to convert IP address to Ipv4Addr".to_string(),
-                        )
-                    })?,
-                    reservation_src_port_number,
-                    disable_randomness,
-                )?)
-            }
-            None => None,
+    ) -> Result<Option<(IpAddr, NatPort)>, AllocatorError> {
+        let reverse_target_ip = *flow_key.data().src_ip();
+        let reverse_target_port = match flow_key.data().proto_key_info() {
+            IpProtoKey::Tcp(tcp) => tcp.src_port.into(),
+            IpProtoKey::Udp(udp) => udp.src_port.into(),
+            IpProtoKey::Icmp(icmp) => NatPort::Identifier(Self::get_icmp_query_id(icmp)?),
         };
-
-        let reverse_dst_mapping = match reverse_pool_dst_opt {
-            Some(pool_dst) => {
-                let reservation_dst_port_number = match flow_key.data().proto_key_info() {
-                    IpProtoKey::Tcp(tcp) => tcp.src_port.into(),
-                    IpProtoKey::Udp(udp) => udp.src_port.into(),
-                    IpProtoKey::Icmp(icmp) => NatPort::Identifier(Self::get_icmp_query_id(icmp)?),
-                };
-
-                Some(pool_dst.reserve(
-                    NatIp::try_from_addr(*flow_key.data().src_ip()).map_err(|()| {
-                        AllocatorError::InternalIssue(
-                            "Failed to convert IP address to Ipv4Addr".to_string(),
-                        )
-                    })?,
-                    reservation_dst_port_number,
-                    disable_randomness,
-                )?)
-            }
-            None => None,
-        };
-
-        Ok((reverse_src_mapping, reverse_dst_mapping))
+        Ok(Some((reverse_target_ip, reverse_target_port)))
     }
 
     fn get_icmp_query_id(key: &IcmpProtoKey) -> Result<u16, AllocatorError> {