From ac2357d978377291108befb40b9a3dcc07e8d6ac Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 16:20:09 -0600 Subject: [PATCH 1/8] test(flow-filter): carry the config oracle through to packet metadata The context property tests stop at a `LookupResult`. The step after it -- turning that result into the destination and NAT flags every downstream NF acts on -- had only hand-written coverage, so the mapping was never checked against a configuration nobody chose. `nf_metadata_matches_config_oracle` closes that hop. It runs generated packets through the real NF and compares the stamped metadata against a prediction derived from `context::fuzz::oracle_lookup` -- the same oracle the context suite uses, now `pub(crate)` so the config's meaning of a route stays stated in one place. Only the `LookupResult` -> metadata step is restated. Scope is flowless packets: bypass, invalidation and the reply paths are not functions of the configuration and are covered elsewhere. Verified by mutation: swapping the static src/dst flags, dropping the masquerade-destination check, attaching the flow key whenever NAT is stateful rather than only alongside static NAT, and dropping a source miss with the wrong `DoneReason` all fail the test. Along the way, two fixes the oracle forced out: `ManifestSpec::strip_nat` was over-strict. Config forbids stateful NAT only opposite *stateful* NAT -- `validate_nat_combinations` says outright that "no NAT or static NAT only is compatible with all other modes on the other side", and `static_nat_plus_masquerade_context` in this very file relies on it. The generator stripped *all* NAT from the far side, so masquerade-opposite-static-NAT peerings never appeared in any generated overlay. That is the only shape producing a route that needs both stateful and static NAT, which is the sole case where the NF retains a flow key -- so the flow-key path was unreachable to every property test in the crate. Downgrading to static NAT instead of flattening to plain makes it reachable: the new test now records ~54k flow-key routes per 30s run, and coverage counters assert it stays that way. The comment above `validate_nat_combinations` claimed it rejects "NAT (static or stateful)" on the far side, which contradicts the table and the code directly beneath it. That reading is the most likely source of the generator's mistake, so correct it. Also adds v6 UDP and ICMPv6 packet builders, so a v6 probe is realized as the packet it describes. ICMPv6 carries a different next header than ICMPv4; the probe is adjusted to match whatever the built packet really carries, so the oracle is never asked about a packet that could not exist. Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpc.rs | 5 +- flow-filter/src/context/fuzz.rs | 6 +- flow-filter/src/context/mod.rs | 4 +- flow-filter/src/fuzz_gen.rs | 31 ++--- flow-filter/src/test_utils.rs | 29 ++++ flow-filter/src/tests.rs | 213 ++++++++++++++++++++++++++++- 6 files changed, 267 insertions(+), 21 deletions(-) diff --git a/config/src/external/overlay/vpc.rs b/config/src/external/overlay/vpc.rs index a8665e8c0a..daac16ec88 100644 --- a/config/src/external/overlay/vpc.rs +++ b/config/src/external/overlay/vpc.rs @@ -151,8 +151,9 @@ impl ValidatedPeering { } fn validate_nat_combinations(&self) -> ConfigResult { - // If stateful NAT is set up on one side of the peering, we don't support NAT (static or - // stateful) on the other side. + // If stateful NAT is set up on one side of the peering, we don't support stateful NAT on + // the other side. Static NAT (and no NAT) opposite stateful is fine -- see the table + // below, which is what this actually enforces. let mut local_has_masquerading = false; let mut local_has_port_forwarding = false; for expose in self.local.valexp() { diff --git a/flow-filter/src/context/fuzz.rs b/flow-filter/src/context/fuzz.rs index 8d790713ae..0ac78ead0a 100644 --- a/flow-filter/src/context/fuzz.rs +++ b/flow-filter/src/context/fuzz.rs @@ -67,7 +67,11 @@ fn consider(best: &mut Option<(Precedence, T)>, precedence: Precedence, value } /// Answer a route lookup directly from the validated overlay. -fn oracle_lookup(overlay: &ValidatedOverlay, probe: &Probe) -> LookupResult { +/// +/// Exposed to the crate because the NF-level property test +/// (`tests::nf_metadata_matches_config_oracle`) predicts a packet's stamped metadata from this same +/// answer: the config's meaning of a route should be stated in exactly one place. +pub(crate) fn oracle_lookup(overlay: &ValidatedOverlay, probe: &Probe) -> LookupResult { let Some(src_vpc) = overlay .vpc_table() .values() diff --git a/flow-filter/src/context/mod.rs b/flow-filter/src/context/mod.rs index 88cc7c8300..a2bd241d81 100644 --- a/flow-filter/src/context/mod.rs +++ b/flow-filter/src/context/mod.rs @@ -9,8 +9,10 @@ use config::ConfigError; use config::external::overlay::ValidatedOverlay; mod display; +// Not private: the NF-level property test in `crate::tests` predicts a packet's stamped metadata +// from this module's config oracle, so that the config's meaning of a route is stated once. #[cfg(test)] -mod fuzz; +pub(crate) mod fuzz; mod tables; #[cfg(test)] mod tests; diff --git a/flow-filter/src/fuzz_gen.rs b/flow-filter/src/fuzz_gen.rs index 819ad985f3..e6fd8cca3e 100644 --- a/flow-filter/src/fuzz_gen.rs +++ b/flow-filter/src/fuzz_gen.rs @@ -95,10 +95,6 @@ impl ExposeSpec { !matches!(self, ExposeSpec::Plain | ExposeSpec::StaticNat) } - fn has_nat(self) -> bool { - !matches!(self, ExposeSpec::Plain) - } - /// Whether this expose gives the source side of a route an unconstrained, connection-initiating /// match: a plain / static-nat / masquerade private block (a `/24` or `/120` with no port /// constraint, `can_init_connection`). Port forwarding cannot initiate, so a pure @@ -147,14 +143,18 @@ impl ManifestSpec { self.expose_specs().any(ExposeSpec::is_stateful) } - fn has_nat(&self) -> bool { - self.expose_specs().any(ExposeSpec::has_nat) - } - - fn strip_nat(&mut self) { + /// Replace every stateful-NAT expose with a static-NAT one. + /// + /// Config forbids stateful NAT on *both* sides of a peering, but static NAT opposite stateful + /// is explicitly permitted (see `ValidatedPeering::validate_nat_combinations`: "no NAT or + /// static NAT only is compatible with all other modes on the other side"). Downgrading rather + /// than flattening to plain is what keeps masquerade-opposite-static-NAT peerings in the + /// generated population -- and those are the only ones that produce a route requiring both + /// stateful and static NAT, which is the sole case in which the NF retains a flow key. + fn strip_stateful_nat(&mut self) { for slot in self.exposes.iter_mut().flatten() { - if slot.has_nat() { - *slot = ExposeSpec::Plain; + if slot.is_stateful() { + *slot = ExposeSpec::StaticNat; } } } @@ -218,11 +218,10 @@ impl OverlaySpec { if peering.local.default && peering.remote.default { peering.remote.drop_default(); } - // Stateful NAT on one side of a peering forbids any NAT on the other side. - if peering.local.has_stateful() && peering.remote.has_nat() { - peering.remote.strip_nat(); - } else if peering.remote.has_stateful() && peering.local.has_nat() { - peering.local.strip_nat(); + // Stateful NAT on one side of a peering forbids stateful NAT on the other. Static NAT + // opposite stateful is legal, so only the stateful side is downgraded. + if peering.local.has_stateful() && peering.remote.has_stateful() { + peering.remote.strip_stateful_nat(); } } // Each VPC may see at most one default destination across all of its peerings. A default diff --git a/flow-filter/src/test_utils.rs b/flow-filter/src/test_utils.rs index c1e6513483..0ae5ea64c2 100644 --- a/flow-filter/src/test_utils.rs +++ b/flow-filter/src/test_utils.rs @@ -200,6 +200,35 @@ pub(crate) fn build_icmp_packet(src: Ipv4Addr, dst: Ipv4Addr) -> Headers { .unwrap() } +pub(crate) fn build_udp_packet_v6(src: Ipv6Addr, dst: Ipv6Addr, sport: u16, dport: u16) -> Headers { + HeaderStack::new() + .eth(|_| {}) + .ipv6(|ip| { + ip.set_source(UnicastIpv6Addr::new(src).unwrap()); + ip.set_destination(dst); + }) + .udp(|udp| { + udp.set_source(UdpPort::try_from(sport).unwrap()); + udp.set_destination(UdpPort::try_from(dport).unwrap()); + }) + .build_headers() + .unwrap() +} + +/// An ICMPv6 packet. Note the next header this carries is `NextHeader::ICMP6`, not the `ICMP` of +/// the v4 builder: a lookup key built for one will not match a packet built by the other. +pub(crate) fn build_icmp_packet_v6(src: Ipv6Addr, dst: Ipv6Addr) -> Headers { + HeaderStack::new() + .eth(|_| {}) + .ipv6(|ip| { + ip.set_source(UnicastIpv6Addr::new(src).unwrap()); + ip.set_destination(dst); + }) + .icmp6(|_| {}) + .build_headers() + .unwrap() +} + pub(crate) fn build_tcp_packet_v6(src: Ipv6Addr, dst: Ipv6Addr, sport: u16, dport: u16) -> Headers { HeaderStack::new() .eth(|_| {}) diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 2e2c84107b..0391e467b1 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -5,13 +5,14 @@ #![cfg(test)] -use crate::FlowFilter; use crate::context::{FlowFilterContext, FlowFilterContextWriter}; +use crate::fuzz_gen::Probe; use crate::test_utils::{ build_icmp_packet, build_nonip_packet, build_tcp_packet, build_tcp_packet_v6, build_udp_packet, context, expose, expose_masquerade, expose_port_forwarding, expose_static, peering, v4, v6, vpcd, }; +use crate::{FlowFilter, LookupResult, NatRequirement}; use concurrency::sync::Arc; use lpm::prefix::L4Protocol; use net::FlowKey; @@ -1061,3 +1062,213 @@ fn burst_processing_upholds_structural_invariants() { } }); } + +// ------------------------------------------------------------------------------------------------- +// End-to-end config oracle: from a generated configuration all the way to a packet's metadata. +// +// The context property tests stop at a `LookupResult`. The step after it -- turning that result +// into the destination and NAT flags the downstream NFs act on -- was covered only by hand-written +// examples, so the mapping was never checked against a configuration nobody chose. This closes +// that last hop: the route is predicted by the *same* config oracle the context suite uses (see +// `context::fuzz::oracle_lookup`), and only the `LookupResult` -> metadata step is restated here. +// +// Scope: packets with no attached flow. Everything a flow contributes -- bypass, invalidation, and +// the reply paths that let an established flow overrule a miss -- is deliberately excluded, since +// that logic is not a function of the configuration and is covered by +// `invalidation_decision_matches_spec` and the flow tests above. + +/// What the NF must leave on a flowless packet. +#[derive(Debug, PartialEq, Eq)] +enum NfOutcome { + /// Dropped for this reason, with no destination stamped. + Dropped(Option), + Routed { + dst_vpcd: Option, + masquerade: bool, + static_nat_src: bool, + static_nat_dst: bool, + port_forwarding: bool, + /// Whether the pre-translation flow key was retained. + flow_key: bool, + }, +} + +/// The outcome the configuration predicts, given the route it resolves to. +/// +/// Without a flow to vouch for it, every miss drops: a destination miss because no peering covers +/// the packet, a source miss because only an established port-forwarding flow could excuse one. A +/// masquerade destination drops for the same reason -- it cannot accept a new connection. +fn expected_outcome(result: LookupResult) -> NfOutcome { + let (dst_vpcd, dst_nat, src_nat) = match result { + LookupResult::Route(route) => route, + LookupResult::SourceMiss(_) | LookupResult::DestinationMiss => { + return NfOutcome::Dropped(Some(DoneReason::Filtered)); + } + }; + if dst_nat == Some(NatRequirement::Masquerade) { + return NfOutcome::Dropped(Some(DoneReason::Filtered)); + } + + let masquerade = src_nat == Some(NatRequirement::Masquerade); + let static_nat_src = src_nat == Some(NatRequirement::Static); + let static_nat_dst = dst_nat == Some(NatRequirement::Static); + let port_forwarding = src_nat == Some(NatRequirement::PortForwarding) + || dst_nat == Some(NatRequirement::PortForwarding); + NfOutcome::Routed { + dst_vpcd: Some(dst_vpcd), + masquerade, + static_nat_src, + static_nat_dst, + port_forwarding, + // Stateful NAT combined with static NAT is the one case that needs the original addresses + // kept, so the right flow-table entries can be built later. + flow_key: (masquerade || port_forwarding) && (static_nat_src || static_nat_dst), + } +} + +fn observed_outcome(pkt: &Packet) -> NfOutcome { + if pkt.is_done() { + return NfOutcome::Dropped(pkt.get_done()); + } + let meta = pkt.meta(); + NfOutcome::Routed { + dst_vpcd: meta.dst_vpcd, + masquerade: meta.requires_masquerade(), + static_nat_src: meta.requires_static_nat_src(), + static_nat_dst: meta.requires_static_nat_dst(), + port_forwarding: meta.requires_port_forwarding(), + flow_key: meta.flow_key.is_some(), + } +} + +/// Realize a probe as a real packet, together with the probe as that packet actually carries it. +/// +/// Not every probe is a packet. Source and destination must agree on IP version (the lookup treats +/// a mismatch as a destination miss, but no such packet exists on the wire), ports on the wire are +/// non-zero (0 is the wildcard the lookup substitutes for a portless packet), and ICMP over v6 is a +/// different next header than over v4. Where the packet cannot carry the probe verbatim, the probe +/// is adjusted to match -- so the oracle is always asked about the packet that was really built. +fn probe_packet(probe: &Probe) -> Option<(Packet, Probe)> { + use crate::test_utils::{build_icmp_packet_v6, build_udp_packet_v6}; + use net::ip::NextHeader; + + let mut probe = *probe; + if let Some((sport, dport)) = probe.ports.as_mut() { + *sport = (*sport).max(1); + *dport = (*dport).max(1); + } + + let headers = match (probe.src_ip, probe.dst_ip) { + (std::net::IpAddr::V4(src), std::net::IpAddr::V4(dst)) => match probe.ports { + Some((sp, dp)) if probe.proto == NextHeader::TCP => build_tcp_packet(src, dst, sp, dp), + Some((sp, dp)) if probe.proto == NextHeader::UDP => build_udp_packet(src, dst, sp, dp), + _ => { + probe.proto = NextHeader::ICMP; + probe.ports = None; + build_icmp_packet(src, dst) + } + }, + (std::net::IpAddr::V6(src), std::net::IpAddr::V6(dst)) => match probe.ports { + Some((sp, dp)) if probe.proto == NextHeader::TCP => { + build_tcp_packet_v6(src, dst, sp, dp) + } + Some((sp, dp)) if probe.proto == NextHeader::UDP => { + build_udp_packet_v6(src, dst, sp, dp) + } + _ => { + probe.proto = NextHeader::ICMP6; + probe.ports = None; + build_icmp_packet_v6(src, dst) + } + }, + _ => return None, + }; + Some((packet(Some(probe.src_vpcd), headers), probe)) +} + +/// The metadata the NF stamps on a flowless packet is exactly what the configuration predicts, for +/// every probe of every generated overlay. +/// +/// Coverage is asserted, not assumed: the counters fail the test if a time-boxed run never routed a +/// packet, never applied a NAT requirement, or never reached the flow-key case (which needs a route +/// requiring both stateful and static NAT, and so exists only on masquerade-opposite-static-NAT +/// peerings). +#[test] +fn nf_metadata_matches_config_oracle() { + use crate::context::fuzz::oracle_lookup; + use crate::fuzz_gen::{OverlaySpec, ProbeSpec}; + use concurrency::sync::LazyLock; + use concurrency::sync::atomic::{AtomicU64, Ordering}; + + // Lazily initialized so this compiles under the loom backend, whose AtomicU64::new is not const. + static ROUTED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static NATTED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static FLOW_KEYED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static DROPPED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; 8])>() + .for_each(|(overlay_spec, probe_specs)| { + // Built inside the closure: the filter holds a classifier, which is not unwind-safe and + // so cannot be captured across bolero's catch_unwind boundary. + let built = overlay_spec.build(); + let (mut flow_filter, _writer) = + make_flow_filter(FlowFilterContext::for_test(&built.overlay)); + + // The overlay's own derived probes route by construction, which is where the NAT flags + // actually get exercised; the generated probes supply the misses and the edges. + let derived = built.routing_probes.iter().copied(); + let generated = probe_specs.iter().map(|spec| spec.resolve(built.blocks)); + for probe in derived.chain(generated) { + let Some((pkt, probe)) = probe_packet(&probe) else { + continue; + }; + let expected = expected_outcome(oracle_lookup(&built.overlay, &probe)); + assert_eq!( + observed_outcome(&run(&mut flow_filter, pkt)), + expected, + "stamped metadata diverges from the configuration for {probe:?}\n\ + spec: {overlay_spec:?}", + ); + + match expected { + NfOutcome::Dropped(_) => { + DROPPED.fetch_add(1, Ordering::Relaxed); + } + NfOutcome::Routed { + masquerade, + static_nat_src, + static_nat_dst, + port_forwarding, + flow_key, + .. + } => { + ROUTED.fetch_add(1, Ordering::Relaxed); + if masquerade || static_nat_src || static_nat_dst || port_forwarding { + NATTED.fetch_add(1, Ordering::Relaxed); + } + if flow_key { + FLOW_KEYED.fetch_add(1, Ordering::Relaxed); + } + } + } + } + }); + + let (routed, natted) = ( + ROUTED.load(Ordering::Relaxed), + NATTED.load(Ordering::Relaxed), + ); + let (flow_keyed, dropped) = ( + FLOW_KEYED.load(Ordering::Relaxed), + DROPPED.load(Ordering::Relaxed), + ); + eprintln!( + "coverage: {routed} routed ({natted} with a NAT requirement, \ + {flow_keyed} retaining a flow key), {dropped} dropped" + ); + assert!(routed >= 1, "no packet was ever routed"); + assert!(natted >= 1, "no route ever carried a NAT requirement"); + assert!(flow_keyed >= 1, "the flow-key case was never reached"); + assert!(dropped >= 1, "no packet was ever dropped"); +} From 0f134140f222dea53b6c6d7e8496c7bfb4d9a745 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 16:50:48 -0600 Subject: [PATCH 2/8] test(flow-filter): fuzz the NF's parser-facing edge with arbitrary header stacks `classify` is the only place this NF reaches into a packet's headers, and every test above it fed exactly four hand-built shapes: v4 TCP, UDP, ICMP, and a bare Ethernet frame. No VLAN tag, no IPv6 extension chain, no fragment, no authentication header, and none of the fuzzed remainder of any header's fields. `net` already ships a bolero header-stack generator; nobody had pointed it at this NF. `arbitrary_header_stacks_uphold_the_config_contract` runs ten stack shapes through the real NF and asserts the stamped metadata equals what the config oracle predicts for the key that stack presents. Addresses are steered into the configured prefixes (a uniformly random address never lands in one, and the run would collapse into destination misses) while every other field stays fuzzed. What the test claims is bounded, and the doc comment says so: `probe_from_packet` extracts the key with the same accessors `classify` uses, so this is not an independent check of that extraction. Its value is that no generated stack panics, the fail-closed invariants hold on shapes nobody picked, and the route -> metadata mapping holds for keys only exotic stacks produce. Coverage counters insist those keys are reached: a typical 1s run sees ~1600 portless IP packets and ~4400 non-transport protocol numbers, neither reachable from the four hand-built shapes. One behaviour it turned up -- an IPv6 extension header masking the transport protocol -- is surprising enough to deserve stating rather than leaving for a reader to infer from a passing property test, so it is pinned separately alongside the other such edges. Requires net's "bolero" feature in dev-dependencies for the generator. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/Cargo.toml | 4 +- flow-filter/src/tests.rs | 326 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 329 insertions(+), 1 deletion(-) diff --git a/flow-filter/Cargo.toml b/flow-filter/Cargo.toml index 00c9063fec..7ce1bb62d8 100644 --- a/flow-filter/Cargo.toml +++ b/flow-filter/Cargo.toml @@ -32,4 +32,6 @@ acl = { workspace = true, features = ["reference"] } bolero = { workspace = true, features = ["std"] } dpdk = { workspace = true, features = ["test"] } lpm = { workspace = true, features = ["testing"] } -net = { workspace = true, features = ["builder"] } +# "bolero" exposes net's fuzz header-stack generator (net::headers::builder::ChainBase), which +# drives the adversarial-header suite against the NF's parser-facing edge. +net = { workspace = true, features = ["builder", "bolero"] } diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 0391e467b1..20ed8de274 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -1126,6 +1126,29 @@ fn expected_outcome(result: LookupResult) -> NfOutcome { } } +/// The lookup key a packet presents, as a [`Probe`] the config oracle can answer. +/// +/// Deliberately the same extraction `FlowFilter::classify` performs, through the same public +/// accessors: this is how a test asks "what did the NF see", not an independent re-derivation of +/// it. Returns `None` for a packet with no IP layer, which never reaches the tables at all. +fn probe_from_packet(pkt: &Packet, src_vpcd: VpcDiscriminant) -> Option { + use net::headers::{TryIp, TryTransport}; + use std::num::NonZero; + + let net = pkt.try_ip()?; + Some(Probe { + src_vpcd, + src_ip: net.src_addr(), + dst_ip: net.dst_addr(), + proto: net.next_header(), + ports: pkt.try_transport().and_then(|t| { + t.src_port() + .map(NonZero::get) + .zip(t.dst_port().map(NonZero::get)) + }), + }) +} + fn observed_outcome(pkt: &Packet) -> NfOutcome { if pkt.is_done() { return NfOutcome::Dropped(pkt.get_done()); @@ -1272,3 +1295,306 @@ fn nf_metadata_matches_config_oracle() { assert!(flow_keyed >= 1, "the flow-key case was never reached"); assert!(dropped >= 1, "no packet was ever dropped"); } + +// ------------------------------------------------------------------------------------------------- +// Adversarial header stacks. +// +// `classify` is this NF's parser-facing edge -- it is the only place that reaches into a packet's +// headers -- and every test above it feeds exactly four hand-built shapes (v4 TCP/UDP/ICMP and a +// bare Ethernet frame). Nothing exercised a VLAN tag, an IPv6 extension-header chain, a fragment, +// an authentication header, or the fuzzed remainder of any header's fields. +// +// `net` already ships a bolero header-stack generator; this points it at the NF. What the test can +// honestly claim is bounded: `probe_from_packet` extracts the lookup key with the same public +// accessors `classify` uses, so this does not independently verify that extraction. Its value is +// that (a) no header stack the generator can build makes the NF panic, (b) the fail-closed and +// burst-structural invariants hold on stacks nobody hand-picked, and (c) the route -> metadata +// mapping still holds for keys that *only* exotic stacks produce -- an extension-header protocol +// number, a portless IP packet, a fragment. Those keys are unreachable from the four hand-built +// shapes, and they are what the coverage counters below insist on reaching. + +mod adversarial_headers { + use super::{ + NfOutcome, expected_outcome, make_flow_filter, observed_outcome, probe_from_packet, + }; + use crate::context::FlowFilterContext; + use crate::context::fuzz::oracle_lookup; + use crate::test_utils::{expose, expose_masquerade, expose_static, overlay, peering, vpcd}; + use bolero::{Driver, ValueGenerator}; + use concurrency::sync::LazyLock; + use concurrency::sync::atomic::{AtomicU64, Ordering}; + use config::external::overlay::ValidatedOverlay; + use net::buffer::TestBuffer; + use net::headers::Headers; + use net::headers::builder::ChainBase; + use net::ip::NextHeader; + use net::ipv4::UnicastIpv4Addr; + use net::ipv6::UnicastIpv6Addr; + use net::packet::{DoneReason, Packet, VpcDiscriminant}; + use net::parse::DeParse; + use pipeline::NetworkFunction; + use std::net::{Ipv4Addr, Ipv6Addr}; + + /// The source VPC every generated packet claims. + fn src_vpcd() -> VpcDiscriminant { + vpcd(100) + } + + /// A two-peering overlay whose prefixes the generated stacks aim at: a v4 peering carrying one + /// expose of each source NAT mode, and a v6 peering so the v6 stacks have somewhere to land. + /// Both manifests of a peering are single-version, as validation requires. + fn wire_overlay() -> ValidatedOverlay { + overlay( + &[("vpc1", 100), ("vpc2", 200), ("vpc3", 300)], + vec![ + peering( + "vpc1-to-vpc2", + ( + "vpc1", + vec![ + expose("1.0.0.0/24"), + expose_static("2.0.0.0/24", "20.0.0.0/24"), + expose_masquerade("3.0.0.0/24", "30.0.0.0/24"), + ], + ), + ("vpc2", vec![expose("5.0.0.0/24")]), + ), + peering( + "vpc1-to-vpc3", + ("vpc1", vec![expose("2001:db8::/32")]), + ("vpc3", vec![expose("2001:db9::/32")]), + ), + ], + ) + } + + // Address pinning. Every other field of every header stays fuzzed; only the addresses are + // steered, because a uniformly random address never lands in a configured prefix and the whole + // run would collapse into destination misses. The fuzzed low byte is kept, so host selection + // (and with it the port-range and prefix-length edges) still comes from the driver. + + fn pin_v4(ip: &mut net::ipv4::Ipv4) { + let src = ip.source().inner().octets(); + // 1/2/3 are the plain, static-NAT and masquerade source exposes; 4 is covered by none. + let src_net = src[0] % 4 + 1; + ip.set_source( + UnicastIpv4Addr::new(Ipv4Addr::new(src_net, 0, 0, src[3])) + .unwrap_or_else(|e| unreachable!("pinned v4 source is unicast: {e:?}")), + ); + let dst = ip.destination().octets(); + // 5 is the peer's expose; 9 is nowhere. + let dst_net = if dst[0].is_multiple_of(4) { 9 } else { 5 }; + ip.set_destination(Ipv4Addr::new(dst_net, 0, 0, dst[3])); + } + + fn pin_v6(ip: &mut net::ipv6::Ipv6) { + let src = ip.source().inner().octets(); + ip.set_source( + UnicastIpv6Addr::new(Ipv6Addr::new( + 0x2001, + 0x0db8, + 0, + 0, + 0, + 0, + 0, + u16::from(src[15]), + )) + .unwrap_or_else(|e| unreachable!("pinned v6 source is unicast: {e:?}")), + ); + let dst = ip.destination().octets(); + // Half the destinations land in the peer's expose, half nowhere near it. + let net = if dst[0].is_multiple_of(4) { + 0x0dbf + } else { + 0x0db9 + }; + ip.set_destination(Ipv6Addr::new( + 0x2001, + net, + 0, + 0, + 0, + 0, + 0, + u16::from(dst[15]), + )); + } + + /// One header stack per shape. Each is its own concrete generator type (`ValueGenerator` has a + /// generic method and so is not object-safe), which is why this dispatches through a `match` + /// rather than a table of boxed generators. + struct AnyStack; + + impl ValueGenerator for AnyStack { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + match driver.produce::()? % 10 { + // No IP layer at all: the NotIp path. + 0 => ChainBase::new().eth(|_| {}).generate(driver), + 1 => ChainBase::new() + .eth(|_| {}) + .ipv4(pin_v4) + .tcp(|_| {}) + .generate(driver), + 2 => ChainBase::new() + .eth(|_| {}) + .ipv4(pin_v4) + .udp(|_| {}) + .generate(driver), + 3 => ChainBase::new() + .eth(|_| {}) + .ipv4(pin_v4) + .icmp4(|_| {}) + .generate(driver), + // A VLAN tag between the Ethernet and IP layers. + 4 => ChainBase::new() + .eth(|_| {}) + .vlan(|_| {}) + .ipv4(pin_v4) + .tcp(|_| {}) + .generate(driver), + // An IPv4 authentication header ahead of the transport. + 5 => ChainBase::new() + .eth(|_| {}) + .ipv4(pin_v4) + .ipv4_auth(|_| {}) + .tcp(|_| {}) + .generate(driver), + 6 => ChainBase::new() + .eth(|_| {}) + .ipv6(pin_v6) + .tcp(|_| {}) + .generate(driver), + 7 => ChainBase::new() + .eth(|_| {}) + .ipv6(pin_v6) + .udp(|_| {}) + .generate(driver), + // IPv6 extension-header chains, ahead of a transport header. + 8 => ChainBase::new() + .eth(|_| {}) + .ipv6(pin_v6) + .hop_by_hop(|_| {}) + .tcp(|_| {}) + .generate(driver), + 9 => ChainBase::new() + .eth(|_| {}) + .ipv6(pin_v6) + .fragment(|_| {}) + .udp(|_| {}) + .generate(driver), + _ => unreachable!("modulo 10"), + } + } + } + + /// Serialize generated headers into a parseable overlay packet. Returns `None` when the stack + /// cannot round-trip through a `TestBuffer` (too large, or not re-parseable) -- those are + /// counted, so a run that silently discarded everything cannot pass. + fn wire_packet(headers: &Headers) -> Option> { + let mut buffer = TestBuffer::new(); + headers.deparse(buffer.as_mut()).ok()?; + let mut packet = Packet::new(buffer).ok()?; + packet.meta_mut().set_overlay(true); + packet.meta_mut().src_vpcd = Some(src_vpcd()); + Some(packet) + } + + /// However exotic the headers, a flowless packet leaves the NF with exactly the metadata the + /// configuration predicts for the key that packet presents -- and a packet with no IP layer is + /// dropped as `NotIp` without ever reaching the tables. + #[test] + fn arbitrary_header_stacks_uphold_the_config_contract() { + // Lazily initialized so this compiles under the loom backend, whose AtomicU64::new is not + // const. + static UNPARSEABLE: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static NOT_IP: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static PORTLESS: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static EXOTIC_PROTO: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static ROUTED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static DROPPED: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + + let overlay = wire_overlay(); + + bolero::check!() + .with_generator(AnyStack) + .for_each(|headers: &Headers| { + // Built inside the closure: the filter holds a classifier, which is not unwind-safe + // and so cannot be captured across bolero's catch_unwind boundary. + let (mut flow_filter, _writer) = + make_flow_filter(FlowFilterContext::for_test(&overlay)); + + let Some(packet) = wire_packet(headers) else { + UNPARSEABLE.fetch_add(1, Ordering::Relaxed); + return; + }; + + // Extract the key before the NF consumes the packet. + let probe = probe_from_packet(&packet, src_vpcd()); + if let Some(probe) = probe.as_ref() { + if probe.ports.is_none() { + PORTLESS.fetch_add(1, Ordering::Relaxed); + } + if !matches!( + probe.proto, + NextHeader::TCP | NextHeader::UDP | NextHeader::ICMP | NextHeader::ICMP6 + ) { + EXOTIC_PROTO.fetch_add(1, Ordering::Relaxed); + } + } else { + NOT_IP.fetch_add(1, Ordering::Relaxed); + } + + let out = flow_filter + .process([packet].into_iter()) + .next() + .unwrap_or_else(|| unreachable!("enforce keeps Filtered and NotIp packets")); + + let expected = match probe.as_ref() { + // No IP layer: dropped before any table is consulted. + None => NfOutcome::Dropped(Some(DoneReason::NotIp)), + Some(probe) => expected_outcome(oracle_lookup(&overlay, probe)), + }; + assert_eq!( + observed_outcome(&out), + expected, + "NF diverged from the configuration on {headers:?}", + ); + + match expected { + NfOutcome::Routed { .. } => ROUTED.fetch_add(1, Ordering::Relaxed), + NfOutcome::Dropped(_) => DROPPED.fetch_add(1, Ordering::Relaxed), + }; + }); + + let counts = [ + ("unparseable", &UNPARSEABLE), + ("not-IP", &NOT_IP), + ("portless IP", &PORTLESS), + ("non-transport proto", &EXOTIC_PROTO), + ("routed", &ROUTED), + ("dropped", &DROPPED), + ] + .map(|(label, counter)| (label, counter.load(Ordering::Relaxed))); + eprintln!( + "coverage: {}", + counts + .iter() + .map(|(label, n)| format!("{n} {label}")) + .collect::>() + .join(", ") + ); + + // The point of the suite is the keys the hand-built shapes cannot produce. If the generator + // stops reaching them -- or starts failing to round-trip everything -- this must fail + // rather than pass on a diet of ordinary v4 TCP. + for (label, count) in counts { + if label == "unparseable" { + continue; + } + assert!(count >= 1, "no {label} packet was ever generated"); + } + } +} From c59e57c579df10d1a97e5ca98b2c7f87fa63fd11 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 16:55:29 -0600 Subject: [PATCH 3/8] test(flow-filter): generate exclusion prefixes, so exposes reach the tables as fans Every expose the generator emitted was a single block -- one `/24`, one `/32`. Real exposes carry `not` / `not_as` exclusions, which validation subtracts and normalizes away, so a `ValidatedExpose` reaching the table builder is normally a *fan* of disjoint prefixes of differing lengths. The generator never produced one, and the whole crate leans on that shape: `rule_priority` orders purely by prefix length. Add an `ExcludeSel` to each generated expose. Punching a single host out of a `/24` expands it to prefixes of every length from `/25` to `/32` at once, which is as hard as the length ordering gets. Constraints the exclusions have to respect, all encoded and commented: - Port forwarding forbids exclusions outright, so only the masquerade half of the Masquerade*PortFw pairs takes one. - Static NAT requires equal address counts on both sides, so exclusions are applied symmetrically to the private and public blocks. - Every variant stays in the block's upper half, because `derive_routing_probes` aims its guaranteed-routing probes at host `.1` and `FW_HOST` sits in the lower half -- so neither the derived probes nor the deliberate nested-overlap cases are disturbed. `exclusions_reach_the_config_as_multi_length_prefix_fans` asserts the fan actually materializes (widest spread must reach 8), because an exclusion that validation collapsed back into its block would leave every test above it quietly passing on the single-prefix exposes it had before. Value shown by mutation rather than argued: truncating `RuleSet::from_overlay` to lower only the *first* prefix of each expose is caught in ~100 iterations with exclusions on, and passes cleanly with them off. That is the same class of blind spot the ACL cross-product had -- when every input has one element, a lowering that drops all but the first is the identity. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/context/fuzz.rs | 52 +++++++++++++ flow-filter/src/fuzz_gen.rs | 133 ++++++++++++++++++++++++++++---- 2 files changed, 170 insertions(+), 15 deletions(-) diff --git a/flow-filter/src/context/fuzz.rs b/flow-filter/src/context/fuzz.rs index 0ac78ead0a..277d8f6085 100644 --- a/flow-filter/src/context/fuzz.rs +++ b/flow-filter/src/context/fuzz.rs @@ -368,3 +368,55 @@ fn reference_lookup_matches_config_oracle() { } }); } + +/// The exclusion prefixes the generator writes survive into the built configuration as a fan of +/// disjoint prefixes with many distinct lengths. +/// +/// Asserted rather than assumed, for the same reason the coverage counters elsewhere are: an +/// exclusion that validation collapsed back into the original block, or that the generator never +/// actually emitted, would leave every test above it passing on the single-prefix exposes it had +/// before -- exactly the failure mode where a generator looks like it covers a shape and does not. +/// +/// Punching one host out of a `/24` yields prefixes of every length from `/25` to `/32`, so a run +/// that ever picks [`ExcludeSel::UpperHost`] must reach a spread of 8. That spread is also what +/// stresses `rule_priority` hardest: it orders purely by prefix length, and its correctness rests +/// on config never letting two rules that can match the same packet share one. The config oracle's +/// `consider` panics on exactly that ambiguity, so the suites above are the ones proving it holds +/// -- this test only proves they are being handed the hard case. +#[test] +fn exclusions_reach_the_config_as_multi_length_prefix_fans() { + use std::collections::BTreeSet; + + // Lazily initialized so this compiles under the loom backend, whose AtomicU64::new is not const. + static WIDEST_SPREAD: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + + bolero::check!() + .with_type::() + .for_each(|overlay_spec| { + let built = overlay_spec.build(); + for vpc in built.overlay.vpc_table().values() { + for peering in vpc.peerings() { + let exposes = peering + .local() + .valexp() + .iter() + .chain(peering.remote().valexp()); + for expose in exposes { + for set in [expose.ips(), expose.public_ips()] { + let lengths: BTreeSet = + set.iter().map(|p| p.prefix().length()).collect(); + WIDEST_SPREAD.fetch_max(lengths.len() as u64, Ordering::Relaxed); + } + } + } + } + }); + + let spread = WIDEST_SPREAD.load(Ordering::Relaxed); + eprintln!("coverage: widest prefix-length spread in a single expose: {spread}"); + assert!( + spread >= 8, + "exclusions never produced a full prefix-length fan (widest spread was {spread}); \ + the generator is emitting single-block exposes and the priority ordering is untested", + ); +} diff --git a/flow-filter/src/fuzz_gen.rs b/flow-filter/src/fuzz_gen.rs index e6fd8cca3e..4f68454886 100644 --- a/flow-filter/src/fuzz_gen.rs +++ b/flow-filter/src/fuzz_gen.rs @@ -122,19 +122,87 @@ impl ExposeSpec { } } +/// An exclusion (`not` / `not_as`) punched out of an expose's block. +/// +/// Exclusions are collapsed away during validation: `VpcExpose::validate` subtracts them and +/// normalizes, so a `ValidatedExpose` carries a *fan* of disjoint prefixes of differing lengths +/// rather than the one block that was written. That fan is the input shape the rest of the crate +/// most depends on and least often sees -- `rule_priority` orders purely by prefix length, and its +/// correctness rests on config guaranteeing that rules which can both match a packet never share +/// one. A block with a single host punched out expands to prefixes of every length from /25 to /32 +/// at once, which is as hard as that ordering gets. +/// +/// Every variant stays inside the block's upper half. Host `.1` of every block must survive, +/// because [`derive_routing_probes`] aims its guaranteed-routing probes there, and an exclusion +/// that swallowed it would turn those probes into misses. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum ExcludeSel { + None, + /// The block's upper half (a `/25`, or `/121` for v6). + UpperHalf, + /// The block's second quarter, leaving runs on both sides of the hole. + SecondQuarter, + /// A single host in the upper half: the widest fan of prefix lengths a single exclusion can + /// produce. + UpperHost(u8), +} + +impl ExcludeSel { + /// The prefix to exclude from block `n`, or `None` to leave the block whole. + fn resolve(self, n: u8, public: bool, v6: bool) -> Option { + let (net4, net6) = if public { (20, "db9") } else { (10, "db8") }; + Some(match (self, v6) { + (ExcludeSel::None, _) => return None, + (ExcludeSel::UpperHalf, false) => format!("{net4}.{n}.0.128/25"), + (ExcludeSel::UpperHalf, true) => format!("2001:{net6}:0:{n:x}::80/121"), + (ExcludeSel::SecondQuarter, false) => format!("{net4}.{n}.0.64/26"), + (ExcludeSel::SecondQuarter, true) => format!("2001:{net6}:0:{n:x}::40/122"), + // Force the host into the upper half so that host .1 (and the nested port-forwarding + // host, FW_HOST) are never the one removed. + (ExcludeSel::UpperHost(h), false) => { + format!("{net4}.{n}.0.{}/32", h | 0x80) + } + (ExcludeSel::UpperHost(h), true) => { + format!("2001:{net6}:0:{n:x}::{:x}/128", h | 0x80) + } + }) + } +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct ExposeEntry { + spec: ExposeSpec, + /// Punched out of this expose's block(s). Ignored for port forwarding, which config forbids + /// exclusions on outright. + exclude: ExcludeSel, +} + +impl ExposeEntry { + const fn plain() -> Self { + Self { + spec: ExposeSpec::Plain, + exclude: ExcludeSel::None, + } + } +} + #[derive(Debug, Clone, Copy, TypeGenerator)] pub(crate) struct ManifestSpec { /// Up to two expose specs (some expand to two actual exposes). - exposes: [Option; 2], + exposes: [Option; 2], /// Whether the manifest carries a default (catch-all) expose. default: bool, } impl ManifestSpec { - fn expose_specs(&self) -> impl Iterator + '_ { + fn entries(&self) -> impl Iterator + '_ { self.exposes.iter().flatten().copied() } + fn expose_specs(&self) -> impl Iterator + '_ { + self.entries().map(|entry| entry.spec) + } + fn is_empty(&self) -> bool { self.exposes.iter().all(Option::is_none) && !self.default } @@ -153,8 +221,8 @@ impl ManifestSpec { /// stateful and static NAT, which is the sole case in which the NF retains a flow key. fn strip_stateful_nat(&mut self) { for slot in self.exposes.iter_mut().flatten() { - if slot.is_stateful() { - *slot = ExposeSpec::StaticNat; + if slot.spec.is_stateful() { + slot.spec = ExposeSpec::StaticNat; } } } @@ -162,7 +230,7 @@ impl ManifestSpec { fn drop_default(&mut self) { self.default = false; if self.is_empty() { - self.exposes[0] = Some(ExposeSpec::Plain); + self.exposes[0] = Some(ExposeEntry::plain()); } } } @@ -199,11 +267,11 @@ impl OverlaySpec { spec.peerings[0] = Some(PeeringSpec { v6: false, local: ManifestSpec { - exposes: [Some(ExposeSpec::Plain), None], + exposes: [Some(ExposeEntry::plain()), None], default: false, }, remote: ManifestSpec { - exposes: [Some(ExposeSpec::Plain), None], + exposes: [Some(ExposeEntry::plain()), None], default: false, }, }); @@ -211,7 +279,7 @@ impl OverlaySpec { for peering in spec.peerings.iter_mut().flatten() { for manifest in [&mut peering.local, &mut peering.remote] { if manifest.is_empty() { - manifest.exposes[0] = Some(ExposeSpec::Plain); + manifest.exposes[0] = Some(ExposeEntry::plain()); } } // A default expose cannot face another default expose within one peering. @@ -332,19 +400,27 @@ fn vpc_name(index: usize) -> String { fn build_manifest(vpc_name: &str, spec: &ManifestSpec, v6: bool, blocks: &mut u8) -> VpcManifest { let mut exposes = Vec::new(); - for expose_spec in spec.expose_specs() { + for entry in spec.entries() { let n = *blocks; *blocks += 1; - match expose_spec { - ExposeSpec::Plain => exposes.push(plain(n, v6)), - ExposeSpec::StaticNat => exposes.push(static_nat(n, v6)), - ExposeSpec::Masquerade => exposes.push(masquerade(n, v6)), + let exclude = entry.exclude; + match entry.spec { + ExposeSpec::Plain => exposes.push(excluding(plain(n, v6), n, v6, exclude)), + ExposeSpec::StaticNat => { + exposes.push(excluding_both(static_nat(n, v6), n, v6, exclude)); + } + ExposeSpec::Masquerade => { + exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); + } + // Port forwarding forbids exclusion prefixes outright, so only the masquerade half of + // these pairs takes one -- which is also what keeps the nested overlap intact, since + // every exclusion stays in the block's upper half and FW_HOST is in the lower. ExposeSpec::MasqueradeNestingPortFw(proto) => { - exposes.push(masquerade(n, v6)); + exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); exposes.push(portfw_host(n, v6, proto)); } ExposeSpec::MasqueradeSameLenPortFw(proto) => { - exposes.push(masquerade(n, v6)); + exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); exposes.push(portfw_block(n, v6, proto)); } ExposeSpec::PortForwarding(proto) => exposes.push(portfw_host(n, v6, proto)), @@ -401,6 +477,33 @@ pub(crate) fn block_addr(n: u8, host: u8, public: bool, v6: bool) -> IpAddr { } } +/// Punch `exclude` out of an expose's private prefixes only. For a plain expose there is no public +/// side to keep in step; validation collapses the exclusion away, leaving a fan of prefixes. +fn excluding(expose: VpcExpose, n: u8, v6: bool, exclude: ExcludeSel) -> VpcExpose { + match exclude.resolve(n, false, v6) { + Some(prefix) => expose.not(prefix.as_str().into()), + None => expose, + } +} + +/// Punch the same-shaped `exclude` out of both the private and the public prefixes. +/// +/// Symmetry is not cosmetic: static NAT requires an equal address count on the two sides +/// (`ConfigError::MismatchedPrefixSizes`), so an exclusion applied to one side only would fail +/// validation for every static-NAT expose the generator emits. +fn excluding_both(expose: VpcExpose, n: u8, v6: bool, exclude: ExcludeSel) -> VpcExpose { + let Some(private) = exclude.resolve(n, false, v6) else { + return expose; + }; + let Some(public) = exclude.resolve(n, true, v6) else { + unreachable!("resolve is None only for ExcludeSel::None, handled above"); + }; + expose + .not(private.as_str().into()) + .not_as(public.as_str().into()) + .unwrap_or_else(|e| unreachable!("exclusion on an expose with a public range: {e}")) +} + fn plain(n: u8, v6: bool) -> VpcExpose { VpcExpose::empty().ip(private_block(n, v6).as_str().into()) } From 50c691b6e4ca893fb69cbdf46be95b4721299aa5 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 16:56:36 -0600 Subject: [PATCH 4/8] test(flow-filter): pin three edges the property suites reach but never state All three are behaviours the fuzz suites hit thousands of times per run and none could describe: a property test says "the NF agreed with the config", not "and here is what the config means". All three read like bugs until explained, which is exactly why they should be tests rather than folklore. `ipv6_extension_header_masks_the_transport_protocol`: `Net::next_header()` reports the IPv6 header's own next-header field, so any extension header puts *its* number there -- while `try_transport()` still walks the chain and finds the real ports. TCP behind a hop-by-hop header therefore presents `(proto = HOPOPT, ports = the TCP ports)`. A protocol-restricted expose lowers to an exact match on the protocol byte and cannot match it, so the traffic is dropped rather than forwarded. Fail-closed, but a functional gap: legitimate TCP over IPv6 carrying extension headers cannot reach a TCP-restricted port-forwarding destination. The test pins both the mechanism and the consequence, and shows the same packet routing through an unrestricted expose to confirm the protocol byte is what excluded it. `portless_packet_cannot_match_a_port_restricted_expose`: a packet with no usable transport ports looks up with port 0, and config forbids port 0 in an expose's ranges, so a port-restricted expose can never match one. Not exotic -- ICMP, non-first fragments, and (by the above) anything behind an IPv6 extension header all present portless keys. Fail-closed, but it makes port-forwarded destinations unreachable to that traffic, including the ICMP errors path MTU discovery needs. The test shows the same packet routing through an unrestricted expose, so the port is demonstrably what excluded it. `flow_from_a_newer_generation_is_honored_for_bypass`: `dst_vpcd_from_valid_flow` rejects only `flow_genid < genid`, so a flow tagged with a generation this worker has not yet observed short-circuits the tables. That is the transient its comment describes -- the control plane has stamped flows with the new generation before this worker reads it, and treating them as stale would tear down every flow the new config just blessed. It was the one direction of the genid comparison nothing covered. The test aims at a destination the tables do not cover, so honouring the flow is observable rather than incidental. The first was found by the adversarial-header suite added two commits back; grouping it here keeps the three fail-closed edges stated in one place, and the portless test's doc comment already cross-references it. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/tests.rs | 190 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 190 insertions(+) diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 20ed8de274..232bf87031 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -1598,3 +1598,193 @@ mod adversarial_headers { } } } + +// ------------------------------------------------------------------------------------------------- +// Documented behaviour: IPv6 extension headers hide the transport protocol. +// +// Found by the adversarial-header suite above, which reaches these keys in the thousands per run. +// Pinned here as its own test because a fuzz run that merely agrees with the config oracle does not +// say *what* the configuration means for these packets, and the answer is surprising enough that a +// future change to it should be deliberate. + +/// `Net::next_header()` reports the IPv6 header's own next-header field. When any extension header +/// is present that field names the *extension*, not the transport -- while `try_transport()` still +/// walks the chain and finds the real ports. +/// +/// So an IPv6 packet carrying TCP behind a hop-by-hop header presents the lookup key +/// `(proto = HOPOPT, ports = the TCP ports)`. A protocol-restricted expose lowers to an exact match +/// on the protocol byte, so it cannot match such a packet, and the traffic is dropped rather than +/// port-forwarded. That is fail-closed, but it is a functional gap: legitimate TCP over IPv6 with +/// extension headers does not reach a TCP-restricted port-forwarding destination. +#[test] +fn ipv6_extension_header_masks_the_transport_protocol() { + use net::headers::builder::HeaderStack; + use net::headers::{TryIp, TryTransport}; + use net::ipv6::UnicastIpv6Addr; + use net::tcp::TcpPort; + + let with_hop_by_hop = || { + HeaderStack::new() + .eth(|_| {}) + .ipv6(|ip| { + ip.set_source(UnicastIpv6Addr::new(v6("2001:db8::1")).unwrap()); + ip.set_destination(v6("2001:db9::5")); + }) + .hop_by_hop(|_| {}) + .tcp(|tcp| { + tcp.set_source(TcpPort::try_from(1234u16).unwrap()); + tcp.set_destination(TcpPort::try_from(80u16).unwrap()); + }) + .build_headers() + .unwrap() + }; + + // The mechanism: the protocol byte names the extension header, but the ports are still found. + let probe_packet = packet(Some(vpcd(100)), with_hop_by_hop()); + let net = probe_packet.try_ip().unwrap(); + assert_eq!( + net.next_header(), + net::ip::NextHeader::new(0), + "an extension header should occupy the next-header field", + ); + assert_eq!( + probe_packet + .try_transport() + .and_then(|t| t.dst_port()) + .map(std::num::NonZero::get), + Some(80), + "the transport header is still reachable behind the extension header", + ); + + // The consequence: a TCP-restricted destination expose cannot match it. + let tcp_only = context( + &[("vpc1", 100), ("vpc2", 200)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("2001:db8::/32")]), + ( + "vpc2", + vec![expose_port_forwarding( + "2001:db9::5/128", + (22, 22), + "2001:db9::5/128", + (80, 80), + Some(L4Protocol::Tcp), + )], + ), + )], + ); + let (mut flow_filter, _writer) = make_flow_filter(tcp_only); + let out = run(&mut flow_filter, packet(Some(vpcd(100)), with_hop_by_hop())); + assert_eq!( + out.get_done(), + Some(DoneReason::Filtered), + "a TCP-restricted expose does not see this packet as TCP, so nothing covers it", + ); + + // The same packet routes when the covering expose is not protocol-restricted, which confirms + // the protocol byte -- not the address or the port -- is what excluded it above. + let any_proto = context( + &[("vpc1", 100), ("vpc2", 200)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("2001:db8::/32")]), + ("vpc2", vec![expose("2001:db9::/32")]), + )], + ); + let (mut flow_filter, _writer) = make_flow_filter(any_proto); + let out = run(&mut flow_filter, packet(Some(vpcd(100)), with_hop_by_hop())); + assert!(!out.is_done(), "{:?}", out.get_done()); + assert_eq!(out.meta().dst_vpcd, Some(vpcd(200))); +} + +// ------------------------------------------------------------------------------------------------- +// Documented behaviour: two more edges the adversarial-header and burst suites reach constantly +// but never pin, because a property test can only say "the NF agreed with the config", not "and +// here is what the config means". + +/// A packet with no usable transport ports looks up with port `0`, and config forbids port `0` in +/// an expose's ranges -- so a port-restricted expose can never match one. +/// +/// This is not an exotic case: an ICMP packet, a non-first fragment, and (per +/// [`ipv6_extension_header_masks_the_transport_protocol`]) anything behind an IPv6 extension header +/// all present portless keys. The adversarial-header suite generates thousands per run. The effect +/// is fail-closed, but it means port-forwarded destinations are unreachable by such traffic -- +/// including the ICMP errors that path MTU discovery depends on. +#[test] +fn portless_packet_cannot_match_a_port_restricted_expose() { + // vpc2 publishes 80.0.0.5 only as a port-forwarding destination on public port 2222. + let (mut flow_filter, _) = make_flow_filter(dst_port_forwarding_context()); + let out = run( + &mut flow_filter, + packet( + Some(vpcd(100)), + build_icmp_packet(v4("10.0.0.5"), v4("80.0.0.5")), + ), + ); + assert_eq!( + out.get_done(), + Some(DoneReason::Filtered), + "a portless packet cannot match the port-restricted expose that publishes this address", + ); + assert_eq!(out.meta().dst_vpcd, None); + + // The same address and protocol route once a covering expose carries no port constraint, which + // confirms the port -- not the address or the protocol -- is what excluded it. + let unrestricted = context( + &[("vpc1", 100), ("vpc2", 200)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("10.0.0.0/24")]), + ("vpc2", vec![expose("80.0.0.0/24")]), + )], + ); + let (mut flow_filter, _writer) = make_flow_filter(unrestricted); + let out = run( + &mut flow_filter, + packet( + Some(vpcd(100)), + build_icmp_packet(v4("10.0.0.5"), v4("80.0.0.5")), + ), + ); + assert!(!out.is_done(), "{:?}", out.get_done()); + assert_eq!(out.meta().dst_vpcd, Some(vpcd(200))); +} + +/// A flow whose generation is *newer* than the pipeline's is trusted for bypass. +/// +/// `dst_vpcd_from_valid_flow` rejects only `flow_genid < genid`, so a flow tagged with a generation +/// the filter has not yet observed short-circuits the tables entirely. That is deliberate -- it is +/// the "small transient period" the function's comment describes, where a reconfiguring control +/// plane has already stamped flows with the new generation but this worker still reads the old one, +/// and treating those as stale would tear down every flow the new config just blessed. +/// +/// Pinned because it is the one direction of the genid comparison nothing else covers, and because +/// it is asymmetric in a way that reads like a bug until the transient is explained: the bypass +/// here wins over what the tables say, exactly as an equal-generation flow would. +#[test] +fn flow_from_a_newer_generation_is_honored_for_bypass() { + let (mut flow_filter, _writer) = make_flow_filter(source_nat_context()); + + // A destination the current tables do not cover at all: without the flow this would be + // Filtered, so honouring the flow is observable rather than incidental. + let mut p = packet( + Some(vpcd(100)), + build_tcp_packet(v4("1.0.0.5"), v4("9.9.9.9"), 1234, 5678), + ); + let flow = attach_flow(&mut p, Some(vpcd(200)), true, false, false); + flow.set_genid(9); + + let out = run(&mut flow_filter, p); + assert!( + !out.is_done(), + "a newer-generation flow must bypass the tables: {:?}", + out.get_done(), + ); + assert_eq!(out.meta().dst_vpcd, Some(vpcd(200))); + assert_eq!( + flow.status(), + FlowStatus::Active, + "the bypass path must not invalidate the flow it just honoured", + ); +} From eb430f237a267f8e386ae010fc359df3042142cf Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 20:43:31 -0600 Subject: [PATCH 5/8] fix(flow-filter): bound rte_acl table names for every counter value `table_name` builds `flow_filter_{base}_{seq}` with a process-global decimal counter, and rte_acl rejects a context name over 31 bytes: dpdk build: "invalid ACL context name: ACL context name is too long (32 > 31 bytes)" Today's bound holds only by accident of how the names happen to be spelled. The 12-byte prefix plus the longest current base (`remote_v4`, 9 bytes) leaves nine digits, so the ceiling is ~10^9 and effectively unreachable -- but it scales with the base, and a base four bytes longer would cut it by four orders of magnitude, to a counter value a long-lived dataplane really does reach. The counter advances once per table built, so a process that rebuilds its tables on every configuration change gets there; and when it does, *every* subsequent table build fails. Reconfiguration stops working with the last good tables left in place. Make the bound hold for every value the counter can take rather than for the values today's names happen to permit. A 3-byte prefix, a base capped at 9 bytes, and a hexadecimal counter (16 bytes at most for a `u64`) give at most 29 bytes. `table_names_fit_the_rte_acl_limit_for_every_counter_value` asserts this against `u64::MAX` rather than against the counter's current value, which is the property the decimal scheme lacked, and a `debug_assert` catches an over-long base at its call site. This lands ahead of the masquerade table added later in this series, whose base is longer than any existing one -- so the constraint is stated before anything leans on it, rather than after. Context names are internal to rte_acl's registry -- nothing renders or persists them -- so renaming is safe. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/context/tables.rs | 63 ++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/flow-filter/src/context/tables.rs b/flow-filter/src/context/tables.rs index d8e98fabf5..7f1ad997a9 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -287,12 +287,37 @@ impl fmt::Debug for AnyTable { // is a thin wrapper over an otherwise-const atomic. static TABLE_SEQ: LazyLock = LazyLock::new(|| AtomicU64::new(0)); -/// A process-unique rte_acl context name (rte_acl rejects duplicate names). +/// The longest name rte_acl accepts (`RTE_ACL_NAMESIZE` less its NUL). +const MAX_TABLE_NAME: usize = 31; +/// The longest `base` [`table_name`] may be given, so that the name fits for *every* counter value. +const MAX_TABLE_BASE: usize = 9; + +/// A process-unique rte_acl context name (rte_acl rejects duplicate names, and names over +/// [`MAX_TABLE_NAME`] bytes). +/// +/// The name has to be both unique and short, and the bound must hold for every value the counter +/// can take -- not merely the values a given run is expected to reach. A short prefix, a `base` of +/// at most [`MAX_TABLE_BASE`] bytes, and a hexadecimal counter (16 bytes at most for a `u64`) give +/// `3 + 9 + 1 + 16 = 29`, which fits with room to spare. +/// +/// This is not hypothetical. With the previous `flow_filter_` prefix, a 13-byte base and a decimal +/// counter, names passed 31 bytes once the counter reached 100_000 -- which a long fuzz run +/// reached, and which a long-lived process rebuilding its tables on every configuration change +/// would reach eventually. The failure mode was that every subsequent table build failed, so +/// reconfiguration stopped working. fn table_name(base: &str) -> String { - format!( - "flow_filter_{base}_{}", - TABLE_SEQ.fetch_add(1, Ordering::Relaxed) - ) + debug_assert!( + base.len() <= MAX_TABLE_BASE, + "table base {base:?} is {} bytes, over the {MAX_TABLE_BASE}-byte budget", + base.len(), + ); + let name = format!("ff_{base}_{:x}", TABLE_SEQ.fetch_add(1, Ordering::Relaxed)); + debug_assert!( + name.len() <= MAX_TABLE_NAME, + "table name {name:?} is {} bytes, over rte_acl's {MAX_TABLE_NAME}-byte limit", + name.len(), + ); + name } /// Build one table from backend-neutral rules using the selected backend. @@ -837,6 +862,34 @@ mod unit_tests { assert_eq!(LocalKey::::N, 5); } + /// Every name `table_name` can produce fits rte_acl's limit, for every counter value. + /// + /// Asserted over `u64::MAX` rather than over the counter's current value: the point is that no + /// amount of running can push a name over the limit, which is exactly the property the previous + /// decimal scheme lacked. + #[test] + fn table_names_fit_the_rte_acl_limit_for_every_counter_value() { + for base in [ + "remote_v4", + "local_v4", + "remote_v6", + "local_v6", + "masq_v4", + "masq_v6", + ] { + assert!( + base.len() <= MAX_TABLE_BASE, + "base {base:?} is over the {MAX_TABLE_BASE}-byte budget", + ); + let longest = format!("ff_{base}_{:x}", u64::MAX); + assert!( + longest.len() <= MAX_TABLE_NAME, + "{longest:?} is {} bytes, over the {MAX_TABLE_NAME}-byte limit", + longest.len(), + ); + } + } + #[test] fn default_tables_are_empty() { let tables = FlowFilterContext::default(); From 72eef158875639d79482df84d14bf6cdc4f60a26 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 4 Aug 2026 01:03:09 -0600 Subject: [PATCH 6/8] refactor(flow-filter): state the single-key lookup sequence once `FlowFilterContext::lookup` is the readable per-packet oracle the property suites run against `lookup_batch`. Its v4 and v6 arms were the same thirty lines twice over -- stage 1 (destination -> `Verdict`), then stage 2 (source -> source NAT) for a hit -- differing only in which pair of typed tables they reached into. Factor that body into `lookup_one`, taking the `Query` the batched path already builds for exactly these fields. No behaviour change: the two arms were identical before and call one function after, and the suite that cross-checks this path against `lookup_batch` is unchanged. This is a prelude. The next commit adds a third lookup stage, and doing it against two copies would state the new sequence twice and leave the two free to drift -- in the one place whose whole job is to be the simple statement the batched path is checked against. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/context/tables.rs | 91 +++++++++++++++++++------------ 1 file changed, 55 insertions(+), 36 deletions(-) diff --git a/flow-filter/src/context/tables.rs b/flow-filter/src/context/tables.rs index 7f1ad997a9..478f3ae235 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -676,50 +676,30 @@ impl FlowFilterContext { let dst_port = dst_port.unwrap_or(0); match (src_ip, dst_ip) { - (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => { - let Some(verdict) = self.remote_v4.lookup(&RemoteKey { - proto, + (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => lookup_one( + &self.remote_v4, + &self.local_v4, + &Query { src_vni, - dst_ip, - dst_port, - }) else { - return LookupResult::DestinationMiss; - }; - match self.local_v4.lookup(&LocalKey { proto, - src_vni, - dst_vni: key_vni(verdict.dst_vpcd), src_ip, - src_port, - }) { - Some(nat_mode) => { - LookupResult::Route((verdict.dst_vpcd, verdict.nat_mode, *nat_mode)) - } - None => LookupResult::SourceMiss(verdict.dst_vpcd), - } - } - (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => { - let Some(verdict) = self.remote_v6.lookup(&RemoteKey { - proto, - src_vni, dst_ip, + src_port, dst_port, - }) else { - return LookupResult::DestinationMiss; - }; - match self.local_v6.lookup(&LocalKey { - proto, + }, + ), + (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => lookup_one( + &self.remote_v6, + &self.local_v6, + &Query { src_vni, - dst_vni: key_vni(verdict.dst_vpcd), + proto, src_ip, + dst_ip, src_port, - }) { - Some(nat_mode) => { - LookupResult::Route((verdict.dst_vpcd, verdict.nat_mode, *nat_mode)) - } - None => LookupResult::SourceMiss(verdict.dst_vpcd), - } - } + dst_port, + }, + ), _ => { debug!( "Source and destination IP versions do not match: src_ip={src_ip:?}, dst_ip={dst_ip:?}", @@ -779,6 +759,45 @@ impl FlowFilterContext { } } +/// Resolve one query against one IP version's tables: stage 1 (destination -> [`Verdict`]), then +/// stage 2 (source -> source NAT) for a stage-1 hit. +/// +/// The two versions ran identical bodies over differently-typed tables, so the lookup sequence was +/// stated twice and had to be edited twice. `Query` already carries exactly these fields for the +/// batched path; reusing it lets the single-key path say the sequence once. +/// +/// Test-only, like its caller: production always runs the batched path. +#[cfg(test)] +fn lookup_one( + remote: &AnyTable, Verdict>, + local: &AnyTable, NatMode>, + q: &Query, +) -> LookupResult +where + RemoteKey: MatchKey, + LocalKey: MatchKey, +{ + let Some(verdict) = remote.lookup(&RemoteKey { + proto: q.proto, + src_vni: q.src_vni, + dst_ip: q.dst_ip, + dst_port: q.dst_port, + }) else { + return LookupResult::DestinationMiss; + }; + + match local.lookup(&LocalKey { + proto: q.proto, + src_vni: q.src_vni, + dst_vni: key_vni(verdict.dst_vpcd), + src_ip: q.src_ip, + src_port: q.src_port, + }) { + Some(nat_mode) => LookupResult::Route((verdict.dst_vpcd, verdict.nat_mode, *nat_mode)), + None => LookupResult::SourceMiss(verdict.dst_vpcd), + } +} + /// The two-pass batched lookup for one IP version. `queries[k]` corresponds to output slot /// `out[idx[k]]`. Runs in `MAX_BATCH`-sized rte_acl calls: stage 1 (destination -> [`Verdict`]), /// then stage 2 (source -> source NAT) over the stage-1 hits only. From 3284b86b82c513b84608a06d0b1b24667dde5027 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 20:32:46 -0600 Subject: [PATCH 7/8] fix(flow-filter): verify a masquerade destination, do not resolve it Config models the destination -> VPC relation as one-to-many. In `config/src/external/overlay/vpcrouting.rs`, `VpcRouteTable` is `destination prefix -> VpcRouteSet`, and `VpcRouteSet` is a `Vec`: struct VpcRouteSet(Vec); `VpcRoute::can_overlap` deliberately exempts masquerade/masquerade, so two peers may masquerade behind one public range and both claim the same destination prefix. Stage 1 of the flow-filter lowers that relation into a *function*: `RemoteKey -> Verdict`, and `Verdict` holds exactly one `dst_vpcd`. Two peers sharing a range therefore produce two rules with identical match sets and identical priorities differing only in the destination VPC, and the collapse is settled by tie-break order -- stable-sort position under the reference backend, and formally unspecified under rte_acl, so the two backends need not even agree. `apply_route` then required the packet's flow to agree with that arbitrary winner, so every masquerade reply toward the *losing* peer was dropped and its flow cancelled. Deterministic, but arbitrary: one of the two peers was simply broken. The root cause is that a masquerade public address is not a routable identifier. It names a NAT pool. It can never accept a new connection, so the only legitimate traffic to it is reply traffic, which by construction rides a flow -- and the flow is the only thing that can distinguish two connections to the same address. State is primary for this traffic class; the table can only ever be a plausibility check. So stop asking the table an unanswerable question. "Which VPC owns 20.0.0.5?" has no unique answer. "Does vpc3 masquerade 20.0.0.5 toward me?" does. Stage 3 (`MasqueradeKey`) carries `dst_vni` as an *input*: the flow supplies the candidate, the table says whether the configuration agrees. - `LookupInput` carries `flow_dst_vpcd`, so verification stays on the batched path with the other stages rather than becoming a per-packet lookup in the NF. - `LookupResult::MasqueradeDestination(Option)` replaces `Route` with a masquerade `dst_nat`. `Some` means verified; `None` means no candidate, or one config rejects. There is no source NAT to report -- such a packet rides a flow that already carries its own state. - Stage 2 and stage 3 are mutually exclusive, so each rte_acl call still asks exactly one question and the burst still makes three calls. The property oracle changes shape to match: a stage-1 match becomes `StageOne`, whose `Masquerade` variant deliberately carries no VPC. That is the same claim the tables now make, stated where the oracle can hold it -- and it is what lets two overlapping masquerade exposes compare *equal*, so `consider` can accept that tie as benign while still failing on a tie whose candidates disagree. Without it the oracle would have to assert that no two exposes ever tie, which is exactly the thing config permits. Note what this does NOT weaken. An earlier proposal was to take the destination straight from the flow; that is wrong, and it is why this is a verification and not a substitution. The old equality check was doing double duty -- a broken disambiguator *and* a working staleness check -- so removing it outright would have let a flow naming a since-unpeered VPC forward traffic there. Asking the configuration answers both at once: `masquerade_reply_is_refused_when_the_flow_names_the_wrong_peer` pins the staleness half, and the pre-existing `masquerade_reply_with_mismatched_flow_ destination_is_filtered` now passes for a stronger reason (config does not agree that VPC owns the address, rather than a tie-break disagreeing). `masquerade_replies_reach_both_peers_sharing_a_public_range` is the regression test: both peers' replies now work, where one was always dropped. Reverting the verification to the old equality rule fails it. Two test-visible consequences of the model change, both intended: - `dst_side_nat_modes` asserted a masquerade destination "resolves as a marker" with a destination VPC. It no longer resolves at all; the test now pins all three verification outcomes. - Derived routing probes at a masquerade destination never route, so they are split into `masquerade_probes` carrying the correct candidate and asserted to verify. That keeps stage 3's *hit* path exercised -- a verification table matching nothing would satisfy every "must drop" assertion in the suite (~13k masquerade destinations per 30s run). Cross-peering overlaps are still absent from the generated overlays, so the property suite does not yet catch this class on its own; that follows in the next commit. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/context/fuzz.rs | 169 +++++++++++-------- flow-filter/src/context/tables.rs | 269 ++++++++++++++++++++++++++---- flow-filter/src/context/tests.rs | 75 ++++++--- flow-filter/src/fuzz_gen.rs | 70 +++++++- flow-filter/src/lib.rs | 47 ++++-- flow-filter/src/tests.rs | 131 ++++++++++++++- 6 files changed, 612 insertions(+), 149 deletions(-) diff --git a/flow-filter/src/context/fuzz.rs b/flow-filter/src/context/fuzz.rs index 277d8f6085..5a112a788c 100644 --- a/flow-filter/src/context/fuzz.rs +++ b/flow-filter/src/context/fuzz.rs @@ -17,6 +17,7 @@ use crate::fuzz_gen::{OverlaySpec, Probe, ProbeSpec, bogus_vpcd}; use concurrency::sync::LazyLock; use concurrency::sync::atomic::{AtomicU64, Ordering}; use config::external::overlay::ValidatedOverlay; +use config::external::overlay::vpcpeering::ValidatedExpose; use lpm::prefix::{IpPrefix, L4Protocol, Prefix, PrefixWithOptionalPorts}; use net::ip::NextHeader; use net::packet::VpcDiscriminant; @@ -54,18 +55,45 @@ fn prefix_allows(prefix: &PrefixWithOptionalPorts, ip: IpAddr, port: u16) -> boo /// equal-length ties. Mirrors `rule_priority` without sharing its encoding. type Precedence = (u8, bool); -/// Keep the strictly-better candidate; equal precedence between candidates that can match the -/// same packet is a generator invariant violation, so fail loudly rather than pick one. -fn consider(best: &mut Option<(Precedence, T)>, precedence: Precedence, value: T) { +/// Keep the strictly-better candidate. +/// +/// Two candidates at equal precedence are only acceptable when they agree on the answer. Config +/// permits exactly one such tie: two peers masquerading behind the same public range, which +/// `VpcRoute::can_overlap` exempts. Both then say "masquerade destination", and since a masquerade +/// destination is verified against the flow rather than resolved from the address (see +/// [`StageOne`]), they say the *same* thing and the tie is harmless. +/// +/// A tie whose candidates disagree is a real ambiguity -- the tables would have to pick one, and +/// nothing says which -- so fail loudly rather than let a test pass on a coin flip. +fn consider( + best: &mut Option<(Precedence, T)>, + precedence: Precedence, + value: T, +) { match best { - Some((current, _)) if *current == precedence => { - panic!("ambiguous match at precedence {precedence:?}: generator invariant violated") + Some((current, existing)) if *current == precedence => { + assert!( + *existing == value, + "ambiguous match at precedence {precedence:?}: {existing:?} vs {value:?}", + ); } Some((current, _)) if *current > precedence => {} _ => *best = Some((precedence, value)), } } +/// What a stage-1 match says about a destination. +/// +/// [`StageOne::Masquerade`] deliberately carries no VPC. A masquerade public address does not +/// identify one -- config lets two peers share a range -- so the destination is verified against +/// the flow's candidate instead. Dropping the VPC here is what makes two overlapping masquerade +/// exposes compare equal, and so what makes the overlap a benign tie rather than an ambiguity. +#[derive(Debug, PartialEq, Eq)] +enum StageOne { + Masquerade, + Resolved(VpcDiscriminant, Option), +} + /// Answer a route lookup directly from the validated overlay. /// /// Exposed to the crate because the NF-level property test @@ -86,7 +114,7 @@ pub(crate) fn oracle_lookup(overlay: &ValidatedOverlay, probe: &Probe) -> Lookup // Stage 1: the destination against every peer's public prefixes. Masquerade exposes are // included (marker rules); a default expose acts as a /0 of the peering's IP version. - let mut verdict: Option<(Precedence, (VpcDiscriminant, Option))> = None; + let mut verdict: Option<(Precedence, StageOne)> = None; for peering in src_vpc.peerings() { let dst_vpcd = VpcDiscriminant::from_vni(peering.remote_vni()); for expose in peering.remote().valexp() { @@ -95,22 +123,48 @@ pub(crate) fn oracle_lookup(overlay: &ValidatedOverlay, probe: &Probe) -> Lookup } for prefix in expose.public_ips() { if prefix_allows(prefix, probe.dst_ip, dport) { + let matched = if expose.has_masquerade() { + StageOne::Masquerade + } else { + StageOne::Resolved(dst_vpcd, NatRequirement::from_expose(expose)) + }; consider( &mut verdict, (prefix.prefix().length(), expose.has_port_forwarding()), - (dst_vpcd, NatRequirement::from_expose(expose)), + matched, ); } } } if peering.remote().has_default_expose() && probe.dst_ip.is_ipv4() == peering.is_v4() { - consider(&mut verdict, (0, false), (dst_vpcd, None)); + consider(&mut verdict, (0, false), StageOne::Resolved(dst_vpcd, None)); } } - let Some((_, (dst_vpcd, dst_nat))) = verdict else { + let Some((_, matched)) = verdict else { return LookupResult::DestinationMiss; }; + // A masquerade destination is verified against the flow's candidate, not resolved from the + // address. Two peers may masquerade behind one range -- config permits it and models the + // destination as a *set* of routes -- so "which VPC owns this address" has no unique answer. + // "Does this VPC masquerade this address for this source" does, and it is asked directly of + // the config here: find the peering to the flow's claimed VPC and check whether one of its + // masquerade exposes covers the destination. + let StageOne::Resolved(dst_vpcd, dst_nat) = matched else { + let verified = probe.flow_dst_vpcd.filter(|candidate| { + src_vpc + .peerings() + .iter() + .filter(|p| VpcDiscriminant::from_vni(p.remote_vni()) == *candidate) + .flat_map(|p| p.remote().valexp()) + .filter(|expose| expose.has_masquerade()) + .filter(|expose| proto_allows(expose.nat_proto(), probe.proto)) + .flat_map(ValidatedExpose::public_ips) + .any(|prefix| prefix_allows(prefix, probe.dst_ip, dport)) + }); + return LookupResult::MasqueradeDestination(verified); + }; + // Stage 2: the source against that peering's private prefixes. Port-forwarding sources are // excluded (they cannot initiate); a default expose acts as a /0 of the peering's version. let peering = src_vpc @@ -165,6 +219,7 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { static ROUTES: LazyLock = LazyLock::new(|| AtomicU64::new(0)); static SOURCE_MISSES: LazyLock = LazyLock::new(|| AtomicU64::new(0)); static DESTINATION_MISSES: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + static MASQUERADE_DESTINATIONS: LazyLock = LazyLock::new(|| AtomicU64::new(0)); bolero::check!() .with_type::<(OverlaySpec, [ProbeSpec; 40])>() @@ -179,21 +234,9 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { // may not get enough of them during a time-boxed bolero run on a loaded CI runner to // get meaningful coverage. for probe in &built.routing_probes { - let want = reference.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports, - ); + let want = reference.lookup(&probe.input()); assert_eq!( - dpdk.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports - ), + dpdk.lookup(&probe.input()), want, "backends disagree on derived routing probe {probe:?}\nspec: {overlay_spec:?}", ); @@ -204,43 +247,46 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { ROUTES.fetch_add(1, Ordering::Relaxed); } + // Derived masquerade probes: each names the peer that really masquerades its + // destination, so stage 3 must confirm it. This is the hit path -- a verification table + // that matched nothing would still pass every "must drop" assertion in this suite. + for probe in &built.masquerade_probes { + let want = reference.lookup(&probe.input()); + assert_eq!( + dpdk.lookup(&probe.input()), + want, + "backends disagree on derived masquerade probe {probe:?}\nspec: {overlay_spec:?}", + ); + assert_eq!( + want, + LookupResult::MasqueradeDestination(probe.flow_dst_vpcd), + "a masquerade destination did not verify against its own peer: {probe:?}\nspec: {overlay_spec:?}", + ); + MASQUERADE_DESTINATIONS.fetch_add(1, Ordering::Relaxed); + } + let probes: Vec = probe_specs .iter() .map(|p| p.resolve(built.blocks)) .collect(); let inputs: Vec = probes .iter() - .map(|p| LookupInput { - src_vpcd: p.src_vpcd, - src_ip: p.src_ip, - dst_ip: p.dst_ip, - proto: p.proto, - ports: p.ports, - }) + .map(|p| p.input()) .collect(); let mut expected = Vec::with_capacity(probes.len()); for probe in &probes { - let want = reference.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports, - ); + let want = reference.lookup(&probe.input()); assert_eq!( - dpdk.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports - ), + dpdk.lookup(&probe.input()), want, "backends disagree on single lookup of {probe:?}\nspec: {overlay_spec:?}", ); match want { LookupResult::Route(_) => ROUTES.fetch_add(1, Ordering::Relaxed), + LookupResult::MasqueradeDestination(_) => { + MASQUERADE_DESTINATIONS.fetch_add(1, Ordering::Relaxed) + } LookupResult::SourceMiss(_) => SOURCE_MISSES.fetch_add(1, Ordering::Relaxed), LookupResult::DestinationMiss => { DESTINATION_MISSES.fetch_add(1, Ordering::Relaxed) @@ -263,6 +309,7 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { // 33 inputs of one version make the first chunk a full MAX_BATCH. let all_miss: Vec = (0..33u8) .map(|i| LookupInput { + flow_dst_vpcd: None, src_vpcd: bogus_vpcd(), src_ip: format!("10.0.0.{i}").parse().unwrap(), dst_ip: "10.0.0.99".parse().unwrap(), @@ -279,11 +326,16 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { }); eprintln!( - "coverage: {} routes, {} source misses, {} destination misses", + "coverage: {} routes, {} masquerade destinations, {} source misses, {} destination misses", ROUTES.load(Ordering::Relaxed), + MASQUERADE_DESTINATIONS.load(Ordering::Relaxed), SOURCE_MISSES.load(Ordering::Relaxed), DESTINATION_MISSES.load(Ordering::Relaxed), ); + assert!( + MASQUERADE_DESTINATIONS.load(Ordering::Relaxed) >= 1, + "no masquerade destination was ever reached, so stage 3 went untested", + ); assert!(ROUTES.load(Ordering::Relaxed) >= 1, "no full routes at all"); assert!( SOURCE_MISSES.load(Ordering::Relaxed) >= 4, @@ -313,29 +365,14 @@ fn batched_lookup_matches_single_lookup() { .iter() .map(|p| p.resolve(built.blocks)) .collect(); - let inputs: Vec = probes - .iter() - .map(|p| LookupInput { - src_vpcd: p.src_vpcd, - src_ip: p.src_ip, - dst_ip: p.dst_ip, - proto: p.proto, - ports: p.ports, - }) - .collect(); + let inputs: Vec = probes.iter().map(|p| p.input()).collect(); let mut out = vec![LookupResult::DestinationMiss; inputs.len()]; tables.lookup_batch(&inputs, &mut out); for (i, probe) in probes.iter().enumerate() { assert_eq!( out[i], - tables.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports - ), + tables.lookup(&probe.input()), "batch slot {i} != single lookup for {probe:?}\nspec: {overlay_spec:?}", ); } @@ -355,13 +392,7 @@ fn reference_lookup_matches_config_oracle() { for probe_spec in probe_specs { let probe = probe_spec.resolve(built.blocks); assert_eq!( - tables.lookup( - probe.src_vpcd, - probe.src_ip, - probe.dst_ip, - probe.proto, - probe.ports - ), + tables.lookup(&probe.input()), oracle_lookup(&built.overlay, &probe), "reference tables disagree with the config oracle on {probe:?}\nspec: {overlay_spec:?}", ); diff --git a/flow-filter/src/context/tables.rs b/flow-filter/src/context/tables.rs index 478f3ae235..208edd7327 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -66,6 +66,19 @@ type Route = (VpcDiscriminant, NatMode, NatMode); pub(crate) enum LookupResult { /// Both stages matched. Route(Route), + /// The destination is a masquerade public address. + /// + /// Carries a destination VPC only when the packet's flow named one and stage 3 confirmed that + /// this VPC masquerades this destination for this source. `None` means nothing vouched for the + /// packet -- no flow, or a flow whose VPC the configuration does not agree owns this address. + /// + /// This is separate from [`LookupResult::Route`] because a masquerade public address does not + /// identify a VPC on its own: config permits two peers to masquerade behind one range and + /// models the result as a set of routes per destination, which stage 1 cannot represent. The + /// destination here is therefore *verified*, never *resolved* -- see [`MasqueradeKey`]. A + /// masquerade destination also cannot accept a new connection, so there is no source NAT to + /// report: the flow carries the state such a packet needs. + MasqueradeDestination(Option), /// Stage 1 resolved the destination VPC, but the source matched nothing. SourceMiss(VpcDiscriminant), /// Stage 1 matched nothing: no peering covers this destination (also used for IP-version @@ -82,6 +95,12 @@ pub(crate) struct LookupInput { pub(crate) dst_ip: IpAddr, pub(crate) proto: NextHeader, pub(crate) ports: Option<(u16, u16)>, + /// The destination VPC recorded on the packet's flow, if it has one. + /// + /// An input to the lookup, not an output: it is the candidate stage 3 verifies when stage 1 + /// reports a masquerade destination. Carried here rather than consulted in the NF so the + /// verification stays on the batched path with the other two stages. + pub(crate) flow_dst_vpcd: Option, } /// Result of a stage-1 (remote/destination) match. @@ -100,6 +119,8 @@ struct Query { dst_ip: I, src_port: u16, dst_port: u16, + /// The candidate destination VPC from the packet's flow (see [`LookupInput::flow_dst_vpcd`]). + flow_dst_vni: Option, } /// Lower a config L4 protocol to a bitmask predicate: a specific protocol matches exactly (every @@ -162,6 +183,32 @@ pub(super) struct LocalKey { src_port: u16, } +/// Stage-3 key: "does `dst_vni` masquerade this destination for `src_vni`?" +/// +/// The question stage 1 cannot answer. A masquerade public address does not identify a VPC: config +/// lets two peers masquerade behind the same range (`VpcRoute::can_overlap` exempts +/// masquerade/masquerade), and models the result as a *set* of routes per destination +/// (`VpcRouteSet` is a `Vec`). Stage 1's `Verdict` holds one `dst_vpcd`, so lowering that set into +/// stage 1 collapses it and rte_acl's highest-priority-wins picks arbitrarily. +/// +/// So this key carries `dst_vni` as an *input* rather than producing it. "Which VPC owns this +/// address?" has no unique answer; "does this VPC own this address for me?" does. The candidate +/// comes from the packet's flow -- the only thing that can distinguish two connections to the same +/// masquerade address -- and this table says whether the configuration agrees. +#[derive(Debug, MatchKey, Clone, PartialEq, Eq)] +pub(super) struct MasqueradeKey { + #[mask] + proto: NextHeader, + #[exact] + src_vni: Vni, + #[exact] + dst_vni: Vni, + #[prefix] + dst_ip: I, + #[range] + dst_port: u16, +} + // ------------------------------------------------------------------------------------------------- // Backend selection. // @@ -508,12 +555,62 @@ fn emit_local( } } +/// Lower a stage-3 (masquerade-verification) rule into the v4 or v6 bucket according to its prefix. +fn emit_masquerade( + v4: &mut Vec, ()>>, + v6: &mut Vec, ()>>, + src_vni: Vni, + dst_vni: Vni, + ip_range: Prefix, + port_range: RangeSpec, + proto: MaskSpec, +) { + // Every rule here answers the same yes/no question, so nothing competes: the key pins both + // VNIs exactly, and within one peering a destination is covered by at most one masquerade + // expose. The priority is pure prefix length, as elsewhere. + let priority = rule_priority(ip_range, false); + match ip_range { + Prefix::IPV4(prefix) => { + let rule = MasqueradeKeyRule:: { + proto, + src_vni: ExactSpec::new(src_vni), + dst_vni: ExactSpec::new(dst_vni), + dst_ip: PrefixSpec::from(prefix), + dst_port: port_range, + }; + v4.push(NeutralRule { + priority, + fields: rule.into_backend_fields::(), + rule, + action: (), + }); + } + Prefix::IPV6(prefix) => { + let rule = MasqueradeKeyRule:: { + proto, + src_vni: ExactSpec::new(src_vni), + dst_vni: ExactSpec::new(dst_vni), + dst_ip: PrefixSpec::from(prefix), + dst_port: port_range, + }; + v6.push(NeutralRule { + priority, + fields: rule.into_backend_fields::(), + rule, + action: (), + }); + } + } +} + #[derive(Default)] struct RuleSet { remote_v4: Vec, Verdict>>, remote_v6: Vec, Verdict>>, local_v4: Vec, NatMode>>, local_v6: Vec, NatMode>>, + masquerade_v4: Vec, ()>>, + masquerade_v6: Vec, ()>>, } impl RuleSet { @@ -553,6 +650,20 @@ impl RuleSet { proto, action, ); + // Stage 3: the same prefix, keyed additionally on the peer's VNI, so a + // masquerade destination can be *verified* against a candidate VPC rather + // than resolved from the address (which two peers may share). + if action.nat_mode == Some(NatRequirement::Masquerade) { + emit_masquerade( + &mut rules.masquerade_v4, + &mut rules.masquerade_v6, + src_vni, + remote_vni, + prefix.prefix(), + prefix.into(), + proto, + ); + } } } if peering.remote().has_default_expose() { @@ -619,6 +730,8 @@ pub struct FlowFilterContext { pub(super) local_v4: AnyTable, NatMode>, pub(super) remote_v6: AnyTable, Verdict>, pub(super) local_v6: AnyTable, NatMode>, + pub(super) masquerade_v4: AnyTable, ()>, + pub(super) masquerade_v6: AnyTable, ()>, } impl Default for FlowFilterContext { @@ -628,6 +741,8 @@ impl Default for FlowFilterContext { local_v4: AnyTable::empty(), remote_v6: AnyTable::empty(), local_v6: AnyTable::empty(), + masquerade_v4: AnyTable::empty(), + masquerade_v6: AnyTable::empty(), } } } @@ -639,6 +754,8 @@ impl fmt::Debug for FlowFilterContext { .field("local_v4", &self.local_v4) .field("remote_v6", &self.remote_v6) .field("local_v6", &self.local_v6) + .field("masquerade_v4", &self.masquerade_v4) + .field("masquerade_v6", &self.masquerade_v6) .finish() } } @@ -656,53 +773,54 @@ impl FlowFilterContext { local_v4: build_table(backend, "local_v4", rules.local_v4)?, remote_v6: build_table(backend, "remote_v6", rules.remote_v6)?, local_v6: build_table(backend, "local_v6", rules.local_v6)?, + masquerade_v4: build_table(backend, "masq_v4", rules.masquerade_v4)?, + masquerade_v6: build_table(backend, "masq_v6", rules.masquerade_v6)?, }) } - // Single-key lookup: the readable per-packet oracle used by tests; production runs - // lookup_batch. The differential test cross-checks the two against each other. + /// Single-key lookup: the readable per-packet oracle used by tests; production runs + /// [`lookup_batch`](Self::lookup_batch). The differential test cross-checks the two against + /// each other, which is why both take the same [`LookupInput`] and share [`lookup_one`]'s + /// stage logic rather than restating it. #[cfg(test)] - pub(super) fn lookup( - &self, - src_vpcd: VpcDiscriminant, - src_ip: IpAddr, - dst_ip: IpAddr, - proto: NextHeader, - ports: Option<(u16, u16)>, - ) -> LookupResult { - let src_vni = key_vni(src_vpcd); - let (src_port, dst_port) = ports.unzip(); - let src_port = src_port.unwrap_or(0); - let dst_port = dst_port.unwrap_or(0); - - match (src_ip, dst_ip) { + pub(super) fn lookup(&self, input: &LookupInput) -> LookupResult { + let src_vni = key_vni(input.src_vpcd); + let (src_port, dst_port) = input.ports.unwrap_or((0, 0)); + let flow_dst_vni = input.flow_dst_vpcd.map(key_vni); + + match (input.src_ip, input.dst_ip) { (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => lookup_one( &self.remote_v4, &self.local_v4, + &self.masquerade_v4, &Query { src_vni, - proto, + proto: input.proto, src_ip, dst_ip, src_port, dst_port, + flow_dst_vni, }, ), (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => lookup_one( &self.remote_v6, &self.local_v6, + &self.masquerade_v6, &Query { src_vni, - proto, + proto: input.proto, src_ip, dst_ip, src_port, dst_port, + flow_dst_vni, }, ), _ => { debug!( - "Source and destination IP versions do not match: src_ip={src_ip:?}, dst_ip={dst_ip:?}", + "Source and destination IP versions do not match: src_ip={:?}, dst_ip={:?}", + input.src_ip, input.dst_ip, ); LookupResult::DestinationMiss } @@ -727,6 +845,7 @@ impl FlowFilterContext { let proto = input.proto; let src_vni = key_vni(input.src_vpcd); let (src_port, dst_port) = input.ports.unwrap_or((0, 0)); + let flow_dst_vni = input.flow_dst_vpcd.map(key_vni); match (input.src_ip, input.dst_ip) { (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => { v4_idx.push(i); @@ -737,6 +856,7 @@ impl FlowFilterContext { dst_ip, src_port, dst_port, + flow_dst_vni, }); } (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => { @@ -748,34 +868,49 @@ impl FlowFilterContext { dst_ip, src_port, dst_port, + flow_dst_vni, }); } _ => { /* version mismatch: leave "out[i] = DestinationMiss" */ } } } - lookup_versioned(&self.remote_v4, &self.local_v4, &v4_q, &v4_idx, out); - lookup_versioned(&self.remote_v6, &self.local_v6, &v6_q, &v6_idx, out); + lookup_versioned( + &self.remote_v4, + &self.local_v4, + &self.masquerade_v4, + &v4_q, + &v4_idx, + out, + ); + lookup_versioned( + &self.remote_v6, + &self.local_v6, + &self.masquerade_v6, + &v6_q, + &v6_idx, + out, + ); } } -/// Resolve one query against one IP version's tables: stage 1 (destination -> [`Verdict`]), then -/// stage 2 (source -> source NAT) for a stage-1 hit. -/// -/// The two versions ran identical bodies over differently-typed tables, so the lookup sequence was -/// stated twice and had to be edited twice. `Query` already carries exactly these fields for the -/// batched path; reusing it lets the single-key path say the sequence once. +/// One query's stages, for a single IP version: the unbatched twin of [`lookup_versioned`]. /// -/// Test-only, like its caller: production always runs the batched path. +/// Kept as its own function so the stage *logic* -- in particular which of stage 2 and stage 3 a +/// stage-1 hit needs -- is written once. The batched path differs only in gathering keys across a +/// burst before each rte_acl call; if the two ever disagree about the stages themselves, the +/// differential property test is comparing two different questions. #[cfg(test)] fn lookup_one( remote: &AnyTable, Verdict>, local: &AnyTable, NatMode>, + masquerade: &AnyTable, ()>, q: &Query, ) -> LookupResult where RemoteKey: MatchKey, LocalKey: MatchKey, + MasqueradeKey: MatchKey, { let Some(verdict) = remote.lookup(&RemoteKey { proto: q.proto, @@ -786,6 +921,24 @@ where return LookupResult::DestinationMiss; }; + // A masquerade destination is verified against the flow's candidate, never resolved from the + // address: see `MasqueradeKey`. No candidate, or one the configuration does not agree with, + // means nothing vouches for the packet. + if verdict.nat_mode == Some(NatRequirement::Masquerade) { + let verified = q.flow_dst_vni.filter(|dst_vni| { + masquerade + .lookup(&MasqueradeKey { + proto: q.proto, + src_vni: q.src_vni, + dst_vni: *dst_vni, + dst_ip: q.dst_ip, + dst_port: q.dst_port, + }) + .is_some() + }); + return LookupResult::MasqueradeDestination(verified.map(VpcDiscriminant::from_vni)); + } + match local.lookup(&LocalKey { proto: q.proto, src_vni: q.src_vni, @@ -798,18 +951,31 @@ where } } -/// The two-pass batched lookup for one IP version. `queries[k]` corresponds to output slot -/// `out[idx[k]]`. Runs in `MAX_BATCH`-sized rte_acl calls: stage 1 (destination -> [`Verdict`]), -/// then stage 2 (source -> source NAT) over the stage-1 hits only. +/// The batched lookup for one IP version. `queries[k]` corresponds to output slot `out[idx[k]]`. +/// Runs in `MAX_BATCH`-sized rte_acl calls: +/// +/// - stage 1 (destination -> [`Verdict`]) for every query; +/// - then, over the stage-1 hits, exactly one of: +/// - stage 3 (masquerade verification) when the verdict is a masquerade destination, because such +/// an address does not identify a VPC and the one stage 1 reports is an arbitrary pick among +/// the peers sharing it -- see [`MasqueradeKey`]; +/// - stage 2 (source -> source NAT) otherwise. +/// +/// The two are mutually exclusive: a masquerade destination cannot accept a new connection, so the +/// only traffic reaching it rides a flow that already carries its own NAT state, and no source NAT +/// needs resolving. Splitting them keeps both on the batched path and keeps each rte_acl call +/// asking a single question. fn lookup_versioned( remote: &AnyTable, Verdict>, local: &AnyTable, NatMode>, + masquerade: &AnyTable, ()>, queries: &[Query], idx: &[usize], out: &mut [LookupResult], ) where RemoteKey: MatchKey, LocalKey: MatchKey, + MasqueradeKey: MatchKey, { for (q_chunk, i_chunk) in queries.chunks(MAX_BATCH).zip(idx.chunks(MAX_BATCH)) { // Stage 1: destination -> Verdict. @@ -825,12 +991,31 @@ fn lookup_versioned( let mut verdicts: Vec> = vec![None; q_chunk.len()]; remote.lookup_batch(&remote_keys, &mut verdicts); - // Stage 2: for the hits only, source -> source NAT. + // Partition the stage-1 hits by which question they still need answered. let mut local_keys: Vec> = Vec::new(); let mut hit_pos: Vec = Vec::new(); + let mut masquerade_keys: Vec> = Vec::new(); + let mut masquerade_pos: Vec = Vec::new(); for (pos, verdict) in verdicts.iter().enumerate() { - if let Some(verdict) = verdict { - let q = &q_chunk[pos]; + let Some(verdict) = verdict else { continue }; + let q = &q_chunk[pos]; + if verdict.nat_mode == Some(NatRequirement::Masquerade) { + // Verify the flow's candidate rather than trusting the verdict's dst_vpcd. With no + // candidate there is nothing to verify, and nothing may pass: settle it here rather + // than sending a key the table cannot answer. + let Some(flow_dst_vni) = q.flow_dst_vni else { + out[i_chunk[pos]] = LookupResult::MasqueradeDestination(None); + continue; + }; + masquerade_keys.push(MasqueradeKey { + proto: q.proto, + src_vni: q.src_vni, + dst_vni: flow_dst_vni, + dst_ip: q.dst_ip, + dst_port: q.dst_port, + }); + masquerade_pos.push(pos); + } else { local_keys.push(LocalKey { proto: q.proto, src_vni: q.src_vni, @@ -841,6 +1026,22 @@ fn lookup_versioned( hit_pos.push(pos); } } + + // Stage 3: does the flow's VPC masquerade this destination for this source? + let mut verified: Vec> = vec![None; masquerade_keys.len()]; + masquerade.lookup_batch(&masquerade_keys, &mut verified); + for (hit, &pos) in masquerade_pos.iter().enumerate() { + let q = &q_chunk[pos]; + // A hit means the configuration agrees the flow's VPC owns this address; only then is + // the candidate promoted to the answer. + out[i_chunk[pos]] = LookupResult::MasqueradeDestination( + verified[hit] + .and(q.flow_dst_vni) + .map(VpcDiscriminant::from_vni), + ); + } + + // Stage 2: for the remaining hits, source -> source NAT. let mut nat_modes: Vec> = vec![None; local_keys.len()]; local.lookup_batch(&local_keys, &mut nat_modes); diff --git a/flow-filter/src/context/tests.rs b/flow-filter/src/context/tests.rs index c94dcd49bb..cbaa5aefcf 100644 --- a/flow-filter/src/context/tests.rs +++ b/flow-filter/src/context/tests.rs @@ -5,8 +5,8 @@ #![cfg(test)] -use super::LookupResult; use super::tables::RuleRow; +use super::{LookupInput, LookupResult}; use crate::test_utils::*; use crate::{FlowFilterContext, NatMode, NatRequirement}; use lpm::prefix::L4Protocol; @@ -38,13 +38,25 @@ fn route( .map(NonZero::get) .zip(t.dst_port().map(NonZero::get)) }); - match context.lookup(src_vpcd, src_ip, dst_ip, proto, ports) { + let input = LookupInput { + src_vpcd, + src_ip, + dst_ip, + proto, + ports, + flow_dst_vpcd: None, + }; + match context.lookup(&input) { LookupResult::Route((dst_vpcd, dst_nat, src_nat)) => Some(Route { dst_vpcd, dst_nat, src_nat, }), - LookupResult::SourceMiss(_) | LookupResult::DestinationMiss => None, + // This helper probes without a flow, so a masquerade destination has no candidate to + // verify and nothing may pass -- the same "no route" answer as a miss. + LookupResult::MasqueradeDestination(_) + | LookupResult::SourceMiss(_) + | LookupResult::DestinationMiss => None, } } @@ -285,16 +297,31 @@ fn dst_side_overlay() -> FlowFilterContext { fn dst_side_nat_modes() { let ctx = dst_side_overlay(); - // Masquerade destination: resolves at table level as a marker (the NF only lets it through - // for reply traffic on an established masquerade flow; see crate::tests) - let masq = route( - &ctx, - vpcd(100), - &build_tcp_packet(v4("10.0.0.5"), v4("70.0.0.10"), 1234, 5678), - ) - .expect("masquerade destination resolves as a marker"); - assert_eq!(masq.dst_vpcd, vpcd(200)); - assert_eq!(masq.dst_nat, Some(NatRequirement::Masquerade)); + // Masquerade destination: never a route. The address alone cannot name a VPC (two peers may + // masquerade behind one range), so the tables verify a candidate the packet's flow supplies. + let masq_input = |flow_dst_vpcd| LookupInput { + src_vpcd: vpcd(100), + src_ip: std::net::IpAddr::V4(v4("10.0.0.5")), + dst_ip: std::net::IpAddr::V4(v4("70.0.0.10")), + proto: net::ip::NextHeader::TCP, + ports: Some((1234, 5678)), + flow_dst_vpcd, + }; + // No candidate: nothing vouches for the packet. + assert_eq!( + ctx.lookup(&masq_input(None)), + LookupResult::MasqueradeDestination(None), + ); + // The peer that really masquerades this destination: confirmed. + assert_eq!( + ctx.lookup(&masq_input(Some(vpcd(200)))), + LookupResult::MasqueradeDestination(Some(vpcd(200))), + ); + // Any other VPC: refused, whether or not it is peered at all. + assert_eq!( + ctx.lookup(&masq_input(Some(vpcd(300)))), + LookupResult::MasqueradeDestination(None), + ); // Port-forwarding destination (matching proto + port): returned let pf = route( @@ -714,10 +741,17 @@ fn reference_and_dpdk_backends_agree() { ]; for &(vni, src_ip, dst_ip, proto, ports) in probes { - let src_vpcd = vpcd(vni); + let input = LookupInput { + src_vpcd: vpcd(vni), + src_ip, + dst_ip, + proto, + ports, + flow_dst_vpcd: None, + }; assert_eq!( - reference.lookup(src_vpcd, src_ip, dst_ip, proto, ports), - dpdk.lookup(src_vpcd, src_ip, dst_ip, proto, ports), + reference.lookup(&input), + dpdk.lookup(&input), "backends disagree on {src_ip} -> {dst_ip} ({proto:?}) from vni {vni}", ); } @@ -728,6 +762,7 @@ fn reference_and_dpdk_backends_agree() { let inputs: Vec = std::iter::repeat_n(probes, 5) .flatten() .map(|&(vni, src_ip, dst_ip, proto, ports)| LookupInput { + flow_dst_vpcd: None, src_vpcd: vpcd(vni), src_ip, dst_ip, @@ -744,13 +779,7 @@ fn reference_and_dpdk_backends_agree() { assert_eq!(ref_out, dpdk_out, "batched backends disagree"); for (i, input) in inputs.iter().enumerate() { - let single = reference.lookup( - input.src_vpcd, - input.src_ip, - input.dst_ip, - input.proto, - input.ports, - ); + let single = reference.lookup(input); assert_eq!(ref_out[i], single, "batched != single at index {i}"); } } diff --git a/flow-filter/src/fuzz_gen.rs b/flow-filter/src/fuzz_gen.rs index 4f68454886..ee18ec9635 100644 --- a/flow-filter/src/fuzz_gen.rs +++ b/flow-filter/src/fuzz_gen.rs @@ -25,6 +25,7 @@ #![cfg(test)] +use crate::context::LookupInput; use bolero::TypeGenerator; use config::external::overlay::vpc::{Vpc, VpcTable}; use config::external::overlay::vpcpeering::{VpcExpose, VpcManifest, VpcPeering, VpcPeeringTable}; @@ -106,6 +107,18 @@ impl ExposeSpec { ) } + /// Whether a destination this expose matches is a *masquerade* destination: one the tables can + /// only verify against a flow's candidate, never resolve from the address. Such a destination + /// never yields a route, so it needs its own derived probes. + fn dest_is_masquerade(self) -> bool { + matches!( + self, + ExposeSpec::Masquerade + | ExposeSpec::MasqueradeNestingPortFw(_) + | ExposeSpec::MasqueradeSameLenPortFw(_) + ) + } + /// Where a destination address this expose matches lives, or `None` if it only matches /// port-forwarded destinations (skipped -- those need a specific public port). `Some(true)` /// means the public block (NAT exposes translate destinations into it); `Some(false)` means the @@ -254,6 +267,10 @@ pub(crate) struct BuiltOverlay { pub(crate) overlay: ValidatedOverlay, pub(crate) blocks: u8, pub(crate) routing_probes: Vec, + /// Probes at a masquerade destination, each carrying the *correct* candidate on its flow. + /// Every one must verify, which is what keeps stage 3's hit path exercised: a verification + /// table that matched nothing would still satisfy every "this must drop" assertion elsewhere. + pub(crate) masquerade_probes: Vec, } impl OverlaySpec { @@ -324,6 +341,7 @@ impl OverlaySpec { let mut peering_table = VpcPeeringTable::new(); let mut blocks: u8 = 0; let mut routing_probes = Vec::new(); + let mut masquerade_probes = Vec::new(); for (slot, peering) in spec.peerings.iter().enumerate() { let Some(peering) = peering else { continue }; let (a, b) = PEERING_PAIRS[slot]; @@ -335,7 +353,9 @@ impl OverlaySpec { let remote = build_manifest(&vpc_name(b), &peering.remote, peering.v6, &mut blocks); derive_routing_probes( &mut routing_probes, + &mut masquerade_probes, VNIS[a], + VNIS[b], peering.v6, (local_base, &peering.local), (remote_base, &peering.remote), @@ -359,6 +379,7 @@ impl OverlaySpec { overlay, blocks, routing_probes, + masquerade_probes, } } } @@ -366,14 +387,18 @@ impl OverlaySpec { /// Append one guaranteed-routing probe for each (source-capable local expose, matchable remote /// expose) pair of a peering. The source lands at host `.1` of a can-init private block and the /// destination at host `.1` of the peer's matching block +#[allow(clippy::too_many_arguments)] // internal builder; grouping the fields would not aid clarity fn derive_routing_probes( out: &mut Vec, + masquerade_out: &mut Vec, src_vni: u32, + remote_vni: u32, v6: bool, (local_base, local): (u8, &ManifestSpec), (remote_base, remote): (u8, &ManifestSpec), ) { let src_vpcd = VpcDiscriminant::from_vni(Vni::new_checked(src_vni).unwrap()); + let remote_vpcd = VpcDiscriminant::from_vni(Vni::new_checked(remote_vni).unwrap()); for (li, lspec) in local.expose_specs().enumerate() { if !lspec.source_capable() { continue; @@ -383,12 +408,29 @@ fn derive_routing_probes( let Some(dst_public) = rspec.dest_public_space() else { continue; }; + let dst_ip = block_addr(remote_base + ri as u8, 1, dst_public, v6); + if rspec.dest_is_masquerade() { + // A masquerade destination never routes. It is reachable only by reply traffic + // whose flow names this very peer, so the probe carries that candidate and must + // come back verified. + masquerade_out.push(Probe { + src_vpcd, + src_ip, + dst_ip, + proto: NextHeader::TCP, + ports: Some((1, 1)), + flow_dst_vpcd: Some(remote_vpcd), + }); + continue; + } out.push(Probe { src_vpcd, src_ip, - dst_ip: block_addr(remote_base + ri as u8, 1, dst_public, v6), + dst_ip, proto: NextHeader::TCP, ports: Some((1, 1)), + // No flow: these probes assert a route resolves from the tables alone. + flow_dst_vpcd: None, }); } } @@ -609,6 +651,8 @@ pub(crate) struct ProbeSpec { proto: ProbeProto, sport: PortSel, dport: PortSel, + /// Which VPC the packet's flow claims as its destination, if any. + flow_dst_sel: Option, } /// A resolved probe: the arguments of one route lookup. @@ -619,6 +663,23 @@ pub(crate) struct Probe { pub(crate) dst_ip: IpAddr, pub(crate) proto: NextHeader, pub(crate) ports: Option<(u16, u16)>, + /// The destination VPC the packet's flow claims, if any -- the candidate the tables verify for + /// a masquerade destination. Generated independently of the rest of the probe, so it covers + /// having no candidate, the right one, a wrong-but-real one, and one no VPC uses. + pub(crate) flow_dst_vpcd: Option, +} + +impl Probe { + pub(crate) fn input(&self) -> LookupInput { + LookupInput { + src_vpcd: self.src_vpcd, + src_ip: self.src_ip, + dst_ip: self.dst_ip, + proto: self.proto, + ports: self.ports, + flow_dst_vpcd: self.flow_dst_vpcd, + } + } } impl ProbeSpec { @@ -648,6 +709,13 @@ impl ProbeSpec { ProbeProto::Icmp => None, _ => Some((self.sport.resolve(), self.dport.resolve())), }, + flow_dst_vpcd: self.flow_dst_sel.map(|sel| { + let vni = match sel as usize % (VNIS.len() + 1) { + i if i < VNIS.len() => VNIS[i], + _ => BOGUS_VNI, + }; + VpcDiscriminant::from_vni(Vni::new_checked(vni).unwrap()) + }), } } } diff --git a/flow-filter/src/lib.rs b/flow-filter/src/lib.rs index c2250bde67..3fa78111b4 100644 --- a/flow-filter/src/lib.rs +++ b/flow-filter/src/lib.rs @@ -148,6 +148,11 @@ impl FlowFilter { .map(NonZero::get) .zip(t.dst_port().map(NonZero::get)) }), + // The candidate destination for a masquerade destination, which the tables verify + // rather than resolve. Passed in even when the flow is stale or inactive: the tables + // only answer whether the configuration permits it, and this function re-checks the + // flow's own state before acting on the answer. + flow_dst_vpcd: attached_flow.as_ref().and_then(|flow| flow.dst_vpcd), }; Classification::Lookup { input, @@ -174,6 +179,30 @@ impl FlowFilter { let nfi = &self.name; let (dst_vpcd, dst_nat_mode, src_nat_mode) = match result { LookupResult::Route(route) => route, + // A masquerade destination cannot accept a new connection, so only reply traffic on an + // established flow may pass -- and only towards the VPC the tables confirmed + // masquerades this destination for this source. The tables cannot name that VPC from + // the address alone (two peers may masquerade behind one range), so they verified the + // candidate the flow supplied; `None` means no candidate, or one the configuration + // does not agree with. Either way nothing vouches for the packet. + LookupResult::MasqueradeDestination(verified) => { + if let Some(dst_vpcd) = verified + && let Some(flow) = + active_stateful_flow(flow_summary, dst_vpcd, |f| f.needs_masquerade) + { + debug!( + "{nfi}: Masquerade destination {dst_vpcd} confirmed by established flow" + ); + Self::tag_for_bypass(packet.meta_mut(), dst_vpcd, flow); + return; + } + debug!( + "{nfi}: Masquerade destination with no flow vouching for it, dropping packet (cannot initiate a connection towards a masquerade expose)" + ); + packet.invalidate_flows(); + packet.done(DoneReason::Filtered); + return; + } LookupResult::SourceMiss(dst_vpcd) => { // Port-forwarding sources are deliberately absent from the local tables; reply // traffic from one rides its established flow. @@ -197,24 +226,6 @@ impl FlowFilter { } }; - // A masquerade destination cannot accept new connections; its rule is in the table only - // so that reply traffic on an established masquerade flow is distinguishable from a - // destination no peering covers. - if dst_nat_mode == Some(NatRequirement::Masquerade) { - if let Some(flow) = active_stateful_flow(flow_summary, dst_vpcd, |f| f.needs_masquerade) - { - debug!("{nfi}: Masquerade destination allowed by established flow"); - Self::tag_for_bypass(packet.meta_mut(), dst_vpcd, flow); - return; - } - debug!( - "{nfi}: Masquerade destination with no established flow, dropping packet (cannot initiate a connection towards a masquerade expose)" - ); - packet.invalidate_flows(); - packet.done(DoneReason::Filtered); - return; - } - debug!( "{nfi}: Packet matches peering configuration, found VPC {dst_vpcd} and NAT modes {src_nat_mode:?} (src), {dst_nat_mode:?} (dst)" ); diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 232bf87031..85cfd704e6 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -1101,13 +1101,14 @@ enum NfOutcome { fn expected_outcome(result: LookupResult) -> NfOutcome { let (dst_vpcd, dst_nat, src_nat) = match result { LookupResult::Route(route) => route, - LookupResult::SourceMiss(_) | LookupResult::DestinationMiss => { + // A masquerade destination needs a flow to vouch for it, and these suites run flowless + // packets, so it always drops -- as do both misses. + LookupResult::MasqueradeDestination(_) + | LookupResult::SourceMiss(_) + | LookupResult::DestinationMiss => { return NfOutcome::Dropped(Some(DoneReason::Filtered)); } }; - if dst_nat == Some(NatRequirement::Masquerade) { - return NfOutcome::Dropped(Some(DoneReason::Filtered)); - } let masquerade = src_nat == Some(NatRequirement::Masquerade); let static_nat_src = src_nat == Some(NatRequirement::Static); @@ -1146,6 +1147,8 @@ fn probe_from_packet(pkt: &Packet, src_vpcd: VpcDiscriminant) -> Opt .map(NonZero::get) .zip(t.dst_port().map(NonZero::get)) }), + // These suites run flowless packets, so there is no candidate to verify. + flow_dst_vpcd: None, }) } @@ -1788,3 +1791,123 @@ fn flow_from_a_newer_generation_is_honored_for_bypass() { "the bypass path must not invalidate the flow it just honoured", ); } + +// ------------------------------------------------------------------------------------------------- +// Cross-peering masquerade overlap. +// +// Config lets two peers masquerade behind the same public range (`VpcRoute::can_overlap` exempts +// masquerade/masquerade) and models the result as a *set* of routes per destination +// (`VpcRouteSet` is a `Vec`). Stage 1 holds one destination VPC per match, so lowering that set +// into it collapses the set and rte_acl's highest-priority-wins picks arbitrarily -- which used to +// mean every masquerade reply toward the losing peer was dropped and its flow torn down. +// +// The destination for such traffic is now verified against the candidate the packet's flow names, +// never resolved from the address. + +/// vpc1 peers with vpc2 and vpc3; both peers masquerade behind the *same* public range. +fn shared_masquerade_range_context() -> FlowFilterContext { + context( + &[("vpc1", 100), ("vpc2", 200), ("vpc3", 300)], + vec![ + peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("10.0.0.0/24")]), + ( + "vpc2", + vec![expose_masquerade("192.168.0.0/24", "20.0.0.0/24")], + ), + ), + peering( + "vpc1-to-vpc3", + ("vpc1", vec![expose("10.0.0.0/24")]), + ( + "vpc3", + vec![expose_masquerade("192.168.1.0/24", "20.0.0.0/24")], + ), + ), + ], + ) +} + +/// Reply traffic reaches *whichever* peer the flow names, not whichever rule happens to sort first. +/// +/// Both directions must work. Before the verification stage one of these two was always dropped and +/// its flow cancelled -- deterministically, but arbitrarily, and differently under rte_acl than +/// under the reference backend, since equal-priority ties are unspecified there. +#[test] +fn masquerade_replies_reach_both_peers_sharing_a_public_range() { + for peer in [200u32, 300u32] { + let (mut flow_filter, _writer) = make_flow_filter(shared_masquerade_range_context()); + // Advance the generation so the flow is outdated and the bypass in `classify` is refused: + // this must be answered by the tables, which is where the ambiguity lived. + set_genid(&mut flow_filter, 5); + + let mut p = packet( + Some(vpcd(100)), + build_tcp_packet(v4("10.0.0.5"), v4("20.0.0.5"), 1234, 80), + ); + let flow = attach_flow(&mut p, Some(vpcd(peer)), true, true, false); + let out = run(&mut flow_filter, p); + + assert!( + !out.is_done(), + "reply on an established masquerade flow to VNI {peer} was dropped: {:?}", + out.get_done(), + ); + assert_eq!( + out.meta().dst_vpcd, + Some(vpcd(peer)), + "reply was sent to the wrong peer", + ); + assert!(out.meta().requires_masquerade()); + assert_ne!( + flow.status(), + FlowStatus::Cancelled, + "a valid flow to VNI {peer} was torn down", + ); + } +} + +/// A flow naming a VPC that does not masquerade the destination is still refused. +/// +/// This is the staleness check the old destination-equality comparison was also providing, and it +/// must survive: verification asks the configuration, so a candidate the configuration does not +/// agree with is rejected exactly as an unrelated one is. vpc3 masquerades `20.0.0.0/24` but not +/// `40.0.0.0/24`, so a flow claiming vpc3 cannot carry traffic to the latter. +#[test] +fn masquerade_reply_is_refused_when_the_flow_names_the_wrong_peer() { + let ctx = context( + &[("vpc1", 100), ("vpc2", 200), ("vpc3", 300)], + vec![ + peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("10.0.0.0/24")]), + ( + "vpc2", + vec![expose_masquerade("192.168.0.0/24", "40.0.0.0/24")], + ), + ), + peering( + "vpc1-to-vpc3", + ("vpc1", vec![expose("10.0.0.0/24")]), + ( + "vpc3", + vec![expose_masquerade("192.168.1.0/24", "20.0.0.0/24")], + ), + ), + ], + ); + let (mut flow_filter, _writer) = make_flow_filter(ctx); + set_genid(&mut flow_filter, 5); + + let mut p = packet( + Some(vpcd(100)), + build_tcp_packet(v4("10.0.0.5"), v4("40.0.0.5"), 1234, 80), + ); + // The destination belongs to vpc2's masquerade range; the flow claims vpc3. + let flow = attach_flow(&mut p, Some(vpcd(300)), true, true, false); + let out = run(&mut flow_filter, p); + + assert_eq!(out.get_done(), Some(DoneReason::Filtered)); + assert_eq!(flow.status(), FlowStatus::Cancelled); +} From 5194e24007e189ddad81cdb12967eef10bfa6d84 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 20:43:31 -0600 Subject: [PATCH 8/8] test(flow-filter): generate cross-peering masquerade overlaps The previous fix made a masquerade destination something the tables verify against the flow's candidate rather than resolve from the address. Nothing in the generated overlays exercised why: the prefix pool gives every expose its own block, so two peers never shared a masquerade range and every destination stayed unambiguous. Mutating the verification back to the old "flow must equal stage 1's arbitrary winner" rule was caught only by the hand-written regression test. `OverlaySpec::shared_masquerade_pool` builds the shape. Plain-masquerade exposes translate into one range shared across the whole overlay, so peers of the same VPC advertise overlapping masquerade destinations -- the one-to-many relation config permits and `VpcRouteSet` already models. Two things this needed, each a lesson about the generator: - Left to chance the overlap appeared in well under 1% of overlays, since it needs two peerings of one VPC to *both* draw a plain masquerade expose. Within the mode it is now arranged: each peering's remote side leads with masquerade and the local side is stripped of stateful NAT (config forbids it on both sides, and normalization would otherwise undo the masquerade). ~14k ambiguous destinations per 60s run. - No probe could reach the shared pool. It sits outside the `0..blocks` range probe selectors are reduced into, so the ambiguous destinations were built and then never looked up. `ProbeSpec::dst_shared_masquerade` aims at it. The oracle needs nothing further: the preceding commit already dropped the VPC from `StageOne::Masquerade`, so two overlapping masquerade exposes compare equal and `consider` treats the tie as benign rather than as the ambiguity it fails on. The combined-masquerade variants keep their own block: they exist to pin the nested and equal-length overlaps, which moving their public side would dissolve. Mutation results, all four combinations: - both lookup paths reverted -> `reference_lookup_matches_config_oracle` fails in ~400 iterations, and the rte_acl differential fails - batched path only -> `batched_lookup_matches_single_lookup` fails, which is what keeps the two paths from drifting apart - either -> the hand-written regression test fails Before this commit only the last of those held. Co-Authored-By: Claude Opus 5 (1M context) --- flow-filter/src/context/fuzz.rs | 64 +++++++++++++-- flow-filter/src/fuzz_gen.rs | 139 +++++++++++++++++++++++++++----- flow-filter/src/tests.rs | 2 +- 3 files changed, 180 insertions(+), 25 deletions(-) diff --git a/flow-filter/src/context/fuzz.rs b/flow-filter/src/context/fuzz.rs index 5a112a788c..d022990430 100644 --- a/flow-filter/src/context/fuzz.rs +++ b/flow-filter/src/context/fuzz.rs @@ -267,7 +267,7 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { let probes: Vec = probe_specs .iter() - .map(|p| p.resolve(built.blocks)) + .map(|p| p.resolve(&built)) .collect(); let inputs: Vec = probes .iter() @@ -361,10 +361,7 @@ fn batched_lookup_matches_single_lookup() { let tables = FlowFilterContext::build(&built.overlay, Backend::Reference) .expect("reference build"); - let probes: Vec = probe_specs - .iter() - .map(|p| p.resolve(built.blocks)) - .collect(); + let probes: Vec = probe_specs.iter().map(|p| p.resolve(&built)).collect(); let inputs: Vec = probes.iter().map(|p| p.input()).collect(); let mut out = vec![LookupResult::DestinationMiss; inputs.len()]; @@ -390,7 +387,7 @@ fn reference_lookup_matches_config_oracle() { let tables = FlowFilterContext::build(&built.overlay, Backend::Reference).expect("reference build"); for probe_spec in probe_specs { - let probe = probe_spec.resolve(built.blocks); + let probe = probe_spec.resolve(&built); assert_eq!( tables.lookup(&probe.input()), oracle_lookup(&built.overlay, &probe), @@ -451,3 +448,58 @@ fn exclusions_reach_the_config_as_multi_length_prefix_fans() { the generator is emitting single-block exposes and the priority ordering is untested", ); } + +/// Generated overlays really do contain cross-peering masquerade overlaps. +/// +/// The shape this whole verification stage exists for: one VPC with two peers masquerading behind +/// the same public range, so a destination address maps to two VPCs at once. Config permits it and +/// models it as a set of routes per destination; stage 1 cannot represent that, which is why the +/// destination is verified against the flow rather than resolved. +/// +/// Asserted because the generator used to exclude this deliberately. Without it every property +/// above passes on overlay shapes where a destination happens to be unambiguous, and the ambiguity +/// -- the only thing stage 3 exists to handle -- is never built. +#[test] +fn cross_peering_masquerade_overlaps_are_generated() { + // Lazily initialized so this compiles under the loom backend, whose AtomicU64::new is not const. + static OVERLAPS: LazyLock = LazyLock::new(|| AtomicU64::new(0)); + + bolero::check!() + .with_type::() + .for_each(|overlay_spec| { + let built = overlay_spec.build(); + for vpc in built.overlay.vpc_table().values() { + // Every masquerade destination this VPC can address, with the peer advertising it. + let mut destinations: Vec<(Prefix, VpcDiscriminant)> = Vec::new(); + for peering in vpc.peerings() { + let dst_vpcd = VpcDiscriminant::from_vni(peering.remote_vni()); + for expose in peering.remote().valexp() { + if !expose.has_masquerade() { + continue; + } + for prefix in expose.public_ips() { + destinations.push((prefix.prefix(), dst_vpcd)); + } + } + } + // An overlap is the same destination claimed by two different peers -- exactly the + // case where the address alone cannot name the destination VPC. + let overlapping = destinations.iter().enumerate().any(|(i, (prefix, vpcd))| { + destinations[i + 1..] + .iter() + .any(|(other, other_vpcd)| other == prefix && other_vpcd != vpcd) + }); + if overlapping { + OVERLAPS.fetch_add(1, Ordering::Relaxed); + } + } + }); + + let overlaps = OVERLAPS.load(Ordering::Relaxed); + eprintln!("coverage: {overlaps} VPCs facing an ambiguous masquerade destination"); + assert!( + overlaps >= 1, + "no cross-peering masquerade overlap was ever generated; \ + the verification stage is untested by the property suite", + ); +} diff --git a/flow-filter/src/fuzz_gen.rs b/flow-filter/src/fuzz_gen.rs index ee18ec9635..5dd2e61e17 100644 --- a/flow-filter/src/fuzz_gen.rs +++ b/flow-filter/src/fuzz_gen.rs @@ -74,7 +74,7 @@ impl FwProto { } } -#[derive(Debug, Clone, Copy, TypeGenerator)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, TypeGenerator)] pub(crate) enum ExposeSpec { Plain, StaticNat, @@ -259,6 +259,14 @@ pub(crate) struct PeeringSpec { #[derive(Debug, Clone, Copy, TypeGenerator)] pub(crate) struct OverlaySpec { peerings: [Option; 4], + /// Put every plain-masquerade expose's public side in one shared range, so peers of the same + /// VPC advertise overlapping masquerade destinations. + /// + /// Overlay-level rather than per-peering because the overlap only exists between *two* + /// peerings of one VPC. Config permits it (`VpcRoute::can_overlap` exempts + /// masquerade/masquerade) and it is the one destination the tables cannot resolve from the + /// address -- only verify against a flow's candidate. + shared_masquerade_pool: bool, } /// A materialized overlay plus the number of allocated prefix blocks (probe specs map their @@ -266,6 +274,9 @@ pub(crate) struct OverlaySpec { pub(crate) struct BuiltOverlay { pub(crate) overlay: ValidatedOverlay, pub(crate) blocks: u8, + /// Whether masquerade destinations live in the shared pool, so probes know the block is worth + /// addressing at all. + pub(crate) shared_masquerade_pool: bool, pub(crate) routing_probes: Vec, /// Probes at a masquerade destination, each carrying the *correct* candidate on its flow. /// Every one must verify, which is what keeps stage 3's hit path exercised: a verification @@ -299,6 +310,38 @@ impl OverlaySpec { manifest.exposes[0] = Some(ExposeEntry::plain()); } } + // Shared-pool mode. The overlap only exists when *two* peers of one VPC masquerade + // behind the same range, and left to chance that is well under 1% of generated + // overlays -- far too rare to catch a regression. So within this mode it is arranged + // rather than hoped for: every peering's remote side leads with a plain masquerade + // expose, which puts every peer of a given VPC on the shared range. + // + // The local side is stripped of stateful NAT because config forbids it on both sides of + // a peering, and the normalization below would otherwise undo the masquerade just + // installed. The second expose slot is left alone, so manifests keep some variety, and + // the other half of generated overlays are untouched by any of this. + if spec.shared_masquerade_pool { + peering.local.strip_stateful_nat(); + peering.remote.exposes[0] = Some(ExposeEntry { + spec: ExposeSpec::Masquerade, + exclude: peering.remote.exposes[0] + .map_or(ExcludeSel::None, |entry| entry.exclude), + }); + // Two shared-pool masquerade exposes in one manifest would claim the same public + // range, which `check_public_prefixes_dont_overlap` rejects. The overlap we want is + // across peerings, so keep at most one per manifest. + for manifest in [&mut peering.local, &mut peering.remote] { + let mut seen = false; + for entry in manifest.exposes.iter_mut().flatten() { + if entry.spec == ExposeSpec::Masquerade { + if seen { + entry.spec = ExposeSpec::StaticNat; + } + seen = true; + } + } + } + } // A default expose cannot face another default expose within one peering. if peering.local.default && peering.remote.default { peering.remote.drop_default(); @@ -348,15 +391,28 @@ impl OverlaySpec { // build_manifest assigns one block per expose spec, in order, so the block index of // expose spec `i` is `base + i`. Record each side's base before it advances. let local_base = blocks; - let local = build_manifest(&vpc_name(a), &peering.local, peering.v6, &mut blocks); + let local = build_manifest( + &vpc_name(a), + &peering.local, + peering.v6, + spec.shared_masquerade_pool, + &mut blocks, + ); let remote_base = blocks; - let remote = build_manifest(&vpc_name(b), &peering.remote, peering.v6, &mut blocks); + let remote = build_manifest( + &vpc_name(b), + &peering.remote, + peering.v6, + spec.shared_masquerade_pool, + &mut blocks, + ); derive_routing_probes( &mut routing_probes, &mut masquerade_probes, VNIS[a], VNIS[b], peering.v6, + spec.shared_masquerade_pool, (local_base, &peering.local), (remote_base, &peering.remote), ); @@ -378,6 +434,7 @@ impl OverlaySpec { BuiltOverlay { overlay, blocks, + shared_masquerade_pool: spec.shared_masquerade_pool, routing_probes, masquerade_probes, } @@ -394,6 +451,7 @@ fn derive_routing_probes( src_vni: u32, remote_vni: u32, v6: bool, + shared_masquerade_pool: bool, (local_base, local): (u8, &ManifestSpec), (remote_base, remote): (u8, &ManifestSpec), ) { @@ -408,7 +466,12 @@ fn derive_routing_probes( let Some(dst_public) = rspec.dest_public_space() else { continue; }; - let dst_ip = block_addr(remote_base + ri as u8, 1, dst_public, v6); + let dst_block = if rspec == ExposeSpec::Masquerade && shared_masquerade_pool { + SHARED_MASQUERADE_BLOCK + } else { + remote_base + ri as u8 + }; + let dst_ip = block_addr(dst_block, 1, dst_public, v6); if rspec.dest_is_masquerade() { // A masquerade destination never routes. It is reachable only by reply traffic // whose flow names this very peer, so the probe carries that candidate and must @@ -440,7 +503,13 @@ fn vpc_name(index: usize) -> String { format!("vpc{}", index + 1) } -fn build_manifest(vpc_name: &str, spec: &ManifestSpec, v6: bool, blocks: &mut u8) -> VpcManifest { +fn build_manifest( + vpc_name: &str, + spec: &ManifestSpec, + v6: bool, + shared_masquerade_pool: bool, + blocks: &mut u8, +) -> VpcManifest { let mut exposes = Vec::new(); for entry in spec.entries() { let n = *blocks; @@ -451,18 +520,27 @@ fn build_manifest(vpc_name: &str, spec: &ManifestSpec, v6: bool, blocks: &mut u8 ExposeSpec::StaticNat => { exposes.push(excluding_both(static_nat(n, v6), n, v6, exclude)); } + // Only the plain variant joins the shared pool. The two combined variants exist to + // pin the nested and equal-length overlaps against their *own* block, which moving + // the public side would dissolve. + ExposeSpec::Masquerade if shared_masquerade_pool => { + // The public side lives in the shared block, so an exclusion keyed on `n` + // would not overlap it. Masquerade has no size-equality requirement between + // its two sides, so excluding from the private side alone is legal. + exposes.push(excluding(masquerade(n, v6, true), n, v6, exclude)); + } ExposeSpec::Masquerade => { - exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); + exposes.push(excluding_both(masquerade(n, v6, false), n, v6, exclude)); } // Port forwarding forbids exclusion prefixes outright, so only the masquerade half of // these pairs takes one -- which is also what keeps the nested overlap intact, since // every exclusion stays in the block's upper half and FW_HOST is in the lower. ExposeSpec::MasqueradeNestingPortFw(proto) => { - exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); + exposes.push(excluding_both(masquerade(n, v6, false), n, v6, exclude)); exposes.push(portfw_host(n, v6, proto)); } ExposeSpec::MasqueradeSameLenPortFw(proto) => { - exposes.push(excluding_both(masquerade(n, v6), n, v6, exclude)); + exposes.push(excluding_both(masquerade(n, v6, false), n, v6, exclude)); exposes.push(portfw_block(n, v6, proto)); } ExposeSpec::PortForwarding(proto) => exposes.push(portfw_host(n, v6, proto)), @@ -559,12 +637,31 @@ fn static_nat(n: u8, v6: bool) -> VpcExpose { .unwrap() } -fn masquerade(n: u8, v6: bool) -> VpcExpose { +/// The public block every shared-pool masquerade expose translates into. +/// +/// Deliberately outside the per-expose pool (`blocks` never reaches 200), so it collides with +/// nothing except other shared-pool masquerade exposes -- which is the entire point. +const SHARED_MASQUERADE_BLOCK: u8 = 200; + +/// A masquerade expose. `shared_pool` puts its *public* side in a range every other shared-pool +/// masquerade expose also uses, while leaving its private side in its own block. +/// +/// That is the cross-peering overlap config permits and `VpcRoute::can_overlap` exempts: two peers +/// masquerading behind one public range. It is generated on purpose because the destination it +/// produces cannot be resolved from the address -- only verified against a flow's candidate -- and +/// a generator that never built one left the whole verification path untested by the property +/// suite. +fn masquerade(n: u8, v6: bool, shared_pool: bool) -> VpcExpose { + let public = if shared_pool { + SHARED_MASQUERADE_BLOCK + } else { + n + }; VpcExpose::empty() .make_masquerade(None) .unwrap() .ip(private_block(n, v6).as_str().into()) - .as_range(public_block(n, v6).as_str().into()) + .as_range(public_block(public, v6).as_str().into()) .unwrap() } @@ -646,6 +743,12 @@ pub(crate) struct ProbeSpec { src_public: bool, src_host: u8, dst_block: u8, + /// Aim the destination at the shared masquerade pool instead of a per-expose block. + /// + /// Without this no generated probe ever reaches a shared-pool masquerade destination -- the + /// pool sits outside the `0..blocks` range the selectors are reduced into -- so the ambiguous + /// destinations the overlay generator works to build would never actually be looked up. + dst_shared_masquerade: bool, dst_public: bool, dst_host: u8, proto: ProbeProto, @@ -683,8 +786,13 @@ impl Probe { } impl ProbeSpec { - pub(crate) fn resolve(&self, blocks: u8) -> Probe { - let nblocks = blocks.max(1); + pub(crate) fn resolve(&self, built: &BuiltOverlay) -> Probe { + let nblocks = built.blocks.max(1); + let dst_block = if self.dst_shared_masquerade && built.shared_masquerade_pool { + SHARED_MASQUERADE_BLOCK + } else { + self.dst_block % nblocks + }; let vni = match self.vni_sel as usize % (VNIS.len() + 1) { i if i < VNIS.len() => VNIS[i], _ => BOGUS_VNI, @@ -698,12 +806,7 @@ impl ProbeSpec { self.src_public, self.v6, ), - dst_ip: block_addr( - self.dst_block % nblocks, - self.dst_host, - self.dst_public, - dst_v6, - ), + dst_ip: block_addr(dst_block, self.dst_host, self.dst_public, dst_v6), proto: self.proto.next_header(), ports: match self.proto { ProbeProto::Icmp => None, diff --git a/flow-filter/src/tests.rs b/flow-filter/src/tests.rs index 85cfd704e6..c6251f08c1 100644 --- a/flow-filter/src/tests.rs +++ b/flow-filter/src/tests.rs @@ -1244,7 +1244,7 @@ fn nf_metadata_matches_config_oracle() { // The overlay's own derived probes route by construction, which is where the NAT flags // actually get exercised; the generated probes supply the misses and the edges. let derived = built.routing_probes.iter().copied(); - let generated = probe_specs.iter().map(|spec| spec.resolve(built.blocks)); + let generated = probe_specs.iter().map(|spec| spec.resolve(&built)); for probe in derived.chain(generated) { let Some((pkt, probe)) = probe_packet(&probe) else { continue;