Uh oh!
There was an error while loading. Please reload this page.
Correctly handle no-network with no-proxy configuration. - #1041
Correctly handle no-network with no-proxy configuration.#1041Darren Hoehna (dhoehna) wants to merge 52 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Enables bridge netfilter in the Azure Pipelines Linux SDK integration lane so LXC schema 0.8 firewall enforcement can initialize correctly.
Changes:
- Loads
br_netfilter. - Enables IPv4 and IPv6 bridge-to-iptables processing.
💡 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.
Uh oh!
There was an error while loading. Please reload this page.
9798424 to
f20b21bComparef20b21b to
de013b4CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/backends/lxc/common/src/network_iptables.rs:4716
- This statement was accidentally joined to the function declaration, so the committed file is not rustfmt-formatted and the repository's
cargo fmt --all -- --checkgate will fail.
fn a_legacy_policy_that_installs_nothing_can_still_need_the_network() {
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/backends/lxc/common/src/lxc_runner.rs:234
- This is fail-open when a container is reused while already running.
set_config_itemonly appends startup configuration, but the runner explicitly accepts an existing running container below; in that case the current veth remains active whilepermits_no_networkalso makes both firewall managers skip their chains, so the workload retains the previous run's network access. Handle this transition before skipping enforcement (for example, reject reuse or stop/reconfigure/restart it), and cover adestroyOnExit: falsenetworked-to-default-deny reuse case.
if permits_no_network(&request.policy, uses_directional_schema) {
// `empty` gives the container its own network namespace holding only
// a loopback device; `up` activates it, keeping 127.0.0.1 available
// to a workload that binds it.
for (key, value) in [("lxc.net.0.type", "empty"), ("lxc.net.0.flags", "up")] {
de013b4 to
216e6baCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/backends/lxc/common/src/lxc_runner.rs:235
- This is unsafe for
destroyOnExit=falsereuse.set_config_itemonly appends to the persistent startup config; if the named container is already running, the lateris_running()branch skips restart, so its existing veth remains active whileinstalls_firewallreturns false. The workload then runs with unfiltered network access. Reconcile the active interface before execution (stop/recreate or reject incompatible reuse), and ensure the persistentemptyentry is removed before a later network-enabled reuse.
for (key, value) in [("lxc.net.0.type", "empty"), ("lxc.net.0.flags", "up")] {
if let Err(e) = container.set_config_item(key, value) {
Uh oh!
There was an error while loading. Please reload this page.
216e6ba to
41cdfb1CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/backends/lxc/common/src/lxc_bindings.rs:334
- This cannot remove mounts written by earlier MXC versions, because the old append path produced unmarked
lxc.mount.entrylines. On the first post-upgrade reuse those stale grants are preserved as operator-owned, so a request with no filesystem grants can still inherit a prior MXC mount. Add a migration/provenance strategy, such as recreating legacy reusable containers or persisting ownership metadata.
/// The rewrite goes through a sibling temporary file and a rename, so an
/// interrupted run leaves the previous config intact rather than a
/// half-written one. Passing an empty slice clears the block and writes no
/// replacement, which is what a run carrying no filesystem policy needs.
A schema 0.8 request that blocks by default and names allowedHosts ran with no firewall and full network reach. The allow list was discarded silently: with no directional sections present the request is planned by the legacy rules, those rules ask the enforcement mode first, and the mode nobody set defaults to capabilities, which installs nothing. Every later branch then declined the policy -- the isolated branch requires an empty allow list -- and it fell through to an unfiltered interface. Neither schema licenses that outcome. The 0.7 network field shape lists allowOutbound, allowLocalNetwork, allowedHosts, blockedHosts, and proxy, and 0.8 keeps the same five for legacy compatibility; enforcementMode is in neither list, and 0.8 removed it outright on the rule that the backend enforces the policy or rejects it. Both versions describe the host lists without qualification: an allow list makes only those hosts reachable on a host-filtering backend, and a block list holds even when outbound is allowed. The legacy planner now filters when either host list is named. Withholding the interface entirely still wins where it applies, since granting nothing is stricter than filtering. a_legacy_policy_that_installs_nothing_can_still_need_the_network asserted the discarded-block-list case as expected behavior. Its subject is that a container still needs an interface when no rules are installed, so it keeps that claim and states it through the proxy case its own comment already named. Bubblewrap reaches the same unfiltered outcome from the same shape and is unchanged here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d71a5e03-9c91-4577-81d7-003733cd8757
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/backends/lxc/common/src/lxc_bindings.rs:168
- This override only replaces
lxc.net.0; an existing or operator-customized container can also definelxc.net.1,lxc.net.2, etc., and those interfaces remain configured. Such a container takes theIsolatedplan but still boots with network connectivity, bypassing the no-interface enforcement. Clear or override every configuredlxc.net.*device (or rewrite a managed network block) before starting in this mode.
// `up` keeps 127.0.0.1 available to a workload that binds it.
StartNetwork::NoInterface => {
&["-s", "lxc.net.0.type=empty", "-s", "lxc.net.0.flags=up"]
}
src/backends/lxc/common/src/lxc_bindings.rs:334
- This migration only recognizes newly marker-fenced entries. Containers preserved by a pre-upgrade MXC run contain the old backend-generated, unmarked
lxc.mount.entrylines, andstrip_managed_mount_entriespreserves them as if they were operator entries. Because this PR now restarts reused containers, those stale grants are read again and a later policy granting no paths still inherits them. Add a fail-closed migration strategy (for example, recreate/reject legacy reused containers or persist ownership metadata) rather than silently retaining indistinguishable old grants.
/// Only the lines this backend wrote are touched. They are fenced between
/// marker comments, so mount entries a template or an operator put in the
/// config by hand survive untouched -- as do the entries in the files the
/// config `lxc.include`s, which are never rewritten here.
///
/// The rewrite goes through a sibling temporary file and a rename, so an
/// interrupted run leaves the previous config intact rather than a
/// half-written one. Passing an empty slice clears the block and writes no
/// replacement, which is what a run carrying no filesystem policy needs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
A run that reuses a container stops and restarts it so this run's network policy applies. From that restart onward the running container belongs to this run, and every exit between it and the requested script has to leave the container not running. Four exits did not. Readiness timeout, a missing init PID, and a failed egress or ingress apply all destroyed the container only when the run owned it -- `destroyOnExit: true`, or the run created it. A reused container under `destroyOnExit: false` was left running, started by this run, with none of the rules the policy asked for. A deny, allow list, block list, or proxy policy collapsed to unrestricted egress on an interface that already had an address. The reviewer named this defect on the success path and offered two remedies: stop and restart before applying the config, or tear down the already-running interface. The restart took the first. These four exits needed the second. They now stop the container when the run will not destroy it, matching the proxy-pin failures directly below them, which have always done this. The no-network case this pull request is about is unaffected either way: it starts with no interface at all, so there is nothing to leave exposed. Also corrects the ingress module header, which said the egress chain lives in the host netns hooked into FORWARD. Both chains are created inside the container's own netns and the egress chain hooks OUTPUT. That is what makes stopping the container sufficient to remove them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3846481-b585-40ad-a6e3-3e9de1861244
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/backends/lxc/common/src/lxc_bindings.rs:329
- The marker scheme cannot clean mounts written by released versions. Before this change,
configure_filesystem_mountsappended barelxc.mount.entrylines throughset_config_item; on the first post-upgrade reuse,strip_managed_mount_entriestreats those old MXC grants as operator-owned and preserves them. A later run with no filesystem grants can therefore retain an earlier read/write host mount. Add a migration/version marker that recreates or safely cleans legacy MXC containers before reuse, plus an upgrade-path test.
/// Only the lines this backend wrote are touched. They are fenced between
/// marker comments, so mount entries a template or an operator put in the
/// config by hand survive untouched -- as do the entries in the files the
/// config `lxc.include`s, which are never rewritten here.
Uh oh!
There was an error while loading. Please reload this page.
Commit aa4d630 moved LXC egress enforcement out of the host's FORWARD chain and into the container's own network namespace, where the chain is hooked into OUTPUT. Seven doc comments on this branch still described the old design. Four named the wrong hook point. One described a host-side veth field on the watchdog's rollback record that does not exist; that record holds the container name, the chains created so far, and the init PID. Two said the chains are scoped to the container's veth interface. They are not. The chain is reached from OUTPUT and covers every packet the container sends; it carries an explicit accept for loopback, and a test forbids any rule from using -i. Comment-only. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e3846481-b585-40ad-a6e3-3e9de1861244
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
A 0.8 request that permits no network and names no proxy has two halves to its promise: the container reaches nothing, and it still has its own loopback. The shipped omitted-network test measures only reachability, which was already denied before this fix, so it reads identically on both sides of the change and cannot tell whether the fix worked. This test measures both halves as output. A control fixture that permits one destination must reach it; when the control cannot, the test skips rather than passing, because a blocked case proves nothing once the harness itself is broken. The test says nothing about how the denial is delivered. Asserting the absence of an interface would pin the current mechanism and go red the day the same denial arrives another way, with the behavior unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ee1d734f-d1bf-4cca-9646-01f815796bf6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 48 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/backends/lxc/common/src/lxc_runner.rs:371
- This moves egress enforcement into a namespace the workload controls. LXC attaches the command as container root with
CAP_NET_ADMIN(also acknowledged indocs/lxc-support/lxc-backend.md:270), so a filtered workload can delete theOUTPUThook/chain and then bypass CIDR, port, deny, and proxy restrictions. The isolated no-interface case is safe, but everyFilteredplan loses the host-protected egress boundary; keep enforcement in a host-controlled scope or dropCAP_NET_ADMINbefore executing untrusted code.
let hook_point = match netns_pid {
Some(pid) => EgressHookPoint::ContainerNetns(pid),
None => EgressHookPoint::Unhooked,
src/backends/lxc/common/src/lxc_bindings.rs:344
- This only removes marker-fenced mounts, but containers preserved by the previous implementation contain MXC's appended
lxc.mount.entrylines without these markers. On the first reuse after upgrading, those old grants are treated as operator entries and remain alongside the new block, so restarting the container can still expose paths granted by an earlier run. Add an explicit migration/recreation strategy for legacy reused containers before preserving unmarked entries.
let mut out = Self::strip_managed_mount_entries(&existing);
A run whose setup fails releases the container before returning the error -- destroying one this run created, stopping one it reused. Each of those releases discarded its own result. A stop or destroy that itself failed left the container running with whatever access an earlier policy gave it, and no line anywhere recorded that it was still up. The release now returns its result, and a failure is named in the log. The seven failure sites share one helper rather than each repeating the destroy-or-stop condition, which is what let the discarded result pass unnoticed at every one of them. No disposition changes. release_kind reproduces the condition each site already carried, the stale-pin site included: it sits inside a branch guarded on !container_created and reduces to the destroy_on_exit check it had before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f1db951f-e8e5-448e-9357-41ffe7b2aab8
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 48 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/backends/lxc/common/src/lxc_bindings.rs:167
NoInterfaceoverrides only network slot 0. A reused or operator-provided LXC config may also definelxc.net.1(and later slots), which remain active, so a deny-all run can still receive an interface and network access. Clear or reject every configured network slot before starting an isolated container rather than overriding onlylxc.net.0.
// `up` keeps 127.0.0.1 available to a workload that binds it.
StartNetwork::NoInterface => {
&["-s", "lxc.net.0.type=empty", "-s", "lxc.net.0.flags=up"]
docs/lxc-support/lxc-backend.md:255
- This describes dispatch by schema version, but the parser now dispatches by the fields used. A 0.8 request carrying legacy fields still follows the legacy rule and installs inbound filtering only when it requests a firewall mode, so the current text incorrectly promises a chain for every networked 0.8 request.
Under 0.7.0, inbound filtering is installed only when the configuration
requests a firewall enforcement mode. Under 0.8.0 it is installed whenever the
container is given a network interface; a policy that permits nothing is given
none, and so needs no chain.
src/backends/lxc/common/src/lxc_bindings.rs:344
- This strips only the new marker-fenced block, so containers preserved by an older MXC build retain every unmarked
lxc.mount.entrythat the previousset_config_itemimplementation appended. On the first reuse after upgrade, stopping and restarting the container reapplies those stale filesystem grants even when the current policy grants nothing. Add a safe migration for legacy MXC mount entries (or recreate/refuse legacy persisted containers) before reuse.
let mut out = Self::strip_managed_mount_entries(&existing);
Darren Hoehna (dhoehna)
commented
Sep 1, 2026
Soham Das (@SohamDas2021) -- all three of your comments are resolved, and the change request is now the only thing holding this up. Your three points were about the bridge-netfilter setup step: that None of that survives on the branch. Egress is enforced in the container's own network namespace Two things have landed since you looked:
CI is 26 of 27 green, including Could you re-review and clear the change request when you get a chance? |
There was a problem hiding this comment.
🔵 Needs a closer look
Namespace-local egress is workload-tamperable, uses a recyclable PID, and leaves migration and legacy isolation gaps.
Review details
Suppressed comments (1)
src/backends/lxc/common/src/lxc_bindings.rs:390
- The migration path leaves stale filesystem grants in place. Containers preserved by the previous implementation contain unmarked
lxc.mount.entrylines because it appended them withset_config_item; this stripper treats every such line as operator-owned, so the first run after upgrade with no filesystem grants still inherits the old host bind. Add a safe legacy ownership migration, or refuse/destroy pre-marker reused containers rather than silently retaining those mounts.
if trimmed == MANAGED_MOUNTS_BEGIN {
inside = true;
continue;
- Files reviewed: 47/48 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| } | ||
| Ok(()) | ||
| } | ||
| let hook_args = ["-I", "OUTPUT", "1", "-j", self.chain_name.as_str()]; |
There was a problem hiding this comment.
Egress now enforces in the container's own netns OUTPUT. lxc.cap.drop is set nowhere in the backend and lxc-attach runs without -u, so container root holds CAP_NET_ADMIN in the very namespace these chains live in and can flush them.
Drop net_admin?
| /// deliberately *not* opened: permitting it needs an ACCEPT naming a host | ||
| /// discovered at runtime, and a client whose renewal goes unanswered | ||
| /// rebinds by broadcast at T2 (RFC 2131), which this rule covers. In | ||
| /// practice most clients never reach this chain at all -- `dhcpcd` and |
There was a problem hiding this comment.
The comment states the bypass outright. CAP_NET_RAW is retained and the host FORWARD hook is gone, so AF_PACKET egress is now matched by no chain at all.
Drop net_raw alongside net_admin and add a raw-socket egress test?
| // A run's config reaches only the start it is passed to, and LXC reads | ||
| // the network section only at start. A container an earlier run left | ||
| // running is still on that run's topology. | ||
| if container.is_running() { |
There was a problem hiding this comment.
There is no lock guarding this? Combined with the config rewrite at line 295, can two concurrent runs sharing a container_id stop each other's live container?
Take an advisory lock keyed by LXC root plus container name, held across the whole lifecycle?
| signal_cleanup::set_active_pid(pid); | ||
| } | ||
| } | ||
| None if plan.installs_firewall() => { |
There was a problem hiding this comment.
Add a runner test forcing init_pid() to None with installs_firewall() == true, asserting non-zero exit and no workload output.
| //! `allowLocalNetwork: true`, its 0.8 successor | ||
| //! `network.ingress.default: "allow"`, and | ||
| //! `network.ingress.hostLoopback: "allow"`, which is new in 0.8 and has no 0.7 | ||
| //! equivalent. LXC has a single inbound chain and the policy carries no way to |
There was a problem hiding this comment.
Why are we talking about config versions in a backend?
The ExecutionRequest is a currency that should save us from having to reason about versions.
There was a problem hiding this comment.
| sudo modprobe br_netfilter | ||
| sudo sysctl -w net.bridge.bridge-nf-call-iptables=1 | ||
| sudo sysctl -w net.bridge.bridge-nf-call-ip6tables=1 |
There was a problem hiding this comment.
question (non-blocking): what was this for again? Is it just an external dependency we no longer need?
| An allow that covers every address is detected: under `defaultPolicy: "block"`, | ||
| an `allowedHosts` entry whose prefix length is zero, alongside a `blockedHosts` | ||
| entry that resolved to nothing, fails firewall setup. The allow is evaluated | ||
| before the closing DROP, so it would accept whatever the blocked host resolves | ||
| to for the container, and deny precedence could not hold. | ||
| One gap remains open and is not detected: an `allowedHosts` entry with a | ||
| bounded prefix may still happen to cover the destination whose `blockedHosts` | ||
| rule went unwritten. Deciding that would require the address the failed entry | ||
| was *meant* to resolve to, which is by definition unavailable, so no check over | ||
| the policy text can be complete — and a partial check would imply a guarantee | ||
| this code cannot make. |
There was a problem hiding this comment.
question: had to read this one multiple times, is it saying that there is are gaps in what lxc supports when you have an allowed host entry + default block? With the newer networking schema changes is this problem still the same? Trying to figure out if this is saying we'll still let things through that the user's config implicitly said to block.
| | `KernelIpv6Disabled` | The probe fails **and** the namespace has no active IPv6 | Skips the IPv6 chain and logs that there is no IPv6 egress to filter — safe, because there is nothing to filter | | ||
| | `UnusableButIpv6Active` | The probe fails **and** the namespace has active IPv6 | **Fails firewall setup** rather than applying an IPv4-only policy that would silently leave IPv6 egress unfiltered | | ||
| Both the probe and the IPv6 reading are scoped to the namespace the rules land in, because a host with IPv6 switched off says nothing about the container being filtered — reading the host there would skip the v6 chain while the rules went into a container that had IPv6, leaving it unfiltered. Activity is read from that namespace's `if_inet6`, and its *existence* is the signal rather than its contents: a container whose IPv6 address has not arrived yet presents the same address-less file as one with IPv6 switched off, and the kernel never creates the file at all when IPv6 is disabled at boot. If the file cannot be read for any other reason the state is *unknown* rather than a confirmed "IPv6 is off", so an unreadable state fails closed. |
There was a problem hiding this comment.
question: for the probe and this might not be something that we solve in this PR, is it a separate exe or just a separate SDK function that is ran in proc?
I'm also wondering if we're going into too much implementation details in this file.
| Inbound rules are installed after the container starts and after egress setup completes, so inbound is unfiltered for a short interval at container startup. The workload script is executed only after installation finishes, so no sandboxed code runs during that interval and the exposure is to external traffic only. Narrowing this interval is tracked separately. | ||
| Inbound default-deny is not a containment boundary against the sandboxed workload. Because the chain lives in the container's own network namespace, the workload can reach it: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chain lives in, where it can flush or delete it. The egress chains are not exposed this way: they sit on the host and hook into `FORWARD` by the container's host-side veth, out of the workload's reach. The asymmetry follows from where each chain is installed. Inbound default-deny therefore closes off external reachability — including for services the workload itself starts — for any workload that does not deliberately tear it down, and does not survive one that does. Making inbound enforcement tamper-proof is tracked in issue #854. | ||
| Default-deny is not a containment boundary against the sandboxed workload, in either direction. Because both chains live in the container's own network namespace, the workload can reach them: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chains live in, where it can flush or delete them. Default-deny therefore holds for any workload that does not deliberately tear it down, and does not survive one that does. Making enforcement tamper-proof is tracked in issue #897. |
There was a problem hiding this comment.
note: As long as the contained workload can't change the configuration then we're ok. Is this saying it can change things while in the lxc container?
| IPv6 is classified with the same three-way probe as egress, against the *container* namespace and from the same signal. A host that reports IPv6 disabled says nothing about the namespace actually being filtered. `UnusableButIpv6Active` inside that namespace fails the run closed rather than enforcing an IPv4-only inbound policy that would leave inbound IPv6 open. | ||
| The signal differs because a container is not a long-running host. Egress reads the *contents* of `/proc/net/if_inet6` and treats loopback-only as inactive, which is a fair reading for a host that has been up for a while. A container has not been: `wait_for_network` returns on the first address of *any* family, so a container whose IPv6 address has not arrived yet presents exactly the same address-less file as one with IPv6 switched off. Inbound therefore keys on that file's *existence*, which is stable — the kernel never creates `/proc/<pid>/net/if_inet6` when IPv6 is disabled at boot. A present but address-less file counts as active, so an unusable `ip6tables` fails the run closed instead of installing IPv4-only enforcement that an IPv6 address arriving moments later would slip past. The trade is deliberate and one-directional: this can abort a run that the egress rule would have let proceed, never the reverse. | ||
| The signal is the file's *existence* rather than its contents, because a container is not a long-running host. `wait_for_network` returns on the first address of *any* family, so a container whose IPv6 address has not arrived yet presents exactly the same address-less file as one with IPv6 switched off. Existence is stable — the kernel never creates `/proc/<pid>/net/if_inet6` when IPv6 is disabled at boot — so a present but address-less file counts as active, and an unusable `ip6tables` fails the run closed instead of installing IPv4-only enforcement that an IPv6 address arriving moments later would slip past. The trade is deliberate and one-directional: this can abort a run that a contents-based reading would have let proceed, never the reverse. |
There was a problem hiding this comment.
question: could we get away with making this shorter? Seems pretty implementation heavy. We can probably talk about the main networking points and configs for lxc but I think the internals can stay high level in this file.
| ); | ||
| } | ||
| // ---- managed mount entries ------------------------------------------- |
There was a problem hiding this comment.
note: fyi on the un needed comment
| /// Build a container whose config file lives in a fresh temp directory, | ||
| /// seeded with `body`. Returns the container and its config path. | ||
| fn container_with_config(body: &str) -> (LxcContainer, std::path::PathBuf) { |
There was a problem hiding this comment.
note: this functions should probably return a Result, as the filesystem functions could fail.
| let _ = container.destroy(); | ||
| } | ||
| return ScriptResponse::error(&format!("Failed to start container: {}", e)); | ||
| return ScriptResponse::error(&format!( |
There was a problem hiding this comment.
note: still not super sure on how we end up reusing a container.
| let _ = writeln!(logger, "Starting LXC container..."); | ||
| if let Err(e) = container.start(network) { | ||
| if self.destroy_on_exit || container_created { |
There was a problem hiding this comment.
thought: hmm this one is slightly tricky. I would have expected this OR operator to be an AND operator.
| runner: &LxcScriptRunner, | ||
| container_created: bool, | ||
| ) -> (ScriptResponse, bool) { | ||
| ) -> (ScriptResponse, bool, bool) { |
There was a problem hiding this comment.
note: would probably prefer returning a struct rather than a tuple here for readability reasons.
| and no address management. | ||
| This is the enforcement point the 0.8.0 networking contract specifies, and it | ||
| is where inbound filtering already lives. |
| // The prefix of the chain is the base rules and the lease-maintenance | ||
| // exemptions; the policy-generated host rules start at the first rule | ||
| // naming a destination of its own. The DHCP exemptions also name one, but | ||
| // theirs is link-scoped and fixed, so they belong to the prefix. | ||
| let is_link_scoped_dhcp = |rule: &Vec<String>| { | ||
| matches!( | ||
| argument_after(rule, "-d"), | ||
| Some("255.255.255.255") | Some("ff02::1:2") | ||
| ) | ||
| }; |
There was a problem hiding this comment.
note: for this one, I'm not really sure what it's doing tbh. The ip addresses are for broadcasting on the network. Is this to allow something specific?
| /// Where the egress chain is hooked, which is what decides whether the policy | ||
| /// it holds filters anything at all. | ||
| /// | ||
| /// The two backends sharing this manager are not in the same situation. LXC | ||
| /// owns a container network namespace and can enforce inside it. Unprivileged | ||
| /// Bubblewrap has none it can use: the sandbox either shares the host's | ||
| /// namespace, where a chain would filter the host itself, or holds a private | ||
| /// one bwrap created that MXC does not manage. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum EgressHookPoint { | ||
| /// The container's own network namespace, entered by this PID. The chain | ||
| /// is hooked into that namespace's `OUTPUT`, and every command that builds |
There was a problem hiding this comment.
question: just to confirm, is the PID the right currency ? I've seen it in other files but just want to confirm it's the right/only thing to use.
| /// Nowhere. The chain is built on the host and never hooked, so nothing | ||
| /// traverses it and the policy is not enforced. | ||
| Unhooked, |
There was a problem hiding this comment.
question: so "unhooked" in this case means pretty much unfiltered, no rules will apply?
| /// The rule keeping a container's traffic to its own loopback out of the | ||
| /// policy, which 0.8 requires of every backend holding a private loopback. | ||
| /// |
There was a problem hiding this comment.
note: we should probably not have to talk about the schema versions in the backends. At the end of the day they receive either the directional networking config or the legacy one.
| /// | ||
| /// The reply is a conntrack-established response, so the ingress chain's | ||
| /// `ESTABLISHED,RELATED` accept readmits it and no inbound rule is needed. | ||
| fn build_dhcp_client_exemption_rule_args( |
There was a problem hiding this comment.
question: do we need this? I get that it's adding a broadcast exemption rule but it seems like we're really in the weeds here.
Sideote: Soham Das (@SohamDas2021), on the MXC side I'm thinking about ways we could reduce the maintenance burden for these modules? Since we're not in the Linux kernel I assumed there are higher level third party libraries we could utilize for filtering.
For example, I searched up on crates.io and there looks like a crate that we could use https://crates.io/crates/nftables for creating and deploying rules. Does this crate help reduce the maintenance burden of this module?
Moving egress enforcement into the container's network namespace put both firewall chains where the workload can reach them. A workload holding CAP_NET_ADMIN could take its own interface down or flush the chains, and the confinement ended there. Drop CAP_NET_ADMIN from the bounding set of the process that runs the workload, before it execs. The bounding set cannot be restored, and lxc-attach only ever drops bits, so a caller that arrives already restricted stays restricted. Container init keeps its own capabilities, which is what lets the guest's DHCP client still run. Only CAP_NET_ADMIN is dropped. CAP_NET_RAW governs whether a workload can open an ICMP socket, and the policy schema already lets a request allow protocol "icmp"; taking that capability away would overrule an explicit allow from underneath. Which traffic may leave is the firewall's decision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ebe9b5fb-6168-4895-b4e2-88f72280c10e
There was a problem hiding this comment.
🟡 Changes recommended
The new namespace enforcement remains bypassable through retained capabilities, privileged persisted startup code, legacy mount grants, and additional configured interfaces.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/backends/lxc/common/src/lxc_bindings.rs:380
- This replacement cannot remove mount grants written by earlier MXC versions. The old
configure_filesystem_mountspath appended ordinary, unmarkedlxc.mount.entrylines, whilestrip_managed_mount_entriesdeliberately preserves every line outside these new markers. After upgrading, a preserved container will therefore keep all legacy MXC mounts when this method rewrites the config, and the forced restart will activate those stale grants for the new run. Please add an upgrade-safe migration strategy (or reject/recreate legacy preserved containers) before claiming per-run mount replacement.
let mut out = Self::strip_managed_mount_entries(&existing);
src/backends/lxc/common/src/lxc_bindings.rs:204
NoInterfaceonly overrideslxc.net.0; LXC configs may define additional indexed devices (lxc.net.1, etc.), especially because this runner reuses already-defined/operator-managed containers. Such a container will still start its remaining interfaces under a policy that promises no network. The no-network start path must clear or override every configured network device, or reject/recreate containers whose topology cannot be reduced to loopback only.
// `up` keeps 127.0.0.1 available to a workload that binds it.
StartNetwork::NoInterface => {
&["-s", "lxc.net.0.type=empty", "-s", "lxc.net.0.flags=up"]
}
- Files reviewed: 49/50 changed files
- Comments generated: 3
- Review effort level: Balanced
| /// Dropping it from the *caller's* bounding set is what reaches the workload. | ||
| /// `lxc-attach` reads the container init's live bounding set and issues drops | ||
| /// to match, never restoring a bit its caller already gave up, and a | ||
| /// bounding-set drop survives `execve` and cannot be undone. Container init | ||
| /// keeps everything, so the guest's own DHCP client still configures the | ||
| /// interface at boot. |
| // `libc` does not export this; the value is from linux/capability.h. | ||
| const CAP_NET_ADMIN: libc::c_ulong = 12; | ||
| // SAFETY: `pre_exec` runs between fork and exec, where only | ||
| // async-signal-safe work is permitted. `prctl` is a bare syscall and this | ||
| // closure allocates nothing and captures nothing. | ||
| unsafe { | ||
| command.pre_exec(|| { | ||
| if libc::prctl(libc::PR_CAPBSET_DROP, CAP_NET_ADMIN, 0, 0, 0) != 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| Ok(()) |
| Inbound rules are installed after the container starts and after egress setup completes, so inbound is unfiltered for a short interval at container startup. The workload script is executed only after installation finishes, so no sandboxed code runs during that interval and the exposure is to external traffic only. Narrowing this interval is tracked separately. | ||
| Inbound default-deny is not a containment boundary against the sandboxed workload. Because the chain lives in the container's own network namespace, the workload can reach it: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chain lives in, where it can flush or delete it. The egress chains are not exposed this way: they sit on the host and hook into `FORWARD` by the container's host-side veth, out of the workload's reach. The asymmetry follows from where each chain is installed. Inbound default-deny therefore closes off external reachability — including for services the workload itself starts — for any workload that does not deliberately tear it down, and does not survive one that does. Making inbound enforcement tamper-proof is tracked in issue #854. | ||
| Default-deny is not a containment boundary against the sandboxed workload, in either direction. Because both chains live in the container's own network namespace, the workload can reach them: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chains live in, where it can flush or delete them. Default-deny therefore holds for any workload that does not deliberately tear it down, and does not survive one that does. Making enforcement tamper-proof is tracked in issue #897. |
b69bd19 shipped the capability drop and proved it with a bash script that read the exit status of an ip link add inside the container. That observable cannot tell confinement from a missing tool: the Alpine image ships no iptables, and an absent binary produces the same refusal the script read as success. Every later variant had the same defect, and asserting the value of a net sysctl measured identical with the capability held and with it dropped. The test now reads CapEff and CapBnd from /proc/self/status inside the container and asserts bit 12 is clear. That is kernel-authored state rather than the outcome of an attempt, it is the same on every distribution, and it needs no tool: a missing file yields no CapEff line, which fails. CapBnd is asserted separately because a process holding the capability only in its bounding set can regain it through a setuid binary. The test carries no #[ignore]. It gates on ready(), matching e2e_bubblewrap_characterization.rs. The Linux build lane skips it with a printed reason instead of hiding it from cargo test. Replaces the bash script and config added in b69bd19, along with the worklist entry and probe crate written for that approach. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ebe9b5fb-6168-4895-b4e2-88f72280c10e
The CapBnd assertion answered a different question: whether the capability could be regained later by exec-ing a file that grants it. The test is about what the workload can do, and CapEff answers that on its own. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ebe9b5fb-6168-4895-b4e2-88f72280c10e
capability_bits had one caller after the bounding-set assertion came out. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ebe9b5fb-6168-4895-b4e2-88f72280c10e
There was a problem hiding this comment.
🔵 Needs a closer look
Raw-packet traffic can bypass the new OUTPUT boundary, while reused legacy mounts and extra configured interfaces can survive the intended isolation.
Review details
Suppressed comments (4)
src/backends/lxc/common/src/lxc_bindings.rs:178
- Dropping only
CAP_NET_ADMINdoes not make the in-namespaceOUTPUTchain a containment boundary. Container root still retainsCAP_NET_RAW, which permitsAF_PACKETsockets; this codebase itself notes that such traffic bypasses netfilter (network_iptables.rs:826-828). A workload can therefore emit raw Ethernet frames without traversing these rules. DropCAP_NET_RAWas well and extend the capability E2E assertion to cover both capability bits.
// `libc` does not export this; the value is from linux/capability.h.
const CAP_NET_ADMIN: libc::c_ulong = 12;
// SAFETY: `pre_exec` runs between fork and exec, where only
// async-signal-safe work is permitted. `prctl` is a bare syscall and this
// closure allocates nothing and captures nothing.
unsafe {
command.pre_exec(|| {
if libc::prctl(libc::PR_CAPBSET_DROP, CAP_NET_ADMIN, 0, 0, 0) != 0 {
src/backends/lxc/common/src/lxc_bindings.rs:380
- This only removes marker-fenced blocks created by the new implementation. Containers preserved by an older MXC have the backend's mounts as ordinary unmarked
lxc.mount.entrylines (the previous path appended them viaset_config_item), so they are preserved as if operator-owned and become active again after this PR restarts the container. A later run granting no filesystem paths can therefore retain an older grant. Add a migration strategy for pre-marker containers (for example, detect an ownership/version marker and recreate or reject legacy reusable containers) plus a regression test seeded with the old config shape.
let mut out = Self::strip_managed_mount_entries(&existing);
src/backends/lxc/common/src/lxc_bindings.rs:203
lxc.net.0.type=emptyoverrides only network slot 0. Because this runner reuses any already-defined container, a config containinglxc.net.1(or another additional slot) will still start that interface, violating theIsolatedplan while the log claims there is no interface. Clear or reject the completelxc.net.*configuration for this start rather than overriding only index 0.
// `up` keeps 127.0.0.1 available to a workload that binds it.
StartNetwork::NoInterface => {
&["-s", "lxc.net.0.type=empty", "-s", "lxc.net.0.flags=up"]
docs/lxc-support/lxc-backend.md:270
- This paragraph is now the opposite of the implementation and the new E2E test: every
lxc-attachgoes throughconfine_network_capabilities, which dropsCAP_NET_ADMIN, and the test asserts it is absent from bothCapEffandCapBnd. Update the backend security description so users do not incorrectly conclude that attached workloads can flush the chains.
Default-deny is not a containment boundary against the sandboxed workload, in either direction. Because both chains live in the container's own network namespace, the workload can reach them: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chains live in, where it can flush or delete them. Default-deny therefore holds for any workload that does not deliberately tear it down, and does not survive one that does. Making enforcement tamper-proof is tracked in issue #897.
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The test asks whether one bit is set. The mask was being carried to the assertion and reduced there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ebe9b5fb-6168-4895-b4e2-88f72280c10e
There was a problem hiding this comment.
🟡 Changes recommended
Namespace identity, raw-socket bypass, and multi-interface isolation issues undermine the intended network boundary.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/backends/lxc/common/src/lxc_bindings.rs:204
NoInterfaceoverrides only network slot 0. LXC supports multiple indexedlxc.net.Ndevices, so a reused or pre-existing container withlxc.net.1(or later) still starts with that interface and violates the no-network guarantee. Clear/override every configured network stanza, or reject containers containing additional slots before starting them.
// `up` keeps 127.0.0.1 available to a workload that binds it.
StartNetwork::NoInterface => {
&["-s", "lxc.net.0.type=empty", "-s", "lxc.net.0.flags=up"]
}
src/backends/lxc/common/src/lxc_bindings.rs:180
- Dropping only
CAP_NET_ADMINdoes not make an in-containerOUTPUT/INPUTfirewall a containment boundary. The attached root workload still retainsCAP_NET_RAW, so it can useAF_PACKETsockets to transmit or receive frames below these iptables hooks; this diff itself notes that udhcpc's AF_PACKET traffic never reaches netfilter innetwork_iptables_proxy_spec.rs:489-491. Please also prevent raw packet sockets (for example by droppingCAP_NET_RAWor applying an appropriate seccomp restriction), or keep enforcement at a hook below packet sockets; add an E2E bypass attempt as well.
// `libc` does not export this; the value is from linux/capability.h.
const CAP_NET_ADMIN: libc::c_ulong = 12;
// SAFETY: `pre_exec` runs between fork and exec, where only
// async-signal-safe work is permitted. `prctl` is a bare syscall and this
// closure allocates nothing and captures nothing.
unsafe {
command.pre_exec(|| {
if libc::prctl(libc::PR_CAPBSET_DROP, CAP_NET_ADMIN, 0, 0, 0) != 0 {
return Err(std::io::Error::last_os_error());
}
docs/lxc-support/lxc-backend.md:270
- This paragraph now contradicts the implementation in this PR:
attach_rundropsCAP_NET_ADMINfrom thelxc-attachbounding set, and the new E2E test asserts that attached workloads do not hold it. Update the documentation to distinguish container init (which retains the capability for boot networking) from attached workloads, and document any remaining bypass limitations after resolving the raw-socket issue.
Default-deny is not a containment boundary against the sandboxed workload, in either direction. Because both chains live in the container's own network namespace, the workload can reach them: MXC creates containers from the stock `lxc-create -t download` template and never sets `lxc.cap.drop` or `lxc.cap.keep`, so LXC's defaults apply — the shared default drops only `mac_admin`, `mac_override`, `sys_time`, `sys_module`, and `sys_rawio`, and an unprivileged user-namespace container starts with a full capability set. `lxc-attach` is invoked without `-u` or `-g`, so the workload runs as container root and holds `CAP_NET_ADMIN` in the namespace the chains live in, where it can flush or delete them. Default-deny therefore holds for any workload that does not deliberately tear it down, and does not survive one that does. Making enforcement tamper-proof is tracked in issue #897.
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Balanced
| // The kernel writes the effective capability set as a hex mask on its own line. | ||
| let holds_net_admin = status | ||
| .lines() | ||
| .find(|line| line.starts_with("CapEff:")) | ||
| .and_then(|line| line.split_whitespace().nth(1)) | ||
| .and_then(|mask| u64::from_str_radix(mask, 16).ok()) | ||
| .map(|mask| mask & CAP_NET_ADMIN != 0) | ||
| .unwrap_or_else(|| panic!("the container reported no readable CapEff line\n{status}")); | ||
| assert!( | ||
| !holds_net_admin, | ||
| "the workload holds CAP_NET_ADMIN and can rewrite the firewall confining it\n{status}" | ||
| ); |
Problem
A container whose configuration permits no network was still given a network
interface with deny rules behind it.
Those rules never matched. The host routes container traffic through the
bridge's address rather than bridging it, and packets arrived on a path the
container-scoped rules did not cover. The policy held only because CI loaded
the
br_netfilterkernel module first. The ADO LXC lane went red when thatstopped covering for it.
Two further faults sat in the same area:
routed by the version it declared rather than the keys it carried, and those
keys were ignored.
rules. LXC reads a container's configuration only as it starts, and a later
run's policy never took effect. A run permitting no network at all inherited
an interface and an allow rule from its predecessor, and reported success.
Fix
A container that permits no network is given no interface at all and keeps only
loopback. There is nothing left to filter.
Egress is enforced inside the container's own network namespace, where the
inbound half already lives. That holds on a stock bridge with
bridge-nf-call-iptablesoff. The LXC test lanes and the SDK integration jobno longer load
br_netfilter.The network shape is chosen from the keys a configuration actually uses instead
of the version it declares. A 0.8 request carrying 0.7 network fields is
honored, including a legacy policy that permits nothing.
A run that finds its container already running stops it first. Every run boots
on the topology its own policy asks for.