prepare_filesystem() should not chown existing directories in read_write

[prekshivyas]

Problem

prepare_filesystem() in crates/openshell-sandbox/src/lib.rs chowns every path in the policy's read_write list to run_as_user:run_as_group before spawning the child process. This happens unconditionally — even for directories that already exist in the container image with intentional ownership.

This means an image author who sets root:root on a read_write directory (e.g., to protect immutable subdirectories via DAC while allowing writes to others) has that ownership silently overridden at sandbox start.

Example (NemoClaw)

Our Dockerfile sets /sandbox/.nemoclaw to root:root 755 to prevent the agent from renaming the root-owned blueprints/ subdirectory. The policy lists it as read_write because sandbox-owned subdirs (state/, migration/) inside it need to be writable.

At sandbox start, prepare_filesystem() chowns /sandbox/.nemoclaw to sandbox:sandbox, removing the DAC protection. QA confirmed the ownership flip at runtime in NVIDIA/NemoClaw#1607 / nvbug 6059437.

We've worked around this with the sticky bit (chmod 1755) in NVIDIA/NemoClaw#1121, which prevents the sandbox user from renaming or deleting root-owned entries even after the ownership flip. But the root cause is the unconditional chown.

Context

This came up during security hardening review for NemoClaw PR #1121 (read-only sandbox filesystem). The NemoClaw team discussed this with our tech lead and agreed the sticky bit is a sufficient short-term workaround, but that prepare_filesystem() respecting existing ownership would be the cleaner long-term fix — both for us and for any other OpenShell image author who intentionally hardens read_write paths.

Proposed Change

Only chown directories that prepare_filesystem() creates. If a directory already exists in the image, respect the image author's ownership:

for path in &policy.filesystem.read_write {
    if let Ok(meta) = std::fs::symlink_metadata(path) {
        if meta.file_type().is_symlink() {
            return Err(...);  // existing symlink check
        }
        // Directory exists — image author set ownership intentionally, leave it
    } else {
        // Directory doesn't exist — create and chown
        std::fs::create_dir_all(path).into_diagnostic()?;
        chown(path, uid, gid).into_diagnostic()?;
    }
}

This is backwards-compatible — directories that don't exist in the image still get created and chowned. Only pre-existing directories are left alone.

[prekshivyas]

Reference implementation available at prekshivyas/OpenShell:fix/prepare-filesystem-respect-existing-ownership — single commit, moves the chown() call into the else branch of the existing symlink_metadata check. Happy to open a PR if given collaborator access, or the OpenShell team can pick it up directly.

[mjamiv]

+1 on this. We run 4 OpenClaw agents in separate OpenShell sandboxes with read_write paths for /sandbox/.openclaw/ and subpaths. We've set chmod 444 on specific files (e.g., persona files) as a soft integrity guard, but the unconditional chown in prepare_filesystem() could interact unexpectedly if we tighten ownership in the future.

The proposed fix (only chowning newly-created directories) seems clean and low-risk. Would be happy to test on our fleet if a PR lands.

[johntmyers]

🏗️ build-plan

Implementation Plan

Issue type: fix
Complexity: Low
Confidence: High — clear path

Summary

prepare_filesystem() currently applies chown() to every filesystem.read_write path, even when the path already exists in the image with intentional ownership. The fix is to preserve the existing symlink guard, but only call create_dir_all() and chown() for paths the supervisor creates at sandbox startup.

Scope

• crates/openshell-sandbox/src/lib.rs: move the ownership change into the directory-creation branch so pre-existing paths keep image-defined ownership while newly-created paths still become writable for the sandbox user.
• crates/openshell-sandbox/src/lib.rs tests: add unit coverage for the create-vs-preserve behavior and keep the existing symlink safety behavior covered.

Implementation Steps

1. Update prepare_filesystem() so an existing non-symlink read_write path is left unchanged, while a missing path is created and then chowned.
2. Add targeted unit tests around prepare_filesystem() using temporary directories to verify newly-created paths are writable by the configured identity path, and pre-existing paths are not re-owned.
3. Run the relevant sandbox crate tests and document any platform-specific gaps if ownership assertions cannot run in an unprivileged environment.

Test Plan

• Unit tests: extend crates/openshell-sandbox/src/lib.rs tests to cover existing path preservation, new path creation, and symlink rejection.
• Integration tests: N/A — the behavior is isolated to a local filesystem preparation helper.
• E2E tests: N/A — no cluster or end-to-end behavior change expected.

Risks & Open Questions

• Ownership assertions may need to use the current test user/group rather than assuming a sandbox account exists in every dev environment.
• Existing non-directory special files in read_write should continue to behave as they do today; the fix should not broaden the function’s responsibilities beyond avoiding unnecessary chown() calls.

Documentation Impact

None expected.

---

Revision 1 — initial plan

[johntmyers]

🏗️ build-from-issue-agent

Implementation Complete

PR: #827

What was built

prepare_filesystem() now preserves ownership on pre-existing read_write paths and only chowns paths the supervisor creates at sandbox startup. The change keeps the symlink safety check in place, adds focused regression tests, and updates the architecture docs that described the old behavior.

Tests

• Unit: 4 tests added
• Integration: N/A
• E2E: N/A

Docs updated

• architecture/sandbox.md
• architecture/security-policy.md

The issue will auto-close when the PR is merged.