[LXC] Enforce the deny-all-except-proxy network policy (model 2) - #798
Open
Darren Hoehna (dhoehna) wants to merge 17 commits into
Open
[LXC] Enforce the deny-all-except-proxy network policy (model 2)#798Darren Hoehna (dhoehna) wants to merge 17 commits into
Darren Hoehna (dhoehna) wants to merge 17 commits into
Conversation
LXC did not scrub proxy environment variables from caller-supplied env, so a caller could point a sandboxed process at an egress path the network policy never authorized, or disable the cooperative proxy outright. Add `apply_proxy_env` to `wxc_common::proxy_env`, the LXC entry point. It delegates to `apply_cooperative_proxy_env` so LXC scrubs and sets exactly the same key set as Bubblewrap and WSLc rather than maintaining a parallel list that can drift. With the proxy disabled the vars are still stripped. It returns `true` unconditionally, including for an empty env: the return value tells the caller to emit `--clear-env`, and an empty vector must still stop `lxc-attach` inheriting the MXC host process environment, which carries both proxy vars and credentials. Add `FTP_PROXY`/`ftp_proxy` to `PROXY_ENV_KEYS`. Both spellings of every family are now present, and the doc comment records why the lower-case duplicates are kept. Tests are black-box integration tests in `tests/proxy_env_spec.rs`, written against the public API by an author who did not see the implementation. All 22 pass; 7 of 7 seeded mutants are caught with no survivors. This is slice 1 of the work previously attempted in PR 632, re-cut from main so each slice is reviewable on its own. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
The test module header described client (a) in the present tense, which read as though the LXC backend already calls `apply_proxy_env`. It does not: the helper has no call site yet, and `attach_run` still derives `--clear-env` solely from `env` being non-empty (`lxc_bindings.rs:90`). Record the divergence while it is cheap to see. `apply_proxy_env` returns `true` even for an empty env so the MXC host environment cannot leak into the container, whereas current code emits no `--clear-env` in that case and pins the behavior with a test at `lxc_bindings.rs:743`. The integration slice has to update both. Comment only. No assertion changed; the tests validate the helper contract, which is what they are for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Model 2 needs the sandbox and the firewall to agree on exactly one proxy endpoint. Otherwise the sandbox re-resolves the hostname itself and, under round-robin or split-horizon DNS, reaches an address the firewall never authorized. PR 632 solved this by rewriting the proxy URL's host to the resolved IP. Review rejected that (comment 3724788051): an `https://`-scheme proxy would then be contacted at an IP literal, so SNI and certificate validation fail unless the proxy certificate carries an IP SAN. Add `ProxyHostPin` and `ProxyAddress::host_pin` instead. These express the mapping as a hosts-file pin, so the hostname stays in the URL and TLS identity is preserved while the endpoint is still forced. `host_pin` returns `None` when the address is already an IP literal, because there is then nothing to resolve. `hosts_line` writes the address bare: a hosts file takes an unbracketed IPv6 literal, unlike a URL host component. Also fix `to_url`. It hardcoded `127.0.0.1` whenever no original URL was recorded, regardless of the actual address. That is reachable: `unix_proxy_coordinator.rs:234` builds a `ProxyAddress` from the configured bind address with no original URL, so a proxy bound to a non-loopback address reported an endpoint it was not listening on -- the same class of defect as the objection above. Every existing caller passes `127.0.0.1`, so their output is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Mutation testing surfaced an equivalent mutant: deleting the
`starts_with('[')` early return from `bracket_if_ipv6` changed no
observable behavior. Verified why, rather than assuming the tests were
weak -- `IpAddr::from_str` rejects brackets, so `[::1]` already fell
through the catch-all arm unchanged and could never be bracketed twice.
The guard was dead code. Remove it and record the reason.
The mirror case is NOT dead, and mutation proves it: replacing
`Self::unbracket(&self.address)` in `host_pin` with the raw field
fails a test. Unbracketing there is what lets a bracketed IPv6 literal
be classified as a literal instead of pinned as though it were a
hostname. Say so in the doc comment, which previously described it as
mere normalization.
Comment and dead-code only. All 566 library tests and 19 spec tests
pass unchanged, and the seeded-mutant suite now runs 9 for 9 with no
survivors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Review on PR 789 pointed out that ProxyHostPin's fields were public Strings, so a caller could set ip to "[::1]", to the empty string, or to text containing a newline, and hosts_line() would emit it verbatim. That is an injection into /etc/hosts: a newline ends the record and starts a second, unauthorized mapping. The type exists to guarantee the sandbox and the firewall agree on one endpoint, so a value that denotes two mappings defeats its whole purpose. The fields are now private and the address is an IpAddr, so no such value can be constructed. IpAddr also renders IPv6 bare, which is what a hosts file requires -- the difference from to_url, which brackets, is now structural instead of a convention a caller has to remember. host_pin returns Result<Option<ProxyHostPin>, WxcError>. Ok(None) keeps its single meaning: the address is an IP literal, so there is nothing to resolve. An empty or malformed hostname is now Err, not None. Folding it into None would have told the caller "no hosts entry required", so a malformed address would silently skip the pin and let the sandbox re-resolve the name -- failing open, which is the defect review objected to elsewhere in this work. Tests are updated in a separate commit by the author who did not write this implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
22 black-box tests against the new host_pin contract, written by an author who has not read models.rs. The empty address moved from Ok(None) to Err, so the test that covered it was rewritten to match on all three arms by name. Asserting is_err() || is_none() would have passed either way, and the whole point of the change is that those two answers are not interchangeable: Ok(None) tells the caller no hosts entry is needed, which is how a malformed address ends up unpinned and the firewall bypassed. Added coverage for the injection strings review called out -- a hostname carrying a newline or a space must be Err and must never reach hosts_line. Dropped the test that stripped brackets from the ip argument; ip is an IpAddr now, so there is no textual form to strip and the behavior no longer exists. Mutation harness: 11 mutants, 11 caught by a failing test, 0 survivors. Four of them removed the last call to a private helper, which the crate's deny-warnings turns into a build failure -- real detection, but by the compiler, which proves nothing about the tests. The harness now suppresses those lints for the mutated build so the suite has to answer for itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
install_firewall_rules built the full deny-all chain and then, when no veth interface was known, logged a warning and returned Ok(()). The chain is only ever reached from FORWARD via `-i <veth>`, so without that hook nothing traverses it: the caller was told the network policy was applied while zero packets were filtered. That is the worst of the three possible outcomes. Installing the rules host-wide instead would at least filter, but unscoped they would hit every container and the host's own traffic. Returning an error loses nothing, because there was no enforcement to lose. This path is only reachable when the caller explicitly asked for firewall enforcement -- apply_firewall_rules returns early unless the mode is Firewall or Both, and NetworkEnforcementMode defaults to Capabilities. So the change cannot affect containers that never wanted a firewall. Rollback and teardown already handle the Err: apply_firewall_rules_inner converts it into a precise teardown of exactly what was created plus residual ownership, and lxc_runner destroys the container rather than starting a workload that believes it is confined. No existing test pinned the old behavior (115/115 still pass), which is itself the point: the fail-open was untested. The four Linux E2E scripts that exercise firewall enforcement already require "FORWARD hook installed" in the output and fail without it, so veth discovery demonstrably succeeds there and this change is a no-op for every run that passes today. Slice 3 of the PR 632 re-cut. Refs AB#62830341.
Six black-box tests for apply_firewall_rules, written against the documented
contract by an author who did not read the implementation, so they describe
the behavior that was intended rather than mirroring whatever the code does.
They pin:
- refusal when the veth interface is unknown, under Firewall and under Both,
separately, so a fix scoped to one enforcement mode cannot pass
- the error names the chain left unenforced, so an operator has something to
search for
- the negative control: the same policy succeeds once an interface is set.
Without it, an apply that always returned Err would pass every other test
- teardown of the chain created before the refusal, asserted as ordering
against the creation command rather than mere presence
- Capabilities-only containers issue no firewall commands at all, which is
what bounds this change's blast radius
Mutation tested: seven seeded defects, all caught by a failing test, no
survivors. The seeds include restoring the old Ok(()) fail-open, dropping the
chain name from the message, applying the check to Firewall but not Both,
inverting the interface check, skipping rollback, and swallowing the error one
layer up in record_apply_outcome. Each mutant compiles with lints silenced, so
a defect detected only by the compiler counts as a harness failure rather than
a pass -- the tests have to answer for themselves.
Attached as a #[path] child module because the fake-firewall seam is
#[cfg(test)] and private, which an integration test -- a separate crate --
cannot reach.
Slice 3 of the PR 632 re-cut. Refs AB#62830341.
…ters
The per-container chain was hooked into FORWARD with `-i <veth>` only. That
matches nothing whenever the veth is enslaved to a bridge, which is the
default LXC topology: the packet is bridged onto `lxcbr0` and then routed off
it, so FORWARD sees the bridge as the input interface and never the veth. The
chain was built correctly, populated correctly, hooked without error, and
traversed by zero packets.
Measured on a live container before this change, with `defaultPolicy: block`
and no allowed hosts: every counter in the chain read 0, the closing DROP
included, and a fetch from inside the container succeeded. Adding a counting
rule on the same traffic in the same FORWARD chain gave 11 packets for
`-i lxcbr0` against 0 for `-i <veth>`.
Install a second hook per family matching `-m physdev --physdev-in <veth>`,
which identifies the bridge port the packet entered on and so stays scoped to
one container -- matching the bridge itself would apply one container's policy
to every container sharing it. The two rules are mutually exclusive for any
given packet, so a directly routed veth is still carried by the `-i` rule and
nothing is counted twice.
Fail closed on the two conditions that would leave the chain unreachable
again, in the same voice as the missing-veth refusal: a bridged veth whose
`bridge-nf-call-{ip,ip6}tables` toggle is absent or 0, and a bridged veth
whose physdev hook will not install. On a directly routed veth the physdev
rule is redundant, so a kernel without the match warns instead of failing.
Teardown removes both forms, built from the same builders used at insertion so
a delete cannot drift from the insert it has to match, and the chain delete now
waits on both hooks because either surviving one still references the chain.
Verified on a live container: `defaultPolicy: block` with no allowed hosts
now blocks, the same policy with `api.github.com` allowed still reaches it,
all five network E2E scripts pass, and teardown leaves no FORWARD reference
and no chain behind.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Two kinds of test, because the defect this slice fixes was invisible to both kinds the repository already had. The unit specs pin the four seams the hook is built from: the two rule-args builders, bridge-enslavement detection, and the bridge-netfilter toggle read. They are written against the documented contract by an author who did not read the implementation. The guarantees that matter most are that the physdev builder never collapses into an input-interface match, that it names one specific bridge port rather than a wildcard, that a delete specification differs from its insert only by the operation -- iptables deletes by full rule specification, so a drifted delete silently leaks the hook -- and that an absent bridge-netfilter toggle reads as inactive, never as safe. Mutation testing over nine seeded defects, including the exact bug this slice fixes: 9 caught, 0 survivors. The E2E script exists because unit tests cannot see the failure at all. Every existing network script asserts that the FORWARD hook was *installed*, which is a log line; the hook installed cleanly, named the right chain, and matched zero packets. So this script asserts the guarantee instead: a destination the policy does not allow must be unreachable from inside the container, and an explicitly allowed one must still be reachable. The allow case is not decoration -- a blocked-only assertion would also pass on a host with no working network, or on a change that broke egress outright. Verified in both directions on live containers. Against the fixed implementation the script passes. Against the implementation from the parent commit it fails on the deny case with "egress succeeded under a default-block policy with no allowed hosts", which is the regression it exists to catch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
…solvable block The per-container chain emitted allow-list rules before block-list rules, and iptables applies first-match-wins within a chain, so a destination named in both `allowedHosts` and `blockedHosts` was ACCEPTed. A code comment recorded that as interim behavior owned by AB#62830341. Emit the block list first so the deny wins. Emission order is the entire precedence mechanism -- there is no separate resolution pass -- so the comment now says that outright, because swapping the two iterators back would reverse the security semantics without failing to compile. A block entry that resolved to no address programmed no rule and logged only a warning. Where the chain ends in ACCEPT that is a fail-open: the unwritten deny rule was the only thing that would have stopped the traffic, and the apply still reported success. `build_policy_rules_logged` now returns `Result` and errors in exactly that case, so the caller rolls back the chains it created rather than leaving a policy it did not enforce. The error is conditioned on the default policy rather than raised for every unresolvable block entry. Where the chain ends in DROP, an entry that resolves to nothing is redundant rather than missing -- the closing rule already denies every destination the allow list did not name -- and erroring there would refuse to start containers whose blocklists name hosts that do not exist, which is the ordinary case. `tests/configs/lxc_network_test.json` blocks `evil.example.com` under `defaultPolicy: block`, and that name is NXDOMAIN. The two tests that pinned allow-before-block ordering are deleted rather than inverted. They asserted the contract this change replaces, and the replacement assertions belong to the `deny_precedence_spec` module, which is authored separately so that the tests proving this change correct are not written by its author. The family-split test kept its subject and gave up only its incidental dependency on rule sequence. Residual gap, documented in the code rather than papered over: under a DROP default, a sufficiently broad allow entry can still cover a destination whose deny rule went unwritten. Detecting that needs the address the entry failed to resolve to, so no predicate over the policy text can be complete, and a partial check would imply a guarantee this code cannot make. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
…eat a block
The implementation commit changed emission order and made an unresolvable
deny entry fatal under an accepting default. This commit is the evidence
that both hold, written against the documented contract rather than against
the code.
Twelve unit tests in a new spec module, authored from headers only by an
agent that never opened `network_iptables.rs`. The author that wrote the
implementation cannot write its tests: a test derived from the
implementation encodes that implementation's bugs as expected behavior and
will pass forever without catching anything.
The tests assert the contract, not the current output:
- a destination in both lists is dropped, for IPv4, for IPv6, and with
several entries in each list
- every DROP is emitted before every ACCEPT, checked by index rather than
by comparing against a fixed expected vector
- an unresolvable blocked host errors under an accepting default and the
error names the host
- the same unresolvable blocked host does not error under a blocking
default, because the closing DROP already denies it
- an unresolvable allowed host never errors under either default
- an unresolvable entry does not suppress a sibling entry's rule or log line
- v4 and v6 destinations land in their own buckets, asserted by parsing each
destination rather than by matching a known list, so the assertion cannot
be satisfied by an implementation that happens to emit the expected values
Mutation testing supplies the proof that these tests can actually fail.
Nine mutants, each a mistake a person could plausibly make in this function:
restore the old emission order, error on every unresolvable block entry,
error on unresolvable allow entries, never error at all, invert the
default-policy test, swap the jump targets, drop the warning line, leak IPv6
destinations into the IPv4 bucket, and omit the host name from the error.
caught=9 survived=0 harness_bugs=0
source restored byte-identical: True
Mutant 1 is the load-bearing one. Two tests pinning the old
allow-before-block order were deleted in the implementation commit, and a
deletion with no replacement would have dropped coverage silently while the
suite stayed green. Killing mutant 1 proves the replacement exists.
The end-to-end guard runs the real binary against a config whose allowed and
blocked lists both contain `0.0.0.0/0` and `::/0`. Literal CIDRs rather
than a hostname, because a hostname is resolved separately for each list
entry and round-robin DNS could hand back different addresses for the allow
and the deny, making the verdict depend on which address the fetch picked.
The control config is load-bearing. It allows the same destination and
blocks nothing, so it must come back reachable. Without it, a host with no
egress at all would produce the same blocked verdict on the overlap case and
look exactly like a pass.
The guard was verified to discriminate by running it against the previous
commit's binary:
b9946e3 ACCEPT then DROP overlap MXC_NET_ALLOWED guard FAILS, exit 1
447f10f DROP then ACCEPT overlap MXC_NET_BLOCKED guard PASSES
Same script, same host, same configs. The control passed in both runs, so
the difference is the rule ordering and not a host that lost its network.
Gates: 154 unit tests pass, clippy -D warnings clean, fmt clean, all seven
LXC end-to-end scripts pass.
The documentation described a firewall that no longer exists. Slices 3, 4, and 5 changed what happens on a missing veth, how the chains reach FORWARD, and which rule wins when the two host lists overlap, and none of it was written down. Four claims were false against the code: - The policy table left precedence unspecified. It is now deny-wins, and the reason -- first match ends chain evaluation -- belongs in the doc, because the ordering is the whole mechanism. - Unresolvable entries were described as always "reported as unresolved and skipped, leaving the rest of the policy in force". That is now conditional: under an accepting default an unresolvable blocked host is fatal. - The FORWARD hook was described as matching the host-side veth as the input interface. That omits the `--physdev-in` bridge-port rule and the `br_netfilter` requirement, which is precisely the omission that let a populated deny-all chain filter nothing. - "If MXC cannot discover the container veth, it skips the FORWARD hook with a warning" was flatly wrong. That path returns an error and rolls back. An independent review caught three further overstatements in the first draft of this text, all of which were mine and all of which were the comfortable direction to be wrong in: - "A deny always wins" is not true. The base chain accepts UDP and TCP port 53 unconditionally and is installed ahead of the policy rules, so DNS to a blocked destination is accepted before its DROP is reached. Narrowing that needs to know which resolver addresses are legitimate and no schema field carries them, so the honest move is to document the exemption rather than imply a guarantee the chain does not provide. - A hostname appearing in both lists is resolved once per entry, so round-robin DNS can return an address for the allow that the deny never saw. The guarantee holds for addresses, not for names. This was already known -- it is why the deny-precedence E2E guard uses literal CIDRs -- and it still did not make it into the prose. - "Two rules per family" is not unconditional. On a directly routed veth a missing physdev match warns and continues, because the interface rule is the one that matches there. Only on a bridged veth is it fatal. The IPv6 bridge toggle is also checked separately and was not mentioned. ## CI `lxc-e2e.yml` runs the suite on a provisioned Ubuntu runner. Until now no workflow executed these scripts at all, which is much of how a firewall that filtered nothing shipped green: the assertions existed and nothing ran them. The workflow enables `br_netfilter` explicitly. Without it a bridged veth never reaches FORWARD, every rule installs cleanly, nothing fires, and the network tests pass against a firewall that filters nothing -- the exact failure they are supposed to detect. `MXC_LXC_TESTS_REQUIRE_EXECUTION` turns an honest skip into a failure. A developer box legitimately lacks ip6tables or LXC and should run what it can, so a skip stays a warning there. A runner provisioned specifically to execute this suite is different: a skip means a prerequisite disappeared, and without this the gate goes green while testing nothing. Verified by running the suite four ways: normal and strict with prerequisites present both pass, and strict with the binary removed exits 1 naming the six skipped tests rather than reporting success.
The first run of this workflow failed three tests, and the three were the positive controls doing exactly what they exist for. GitHub-hosted runners ship Docker, and Docker sets the IPv4 FORWARD policy to DROP. That broke the tests twice over. Outright: MXC hooks its chain on traffic leaving the container, so an allowed request is accepted on the way out, but the reply arrives in the opposite direction, matches no MXC rule, falls through to the policy, and is dropped. DNS still resolved, because dnsmasq on lxcbr0 is host-local and never traverses FORWARD, so the symptom was a resolved address that then timed out: \wget: can't connect to remote host (140.82.116.5)\. IPv4 only, which matches Docker leaving the IPv6 policy at ACCEPT. And silently: under a DROP policy a container with no working MXC hook at all is equally unreachable, so the deny cases would have reported success against a firewall that filters nothing. That is the exact bug this suite exists to detect and the reason these tests carry positive controls. Without the controls this run would have been a green gate over a dead network. Setting the policy to ACCEPT restores the condition the tests were written for: the host forwards by default, so the only thing that can block container traffic is a rule MXC installed, and a missing hook fails the deny case loudly. A conntrack RELATED,ESTABLISHED rule would have fixed the reply path while leaving the vacuous pass in place, so it is the wrong fix. The environment step now prints both FORWARD policies, because a future runner image that reintroduces DROP would otherwise present as an unexplained timeout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Darren Hoehna (dhoehna)
requested review from
a team
and
a balanced review from Copilot
August 9, 2026 21:36
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
This was referenced Aug 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Strengthens LXC outbound firewall enforcement, deny precedence, proxy handling utilities, and E2E validation.
Changes:
- Adds veth-scoped FORWARD hooks and fail-closed enforcement.
- Emits deny rules before allow rules.
- Adds proxy utilities, tests, documentation, and LXC CI coverage.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/scripts/run_lxc_network_enforcement_test.sh |
Tests effective allow/block enforcement. |
tests/scripts/run_lxc_network_deny_precedence_test.sh |
Tests deny-wins behavior. |
tests/scripts/run_lxc_all_tests.sh |
Adds tests and strict CI mode. |
tests/configs/lxc_network_enforcement_deny.json |
Defines default-deny case. |
tests/configs/lxc_network_enforcement_allow.json |
Defines explicit-allow case. |
tests/configs/lxc_network_deny_precedence_overlap.json |
Defines overlapping rules case. |
tests/configs/lxc_network_deny_precedence_control.json |
Defines precedence control case. |
src/core/wxc_common/tests/proxy_env_spec.rs |
Tests proxy environment hygiene. |
src/core/wxc_common/tests/proxy_address_spec.rs |
Tests proxy URL and host-pin behavior. |
src/core/wxc_common/src/proxy_env.rs |
Adds LXC proxy environment helper. |
src/core/wxc_common/src/models.rs |
Adds proxy host pinning model. |
src/backends/lxc/common/src/network_iptables.rs |
Implements hooks, precedence, and fail-closed behavior. |
src/backends/lxc/common/src/network_iptables_veth_spec.rs |
Tests missing-veth handling. |
src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs |
Tests FORWARD hook construction. |
src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs |
Tests ordering and resolution failures. |
docs/lxc-support/lxc-backend.md |
Documents updated firewall semantics. |
.github/workflows/lxc-e2e.yml |
Adds LXC E2E workflow. |
Suppressed comments (1)
src/backends/lxc/common/src/network_iptables.rs:1291
- The IPv6 physdev hook has the same signal window:
ip6tables -Ican succeed beforev6_physdev_hookis recorded and published, leaving the watchdog unable to remove the live hook. Publish pending ownership before the insert and distinguish an absent rule from a failed removal during rollback.
bridged,
"ip6tables",
logger,
)?;
Self::publish_created(created);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// caller to emit `--clear-env`, and an empty vector must still stop | ||
| /// `lxc-attach` inheriting the MXC host process environment, which carries | ||
| /// both proxy vars and credentials. | ||
| pub fn apply_proxy_env(env: &mut Vec<String>, proxy: &ProxyConfig) -> bool { |
Comment on lines
+1314
to
+1318
| return Err(format!( | ||
| "No veth interface for container; cannot scope iptables rules to chain {}. \ | ||
| The chain would never be reached from FORWARD, so the network policy would \ | ||
| not be enforced. Refusing to report success for an unenforceable policy.", | ||
| self.chain_name |
| for (host, action) in entries { | ||
| let destinations = Self::resolve_host(host); | ||
| if destinations.is_empty() { | ||
| if default_permits && matches!(action, RuleAction::Deny) { |
Comment on lines
+1252
to
+1256
| bridged, | ||
| "iptables", | ||
| logger, | ||
| )?; | ||
| Self::publish_created(created); |
Slice 3 made a missing veth fatal: install_firewall_rules returned Err so a container could never start believing it was confined by a chain that FORWARD never reaches. That is right for LXC, which always names a veth once the container is running, so arriving at rule installation without one means the lookup lost it. Bubblewrap has no veth at all. Unprivileged bwrap either shares the host network namespace or gets a private one, and neither yields a host-side interface to match on -- bwrap_command.rs says so directly. bwrap_runner builds a NetworkIptablesManager and never calls set_veth_interface, so every Bubblewrap sandbox requesting Firewall or Both mode with host rules hit the new Err and failed to start. On main that path logged a warning and continued. No test covered it, so all six CI workflows stayed green. Make the strictness a property the caller declares. The default still fails closed, so both veth-spec tests and the LXC contract are unchanged. Bubblewrap calls allow_missing_veth_interface and keeps the pre-existing warn-and-skip, which leaves its policy unenforced -- a real gap, but a pre-existing one that belongs to Bubblewrap's own work item rather than to this LXC change. Adds three tests: the declared-missing case must succeed under Firewall and Both, and a manager that never declared it must still fail closed, so the two behaviors cannot collapse into one. Found by an independent reviewer auditing whether pre-existing tests needed to change; the regression was invisible because bwrap_common was never in the packages this branch had been testing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Mutation M4 -- delete the allow_missing_veth_interface call from bwrap_runner -- survived the whole suite. That is the same blind spot that let the regression land: the declaration lived inline in a 300-line execute function where no test could reach it. Extract build_firewall_manager so the declaration has a seam, and assert on it via a new veth_scoping_is_optional accessor rather than by standing up a real firewall -- lxc_common's fake-firewall seam is cfg(test) and so is invisible to bwrap_common. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Mutation M5 -- make veth_scoping_is_optional always return true -- survived, so the Bubblewrap suite would have passed on an accessor that could not say no. Assert a fresh manager reports false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/backends/lxc/common/src/network_iptables.rs:738
- Mandatory: this condition fails open under a block default when an allow entry overlaps the unresolved deny. For example,
allowedHosts: ["0.0.0.0/0"]plus a temporarily unresolvable blocked hostname installs no DROP for that hostname, then the broad ACCEPT permits it once the container resolves it. Reject unresolved deny entries regardless of the default policy unless enforcement can prove no allow/base rule covers them.
if default_permits && matches!(action, RuleAction::Deny) {
src/backends/lxc/common/src/network_iptables.rs:1282
- A fatal signal can leak the new IPv4 physdev hook.
install_physdev_hookexecutesiptables -Ibefore returning, but ownership is not published until afterward; the watchdog can snapshotv4_physdev_hook = falsein that interval, remove only the interface hook, and leave the physdev reference and chain behind. Publish intent while synchronizing with the watchdog (and make rollback distinguish an absent rule) so every successfully installed hook is visible atomically.
created.v4_physdev_hook = Self::install_physdev_hook(
Self::run_iptables_rule_args,
iface,
&chain_name,
bridged,
src/backends/lxc/common/src/network_iptables.rs:1317
- The IPv6 physdev hook has the same signal-time ownership race:
ip6tables -Imay succeed beforev6_physdev_hookis assigned and published, allowing the watchdog to miss the live hook and leave it referencing the chain. Make installation and publication atomic from the watchdog's perspective, with absence-aware rollback for a published intent whose insert fails.
created.v6_physdev_hook = Self::install_physdev_hook(
Self::run_ip6tables_rule_args,
iface,
&chain_name,
bridged,
Comment on lines
1235
to
+1239
| // Hook the chains into FORWARD for the container's egress traffic. | ||
| // Packets originating in the container arrive at the host on the | ||
| // host-side veth, so they match FORWARD by input interface (`-i`); | ||
| // `-o` would instead match traffic flowing toward the container. | ||
| // | ||
| // Two rules per family, because the input interface FORWARD sees | ||
| // depends on how the veth is attached. A veth routed directly by the | ||
| // host arrives as `-i <veth>`. A veth enslaved to a bridge -- the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-implementation of PR #632 ("deny-all-except-proxy", model 2) cut fresh from
main.632 could not be finished on its branch: 3,593 insertions across 17 files in 33 commits, 63 review comments of which 52 were substantive correctness objections, and it no longer merged with
main(4 conflicting files, 22 hunks). This PR rebuilds the same behavior frommainin 14 commits over 17 files, addressing the objections rather than carrying them forward. #632 is closed.Refs AB#62830341.
Roadmap items
Read the proxy rows carefully.
apply_proxy_envhas no call site anywhere in the tree, andnetwork.proxyis still rejected for LXC at parse time (config_parser.rs:3460— "lxc is not allowed -- proxy is gated to processcontainer + bubblewrap"). The proxy commits here make the shared helper and the address model correct and fully tested; they do not change LXC runtime behavior. Wiring them into the LXC runner is follow-up work.These are the LXC rows. The Bubblewrap and WSLC sections carry their own N1/N4 rows, which this PR does not touch. Row 18 (N7) schema migration is untouched and still blocks #634 and #627.
What changed
Proxy env-var hygiene (
proxy_env.rs). The helper now scrubs every managed key before setting the configured proxy, matched case-insensitively across both spellings, neutralizesNO_PROXYto empty rather than pointing it at the proxy URL, and redacts credentials from logged URLs. LXC still does not call it — the helper has no call site in the tree, so this is a corrected and tested building block, not a behavior change for LXC. Commit 22098a2 records that divergence in the spec header rather than papering over it.Proxy address model (
models.rs). The old code rewrote the URL host to the resolved IP, which breaks TLS unless the certificate carries an IP SAN — the objection raised on #632. The address is now pinned as a distinct value and the original URL is preserved verbatim, so scheme, credentials, path, query, and trailing slash all survive. An unpinnable address is unrepresentable rather than silently mishandled. This is consumed by the AppContainer and Bubblewrap paths; LXC rejectsnetwork.proxyat parse time and is unaffected.Fail closed when rules cannot be scoped (
network_iptables.rs).install_firewall_ruleslogged"Warning: No veth interface set… Skipping FORWARD hook."and returnedOk(()), soapply_firewall_rulesreported success with a fully-populated but entirely unhooked chain — a container that looked filtered and was not. It now returnsErr, names the unenforced chain, and tears down the chain it created. A capabilities-only container is unaffected.Hook the chain onto the bridge port (
network_iptables.rs). The chain existed but nothing jumped to it. It is now hooked with--physdev-inon the container veth, which is what makes the rules filter at all.Deny-wins precedence (
network_iptables.rs). Rules were emitted allow-list first, so a destination in both lists was ACCEPTed — the GA spec requires the deny to win.blocked_hostsare now emitted beforeallowed_hosts. An unresolvable deny entry under anallowdefault is now an error rather than a warning: the chain ends in ACCEPT, so the entry that failed to resolve was the only thing protecting that destination. Under ablockdefault the closing DROP already covers it, so it stays a warning.Docs and CI (
lxc-backend.md,lxc-e2e.yml). The docs claimed the FORWARD hook was skipped with a warning when the veth could not be discovered, which is now false in both directions. Corrected, along with the precedence rules, the unresolvable-entry table, and thephysdev/br_netfilterrequirement.lxc-e2e.ymlis the first CI that runs the LXC E2E scripts at all.Evidence
lxc_common, 44 in thewxc_commonproxy specs, 0 failures.clippy -D warningsandcargo fmt --checkclean.MXC_LXC_TESTS_REQUIRE_EXECUTION=1.MXC_NET_ALLOWEDand fails the guard; the fixed binary reportsMXC_NET_BLOCKEDand passes. The control case passes on both, so the difference is the rule ordering and not a broken host.Tests for each behavior were written by an agent that had not seen the implementation, from the documented contract only. The mutation run exists to prove those tests actually bite.
One CI finding worth keeping
The new workflow failed on its first run: 3 of 14 tests failed, and all three were positive controls refusing to report success. GitHub-hosted runners ship Docker, which sets
-P FORWARD DROPon IPv4. MXC hooks container egress, so the reply packet matches no MXC rule, falls through to the policy, and dies — DNS still worked, because dnsmasq onlxcbr0is host-local and never traverses FORWARD, so it presented as a resolved address that timed out.The worse half is silent: under a DROP policy a container with no MXC hook at all is equally unreachable, so every deny case would have passed vacuously over a firewall filtering nothing. The workflow sets
-P FORWARD ACCEPTfor this reason. A conntrackRELATED,ESTABLISHEDrule would have cured the visible symptom and preserved the vacuous pass.A Bubblewrap regression this PR introduced, and the fix
Slice 3 made a missing veth fatal:
install_firewall_rulesreturnsErrrather than hand back a deny-all chain that FORWARD never reaches. That is right for LXC, which always names a veth once the container is running, so arriving at rule installation without one means the lookup lost it.Bubblewrap has no veth at all.
bwrap_runnerbuilds aNetworkIptablesManagerand never callsset_veth_interface, because unprivileged bwrap either shares the host network namespace or gets a private one and neither yields a host-side interface to match on —bwrap_command.rssays so directly. So every Bubblewrap sandbox requestingFirewallorBothwith host rules and no cooperative proxy would have failed to start. Onmainthat path logged a warning and continued.Strictness is now a property the caller declares. The default still fails closed, so the veth spec and the LXC contract are unchanged; Bubblewrap declares the absence and keeps the pre-existing warn-and-skip.
That leaves Bubblewrap's policy unenforced when it asks for firewall mode. It is a real gap and it is pre-existing — making bwrap fail closed is a defensible decision, but it belongs to Bubblewrap's own work item, not smuggled into an LXC change.
Why six green workflows missed it
The branch had only ever unit-tested
lxc_commonandwxc_common. The regression lives inbwrap_common, where no test reached the call, and the in-tree fixture that would have hit it —tests/configs/bubblewrap_network_firewall.json— is not referenced byrun_bwrap_all_tests.sh. Consumer packages are now tested by name:bwrap_common61,mxc_engine35,seatbelt_common54,appcontainer_common185.Mutation over the fix: 6 mutants, 6 caught. Deleting the declaration survived while it was inline at the call site, which is why it now lives in
build_firewall_managerwith a test on it.Deliberately not in scope
apply_proxy_envfrom the LXC runner, lifting the parse-time gate onnetwork.proxyfor LXC, and adding the proxy-port-only egress rule with a DROP for everything else. That is the second half of the work item ask and the literal "except-proxy" of model 2; the default-deny half is what landed here.MXC_SKIP_LXC_NETWORK_TESTS=1inSDK.Integration.Test.Job.ymlis untouched. Its stated rationale is that the container's eth0 stays IPv4-less so DNS fails, but containers inlxc-e2e.ymldo get a10.0.3.xlease and resolve after a poll. That is evidence for revisiting roadmap row 31, not a claim that the SDK integration tests pass.Microsoft Reviewers: Open in CodeFlow