tools/sandbox: update with new features, variants, and tests - #3566
Open
gzvolsky-tm wants to merge 5 commits into
Open
tools/sandbox: update with new features, variants, and tests#3566gzvolsky-tm wants to merge 5 commits into
gzvolsky-tm wants to merge 5 commits into
Conversation
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.
Collaborator
|
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.
gzvolsky-tm
force-pushed
the
gzvolsky/sandbox-uid-gid-mappings
branch
from
August 6, 2026 13:27
f6d40cf to
151a097
Compare
- 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).
gzvolsky-tm
force-pushed
the
gzvolsky/sandbox-uid-gid-mappings
branch
from
August 6, 2026 18:25
88fcb15 to
8807a2a
Compare
gzvolsky-tm
marked this pull request as ready for review
August 6, 2026 18:25
toastwaffle
requested changes
Aug 7, 2026
| @@ -0,0 +1,15 @@ | |||
| go_test( | |||
Contributor
There was a problem hiding this comment.
Why put the tests in a subdirectory?
|
|
||
| static int cloned_pid; | ||
|
|
||
| int perror_sock(char *errmsg, const int sock) { |
Contributor
There was a problem hiding this comment.
Suggested change
| int perror_sock(char *errmsg, const int sock) { | |
| inline int perror_sock(char *errmsg, const int sock) { |
Comment on lines
+28
to
+29
| When the network namespace is used, the loopback interface is brought up with an additional IP | ||
| address. `SANDBOX_LOCAL_IP` defines, defined empty string disables the extra address. |
Contributor
There was a problem hiding this comment.
Suggested change
| When the network namespace is used, the loopback interface is brought up with an additional IP | |
| address. `SANDBOX_LOCAL_IP` defines, defined empty string disables the extra address. | |
| When the network namespace is used, the loopback interface is brought up with an additional IP | |
| address, which defaults to 10.1.1.1. The `SANDBOX_LOCAL_IP` environment variable can be used | |
| to change this IP, and setting it to the empty string disables the extra address entirely. |
Comment on lines
+7
to
+10
| labels = [ | ||
| "localonly", | ||
| "manual", | ||
| ], |
Contributor
There was a problem hiding this comment.
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?
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
3 participants
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.
This PR brings several enhancements, reliability improvements, and tests to the sandbox:
SANDBOX_UID_MAPandSANDBOX_GID_MAPenvironment variables. This allows mapping multiple UID/GID ranges into the user namespace utilizingnewuidmapandnewgidmap. Both must be set together.MOUNT_PROC=0to skip remounting/proc. This provides a workaround for environments (like newer systemd or certain container runtimes) that prevent remounting a partially masked/proc.SANDBOX_FILE_MOUNTSto allow bind-mounting a comma-separated list of specific files into the sandbox.10.1.1.1, added in Add local IP and default gateway to network for sandbox tool #3310) can now be overridden viaSANDBOX_LOCAL_IP, or disabled by setting it to the empty string./dev/shmso nothing can be inadvertently shared through it.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.SIGTERMforwarding from the parent to the sandboxed child process.sandbox_test.gotest suite to test the network/mount isolation,/procremounting, ID mapping, and process lifecycle across all combinations of the environment knobs.manual/localonlyuntil the images are updated.Behaviour changes
/procis now mounted read-only. This may affect tests that write to it, e.g. to/proc/self/oom_score_adj.TMP_DIRbind mount is now recursive (MS_REC), so submounts ofTMP_DIRare visible inside the sandbox.TMP_DIRis/tmpor 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.nosuid/nodev/noexec/noatimemount flags instead of silently dropping them.Breaking change
nonet_sandboxis 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