feat(sandbox): add sandbox-network to run bwrap commands offline - #255
Merged
Conversation
New `sandbox-network` config key (boolean, default true) and `--sandbox-network <true|false>` CLI flag. When resolved false, the bwrap branch of `wrap_command` appends `--unshare-net`, so each sandboxed bash command runs in a fresh network namespace holding only its own private loopback: a server the command starts and uses within that same command still works on 127.0.0.1, while the internet, the LAN, and anything listening on the host's loopback are unreachable, and nothing survives into the next bash call. Breaking host dev servers, private registries and workflows split across several bash calls is why the default stays true. The /etc/resolv.conf bind and every other mount stay exactly as they were, so the setting is a one-flag diff in the argv. The key is a modifier, not an enforcer: it never implies `sandbox = true`. A session that turns the network off with the sandbox disabled gets one warning through the shared `build_sandbox` warning channel, which both entry points already log once per session. Failed commands get one extra line in the tool result when the network really was unshared and stderr matches a narrow set of no-network failures. It rides the existing mask-hint plumbing, shares its single append pass, and is gated on the same "did the bwrap policy actually run" predicate, which `masking_active` is generalized into.
CONFIG.md gets the key-table row (marked bwrap-only) and a Sandbox network isolation section covering the value-taking CLI flag, the modifier semantics, the untrusted-checkout caveat, and what "no network" actually means here: a private loopback per command, with the host and everything on its loopback gone. SECURITY.md rewrites the "Network access is fully open" gap: masking limits what a command can read and an offline namespace removes the way out, so the two together narrow the exfiltration path for the bash tool, which is all the sandbox ever covers. The per-backend table, the abstract Unix socket consequence, and the best-effort-versus-guarantee section follow. README and GET_STARTED get the same tradeoff in short form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
sandbox-network(config key, defaulttrue) and--sandbox-network[=true|false](CLI flag): when resolvedfalse, the bwrap sandbox runs bash commands with--unshare-net, cutting the command's own network access. Ships with a network-failure hint appended to failed tool results, a proactive notice in the bash tool description, an effect-aware--print-configrow, and a docs pass that states the real semantics, including the parts that stay open.Motivation
PR #245 masked credential directories, which is the "reads your secrets" half of the exfiltration story. This PR adds the other half for the bash tool: with
sandbox-network = false, a sandboxed command cannot ship what it read over the network. SECURITY.md's bwrap gap list has carried "network access is fully open" since #244; this closes that row for users who opt in, while the default keeps today's behavior exactly.Design decisions
true.--unshare-netgives each bash command a fresh network namespace with its own private loopback, so a server started and used within one command still works on 127.0.0.1. What breaks is reaching the host: package registries, already-running dev servers, and anything cross-command, since the namespace dies with the command. That is too disruptive to force on, so offline is opt-in and the docs say precisely what changes.sandbox-exposeand unlikesandbox-required, the key does not switch the sandbox on. Settingfalsewhile the sandbox is off produces a one-shot startup warning, and only pairing withsandbox-requiredturns the promise into a guarantee;--print-configannotates the row with(no effect: ...)whenever the sandbox is off, the backend is not bwrap, or bwrap is not installed, so the readout never claims an isolation that will not happen.could not resolve host,name resolution,network is unreachable,connection refused,cannot assign requested address), matched case-insensitively against stderr with no allocation, gated on the bwrap policy actually applying. The command string is not scanned: a command mentioning a phrase is not failing because of it.connection refusedis included because host-localhost services are the most common casualty, and on systemd-resolved hosts DNS against the bound 127.0.0.53 resolver surfaces as exactly that. The hint text is fixed, so untrusted stderr can trigger the note but never inject content into it, and the user remains the gate for any change; this mirrors the merged mask-hint mechanism from feat(sandbox): mask credential directories inside the bwrap sandbox #245.--ro-bind / /root bind regardless, so skipping the bind hides nothing and would only fork the assembly order.Testing
--all-features(the ACP entry point is feature-gated);cargo fmt --checkandcargo clippy --all-targets --all-features -D warningsboth clean.src/tests/sandbox_network_tests.rs(argv contains--unshare-netiff resolved false, whole-argv byte-identity otherwise, CLI/config precedence in both directions, resolver-bind invariant via an injected resolver path, conflict warning on the warnings channel, hint pattern positives and negatives including esbuild/npm ERESOLVE non-matches, bash description notice on and off) plus--print-configvariants for all effect states insrc/tests/print_config_tests.rs. Tests assert at the bwrap argument assembly layer through the existing injection seams and never touch host bwrap.--print-configshowsfalse (no effect: bwrap is not installed)/(no effect: sandbox is off)/(no effect: bwrap backend only)in the corresponding states, and clap rejects invalid flag values.Reviewing
Start with
src/sandbox.rs: the--unshare-netline inwrap_command's unshare block,NetworkEffect, and the hint patterns are the behavior core. Thensrc/agent/tools/bash.rsfor the single-pass hint append and the description notice,src/cli.rsandsrc/print.rsfor the flag and the row, and SECURITY.md for the threat-model wording. The test diff is large but mostly mechanical: four sandbox test files now share onesandbox_support.rsscaffolding module instead of carrying private copies, which is where most of the deleted lines went. LoC split: logic +310/-33, tests +651/-156, docs +153/-8.Notes
--unshare-netround trip (loopback up within one command, host localhost unreachable, internet unreachable) would harden this and the two earlier sandbox PRs alike; happy to add one in a follow-up if wanted.connect(2)), so/run/docker.sockon a Docker host is a full residual escape. SECURITY.md now states this plainly instead of implying the mounts cover it; masking host socket paths is named there as plausible future hardening, out of scope here.unshare(CLONE_NEWNET)itself is denied (nested containers, restrictive seccomp) fails loudly with bwrap's own error rather than falling back to an online run; failing closed is the intended direction, and the error names bwrap so it is attributable.sandbox-exposesection already had. Whether security keys should be exempt from local override is a pre-existing question that deserves its own discussion rather than a rider here./sysis a host bind, so interface names and MAC addresses remain readable inside the offline namespace; SECURITY.md scopes the "host is unreachable" claim to routing accordingly.2>&1) and localized error text; both are documented as accepted best-effort limits, and a missed hint is harmless.