Uh oh!
There was an error while loading. Please reload this page.
[LXC] Eliminate accidental firewall chain name collisions - #780
Merged
Conversation
Two distinct containers could share one iptables chain, so tearing down the first flushed and deleted the second's chain and left it running with no egress filtering. The chain name was built by filtering the container name to `is_alphanumeric() || '-' || '_'`, taking 20 characters, and prepending `MXC-`. Three separate defects fell out of that: - Filtering is lossy, so `a.b` and `ab` both produced `MXC-ab`. - Truncation is lossy, so any two names agreeing on their first 20 sanitized characters produced one chain. - `char::is_alphanumeric` is Unicode-aware and `take(20)` counts chars rather than bytes, so 20 retained characters could reach 80 bytes. Identity now lives in a hash over the original name: `MXC-<slug>-<hash>`, where the hash is the leading 10 bytes of SHA-256 in lowercase base32 (16 characters, 80 bits) and the slug is at most 7 characters kept only so an operator reading `iptables -S` can guess the owner. The slug carries no identity, so neither filtering nor truncation can cause a collision. The 28-character ceiling was measured rather than inferred: `iptables -N` accepts 28 and rejects 29 with "chain name ... too long (must be under 29 chars)". 4 + 7 + 1 + 16 lands exactly on it, and every generated shape was confirmed accepted by both iptables and ip6tables. This addresses accidental collision only. It does not make chain names resistant to an adversary who chooses container names: 80 bits gives a ~2^40 birthday bound, and the 28-character ceiling caps even a hash-only name near 120 bits. Adversarial ownership needs a persisted, verified ownership record, which is left for separate work rather than assumed away here. `sha2` was already in Cargo.lock transitively, so promoting it to a direct dependency adds no new package; the lockfile change is a single edge. Two existing tests encoded the defective contract and were updated: `chain_name_sanitization` asserted `MXC-my-container_123`, and `chain_name_truncation` asserted a 24-character cap. Six teardown fixtures hardcoded literal chain names and now derive them. Verification: 115 lib tests and 21 new black-box spec tests pass, clippy is clean at `-D warnings`, and fmt is clean. The spec tests were written by agents that had not read the implementation, then checked with 17 mutations: 14 were caught. The 3 survivors are equivalent mutants -- a base32 loop bound that is byte-identical at the fixed 10-byte width, a tail-emit branch that is dead because 80 mod 5 is 0, and hashing the reversed name, which is a bijection and so preserves injectivity exactly. 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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Makes LXC/Bubblewrap firewall chain names ASCII-safe, length-bounded, and collision-resistant.
Changes:
- Adds SHA-256/Base32 chain identity with readable slugs.
- Exposes chain names for validation.
- Adds extensive black-box tests and updates dependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Cargo.toml | Adds workspace SHA-256 dependency. |
src/Cargo.lock | Records direct LXC dependency. |
src/backends/lxc/common/Cargo.toml | Enables SHA-256 for LXC common. |
src/backends/lxc/common/src/network_iptables.rs | Implements hashed chain naming and updates tests. |
src/backends/lxc/common/tests/chain_name_spec.rs | Adds chain-name specification tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Two review comments, plus a defect the second one led to. The BASE32_LOWER comment claimed base32 "buys four extra hash bits" over hex, which is wrong. Base32 packs 5 bits per character against hex's 4, so the 16-character hash field carries 80 bits where hex would carry 64, and hex would need 20 characters for the same 80 bits. The comment now says that. The second comment asked for known-answer tests. The PR description had claimed a name-reversing mutation was uncatchable in principle; that was wrong. chain_name_spec.rs gains three KATs pinning literal digests, and a mutation run confirms they catch it: seeding hash_reversed_name now fails three tests, where before it survived. The digests agree three ways -- an independent Python oracle, a second oracle written by an agent that was not allowed to read the implementation, and the shipped Rust. Writing those tests surfaced a defect this PR introduces. Four LXC network test scripts hard-code the pre-hash chain name, for example MXC-CLI-LXC-Network-Inva, which the new derivation can never produce. Three of them also grep for a programmed rule and so would fail loudly on an LXC host. run_lxc_network_invalid_cidr_test.sh is worse: its only chain assertion is a cleanup check against a chain that can no longer exist, so it passed while asserting nothing. None of this shows up in CI, because all four skip without root and LXC. The scripts now derive the chain name from the run's own debug output and assert its shape and length ceiling, rather than restating the derivation in bash and creating a third implementation to keep in sync. Cleanup is checked by diffing MXC-prefixed chains against a snapshot taken before the run, so a chain left behind by an earlier failed run is not blamed on this one. The slug composition rule was undocumented, and the new shape assertion depends on it, so chain_name_for now states it. Verified: cargo fmt, clippy -D warnings, 115 lib tests, 24 spec tests. The three shell helpers were extracted from the shipped script and exercised in WSL as root against real iptables -- 14 of 14, including detection of a deliberately leaked chain and correct non-blaming of a pre-existing one. 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 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/backends/lxc/common/src/network_iptables.rs:240
- This new collision model leaves several comments in this file stale: lines 60–61, 1155–1156, and 1251–1253 still say names truncate at 20 characters and can collide because of sanitization/truncation. Update those explanations to the new hash-collision threat model so the ownership safeguards are documented accurately.
/// The hash is taken over the *original* container name, so container names
/// that differ only in characters the slug drops, or only past the slug's
/// length, still receive different chains. Two names collide only if their
/// SHA-256 digests collide in the leading 80 bits.
tests/scripts/run_lxc_network_ipv6_cidr_test.sh:69
- The explicit “In scope” list names only
network_iptables.rs, the two Cargo manifests, and the new spec test, and the PR says no file outside that list changes. This file plus the other three LXC network scripts are outside that contract. Please either include these script updates in the declared scope and verification or remove them; as written, the frozen scope is inaccurate.
# List the MXC-owned chains a tool currently holds. The chain name is derived
# from a digest of the container name, so a hard-coded literal rots the moment
# that derivation changes, and a cleanup assertion naming a chain that can no
# longer exist passes while testing nothing. Matching the MXC- prefix stays
# correct across naming changes.
src/backends/lxc/common/src/network_iptables.rs:173
- “Unique” overstates this 80-bit truncated hash: the function is collision-resistant, not injective, and the new threat-model documentation explicitly allows hash collisions. Describe the field as collision-resistant so the API documentation matches the implemented guarantee.
This issue also appears on line 237 of the same file.
/// Chain name unique to this container, as built by [`chain_name_for`].
chain_name: String,
Both found by an independent reviewer dispatched against this change. assert_no_new_mxc_chains consumed the chain listing through a process substitution, whose exit status is not the loop's. A failed enumeration therefore read as zero chains and the assertion passed while verifying nothing -- the same vacuous-pass class this PR set out to remove, and a control run confirms the old form printed a clean result against a listing command that does not exist. The listing is now captured first, and a failure to enumerate is a test failure rather than a silent pass. The BASE32_LOWER comment claimed the 28-byte ceiling leaves no room for a 20-character hex hash. That is wrong: MXC- plus 20 hex is 24 characters and fits. Hex fails for a different reason -- MXC-, the slug, and the slug's separator take 12 of the 28 bytes, leaving exactly 16 for the hash, so hex could carry 80 bits only by giving up the slug entirely. The comment now says that. This is the second correction to this comment; the first fixed the bit arithmetic and left the fit claim wrong. Verified: cargo fmt, 115 lib tests, bash -n on all four scripts, and 16 of 16 on the extracted shell helpers exercised in WSL as root against real iptables, now including a control proving the old form passed silently and the new one does not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
The chain-name spec was written against the documented contract, but without
consulting the testing corpus that governs how these suites are meant to be
derived. Re-deriving the suite against it surfaced four gaps, each of which a
plausible mutant slips through:
- `slug_keeps_underscores` -- the contract lists `_` in the slug alphabet, but
every prior vector used only letters, digits, and `-`.
- `slug_preserves_ascii_letter_case` -- only the hash is documented as
lowercased, so an uppercase letter must survive verbatim into the slug. The
prior case tests all varied the hash, never the slug.
- `single_sluggable_char_yields_a_one_char_slug` -- the `{1,7}` upper bound and
the slug-less form were pinned; the lower bound was not.
- `every_output_matches_the_documented_integration_script_shape` -- four bash
integration scripts grep the chain name out of debug logs against
`^MXC-([A-Za-z0-9_-]{1,7}-)?[a-z2-7]{16}$`, which makes that shape a
client-visible contract. The suite checked fragments of it (prefix, charset,
hash alphabet, length) but never the composed shape those scripts depend on.
The shape matcher is hand-rolled rather than pulling in a regex dependency. It
checks `is_ascii()` before splitting the trailing 16 bytes, because a non-ASCII
input would otherwise panic on a char boundary rather than return false.
Mutation testing, not the green run, is the evidence these tests work. Four
mutants of the slug rule -- dropping `_` from the alphabet, lowercasing the
slug, an off-by-one on the length, and reversing it -- are all caught. The
first two are each killed by exactly one test, and it is one of the tests added
here, so both would have survived the previous 24-test suite.
Tests: 24 -> 28, all green; fmt and clippy clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3dFound while re-reading the PR for stale prose. Five comments state, in the present tense, that chain names truncate at 20 characters and can therefore collide across containers. This PR is what makes that false: names are now MXC- plus a 7-character slug plus a 16-character base32 hash over the full container name, capped at 28 bytes, and historical_truncation_collision_is_gone pins exactly that. Leaving the comments in place would have the code assert the opposite of its own test suite. The guard each comment justifies is still needed, so only the reason changes. chain_name_for is a pure function of the container name -- SHA-256 over the name plus a slug drawn from it, with no PID, no timestamp, and no randomness -- so every run of a given container name maps to the same chain. Acting on a chain this attempt did not create can therefore tear down a chain belonging to an earlier or concurrent run of the same name. That is a real hazard and the ownership record is what prevents it; it simply is not a truncation collision. chain_name_for's own doc comment already made this point at the other end of the file, saying the ownership record, not a longer name, is what defends against chosen names. chain_name_truncation is renamed to chain_name_respects_the_length_ceiling. It asserts len() <= CHAIN_NAME_MAX_LEN and never asserted truncation; under this PR a long name is slugged and hashed rather than cut, so the old name described a mechanism that no longer exists. No behavior change. Verified: cargo fmt clean, 119 lib tests and 28 spec tests green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Soham Das (SohamDas2021)
approved these changes
Aug 10, 2026
Uh oh!
There was an error while loading. Please reload this page.
Darren Hoehna (dhoehna) added a commit
that referenced
this pull request
Aug 10, 2026
PR #780 landed on main and gives every chain a hashed `MXC-<slug>-<hash>` name, so the literals these specs matched on ("MXC-acme-web", "MXC-proxy-order", and seven more) no longer name any chain the manager creates. Two of them failed outright. The other five were worse: they filtered the issued commands by the stale name, got an empty list back, and looped zero times, so they passed while asserting nothing. Three of those are the "proxy mode omits X" tests, whose whole job is to notice an unwanted rule. Both are fixed by asking the manager for its own chain name rather than restating it, so the specs follow any future renaming. The three loop-based tests also assert the filtered list is non-empty, which is what would have caught this as a failure instead of a silent pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
Darren Hoehna (dhoehna) added a commit
that referenced
this pull request
Aug 14, 2026
* [LXC] Scrub and apply proxy env vars through the shared helper
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
* Correct the LXC client note: the integration is planned, not wired
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
* [LXC] Pin the proxy hostname instead of rewriting the URL host
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
* Drop a dead bracket guard and document why unbracketing is load-bearing
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
* [LXC] Make an unpinnable proxy address unrepresentable
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
* [LXC] Update proxy address spec for the unpinnable-address fix
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
* [LXC] Fail closed when firewall rules cannot be scoped to the container
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.
* [LXC] Spec the fail-closed contract for unscopeable firewall rules
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.
* [LXC] Hook the firewall chain onto the bridge port so it actually filters
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
* [LXC] Spec the FORWARD hook contract and assert enforcement end to end
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
* [LXC] Make deny rules win over allow rules and fail closed on an unresolvable 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
* [LXC] Spec deny precedence and assert an overlapping allow cannot defeat 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.
* [LXC] Correct the network policy docs and make the E2E suite gate in CI
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.
* [LXC] Set FORWARD to ACCEPT in CI so only MXC rules can block
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
* Keep Bubblewrap startable when no veth exists to scope the chain to
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
* Cover the Bubblewrap veth declaration so deleting it fails a test
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
* Pin the negative case of the missing-veth accessor
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
* [LXC] Admit network.proxy for LXC and reject the forms it cannot reach
Roadmap rows 17 and 22 (AB#62830341) put an LXC container behind a
cooperative proxy, but the parser still refused `network.proxy` for the
LXC backend outright. Add `lxc` to the supported list, and add the three
validations that make the admitted configs the ones that can actually
work.
`network.proxy.localhost` maps to 127.0.0.1, which inside an LXC network
namespace is the *container's* loopback, not the host's. The injected
HTTP(S)_PROXY would point at nothing and the iptables proxy-allow rule
would never match, so the container would silently get no working proxy
under a deny-all-except-proxy policy. A `url`-form proxy whose host is a
loopback literal is unreachable for the same reason, so `host_is_loopback`
rejects 127.0.0.0/8, ::1, bracketed `[::1]`, and the name `localhost`.
`builtinTestServer` is refused because LXC enforces a configured address
with iptables rather than launching the builtin testing proxy.
Rejection is at parse time because all three forms are literals visible
here. A hostname that only *resolves* to loopback is not caught; that
residual gap is recorded in the code comment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* [LXC] Let a caller force lxc-attach to clear the inherited environment
`build_attach_args` emitted `--clear-env` only when the caller supplied a
non-empty env. That is exactly wrong for proxy-env hygiene (roadmap row
22, AB#62830341): once `apply_proxy_env` has scrubbed every inherited
proxy variable the env can legitimately be empty, and the empty case then
fell back to keep-env mode and let `lxc-attach` inherit the whole MXC host
process environment -- HTTP_PROXY, HTTPS_PROXY, and whatever credentials a
CI agent happens to be carrying.
Add `build_attach_args_with_env_control` with an explicit `force_clear_env`
flag and thread it through `attach_run` on both the Linux path and the
Windows clippy stub. `build_attach_args` survives as a `#[cfg(test)]`
wrapper pinning `false`, so the existing argv tests keep asserting the
legacy shape.
The new tests drive `apply_proxy_env` and the argv builder together, so
what is asserted is the observable `lxc-attach` command line rather than
an intermediate boolean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* [LXC] Wire the proxy-env scrub into the production execution path
`wxc_common::proxy_env::apply_proxy_env` shipped with 44 spec tests and
zero production callers -- `git grep apply_proxy_env -- src/backends`
returned nothing, so on LXC every inherited proxy variable reached the
container untouched and no configured proxy was ever injected. That is
roadmap row 22 (AB#62830341) in full, and it was silently dropped when
this branch was re-cut from PR #632.
Call it on the request env immediately before `attach_run` and hand the
returned flag to `force_clear_env`. Both halves matter: the scrub removes
a caller-supplied HTTP_PROXY that would otherwise point the sandbox at an
egress path the policy never authorized, and the flag stops an emptied env
falling back to keep-env mode and inheriting the host's variables instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* [LXC] Restrict egress to the proxy endpoint when one is configured
Roadmap row 17 (N5). A policy that names a network proxy is a statement
that the container reaches the internet through that proxy and not
otherwise, but the chain built for it was the ordinary allow/block chain:
the proxy was injected into the environment and nothing stopped the
container from ignoring it.
Resolve the proxy once, in apply_firewall_rules, before any rule is
installed, and build the chain from that single resolution. A proxied
chain carries the proxy ACCEPTs and its closing DROP and nothing else.
The base exemptions are deliberately absent. There is no port 53 accept
because an unscoped one is a standing DNS-tunnel exfil path through a
posture whose whole point is that the proxy is the only reachable
destination; the container resolves the proxy from its hosts-file pin
instead. There is no loopback or ESTABLISHED,RELATED accept because
neither describes traffic this chain sees. The allow and block lists are
not programmed either: a block entry is redundant under the closing DROP,
and an allow entry naming anything but the proxy contradicts the model.
The catch-all is forced to DROP regardless of defaultPolicy, since an
ACCEPT terminal would make the proxy ACCEPT above it meaningless. The
IPv6 chain gets its closing DROP alone, because the proxy endpoint is
IPv4 -- fail-closed rather than unfiltered.
An IPv6 proxy endpoint is refused explicitly instead of falling through
IPv4 endpoint selection, which would discard it silently and leave a
deny-all container whose proxy was never authorized.
The pin comes from this same resolution rather than a second lookup. Two
lookups of one name can disagree under round-robin DNS, and a container
pinned to an address this chain did not allow cannot reach its proxy at
all.
Every resolved IPv4 address is opened, not just the pinned one. They all
belong to the configured proxy host, so the posture is unchanged, and a
client that resolves the name by some other means still reaches the proxy.
Deny-precedence (row 16) is untouched: proxy mode emits no allow or deny
host rules at all, and the non-proxy path is unchanged.
* [LXC] Pin the proxy host inside the container before running the script
A proxied chain opens no port 53, so a container handed
HTTP_PROXY=http://proxy.example.com:8080 has no resolver to find its
proxy with. Even with one it could pick an address the chain does not
allow, because the firewall authorized the addresses a single lookup on
the host returned.
Write the pin the firewall recorded into the container's /etc/hosts
before the script runs, so the name in the URL resolves to an address the
chain allows. The URL itself is left alone: rewriting its host to an IP
literal breaks SNI and certificate validation for an https:// proxy,
which is why review rejected that approach.
Fail closed if the write does not succeed. Without the pin the proxy is
unreachable, so running the script would only produce a confusing failure
inside a container that can reach nothing.
The command is idempotent, because a container reused with
destroyOnExit=false would otherwise accumulate entries and the first
match in a hosts file wins -- a stale line would shadow the current pin.
It rewrites the file with cat rather than mv, since LXC may bind-mount
/etc/hosts and replacing the inode would leave the container reading the
old one. Only grep, printf, cat, and rm are used, so it runs under
BusyBox. Single-quoting the line is safe by construction: ProxyHostPin
can only be built from a validated hostname and a parsed IpAddr, so it
cannot contain a quote, a space, or a newline.
Also wait for the container network when a proxy is configured. Without
this the proxy connection could be attempted before the veth is up.
* [LXC] Add the deny-all-except-proxy integration test
Ported from PR 632. Proves the model from the outside: with a proxy
configured under defaultPolicy=block/enforcementMode=firewall, the
container reaches the world only through the proxy, and the direct IPv4,
direct IPv6, and DNS paths are all dropped.
The proxy is locally controlled -- a small forward proxy the script starts
on the lxcbr0 gateway address -- so the positive path needs no external
internet and the negative paths target fixed public IPs that never resolve
in-container.
The fixture drift guard runs unconditionally, so the file is never wholly
conditional: it fails loudly if the fixture stops saying what the
assertions assume. The live half exits 77 when a prerequisite is missing,
which run_lxc_all_tests.sh already classifies as a skip and reports
separately, so a skip is never tallied as a pass.
Unproven: this needs Linux, root, LXC, and python3, and no CI job invokes
the LXC suite.
* [LXC] Document the cooperative-proxy posture
schema.md said WSLC was the only own-netns backend supporting the proxy,
which is no longer true, and said nothing about the proxy being enforced
rather than advisory on LXC.
lxc-backend.md gains a section for the posture itself: the four ways a
proxied chain differs from the ordinary one and why each difference is
load-bearing, why only the url form is accepted, what the IPv6 chain
carries, and why the hosts-file pin exists at all now that DNS is closed.
* [LXC] Scope the integration test's DNS claims to what FORWARD can see
The fixture asserted DNS_BLOCKED, which the chain cannot honestly promise.
The chain is hooked into FORWARD, so it governs traffic the host *routes* for
the container. DNS aimed at the bridge gateway itself -- 10.0.3.1, where LXC's
dnsmasq listens -- is delivered locally and traverses INPUT, never FORWARD.
Counting rules installed in both chains during a live run recorded 2 packets on
the INPUT probe and 0 on the FORWARD probe for container DNS.
So the single DNS assertion is split. DNS to an off-bridge resolver is
forwarded traffic, the chain governs it, and it is still asserted as
FORWARDED_DNS_BLOCKED. DNS to the gateway's own resolver is reported as
GATEWAY_DNS_* rather than asserted, so the gap stays visible in the output
instead of becoming either a false pass or a failure of something this work
item does not cover. Closing it needs an INPUT hook, tracked separately.
The same measurement bounds PROXY_OK. The proxy here runs on the host bridge
IP, so its packets also take INPUT (6 on the INPUT probe, 0 in FORWARD) and the
proxy ACCEPT rule is not what admits them. PROXY_OK proves the env-var
injection and the host pin are right and that deny-all did not break the proxy
path; it does not exercise the ACCEPT rule. That rule is covered by the unit
specs in network_iptables_proxy_spec.rs, and in production by an off-host
proxy. The header now says so rather than implying the test proves more than
it does.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Require a firewall enforcement mode for network.proxy
`network.enforcementMode` defaults to `capabilities`, and under that mode
`apply_firewall_rules` returns early without installing a single rule. The
runner, meanwhile, injects HTTP(S)_PROXY unconditionally. A config that named
a proxy but omitted `enforcementMode` therefore produced the worst of both
halves: the environment variables said "everything goes through the proxy",
while egress stayed completely unrestricted. Any client that ignores those
variables -- a raw socket, a statically linked binary, anything hostile --
went straight out. The config read as deny-all-except-proxy and enforced
neither part of it.
Reject that combination at parse time instead of auto-promoting to `firewall`.
Auto-promotion would silently install rules the caller never asked for, which
is the same class of surprise the neighboring Bubblewrap and Seatbelt guards
exist to prevent; they reject rather than reinterpret, and this follows them.
Rejecting is also the honest failure. A caller who wanted an enforced proxy
gets a message naming the setting to add, and a caller who genuinely wanted
cooperative-only proxying learns that LXC does not offer it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Fail closed when an allow rule can outrank an unresolvable deny
Under a denying default an unresolvable blocked host was downgraded to a
warning, reasoning that the chain's closing DROP already covers whatever the
missing rule would have covered. That is true only while the closing DROP is
what the traffic actually reaches. Allow rules are evaluated first, and
`resolve_host` passes CIDRs through unchanged, so a single entry such as
`0.0.0.0/0` legally emits an ACCEPT for the entire address space ahead of it.
The destination the operator explicitly named as blocked is then accepted by a
rule written for an unrelated purpose, and the only trace is a warning line.
The previous note in the doc comment claimed this gap could not be closed
without knowing the address the deny failed to resolve to. That framing is
what hid the fix: the unknown address is the reason to fail, not an obstacle
to deciding. Precisely because the address is unknown, no allow rule can be
shown not to cover it, so deny precedence cannot be established at all. A
deny that cannot be shown to win does not win.
Fail closed on that combination and name both halves in the error, so the
operator is told which blocked host is unresolvable and why the allow list is
implicated. The no-allow case keeps its warning, since with nothing able to
ACCEPT first the original reasoning still holds.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Claim FORWARD hooks before installing them, not after
Ownership was published after each `iptables -I` returned, which still leaves
a window the signal handler cannot see through: the kernel has accepted the
rule, this process has not yet recorded it, and a fatal signal landing in
between finds a snapshot that does not mention the hook. Cleanup then skips
it, and the surviving rule holds a reference that keeps the chain undeletable,
so the leak outlives the process that created it. The physdev hook is the
easiest one to lose this way, but the plain interface hook has the same shape.
Claim each hook before the command runs. That converts the failure mode from
an under-claim to an over-claim, and an over-claimed hook costs nothing:
removal is by full rule specification, which names this attempt's own chain,
so a `-D` matching nothing is a no-op and cannot touch another container.
Chains deliberately keep the old order. Unlike `-I`, which always inserts,
`-N` fails when the name is already taken, and the chain that already exists
in that case belongs to someone else. Claiming a chain up front would let the
rollback of a failed create delete a live chain this attempt never installed,
trading a leak for the removal of another container's enforcement. That is a
worse trade, so it is not made, and the reasoning is recorded next to the code
rather than left to be rediscovered.
The test drives the observable that separates the two orderings: a hook whose
insert did not complete must still be removed by the rollback. It fails
against the previous ordering, which issued the insert and then went straight
to flushing the chain without ever attempting the delete.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Clear a stale proxy host pin when a run pins nothing
Pinning is self-cleaning: the command filters its own marker out of
/etc/hosts before appending the new mapping, so a run that pins always
replaces whatever the last one left. A run that pins *nothing* never reaches
that path, and with destroyOnExit=false the container outlives the run that
wrote the pin. The next execution then starts against an /etc/hosts still
mapping a hostname to an address only the previous policy authorized.
That is a policy bypass rather than untidiness. The firewall rules for the
new run are built from the addresses this run resolves, so a deny can be
programmed against the address a hostname resolves to now while the container
continues to reach the address pinned earlier. The DROP is real, correct, and
aimed at somewhere the traffic no longer goes.
Clear the pin whenever this run has none and did not create the container --
a container this run created cannot be carrying one. Treat a failed clear the
way a failed pin is treated, because a pin that cannot be removed cannot be
reasoned about: the policy is unenforceable, so the script does not run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Derive the chain name in the specs instead of hard-coding it
PR #780 landed on main and gives every chain a hashed
`MXC-<slug>-<hash>` name, so the literals these specs matched on
("MXC-acme-web", "MXC-proxy-order", and seven more) no longer name any
chain the manager creates. Two of them failed outright.
The other five were worse: they filtered the issued commands by the
stale name, got an empty list back, and looped zero times, so they
passed while asserting nothing. Three of those are the "proxy mode
omits X" tests, whose whole job is to notice an unwanted rule.
Both are fixed by asking the manager for its own chain name rather than
restating it, so the specs follow any future renaming. The three
loop-based tests also assert the filtered list is non-empty, which is
what would have caught this as a failure instead of a silent pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Narrow the deny-precedence failure to a catch-all allow
cfd062c failed the apply whenever an unresolvable blocked host sat
beside any programmed allow. That was too broad, and the repository's
own lxc_network_test.json is the counterexample: it allows
api.github.com and blocks evil.example.com, which does not exist. The
allow resolves to a handful of GitHub addresses, the chain's closing
DROP still covers everything else, and nothing there shows the blocked
host is one of those addresses -- yet the apply was rejected. CI caught
it; my local run passed only because the name resolved here.
The check now fires when an allow entry has a prefix length of zero.
That is the case where the deny is *provably* defeated: 0.0.0.0/0 or
::/0 accepts whatever the blocked host turns out to resolve to for the
container, so no further evidence could rescue it.
Narrower allows go back to a warning. I had argued the opposite --
that an unknown deny address means no allow can be shown to miss it --
and the reasoning was symmetric but the consequence was not. Rejecting
an allowlist beside a blocked host that no longer exists makes an
ordinary policy a hard error, and the cheapest way to clear that error
is to delete the blocklist entry. Trading a recorded warning for a
silently shortened blocklist leaves the deployment less protected than
the residual risk being removed.
The tests cover both directions: a /0 allow in either family is fatal,
a literal and a /24 are not. Mutating covers_every_address to never
fire loses the two fatal tests; mutating it to accept any prefix loses
the /24 test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Reject proxy URLs that carry credentials
lxc-attach receives the proxy environment as --set-var command-line
arguments (lxc_bindings.rs:117) and is spawned as a real process
(lxc_bindings.rs:183), so any userinfo in the proxy URL lands in
/proc/<pid>/cmdline, which is world-readable at the default hidepid=0.
ProxyAddress::to_url returns original_url verbatim -- the repository's own
proxy_address_spec.rs:158 asserts credentials survive that round trip -- so
nothing between the config file and argv strips them.
The codebase already treats userinfo as secret when it logs: redact_proxy_url
exists in proxy_env.rs for exactly that, and config_parser has a test
asserting a scheme error does not echo a password. Redaction covered the
logs and missed argv.
Reject the config at parse time instead of trying to redact at the boundary.
Detection and the error message both go through redact_proxy_url, so the two
cannot drift apart and the rejection itself cannot become the leak it guards
against.
Scoped to the LXC backend. bwrap_command.rs:296 has the same shape, but that
line is already on main and this branch does not touch that file, so fixing
it here would widen the diff past what this PR is for.
Mutating the guard to never fire loses two of the new tests; mutating it to
fire on any URL loses four, two of which predate this change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Stop staging /etc/hosts through a predictable /tmp file
The proxy pin filtered /etc/hosts into /tmp/.mxc-hosts and copied it back.
/tmp belongs to the container, the name is fixed, and `>` follows symlinks --
so on a container reused across runs (destroy_on_exit = false, which is what
makes the pin need to be idempotent in the first place) a previous workload
could pre-create that name as a symlink and aim the redirect somewhere else.
The command runs privileged through lxc-attach, so the target could be
another container file or a host path exposed through a writable bind mount.
Measured both forms against a planted symlink. The old one overwrote the
link target; the new one left it untouched and wrote /etc/hosts correctly.
The kept lines now stage in a shell variable instead. That removes a failure
window rather than adding one: the substitution completes before the redirect
truncates /etc/hosts, so nothing that could fail runs against the truncated
file -- where the old form still had a `cat` to survive. It also drops two
external commands, leaving grep and printf.
Verified the emitted shell under both bash and BusyBox ash: empty file,
existing content, four consecutive pins leaving one marker, unpin restoring
the original, unpin with no pin present exiting 0, unpin down to an empty
file, and a missing /etc/hosts.
The unpin test asserted the command contained no `printf` at all. That
worked only while printf was the sole way a line could be written, and
re-emitting the kept lines needs one now. Replaced it with the invariant it
was standing in for, which the test already asserted alongside it: the
marker's single appearance is inside the filter, so no marked line can be
written.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Fail closed on a proxy the enforcement mode will not enforce
apply_firewall_rules returned Ok(true) whenever the enforcement mode was not
firewall or both, which reports success for an enforcement that did not
happen. With a proxy in the same policy that is the dangerous outcome rather
than the safe one: the runner injects HTTP(S)_PROXY from that policy either
way, so the container advertises a proxy and restricts nothing, and any
client ignoring the environment reaches the network directly.
The JSON parser already rejects this combination, but the parser is not the
only door. LxcScriptRunner::execute and mxc_engine::run take an
already-built ExecutionRequest, and NetworkEnforcementMode derives Default as
Capabilities (models.rs:302-309) -- so a policy constructed in code lands in
the unenforced mode without anyone choosing it. The guard belongs in the
layer that can observe whether rules were installed, because that is the
layer every caller passes through.
The refusal is narrow: capabilities without a proxy is still an ordinary
supported no-op. builtinTestServer is covered too, since it enables the
proxy without an address and takes the same injection path, so the gate
cannot key on the address alone.
Mutating the guard to never fire loses the two refusal tests; mutating it to
always fire loses three, two of which predate this change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Carry the reply path for allowed outbound connections
Both FORWARD hooks are ingress-only: one matches `-i <veth>`, the other
`--physdev-in <veth>`. A reply arrives in the opposite direction, matches
neither, and falls through to the host's FORWARD policy. Under Docker's DROP
default that reply is dropped, so an explicitly allowed destination -- and the
proxy itself, when it is off-host -- is unreachable. The chain is fully
populated and enforcing; the connection simply never completes.
The reviewer raised this on the E2E workflow, where `-P FORWARD ACCEPT` hides
it. I first assumed Ubuntu's lxc-net would carry the reply with its usual
`-o lxcbr0 -j ACCEPT`, which would have made this a test-only artifact. The
FORWARD chain captured from a real CI run refutes that: it holds only
DOCKER-USER and DOCKER-FORWARD, with no lxcbr0 rule at any point in the run.
The defect is in the product, not the workflow.
Install a return-path ACCEPT per family, in both attachment forms, scoped to
the container's own port:
-I FORWARD -o <veth> -m state --state ESTABLISHED,RELATED -j ACCEPT
-I FORWARD -m physdev --physdev-out <veth> -m state \
--state ESTABLISHED,RELATED -j ACCEPT
Jumping the reply direction into the MXC chain would have been shorter, since
the chain already opens with an ESTABLISHED,RELATED accept. It is wrong: the
chain's remaining rules are `-d <destination>` egress shapes, so inbound NEW
packets would be tested against them and, under `defaultPolicy: allow`, reach
the chain's closing ACCEPT. That is an inbound enforcement surface acquired by
accident, with the wrong semantics. The state match keeps these rules unable
to admit anything conntrack does not already know about.
Install failure is a warning, not an error. The ingress hook is what confines
traffic to the chain, so losing it fails open and must be fatal; a rule that
only ever ACCEPTs can at worst leave the container less connected. Refusing to
start a container whose policy is fully installed would be the wrong trade.
Ownership is claimed before insertion, matching the hooks: a signal between the
kernel accepting the rule and the process recording it would otherwise leak an
ACCEPT naming a veth the kernel is free to hand to a different container.
Teardown deletes from the same builders, and the return rules do not gate the
chain delete because they never reference the chain.
Thirteen tests, all mutation-tested in both directions. The first pass of the
lifecycle tests was itself defective: the builders are family-agnostic, so an
IPv4 and an IPv6 rule differ only by which binary issued them, and assertions
that ignored the binary were satisfied by whichever family still worked. Two
mutations survived because of it. The assertions now pin the tool.
Still outstanding, and named in the PR description: dropping
`-P FORWARD ACCEPT` from the E2E workflow and asserting the hook packet
counters, so the deny cases cannot pass vacuously.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Run the E2E suite under the forward policy production has
The suite forced `-P FORWARD ACCEPT`. That was load-bearing for a real reason:
the FORWARD hooks were ingress-only, so the reply to an allowed request matched
no MXC rule and was dropped by the policy, and an explicitly allowed
destination looked unreachable. Forcing ACCEPT made the suite pass -- and hid
the defect from every run. It is fixed in 8e1cf62, and leaving ACCEPT in place
would now hide whether that fix works.
The other reason for forcing ACCEPT was vacuity: under DROP a container with no
working hook is equally unreachable, so a deny-only assertion would report
success against a firewall filtering nothing. That is answered by pairing, not
by policy, and the pairing already exists. Every script here that asserts a
block also asserts a reachability in the same run -- the allow cases in
run_lxc_network_enforcement_test.sh and run_lxc_network_deny_precedence_test.sh,
and proxy reachability in run_lxc_network_proxy_test.sh. A broken hook or a
missing return rule fails those loudly under either policy. The remaining
network scripts assert programmed rule shapes and log lines, which do not
depend on the forward policy at all.
Set DROP explicitly rather than inheriting whatever Docker left, so a future
runner image that happens to default to ACCEPT cannot silently weaken the
suite.
This also makes the run the measurement I could not make locally: whether
`--physdev-out` matches bridged return traffic. The ingress direction was
measured at 11 packets against 0 for the interface form; the reverse was not.
If it does not match, the allow cases fail here rather than in a customer's
DROP-policy host.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* Revert "[LXC] Run the E2E suite under the forward policy production has"
This reverts 739d23f. The flip did its job: it failed, and the failure is a
real finding rather than a harness problem.
Under `-P FORWARD DROP` the enforcement suite's allow case failed --
`MXC_NET_BLOCKED` for a destination the policy explicitly allowed -- with the
return-path rules from 8e1cf62 installed and no warning logged. So the rules
install and still do not carry the reply.
The cause is documented in iptables-extensions(8): `--physdev-out` names "a
bridge port via which a packet is going to be sent (for bridged packets
entering the FORWARD and POSTROUTING chains)". A reply from the internet
arrives on the host's uplink and is *routed* toward lxcbr0; the bridge port has
not been selected when FORWARD runs, so the physdev form cannot match. The
interface form cannot match either, because the routing output device is
lxcbr0, not the veth. The direction is asymmetric on purpose: `--physdev-in`
works because the packet demonstrably arrived on the veth, and the ingress
hooks measured 11 packets against 0.
Restoring `-P FORWARD ACCEPT` keeps this suite green while the return path is
scoped correctly. That is not a fix and is not being presented as one; the
gap is recorded in the PR description and on the review thread. The scoping
that can work is the container's own address rather than its port -- the
address is already discovered in lxc_runner.rs `wait_for_network`, which today
logs it and throws it away.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Enforce the no-credential proxy invariant at the runner boundary
130b1c9 rejected credential-bearing proxy URLs, but only in config_parser.
ExecutionRequest and ProxyAddress::from_url are both public, so a caller can
build a request the parser never saw and hand it straight to LxcScriptRunner.
From there apply_proxy_env sets HTTP(S)_PROXY to to_url(), which returns the
original URL verbatim, and build_attach_args_with_env_control turns every
environment entry into a --set-var=KEY=VALUE argument of the lxc-attach process
this backend spawns (lxc_bindings.rs:117). The password lands in
/proc/<pid>/cmdline, world-readable at the default hidepid=0 -- exactly the
exposure the parser guard was added to prevent.
Guard the boundary that actually spawns the process. The check sits ahead of
container creation and firewall programming, so a rejected request leaves no
state to clean up, and the message is built from the redacted URL so the
rejection cannot become the leak it is rejecting.
Both call sites now share one predicate, proxy_url_has_credentials, rather than
each open-coding the test -- the parser previously asked whether redaction
changed the string, which reports a URL whose userinfo is already "***" as
clean. proxy_env_spec.rs pins that case so the weaker form cannot come back.
Tests: 6 spec tests for the predicate, 4 for the runner guard, including the
anti-vacuity case (a credential-free URL must clear the guard) and an ordering
case (the refusal must precede any container work). Six mutations -- guard
removed, message rebuilt from the raw URL, predicate always true, naive
contains('@'), the redaction-comparison implementation, and the guard moved
after the container announcement -- were all caught by assertion failures
rather than by compile errors.
2319 passed, 1 failed (the pre-existing BitLocker D:\secrets test).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* [LXC] Redact the proxy URL in the diagnostics that run before the guard
Raised in review. The LXC credential guard runs after convert_wire_proxy
succeeds, so a credential-bearing URL that fails an earlier check never reaches
it. The host and port diagnostics interpolated the raw url_str, so
"http://alice:hunter2@proxy.example.com" -- no port -- put the password in the
error and the log, which is precisely what the guard downstream exists to
prevent.
Redact once at the top of the block and use that in every diagnostic, rather
than redacting per site. Per-site redaction is what produced this miss: the
scheme error was redacted and the two beside it were not.
redact_proxy_url also gave up when the string had no "://" and returned it
verbatim. url::Url::parse accepts "alice:hunter2@example.com" as scheme
"alice", so such a URL reached the scheme diagnostic with the password intact.
It now redacts the scheme:opaque form too.
Tests: a parser test for a portless credential-bearing URL, plus two spec tests
for the opaque form and its complement. Mutations: the port diagnostic rebuilt
from the raw URL, and the opaque redaction turned into a no-op, were both caught
by assertion failures.
A third mutation -- the host diagnostic rebuilt from the raw URL -- survived,
and I am recording that rather than leaving it implied. It survived because
the branch is unreachable, not because it is untested: for http/https,
url::Url::parse rejects every empty-host input ("empty host") before host_str()
is consulted, and that error path formats the ParseError, not the URL. I probed
it directly with http://alice:hunter2@, http://@, https://alice@,
http://alice:hunter2@/x, http://:8080, and https://user:pw@?q=1 -- all rejected
at parse. The redacted form is kept there anyway since it costs nothing and the
branch would otherwise be a trap for a future scheme.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
* Treat an unreadable sysfs as bridged instead of as directly routed
`veth_is_bridge_enslaved_in` decided bridging with
`root.join(iface).join("master").exists()`. `Path::exists()` folds every
metadata error into `false`, so a masked, unmounted, or permission-denied
`/sys/class/net` was read as a positive "this veth is directly routed"
finding. That boolean gates two things: whether `br_netfilter` is required
at all, and whether a failed physdev FORWARD hook is fatal or a warning. So
the failure was silent and it failed open -- setup reported success while
neither hook could match.
Absence of the sysfs entry is not evidence about the topology.
`discover_veth_interface` parses `lxc-info` output, not sysfs, so the veth
can be known to exist while its sysfs entry is unreadable. The two sources
are independent.
The probe now returns `VethTopology::{Bridged, DirectlyRouted, Unknown}` and
only a positive `DirectlyRouted` finding earns the relaxed treatment.
`Unknown` is handled as bridged, which keeps a failed physdev hook fatal, and
is logged -- without the log line the fail-closed choice is undiagnosable in
the field. The probe uses `symlink_metadata` rather than `exists`, because
`master` is a symlink and `exists()` follows it, reporting a dangling
`master` as absent.
One existing assertion is reversed by this.
`a_missing_interface_directory_is_not_bridge_enslaved` asserted that a
missing interface directory means "not enslaved"; it now asserts `Unknown`.
That is a contract change driven by the finding above, not a convenience, and
the reasoning is rec…
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 freeto 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.
Linked work item: AB#62953349 — [LXC] State-aware network
Summary
Make
NetworkIptablesManager's firewall chain name a total, ASCII-safe,length-bounded, hash-derived function of the container name, so that two
distinct containers can no longer accidentally share one chain.
On
mainthe name isMXC-plus the first 20 characters that survive aUnicode-aware alphanumeric filter. Three distinct names can therefore share
one chain —
a.bandabcollapse to the same string, any two names agreeingon their first 20 retained characters truncate together, and because
char::is_alphanumericaccepts non-ASCII whiletake(20)counts charactersrather than bytes, 20 retained characters can be up to 80 bytes and exceed what
iptables will accept.
A shared chain is fail-open: one container's teardown flushes and deletes
another's chain, leaving the second running with no egress filtering. Same
hazard class as the merged #724, one layer up.
What it does
hash— first 10 bytes ofSHA-256(container_name), base32-lowercase, 16characters, 80 bits. Taken over the original bytes, never the slug, so
neither filtering nor length capping can cause a collision. All identity
lives here.
slug— up to 7 characters of the original name keeping only[A-Za-z0-9_-], so an operator readingiptables -Scan guess whichcontainer a chain belongs to. It carries no identity.
The 28-character ceiling is measured, not inferred:
iptables -Non this hostaccepts 28 and rejects 29 with "chain name … too long (must be under 29
chars)".
XT_EXTENSION_MAXNAMELENis 29 including the NUL.The four
tests/scripts/run_lxc_network_*.shscripts read the chain name backfrom the run's own
--debugoutput and assert its shape and length ceiling,rather than reimplementing SHA-256 and base32 in bash as a third copy of the
derivation. Cleanup is checked by diffing
MXC--prefixed chains against asnapshot taken before the run, so a chain left by an earlier failed run is not
blamed on this one.
Bound on the claim
This fixes accidental collision. It does not make chain names resistant to
an adversary who chooses container names: 80 bits gives a ~2^40 birthday bound,
and the 28-character ceiling caps even an all-hash name near ~120 bits.
Adversarial ownership needs a persisted, verified ownership record, which is
deferred rather than assumed away.
Out of scope
INPUT-chain coverage for host-local traffic.state_aware.rs, the Node SDK, PTY, signal cleanup.Dependency note
sha2is already inCargo.locktransitively, as aredigest,hex, andblake3. Promotingsha2 = "0.10"to a direct dependency adds zero newpackages to the supply chain.
Validation
cargo test -p lxc_common --lib— 119 passed.cargo test -p lxc_common --test chain_name_spec— 28 passed. Writtenby sub-agents that had not read the implementation, per the
blackbox-unit-testsskill.cargo clippy -p lxc_common --all-targets --all-features -- -D warnings—clean.
cargo fmt --check— clean.suite kills entropy-reducing mutants (a narrowed base32 mask halving 80 bits
to 64; a wrong-direction shift collapsing them to 30 with five constant
positions), a lowercase-before-hash mutant that would collide
container-Awith
container-a, and a name-reversing mutant caught by three known-answertests whose digests agree three ways — an independent Python oracle, a second
oracle written by an agent barred from reading the implementation, and the
shipped Rust. The 2 survivors are equivalent at this width:
>=5→>5isbyte-identical across 3,000 inputs because bit boundaries are data-independent
at a fixed 10-byte width, and the tail-emit branch is dead because 80 mod 5
is 0.
28-character maximum
MXC-abcdefg-qrstuvwxyz234567were created and deletedwith both
iptables -Nandip6tables -Nin WSL. All accepted, zeroresidue.
iptables— 16 of16, including rejecting the old name format, detecting a deliberately
leaked chain, and correctly not blaming a pre-existing one. The scripts
themselves need root and LXC and cannot run in CI, where they exit 77.
Microsoft Reviewers: Open in CodeFlow