feat(sandbox): honor OCI image working directories - #2530
feat(sandbox): honor OCI image working directories#2530matthewgrossman wants to merge 10 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
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: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
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: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Re-check After Author and Maintainer UpdateThanks @matthewgrossman. I re-evaluated current head Head SHA: What I checked:
Disposition: the protected-path drift concern and the Remaining item:
Checks: 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 16 review rounds, 13 finding-bearing rounds, 17 unique findings, 0 duplicate finding-ID occurrences, and 0 findings repeated across review heads. Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Re-check After Author UpdateThanks @matthewgrossman. I re-evaluated current head Head SHA: What I checked:
Disposition: the refactor does not materially invalidate prior finding resolutions and introduces no new Critical blocker. Durable earlier findings retain their recorded dispositions. Remaining item:
Checks: 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 17 review rounds, 13 finding-bearing rounds, 17 unique findings, 0 duplicate finding-ID occurrences, and 0 findings repeated across review heads. Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Re-check After Author UpdateThanks @matthewgrossman. I re-evaluated current head Head SHA: What I checked:
Disposition: no new technical blocker was found, and durable earlier findings retain their recorded dispositions. Reviewer telemetry for this round is 0 proposed findings, 0 blockers, 0 unchanged-code proposals, 0 duplicate invariants, and 0 missing-reproducer downgrades. Ledger telemetry is 18 review rounds, 13 finding-bearing rounds, 17 unique findings, 0 duplicate finding-ID occurrences, and 0 findings repeated across review heads. Checks: Next state: |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
Independent context review:
|
Summary
This PR lets a Docker sandbox use the non-root user and working directory already declared by an ordinary container image instead of requiring
/sandboxto be its one blessed workspace.For example, this small Ubuntu image now works as-is:
OpenShell runs the agent as the image's
ubuntuuser with both its current directory andHOMEset to/workspace. The user can write there because the image already grants that access. OpenShell does not create/sandbox, change ownership or permissions, or grant the user new filesystem authority.Previously, OpenShell ignored this usable OCI
WORKDIRand still treated/sandboxas the workspace. Images therefore needed to be modified around OpenShell's directory convention even when they already declared a valid non-root user and writable workdir.Images with an empty workdir,
WORKDIR /, orWORKDIR /sandboxretain the existing OpenShell-managed/sandboxcompatibility behavior. Podman, Kubernetes/OpenShift, and VM also retain/sandboxin this PR.This work was originally developed as a stack on #2509, which added policy-first OCI image identity and has since merged. The original scope also included Podman support; that work was extracted into #2563, which is stacked on this PR because Podman's persistent workspace volume requires additional validation machinery.
Related Issue
Part of #2526
Changes
WorkingDirandUserfrom the same immutable Docker image and pass the resolved workspace through the existing supervisor workdir argument/sandboxcompatibility behavior unchanged for images with an empty, root, or explicit/sandboxworkdirVOLUMEdeclarations 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-commitpassesWorkingDirbehavior, direct/SSH access, transfers, and nested volumesopenshell sandbox create; verified UID/GID 1000,PWD=/workspace,HOME=/workspace, write access, and fulliproute2Checklist