windows-sandbox: add runner IPC foundation for future unified_exec#14139
windows-sandbox: add runner IPC foundation for future unified_exec#14139iceweasel-oai wants to merge 7 commits intomainfrom
Conversation
iceweasel-oai
force-pushed
the
windows-runner-ipc-foundation
branch
from
c4c614d to
77e965f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
Couple comments, mostly for my understanding. Overall this approach feels reasonable!
| } | ||
|
|
||
| /// Pick an effective CWD, using a junction if the ACL helper is active. | ||
| fn effective_cwd(req_cwd: &Path, log_dir: Option<&Path>) -> PathBuf { |
There was a problem hiding this comment.
am i misremembering or do we already have logic for this somewhere?
There was a problem hiding this comment.
you are remembering correctly. Part of this PR is moving code around so it's clear what sandbox code is for elevated vs legacy vs shared. command_runner_win.rs got moved into /elevated/ but also changed enough such that github didn't recognize it as a move. So there is some stuff here that was pre-existing, like this
There was a problem hiding this comment.
ack, ty! i skimmed command_runner_win but missed it
| //! It is **elevated-path only**: the parent uses it to bootstrap the runner and | ||
| //! stream unified_exec I/O over named pipes. The legacy restricted‑token path does | ||
| //! not use this protocol, and non‑unified exec capture uses it only when running | ||
| //! through the elevated runner. |
There was a problem hiding this comment.
Parsing this out - does this mean unified_exec can only run inside the elevated sandbox? Do we / will we validate this elsewhere?
There was a problem hiding this comment.
no, it just means that the framed IPC approach is only used in the elevated path because we have the middleman of the command runner.exe. I can make this comment a bit clearer
|
|
||
| /// Creates a named pipe with permissive ACLs so the sandbox user can connect. | ||
| pub fn create_named_pipe(name: &str, access: u32) -> io::Result<HANDLE> { | ||
| // Allow sandbox users to connect by granting Everyone full access on the pipe. |
There was a problem hiding this comment.
I'm not sure about the security implications of this - wouldn't this imply other processes could use this pipe?
There was a problem hiding this comment.
yeah I think it does mean that. I think I can tighten this up to only allow the real user and the sandbox user to access it.
iceweasel-oai
force-pushed
the
windows-runner-ipc-foundation
branch
from
b394619 to
c743a30
Compare
3 participants
Summary
This PR introduces the Windows sandbox runner IPC foundation that later unified_exec work will build on.
The key point is that this is intentionally infrastructure-only. The new IPC transport, runner plumbing, and ConPTY helpers are added here, but the active elevated Windows sandbox path still uses the existing request-file bootstrap. In other words, this change prepares the transport and module layout we need for unified_exec without switching production behavior over yet.
Part of this PR is also a source-layout cleanup: some Windows sandbox files are moved into more explicit
elevated/,conpty/, and shared locations so it is clearer which code is for the elevated sandbox flow, which code is legacy/direct-spawn behavior, and which helpers are shared between them. That reorganization is intentional in this first PR so later behavioral changes do not also have to carry a large amount of file-move churn.Why This Is Needed For unified_exec
Windows elevated sandboxed unified_exec needs a long-lived, bidirectional control channel between the CLI and a helper process running under the sandbox user. That channel has to support:
The existing elevated one-shot path is built around a request-file bootstrap and does not provide those primitives cleanly. Before we can turn on Windows sandbox unified_exec, we need the underlying runner protocol and transport layer that can carry those lifecycle events and streams.
Why Windows Needs More Machinery Than Linux Or macOS
Linux and macOS can generally build unified_exec on top of the existing sandbox/process model: the parent can spawn the child directly, retain normal ownership of stdio or PTY handles, and manage the lifetime of the sandboxed process without introducing a second control process.
Windows elevated sandboxing is different. To run inside the sandbox boundary, we cross into a different user/security context and then need to manage a long-lived process from outside that boundary. That means we need an explicit helper process plus an IPC transport to carry spawn, stdin, output, and exit events back and forth. The extra code here is mostly that missing Windows sandbox infrastructure, not a conceptual difference in unified_exec itself.
What This PR Adds
What This PR Does Not Yet Do
So while this code compiles and the new path has basic validation, it is not yet the exercised production path. That is intentional for this first PR: the goal here is to land the transport and runner foundation cleanly before later PRs start routing real command execution through it.
Follow-Ups
Planned follow-up PRs will:
Validation
cargo build -p codex-windows-sandbox