fix(sandbox): fall back to --map-auto when root-user mapping is restricted - #3280
Conversation
No behavior change. Move the inline probe out of unshare_user_namespace_works into a reusable unshare_probe helper and a cached working_unshare_mapping() that picks the first working candidate from UNSHARE_MAPPING_CANDIDATES, so the launcher and the capability probe share one code path.
…icted Plain `unshare --user --map-root-user` fails on kernels and containers that block unprivileged writes to /proc/self/uid_map (e.g. GitHub Actions, restricted AppArmor profiles). On those systems util-linux delegates to the setuid newuidmap/newgidmap helpers when --map-auto is also present. Add the combined form as a fallback candidate and build the launcher args from the probed mapping, so systems without newuidmap/newgidmap or a /etc/subuid range keep using the plain form.
d17590b to
277fdda
Compare
|
Solid fix. The fallback from --map-root-user to --map-auto is the correct approach for environments where /proc/self/uid_map is restricted (GitHub Actions, AppArmor, some Docker setups). Probing both at startup and caching the result avoids runtime surprises. One thing to verify: does the --map-auto path require |
… fallback The fallback candidate relies on the setuid newuidmap/newgidmap helpers (uidmap package) plus a subuid/subgid range for the current user. Note in the candidate docs that the startup probe rejects the candidate when those are missing, so the plain --map-root-user form is used instead.
|
Good question — yes, the Dependency: when Why this is safe: the startup probe ("Probing both at startup") runs the exact candidate command, so it covers this automatically. Verified empirically: So on systems without Documented this dependency in the candidate docs — see the new commit (9cbe6d9) in this PR. |
|
Thanks for the detailed clarification. The startup probe covering the dependency automatically is exactly what I was hoping for — no need for extra config or docs then. LGTM on the approach. |
|
Hi @code-yeongyu @Yeachan-Heo — quick heads-up on this PR (replacement for the closed #3013). Following @code-yeongyu's earlier request, the branch has been rebased onto the latest One thing needs a maintainer's help: since this is a fork PR, the |
|
The rebase looks clean and the --map-auto fallback approach is the right direction. Since this supersedes #3013, the commit history is well-structured. I think this is good to go once CI passes. |
|
@EmreCelenli — one small ask: this fork PR's CI is waiting for a maintainer to approve the workflows ( |
|
Hi @Einspanner123, I don't have permission to approve the workflow run, there's no CI approval option on my end. You'll probably need @code-yeongyu or @Yeachan-Heo for that instead. |
|
Thanks @EmreCelenli for checking — appreciate the help! |
code-yeongyu
left a comment
There was a problem hiding this comment.
Thanks for the PR — the OnceLock-cached probe, candidate ordering, and the order-guarding unit test are all well done, and the intent (recover environments that block direct /proc/self/uid_map writes) is sound. However, I verified this on CI and it regresses the workspace test suite on ubuntu-latest: the probe's success criterion is too weak. Requesting changes.
Evidence
- PR CI fails (both workflows, runs 30689673366 / 30689673396):
mock_parity_harness::clean_env_cli_reaches_mock_anthropic_service_across_scripted_parity_scenariosfails atassert_bash_stdout_roundtrip— bash tool returned empty stdout, expectedalpha from bash. - Control: main passes today. I dispatched
rust-ci.ymlonmain(run 31091226038, sameubuntu-latestimage):cargo test --workspaceis green. So this change is the cause, not runner drift. - Runner ground truth (probe run on
ubuntu-latest, commands executed verbatim on the runner):unshare --user --map-root-user true→ rc=1:unshare: write failed /proc/self/uid_map: Operation not permittedunshare --user --map-root-user --map-auto true→ rc=0 — the fallback probe passes, so the sandbox becomes activeunshare --user --map-root-user --map-auto --mount --ipc --pid --uts --fork sh -lc "echo alpha"(the real launcher shape) → rc=1:unshare: cannot change root filesystem propagation: Permission denied
Root cause
unshare_probe validates only the mapping flags with the trivial program true. The actual launcher (build_linux_sandbox_command) always appends --mount --ipc --pid --uts --fork — and on GitHub runners AppArmor blocks mount propagation inside the user namespace. The probe therefore activates a sandbox whose launcher can never start; every sandboxed tool call dies with empty output.
Required change
Probe the full launcher shape, not just the mapping: each candidate must carry the complete flag set the launcher uses, e.g.
const UNSHARE_MAPPING_CANDIDATES: &[&[&str]] = &[
&["--user", "--map-root-user", "--mount", "--ipc", "--pid", "--uts", "--fork"],
&["--user", "--map-root-user", "--map-auto", "--mount", "--ipc", "--pid", "--uts", "--fork"],
];and update mapping_candidates_prefer_plain_root_mapping to also assert the namespace flags are present per candidate. I prototyped exactly this in a scratch worktree (not pushed): cargo fmt/clippy -p runtime --all-targets clean, all 8 sandbox tests + full runtime suite pass, and on the GH runner both candidates are then rejected — the sandbox correctly stays disabled there (mount propagation in a userns is blocked on GH runners, so no unshare-based sandbox can work), while systems where the full launch works keep the fallback. Happy to share the prototype diff.
Also
- The doc comment on
UNSHARE_MAPPING_CANDIDATEScites "e.g. GitHub Actions" as an environment the fallback helps — empirically it does not: onubuntu-latestthe fallback passes the mapping-only probe but the full launch fails. Please adjust the example so operators aren't misled (e.g. hardened containers/seccomp setups that block direct uid_map writes but permit the full launch). - Nit (non-blocking):
unshare_probesuppresses stderr; atracing::debug!with the last candidate's stderr would help diagnose restricted environments. - Nit (non-blocking): the
.unwrap_or(UNSHARE_MAPPING_CANDIDATES[0])inbuild_linux_sandbox_commandis unreachable through the status-gated path (namespace_activerequires a successful probe); fine as a defensive default.
Local QA (macOS worktree at PR head): cargo fmt --check clean, cargo test -p runtime sandbox 8/8 green, claw sandbox/status/doctor --output-format json all exit 0 with the expected graceful fallback_reason (unshare absent on macOS).
| ]; | ||
|
|
||
| /// Probe a candidate `unshare` mapping invocation with a trivial program. | ||
| fn unshare_probe(args: &[&str]) -> bool { |
There was a problem hiding this comment.
This probe is the regression. It validates only the mapping flags against the trivial program true, but the real launcher always adds --mount --ipc --pid --uts --fork — and on GitHub ubuntu-latest, AppArmor blocks mount propagation inside the user namespace. Empirically on the runner: unshare --user --map-root-user --map-auto true exits 0 (probe passes → sandbox activates), but unshare --user --map-root-user --map-auto --mount --ipc --pid --uts --fork sh -lc "echo alpha" exits 1 with cannot change root filesystem propagation: Permission denied. Result: CI's mock_parity_harness bash roundtrip returns empty stdout and fails (main control run passes). Please probe the full launcher argument shape — candidates should carry --mount --ipc --pid --uts --fork alongside the mapping flags — so probe success implies launch success.
| /// package on Debian/Ubuntu) and on the current user having a range in | ||
| /// `/etc/subuid` and `/etc/subgid`. When either is missing, `--map-auto` | ||
| /// fails and the startup probe rejects the candidate, keeping the plain form. | ||
| const UNSHARE_MAPPING_CANDIDATES: &[&[&str]] = &[ |
There was a problem hiding this comment.
The "e.g. GitHub Actions" example is inaccurate: on ubuntu-latest the fallback passes this mapping-only probe but the full launcher (with --mount --ipc --pid --uts --fork) still fails (cannot change root filesystem propagation: Permission denied), so the sandbox stays broken/disabled there either way. Suggest citing an environment that blocks direct uid_map writes but permits the full launch (e.g. certain hardened containers/seccomp profiles), so the docs match reality.
| } | ||
|
|
||
| #[test] | ||
| fn mapping_candidates_prefer_plain_root_mapping() { |
There was a problem hiding this comment.
Once the candidates carry the full launcher flag set, please extend this test to assert each candidate contains the namespace flags (--mount, --ipc, --pid, --uts, --fork) in addition to the mapping flags and ordering — so a future edit can't silently shrink the probe shape back to mapping-only. (I prototyped this: pure string assertions, runs fine on macOS.)
| /// | ||
| /// Probes are cached for the process lifetime; a missing `unshare` binary or a | ||
| /// kernel that refuses every mapping yields `None`. | ||
| fn working_unshare_mapping() -> Option<&'static [&'static str]> { |
There was a problem hiding this comment.
The OnceLock<Option<&'static [&'static str]>> caching is exactly right — including caching the "nothing works" case so a restricted host isn't re-probed on every call. Nice.
|
Quick note: I have the full-shape probe fix implemented and verified locally (fmt/clippy clean, 8/8 sandbox tests + full runtime suite) and it is staged on your branch (commit 3e28c2d, not yet pushed). I'd prefer you implement it your way if you'd like — but if I don't hear back by ~12:10 UTC I will push it to the PR branch so CI can validate it, and then re-review. The prototype diff is also available on request. |
The startup probe validated only the mapping flags against the trivial program `true`, but the real launcher always adds --mount --ipc --pid --uts --fork. On environments where the user namespace is created but mount propagation inside it is restricted (e.g. AppArmor-restricted CI runners), the fallback mapping passed the probe and the sandbox activated, yet every sandboxed command died with "cannot change root filesystem propagation: Permission denied", silently returning empty tool output and breaking the mock parity suite. The candidates now define the complete static launcher shape (mapping flags + namespace flags), so probe success implies launch success; the launcher reuses the candidate instead of re-appending the namespace flags, keeping probe and launch as one source of truth. The order-guarding test asserts the namespace flags are present in every candidate. Co-authored-by: linkst <2024023709@m.scnu.edu.cn>
code-yeongyu
left a comment
There was a problem hiding this comment.
The full-launcher-shape probe fix resolved the regression I flagged in review. Verified end to end:
- The probe now runs each mapping candidate with the complete static launcher shape (
--mount --ipc --pid --uts --fork), so probe success implies launch success; the launcher reuses the candidate as its single source of truth instead of re-appending the namespace flags. - On ubuntu-latest the probe correctly rejects both candidates (uid_map writes and mount propagation in userns are both blocked by AppArmor), so the sandbox stays disabled there and the mock parity suite is green — the exact regression from the mapping-only probe is gone.
- Local verification on the fix commit: fmt clean, sandbox.rs clippy-clean, 8/8 sandbox tests, full workspace test suite green (37/37 binaries, including mock_parity_harness).
- CI: both workflows green on this head https://github.com/ultraworkers/claw-code/actions/runs/31096387114 and https://github.com/ultraworkers/claw-code/actions/runs/31096387075.
The docs update (honest example instead of the misleading GitHub Actions claim, --net trade-off documented) is accurate. Nice work on the original probe/caching design and on taking the review feedback; merging.
|
The OnceLock-cached probe design and full-launcher-shape approach are well-considered. Since the probe now carries --mount --ipc --pid --uts --fork, consider adding a bwrap smoke check if available — some restricted environments allow unshare but silently reject bind mounts inside the namespace, which would still trip the sandbox at launch time. |
Replaces the closed #3013 (stale, conflicts with main) with a fresh rebase onto
ultraworkers:main.Problem
unshare --user --map-root-userfails on kernels and containers that block unprivileged writes to/proc/self/uid_map(e.g. GitHub Actions, restricted AppArmor profiles, some container runtimes) with EPERM. As a result the sandbox silently disables itself even though a working mapping exists.Fix
util-linux delegates to the setuid
newuidmap/newgidmaphelpers when--map-autois also present. Probe both candidate mappings at startup and prefer the plain form:--user --map-root-user(works on most systems, no extra deps)--user --map-root-user --map-auto(fallback for restricted kernels/containers)The chosen mapping is cached and reused by both the capability probe and the launcher, so the sandbox now enables on systems where only the fallback works. The plain form stays first, so systems without
newuidmap/newgidmapor a/etc/subuidrange are unaffected.Note: the
--map-autofallback depends on the setuidnewuidmap/newgidmaphelpers (theuidmappackage on Debian/Ubuntu) and on the current user having a subuid/subgid range. The startup probe covers this dependency: when the helpers or range are missing,unshare --map-autofails and the plain form is used (verified:unshare --user --map-root-user --map-auto trueexits 127 withfailed to execute newuidmapwithout the helper).Verification
unshare --user --map-root-user truefails, combined form succeedscargo test -p runtime sandboxpasses (incl. new test guarding candidate order)cargo clippyclean for the changed file