docs(proposal): add build isolation design for sandboxed builds#1077
docs(proposal): add build isolation design for sandboxed builds#1077pavank63 wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new proposal document describing a Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/build-isolation.md`:
- Around line 206-210: The sentence claiming "Works in unprivileged Podman and
Docker containers" is contradictory because Docker's default seccomp may block
unshare; update the text around that sentence to make Docker support conditional
on seccomp/user namespace configuration: explicitly state that Podman works
unprivileged, and for Docker note that it only works if the container runtime
permits user namespaces/unshare (e.g., using a permissive seccomp profile or
enabling userns), and keep the existing note about Ubuntu 24.04 requiring sysctl
kernel.apparmor_restrict_unprivileged_userns=0; reference the existing terms
"unprivileged Podman and Docker containers", "unshare", and "sysctl
kernel.apparmor_restrict_unprivileged_userns=0" when making the clarification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81f43311-90aa-45a7-ac42-a78c1e652930
📒 Files selected for processing (2)
docs/proposals/build-isolation.mddocs/proposals/index.rst
pavank63
force-pushed
the
proposal/build-isolation
branch
from
60e3892 to
5fb0f31
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
The proposal is strong on threat modeling but needs tightening on the integration details. I have added few comments / questions
| #### 1. Ephemeral Unix user | ||
|
|
||
| Before each build invocation, the isolation script creates a | ||
| short-lived system user with `useradd` and removes it with `userdel` |
There was a problem hiding this comment.
useradd / userdel, can modify /etc/passwd and /etc/shadow from what I know. This means fromager (or the isolation script) must run as root inside the container. That's a major assumption that isn't mentioned anywhere. What happens if fromager runs as a non-root user?
|
|
||
| Before each build invocation, the isolation script creates a | ||
| short-lived system user with `useradd` and removes it with `userdel` | ||
| on exit (via `trap EXIT`). The user has: |
There was a problem hiding this comment.
The userdel runs in a trap EXIT handler, but SIGKILL cannot be trapped. If a build gets OOM-killed or force-killed, the ephemeral user leaks. Over a long bootstrap run with hundreds of packages, this could accumulate orphaned fmr_* users in /etc/passwd.
Can we add that as a limitation?
| ## Goals | ||
|
|
||
| - A `--build-isolation/--no-build-isolation` CLI flag (default off) | ||
| that supersedes `--network-isolation` for build steps |
There was a problem hiding this comment.
Clarification question: What happens with these combinations?
-
- --build-isolation --network-isolation — redundant? Does build isolation absorb network isolation for build steps while network isolation still applies to non-build steps?
-
- --build-isolation --no-network-isolation — does the user get network isolation for builds anyway (since build isolation includes it)?
-
- --no-build-isolation --network-isolation — today's behavior?
Looking at the current code, network_isolation is passed to _run_hook_with_extra_environ for build hooks but also to _createenv for venv creation. Does build isolation apply to venv creation too, or
only PEP 517 hooks?
|
|
||
| #### BuildEnvironment (`build_environment.py`) | ||
|
|
||
| - `run()` method accepts `build_isolation` parameter, defaults to |
There was a problem hiding this comment.
This doesn't look right. Looking at the actual code in dependencies.py:547-553, _run_hook_with_extra_environ calls external_commands.run() directly — it doesn't go
through BuildEnvironment.run(). This matters because BuildEnvironment.run() is where env var setup (like CARGO_NET_OFFLINE) happens. The proposal needs to either change the hook runner to go through BuildEnvironment.run(), or duplicate that logic.
Add design proposal for --build-isolation flag that sandboxes PEP 517 build backend subprocesses using ephemeral Unix users and Linux namespaces. Includes security findings from proof-of-concept testing with build-attack-test package. Signed-off-by: Pavan Kalyan Reddy Cherupally <pcherupa@redhat.com> Co-Authored-By: Claude <claude@anthropic.com>
pavank63
force-pushed
the
proposal/build-isolation
branch
from
5fb0f31 to
b632624
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/proposals/build-isolation.md (3)
47-48:⚠️ Potential issue | 🟠 MajorCondition Docker support; current statement is factually over-broad.
Line 47 claims unprivileged Podman/Docker support unconditionally, but Docker commonly blocks
unshareunder default seccomp/userns settings. Make Docker support explicitly conditional.Proposed doc fix
-- Works in unprivileged containers (Podman/Docker) without -- `--privileged` or `--cap-add SYS_ADMIN` +- Works in unprivileged Podman containers without `--privileged` or + `--cap-add SYS_ADMIN`. +- Docker support is conditional on seccomp/user-namespace settings + that permit `unshare`.As per coding guidelines
**/*.md: Only comment on factual errors or broken links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/build-isolation.md` around lines 47 - 48, The statement "Works in unprivileged containers (Podman/Docker) without `--privileged` or `--cap-add SYS_ADMIN`" is too broad; update the sentence to make Docker support conditional by noting that unprivileged operation works on Podman and on Docker only when userns/seccomp configurations permit `unshare` (or when explicit flags are used), e.g. replace the current line with a conditional phrasing that calls out Podman as supported unprivileged and Docker as supported only with appropriate user namespace or seccomp configuration or when privileged/capabilities are granted.
61-62:⚠️ Potential issue | 🟠 Major
trap EXITcleanup is not guaranteed in kill/OOM scenarios.Line 61-62 states cleanup occurs on exit via trap, but
SIGKILL/OOM kills bypass traps and can leakfmr_*users. Document this as a known limitation.Proposed doc fix
- is created with `useradd` and removed on exit via `trap EXIT`. The user + is created with `useradd` and removed on normal exit via `trap EXIT`. + (Limitation: `SIGKILL`/OOM kill can bypass trap cleanup and leave orphaned + `fmr_*` users.) The userAs per coding guidelines
**/*.md: Only comment on factual errors or broken links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/build-isolation.md` around lines 61 - 62, Update the text around the `trap EXIT` description to explicitly document that cleanup via `trap EXIT` is not guaranteed for SIGKILL or OOM situations and therefore `fmr_*` users may be leaked; mention this as a known limitation and suggest possible mitigations (e.g., external watchdog/garbage-collector, systemd unit with KillMode, or periodic cleanup job) so readers are aware the user removal can fail under kill/OOM conditions.
60-63:⚠️ Potential issue | 🟠 MajorDocument required privileges for
useradd/userdel.The flow in Line 60-63 assumes permissions to modify
/etc/passwdand/etc/shadow. Add an explicit prerequisite (root or equivalent container capability), and note behavior when running as non-root.Proposed doc fix
Build isolation combines an ephemeral Unix user with Linux namespace isolation. Before each build, a short-lived system user (`fmr_<random>`) is created with `useradd` and removed on exit via `trap EXIT`. The user has no home directory and no login shell, so it cannot read root-owned credential files like `.netrc` (mode 600). After dropping to the ephemeral user with `setpriv`, the script enters new namespaces with `unshare`: + +Prerequisite: the wrapper must run with privileges sufficient for +`useradd/userdel` (typically root inside the build container/host). +When run as non-root without those privileges, build isolation cannot +create the ephemeral user and should fail fast.As per coding guidelines
**/*.md: Only comment on factual errors or broken links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/build-isolation.md` around lines 60 - 63, The text describing creation/removal of the short-lived system user (useradd/userdel creating fmr_<random> and cleanup via trap EXIT) must state that this requires permission to modify /etc/passwd and /etc/shadow (i.e., running as root or a container with equivalent capabilities to update system accounts); update the doc to add a prerequisite line that says “requires root or equivalent Linux capabilities to run useradd/userdel,” and explicitly document expected behavior when run as non-root (useradd will fail and the build should either error out or use an alternative strategy such as using an existing unprivileged user, a user namespace, or an ephemeral home-dir approach). Ensure you reference the existing symbols useradd, userdel, trap EXIT, fmr_<random>, /etc/passwd and /etc/shadow so readers can locate the relevant section.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/proposals/build-isolation.md`:
- Around line 47-48: The statement "Works in unprivileged containers
(Podman/Docker) without `--privileged` or `--cap-add SYS_ADMIN`" is too broad;
update the sentence to make Docker support conditional by noting that
unprivileged operation works on Podman and on Docker only when userns/seccomp
configurations permit `unshare` (or when explicit flags are used), e.g. replace
the current line with a conditional phrasing that calls out Podman as supported
unprivileged and Docker as supported only with appropriate user namespace or
seccomp configuration or when privileged/capabilities are granted.
- Around line 61-62: Update the text around the `trap EXIT` description to
explicitly document that cleanup via `trap EXIT` is not guaranteed for SIGKILL
or OOM situations and therefore `fmr_*` users may be leaked; mention this as a
known limitation and suggest possible mitigations (e.g., external
watchdog/garbage-collector, systemd unit with KillMode, or periodic cleanup job)
so readers are aware the user removal can fail under kill/OOM conditions.
- Around line 60-63: The text describing creation/removal of the short-lived
system user (useradd/userdel creating fmr_<random> and cleanup via trap EXIT)
must state that this requires permission to modify /etc/passwd and /etc/shadow
(i.e., running as root or a container with equivalent capabilities to update
system accounts); update the doc to add a prerequisite line that says “requires
root or equivalent Linux capabilities to run useradd/userdel,” and explicitly
document expected behavior when run as non-root (useradd will fail and the build
should either error out or use an alternative strategy such as using an existing
unprivileged user, a user namespace, or an ephemeral home-dir approach). Ensure
you reference the existing symbols useradd, userdel, trap EXIT, fmr_<random>,
/etc/passwd and /etc/shadow so readers can locate the relevant section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40e894b7-b62c-4db4-962e-1afaa6d6baba
📒 Files selected for processing (2)
docs/proposals/build-isolation.mddocs/proposals/index.rst
✅ Files skipped from review due to trivial changes (1)
- docs/proposals/index.rst
2 participants
Add design proposal for --build-isolation flag that sandboxes PEP 517 build backend subprocesses using ephemeral Unix users and Linux namespaces. Includes security findings from proof-of-concept testing with build-attack-test package.
See: #1019