Repository navigation
[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.
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.
6be9651 to
703e09a
Compare
| /// `build_args` appends this last, so its position identifies where the | ||
| /// options end -- unlike a bare `--` token, which a caller-supplied | ||
| /// environment value can also be. Shared so the two cannot drift apart. | ||
| pub(crate) const COMMAND_TAIL: [&str; 3] = ["--", "sh", "-c"]; |
There was a problem hiding this comment.
If this is always last and only 1 place uses this I suggest to move the constant to where it is used. That'll show the "Has to be on the tail" requirement more clearly.
There was a problem hiding this comment.
It has two users in different modules, which is why it's shared rather than local: bwrap_command.rs:388 appends it, and proxy_network.rs:993-994 reads it back to find where the options end (args[sep..sep + COMMAND_TAIL.len()] == COMMAND_TAIL[..]). Moving it to either site would let the writer and the reader drift apart -- the exact failure the shared constant prevents -- so I'd prefer to keep it here.
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:973
- A root denial is missed here: trimming
"/"yields an empty string, and"/etc/hosts".strip_prefix("")leaves a remainder that does not start with/. A schema-0.8 hostname proxy can therefore be combined withdeniedPaths: ["/"]; the later pin bind re-exposes/etc/hostsdespite that denial. Use component-aware path containment (for example,Path::starts_withwith an empty-path guard) so/and other true ancestors are recognized.
let path = path.trim_end_matches('/');
path == SANDBOX_HOSTS_PATH
|| SANDBOX_HOSTS_PATH
.strip_prefix(path)
.is_some_and(|rest| rest.starts_with('/'))
src/backends/bubblewrap/common/src/bwrap_runner.rs:136
- This deliberately skips DNS during
validate, which contradicts the PR note that validation performs a lookup and execution repeats it. As written, an unresolvable or IPv6-only hostname passesvalidateand fails only inspawn; the new test at lines 1389–1396 explicitly locks in that opposite behavior. Either resolve and discard the validation-time result (while still resolving once for the runtime pin/rule pair), or update the PR contract and rationale to state that hostname reachability is runtime-only.
if let Err(error) = proxy_network::SandboxProxy::check_without_resolving(address) {
Darren Hoehna (dhoehna)
left a comment
There was a problem hiding this comment.
Looks good to me. I don't know enogh about bubblewrap's network model to comment on logic. But the code is sound.
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
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3
703e09a to
c3a2244
Compare
…ch host loopback 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 (1)
tests/scripts/run_bwrap_network_proxy_test.sh:246
- This E2E performs a lookup here to choose the proxy's bind address, while
SandboxProxy::resolveperforms another lookup later to choose the pin/firewall address. On a multi-address or round-robin hostname those calls can select different first IPv4 answers, so the test intermittently points the sandbox at an address where its proxy is not listening—the exact two-lookup failure the production change avoids. Arrange for the test proxy to listen on every eligible local answer (or otherwise provide one controlled hostname/address mapping) rather than binding only this separate lookup's first result.
PROXY_BIND="$(getent ahostsv4 "$PROXY_HOST" 2>/dev/null | awk 'NR==1 {print $1}')"
| /// can no longer select: with DNS closed, the pin is its only resolution path. | ||
| fn resolve_ipv4(host: &str, port: u16) -> Result<Ipv4Addr, String> { | ||
| let resolved: Vec<IpAddr> = (host, port) | ||
| .to_socket_addrs() |
There was a problem hiding this comment.
note: This one is a blocking DNS resolution call that could exceed the amount of time the user added to their timeout config. We can probably use the tokio crate for this so we have asynchronous timeouts. That said, it's pretty heavy.
Thinking about this some more isn't the host always on 127.0.0.1 or [::1] ?
There was a problem hiding this comment.
Fixed! resolve_ipv4 now runs the lookup on a thread with a 15s recv_timeout bound (with_deadline), which is what tokio would give us here anyway since lookup_host also delegates to blocking getaddrinfo; it bounds the wait, not the query, as getaddrinfo can't be cancelled.
On loopback: only builtinTestServer/localhost:<port> bind loopback, and those never reach the resolver; network.proxy.url takes an arbitrary host, which is why the pin exists. DNS is closed in the sandbox, so we resolve on the host and inject /etc/hosts
…startup 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 1 comment.
Suppressed comments (1)
src/backends/bubblewrap/common/src/bwrap_runner.rs:126
- This contradicts the PR description's statement that
validateperforms a DNS lookup andrunrepeats it: this path explicitly accepts every hostname without resolving it, so an unresolvable endpoint also passes validation and--dry-run. Either restore the documented validation behavior or update the PR description to state that DNS-dependent validation is deferred until execution.
// 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.
| thread::Builder::new() | ||
| .name(label.to_string()) | ||
| .spawn(move || { | ||
| // Abandoned on expiry: the receiver is gone, so this send fails | ||
| // and the answer is discarded rather than blocking the thread. | ||
| let _ = sender.send(work()); | ||
| })?; |
There was a problem hiding this comment.
FYI Soham Das (@SohamDas2021), searching online looks like we may potentially be able to get around this by using https://lib.rs/crates/rsdns or https://crates.io/crates/async-std-resolver libraries. But yea even Tokio would have this issue I think.
There was a problem hiding this comment.
Ty! Keeping a note to revisit in the next week.
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