fix(mxc): reject unsupported live policy updates - #3480
shailendra-nv merged 10 commits into
Conversation
Verification record (Windows ARM64)Base: Before (unmodified tip): After: the operator update returns The MXC capability test, format, diff, and focused server/CLI/MXC clippy checks also passed (pre-existing warnings only). Unix-only driver packages cannot be checked on this native Windows target because the upstream Commit |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
02f2c49 to
cbe9e23
Compare
CI base-state noteThe I have intentionally not imported that unrelated generated lockfile refresh into this narrowly scoped change. All other completed checks are green; the three shared Rust matrix jobs are still in progress. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Independent Windows ARM64 MXC ProcessContainer validation completed on testpc: the sandbox reached Ready with AgentRunning and ConfigurationReady; a v2 filesystem-policy update returned FailedPrecondition; the sandbox remained at revision 1; and policy history retained only v1 as Loaded. |
There was a problem hiding this comment.
Requesting changes for two remaining gaps in the live-policy safety boundary:
- The capability check only guards sandbox-scoped UpdateConfig; global writes and policy-advisor approval and undo paths can still persist revisions for MXC.
policy gettreats a persisted global revision as loaded and active without runtime acknowledgement.
Please centralize the capability check across every operator-authored mutation path, avoid reporting global activation without per-sandbox confirmation, and add regression coverage for global updates plus manual, bulk, automatic approval, and undo. The direct set and update gate and compatibility-preserving capability shape are otherwise sound.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Review changes are pushed in 9fb6fe9. Qualification:
The regressions cover global replacement/deletion, direct operator updates, manual approval, bulk approval, automatic approval, approved-chunk rejection, and undo, and verify rejected operations do not persist or advance policy state. Sandbox-authored startup sync remains supported. @shailendra-nv ready for re-review. |
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes for two remaining gaps in the MXC live-policy invariant:
- The global no-sandbox check is not serialized with provider-free sandbox creation, so a concurrent create and global update can both succeed with different effective policies.
- Provider attachment/detachment and provider-profile fanout still mutate composed effective policy without consulting the live-update capability.
Resolution: make sandbox creation atomic with the global-policy transition for drivers without live updates; apply the capability guard to every provider-driven effective-policy mutation affecting existing sandboxes; add concurrency and provider-path regressions. Please also document the new optional capability contract in architecture/compute-runtimes.md.
The direct set/merge, global sequential, manual/bulk/automatic approval, reject, and undo paths added in 9fb6fe9 are otherwise correctly gated, and current CI is green.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed both capability-boundary/race threads in |
|
@shailendra-nv All requested updates are pushed, the addressed threads are resolved, and validation evidence is posted above. Could you please re-review? |
|
Follow-up CI fix pushed in |
|
Second CI lint follow-up pushed in |
|
Test follow-up pushed in |
|
CI confirmation: the complete Branch Checks matrix is green on |
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes at head 2f7dd18 for one synchronization regression and the remaining contract documentation gap:
- The provider-free create fix now holds the process-wide sandbox synchronization guard across the complete driver CreateSandbox RPC for every driver. A slow provision therefore serializes otherwise independent creates and blocks global/provider mutations even when live policy reload is supported.
- The optional live-update capability and its expanded MXC behavior remain incompletely documented: attachment, detachment, and attached-profile updates are now rejected, but the operator docs and CLI skill describe only policy replacement/merge rejection.
Resolution: preserve the MXC create/global-policy invariant with a capability-conditional or narrower critical section, add a live-update-driver concurrency regression, and document the optional capability plus every affected mutation workflow.
The earlier policy-boundary findings are otherwise resolved: provider-free creation is synchronized, and provider attachment, detachment, and profile fanout are gated with regression coverage. Current CI is green.
|
Addressed the latest requested changes in
Local validation on Windows ARM64:
The two review threads are resolved against the pushed commit. @shailendra-nv, ready for re-review after CI completes. |
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes at head dbf328b for a provider-backed creation race introduced while narrowing the synchronization scope.
The new capability-only condition correctly restores concurrent provider-free creates for live-update drivers, but it also removes the pre-existing sandbox_sync_guard from live-update-driver creates that start with providers. A concurrent profile mutation can scan before the sandbox is persisted, commit after create-time provider policy is snapshotted, and leave the new runtime starting from stale provider-composed policy without identifying that sandbox for reload.
Resolution: keep provider-backed creates serialized while allowing provider-free creates to bypass the guard on capable drivers—for example, acquire it when live updates are unsupported OR the requested provider list is non-empty. Retain the new provider-free concurrency test and restore regression coverage proving a provider-backed create still waits for the guard.
The capability, operator documentation, and prior broad-serialization finding are otherwise resolved. Current CI has no failures; the three Rust jobs are still running.
|
Follow-up review fix pushed in
Revalidated with |
|
CI follow-up pushed in Focused Windows ARM64 proof with the qualification-kit Z3 library:
Result: 1 passed, 0 failed. Formatting and |
Summary
FAILED_PRECONDITIONbefore creating a pending revision, while preserving sandbox-authored startup sync.openshell policy getquery the persisted status for the exact sandbox revision and report the real active version instead of hard-codingEffective; global effective policy remains workspace-readable without querying the admin-only global history endpoint.Internal tracking issue.
Root cause and scope
The gateway deliberately allowed additive filesystem changes because they are safe to store for a later restart, but MXC cannot reload policy into an already-created sandbox. The handler therefore accepted an operator update and persisted a pending revision that could not become active. Separately, the CLI's latest-policy path hard-coded the status and active version, contradicting
policy listand the persisted policy history.The fix is capability-based rather than a handler special case. Older/external drivers that omit the optional capability retain the existing behavior; the in-tree MXC driver explicitly opts out. Settings updates, sandbox-authored startup synchronization, and workspace access to an effective global policy are unchanged.
Reproduction on the unmodified tip
upstream/windowsatfb2980e077288b61ef03a2e6187e162d158526aacargo test -p openshell-server --lib --features bundled-z3 --target aarch64-pc-windows-msvc validate_static_fields_allows_additive_filesystem -- --nocapturependingwhile the sandbox remained active on v1;GetSandboxConfigserved v2, and the CLI latest-policy path labeled iteffective.After
FAILED_PRECONDITIONwith delete/recreate guidance.policy getnow reportsPendingand the actual active version, matching policy history/list semantics.Validation
cargo test -p openshell-server --lib --features bundled-z3 --target aarch64-pc-windows-msvc mxc_ -- --nocapture --test-threads=1— 3 passed (including both new gate/sync regressions)cargo test -p openshell-cli --test sandbox_name_fallback_integration --target aarch64-pc-windows-msvc policy_get_ -- --nocapture --test-threads=1— 5 passed, including pending/active consistency and global-policy access regression coveragecargo test -p openshell-driver-mxc --lib --target aarch64-pc-windows-msvc ui_policy_capability_tracks_configured_backend -- --nocapture— passedcargo fmt --all -- --check— passedgit diff --check— passedcargo clippy -p openshell-server --lib --features bundled-z3 --target aarch64-pc-windows-msvc— passed; only pre-existing warningscargo clippy -p openshell-cli --test sandbox_name_fallback_integration --target aarch64-pc-windows-msvc— passed; only a pre-existing dependency warningcargo clippy -p openshell-driver-mxc --lib --target aarch64-pc-windows-msvc— passed; only pre-existing warningsA combined check of Unix-only compute drivers cannot run on Windows ARM64 because their
opensshdependency intentionally emitsThis crate can only be used on unix; their changes are the mechanical initialization of the new optional protobuf field. Server, CLI, and MXC Windows targets compile and pass.Security impact and residual risk
The gateway now fails closed for the shipped MXC driver instead of accepting a filesystem grant that the live sandbox cannot enforce. Existing sandbox policy is not weakened, and rejected updates produce no pending state. Drivers that do not report the new optional capability preserve legacy behavior for protocol compatibility; MXC explicitly reports
false. Testing used native Windows ARM64; no x64-only input was required.Commit
cbe9e23806ba7802a8fe50de63a779810c89e325is SSH-signed and GitHub reportsverified: true(reason: valid).