Skip to content

[Bubblewrap] Support hostname proxy endpoints under proxy-only egress - #940

Open
Soham Das (SohamDas2021) wants to merge 4 commits into
user/sodas/bwrap-proxy-egress-enforcementfrom
user/sodas/bwrap-proxy-host-pin
Open

[Bubblewrap] Support hostname proxy endpoints under proxy-only egress#940
Soham Das (SohamDas2021) wants to merge 4 commits into
user/sodas/bwrap-proxy-egress-enforcementfrom
user/sodas/bwrap-proxy-host-pin

Conversation

@SohamDas2021

@SohamDas2021 Soham Das (SohamDas2021) commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Description

Stacked on #931. Lets network.proxy name a hostname (not just an IP literal) under Bubblewrap proxy-only egress, by porting the ProxyHostPin approach already used by LXC.

sandbox_proxy_address is replaced by SandboxProxy::resolve, which returns both the URL handed to the workload and the ProxyEgress { 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.

  • Hostnames keep their configured URL and are pinned via a private /etc/hosts bind mount, so Host headers and proxy-auth realms still match. With DNS closed, the pin is the only resolution path, so only the first IPv4 answer is opened.
  • IP literals are rewritten to the slirp gateway as before. The 0.0.0.0/:: wildcard rewrites and the specific ::1 rejection from [Bubblewrap] Enforce proxy-only egress for private networking #931 are preserved.
  • resolve_with takes 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_common 122 pass (112 existing + 10 new) · cargo fmt --check · cargo clippy -D warnings · validate-configs.js 234 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

  • The LXC and Bubblewrap resolution helpers are deliberately not shared for now; rationale and the two verified divergences are recorded in [LXC/Bubblewrap] Network policy: egress enforcement, hook scoping, and proxy pinning #896 (comment). Notably :: is rejected by LXC's proxy path but rewritten by Bubblewrap — that looks like an LXC bug, tracked there rather than fixed here.
  • validate now performs a DNS lookup that run deliberately repeats, so a late DNS change is caught at execution rather than trusted from validation time.
  • docs/sandbox-policy/0.8.0/networking/networking.md D5 ("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

📋 Issue Type

  • Bug fix
  • Feature
  • Task

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 GitHub
Actions build; it runs on merge to main, and Microsoft reviewers with write access can trigger it
on a PR with /azp run. See docs/pull-requests.md.

If the dependency-feed-check check fails on a new dependency, the crate must be added to
the feed before the PR can pass. See docs/pull-requests.md
for the steps.

Microsoft Reviewers: Open in CodeFlow

@SohamDas2021
Soham Das (SohamDas2021) requested a review from a team as a code owner August 18, 2026 23:14
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by bwrap_command.rs:334-339, then reopened here with a readable copy of the host file, violating the deniedPaths contract. 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.

Comment thread src/backends/bubblewrap/common/src/proxy_network.rs Outdated
Comment thread src/backends/bubblewrap/common/src/proxy_network.rs Outdated
Comment thread tests/scripts/run_bwrap_network_proxy_test.sh Outdated
Copilot AI review requested due to automatic review settings August 19, 2026 01:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 EXIT trap that removes $STUB_DIR and $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 into cleanup_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'"

Comment thread src/backends/bubblewrap/common/src/proxy_network.rs
Comment thread tests/scripts/run_bwrap_network_proxy_test.sh Outdated
Copilot AI review requested due to automatic review settings August 19, 2026 01:56
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the user/sodas/bwrap-proxy-host-pin branch from 51afca9 to bfd56db Compare August 19, 2026 01:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as 127.0.1.1. A proxy that resolves and binds to 127.0.1.1 will therefore be pinned to 10.0.2.2 but remain unreachable; the E2E test masks this by rebinding every 127.* answer to 127.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 validate performs a DNS lookup and run deliberately 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) {

Comment thread src/backends/bubblewrap/common/src/bwrap_runner.rs
Copilot AI review requested due to automatic review settings August 19, 2026 03:39
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the user/sodas/bwrap-proxy-host-pin branch from bfd56db to 33567cc Compare August 19, 2026 03:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_paths later 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) {

Copilot AI review requested due to automatic review settings August 19, 2026 04:22
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the user/sodas/bwrap-proxy-host-pin branch from 33567cc to c842fb7 Compare August 19, 2026 04:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 validate performs a DNS lookup that run repeats, 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 during spawn. 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(());

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::resolve performs 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 emit PIN_PRESENT_OK and reach the proxy using the pre-existing mapping. Assert that the generated pin is the first /etc/hosts mapping 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 validate performs a DNS lookup that execution repeats, but this path explicitly defers every hostname lookup to spawn (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
Copilot AI review requested due to automatic review settings August 20, 2026 07:18
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the user/sodas/bwrap-proxy-host-pin branch from 5f4ba52 to 5cfb411 Compare August 20, 2026 07:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment on lines +839 to +841
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}"))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 serving localhost;
  • matches case-insensitively, since DNS names are — otherwise Proxy.EXAMPLE survives and reintroduces the competing mapping;
  • passes untouched lines through byte for byte (real /etc/hosts is 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> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI review requested due to automatic review settings August 20, 2026 08:02
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ae9db9df-6439-46c3-9cb4-fd70e7cc43a3

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/8 address is silently changed to the slirp gateway. The new E2E test itself documents that this gateway reaches host 127.0.0.1, not the resolved address (Ubuntu commonly returns 127.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.1 loopback 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 exec has no command here, 2>/dev/null permanently redirects the current shell's stderr rather than applying only to this cleanup operation. cleanup_pin is 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

Copilot AI review requested due to automatic review settings August 20, 2026 08:07
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the user/sodas/bwrap-proxy-host-pin branch from 8410b70 to 6be9651 Compare August 20, 2026 08:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_DIR and BWRAP_STUB_DIR are 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 validate now performs a DNS lookup that run repeats, but this branch explicitly returns success whenever resolution would be needed, leaving the only lookup to spawn. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants