[LXC] Hook the firewall chain onto the bridge port so it actually filters - #792
Closed
Darren Hoehna (dhoehna) wants to merge 4 commits into
Closed
[LXC] Hook the firewall chain onto the bridge port so it actually filters#792Darren Hoehna (dhoehna) wants to merge 4 commits into
Darren Hoehna (dhoehna) wants to merge 4 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
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Darren Hoehna (dhoehna)
changed the base branch from
user/dahoehna/lxc-net-loopback-validation
to
main
August 9, 2026 03:18
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
This was referenced Aug 9, 2026
Contributor
Author
|
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 |
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
1 participant
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.
The problem
The per-container firewall chain was hooked into
FORWARDwith-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
lxcbr0and thenrouted off it, so by the time
FORWARDsees it the input interface is thebridge, never the veth.
The chain was created correctly, populated correctly, hooked without error,
and traversed by zero packets.
Measured on a live container before this change, with
defaultPolicy: blockand no allowed hosts:
Every counter zero, the closing DROP included, and a fetch from inside the
container succeeded anyway. Adding a counting rule to the same
FORWARDchain on the same traffic separated the two cases cleanly:
-i lxcbr0 -j <counter>-i <veth> -j MXC-…(what the code installed)The fix
Install a second hook per family matching
-m physdev --physdev-in <veth>.That match names the bridge port the packet entered on, so the chain stays
scoped to a single 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
-irule 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 from #790:
bridge-nf-call-{ip,ip6}tablestoggle is absent or0,because bridged packets are then not delivered to iptables at all;
only one its packets can match.
On a directly routed veth the physdev rule is redundant, so a kernel without
the match warns rather than refusing a container that is in fact filtered
correctly.
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.
Why the diff is larger than the earlier slices in this series
A hook that installs cleanly and matches nothing is indistinguishable from a
working firewall in every test this repository had. Shrinking the change to
match the shape of #788/#789/#790 would have meant shipping one half of it.
Verification
Live containers, same binary, same configs, before and after:
block, no allowed hostsblock,api.github.comallowedThe positive control matters as much as the negative one: blocking everything
would also have produced a blocked result.
cargo test -p lxc_common --lib— 144 passed, 0 failed (121 before).cargo clippy -p lxc_common --all-targets -- -D warnings— clean.cargo fmt --check— clean.here — 9 caught, 0 survivors.
Tests
The 23 unit specs were written against the documented contract by an author
who did not read the implementation, and live in their own file.
The new E2E script exists because unit tests cannot see this failure at all.
Every existing network script asserts that the FORWARD hook was installed --
a log line that printed correctly throughout. The new one asserts the
guarantee: a destination the policy does not allow must be unreachable from
inside the container, and an explicitly allowed one must still be reachable.
It was checked in both directions. Against this branch it 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, whichis the regression it exists to catch.
Note for reviewers: no CI workflow currently runs the LXC E2E scripts, which
is a large part of how this shipped unnoticed.
Base
Targets
mainso CI runs: theBuildworkflow only triggers for pull requestswhose base is
main,feature/*, oruser/*, and*does not match/, soa PR based on
user/dahoehna/...gets no build at all.It therefore carries #790's two commits as well as its own, because it rewrites
the hook block immediately adjacent to that PR's
elsebranch in the samefunction. Once #790 merges this diff shrinks to its own two commits with no
action needed. Review commits
f51dc1bandb9946e3; the two below themare #790, already reviewed there.
Not in this PR
next slice. Both rearrange or add rules inside the chain, which is
cosmetic until the chain is reachable -- which is what this PR fixes.
knowing which resolver addresses are legitimate, and no schema field carries
that today; it is also load-bearing, since containers resolve names through
it. It waits on the GA network schema alongside [Bubblewrap/LXC] Address network policy gaps - schema #634 and User/dahoehna/honoring inbound rule #627.
Refs AB#62830341.