[LXC] Make deny rules win over allow rules and fail closed on an unresolvable block by dhoehna · Pull Request #796 · microsoft/mxc

Darren Hoehna (dhoehna) · 2026-08-09T20:08:23Z

Problem

iptables evaluates a chain top to bottom and stops at the first matching
rule, so precedence in a chain is decided entirely by emission order.
build_policy_rules_logged emitted every allowedHosts ACCEPT rule before
every blockedHosts DROP rule.

A destination named in both lists was therefore reachable, and any allow entry
broad enough to cover a blocked address — 0.0.0.0/0, or a CIDR containing it
— silently defeated the block. The chain looked fully populated and filtered
nothing on that destination.

Measured on the previous commit's binary, same script and same host:

Programmed iptables rule: -A MXC-CLI-LXC-Net-DenyWins -d 0.0.0.0/0 -j ACCEPT
Programmed iptables rule: -A MXC-CLI-LXC-Net-DenyWins -d 0.0.0.0/0 -j DROP
MXC_NET_ALLOWED

A second defect sits in the same function. An entry that resolves to nothing
produces no rule, and the code warned and continued in every case. Whether
that is a hole depends on the closing default rule:

defaultPolicy: block — the closing DROP already denies the destination.
The unwritten rule was redundant. Warning is correct.
defaultPolicy: allow — the chain ends in ACCEPT, so the unwritten DROP was
the only thing that would have denied that destination. Skipping it
silently converts a deny into an allow.

Change

Emit blockedHosts rules before allowedHosts rules, so a deny always wins
over an overlapping allow.

Return Result from build_policy_rules_logged and fail closed when — and
only when — the default policy is Allow and a blockedHosts entry resolved
to nothing. install_firewall_rules propagates with ?, so the chains
created so far are rolled back and the caller does not receive a container it
believes is confined.

Why not error on every unresolvable deny entry

That was the first design, and it is wrong. tests/configs/lxc_network_test.json:19
carries blockedHosts: ["evil.example.com"] under defaultPolicy: block, and
that hostname is NXDOMAIN — verified by resolving it rather than by reasoning
about it. The blunt predicate would have broken the flagship LXC network
end-to-end config for a case that has no security hole in it, because the
closing DROP already covers it.

Residual gap, deliberately left open

Under a DROP default, an allow entry broad enough to cover a destination whose
deny rule went unwritten still reaches that destination. Detecting it
requires the address the failed entry was meant to resolve to, which is by
definition unavailable. No predicate over the policy text can be complete,
and a partial one would imply a guarantee this code cannot make. It is
documented in the source and in docs/lxc-support/lxc-backend.md rather than
half-fixed.

Tests

Twelve unit tests written from the documented contract by an agent that never
opened network_iptables.rs. A test written after reading the
implementation encodes that implementation's bugs as expected behavior.

Nine mutants, each a mistake a person could plausibly make here — restore the
old order, error on every unresolvable block, error on unresolvable allows,
never error, invert the default-policy test, swap the jump targets, drop the
warning, leak IPv6 into the IPv4 bucket, 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 drops coverage silently while the suite stays
green. Killing mutant 1 proves the replacement exists.

End-to-end guard, and proof that it is a guard

tests/scripts/run_lxc_network_deny_precedence_test.sh 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: a hostname is resolved
separately for each list entry, so round-robin DNS could hand back different
addresses for the allow and the deny and make the verdict depend on which
address the fetch picked.

The control config allows the same destination and blocks nothing, so it must
come back reachable. Without it, a host with no egress would produce the same
blocked verdict on the overlap case and look exactly like a pass.

The guard was run against the previous commit's binary in an isolated
worktree to confirm it discriminates:

binary	rule order	overlap verdict	guard
`b9946e3`	ACCEPT then DROP	`MXC_NET_ALLOWED`	FAIL, exit 1
this PR	DROP then ACCEPT	`MXC_NET_BLOCKED`	PASS

The control passed in both runs, so the difference is the ordering and not a
host that lost its network.

Gates

154 unit tests, clippy -D warnings clean, fmt clean, all seven LXC
end-to-end scripts pass.

Stack

Slice 5 of six re-implementing #632 fresh off main. Sits on #788, #789,
#790, and #792; targets main so CI actually runs — a user/* base matches
no branch filter in Build.yml and yields one check instead of 23. Review
the top two commits.

Microsoft Reviewers: Open in CodeFlow

Refs AB#62830341.

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.

azure-pipelines · 2026-08-09T20:08:32Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

Pull request overview

Hardens LXC firewall enforcement, deny precedence, and failure handling.

Changes:

