[Bubblewrap] Reject proxy with firewall enforcement, warn on unreachable IPv6 allows, and assert the v6 ingress chain - #1052
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
Hardens Bubblewrap network validation and expands live IPv6 ruleset coverage.
Changes:
- Adds parser/runner parity for proxy plus firewall rejection.
- Reuses validated egress plans and directional-shape detection.
- Tests IPv6/ICMP rendering and both ingress chains. The described IPv6 warning is missing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/backends/bubblewrap/common/src/bwrap_command.rs |
Adds shared directional detection and proxy/firewall validation. |
src/backends/bubblewrap/common/src/bwrap_runner.rs |
Reuses validated egress plans in spawning. |
src/backends/bubblewrap/common/src/network_rules.rs |
Uses shared directional detection. |
src/core/wxc_common/src/config_parser.rs |
Uses the shared rejection message. |
src/core/wxc_common/src/error.rs |
Defines the shared rejection message. |
tests/configs/bubblewrap_network_directional_ipv6_icmp.json |
Adds IPv6 and ICMP rule coverage. |
tests/scripts/run_bwrap_directional_test.sh |
Runs the new rendering test. |
tests/scripts/run_bwrap_inbound_deny_test.sh |
Verifies IPv4 and IPv6 ingress chains. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
78cc2dd to
44b5beb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/backends/bubblewrap/common/src/bwrap_runner.rs:373
- Use the logger's retained warning channel here.
mxc_engine::spawnonly propagateslogger.take_warnings()intoSandbox::warnings();log_linewrites to the temporary buffer, which that path discards. Consequently programmatic callers do not hear this warning, contrary to the intended behavior. The unit test should also assertlogger.warnings()after switching towarning_line.
logger.log_line(&format!(
"WARNING: Bubblewrap allows {} IPv6 destination(s) ({}), but the sandbox \
namespace has no IPv6 connectivity: slirp4netns is launched without \
'--enable-ipv6', so these rules install and are never traversed. The \
destination stays unreachable despite the rule. Use an IPv4 address, or \
network.proxy, if the workload needs to reach it.",
targets.len(),
targets.join(", ")
));
…ws, and assert the v6 ingress chain Co-authored-by: Copilot <[email protected]> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
44b5beb to
201e66d
Compare
| assert!(external_proxy_host_rules_rejection(&r).is_none()); | ||
| } | ||
|
|
||
| /// The parser refuses this pairing; the runner did not, and could not: |
There was a problem hiding this comment.
/// The parser refuses this pairing; the runner did not, and could not: if the parser rejects the combo why have a bubblewrap test?
| /// configs the schema accepts today (see #955). Emitted from `spawn`, which has | ||
| /// a logger and which every caller reaches, so the JSON and programmatic paths | ||
| /// both hear it from one site. | ||
| fn warn_unreachable_v6_targets(plan: &network_rules::EgressPlan, logger: &mut Logger) { |
There was a problem hiding this comment.
Should this be a warning, or an error?
| chain never authorized. An IPv6 rule programs `ip6tables`, but the sandbox's | ||
| namespace has no IPv6 connectivity today — slirp4netns is launched without | ||
| `--enable-ipv6` — so an allowed IPv6 destination stays unreachable regardless | ||
| of the rule (see #955). The terminal verdict of the unmatched family still | ||
| follows `defaultPolicy`, so a v4-only allowlist under `block` does not leave | ||
| IPv6 open. | ||
| of the rule (see #955). MXC emits a warning naming those destinations rather | ||
| than refusing them, since the posture fails closed. The terminal verdict of the | ||
| unmatched family still follows `defaultPolicy`, so a v4-only allowlist under | ||
| `block` does not leave IPv6 open. |
There was a problem hiding this comment.
question (non-blocking): can't tell but is this saying that even with slirp4netns bubblewrap can't do ipv6 filtering?
| // Reject any policy path the invoking user cannot access, so the sandbox | ||
| // never gains access the caller lacks. Runs after object normalization | ||
| // so it sees the already-tightened intents. | ||
| // Delegation check (D3): reject any policy path the invoking user cannot |
There was a problem hiding this comment.
note: this one still says "D3" in the comments.
📖 Description
.github/copilot-instructions.md.Summary
Addresses items 2, 3 and 6 of #955.
Item 2 — runner twin for the proxy + firewall-enforcement rejection. The parser rejects
network.proxycombined withnetwork.enforcementModeoffirewallorboth, butvalidate()had no twin — and could not reach one as written, sinceResolvedNetworkMode::from_requesttestsproxy_activefirst and returnsProxyOnlybeforeenforcementModeis ever consulted. A programmatic caller therefore had an explicitly-set, security-relevant field silently discarded. Addsproxy_with_firewall_rejectionalongside the existing runner checks.Per the issue's ask that the two paths agree "verbatim" and "cannot drift", the message is hoisted into a shared
BWRAP_PROXY_WITH_FIREWALL_MSGinwxc_common::errorand consumed by both layers, rather than hand-duplicated asBWRAP_EXTERNAL_PROXY_HOST_RULESdoes. The parser side is a pure substitution with no behavior change.The gate is deliberately not schema-gated, matching the parser, and only fires when a caller explicitly asks for
firewall/both— the default enforcement mode isCapabilities.Item 3 — warn on IPv6 allows that cannot carry traffic. An IPv6 entry in
allowedHostsvalidates, renders, and installs, butslirp4netnsruns without--enable-ipv6, so the destination is unreachable regardless of the rule. This does not take either remedy the issue proposes: enabling IPv6 needs the RFC 4890 ICMPv6 carve-outs and v6-capable CI, and rejecting IPv6 rule addresses would break a config surface the schema accepts today. Since the posture already fails closed, the defect worth fixing is the silence —spawnnow warns, naming the affected destinations. It lives in the runner so both the JSON and programmatic paths hear it from one site, and denies are excluded, since an unreachable deny is the posture the caller asked for. See the issue discussion for the proposal to close item 3 on this basis.Item 6 — assert the IPv6 ingress chain.
run_bwrap_inbound_deny_test.shinspected onlyiptables, so nothing pinned the v6 chain's content against a live kernel. The per-family assertions are factored intoassert_ingress_chain <tool>and run for bothiptablesandip6tables, including the schema-0.7 absence check. Adds anip6tablesdependency probe so the test skips rather than fails where it is unavailable.Items 4 and 5 are already fixed on
main—PrivateNetworkUse::remedygives firewall-mode callers mode-appropriate advice, andrule_install_timeoutnow addsRULE_INSTALL_HEADROOM, caps atRULE_INSTALL_CEILING, and clamps to therequest's own timeout. Item 1 is not addressed here — see the issue comment.
Stacked on #1051, which extracts the
validate_preparedhelper that item 2 inserts into. Please merge #1051 first.Validation
cargo fmt --all -- --check;cargo clippy --workspace --all-targets -D warningson both the host target andx86_64-unknown-linux-gnucargo test -p bwrap_common -p wxc_commonrun_bwrap_network_proxy_test.sh(14/14),run_bwrap_directional_test.sh(9/9), andsudo tests/scripts/run_bwrap_inbound_deny_test.shall pass, with the newip6tables MXC_INGRESSassertion green — confirmingip6tables -Snormalizes match output identically toiptables -S.Note that item 2's runner gate is unreachable from JSON configs by design: the parser rejects first. It guards the programmatic path only, so it is covered by the unit test
a_proxy_with_firewall_enforcement_is_rejectedrather than by an end-to-end config.Closes #955
📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See docs/pull-requests.md.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow