Repository navigation
feat(sandbox): opt-in job-control signals to the caller's own process group - #5161
vineethsai7 wants to merge 2 commits into
Conversation
Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Send signals to other checked group members before the notifying process so a terminating signal cannot abort remaining delivery. Assert the broadcast barrier in isolation and describe the full opt-in scope. Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c89b986d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| #[serde(default, skip_serializing_if = "String::is_empty")] | ||
| pub run_as_group: String, | ||
| #[serde(default, skip_serializing_if = "std::ops::Not::not")] | ||
| pub allow_process_group_signals: bool, |
There was a problem hiding this comment.
With a boundary where this flag is false or omitted and a candidate where it is true, the standalone containment prover still returns Within: ContainmentPolicy now carries this schema type, but crates/openshell-prover/src/containment/execution.rs::check compares only run_as_user and run_as_group, and common validation does not reject the new control. This lets a subagent policy gain process-group signal authority, including SIGKILL, while the boundary check reports success; model false-to-true as exceeding the boundary or fail closed as unsupported.
AGENTS.md reference: AGENTS.md:L57-L58
Useful? React with 👍 / 👎.
| process: Some(ProcessPolicy { | ||
| run_as_user: "sandbox".into(), | ||
| run_as_group: "sandbox".into(), | ||
| allow_process_group_signals: false, |
There was a problem hiding this comment.
When an MXC policy sets allow_process_group_signals: true, add_static_policy_loss still inspects only the two identity fields, so EmbeddedPolicyMapper accepts the sandbox without enforcing this control or reporting it as an unsupported loss. This drift fixture sets the boolean to false, which the serializer omits, so the guard cannot expose the missing mapping; exercise the enabled value and reject, delegate, or emit a loss instead of silently dropping the policy request.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
Summary
Add an opt-in process policy for job-control signals in Linux sandbox workloads. Group signals reach the existing seccomp notification broker, which verifies the target group and signals its workload members through retained pidfds. This is a draft design proposal while issue #5148 is still
state:triage-needed; maintainer feedback on the boundary and policy shape is welcome.Related Issue
Part of #5148. This change covers the shared Linux sandbox signal path; driver-specific interactive end-to-end validation remains open.
Changes
process.allow_process_group_signalsto authored policies, the public protobuf/API, the core and OPA policy conversions, and the sandbox boundary protocol. The flag defaults tofalseand is fixed at sandbox creation.kill(0, sig),kill(-pgid, sig), andrt_sigqueueinfogroup targets reach the existing signal notification path. Continue to denykill(-1, sig)in the child filter.SIGCONTguard.Design and threat model
A workload may share a UID and session with sandbox workers, and it may use
setpgidto join a group containing a trusted worker. An unchecked group signal could stop or kill that worker. The broker rejects a target group when it observes a process outside the caller's workload launch or session. It retains each accepted process with a pidfd before signaling, so a later process-group change cannot redirect a group-wide kernel signal to a trusted process. Broadcastkill(-1, ...), PGID 1, and groups from another launch or session remain denied.The current blanket deny breaks shell job control. Allowing the kernel group signal outright would reopen the trusted-group case. A new
setpgid/setsidinterception layer would add machinery to a process-signal path that already has a notification broker; checking at delivery keeps ordinary shell group creation available and fails closed for mixed groups. Network middleware, interceptors, and providers do not mediate process signals.Group membership is read from
/procfor each request. This costs a scan of the visible process table and delivers to a checked snapshot; a member that joins afterward may miss that signal. All collected targets are checked before any signal is sent, and each delivery rechecks the live notification and frozen state.Example
Create a policy file with the opt-in field:
The console sequence is the intended user flow; the automated test covers group
SIGTSTPandSIGCONT, while the interactive driver checks below remain to be run. No new OCSF event or event schema is introduced.Compatibility
Omitting the field preserves the existing group-signal denial. Old policy YAML and boundary JSON deserialize with the flag off; protobuf readers ignore the new fields. Existing single-process signal behavior is unchanged. The server rejects attempts to change the process policy on a running sandbox. The implementation is in the shared Linux guest sandbox path used by the drivers.
If a workload descendant is reparented after its launch process exits, it no longer has a verifiable launch root and group delivery fails closed. Ordinary interactive shell jobs retain a live shell parent.
Release Notes
Sandbox policies can opt in to shell job-control signals with
process.allow_process_group_signals: true. The default remains off.Testing
Verification performed:
cargo fmt --all -- --check: passed.-D warningsfor all touched Rust crates and the sandboxperf-harnessfeature: passed.SIGTSTP/SIGCONT, forbidden group targets, policy round trips, and legacy stored policy decoding.gofmtcheck, TypeScript protobuf generation and typecheck, Python protobuf generation, andbuf lint: passed.No driver E2E was run; those test hosts were outside this pod's scope.
Follow-ups
Ctrl-Z/fgflow and the group-signal security probes through Docker, Podman, Kubernetes, and MicroVM drivers on supported test hosts.Checklist
Review follow-up
Local review: REVIEW-5161.md.
8c89b986d09a2e885ee8720036c4678c9e46c266. Deliver checked group members before the notifying caller so a terminating signal cannot abort delivery to later members. The ordering regression failed before the fix and passed after it; the groupSIGKILLintegration test passed.8c89b986d09a2e885ee8720036c4678c9e46c266. Assert the-1broadcast and PGID 1 exclusion in a child-filter-only test forkillandrt_sigqueueinfo, and assert queued trusted-group denial. The workload test also denies groups outside the caller's launch or session and the trusted sandbox group.8c89b986d09a2e885ee8720036c4678c9e46c266. State that the opt-in permits all valid group signals, including termination signals, in docs, public API comments, generated Go protobuf comments, Go SDK comments, and the policy-generation skill.Checks through
dcb:cargo fmt --all -- --check; touched-crate clippy with--all-targets -- -D warnings; sandboxperf-harnessclippy; touched-crate library tests (5,794 passed, 40 ignored, 0 failed); Go converter tests andgofmt -d; docs navigation, Markdown lint, and Fern check (0 errors, 3 warnings). Driver E2E remains the existing follow-up above.Manual verification
Result: PASS. A real Docker sandbox denied group signals on upstream main and on the PR head with the new field omitted. With
process.allow_process_group_signals: true, the caller's group stopped and resumed, a caught groupSIGTSTParrived, and broadcastkill(-1, 0)remained denied.Environment
1afd9f69d118eefa6972c000830899f9c68d053a. After: PR head8c89b986d09a2e885ee8720036c4678c9e46c266. Each commit's CLI, gateway, supervisor, and sandbox binaries were built on Ubuntu 24.04 throughdcb up-os-5148usingnice -n 15 cargo build --release --locked -p openshell-cli -p openshell-gateway -p openshell-supervisor -p openshell-sandbox --features openshell-gateway/vendored-z3.Steps
Start the commit-matched private gateway. Create a sandbox with
openshell -g <gateway> sandbox create --name <sandbox> --detach --no-tty -- sleep 600. Run this once on main and once on the PR head with the policy field omitted. Both reachedReady.On the PR head, create a third sandbox with
--policy <policy.yaml>. The authored policy used the default policy'sfilesystem_policyread-only paths (/bin,/usr,/lib,/proc,/dev/urandom,/etc,/var/log), read-write paths (/tmp,/dev/null),include_workdir: true, andlandlock.compatibility: best_effort, plus:The PR body's process-only policy example could not launch
sleephere: it replaced the default filesystem grants and failed withPermission denied (os error 13). Adding the default grants allowed the opt-in sandbox to reachReady;sandbox get -o jsonreportedpolicy.process.allow_process_group_signals: true.In each sandbox, run the same non-TTY probe with
openshell sandbox exec --no-tty --no-login-shell -- setsid -w bash -c '<probe>':setsidisolates the workload session. Bash job control puts the helper in another process group in that session. The caller useskill(0, SIGSTOP)on its own group; the helper useskill(-pgid, SIGCONT)to resume it.In the opt-in sandbox, also run
setsid -w bash -c 'trap "echo TSTP_RECEIVED" TSTP; kill -TSTP 0; echo "TSTP_RC:$?"'andbash -c 'kill -0 -- -1; echo "BROADCAST_RC:$?"'.Before: upstream main
The sandbox reached
Ready. Both group calls returnedEPERM:AFTER_STOP_RC:1appeared before the helper's one-second wait finished, so the caller did not stop;HELPER_CONT_RC:1followed. The CLI exec itself exited 0 because the probe recorded the individual signal results and completed.After: PR head
With the field omitted, the PR sandbox also reached
Readyand produced the same twoEPERMresults. With the flag enabled, the helper'sSIGCONTline appeared beforeAFTER_STOP_RC:0, proving that the caller stopped and then resumed. The helper's groupSIGCONTreturned 0. The caught groupSIGTSTPreturned 0 and printed its trap marker.kill(-1, 0)returnedEPERM.Short excerpts
Cleanup readback found no proof account or HOME, private listener, proof process, container, image, or volume. The existing host OpenShell listener retained its original PID. The dev worktree is clean at the PR head; no commit or push was made.
Not covered
The interactive TTY
Ctrl-Z/fgsequence and Podman, Kubernetes, and MicroVM drivers were not run. This run did not probert_sigqueueinfo, mixed trusted groups, other sessions or launches, the frozen-workloadSIGCONTguard, or policy-update rejection. The main comparison used the PR branch point; later upstream main commits were not separately tested.