Adds bridge-aware physdev hooks and fail-closed validation.
Emits deny rules before allow rules and errors on selected resolution failures.
Adds unit and end-to-end coverage.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/backends/lxc/common/src/network_iptables.rs`	Implements firewall ordering, bridge hooks, validation, and cleanup.
`src/backends/lxc/common/src/network_iptables_veth_spec.rs`	Tests missing-veth behavior.
`src/backends/lxc/common/src/network_iptables_forward_hook_spec.rs`	Tests hook construction and topology detection.
`src/backends/lxc/common/src/network_iptables_deny_precedence_spec.rs`	Tests deny ordering and resolution failures.
`tests/scripts/run_lxc_all_tests.sh`	Registers new LXC tests.
`tests/scripts/run_lxc_network_enforcement_test.sh`	Verifies behavioral enforcement.
`tests/scripts/run_lxc_network_deny_precedence_test.sh`	Verifies deny precedence.
`tests/configs/lxc_network_enforcement_deny.json`	Defines default-deny test case.
`tests/configs/lxc_network_enforcement_allow.json`	Defines explicit-allow control.
`tests/configs/lxc_network_deny_precedence_overlap.json`	Defines overlapping-list test case.
`tests/configs/lxc_network_deny_precedence_control.json`	Defines precedence control case.

Suppressed comments (3)

src/backends/lxc/common/src/network_iptables.rs:1180

The deny rules are still appended after build_base_chain_rule_args, whose UDP/TCP port-53 ACCEPT rules are installed first. Because iptables is first-match-wins, traffic to a blocked destination on port 53 never reaches the new DROP rule, so blockedHosts does not actually win over every allow and violates the documented all-ports behavior. Place destination DROP rules ahead of the DNS carve-out (or scope that carve-out to trusted resolvers).

        let policy_rules = Self::build_policy_rules_logged(&self.chain_name, policy, logger)?;

src/backends/lxc/common/src/network_iptables.rs:1283

The IPv6 physdev hook has the same signal race as the IPv4 hook: a signal can snapshot v6_physdev_hook == false after ip6tables succeeds but before the result is assigned and published, leaking the live hook/chain. Publish provisional ownership before installation and reconcile it during rollback.

                created.v6_physdev_hook = Self::install_physdev_hook(

src/backends/lxc/common/src/network_iptables.rs:708

The backend guide still says every malformed/unresolved host entry is warned and skipped (docs/lxc-support/lxc-backend.md:121), but this branch now returns an error for an unresolved blocked host under an allow default. Update the documented contract so users can predict when container startup fails.

                if default_permits && matches!(action, RuleAction::Deny) {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

        for (host, action) in entries {
            let destinations = Self::resolve_host(host);
            if destinations.is_empty() {
+                if default_permits && matches!(action, RuleAction::Deny) {


+    fn veth_is_bridge_enslaved_in(sysfs_net_root: &Path, iface: &str) -> bool {
+        sysfs_net_root.join(iface).join("master").exists()


            created.v4_hook = true;
            Self::publish_created(created);
+
+            created.v4_physdev_hook = Self::install_physdev_hook(


+            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


Darren Hoehna (dhoehna) · 2026-08-09T21:37:11Z

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 main as a single branch, verified as a unit: 158 + 44 tests passing, clippy and fmt clean, 14/14 E2E. Closing this in favor of that one. No content is lost.

Darren Hoehna (dhoehna) and others added 6 commits August 8, 2026 19:01

Copilot AI balanced review requested due to automatic review settings August 9, 2026 20:08

Darren Hoehna (dhoehna) requested a review from a team as a code owner August 9, 2026 20:08

Copilot started reviewing on behalf of Darren Hoehna (dhoehna) August 9, 2026 20:08 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Darren Hoehna (dhoehna) closed this Aug 9, 2026

Navigation Menu

Uh oh!

[LXC] Make deny rules win over allow rules and fail closed on an unresolvable block - #796

[LXC] Make deny rules win over allow rules and fail closed on an unresolvable block#796
Darren Hoehna (dhoehna) wants to merge 6 commits into
mainfrom
user/dahoehna/lxc-net-deny-precedence

Darren Hoehna (dhoehna) commented Aug 9, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Darren Hoehna (dhoehna) commented Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		fn veth_is_bridge_enslaved_in(sysfs_net_root: &Path, iface: &str) -> bool {
		sysfs_net_root.join(iface).join("master").exists()

Uh oh!

Conversation

Darren Hoehna (dhoehna) commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Why not error on every unresolvable deny entry

Residual gap, deliberately left open

Tests

End-to-end guard, and proof that it is a guard

Gates

Stack

Microsoft Reviewers: Open in CodeFlow

Uh oh!

azure-pipelines Bot commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Darren Hoehna (dhoehna) commented Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Darren Hoehna (dhoehna) commented Aug 9, 2026 •

edited

Loading