[LXC] Fail closed when firewall rules cannot be scoped to the container

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 <veth>`. 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;

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()
+    );
+}