feat(sandbox): honor OCI image working directories - #2530
feat(sandbox): honor OCI image working directories#2530matthewgrossman wants to merge 9 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>
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: |
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: |
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