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 f5a74eed8..ffefbfbe7 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 @@ -930,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( @@ -1055,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( @@ -1068,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( @@ -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,21 +1135,39 @@ 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())); 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] @@ -1247,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(), @@ -1256,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)); @@ -1533,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(), @@ -1543,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)); @@ -1825,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(), @@ -1836,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));