[Bubblewrap] Support hostname proxy endpoints under proxy-only egress - #940
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
Adds hostname proxy support to Bubblewrap’s schema 0.8+ proxy-only network isolation.
Changes:
- Resolves and pins hostname proxy endpoints to
/etc/hosts. - Keeps firewall authorization and sandbox resolution aligned.
- Adds unit, integration, and documentation coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/backends/bubblewrap/common/src/proxy_network.rs |
Implements resolution, pinning, and hosts-file mounting. |
src/backends/bubblewrap/common/src/bwrap_runner.rs |
Integrates resolved proxies into execution. |
tests/configs/bubblewrap_network_proxy_hostname.json |
Defines the hostname proxy test case. |
tests/scripts/run_bwrap_network_proxy_test.sh |
Runs the new end-to-end test. |
docs/schema.md |
Documents schema-level behavior. |
docs/bwrap-support/bubblewrap-backend.md |
Documents backend semantics and limitations. |
Suppressed comments (1)
src/backends/bubblewrap/common/src/proxy_network.rs:829
- This final bind unconditionally shadows the filesystem policy mounts. For example,
deniedPaths: ["/etc/hosts"]is masked bybwrap_command.rs:334-339, then reopened here with a readable copy of the host file, violating thedeniedPathscontract. Reject this policy combination or preserve the denial instead of overriding it.
[
"--ro-bind".to_string(),
hosts_path.to_string(),
SANDBOX_HOSTS_PATH.to_string(),
],
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
tests/scripts/run_bwrap_network_proxy_test.sh:278
- This replaces the existing
EXITtrap that removes$STUB_DIRand$BWRAP_STUB_DIR, and line 326 later clears the replacement. Consequently those earlier temporary directories leak on the successful path and on failures after this point. Chain the previous cleanup intocleanup_pin(including its explicit call) rather than overwriting and discarding it.
trap cleanup_pin EXIT
tests/configs/bubblewrap_network_proxy_hostname.json:6
- This only proves that the chosen hostname resolves, not that the generated pin is mounted or points to the firewall-authorized address. The host name is selected precisely because it already resolves, and the implementation preserves the host’s existing
/etc/hosts; on a routable-host entry both this check and the proxy request can still pass if the pin bind is absent. Template the expected sandbox-facing IP into the config and assert the exact first resolution/pin mapping so the integration test is non-vacuous.
"commandLine": "bash -c 'set -u; if ! getent hosts \"{{PROXY_HOST}}\" >/dev/null; then echo PIN_MISSING; exit 1; fi; echo PIN_PRESENT_OK; if ! curl -fsSL --max-time 15 https://api.github.com/zen >/dev/null; then echo PIN_PROXY_UNREACHABLE; exit 1; fi; echo PIN_PROXY_OK; timeout 6 bash -c \"exec 3<>/dev/tcp/1.1.1.1/443\" >/dev/null 2>&1; rc=$?; if [ \"$rc\" = 0 ]; then echo PIN_DIRECT_EGRESS_LEAKED; exit 1; fi; echo PIN_DIRECT_BLOCKED_OK'"
51afca9 to
bfd56db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/backends/bubblewrap/common/src/proxy_network.rs:334
- Translating every loopback answer changes the endpoint to host
127.0.0.1, which is not equivalent to another address such as127.0.1.1. A proxy that resolves and binds to127.0.1.1will therefore be pinned to10.0.2.2but remain unreachable; the E2E test masks this by rebinding every127.*answer to127.0.0.1. Preserve the resolved listener when possible, or reject loopback answers that the slirp gateway cannot reach.
fn sandbox_facing_ip(resolved: Ipv4Addr) -> Ipv4Addr {
if resolved.is_loopback() || resolved.is_unspecified() {
SLIRP_HOST_GATEWAY_IP
src/backends/bubblewrap/common/src/bwrap_runner.rs:135
- The PR description states that
validateperforms a DNS lookup andrundeliberately repeats it, but this path explicitly skips resolution for every hostname. As a result, dry-run validation accepts unresolvable and IPv6-only proxy names instead of reporting the stated validation-time error. Either perform the disposable validation lookup here and retain the authoritative runtime lookup, or update the documented validation behavior.
if let Err(error) = proxy_network::SandboxProxy::check_without_resolving(address) {
bfd56db to
33567cc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/backends/bubblewrap/common/src/bwrap_runner.rs:140
- This conflict check runs before Bubblewrap resolves
deniedPaths. For example,deniedPaths: ["/etc/../etc/hosts"](or a symlink resolving to/etc/hosts) passes here;resolve_denied_pathslater rewrites it to/etc/hosts, and the pin bind then overrides that denial. Recheck the finalized, resolved policy before spawning so a hostname proxy cannot expose a path the effective policy denies.
if let Err(error) =
proxy_network::check_hosts_pin_against_policy(address, &request.policy)
{
src/backends/bubblewrap/common/src/bwrap_runner.rs:135
- The PR description says validation performs a DNS lookup that execution repeats, but this call deliberately suppresses hostname resolution (and the new unit test asserts that behavior). Either perform the stated validation lookup or update the PR description so the documented lifecycle matches the implementation.
if let Err(error) = proxy_network::SandboxProxy::check_without_resolving(address) {
33567cc to
c842fb7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/backends/bubblewrap/common/src/proxy_network.rs:204
- The PR description says
validateperforms a DNS lookup thatrunrepeats, but this branch explicitly returns success whenever resolution would be required. As a result, dry-run/validation accepts an unresolvable hostname and the failure appears only duringspawn. Please align the implementation or the PR description so the advertised validation semantics are accurate.
let (needs_lookup, checked) = Self::inspect_without_resolving(configured);
if needs_lookup {
return Ok(());
c842fb7 to
5f4ba52
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/scripts/run_bwrap_network_proxy_test.sh:258
- This adds a second, independent lookup of the proxy hostname before
SandboxProxy::resolveperforms its own lookup. On a multi-address or round-robin host, the test proxy can bind address A while the sandbox pin and firewall authorize address B, making this E2E fail even though the implementation is correct. Bind the test proxy on all local IPv4 interfaces (or otherwise share one recorded resolution) so only the runner selects the pinned address.
PROXY_BIND="$(getent ahostsv4 "$PROXY_HOST" 2>/dev/null | awk 'NR==1 {print $1}')"
tests/configs/bubblewrap_network_proxy_hostname.json:6
- This check does not prove that the generated pin was mounted: Bubblewrap already exposes the host's
/etc/hosts, and this test deliberately uses the host's own hostname, which is commonly mapped there already. A regression that omits the pin can therefore still emitPIN_PRESENT_OKand reach the proxy using the pre-existing mapping. Assert that the generated pin is the first/etc/hostsmapping for{{PROXY_HOST}}so the E2E exercises the new mount rather than merely hostname resolution.
"commandLine": "bash -c 'set -u; if ! getent hosts \"{{PROXY_HOST}}\" >/dev/null; then echo PIN_MISSING; exit 1; fi; echo PIN_PRESENT_OK; if ! curl -fsSL --max-time 15 https://api.github.com/zen >/dev/null; then echo PIN_PROXY_UNREACHABLE; exit 1; fi; echo PIN_PROXY_OK; timeout 6 bash -c \"exec 3<>/dev/tcp/1.1.1.1/443\" >/dev/null 2>&1; rc=$?; if [ \"$rc\" = 0 ]; then echo PIN_DIRECT_EGRESS_LEAKED; exit 1; fi; echo PIN_DIRECT_BLOCKED_OK'"
src/backends/bubblewrap/common/src/bwrap_runner.rs:125
- The PR notes say
validateperforms a DNS lookup that execution repeats, but this path explicitly defers every hostname lookup tospawn(and the new unit test asserts that behavior). As implemented, an unresolvable or IPv6-only hostname passes validation and fails only during execution. Please reconcile the PR description with the intended contract, or perform the advertised validation lookup if that timing is required.
// A hostname is deliberately left to `run`: its verdict needs a lookup,
// and the answer is the pin the sandbox is given, so resolving here
// would either resolve twice or pin an address the egress chain never
// opened.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
…ema reference Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
5f4ba52 to
5cfb411
Compare
| let contents = format!("{}\n{}", pin.hosts_line(), existing); | ||
| fs::write(path, contents) | ||
| .map_err(|error| format!("Bubblewrap: failed to write the proxy hosts pin: {error}")) |
There was a problem hiding this comment.
Confirmed and fixed in 8410b70 — and the failure is stronger than "does not guarantee". Measured through real glibc, with the pin on line 1 and a stale loopback entry left below it:
BEFORE -> ['127.0.0.1', '10.0.2.2', '10.9.9.9'] # loopback returned FIRST
AFTER -> ['10.0.2.2'] # localhost still resolves
So the doc comment this function carried ("the pin goes first because the first match wins in a hosts file") was simply wrong: glibc's files backend collects every matching line and getaddrinfo then re-sorts them by RFC 6724, which promotes loopback above the pin. File order buys nothing.
write_pinned_hosts now strips the pinned name from the preserved entries via strip_host_from_hosts:
- removes only the name, never the whole line, so
127.0.0.1 localhost <proxy>keeps servinglocalhost; - matches case-insensitively, since DNS names are — otherwise
Proxy.EXAMPLEsurvives and reintroduces the competing mapping; - passes untouched lines through byte for byte (real
/etc/hostsis tab-aligned; normalizing it would churn the file for no benefit); - keeps a trailing comment when an entry is fully consumed.
Six tests added, including the requested duplicate-host regression plus an end-to-end check that exactly one mapping for the pinned name survives against the real host file. The now-false rationale in pinned_hosts_file_puts_the_pin_first was corrected too.
| /// the mappings a workload expects (`localhost` above all). They are read | ||
| /// before the file is created, so a read failure cannot leave a half-written | ||
| /// pin behind. | ||
| fn write_pinned_hosts(path: &PathBuf, pin: &ProxyHostPin) -> Result<(), String> { |
There was a problem hiding this comment.
Changed to &Path in 8410b70, but flagging that the stated rationale does not hold: ptr_arg does not fire here, so there was no -D warnings build failure to avoid.
Verified rather than assumed — touched the file to defeat clippy's cache, confirmed there is no allow(clippy::ptr_arg) and no workspace [lints] config, and then validated the methodology by injecting a probe function taking both &String and &PathBuf:
warning: writing `&String` instead of `&str` ... <- fired
(no diagnostic for the &PathBuf parameter) <- did not fire
clippy 1.93 flagged the &String and ignored the &PathBuf. Taking the change anyway since &Path is the more idiomatic signature and every caller already coerces, but the CI-failure premise was incorrect.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/backends/bubblewrap/common/src/proxy_network.rs:334
- A hostname that resolves to any
127/8address is silently changed to the slirp gateway. The new E2E test itself documents that this gateway reaches host127.0.0.1, not the resolved address (Ubuntu commonly returns127.0.1.1), so a valid proxy bound only to that resolved loopback address passes resolution but is then unreachable. Avoid accepting and remapping non-127.0.0.1loopback answers without verifying that the proxy is reachable through the gateway; otherwise reject them with an actionable error.
if resolved.is_loopback() || resolved.is_unspecified() {
SLIRP_HOST_GATEWAY_IP
tests/scripts/run_bwrap_network_proxy_test.sh:283
- Because
exechas no command here,2>/dev/nullpermanently redirects the current shell's stderr rather than applying only to this cleanup operation.cleanup_pinis called explicitly before the subsequent denial and legacy cases, so failures after that point can lose their stderr diagnostics. Remove this redirection or scope it in a subshell/group while closing fd 9.
exec 9>&- 2>/dev/null || true
8410b70 to
6be9651
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/scripts/run_bwrap_network_proxy_test.sh:286
- Installing this trap replaces the earlier EXIT trap at line 159, so
STUB_DIRandBWRAP_STUB_DIRare leaked on both the normal path (the new trap is later cleared) and failures during this test. Remove those directories before replacing the trap, or include their cleanup in the new cleanup path.
trap cleanup_pin EXIT
src/backends/bubblewrap/common/src/bwrap_runner.rs:125
- The PR notes say
validatenow performs a DNS lookup thatrunrepeats, but this branch explicitly returns success whenever resolution would be needed, leaving the only lookup tospawn. Either update the PR description to document the single runtime lookup or implement the stated validation behavior; currently callers validating an unresolvable hostname receive a different result than the description promises.
// A hostname is deliberately left to `run`: its verdict needs a lookup,
// and the answer is the pin the sandbox is given, so resolving here
// would either resolve twice or pin an address the egress chain never
// opened.
Description
Stacked on #931. Lets
network.proxyname a hostname (not just an IP literal) under Bubblewrap proxy-only egress, by porting theProxyHostPinapproach already used by LXC.sandbox_proxy_addressis replaced bySandboxProxy::resolve, which returns both the URL handed to the workload and theProxyEgress { ip, port, pin }the firewall authorizes — from a single lookup. Two lookups can disagree under round-robin or short TTLs, which would pin the sandbox to an address the chain never opened./etc/hostsbind mount, soHostheaders and proxy-auth realms still match. With DNS closed, the pin is the only resolution path, so only the first IPv4 answer is opened.0.0.0.0/::wildcard rewrites and the specific::1rejection from [Bubblewrap] Enforce proxy-only egress for private networking #931 are preserved.resolve_withtakes an injected resolver, so pin behaviour is unit-testable without DNS.Behaviour is unchanged on schema 0.6/0.7, which stay cooperative-only.
Validation
cargo test -p bwrap_common122 pass (112 existing + 10 new) ·cargo fmt --check·cargo clippy -D warnings·validate-configs.js234 configs · full Bubblewrap E2E suite (run_bwrap_all_tests.sh), including a new hostname-pin case (tests/configs/bubblewrap_network_proxy_hostname.json) asserting the pin resolves, egress via the proxy succeeds, and direct egress stays blocked.Notes for reviewers
::is rejected by LXC's proxy path but rewritten by Bubblewrap — that looks like an LXC bug, tracked there rather than fixed here.validatenow performs a DNS lookup thatrundeliberately repeats, so a late DNS change is caught at execution rather than trusted from validation time.docs/sandbox-policy/0.8.0/networking/networking.mdD5 ("remote proxies are out of scope for GA") is left untouched: it is a GA scope decision that LXC already contradicts today, independent of this branch.Related Issues
AB#62864253
Signed the Contributor License Agreement
Linked to an issue
Updated documentation (if applicable)
Updated Copilot instructions (if build, architecture, or conventions changed)
If this PR changes
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 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