[LXC] Address network policy gaps - model 2 (deny-all-except-proxy) - #632
Conversation
- Fail-fast when no veth in firewall mode (no more silent skip of the FORWARD hook). - Emit deny rules before allow rules (deny-wins); pure ordered rule-builder + tests. - Inject HTTP(S)_PROXY + iptables proxy-only egress; scrub all inherited proxy env vars first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4
There was a problem hiding this comment.
Pull request overview
This PR implements a “deny-all-except-proxy” network model for the LXC backend, aiming to close enforcement gaps by failing fast when the FORWARD hook can’t be safely scoped, enforcing deny-wins rule ordering, and adding proxy-based egress restrictions plus proxy environment-variable hygiene for LXC executions.
Changes:
- Refactors LXC iptables rule construction into an ordered rule builder (deny before allow) and fails fast when a veth interface is unavailable for scoping.
- Adds LXC proxy enforcement that allows egress only to the configured proxy endpoint(s) and injects/scrubs proxy env vars for the container process.
- Extends LXC attach-run environment handling with a “force clear env” control to prevent inherited proxy variables from leaking when the caller env becomes empty after scrubbing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/wxc_common/src/proxy_env.rs | New helper + unit tests for scrubbing and applying proxy-related env vars. |
| src/core/wxc_common/src/lib.rs | Exposes the new proxy_env module. |
| src/core/wxc_common/src/config_parser.rs | Allows network.proxy for lxc, adds LXC-specific validation and updates/extends tests. |
| src/backends/lxc/common/src/network_iptables.rs | Implements proxy-only egress rules, deny-wins ordering, and fail-fast veth scoping; adds tests. |
| src/backends/lxc/common/src/lxc_runner.rs | Uses apply_proxy_env and passes a force-clear-env flag into attach_run; includes proxy in “needs network” detection. |
| src/backends/lxc/common/src/lxc_bindings.rs | Adds force_clear_env support to attach_run argument building and tests it. |
…NS in proxy mode Addresses Copilot reviewer feedback on the deny-all-except-proxy model: - Reject network.proxy.localhost for LXC (127.0.0.1 is the container loopback, unreachable from the container netns); require network.proxy.url with a routable host. Update the LXC proxy test to use a url and add a rejection test. - In proxy mode, only open outbound DNS when the proxy is addressed by hostname; when it is an IP literal keep the sandbox fully closed. Fix the misleading log line and add a host_is_ip_literal helper + unit test. - Correct the build_attach_args doc comment: it is #[cfg(test)]-only; move the 'Linux + test builds' note onto build_attach_args_with_env_control. - Note in docs/schema.md that LXC requires a url proxy (not loopback). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| logger, | ||
| )?; | ||
|
|
||
| self.rules_applied = true; |
There was a problem hiding this comment.
Same orphan-chain leak flagged in #631 https://github.com/microsoft/mxc/pull/631/changes#r3582849132
There was a problem hiding this comment.
Fixed in 3fd32ed.
apply_firewall_rules is now a thin wrapper around a fallible apply_firewall_rules_inner; on Err it logs, calls a new shared teardown_chains, and re-propagates. teardown_chains deletes the FORWARD hooks and flushes/deletes the chains in both tables, and every step is best-effort, so it is safe to call when only part of the state was created.
That was the real failure mode: remove_firewall_rules short-circuits on rules_applied == false, and rules_applied was only set after the last rule succeeded. So a mid-setup failure left the chain behind and every later attempt failed permanently on -N ("chain already exists") until someone cleaned up by hand.
remove_firewall_rules now delegates to the same teardown_chains, so the rollback path and the normal cleanup path can't drift apart.
There was a problem hiding this comment.
Fixed. Rollback is now scoped to what this specific apply created. CreatedFirewallState (network_iptables.rs:30-52) records each object as it is made, and the error path in apply_firewall_rules calls teardown_created(created, ...) (network_iptables.rs:588, def at :803), so it removes only this call's own chain, hooks, and DHCP rules. The chain flags (v4_chain/v6_chain) are set only after -N succeeds (:651, :654), so a failed -N on a truncated name that collides with a live container's chain never triggers a flush or delete of that incumbent.
| "ACCEPT".to_string(), | ||
| ]); | ||
| } | ||
| rules.push(vec![ |
There was a problem hiding this comment.
deny-all-except-proxy is IPv4-only; IPv6 egress is unfiltered?
This DROP- the "deny all other outbound" that the whole model-2 posture rests on, is only ever applied to the v4 table: every rule here is run through the iptables binary (run_iptables / run_iptables_args), and there's no ip6tables path anywhere in this file. So, on a container with IPv6 connectivity, all IPv6 egress bypasses this chain entirely and the "except-proxy" guarantee doesn't hold.
To be clear this is pre-existing v4-only behavior (resolve_host at :100-119 already drops v6, and run_iptables predates this PR), but this PR is what makes it load-bearing: before, v4-only filtering was a best-effort allow/block list; now it's the enforcement boundary of a deny-all-except-proxy security model, so the IPv6 gap becomes a real bypass rather than a minor omission.
Suggested fix: mirror the chain to ip6tables with a v6 DROP-all?
There was a problem hiding this comment.
Fixed in 3fd32ed. Agreed this was a real bypass rather than a theoretical one: with no v6 chain, a dual-stack container reached the internet over IPv6 while the v4 chain dropped everything.
The chain is now created and hooked in ip6tables too. Every per-destination rule stays IPv4 (resolve_host keeps only A records, and the proxy endpoint is a v4 literal), so the v6 chain carries the base rules — loopback and ESTABLISHED,RELATED, both family-agnostic — plus the same closing stance. In proxy mode that stance is DROP, which is what actually closes the hole.
Two things worth flagging:
- It sits behind an
ip6tables -Sprobe, which fails both when the binary is missing and when the kernel has IPv6 disabled. In either case we warn and still enforce the v4 policy rather than failing a policy that worked before dual-stack support existed — and such a host has no v6 egress to leak. In proxy mode the warning states explicitly that the rule set is IPv4-only. - Residual gap I deliberately did not close here: with
defaultPolicy: allowplusblockedHosts, those hosts stay reachable over IPv6. Blocking them needs AAAA resolution, which is [Bubblewrap/LXC] Address common network policy gaps - model 1 #631's territory, so I mirrored the v4 default instead of silently changingallowsemantics. Happy to file that as a follow-up work item if you'd rather track it.
Covered by base_chain_rules_are_family_agnostic (the shared argv must not name an address family) and proxy_mode_default_action_is_drop_regardless_of_default_policy.
There was a problem hiding this comment.
Fixed. The chain is mirrored to ip6tables now: the v6 -N is at network_iptables.rs:653, the closing DROP stance is mirrored in at :680-695, and FORWARD/INPUT are hooked over ip6tables at :762-771. On top of that, if ip6tables is unusable but the host has a live IPv6 stack and the v6 stance is DROP, setup fails closed instead of handing the container unfiltered v6 (:618-651, host_has_ipv6 at :204). So a dual-stack container can no longer get out over IPv6 while the v4 chain drops everything.
|
|
||
| // Hook the chain into FORWARD for the container's traffic | ||
| if let Some(ref iface) = self.veth_interface { | ||
| if allow_dns { |
There was a problem hiding this comment.
DNS opened to the whole internet in proxy+hostname mode?
ACCEPTs udp/tcp 53 with no -d, so under "deny-all-except-proxy" the container still has an unrestricted DNS channel to any resolver- a standing DNS-tunnel exfil path. Since the proxy is already resolved host-side, consider injecting the resolved IP into HTTP_PROXY and keeping DNS closed, or scope the DNS ACCEPT to the configured resolver(s).
There was a problem hiding this comment.
Fixed in 3fd32ed. DNS is now closed unconditionally in proxy mode — allow_dns = !proxy_enabled.
The old condition opened port 53 to the entire internet whenever the proxy was addressed by hostname. In a posture whose whole point is that the proxy is the only reachable destination, that left a standing DNS-tunnel exfil path.
What makes closing it possible is your suggestion on the other thread: the proxy hostname is resolved once on the host and the resulting literal is injected into HTTP(S)_PROXY, so the container never needs a resolver at all. Details in the reply there.
There was a problem hiding this comment.
Fixed. build_chain_rules returns early in proxy mode (network_iptables.rs:314) before the port-53 ACCEPT is ever emitted, so under deny-all-except-proxy there is no DNS channel at all. The DNS carve-out at :328-347 only runs outside proxy mode, where the allow/block lists actually need the container to resolve names. The proxy itself is resolved host-side and pinned (see the proxy-pin thread), so the container never needs a resolver.
| return Err("Network proxy port must be between 1 and 65535".to_string()); | ||
| } | ||
|
|
||
| let ips = Self::resolve_host(address.host()); |
There was a problem hiding this comment.
The ACCEPT rule targets the host-side resolution of the proxy hostname, but the container is handed HTTP_PROXY=http://proxy.example.com:8080 (hostname, via to_url()) and resolves it itself. Under round-robin/split-horizon DNS the container may pick a different IP than the one in the ACCEPT rule → proxy unreachable while everything else is dropped. Can we pin the injected proxy to the resolved IP fixes both this and the DNS-hole above?
There was a problem hiding this comment.
Fixed in 3fd32ed, taking your suggestion.
NetworkIptablesManager::pin_proxy_to_resolved_ip resolves the proxy host once, host-side, and returns a ProxyConfig whose address is that resolved literal. lxc_runner calls it before anything consumes the policy and hands the same pinned policy to both apply_firewall_rules and apply_proxy_env, so the ACCEPT rule and the container's HTTP(S)_PROXY can no longer disagree. That was the bug you spotted: the container re-resolved the name itself and, under round-robin or split-horizon DNS, could pick an address the firewall never allowed.
Details:
ProxyAddress::pinned_to_ipswaps only the host, preserving scheme, credentials, port and path, and doesn't add a trailing slash the caller didn't configure. It has to setoriginal_url, sinceto_url()otherwise falls back to a hardcoded127.0.0.1.- No-op for a disabled proxy, the built-in test server, or an address that is already an IP literal.
- Multi-A-record hosts collapse to the first address and log that they did. That is the intent — both sides then agree on one IP instead of racing DNS.
- An unresolvable proxy host now fails setup rather than silently producing a chain that drops everything.
- This is also what lets DNS stay closed, per the other thread.
Covered by pin_proxy_rewrites_hostname_to_resolved_ip, pin_proxy_leaves_ip_literals_untouched, pin_proxy_is_a_noop_when_disabled, pin_proxy_errors_when_hostname_does_not_resolve, plus four pinned_to_ip URL-shape tests in wxc_common.
There was a problem hiding this comment.
Fixed. The proxy hostname is resolved once on the host and pinned to that literal before anything consumes it. pin_proxy_to_resolved_ip (network_iptables.rs:440-490) rewrites the address to the resolved IP, and lxc_runner calls it at lxc_runner.rs:219 before the firewall ACCEPT and the injected HTTP(S)_PROXY are built (:301). Both sides name the same IP now, so round-robin or split-horizon DNS cannot hand the container a different address than the ACCEPT rule. Because the container no longer resolves anything, DNS stays closed, which is what closes the DNS hole in the sibling thread too.
There was a problem hiding this comment.
Fixed. pin_proxy_to_resolved_ip now resolves the hostname once on the host, rewrites the effective proxy address to that IP, and lxc_runner.rs does this before container.start(). The same effective_policy is then used for both the firewall and HTTP(S)_PROXY, so the container cannot resolve a different endpoint. This has not been exercised against live LXC or iptables.
|
Shall we add LXC proxy.url test config json? |
…json) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
…t-proxy Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
🧪 Local test re-verification — 2026-07-17Re-ran the test suites locally at branch tip Windows host (
Linux (WSL2 Ubuntu-24.04,
Note: the |
…et-model2-deny-all-except-proxy # Conflicts: # src/core/wxc_common/src/config_parser.rs
…proxy, DNS closed
Firewall setup could fail partway and leave orphan chains, only filtered
IPv4, opened DNS to the whole internet, and hooked FORWARD on the wrong
direction. Each of those either breaks a retry or defeats the
deny-all-except-proxy posture this PR exists to add.
- Roll back partial state. apply_firewall_rules now wraps a fallible inner
body and, on failure, flushes and deletes whatever was created via a
shared teardown_chains. Previously remove_firewall_rules short-circuited
on rules_applied == false, so a mid-setup failure left the chain behind
and every later attempt failed on -N ("chain already exists").
- Mirror the chain into ip6tables. Without a v6 chain a dual-stack
container reached the internet over IPv6 while the v4 chain dropped
everything -- a straight bypass of model 2. All destination rules stay
IPv4 (resolve_host keeps only A records), so the v6 chain carries the
base rules plus the same closing stance. Guarded by an ip6tables -S
probe: IPv4-only hosts warn and continue rather than failing a policy
that worked before, and such a host has no v6 egress to leak.
- Keep DNS closed in proxy mode. The proxy host is now resolved once on
the host and the resulting literal is injected into HTTP(S)_PROXY, so
the container never needs a resolver. This also makes the ACCEPT rule
and the container's proxy setting name the same endpoint; previously
the container re-resolved the hostname and could pick a different
address under round-robin or split-horizon DNS and be dropped by its
own policy. An unresolvable proxy host now fails setup instead of
silently producing a chain that drops everything.
- Hook FORWARD with -i, not -o. Container-originated packets arrive at
the host on the host-side veth, so egress matches -i; -o matched
traffic toward the container, leaving egress unfiltered. The teardown
-D uses -i for the same reason, or the hook leaks. Same fix microsoft#631 made
in 96af8f9.
- Proxy mode now closes with DROP regardless of defaultPolicy, so an
explicit defaultPolicy=allow cannot reopen the chain.
Known gap, deliberately not addressed here: with defaultPolicy=allow and
blockedHosts, those hosts remain reachable over IPv6. Blocking them needs
AAAA resolution, which belongs with the IPv6/CIDR work in microsoft#631.
Tests: 392 pass on Linux (cargo test -p lxc_common -p wxc_common),
clippy -D warnings clean, rustfmt clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
src/core/wxc_common/src/config_parser.rs:819
- This rejects only the
localhostshorthand, but the requiredurlform still acceptshttp://localhost:8080,http://127.0.0.1:8080, and other loopback literals. Those resolve/pin to the same container-local loopback and are equally unreachable, so the accepted configuration deterministically fails at execution. Validate the URL host (and the resolved endpoint) is non-loopback for LXC.
if containment == ContainmentBackend::Lxc && proxy_used_localhost {
let msg = "LXC: network.proxy.localhost is not reachable from the \
container network namespace (127.0.0.1 is the container \
loopback); use network.proxy.url with a host routable from \
inside the container";
src/backends/lxc/common/src/network_iptables.rs:360
- IPv6 proxy URLs are treated as already pinned here, but they cannot reach the rule builder:
resolve_proxy_endpointscalls the IPv4-onlyresolve_host, which drops every IPv6 literal and returns “Could not resolve network proxy host.” Since the generic proxy parser accepts bracketed IPv6 URLs and this PR adds an IPv6 chain, either build the proxy ACCEPT inip6tablesor reject IPv6 proxies explicitly during LXC validation.
if Self::host_is_ip_literal(address.host()) {
return Ok(proxy.clone());
}
let ips = Self::resolve_host(address.host());
src/backends/lxc/common/src/network_iptables.rs:532
- These unrestricted port-53 accepts are inserted before the blocked-destination rules. Consequently a host listed in
blockedHostsis still reachable over TCP/UDP 53, so deny does not actually win over the infrastructure DNS allowance. Emit explicit deny rules first and scope DNS access to the resolver destinations the container is expected to use.
if allow_dns {
for protocol in ["udp", "tcp"] {
Self::run_iptables(
&[
"-A",
src/core/wxc_common/src/models.rs:460
pinned.pop()removes the final character of the whole serialized URL, not necessarily the synthetic/. For a valid URL such ashttp://proxy.example:8080?token=abc, serialization produceshttp://IP:8080/?token=abc, and this code returns a URL ending inab, corrupting the query while leaving the slash. Remove the slash immediately before the query/fragment boundary instead.
let mut pinned = parsed.to_string();
// `Url::to_string` normalises an empty path to "/"; drop it again when
// the configured URL had none, so the injected value keeps the shape
// the caller supplied.
if parsed.path() == "/" && !raw.ends_with('/') {
pinned.pop();
}
| "-A".to_string(), | ||
| chain_name.to_string(), | ||
| "-m".to_string(), | ||
| "state".to_string(), | ||
| "--state".to_string(), | ||
| "ESTABLISHED,RELATED".to_string(), | ||
| "-j".to_string(), | ||
| "ACCEPT".to_string(), | ||
| ], |
There was a problem hiding this comment.
Fixed in 4c71e2e — you're right, and the reasoning about direction is the key part.
The chain is only ever reached from a hook scoped to -i <veth>, so reply packets (which arrive -o <veth>) never traverse it. The ESTABLISHED,RELATED accept therefore did nothing for return traffic; its only effect was to let flows the container opened before the chain was installed keep running straight through the deny-all policy.
Removing it doesn't break permitted traffic, because every remaining rule matches on destination (-d <proxy_ip> --dport <port>, -d <allowed_ip>), which holds for every packet of a flow rather than only the SYN — no conntrack exemption is needed.
I removed the -i lo accept in the same change: the input interface is <veth> by construction, so that rule could never match either.
New test egress_chain_has_no_conntrack_or_loopback_accept asserts no rule in the chain carries --state / --ctstate / ESTABLISHED,RELATED / lo.
| } else if proxy_enabled { | ||
| logger.log_line( | ||
| "Warning: No veth interface set for container. \ | ||
| Cannot scope iptables rules. Skipping FORWARD hook.", | ||
| "Warning: ip6tables unavailable, so the deny-all-except-proxy rule set is \ | ||
| IPv4-only; IPv6 egress is unfiltered if the host has IPv6 connectivity.", | ||
| ); |
There was a problem hiding this comment.
Fixed in 4c71e2e. You're right that the probe conflated the two cases — that's my bug: I added the blanket skip to resolve a review comment about ip6tables being a hard dependency on IPv4-only hosts, and in doing so made it fail-open on hosts that do have IPv6.
The two are now separated. ip6tables_available() still answers "can I run it", and a new host_has_ipv6() answers "is there IPv6 to filter" by reading /proc/net/if_inet6 — that file exists only when the kernel's IPv6 support is present, and is empty when IPv6 is administratively disabled (net.ipv6.conf.all.disable_ipv6=1). Unreadable is treated as "IPv6 is live" so the ambiguous case fails closed.
Behavior now:
- ip6tables usable → unchanged, v6 chain installed.
- ip6tables unusable, no live IPv6 stack → skip v6, enforce IPv4, log it. This is the pure-IPv4 host the earlier comment was about, and it has no v6 egress to leak.
- ip6tables unusable, IPv6 live, and the policy's v6 stance is DROP (proxy mode, or
defaultPolicy: block) → setup fails with an actionable message rather than handing the container unfiltered IPv6. - ip6tables unusable, IPv6 live, v6 stance is ACCEPT → log that IPv4-only enforcement is in effect; nothing is being silently dropped, since the v6 chain would have accepted anyway.
| // 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 and | ||
| // leave container egress — the thing this policy exists to restrict — | ||
| // entirely unfiltered. | ||
| Self::run_iptables( | ||
| &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name], | ||
| logger, |
There was a problem hiding this comment.
Fixed in 4c71e2e. Confirmed — netfilter delivers locally-destined packets via PREROUTING → INPUT and never through FORWARD, so a FORWARD-only hook left the bridge gateway and every service on the host reachable from inside the container. That defeats the model-2 guarantee regardless of how complete the v4/v6 chains are.
The same chain is now hooked into INPUT as well, scoped identically to -i <veth>, in both address families. Reusing the chain gives the right semantics for free: a host-local proxy is still permitted by its own ACCEPT rule, while everything else on the host falls through to the chain's DROP.
One carve-out was needed. lxc-net runs dnsmasq on the bridge, so a DHCPREQUEST at lease-renewal time is addressed to the host and would have hit the new DROP, costing the container its IP mid-run. udp/67 (and udp/547 for DHCPv6) is inserted ahead of the chain jump — -I twice, jump first — so renewal survives. It's a link-local exchange with the bridge, not an egress path, so it doesn't weaken the posture.
Worth flagging on validation: this is covered by unit tests at the rule-construction level, but I don't have an LXC host to exercise the live INPUT path against, so the DHCP carve-out in particular hasn't been observed working end-to-end.
Conflict in config_parser.rs was the network-proxy backend allowlist. upstream/main added Seatbelt to the message but does not know about LXC proxy support, which is what this branch adds. Kept this branch's side: 'lxc' stays in the supported-backends message, and the two LXC-specific rejections (builtinTestServer, and localhost -- 127.0.0.1 is the container's own loopback inside its netns, not the host) are preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (7)
src/backends/lxc/common/src/network_iptables.rs:589
- This branch knowingly succeeds with IPv6 egress unfiltered.
ip6tables -Sfailure does not prove IPv6 is disabled: a dual-stack host may simply lack the binary or encounter a transient/permission failure. In proxy mode that defeats the core deny-all-except-proxy guarantee. Fail setup unless IPv6 is independently proven disabled, or enforce the v6 policy through another available firewall interface.
} else if proxy_enabled {
logger.log_line(
"Warning: ip6tables unavailable, so the deny-all-except-proxy rule set is \
IPv4-only; IPv6 egress is unfiltered if the host has IPv6 connectivity.",
);
src/backends/lxc/common/src/network_iptables.rs:507
- The unconditional
ESTABLISHED,RELATEDrule is evaluated before the proxy allow/drop rules. This runner can reuse an already-running container and also starts a new container before installing the FORWARD hook, so a process can already hold a direct connection; subsequent packets on that connection bypass the deny-all-except-proxy DROP. In proxy mode, omit this broad state rule (the proxy destination/port ACCEPT already permits that flow) or scope it to the proxy endpoint.
// Always allow loopback and established connections, in both families.
let base_rules = Self::build_base_chain_rule_args(&self.chain_name);
for args in &base_rules {
Self::run_iptables_args(args, logger)?;
}
src/core/wxc_common/src/config_parser.rs:945
- This only rejects the
localhostshorthand;network.proxy.urlstill acceptshttp://localhost:8080,http://127.0.0.1:8080, andhttp://[::1]:8080. The hostname form is pinned to a loopback literal, and all of these then point the container at its own namespace, reproducing the unreachable-proxy problem this guard intends to prevent. Reject loopback URL hosts as well, including hostnames that resolve to loopback.
if containment == ContainmentBackend::Lxc && proxy_used_localhost {
let msg = "LXC: network.proxy.localhost is not reachable from the \
container network namespace (127.0.0.1 is the container \
loopback); use network.proxy.url with a host routable from \
inside the container";
logger.log_line(msg);
return Err(WxcError::ConfigParse(msg.to_string()));
src/core/wxc_common/src/models.rs:460
pinned.pop()removes the final character, but the normalized slash is not final when the URL has a query or fragment. For example,http://proxy:8080?token=abcis rewritten tohttp://ip:8080/?token=ab, corrupting the proxy URL. Remove the slash at the path boundary instead of blindly popping the last byte.
if parsed.path() == "/" && !raw.ends_with('/') {
pinned.pop();
src/core/wxc_common/src/models.rs:433
- Replacing the hostname in an
https://proxy URL also changes its TLS server identity and SNI. A normal proxy certificate forproxy.example.comwill fail validation when clients are handedhttps://10.0.0.7:8443, so the HTTPS proxy form exercised by the new test becomes unusable unless the certificate contains that IP. Either reject HTTPS proxy URLs for this pinning model or pin name resolution inside the container while retaining the original hostname in the URL.
/// Used to pin a hostname-based proxy to the address the host actually
/// resolved, so the firewall rule and the `HTTP(S)_PROXY` handed to the
/// sandbox agree on one endpoint. Without this the sandbox re-resolves the
/// hostname itself and, under round-robin or split-horizon DNS, can pick an
/// address the firewall never allowed.
pub fn pinned_to_ip(&self, ip: &str) -> Self {
src/backends/lxc/common/src/network_iptables.rs:360
- IPv6 proxy endpoints are classified as IP literals and returned unchanged here, but
resolve_proxy_endpointssubsequently calls the IPv4-onlyresolve_host, which drops IPv6 literals and AAAA-only results. Thus a parser-accepted IPv6 proxy URL always fails setup and no proxy ACCEPT is emitted in the v6 chain. Route endpoints by address family and emit the ACCEPT in the matching table, or reject IPv6 proxy endpoints during config validation with a clear error.
if Self::host_is_ip_literal(address.host()) {
return Ok(proxy.clone());
}
let ips = Self::resolve_host(address.host());
tests/configs/lxc_network_proxy.json:18
- This fixture is not invoked by
run_lxc_all_tests.sh, andproxy.example.comdoes not provide a test proxy, so it cannot exercise the new firewall/proxy behavior. The pure rule-builder tests cannot catch wrong interface direction, missing firewall tools, or an actual IPv6 bypass. Add an LXC integration script with a locally controlled proxy that asserts proxy traffic succeeds while direct IPv4, IPv6, and DNS traffic fail.
"proxy": { "url": "http://proxy.example.com:8080" },
| logger.log_line(&format!( | ||
| "Firewall setup failed: {}. Cleaning up partial iptables state.", | ||
| e | ||
| )); | ||
| self.teardown_chains(logger); |
There was a problem hiding this comment.
Fixed in 4c71e2e — and this one was mine: I added the unconditional rollback to resolve an earlier comment about orphan chains, without accounting for the truncated-name collision you've identified.
Apply now records what it actually created in a CreatedFirewallState, and the rollback removes only those objects. The ordering rules matter and are commented in the code:
- Chain flags are set only after
-Nsucceeds.-F/-Xare name-scoped and would hit a colliding container, so a failed-Nnow leavesv4_chain/v6_chainfalse and the rollback is a no-op — the pre-existing chain is untouched, and its FORWARD/INPUT hooks keep pointing at a populated chain. - Hook and DHCP flags are set before the insert. Every hook delete is scoped to this container's veth (
-D FORWARD -i <our-veth> -j <chain>), so a-Dfor a rule never inserted is a harmless no-op that cannot affect another container — whereas marking afterwards would leak the FORWARD hook if the INPUT insert failed.
teardown_chains (post-success cleanup and force_cleanup) is now teardown_created(CreatedFirewallState::all()), so the two paths can't drift. Covered by rollback_state_starts_empty_so_a_failed_chain_create_touches_nothing and full_teardown_state_covers_every_created_object.
Note the collision itself still exists — two containers whose first 20 sanitized name characters match will contend for one chain name. This change stops the rollback from corrupting the incumbent, but the second container still fails to start. Making the chain name collision-free is a separate change; happy to file it as a work item.
…et-model2-deny-all-except-proxy # Conflicts: # docs/schema.md # src/core/wxc_common/src/config_parser.rs # src/core/wxc_common/src/proxy_env.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/backends/lxc/common/src/network_iptables.rs:601
- Hooking only
FORWARDdoes not cover packets addressed to the LXC host itself; those enter the host'sINPUTchain. A workload can therefore still reach host bridge/gateway services on arbitrary ports without ever traversing this proxy-only chain, so the proxy is not actually the only reachable destination. Add corresponding veth-scoped host-input enforcement (and symmetric cleanup), while preserving any host traffic the container explicitly needs.
Self::run_iptables(
&["-I", "FORWARD", "-i", iface, "-j", &self.chain_name],
logger,
)?;
src/backends/lxc/common/src/network_iptables.rs:469
- Rollback currently deletes state merely because setup failed, even when this invocation never created it. Chain names are sanitized and truncated to 20 characters, so two container IDs can collide; if the second
-Nfails with “chain already exists,” this teardown flushes the first container's live chain, leaving its existing FORWARD hook pointing at an empty chain and potentially bypassing its policy. Track which chains/hooks this attempt successfully created and roll back only those resources (or make chain ownership collision-resistant).
self.teardown_chains(logger);
src/backends/lxc/common/src/network_iptables.rs:148
- A nonzero
ip6tables -Sdoes not prove that IPv6 is disabled: it can also mean the binary is missing on an IPv6-capable host, insufficient permission, or a transient xtables/nftables failure. Treating every such result as “IPv6 unavailable” lets proxy-mode execution continue with IPv6 completely unfiltered (as the later warning acknowledges), violating the deny-all-except-proxy guarantee. Proxy mode must fail closed unless the host can positively establish that the container has no IPv6 connectivity; otherwise propagate the probe failure.
Ok(output) => {
let stderr = String::from_utf8_lossy(&output.stderr);
logger.log_line(&format!(
"ip6tables unavailable ({}); skipping IPv6 firewall rules.",
stderr.trim()
src/backends/lxc/common/src/network_iptables.rs:357
- IPv6 proxy URLs are accepted by the common parser, and this branch recognizes an IPv6 host as a literal and leaves it unchanged. However,
resolve_proxy_endpointsthen calls the IPv4-onlyresolve_host, gets an empty vector, and rejects setup, so an endpoint such ashttp://[2001:db8::1]:8080cannot use the newly added IPv6 chain. Route proxy endpoints by address family and emit the IPv6 ACCEPT inip6tables, or reject IPv6 proxy URLs explicitly at LXC validation rather than accepting them and failing at runtime.
if Self::host_is_ip_literal(address.host()) {
return Ok(proxy.clone());
src/core/wxc_common/src/models.rs:459
- For a valid proxy URL with an empty path and a query or fragment, such as
http://proxy:8080?token=x,Url::to_string()produceshttp://ip:8080/?token=x;pinned.pop()then removes the final query/fragment character rather than the slash. Only pop when the serialized value actually ends in/, otherwise leave the normalized slash in place so credentials/options are not corrupted.
if parsed.path() == "/" && !raw.ends_with('/') {
pinned.pop();
tests/configs/lxc_network_proxy.json:18
- This config is not invoked by
tests/scripts/run_lxc_all_tests.sh(which only runslxc_network_test.json), andproxy.example.comis intentionally non-resolvable, so it cannot exercise a successful proxy-only run even if invoked. Add an LXC integration test backed by a reachable local proxy and assert both that proxied traffic succeeds and that direct IPv4/IPv6 plus inherited proxy exemptions are blocked; otherwise the security-sensitive iptables/ip6tables wiring remains untested.
"proxy": { "url": "http://proxy.example.com:8080" },
… firewall - Remove the ESTABLISHED,RELATED and -i lo accepts from the container chain. The chain is only reached from hooks scoped to -i <veth>, so reply traffic never traverses it and the conntrack accept only let flows opened before the chain existed keep running through the deny-all policy. - Hook the chain into INPUT as well as FORWARD. Netfilter sends host-destined packets to INPUT, so a FORWARD-only hook left the bridge gateway and every host service reachable from the container. DHCP (udp/67, udp/547) is accepted ahead of the jump so lease renewal still works. - Fail setup when ip6tables is unusable but the host has a live IPv6 stack and the policy's v6 stance is DROP. The previous probe conflated 'IPv6 is disabled' with 'ip6tables is missing' and silently left IPv6 unfiltered. - Track which chains/hooks each apply actually created and roll back only those. Chain names are truncated to 20 characters, so a blind rollback after a failed -N flushed a colliding live container's chain. - Fold the DNS carve-out into the chain builder so the whole chain is constructed and tested in one place, with deny rules ahead of it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
src/backends/lxc/common/src/network_iptables.rs:1022
- The IPv6 chain is also flushed while a failed hook deletion may leave it referenced. That turns the remaining jump into a pass-through and defeats the default DROP before
-Xreports failure. Only flush/delete this chain after every owned IPv6 hook is gone.
if created.v6_chain {
let _ = Self::run_ip6tables(&["-F", &self.chain_name], logger);
if Self::run_ip6tables(&["-X", &self.chain_name], logger).is_ok() {
src/backends/lxc/common/src/network_iptables.rs:940
- A successful apply does not necessarily create everything: IPv6 may be unavailable, and Bubblewrap creates chains without veth hooks or DHCP rules. Replacing that actual state with
all()manufactures ownership; no-veth teardown cannot clear the synthetic hook flags, while signal publication can later advertise resources this process never owned. Retain the exactcreatedstate in the manager and tear down that state instead.
fn teardown_chains(&self, logger: &mut Logger) -> CreatedFirewallState {
self.teardown_created(CreatedFirewallState::all(), logger)
src/backends/lxc/common/src/network_iptables.rs:41
v4_hooksandv6_hookseach conflate two independently fallible objects (FORWARD and INPUT). If one insert/delete succeeds and the other fails, the residual cannot represent which hook remains; on retry, deletion of the already-removed first hook fails andIterator::allshort-circuits before attempting the surviving hook. Track each family/hook independently and attempt every owned deletion so partial setup and teardown can converge.
v4_hooks: bool,
v6_hooks: bool,
tests/configs/lxc_network_proxy.json:6
- These negative checks classify every
wget/nslookupfailure as firewall enforcement. On an offline host, with no IPv6 route, broken DNS, or a missing resolver utility, all three “BLOCKED” sentinels can appear even if the firewall is absent. Establish reachable control endpoints before applying the policy (prefer locally hosted direct IPv4/IPv6 and DNS targets), then verify the same endpoints fail under the sandbox policy.
"commandLine": "ok=PROXY_FAIL; if wget -qO- --timeout=10 http://sentinel.invalid/ 2>/dev/null | grep -q MXC_PROXY_SENTINEL; then ok=PROXY_OK; fi; echo \"$ok\"; if http_proxy= https_proxy= HTTP_PROXY= HTTPS_PROXY= all_proxy= ALL_PROXY= wget -qO- --timeout=5 http://1.1.1.1/ >/dev/null 2>&1; then echo DIRECT_IPV4_LEAK; else echo DIRECT_IPV4_BLOCKED; fi; if ip -6 addr show scope global 2>/dev/null | grep -q inet6; then if http_proxy= https_proxy= HTTP_PROXY= HTTPS_PROXY= all_proxy= ALL_PROXY= wget -qO- --timeout=5 'http://[2606:4700:4700::1111]/' >/dev/null 2>&1; then echo DIRECT_IPV6_LEAK; else echo DIRECT_IPV6_BLOCKED; fi; else echo DIRECT_IPV6_SKIP_NO_STACK; fi; if nslookup example.com >/dev/null 2>&1; then echo DNS_LEAK; else echo DNS_BLOCKED; fi"
| if created.v4_chain { | ||
| let _ = Self::run_iptables(&["-F", &self.chain_name], logger); | ||
| if Self::run_iptables(&["-X", &self.chain_name], logger).is_ok() { |
There was a problem hiding this comment.
Correct, and fixed in 10f0599. -X failing on a referenced chain is harmless, but -F succeeds no matter who points at the chain, and an empty user chain returns to the caller instead of reaching its closing DROP. A container whose hook survived teardown kept running with the jump intact and nothing behind it, which is fail-open for as long as it lives.
Both commands are now gated on the hooks being confirmed gone, so the chain stays populated and stays published and a later pass retries. I extracted teardown_chain to make the ordering testable without iptables -- the previous shape had no test at all, and a mutant that flushed unconditionally survived the entire suite.
| Self::run_iptables(&["-N", &self.chain_name], logger)?; | ||
| created.v4_chain = true; | ||
| Self::publish_created(created); |
There was a problem hiding this comment.
Confirmed, still open, and I am recording it as a known gap rather than attempting a blind fix.
The window is real and it is narrow: -N succeeds, and the very next statement publishes ownership. A signal in between leaves a chain that exists and is unpublished, so the watchdog skips it. I cannot close it by reordering, because publishing before -N is worse -- a process whose -N lost the race to a concurrent start would then own a chain it did not create and delete the winner's firewall on a signal. Getting this right needs the publish and the command to be atomic with respect to the watchdog, which means either blocking signals around the pair or a cleanup design that is safe to run against a chain someone else owns.
Neither is a small change, and neither can be validated on this workstation: reproducing it needs Linux with real signal delivery, and every iptables command fails at spawn here. Landing a fix I cannot test for a race I cannot reproduce is how the wrong invariant gets shipped with a green suite behind it.
What the outcome actually costs is a leaked chain -- host state with nobody to remove it -- not a container running unfiltered. That is a leak, not a fail-open. I have written it into the PR description as a known gap so it survives this thread.
| return; | ||
| } | ||
| let mut mgr = Self::new(container_name); | ||
| if let Some(v) = veth_interface { | ||
| mgr.set_veth_interface(v); |
There was a problem hiding this comment.
I checked this one against the branch and it no longer holds. force_cleanup sets mgr.created = created -- the published record verbatim -- and remove_firewall_rules tears down self.created. Nothing on that path expands to CreatedFirewallState::all().
The expansion you are describing was the original bug, and it is what this PR fixed. all() is now #[cfg(test)] (network_iptables.rs:79), so a production path that called it would not compile. It survives only as the stale superset a test hands to teardown to prove teardown replaces it.
The partial-record case you raise is the right thing to worry about and it is handled by construction: a signal after IPv4 -N but before IPv6 -N finds a record with v4_chain set and v6_chain clear, and teardown acts on exactly that. It issues no ip6tables command at all, so there is nothing for a colliding container to lose.
The previous commit made an unresolvable `blockedHosts` entry fatal only under `defaultPolicy: allow`, on the reasoning that default-block's closing DROP already covers a host that got no DROP rule of its own. An independent review checked that reasoning against the chain the builder actually emits, and it is false. Outside proxy mode `build_chain_rules` emits an unscoped port-53 ACCEPT -- `-p udp --dport 53 -j ACCEPT` and the tcp equivalent, with no `-d` -- and it emits it *after* the blocked-host DROPs. iptables is first-match-wins, so a blocked host with no DROP rule of its own is still reachable on port 53. The closing DROP covers every other port, not that one. That leaves a DNS-tunnel path to a host the operator explicitly named as blocked, which is the exfil route the deny posture exists to close. So the entry is fatal under both default policies now, and the parameter that selected between them is gone. `allowedHosts` stays best-effort in both cases: an unresolved allow only ever removes reachability. The reasoning is now pinned by a test rather than by a comment. `the_port_53_accept_follows_the_block_rules_and_names_no_destination` asserts both facts it depends on -- that the DNS accept follows the block rules, and that it names no destination -- so if either changes, the test says so instead of the comment quietly going stale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6
force_cleanup took the published ownership record but used it only as an emptiness check, then removed CreatedFirewallState::all() through teardown_chains. Chain names truncate to 20 characters and can collide, so acting on the full set could remove a different live container's chain and hooks while it was still running -- failing that container open. The manager now carries the record it published (created), apply and remove keep it current, and force_cleanup adopts the signal-time record before teardown. teardown_chains is deleted; it was the only path that substituted all(), and all() is now test-only so no production path can reintroduce the assumption. Two tests cover it, both mutation-verified against restoring the old behavior: one proves force_cleanup republishes exactly the record it was handed, one proves an empty record leaves the registry untouched. The observable is the republished record rather than the log, because on a host with no iptables binary every command fails at spawn before anything is logged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/scripts/run_lxc_all_tests.sh:49
- This runner appears to treat any non-zero exit as FAIL (per the shared
run_testpattern), butrun_lxc_network_proxy_test.shintentionally uses exit 77 for an honest skip. As-is, adding this test will causerun_lxc_all_tests.shto fail on hosts lacking prerequisites (non-root/non-Linux/no LXC/python3), which makes the aggregate runner less usable. Consider extendingrun_lxc_all_tests.shto recognize exit 77 as SKIP (similar torun_bwrap_all_tests.sh), or change the proxy test’s exit strategy if the LXC runner intentionally doesn’t support skips.
run_test "LXC Network Proxy" "$SCRIPT_DIR/run_lxc_network_proxy_test.sh"
src/core/wxc_common/src/models_proxy_url_spec_tests.rs:256
- These assertions likely don’t match
url::Urlbehavior:Url::host_str()commonly returns the IPv6 literal without brackets (e.g.\"::1\"), while the serialized URL string includes brackets. This can make the new spec tests fail even when the rewrite output is correct. A more robust check is to assertoutcontains the bracketed form (\"[::1]\") while assertingparsed.host_str()equalsSome(\"::1\"). The same issue appears in thepinned_to_ip_*ipv6*tests that also comparehost_str()against a bracketed value.
assert_eq!(
parsed.host_str(),
Some("[::1]"),
"parsed host must be [::1] (url crate includes brackets in host_str for IPv6); output={out:?}"
);
src/core/wxc_common/src/config_parser.rs:440
- The wording “as stored by the proxy URL parser” is likely inaccurate/misleading:
convert_wire_proxystoresparsed.host_str().to_string(), andhost_str()is typically unbracketed for IPv6 (the brackets are URL syntax, not the host value). Sincehost_is_loopbackalready supports both forms by stripping brackets, consider rephrasing to something like “Accepts bracketed IPv6 literals if present (e.g.[::1])” to avoid implying the parser produces bracketed hosts.
/// Whether `host` is a loopback endpoint that a container cannot reach through
/// its own network namespace: 127.0.0.0/8, ::1, or the name "localhost".
///
/// Accepts bracketed IPv6 literals (e.g. `[::1]`) as stored by the proxy URL
/// parser. Used to reject loopback proxy hosts under the LXC deny-all model,
/// where the container's loopback is not the host's.
src/backends/lxc/common/src/network_iptables.rs:436
- PR description states: “An unresolvable
blockedHostsentry fails the start underdefaultPolicy: allow… UnderdefaultPolicy: block… skipped with a warning.” However, the implementation here makes unresolvedblockedHostsfatal unconditionally (with rationale about the DNS carve-out). Please align the PR description with the implemented behavior, or adjust the code to match the stated behavior (and explain how DNS/port-53 handling remains safe in that case).
/// Resolve `blockedHosts` to IPv4 addresses, failing setup when an entry
/// resolves to nothing.
///
/// An unresolved *blocked* host is not the same as an unresolved *allowed*
/// host. A blocked host that produces no DROP rule stays reachable while
/// setup reports success — a fail-open — so the only fail-closed outcome is
/// to refuse setup. An unresolved *allowed* host only ever removes
/// reachability, so it stays best-effort.
///
/// This is fatal under both default policies. Default-allow is the obvious
/// case: the chain falls through to ACCEPT. Default-block is not safe
Hook removal used Iterator::all, which stops at the first failure. One ownership flag covers the FORWARD+INPUT pair, so a failed FORWARD delete left INPUT untried -- and because the flag then stays set, every retry failed on FORWARD again and never reached INPUT. Worse, a retry after a FORWARD delete that did succeed fails precisely because it succeeded, so the pair could never converge. The leaked hook is the INPUT one, which carries traffic addressed to the host itself: the hole the chain exists to close. The loop is now a named helper that attempts every hook and reports whether all of them succeeded. A -D for a hook that is already gone is a harmless no-op -- it is scoped to this container's own veth and cannot match another container's rule -- so attempting all of them costs nothing. Two tests cover it through the helper rather than through iptables, which cannot run on the build host: one proves a failing first hook does not stop the second being attempted, one proves ownership is given up only when the whole pair went. Mutation-verified: restoring Iterator::all fails the first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/backends/lxc/common/src/network_iptables.rs:809
- There is still a signal race between successful chain creation and ownership publication. If the watchdog snapshots
ActiveSandboxafteriptables -Nreturns but beforev4_chainis set/published, it sees an empty record, performs no firewall cleanup, and exits the process while the newly created chain remains leaked; the same window exists for the IPv6 chain. Make chain creation and ownership handoff atomic with respect to the watchdog (for example via a shared transaction/lock), without pre-claiming a chain whose-Nmay lose a name collision.
Self::run_iptables(&["-N", &self.chain_name], logger)?;
created.v4_chain = true;
Self::publish_created(created);
src/backends/lxc/common/src/network_iptables.rs:619
- The parse-time lexical check does not cover hostnames that resolve to loopback. For example,
http://localhost.:8080passeshost_is_loopback, resolves here to127.0.0.1, and is pinned without revalidation; setup then reports success while the container is pointed at its own unreachable loopback and all other egress is dropped. Reject a selected resolved address whenIpAddr::is_loopback()before callingpinned_to_ip(and update the localhost-based unit test to use an injected/non-loopback resolver result).
let ips = Self::resolve_host(address.host());
let Some(ip) = ips.first() else {
return Err(format!(
"Could not resolve network proxy host '{}' to an IPv4 address",
address.host()
));
};
| if created.v4_hooks { | ||
| let removed = remove_every_hook(HOOKS, |hook| { | ||
| Self::run_iptables(&["-D", hook, "-i", iface, "-j", &self.chain_name], logger) | ||
| .is_ok() | ||
| }); | ||
| if removed { | ||
| residual.v4_hooks = false; | ||
| } |
There was a problem hiding this comment.
The failure mode is real and is fixed in 10f0599, but not the way you suggested, and the difference matters.
Per-hook flags do not fix it on their own. Ownership is published before the -I that installs a hook, deliberately, to close the window where a signal would find the rule unpublished. So a per-hook flag can be published ahead of an -I that then fails, and -D for a rule that was never installed fails too -- the flag sticks exactly as the shared one did. Splitting the record changes its granularity, not the question being asked of iptables.
What is wrong is treating a failed delete as evidence the hook is still there. The postcondition that matters is absence. A failed -D is now followed by -C, and a hook that is not present counts as removed.
-C answers 0 for present and 1 for absent; any other exit, or a failure to run the binary, means the question was not answered, and an unanswered question reads as still-installed. The opposite default would let a broken or permission-denied iptables report a live hook as gone and let the caller flush a chain something still jumps to. Being wrong in this direction only costs a retry.
Mutation-verified: reverting to delete-must-succeed fails the new test.
Two more review findings, both correct, both about teardown reaching a stable state rather than about the commands it issues. Hook ownership could never be given up ---------------------------------------- Ownership is published before the -I that installs a hook, to close the window where a signal would find the rule unpublished. The cost is that the record can name a hook whose insert then failed -- and -D for a rule that was never installed fails too. Clearing the flag only on a successful delete strands exactly that case: the flag stays set, so the chain is never deleted, and every later pass re-runs a delete that cannot succeed. Nothing converges and the chain leaks for the life of the host. The reviewer proposed tracking each hook independently. That is not sufficient on its own: a per-hook flag published ahead of a failed -I sticks in exactly the same way, because the problem is not the granularity of the record but the question being asked of iptables. What matters is whether the hook is absent, not whether the delete succeeded, so a failed delete is now followed by -C and absence counts as removed. -C answers 0 for present and 1 for absent. Any other exit, or a failure to run the binary, means the question was not answered, and an unanswered question reads as still-installed -- the opposite default would let a broken or permission-denied iptables report a live hook as gone. Being wrong in this direction only costs a retry. A referenced chain was flushed before it was deleted ----------------------------------------------------- -F ran whenever this manager had created the chain, without regard to whether a hook still pointed at it. -X failing on a referenced chain is harmless, but -F succeeds regardless of who references it, and an empty user chain returns to the caller instead of reaching its closing DROP. A container whose hook survived teardown therefore kept running with the jump intact and nothing behind it -- fail-open for as long as it lives, which for a preserved container is indefinitely. Both commands are now gated on the hooks being confirmed gone. The chain stays populated and stays published, so the intermediate state fails closed and a later pass retries. Extracted teardown_chain so the ordering is testable without iptables; the previous shape had no test, and a mutant that flushed unconditionally survived the whole suite. 81 tests pass. Both fixes mutation-verified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/backends/lxc/common/src/network_iptables.rs:872
- The ownership update is not atomic with chain creation. A signal can wake the watchdog while
iptables -Nis running or after it succeeds but beforepublish_createdacquires the mutex; the watchdog then snapshots an empty record, exits the process, and leaves the newly created chain unowned. Teardown has the inverse stale-snapshot window before residual publication. Serialize each firewall mutation and its ownership transition with the watchdog snapshot lock so the registry always matches netfilter state.
Self::run_iptables(&["-N", &self.chain_name], logger)?;
created.v4_chain = true;
Self::publish_created(created);
tests/configs/lxc_network_proxy.json:6
- These “blocked” checks treat every connection failure as proof that the firewall dropped the packet.
1.1.1.1may be unroutable or not return HTTP, an assigned global IPv6 address does not prove a working route to the IPv6 target, andnslookupcan fail for reasons unrelated to filtering. The test can therefore pass with the hooks absent. Use locally controlled, baseline-verified listeners/resolvers on destinations that the policy should deny before asserting that policy application makes them unreachable.
"commandLine": "ok=PROXY_FAIL; if wget -qO- --timeout=10 http://sentinel.invalid/ 2>/dev/null | grep -q MXC_PROXY_SENTINEL; then ok=PROXY_OK; fi; echo \"$ok\"; if http_proxy= https_proxy= HTTP_PROXY= HTTPS_PROXY= all_proxy= ALL_PROXY= wget -qO- --timeout=5 http://1.1.1.1/ >/dev/null 2>&1; then echo DIRECT_IPV4_LEAK; else echo DIRECT_IPV4_BLOCKED; fi; if ip -6 addr show scope global 2>/dev/null | grep -q inet6; then if http_proxy= https_proxy= HTTP_PROXY= HTTPS_PROXY= all_proxy= ALL_PROXY= wget -qO- --timeout=5 'http://[2606:4700:4700::1111]/' >/dev/null 2>&1; then echo DIRECT_IPV6_LEAK; else echo DIRECT_IPV6_BLOCKED; fi; else echo DIRECT_IPV6_SKIP_NO_STACK; fi; if nslookup example.com >/dev/null 2>&1; then echo DNS_LEAK; else echo DNS_BLOCKED; fi"
src/core/wxc_common/src/proxy_env.rs:155
- Returning
trueunconditionally changes every LXC run with an emptyprocess.envto pass--clear-env, even when no proxy is configured.docs/lxc-support/lxc-backend.md:94-98still promises that an empty/absent environment preserves host inheritance. Update that public behavior documentation in this PR so users do not rely on inheritance that no longer occurs.
// Always return true so the caller emits --clear-env even when the input
// was empty — an empty env must still prevent lxc-attach from inheriting
// the full MXC host process environment (including proxy vars and tokens).
env.retain(|entry| !is_managed_proxy_key(env_key(entry)));
true
docs/lxc-support/lxc-backend.md:153
- This describes both host lists as silently dropping IPv6 results, but
resolve_blocked_hostsnow aborts setup whenever an entry yields no IPv4 address. It also implies all IPv6 egress follows IPv4 filtering, while underdefaultPolicy: allowthe v6 chain contains only ACCEPT and resolved blocked hosts remain reachable over IPv6. Document the distinct allow/block behavior and the default-allow limitation accurately.
**IPv4 only for host lists.** 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. The parallel ip6tables chain still carries the
default stance, so IPv6 egress is dropped whenever IPv4 egress is.
docs/lxc-support/lxc-backend.md:122
- The table does not state that proxy mode ignores
allowedHostsandblockedHosts; readers can reasonably infer that these rows combine. Since the implementation intentionally discards both lists whenever a proxy endpoint exists, make that precedence explicit in the proxy row.
| `proxy` | ACCEPT for the proxy endpoint only, then DROP |
teardown_created already computes, per family, whether the FORWARD hook delete succeeded -- but the flush and delete that follow ignored it. -F succeeds regardless of who references the chain, and an emptied user chain returns to its caller instead of reaching its own closing DROP, so flushing a still-hooked chain unfilters a container that may still be running. The -X would have failed anyway, since iptables refuses to delete a referenced chain, so the flush bought nothing and cost the container its filtering. Gate the whole step -- flush included -- on that family's hook being confirmed gone, and keep the chain published so a later pass retries. The gate is per family because the two chains live in different tables and are referenced independently. This is inherited from main, which flushes unconditionally, but this branch rewrote the block and doubled the exposure by adding a second address family. microsoft#632 and microsoft#633 fix the same fail-open in the same file; teardown_chain is deliberately identical to the one microsoft#632 landed, so whichever merges second resolves to a no-op.
…2830559) (#724) * [LXC] Filter IPv6 destinations and CIDR ranges in firewall mode (AB#62830559) Firewall mode resolved `allowedHosts` / `blockedHosts` to IPv4 only. On a dual-stack host, traffic to the same destination over IPv6 bypassed the firewall entirely, and any CIDR entry (v4 or v6) failed to parse as an address, then failed DNS resolution, and was dropped. Changes, all confined to the LXC backend: - `resolve_host` returns IPv4 and IPv6 destinations separately. Hostnames resolve to both A and AAAA records; bare literals and validated CIDR blocks pass through in their own family. - `destination_family` validates CIDR syntax and prefix length (<=32 for IPv4, <=128 for IPv6). Malformed entries are reported as unresolved and skipped rather than handed to iptables, which would reject them at apply time and abort setup for the whole policy. - IPv4 rules go to `iptables`, IPv6 rules to `ip6tables`, with parallel per-container chains and FORWARD hooks. - `ip6tables` is probed once. When it is missing or IPv6 is disabled in the kernel, the IPv4 chain is still applied and the number of unapplied IPv6 rules is logged, instead of failing a policy that worked before dual-stack support. - Setup failures after partial chain creation are rolled back, and teardown removes both families' hooks and chains. Scope: this covers the IPv6 + CIDR item of AB#62830559 only. Port and protocol filtering are not included -- they require structured egress rules in the config schema (AB#62830582), which is not in main. Tests: 8 new unit tests for family routing, CIDR pass-through, prefix and syntax rejection, and allow/block ordering; 2 integration configs and scripts wired into run_lxc_all_tests.sh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Add spec-derived unit and E2E tests for IPv6/CIDR filtering (AB#62830559) Tests were written black-box from roadmap item 19, AB#62830559 and the public doc comments, without reading network_iptables.rs, so they pin the specified contract rather than the current implementation. Unit tests (24 new, in two child modules of network_iptables): resolution/CIDR contract - family routing, CIDR passthrough, host bits not required to be zero, prefix bounds at 0/32 and 0/128, malformed syntax, IPv4-mapped IPv6, dual-stack hostname resolution; and rule generation - per-family bucketing, ACCEPT/DROP mapping, allow-before-block ordering in both families, family-agnostic base rules, chain-name cap. E2E: lxc_network_dualstack_hostname covers hostnames with both A and AAAA records (the bypass this work item fixes) alongside mixed-family literals and CIDRs; lxc_network_cidr_boundary covers /0, /32, /128, non-zero host bits and the previously untested defaultPolicy=allow path. Both wired into run_lxc_all_tests.sh. No change to wire.rs, models.rs, config_parser.rs, schemas/ or sdk/. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Close a false negative in the IPv6 DNS resolution tests (AB#62830559) Mutation testing showed that inverting the DNS branch so AAAA records are pushed into the IPv4 bucket - the exact dual-stack bypass this work item fixes - left the suite green. The only hostname test used localhost, which resolves to 127.0.0.1 only on many hosts, so the v6 arm of the DNS path was never executed. Adds a family-purity invariant asserting every destination in a bucket belongs to that bucket's family, exercised over well-known dual-stack names. It now kills that mutation. All 9 mutations tried against the module are caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Restore the quarantined CIDR prefix test instead of relaxing it (AB#62830559) A spec-derived test asserting that '10.0.0.0/+24' is rejected was failing. It was rewritten to assert the current behaviour instead of being left as a finding, which is the wrong resolution: whether MXC should accept a permissive prefix spelling in a security policy file is a design decision, not something to settle by editing the test. The original assertion is restored verbatim and marked #[ignore] so the finding stays visible in test output pending a decision. The separate assertion that a leading '+' cannot smuggle an out-of-range prefix past the family bound check is kept as a passing test, since prefix bounds are unambiguous. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Fix empty-host and plus-prefix destination parsing (AB#62830559) Two defects found by a coverage audit of this branch, both caught by spec-derived tests written black-box against the roadmap contract. resolve_host("") fell through to DNS resolution, where format!("{}:0", host) produces ":0". Winsock resolves that to every local interface address, so an empty allowedHosts entry emitted rules for the host's own LAN and link-local addresses. glibc rejects it, so this reproduced only on Windows -- it turned CI red on windows/x64 and windows/arm64. config_parser assigns host lists verbatim, so an empty string does reach resolve_host from a policy file. destination_family validated the CIDR prefix with u8::from_str, which accepts a leading '+'. 10.0.0.0/+24 was forwarded to iptables, which silently canonicalizes it to 10.0.0.0/24, so a policy typo was applied instead of being reported by the unresolved-host warning that run_lxc_network_invalid_cidr_test.sh exists to guarantee. The prefix must now be ASCII digits, which also subsumes the embedded-slash case. The test for this was previously quarantined pending a bad-code/bad-test ruling; the ruling is bad code, so it is now un-ignored. Also adds lifecycle tests pinning three behaviours a cargo-mutants run proved were unpinned: a new manager reports no rules applied, a non-firewall enforcement mode is a successful no-op, and the enforcement-mode gate is not inverted. The last matters most -- an inverted gate would silently skip all filtering while reporting success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Make the four network E2E scripts actually run (AB#62830559) None of these four scripts had ever executed a single firewall assertion since they were added. Two independent causes: - every config declared "version": "0.4.0-alpha", but the parser accepts >=0.6 <=0.8, so each run died at config parse - lxc-exec buffers diagnostics unless --debug is passed, so the log lines the scripts assert on were never emitted even after the version bump Bumps the configs to 0.6.0-alpha, matching the sibling LXC configs, passes --debug, and adds post-run iptables/ip6tables assertions that the per-container chain is torn down rather than leaked. Verified by running all four as root under WSL: each creates a real container, programs real v4/v6 chains, and cleans up. Assertion liveness was confirmed by flipping defaultPolicy in a config and observing exit 1 with "FAIL: default-deny policy was not applied." Also normalizes lxc_network_ipv6_cidr.json to LF; it was the only one of the four committed with CRLF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd * [LXC] Harden iptables enforcement backend (AB#62830559) Address four PR #724 review threads that are an interwoven refactor of the same enforcement path in network_iptables.rs: - Resolve each allow/block destination exactly once. The apply path previously resolved a host for the unresolved-host warning and then a second time inside rule construction; two lookups of the same name can disagree under DNS round-robin or a TTL expiry, so the installed rule need not match the logged one. build_policy_rules_logged now resolves once and reuses that result for both. The pure builders that resolve are gated to test-only. - Extract enforcement_mode_uses_firewall as a pure predicate and test it directly, instead of the lifecycle test invoking apply_firewall_rules (which shells out to the host firewall) for the Firewall and Both cases. - Fail closed when IPv6 is active but ip6tables is unusable. The old boolean probe skipped IPv6 for every ip6tables failure, marking the policy applied while IPv6 egress went unfiltered. classify_ip6tables_status now distinguishes a kernel with no active IPv6 (safe to skip) from an IPv6-capable host whose ip6tables is missing or broken (setup fails). host_has_active_ipv6 reads /proc/net/if_inet6, which the kernel populates only when the IPv6 stack is loaded and addresses exist. - Track which per-family chains and FORWARD hooks each attempt created and roll back only those. Rollback previously tore down chains unconditionally, and since chain names truncate at 20 characters a partial-failure rollback could delete a chain belonging to a different container. teardown_created acts on the recorded CreatedResources. Also log a positive confirmation when a FORWARD hook is installed, so the E2E scripts can assert on it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [LXC] Query iptables tables directly in E2E cleanup probes (AB#62830559) The four LXC network E2E scripts probed for a leftover chain with `sudo -n iptables -S`. Under `sudo -n`, a host without passwordless sudo fails the probe for a reason unrelated to whether the chain exists, so the cleanup assertion could pass without ever having checked. The LXC suite already requires root (run_lxc_all_tests.sh), so query iptables and ip6tables directly instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [LXC] Assert the FORWARD hook was installed in E2E scripts (AB#62830559) All four LXC network E2E scripts could report PASS while the per-container chain was never hooked into FORWARD: the code emits a skipped-hook warning that nothing checked, so an undiscovered veth silently enforced nothing. Each script now fails on the "Skipping FORWARD hook" warning and requires the positive "FORWARD hook installed" confirmation before reporting PASS. This pairs with the confirmation log line added to the enforcement backend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [LXC] Remove external network dependency from CIDR boundary test (AB#62830559) The boundary fixture ran `wget -qO- https://api.github.com/zen`, so the test failed whenever the network or the remote host was down, independent of the code under test. Replace it with the local success command `true` so a non-zero lxc-exec status reflects a firewall-setup failure on the boundary-valid prefixes rather than an unrelated outage, and note that in the script's status check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [LXC] Bind rulegen ordering tests to the shipping policy-rule path The #[cfg(test)] build_policy_rule_args reimplemented allow-before-block ordering as two separate loops, so the rulegen ordering specs asserted against a duplicate that production never runs. A future change to emission order in build_policy_rules_logged (the AB#62830341 deny-precedence work) would have left those ordering tests green while shipping first-match-wins. Make build_policy_rule_args a thin test-only shim that delegates to the shipping build_policy_rules_logged with a throwaway buffer logger, so the ordering assertions bind to production code again. No test cases added or changed. Move the deny-precedence contract docstring onto the shipping function it now guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add exhaustive black-box unit tests for classify_ip6tables_status Closes the testability gap flagged in the type doc comment: the function was extracted to be pure so the fail-open vs fail-closed decision could be unit-tested without a privileged Linux host, but had zero tests. Nine tests, all derived from the documented contract only: - Exhaustive 4-case truth table (both boolean inputs x all combinations) - Invariant: working probe always yields Available - Invariant: failed probe never yields Available - Security invariant: UnusableButIpv6Active is reachable only when probe=false AND ipv6_active=true; unreachable under every other input - Invariant: KernelIpv6Disabled reachable only when probe=false AND ipv6_active=false - Discriminant-distinctness: all three variants are distinct under PartialEq All 6 mutations (swap outcomes, fail-open collapse, fail-closed collapse, probe invert, ipv6_active invert, always-Available) are caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [LXC] Make IPv6 network tests fail when behavior is removed (PR 724 review) Address test-honesty defects found by evidence-based review; every fix is proven with mutation testing (before: mutant survives; after: mutant caught). Task 1 -- DNS bucket test was vacuous. The family split is factored into a pure `bucket_resolved_addrs` and the AAAA-in-v6-bucket test now injects addresses and asserts the v6 bucket is non-empty and family-pure, so deleting the IPv6 result path fails it. A separate live characterization asserts only the purity invariant, with no warning-that-still-passes. Task 2 -- failure to read IPv6 state was treated as confirmed inactivity. Factored the parse/classify into pure `classify_host_ipv6_state` (file content and read-error as input) and `ipv6_state_treated_as_active`. A NotFound read (IPv6 disabled) stays a confirmed negative; any other read error is Unknown and treated as active so it fails closed instead of leaving IPv6 egress unfiltered. Loopback-only `::1` on `lo` no longer counts as active. Added spec tests for the whole mapping (previously untested). Task 3 -- E2E scripts could count skips as passes. The aggregate runner now treats exit 77 as SKIPPED (never PASS) and flags a run that executed nothing; each script honestly skips on missing root/iptables/ip6tables/LXC/binary. Added assertions on the actual programmed destination rules (via a new per-rule debug log) so deleting destination-rule emission fails the scripts, and the dual-stack script now asserts a positive IPv6 rule exists, including a hostname-derived AAAA rule when external DNS is available. Task 4 -- corrected docs to describe the three-way ip6tables classification (Available / KernelIpv6Disabled / UnusableButIpv6Active) and that an active host with unusable ip6tables fails setup rather than skipping IPv6. Runtime behavior of the shell E2E scripts is UNVERIFIED here (Windows; no root/iptables/netns); scripts were syntax-checked with bash -n only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on IPv6/CIDR egress filtering Signal-time cleanup now removes only what it created ---------------------------------------------------- `force_cleanup` runs on the watchdog thread and had no access to the manager's `CreatedResources`, so it hardcoded `v4_chain: true` and `v6_chain: true`. On a signal that arrived before the chains existed it issued `-F`/`-X` against names it did not own, and because chain names truncate at 20 characters that name can belong to a different container. `CreatedResources` now lives in `signal_cleanup::ActiveSandbox` alongside the container name and veth, behind the same mutex, so the watchdog takes one coherent snapshot and can never pair one container's identity with another's ownership record. Every creation site publishes incrementally, so a signal arriving mid-apply still sees the half-built set instead of an empty one. `force_cleanup` takes the record as a parameter and returns immediately when it is empty, running zero iptables commands. `teardown_created` now returns the residual set — ownership bits are cleared only when the removal command actually succeeds — so a failed removal is not recorded as a completed one. IPv4-mapped IPv6 destinations no longer fail open ------------------------------------------------- A dual-stack socket sending to `::ffff:1.2.3.4` makes Linux emit a real IPv4 packet, so an `ip6tables -d ::ffff:1.2.3.4` rule never matches and a mapped `blockedHosts` entry silently failed open under an allow default policy. Mapped literals and mapped CIDRs are now rewritten to their IPv4 form and filed into the IPv4 bucket. Prefixes shorter than /96 span outside the mapped range and are deliberately left as IPv6. An unreadable /proc is no longer a confirmed "IPv6 is off" ---------------------------------------------------------- `NotFound` on `/proc/net/if_inet6` now maps to `Inactive` only when `/proc/net` itself exists. In a mount namespace without `/proc` the answer is `Unknown`, which fails closed. Review housekeeping ------------------- Configs added by this change now declare `0.8.0-alpha`, matching `CURRENT_SCHEMA_VERSION`. This selects an existing schema version; it does not modify any schema. The five `#[path]` spec-test files are folded into the inline `mod tests` in `network_iptables.rs`, matching the repo convention (123 inline test modules against 5 `#[path]` files, all five of which this change added). The enforcement-mode test asserted the production predicate against a second copy of the same predicate, so both could be wrong together. It now asserts literal expected values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * Test the signal-time ownership guard rather than its predicate The existing test asserted that an empty CreatedResources reports itself as empty, which is a property of the record and not of the code that consults it. Deleting the guard in force_cleanup left every test passing while restoring the exact failure the record was added to prevent: a process that created nothing tearing down the chain of a concurrent start that did. The new test drives force_cleanup itself and reads the log as the observable, since the teardown announces the chain by name before it touches anything. A positive control with one resource published proves the assertion discriminates between the two cases rather than watching a permanently silent function. Verified by mutation: with the guard deleted the test fails on the expected assertion, and it passes with the guard restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * Keep ownership of iptables state a failed rollback could not remove apply_firewall_rules_inner rolled back what it created, but discarded the result of that rollback with `let _ =`. A removal command can itself fail, so the outer error arm then left self.created empty and rules_applied false while a chain or FORWARD hook was still installed. rules_applied gates both remove_firewall_rules and Drop, so nothing afterward knew the survivors were ours and the leak was permanent for the life of the process. The inner call now returns the residual alongside the error, and the outer arm adopts it: ownership is retained exactly when something survived, and the two cases log differently so the distinction is visible in a failure report. The signal path was already covered, because teardown_created publishes the residual before returning; this closes the ordinary Drop and remove path. Mutation-tested: discarding the residual again makes the new test fail on the retention assertion rather than passing quietly. The test drives remove_firewall_rules and observes the log rather than asserting rules_applied, and carries a negative control so it cannot pass by retaining unconditionally. Not verified: no live iptables. On this Windows host every firewall command fails, so the residual path is exercised through the ownership record and the logger rather than against real rule state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * Keep ownership when the removal path's own commands fail The previous commit fixed this for the failed-apply rollback but left the same defect on the ordinary removal path, and the reviewer caught it there. remove_firewall_rules cleared rules_applied unconditionally, even when teardown_created reported a non-empty residual. Since Drop is gated on that same flag, a teardown whose commands failed reported itself done and threw away the last retry, leaving the chain installed for the life of the process. Both paths now share retain_residual_ownership, which keeps the gate open exactly when something survived. They have the same obligation, so having one of them get it right and the other not was the underlying problem. Mutation-tested: clearing the flag unconditionally again makes the new test fail. The test drives a second removal -- the call Drop makes -- and observes the log, because a closed gate short-circuits before the teardown announces the chain. Not verified: no live iptables. On this Windows host every firewall command fails, which is what makes the non-empty residual reachable in a test at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * Refuse a second apply while the first still owns firewall state An independent review of the previous two commits found that both arms of `apply_firewall_rules` replace `self.created` with the current attempt's set. A manager that already owned resources and was asked to apply again would therefore drop the earlier record. If the second attempt then failed before creating anything, `retain_residual_ownership` would overwrite the record with an empty set and clear `rules_applied`, so `Drop` skipped cleanup and whatever the first attempt left behind was stranded permanently. The same empty record was published to the signal registry, so the watchdog lost it too. Every production caller builds a manager immediately before its single apply, so this is not reachable today. That is exactly why it is worth closing now: the invariant is currently held by convention at four call sites rather than by the type, and nothing tells the next caller. Refusing the second apply makes it unreachable by construction instead. The guard keys on live ownership rather than on having ever applied, so a manager whose removal succeeded can still be reused. The negative control test covers that: a fresh manager must reach its commands rather than trip the gate. Mutation-verified. Weakening the guard to `self.rules_applied && false` fails `a_second_apply_is_refused_while_the_first_still_owns_resources` rather than passing quietly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * [LXC] Test the arm that adopts a failed rollback's residual The existing test for residual ownership starts from a residual that has already been retained: it calls retain_residual_ownership itself and then checks teardown runs. That covers the mechanism but not the decision. The failure arm of apply_firewall_rules could discard the residual it was handed and the test would still pass, because it never goes through that arm. That branch is also the one least likely to be reached by accident. On any host without iptables the inner apply fails on its first command, so it rolls back nothing and reports an empty residual -- the interesting case needs a rollback whose own removal command failed, which no unit test can produce by running the real thing. Extracted the recording step as record_apply_outcome so a test can hand it exactly that outcome, and added a test that drives it and then asserts the downstream teardown still names the chain. A negative control covers the clean-failure side, so the assertion cannot be satisfied by retaining unconditionally. Mutation-verified, and the mutation is what makes the point: making the failure arm drop the residual kills the new test and leaves the old one green. 112 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 * [LXC] Stop flushing a chain FORWARD still jumps to teardown_created already computes, per family, whether the FORWARD hook delete succeeded -- but the flush and delete that follow ignored it. -F succeeds regardless of who references the chain, and an emptied user chain returns to its caller instead of reaching its own closing DROP, so flushing a still-hooked chain unfilters a container that may still be running. The -X would have failed anyway, since iptables refuses to delete a referenced chain, so the flush bought nothing and cost the container its filtering. Gate the whole step -- flush included -- on that family's hook being confirmed gone, and keep the chain published so a later pass retries. The gate is per family because the two chains live in different tables and are referenced independently. This is inherited from main, which flushes unconditionally, but this branch rewrote the block and doubled the exposure by adding a second address family. #632 and #633 fix the same fail-open in the same file; teardown_chain is deliberately identical to the one #632 landed, so whichever merges second resolves to a no-op. * [LXC] Keep the firewall unit tests off the host's iptables The unit tests drove force_cleanup, remove_firewall_rules, and apply_firewall_rules all the way down to Command::new, so run as root they flushed and deleted whatever live chain answered to a colliding MXC-<name>. Chain names sanitize and truncate to 20 characters, so a collision with a running container is reachable rather than theoretical. Worse, the tests that need an iptables command to fail never arranged it. They inherited the failure from the host, which is why one of them carried the comment "on this host every iptables command fails" -- an outcome that reverses on a host where iptables works. Add an opt-in interception point in run_firewall_command, the single place where the argv is complete and the last one before the spawn, backed by thread-local storage because the firewall entry points are associated functions with no self to carry a runner and cargo test runs its threads in parallel. A test that installs no fake reaches the real binary exactly as before, so the ~70 tests in this file that never touch a firewall command are unaffected and the real-binary path is preserved for integration. This placement reaches Drop and force_cleanup, which a runner passed as a parameter cannot, and it adds no field to NetworkIptablesManager -- which matters because SandboxProcess requires Send and BwrapChild holds one. The six affected tests now assert the command sequence directly instead of scraping the log, and script their own failures. That also makes the -X success arm reachable for the first time: a chain whose delete succeeds is released and Drop finds nothing to retry. The negative controls keep their empty-log assertion alongside the new empty-argv one. The log assertion is load-bearing: with an empty ownership record no command is issued either way, so an argv-only assertion would not catch deletion of the is_empty() guard. Both were confirmed by mutation. ip6tables_status gets the same treatment, since its read-only probe is a second spawn that the apply path reaches before any chain exists. A fake always reports the tool available, which the classifier maps to Available without consulting the host; reporting it unusable would not be host-independent, so those branches stay covered by the pure-function tests of classify_ip6tables_status. Addresses review feedback on network_iptables.rs:1228. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Darren Hoehna <Darren.Hoehna@microsoft.com> Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6 Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
|
Closing this PR. The model-2 work is being re-cut from Two things drove that, and the review backlog is the bigger one. This branch carries 63 review comments across 33 commits, and 52 of them are substantive design or correctness objections rather than nits. At 3,593 insertions across 17 files, each individual correctness fix became hard to verify in isolation, which is the same failure that ended #633. Separately, the branch no longer merges with The review comments here are the input to the re-cut, not discarded. Six substantive themes were extracted and are being carried forward as design constraints on the new work:
The first slice is already up: #788 covers the proxy environment-variable hygiene (roadmap requirement 4) — scrubbing every caller-supplied proxy variable so a workload cannot redirect or disable the cooperative proxy through its own environment. It is deliberately small and carries no firewall code. Still to be re-cut, each as its own PR: the proxy URL and address model, loopback proxy validation, the The branch itself is preserved and nothing is lost. |
Linked work item: AB#62830341 — [LXC] Address network policy gaps - model 2 (deny-all-except-proxy)
Summary
Implements the deny-all-except-proxy network model for LXC: when a proxy is configured, the proxy
address:portis the only destination the container can reach and everything else is dropped, in bothiptablesandip6tables. The posture is fail-closed by design.iptablesandip6tablesso a dual-stack container cannot slip out over IPv6 while the v4 chain drops everything.allowedHostsandblockedHostsdo not apply in proxy mode. When a proxy is configured the chain contains only the proxy ACCEPT and the default DROP, so blocked hosts stay blocked and hosts named inallowedHostsbecome unreachable. Reaching anallowedHostsdestination alongside a proxy is not supported under model 2; the proxy is the single permitted egress. This is the one behavior change a caller could notice as a loss of reachability, so it is called out explicitly rather than left to be discovered.-i <veth>in FORWARD and INPUT. Container-originated packets arrive on the host-side veth, so egress is matched by input interface. FORWARD alone leaves host-destined packets routed through INPUT reachable, so a matching INPUT hook filters container→host traffic; a host-local proxy is still permitted by the shared chain's own ACCEPT rule. DHCP (udp/67,udp/547) is accepted ahead of the jump so lease renewal survives.lxc_runner, not the shared manager. The iptables manager is shared with Bubblewrap, which runs in the host network namespace and has no veth. When no veth is set it builds the policy chain and skips the veth-scoped hooks instead of rejecting, so a Bubblewrap firewall request still startsbwrap. LXC's fail-closed invariant — refuse to start when firewall enforcement is needed but the veth cannot be discovered — lives inlxc_runner(reconcile_veth). The residual Bubblewrap gap, where a policy chain exists with nothing hooked to it, is tracked in [LXC/Bubblewrap] Firewall mode fails open when no veth interface is set (no FORWARD hook) #755.HTTP(S)_PROXY, so the firewall rule and the container's proxy setting name the same endpoint. Because the container never needs a resolver, port 53 stays shut in proxy mode and there is no DNS-tunnel exfil path. An unresolvable proxy host fails setup rather than yielding a chain that drops everything.https://proxy URL to an IP leaves the client validating the proxy's certificate against an IP with no SNI, which fails unless the certificate carries a matching IP SAN. Setup refuses with an error naming both remedies: address the proxy by IP, or use anhttp://proxy URL, which still reacheshttpsdestinations through CONNECT.ip6tablesis unusable but the host has a live IPv6 stack and the policy's v6 stance is DROP, setup fails instead of silently handing the container unfiltered IPv6. A host with no IPv6 stack still enforces the IPv4 policy alone.blockedHostsentry fails the start. It produces no DROP rule, so the host the policy names stays reachable while setup reports success. This is fatal under both default policies. Default-allow is the obvious case, where the chain falls through to ACCEPT. Default-block is not safe either, for a reason that is easy to miss: outside proxy mode the chain carries an unscoped port-53 ACCEPT emitted after the block rules, so under first-match-wins a blocked host with no DROP of its own is still reachable on port 53 — the closing DROP covers every other port, not that one.allowedHostsstays best-effort in both cases, because an unresolved allow only ever removes reachability.HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/FTP_PROXY/NO_PROXY(plus lowercase, matched case-insensitively) are scrubbed before the configured values are set, andNO_PROXYis forced empty so an inherited exemption cannot steer traffic around the proxy. With the proxy disabled,apply_proxy_envreports that the environment must be cleared, so--clear-envis emitted even when the request carried no environment at all.network.proxy.builtinTestServer,network.proxy.localhost, and loopback URL literals (127.0.0.0/8,::1,localhost) are rejected because they name the container's own loopback, not the host. IPv6 proxy endpoints are rejected because the proxy rule is emitted with IPv4iptablesonly. A routable IPv4network.proxy.urlis required.Ownership of installed rules
A partly-failed setup or teardown must leave the record of what exists exactly matching what exists, because that record is what the error path,
Drop, and the signal watchdog all act on.-Dleft the later hooks installed while the failure was reported once; the deletes are veth-scoped, so attempting all of them cannot touch another container's rules.-Ithen failed — and-Dfor a rule that was never installed fails too. Clearing only on a successful delete left such a flag set forever: every retry re-ran a delete that could not succeed, the chain stayed owned but unremovable, andDropnever converged. A failed delete is now followed by aniptables -Cexistence check, and a hook that is not there counts as removed. An unanswerable check reads as "still installed", because being wrong that way costs a retry while the opposite lets a broken or permission-denied iptables report a live hook as gone.-Fsucceeds regardless of who references the chain, and an emptied user chain returns to its caller instead of reaching its own closing DROP, so flushing a still-hooked chain unfilters a container that may still be running. The-Xwould have failed anyway, since iptables refuses to delete a referenced chain, so the flush bought nothing and cost the container its filtering. The whole step — flush included — is gated on the hooks being gone, and the chain stays published so a later pass retries.Merge-order constraints with #724 and #633
network_iptables.rsand conflict. Whoever merges second must resolve keeping this PR's deny-before-allow ordering; resolving toward [LXC] Filter IPv6 destinations and CIDR ranges in firewall mode (AB#62830559) #724 regresses deny-wins precedence.teardown_chainis byte-identical to this one, so that hunk resolves to a no-op. [LXC] State-aware lifecycle management #633 reaches the same outcome by re-readingFORWARDinstead of trusting the delete's exit code. Either resolution is correct; neither may be resolved by takingmain's unconditional flush.Known gaps
container.start()and the firewall hook installation. Closing it needs a veth-scoped quarantine chain installed before start and swapped atomically for the real policy, which is a new enforcement stage rather than a reordering. Tracked in [LXC] Container runs unfiltered between start and firewall hook installation #764. ([LXC] State-aware lifecycle management #633 installs the firewall before start for the state-aware path; this branch's runner path is unchanged.)defaultPolicy: allowplusblockedHosts, hosts that do resolve remain reachable over IPv6, because host rules on this branch resolve to IPv4 addresses only. [LXC] Filter IPv6 destinations and CIDR ranges in firewall mode (AB#62830559) #724 adds A/AAAA resolution and buckets each resolved address into the matchingiptablesorip6tablesrule set.Validation
Re-verified on this branch (
10f0599) on the Windows dev box:cargo test -p lxc_common -p wxc_common --lib— 683 passed (lxc_common 81, wxc_common 602), 0 failed, 0 ignored. This is a Windows run, so it includes the Windows-gated tests a Linux run excludes.cargo clippy -p lxc_common -p wxc_common --all-targets -- -D warnings— clean.cargo fmt --all -- --check— clean.ordered_egress_rules_put_deny_before_allow), the pure veth reconciliation decision (reconcile_veth), attach-argument /--clear-envconstruction, and IPv6 bracketing. The live INPUT hook, the DHCP carve-out,ip6tablesenforcement, real iptables enforcement, real signal delivery, and TLS behavior are not exercised by the unit tests.remove_every_hook,teardown_chain) rather than through the real path, and that is a necessity rather than a preference: on Windows every firewall command fails at spawn, which is before anything is logged, so the logger observes nothing and a teardown's residual always equals what was created. A test driving the real path could not observe an ordering at all. What the seams cannot reach is the wiring that feeds them, or a real-Xsucceeding.tests/scripts/run_bwrap_network_firewall_test.shandtests/scripts/run_lxc_network_proxy_test.shrequire Linux and root; they exit 77 (Automake skip) when root, LXC,bwrap,iptables, or the built binaries are missing, and the aggregate runners count that as a skip, not a pass. They are not executed on the Windows dev box.Microsoft Reviewers: Open in CodeFlow