From a9f08cdb4fa7e87b62feb174f1f51cf8b6ea21b7 Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Fri, 13 Mar 2026 17:19:03 +0000 Subject: [PATCH 1/2] test(flow-filter): Add helper to fake flow session info for packet Add a helper function to fake flow session information for a packet, setting the destination VPC, but using some mock-up data for stateful NAT or port forwarding context, because we don't need to read it in the tests (and setting the real data might involve sorting out circular dependencies). This is in prevision for the addition of more checks setting flow session information. Signed-off-by: Quentin Monnet --- flow-filter/src/tests.rs | 50 ++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index f5a74eed8..77b555992 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -179,6 +179,32 @@ fn create_test_icmp_v4_packet( packet } +fn fake_flow_session( + packet: &mut Packet, + dst_vpcd: VpcDiscriminant, + set_nat_state: bool, + set_port_fw_state: bool, +) { + // Create flow_info with dst_vpcd and NAT info and attach it to the packet + let flow_info = FlowInfo::new(std::time::Instant::now() + std::time::Duration::from_secs(60)); + let mut binding = flow_info.locked.write().unwrap(); + binding.dst_vpcd = Some(Box::new(dst_vpcd)); + if set_nat_state { + // Content should be a NatFlowState object but we can't include it in this crate without + // introducing a circular dependency; just use a bool, as we don't attempt to downcast + // it anyway. + binding.nat_state = Some(Box::new(true)); + } + if set_port_fw_state { + // Content should be a PortFwState object but we can't include it in this crate without + // introducing a circular dependency; just use a bool, as we don't attempt to downcast + // it anyway. + binding.port_fw_state = Some(Box::new(true)); + } + drop(binding); + packet.meta_mut().flow_info = Some(Arc::new(flow_info)); +} + #[test] fn test_flow_filter_packet_allowed() { // Setup table @@ -1094,17 +1120,7 @@ fn test_flow_filter_table_check_stateful_nat_plus_peer_forwarding() { 2000, 456, ); - // Create flow_info with dst_vpcd and stateful NAT info and attach it to the packet - let flow_info = FlowInfo::new(std::time::Instant::now() + std::time::Duration::from_secs(60)); - let mut binding = flow_info.locked.write().unwrap(); - binding.dst_vpcd = Some(Box::new(VpcDiscriminant::from(vni2))); - // Content should be a NatFlowState object but we can't include it in this crate without - // introducing a circular dependency; just use a bool, as we don't attempt to downcast it - // anyway. - binding.nat_state = Some(Box::new(true)); - drop(binding); - packet.meta_mut().flow_info = Some(Arc::new(flow_info)); - + fake_flow_session(&mut packet, vni2.into(), true, false); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vni2.into())); @@ -1119,17 +1135,7 @@ fn test_flow_filter_table_check_stateful_nat_plus_peer_forwarding() { 2000, 456, ); - // Create flow_info with dst_vpcd and port forwarding info and attach it to the packet - let flow_info = FlowInfo::new(std::time::Instant::now() + std::time::Duration::from_secs(60)); - let mut binding = flow_info.locked.write().unwrap(); - binding.dst_vpcd = Some(Box::new(VpcDiscriminant::from(vni2))); - // Content should be a PortFwState object but we can't include it in this crate without - // introducing a circular dependency; just use a bool, as we don't attempt to downcast it - // anyway. - binding.port_fw_state = Some(Box::new(true)); - drop(binding); - packet.meta_mut().flow_info = Some(Arc::new(flow_info)); - + fake_flow_session(&mut packet, vni2.into(), false, true); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vni2.into())); From 954346ede1a68b6949ced1ea0d40e4f36990019e Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Fri, 13 Mar 2026 17:22:55 +0000 Subject: [PATCH 2/2] feat(flow-filter): Filter packets with invalid NAT requirements Some NAT requirements are not currently supported, including: - Masquerading for destination IP address, when the packet has no flow information attached - Port forwarding for the source IP address/port, when the packet has no flow information attached The flow-filter stage has visibility on these NAT requirements, and on the availability of flow session information for the packet. And yet, on non-ambiguous lookup results, it will let packets go through even if the NAT requirements are not valid. One consequence is that additional processing is required, because it falls down to the relevant NAT stages to check their context and dump the packet in that case. Another consequence is that, once a NAT stage eventually dumps the packet, it may do so for reasons that may not obvious when looking at the log. For example, we've observed logs such as: ERROR dp-worker-8 dataplane_nat::stateful::apalloc: 256: No address pool found for source address 10.50.2.2. Did we hit a bug when building the stateful NAT allocator? ERROR dp-worker-8 dataplane_nat::stateful: 513: stateful-NAT: Error processing packet: allocation failed: new NAT session creation denied These logs are not incorrect, in the sense that in the context of the stateful NAT stage, reaching that point might be a bug if we assumed that the packet did require to be NAT-ed. So in this commit, we add a check to the flow-filter stage to check the two cases described above, and to drop the packet with more helpful log information when we get invalid NAT requirements. There are two cases when we need to check these requirements: 1) When we find a single remote data information object from the flow-filter table lookup, to validate that the NAT requirements in that object are supported. 2) When we find multiple remote data information objects from the flow-filter table lookup, and filtering them on the L4 protocol leaves only a single one to use in deal_with_multiple_matches(). This situation is similar to the first case. 3) However, when we have two matching objects after filtering on the L4 protocol in deal_with_multiple_matches(), then the logic of the function already discards invalid combinations (we'll find no valid case and will return None at the end of the function), so no additional check is required on that branch. We also adjust and compensate the unit tests affected by the change. Signed-off-by: Quentin Monnet --- flow-filter/src/lib.rs | 51 ++++++++++++++++++++ flow-filter/src/tests.rs | 101 +++++++++++++++++++++++++++++++++------ 2 files changed, 138 insertions(+), 14 deletions(-) diff --git a/flow-filter/src/lib.rs b/flow-filter/src/lib.rs index 5a8b712ed..757cfef7d 100644 --- a/flow-filter/src/lib.rs +++ b/flow-filter/src/lib.rs @@ -102,6 +102,17 @@ impl FlowFilter { None } Some(VpcdLookupResult::Single(dst_data)) => { + // Check NAT requirements are sensible + if self + .check_nat_requirements(packet, &dst_data, true) + .is_err() + { + debug!( + "{nfi}: Invalid NAT requirements found for flow {tuple}, dropping packet" + ); + packet.done(DoneReason::Filtered); + return; + } Self::set_nat_requirements(packet, &dst_data); Some(dst_data.vpcd) } @@ -270,6 +281,14 @@ impl FlowFilter { // requirement. if data_set.len() == 1 { let dst_data = data_set.iter().next().unwrap_or_else(|| unreachable!()); + // Check NAT requirements are sensible - no need to check flow availability, we know we + // don't have an active flow if we reached that point. + if self + .check_nat_requirements(packet, dst_data, false) + .is_err() + { + return None; + } Self::set_nat_requirements(packet, dst_data); return Some(first_vpcd); } @@ -313,6 +332,38 @@ impl FlowFilter { None } + /// Check if the packet has valid NAT requirements. + fn check_nat_requirements( + &self, + packet: &Packet, + dst_data: &RemoteData, + needs_flow_verif: bool, + ) -> Result<(), ()> { + if needs_flow_verif && packet.active_flow_info().is_some() { + return Ok(()); + } + + // We have no valid flow table entry for the packet: in this case, some NAT requirements are + // not supported. + let nfi = &self.name; + if matches!(dst_data.dst_nat_req, Some(NatRequirement::Stateful)) { + debug!( + "{nfi}: Packet requires destination NAT with masquerade, but packet does not contain flow-info" + ); + return Err(()); + } + if matches!( + dst_data.src_nat_req, + Some(NatRequirement::PortForwarding(_)) + ) { + debug!( + "{nfi}: Packet requires source NAT with port forwarding, but packet does not contain flow-info" + ); + return Err(()); + } + Ok(()) + } + /// Set NAT requirements on the packet based on the remote data object. fn set_nat_requirements(packet: &mut Packet, data: &RemoteData) { if data.requires_stateful_nat() { diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 77b555992..ffefbfbe7 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -956,9 +956,9 @@ fn test_flow_filter_table_check_nat_requirements() { "70.0.0.10".parse().unwrap(), ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); - assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); - assert_eq!(packet_out.meta().dst_vpcd, Some(vpcd(vni2.into()))); - assert!(needs_masquerade(&packet_out)); + // These are invalid NAT requirements because we cannot currently initiate a connection towards + // an expose using masquerading, and here there is no flow info attached to packet. + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); // src: stateful NAT, dst: default (no NAT) let packet = create_test_packet( @@ -1081,9 +1081,9 @@ fn test_flow_filter_table_check_stateful_nat_plus_peer_forwarding() { 2000, ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); - assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); - assert_eq!(packet_out.meta().dst_vpcd, Some(vni1.into())); - assert!(needs_masquerade(&packet_out)); + // We don't have a flow-info for the packet, and cannot initiate the connection towards an + // expose using stateful NAT. + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); // VPC 2 to VPC 1, outside of port forwarding port range: reverse stateful NAT let packet = create_test_ipv4_udp_packet_with_ports( @@ -1094,9 +1094,9 @@ fn test_flow_filter_table_check_stateful_nat_plus_peer_forwarding() { 123, ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); - assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); - assert_eq!(packet_out.meta().dst_vpcd, Some(vni1.into())); - assert!(needs_masquerade(&packet_out)); + // We don't have a flow-info for the packet, and cannot initiate the connection towards an + // expose using stateful NAT. + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); // VPC 2 to VPC 1, inside of port forwarding range: port forwarding let packet = create_test_ipv4_udp_packet_with_ports( @@ -1140,6 +1140,34 @@ fn test_flow_filter_table_check_stateful_nat_plus_peer_forwarding() { assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vni2.into())); assert!(needs_port_forwarding(&packet_out)); + + // VPC 2 to VPC 1, outside of port forwarding port range, with flow_info attached for stateful NAT: reverse stateful NAT + let mut packet = create_test_ipv4_udp_packet_with_ports( + Some(vni2.into()), + "5.0.0.10".parse().unwrap(), + "100.0.0.27".parse().unwrap(), + 456, + 123, + ); + fake_flow_session(&mut packet, vni1.into(), true, false); + let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); + assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); + assert_eq!(packet_out.meta().dst_vpcd, Some(vni1.into())); + assert!(needs_masquerade(&packet_out)); + + // VPC 2 to VPC 1, outside of port forwarding IP range, with flow_info attached for stateful NAT: reverse stateful NAT + let mut packet = create_test_ipv4_udp_packet_with_ports( + Some(vni2.into()), + "5.0.0.10".parse().unwrap(), + "100.0.0.4".parse().unwrap(), + 456, + 2000, + ); + fake_flow_session(&mut packet, vni1.into(), true, false); + let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); + assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); + assert_eq!(packet_out.meta().dst_vpcd, Some(vni1.into())); + assert!(needs_masquerade(&packet_out)); } #[test] @@ -1253,7 +1281,7 @@ fn test_flow_filter_protocol_aware_port_forwarding() { assert!(needs_port_forwarding(&packet_out)); // UDP packet inside port forwarding range: TCP-only port forwarding is filtered out, - // only stateful NAT remains -> stateful NAT (not dropped!) + // only stateful NAT remains (destination NAT), with no flow table entry -> drop packet let packet = create_test_ipv4_udp_packet_with_ports( Some(vni2.into()), "5.0.0.10".parse().unwrap(), @@ -1262,6 +1290,19 @@ fn test_flow_filter_protocol_aware_port_forwarding() { 3000, ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); + + // UDP packet inside port forwarding range: TCP-only port forwarding is filtered out, + // only stateful NAT remains, with flow table entry -> stateful NAT (not dropped!) + let mut packet = create_test_ipv4_udp_packet_with_ports( + Some(vni2.into()), + "5.0.0.10".parse().unwrap(), + "100.0.0.27".parse().unwrap(), + 456, + 3000, + ); + fake_flow_session(&mut packet, vni1.into(), true, false); + let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vni1.into())); assert!(needs_masquerade(&packet_out)); @@ -1539,8 +1580,9 @@ fn test_flow_filter_table_from_overlay_masquerade_port_forwarding_private_ips_ov // VPC-2 -> VPC-1: 192.168.90.100:22 -> 192.168.50.7:6789 // - // Must use port forwarding even though we have overlap on source IP/port with masquerading - // rule, because of unambiguous destination + // Overlap on source IP/port with masquerading rule, the destination is unambiguous and we find + // a source NAT port forwarding requirement, but there's no associated flow table entry and we + // cannot initiate a port forwarding session on the source side, so we drop let packet = create_test_ipv4_tcp_packet_with_ports( Some(vpcd(vni2.into())), "192.168.90.100".parse().unwrap(), @@ -1549,6 +1591,22 @@ fn test_flow_filter_table_from_overlay_masquerade_port_forwarding_private_ips_ov 6789, ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); + + // VPC-2 -> VPC-1: 192.168.90.100:22 -> 192.168.50.7:6789 + // + // Must use port forwarding even though we have overlap on source IP/port with masquerading + // rule, because of unambiguous destination, and we have a flow table entry so source-side port + // forwarding is allowed + let mut packet = create_test_ipv4_tcp_packet_with_ports( + Some(vpcd(vni2.into())), + "192.168.90.100".parse().unwrap(), + "192.168.50.7".parse().unwrap(), + 22, + 6789, + ); + fake_flow_session(&mut packet, vni1.into(), false, true); + let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vpcd(vni1.into()))); assert!(needs_port_forwarding(&packet_out)); @@ -1831,9 +1889,10 @@ fn test_flow_filter_table_from_overlay_masquerade_port_forwarding_private_ips_ov // Test with packets - // VPC-1 -> VPC-2, outside of port forwarding range + // VPC-1 -> VPC-2, outside of port forwarding range, no flow info attached // - // Only masquerading applies. + // Only masquerading applies but we cannot initiate the connection towards the expose using + // masquerading. let packet = create_test_ipv4_tcp_packet_with_ports( Some(vpcd(vni1.into())), "7.7.7.7".parse().unwrap(), @@ -1842,6 +1901,20 @@ fn test_flow_filter_table_from_overlay_masquerade_port_forwarding_private_ips_ov 5678, ); let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); + assert_eq!(packet_out.get_done(), Some(DoneReason::Filtered)); + + // VPC-1 -> VPC-2, outside of port forwarding range, with flow info attached + // + // Only masquerading applies. + let mut packet = create_test_ipv4_tcp_packet_with_ports( + Some(vpcd(vni1.into())), + "7.7.7.7".parse().unwrap(), + "10.0.0.1".parse().unwrap(), + 1234, + 5678, + ); + fake_flow_session(&mut packet, vni2.into(), true, false); + let packet_out = flow_filter.process([packet].into_iter()).next().unwrap(); assert!(!packet_out.is_done(), "{:?}", packet_out.get_done()); assert_eq!(packet_out.meta().dst_vpcd, Some(vpcd(vni2.into()))); assert!(needs_masquerade(&packet_out));