From 8e87c5bf571eca786444b5399802df85255cf538 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Fri, 10 Jul 2026 15:36:39 -0700 Subject: [PATCH 1/7] [LXC/Bwrap] Network model 1: IPv6+CIDR, port, and protocol filtering (AB#62830559) - resolve_host returns dual-stack; add ip6tables v6 chain mirroring the v4 chain. - CIDR (v4/v6) passthrough; per-rule --dport and -p tcp/udp/icmp via new EgressRule model field. - Pure rule-builder helpers with unit tests; update legacy IPv6-drop tests to dual-stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4 --- .../lxc/common/src/network_iptables.rs | 747 ++++++++++++++---- src/core/wxc_common/src/models.rs | 39 + 2 files changed, 654 insertions(+), 132 deletions(-) diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 83374c1e8..c832a05eb 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -4,13 +4,47 @@ //! Network policy enforcement via iptables rules scoped to the LXC container. //! //! Maps the platform-agnostic `ContainerPolicy` network settings to iptables -//! rules applied to the container's virtual ethernet (veth) interface. +//! and ip6tables rules applied to the container's virtual ethernet (veth) +//! interface. -use std::net::ToSocketAddrs; +use std::net::{IpAddr, ToSocketAddrs}; use std::process::Command; use wxc_common::logger::Logger; -use wxc_common::models::{ContainerPolicy, NetworkEnforcementMode, NetworkPolicy}; +use wxc_common::models::{ + ContainerPolicy, EgressRule, NetworkEnforcementMode, NetworkPolicy, Protocol, RuleAction, +}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum IpFamily { + V4, + V6, +} + +#[derive(Debug, Default, Clone, PartialEq, Eq)] +struct ResolvedDestinations { + ipv4: Vec, + ipv6: Vec, +} + +impl ResolvedDestinations { + fn is_empty(&self) -> bool { + self.ipv4.is_empty() && self.ipv6.is_empty() + } +} + +#[derive(Debug, Default, Clone, PartialEq, Eq)] +struct FirewallRuleArgs { + ipv4: Vec>, + ipv6: Vec>, +} + +impl FirewallRuleArgs { + fn extend(&mut self, other: FirewallRuleArgs) { + self.ipv4.extend(other.ipv4); + self.ipv6.extend(other.ipv6); + } +} /// Manages iptables rules for an LXC container's network policy. pub struct NetworkIptablesManager { @@ -81,47 +115,291 @@ impl NetworkIptablesManager { self.veth_interface = Some(iface.to_string()); } - /// Resolve a hostname to IPv4 addresses. + /// Resolve a destination string to IPv4 and IPv6 firewall destinations. /// - /// IPv6 records (AAAA from DNS, or IPv6 literals like `"::1"` / - /// IPv4-mapped IPv6 like `"::ffff:127.0.0.1"`) are silently dropped - /// because `apply_firewall_rules` only invokes `iptables` (the IPv4 - /// tool), which rejects IPv6 destinations. Full dual-stack support - /// via parallel `ip6tables` rules would require a separate change. - /// A host that resolves only to AAAA records will return an empty - /// vec, meaning no allow/deny rule is emitted and the host is - /// effectively unreachable from the sandbox under firewall mode. - fn resolve_host(host: &str) -> Vec { - // Try as IP address first - if let Ok(addr) = host.parse::() { - return if addr.is_ipv4() { - vec![host.to_string()] - } else { - Vec::new() + /// Bare IPv4/IPv6 literals are retained in their matching family. CIDR + /// strings are accepted after validating the network address and prefix + /// length, then passed through unchanged. Hostnames are resolved to both A + /// and AAAA records so IPv4 destinations route to `iptables` and IPv6 + /// destinations route to `ip6tables`. + fn resolve_host(host: &str) -> ResolvedDestinations { + if host.contains('/') { + return match Self::destination_family(host) { + Some(IpFamily::V4) => ResolvedDestinations { + ipv4: vec![host.to_string()], + ipv6: Vec::new(), + }, + Some(IpFamily::V6) => ResolvedDestinations { + ipv4: Vec::new(), + ipv6: vec![host.to_string()], + }, + None => ResolvedDestinations::default(), + }; + } + + // Try as IP address first. + if let Ok(addr) = host.parse::() { + return match addr { + IpAddr::V4(_) => ResolvedDestinations { + ipv4: vec![host.to_string()], + ipv6: Vec::new(), + }, + IpAddr::V6(_) => ResolvedDestinations { + ipv4: Vec::new(), + ipv6: vec![host.to_string()], + }, }; } - // Try DNS resolution - match format!("{}:0", host).to_socket_addrs() { - Ok(addrs) => addrs - .map(|a| a.ip()) - .filter(|ip| ip.is_ipv4()) - .map(|ip| ip.to_string()) - .collect(), - Err(_) => Vec::new(), + // Try DNS resolution. + let mut resolved = ResolvedDestinations::default(); + if let Ok(addrs) = format!("{}:0", host).to_socket_addrs() { + for addr in addrs { + match addr.ip() { + IpAddr::V4(ip) => resolved.ipv4.push(ip.to_string()), + IpAddr::V6(ip) => resolved.ipv6.push(ip.to_string()), + } + } + } + resolved + } + + fn destination_family(destination: &str) -> Option { + if let Some((network, prefix)) = destination.split_once('/') { + if network.is_empty() || prefix.is_empty() || prefix.contains('/') { + return None; + } + + let addr = network.parse::().ok()?; + let prefix = prefix.parse::().ok()?; + return match addr { + IpAddr::V4(_) if prefix <= 32 => Some(IpFamily::V4), + IpAddr::V6(_) if prefix <= 128 => Some(IpFamily::V6), + _ => None, + }; + } + + match destination.parse::().ok()? { + IpAddr::V4(_) => Some(IpFamily::V4), + IpAddr::V6(_) => Some(IpFamily::V6), + } + } + + fn protocol_arg(protocol: &Protocol) -> &'static str { + match protocol { + Protocol::Tcp => "tcp", + Protocol::Udp => "udp", + Protocol::Icmp => "icmp", + } + } + + fn rule_action_arg(action: &RuleAction) -> &'static str { + match action { + RuleAction::Allow => "ACCEPT", + RuleAction::Deny => "DROP", + } + } + + fn build_base_chain_rule_args(chain_name: &str) -> Vec> { + vec![ + vec!["-A", chain_name, "-i", "lo", "-j", "ACCEPT"], + vec![ + "-A", + chain_name, + "-m", + "state", + "--state", + "ESTABLISHED,RELATED", + "-j", + "ACCEPT", + ], + vec![ + "-A", chain_name, "-p", "udp", "--dport", "53", "-j", "ACCEPT", + ], + vec![ + "-A", chain_name, "-p", "tcp", "--dport", "53", "-j", "ACCEPT", + ], + ] + .into_iter() + .map(|args| args.into_iter().map(String::from).collect()) + .collect() + } + + fn build_default_policy_rule_arg(chain_name: &str, policy: NetworkPolicy) -> Vec { + let default_action = match policy { + NetworkPolicy::Block => "DROP", + NetworkPolicy::Allow => "ACCEPT", + }; + vec!["-A", chain_name, "-j", default_action] + .into_iter() + .map(String::from) + .collect() + } + + fn build_resolved_destination_rule_args( + chain_name: &str, + destinations: &ResolvedDestinations, + action: &RuleAction, + ) -> FirewallRuleArgs { + let mut args = FirewallRuleArgs::default(); + for destination in &destinations.ipv4 { + args.ipv4.push(Self::build_single_rule_args( + chain_name, + destination, + action, + None, + None, + )); + } + for destination in &destinations.ipv6 { + args.ipv6.push(Self::build_single_rule_args( + chain_name, + destination, + action, + None, + None, + )); } + args + } + + fn build_destination_rule_args( + chain_name: &str, + destination: &str, + action: &RuleAction, + protocols: &[Protocol], + ports: &[u16], + ) -> FirewallRuleArgs { + let Some(family) = Self::destination_family(destination) else { + return FirewallRuleArgs::default(); + }; + + let protocol_options: Vec> = if protocols.is_empty() && ports.is_empty() { + vec![None] + } else if protocols.is_empty() { + vec![Some(Protocol::Tcp), Some(Protocol::Udp)] + } else { + protocols.iter().cloned().map(Some).collect() + }; + let port_options: Vec> = if ports.is_empty() { + vec![None] + } else { + ports.iter().copied().map(Some).collect() + }; + + let mut args = FirewallRuleArgs::default(); + for protocol in &protocol_options { + for port in &port_options { + let rule = Self::build_single_rule_args( + chain_name, + destination, + action, + protocol.as_ref(), + *port, + ); + match family { + IpFamily::V4 => args.ipv4.push(rule), + IpFamily::V6 => args.ipv6.push(rule), + } + } + } + args + } + + fn build_single_rule_args( + chain_name: &str, + destination: &str, + action: &RuleAction, + protocol: Option<&Protocol>, + port: Option, + ) -> Vec { + let mut args = vec![ + "-A".to_string(), + chain_name.to_string(), + "-d".to_string(), + destination.to_string(), + ]; + if let Some(protocol) = protocol { + args.push("-p".to_string()); + args.push(Self::protocol_arg(protocol).to_string()); + } + if let Some(port) = port { + args.push("--dport".to_string()); + args.push(port.to_string()); + } + args.push("-j".to_string()); + args.push(Self::rule_action_arg(action).to_string()); + args + } + + fn build_legacy_host_rule_args( + chain_name: &str, + host: &str, + action: &RuleAction, + ) -> FirewallRuleArgs { + let destinations = Self::resolve_host(host); + Self::build_resolved_destination_rule_args(chain_name, &destinations, action) + } + + fn build_egress_rule_args(chain_name: &str, rule: &EgressRule) -> FirewallRuleArgs { + let mut args = FirewallRuleArgs::default(); + for destination in &rule.destinations { + args.extend(Self::build_destination_rule_args( + chain_name, + destination, + &rule.action, + &rule.protocols, + &rule.ports, + )); + } + args + } + + fn build_policy_rule_args(chain_name: &str, policy: &ContainerPolicy) -> FirewallRuleArgs { + let mut args = FirewallRuleArgs::default(); + for host in &policy.allowed_hosts { + args.extend(Self::build_legacy_host_rule_args( + chain_name, + host, + &RuleAction::Allow, + )); + } + for host in &policy.blocked_hosts { + args.extend(Self::build_legacy_host_rule_args( + chain_name, + host, + &RuleAction::Deny, + )); + } + for rule in &policy.egress_rules { + args.extend(Self::build_egress_rule_args(chain_name, rule)); + } + args } /// Run an iptables command and return success/failure. fn run_iptables(args: &[&str], logger: &mut Logger) -> Result { - let output = Command::new("iptables") + Self::run_firewall_command("iptables", args, logger) + } + + /// Run an ip6tables command and return success/failure. + fn run_ip6tables(args: &[&str], logger: &mut Logger) -> Result { + Self::run_firewall_command("ip6tables", args, logger) + } + + fn run_firewall_command( + command: &str, + args: &[&str], + logger: &mut Logger, + ) -> Result { + let output = Command::new(command) .args(args) .output() - .map_err(|e| format!("Failed to run iptables: {}", e))?; + .map_err(|e| format!("Failed to run {}: {}", command, e))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - let msg = format!("iptables {} failed: {}", args.join(" "), stderr); + let msg = format!("{} {} failed: {}", command, args.join(" "), stderr); logger.log_line(&msg); return Err(msg); } @@ -129,6 +407,22 @@ impl NetworkIptablesManager { Ok(true) } + fn run_iptables_rule_args(args: &[Vec], logger: &mut Logger) -> Result<(), String> { + for rule in args { + let rule_args: Vec<&str> = rule.iter().map(String::as_str).collect(); + Self::run_iptables(&rule_args, logger)?; + } + Ok(()) + } + + fn run_ip6tables_rule_args(args: &[Vec], logger: &mut Logger) -> Result<(), String> { + for rule in args { + let rule_args: Vec<&str> = rule.iter().map(String::as_str).collect(); + Self::run_ip6tables(&rule_args, logger)?; + } + Ok(()) + } + /// Apply network firewall rules based on the container policy. pub fn apply_firewall_rules( &mut self, @@ -145,98 +439,54 @@ impl NetworkIptablesManager { return Ok(true); } - logger.log_line(&format!("Creating iptables chain: {}", self.chain_name)); + logger.log_line(&format!( + "Creating iptables/ip6tables chain: {}", + self.chain_name + )); - // Create custom chain + // Create custom chains. Self::run_iptables(&["-N", &self.chain_name], logger)?; - - // Always allow loopback and established connections - Self::run_iptables( - &["-A", &self.chain_name, "-i", "lo", "-j", "ACCEPT"], - logger, - )?; - Self::run_iptables( - &[ - "-A", - &self.chain_name, - "-m", - "state", - "--state", - "ESTABLISHED,RELATED", - "-j", - "ACCEPT", - ], - logger, - )?; - - // Allow DNS (needed for hostname resolution) - Self::run_iptables( - &[ - "-A", - &self.chain_name, - "-p", - "udp", - "--dport", - "53", - "-j", - "ACCEPT", - ], - logger, - )?; - Self::run_iptables( - &[ - "-A", - &self.chain_name, - "-p", - "tcp", - "--dport", - "53", - "-j", - "ACCEPT", - ], - logger, - )?; - - // Add allowed host rules - for host in &policy.allowed_hosts { - let ips = Self::resolve_host(host); - if ips.is_empty() { + Self::run_ip6tables(&["-N", &self.chain_name], logger)?; + + let base_rules = Self::build_base_chain_rule_args(&self.chain_name); + Self::run_iptables_rule_args(&base_rules, logger)?; + Self::run_ip6tables_rule_args(&base_rules, logger)?; + + for host in policy + .allowed_hosts + .iter() + .chain(policy.blocked_hosts.iter()) + { + if Self::resolve_host(host).is_empty() { logger.log_line(&format!("Warning: could not resolve host '{}'", host)); - continue; - } - for ip in &ips { - logger.log_line(&format!("Allowing host: {} ({})", host, ip)); - Self::run_iptables(&["-A", &self.chain_name, "-d", ip, "-j", "ACCEPT"], logger)?; } } - // Add blocked host rules - for host in &policy.blocked_hosts { - let ips = Self::resolve_host(host); - if ips.is_empty() { - logger.log_line(&format!("Warning: could not resolve host '{}'", host)); - continue; - } - for ip in &ips { - logger.log_line(&format!("Blocking host: {} ({})", host, ip)); - Self::run_iptables(&["-A", &self.chain_name, "-d", ip, "-j", "DROP"], logger)?; - } - } + let policy_rules = Self::build_policy_rule_args(&self.chain_name, policy); + Self::run_iptables_rule_args(&policy_rules.ipv4, logger)?; + Self::run_ip6tables_rule_args(&policy_rules.ipv6, logger)?; - // Append default policy at end of chain - let default_action = match policy.default_network_policy { - NetworkPolicy::Block => "DROP", - NetworkPolicy::Allow => "ACCEPT", - }; + // Append default policy at end of each chain. + let default_rule = Self::build_default_policy_rule_arg( + &self.chain_name, + policy.default_network_policy.clone(), + ); + let default_args: Vec<&str> = default_rule.iter().map(String::as_str).collect(); + let default_action = default_args.last().copied().unwrap_or("ACCEPT"); logger.log_line(&format!("Default network policy: {}", default_action)); - Self::run_iptables(&["-A", &self.chain_name, "-j", default_action], logger)?; + Self::run_iptables(&default_args, logger)?; + Self::run_ip6tables(&default_args, logger)?; - // Hook the chain into FORWARD for the container's traffic + // Hook the chains into FORWARD for the container's traffic. if let Some(ref iface) = self.veth_interface { Self::run_iptables( &["-I", "FORWARD", "-o", iface, "-j", &self.chain_name], logger, )?; + Self::run_ip6tables( + &["-I", "FORWARD", "-o", iface, "-j", &self.chain_name], + logger, + )?; } 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. @@ -250,13 +500,16 @@ impl NetworkIptablesManager { Ok(true) } - /// Remove all iptables rules created by this manager. + /// Remove all iptables/ip6tables rules created by this manager. pub fn remove_firewall_rules(&mut self, logger: &mut Logger) -> Result<(), String> { if !self.rules_applied { return Ok(()); } - logger.log_line(&format!("Removing iptables chain: {}", self.chain_name)); + logger.log_line(&format!( + "Removing iptables/ip6tables chain: {}", + self.chain_name + )); // Remove from FORWARD (only if we had a veth interface and hooked it) if let Some(ref iface) = self.veth_interface { @@ -264,11 +517,17 @@ impl NetworkIptablesManager { &["-D", "FORWARD", "-o", iface, "-j", &self.chain_name], logger, ); + let _ = Self::run_ip6tables( + &["-D", "FORWARD", "-o", iface, "-j", &self.chain_name], + logger, + ); } - // Flush and delete the chain + // Flush and delete the chains. let _ = Self::run_iptables(&["-F", &self.chain_name], logger); let _ = Self::run_iptables(&["-X", &self.chain_name], logger); + let _ = Self::run_ip6tables(&["-F", &self.chain_name], logger); + let _ = Self::run_ip6tables(&["-X", &self.chain_name], logger); self.rules_applied = false; Ok(()) @@ -306,6 +565,10 @@ impl Drop for NetworkIptablesManager { mod tests { use super::*; + fn strings(args: &[&str]) -> Vec { + args.iter().map(|arg| arg.to_string()).collect() + } + #[test] fn chain_name_sanitization() { let mgr = NetworkIptablesManager::new("my-container_123"); @@ -323,39 +586,259 @@ mod tests { #[test] fn resolve_ip_address() { let ips = NetworkIptablesManager::resolve_host("127.0.0.1"); - assert_eq!(ips, vec!["127.0.0.1"]); + assert_eq!(ips.ipv4, vec!["127.0.0.1"]); + assert!(ips.ipv6.is_empty()); } #[test] - fn resolve_host_drops_ipv6_literal() { - // IPv6 literals must be silently dropped — `iptables` (v4) would - // reject them and fail the whole `apply_firewall_rules` call. + fn resolve_host_retains_ipv6_literal() { let ips = NetworkIptablesManager::resolve_host("::1"); - assert!( - ips.is_empty(), - "expected empty vec for IPv6 literal, got {:?}", - ips - ); + assert!(ips.ipv4.is_empty()); + assert_eq!(ips.ipv6, vec!["::1"]); } #[test] - fn resolve_host_drops_ipv4_mapped_ipv6_literal() { - // `::ffff:127.0.0.1` parses as `IpAddr::V6` and is the v6 - // wire-format encoding of an v4 address — `iptables` would - // still reject it as a v6 destination, so we drop it. + fn resolve_host_retains_ipv4_mapped_ipv6_literal() { let ips = NetworkIptablesManager::resolve_host("::ffff:127.0.0.1"); - assert!( - ips.is_empty(), - "expected empty vec for v4-mapped-v6 literal, got {:?}", - ips - ); + assert!(ips.ipv4.is_empty()); + assert_eq!(ips.ipv6, vec!["::ffff:127.0.0.1"]); } #[test] fn resolve_host_keeps_ipv4_literal_unchanged() { - // Round-trip: v4 literals must pass through verbatim — the - // IPv4-only filter must not regress the happy path. + // Round-trip: v4 literals must pass through verbatim. let ips = NetworkIptablesManager::resolve_host("10.0.0.1"); - assert_eq!(ips, vec!["10.0.0.1"]); + assert_eq!(ips.ipv4, vec!["10.0.0.1"]); + assert!(ips.ipv6.is_empty()); + } + + #[test] + fn resolve_host_retains_valid_cidr_by_family() { + let v4 = NetworkIptablesManager::resolve_host("140.82.112.0/20"); + assert_eq!(v4.ipv4, vec!["140.82.112.0/20"]); + assert!(v4.ipv6.is_empty()); + + let v6 = NetworkIptablesManager::resolve_host("2606:50c0::/32"); + assert!(v6.ipv4.is_empty()); + assert_eq!(v6.ipv6, vec!["2606:50c0::/32"]); + } + + #[test] + fn resolve_host_rejects_invalid_cidr_prefix() { + assert!(NetworkIptablesManager::resolve_host("140.82.112.0/33").is_empty()); + assert!(NetworkIptablesManager::resolve_host("2606:50c0::/129").is_empty()); + assert!(NetworkIptablesManager::resolve_host("140.82.112.0/not-a-prefix").is_empty()); + } + + #[test] + fn build_egress_rule_args_routes_ipv4_to_iptables_args() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + action: RuleAction::Allow, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-j", + "ACCEPT", + ])] + ); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_egress_rule_args_routes_ipv6_to_ip6tables_args() { + let rule = EgressRule { + destinations: vec!["2606:50c0:8000::64".to_string()], + action: RuleAction::Deny, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert!(args.ipv4.is_empty()); + assert_eq!( + args.ipv6, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "2606:50c0:8000::64", + "-j", + "DROP", + ])] + ); + } + + #[test] + fn build_egress_rule_args_passes_cidr_through() { + let rule = EgressRule { + destinations: vec!["140.82.112.0/20".to_string(), "2606:50c0::/32".to_string()], + action: RuleAction::Allow, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.0/20", + "-j", + "ACCEPT", + ])] + ); + assert_eq!( + args.ipv6, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "2606:50c0::/32", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_adds_protocol_and_dport() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![443], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "443", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_cross_products_multi_port_multi_proto() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![80, 443], + protocols: vec![Protocol::Tcp, Protocol::Udp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![ + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "80", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "443", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "udp", + "--dport", + "80", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "udp", + "--dport", + "443", + "-j", + "ACCEPT", + ]), + ] + ); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_policy_rule_args_includes_legacy_and_egress_rules() { + let policy = ContainerPolicy { + allowed_hosts: vec!["10.0.0.1".to_string()], + blocked_hosts: vec!["2606:50c0::/32".to_string()], + egress_rules: vec![EgressRule { + destinations: vec!["192.0.2.0/24".to_string()], + action: RuleAction::Deny, + ..Default::default() + }], + ..Default::default() + }; + + let args = NetworkIptablesManager::build_policy_rule_args("MXC-test", &policy); + + assert_eq!( + args.ipv4, + vec![ + strings(&["-A", "MXC-test", "-d", "10.0.0.1", "-j", "ACCEPT"]), + strings(&["-A", "MXC-test", "-d", "192.0.2.0/24", "-j", "DROP"]), + ] + ); + assert_eq!( + args.ipv6, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "2606:50c0::/32", + "-j", + "DROP", + ])] + ); } } diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index f6e403a5f..29023ea4d 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -375,6 +375,44 @@ impl From for NetworkEnforcementMode { } } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum Protocol { + Tcp, + Udp, + Icmp, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum RuleAction { + Allow, + Deny, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(default)] +pub struct EgressRule { + /// IP or CIDR destinations, split by backend into IPv4 and IPv6 rules. + pub destinations: Vec, + /// Destination ports. Empty means all ports. + pub ports: Vec, + /// IP protocols. Empty means all protocols. + pub protocols: Vec, + pub action: RuleAction, +} + +impl Default for EgressRule { + fn default() -> Self { + Self { + destinations: Vec::new(), + ports: Vec::new(), + protocols: Vec::new(), + action: RuleAction::Allow, + } + } +} + #[derive(Debug, Clone)] pub struct ProxyAddress { pub address: String, @@ -547,6 +585,7 @@ pub struct ContainerPolicy { pub allow_local_network: bool, pub allowed_hosts: Vec, pub blocked_hosts: Vec, + pub egress_rules: Vec, #[serde(skip)] pub network_proxy: ProxyConfig, /// Cross-platform UI policy. From 0b8560b3ba630cd0dc36b2173a1a7585e4d4c4bb Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Mon, 13 Jul 2026 14:21:08 -0700 Subject: [PATCH 2/7] Address Copilot review feedback on network_iptables (PR #631) Resolves the five Copilot reviewer comments: - resolve_host doc comment now accurately describes CIDR validation (address parses + prefix in range; host bits are not required to be zero since iptables/ip6tables apply the mask). - protocol_arg is address-family aware: IPv6 ICMP rules use `ipv6-icmp` (ip6tables rejects `icmp`). - ICMP rules never emit `--dport`; the port dimension is collapsed for portless protocols so no invalid or duplicate rules are generated. - FORWARD hook and its cleanup now match container egress by input interface (`-i `) instead of `-o `, and the delete matches the insert. Adds unit tests for IPv4/IPv6 ICMP protocol naming and ICMP dport suppression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../lxc/common/src/network_iptables.rs | 151 ++++++++++++++++-- 1 file changed, 136 insertions(+), 15 deletions(-) diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index c832a05eb..2efc51378 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -118,10 +118,12 @@ impl NetworkIptablesManager { /// Resolve a destination string to IPv4 and IPv6 firewall destinations. /// /// Bare IPv4/IPv6 literals are retained in their matching family. CIDR - /// strings are accepted after validating the network address and prefix - /// length, then passed through unchanged. Hostnames are resolved to both A - /// and AAAA records so IPv4 destinations route to `iptables` and IPv6 - /// destinations route to `ip6tables`. + /// strings are accepted after validating that the address parses and the + /// prefix length is within range for its family; the host bits are not + /// required to be zero, since `iptables`/`ip6tables` apply the prefix mask + /// themselves. Validated CIDRs are passed through unchanged. Hostnames are + /// resolved to both A and AAAA records so IPv4 destinations route to + /// `iptables` and IPv6 destinations route to `ip6tables`. fn resolve_host(host: &str) -> ResolvedDestinations { if host.contains('/') { return match Self::destination_family(host) { @@ -185,14 +187,27 @@ impl NetworkIptablesManager { } } - fn protocol_arg(protocol: &Protocol) -> &'static str { + /// The iptables/ip6tables `-p` protocol name for a rule in the given + /// address family. ICMP is family-specific: IPv4 uses `icmp` while IPv6 + /// uses the `ipv6-icmp` name that ip6tables expects (ip6tables rejects + /// `icmp`). + fn protocol_arg(protocol: &Protocol, family: IpFamily) -> &'static str { match protocol { Protocol::Tcp => "tcp", Protocol::Udp => "udp", - Protocol::Icmp => "icmp", + Protocol::Icmp => match family { + IpFamily::V4 => "icmp", + IpFamily::V6 => "ipv6-icmp", + }, } } + /// Whether a protocol carries transport-layer ports and therefore supports + /// `--dport`. ICMP/ICMPv6 have no ports. + fn protocol_supports_ports(protocol: &Protocol) -> bool { + matches!(protocol, Protocol::Tcp | Protocol::Udp) + } + fn rule_action_arg(action: &RuleAction) -> &'static str { match action { RuleAction::Allow => "ACCEPT", @@ -249,6 +264,7 @@ impl NetworkIptablesManager { action, None, None, + IpFamily::V4, )); } for destination in &destinations.ipv6 { @@ -258,6 +274,7 @@ impl NetworkIptablesManager { action, None, None, + IpFamily::V6, )); } args @@ -287,15 +304,25 @@ impl NetworkIptablesManager { ports.iter().copied().map(Some).collect() }; + // ICMP/ICMPv6 carry no ports, so collapse the port dimension for those + // protocols. This avoids emitting an invalid `--dport` on an ICMP rule + // (which iptables/ip6tables reject) and prevents duplicating the same + // portless rule once per configured port. + let portless = [None]; let mut args = FirewallRuleArgs::default(); for protocol in &protocol_options { - for port in &port_options { + let ports_for_protocol: &[Option] = match protocol.as_ref() { + Some(p) if !Self::protocol_supports_ports(p) => &portless, + _ => &port_options, + }; + for port in ports_for_protocol { let rule = Self::build_single_rule_args( chain_name, destination, action, protocol.as_ref(), *port, + family, ); match family { IpFamily::V4 => args.ipv4.push(rule), @@ -312,6 +339,7 @@ impl NetworkIptablesManager { action: &RuleAction, protocol: Option<&Protocol>, port: Option, + family: IpFamily, ) -> Vec { let mut args = vec![ "-A".to_string(), @@ -321,9 +349,13 @@ impl NetworkIptablesManager { ]; if let Some(protocol) = protocol { args.push("-p".to_string()); - args.push(Self::protocol_arg(protocol).to_string()); + args.push(Self::protocol_arg(protocol, family).to_string()); } - if let Some(port) = port { + // `--dport` is only valid for port-bearing protocols (TCP/UDP); never + // emit it for ICMP/ICMPv6 (or when no protocol is set), where it would + // make iptables/ip6tables reject the rule. + let port_supported = protocol.is_some_and(Self::protocol_supports_ports); + if let Some(port) = port.filter(|_| port_supported) { args.push("--dport".to_string()); args.push(port.to_string()); } @@ -477,14 +509,17 @@ impl NetworkIptablesManager { Self::run_iptables(&default_args, logger)?; Self::run_ip6tables(&default_args, logger)?; - // Hook the chains into FORWARD for the container's traffic. + // 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. if let Some(ref iface) = self.veth_interface { Self::run_iptables( - &["-I", "FORWARD", "-o", iface, "-j", &self.chain_name], + &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], logger, )?; Self::run_ip6tables( - &["-I", "FORWARD", "-o", iface, "-j", &self.chain_name], + &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], logger, )?; } else { @@ -511,14 +546,16 @@ impl NetworkIptablesManager { self.chain_name )); - // Remove from FORWARD (only if we had a veth interface and hooked it) + // Remove from FORWARD (only if we had a veth interface and hooked it). + // Must match the `-i` direction used at insertion so the delete finds + // the rule; a `-o` delete would leak the FORWARD hook. if let Some(ref iface) = self.veth_interface { let _ = Self::run_iptables( - &["-D", "FORWARD", "-o", iface, "-j", &self.chain_name], + &["-D", "FORWARD", "-i", iface, "-j", &self.chain_name], logger, ); let _ = Self::run_ip6tables( - &["-D", "FORWARD", "-o", iface, "-j", &self.chain_name], + &["-D", "FORWARD", "-i", iface, "-j", &self.chain_name], logger, ); } @@ -841,4 +878,88 @@ mod tests { ])] ); } + + #[test] + fn build_egress_rule_args_uses_ipv4_icmp_protocol_name() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + protocols: vec![Protocol::Icmp], + action: RuleAction::Allow, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert!(args.ipv6.is_empty()); + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "icmp", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_uses_ipv6_icmp_protocol_name() { + // ip6tables requires the `ipv6-icmp` protocol name; `icmp` is rejected. + let rule = EgressRule { + destinations: vec!["2606:50c0::1".to_string()], + protocols: vec![Protocol::Icmp], + action: RuleAction::Allow, + ..Default::default() + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert!(args.ipv4.is_empty()); + assert_eq!( + args.ipv6, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "2606:50c0::1", + "-p", + "ipv6-icmp", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_omits_dport_for_icmp_even_with_ports() { + // ICMP has no transport ports: a configured port list must not emit + // `--dport` and must not fan out into one rule per port. + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![80, 443], + protocols: vec![Protocol::Icmp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "icmp", + "-j", + "ACCEPT", + ])] + ); + assert!(args.ipv6.is_empty()); + } } From 96af8f958ce2755923bc304e37cfdfa3a8ad900f Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Thu, 16 Jul 2026 11:22:29 -0700 Subject: [PATCH 3/7] Address net-model-1 review feedback: ip6tables probe, apply rollback, fail-closed egress default - network_iptables: probe ip6tables once and skip the parallel v6 chain (with a warning) when the binary is absent or IPv6 is disabled, so IPv4-only hosts no longer fail the whole firewall setup. - network_iptables: roll back partially-created chains/FORWARD hooks on any apply error so a retry does not hit "chain already exists" and leak state; share the teardown path with remove_firewall_rules. - models: EgressRule default action is now Deny (fail-closed) so an under-specified egress rule cannot silently widen access. - network_iptables: document the interim allow-before-deny ordering (deny-precedence reconciliation tracked by AB#62830341). - tests: add lxc + bubblewrap network configs exercising IPv6 and CIDR host filtering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../lxc/common/src/network_iptables.rs | 144 +++++++++++++++--- src/core/wxc_common/src/models.rs | 25 ++- .../configs/bubblewrap_network_ipv6_cidr.json | 22 +++ tests/configs/lxc_network_ipv6_cidr.json | 29 ++++ 4 files changed, 198 insertions(+), 22 deletions(-) create mode 100644 tests/configs/bubblewrap_network_ipv6_cidr.json create mode 100644 tests/configs/lxc_network_ipv6_cidr.json diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 2efc51378..f49bd5f35 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -387,6 +387,17 @@ impl NetworkIptablesManager { args } + /// Build the allow/deny rule args for a container policy. + /// + /// NOTE — interim ordering (tracked by AB#62830341): rules are emitted in + /// allow-list, then block-list, then `egress_rules` (author) 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, + /// and `egress_rules` carry no deny priority. Reconciling this to the + /// GA "deny-wins" ordering across the combined allow/deny model is owned + /// by net-model-2 (AB#62830341); until then callers must not assume + /// deny-precedence. fn build_policy_rule_args(chain_name: &str, policy: &ContainerPolicy) -> FirewallRuleArgs { let mut args = FirewallRuleArgs::default(); for host in &policy.allowed_hosts { @@ -419,6 +430,36 @@ impl NetworkIptablesManager { Self::run_firewall_command("ip6tables", args, logger) } + /// Probe whether `ip6tables` can be used on this host. + /// + /// Runs a harmless, read-only `ip6tables -S` (list the filter table). + /// This fails both when the binary is missing (IPv4-only images) and when + /// the kernel has IPv6 disabled (`ip6tables` reports the table cannot be + /// initialized). In either case the caller skips the parallel v6 chain and + /// warns, instead of aborting an otherwise-valid IPv4 policy — a hard + /// dependency on ip6tables would break pure-IPv4 hosts that worked before + /// dual-stack support was added. + fn ip6tables_available(logger: &mut Logger) -> bool { + match Command::new("ip6tables").arg("-S").output() { + Ok(output) if output.status.success() => true, + Ok(output) => { + let stderr = String::from_utf8_lossy(&output.stderr); + logger.log_line(&format!( + "ip6tables unavailable ({}); skipping IPv6 firewall rules.", + stderr.trim() + )); + false + } + Err(e) => { + logger.log_line(&format!( + "ip6tables not found ({}); skipping IPv6 firewall rules.", + e + )); + false + } + } + } + fn run_firewall_command( command: &str, args: &[&str], @@ -456,6 +497,11 @@ impl NetworkIptablesManager { } /// Apply network firewall rules based on the container policy. + /// + /// On any failure after the per-container chains are created, the partially + /// applied state is torn down before the error is returned, so a retry does + /// not trip over a leftover `MXC-` chain ("chain already exists") and + /// leak rules permanently. pub fn apply_firewall_rules( &mut self, policy: &ContainerPolicy, @@ -471,18 +517,55 @@ impl NetworkIptablesManager { return Ok(true); } + match self.apply_firewall_rules_inner(policy, logger) { + Ok(()) => { + self.rules_applied = true; + Ok(true) + } + Err(e) => { + // Roll back whatever was created before the failure. Without + // this, `remove_firewall_rules` short-circuits on + // `rules_applied == false` and the orphan chain(s) survive, so + // the next attempt fails permanently on `-N` ("chain already + // exists") until someone cleans up by hand. + logger.log_line(&format!( + "Firewall setup failed: {}. Cleaning up partial iptables state.", + e + )); + self.teardown_chains(logger); + Err(e) + } + } + } + + /// Fallible body of [`Self::apply_firewall_rules`]. Kept separate so the + /// public method can roll back partial state on the error path. + fn apply_firewall_rules_inner( + &self, + policy: &ContainerPolicy, + logger: &mut Logger, + ) -> Result<(), String> { logger.log_line(&format!( "Creating iptables/ip6tables chain: {}", self.chain_name )); + // Probe ip6tables once. On IPv4-only hosts (binary absent or IPv6 + // disabled in the kernel) enforce the v4 policy and skip the v6 chain + // rather than failing setup for a policy that worked before dual-stack. + let ipv6_enabled = Self::ip6tables_available(logger); + // Create custom chains. Self::run_iptables(&["-N", &self.chain_name], logger)?; - Self::run_ip6tables(&["-N", &self.chain_name], logger)?; + if ipv6_enabled { + Self::run_ip6tables(&["-N", &self.chain_name], logger)?; + } let base_rules = Self::build_base_chain_rule_args(&self.chain_name); Self::run_iptables_rule_args(&base_rules, logger)?; - Self::run_ip6tables_rule_args(&base_rules, logger)?; + if ipv6_enabled { + Self::run_ip6tables_rule_args(&base_rules, logger)?; + } for host in policy .allowed_hosts @@ -496,7 +579,15 @@ impl NetworkIptablesManager { let policy_rules = Self::build_policy_rule_args(&self.chain_name, policy); Self::run_iptables_rule_args(&policy_rules.ipv4, logger)?; - Self::run_ip6tables_rule_args(&policy_rules.ipv6, logger)?; + if ipv6_enabled { + Self::run_ip6tables_rule_args(&policy_rules.ipv6, logger)?; + } else if !policy_rules.ipv6.is_empty() { + logger.log_line(&format!( + "Warning: {} IPv6 firewall rule(s) not applied because ip6tables \ + is unavailable; IPv6 egress is unfiltered on this host.", + policy_rules.ipv6.len() + )); + } // Append default policy at end of each chain. let default_rule = Self::build_default_policy_rule_arg( @@ -507,7 +598,9 @@ impl NetworkIptablesManager { let default_action = default_args.last().copied().unwrap_or("ACCEPT"); logger.log_line(&format!("Default network policy: {}", default_action)); Self::run_iptables(&default_args, logger)?; - Self::run_ip6tables(&default_args, logger)?; + if ipv6_enabled { + Self::run_ip6tables(&default_args, logger)?; + } // Hook the chains into FORWARD for the container's egress traffic. // Packets originating in the container arrive at the host on the @@ -518,10 +611,12 @@ impl NetworkIptablesManager { &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], logger, )?; - Self::run_ip6tables( - &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], - logger, - )?; + if ipv6_enabled { + Self::run_ip6tables( + &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], + logger, + )?; + } } 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. @@ -531,21 +626,14 @@ impl NetworkIptablesManager { ); } - self.rules_applied = true; - Ok(true) + Ok(()) } - /// Remove all iptables/ip6tables rules created by this manager. - pub fn remove_firewall_rules(&mut self, logger: &mut Logger) -> Result<(), String> { - if !self.rules_applied { - return Ok(()); - } - - logger.log_line(&format!( - "Removing iptables/ip6tables chain: {}", - self.chain_name - )); - + /// Best-effort removal of the FORWARD hooks and per-container chains in + /// both tables. Safe to call even when only part of the state was created + /// (a missing rule/chain just makes the individual `-D`/`-F`/`-X` call + /// no-op), so it doubles as the rollback path for a failed apply. + fn teardown_chains(&self, logger: &mut Logger) { // Remove from FORWARD (only if we had a veth interface and hooked it). // Must match the `-i` direction used at insertion so the delete finds // the rule; a `-o` delete would leak the FORWARD hook. @@ -565,6 +653,20 @@ impl NetworkIptablesManager { let _ = Self::run_iptables(&["-X", &self.chain_name], logger); let _ = Self::run_ip6tables(&["-F", &self.chain_name], logger); let _ = Self::run_ip6tables(&["-X", &self.chain_name], logger); + } + + /// Remove all iptables/ip6tables rules created by this manager. + pub fn remove_firewall_rules(&mut self, logger: &mut Logger) -> Result<(), String> { + if !self.rules_applied { + return Ok(()); + } + + logger.log_line(&format!( + "Removing iptables/ip6tables chain: {}", + self.chain_name + )); + + self.teardown_chains(logger); self.rules_applied = false; Ok(()) diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index dc568548b..401e29978 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -401,16 +401,24 @@ pub struct EgressRule { pub ports: Vec, /// IP protocols. Empty means all protocols. pub protocols: Vec, + /// Whether matching traffic is allowed or denied. A rule deserialized with + /// a missing `action` defaults to [`RuleAction::Deny`] (fail-closed), so an + /// under-specified egress rule cannot silently widen access. pub action: RuleAction, } impl Default for EgressRule { fn default() -> Self { + // Fail closed: an egress rule with an unspecified action denies rather + // than allows. `EgressRule` is `#[serde(default)]`, so a rule + // deserialized without an `action` inherits this default; defaulting to + // Allow would silently turn an under-specified security policy into an + // ACCEPT, which is the wrong direction for a containment boundary. Self { destinations: Vec::new(), ports: Vec::new(), protocols: Vec::new(), - action: RuleAction::Allow, + action: RuleAction::Deny, } } } @@ -911,4 +919,19 @@ mod tests { ); assert!(parsed.user.is_some()); } + + #[test] + fn egress_rule_default_action_is_deny() { + // Security invariant: an unspecified action must fail closed. + assert_eq!(EgressRule::default().action, RuleAction::Deny); + } + + #[test] + fn egress_rule_missing_action_deserializes_to_deny() { + // `#[serde(default)]` fills a missing `action` from `EgressRule::default`, + // so an under-specified rule denies rather than silently allowing. + let rule: EgressRule = + serde_json::from_value(json!({ "destinations": ["10.0.0.1"] })).unwrap(); + assert_eq!(rule.action, RuleAction::Deny); + } } diff --git a/tests/configs/bubblewrap_network_ipv6_cidr.json b/tests/configs/bubblewrap_network_ipv6_cidr.json new file mode 100644 index 000000000..1595b082a --- /dev/null +++ b/tests/configs/bubblewrap_network_ipv6_cidr.json @@ -0,0 +1,22 @@ +{ + "version": "0.6.0-alpha", + "containerId": "CLI-Bubblewrap-Network-IPv6-CIDR", + "containment": "bubblewrap", + "process": { + "commandLine": "wget -qO- https://api.github.com/zen" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": [ + "140.82.112.0/20", + "2606:50c0::/32", + "2606:50c0:8000::153" + ], + "blockedHosts": [ + "10.0.0.0/8", + "2001:db8::/32", + "fe80::1" + ] + } +} diff --git a/tests/configs/lxc_network_ipv6_cidr.json b/tests/configs/lxc_network_ipv6_cidr.json new file mode 100644 index 000000000..b8ca0bad2 --- /dev/null +++ b/tests/configs/lxc_network_ipv6_cidr.json @@ -0,0 +1,29 @@ +{ + "version": "0.4.0-alpha", + "containerId": "CLI-LXC-Network-IPv6-CIDR", + "containment": "lxc", + "process": { + "commandLine": "wget -qO- https://api.github.com/zen" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": [ + "140.82.112.0/20", + "2606:50c0::/32", + "2606:50c0:8000::153" + ], + "blockedHosts": [ + "10.0.0.0/8", + "2001:db8::/32", + "fe80::1" + ] + } +} From df26d9ab33568e77296f297a34c2f810ee8085b6 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Mon, 20 Jul 2026 11:17:30 -0700 Subject: [PATCH 4/7] ci: re-trigger flaky linux SDK integration proxy test The 'SDK Integration Tests (linux)' job failed on the pre-existing proxy test 'should enforce allowedHosts at the proxy layer' because the allowed-host sentinel 'curl https://api.github.com/zen' hit a GitHub API 403 rate-limit on the shared runner IP (two earlier tests in the same job fetched the same URL successfully). This is environmental and unrelated to this PR, which only touches the iptables/ip6tables enforcement path (models.rs is additive-only; the proxy allowlist path is untouched). Empty commit to re-run CI on a fresh runner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> From 1c794d79a8aa1d83e5f5a2036c7529b22cc3b01d Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Fri, 31 Jul 2026 10:55:30 -0700 Subject: [PATCH 5/7] test(lxc): run the IPv6/CIDR network config in the LXC suite tests/configs/lxc_network_ipv6_cidr.json was added in 96af8f9 but nothing ever executed it. Add run_lxc_network_ipv6_cidr_test.sh and register it in run_lxc_all_tests.sh so the IPv6-literal, IPv6-CIDR and IPv4-CIDR entries are actually exercised. The script asserts on lxc-exec output that no host or CIDR entry failed to resolve, that no iptables/ip6tables rule was rejected (which is how a v4/v6 address-family routing mistake surfaces, since a rejected rule aborts firewall setup), that the default DROP policy was applied, and that the IPv6 rules were not silently skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd --- tests/scripts/run_lxc_all_tests.sh | 1 + .../scripts/run_lxc_network_ipv6_cidr_test.sh | 65 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 tests/scripts/run_lxc_network_ipv6_cidr_test.sh diff --git a/tests/scripts/run_lxc_all_tests.sh b/tests/scripts/run_lxc_all_tests.sh index 4121fb574..e6e739206 100644 --- a/tests/scripts/run_lxc_all_tests.sh +++ b/tests/scripts/run_lxc_all_tests.sh @@ -46,6 +46,7 @@ run_test "LXC Object Validation" "$SCRIPT_DIR/run_lxc_object_test.sh" run_test "LXC Most-Specific Path" "$SCRIPT_DIR/run_lxc_most_specific_test.sh" run_test "LXC Denied Masking" "$SCRIPT_DIR/run_lxc_denied_masking_test.sh" run_test "LXC Network" "$SCRIPT_DIR/run_lxc_network_test.sh" +run_test "LXC Network IPv6+CIDR" "$SCRIPT_DIR/run_lxc_network_ipv6_cidr_test.sh" run_test "LXC Timeout" "$SCRIPT_DIR/run_lxc_timeout_test.sh" run_test "LXC Env+Cwd" "$SCRIPT_DIR/run_lxc_env_cwd_test.sh" diff --git a/tests/scripts/run_lxc_network_ipv6_cidr_test.sh b/tests/scripts/run_lxc_network_ipv6_cidr_test.sh new file mode 100644 index 000000000..306afc5df --- /dev/null +++ b/tests/scripts/run_lxc_network_ipv6_cidr_test.sh @@ -0,0 +1,65 @@ +#!/bin/bash +# LXC IPv6 + CIDR network filtering test +# +# Exercises tests/configs/lxc_network_ipv6_cidr.json, whose allow/block lists +# carry IPv4 CIDRs, IPv6 CIDRs, and IPv6 literals. The assertions are on the +# firewall setup rather than on whether the container reaches the network: +# reachability depends on the host's uplink, but rule programming does not. +# +# A misrouted address family is a hard failure, not a silent one -- +# `run_firewall_command` returns Err when iptables/ip6tables rejects a rule, so +# handing an IPv6 CIDR to iptables (or a v4 CIDR to ip6tables) aborts setup and +# is caught here. +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 + +if [ ! -f "$LXC_EXEC" ]; then + echo "Error: lxc-exec not found. Run build.sh first." + exit 1 +fi + +CONFIG="$REPO_DIR/tests/configs/lxc_network_ipv6_cidr.json" + +echo "Running LXC IPv6/CIDR network filtering test..." + +# The container command may fail on a host with no outbound route; the firewall +# assertions below are what this test is about. +OUTPUT=$("$LXC_EXEC" "$CONFIG" 2>&1 || true) +echo "$OUTPUT" + +fail() { + echo "FAIL: $1" + exit 1 +} + +# Every allow/block entry must survive resolution. An unparsed CIDR or IPv6 +# literal is reported here instead of silently dropping a rule. +if echo "$OUTPUT" | grep -q "could not resolve host"; then + echo "$OUTPUT" | grep "could not resolve host" + fail "an IPv6 literal or CIDR entry was not resolved." +fi + +# A rejected rule aborts setup. +if echo "$OUTPUT" | grep -qE "^(ip6?tables) .* failed:|Firewall setup failed:"; then + fail "iptables/ip6tables rejected a rule." +fi + +if ! echo "$OUTPUT" | grep -q "Default network policy: DROP"; then + fail "default-deny policy was not applied." +fi + +# The v6 half is the point of the test: if ip6tables is unusable the v6 rules +# are skipped with a warning, which would make this a v4-only run. +if echo "$OUTPUT" | grep -q "IPv6 firewall rule(s) not applied"; then + fail "IPv6 rules were skipped; ip6tables is unusable on this host." +fi + +echo "PASS: IPv6 and CIDR entries were resolved and programmed." +echo "LXC IPv6/CIDR network filtering test complete." From a00c75c6a1577537a733a37cb1db21f77dbc225c Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Fri, 31 Jul 2026 12:48:47 -0700 Subject: [PATCH 6/7] Add egress port ranges, invalid-CIDR coverage, and correct LXC network docs Completes the remaining asks on the LXC network-policy work item. Port ranges: EgressRule.ports becomes Vec, an untagged enum accepting a bare port number or an inclusive { start, end } object. The LXC backend emits --dport start:end, normalizes start == end to a single port, and skips selectors with start > end after logging a warning, since passing them to iptables would fail the whole apply. ICMP port collapsing is unchanged. Tests: an integration test asserting invalid CIDR entries are warned and skipped without failing firewall setup, and the IPv6/CIDR test now checks each configured host individually and stays in sync with its config. Docs: lxc-backend.md claimed firewall mode was IPv4-only and that cleanup depended on a removeRulesOnExit field. Neither is true. It now describes dual-stack chains, CIDR and hostname handling, the ip6tables-unavailable and missing-veth fallbacks, and automatic teardown. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd --- docs/lxc-support/lxc-backend.md | 18 +- .../lxc/common/src/network_iptables.rs | 318 +++++++++++++++++- src/core/wxc_common/src/models.rs | 124 ++++++- tests/configs/lxc_network_invalid_cidr.json | 25 ++ tests/scripts/run_lxc_all_tests.sh | 1 + .../run_lxc_network_invalid_cidr_test.sh | 57 ++++ .../scripts/run_lxc_network_ipv6_cidr_test.sh | 57 +++- 7 files changed, 572 insertions(+), 28 deletions(-) create mode 100644 tests/configs/lxc_network_invalid_cidr.json create mode 100644 tests/scripts/run_lxc_network_invalid_cidr_test.sh diff --git a/docs/lxc-support/lxc-backend.md b/docs/lxc-support/lxc-backend.md index b94cd2861..497d8e9ee 100644 --- a/docs/lxc-support/lxc-backend.md +++ b/docs/lxc-support/lxc-backend.md @@ -109,18 +109,22 @@ Filesystem policies are enforced via bind mounts in the container configuration: ## Network Policy -Network policies are enforced via iptables/nftables rules applied to the container's virtual ethernet (veth) interface: +Network policies are enforced with parallel `iptables` and `ip6tables` chains scoped to the container's virtual ethernet (veth) interface: | Policy | Implementation | |--------|---------------| -| `defaultPolicy: "block"` | Default DROP rule on container veth | -| `defaultPolicy: "allow"` | Default ACCEPT rule on container veth | -| `allowedHosts` | ACCEPT rules for specific IPs/CIDRs | -| `blockedHosts` | DROP rules for specific IPs/CIDRs | +| `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 | -Rules are automatically cleaned up when the container exits (if `removeRulesOnExit` is `true`). +`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. These host-list rules match all ports and protocols. The current config file schema does not expose port- or protocol-specific egress rules. -**IPv4 only.** Firewall mode resolves `allowedHosts` / `blockedHosts` to IPv4 addresses only; AAAA (IPv6) records and IPv6 literals are silently dropped. A host that has only AAAA records is effectively unreachable from the sandbox under firewall mode. +If `ip6tables` is unavailable or IPv6 is disabled in the host kernel, MXC applies the IPv4 chain, skips IPv6 rules, and logs a warning with the number of unapplied IPv6 rules. On such hosts, IPv6 egress is 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. + +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. ## Usage diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index f49bd5f35..9cc4f4810 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -12,7 +12,8 @@ use std::process::Command; use wxc_common::logger::Logger; use wxc_common::models::{ - ContainerPolicy, EgressRule, NetworkEnforcementMode, NetworkPolicy, Protocol, RuleAction, + ContainerPolicy, EgressRule, NetworkEnforcementMode, NetworkPolicy, PortSpec, Protocol, + RuleAction, }; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -285,7 +286,7 @@ impl NetworkIptablesManager { destination: &str, action: &RuleAction, protocols: &[Protocol], - ports: &[u16], + ports: &[PortSpec], ) -> FirewallRuleArgs { let Some(family) = Self::destination_family(destination) else { return FirewallRuleArgs::default(); @@ -298,10 +299,17 @@ impl NetworkIptablesManager { } else { protocols.iter().cloned().map(Some).collect() }; - let port_options: Vec> = if ports.is_empty() { + let port_options: Vec> = if ports.is_empty() { vec![None] } else { - ports.iter().copied().map(Some).collect() + ports + .iter() + .filter(|port| port.is_valid()) + .map(Some) + .collect() + }; + if port_options.is_empty() { + return FirewallRuleArgs::default(); }; // ICMP/ICMPv6 carry no ports, so collapse the port dimension for those @@ -311,7 +319,7 @@ impl NetworkIptablesManager { let portless = [None]; let mut args = FirewallRuleArgs::default(); for protocol in &protocol_options { - let ports_for_protocol: &[Option] = match protocol.as_ref() { + let ports_for_protocol: &[Option<&PortSpec>] = match protocol.as_ref() { Some(p) if !Self::protocol_supports_ports(p) => &portless, _ => &port_options, }; @@ -338,7 +346,7 @@ impl NetworkIptablesManager { destination: &str, action: &RuleAction, protocol: Option<&Protocol>, - port: Option, + port: Option<&PortSpec>, family: IpFamily, ) -> Vec { let mut args = vec![ @@ -356,8 +364,10 @@ impl NetworkIptablesManager { // make iptables/ip6tables reject the rule. let port_supported = protocol.is_some_and(Self::protocol_supports_ports); if let Some(port) = port.filter(|_| port_supported) { - args.push("--dport".to_string()); - args.push(port.to_string()); + if let Some(dport) = port.iptables_dport_arg() { + args.push("--dport".to_string()); + args.push(dport); + } } args.push("-j".to_string()); args.push(Self::rule_action_arg(action).to_string()); @@ -387,6 +397,21 @@ impl NetworkIptablesManager { args } + /// Invalid port ranges are skipped instead of being passed to iptables, + /// which would reject the command and roll back the whole firewall apply. + fn log_invalid_port_selectors(policy: &ContainerPolicy, logger: &mut Logger) { + for rule in &policy.egress_rules { + for port in rule.ports.iter().filter(|port| !port.is_valid()) { + if let PortSpec::Range { start, end } = port { + logger.log_line(&format!( + "Warning: skipping invalid egress port range '{}:{}' (start > end)", + start, end + )); + } + } + } + } + /// Build the allow/deny rule args for a container policy. /// /// NOTE — interim ordering (tracked by AB#62830341): rules are emitted in @@ -577,6 +602,8 @@ impl NetworkIptablesManager { } } + Self::log_invalid_port_selectors(policy, logger); + let policy_rules = Self::build_policy_rule_args(&self.chain_name, policy); Self::run_iptables_rule_args(&policy_rules.ipv4, logger)?; if ipv6_enabled { @@ -708,6 +735,14 @@ mod tests { args.iter().map(|arg| arg.to_string()).collect() } + fn single(port: u16) -> PortSpec { + PortSpec::Single(port) + } + + fn range(start: u16, end: u16) -> PortSpec { + PortSpec::Range { start, end } + } + #[test] fn chain_name_sanitization() { let mgr = NetworkIptablesManager::new("my-container_123"); @@ -855,7 +890,7 @@ mod tests { fn build_egress_rule_args_adds_protocol_and_dport() { let rule = EgressRule { destinations: vec!["140.82.112.4".to_string()], - ports: vec![443], + ports: vec![single(443)], protocols: vec![Protocol::Tcp], action: RuleAction::Allow, }; @@ -883,7 +918,7 @@ mod tests { fn build_egress_rule_args_cross_products_multi_port_multi_proto() { let rule = EgressRule { destinations: vec!["140.82.112.4".to_string()], - ports: vec![80, 443], + ports: vec![single(80), single(443)], protocols: vec![Protocol::Tcp, Protocol::Udp], action: RuleAction::Allow, }; @@ -1042,7 +1077,244 @@ mod tests { // `--dport` and must not fan out into one rule per port. let rule = EgressRule { destinations: vec!["140.82.112.4".to_string()], - ports: vec![80, 443], + ports: vec![single(80), single(443)], + protocols: vec![Protocol::Icmp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "icmp", + "-j", + "ACCEPT", + ])] + ); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_egress_rule_args_emits_port_range_for_ipv4_tcp() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![range(8000, 8999)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "8000:8999", + "-j", + "ACCEPT", + ])] + ); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_egress_rule_args_emits_port_range_for_ipv6_tcp() { + let rule = EgressRule { + destinations: vec!["2606:50c0::1".to_string()], + ports: vec![range(8000, 8999)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert!(args.ipv4.is_empty()); + assert_eq!( + args.ipv6, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "2606:50c0::1", + "-p", + "tcp", + "--dport", + "8000:8999", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_normalizes_single_value_range() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![range(443, 443)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "443", + "-j", + "ACCEPT", + ])] + ); + } + + #[test] + fn build_egress_rule_args_skips_invalid_port_range() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![range(900, 100)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert!(args.ipv4.is_empty()); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_egress_rule_args_mixes_single_port_and_range() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![single(443), range(8000, 8999)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![ + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "443", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "8000:8999", + "-j", + "ACCEPT", + ]), + ] + ); + } + + #[test] + fn build_egress_rule_args_cross_products_single_range_and_protocols() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![single(443), range(8000, 8999)], + protocols: vec![Protocol::Tcp, Protocol::Udp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + + assert_eq!( + args.ipv4, + vec![ + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "443", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "tcp", + "--dport", + "8000:8999", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "udp", + "--dport", + "443", + "-j", + "ACCEPT", + ]), + strings(&[ + "-A", + "MXC-test", + "-d", + "140.82.112.4", + "-p", + "udp", + "--dport", + "8000:8999", + "-j", + "ACCEPT", + ]), + ] + ); + assert!(args.ipv6.is_empty()); + } + + #[test] + fn build_egress_rule_args_omits_dport_for_icmp_with_range() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![range(8000, 8999)], protocols: vec![Protocol::Icmp], action: RuleAction::Allow, }; @@ -1064,4 +1336,28 @@ mod tests { ); assert!(args.ipv6.is_empty()); } + + #[test] + fn build_egress_rule_args_handles_port_boundaries() { + let rule = EgressRule { + destinations: vec!["140.82.112.4".to_string()], + ports: vec![single(0), single(65535), range(0, 65535), range(1, 65535)], + protocols: vec![Protocol::Tcp], + action: RuleAction::Allow, + }; + + let args = NetworkIptablesManager::build_egress_rule_args("MXC-test", &rule); + let dports: Vec<&str> = args + .ipv4 + .iter() + .filter_map(|rule| { + rule.iter() + .position(|arg| arg == "--dport") + .map(|index| rule[index + 1].as_str()) + }) + .collect(); + + assert_eq!(dports, vec!["0", "65535", "0:65535", "1:65535"]); + assert!(args.ipv6.is_empty()); + } } diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index ab89fe772..011c7441e 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -393,13 +393,39 @@ pub enum RuleAction { Deny, } +/// A destination port selector: a single port or an inclusive range. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(untagged)] +pub enum PortSpec { + Single(u16), + Range { start: u16, end: u16 }, +} + +impl PortSpec { + pub fn is_valid(&self) -> bool { + match self { + Self::Single(_) => true, + Self::Range { start, end } => start <= end, + } + } + + pub fn iptables_dport_arg(&self) -> Option { + match self { + Self::Single(port) => Some(port.to_string()), + Self::Range { start, end } if start < end => Some(format!("{start}:{end}")), + Self::Range { start, end } if start == end => Some(start.to_string()), + Self::Range { .. } => None, + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(default)] pub struct EgressRule { /// IP or CIDR destinations, split by backend into IPv4 and IPv6 rules. pub destinations: Vec, /// Destination ports. Empty means all ports. - pub ports: Vec, + pub ports: Vec, /// IP protocols. Empty means all protocols. pub protocols: Vec, /// Whether matching traffic is allowed or denied. A rule deserialized with @@ -935,4 +961,100 @@ mod tests { serde_json::from_value(json!({ "destinations": ["10.0.0.1"] })).unwrap(); assert_eq!(rule.action, RuleAction::Deny); } + + #[test] + fn port_spec_deserializes_bare_number_as_single_port() { + let port: PortSpec = serde_json::from_value(json!(443)).unwrap(); + assert_eq!(port, PortSpec::Single(443)); + assert_eq!(serde_json::to_value(&port).unwrap(), json!(443)); + } + + #[test] + fn port_spec_deserializes_range_object() { + let port: PortSpec = serde_json::from_value(json!({ "start": 8000, "end": 8999 })).unwrap(); + assert_eq!( + port, + PortSpec::Range { + start: 8000, + end: 8999 + } + ); + assert_eq!( + serde_json::to_value(&port).unwrap(), + json!({ "start": 8000, "end": 8999 }) + ); + } + + #[test] + fn egress_rule_deserializes_port_specs_and_defaults_missing_action_to_deny() { + let rule: EgressRule = serde_json::from_value(json!({ + "destinations": ["10.0.0.1"], + "ports": [443, { "start": 8000, "end": 8999 }] + })) + .unwrap(); + + assert_eq!(rule.action, RuleAction::Deny); + assert_eq!( + rule.ports, + vec![ + PortSpec::Single(443), + PortSpec::Range { + start: 8000, + end: 8999 + } + ] + ); + } + + #[test] + fn port_spec_iptables_arg_normalizes_and_validates_ranges() { + assert_eq!( + PortSpec::Single(0).iptables_dport_arg().as_deref(), + Some("0") + ); + assert_eq!( + PortSpec::Single(65535).iptables_dport_arg().as_deref(), + Some("65535") + ); + assert_eq!( + PortSpec::Range { + start: 0, + end: 65535 + } + .iptables_dport_arg() + .as_deref(), + Some("0:65535") + ); + assert_eq!( + PortSpec::Range { + start: 1, + end: 65535 + } + .iptables_dport_arg() + .as_deref(), + Some("1:65535") + ); + assert_eq!( + PortSpec::Range { + start: 443, + end: 443 + } + .iptables_dport_arg() + .as_deref(), + Some("443") + ); + assert!(!PortSpec::Range { + start: 900, + end: 100 + } + .is_valid()); + assert_eq!( + PortSpec::Range { + start: 900, + end: 100 + } + .iptables_dport_arg(), + None + ); + } } diff --git a/tests/configs/lxc_network_invalid_cidr.json b/tests/configs/lxc_network_invalid_cidr.json new file mode 100644 index 000000000..59558f71a --- /dev/null +++ b/tests/configs/lxc_network_invalid_cidr.json @@ -0,0 +1,25 @@ +{ + "version": "0.4.0-alpha", + "containerId": "CLI-LXC-Network-Invalid-CIDR", + "containment": "lxc", + "process": { + "commandLine": "wget -qO- https://api.github.com/zen" + }, + "lifecycle": { + "destroyOnExit": true + }, + "lxc": { + "distribution": "alpine", + "release": "3.23" + }, + "network": { + "defaultPolicy": "block", + "enforcementMode": "firewall", + "allowedHosts": [ + "140.82.112.0/33", + "2606:50c0::/129", + "140.82.112.0/not-a-prefix" + ], + "blockedHosts": [] + } +} diff --git a/tests/scripts/run_lxc_all_tests.sh b/tests/scripts/run_lxc_all_tests.sh index e6e739206..9ba3ff839 100644 --- a/tests/scripts/run_lxc_all_tests.sh +++ b/tests/scripts/run_lxc_all_tests.sh @@ -47,6 +47,7 @@ run_test "LXC Most-Specific Path" "$SCRIPT_DIR/run_lxc_most_specific_test.sh" run_test "LXC Denied Masking" "$SCRIPT_DIR/run_lxc_denied_masking_test.sh" run_test "LXC Network" "$SCRIPT_DIR/run_lxc_network_test.sh" 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 Timeout" "$SCRIPT_DIR/run_lxc_timeout_test.sh" run_test "LXC Env+Cwd" "$SCRIPT_DIR/run_lxc_env_cwd_test.sh" diff --git a/tests/scripts/run_lxc_network_invalid_cidr_test.sh b/tests/scripts/run_lxc_network_invalid_cidr_test.sh new file mode 100644 index 000000000..c7401588f --- /dev/null +++ b/tests/scripts/run_lxc_network_invalid_cidr_test.sh @@ -0,0 +1,57 @@ +#!/bin/bash +# LXC invalid CIDR network filtering test +# +# Invalid CIDR entries should be reported as unresolved hosts and then skipped; +# they must not make firewall setup fail for the rest of the policy. +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 + +if [ ! -f "$LXC_EXEC" ]; then + echo "Error: lxc-exec not found. Run build.sh first." + exit 1 +fi + +CONFIG="$REPO_DIR/tests/configs/lxc_network_invalid_cidr.json" +INVALID_HOSTS=( + "140.82.112.0/33" + "2606:50c0::/129" + "140.82.112.0/not-a-prefix" +) + +fail() { + echo "FAIL: $1" + exit 1 +} + +echo "Running LXC invalid CIDR network filtering test..." + +# The process may fail because the default policy blocks egress; this test is +# only asserting firewall validation and setup behavior. +OUTPUT=$("$LXC_EXEC" "$CONFIG" 2>&1 || true) +echo "$OUTPUT" + +for host in "${INVALID_HOSTS[@]}"; do + if ! echo "$OUTPUT" | grep -Fq "Warning: could not resolve host '$host'"; then + fail "invalid host '$host' did not produce an unresolved-host warning." + fi +done + +# Invalid CIDRs are warned about and omitted from rule generation; applying the +# remaining firewall policy should still succeed. +if echo "$OUTPUT" | grep -qE "^(ip6?tables) .* failed:|Firewall setup failed:"; then + fail "invalid CIDR entry caused firewall setup to fail." +fi + +if ! echo "$OUTPUT" | grep -q "Default network policy: DROP"; then + fail "default-deny policy was not applied." +fi + +echo "PASS: invalid CIDR entries were warned about without failing firewall setup." +echo "LXC invalid CIDR network filtering test complete." diff --git a/tests/scripts/run_lxc_network_ipv6_cidr_test.sh b/tests/scripts/run_lxc_network_ipv6_cidr_test.sh index 306afc5df..4c1c6d212 100644 --- a/tests/scripts/run_lxc_network_ipv6_cidr_test.sh +++ b/tests/scripts/run_lxc_network_ipv6_cidr_test.sh @@ -26,6 +26,49 @@ if [ ! -f "$LXC_EXEC" ]; then fi CONFIG="$REPO_DIR/tests/configs/lxc_network_ipv6_cidr.json" +EXPECTED_HOSTS=( + "140.82.112.0/20" + "2606:50c0::/32" + "2606:50c0:8000::153" + "10.0.0.0/8" + "2001:db8::/32" + "fe80::1" +) + +fail() { + echo "FAIL: $1" + exit 1 +} + +load_config_hosts() { + if command -v python3 >/dev/null 2>&1; then + python3 -c 'import json, sys; data=json.load(open(sys.argv[1], encoding="utf-8")); net=data["network"]; print("\n".join(net.get("allowedHosts", []) + net.get("blockedHosts", [])))' "$CONFIG" + else + awk ' + /"allowedHosts"[[:space:]]*:/ { in_hosts=1; next } + /"blockedHosts"[[:space:]]*:/ { in_hosts=1; next } + in_hosts && /]/ { in_hosts=0; next } + in_hosts { print } + ' "$CONFIG" | sed -n 's/^[[:space:]]*"\([^"]*\)".*/\1/p' + fi +} + +mapfile -t CONFIG_HOSTS < <(load_config_hosts) +if [ "${#CONFIG_HOSTS[@]}" -ne "${#EXPECTED_HOSTS[@]}" ]; then + fail "config host count ${#CONFIG_HOSTS[@]} does not match expected count ${#EXPECTED_HOSTS[@]}." +fi +for expected in "${EXPECTED_HOSTS[@]}"; do + found=0 + for actual in "${CONFIG_HOSTS[@]}"; do + if [ "$actual" = "$expected" ]; then + found=1 + break + fi + done + if [ "$found" -ne 1 ]; then + fail "expected host '$expected' is missing from $CONFIG." + fi +done echo "Running LXC IPv6/CIDR network filtering test..." @@ -34,17 +77,13 @@ echo "Running LXC IPv6/CIDR network filtering test..." OUTPUT=$("$LXC_EXEC" "$CONFIG" 2>&1 || true) echo "$OUTPUT" -fail() { - echo "FAIL: $1" - exit 1 -} - # Every allow/block entry must survive resolution. An unparsed CIDR or IPv6 # literal is reported here instead of silently dropping a rule. -if echo "$OUTPUT" | grep -q "could not resolve host"; then - echo "$OUTPUT" | grep "could not resolve host" - fail "an IPv6 literal or CIDR entry was not resolved." -fi +for host in "${EXPECTED_HOSTS[@]}"; do + if echo "$OUTPUT" | grep -Fq "Warning: could not resolve host '$host'"; then + fail "host '$host' was not resolved." + fi +done # A rejected rule aborts setup. if echo "$OUTPUT" | grep -qE "^(ip6?tables) .* failed:|Firewall setup failed:"; then From 2e1a249b67bf19165a879d98066f6c004c8084e4 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Fri, 31 Jul 2026 13:04:56 -0700 Subject: [PATCH 7/7] Reject inverted egress port ranges instead of skipping them Skipping a malformed port selector is unsafe for a Deny rule: the traffic it was meant to block falls through to the default policy, which silently widens access under defaultPolicy: allow. Skipping an Allow rule is fail-closed, so the previous behavior was asymmetric. apply_firewall_rules now validates port selectors before creating any chain, so a malformed policy fails before it mutates host firewall state and there is nothing to roll back. This matches the sandbox policy spec, which rejects a configuration a backend cannot enforce rather than running it advisory. PortSpec keeps its interim { start, end } shape; the spec expresses ranges as flat port + endPort, and reconciling to that belongs with the schema work that adds a parser for egress_rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd --- .../lxc/common/src/network_iptables.rs | 59 ++++++++++++++++--- src/core/wxc_common/src/models.rs | 5 ++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/backends/lxc/common/src/network_iptables.rs b/src/backends/lxc/common/src/network_iptables.rs index 9cc4f4810..68e9190e0 100644 --- a/src/backends/lxc/common/src/network_iptables.rs +++ b/src/backends/lxc/common/src/network_iptables.rs @@ -397,19 +397,27 @@ impl NetworkIptablesManager { args } - /// Invalid port ranges are skipped instead of being passed to iptables, - /// which would reject the command and roll back the whole firewall apply. - fn log_invalid_port_selectors(policy: &ContainerPolicy, logger: &mut Logger) { + /// Reject a policy containing a malformed port selector instead of + /// dropping the offending rule. Skipping is unsafe for a `Deny` rule: the + /// traffic it was meant to block would fall through to the default policy, + /// which silently widens access under `defaultPolicy: allow`. The sandbox + /// policy spec takes the same position — a configuration a backend cannot + /// enforce is rejected rather than run advisory. + /// + /// Called before any chain is created so a bad policy fails before it + /// mutates host firewall state. + fn validate_port_selectors(policy: &ContainerPolicy) -> Result<(), String> { for rule in &policy.egress_rules { for port in rule.ports.iter().filter(|port| !port.is_valid()) { if let PortSpec::Range { start, end } = port { - logger.log_line(&format!( - "Warning: skipping invalid egress port range '{}:{}' (start > end)", + return Err(format!( + "invalid egress port range '{}:{}': start must not be greater than end", start, end )); } } } + Ok(()) } /// Build the allow/deny rule args for a container policy. @@ -575,6 +583,10 @@ impl NetworkIptablesManager { self.chain_name )); + // Reject a malformed policy before creating any chain, so nothing has + // to be rolled back and a Deny rule is never silently dropped. + Self::validate_port_selectors(policy)?; + // Probe ip6tables once. On IPv4-only hosts (binary absent or IPv6 // disabled in the kernel) enforce the v4 policy and skip the v6 chain // rather than failing setup for a policy that worked before dual-stack. @@ -602,8 +614,6 @@ impl NetworkIptablesManager { } } - Self::log_invalid_port_selectors(policy, logger); - let policy_rules = Self::build_policy_rule_args(&self.chain_name, policy); Self::run_iptables_rule_args(&policy_rules.ipv4, logger)?; if ipv6_enabled { @@ -1016,6 +1026,41 @@ mod tests { ); } + #[test] + fn validate_port_selectors_rejects_inverted_range() { + let policy = ContainerPolicy { + egress_rules: vec![EgressRule { + destinations: vec!["10.0.0.1".to_string()], + ports: vec![range(9000, 8000)], + action: RuleAction::Deny, + ..Default::default() + }], + ..Default::default() + }; + + let err = NetworkIptablesManager::validate_port_selectors(&policy) + .expect_err("an inverted port range must be rejected, not silently skipped"); + assert!( + err.contains("9000:8000"), + "error should name the offending range, got: {err}" + ); + } + + #[test] + fn validate_port_selectors_accepts_valid_and_single_value_ranges() { + let policy = ContainerPolicy { + egress_rules: vec![EgressRule { + destinations: vec!["10.0.0.1".to_string()], + ports: vec![single(443), range(8000, 8999), range(443, 443)], + action: RuleAction::Allow, + ..Default::default() + }], + ..Default::default() + }; + + assert!(NetworkIptablesManager::validate_port_selectors(&policy).is_ok()); + } + #[test] fn build_egress_rule_args_uses_ipv4_icmp_protocol_name() { let rule = EgressRule { diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index 011c7441e..ee04818eb 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -394,6 +394,11 @@ pub enum RuleAction { } /// A destination port selector: a single port or an inclusive range. +/// +/// Interim shape. The sandbox policy spec expresses ranges as flat +/// `ports[].port` + `ports[].endPort` (Kubernetes `endPort` style); this +/// enum will be reconciled to that shape once the schema work lands and +/// there is a parser that populates `egress_rules`. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(untagged)] pub enum PortSpec {