tools/sandbox: update with new features, variants, and tests by gzvolsky-tm · Pull Request #3566 · thought-machine/please

gzvolsky-tm · 2026-08-03T14:42:06Z

This PR brings several enhancements, reliability improvements, and tests to the sandbox:

Add support for SANDBOX_UID_MAP and SANDBOX_GID_MAP environment variables. This allows mapping multiple UID/GID ranges into the user namespace utilizing newuidmap and newgidmap. Both must be set together.
Add MOUNT_PROC=0 to skip remounting /proc. This provides a workaround for environments (like newer systemd or certain container runtimes) that prevent remounting a partially masked /proc.
Introduce SANDBOX_FILE_MOUNTS to allow bind-mounting a comma-separated list of specific files into the sandbox.
The extra loopback IP address (10.1.1.1, added in Add local IP and default gateway to network for sandbox tool #3310) can now be overridden via SANDBOX_LOCAL_IP, or disabled by setting it to the empty string.
Mount a tmpfs over /dev/shm so nothing can be inadvertently shared through it.
Introduce a pipe (sync_fd) to synchronize the parent and child processes. This prevents the child from hanging indefinitely if the parent dies before the sandbox setup is complete.
Add SIGTERM forwarding from the parent to the sandboxed child process.
Add a README documenting the sandbox, its configurable environment variables, and how to use them with Please.
Add a sandbox_test.go test suite to test the network/mount isolation, /proc remounting, ID mapping, and process lifecycle across all combinations of the environment knobs.
Install uidmap (shadow-uidmap on alpine) in the CI images and delegate root:0:65536 in /etc/subuid and /etc/subgid, since the sandbox execs newuidmap/newgidmap even for root. The sandbox tests must stay manual/localonly until the images are updated.

Behaviour changes

/proc is now mounted read-only. This may affect tests that write to it, e.g. to /proc/self/oom_score_adj.
The TMP_DIR bind mount is now recursive (MS_REC), so submounts of TMP_DIR are visible inside the sandbox.
When TMP_DIR is /tmp or lives under it, the sandbox no longer returns early: it still bind-mounts it to /tmp/plz_sandbox, remounts the root read-only, and changes into it.
The root read-only remount now preserves the existing nosuid/nodev/noexec/noatime mount flags instead of silently dropping them.

Breaking change

Removing nonet_sandbox is a breaking change for downstream repositories that referenced it, and for remote execution workers that invoked a variant binary and relied on its defaults. The migration path is to use a wrapper that defines the environment (see tools/sandbox/README.md).

Resolves #3541

This commit brings several enhancements, reliability improvements, and tests to the sandbox: - Add support for `SANDBOX_UID_MAP` and `SANDBOX_GID_MAP` environment variables. This allows mapping multiple UID/GID ranges into the user namespace utilizing `newuidmap` and `newgidmap`. - Add `noproc_sandbox` and `nonetproc_sandbox` which skip remounting `/proc`. This provides a workaround for environments (like newer systemd or certain container runtimes) that prevent remounting a partially masked `/proc`. - Introduce `SANDBOX_FILE_MOUNTS` to allow bind-mounting a comma-separated list of specific files into the sandbox. - The loopback IP address can now be specified via `SANDBOX_LOCAL_IP`. - Introduce a pipe (`sync_fd`) to synchronize the parent and child processes. This prevents the child from hanging indefinitely if the parent dies before the sandbox setup is complete. - Add `SIGTERM` forwarding from the parent to the sandboxed child process. - Add a README documenting the sandbox, its variants, and configurable environment variables. - Add a `sandbox_test.go` test suite to test the network/mount isolation, ID mapping, and process lifecycle across all sandbox variants.

peterebden · 2026-08-04T08:25:34Z

Maybe let's think about what the structure of these binaries should look like. I'm not sure that it's very optimal to ship multiple different ones with Please for the different combinations of things that it could sandbox - maybe that should be some flag / env var thing instead on a single binary.

…ailures Backward compatibility: - add_local_ip: add 10.1.1.1 to the loopback interface by default again, for backward compatibility with the previous upstream behaviour (#3310). SANDBOX_LOCAL_IP now overrides the address, and setting it to empty string disables it. Build fixes: - Fix test name clash by renaming sandbox_test to sandbox_go_test - Use the name `please_sandbox` consistently - Fix Darwin and FreeBSD builds by updating the contain() stub and moving <string.h> back out of the #ifdef __linux__ block. - Restore static = (CONFIG.get("STATIC_SANDBOX") is not None) so the alpine release links the sandboxes statically (defined in .plzconfig.alpine). - Drop inline from perror_sock: the inline definition without static emits no external symbol, breaking -O0 links. Tests: - Keep SANDBOX_UID_MAP/SANDBOX_GID_MAP outside ids below 65536 so the mappings work in constrained container uid spaces. - Assert network isolation via curl's exit code (7, couldn't connect) instead of error text (wording changed in curl 8). - Install uidmap (shadow-uidmap on alpine) in the CI images and delegate root:0:65536 in /etc/subuid and /etc/subgid, since the sandbox execs newuidmap/newgidmap, which check subordinate id delegation even for root.

Move the sandbox Go test into its own directory to avoid including adjacent C files in the Go package. Fixes ``` level=error msg="[linters_context] typechecking error: C source files not allowed when not using cgo or SWIG: main.c nonet_main.c nonetproc_main.c noproc_main.c sandbox.c" ```

- Restore SIGKILL as the parent-death signal. The sandboxed command is PID 1 in the new namespace, so the kernel discards SIGTERM and the command leaks as an orphan process when the sandbox crashes. - map_ids: return an error when waitpid fails instead of reading an uninitialised status. - Tolerate an absent /dev/shm. - Require SANDBOX_UID_MAP and SANDBOX_GID_MAP to be set together. The other options, defaulting to a trivial mapping or mirroring the other mapping, made an assumption that isn't always correct. - Fix the TMP_DIR-under-/tmp check to exclude `/tmp[^/]+`. - Exec new*idmap via PATH search. This is safe because the sandbox binary itself runs with neither setuid bits nor file capabilities. - Fix fmt.Sprintf lint errors.

- Remove `nonet_sandbox`, `noproc_sandbox`, and `nonetproc_sandbox`. Under local execution Please sets `SHARE_NETWORK` and `SHARE_MOUNT` explicitly on every sandboxed invocation, so the variants' flipped defaults only took effect where something else invoked them, e.g. remote execution workers. - Add `MOUNT_PROC=0` option to `please_sandbox` to allow disabling `/proc` remounting. - Add README section explaining how to use the environment variables with Please. - Refactor per-binary test matrices whose premises were per-variant defaults and multiple binaries. BREAKING CHANGE: Removing `nonet_sandbox` is a breaking change for downstream repositories that referenced it, and for remote execution workers that invoked a variant binary and relied on its defaults. The migration path is to use a wrapper that defines the environment (see tools/sandbox/README.md).

toastwaffle · 2026-08-07T11:45:22Z

@@ -0,0 +1,15 @@
+go_test(


Why put the tests in a subdirectory?

We got this lint error when the C and Go files were colocated:

level=error msg="[linters_context] typechecking error: C source files not allowed when not using cgo or SWIG: main.c nonet_main.c nonetproc_main.c noproc_main.c sandbox.c"

toastwaffle · 2026-08-07T11:48:44Z

+
+static int cloned_pid;

 int perror_sock(char *errmsg, const int sock) {


Suggested change

int perror_sock(char *errmsg, const int sock) {

inline int perror_sock(char *errmsg, const int sock) {

The inline was removed here: 5b8d710 . I assume it was removed because the keyword has different meanings C/C++ and based on whether it's just inline, inline static, or inline extern, and removing it made the code more readable than changing it to inline static.

toastwaffle · 2026-08-07T12:04:52Z

+    labels = [
+        "localonly",
+        "manual",
+    ],


Presumably these tests aren't currently being run by CircleCI? Can we remove these labels? Do we need to make changes to the CircleCI config?

If I removed manual right now, it would be executed in this step: https://app.circleci.com/pipelines/github/thought-machine/please/7814/workflows/d70458df-9432-4518-bcf0-06c5bde6eec3/jobs/63145/parallel-runs/0/steps/0-105 .

I would like to remove it and see whether it passes in CI once we update the CI image references to a version that includes the dependencies added in this PR. It does pass when run locally in the images defined here.

Let's split out the image changes to a separate PR and get that landed first?

Happy to split the image changes if you'd prefer, but while testing them I found that they're no longer a pre-requisite: the sandbox tests can't run with the docker executor that is used by the current jobs (mount syscall is blocked and CircleCI doesn't expose configuration to allow it). To work around this, I updated this PR with new jobs that run the tests using machine executors and added them to the release requirements. See example run from the latest commit. The jobs currently apply the image changes at container startup. Once the images are updated, these steps can be removed (tracked in #3571), but the rest of this setup will stay.

peterebden

This is good by me, although I like Sam's idea of splitting out the image changes so the tests will work.

peterebden · 2026-08-10T14:20:13Z

+In addition to the above, when enabling the mount namespace, the sandbox will remount /proc, so any
+process that use it will have access to accurate information of the PID namespace (otherwise they'd
+still have access to /proc from the parent namespace). There's however a specific edge case in the
+Linux kernel, that prevents /proc from being remounted in a mount namespace, when the parent /proc
+is not fully visible, [see this commit](https://github.com/torvalds/linux/commit/1b852bceb0d1).
+
+This is an issue with most container runtimes (and therefore Kubernetes), as by default, they will
+hide some part of /proc in a container to reduce attack surface, eg
+[see this docker PR](https://github.com/docker/cli/pull/1808).


I'm a little unclear what the conclusion of this section is? I remember the issue (remember bashing my head against it for a while) but is this something the sandbox works around, is it just a limitation of cases where it doesn't work, or do we intend to try to fix it in the future?

The conclusion is that users of the sandbox have two options: Either unmask proc (k8s pod procMount: Unmasked securityContext option, which is less of an issue when combined with hostUsers: false), or disable the /proc isolation, in which case the sandboxed process will be able to see (but not send signals to) processes outside the sandbox. I'll update the README to mention this.

- Run the sandbox tests on machine executors via rootless podman. The docker executor was not sufficient ("remount: Permission denied"), likely due to apparmor settings that are not configurable. //tools/sandbox/test:sandbox_test therefore remains manual. - Make release-gs conditional on the new sandbox tests - Update README - TODO: Remove container setup steps once the new images are tagged: #3571

gzvolsky-tm added 3 commits August 5, 2026 10:46

gzvolsky-tm force-pushed the gzvolsky/sandbox-uid-gid-mappings branch from f6d40cf to 151a097 Compare August 6, 2026 13:27

gzvolsky-tm force-pushed the gzvolsky/sandbox-uid-gid-mappings branch from 88fcb15 to 8807a2a Compare August 6, 2026 18:25

gzvolsky-tm marked this pull request as ready for review August 6, 2026 18:25

gzvolsky-tm requested review from chrisnovakovic, peterebden and toastwaffle August 6, 2026 18:26

toastwaffle requested changes Aug 7, 2026

View reviewed changes

peterebden reviewed Aug 10, 2026

View reviewed changes

gzvolsky-tm mentioned this pull request Aug 11, 2026

chore: remove duplicated setup steps once new images are tagged #3571

Open

gzvolsky-tm requested a review from toastwaffle August 11, 2026 13:34

Navigation Menu

Uh oh!

tools/sandbox: update with new features, variants, and tests - #3566

tools/sandbox: update with new features, variants, and tests#3566
gzvolsky-tm wants to merge 6 commits into
masterfrom
gzvolsky/sandbox-uid-gid-mappings

gzvolsky-tm commented Aug 3, 2026 •

edited

Loading

Uh oh!

peterebden commented Aug 4, 2026

Uh oh!

toastwaffle Aug 7, 2026

Uh oh!

grantzvolsky Aug 7, 2026

Uh oh!

toastwaffle Aug 7, 2026

Uh oh!

grantzvolsky Aug 7, 2026

Uh oh!

Uh oh!

toastwaffle Aug 7, 2026

Uh oh!

grantzvolsky Aug 7, 2026

Uh oh!

toastwaffle Aug 7, 2026

Uh oh!

grantzvolsky Aug 11, 2026

Uh oh!

peterebden left a comment

Uh oh!

peterebden Aug 10, 2026

Uh oh!

grantzvolsky Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		static int cloned_pid;

		int perror_sock(char *errmsg, const int sock) {

	int perror_sock(char *errmsg, const int sock) {
	inline int perror_sock(char *errmsg, const int sock) {

Uh oh!

Conversation

gzvolsky-tm commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behaviour changes

Breaking change

Uh oh!

peterebden commented Aug 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterebden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzvolsky-tm commented Aug 3, 2026 •

edited

Loading