From 81d4b7ed6eab58045aa6c44479264e5763184269 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 18:59:03 -0700 Subject: [PATCH 1/6] [LXC] Fail closed when firewall rules cannot be scoped to the container install_firewall_rules built the full deny-all chain and then, when no veth interface was known, logged a warning and returned Ok(()). The chain is only ever reached from FORWARD via `-i `, so without that hook nothing traverses it: the caller was told the network policy was applied while zero packets were filtered. That is the worst of the three possible outcomes. Installing the rules host-wide instead would at least filter, but unscoped they would hit every container and the host's own traffic. Returning an error loses nothing, because there was no enforcement to lose. This path is only reachable when the caller explicitly asked for firewall enforcement -- apply_firewall_rules returns early unless the mode is Firewall or Both, and NetworkEnforcementMode defaults to Capabilities. So the change cannot affect containers that never wanted a firewall. Rollback and teardown already handle the Err: apply_firewall_rules_inner converts it into a precise teardown of exactly what was created plus residual ownership, and lxc_runner destroys the container rather than starting a workload that believes it is confined. No existing test pinned the old behavior (115/115 still pass), which is itself the point: the fail-open was untested. The four Linux E2E scripts that exercise firewall enforcement already require "FORWARD hook installed" in the output and fail without it, so veth discovery demonstrably succeeds there and this change is a no-op for every run that passes today. Slice 3 of the PR 632 re-cut. Refs AB#62830341. --- .../lxc/common/src/network_iptables.rs | 35 +++++++++++++++---- .../common/src/network_iptables_veth_spec.rs | 7 ++++ 2 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 src/backends/lxc/common/src/network_iptables_veth_spec.rs diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 79f5fe96..888f6123 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -1049,12 +1049,26 @@ impl NetworkIptablesManager { )); } } else { - // Without a veth interface, we cannot safely scope rules to the container. - // Refuse to apply host-wide rules to avoid affecting all host traffic. - logger.log_line( - "Warning: No veth interface set for container. \ - Cannot scope iptables rules. Skipping FORWARD hook.", - ); + // Without a veth interface there is nothing to hook the chain to, + // and an unhooked chain is never traversed: FORWARD only reaches it + // via `-i `. Reporting success here would hand the caller a + // fully populated deny-all chain that filters nothing, which is + // strictly worse than no firewall at all because it looks enforced. + // + // The alternative -- installing the rules host-wide so they do take + // effect -- is not acceptable either: unscoped they would apply to + // every container and to the host's own traffic. + // + // So the only honest outcome is to fail. `apply_firewall_rules_inner` + // rolls back the chains recorded in `created`, and `lxc_runner` + // destroys the container rather than starting a workload that + // believes it is confined. + return Err(format!( + "No veth interface for container; cannot scope iptables rules to chain {}. \ + The chain would never be reached from FORWARD, so the network policy would \ + not be enforced. Refusing to report success for an unenforceable policy.", + self.chain_name + )); } Ok(()) @@ -1228,6 +1242,15 @@ impl Drop for NetworkIptablesManager { /// because `cargo test` runs tests in parallel -- a process-global fake would /// have to be serialized behind a lock and would let one test observe /// another's commands. +/// Spec for the fail-closed behavior when rules cannot be scoped to the +/// container. Attached as a child module rather than a `tests/` integration +/// test because the `test_firewall` seam below is `#[cfg(test)]`, which an +/// integration test -- a separate crate -- can never see. Kept in its own file +/// so this one does not grow further. +#[cfg(test)] +#[path = "network_iptables_veth_spec.rs"] +mod veth_spec; + #[cfg(test)] mod test_firewall { use std::cell::RefCell; diff --git a/src/backends/lxc/common/src/network_iptables_veth_spec.rs b/src/backends/lxc/common/src/network_iptables_veth_spec.rs new file mode 100644 index 00000000..8f6085c4 --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_veth_spec.rs @@ -0,0 +1,7 @@ +//! Spec for the fail-closed contract of `apply_firewall_rules`: when the +//! firewall cannot be scoped to the container, the caller must be told the +//! policy was not applied rather than being handed a chain that filters +//! nothing. +//! +//! Attached to `network_iptables` as a child module via `#[path]`, so it can +//! reach the `#[cfg(test)]` fake-firewall seam. From 80fdb736ab3388f2e003375f3d5a9da837c07d9f Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 19:15:27 -0700 Subject: [PATCH 2/6] [LXC] Spec the fail-closed contract for unscopeable firewall rules Six black-box tests for apply_firewall_rules, written against the documented contract by an author who did not read the implementation, so they describe the behavior that was intended rather than mirroring whatever the code does. They pin: - refusal when the veth interface is unknown, under Firewall and under Both, separately, so a fix scoped to one enforcement mode cannot pass - the error names the chain left unenforced, so an operator has something to search for - the negative control: the same policy succeeds once an interface is set. Without it, an apply that always returned Err would pass every other test - teardown of the chain created before the refusal, asserted as ordering against the creation command rather than mere presence - Capabilities-only containers issue no firewall commands at all, which is what bounds this change's blast radius Mutation tested: seven seeded defects, all caught by a failing test, no survivors. The seeds include restoring the old Ok(()) fail-open, dropping the chain name from the message, applying the check to Firewall but not Both, inverting the interface check, skipping rollback, and swallowing the error one layer up in record_apply_outcome. Each mutant compiles with lints silenced, so a defect detected only by the compiler counts as a harness failure rather than a pass -- the tests have to answer for themselves. Attached as a #[path] child module because the fake-firewall seam is #[cfg(test)] and private, which an integration test -- a separate crate -- cannot reach. Slice 3 of the PR 632 re-cut. Refs AB#62830341. --- .../common/src/network_iptables_veth_spec.rs | 200 +++++++++++++++++- 1 file changed, 193 insertions(+), 7 deletions(-) diff --git a/src/backends/lxc/common/src/network_iptables_veth_spec.rs b/src/backends/lxc/common/src/network_iptables_veth_spec.rs index 8f6085c4..1b565cff 100644 --- a/src/backends/lxc/common/src/network_iptables_veth_spec.rs +++ b/src/backends/lxc/common/src/network_iptables_veth_spec.rs @@ -1,7 +1,193 @@ -//! Spec for the fail-closed contract of `apply_firewall_rules`: when the -//! firewall cannot be scoped to the container, the caller must be told the -//! policy was not applied rather than being handed a chain that filters -//! nothing. -//! -//! Attached to `network_iptables` as a child module via `#[path]`, so it can -//! reach the `#[cfg(test)]` fake-firewall seam. +//! Spec for the fail-closed contract of `apply_firewall_rules`: when the +//! firewall cannot be scoped to the container, the caller must be told the +//! policy was not applied rather than being handed a chain that filters +//! nothing. +//! +//! Attached to `network_iptables` as a child module via `#[path]`, so it can +//! reach the `#[cfg(test)]` fake-firewall seam. + +use super::*; +use wxc_common::logger::{Logger, Mode}; +use wxc_common::models::{ContainerPolicy, NetworkEnforcementMode}; + +/// Build a policy that requests the given network enforcement mode, leaving +/// every other field at its default. +fn policy_requesting(mode: NetworkEnforcementMode) -> ContainerPolicy { + ContainerPolicy { + network_enforcement_mode: mode, + ..Default::default() + } +} + +// A chain that is never hooked to the container's veth interface is a chain +// no packet ever traverses. If the manager does not know which veth belongs +// to the container, it must refuse rather than report success on a firewall +// that filters nothing. This covers the `Firewall` half of R1; `Both` is +// covered separately below so a fix scoped to only one enforcement mode +// cannot pass the suite. +#[test] +fn apply_is_refused_when_the_container_interface_is_unknown_in_firewall_mode() { + let _fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("ctrl-firewall"); + let policy = policy_requesting(NetworkEnforcementMode::Firewall); + let mut logger = Logger::new(Mode::Buffer); + + let result = manager.apply_firewall_rules(&policy, &mut logger); + + assert!( + result.is_err(), + "Firewall mode with no veth interface set must fail closed, got {:?}", + result + ); +} + +// Same hazard as above under `Both`, which also requests firewall +// enforcement. A fix that only checks the interface in the `Firewall` arm +// would leave `Both` silently unenforced, and only a dedicated test for this +// mode would catch it. +#[test] +fn apply_is_refused_when_the_container_interface_is_unknown_in_both_mode() { + let _fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("ctrl-both"); + let policy = policy_requesting(NetworkEnforcementMode::Both); + let mut logger = Logger::new(Mode::Buffer); + + let result = manager.apply_firewall_rules(&policy, &mut logger); + + assert!( + result.is_err(), + "Both mode with no veth interface set must fail closed, got {:?}", + result + ); +} + +// A caller who is told "firewall applied" while the interface was never known +// deserves an error that says what to check. If the message drops the chain +// name or the "will not be enforced" meaning, an operator debugging why a +// container's traffic is unfiltered has nothing to search logs for. +#[test] +fn refusal_error_names_the_unenforced_chain() { + let _fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("acme-web"); + let policy = policy_requesting(NetworkEnforcementMode::Firewall); + let mut logger = Logger::new(Mode::Buffer); + + let err = manager + .apply_firewall_rules(&policy, &mut logger) + .expect_err("Firewall mode with no veth interface set must fail closed"); + + let chain = "MXC-acme-web"; + assert!( + err.contains(chain), + "error must name the chain left unenforced ({chain}), got: {err}" + ); + + let lower = err.to_lowercase(); + assert!( + lower.contains("not") && lower.contains("enforc"), + "error must convey that the policy will not be enforced, got: {err}" + ); +} + +// Negative control for R1: the only thing that changes here is that the veth +// interface is now known. Without this test, R1's failures would prove +// nothing about the interface check specifically -- an `apply_firewall_rules` +// that always returned `Err` would also pass every R1 test above. +#[test] +fn apply_succeeds_once_the_veth_interface_is_known() { + let fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("ctrl-negative"); + manager.set_veth_interface("veth-ctrl0"); + let policy = policy_requesting(NetworkEnforcementMode::Firewall); + let mut logger = Logger::new(Mode::Buffer); + let _ = fake.forget_issued(); + + let result = manager.apply_firewall_rules(&policy, &mut logger); + + assert!( + result.is_ok(), + "the same Firewall policy that fails with no veth interface must succeed once one is set, got {:?}", + result + ); + assert!( + !fake.issued().is_empty(), + "a successful Firewall apply must actually issue iptables commands, not just report success" + ); +} + +// A caller who is refused must not be left holding a chain on the host: an +// unhooked-but-still-installed chain is inert today but becomes a liability +// the moment anything later hooks a chain by that name. The failed apply +// must tear down what it created, not merely stop short of hooking it up. +#[test] +fn apply_tears_down_the_chain_it_created_when_it_fails_closed() { + let fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("ctrl-teardown"); + let policy = policy_requesting(NetworkEnforcementMode::Firewall); + let mut logger = Logger::new(Mode::Buffer); + let _ = fake.forget_issued(); + + let result = manager.apply_firewall_rules(&policy, &mut logger); + assert!( + result.is_err(), + "expected the apply to fail closed so the teardown path runs, got {:?}", + result + ); + + let issued = fake.issued(); + let chain = "MXC-ctrl-teardown"; + let creation_index = issued + .iter() + .position(|cmd| cmd.iter().any(|a| a == "-N") && cmd.iter().any(|a| a == chain)) + .unwrap_or_else(|| { + panic!( + "expected a chain-creation (-N) command naming {chain} before the failure, issued: {:?}", + issued + ) + }); + let teardown_index = issued + .iter() + .position(|cmd| { + (cmd.iter().any(|a| a == "-F") || cmd.iter().any(|a| a == "-X")) + && cmd.iter().any(|a| a == chain) + }) + .unwrap_or_else(|| { + panic!( + "expected a teardown (-F/-X) command naming {chain} after the failed apply, issued: {:?}", + issued + ) + }); + + assert!( + teardown_index > creation_index, + "teardown of {chain} must be issued after its creation, issued: {:?}", + issued + ); +} + +// A container that never asked for a firewall (`Capabilities` is the default +// enforcement mode) must not be punished for an interface the caller was +// never required to set. Any firewall command touching the host here would +// be an unrequested side effect on a container that opted out of firewalling +// entirely. +#[test] +fn capabilities_only_container_is_unaffected_by_a_missing_veth_interface() { + let fake = super::test_firewall::install(); + let mut manager = NetworkIptablesManager::new("ctrl-capsonly"); + let policy = policy_requesting(NetworkEnforcementMode::Capabilities); + let mut logger = Logger::new(Mode::Buffer); + let _ = fake.forget_issued(); + + let result = manager.apply_firewall_rules(&policy, &mut logger); + + assert!( + result.is_ok(), + "Capabilities mode must not fail just because the veth interface is unknown, got {:?}", + result + ); + assert!( + fake.issued().is_empty(), + "Capabilities-only enforcement must not issue any iptables commands, issued: {:?}", + fake.issued() + ); +} From f51dc1b91c94ad41248d1419a84bc7048a169cfe Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 20:00:57 -0700 Subject: [PATCH 3/6] [LXC] Hook the firewall chain onto the bridge port so it actually filters The per-container chain was hooked into FORWARD with `-i ` only. That matches nothing whenever the veth is enslaved to a bridge, which is the default LXC topology: the packet is bridged onto `lxcbr0` and then routed off it, so FORWARD sees the bridge as the input interface and never the veth. The chain was built correctly, populated correctly, hooked without error, and traversed by zero packets. Measured on a live container before this change, with `defaultPolicy: block` and no allowed hosts: every counter in the chain read 0, the closing DROP included, and a fetch from inside the container succeeded. Adding a counting rule on the same traffic in the same FORWARD chain gave 11 packets for `-i lxcbr0` against 0 for `-i `. Install a second hook per family matching `-m physdev --physdev-in `, which identifies the bridge port the packet entered on and so stays scoped to one container -- matching the bridge itself would apply one container's policy to every container sharing it. The two rules are mutually exclusive for any given packet, so a directly routed veth is still carried by the `-i` rule and nothing is counted twice. Fail closed on the two conditions that would leave the chain unreachable again, in the same voice as the missing-veth refusal: a bridged veth whose `bridge-nf-call-{ip,ip6}tables` toggle is absent or 0, and a bridged veth whose physdev hook will not install. On a directly routed veth the physdev rule is redundant, so a kernel without the match warns instead of failing. Teardown removes both forms, built from the same builders used at insertion so a delete cannot drift from the insert it has to match, and the chain delete now waits on both hooks because either surviving one still references the chain. Verified on a live container: `defaultPolicy: block` with no allowed hosts now blocks, the same policy with `api.github.com` allowed still reaches it, all five network E2E scripts pass, and teardown leaves no FORWARD reference and no chain behind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- .../lxc/common/src/network_iptables.rs | 299 ++++++++++++++++-- .../src/network_iptables_forward_hook_spec.rs | 8 + 2 files changed, 282 insertions(+), 25 deletions(-) create mode 100644 src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 888f6123..7a8555ff 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -8,6 +8,7 @@ //! interface. use std::net::{IpAddr, Ipv6Addr, ToSocketAddrs}; +use std::path::Path; use std::process::Command; use wxc_common::logger::Logger; @@ -19,6 +20,16 @@ enum IpFamily { V6, } +/// Where the kernel reports per-interface attributes. Injectable in tests via +/// the `_in` form of the lookup below. +const SYSFS_NET_ROOT: &str = "/sys/class/net"; + +/// Toggles that decide whether bridged packets are handed to iptables and +/// ip6tables at all. A bridged container's chain is unreachable unless the +/// matching one reads `1`. +const BRIDGE_NF_CALL_IPTABLES: &str = "/proc/sys/net/bridge/bridge-nf-call-iptables"; +const BRIDGE_NF_CALL_IP6TABLES: &str = "/proc/sys/net/bridge/bridge-nf-call-ip6tables"; + /// Whether a host-list entry produces an ACCEPT or a DROP rule. Local to this /// backend: it distinguishes `allowedHosts` from `blockedHosts` and is not a /// policy-schema type. @@ -68,6 +79,8 @@ pub(crate) struct CreatedResources { v6_chain: bool, v4_hook: bool, v6_hook: bool, + v4_physdev_hook: bool, + v6_physdev_hook: bool, } /// Flush and delete the chain, reporting whether it is still owned afterward. @@ -106,7 +119,12 @@ impl CreatedResources { /// compiled on every target so Windows and macOS CI still type-check it. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] fn is_empty(&self) -> bool { - !self.v4_chain && !self.v6_chain && !self.v4_hook && !self.v6_hook + !self.v4_chain + && !self.v6_chain + && !self.v4_hook + && !self.v6_hook + && !self.v4_physdev_hook + && !self.v6_physdev_hook } /// Test-only constructor so `signal_cleanup`'s tests can build a @@ -120,6 +138,8 @@ impl CreatedResources { v6_chain, v4_hook, v6_hook, + v4_physdev_hook: false, + v6_physdev_hook: false, } } } @@ -240,6 +260,126 @@ impl NetworkIptablesManager { self.veth_interface = Some(iface.to_string()); } + /// Build one FORWARD hook rule matching the veth as the input interface. + /// + /// `op` is `-I` to install or `-D` to remove. Both come from this one + /// builder so a delete can never drift from the insert it has to match: + /// iptables deletes by full rule specification, and a spec that differs by + /// even one match leaves the hook in place. + fn build_forward_hook_iface_rule_args(op: &str, iface: &str, chain_name: &str) -> Vec { + vec![ + op.to_string(), + "FORWARD".to_string(), + "-i".to_string(), + iface.to_string(), + "-j".to_string(), + chain_name.to_string(), + ] + } + + /// Build one FORWARD hook rule matching the veth as the *bridge port* the + /// packet entered on. + /// + /// This is the rule that does the work whenever the container is attached + /// to a bridge, which is the default LXC topology (`lxc.net.0.link` set to + /// `lxcbr0`). A packet leaving such a container is bridged onto `lxcbr0` + /// and then routed off it, so by the time FORWARD sees the packet its + /// input interface is the bridge and not the veth -- an `-i ` rule + /// matches nothing at all. Measured on a live container: with both rules + /// present in FORWARD and the same traffic flowing, the `--physdev-in` + /// rule counted 11 packets while the `-i` rule counted zero. + /// + /// `--physdev-in` still names one specific bridge port, so the chain stays + /// scoped to a single container. Matching the bridge itself would apply + /// one container's policy to every container sharing it. + fn build_forward_hook_physdev_rule_args( + op: &str, + iface: &str, + chain_name: &str, + ) -> Vec { + vec![ + op.to_string(), + "FORWARD".to_string(), + "-m".to_string(), + "physdev".to_string(), + "--physdev-in".to_string(), + iface.to_string(), + "-j".to_string(), + chain_name.to_string(), + ] + } + + /// Whether `iface` is enslaved to a bridge, looked up under an injectable + /// sysfs root so this is testable without a live interface. + /// + /// The kernel exposes `master` only for an enslaved interface, so its mere + /// presence is the answer. + fn veth_is_bridge_enslaved_in(sysfs_net_root: &Path, iface: &str) -> bool { + sysfs_net_root.join(iface).join("master").exists() + } + + /// Whether bridged traffic is delivered to iptables at all, read from an + /// injectable path. + /// + /// The file exists only when `br_netfilter` is loaded, and a value of `1` + /// is what makes `--physdev-in` able to match. Absent or `0`, a bridged + /// container's packets bypass these chains entirely. + fn bridge_netfilter_active_at(path: &Path) -> bool { + std::fs::read_to_string(path) + .map(|contents| contents.trim() == "1") + .unwrap_or(false) + } + + /// Production wrapper over [`Self::veth_is_bridge_enslaved_in`]. + fn veth_is_bridge_enslaved(iface: &str) -> bool { + Self::veth_is_bridge_enslaved_in(Path::new(SYSFS_NET_ROOT), iface) + } + + /// Production wrapper over [`Self::bridge_netfilter_active_at`]. + fn bridge_netfilter_active(path: &str) -> bool { + Self::bridge_netfilter_active_at(Path::new(path)) + } + + /// Install the `--physdev-in` FORWARD hook for one family. + /// + /// Whether a failure here is fatal depends entirely on the topology, so + /// the decision lives in one place rather than being duplicated per + /// family. On a bridged veth this rule is the only one that can ever + /// match, so failing to install it means the policy is not enforced and + /// the caller must not be told otherwise. On a directly routed veth the + /// `-i` rule already carries the traffic and this one is redundant, so a + /// host whose kernel lacks the `physdev` match is still correctly + /// filtered and only warrants a warning. + fn install_physdev_hook( + run: fn(&[Vec], &mut Logger) -> Result<(), String>, + iface: &str, + chain_name: &str, + bridged: bool, + tool: &str, + logger: &mut Logger, + ) -> Result { + let rule = Self::build_forward_hook_physdev_rule_args("-I", iface, chain_name); + match run(&[rule], logger) { + Ok(()) => Ok(true), + Err(err) if bridged => Err(format!( + "Failed to install the physdev FORWARD hook on bridged veth {} for chain {} \ + ({}): {}. That rule is the only one a bridged container's packets can match, \ + so the policy would not be enforced. Refusing to report success for an \ + unenforceable policy.", + iface, chain_name, tool, err + )), + Err(err) => { + logger.log_line(&format!( + "Warning: could not install the physdev FORWARD hook on {} for chain {} \ + ({}): {}. The veth is not bridged, so the interface hook already carries \ + this container's traffic.", + iface, chain_name, tool, err + )); + Ok(false) + } + } + } + /// Resolve a destination string to IPv4 and IPv6 firewall destinations. /// /// Bare IPv4/IPv6 literals are retained in their matching family. CIDR @@ -1022,36 +1162,103 @@ impl NetworkIptablesManager { } // Hook the chains into FORWARD for the container's egress traffic. - // Packets originating in the container arrive at the host on the - // host-side veth, so they match FORWARD by input interface (`-i`); - // `-o` would instead match traffic flowing toward the container. + // + // Two rules per family, because the input interface FORWARD sees + // depends on how the veth is attached. A veth routed directly by the + // host arrives as `-i `. A veth enslaved to a bridge -- the + // default LXC topology -- arrives as `-i `, and only + // `--physdev-in ` still identifies the container. Installing + // only the first is what let a fully populated deny-all chain sit in + // the ruleset filtering nothing. + // + // The two are mutually exclusive for any given packet, so no packet is + // counted twice. `-o` would instead match traffic flowing toward the + // container. if let Some(ref iface) = self.veth_interface { - Self::run_iptables( - &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], + let bridged = Self::veth_is_bridge_enslaved(iface); + let chain_name = self.chain_name.clone(); + + // On a bridged veth the physdev rule is the only one that can + // match, and it can only match while br_netfilter is delivering + // bridged packets to iptables. Without that, both rules install + // cleanly and neither ever fires, which is the exact failure this + // change exists to remove: a chain that looks enforced and is not. + if bridged && !Self::bridge_netfilter_active(BRIDGE_NF_CALL_IPTABLES) { + return Err(format!( + "Container veth {} is enslaved to a bridge but bridged packets are not \ + delivered to iptables ({} is absent or 0), so chain {} could never be \ + reached from FORWARD. Refusing to report success for an unenforceable \ + policy.", + iface, BRIDGE_NF_CALL_IPTABLES, chain_name + )); + } + + Self::run_iptables_rule_args( + &[Self::build_forward_hook_iface_rule_args( + "-I", + iface, + &chain_name, + )], logger, )?; created.v4_hook = true; Self::publish_created(created); + + created.v4_physdev_hook = Self::install_physdev_hook( + Self::run_iptables_rule_args, + iface, + &chain_name, + bridged, + "iptables", + logger, + )?; + Self::publish_created(created); logger.log_line(&format!( "FORWARD hook installed on {} for chain {} (iptables).", - iface, self.chain_name + iface, chain_name )); if ipv6_enabled { - Self::run_ip6tables( - &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], + if bridged && !Self::bridge_netfilter_active(BRIDGE_NF_CALL_IP6TABLES) { + return Err(format!( + "Container veth {} is enslaved to a bridge but bridged packets are not \ + delivered to ip6tables ({} is absent or 0), so chain {} could never be \ + reached from FORWARD for IPv6. Refusing to report success for an \ + unenforceable policy.", + iface, BRIDGE_NF_CALL_IP6TABLES, chain_name + )); + } + + Self::run_ip6tables_rule_args( + &[Self::build_forward_hook_iface_rule_args( + "-I", + iface, + &chain_name, + )], logger, )?; created.v6_hook = true; Self::publish_created(created); + + created.v6_physdev_hook = Self::install_physdev_hook( + Self::run_ip6tables_rule_args, + iface, + &chain_name, + bridged, + "ip6tables", + logger, + )?; + Self::publish_created(created); + logger.log_line(&format!( "FORWARD hook installed on {} for chain {} (ip6tables).", - iface, self.chain_name + iface, chain_name )); } } else { // Without a veth interface there is nothing to hook the chain to, - // and an unhooked chain is never traversed: FORWARD only reaches it - // via `-i `. Reporting success here would hand the caller a + // and an unhooked chain is never traversed: FORWARD reaches it only + // via a rule naming the veth, whether as the input interface or as + // the bridge port. Reporting success here would hand the caller a // fully populated deny-all chain that filters nothing, which is // strictly worse than no firewall at all because it looks enforced. // @@ -1106,32 +1313,68 @@ impl NetworkIptablesManager { ) -> CreatedResources { let mut residual = *created; - // Remove from FORWARD only for families this attempt hooked. Must - // match the `-i` direction used at insertion so the delete finds the - // rule; a `-o` delete would leak the FORWARD hook. + // Remove from FORWARD only for families this attempt hooked, and only + // the hook forms it actually installed. Both specs come from the same + // builders used at insertion, because iptables deletes by full rule + // specification: a spec that differs by even one match -- `-o` instead + // of `-i`, or the interface rule standing in for the physdev one -- + // finds nothing and leaks the hook. if let Some(iface) = veth_interface { if created.v4_hook - && Self::run_iptables(&["-D", "FORWARD", "-i", iface, "-j", chain_name], logger) - .is_ok() + && Self::run_iptables_rule_args( + &[Self::build_forward_hook_iface_rule_args( + "-D", iface, chain_name, + )], + logger, + ) + .is_ok() { residual.v4_hook = false; } + if created.v4_physdev_hook + && Self::run_iptables_rule_args( + &[Self::build_forward_hook_physdev_rule_args( + "-D", iface, chain_name, + )], + logger, + ) + .is_ok() + { + residual.v4_physdev_hook = false; + } if created.v6_hook - && Self::run_ip6tables(&["-D", "FORWARD", "-i", iface, "-j", chain_name], logger) - .is_ok() + && Self::run_ip6tables_rule_args( + &[Self::build_forward_hook_iface_rule_args( + "-D", iface, chain_name, + )], + logger, + ) + .is_ok() { residual.v6_hook = false; } + if created.v6_physdev_hook + && Self::run_ip6tables_rule_args( + &[Self::build_forward_hook_physdev_rule_args( + "-D", iface, chain_name, + )], + logger, + ) + .is_ok() + { + residual.v6_physdev_hook = false; + } } // Flush and delete only the chains this attempt created, and only once - // that family's FORWARD hook is confirmed gone. `-X` is the command - // that actually relinquishes the chain, so ownership is only cleared - // when it succeeds. The gate is per family because the two chains live - // in different tables and are referenced independently. + // every FORWARD hook for that family is confirmed gone. `-X` is the + // command that actually relinquishes the chain, so ownership is only + // cleared when it succeeds. Either surviving hook still references the + // chain, so both gate the delete. The gate is per family because the + // two chains live in different tables and are referenced independently. residual.v4_chain = teardown_chain( created.v4_chain, - residual.v4_hook, + residual.v4_hook || residual.v4_physdev_hook, logger, |logger| { let _ = Self::run_iptables(&["-F", chain_name], logger); @@ -1140,7 +1383,7 @@ impl NetworkIptablesManager { ); residual.v6_chain = teardown_chain( created.v6_chain, - residual.v6_hook, + residual.v6_hook || residual.v6_physdev_hook, logger, |logger| { let _ = Self::run_ip6tables(&["-F", chain_name], logger); @@ -1251,6 +1494,12 @@ impl Drop for NetworkIptablesManager { #[path = "network_iptables_veth_spec.rs"] mod veth_spec; +/// Black-box specification for the FORWARD hook wiring, kept in its own file +/// for the same reason as `veth_spec`. +#[cfg(test)] +#[path = "network_iptables_forward_hook_spec.rs"] +mod forward_hook_spec; + #[cfg(test)] mod test_firewall { use std::cell::RefCell; diff --git a/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs b/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs new file mode 100644 index 00000000..f82ad199 --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs @@ -0,0 +1,8 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Black-box specification for the FORWARD hook that steers a container's +//! egress into its own chain. +//! +//! Written against the documented contract of the hook builders and the +//! topology detectors, not against their bodies. From b9946e324a9f8c353e959857aa919bd9082a61f7 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 20:15:18 -0700 Subject: [PATCH 4/6] [LXC] Spec the FORWARD hook contract and assert enforcement end to end Two kinds of test, because the defect this slice fixes was invisible to both kinds the repository already had. The unit specs pin the four seams the hook is built from: the two rule-args builders, bridge-enslavement detection, and the bridge-netfilter toggle read. They are written against the documented contract by an author who did not read the implementation. The guarantees that matter most are that the physdev builder never collapses into an input-interface match, that it names one specific bridge port rather than a wildcard, that a delete specification differs from its insert only by the operation -- iptables deletes by full rule specification, so a drifted delete silently leaks the hook -- and that an absent bridge-netfilter toggle reads as inactive, never as safe. Mutation testing over nine seeded defects, including the exact bug this slice fixes: 9 caught, 0 survivors. The E2E script exists because unit tests cannot see the failure at all. Every existing network script asserts that the FORWARD hook was *installed*, which is a log line; the hook installed cleanly, named the right chain, and matched zero packets. So this script asserts the guarantee instead: a destination the policy does not allow must be unreachable from inside the container, and an explicitly allowed one must still be reachable. The allow case is not decoration -- a blocked-only assertion would also pass on a host with no working network, or on a change that broke egress outright. Verified in both directions on live containers. Against the fixed implementation the script passes. Against the implementation from the parent commit it fails on the deny case with "egress succeeded under a default-block policy with no allowed hosts", which is the regression it exists to catch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- .../src/network_iptables_forward_hook_spec.rs | 450 ++++++++++++++++++ .../lxc_network_enforcement_allow.json | 21 + .../configs/lxc_network_enforcement_deny.json | 21 + tests/scripts/run_lxc_all_tests.sh | 1 + .../run_lxc_network_enforcement_test.sh | 102 ++++ 5 files changed, 595 insertions(+) create mode 100644 tests/configs/lxc_network_enforcement_allow.json create mode 100644 tests/configs/lxc_network_enforcement_deny.json create mode 100644 tests/scripts/run_lxc_network_enforcement_test.sh diff --git a/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs b/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs index f82ad199..c87356ee 100644 --- a/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs +++ b/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs @@ -6,3 +6,453 @@ //! //! Written against the documented contract of the hook builders and the //! topology detectors, not against their bodies. + +use super::*; +use std::fs; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU32, Ordering}; + +/// Hand back a directory path under the OS temp root that no other test (or +/// prior run) has used, so sysfs and netfilter fixtures never collide when +/// tests run concurrently in the same process. +fn fresh_fixture_dir(label: &str) -> PathBuf { + static SEQ: AtomicU32 = AtomicU32::new(0); + let seq = SEQ.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + std::env::temp_dir().join(format!("mxc-forward-hook-spec-{label}-{pid}-{seq}")) +} + +// The op token controls whether this is an install or a removal, and +// iptables reads the operation as the first word of the command; if it were +// buried elsewhere the CLI invocation would not do what the caller asked. +#[test] +fn iface_hook_rule_args_start_with_the_requested_operation() { + let install = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", "veth10", "MXC-tenant10"); + let delete = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-D", "veth10", "MXC-tenant10"); + + assert_eq!(install.first().map(String::as_str), Some("-I")); + assert_eq!(delete.first().map(String::as_str), Some("-D")); +} + +// This rule must be installed into the kernel's FORWARD chain specifically; +// any other chain would never see forwarded container traffic at all. +#[test] +fn iface_hook_rule_args_operate_on_the_forward_chain() { + let args = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", "veth11", "MXC-tenant11"); + + assert_eq!( + args.get(1).map(String::as_str), + Some("FORWARD"), + "expected the chain immediately after the operation to be FORWARD, got: {args:?}" + ); +} + +// The whole point of this builder is to match on the veth's own input +// interface, naming the specific interface passed in. +#[test] +fn iface_hook_rule_args_match_on_the_named_input_interface() { + let iface = "veth12"; + let args = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", iface, "MXC-tenant12"); + + let i_index = args + .iter() + .position(|a| a == "-i") + .expect("expected an -i input-interface match in the rule args"); + assert_eq!( + args.get(i_index + 1).map(String::as_str), + Some(iface), + "expected the -i match to name {iface}, got: {args:?}" + ); +} + +// A rule that matches the right interface but jumps to the wrong chain (or +// no chain) would never hook the container's own filtering. +#[test] +fn iface_hook_rule_args_jump_to_the_named_chain() { + let chain_name = "MXC-tenant13"; + let args = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", "veth13", chain_name); + + let j_index = args + .iter() + .position(|a| a == "-j") + .expect("expected a -j jump target in the rule args"); + assert_eq!( + args.get(j_index + 1).map(String::as_str), + Some(chain_name), + "expected the -j target to be {chain_name}, got: {args:?}" + ); +} + +// If this builder ever picked up a physdev match too, it would silently +// start behaving like the bridged-topology rule, defeating the reason the +// two builders are separate functions. +#[test] +fn iface_hook_rule_args_never_carry_a_physdev_match() { + let args = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", "veth14", "MXC-tenant14"); + + assert!( + !args.iter().any(|a| a == "physdev" || a == "--physdev-in"), + "an input-interface rule must not also carry a physdev match, got: {args:?}" + ); +} + +// A delete that is not token-for-token identical to its insert (apart from +// the operation) will not match anything in the kernel's rule table, and the +// rule it was supposed to remove leaks. +#[test] +fn iface_hook_delete_spec_differs_from_its_insert_only_by_the_operation() { + let iface = "veth15"; + let chain_name = "MXC-tenant15"; + let install = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", iface, chain_name); + let delete = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-D", iface, chain_name); + + assert_eq!( + install.len(), + delete.len(), + "install and delete rule specs must have the same number of tokens, install: {install:?}, delete: {delete:?}" + ); + assert_ne!( + install[0], delete[0], + "the first token is the operation and must differ between install and delete" + ); + assert_eq!( + &install[1..], + &delete[1..], + "every token besides the operation must match exactly, or the delete will not find the rule the install created" + ); +} + +// Same operation-placement guarantee as the interface builder, so an install +// and a delete of a physdev rule both do what the caller asked. +#[test] +fn physdev_hook_rule_args_start_with_the_requested_operation() { + let install = NetworkIptablesManager::build_forward_hook_physdev_rule_args( + "-I", + "veth20", + "MXC-tenant20", + ); + let delete = NetworkIptablesManager::build_forward_hook_physdev_rule_args( + "-D", + "veth20", + "MXC-tenant20", + ); + + assert_eq!(install.first().map(String::as_str), Some("-I")); + assert_eq!(delete.first().map(String::as_str), Some("-D")); +} + +// This rule must also land in the kernel's FORWARD chain -- the physdev +// match only changes what is matched within that chain, not which chain it +// is installed into. +#[test] +fn physdev_hook_rule_args_operate_on_the_forward_chain() { + let args = NetworkIptablesManager::build_forward_hook_physdev_rule_args( + "-I", + "veth21", + "MXC-tenant21", + ); + + assert_eq!( + args.get(1).map(String::as_str), + Some("FORWARD"), + "expected the chain immediately after the operation to be FORWARD, got: {args:?}" + ); +} + +// Once a veth is bridge-enslaved, the bridge port it entered on is the only +// thing that still identifies that one container, so the exact token +// sequence iptables needs for the physdev match -- not just "physdev appears +// somewhere" -- is the contract itself. +#[test] +fn physdev_hook_rule_args_match_the_named_physdev_in_port() { + let iface = "veth-c9f3"; + let chain_name = "MXC-tenant-c9f3"; + let args = + NetworkIptablesManager::build_forward_hook_physdev_rule_args("-I", iface, chain_name); + + let expected: Vec = ["-m", "physdev", "--physdev-in", iface] + .iter() + .map(|s| s.to_string()) + .collect(); + let found = args + .windows(expected.len()) + .any(|w| w == expected.as_slice()); + + assert!( + found, + "expected the contiguous sequence {expected:?} in the physdev rule args, got: {args:?}" + ); +} + +// A physdev rule that matches the right bridge port but jumps to the wrong +// chain would leave the container's own filtering unhooked, same as the +// interface builder's equivalent guarantee. +#[test] +fn physdev_hook_rule_args_jump_to_the_named_chain() { + let chain_name = "MXC-tenant23"; + let args = + NetworkIptablesManager::build_forward_hook_physdev_rule_args("-I", "veth23", chain_name); + + let j_index = args + .iter() + .position(|a| a == "-j") + .expect("expected a -j jump target in the rule args"); + assert_eq!( + args.get(j_index + 1).map(String::as_str), + Some(chain_name), + "expected the -j target to be {chain_name}, got: {args:?}" + ); +} + +// Once a veth is bridge-enslaved, FORWARD sees the bridge as the input +// interface, not the veth; an -i match naming the veth would match nothing +// at all, so this builder must not carry one. +#[test] +fn physdev_hook_rule_args_never_carry_an_input_interface_match() { + let args = NetworkIptablesManager::build_forward_hook_physdev_rule_args( + "-I", + "veth24", + "MXC-tenant24", + ); + + assert!( + !args.iter().any(|a| a == "-i"), + "a physdev-matched rule must not also carry an -i input-interface match, got: {args:?}" + ); +} + +// Same leak hazard as the interface builder's delete/insert invariant: a +// physdev delete spec that drifts from its insert will not find the rule and +// leaves it installed on the host forever. +#[test] +fn physdev_hook_delete_spec_differs_from_its_insert_only_by_the_operation() { + let iface = "veth25"; + let chain_name = "MXC-tenant25"; + let install = + NetworkIptablesManager::build_forward_hook_physdev_rule_args("-I", iface, chain_name); + let delete = + NetworkIptablesManager::build_forward_hook_physdev_rule_args("-D", iface, chain_name); + + assert_eq!( + install.len(), + delete.len(), + "install and delete rule specs must have the same number of tokens, install: {install:?}, delete: {delete:?}" + ); + assert_ne!( + install[0], delete[0], + "the first token is the operation and must differ between install and delete" + ); + assert_eq!( + &install[1..], + &delete[1..], + "every token besides the operation must match exactly, or the delete will not find the rule the install created" + ); +} + +// The two builders exist because a directly routed veth and a +// bridge-enslaved veth need different matches to see the same packets. If +// they ever produced identical rule specs, one of those two topologies would +// silently collapse onto the other's match, bringing back the bug this +// change fixes. +#[test] +fn the_iface_and_physdev_hook_builders_never_produce_the_same_rule_specification() { + let iface = "veth26"; + let chain_name = "MXC-tenant26"; + let iface_rule = + NetworkIptablesManager::build_forward_hook_iface_rule_args("-I", iface, chain_name); + let physdev_rule = + NetworkIptablesManager::build_forward_hook_physdev_rule_args("-I", iface, chain_name); + + assert_ne!( + iface_rule, physdev_rule, + "the input-interface rule and the physdev rule must differ, or bridged and directly \ + routed containers would collapse onto the same match" + ); +} + +// The kernel only creates a `master` entry once an interface is enslaved to +// a bridge, so its presence alone is what this function is allowed to trust. +#[test] +fn an_interface_with_a_master_entry_is_reported_as_bridge_enslaved() { + let root = fresh_fixture_dir("enslaved"); + let iface_dir = root.join("veth-a1b2"); + fs::create_dir_all(&iface_dir).expect("failed to create the fake sysfs interface directory"); + fs::write(iface_dir.join("master"), "").expect("failed to create the fake master entry"); + + let result = NetworkIptablesManager::veth_is_bridge_enslaved_in(&root, "veth-a1b2"); + + fs::remove_dir_all(&root).expect("failed to clean up the fake sysfs root"); + assert!( + result, + "an interface with a master entry must be reported as bridge-enslaved" + ); +} + +// A veth that is not enslaved has an interface directory but no `master` +// entry inside it; this is the ordinary "routed directly" topology. +#[test] +fn an_interface_without_a_master_entry_is_not_bridge_enslaved() { + let root = fresh_fixture_dir("unenslaved"); + let iface_dir = root.join("veth-d4e5"); + fs::create_dir_all(&iface_dir).expect("failed to create the fake sysfs interface directory"); + + let result = NetworkIptablesManager::veth_is_bridge_enslaved_in(&root, "veth-d4e5"); + + fs::remove_dir_all(&root).expect("failed to clean up the fake sysfs root"); + assert!( + !result, + "an interface directory with no master entry must not be reported as bridge-enslaved" + ); +} + +// If the interface itself has no sysfs directory at all -- for example a +// name that does not exist on the host -- there is nothing to be enslaved, +// and the function must say so rather than erroring. +#[test] +fn a_missing_interface_directory_is_not_bridge_enslaved() { + let root = fresh_fixture_dir("missing-iface"); + fs::create_dir_all(&root).expect("failed to create the fake sysfs root"); + + let result = NetworkIptablesManager::veth_is_bridge_enslaved_in(&root, "veth-ghost"); + + fs::remove_dir_all(&root).expect("failed to clean up the fake sysfs root"); + assert!( + !result, + "an interface with no sysfs directory at all must not be reported as bridge-enslaved" + ); +} + +// The toggle file's documented "on" value is exactly "1"; this is the +// baseline positive case every other Function 4 test is a variation of. +#[test] +fn a_bridge_netfilter_toggle_of_1_is_reported_active() { + let dir = fresh_fixture_dir("nf-on"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, "1").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + result, + "a toggle file containing exactly \"1\" must be reported as active" + ); +} + +// The real kernel file ends in a newline; a comparison that forgets to trim +// would treat every real, active system as inactive. +#[test] +fn a_bridge_netfilter_toggle_of_1_with_a_trailing_newline_is_reported_active() { + let dir = fresh_fixture_dir("nf-on-newline"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, "1\n").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + result, + "a toggle file containing \"1\\n\", matching the real kernel file's trailing newline, must be reported as active" + ); +} + +// "0" is the documented "off" value and must read as inactive, not merely as +// "not 1 so default to something". +#[test] +fn a_bridge_netfilter_toggle_of_0_is_not_active() { + let dir = fresh_fixture_dir("nf-off"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, "0").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + !result, + "a toggle file containing \"0\" must not be reported as active" + ); +} + +// Absence means the bridge-netfilter machinery is not loaded at all, which +// is the unsafe case: it must never be mistaken for "on". +#[test] +fn a_missing_bridge_netfilter_toggle_is_not_active() { + let dir = fresh_fixture_dir("nf-missing"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + !result, + "a toggle file that does not exist at all must not be reported as active" + ); +} + +// Empty contents are neither "1" nor "0"; the function must not treat a +// truncated or not-yet-written file as active. +#[test] +fn an_empty_bridge_netfilter_toggle_is_not_active() { + let dir = fresh_fixture_dir("nf-empty"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, "").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + !result, + "a toggle file with empty contents must not be reported as active" + ); +} + +// Whitespace-only contents must not survive trimming into an empty string +// that somehow compares equal to "1"; it must compare as not-"1" and read as +// inactive. +#[test] +fn a_whitespace_only_bridge_netfilter_toggle_is_not_active() { + let dir = fresh_fixture_dir("nf-whitespace"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, " \n\t ").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + !result, + "a toggle file with only whitespace must not be reported as active" + ); +} + +// Any value that is not exactly "1" must read as inactive, not just values +// that happen to be "0"; otherwise a fail-open bug could hide behind an +// unexpected value like a stray "2". +#[test] +fn a_bridge_netfilter_toggle_with_an_unrecognized_value_is_not_active() { + let dir = fresh_fixture_dir("nf-unrecognized"); + fs::create_dir_all(&dir).expect("failed to create the fake proc directory"); + let toggle = dir.join("bridge-nf-call-iptables"); + fs::write(&toggle, "2").expect("failed to write the fake netfilter toggle"); + + let result = NetworkIptablesManager::bridge_netfilter_active_at(&toggle); + + fs::remove_dir_all(&dir).expect("failed to clean up the fake proc directory"); + assert!( + !result, + "a toggle file containing a value other than \"1\" must not be reported as active" + ); +} diff --git a/tests/configs/lxc_network_enforcement_allow.json b/tests/configs/lxc_network_enforcement_allow.json new file mode 100644 index 00000000..618688d9 --- /dev/null +++ b/tests/configs/lxc_network_enforcement_allow.json @@ -0,0 +1,21 @@ +{ + "version": "0.8.0-alpha", + "containerId": "CLI-LXC-Net-Allow", + "containment": "lxc", + "process": { + "commandLine": "sh -c \"wget -qO- --timeout=10 https://api.github.com/zen >/dev/null 2>&1 && echo MXC_NET_ALLOWED || echo MXC_NET_BLOCKED\"" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": ["api.github.com"], + "blockedHosts": [] + } +} \ No newline at end of file diff --git a/tests/configs/lxc_network_enforcement_deny.json b/tests/configs/lxc_network_enforcement_deny.json new file mode 100644 index 00000000..73cd2a35 --- /dev/null +++ b/tests/configs/lxc_network_enforcement_deny.json @@ -0,0 +1,21 @@ +{ + "version": "0.8.0-alpha", + "containerId": "CLI-LXC-Net-Deny", + "containment": "lxc", + "process": { + "commandLine": "sh -c \"wget -qO- --timeout=8 https://api.github.com/zen >/dev/null 2>&1 && echo MXC_NET_ALLOWED || echo MXC_NET_BLOCKED\"" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": [], + "blockedHosts": [] + } +} \ No newline at end of file diff --git a/tests/scripts/run_lxc_all_tests.sh b/tests/scripts/run_lxc_all_tests.sh index 0510bb4d..6410c85b 100644 --- a/tests/scripts/run_lxc_all_tests.sh +++ b/tests/scripts/run_lxc_all_tests.sh @@ -66,6 +66,7 @@ run_test "LXC Network IPv6+CIDR" "$SCRIPT_DIR/run_lxc_network_ipv6_cidr_test.sh" run_test "LXC Network Invalid CIDR" "$SCRIPT_DIR/run_lxc_network_invalid_cidr_test.sh" run_test "LXC Network Dual-Stack Hostname" "$SCRIPT_DIR/run_lxc_network_dualstack_test.sh" run_test "LXC Network CIDR Boundary" "$SCRIPT_DIR/run_lxc_network_cidr_boundary_test.sh" +run_test "LXC Network Enforcement" "$SCRIPT_DIR/run_lxc_network_enforcement_test.sh" run_test "LXC Timeout" "$SCRIPT_DIR/run_lxc_timeout_test.sh" run_test "LXC Env+Cwd" "$SCRIPT_DIR/run_lxc_env_cwd_test.sh" diff --git a/tests/scripts/run_lxc_network_enforcement_test.sh b/tests/scripts/run_lxc_network_enforcement_test.sh new file mode 100644 index 00000000..9887c87e --- /dev/null +++ b/tests/scripts/run_lxc_network_enforcement_test.sh @@ -0,0 +1,102 @@ +#!/bin/bash +# LXC network policy enforcement test +# +# Every other network script asserts that the FORWARD hook was *installed*. +# That is a log line, and a hook can install cleanly, name the right chain, +# and still match no packet -- which is exactly how a fully populated deny-all +# chain that filtered nothing once passed every script in this directory. +# +# This script asserts the guarantee itself rather than the log: a destination +# the policy does not allow must be unreachable from inside the container. +# +# Both directions are required, and the allow case is not decoration. A +# blocked-only assertion would also pass on a host with no working network at +# all, or on a change that broke egress outright, so it proves nothing on its +# own. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_DIR="$(dirname "$(dirname "$SCRIPT_DIR")")" +LXC_EXEC="$REPO_DIR/src/target/release/lxc-exec" + +if [ ! -f "$LXC_EXEC" ]; then + LXC_EXEC="$REPO_DIR/src/target/debug/lxc-exec" +fi + +# An honest skip for a missing prerequisite: exit 77 so run_lxc_all_tests.sh +# records SKIPPED rather than PASS. A suite that could not run must not look green. +SKIP_EXIT=77 +skip() { + echo "SKIP: $1" + exit "$SKIP_EXIT" +} + +[ "$(id -u)" -eq 0 ] || skip "requires root for iptables/ip6tables and LXC." +command -v iptables >/dev/null 2>&1 || skip "iptables is not installed." +command -v ip6tables >/dev/null 2>&1 || skip "ip6tables is not installed." +command -v lxc-create >/dev/null 2>&1 || skip "LXC (lxc-create) is not installed." +[ -f "$LXC_EXEC" ] || skip "lxc-exec binary not built; run build.sh first." + +DENY_CONFIG="$REPO_DIR/tests/configs/lxc_network_enforcement_deny.json" +ALLOW_CONFIG="$REPO_DIR/tests/configs/lxc_network_enforcement_allow.json" +DENY_CHAIN="MXC-CLI-LXC-Net-Deny" +ALLOW_CHAIN="MXC-CLI-LXC-Net-Allow" + +fail() { + echo "FAIL: $1" + exit 1 +} + +assert_firewall_chain_cleaned_up() { + if iptables -S "$1" >/dev/null 2>&1; then + fail "iptables chain '$1' was left behind after lxc-exec completed." + fi + if ip6tables -S "$1" >/dev/null 2>&1; then + fail "ip6tables chain '$1' was left behind after lxc-exec completed." + fi +} + +# A hook that references the chain but survives teardown leaves the next +# container's traffic running through a stale rule, so the reference count +# matters as much as the chain itself. +assert_no_forward_reference() { + if iptables -S FORWARD 2>/dev/null | grep -Fq -- "$1"; then + fail "a FORWARD rule still references chain '$1' after teardown." + fi +} + +echo "Running LXC network policy enforcement test..." + +# The container reports the outcome itself rather than relying on its exit +# code, so a wrapper that swallows or rewrites the status cannot turn a +# reachable destination into an apparent block. +echo "--- deny case: default policy blocks, nothing allowed ---" +DENY_OUTPUT=$("$LXC_EXEC" --debug "$DENY_CONFIG" 2>&1 || true) +echo "$DENY_OUTPUT" + +if echo "$DENY_OUTPUT" | grep -Fq "MXC_NET_ALLOWED"; then + fail "egress succeeded under a default-block policy with no allowed hosts. The chain is not filtering this container's traffic." +fi +if ! echo "$DENY_OUTPUT" | grep -Fq "MXC_NET_BLOCKED"; then + fail "the deny case produced no verdict at all; the container command did not run." +fi + +assert_no_forward_reference "$DENY_CHAIN" +assert_firewall_chain_cleaned_up "$DENY_CHAIN" + +echo "--- allow case: same default, destination explicitly allowed ---" +ALLOW_OUTPUT=$("$LXC_EXEC" --debug "$ALLOW_CONFIG" 2>&1 || true) +echo "$ALLOW_OUTPUT" + +if echo "$ALLOW_OUTPUT" | grep -Fq "MXC_NET_BLOCKED"; then + fail "an explicitly allowed destination was unreachable. The policy is over-blocking, so the deny case above proves nothing." +fi +if ! echo "$ALLOW_OUTPUT" | grep -Fq "MXC_NET_ALLOWED"; then + fail "the allow case produced no verdict at all; the container command did not run." +fi + +assert_no_forward_reference "$ALLOW_CHAIN" +assert_firewall_chain_cleaned_up "$ALLOW_CHAIN" + +echo "PASS: a disallowed destination was blocked and an allowed destination was reachable." +echo "LXC network policy enforcement test complete." \ No newline at end of file From 447f10fb6e869d2a33afbb43f51a701f273d19b7 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sun, 9 Aug 2026 12:49:19 -0700 Subject: [PATCH 5/6] [LXC] Make deny rules win over allow rules and fail closed on an unresolvable block The per-container chain emitted allow-list rules before block-list rules, and iptables applies first-match-wins within a chain, so a destination named in both `allowedHosts` and `blockedHosts` was ACCEPTed. A code comment recorded that as interim behavior owned by AB#62830341. Emit the block list first so the deny wins. Emission order is the entire precedence mechanism -- there is no separate resolution pass -- so the comment now says that outright, because swapping the two iterators back would reverse the security semantics without failing to compile. A block entry that resolved to no address programmed no rule and logged only a warning. Where the chain ends in ACCEPT that is a fail-open: the unwritten deny rule was the only thing that would have stopped the traffic, and the apply still reported success. `build_policy_rules_logged` now returns `Result` and errors in exactly that case, so the caller rolls back the chains it created rather than leaving a policy it did not enforce. The error is conditioned on the default policy rather than raised for every unresolvable block entry. Where the chain ends in DROP, an entry that resolves to nothing is redundant rather than missing -- the closing rule already denies every destination the allow list did not name -- and erroring there would refuse to start containers whose blocklists name hosts that do not exist, which is the ordinary case. `tests/configs/lxc_network_test.json` blocks `evil.example.com` under `defaultPolicy: block`, and that name is NXDOMAIN. The two tests that pinned allow-before-block ordering are deleted rather than inverted. They asserted the contract this change replaces, and the replacement assertions belong to the `deny_precedence_spec` module, which is authored separately so that the tests proving this change correct are not written by its author. The family-split test kept its subject and gave up only its incidental dependency on rule sequence. Residual gap, documented in the code rather than papered over: under a DROP default, a sufficiently broad allow entry can still cover a destination whose deny rule went unwritten. Detecting that needs the address the entry failed to resolve to, so no predicate over the policy text can be complete, and a partial check would imply a guarantee this code cannot make. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- .../lxc/common/src/network_iptables.rs | 174 ++++++++++-------- .../network_iptables_deny_precedence_spec.rs | 11 ++ 2 files changed, 106 insertions(+), 79 deletions(-) create mode 100644 src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 7a8555ff..7f9ca0ee 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -626,17 +626,24 @@ impl NetworkIptablesManager { /// Build the allow/deny rule args for a container policy. /// /// Test-only shim over the shipping path [`Self::build_policy_rules_logged`] - /// so the rulegen spec assertions — including the allow-before-block - /// ordering that is a tracked security-semantics contract (AB#62830341) — - /// bind to the code that actually runs, not to a duplicate iteration. The + /// so the rulegen spec assertions — including the deny-before-allow + /// ordering that is a security-semantics contract (AB#62830341) — bind to + /// the code that actually runs, not to a duplicate iteration. The /// unresolved-host warning is irrelevant to rule generation, so it is /// discarded to a buffer logger. Production must never call this: it takes /// no logger and would resolve entries a second time relative to the /// warning pass. + /// + /// This shim panics on the unresolvable-block-entry error so that the many + /// rulegen assertions over well-formed policies keep a plain return type. A + /// test that exercises the error path must call + /// [`Self::build_policy_rules_logged`] directly and inspect the `Result`. #[cfg(test)] fn build_policy_rule_args(chain_name: &str, policy: &ContainerPolicy) -> FirewallRuleArgs { let mut logger = wxc_common::logger::Logger::new(wxc_common::logger::Mode::Buffer); - Self::build_policy_rules_logged(chain_name, policy, &mut logger) + Self::build_policy_rules_logged(chain_name, policy, &mut logger).expect( + "test policy should not pair an accepting default with an unresolvable block entry", + ) } /// Resolve every allow/block entry exactly once and build the rule args @@ -650,32 +657,63 @@ impl NetworkIptablesManager { /// expires between the calls — so the rule installed would not match the /// rule that was validated and logged. /// - /// NOTE — interim ordering (tracked by AB#62830341): rules are emitted in - /// allow-list then block-list order, and iptables/ip6tables apply - /// first-match-wins within the chain. This model-1 change therefore does - /// **not** yet implement deny-precedence: a destination present in both - /// the allow and block lists is ACCEPTed. Reconciling this to the GA - /// "deny-wins" ordering is owned by net-model-2 (AB#62830341); until then - /// callers must not assume deny-precedence. + /// Deny-precedence (AB#62830341): block-list rules are emitted before + /// allow-list rules, and iptables/ip6tables apply first-match-wins within + /// the chain, so a destination present in both lists is DROPped. Emission + /// order is the entire precedence mechanism — there is no separate + /// resolution pass — so swapping these two iterators silently reverses the + /// security semantics of every policy whose lists overlap. + /// + /// A block entry that resolves to nothing programs no rule. That is a + /// containment failure only when something else would then permit the + /// destination, so the response depends on the default policy. Under + /// [`NetworkPolicy::Allow`] the chain ends in ACCEPT and the unwritten deny + /// rule was the only thing that would have stopped the traffic, so the + /// apply fails closed with an error rather than reporting success over a + /// policy it did not enforce. Under [`NetworkPolicy::Block`] the closing + /// DROP already denies every destination the allow list did not name, so an + /// unresolvable block entry is redundant rather than missing — the ordinary + /// case being a blocklist naming a host that does not exist at all — and a + /// warning is the proportionate response. + /// + /// Residual gap, deliberately not closed here: under [`NetworkPolicy::Block`] + /// a sufficiently broad allow entry can still cover a destination whose deny + /// rule went unwritten. Detecting that needs the address the entry failed to + /// resolve to, so no predicate over the policy text can be complete, and a + /// partial check would imply a guarantee this code cannot make. + /// + /// An unresolvable allow entry is always a warning: it withholds traffic + /// that was meant to be permitted, which costs availability and cannot + /// widen what the container can reach. fn build_policy_rules_logged( chain_name: &str, policy: &ContainerPolicy, logger: &mut Logger, - ) -> FirewallRuleArgs { + ) -> Result { + let default_permits = matches!(policy.default_network_policy, NetworkPolicy::Allow); let mut args = FirewallRuleArgs::default(); let entries = policy - .allowed_hosts + .blocked_hosts .iter() - .map(|host| (host, RuleAction::Allow)) + .map(|host| (host, RuleAction::Deny)) .chain( policy - .blocked_hosts + .allowed_hosts .iter() - .map(|host| (host, RuleAction::Deny)), + .map(|host| (host, RuleAction::Allow)), ); for (host, action) in entries { let destinations = Self::resolve_host(host); if destinations.is_empty() { + if default_permits && matches!(action, RuleAction::Deny) { + return Err(format!( + "blocked host '{}' resolved to no address, so no rule can be \ + programmed to deny it, and the default network policy accepts \ + what no rule matches; refusing to apply a policy that would \ + leave it reachable", + host + )); + } logger.log_line(&format!("Warning: could not resolve host '{}'", host)); } let rule_args = @@ -694,7 +732,7 @@ impl NetworkIptablesManager { } args.extend(rule_args); } - args + Ok(args) } /// Run an iptables command and return success/failure. @@ -1135,8 +1173,11 @@ impl NetworkIptablesManager { // Resolve every allow/block entry exactly once and reuse that single // resolution for both the unresolved-host warning and rule // construction, so the rule installed matches the entry that was - // validated and logged. - let policy_rules = Self::build_policy_rules_logged(&self.chain_name, policy, logger); + // validated and logged. A block entry that resolves to nothing is an + // error here rather than a warning, and propagating it aborts the + // apply so the caller rolls back the chains created above instead of + // leaving a chain that is missing one of its deny rules. + let policy_rules = Self::build_policy_rules_logged(&self.chain_name, policy, logger)?; Self::run_iptables_rule_args(&policy_rules.ipv4, logger)?; if ipv6_enabled { Self::run_ip6tables_rule_args(&policy_rules.ipv6, logger)?; @@ -1500,6 +1541,13 @@ mod veth_spec; #[path = "network_iptables_forward_hook_spec.rs"] mod forward_hook_spec; +/// Black-box specification for deny-precedence ordering and the fail-closed +/// response to an unresolvable block entry, kept in its own file for the same +/// reason as `veth_spec`. +#[cfg(test)] +#[path = "network_iptables_deny_precedence_spec.rs"] +mod deny_precedence_spec; + #[cfg(test)] mod test_firewall { use std::cell::RefCell; @@ -2188,20 +2236,34 @@ mod tests { let args = NetworkIptablesManager::build_policy_rule_args("MXC-test", &policy); - assert_eq!( - args.ipv4, - vec![ - strings(&["-A", "MXC-test", "-d", "140.82.112.0/20", "-j", "ACCEPT"]), - strings(&["-A", "MXC-test", "-d", "10.0.0.0/8", "-j", "DROP"]), - ] - ); - assert_eq!( - args.ipv6, - vec![ - strings(&["-A", "MXC-test", "-d", "2606:50c0::/32", "-j", "ACCEPT"]), - strings(&["-A", "MXC-test", "-d", "2001:db8::/32", "-j", "DROP"]), - ] - ); + // Membership rather than sequence: this test owns the family split, and + // the order the two lists are emitted in is the deny-precedence + // contract, asserted by the deny_precedence_spec module. + let expected_v4 = vec![ + strings(&["-A", "MXC-test", "-d", "140.82.112.0/20", "-j", "ACCEPT"]), + strings(&["-A", "MXC-test", "-d", "10.0.0.0/8", "-j", "DROP"]), + ]; + let expected_v6 = vec![ + strings(&["-A", "MXC-test", "-d", "2606:50c0::/32", "-j", "ACCEPT"]), + strings(&["-A", "MXC-test", "-d", "2001:db8::/32", "-j", "DROP"]), + ]; + + assert_eq!(args.ipv4.len(), expected_v4.len()); + for rule in &expected_v4 { + assert!( + args.ipv4.contains(rule), + "IPv4 rules should contain {rule:?}; actual: {:?}", + args.ipv4 + ); + } + assert_eq!(args.ipv6.len(), expected_v6.len()); + for rule in &expected_v6 { + assert!( + args.ipv6.contains(rule), + "IPv6 rules should contain {rule:?}; actual: {:?}", + args.ipv6 + ); + } } #[test] @@ -2745,52 +2807,6 @@ mod tests { } } - #[test] - fn allow_list_rules_are_emitted_before_block_list_rules_for_same_ipv4_destination() { - let destination = "203.0.113.44"; - let policy = policy_with_hosts(&[destination], &[destination]); - let rules = NetworkIptablesManager::build_policy_rule_args("MXC-order-v4", &policy); - let rendered: Vec = rules.ipv4.iter().map(|rule| joined(rule)).collect(); - - let accept_index = rendered - .iter() - .position(|rule| rule.contains(destination) && rule.contains("-j ACCEPT")) - .expect("IPv4 ACCEPT rule for duplicate destination should exist"); - let drop_index = rendered - .iter() - .position(|rule| rule.contains(destination) && rule.contains("-j DROP")) - .expect("IPv4 DROP rule for duplicate destination should exist"); - - // SPEC_BRIEF §3 pins this interim AB#62830341 behavior until deny-precedence lands. - assert!( - accept_index < drop_index, - "IPv4 duplicate {destination} should ACCEPT before DROP; actual order: {rendered:?}" - ); - } - - #[test] - fn allow_list_rules_are_emitted_before_block_list_rules_for_same_ipv6_destination() { - let destination = "2001:db8::44"; - let policy = policy_with_hosts(&[destination], &[destination]); - let rules = NetworkIptablesManager::build_policy_rule_args("MXC-order-v6", &policy); - let rendered: Vec = rules.ipv6.iter().map(|rule| joined(rule)).collect(); - - let accept_index = rendered - .iter() - .position(|rule| rule.contains(destination) && rule.contains("-j ACCEPT")) - .expect("IPv6 ACCEPT rule for duplicate destination should exist"); - let drop_index = rendered - .iter() - .position(|rule| rule.contains(destination) && rule.contains("-j DROP")) - .expect("IPv6 DROP rule for duplicate destination should exist"); - - // SPEC_BRIEF §3 says allow-before-block ordering applies to both iptables buckets. - assert!( - accept_index < drop_index, - "IPv6 duplicate {destination} should ACCEPT before DROP; actual order: {rendered:?}" - ); - } - #[test] fn base_chain_rules_are_four_family_agnostic_rules_in_documented_order() { let chain_name = "MXC-base"; diff --git a/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs b/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs new file mode 100644 index 00000000..f8033746 --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Black-box specification for deny-precedence and for the fail-closed +//! response to a block-list entry that resolves to no address. +//! +//! Written against the documented contract of the policy rule builder, not +//! against its body. +//! +//! Add `use super::*;` when the first test lands; an unused import fails the +//! `-D warnings` gate while this module is still empty. From 25614e02820129bfa9b8b2a3905a061ce2f77e64 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sun, 9 Aug 2026 13:07:25 -0700 Subject: [PATCH 6/6] [LXC] Spec deny precedence and assert an overlapping allow cannot defeat a block The implementation commit changed emission order and made an unresolvable deny entry fatal under an accepting default. This commit is the evidence that both hold, written against the documented contract rather than against the code. Twelve unit tests in a new spec module, authored from headers only by an agent that never opened `network_iptables.rs`. The author that wrote the implementation cannot write its tests: a test derived from the implementation encodes that implementation's bugs as expected behavior and will pass forever without catching anything. The tests assert the contract, not the current output: - a destination in both lists is dropped, for IPv4, for IPv6, and with several entries in each list - every DROP is emitted before every ACCEPT, checked by index rather than by comparing against a fixed expected vector - an unresolvable blocked host errors under an accepting default and the error names the host - the same unresolvable blocked host does not error under a blocking default, because the closing DROP already denies it - an unresolvable allowed host never errors under either default - an unresolvable entry does not suppress a sibling entry's rule or log line - v4 and v6 destinations land in their own buckets, asserted by parsing each destination rather than by matching a known list, so the assertion cannot be satisfied by an implementation that happens to emit the expected values Mutation testing supplies the proof that these tests can actually fail. Nine mutants, each a mistake a person could plausibly make in this function: restore the old emission order, error on every unresolvable block entry, error on unresolvable allow entries, never error at all, invert the default-policy test, swap the jump targets, drop the warning line, leak IPv6 destinations into the IPv4 bucket, and omit the host name from the error. caught=9 survived=0 harness_bugs=0 source restored byte-identical: True Mutant 1 is the load-bearing one. Two tests pinning the old allow-before-block order were deleted in the implementation commit, and a deletion with no replacement would have dropped coverage silently while the suite stayed green. Killing mutant 1 proves the replacement exists. The end-to-end guard runs the real binary against a config whose allowed and blocked lists both contain `0.0.0.0/0` and `::/0`. Literal CIDRs rather than a hostname, because a hostname is resolved separately for each list entry and round-robin DNS could hand back different addresses for the allow and the deny, making the verdict depend on which address the fetch picked. The control config is load-bearing. It allows the same destination and blocks nothing, so it must come back reachable. Without it, a host with no egress at all would produce the same blocked verdict on the overlap case and look exactly like a pass. The guard was verified to discriminate by running it against the previous commit's binary: b9946e3 ACCEPT then DROP overlap MXC_NET_ALLOWED guard FAILS, exit 1 447f10f DROP then ACCEPT overlap MXC_NET_BLOCKED guard PASSES Same script, same host, same configs. The control passed in both runs, so the difference is the rule ordering and not a host that lost its network. Gates: 154 unit tests pass, clippy -D warnings clean, fmt clean, all seven LXC end-to-end scripts pass. --- .../network_iptables_deny_precedence_spec.rs | 642 +++++++++++++++++- .../lxc_network_deny_precedence_control.json | 21 + .../lxc_network_deny_precedence_overlap.json | 21 + tests/scripts/run_lxc_all_tests.sh | 1 + .../run_lxc_network_deny_precedence_test.sh | 96 +++ 5 files changed, 770 insertions(+), 11 deletions(-) create mode 100644 tests/configs/lxc_network_deny_precedence_control.json create mode 100644 tests/configs/lxc_network_deny_precedence_overlap.json create mode 100644 tests/scripts/run_lxc_network_deny_precedence_test.sh diff --git a/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs b/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs index f8033746..14cd0ea2 100644 --- a/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs +++ b/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs @@ -1,11 +1,631 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -//! Black-box specification for deny-precedence and for the fail-closed -//! response to a block-list entry that resolves to no address. -//! -//! Written against the documented contract of the policy rule builder, not -//! against its body. -//! -//! Add `use super::*;` when the first test lands; an unused import fails the -//! `-D warnings` gate while this module is still empty. +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Black-box specification for deny-precedence and for the fail-closed +//! response to a block-list entry that resolves to no address. +//! +//! Written against the documented contract of the policy rule builder, not +//! against its body. +//! +//! Structural tests (ordering, rule shape, family split) call the test-only +//! `build_policy_rule_args` shim, which panics rather than returning `Err`. +//! Tests that must observe the `Result` or the logger buffer call +//! `build_policy_rules_logged` directly. + +use super::*; +// `super::*` re-exports `Logger` (the parent module uses it in its own +// signatures) but not `Mode`, which the parent never names directly. +use wxc_common::logger::Mode; + +/// Chain name shared by tests that do not care about its exact value. A +/// couple of tests use a distinct literal on purpose, to prove the chain +/// name is threaded through rather than hard-coded. +const CHAIN: &str = "mxc_test_chain"; + +// --------------------------------------------------------------------------- +// Shared helpers. +// --------------------------------------------------------------------------- + +/// Render a rule as `&str` slices, so it can be compared against a literal +/// without allocating `String`s for the expected side. +fn as_str_slice(rule: &[String]) -> Vec<&str> { + rule.iter().map(String::as_str).collect() +} + +/// The destination argument (`-d `) of a rule. +fn destination_of(rule: &[String]) -> &str { + let index = rule + .iter() + .position(|arg| arg == "-d") + .unwrap_or_else(|| panic!("rule has no '-d' flag; actual: {rule:?}")); + &rule[index + 1] +} + +/// The jump target argument (`-j `) of a rule. +fn action_of(rule: &[String]) -> &str { + let index = rule + .iter() + .position(|arg| arg == "-j") + .unwrap_or_else(|| panic!("rule has no '-j' flag; actual: {rule:?}")); + &rule[index + 1] +} + +/// The largest index whose rule targets `DROP`, or `None` if `rules` has no +/// deny rules. +fn last_drop_index(rules: &[Vec]) -> Option { + rules.iter().rposition(|rule| action_of(rule) == "DROP") +} + +/// The smallest index whose rule targets `ACCEPT`, or `None` if `rules` has +/// no allow rules. +fn first_accept_index(rules: &[Vec]) -> Option { + rules.iter().position(|rule| action_of(rule) == "ACCEPT") +} + +/// `items`, sorted, so two destination sets can be compared without caring +/// about the order the implementation happened to produce them in. +fn sorted<'a>(items: &[&'a str]) -> Vec<&'a str> { + let mut items = items.to_vec(); + items.sort_unstable(); + items +} + +/// Assert that `rules` contains exactly the given DROP and ACCEPT +/// destinations, as sets, and that every DROP rule precedes every ACCEPT +/// rule -- the B1 deny-precedence guarantee. Order within a single action +/// is not part of the documented contract, so it is deliberately not +/// checked here. +fn assert_deny_precedence( + rules: &[Vec], + expected_drop_destinations: &[&str], + expected_accept_destinations: &[&str], +) { + let mut drop_destinations: Vec<&str> = Vec::new(); + let mut accept_destinations: Vec<&str> = Vec::new(); + for rule in rules { + match action_of(rule) { + "DROP" => drop_destinations.push(destination_of(rule)), + "ACCEPT" => accept_destinations.push(destination_of(rule)), + other => panic!("unexpected -j target '{other}'; actual rule: {rule:?}"), + } + } + + assert_eq!( + sorted(&drop_destinations), + sorted(expected_drop_destinations), + "DROP destinations did not match expected set; actual rules: {rules:?}" + ); + assert_eq!( + sorted(&accept_destinations), + sorted(expected_accept_destinations), + "ACCEPT destinations did not match expected set; actual rules: {rules:?}" + ); + + if let (Some(last_drop), Some(first_accept)) = + (last_drop_index(rules), first_accept_index(rules)) + { + assert!( + last_drop < first_accept, + "every DROP rule must precede every ACCEPT rule (B1); \ + last DROP at index {last_drop}, first ACCEPT at index {first_accept}; \ + actual rules: {rules:?}" + ); + } +} + +/// Unwrap `result`, panicking with the `Err` payload if it is an `Err`. +/// Never formats the `Ok` payload, since `FirewallRuleArgs` is not +/// documented to implement `Debug`. +fn expect_ok(result: Result, context: &str) -> FirewallRuleArgs { + match result { + Ok(args) => args, + Err(err) => panic!("{context}; actual Err: {err:?}"), + } +} + +/// Whether `destination` (a bare address or an address/prefix CIDR) parses +/// as IPv4. Used to state the family-split guarantee as an invariant over +/// whatever the implementation actually produced, rather than as a +/// hard-coded list of which literals are which family. +fn parses_as_ipv4(destination: &str) -> bool { + let address = destination.split('/').next().unwrap_or(destination); + address.parse::().is_ok() +} + +/// Whether `destination` (a bare address or an address/prefix CIDR) parses +/// as IPv6. See `parses_as_ipv4` for why this is an invariant, not a table. +fn parses_as_ipv6(destination: &str) -> bool { + let address = destination.split('/').next().unwrap_or(destination); + address.parse::().is_ok() +} + +// --------------------------------------------------------------------------- +// B1 -- deny precedence: blocked-host rules precede allowed-host rules. +// --------------------------------------------------------------------------- + +#[test] +fn a_destination_in_both_lists_is_dropped_because_deny_rules_are_emitted_first() { + let destination = "203.0.113.44"; + let policy = ContainerPolicy { + blocked_hosts: vec![destination.to_string()], + allowed_hosts: vec![destination.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args(CHAIN, &policy); + + // B1: both rules are present -- there is no de-duplication pass -- and + // the DROP rule precedes the ACCEPT rule so first-match-wins denies. + assert_eq!( + args.ipv4.len(), + 2, + "expected one DROP rule and one ACCEPT rule for a doubly-listed \ + destination; actual: {:?}", + args.ipv4 + ); + assert_eq!( + as_str_slice(&args.ipv4[0]), + vec!["-A", CHAIN, "-d", destination, "-j", "DROP"], + "the deny rule must be emitted first; actual first rule: {:?}", + args.ipv4[0] + ); + assert_eq!( + as_str_slice(&args.ipv4[1]), + vec!["-A", CHAIN, "-d", destination, "-j", "ACCEPT"], + "the allow rule must follow the deny rule; actual second rule: {:?}", + args.ipv4[1] + ); + assert!( + args.ipv6.is_empty(), + "an IPv4-only policy must not produce IPv6 rules; actual: {:?}", + args.ipv6 + ); +} + +#[test] +fn an_ipv6_destination_in_both_lists_is_dropped_because_deny_rules_are_emitted_first() { + let destination = "2001:db8::44"; + let policy = ContainerPolicy { + blocked_hosts: vec![destination.to_string()], + allowed_hosts: vec![destination.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args(CHAIN, &policy); + + assert_eq!( + args.ipv6.len(), + 2, + "expected one DROP rule and one ACCEPT rule for a doubly-listed \ + IPv6 destination; actual: {:?}", + args.ipv6 + ); + assert_eq!( + as_str_slice(&args.ipv6[0]), + vec!["-A", CHAIN, "-d", destination, "-j", "DROP"], + "the deny rule must be emitted first; actual first rule: {:?}", + args.ipv6[0] + ); + assert_eq!( + as_str_slice(&args.ipv6[1]), + vec!["-A", CHAIN, "-d", destination, "-j", "ACCEPT"], + "the allow rule must follow the deny rule; actual second rule: {:?}", + args.ipv6[1] + ); + assert!( + args.ipv4.is_empty(), + "an IPv6-only policy must not produce IPv4 rules; actual: {:?}", + args.ipv4 + ); +} + +#[test] +fn deny_precedence_holds_across_both_families_with_several_entries_in_each_list() { + let policy = ContainerPolicy { + blocked_hosts: vec![ + "10.0.0.0/8".to_string(), + "198.51.100.42/32".to_string(), + "2606:50c0::/32".to_string(), + ], + allowed_hosts: vec![ + "140.82.112.0/20".to_string(), + "203.0.113.44".to_string(), + "2001:db8::/32".to_string(), + "2001:db8::44".to_string(), + ], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args(CHAIN, &policy); + + assert_eq!( + args.ipv4.len(), + 4, + "2 blocked + 2 allowed IPv4 destinations must produce 4 IPv4 rules; \ + actual: {:?}", + args.ipv4 + ); + assert_deny_precedence( + &args.ipv4, + &["10.0.0.0/8", "198.51.100.42/32"], + &["140.82.112.0/20", "203.0.113.44"], + ); + + assert_eq!( + args.ipv6.len(), + 3, + "1 blocked + 2 allowed IPv6 destinations must produce 3 IPv6 rules; \ + actual: {:?}", + args.ipv6 + ); + assert_deny_precedence( + &args.ipv6, + &["2606:50c0::/32"], + &["2001:db8::/32", "2001:db8::44"], + ); +} + +// --------------------------------------------------------------------------- +// B4 -- unresolvable entries: fail closed only for a blocked host under an +// Allow default; otherwise log a warning and continue. +// --------------------------------------------------------------------------- + +#[test] +fn an_unresolvable_blocked_host_errors_under_an_allow_default_and_names_the_host() { + let host = "140.82.112.0/not-a-prefix"; + let policy = ContainerPolicy { + blocked_hosts: vec![host.to_string()], + default_network_policy: NetworkPolicy::Allow, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + + let err = match result { + Err(err) => err, + Ok(args) => panic!( + "expected Err: a blocked, unresolvable host under an Allow \ + default leaves nothing to stop traffic (B4); actual ipv4: {:?}, \ + ipv6: {:?}", + args.ipv4, args.ipv6 + ), + }; + assert!( + err.contains(host), + "the error message must name the offending host '{host}'; actual \ + message: {err:?}" + ); +} + +#[test] +fn the_same_unresolvable_blocked_host_does_not_error_under_a_block_default() { + let host = "140.82.112.0/not-a-prefix"; + let policy = ContainerPolicy { + blocked_hosts: vec![host.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + let args = expect_ok( + result, + "a Block default already denies everything the allow list did not \ + name, so an unresolvable block entry is redundant, not fatal (B4)", + ); + + assert!( + args.ipv4.is_empty() && args.ipv6.is_empty(), + "an unresolvable entry contributes no rules; actual ipv4: {:?}, \ + ipv6: {:?}", + args.ipv4, + args.ipv6 + ); + + let expected_warning = format!("Warning: could not resolve host '{host}'"); + assert!( + logger + .get_buffer() + .lines() + .any(|line| line == expected_warning), + "expected the exact warning line {expected_warning:?}; actual \ + buffer: {:?}", + logger.get_buffer() + ); +} + +#[test] +fn an_unresolvable_allowed_host_never_errors_under_an_allow_default() { + let host = "/20"; + let policy = ContainerPolicy { + allowed_hosts: vec![host.to_string()], + default_network_policy: NetworkPolicy::Allow, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + let args = expect_ok( + result, + "B4 reserves Err for an unresolvable BLOCK entry under an Allow \ + default; an unresolvable ALLOW entry must never error", + ); + + assert!( + args.ipv4.is_empty() && args.ipv6.is_empty(), + "an unresolvable entry contributes no rules; actual ipv4: {:?}, \ + ipv6: {:?}", + args.ipv4, + args.ipv6 + ); + + let expected_warning = format!("Warning: could not resolve host '{host}'"); + assert!( + logger + .get_buffer() + .lines() + .any(|line| line == expected_warning), + "expected the exact warning line {expected_warning:?}; actual \ + buffer: {:?}", + logger.get_buffer() + ); +} + +#[test] +fn an_unresolvable_allowed_host_never_errors_under_a_block_default() { + let host = "140.82.112.0/20/8"; + let policy = ContainerPolicy { + allowed_hosts: vec![host.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + let args = expect_ok( + result, + "an unresolvable ALLOW entry must never error, regardless of the \ + default network policy (B4)", + ); + + assert!( + args.ipv4.is_empty() && args.ipv6.is_empty(), + "an unresolvable entry contributes no rules; actual ipv4: {:?}, \ + ipv6: {:?}", + args.ipv4, + args.ipv6 + ); + + let expected_warning = format!("Warning: could not resolve host '{host}'"); + assert!( + logger + .get_buffer() + .lines() + .any(|line| line == expected_warning), + "expected the exact warning line {expected_warning:?}; actual \ + buffer: {:?}", + logger.get_buffer() + ); +} + +#[test] +fn an_unresolvable_entry_does_not_suppress_a_sibling_entrys_rule_or_log_line() { + let good_destination = "198.51.100.42/32"; + let bad_host = "2606:50c0::/129"; + let policy = ContainerPolicy { + blocked_hosts: vec![good_destination.to_string(), bad_host.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + let args = expect_ok( + result, + "an unresolvable block entry under a Block default must not error, \ + and must not stop a sibling entry in the same call from producing \ + a rule (B4)", + ); + + assert_eq!( + args.ipv4.len(), + 1, + "the resolvable sibling must still produce exactly one rule; \ + actual: {:?}", + args.ipv4 + ); + assert_eq!( + as_str_slice(&args.ipv4[0]), + vec!["-A", CHAIN, "-d", good_destination, "-j", "DROP"], + "actual rule: {:?}", + args.ipv4[0] + ); + + let buffer = logger.get_buffer(); + let expected_warning = format!("Warning: could not resolve host '{bad_host}'"); + assert!( + buffer.lines().any(|line| line == expected_warning), + "expected the warning line for the unresolvable sibling; actual \ + buffer: {buffer:?}" + ); + let expected_programmed_line = + format!("Programmed iptables rule: -A {CHAIN} -d {good_destination} -j DROP"); + assert!( + buffer.lines().any(|line| line == expected_programmed_line), + "expected the programmed-rule line for the resolvable sibling; \ + actual buffer: {buffer:?}" + ); +} + +// --------------------------------------------------------------------------- +// B2 / B3 -- rule shape and IPv4 / IPv6 family split. +// --------------------------------------------------------------------------- + +#[test] +fn emitted_rules_have_the_exact_iptables_shape_for_both_allow_and_block_actions() { + let allowed = "203.0.113.44"; + let blocked = "10.0.0.0/8"; + let chain = "mxc_shape_chain"; + let policy = ContainerPolicy { + allowed_hosts: vec![allowed.to_string()], + blocked_hosts: vec![blocked.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args(chain, &policy); + + assert_eq!( + args.ipv4.len(), + 2, + "one block entry and one allow entry must produce exactly 2 rules; \ + actual: {:?}", + args.ipv4 + ); + // B2: exactly `["-A", chain_name, "-d", destination, "-j", target]` -- + // no more, no fewer arguments, and in this order. + assert_eq!( + as_str_slice(&args.ipv4[0]), + vec!["-A", chain, "-d", blocked, "-j", "DROP"], + "actual rule: {:?}", + args.ipv4[0] + ); + assert_eq!( + as_str_slice(&args.ipv4[1]), + vec!["-A", chain, "-d", allowed, "-j", "ACCEPT"], + "actual rule: {:?}", + args.ipv4[1] + ); + for rule in &args.ipv4 { + assert_eq!( + rule.len(), + 6, + "a rule must have exactly 6 arguments; actual: {rule:?}" + ); + } +} + +#[test] +fn ipv4_and_ipv6_destinations_are_split_into_the_correct_bucket_and_never_cross_over() { + let policy = ContainerPolicy { + allowed_hosts: vec!["140.82.112.0/20".to_string(), "2001:db8::/32".to_string()], + blocked_hosts: vec!["198.51.100.42/32".to_string(), "fe80::1".to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args(CHAIN, &policy); + + // Property, not an enumerated example: every destination placed in the + // v4 bucket must itself parse as IPv4, and likewise for v6. This is what + // actually catches a leak, unlike checking the four inputs by name. + for rule in &args.ipv4 { + let destination = destination_of(rule); + assert!( + parses_as_ipv4(destination), + "a destination in the ipv4 bucket must parse as IPv4; actual \ + destination: {destination:?}" + ); + } + for rule in &args.ipv6 { + let destination = destination_of(rule); + assert!( + parses_as_ipv6(destination), + "a destination in the ipv6 bucket must parse as IPv6; actual \ + destination: {destination:?}" + ); + } + + assert_eq!( + args.ipv4.len(), + 2, + "2 of the 4 destinations are IPv4; actual: {:?}", + args.ipv4 + ); + assert_eq!( + args.ipv6.len(), + 2, + "2 of the 4 destinations are IPv6; actual: {:?}", + args.ipv6 + ); +} + +// --------------------------------------------------------------------------- +// B5 -- programmed-rule logging. +// --------------------------------------------------------------------------- + +#[test] +fn programmed_rules_are_logged_with_the_exact_iptables_and_ip6tables_prefixes() { + let allowed_v4 = "203.0.113.44"; + let blocked_v6 = "2606:50c0::/32"; + let chain = "mxc_log_chain"; + let policy = ContainerPolicy { + allowed_hosts: vec![allowed_v4.to_string()], + blocked_hosts: vec![blocked_v6.to_string()], + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(chain, &policy, &mut logger); + let args = expect_ok(result, "both entries resolve, so no error is expected here"); + + assert_eq!(args.ipv4.len(), 1, "actual: {:?}", args.ipv4); + assert_eq!(args.ipv6.len(), 1, "actual: {:?}", args.ipv6); + + let buffer = logger.get_buffer(); + // Hard-coded from B5's documented format, not derived from `args`, so + // this test still pins the log format even if the rule-content tests + // elsewhere were themselves wrong. + let expected_ipv4_line = + format!("Programmed iptables rule: -A {chain} -d {allowed_v4} -j ACCEPT"); + let expected_ipv6_line = + format!("Programmed ip6tables rule: -A {chain} -d {blocked_v6} -j DROP"); + assert!( + buffer.lines().any(|line| line == expected_ipv4_line), + "expected the IPv4 programmed-rule line {expected_ipv4_line:?}; \ + actual buffer: {buffer:?}" + ); + assert!( + buffer.lines().any(|line| line == expected_ipv6_line), + "expected the IPv6 programmed-rule line {expected_ipv6_line:?}; \ + actual buffer: {buffer:?}" + ); +} + +// --------------------------------------------------------------------------- +// B6 -- empty policy. +// --------------------------------------------------------------------------- + +#[test] +fn an_empty_policy_produces_an_empty_ok_result_with_no_log_output() { + let policy = ContainerPolicy { + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }; + let mut logger = Logger::new(Mode::Buffer); + + let result = NetworkIptablesManager::build_policy_rules_logged(CHAIN, &policy, &mut logger); + let args = expect_ok(result, "B6: empty host lists must still return Ok"); + + assert!( + args.ipv4.is_empty(), + "an empty policy must produce no IPv4 rules; actual: {:?}", + args.ipv4 + ); + assert!( + args.ipv6.is_empty(), + "an empty policy must produce no IPv6 rules; actual: {:?}", + args.ipv6 + ); + assert!( + logger.get_buffer().is_empty(), + "with nothing to program and nothing unresolvable, nothing should \ + be logged; actual buffer: {:?}", + logger.get_buffer() + ); +} diff --git a/tests/configs/lxc_network_deny_precedence_control.json b/tests/configs/lxc_network_deny_precedence_control.json new file mode 100644 index 00000000..8c7c1178 --- /dev/null +++ b/tests/configs/lxc_network_deny_precedence_control.json @@ -0,0 +1,21 @@ +{ + "version": "0.8.0-alpha", + "containerId": "CLI-LXC-Net-DenyCtl", + "containment": "lxc", + "process": { + "commandLine": "sh -c \"wget -qO- --timeout=10 https://api.github.com/zen >/dev/null 2>&1 && echo MXC_NET_ALLOWED || echo MXC_NET_BLOCKED\"" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": ["0.0.0.0/0", "::/0"], + "blockedHosts": [] + } +} diff --git a/tests/configs/lxc_network_deny_precedence_overlap.json b/tests/configs/lxc_network_deny_precedence_overlap.json new file mode 100644 index 00000000..15e3277c --- /dev/null +++ b/tests/configs/lxc_network_deny_precedence_overlap.json @@ -0,0 +1,21 @@ +{ + "version": "0.8.0-alpha", + "containerId": "CLI-LXC-Net-DenyWins", + "containment": "lxc", + "process": { + "commandLine": "sh -c \"wget -qO- --timeout=10 https://api.github.com/zen >/dev/null 2>&1 && echo MXC_NET_ALLOWED || echo MXC_NET_BLOCKED\"" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": ["0.0.0.0/0", "::/0"], + "blockedHosts": ["0.0.0.0/0", "::/0"] + } +} diff --git a/tests/scripts/run_lxc_all_tests.sh b/tests/scripts/run_lxc_all_tests.sh index 6410c85b..8caa0579 100644 --- a/tests/scripts/run_lxc_all_tests.sh +++ b/tests/scripts/run_lxc_all_tests.sh @@ -67,6 +67,7 @@ run_test "LXC Network Invalid CIDR" "$SCRIPT_DIR/run_lxc_network_invalid_cidr_te run_test "LXC Network Dual-Stack Hostname" "$SCRIPT_DIR/run_lxc_network_dualstack_test.sh" run_test "LXC Network CIDR Boundary" "$SCRIPT_DIR/run_lxc_network_cidr_boundary_test.sh" run_test "LXC Network Enforcement" "$SCRIPT_DIR/run_lxc_network_enforcement_test.sh" +run_test "LXC Network Deny Precedence" "$SCRIPT_DIR/run_lxc_network_deny_precedence_test.sh" run_test "LXC Timeout" "$SCRIPT_DIR/run_lxc_timeout_test.sh" run_test "LXC Env+Cwd" "$SCRIPT_DIR/run_lxc_env_cwd_test.sh" diff --git a/tests/scripts/run_lxc_network_deny_precedence_test.sh b/tests/scripts/run_lxc_network_deny_precedence_test.sh new file mode 100644 index 00000000..770ffbfb --- /dev/null +++ b/tests/scripts/run_lxc_network_deny_precedence_test.sh @@ -0,0 +1,96 @@ +#!/bin/bash +# LXC deny-precedence enforcement test +# +# A destination named in both allowedHosts and blockedHosts must be blocked. +# The chain is first-match-wins, so this is decided entirely by which list is +# emitted first -- there is no separate precedence pass to assert on. That +# makes it invisible to any test that only inspects rules individually, and it +# is why this assertion is behavioral rather than a log grep. +# +# Both configs name the same destination set, 0.0.0.0/0 and ::/0, so the rules +# are literal CIDRs rather than a hostname resolved once per list entry. A +# hostname would be resolved separately for the allow entry and the block +# entry, and round-robin DNS could hand back different addresses for the two, +# which would make the outcome depend on which address wget happened to pick. +# +# The control run is what makes the overlap run mean anything. Without it, a +# host with no working egress at all -- or a change that broke networking +# outright -- would produce the same blocked verdict and look like a pass. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_DIR="$(dirname "$(dirname "$SCRIPT_DIR")")" +LXC_EXEC="$REPO_DIR/src/target/release/lxc-exec" + +if [ ! -f "$LXC_EXEC" ]; then + LXC_EXEC="$REPO_DIR/src/target/debug/lxc-exec" +fi + +# An honest skip for a missing prerequisite: exit 77 so run_lxc_all_tests.sh +# records SKIPPED rather than PASS. A suite that could not run must not look green. +SKIP_EXIT=77 +skip() { + echo "SKIP: $1" + exit "$SKIP_EXIT" +} + +[ "$(id -u)" -eq 0 ] || skip "requires root for iptables/ip6tables and LXC." +command -v iptables >/dev/null 2>&1 || skip "iptables is not installed." +command -v ip6tables >/dev/null 2>&1 || skip "ip6tables is not installed." +command -v lxc-create >/dev/null 2>&1 || skip "LXC (lxc-create) is not installed." +[ -f "$LXC_EXEC" ] || skip "lxc-exec binary not built; run build.sh first." + +OVERLAP_CONFIG="$REPO_DIR/tests/configs/lxc_network_deny_precedence_overlap.json" +CONTROL_CONFIG="$REPO_DIR/tests/configs/lxc_network_deny_precedence_control.json" +OVERLAP_CHAIN="MXC-CLI-LXC-Net-DenyWins" +CONTROL_CHAIN="MXC-CLI-LXC-Net-DenyCtl" + +fail() { + echo "FAIL: $1" + exit 1 +} + +assert_firewall_chain_cleaned_up() { + if iptables -S "$1" >/dev/null 2>&1; then + fail "iptables chain '$1' was left behind after lxc-exec completed." + fi + if ip6tables -S "$1" >/dev/null 2>&1; then + fail "ip6tables chain '$1' was left behind after lxc-exec completed." + fi +} + +assert_no_forward_reference() { + if iptables -S FORWARD 2>/dev/null | grep -Fq -- "$1"; then + fail "a FORWARD rule still references chain '$1' after teardown." + fi +} + +echo "Running LXC deny-precedence enforcement test..." + +echo "--- control: destination allowed, nothing blocked ---" +CONTROL_OUTPUT=$("$LXC_EXEC" --debug "$CONTROL_CONFIG" 2>&1 || true) +echo "$CONTROL_OUTPUT" + +if ! echo "$CONTROL_OUTPUT" | grep -Fq "MXC_NET_ALLOWED"; then + fail "the control destination was unreachable with an allow-everything policy, so this host cannot distinguish a deny-precedence failure from a broken network." +fi + +assert_no_forward_reference "$CONTROL_CHAIN" +assert_firewall_chain_cleaned_up "$CONTROL_CHAIN" + +echo "--- overlap: same destination in both allowedHosts and blockedHosts ---" +OVERLAP_OUTPUT=$("$LXC_EXEC" --debug "$OVERLAP_CONFIG" 2>&1 || true) +echo "$OVERLAP_OUTPUT" + +if echo "$OVERLAP_OUTPUT" | grep -Fq "MXC_NET_ALLOWED"; then + fail "a destination present in BOTH allowedHosts and blockedHosts was reachable. Allow rules are winning over deny rules, so a blocklist entry can be silently defeated by an overlapping allowlist entry." +fi +if ! echo "$OVERLAP_OUTPUT" | grep -Fq "MXC_NET_BLOCKED"; then + fail "the overlap case produced no verdict at all; the container command did not run." +fi + +assert_no_forward_reference "$OVERLAP_CHAIN" +assert_firewall_chain_cleaned_up "$OVERLAP_CHAIN" + +echo "PASS: a destination in both lists was blocked, and the same destination was reachable when only allowed." +echo "LXC deny-precedence enforcement test complete."