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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 57 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces documentation for a build isolation feature proposal. The proposal specifies a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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
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
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.
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