diff --git a/.github/workflows/lxc-e2e.yml b/.github/workflows/lxc-e2e.yml new file mode 100644 index 000000000..0d6f81172 --- /dev/null +++ b/.github/workflows/lxc-e2e.yml @@ -0,0 +1,133 @@ +name: LXC E2E Tests + +on: + push: + branches: [main] + pull_request: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +jobs: + lxc-e2e: + name: LXC-Exec Container and Network Policy + runs-on: ubuntu-latest + timeout-minutes: 45 + + steps: + - uses: actions/checkout@v6 + + - name: Setup Rust toolchain + run: rustup update stable + + - name: Point cargo at the MxcDependencies feed + uses: ./.github/actions/setup-cargo-feed + + - name: Cache Rust build artifacts + uses: Swatinem/rust-cache@v2 + with: + workspaces: src + + - name: Install LXC and firewall tooling + run: | + sudo apt-get update + sudo DEBIAN_FRONTEND=noninteractive apt-get install -y \ + lxc lxc-templates lxc-utils iptables debootstrap uidmap bridge-utils + + # A bridged veth only reaches the FORWARD chain while br_netfilter is + # delivering bridged packets to iptables. Without it the firewall rules + # install cleanly and never fire, so the network policy tests would pass + # against a firewall that filters nothing. + - name: Enable bridge netfilter + run: | + sudo modprobe br_netfilter + sudo sysctl -w net.bridge.bridge-nf-call-iptables=1 + sudo sysctl -w net.bridge.bridge-nf-call-ip6tables=1 + + # GitHub-hosted runners ship Docker, and Docker sets the IPv4 FORWARD + # policy to DROP. That breaks these tests twice over. + # + # First, it breaks them outright. MXC hooks its chain on traffic leaving + # the container (`-i ` / `--physdev-in `), so an allowed + # request is accepted on the way out -- but the reply arrives in the + # opposite direction, matches no MXC rule, falls through to the policy, + # and is dropped. The connection times out and an explicitly allowed + # destination looks unreachable. Observed exactly that: DNS resolved, + # because dnsmasq on lxcbr0 is host-local and never traverses FORWARD, + # and then `wget: can't connect to remote host (140.82.116.5)`. + # + # Second, and worse, it would make the deny cases meaningless. Under a + # DROP policy a container with NO working MXC hook at all is also + # unreachable, so the enforcement and deny-precedence tests would report + # success against a firewall that filters nothing -- which is the precise + # bug this suite exists to detect, and the reason these tests carry + # positive controls. + # + # Setting the policy to ACCEPT restores the condition the tests were + # written for: the host forwards by default, so the ONLY thing that can + # block container traffic is a rule MXC installed. A missing hook then + # shows up as an unexpected success and fails the deny case loudly. + # A narrower conntrack RELATED,ESTABLISHED rule would fix the reply path + # but leave the DROP policy, and with it the vacuous pass. + - name: Let the host forward, so only MXC rules can block + run: | + sudo iptables -P FORWARD ACCEPT + sudo ip6tables -P FORWARD ACCEPT + sudo iptables -S FORWARD | head -5 + + - name: Report the environment these tests depend on + run: | + echo "--- kernel ---" + uname -a + echo "--- lxc ---" + lxc-create --version || echo "MISSING lxc-create" + echo "--- iptables ---" + sudo iptables --version || echo "MISSING iptables" + sudo ip6tables --version || echo "MISSING ip6tables" + echo "--- forward policy (must be ACCEPT, or deny cases pass vacuously) ---" + sudo iptables -S FORWARD | head -1 + sudo ip6tables -S FORWARD | head -1 + echo "--- bridge netfilter ---" + cat /proc/sys/net/bridge/bridge-nf-call-iptables || echo "MISSING bridge-nf-call-iptables" + cat /proc/sys/net/bridge/bridge-nf-call-ip6tables || echo "MISSING bridge-nf-call-ip6tables" + echo "--- host ipv6 ---" + cat /proc/net/if_inet6 || echo "no /proc/net/if_inet6 (IPv6 disabled)" + + - name: Build lxc-exec + working-directory: src + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: cargo build --release -p lxc --bin lxc-exec + + # MXC_LXC_TESTS_REQUIRE_EXECUTION turns an honest skip into a failure. + # On a developer box a missing ip6tables is a reason to run what you can. + # Here the runner is provisioned specifically to execute this suite, so a + # skip means a prerequisite disappeared and the gate would go green while + # testing nothing. + - name: Run LXC E2E suite + env: + MXC_LXC_TESTS_REQUIRE_EXECUTION: "1" + run: sudo --preserve-env=MXC_LXC_TESTS_REQUIRE_EXECUTION bash tests/scripts/run_lxc_all_tests.sh + + - name: Show leftover firewall state on failure + if: failure() + run: | + echo "--- FORWARD chain ---" + sudo iptables -S FORWARD || true + sudo ip6tables -S FORWARD || true + echo "--- MXC chains ---" + sudo iptables -S | grep -E '^-N MXC-' || echo "none" + sudo ip6tables -S | grep -E '^-N MXC-' || echo "none" + + - name: Upload logs on failure + if: failure() || cancelled() + uses: actions/upload-artifact@v6 + with: + name: lxc-e2e-logs-${{ github.event.pull_request.number || github.run_number }} + retention-days: 7 + path: | + logs/ + **/*.log diff --git a/docs/lxc-support/lxc-backend.md b/docs/lxc-support/lxc-backend.md index 72c186c57..1f332ebab 100644 --- a/docs/lxc-support/lxc-backend.md +++ b/docs/lxc-support/lxc-backend.md @@ -116,9 +116,49 @@ Network policies are enforced with parallel `iptables` and `ip6tables` chains sc | `defaultPolicy: "block"` | Final DROP rule in the container chain | | `defaultPolicy: "allow"` | Final ACCEPT rule in the container chain | | `allowedHosts` | ACCEPT rules for IP literals, CIDR blocks, or resolved hostnames | -| `blockedHosts` | DROP rules for IP literals, CIDR blocks, or resolved hostnames | - -`allowedHosts` and `blockedHosts` entries may be bare IPv4/IPv6 literals, IPv4/IPv6 CIDR blocks, or hostnames. Hostnames are resolved to both A and AAAA records; IPv4 destinations are applied to the `iptables` chain and IPv6 destinations are applied to the `ip6tables` chain. Entries whose CIDR prefix is out of range for its family (or otherwise malformed) are reported as unresolved and skipped, leaving the rest of the policy in force. Host-list rules match all ports and protocols; port- and protocol-specific egress rules are not supported. +| `blockedHosts` | DROP rules for IP literals, CIDR blocks, or resolved hostnames, emitted *before* the ACCEPT rules | + +**A deny wins over an overlapping allow.** `iptables` evaluates a chain top to +bottom and stops at the first match, so precedence is decided purely by +emission order. All `blockedHosts` rules are emitted ahead of all +`allowedHosts` rules, which means a destination named by both lists is dropped. +Without that ordering an allow entry broad enough to cover a blocked +destination — `0.0.0.0/0`, or a CIDR containing the blocked address — silently +defeats the block, and the resulting chain looks fully populated while +filtering nothing. + +Two limits on that guarantee are worth stating plainly, because "deny always +wins" is not true without them: + +- **DNS is exempt.** The base chain accepts UDP and TCP destination port 53 + unconditionally and is installed ahead of the generated policy rules, so + port-53 traffic to a blocked destination is accepted before its DROP rule is + reached. Narrowing that rule needs to know which resolver addresses are + legitimate, and no schema field carries them today. +- **A hostname in both lists is resolved twice.** Each list entry is resolved + independently, so a name behind round-robin DNS can return one address for + the `blockedHosts` entry and a different one for the `allowedHosts` entry. + The guarantee holds for *addresses*, not for names. Use literal IPs or CIDRs + when a destination must be denied deterministically. + +`allowedHosts` and `blockedHosts` entries may be bare IPv4/IPv6 literals, IPv4/IPv6 CIDR blocks, or hostnames. Hostnames are resolved to both A and AAAA records; IPv4 destinations are applied to the `iptables` chain and IPv6 destinations are applied to the `ip6tables` chain. Host-list rules match all ports and protocols; port- and protocol-specific egress rules are not supported. + +An entry that resolves to nothing — an unknown hostname, or a CIDR prefix out +of range for its family — cannot be turned into a rule. What that costs +depends on the entry and on `defaultPolicy`: + +| Entry | `defaultPolicy` | Behavior | +|-------|-----------------|----------| +| `allowedHosts` | either | Reported as unresolved and skipped. Failing to write an ACCEPT rule can only make the policy more restrictive | +| `blockedHosts` | `block` | Reported as unresolved and skipped. The closing DROP already denies the destination, so the unwritten rule was redundant | +| `blockedHosts` | `allow` | **Fails firewall setup.** The chain ends in ACCEPT, so the unwritten DROP was the only thing that would have denied that destination, and skipping it silently converts a deny into an allow | + +One gap remains open and is not detected: under `defaultPolicy: "block"`, an +`allowedHosts` entry broad enough to cover a destination whose `blockedHosts` +rule went unwritten still reaches that destination. Detecting it would require +the address the failed entry was *meant* to resolve to, which is by definition +unavailable, so no check over the policy text can be complete — and a partial +check would imply a guarantee this code cannot make. Before programming the IPv6 chain, MXC probes `ip6tables` with a read-only `ip6tables -S` and classifies the result three ways: @@ -130,7 +170,40 @@ Before programming the IPv6 chain, MXC probes `ip6tables` with a read-only `ip6t Host IPv6 activity is read from `/proc/net/if_inet6`: a non-loopback interface with an IPv6 address counts as active, while loopback-only `::1` on `lo` (present even on IPv4-only hosts) does not. If that file cannot be read at all — as opposed to being absent, which means IPv6 is disabled — the state is treated as *unknown* rather than as a confirmed "IPv6 is off", so an unreadable IPv6 state fails closed instead of leaving IPv6 unfiltered. -The chains are hooked into `FORWARD` for container egress by matching the host-side veth as the input interface. If MXC cannot discover the container veth, it skips the `FORWARD` hook with a warning rather than applying host-wide rules. +The chains are hooked into `FORWARD` for container egress with **up to two +rules per family**, because the input interface `FORWARD` sees depends on how +the veth is attached: + +| Attachment | Rule that matches | +|------------|-------------------| +| veth routed directly by the host | `-i ` | +| veth enslaved to a bridge (the default LXC topology) | `-m physdev --physdev-in ` | + +The two are mutually exclusive for any given packet, so nothing is counted +twice. Installing only `-i ` is what previously let a fully populated +deny-all chain sit in the ruleset filtering nothing on the default bridged +topology. + +The `physdev` rule is required only on a bridged veth. On a directly routed +veth a host whose kernel lacks the `physdev` match logs a warning and +continues with the interface rule alone, which is the rule that matches there; +on a bridged veth the same failure is fatal, because `physdev` is the only +rule that could ever match. + +A bridged veth additionally requires `br_netfilter` to be delivering bridged +packets to iptables. With `/proc/sys/net/bridge/bridge-nf-call-iptables` absent +or `0`, both hook rules install cleanly and neither ever fires. MXC reads that +file and **fails firewall setup** rather than reporting success for a chain +that could never be reached. When the IPv6 chain is programmed, +`/proc/sys/net/bridge/bridge-nf-call-ip6tables` is checked separately and to +the same standard. + +If MXC cannot discover the container veth at all, firewall setup **fails** and +the partially created chains are rolled back. An unhooked chain is never +traversed, so reporting success would hand the caller a deny-all chain that +filters nothing — strictly worse than no firewall, because it looks enforced. +Installing the rules host-wide instead is not an option either: unscoped, they +would apply to every container and to the host's own traffic. Firewall state is torn down automatically with best-effort removal of the `FORWARD` hooks and both per-container chains; there is no network-policy opt-out field. Setup failures after partial creation are rolled back before returning an error, so retries do not trip over leftover chains. diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 79f5fe96f..7f9ca0eea 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 @@ -486,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 @@ -510,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 = @@ -554,7 +732,7 @@ impl NetworkIptablesManager { } args.extend(rule_args); } - args + Ok(args) } /// Run an iptables command and return success/failure. @@ -995,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)?; @@ -1022,39 +1203,120 @@ 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, 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 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. + // + // 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(()) @@ -1092,32 +1354,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); @@ -1126,7 +1424,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); @@ -1228,6 +1526,28 @@ 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; + +/// 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; + +/// 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; @@ -1916,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] @@ -2473,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 000000000..14cd0ea26 --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs @@ -0,0 +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. +//! +//! 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/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 000000000..c87356eea --- /dev/null +++ b/src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs @@ -0,0 +1,458 @@ +// 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. + +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/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 000000000..1b565cff6 --- /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() + ); +} 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 000000000..8c7c11780 --- /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 000000000..15e3277c5 --- /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/configs/lxc_network_enforcement_allow.json b/tests/configs/lxc_network_enforcement_allow.json new file mode 100644 index 000000000..618688d9d --- /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 000000000..73cd2a35b --- /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 0510bb4d8..e72108311 100644 --- a/tests/scripts/run_lxc_all_tests.sh +++ b/tests/scripts/run_lxc_all_tests.sh @@ -66,6 +66,8 @@ 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 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" @@ -79,6 +81,22 @@ fi if [ "$PASSED" -eq 0 ] && [ "$FAILED" -eq 0 ]; then echo "WARNING: no tests actually executed; every test was skipped." fi +# Strict mode, for continuous integration. A developer box legitimately lacks +# ip6tables or LXC and should be able to run what it can, so a skip is only a +# warning there. On a runner provisioned to execute this suite, a skip means a +# prerequisite silently disappeared, and the gate would then go green while +# testing nothing -- which is the precise way an unenforced firewall shipped. +if [ "${MXC_LXC_TESTS_REQUIRE_EXECUTION:-0}" != "0" ]; then + if [ "$PASSED" -eq 0 ] && [ "$FAILED" -eq 0 ]; then + echo "ERROR: strict mode: no test executed. Refusing to report success." + exit 1 + fi + if [ "$SKIPPED" -gt 0 ]; then + echo "ERROR: strict mode: $SKIPPED test(s) skipped a prerequisite that this" + echo "environment is supposed to provide. Refusing to report success." + exit 1 + fi +fi if [ $FAILED -gt 0 ]; then echo -e "Failures:$FAILURES" exit 1 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 000000000..770ffbfb2 --- /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." 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 000000000..9887c87e9 --- /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