[LXC] Enforce the deny-all-except-proxy network policy (model 2) - #798
Open
Darren Hoehna (dhoehna) wants to merge 60 commits into
Open
[LXC] Enforce the deny-all-except-proxy network policy (model 2)#798Darren Hoehna (dhoehna) wants to merge 60 commits into
Darren Hoehna (dhoehna) wants to merge 60 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.
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 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/scripts/run_lxc_network_enforcement_test.sh:43
- These literals no longer name the chains created by
NetworkIptablesManager: after #780, names use the hashedMXC-<slug>-<hash>format. Both cleanup checks therefore query nonexistent chains and pass even if this test leaks its real chain or FORWARD hook. Derive each chain from that run's debug output (asrun_lxc_network_cidr_boundary_test.sh:105-119does), or snapshot/diff allMXC-chains around each run.
DENY_CHAIN="MXC-CLI-LXC-Net-Deny"
ALLOW_CHAIN="MXC-CLI-LXC-Net-Allow"
tests/scripts/run_lxc_network_deny_precedence_test.sh:46
- These pre-hash chain names cannot match the chains created after #780, so
assert_no_forward_referenceandassert_firewall_chain_cleaned_upare currently vacuous. Read each actual hashed name from its run's debug output or compareMXC-chain snapshots before and after the run.
OVERLAP_CHAIN="MXC-CLI-LXC-Net-DenyWins"
CONTROL_CHAIN="MXC-CLI-LXC-Net-DenyCtl"
src/core/wxc_common/src/proxy_env.rs:342
- This new contract always forces
--clear-env, including when no proxy and noprocess.envwere supplied. That contradictsdocs/lxc-support/lxc-backend.md:96, which promises that an empty/absent environment preserves host inheritance. Because this changes existing LXC execution semantics beyond proxy-enabled requests, update the public documentation (and release notes if applicable), or scope the forced clear to the cases intended to change.
/// Returns whether the caller must force a clean environment. This is always
/// `true`, including when `env` ends up empty: 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.
docs/lxc-support/lxc-backend.md:223
- The documented acceptance rules omit two mandatory constraints introduced by this PR: LXC rejects proxy URLs with inline credentials, and an enabled proxy requires
enforcementModeto befirewallorboth(the defaultcapabilitiesmode is rejected). Without these requirements, the documented URL example can still fail unexpectedly for users.
Only the `{ "url": "http://proxy.example:8080" }` form is accepted. The LXC
container has its own network namespace, so `{ "localhost": <port> }` names the
*container's* loopback rather than the host's — the injected proxy would be
unreachable and the firewall rule would never match. `{ "builtinTestServer":
true }` is rejected for the same reason, as is a `url` whose host is a loopback
src/core/wxc_common/tests/proxy_env_spec.rs:17
- This test-module overview is already stale in the same PR: LXC now calls
apply_proxy_env, threads the returned flag intoattach_run, and can force--clear-envfor an empty environment. Describing that integration as planned and the old behavior as current misstates what these tests cover.
//! (a) LXC backend (PLANNED integration, not yet wired) -- will call
//! `apply_proxy_env` and use the returned bool to decide whether to pass
//! `--clear-env` to `lxc-attach`. Today `attach_run` derives `--clear-env`
//! solely from `env` being non-empty (`lxc_bindings.rs:90`). The empty-env
//! case is where the helper contract and current behavior diverge:
//! `apply_proxy_env` returns `true` even for an empty env so the host
//! environment cannot leak, whereas current code emits no `--clear-env`
//! then. Wiring this in must update `lxc_bindings.rs` and the test at
//! `lxc_bindings.rs:743` that pins the current empty-env rule. These tests
//! validate the helper contract, not existing LXC behavior.
Review argued that conntrack creates state before the filter/FORWARD decision,
so a packet the MXC chain drops would still leave a NEW entry, and a
cooperating peer could then send a reverse packet classified ESTABLISHED that
this rule accepts. That would make the rule a genuine widening.
Measured on the directly routed topology the argument names, with a positive
control so a negative result means something. Two network namespaces either
side of a forwarding host, FORWARD policy DROP, this exact rule installed, and
an arrival counter inside the container.
outbound ACCEPTed: 2 conntrack entries, this rule matched 3 times,
3 packets reached the container
outbound DROPPED: 0 conntrack entries, this rule matched 0 times,
0 packets reached the container
The reverse packets fell to the policy DROP. The mechanism does not hold:
conntrack attaches an unconfirmed entry at PREROUTING, but only
nf_conntrack_confirm inserts it into the table and that runs after the FORWARD
verdict, so a dropped packet is freed and takes its unconfirmed entry with it.
The comment was wrong in a different way, though, and that part is fixed. It
said a flow could only have state because its outbound direction was accepted
by this chain. State can also exist because the *host* authorized an inbound
flow, and this rule accepts that flow's continuation. That is what stateful
filtering means rather than a widening -- the first packet still had to pass
the host's own policy -- but the comment overstated the invariant, so it now
says the accurate thing and cites the measurement.
Not established: the same experiment on the bridged topology. Its positive
control failed -- no packet reached the container even with the outbound
allowed -- so that harness proves nothing in either direction and no claim is
made from it. Bridged behavior is covered by the LXC E2E job against real
containers, which passes.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tests/scripts/run_lxc_network_enforcement_test.sh:43
chain_name_fornow always emits a hashed chain name, so these legacy literals can never identify the chains created by either run. The cleanup and surviving-FORWARD-reference checks therefore pass vacuously even if the real chain leaks. Derive each chain from that run's debug output or compare pre/postMXC-chain snapshots, as the CIDR-boundary script does.
DENY_CHAIN="MXC-CLI-LXC-Net-Deny"
ALLOW_CHAIN="MXC-CLI-LXC-Net-Allow"
tests/scripts/run_lxc_network_deny_precedence_test.sh:46
- These are pre-hash chain literals, while
chain_name_foralways appends the generated hash. Consequently the cleanup assertions only inspect names that this test never creates and cannot detect leaked chains or FORWARD hooks. Parse each actual chain name from the corresponding run output or use before/afterMXC-chain snapshots.
OVERLAP_CHAIN="MXC-CLI-LXC-Net-DenyWins"
CONTROL_CHAIN="MXC-CLI-LXC-Net-DenyCtl"
docs/lxc-support/lxc-backend.md:223
- This public guide omits two new acceptance constraints:
network.enforcementModemust explicitly befirewallorboth, and URLs containing userinfo credentials are rejected. As written, a user following the documented URL form can still receive an unexplained policy error.
Only the `{ "url": "http://proxy.example:8080" }` form is accepted. The LXC
container has its own network namespace, so `{ "localhost": <port> }` names the
*container's* loopback rather than the host's — the injected proxy would be
unreachable and the firewall rule would never match. `{ "builtinTestServer":
true }` is rejected for the same reason, as is a `url` whose host is a loopback
tests/scripts/run_lxc_network_proxy_test.sh:45
- This note is now false:
.github/workflows/lxc-e2e.ymlprovisions these dependencies and invokes the LXC suite. Keeping it says the test is unproven even after that CI job runs.
# Requires Linux, root, LXC, and python3. It cannot run on the Windows dev box
# and no CI job invokes the LXC suite, so treat it as unproven until executed
# on a Linux host.
Two more ways a proxy password reached argv, and a change to how the
guard is tested so the next one is found by a machine rather than by a
reviewer.
`::@host` was read as empty userinfo. It is not: the first colon
separates an empty username from a password of `:`. Measured against
the parser the backend already uses, `http://::@h` yields
`password = Some("%3A")` while the guard said there was nothing there.
A backslash after a scheme was only honoured for the WHATWG special
schemes. That was modelled on the parser, and it was the wrong model.
The parser is authoritative for whether it *sees* userinfo; it is not
authoritative for whether a password is sitting in a string that
reaches argv and the failure diagnostic. `socks5:\/alice:hunter2@host`
parses as an opaque path with no userinfo, and still carries the
password in plain text. The two ways of being wrong do not cost the
same -- over-reporting rejects a config, under-reporting publishes a
secret -- so the equivalence now applies to every scheme.
The differential test that found the backslash case listed its inputs by
hand, and a hand-written list had by then missed two shapes. It now
generates them: 7 schemes x 7 separators x 12 userinfos x 3 hosts x 5
tails x 5 paddings, over 500 of which the parser accepts. A miss is a
hard failure; an over-report is only a failure when the string contains
no `@` at all, since without one there is nothing to be suspicious of.
Both fixes are mutation-tested. Reverting the userinfo check kills two
tests, and dropping the backslash equivalence kills four -- the
generated corpus catches each on its own.
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 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (6)
.github/workflows/lxc-e2e.yml:73
- This proposed fix is now known not to work on the default bridged topology. The added per-veth return rules are inert there because FORWARD sees
lxcbr0and--physdev-outis not selected yet; the PR description records the failedFORWARD DROPexperiment. Update this comment so future work does not repeat the disproven fix.
# A narrower conntrack RELATED,ESTABLISHED rule would fix the reply path
# but leave the DROP policy, and with it the vacuous pass.
tests/scripts/run_lxc_network_enforcement_test.sh:43
- These are no longer the chains created for these container IDs.
chain_name_fornow emitsMXC-<slug>-<hash>(src/backends/lxc/common/src/network_iptables.rs:309-323), so both cleanup helpers inspect nonexistent legacy names and pass even if the real chain or its FORWARD hook leaks. Derive each chain from that run's debug output, or snapshot/compare allMXC-chains asrun_lxc_network_cidr_boundary_test.sh:100-119does.
DENY_CHAIN="MXC-CLI-LXC-Net-Deny"
ALLOW_CHAIN="MXC-CLI-LXC-Net-Allow"
tests/scripts/run_lxc_network_deny_precedence_test.sh:46
- These legacy constants cannot name the actual hashed chains produced by
chain_name_for(src/backends/lxc/common/src/network_iptables.rs:309-323). Consequently the cleanup and stale-reference checks are vacuous and will pass when the realMXC-<slug>-<hash>chain leaks. Read each chain from its run's debug output or use the repository's before/afterMXC-chain snapshot pattern.
OVERLAP_CHAIN="MXC-CLI-LXC-Net-DenyWins"
CONTROL_CHAIN="MXC-CLI-LXC-Net-DenyCtl"
docs/schema.md:67
- This overstates the enforcement boundary. The PR's measured behavior and
run_lxc_network_proxy_test.sh:200-209show that bridge-gateway DNS and other host-local traffic traverse INPUT, not this FORWARD-only chain, and remain reachable. Document that only forwarded/off-host egress is deny-all-except-proxy rather than claiming all egress and DNS are restricted.
// Under LXC the proxy is enforced: egress is restricted
// to the proxy endpoint and nothing else, so the
// allow/block host lists and DNS are not opened.
docs/lxc-support/lxc-backend.md:216
- The documented “nothing else” guarantee is false for host-local destinations. This PR explicitly measures bridge-gateway DNS and a host-resident proxy on INPUT with zero FORWARD packets, so applications can still reach host services that bypass this chain. Scope this guarantee to forwarded/off-host traffic and call out the INPUT gap.
`network.proxy` puts the container in a "deny all except the proxy" posture:
egress is restricted to the proxy endpoint, and `HTTP_PROXY`/`HTTPS_PROXY` are
injected so a cooperating client uses it. The env vars are the routing hint;
the firewall is the enforcement, so an application that ignores them reaches
nothing rather than reaching the internet directly.
tests/scripts/run_lxc_network_proxy_test.sh:35
- This fixture uses the IP-literal proxy
10.0.3.1, for whichProxyAddress::host_pinreturnsNone; no/etc/hostspin is written.PROXY_OKtherefore proves env injection and host-local proxy access, but not the hosts-pin path as claimed. Remove that claim or use a hostname fixture whose mapping is actually pinned.
# FORWARD). PROXY_OK proves the env-var injection and the hosts pin are right
# and that the deny-all posture did not break the proxy path; it does not
run_lxc_network_enforcement_test.sh and run_lxc_network_deny_precedence_test.sh hardcoded chain names such as MXC-CLI-LXC-Net-Deny. Chain names are derived -- chain_name_for produces MXC-<slug>-<hash>, where the hash is 16 base32 characters of the SHA-256 of the container name -- so a literal name never matches a real chain. `iptables -S "$CHAIN"` therefore always failed, and the cleanup check read that failure as "the chain was properly removed." Both scripts passed while inspecting nothing. The four other network scripts already derived the name from the chain-creation log line and took a before/after snapshot of MXC chains. Both broken scripts now follow that pattern: mxc_chains, assert_no_new_mxc_chains, derive_chain_name, a shape check on the derived name, and a snapshot taken before each run. Add chain_name_script_drift_spec.rs to keep this from coming back. It is a drift guard over the scripts, not a unit test -- it reads files, so it lives in its own file and leaves chain_name_spec.rs dependency-free. chain_name_spec.rs already had 20 green naming tests on the day these scripts were broken, so more tests of chain_name_for could not have caught this; the guard has to read the scripts. It fails if any script names a specific chain, if the shape checks copy-pasted across five scripts drift apart, if the pinned shape stops accepting what chain_name_for actually produces, if a script derives a name without validating it, or if a script asserts on a chain name it never derived. Also correct six documentation and comment claims that the implementation contradicts: - schema.md said the LXC proxy allowance covers egress; it covers forwarded egress, and traffic to the bridge gateway arrives on INPUT instead. - lxc-backend.md said a blocked container "reaches nothing," and did not mention the FORWARD/INPUT split or the two parse-time rejections (enforcementMode must be firewall or both; the proxy URL may not carry credentials, since argv is world-readable through /proc/<pid>/cmdline). - run_lxc_network_proxy_test.sh claimed the fixture pins a hosts entry. The fixture proxy is http://10.0.3.1:3128, an IP literal, and host_pin returns Ok(None) for those, so no pin is written. It also claimed no CI job invokes the LXC suite; lxc-e2e.yml does. - lxc-e2e.yml proposed a conntrack return rule as the fix for the return path. The PR measured both return-rule forms inert. Address-scoping is the fix and is deferred, so the comment now records the measurement rather than a disproven proposal. The return-path defect itself is unchanged and still deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f151c717-2eee-4d1d-8498-081504aef847
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/backends/lxc/common/src/network_iptables.rs:1040
- A hostname that resolves to
127.0.0.0/8is still accepted here. Its first A record is then authorized and pinned into the container, where that address is the container's own loopback, so the accepted deny-all-except-proxy configuration cannot reach its proxy. This is the same invalid posture rejected for loopback literals in the parser; reject loopback-resolved A records before creating endpoints/the pin, with coverage for a resolved loopback hostname.
let resolved = Self::resolve_host(address.host());
if resolved.ipv4.is_empty() {
src/core/wxc_common/src/proxy_env.rs:345
- Returning
trueunconditionally means every LXC invocation now passes--clear-env, including requests with no proxy and noprocess.env. This intentionally improves proxy hygiene, but it contradictsdocs/lxc-support/lxc-backend.md:94-96, which still promises legacy host-environment inheritance whenprocess.envis absent or empty. Update that public behavior documentation (and the environment test expectations) to reflect the new semantics.
// With the proxy disabled the vars are still stripped, so a caller cannot
// point the sandbox at an egress path the policy never authorized.
env.retain(|entry| !is_managed_proxy_key(env_key(entry)));
true
src/core/wxc_common/src/config_parser.rs:1009
- This expands proxy support to LXC, but the public Node SDK contract remains contradictory:
sdk/node/src/types.ts:212-215omits LXC from the supported backends and describes the listed non-Windows implementations as cooperative env-var proxies, whereas LXC now adds firewall enforcement. Update the SDK JSDoc/README so consumers can discover and correctly understand this backend behavior.
if proxy_config.is_enabled()
&& containment != ContainmentBackend::ProcessContainer
&& containment != ContainmentBackend::Bubblewrap
&& containment != ContainmentBackend::Lxc
&& containment != ContainmentBackend::Seatbelt
&& containment != ContainmentBackend::Wslc
src/core/wxc_common/tests/proxy_address_spec.rs:37
- This client-status note is already stale:
NetworkIptablesManager::build_proxy_host_pincallshost_pin, andLxcScriptRunnerconsumes the resultingProxyHostPinto rewrite/etc/hosts. Keeping the test's contract commentary synchronized with the production wiring avoids misleading future maintainers.
//! * The pin surface (`host_pin`, `hosts_line`, `ProxyHostPin`) still has no
//! callers. It is planned wiring for the firewall / hosts-file consumer, so
//! the tests below name that consumer as planned, not present. `ProxyHostPin`
//! has no public constructor -- the only way to obtain one is `host_pin` on a
//! hostname -- so the tests build pins that way through the `pin_for` helper.
| // failing would only refuse to start a sandbox that was never going | ||
| // to be scopable. It keeps the pre-existing skip, which leaves the | ||
| // policy unenforced -- see `allow_missing_veth_interface`. | ||
| if !self.veth_scoping_optional { |
Contributor
There was a problem hiding this comment.
In firewall/both mode bwrap shares the host netns (no veth), so rules are built but never hooked; apply_firewall_rules returns Ok(true). Full network, reported as enforced. Shall we reject firewall/both for bwrap when unscopable instead of skipping.
We had a tracking issue on this- #755
| /// default bridged topology the packet's output interface is `lxcbr0`, not | ||
| /// the veth, so the `-o <veth>` rule matches nothing and only | ||
| /// `--physdev-out` names the specific container. | ||
| fn build_forward_return_physdev_rule_args(op: &str, iface: &str) -> Vec<String> { |
Contributor
There was a problem hiding this comment.
Return rules use -o / --physdev-out; GPT says on bridged hosts with FORWARD DROP these don't match, so allowed proxy replies time out? Validate reply-path on bridged CI?
| /// its target created by the redirect -- a write to an attacker-named | ||
| /// path, which on a writable host bind mount lands outside the | ||
| /// container. | ||
| fn hosts_read_prologue() -> String { |
Contributor
There was a problem hiding this comment.
check-then-write /etc/hosts is a TOCTOU in a reused container. Opus: -h symlink guard + in-variable staging mitigate it. Confirm the rewrite is a single O_NOFOLLOW open?
Sixteen comments narrated this PR's own development -- what an earlier version asserted, what a review raised, which hand-written list failed. Git holds that history. Each is restated as the constraint it was standing in for, in present tense, so the reason a design exists survives without the story of arriving at it. Two are corrections rather than rephrasings. The `is_uri_scheme` and port-colon comments described the naive first-colon parse in bare present tense, which reads as a claim about the current guard. They are conditional now: `is_uri_scheme` rejects a prefix containing `@`, so the value goes through as an authority and the `@` is found. Comment-only -- no executable line changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f151c717-2eee-4d1d-8498-081504aef847
The `VethTopology` doc and its variants described the sysfs probe rather than the topology: which file `Path::exists` follows, which branch a caller takes, what a two-state check would get wrong. The code shows all of that. Each variant now names the finding it stands for. Same treatment for `veth_scoping_optional`, `topology_override`, `proxy_pin`, and the `fail_matching` test-double field. The fail-closed contract on an unknown topology is unchanged and stays where it is enforced -- the branch at `treat_as_bridged` and the test that asserts the warning is logged. Comment-only -- no executable line changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f151c717-2eee-4d1d-8498-081504aef847
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/backends/lxc/common/src/network_iptables.rs:1040
- The backend still accepts loopback proxy endpoints when callers bypass JSON parsing, and it also accepts names such as
localhost.that resolve to 127/8. This resolution then opens and pins the loopback address, but inside LXC it denotes the container itself, so the configured proxy is guaranteed unreachable. Reject resolved loopback addresses here (or discard them and require at least one routable IPv4), rather than relying only on the parser's textual check.
let resolved = Self::resolve_host(address.host());
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/backends/lxc/common/src/network_iptables.rs:1940
- These return rules are claimed before insertion, but unlike the chain hooks their rule specification contains no per-manager chain name. If insertion fails before adding a rule (or the watchdog races this assignment) while an identical rule already exists for the same reused veth, teardown's
-Dcan delete that other manager's rule. Give return rules a unique identity, such as a chain-derived iptables comment included in both insert and delete, before publishing ownership.
created.v4_return = true;
created.v4_physdev_return = true;
Self::publish_created(created);
src/backends/lxc/common/src/network_iptables.rs:1027
ProxyAddress::from_urland its fields are public, so parser-bypassing callers can supply anoriginal_urlwhose host or port differs fromaddress.host()/port(). This code opens and pins the latter, whileapply_proxy_envlater injectsoriginal_urlverbatim, yielding a deny-all container that advertises a different, unreachable proxy. Validate that the original URL resolves to the same host and effective port at the LXC boundary, or derive both firewall and environment values from one parsed representation.
let resolved = Self::resolve_host(address.host());
tests/scripts/run_lxc_network_proxy_test.sh:224
- A global IPv6 address does not establish usable IPv6 egress. Hosts commonly have a global/ULA address but no default route, so the fixture's
wgetfails and emitsDIRECT_IPV6_BLOCKED; this branch then reports the firewall as verified even if the IPv6 hook never matched. Add a positive IPv6 reachability control (for example, a separate permissive run to the same literal) and treat lack of baseline reachability as a skip rather than a block.
elif ! grep -q "DIRECT_IPV6_BLOCKED" <<<"$OUT"; then
fail "no IPv6 verdict in container output (expected DIRECT_IPV6_BLOCKED or the skip marker)"
src/core/wxc_common/src/config_parser_loopback_spec_tests.rs:225
- This assertion codifies a reachable loopback spelling as non-loopback. A trailing dot only makes
localhost.an absolute DNS name; it still denotes localhost, so LXC will pin it to the container's loopback and the configured proxy is unreachable. Normalize one trailing root dot before the case-insensitive localhost comparison and invert this test.
!host_is_loopback("localhost."),
The `treat_as_bridged` doc described which branch downgrades a physdev failure and what an unknown topology does downstream. Both are reachable by reading the body and following the calls. What is not reachable that way is why the function tests for `DirectlyRouted` rather than for `Bridged`: the mapping is deliberately asymmetric, because bridged is the conservative answer and only positive evidence earns the other one. Comment-only -- no executable line changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f151c717-2eee-4d1d-8498-081504aef847
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/core/wxc_common/src/config_parser.rs:459
- This exact-name check accepts
http://localhost.:PORT, although the trailing-dot form is the absolute DNS spelling oflocalhostand resolves to the container's loopback. LXC then installs a deny-all proxy policy whose only configured proxy is unreachable, instead of applying the advertised parse-time loopback rejection. Treatlocalhost.as loopback and update the characterization test that currently pins the opposite result.
fn host_is_loopback(host: &str) -> bool {
if host.eq_ignore_ascii_case("localhost") {
return true;
tests/scripts/run_lxc_all_tests.sh:99
SKIPPEDonly counts child scripts that exit 77, but skipped subcases still exit 0. For example, the new proxy test printsSKIP: direct-IPv6 drop UNVERIFIEDatrun_lxc_network_proxy_test.sh:222, and the existing dual-stack test does the same at lines 204/241/244. Strict mode therefore reports success while these assertions were not executed, contrary to the stated CI guarantee. Propagate subcase skips to the runner (or provision the missing capability and fail the child) so strict mode can detect them.
if [ "$SKIPPED" -gt 0 ]; then
echo "ERROR: strict mode: $SKIPPED test(s) skipped a prerequisite that this"
echo "environment is supposed to provide. Refusing to report success."
exit 1
Comment on lines
+1011
to
+1013
| let address = policy.network_proxy.address.as_ref().ok_or_else(|| { | ||
| "Network proxy is enabled but no proxy address is configured".to_string() | ||
| })?; |
A function's doc comment now carries only what a reader cannot reach by reading the body or following its calls: why a shape is what it is, why an asymmetry exists, which constant is load-bearing, and what breaks silently when it changes. Narration of the body, of which branch a caller takes, and of which helper is called next is gone. Where a comment describes an approach the code does not take, it uses conditional mood, so it cannot be misread as a claim about the shipped guard. Two factual corrections in build_hosts_pin_command: the redirected group still runs a test builtin and two writes, so the claim that nothing past the open can fail is replaced by the narrower true one that nothing reads the filesystem again; and hosts_line necessarily contains the separator space, so the single-quoting argument now rests on the absence of quotes and newlines. Comment-only. No executable line changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f151c717-2eee-4d1d-8498-081504aef847
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tests/scripts/run_lxc_network_proxy_test.sh:225
- Strict mode cannot detect this partial skip: this branch prints
SKIP, but the script continues to the finalPASSand exits 0, sorun_lxc_all_tests.shincrementsPASSEDandMXC_LXC_TESTS_REQUIRE_EXECUTION=1remains green. On the default IPv4-only LXC bridge, CI can therefore report “0 skipped” without behaviorally exercising the new IPv6 deny. Propagate exit 77 (or another partial-skip status understood by the suite) when no global IPv6 stack is available.
if grep -q "DIRECT_IPV6_SKIP_NO_STACK" <<<"$OUT"; then
echo "SKIP: direct-IPv6 drop UNVERIFIED — container has no global IPv6 stack"
elif ! grep -q "DIRECT_IPV6_BLOCKED" <<<"$OUT"; then
fail "no IPv6 verdict in container output (expected DIRECT_IPV6_BLOCKED or the skip marker)"
src/backends/lxc/common/src/lxc_runner.rs:438
- Appending the pin does not guarantee this hostname resolves to the authorized address when
/etc/hostsalready contains an unmarked entry for the same name. On a reused container, a previous workload can leave such an earlier entry; the resolver may select it before this appended line, making the enforced proxy unreachable even though pinning reports success. Remove or replace all existing mappings forpin.hostname()before appending the marked mapping, not only prior MXC-marked lines.
format!(
"{}{{ if [ -n \"$kept\" ]; then printf '%s\\n' \"$kept\"; fi; \
printf '%s {marker}\\n' '{hosts_line}'; }} > /etc/hosts",
Self::hosts_read_prologue(),
marker = HOSTS_PIN_MARKER,
hosts_line = hosts_line
)
src/core/wxc_common/tests/proxy_env_spec.rs:17
- This client-status note is already stale: the PR wires
apply_proxy_envintolxc_runner.rsand threads its flag throughattach_run, so LXC is no longer “PLANNED integration,” nor does attach argument construction rely solely on a non-empty environment. Update the test contract comment so future maintainers do not treat the shipping behavior as an outstanding task.
//! (a) LXC backend (PLANNED integration, not yet wired) -- will call
//! `apply_proxy_env` and use the returned bool to decide whether to pass
//! `--clear-env` to `lxc-attach`. Today `attach_run` derives `--clear-env`
//! solely from `env` being non-empty (`lxc_bindings.rs:90`). The empty-env
//! case is where the helper contract and current behavior diverge:
//! `apply_proxy_env` returns `true` even for an empty env so the host
//! environment cannot leak, whereas current code emits no `--clear-env`
//! then. Wiring this in must update `lxc_bindings.rs` and the test at
//! `lxc_bindings.rs:743` that pins the current empty-env rule. These tests
//! validate the helper contract, not existing LXC behavior.
src/core/wxc_common/src/config_parser.rs:1064
- This loopback invariant is only enforced by JSON parsing. Public callers can construct an
ExecutionRequestwithProxyAddress::new("127.0.0.1", port)and invoke the LXC runner directly;resolve_proxy_endpointsthen accepts it, while the injected URL points at the container's own loopback and the FORWARD ACCEPT can never authorize that traffic. Restate the rejection at the LXC runner/backend boundary, as this PR already does for enforcement mode and credential-bearing URLs.
if containment == ContainmentBackend::Lxc {
if let Some(host) = proxy_config.address.as_ref().map(|addr| addr.host()) {
if host_is_loopback(host) {
let msg = "LXC: network.proxy.url host is a loopback address \
(127.0.0.0/8, ::1, or localhost), which names the \
container's own loopback rather than the host; use a \
proxy host routable from inside the container";
logger.log_line(msg);
return Err(WxcError::ConfigParse(msg.to_string()));
}
}
Comment on lines
+130
to
+137
| Two limits on that guarantee are worth stating plainly, because "deny always | ||
| wins" is not true without them: | ||
|
|
||
| - **DNS is exempt.** The base chain accepts UDP and TCP destination port 53 | ||
| unconditionally and is installed ahead of the generated policy rules, so | ||
| port-53 traffic to a blocked destination is accepted before its DROP rule is | ||
| reached. Narrowing that rule needs to know which resolver addresses are | ||
| legitimate, and no schema field carries them today. |
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.
Refs AB#62830341 — [LXC] Network policy model 2. The GA-blocked LXC network
work is tracked separately under AB#63505947.
Summary
Give LXC a deny-by-default outbound network posture, with exactly one exception:
a cooperative proxy.
Covers Linux roadmap N1 (default-deny outbound), N4 (deny-wins
precedence), N5 (proxy env vars and enforcement), and row 22 (proxy
env-var hygiene) for LXC. The N7 GA schema migration is untouched.
What it does
enforcementModeoffirewallorboth,each container gets its own
MXC-<slug>-<hash>chain, hooked intoFORWARDwith
--physdev-inon the container veth. The physdev match is what makesthe rules fire at all: the veth is enslaved to
lxcbr0, so a plain-i <veth>match takes no packets. The default mode,capabilities,installs no rules.
determined, setup returns
Errand names the unenforced chain instead ofreporting success with a fully populated chain that nothing jumps to.
Rollback is best-effort: anything a removal command fails to delete stays
owned, so teardown retries it rather than stranding it. Bubblewrap has no
host-side veth by construction, so it declares that and keeps its existing
warn-and-skip.
blocked_hostsare programmed ahead ofallowed_hosts, so anaddress named in both is dropped. The base port-53 allowance precedes both.
case-insensitively, so a sandboxed process cannot override or disable the
proxy.
HTTP_PROXY,HTTPS_PROXY, andALL_PROXYare set to the configuredURL in both spellings, and
NO_PROXYis forced empty so an image-baked valueexempts nothing. In proxy mode the chain carries the proxy ACCEPTs and its
closing DROP and nothing else:
allowedHostsandblockedHostsare notprogrammed — a warning says so — and IPv6 egress is denied outright.
enforcementModeoffirewallorboth: undercapabilitiesno rules are installed, so theconfig would read as deny-all-except-proxy while enforcing neither half. A
proxy URL carrying inline credentials is refused, because LXC passes it to
lxc-attachas a--set-varargument and the argument vector is visible in/proc. Loopback and built-in-test-server proxies are refused as well, sincethe container's loopback is not the host's.
.github/workflows/lxc-e2e.ymlinstalls the LXC stack,enables
br_netfilter, and runs the enforcement suite against real containersand real iptables, with
MXC_LXC_TESTS_REQUIRE_EXECUTION=1so a skipped casefails instead of passing quietly.
Limits
notably DNS served on
lxcbr0— arrives onINPUTand is not covered.workflow sets
-P FORWARD ACCEPT.network.egressschema are AB#63505947.Validation
cargo test -p lxc_common -p wxc_common— 946 tests, 0 failures.cargo clippy --all-targets --all-features -- -D warningsandcargo fmt --check— clean.and iptables. Green in CI as LXC-Exec Container and Network Policy.
Microsoft Reviewers: Open in CodeFlow