[LXC] Correct the network policy docs and make the E2E suite gate in CI - #797
[LXC] Correct the network policy docs and make the E2E suite gate in CI#797Darren Hoehna (dhoehna) wants to merge 8 commits into
Conversation
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.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Corrects LXC firewall behavior and documentation while adding CI-gated end-to-end coverage.
Changes:
- Enforces bridged-veth hooks, deny precedence, and fail-closed behavior.
- Adds unit and behavioral LXC network tests.
- Adds strict LXC CI execution and updated documentation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/lxc-e2e.yml |
Adds the LXC E2E workflow. |
docs/lxc-support/lxc-backend.md |
Documents actual firewall semantics. |
src/backends/lxc/common/src/network_iptables.rs |
Updates firewall generation, hooks, and cleanup. |
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 deny ordering and resolution failures. |
tests/scripts/run_lxc_all_tests.sh |
Adds tests and strict execution mode. |
tests/scripts/run_lxc_network_enforcement_test.sh |
Tests effective firewall enforcement. |
tests/scripts/run_lxc_network_deny_precedence_test.sh |
Tests deny-over-allow behavior. |
tests/configs/lxc_network_enforcement_deny.json |
Defines the deny scenario. |
tests/configs/lxc_network_enforcement_allow.json |
Defines the allow control. |
tests/configs/lxc_network_deny_precedence_overlap.json |
Defines overlapping host rules. |
tests/configs/lxc_network_deny_precedence_control.json |
Defines the precedence control. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 |
| created.v4_hook = true; | ||
| Self::publish_created(created); | ||
|
|
||
| created.v4_physdev_hook = Self::install_physdev_hook( |
| "network": { | ||
| "defaultPolicy": "block", | ||
| "enforcementMode": "firewall", | ||
| "allowedHosts": ["api.github.com"], |
| @@ -0,0 +1,193 @@ | |||
| //! Spec for the fail-closed contract of `apply_firewall_rules`: when the | |||
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/backends/lxc/common/src/network_iptables.rs:1318
- This manager is also called by Bubblewrap for per-host firewall policies (
bwrap_runner.rs:203-225), and Bubblewrap never has a veth to set. This branch therefore makes every documented BubblewrapallowedHosts/blockedHostsfirewall request fail before spawning. Keep the missing-veth refusal in the LXC runner, or introduce a separate scoped implementation for Bubblewrap, rather than imposing the LXC invariant in the shared manager.
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
.github/workflows/lxc-e2e.yml:113
- The failure-only artifact step currently has nothing to upload: no suite step creates
logs/or a.logfile, and the first workflow failure reported “No files were found.” Capture the suite output explicitly so failures retain diagnostics.
run: sudo --preserve-env=MXC_LXC_TESTS_REQUIRE_EXECUTION bash tests/scripts/run_lxc_all_tests.sh
docs/lxc-support/lxc-backend.md:142
- There is a third precedence exception: the base chain's
ESTABLISHED,RELATEDACCEPT is also ahead of every generated DROP. Because the runner can attach policy to an already-running/reused container, an existing connection to a newly blocked address remains accepted. Document this alongside DNS, or change the rule placement/connection handling before claiming these are the only limits.
Two limits on that guarantee are worth stating plainly, because "deny always
wins" is not true without them:
| // 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 |
|
Superseded by #798. This work was split into six PRs on my initiative; it should have been one. Four of the six (#790, #792, #796, #797) were cumulatively stacked, so each one re-rendered the previous diff rather than reducing what a reviewer had to read, and they forced a merge order for no benefit. All of the changes here are in #798, cut fresh from |
Problem
The documentation described a firewall that no longer exists, and the
end-to-end scripts that would have caught the drift were never run by CI.
Four claims in
docs/lxc-support/lxc-backend.mdwere false against the code:FORWARD"by matching the host-side veth as the input interface"--physdev-inbridge-port rule and thebr_netfilterrequirementFORWARDhook with a warning"What an independent review caught in my first draft
All three were mine, and all three were wrong in the comfortable direction —
they made the firewall sound stronger than it is.
unconditionally and is installed ahead of the generated policy rules, so
DNS traffic to a blocked destination is accepted before its DROP is reached.
Narrowing that rule requires knowing which resolver addresses are
legitimate, and no schema field carries them. The honest move is to
document the exemption, not to imply a guarantee the chain does not provide.
independently, so round-robin DNS can hand an address to the allow that the
deny never saw. The guarantee holds for addresses, not names. This was
already known — it is exactly why the deny-precedence E2E guard uses literal
CIDRs — and it still did not make it into the prose.
missing
physdevmatch warns and continues, because the interface rule isthe one that matches there. Only on a bridged veth is it fatal. The IPv6
bridge toggle is checked separately and went unmentioned.
CI
lxc-e2e.ymlruns the suite on a provisioned Ubuntu runner. No workflowexecuted these scripts at all before this, which is much of how a firewall
that filtered nothing shipped green: the assertions existed and nothing ran
them.
The workflow enables
br_netfilterexplicitly. Without it a bridged vethnever reaches
FORWARD, every rule installs cleanly, nothing fires, and thenetwork tests pass against a firewall that filters nothing — the exact failure
they exist to detect.
MXC_LXC_TESTS_REQUIRE_EXECUTIONturns an honest skip into a failure. Adeveloper 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 four ways:
Stack
Slice 6 of six re-implementing #632 fresh off
main. Sits on #788, #789,#790, #792, and #796; targets
mainso CI runs. Review the top commit.The new workflow's first run on this PR is itself the experiment — if the
GitHub-hosted runner cannot provide LXC or
br_netfilter, strict mode willsay so loudly rather than passing silently.
Microsoft Reviewers: Open in CodeFlow
Refs AB#62830341.