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..1b565cff --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_veth_spec.rs @@ -0,0 +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. + +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() + ); +}