feat(sandbox): honor OCI image working directories - #2530
feat(sandbox): honor OCI image working directories#2530matthewgrossman wants to merge 6 commits into
Conversation
2c9c7b7 to
6838984
Compare
|
🌿 Preview your docs: https://nvidia-preview-pr-2530.docs.buildwithfern.com/openshell |
BlockedGator is blocked because PR #2530 currently has merge conflicts ( Next action: @matthewgrossman, please rebase or merge the base branch and resolve the conflicts, then push an updated head SHA so gator can run the independent review. |
6838984 to
493abe0
Compare
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements linked issue #2526, which is already marked state:agent-ready / state:pr-opened, and the current head is open, ready for review, and no longer conflict-blocked.
Head SHA: 493abe06869d41a5b161a1a97abc21c0c3ce6436
Review findings:
- Warning: Docker/Podman pass the resolved workspace to the supervisor, but the create specs still appear to inherit the image process cwd. For hand-crafted OCI images where
Config.WorkingDiris missing on disk or is not a directory, the container runtime may fail before the supervisor can run the intended workspace validation/preparation path. Please consider starting the supervisor from a known-safe cwd such as/while passing--workdir <resolved-root>separately, and add coverage for missing/non-directoryWorkingDir. - See the inline warning about accepting whitespace/control-character OCI
WorkingDirvalues before SSH discovery and shell-text flows consume the path.
Docs: Fern docs and relevant agent skill guidance were updated; no docs navigation change appears required because no new page was added.
Next state: gator:in-review
c02986a to
d14d6c9
Compare
6d53441 to
254364a
Compare
d14d6c9 to
41b25a7
Compare
|
Label |
PR Review StatusValidation: PR #2530 implements linked issue #2526, which is already marked Review findings:
Docs: Fern docs and relevant agent skill guidance were updated; no docs navigation change appears required because no new page was added. E2E: Applied Checks: Required checks are not green yet. Next state: |
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements maintainer-authored, agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: 6bafbb649530f60871f7b4a545a6c56bfde5ead0
Review findings:
- One warning remains about guaranteeing that an existing OCI workspace is writable after ownership preparation; see the inline comment.
- The prior whitespace/control-character OCI
WorkingDirfinding remains addressed and was not re-raised.
Docs: Fern docs and relevant agent skill guidance are updated; no navigation change is needed because no page was added.
E2E: test:e2e remains appropriate for the sandbox lifecycle, policy, Docker/Podman, and SSH workspace changes.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements maintainer-authored, agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: ad58a35413db4717d8d487b782d2f45465d9bd18
Review findings:
- One warning remains about guaranteeing that an existing OCI workspace is writable after ownership preparation; see the inline comment.
- The new head addresses the SSH connection-limit path, but it does not change the workspace ownership/preparation code behind the unresolved warning.
- The prior whitespace/control-character OCI
WorkingDirfinding remains addressed and was not re-raised.
Docs: Fern docs and relevant agent skill guidance are updated; no navigation change is needed because no page was added.
E2E: test:e2e remains appropriate for the sandbox lifecycle, policy, Docker/Podman, and SSH workspace changes.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: ccba5bb168afa4d7b554bb1b42b121871c745fd0
Thanks @matthewgrossman. I checked your latest replies against the current diff: the whitespace/control-character validation and existing 0555 workspace permission findings are resolved and were not re-raised.
Review findings:
- Three distinct warnings remain; see the inline comments for the Docker pre-inspection mount check, inaccessible workspace ancestors, and incomplete protection of system paths selected through OCI
WorkingDir.
Docs: Fern docs and the relevant generate-sandbox-policy / openshell-cli skill guidance are updated; no navigation change is needed because no page was added or moved.
E2E: test:e2e remains appropriate for the sandbox lifecycle, policy, Docker/Podman, and SSH workspace changes.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: babd6ba679067f40eaacbf5bfad4a65283e4acaa
Thanks @matthewgrossman. I checked your current-head replies against the diff: the Docker placeholder-metadata mount collision, inaccessible 0700 ancestor case, and protected-system-tree coverage are addressed and were not re-raised.
Review findings:
- One distinct warning remains for workspace ancestors traversable only through a named user's supplementary groups; see the inline comment.
- One docs wording correction is suggested; see the inline comment.
Docs: Relevant Fern docs and agent skill guidance are updated, and no navigation change is needed because no page was added or moved. The baseline-filesystem wording should be qualified as noted inline.
E2E: test:e2e remains appropriate for the sandbox lifecycle, policy, Docker/Podman, SSH, transfer, and editor-workspace changes.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: 0af4badfd437830521dade6f36439b12da9bb514
Thanks @elezar. I checked the questions you raised about existing upload targets, relative workspace paths, containment-helper reuse, dynamic mount collisions, stale comments, the supervisor workdir contract, and naming. The current commit addresses those points: it documents merge/overwrite behavior, accepts workspace-relative downloads with containment checks, reuses the core path helper, clarifies the dynamic collision and Kubernetes workdir contracts, and updates the naming/comments.
Review findings:
- Warning:
crates/openshell-prover/src/policy.rs:260-273still materializes literal/sandboxwhenfilesystem.include_workdiris true, while this PR changes that setting to mean the driver-resolved workspace.model::encode_filesystem()therefore receives stale filesystem facts for non-default OCI workspaces, even though the current prover queries do not yet consume them. This cross-file finding is summarized here because the relevant lines are outside the current diff. Please represent the workdir symbolically or pass the resolved workspace into the prover, and cover a non-default root such as/workspace/project. - Suggestion: The new documented behavior for uploading into an existing named directory (overwrite matching entries while preserving unrelated entries) lacks an E2E case. Consider adding one conflicting remote file and one unrelated remote file, then verify overwrite and preservation.
- See the inline suggestion about Podman coverage for nested-volume ownership.
- The eight durable earlier gator findings remain addressed and were not re-raised.
Docs: Fern docs and related agent skills cover the workspace-aware transfer, policy, and compute-driver behavior. No navigation change is needed because no page was added or moved.
Checks: Branch Checks, DCO, Helm Lint, and Docker E2E are green. The required E2E aggregate is still pending, with one Podman job currently failed and other jobs still running; CI classification should wait until the run completes.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: d6ea9d52ab9a57f07a6f513a5c7dd95ce91adaa9
Thanks @matthewgrossman. I checked the two new commits after 0af4badfd437830521dade6f36439b12da9bb514 against the prior feedback. The nested-volume ownership E2E now runs for both Docker and Podman, so that suggestion is addressed. The durable earlier findings remain resolved and were not re-raised.
Review findings:
- Warning:
crates/openshell-prover/src/policy.rs:260-273still expandsfilesystem.include_workdirto literal/sandbox, so the prover models stale filesystem facts for a non-default OCI workspace even though the runtime now uses the driver-resolved root. This remains outside the current diff. Please represent the workdir symbolically or pass the resolved workspace into the prover, cover/workspace/project, and preserve explicit/sandboxpaths literally. - See the inline suggestion for the remaining upload merge/overwrite E2E gap.
Docs: Fern docs and relevant policy/CLI skill guidance cover the workspace-aware behavior; no navigation update is needed because no page was added or moved.
E2E: test:e2e remains appropriate. Podman nested-volume coverage is now present, but the upload merge contract still lacks the focused case noted inline.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: 12b0bd6d25b6246cbec861fc95ed36ac13a5082e
Thanks @matthewgrossman. I checked the new current-head commit after d6ea9d52ab9a57f07a6f513a5c7dd95ce91adaa9. Building the OCI fixture with the active container engine is correct and introduces no apparent regression, but it does not address the remaining general findings.
Review findings:
- Warning:
crates/openshell-prover/src/policy.rs:270still expandsfilesystem.include_workdirto literal/sandbox, so the prover models stale filesystem facts for non-default OCI workspaces. This is outside the current diff. Please pass the resolved workspace into the prover or represent it symbolically, preserve explicit/sandboxpaths literally, and cover/workspace/project. - Warning:
.agents/skills/debug-openshell-cluster/SKILL.md:151covers OCIUSERdiagnostics but not the newWorkingDirreadiness failures routed there by the agent-infrastructure maintenance map. This is outside the current diff. Please add Docker/Podman guidance for invalid or protected paths, symlink/non-directory roots, and inaccessible ancestors. - See the inline suggestion for the still-missing upload merge/overwrite E2E case.
- Durable earlier findings remain addressed and were not re-raised.
Docs: Fern docs cover the direct workspace, transfer, and policy UX, and no navigation change is needed because no page was added or moved. The companion debug-openshell-cluster guidance remains incomplete as noted above.
E2E: test:e2e remains appropriate for the sandbox lifecycle, policy, Docker/Podman, SSH, transfer, and editor-workspace changes.
Next state: gator:in-review
PR Review StatusValidation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid. Thanks @matthewgrossman. I checked the new current-head E2E after Review findings:
Docs: Relevant Fern docs and the policy/CLI skill guidance are updated, and no navigation change is needed because no page was added or moved. The companion cluster-debug guidance remains incomplete as noted above. E2E: Next state: |
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: a672d3c7c883341a183f0b97022771b6cf0a904d
Thanks @matthewgrossman. I checked the current-head changes against the previous disposition. The prover now models an unresolved OCI workdir symbolically instead of inventing literal /sandbox, the Docker/Podman cluster-debug guidance covers the previously missing WorkingDir failure modes, and the upload merge/overwrite E2E is present. Those earlier findings are resolved, and the durable resolved findings were not re-raised.
Review findings:
- Two distinct Podman warnings remain, plus one companion-skill filter correction; see the inline comments.
- The capability-free UID-0 Podman probe rejects a valid workdir beneath an owner-only parent even when the final sandbox identity can traverse it.
- Semantic probe failures are reduced to an exit code and removed before an actionable diagnostic can reach operators.
Docs: Fern documentation covers OCI workspace selection, filesystem policy behavior, transfers, custom-image requirements, and driver differences. No navigation update is needed because no page was added or moved. The cluster-debug probe-name filter needs the inline correction.
E2E: test:e2e remains appropriate. Add focused Podman coverage for a valid workdir beneath a 0700 parent owned by the eventual sandbox user.
Next state: gator:in-review
|
Addressing the two warnings from the prior Gator summary:
|
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, and project-valid.
Head SHA: 1d2f22e7c5413339d458b2ea5c353c1a185165f4
Thanks @matthewgrossman. I checked the current-head Podman changes against the previous disposition. The probe now validates as the final sandbox identity, captures bounded diagnostics before cleanup, covers the owner-only parent case, and uses the corrected probe-name filter. Those findings are resolved, and durable earlier findings were not re-raised.
Review findings:
- One warning remains about the scope of the global Podman lifecycle lock; see the inline comment.
Docs: Fern docs, architecture guidance, and related agent skills are sufficient. No navigation update is needed because no page was added or moved.
E2E: test:e2e remains appropriate. Add a focused concurrency regression for the lifecycle-lock scope.
Next state: gator:in-review
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2530 implements agent-ready issue #2526 and remains open, non-draft, mergeable, and project-valid.
Head SHA: ed0619ac55022bc17ac1be741c455112aec43337
Thanks @matthewgrossman. I checked the current-head change against the previous Podman lifecycle-lock finding. Create/start and removal remain serialized, while polling and bounded log retrieval now run without holding the global lock; the added concurrency regression covers that scope, so the prior finding is resolved.
Review findings:
- Critical (CWE-269): Docker can inherit forged workspace-attestation variables from the image because the driver removes those keys instead of explicitly overriding them; see the inline comment.
- Warning: The credential-dropped workspace probe still decides access from Unix mode bits rather than the kernel's ACL/LSM access decision; see the inline comment.
- Durable earlier gator findings remain addressed and were not re-raised.
Docs: Existing Fern pages, architecture guidance, and related agent skills cover the workspace behavior; no navigation change is needed because no page was added or moved.
E2E: test:e2e remains appropriate for this sandbox lifecycle, policy, Docker/Podman, SSH, transfer, and editor-workspace change.
Next state: gator:in-review
Review Convergence CheckpointHead SHA: Thirteen finding-bearing review rounds have completed, so Gator has reached the mandatory maintainer convergence checkpoint. Thanks @matthewgrossman. I checked the latest author-only delta after Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: please choose one of the checkpoint outcomes—accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
Review Convergence CheckpointHead SHA: Thirteen finding-bearing review rounds have completed, so Gator remains at the mandatory maintainer convergence checkpoint. Thanks @matthewgrossman. I checked both author-only commits after checkpoint head Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: please choose one of the checkpoint outcomes—accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
6855d4e to
6b706bf
Compare
Review Convergence CheckpointHead SHA: Thirteen finding-bearing review rounds have completed, so Gator remains at the mandatory maintainer convergence checkpoint. Thanks @matthewgrossman. I checked the force-pushed, squashed author patch against checkpoint head Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: please accept the current Docker-only scope, confirm a split follow-up for Podman, waive the remaining access-validation obligation, or explicitly authorize another autonomous review round. Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Review Convergence CheckpointHead SHA: Thirteen finding-bearing review rounds have completed, so Gator remains at the mandatory maintainer convergence checkpoint. Thanks @matthewgrossman. I checked the merge-from-main commit after checkpoint head Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: please accept the current Docker-only scope, confirm the Podman split, waive the remaining access-validation obligation, or explicitly authorize another autonomous review round. Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Re-check After Maintainer UpdateThanks @matthewgrossman. I verified your repository-admin authority and re-evaluated current head Head SHA: What I checked:
Checkpoint disposition: resolved. The Podman split supplies the requested maintainer scope decision, and the current delta addresses the remaining kernel-effective access obligation. Earlier durable findings retain their recorded dispositions. Pipeline disposition: author action is still required.
Please fix the compile/clippy errors and push a new head. Reviewer-quality signals: 0 proposed findings, 0 unchanged-code proposals, 0 duplicate invariants, and 0 missing-reproducer downgrades. Ledger telemetry remains 14 review rounds, 13 finding-bearing rounds, 17 unique findings, and no duplicate finding IDs. Next state: |
| /// Keep this list tied to concrete `OpenShell` control resources. Workspace | ||
| /// safety itself is enforced from the image's ownership and mode metadata, | ||
| /// not by guessing which distro paths are sensitive. | ||
| const OPENSHELL_CONTROL_PATHS: &[&str] = &[ |
There was a problem hiding this comment.
will this list need to be updated frequently? what I'm worried about is that some new path that the supervisor relies on /etc/something starts to happen, but we don't add it here, and then some sandbox image is allowed to set its WORKDIR /etc/something, and then can write whatever to it, influencing how the supervisor runs
|
|
||
| Image `VOLUME` declarations must not cover the workdir or one of its parents | ||
| because Docker would mount the volume before the supervisor could validate the | ||
| immutable image path. |
There was a problem hiding this comment.
how would this work in normal docker images; can a VOLUME shadow a WORKDIR?
and how do we guarantee that in this PR? doing manual path arithmetic?
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
| resolve_oci_supplementary_gids_at(declaration, primary_gid, Path::new(GROUP_PATH)) | ||
| } | ||
|
|
||
| fn resolve_oci_supplementary_gids_at( |
There was a problem hiding this comment.
why did this have to get added in this PR? I would've expected the previous USER PR to handle this
Re-check After Author and Maintainer UpdateThanks @matthewgrossman. I re-evaluated current head Head SHA: What I checked:
Disposition: the previous compile/clippy blocker is resolved. Remaining items:
Durable earlier findings retain their recorded dispositions. Reviewer telemetry for this round: 0 proposed findings, 0 blockers, 0 unchanged-code proposals, 0 duplicate invariants, and 0 missing-reproducer downgrades. Ledger telemetry is 15 review rounds, 13 finding-bearing rounds, 17 unique findings, 0 duplicate finding-ID occurrences, and 0 findings repeated across review heads. Next state: |
Summary
Docker now honors a usable OCI
Config.WorkingDiras the sandbox workspace, with an OpenShell-managed/sandboxfallback for empty or root declarations. The resolved workspace is used consistently by the Docker driver, supervisor, filesystem policy, SSH transfers, and editor launch. Podman support was extracted into the follow-up PR linked below; Podman, Kubernetes/OpenShift, and VM retain/sandboxin this PR.This work was originally developed as a stack on #2509. That PR has merged, and this PR now targets
maindirectly. The original scope also included Podman support; that work was extracted into #2563, which is stacked on this PR.Related Issue
Part of #2526
Changes
WorkingDirandUserfrom the same immutable Docker image and pass the resolved root through the existing supervisor workdir argument/sandboxcompatibility behavior unchanged for images with an empty or root workdirVOLUMEdeclarations and OpenShell control mounts that would mask or collide with the resolved workdirfilesystem.include_workdirinstead of automatically granting literal/sandbox/sandboxbehavior for Podman, Kubernetes/OpenShift, and VMWhy kernel-effective access checks
Linux access is not determined by ownership and
chmodmode bits alone. A POSIX ACL can grant a particular image user access that mode-bit inspection would reject, while an enforcing Linux Security Module such as SELinux or AppArmor can deny access that mode bits appear to allow.OpenShell does not add ACL or LSM policy management in this PR. Instead, a one-shot validator assumes the final sandbox UID, GID, and supplementary groups and asks the Linux kernel whether that identity can traverse the path and write and enter the workdir. This delegates the decision to the same authority the workload will encounter, avoids maintaining an incomplete permission model in OpenShell, and preserves the central rule that selecting an image workdir must not expand the image user's authority.
Testing
mise run pre-commitpassesChecklist