From 572b1e04238cb5d1a0437d9774b068cfc87194ce Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 31 Jul 2026 16:06:52 -0600 Subject: [PATCH 1/3] test(acl-filter): compare lowering with config semantics Generate valid ACL overlays and compare reference-table lookups with an independent oracle over the validated config. Cover ordering, direction, prefix cross-products, protocols, metadata, IP versions, and defaults. Compare the same cases with rte_acl to cover backend encoding and priority. Coverage counters prevent vacuous short runs. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- Cargo.lock | 1 + acl-filter/Cargo.toml | 1 + acl-filter/src/fuzz.rs | 376 +++++++++++++++++++++++++ acl-filter/src/fuzz_gen.rs | 562 +++++++++++++++++++++++++++++++++++++ acl-filter/src/lib.rs | 4 + 5 files changed, 944 insertions(+) create mode 100644 acl-filter/src/fuzz.rs create mode 100644 acl-filter/src/fuzz_gen.rs diff --git a/Cargo.lock b/Cargo.lock index 630c9f6dd8..59f514d47e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1243,6 +1243,7 @@ dependencies = [ name = "dataplane-acl-filter" version = "0.24.0" dependencies = [ + "bolero", "dataplane-acl", "dataplane-common", "dataplane-concurrency", diff --git a/acl-filter/Cargo.toml b/acl-filter/Cargo.toml index 2dfc98aa11..232547ea03 100644 --- a/acl-filter/Cargo.toml +++ b/acl-filter/Cargo.toml @@ -26,6 +26,7 @@ tracing = { workspace = true } # EAL-free semantic suite. It is `cfg(test)`-gated in the source, so it is never part of a # production build; this dev-dep just makes `acl::reference` available to test builds. acl = { workspace = true, features = ["reference"] } +bolero = { workspace = true, features = ["std"] } dpdk = { workspace = true, features = ["test"] } flow-entry = { workspace = true } flow-filter = { workspace = true } diff --git a/acl-filter/src/fuzz.rs b/acl-filter/src/fuzz.rs new file mode 100644 index 0000000000..c52ffd97a2 --- /dev/null +++ b/acl-filter/src/fuzz.rs @@ -0,0 +1,376 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Property tests for ACL lowering and lookup. +//! +//! The oracle evaluates the validated config directly, without using lowered rules or a +//! classifier. This keeps lowering mistakes independent from the expected result. + +#![cfg(test)] + +use crate::PacketSummary; +use crate::context::{AclTables, Backend, LookupResult}; +use crate::fuzz_gen::{OverlaySpec, ProbeSpec, vni}; +use concurrency::sync::LazyLock; +use concurrency::sync::atomic::{AtomicU64, Ordering}; +use config::external::overlay::ValidatedOverlay; +use config::external::overlay::acl::{AclAction, AclProtoMatch, AclScope, ValidatedAclRule}; +use config::external::overlay::vpc::ValidatedPeering; +use lpm::prefix::{IpPrefix, Prefix, PrefixPortsSet, PrefixWithOptionalPorts}; +use net::ip::NextHeader; +use net::vxlan::Vni; +use std::net::IpAddr; + +// ------------------------------------------------------------------------------------------------- +// The config-semantics oracle. + +/// The outcome of the first matching config rule. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct OracleVerdict { + action: AclAction, + log: bool, + scope: AclScope, +} + +impl From<&LookupResult> for OracleVerdict { + fn from(result: &LookupResult) -> Self { + Self { + action: result.action, + log: result.log, + scope: result.scope, + } + } +} + +fn proto_allows(rule: AclProtoMatch, packet: NextHeader) -> bool { + match rule { + AclProtoMatch::Any => true, + AclProtoMatch::Tcp => packet == NextHeader::TCP, + AclProtoMatch::Udp => packet == NextHeader::UDP, + AclProtoMatch::Other(p) => packet == NextHeader::new(p), + } +} + +fn entry_allows(entry: &PrefixWithOptionalPorts, ip: IpAddr, port: u16) -> bool { + let covers = match (entry.prefix(), ip) { + (Prefix::IPV4(p), IpAddr::V4(a)) => p.covers_addr(&a), + (Prefix::IPV6(p), IpAddr::V6(a)) => p.covers_addr(&a), + _ => false, + }; + covers + && entry + .ports() + .is_none_or(|r| r.start() <= port && port <= r.end()) +} + +fn side_allows(set: &PrefixPortsSet, ip: IpAddr, port: u16) -> bool { + set.iter().any(|entry| entry_allows(entry, ip, port)) +} + +fn rule_matches(rule: &ValidatedAclRule, packet: &PacketSummary) -> bool { + // Match `AclTables::lookup`: cross-version packets consult neither table. + if packet.src_ip.is_ipv4() != packet.dst_ip.is_ipv4() { + return false; + } + let pattern = rule.pattern(); + let (sport, dport) = packet.ports.unwrap_or((0, 0)); + proto_allows(pattern.proto(), packet.proto) + && side_allows(pattern.src(), packet.src_ip, sport) + && side_allows(pattern.dst(), packet.dst_ip, dport) +} + +/// Find a peering in the packet's direction. +fn directed_peering( + overlay: &ValidatedOverlay, + src_vni: Vni, + dst_vni: Vni, +) -> Option<&ValidatedPeering> { + overlay + .vpc_table() + .values() + .find(|vpc| vpc.vni() == src_vni)? + .peerings() + .iter() + .find(|peering| peering.remote_vni() == dst_vni) +} + +/// Return the first matching rule from the directed peering. +/// Exact VNI fields prevent rules from other peerings or directions from competing. +fn oracle_lookup(overlay: &ValidatedOverlay, packet: &PacketSummary) -> Option { + let peering = directed_peering(overlay, packet.src_vni, packet.dst_vni)?; + let acl = peering.acl().as_ref()?; + acl.rules() + .iter() + .filter(|rule| rule.from() == peering.local().name()) + .find(|rule| rule_matches(rule, packet)) + .map(|rule| OracleVerdict { + action: rule.action(), + log: rule.log(), + scope: rule.scope(), + }) +} + +fn oracle_default_action( + overlay: &ValidatedOverlay, + src_vni: Vni, + dst_vni: Vni, +) -> Option { + Some( + directed_peering(overlay, src_vni, dst_vni)? + .acl() + .as_ref()? + .default_action(), + ) +} + +fn resolved_action(rule: Option, default: Option) -> AclAction { + rule.map_or_else(|| default.unwrap_or(AclAction::Allow), |v| v.action) +} + +// ------------------------------------------------------------------------------------------------- +// Properties. + +/// Ensure short property-test runs exercise each outcome. +struct Coverage { + rule_allows: LazyLock, + rule_denies: LazyLock, + default_falls: LazyLock, + unconfigured: LazyLock, +} + +impl Coverage { + const fn new() -> Self { + Self { + rule_allows: LazyLock::new(|| AtomicU64::new(0)), + rule_denies: LazyLock::new(|| AtomicU64::new(0)), + default_falls: LazyLock::new(|| AtomicU64::new(0)), + unconfigured: LazyLock::new(|| AtomicU64::new(0)), + } + } + + fn record(&self, verdict: Option, has_default: bool) { + let counter = match (verdict, has_default) { + (Some(v), _) if v.action == AclAction::Allow => &self.rule_allows, + (Some(_), _) => &self.rule_denies, + (None, true) => &self.default_falls, + (None, false) => &self.unconfigured, + }; + counter.fetch_add(1, Ordering::Relaxed); + } + + fn assert_reached(&self, label: &str) { + let (allows, denies) = ( + self.rule_allows.load(Ordering::Relaxed), + self.rule_denies.load(Ordering::Relaxed), + ); + let (defaults, unconfigured) = ( + self.default_falls.load(Ordering::Relaxed), + self.unconfigured.load(Ordering::Relaxed), + ); + eprintln!( + "{label} coverage: {allows} rule allows, {denies} rule denies, \ + {defaults} default fallbacks, {unconfigured} unconfigured pairs" + ); + assert!(allows >= 1, "{label}: no rule ever allowed a packet"); + assert!(denies >= 1, "{label}: no rule ever denied a packet"); + assert!(defaults >= 1, "{label}: never fell through to a default"); + assert!( + unconfigured >= 1, + "{label}: never probed a pair with no ACL at all" + ); + } +} + +/// Lowered reference tables must match the validated config. +#[test] +fn reference_lookup_matches_config_oracle() { + static COVERAGE: Coverage = Coverage::new(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; 8])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let tables = AclTables::build(&built.overlay, Backend::Reference) + .expect("reference backend build is infallible"); + + for probe_spec in probe_specs { + let probe = probe_spec.resolve(&built); + let want = oracle_lookup(&built.overlay, &probe); + let got = tables.lookup(&probe).map(OracleVerdict::from); + assert_eq!( + got, want, + "tables disagree with the config oracle on {probe:?}\nspec: {overlay_spec:?}", + ); + + // Defaults are lowered separately from rules, so compare them separately. + let want_default = + oracle_default_action(&built.overlay, probe.src_vni, probe.dst_vni); + let got_default = tables.find_default_action(probe.src_vni, probe.dst_vni); + assert_eq!( + got_default, want_default, + "default action disagrees for {} -> {}\nspec: {overlay_spec:?}", + probe.src_vni, probe.dst_vni, + ); + + assert_eq!( + resolved_action(got, got_default), + resolved_action(want, want_default), + "resolved action disagrees on {probe:?}\nspec: {overlay_spec:?}", + ); + + COVERAGE.record(want, want_default.is_some()); + } + }); + + COVERAGE.assert_reached("reference vs oracle"); +} + +/// Earlier matching rules must take precedence. +/// The DPDK differential test covers the priority encoding; the reference backend is first-match. +#[test] +fn earlier_rules_win_over_later_matching_rules() { + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; 8])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let tables = AclTables::build(&built.overlay, Backend::Reference) + .expect("reference backend build is infallible"); + + for probe_spec in probe_specs { + let probe = probe_spec.resolve(&built); + let Some(peering) = directed_peering(&built.overlay, probe.src_vni, probe.dst_vni) + else { + continue; + }; + let Some(acl) = peering.acl().as_ref() else { + continue; + }; + + let matching: Vec<&ValidatedAclRule> = acl + .rules() + .iter() + .filter(|rule| rule.from() == peering.local().name()) + .filter(|rule| rule_matches(rule, &probe)) + .collect(); + + let got = tables.lookup(&probe).map(OracleVerdict::from); + match matching.first() { + Some(first) => assert_eq!( + got, + Some(OracleVerdict { + action: first.action(), + log: first.log(), + scope: first.scope(), + }), + "a later rule beat the first of {} matching rules on {probe:?}\n\ + spec: {overlay_spec:?}", + matching.len(), + ), + None => assert_eq!( + got, None, + "table matched a rule the config says cannot match {probe:?}\n\ + spec: {overlay_spec:?}", + ), + } + } + }); +} + +/// Generated cases must behave identically under rte_acl and the reference backend. +#[test] +#[dpdk::with_eal] +fn dpdk_backend_matches_reference_on_generated_overlays() { + static COVERAGE: Coverage = Coverage::new(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; 8])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let reference = AclTables::build(&built.overlay, Backend::Reference) + .expect("reference backend build is infallible"); + let dpdk = + AclTables::build(&built.overlay, Backend::Dpdk).expect("rte_acl backend build"); + + for probe_spec in probe_specs { + let probe = probe_spec.resolve(&built); + let want = reference.lookup(&probe).map(OracleVerdict::from); + assert_eq!( + dpdk.lookup(&probe).map(OracleVerdict::from), + want, + "backends disagree on {probe:?}\nspec: {overlay_spec:?}", + ); + + let want_default = reference.find_default_action(probe.src_vni, probe.dst_vni); + assert_eq!( + dpdk.find_default_action(probe.src_vni, probe.dst_vni), + want_default, + "backends disagree on the default action for {} -> {}\nspec: {overlay_spec:?}", + probe.src_vni, + probe.dst_vni, + ); + + COVERAGE.record(want, want_default.is_some()); + } + }); + + COVERAGE.assert_reached("dpdk vs reference"); +} + +/// A default exists exactly when the directed peering has an ACL. +#[test] +fn absent_acl_and_absent_peering_have_no_default_action() { + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; 4])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let tables = AclTables::build(&built.overlay, Backend::Reference) + .expect("reference backend build is infallible"); + + for probe_spec in probe_specs { + let probe = probe_spec.resolve(&built); + let peering = directed_peering(&built.overlay, probe.src_vni, probe.dst_vni); + let configured = peering.is_some_and(|p| p.acl().is_some()); + assert_eq!( + tables + .find_default_action(probe.src_vni, probe.dst_vni) + .is_some(), + configured, + "a default action exists exactly when the pair is peered and has an ACL: \ + {} -> {}\nspec: {overlay_spec:?}", + probe.src_vni, + probe.dst_vni, + ); + if !configured { + assert_eq!( + tables.lookup(&probe).map(OracleVerdict::from), + None, + "an unconfigured pair must match no rule: {probe:?}\n\ + spec: {overlay_spec:?}", + ); + } + } + }); +} + +/// Keep the bogus probe VNI distinct from generated VPCs. +#[test] +fn generated_overlays_use_the_declared_vnis() { + use crate::fuzz_gen::VNIS; + + bolero::check!() + .with_type::() + .for_each(|overlay_spec| { + let built = overlay_spec.build(); + let vnis: Vec = built + .overlay + .vpc_table() + .values() + .map(|vpc| vpc.vni()) + .collect(); + assert_eq!( + vnis, + VNIS.iter().copied().map(vni).collect::>(), + "generated VPC table drifted from the declared VNIs", + ); + }); +} diff --git a/acl-filter/src/fuzz_gen.rs b/acl-filter/src/fuzz_gen.rs new file mode 100644 index 0000000000..56e9db5d20 --- /dev/null +++ b/acl-filter/src/fuzz_gen.rs @@ -0,0 +1,562 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Generators for ACL property tests. +//! +//! [`OverlaySpec::build`] normalizes a compact spec and validates the resulting config. Probes are +//! biased toward generated peerings so short runs exercise rule matches as well as misses. +//! +//! This remains separate from `flow-filter`'s generator: flow-filter needs disjoint prefix blocks, +//! while ACL ordering requires overlapping patterns. Invalid cross-version rules are out of scope +//! because config validation rejects them before lowering. + +#![cfg(test)] + +use crate::PacketSummary; +use bolero::TypeGenerator; +use config::external::overlay::acl::{ + Acl, AclAction, AclPattern, AclProtoMatch, AclRule, AclScope, +}; +use config::external::overlay::vpc::{Vpc, VpcTable}; +use config::external::overlay::vpcpeering::{VpcExpose, VpcManifest, VpcPeering, VpcPeeringTable}; +use config::external::overlay::{Overlay, ValidatedOverlay}; +use lpm::prefix::{PortRange, Prefix, PrefixPortsSet, PrefixWithOptionalPorts}; +use net::ip::NextHeader; +use net::vxlan::Vni; +use std::net::IpAddr; + +pub(crate) const VNIS: [u32; 3] = [100, 200, 300]; +/// A VNI outside [`VNIS`]. +const BOGUS_VNI: u32 = 999; + +/// The unique VPC pair assigned to each peering slot. +const PEERING_PAIRS: [(usize, usize); 3] = [(0, 1), (0, 2), (1, 2)]; + +pub(crate) fn vni(id: u32) -> Vni { + Vni::new_checked(id).unwrap_or_else(|e| unreachable!("{id} is a valid VNI: {e:?}")) +} + +// ------------------------------------------------------------------------------------------------- +// Prefix pool. +// +// Each peering side owns a distinct block; rule patterns overlap within it. + +fn private_block(n: u8, v6: bool) -> String { + if v6 { + format!("2001:db8:0:{n:x}::/120") + } else { + format!("10.{n}.0.0/24") + } +} + +fn public_block(n: u8, v6: bool) -> String { + if v6 { + format!("2001:db9:0:{n:x}::/120") + } else { + format!("20.{n}.0.0/24") + } +} + +/// An address inside block `n`. +pub(crate) fn block_addr(n: u8, host: u8, public: bool, v6: bool) -> IpAddr { + if v6 { + let net = if public { "db9" } else { "db8" }; + format!("2001:{net}:0:{n:x}::{host:x}") + .parse() + .unwrap_or_else(|e| unreachable!("generated v6 address must parse: {e}")) + } else { + let net = if public { 20 } else { 10 }; + format!("{net}.{n}.0.{host}") + .parse() + .unwrap_or_else(|e| unreachable!("generated v4 address must parse: {e}")) + } +} + +// ------------------------------------------------------------------------------------------------- +// Rule pattern selectors. + +/// An overlapping region within a generated block. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum PrefixSel { + Block, + LowerHalf, + UpperHalf, + Host(u8), +} + +impl PrefixSel { + fn resolve(self, n: u8, public: bool, v6: bool) -> Prefix { + let block = if public { + public_block(n, v6) + } else { + private_block(n, v6) + }; + let text = match (self, v6) { + (PrefixSel::Block, _) => block, + (PrefixSel::LowerHalf, false) => block.replace("/24", "/25"), + (PrefixSel::UpperHalf, false) => block.replace(".0.0/24", ".0.128/25"), + (PrefixSel::LowerHalf, true) => block.replace("/120", "/121"), + (PrefixSel::UpperHalf, true) => block.replace("::/120", "::80/121"), + (PrefixSel::Host(h), false) => block.replace(".0.0/24", &format!(".0.{h}/32")), + (PrefixSel::Host(h), true) => block.replace("::/120", &format!("::{h:x}/128")), + }; + Prefix::from(text.as_str()) + } +} + +/// One side of a rule pattern. Two entries make lowering's cross-product observable. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum SideSel { + /// Validation expands this to the manifest's coverage set. + All, + One(PrefixSel), + Two(PrefixSel, PrefixSel), +} + +impl SideSel { + fn resolve(self, n: u8, public: bool, v6: bool) -> Vec { + match self { + SideSel::All => Vec::new(), + SideSel::One(a) => vec![a.resolve(n, public, v6)], + SideSel::Two(a, b) => { + vec![a.resolve(n, public, v6), b.resolve(n, public, v6)] + } + } + } +} + +/// A generated TCP or UDP port constraint. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum PortSel { + None, + Low, + High, + /// A port nested inside `Low`. + Single(u8), +} + +impl PortSel { + fn resolve(self) -> Option { + let range = |lo: u16, hi: u16| { + PortRange::new(lo, hi).unwrap_or_else(|e| unreachable!("valid port range: {e:?}")) + }; + match self { + PortSel::None => None, + PortSel::Low => Some(range(1, 1023)), + PortSel::High => Some(range(1024, u16::MAX)), + PortSel::Single(k) => { + let port = 500 + u16::from(k); + Some(range(port, port)) + } + } + } +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum ProtoSel { + Any, + Tcp, + Udp, + /// A protocol number, including aliases for TCP and UDP. + Other(u8), +} + +impl ProtoSel { + fn to_config(self) -> AclProtoMatch { + match self { + ProtoSel::Any => AclProtoMatch::Any, + ProtoSel::Tcp => AclProtoMatch::Tcp, + ProtoSel::Udp => AclProtoMatch::Udp, + ProtoSel::Other(p) => AclProtoMatch::Other(p), + } + } + + fn allows_ports(self) -> bool { + matches!(self, ProtoSel::Tcp | ProtoSel::Udp) + } +} + +// ------------------------------------------------------------------------------------------------- +// Specs. + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct RuleSpec { + /// Apply the rule from remote to local. + reverse: bool, + allow: bool, + /// Request flow scope; normalization adds the required NAT. + flow_scope: bool, + log: bool, + proto: ProtoSel, + src: SideSel, + dst: SideSel, + src_ports: PortSel, + dst_ports: PortSel, +} + +/// Ensures a generated ACL is non-empty. +const FALLBACK_RULE: RuleSpec = RuleSpec { + reverse: false, + allow: true, + flow_scope: false, + log: false, + proto: ProtoSel::Any, + src: SideSel::All, + dst: SideSel::All, + src_ports: PortSel::None, + dst_ports: PortSel::None, +}; + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct AclSpec { + default_allow: bool, + /// Up to four rules in precedence order. + rules: [Option; 4], +} + +impl AclSpec { + fn rule_specs(&self) -> impl Iterator + '_ { + self.rules.iter().flatten().copied() + } +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct PeeringSpec { + /// IP version for both manifests. + v6: bool, + /// Masquerade changes a side's public prefix. + local_masq: bool, + remote_masq: bool, + /// `None` leaves the peering without an ACL. + acl: Option, +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct OverlaySpec { + peerings: [Option; 3], +} + +/// One direction of a generated peering, used to bias probes toward matches. +#[derive(Debug, Clone, Copy)] +pub(crate) struct Anchor { + src_vni: u32, + dst_vni: u32, + /// The source side's private block. + src_block: u8, + /// The destination side's advertised block. + dst_block: u8, + dst_public: bool, + v6: bool, +} + +/// A validated overlay and its probe metadata. +pub(crate) struct BuiltOverlay { + pub(crate) overlay: ValidatedOverlay, + pub(crate) blocks: u8, + pub(crate) anchors: Vec, +} + +impl OverlaySpec { + /// Normalize and validate the generated config. + pub(crate) fn build(&self) -> BuiltOverlay { + let mut spec = *self; + + // Keep at least one peering to probe. + if spec.peerings.iter().all(Option::is_none) { + spec.peerings[0] = Some(PeeringSpec { + v6: false, + local_masq: false, + remote_masq: false, + acl: Some(AclSpec { + default_allow: false, + rules: [Some(FALLBACK_RULE), None, None, None], + }), + }); + } + for peering in spec.peerings.iter_mut().flatten() { + if let Some(acl) = peering.acl.as_mut() + && acl.rules.iter().all(Option::is_none) + { + acl.rules[0] = Some(FALLBACK_RULE); + } + // Flow scope requires stateful NAT on one side. Stateful NAT cannot be on both sides. + let wants_flow_scope = peering + .acl + .iter() + .flat_map(AclSpec::rule_specs) + .any(|rule| rule.flow_scope); + if wants_flow_scope { + peering.local_masq = true; + } + if peering.local_masq { + peering.remote_masq = false; + } + } + + // Materialize. + let mut vpc_table = VpcTable::new(); + for (i, id) in VNIS.iter().enumerate() { + vpc_table + .add( + Vpc::new(&vpc_name(i), &format!("VPC{:02}", i + 1), *id) + .unwrap_or_else(|e| unreachable!("valid VPC: {e}")), + ) + .unwrap_or_else(|e| unreachable!("distinct VPCs: {e}")); + } + let mut peering_table = VpcPeeringTable::new(); + let mut blocks: u8 = 0; + let mut anchors = Vec::new(); + for (slot, peering) in spec.peerings.iter().enumerate() { + let Some(peering) = peering else { continue }; + let (a, b) = PEERING_PAIRS[slot]; + let local_block = blocks; + let remote_block = blocks + 1; + blocks += 2; + + // Anchor both rule directions. + anchors.push(Anchor { + src_vni: VNIS[a], + dst_vni: VNIS[b], + src_block: local_block, + dst_block: remote_block, + dst_public: peering.remote_masq, + v6: peering.v6, + }); + anchors.push(Anchor { + src_vni: VNIS[b], + dst_vni: VNIS[a], + src_block: remote_block, + dst_block: local_block, + dst_public: peering.local_masq, + v6: peering.v6, + }); + + let local = manifest(&vpc_name(a), local_block, peering.local_masq, peering.v6); + let remote = manifest(&vpc_name(b), remote_block, peering.remote_masq, peering.v6); + let mut built = VpcPeering::with_default_group( + &format!("{}-to-{}", vpc_name(a), vpc_name(b)), + local, + remote, + ); + built.acl = peering.acl.map(|acl| { + build_acl( + &acl, + (&vpc_name(a), local_block, peering.local_masq), + (&vpc_name(b), remote_block, peering.remote_masq), + peering.v6, + ) + }); + peering_table + .add(built) + .unwrap_or_else(|e| unreachable!("distinct peerings: {e}")); + } + + let overlay = Overlay::new(vpc_table, peering_table) + .validate() + .unwrap_or_else(|e| { + panic!( + "generated overlay must validate (generator/config drift): {e}\nspec: {spec:?}" + ) + }); + BuiltOverlay { + overlay, + blocks, + anchors, + } + } +} + +fn vpc_name(index: usize) -> String { + format!("vpc{}", index + 1) +} + +/// Build one plain or masqueraded expose over block `n`. +fn manifest(name: &str, n: u8, masquerade: bool, v6: bool) -> VpcManifest { + let expose = if masquerade { + VpcExpose::empty() + .make_masquerade(None) + .unwrap_or_else(|e| unreachable!("masquerade on an empty expose: {e}")) + .ip(private_block(n, v6).as_str().into()) + .as_range(public_block(n, v6).as_str().into()) + .unwrap_or_else(|e| unreachable!("equal-size public range: {e}")) + } else { + VpcExpose::empty().ip(private_block(n, v6).as_str().into()) + }; + VpcManifest::with_exposes(name, vec![expose]) +} + +fn build_acl(spec: &AclSpec, local: (&str, u8, bool), remote: (&str, u8, bool), v6: bool) -> Acl { + let (local_name, local_block, local_masq) = local; + let (remote_name, remote_block, remote_masq) = remote; + + let rules = spec + .rule_specs() + .enumerate() + .map(|(i, rule)| { + // Rules match the source's private space and destination's public space. + let ((from, src_block), (to, dst_block, dst_masq)) = if rule.reverse { + ( + (remote_name, remote_block), + (local_name, local_block, local_masq), + ) + } else { + ( + (local_name, local_block), + (remote_name, remote_block, remote_masq), + ) + }; + + let (src_ports, dst_ports) = if rule.proto.allows_ports() { + (rule.src_ports.resolve(), rule.dst_ports.resolve()) + } else { + (None, None) + }; + + AclRule { + name: format!("rule{i}"), + from: from.to_owned(), + to: to.to_owned(), + action: if rule.allow { + AclAction::Allow + } else { + AclAction::Deny + }, + pattern: AclPattern { + src: prefix_set(rule.src.resolve(src_block, false, v6), src_ports), + dst: prefix_set(rule.dst.resolve(dst_block, dst_masq, v6), dst_ports), + src_any_ports: Vec::new(), + dst_any_ports: Vec::new(), + proto: rule.proto.to_config(), + }, + scope: if rule.flow_scope { + AclScope::Flow + } else { + AclScope::Packet + }, + log: rule.log, + } + }) + .collect(); + + Acl::new( + if spec.default_allow { + AclAction::Allow + } else { + AclAction::Deny + }, + rules, + ) +} + +fn prefix_set(prefixes: Vec, ports: Option) -> PrefixPortsSet { + prefixes + .into_iter() + .map(|prefix| PrefixWithOptionalPorts::new(prefix, ports)) + .collect() +} + +// ------------------------------------------------------------------------------------------------- +// Probes. + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum ProbeProto { + Tcp, + Udp, + Icmp, + Other(u8), +} + +impl ProbeProto { + fn next_header(self) -> NextHeader { + match self { + ProbeProto::Tcp => NextHeader::TCP, + ProbeProto::Udp => NextHeader::UDP, + ProbeProto::Icmp => NextHeader::ICMP, + ProbeProto::Other(p) => NextHeader::new(p), + } + } + + fn has_ports(self) -> bool { + matches!(self, ProbeProto::Tcp | ProbeProto::Udp) + } +} + +/// A port biased toward generated range boundaries. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum ProbePort { + Exact(u16), + WellKnown(u8), + Nested(u8), +} + +impl ProbePort { + fn resolve(self) -> u16 { + match self { + ProbePort::Exact(p) => p, + ProbePort::WellKnown(k) => 1 + u16::from(k) * 4, + ProbePort::Nested(k) => 499 + u16::from(k), + } + } +} + +/// One deliberate departure from a peering anchor. +/// `Option` keeps half of probes on the match path. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum Stray { + SrcVni, + DstVni, + /// Use the reverse VNI pair without reversing addresses. + SwapVnis, + SrcBlock(u8), + DstBlock(u8), + SrcPublic, + CrossVersion, +} + +/// A generated packet relative to a peering anchor. +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct ProbeSpec { + anchor_sel: u8, + stray: Option, + src_host: u8, + dst_host: u8, + proto: ProbeProto, + sport: ProbePort, + dport: ProbePort, +} + +impl ProbeSpec { + pub(crate) fn resolve(&self, built: &BuiltOverlay) -> PacketSummary { + let anchor = built.anchors[self.anchor_sel as usize % built.anchors.len()]; + let nblocks = built.blocks.max(1); + + let mut src_vni = anchor.src_vni; + let mut dst_vni = anchor.dst_vni; + let mut src_block = anchor.src_block; + let mut dst_block = anchor.dst_block; + let mut src_public = false; + let mut dst_v6 = anchor.v6; + match self.stray { + None => {} + Some(Stray::SrcVni) => src_vni = BOGUS_VNI, + Some(Stray::DstVni) => dst_vni = BOGUS_VNI, + Some(Stray::SwapVnis) => std::mem::swap(&mut src_vni, &mut dst_vni), + Some(Stray::SrcBlock(b)) => src_block = b % nblocks, + Some(Stray::DstBlock(b)) => dst_block = b % nblocks, + Some(Stray::SrcPublic) => src_public = true, + Some(Stray::CrossVersion) => dst_v6 = !anchor.v6, + } + + PacketSummary { + src_vni: vni(src_vni), + dst_vni: vni(dst_vni), + src_ip: block_addr(src_block, self.src_host, src_public, anchor.v6), + dst_ip: block_addr(dst_block, self.dst_host, anchor.dst_public, dst_v6), + proto: self.proto.next_header(), + ports: self + .proto + .has_ports() + .then(|| (self.sport.resolve(), self.dport.resolve())), + } + } +} diff --git a/acl-filter/src/lib.rs b/acl-filter/src/lib.rs index aaafa896ff..3e17f71453 100644 --- a/acl-filter/src/lib.rs +++ b/acl-filter/src/lib.rs @@ -21,6 +21,10 @@ mod access; mod context; mod display; +#[cfg(test)] +mod fuzz; +#[cfg(test)] +mod fuzz_gen; #[cfg(test)] mod tests; From 39af3b9b2442f56e3afcd16551b564e78a71242b Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 16:33:58 -0600 Subject: [PATCH 2/3] fix(config): reject mixed-IP manifests Expose validation checks one expose at a time, so a manifest could still combine IPv4 and IPv6. Filters choose one table version per peering and could omit rules for the other version. Require one IP version across a manifest's non-default exposes. Add tests for both expose orders, default exposes, and valid single-version manifests. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- .../src/external/overlay/validation_tests.rs | 49 +++++++++++++++++++ config/src/external/overlay/vpcpeering.rs | 26 ++++++++++ flow-filter/src/fuzz_gen.rs | 6 +-- 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/config/src/external/overlay/validation_tests.rs b/config/src/external/overlay/validation_tests.rs index f33ba4fc7a..571811ebc8 100644 --- a/config/src/external/overlay/validation_tests.rs +++ b/config/src/external/overlay/validation_tests.rs @@ -741,6 +741,55 @@ mod test { // VpcManifest validation, overlap and NAT checks // ================================================================================== + // Reject mixed versions across exposes, not only within one expose. + #[test] + fn test_manifest_mixing_ip_versions_rejected() { + let mut manifest = VpcManifest::new("VPC-1"); + manifest.add_expose(VpcExpose::empty().ip("10.0.0.0/24".into())); + manifest.add_expose(VpcExpose::empty().ip("2001:db8::/32".into())); + let result = manifest.validate(); + assert!( + matches!(result, Err(ConfigError::Forbidden(_))), + "a manifest mixing IPv4 and IPv6 exposes must be rejected: {result:?}", + ); + } + + // The result must not depend on expose order. + #[test] + fn test_manifest_mixing_ip_versions_rejected_either_order() { + let mut manifest = VpcManifest::new("VPC-1"); + manifest.add_expose(VpcExpose::empty().ip("2001:db8::/32".into())); + manifest.add_expose(VpcExpose::empty().ip("10.0.0.0/24".into())); + assert!(matches!( + manifest.validate(), + Err(ConfigError::Forbidden(_)) + )); + } + + // Default exposes do not constrain the manifest's IP version. + #[test] + fn test_manifest_default_expose_does_not_constrain_ip_version() { + for ip in ["10.0.0.0/24", "2001:db8::/32"] { + let mut manifest = VpcManifest::new("VPC-1"); + manifest.add_expose(VpcExpose::empty().set_default()); + manifest.add_expose(VpcExpose::empty().ip(ip.into())); + let result = manifest.validate(); + assert!( + result.is_ok(), + "a default expose alongside {ip} must be accepted: {result:?}", + ); + } + } + + // IPv6-only manifests remain valid. + #[test] + fn test_manifest_single_ip_version_accepted() { + let mut v6 = VpcManifest::new("VPC-1"); + v6.add_expose(VpcExpose::empty().ip("2001:db8::/32".into())); + v6.add_expose(VpcExpose::empty().ip("2001:db9::/32".into())); + assert!(v6.validate().is_ok()); + } + // Two no-NAT exposes with disjoint ips passes #[test] fn test_no_nat_disjoint_ips_passes() { diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index fce928055e..d5f641a26c 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -692,6 +692,7 @@ impl VpcManifest { valid_manifest_candidate.valexp.push(expose.validate()?); } + valid_manifest_candidate.validate_single_ip_version()?; valid_manifest_candidate.validate_expose_collisions()?; Ok(valid_manifest_candidate) } @@ -776,6 +777,31 @@ impl ValidatedManifest { self.valexp.len() == 1 && self.valexp.first().is_some_and(ValidatedExpose::is_default) } + /// Reject manifests containing both IPv4 and IPv6 exposes. + /// Default exposes are version-neutral. + fn validate_single_ip_version(&self) -> ConfigResult { + let mut version: Option = None; + for expose in &self.valexp { + // A default expose has no IP version. + let is_v4 = if expose.is_v4() { + true + } else if expose.is_v6() { + false + } else { + continue; + }; + match version { + Some(seen) if seen != is_v4 => { + return Err(ConfigError::Forbidden( + "A manifest cannot mix IPv4 and IPv6 expose blocks", + )); + } + _ => version = Some(is_v4), + } + } + Ok(()) + } + fn validate_expose_collisions(&self) -> ConfigResult { // Check that prefixes in each expose don't overlap with prefixes in other exposes for (index, expose_left) in self.valexp.iter().enumerate() { diff --git a/flow-filter/src/fuzz_gen.rs b/flow-filter/src/fuzz_gen.rs index b0138ed0fc..43b4bffdd5 100644 --- a/flow-filter/src/fuzz_gen.rs +++ b/flow-filter/src/fuzz_gen.rs @@ -18,10 +18,8 @@ //! tie that the port-forwarding tie-break bit resolves), //! - two port-forwarding exposes sharing prefixes and ports, distinguished only by L4 protocol. //! -//! Deliberately out of scope: cross-peering masquerade/masquerade destination overlaps (legal, -//! but the winning destination VPC of the resulting equal-priority marker rules is unspecified, -//! and benign only because the NF gates masquerade verdicts on the flow's destination), and -//! mixed-IP-version manifests (they currently pass validation but yield one-sided tables). +//! Cross-peering masquerade destination overlaps are out of scope because their equal-priority +//! winner is unspecified. Mixed-IP manifests are rejected by config validation. #![cfg(test)] From b0ed0e4576d63cf6eb75209b1f3a6fc6781ea30c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 4 Aug 2026 00:04:34 -0600 Subject: [PATCH 3/3] fix(acl-filter): reject rules with the wrong IP version lower_rules previously dropped rules that did not match the selected table's IP version. Return FailureApply instead so an invariant violation rejects reconfiguration rather than silently omitting a rule. Validated manifests prevent this case; the check is defense in depth. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- acl-filter/src/access.rs | 11 +++++--- acl-filter/src/context.rs | 58 +++++++++++++++++++++++++++------------ acl-filter/src/fuzz.rs | 8 +++--- 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/acl-filter/src/access.rs b/acl-filter/src/access.rs index 72554bf7d9..e861241656 100644 --- a/acl-filter/src/access.rs +++ b/acl-filter/src/access.rs @@ -25,12 +25,15 @@ impl TryFrom<&ValidatedOverlay> for AclFilterContext { #[cfg(test)] impl AclFilterContext { - /// Build a context using the reference backend, for tests that want the fast, EAL-free oracle. - /// Production goes through [`TryFrom`], which uses the rte_acl backend. + /// Build an EAL-free reference context for tests. + /// + /// # Panics + /// + /// Panics if the validated overlay cannot be lowered. pub(crate) fn for_test(overlay: &ValidatedOverlay) -> Self { use crate::context::Backend; - let acls = AclTables::build(overlay, Backend::Reference) - .expect("reference backend build is infallible"); + let acls = + AclTables::build(overlay, Backend::Reference).expect("validated overlay must lower"); Self { acls } } diff --git a/acl-filter/src/context.rs b/acl-filter/src/context.rs index ac3c2768fd..20f1bab76e 100644 --- a/acl-filter/src/context.rs +++ b/acl-filter/src/context.rs @@ -212,9 +212,8 @@ impl AclKey { } } -/// Lower a single rule to backend-neutral field predicates for the concrete IP version of its -/// prefixes. Returns `None` if the source and destination prefixes disagree on IP version, which -/// the config validation already rules out for a well-formed peering. +/// Lower one rule to backend-neutral predicates. +/// Returns `None` if either prefix does not match `T`. fn rule_predicates( proto: AclProtoMatch, src_vni: Vni, @@ -270,14 +269,20 @@ impl IpVersion for Ipv6Addr { } } -/// Lower all rules for one IP version into `(predicates, action)` pairs, preserving order (which is -/// the precedence). A missing prefix or port range becomes the wildcard for that field. -fn lower_rules( - rules: &[PeeringAclRule], -) -> Vec<(AclKeyRule, Vec, LookupResult)> { +/// A typed rule, its backend predicates, and its action. +type LoweredRule = (AclKeyRule, Vec, LookupResult); + +/// Lower rules for `T` in first-match order. +/// Missing prefixes and port ranges become wildcards. +/// +/// # Errors +/// +/// Returns an error if a prefix has the wrong IP version. +/// Validation should make this unreachable; returning an error avoids silently dropping the rule. +fn lower_rules(rules: &[PeeringAclRule]) -> Result>, ConfigError> { rules .iter() - .filter_map(|rule| { + .map(|rule| { let (key_rule, fields) = rule_predicates( rule.proto, rule.src_vni, @@ -288,8 +293,21 @@ fn lower_rules( .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), rule.dst_port_range .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), - )?; - Some(( + ) + .ok_or_else(|| { + let (src, dst) = (rule.src_ip_range, rule.dst_ip_range); + error!( + "ACL rule for {} -> {} does not match the IP version of the table it was \ + filed under (src {src:?}, dst {dst:?}); refusing the configuration", + rule.src_vni, rule.dst_vni, + ); + ConfigError::FailureApply(format!( + "ACL rule for VNI {} -> {} has prefixes (src {src:?}, dst {dst:?}) that do \ + not match the table's IP version", + rule.src_vni, rule.dst_vni, + )) + })?; + Ok(( key_rule, fields, LookupResult { @@ -494,12 +512,18 @@ impl Default for AclTables { impl AclTables { pub(super) fn build(overlay: &ValidatedOverlay, backend: Backend) -> Result { let ruleset = PeeringAclRuleSet::from(overlay); - let v4 = - build_table::, _>(backend, "v4", lower_rules::(&ruleset.v4)) - .map_err(ConfigError::FailureApply)?; - let v6 = - build_table::, _>(backend, "v6", lower_rules::(&ruleset.v6)) - .map_err(ConfigError::FailureApply)?; + let v4 = build_table::, _>( + backend, + "v4", + lower_rules::(&ruleset.v4)?, + ) + .map_err(ConfigError::FailureApply)?; + let v6 = build_table::, _>( + backend, + "v6", + lower_rules::(&ruleset.v6)?, + ) + .map_err(ConfigError::FailureApply)?; Ok(Self { v4, v6, diff --git a/acl-filter/src/fuzz.rs b/acl-filter/src/fuzz.rs index c52ffd97a2..156fbe82c9 100644 --- a/acl-filter/src/fuzz.rs +++ b/acl-filter/src/fuzz.rs @@ -191,7 +191,7 @@ fn reference_lookup_matches_config_oracle() { .for_each(|(overlay_spec, probe_specs)| { let built = overlay_spec.build(); let tables = AclTables::build(&built.overlay, Backend::Reference) - .expect("reference backend build is infallible"); + .expect("validated overlay must lower"); for probe_spec in probe_specs { let probe = probe_spec.resolve(&built); @@ -234,7 +234,7 @@ fn earlier_rules_win_over_later_matching_rules() { .for_each(|(overlay_spec, probe_specs)| { let built = overlay_spec.build(); let tables = AclTables::build(&built.overlay, Backend::Reference) - .expect("reference backend build is infallible"); + .expect("validated overlay must lower"); for probe_spec in probe_specs { let probe = probe_spec.resolve(&built); @@ -287,7 +287,7 @@ fn dpdk_backend_matches_reference_on_generated_overlays() { .for_each(|(overlay_spec, probe_specs)| { let built = overlay_spec.build(); let reference = AclTables::build(&built.overlay, Backend::Reference) - .expect("reference backend build is infallible"); + .expect("validated overlay must lower"); let dpdk = AclTables::build(&built.overlay, Backend::Dpdk).expect("rte_acl backend build"); @@ -324,7 +324,7 @@ fn absent_acl_and_absent_peering_have_no_default_action() { .for_each(|(overlay_spec, probe_specs)| { let built = overlay_spec.build(); let tables = AclTables::build(&built.overlay, Backend::Reference) - .expect("reference backend build is infallible"); + .expect("validated overlay must lower"); for probe_spec in probe_specs { let probe = probe_spec.resolve(&built);