Repository navigation
[Bubblewrap] Resolve DNS when the host uses a loopback stub resolver - #1453
Soham Das (SohamDas2021) wants to merge 2 commits into
Conversation
… resolves through a loopback stub Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7decf7b-a659-437e-a056-d3269346bb89
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7decf7b-a659-437e-a056-d3269346bb89
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Resolver fallback can abort or fail silently on non-UTF-8 paths, and the UDP-only firewall guidance omits required TCP fallback.
2 open findings
What changed in this PR
Fixes Bubblewrap DNS resolution when hosts use loopback stub resolvers by routing sandbox DNS through slirp.
Changes:
- Adds loopback resolver detection, pinning, and firewall diagnostics.
- Extracts shared symlink resolution logic.
- Adds documentation, unit tests, and an end-to-end DNS test.
| File | Description |
|---|---|
tests/scripts/run_bwrap_dns_test.sh |
Tests loopback-stub DNS resolution. |
tests/scripts/run_bwrap_all_tests.sh |
Registers the DNS test. |
tests/configs/bubblewrap_network_dns_stub.json |
Provides the E2E policy fixture. |
src/mxc-sdk/src/core/mxc_common/mod.rs |
Exposes the symlink helper. |
src/mxc-sdk/src/core/mxc_common/filesystem_symlink.rs |
Implements shared symlink resolution. |
src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs |
Detects, stages, and mounts resolver pins. |
src/mxc-sdk/src/backends/bubblewrap/common/network_rules.rs |
Evaluates DNS-forwarder firewall admission. |
src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs |
Integrates resolver pinning and warnings. |
src/mxc-sdk/src/backends/bubblewrap/common/bwrap_command.rs |
Clarifies resolver mount behavior. |
docs/backends/bwrap/bubblewrap-backend.md |
Documents loopback resolver handling. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resolver: Option<&proxy_network::ResolverPin>, | ||
| logger: &mut Logger, | ||
| ) { | ||
| if resolver.is_none() || plan.admits_udp(proxy_network::SLIRP_DNS_FORWARDER_IP, DNS_PORT) { |
| } | ||
| }; | ||
|
|
||
| let destination_text = destination.to_str()?.to_string(); |
Gudge (MGudgin)
left a comment
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Review summary
I reviewed HEAD 126e38a against the PR diff. One High, merge-blocking issue is inline: the optional resolver bind can abort a sandbox when a host-real /var/run and the sandbox's synthetic /var/run -> /run disagree. I reproduced the bwrap failure with a minimal mount sequence: without the bind exit 0; with the bind exit 1 (Can't mkdir parents ...: No such file or directory). The other inline comments are nonblocking corrections, diagnostics, or targeted tests.
Claim-only finding (nonblocking)
Medium — a changed resolver target does not always fail open. claim_mismatch at the PR description's statements that "The pin never costs you a sandbox" and a resolver-path change "fails open". ResolverPin::from_resolver checks the destination before starting the supervisor, but bwrap mounts it afterward. In a separate minimal bwrap probe, an absent file under a read-only resolver-directory bind let the sandbox launch without the pin (exit 0), whereas adding the pin failed with Can't create file ...: Read-only file system (exit 1). This verifies the consequence if the file disappears during that interval, not the timing race itself. Recheck immediately before inserting the bind to narrow the interval and warn on detected changes; do not promise that all later changes fail open without an atomic solution.
Other nonblocking observations
- Low (
introduced_by_change) — the real-bwrap E2E does not deterministically exercise a symlinked resolver ancestor.tests/scripts/run_bwrap_dns_test.sh:73publishes its stub at the host's existingreadlink -f /etc/resolv.confresult. A regular-file host never exercises the symlink mount failure. Add a namespace-private symlinked-ancestor fixture, ideally alongside the/var/runregression above. - Low (
introduced_by_change) — the non-NotFound resolver read failure lacks a warning regression test.proxy_network.rs:1450-1455declines the pin with a warning, but no fixture asserts the warning for EISDIR or invalid UTF-8 content.
Verified clean and scope
The proxy-only branch passes resolver: None and ruleless-deny does not start slirp (bwrap_runner.rs:425-473); neither acquires this pin. The new predicate declines routable and IPv6-only host resolvers (proxy_network.rs:1545-1572). Test markers increased from 62 to 71 in network_rules.rs, 122 to 152 in proxy_network.rs, and 0 to 6 in filesystem_symlink.rs; the gaps noted inline are specific missing scenarios, not an absence of tests. I did not run the full test suite while filing this review; the bwrap probes above were run on Ubuntu WSL.
Verified pre-existing, not attributed to this PR: LXC's resolve_through_symlinks implementation in src/mxc-sdk/src/backends/lxc/common/filesystem_mounts.rs is byte-identical between the captured base and head. The existing duplication is context, not a merge condition. The PR branch is behind the reported base (merge-base 1ce2f68, base e157fa4); the captured three-dot diff matches gh pr diff 1453 exactly.
| /// entry is: every component, not only the leaf. Resolving less than that also | ||
| /// made the policy checks compare a spelling the mount would never use. | ||
| fn resolver_target_of(start: &Path) -> Result<PathBuf, String> { | ||
| let resolved = resolve_through_symlinks(start) |
There was a problem hiding this comment.
High (correctness) — host resolution can produce a destination bwrap cannot mount.
Attribution: introduced_by_change — the new resolver pin uses the host-resolved path for a required --ro-bind, but the existing bwrap baseline synthesizes /var/run -> /run (bwrap_command.rs:363). On a host where /var/run is a real directory and /etc/resolv.conf points beneath it, this function retains /var/run/... while the sandbox resolves that spelling through /run. If the corresponding /run parent is absent, bwrap aborts rather than skipping the optional pin. A minimal Ubuntu WSL bwrap probe with that synthetic link exits 0 without the bind and 1 with it (Can't mkdir parents ...: No such file or directory).
Fix: Check the destination against the sandbox's mount/symlink layout before emitting the bind. If it cannot be mounted safely, warn and skip the pin; add a private-namespace regression for the host-real /var/run layout. This is the one issue supporting Changes Requested.
|
|
||
| // Naming the resolver itself is the caller supplying their own, which | ||
| // the pin would otherwise mount over. | ||
| if policy_names_path(policy, &resolver_path.to_string_lossy()) |
There was a problem hiding this comment.
Medium (reliability) — an explicit mount of the host stub silently leaves DNS unreachable.
Attribution: introduced_by_change — this new branch detects a loopback-only host resolver, then returns None with log_line rather than a warning. A caller who followed the backend documentation and grants the resolver target via readonlyPaths still gets a file containing nameserver 127.0.0.53; inside the network namespace that names the sandbox's own loopback. the_decision_declines_a_caller_supplied_resolver even asserts no warning.
Fix: Preserve the caller's explicit resolver choice, but warn that a loopback-only mounted resolver remains unreachable and explain that they need a reachable resolver or must remove the exact-file grant to let the pin apply.
| /// endpoint is rendered without a prefix while a lowered peer keeps one. | ||
| /// An unparseable block reports no match, which can only make a diagnostic | ||
| /// quieter than the chain it describes -- never more permissive. | ||
| fn contains_v4(&self, address: Ipv4Addr) -> bool { |
There was a problem hiding this comment.
Low (maintainability and test coverage) — DNS diagnostics independently interpret rendered CIDR text.
Attribution: introduced_by_change — contains_v4 parses self.text and computes a CIDR mask separately from the existing rule rendering; a future rule representation can diverge from admits_udp without changing the installed chain. The new tests cover /32, /24 and /8, but not this function's special /0 branch, bare-IP branch, or a v6-only rule deciding the v4 verdict. There is no demonstrated disagreement for the rules currently supported, so this is nonblocking.
Fix: Reuse a parsed CIDR representation where practical; add focused admits_udp(10.0.2.3, 53) cases for 0.0.0.0/0, a bare IPv4 address, and a v6-only allow under default deny.
| resolver: Option<&proxy_network::ResolverPin>, | ||
| logger: &mut Logger, | ||
| ) { | ||
| if resolver.is_none() || plan.admits_udp(proxy_network::SLIRP_DNS_FORWARDER_IP, DNS_PORT) { |
There was a problem hiding this comment.
Medium (correctness) — a UDP-only admission check gives the wrong advice for TCP DNS.
Attribution: introduced_by_change — the replacement retains the host's options line, including use-vc, but this newly added diagnostic checks only UDP/53. glibc's resolv.conf(5) specifies that use-vc forces TCP DNS: with only the suggested UDP rule, the warning is suppressed even though the resolver's TCP packets are blocked by a deny-default chain.
Fix: Account for use-vc when evaluating and explaining the required egress rule; check what slirp supports for TCP DNS rather than suggesting that UDP/53 always suffices.
|
|
||
| if let Some((source, destination)) = &self.resolver { | ||
| let source = source | ||
| .to_str() |
There was a problem hiding this comment.
Medium (correctness) — a non-UTF-8 temp path makes an optional pin fatal.
Attribution: introduced_by_change — stage_resolver can successfully write a PathBuf under a valid Unix TMPDIR containing non-UTF-8 bytes, but source.to_str() here returns Err from configure_bwrap; spawn_bwrap then tears down networking and rejects the sandbox. Previously, firewall-enforced runs without the resolver pin did not need this conversion.
Fix: Validate the staged source path while staging and decline this optional pin with an actionable warning before configuring bwrap, preserving the run.
| // Naming the resolver itself is the caller supplying their own, which | ||
| // the pin would otherwise mount over. | ||
| if policy_names_path(policy, &resolver_path.to_string_lossy()) | ||
| || policy_names_path(policy, &destination_text) |
There was a problem hiding this comment.
Medium (test coverage) — the caller-owned resolver exception is not exercised across a symlink.
Attribution: introduced_by_change — this newly added second clause is supposed to protect an explicit mount of the final target, while the first clause protects /etc/resolv.conf itself. Existing from_resolver fixtures use a regular file, where those spellings coincide; the separate policy_names_path test does not exercise the whole decision. A regression could therefore override a caller-supplied resolver on a standard symlinked host without failing the tests.
Fix: Exercise from_resolver with /etc/resolv.conf -> /run/.../stub.conf, separately granting the symlink spelling and the target (readonly and readwrite), and assert no pin.
|
|
||
| Some(Self { | ||
| destination, | ||
| contents: render_pinned_resolv_conf(&host_contents), |
There was a problem hiding this comment.
Low (security) — a resolver file outside the filesystem grants can be copied into the sandbox.
Attribution: introduced_by_change — from_resolver checks explicit denial, but not whether a baseline bind or policy grant exposed the resolved target; the new generated file copies every non-nameserver line. If /etc/resolv.conf points to an ungranted /opt/netcfg/resolv.conf with loopback DNS, the old sandbox has a dangling link, while this pin creates a visible file containing that host file's comments, domains and options. A minimal bwrap probe confirmed that a bind can create and expose /opt/netcfg/resolv.conf without granting /opt.
Fix: Avoid carrying host-file content across an ungranted path without an explicit policy decision: either require the destination to be baseline/policy-visible or restrict copied content and document the exception.
| } | ||
| }; | ||
|
|
||
| let destination_text = destination.to_str()?.to_string(); |
There was a problem hiding this comment.
Low (reliability) — non-UTF-8 destination silently declines the pin.
Attribution: introduced_by_change — destination.to_str()? returns None after detecting loopback-only host DNS but does not use warning_line. The sandbox keeps unreachable DNS without the warning promised for fallback cases.
Fix: Replace the ? with a branch that warns and declines the optional pin. Keep the validated string rather than converting this same destination again during staging.
| }, | ||
| }; | ||
| let resolver = proxy_network::ResolverPin::for_host(&request.policy, logger); | ||
| warn_resolver_blocked_by_egress(&plan, resolver.as_ref(), logger); |
There was a problem hiding this comment.
Low (reliability) — the egress warning can describe an unapplied resolver pin.
Attribution: introduced_by_change — this warning runs before ProxyNetworkNamespace::start calls stage_resolver. If staging fails, the user is told both to allow 10.0.2.3:53 and that the sandbox kept its original loopback resolver; adding that rule cannot fix the latter state.
Fix: Warn about a blocked DNS forwarder only after the replacement was successfully staged and will be included in bwrap's arguments.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
📖 Description
On hosts that resolve through a loopback nameserver —
systemd-resolved's127.0.0.53(the default on Ubuntu and Fedora), a localdnsmasq, Pi-hole — the Bubblewrap sandbox could not resolve any hostname. The sandbox runs in its own network namespace, so that address is its own empty loopback rather than the host's resolver. Connections to numeric addresses kept working, which is why every existing network test passed.Under address filtering, the sandbox is now pointed at slirp's built-in DNS forwarder
10.0.2.3. libslirp rewrites a query sent there to the host's real nameserver and sends it from the host's network namespace, where the loopback resolver is reachable.search,options, and every other directive are carried over unchanged.The replacement only applies when the host's own resolvers are all loopback and at least one is IPv4, so a host that resolves today is untouched, and it never applies when libslirp would have no IPv4 nameserver to forward to. The generated file is mounted over the path the
/etc/resolv.confsymlink chain ends at, resolved through every component — bwrap cannot create a mount point beneath an unresolved symlink and aborts the sandbox when asked to.The pin never costs you a sandbox. It improves on a resolver the sandbox already cannot reach, so every failure gives up the pin rather than the run: an unreadable resolver, a chain that cannot be resolved, a path the filesystem policy denies, and a failure to write the replacement all leave the sandbox exactly as it would have started without this feature, each with a warning naming the cause.
Proxy mode and ruleless-deny are deliberately unchanged: a proxied chain opens no port 53 and the proxy resolves, and an isolated sandbox has no connectivity to resolve with.
Limitations
egress.default: "deny"with rules, the chain still governs: a query to10.0.2.3is dropped unless a rule admits it. This was already true before this change. MXC now warns at launch when it pins the resolver and the chain admits nothing to10.0.2.3:53, naming the rule to add rather than opening the port on the caller's behalf.-d 10.0.2.2/32 -j DROPdoes not cover it. Name resolution is the one host-loopback service an address-filtered sandbox can reach. Documented under Loopback resolvers.🔗 References
Resolves github/copilot-cli#5027
🔍 Validation
New E2E suite
tests/scripts/run_bwrap_dns_test.sh, registered inrun_bwrap_all_tests.sh. It builds the loopback-resolver condition rather than requiring it — re-executing in a private user/mount/network namespace, serving DNS on127.0.0.53, and pointing the resolver at it — so it behaves the same on a developer box and on a runner. It asserts a hostname resolves inside the sandbox, with a host-side control first so a sandbox failure is attributable. Verified it catches the bug: exit 1 against a binary built without the fix, exit 0 with it.The reported failure and the two sandbox-aborting regressions found while reviewing it were each reproduced against the real
lxc-execbefore and after the fix.src/,cargo fmt --all -- --check: passed.cargo clippy -p mxc-sdk --all-targets -- -D warnings: passed on Windows andx86_64-unknown-linux-gnu.cargo test -p mxc-sdk --lib: 1981 passed. Two failures inmxc_ptyandsdk_v1_conformancereproduce identically at this branch's merge-base and are unrelated.cargo test -p mxc-sdk --lib bubblewrap::common: 342 passed.deniedPathsmasking need passwordless sudo, which this host does not provide; CI covers them.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes📋 Issue Type
Microsoft Reviewers: Open in CodeFlow