diff --git a/docs/backends/bwrap/bubblewrap-backend.md b/docs/backends/bwrap/bubblewrap-backend.md index 0c93f1e157..cc6c2c3b66 100644 --- a/docs/backends/bwrap/bubblewrap-backend.md +++ b/docs/backends/bwrap/bubblewrap-backend.md @@ -991,6 +991,18 @@ isolated wrapper that removes `--die-with-parent`, then checks explicit kill, timeout, and handle-drop cleanup. This reproduces the absence of parent-death protection during startup without relying on a scheduling race. +## Troubleshooting + +These failures carry the `backend_error` code and exit `-1`. + +| Message | Meaning | +|---|---| +| `Bubblewrap (bwrap) is not installed or not on PATH` / version too old | The host has no usable Bubblewrap; install `bubblewrap` 0.5+ | +| `requires 'slirp4netns' on PATH` / `requires unprivileged user and network namespaces` | The host cannot provide the sandbox network | +| `script_code is empty` / `must not carry credentials` / an egress or proxy refusal | The request is at fault; change it and retry | +| `failed to spawn bwrap` / `failed to allocate PTY` / proxy namespace start failure | The request was accepted but the launch faulted | +| `failed to wrap stdio pipes` | The sandbox started and then faulted | + ## Limitations - **Linux only** — Bubblewrap requires Linux kernel namespaces diff --git a/docs/backends/hyperlight/hyperlight-backend.md b/docs/backends/hyperlight/hyperlight-backend.md index ad22aa2db2..c64f06789d 100644 --- a/docs/backends/hyperlight/hyperlight-backend.md +++ b/docs/backends/hyperlight/hyperlight-backend.md @@ -115,7 +115,7 @@ to the snapshot between calls. | Aspect | Behaviour | |---|---| -| Exit code | The script's: `sys.exit(N)`, `process.exit(N)` or `exit N` gives `N`, an uncaught exception gives `1`, a runner error gives `-1` with the reason in `error_message` | +| Exit code | The script's: `sys.exit(N)`, `process.exit(N)` or `exit N` gives `N`, an uncaught exception gives `1`. A refused request gives `1` and a host or runtime fault gives `-1`, with the reason in `error_message` | | Timeout | `process.timeout` bounds the run; a guest that overruns is interrupted and the run reports `execution timed out` | | stdout / stderr | Inherited by the guest; `ScriptResponse.standard_out` stays empty, so capture at the process level | @@ -155,11 +155,18 @@ contain whitespace, `:` or brackets, and two mounts may not share one. ## Troubleshooting +Failures carry an error code: `policy_validation` (exit 1) when the request +asks for something the backend refuses, `backend_unavailable` (exit -1) when +the host cannot provide the backend at all, and `backend_error` (exit -1) for +setup and runtime faults. + | Message | Meaning | |---|---| -| `no hyperlight image found for the runtime` | No home holds that runtime; run `--setup-hyperlight=`, or set `MXC_HYPERLIGHT_HOME` | +| `hyperlight policy error: …` | The request is at fault (`policy_validation`); change it and retry | +| `no hyperlight image found for the runtime` | No home holds that runtime (`backend_unavailable`); run `--setup-hyperlight=`, or set `MXC_HYPERLIGHT_HOME` | | `holds a rootfs from another hyperlight-unikraft release` | The directory predates the crate the binary was built with; `--setup-hyperlight=` rebuilds it | -| `Hyperlight requires KVM` / `Hyperlight requires Windows Hypervisor Platform` | No hypervisor for this process (error code `backend_unavailable`): on Linux `/dev/kvm` is missing or not readable and writable, on Windows WHP is not enabled | +| `Hyperlight requires KVM` / `Hyperlight requires Windows Hypervisor Platform` | No hypervisor for this process (`backend_unavailable`): on Linux `/dev/kvm` is missing or not readable and writable, on Windows WHP is not enabled | +| `hyperlight setup error: …` / `hyperlight runtime error: …` | The run was accepted but faulted (`backend_error`) | | `execution timed out after N s` | The script overran `process.timeout` | Run with `--debug` to see the home chosen, restore and call timings, and diff --git a/docs/backends/lxc/lxc-backend.md b/docs/backends/lxc/lxc-backend.md index eee82f05db..d027d66365 100644 --- a/docs/backends/lxc/lxc-backend.md +++ b/docs/backends/lxc/lxc-backend.md @@ -324,6 +324,18 @@ streaming in a long-lived service. sudo lxc-checkconfig ``` +## Troubleshooting + +These failures carry the `backend_error` code and exit `-1`. + +| Message | Meaning | +|---|---| +| `the LXC runtime is not available on this host` | `lxc-ls --version` did not run; install the LXC tools | +| `sandboxes can only be launched on Linux` | Wrong OS for the backend | +| `distribution and release are required` / a proxy, egress or inherited-stdio refusal | The request is at fault; change it and retry | +| `Failed to create container` / `Failed to start container` / `container '…' is already in use` | The request was accepted but the launch faulted | +| `did not receive an IPv4 address` / `Failed to discover the container init PID` | The container started and then faulted before the script ran | + ## Limitations - **The workload can send only ICMP Echo.** A `protocol: "icmp"` rule lets every diff --git a/docs/development/architecture/backends/isolation-session/state-aware-rust.md b/docs/development/architecture/backends/isolation-session/state-aware-rust.md index bace284f57..3a9faf9f40 100644 --- a/docs/development/architecture/backends/isolation-session/state-aware-rust.md +++ b/docs/development/architecture/backends/isolation-session/state-aware-rust.md @@ -496,13 +496,11 @@ State-aware exec (and other phases) use OS-level cancellation in v1: - On the `wxc-exec` route, cancellation is process termination. - The agent process's pipes EOF, the relay threads exit. - The OS-side service's per-process timer (set from - `process.timeout`) reaps the agent if the runner does not. On the - streaming path that timer is armed with a **margin** past the caller's - deadline, so it acts purely as a watchdog: the service kills with an - ordinary exit code (the host suite pins it as exit code 1), so if it - fired first a genuine timeout would be indistinguishable from a normal - exit and could not be reported as one. The run-to-completion path arms - it unchanged — it has no timeout channel to report through. + `process.timeout`) reaps the agent if the runner does not. That timer is + armed with a **margin** past the caller's deadline, so it acts purely as + a watchdog: the service kills with an ordinary exit code, so if it fired + first a genuine timeout would be indistinguishable from a normal exit + and could not be reported as one. - The runner's existing 3-tier shutdown (`CloseStandardInput` → `SendCtrlClose` → `Terminate`) handles the timeout case from inside the agent process before returning. diff --git a/docs/development/architecture/container-lifecycle.md b/docs/development/architecture/container-lifecycle.md index f620b70255..1cc78f6f18 100644 --- a/docs/development/architecture/container-lifecycle.md +++ b/docs/development/architecture/container-lifecycle.md @@ -616,9 +616,9 @@ diagnostic. The rejection is recorded through the same `ConfigRejected` audit pa Diagnostics are never duplicated to stdout, and lifecycle stderr is not gated on `--debug`. -Without `--operation`, `wxc-exec` accepts only one-shot requests. Input-source and -pre-parse failures on that path retain the legacy primary diagnostic on stderr and do -not emit a lifecycle envelope. Raw SDK and FFI lifecycle calls do not use this CLI +Without `--operation`, `wxc-exec` accepts only create-and-run requests. Input-source +and pre-parse failures on that path emit a typed `{error}` envelope on stderr rather +than the lifecycle envelope on stdout. Raw SDK and FFI lifecycle calls do not use this CLI stream protocol; they return their status and error data through their binding result surfaces. @@ -671,7 +671,10 @@ because a script's stderr may not end in a newline. `code` and `message` are always present. `code` is the machine-readable category a consumer branches on; `message` is the human-readable description, and for a failure raised by an underlying platform API it is that API's own message, passed through -verbatim rather than concatenated with the other fields. +verbatim rather than concatenated with the other fields. The executor's own exit +code follows `code`: a refused request exits `1`, and an unavailable host or a +runtime fault exits `-1`. See the error contract in +[versioning.md](versioning.md#error-contract). The three optional named fields carry structured failure detail. `operation` and `nativeCode` describe an underlying platform call; `remediation` describes the failure: diff --git a/docs/development/architecture/versioning.md b/docs/development/architecture/versioning.md index a60da5dee3..74c3774a1f 100644 --- a/docs/development/architecture/versioning.md +++ b/docs/development/architecture/versioning.md @@ -712,6 +712,49 @@ Negotiation failures are **typed and actionable** — never a silent fallback: unsupported field. Security policy is deterministic — no relaxation, no fuzzy fallback. +Every executor failure is classified by **which side can act on it**, and that +classification drives both the wire code and the process exit code: + +| Side at fault | Failure phase | Wire code | Exit code | +|---|---|---|---| +| The request — a different request succeeds | `Rejected` | `policy_validation` | `1` | +| The host — no request change helps | `BackendUnavailable` | `backend_unavailable` | `-1` | +| The runtime, during an accepted run | all other phases | `backend_error` | `-1` | + +Common cases: + +| Failure | Wire code | +|---|---| +| Policy field the backend cannot enforce (e.g. ProcessContainer `enumeratePaths`, `deniedPaths`, LPAC) | `backend_unavailable`¹ | +| Policy path the invoking user cannot access | `backend_error`¹ | +| Host fault while checking a policy path (I/O error, unresolvable path) | `backend_error` | +| Missing host capability (e.g. no native PSEC, no denial-capture provider) | `backend_unavailable` | +| Backend not compiled into this build | `unsupported_containment`¹ | +| Experimental backend without the experimental opt-in | `backend_unavailable` | +| Backend for another OS: create-and-run / container lifecycle operations | `unsupported_containment` / `backend_unavailable` | + +Unsupported policy is never silently dropped, and no failure above starts the +run. The phase-specific codes `malformed_request`, `unsupported_containment`, +`unsupported_phase`, and `malformed_id` also exit `1`. + +¹ Kept for SDK v1 compatibility, as are some backend refusals that still report +`backend_error`. See the [SDK v2 error codes plan](../plans/sdk-v2-error-codes.md). + +Envelope lines share stderr with the workload's own output, so a workload can +print text that looks like an envelope. Treat stderr envelopes as diagnostics, +not as an authenticated result; the exit code and a held `ScriptResponse` are +authoritative. + +`ScriptResponse` carries an optional `warnings` array of diagnostic strings. +Warnings are advisory and never change the exit code; they are omitted from the +wire entirely when a run produced none. The serialized response and the +executor's stderr stream are two distinct shapes of the same warnings: on the +response they are a `warnings: string[]` field, while an executor writes each +one to stderr as its own line-delimited JSON object, +`{"warning":{"message":"..."}}`, matching the error envelope +`{"error":{"code":"...","message":"..."}}`. A caller reading stderr must parse +the per-line objects; the array exists only on a response it holds directly. + The MXC ↔ OS contract therefore reports: which feature failed, whether it was a version mismatch or a runtime/capability unavailability (e.g. Hyper-V off), and what the user should do (upgrade OS, enable feature, change the config). diff --git a/docs/development/plans/sdk-v2-error-codes.md b/docs/development/plans/sdk-v2-error-codes.md new file mode 100644 index 0000000000..8e7bc3d4e7 --- /dev/null +++ b/docs/development/plans/sdk-v2-error-codes.md @@ -0,0 +1,41 @@ + + +# SDK v2 Error Codes + +> **Audience:** MXC developers + +> **Status:** WIP, tracked by [#1595](https://github.com/microsoft/mxc/issues/1595) + +Failures are meant to be classified by the side that can fix them: the request +(`policy_validation`, exit `1`), the host (`backend_unavailable`, exit `-1`), or +the runtime (`backend_error`, exit `-1`). See the +[error contract](../architecture/versioning.md#error-contract). + +The sites below still report their SDK v1 code, because changing it would break +published SDK callers. Each one has a `// SDK v2:` comment that names its +intended code. Move them together in the next major SDK version. + +| Site | SDK v1 | SDK v2 | +|---|---|---| +| Shared validator: empty script, `enumeratePaths` off ProcessContainer | `backend_error` | `policy_validation` | +| Policy path checks (`FilesystemPolicyError::into_response`) | `backend_error` | `policy_validation`, or `backend_error` with phase `LaunchFailed` for host faults | +| Backend not compiled in (`mxc_engine` `run` and `dispatch`) | `unsupported_containment` | `backend_unavailable` | +| ProcessContainer: unenforceable field (`deniedPaths`, `enumeratePaths`, LPAC) | `backend_unavailable` | `policy_validation` | +| ProcessContainer: PSEC baseline unsupported | `policy_validation` | `backend_unavailable` | +| Bubblewrap: request refusals | `backend_error` | `policy_validation` | +| Bubblewrap: missing or too-old `bwrap`, no network support; proxy lookup failure | `backend_error` | `backend_unavailable`; phase `LaunchFailed` | +| LXC: request refusals | `backend_error` | `policy_validation` | +| LXC: tooling missing, streaming off Linux | `backend_error` | `backend_unavailable` | +| Seatbelt: refusals (`rejected_response`) | `backend_error` | `policy_validation` | +| WSLC: policy-path host faults | `policy_validation` | `backend_error` | +| IsolationSession one-shot: lifecycle refusals, `IsolationSessionError` conversion | `backend_error` | `policy_validation` / `backend_unavailable` | +| IsolationSession one-shot: `validate` and launch failures always use phases `Rejected` and `LaunchFailed` | as forced | the phase from the underlying error | +| IsolationSession relayed exec timeout | exit code `-1` | `backend_error` "script timed out" | + +## Remaining work + +- Switch each site to its SDK v2 code behind the v2 SDK surface, keeping v1 callers on the v1 codes. +- Update the Rust, .NET and Node references under `docs/api-reference/`, the backend guides and the host test scripts under `tests/scripts/`. diff --git a/src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs b/src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs index ac86b7b120..9c93a8ac42 100644 --- a/src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs +++ b/src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs @@ -22,10 +22,11 @@ use std::process::{Child, ChildStdin, Command, ExitStatus, Stdio}; use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; use std::time::{Duration, Instant}; -use crate::mxc_common::filesystem_symlink::resolve_through_symlinks; +use crate::mxc_common::filesystem_object::FilesystemPolicyError; +use crate::mxc_common::filesystem_symlink::{is_policy_path_error, resolve_through_symlinks}; use crate::mxc_common::interruptible_reader::{wrap_pipe, InterruptibleReader, ReadCanceller}; use crate::mxc_common::logger::Logger; -use crate::mxc_common::models::{ExecutionRequest, ScriptResponse}; +use crate::mxc_common::models::{ExecutionRequest, FailurePhase, ScriptResponse}; use crate::mxc_common::sandbox_process::{ boxed_closer, cancel_and_join_discard, duplicate_and_take_native_stdio, group_kill, spawn_discard, take_boxed_read, take_boxed_write, wait_with_timeout, NativeStdio, PtySize, @@ -45,6 +46,15 @@ use crate::bwrap_common::{ const DNS_PORT: u16 = 53; +// SDK v2: Rejected -> rejected (policy_validation), Lookup -> failed(LaunchFailed). +// See docs/development/plans/sdk-v2-error-codes.md. +fn proxy_resolve_failure_response(error: proxy_network::ProxyResolveError) -> ScriptResponse { + match error { + proxy_network::ProxyResolveError::Rejected(message) + | proxy_network::ProxyResolveError::Lookup(message) => ScriptResponse::error(&message), + } +} + /// Bubblewrap sandbox runner. Uses only shared `ContainerPolicy` fields — /// no backend-specific config struct required. #[derive(Default)] @@ -114,22 +124,18 @@ impl SandboxBackend for BubblewrapScriptRunner { &normalized } Ok(None) => request, - Err(msg) => return Err(ScriptResponse::error(&msg)), + Err(error) => return Err(error.into_response()), }; // Reject any policy path the invoking user cannot access, so the sandbox // never gains access the caller lacks. Runs after object normalization // so it sees the already-tightened intents. - if let Err(msg) = crate::mxc_common::filesystem_access::check_delegation(&request.policy) { - return Err(ScriptResponse::error(&msg)); - } + crate::mxc_common::filesystem_access::check_delegation(&request.policy) + .map_err(|error| error.into_response())?; // Resolve denied paths that traverse a symlink to their real host path // and classify each as a file/dir mask (see [`resolve_denied_paths`]). // Only clones the request when a path needs rewriting (common case: // none). See docs/backends/bwrap/bubblewrap-backend.md. - let plan = match resolve_denied_paths(&request.policy, logger) { - Ok(plan) => plan, - Err(msg) => return Err(ScriptResponse::error(&msg)), - }; + let plan = resolve_denied_paths(&request.policy, logger).map_err(|e| e.into_response())?; let resolved; let request = match plan.paths { Some(denied_paths) => { @@ -146,6 +152,7 @@ impl SandboxBackend for BubblewrapScriptRunner { // The masks are built from the list above, not from the one the caller // wrote, so the pin conflict has to be judged against it too. if let Err(msg) = check_pin_against_denied_hosts(request) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&msg)); } warn_resolver_mount_hint(request, logger); @@ -189,6 +196,7 @@ impl BubblewrapScriptRunner { // probe so config errors are reported deterministically even on // hosts without bwrap installed. if request.script_code.is_empty() { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "script_code is empty — nothing to execute.", )); @@ -217,6 +225,7 @@ impl BubblewrapScriptRunner { if crate::mxc_common::proxy_env::proxy_url_has_credentials(&url) { // Built from the redacted form so the rejection cannot become // the leak it is rejecting. + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&format!( "Bubblewrap: runtimeConfig.networkProxy must not carry credentials ('{}'). \ Bubblewrap passes the proxy URL to bwrap as a --setenv command-line \ @@ -233,9 +242,11 @@ impl BubblewrapScriptRunner { // Programmatic requests also need the parser's proxy-only restriction: // direct egress must not be accepted and then discarded at launch. if let Some(reason) = bwrap_command::proxy_with_egress_rejection(request) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(reason)); } if let Some(reason) = bwrap_command::directional_network_rejection(request) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(reason)); } @@ -258,11 +269,13 @@ impl BubblewrapScriptRunner { if proxy_only { if let Some(address) = request.policy.network_proxy.address.as_ref() { if let Err(error) = proxy_network::SandboxProxy::check_without_resolving(address) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&error)); } if let Err(error) = proxy_network::check_hosts_pin_against_policy(address, &request.policy) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&error)); } // Repeated in `spawn` against the normalized policy. @@ -277,6 +290,7 @@ impl BubblewrapScriptRunner { let egress_plan = if firewall_enforced { match network_rules::EgressPlan::for_request(request) { Ok(plan) => Some(plan), + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. Err(error) => return Err(ScriptResponse::error(&error)), } } else { @@ -287,6 +301,7 @@ impl BubblewrapScriptRunner { // builder emits — an old binary would otherwise fail at spawn time with // an opaque "unknown option" error. if let Err(err) = probe() { + // SDK v2: unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&err.to_string())); } if proxy_only || firewall_enforced { @@ -298,6 +313,7 @@ impl BubblewrapScriptRunner { proxy_network::PrivateNetworkUse::FirewallEnforcement }; if let Err(error) = proxy_network::probe_dependencies(use_case) { + // SDK v2: unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&error)); } } @@ -457,7 +473,7 @@ impl BubblewrapScriptRunner { })?; Some( proxy_network::SandboxProxy::resolve(address) - .map_err(|error| ScriptResponse::error(&error))?, + .map_err(proxy_resolve_failure_response)?, ) } else { None @@ -485,7 +501,9 @@ impl BubblewrapScriptRunner { request.script_timeout, ) { Ok(network) => Some(network), - Err(error) => return Err(ScriptResponse::error(&error)), + Err(error) => { + return Err(ScriptResponse::failed(FailurePhase::LaunchFailed, &error)) + } } } // Direct directional egress uses the same private namespace. @@ -498,6 +516,7 @@ impl BubblewrapScriptRunner { Some(plan) => plan, None => match network_rules::EgressPlan::for_request(request) { Ok(plan) => plan, + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. Err(error) => return Err(ScriptResponse::error(&error)), }, }; @@ -514,7 +533,9 @@ impl BubblewrapScriptRunner { warn_resolver_blocked_by_egress(&plan, network.resolver_pinned(), logger); Some(network) } - Err(error) => return Err(ScriptResponse::error(&error)), + Err(error) => { + return Err(ScriptResponse::failed(FailurePhase::LaunchFailed, &error)) + } } } None => None, @@ -524,7 +545,7 @@ impl BubblewrapScriptRunner { Ok(file) => file, Err(error) => { stop_proxy_network(&mut proxy_network, logger); - return Err(ScriptResponse::error(&error)); + return Err(ScriptResponse::failed(FailurePhase::LaunchFailed, &error)); } }; let mut args = bwrap_command::build_args_classified_with_mode( @@ -543,7 +564,7 @@ impl BubblewrapScriptRunner { Ok(startup) => Some(startup), Err(error) => { stop_proxy_network(&mut proxy_network, logger); - return Err(ScriptResponse::error(&error)); + return Err(ScriptResponse::failed(FailurePhase::LaunchFailed, &error)); } }, None => None, @@ -587,9 +608,10 @@ impl BubblewrapScriptRunner { Ok(pty) => Some(pty), Err(error) => { stop_proxy_network(&mut proxy_network, logger); - return Err(ScriptResponse::error(&format!( - "Bubblewrap: failed to allocate PTY: {error}" - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!("Bubblewrap: failed to allocate PTY: {error}"), + )); } }, }; @@ -616,10 +638,10 @@ impl BubblewrapScriptRunner { Ok(process) => process, Err(error) => { stop_proxy_network(&mut proxy_network, logger); - return Err(ScriptResponse::error(&format!( - "Bubblewrap: failed to spawn bwrap: {}", - error - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!("Bubblewrap: failed to spawn bwrap: {}", error), + )); } }; @@ -642,7 +664,10 @@ impl BubblewrapScriptRunner { let _ = child.kill(); let _ = child.wait(); stop_proxy_network(&mut proxy_network, logger); - return Err(ScriptResponse::error(&error)); + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &error, + )); } } @@ -665,10 +690,13 @@ impl BubblewrapScriptRunner { let _ = child.wait(); stop_proxy_network(&mut proxy_network, logger); let error = out_result.err().or(err_result.err()); - return Err(ScriptResponse::error(&format!( - "Bubblewrap: failed to wrap stdio pipes: {}", - error.map_or_else(|| "unknown error".to_string(), |e| e.to_string()), - ))); + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &format!( + "Bubblewrap: failed to wrap stdio pipes: {}", + error.map_or_else(|| "unknown error".to_string(), |e| e.to_string()), + ), + )); } }; let timeout = if request.script_timeout == 0 { @@ -1171,23 +1199,28 @@ struct DeniedPlan { fn resolve_denied_paths( policy: &crate::mxc_common::models::ContainerPolicy, logger: &mut Logger, -) -> Result { +) -> Result { let mut changed = false; let mut out = Vec::with_capacity(policy.denied_paths.len()); let mut files = HashSet::new(); for p in &policy.denied_paths { if let Some(resolved) = resolve_through_symlinks(Path::new(p)) { if let Some(error) = resolved.error { - return Err(format!( + let message = format!( "Bubblewrap: deniedPaths entry '{p}' could not be resolved ({error}); \ refusing to start." - )); + ); + return Err(if is_policy_path_error(&error) { + FilesystemPolicyError::Policy(message) + } else { + FilesystemPolicyError::Host(message) + }); } let resolved = resolved.path.to_str().ok_or_else(|| { - format!( + FilesystemPolicyError::Policy(format!( "Bubblewrap: deniedPaths entry '{p}' resolves to a non-UTF-8 host path that \ cannot be safely masked; refusing to start." - ) + )) })?; if resolved != p.as_str() { let _ = writeln!( @@ -1236,6 +1269,39 @@ mod tests { } } + #[test] + fn a_failed_bwrap_spawn_is_a_launch_failure() { + use crate::mxc_common::logger::Mode; + use crate::mxc_common::mxc_error::MxcErrorCode; + + // A NUL in an argument makes `Command::spawn` fail before exec. + let request = ExecutionRequest { + script_code: "echo \0".into(), + ..Default::default() + }; + let Some(resp) = BubblewrapScriptRunner::new() + .spawn_bwrap( + &request, + &HashSet::new(), + None, + &mut Logger::new(Mode::Buffer), + StdioMode::Pipes, + ) + .err() + else { + panic!("spawning bwrap with a NUL argument must fail"); + }; + + assert_eq!(resp.failure_phase, FailurePhase::LaunchFailed); + assert_eq!(resp.failure_phase.error_code(), MxcErrorCode::BackendError); + assert_eq!(resp.exit_code, -1); + assert!( + resp.error_message.contains("failed to spawn bwrap"), + "got: {}", + resp.error_message + ); + } + #[test] fn resolver_hint_is_retained_for_unmatched_absolute_targets() { use crate::mxc_common::logger::Mode; @@ -2149,6 +2215,46 @@ mod tests { } } + // SDK v2: refusals report policy_validation and a missing bwrap reports + // backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. + #[test] + fn refusals_and_a_missing_bwrap_report_backend_error() { + use crate::mxc_common::mxc_error::MxcErrorCode; + + let runner = BubblewrapScriptRunner::new(); + let empty_script = ExecutionRequest { + script_code: String::new(), + ..base_request() + }; + + for (what, error) in [ + ( + "empty script_code", + runner.validate(&empty_script).unwrap_err(), + ), + ( + "proxy credentials", + runner + .validate(&request_with_a_credential_bearing_proxy()) + .unwrap_err(), + ), + ( + "missing bwrap", + runner + .validate_prepared_with_probe(&base_request(), || { + Err(bwrap_version::BwrapUnavailable::NotFound) + }) + .unwrap_err(), + ), + ] { + assert_eq!( + error.failure_phase.error_code(), + MxcErrorCode::BackendError, + "input={what}; output={error:?}" + ); + } + } + /// A denied symlink pointing at a **directory** is rewritten to its canonical /// target so the mask lands on the real directory (bwrap cannot mount a mask /// over a symlink whose parent is bound). The resolved directory is @@ -2329,7 +2435,10 @@ mod tests { let error = resolve_denied_paths(&policy, &mut logger) .err() .unwrap_or_else(|| panic!("{} must fail closed", denied.display())); - assert!(error.contains("could not be resolved"), "{error}"); + assert!( + matches!(&error, FilesystemPolicyError::Policy(m) if m.contains("could not be resolved")), + "{error:?}" + ); } } diff --git a/src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs b/src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs index 73e8742895..4ad34a798d 100644 --- a/src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs +++ b/src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs @@ -289,6 +289,35 @@ pub(crate) struct SandboxProxy { egress: ProxyEgress, } +/// Why [`SandboxProxy::resolve`] failed. +#[derive(Debug)] +pub(crate) enum ProxyResolveError { + /// The endpoint, or the address it resolves to, cannot be enforced. + Rejected(String), + /// The host lookup itself did not produce an answer. + Lookup(String), +} + +impl ProxyResolveError { + pub(crate) fn message(&self) -> &str { + match self { + Self::Rejected(message) | Self::Lookup(message) => message, + } + } +} + +impl std::fmt::Display for ProxyResolveError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.message()) + } +} + +impl From for ProxyResolveError { + fn from(message: String) -> Self { + Self::Rejected(message) + } +} + impl SandboxProxy { /// Derive the sandbox-visible proxy from the configured one. /// @@ -296,7 +325,7 @@ impl SandboxProxy { /// an address: the workload then presents the `Host` header and proxy-auth /// realm the operator configured. An IP literal has no name to pin, so a /// loopback literal is rewritten to slirp's gateway instead. - pub(crate) fn resolve(configured: &ProxyAddress) -> Result { + pub(crate) fn resolve(configured: &ProxyAddress) -> Result { Self::resolve_with(configured, resolve_ipv4) } @@ -311,7 +340,7 @@ impl SandboxProxy { if needs_lookup { return Ok(()); } - checked + checked.map_err(|error| error.message().to_string()) } /// Run the static checks, reporting whether a lookup would have followed. @@ -320,11 +349,13 @@ impl SandboxProxy { /// also exactly when a hosts pin is created -- an IP literal resolves to /// `Ok(None)` from [`ProxyAddress::host_pin`]. Callers use the flag to /// reason about the pin without duplicating that classification. - fn inspect_without_resolving(configured: &ProxyAddress) -> (bool, Result<(), String>) { + fn inspect_without_resolving( + configured: &ProxyAddress, + ) -> (bool, Result<(), ProxyResolveError>) { let asked_to_resolve = Cell::new(false); let checked = Self::resolve_with(configured, |_, _| { asked_to_resolve.set(true); - Err(String::new()) + Err(ProxyResolveError::Lookup(String::new())) }); (asked_to_resolve.get(), checked.map(|_| ())) @@ -337,10 +368,14 @@ impl SandboxProxy { /// a lookup whose answer the test does not control. fn resolve_with( configured: &ProxyAddress, - resolve: impl Fn(&str, u16) -> Result, - ) -> Result { + resolve: impl Fn(&str, u16) -> Result, + ) -> Result { if configured.port() == 0 { - return Err("Bubblewrap: proxy-only egress requires a non-zero proxy port".to_string()); + return Err( + "Bubblewrap: proxy-only egress requires a non-zero proxy port" + .to_string() + .into(), + ); } let host = configured.host().trim_matches(['[', ']']); @@ -376,7 +411,7 @@ impl SandboxProxy { // an address nothing answers on -- failing at connect time rather // than at policy time. if ip.is_unspecified() { - return Self::rewritten(configured); + return Ok(Self::rewritten(configured)?); } if ip.is_loopback() { return Err(format!( @@ -384,13 +419,14 @@ impl SandboxProxy { network namespace cannot reach; bind the proxy to 127.0.0.1 or a dual-stack \ wildcard address instead. The egress rule is emitted with IPv4 iptables.", configured.host() - )); + ) + .into()); } - return Err(ipv6_unsupported(host)); + return Err(ipv6_unsupported(host).into()); } if is_reserved_loopback_name { - return Self::rewritten(configured); + return Ok(Self::rewritten(configured)?); } let resolved = resolve(host, configured.port())?; @@ -515,27 +551,29 @@ fn with_deadline( /// `getaddrinfo` cannot be cancelled, so the lookup is moved to a thread and /// abandoned once the bound expires -- the wait is bounded even though the /// query itself keeps running until the resolver gives up. -fn resolve_ipv4(host: &str, port: u16) -> Result { +fn resolve_ipv4(host: &str, port: u16) -> Result { let query = host.to_string(); let waited = with_deadline(RESOLVE_TIMEOUT_LABEL, RESOLVE_TIMEOUT, move || { lookup_ipv4(&query, port) }) .map_err(|error| { - format!("Bubblewrap: could not start the lookup of proxy host '{host}': {error}") + ProxyResolveError::Lookup(format!( + "Bubblewrap: could not start the lookup of proxy host '{host}': {error}" + )) })?; match waited { Ok(resolved) => resolved, - Err(mpsc::RecvTimeoutError::Timeout) => Err(format!( + Err(mpsc::RecvTimeoutError::Timeout) => Err(ProxyResolveError::Lookup(format!( "Bubblewrap: resolving proxy host '{host}' exceeded {}s. The proxy endpoint is \ resolved on the host because DNS is closed inside the sandbox, so an unresponsive \ host resolver blocks the sandbox from starting. Check the host's DNS configuration, \ or give the proxy as an IPv4 address to skip resolution.", RESOLVE_TIMEOUT.as_secs() - )), - Err(mpsc::RecvTimeoutError::Disconnected) => Err(format!( + ))), + Err(mpsc::RecvTimeoutError::Disconnected) => Err(ProxyResolveError::Lookup(format!( "Bubblewrap: the lookup of proxy host '{host}' ended without an answer." - )), + ))), } } @@ -544,14 +582,14 @@ fn resolve_ipv4(host: &str, port: u16) -> Result { /// Only the first IPv4 answer is used, and it is the same one the sandbox is /// pinned to. Opening the rest would widen the chain to addresses the sandbox /// can no longer select: with DNS closed, the pin is its only resolution path. -fn lookup_ipv4(host: &str, port: u16) -> Result { +fn lookup_ipv4(host: &str, port: u16) -> Result { let resolved: Vec = (host, port) .to_socket_addrs() .map_err(|error| { - format!( + ProxyResolveError::Lookup(format!( "Bubblewrap: could not resolve proxy host '{host}': {error}. The proxy endpoint \ is resolved on the host because DNS is closed inside the sandbox." - ) + )) })? .map(|addr| addr.ip()) .collect(); @@ -563,11 +601,11 @@ fn lookup_ipv4(host: &str, port: u16) -> Result { // A name with AAAA records and no A records is the same unenforceable case // as an IPv6 literal, so say so rather than claiming it does not resolve. if resolved.is_empty() { - Err(format!( + Err(ProxyResolveError::Lookup(format!( "Bubblewrap: proxy host '{host}' did not resolve to any address." - )) + ))) } else { - Err(ipv6_unsupported(host)) + Err(ipv6_unsupported(host).into()) } } @@ -2514,6 +2552,9 @@ mod tests { let error = SandboxProxy::resolve_with(&address, resolver([10, 0, 2, 2])) .expect_err("the slirp gateway must not be pinned as if it were a host address"); + let ProxyResolveError::Rejected(error) = error else { + panic!("a reserved answer is a policy refusal: {error}"); + }; assert!( error.contains("10.0.2.0/24") && error.contains("127.0.0.1"), "error should name the reserved range and the supported alternative: {error}" @@ -2587,7 +2628,7 @@ mod tests { /// A resolver that answers every name with `ip`, so pin decisions can be /// tested without a lookup the test does not control. - fn resolver(ip: [u8; 4]) -> impl Fn(&str, u16) -> Result { + fn resolver(ip: [u8; 4]) -> impl Fn(&str, u16) -> Result { move |_, _| Ok(Ipv4Addr::from(ip)) } @@ -2729,7 +2770,7 @@ mod tests { let loopback = ProxyAddress::new("[::1]".into(), 8080); let error = SandboxProxy::resolve(&loopback).unwrap_err(); assert!( - error.contains("IPv4"), + error.message().contains("IPv4"), "error should explain the IPv4 requirement: {error}" ); @@ -2749,12 +2790,39 @@ mod tests { ); let error = SandboxProxy::resolve(&address).unwrap_err(); + // A lookup failure is a host fault, not a refusal of the policy. + let ProxyResolveError::Lookup(error) = error else { + panic!("an unresolvable name is a lookup failure: {error}"); + }; assert!( error.contains("proxy.corp.invalid"), "error should name the offending host: {error}" ); } + #[test] + fn a_failed_lookup_is_not_reported_as_a_refusal() { + let address = ProxyAddress::new("proxy.example".into(), 3128); + let error = SandboxProxy::resolve_with(&address, |_, _| { + Err(ProxyResolveError::Lookup("timed out".into())) + }) + .unwrap_err(); + + assert!(matches!(error, ProxyResolveError::Lookup(_)), "{error}"); + } + + #[test] + fn static_rejections_are_refusals() { + for host in ["[::1]", "2001:db8::1", "10.0.2.2"] { + let address = ProxyAddress::new(host.into(), 8080); + let error = SandboxProxy::resolve(&address).unwrap_err(); + assert!( + matches!(error, ProxyResolveError::Rejected(_)), + "{host}: {error}" + ); + } + } + /// `::1` used to be rewritten to the IPv4 gateway, which handed the sandbox /// an address the proxy never listens on: an IPv6-loopback listener does /// not accept the IPv4 connection slirp's gateway produces. Rejecting at @@ -2766,7 +2834,7 @@ mod tests { let error = SandboxProxy::resolve(&address).unwrap_err(); assert!( - error.contains("IPv6 loopback"), + error.message().contains("IPv6 loopback"), "error should name the unreachable address family: {error}" ); } @@ -2788,7 +2856,7 @@ mod tests { let error = SandboxProxy::resolve(&address).unwrap_err(); assert!( - error.contains("non-zero"), + error.message().contains("non-zero"), "error should explain the port requirement: {error}" ); } diff --git a/src/mxc-sdk/src/backends/hyperlight/common/mod.rs b/src/mxc-sdk/src/backends/hyperlight/common/mod.rs index 133f2dd22e..e592858a73 100644 --- a/src/mxc-sdk/src/backends/hyperlight/common/mod.rs +++ b/src/mxc-sdk/src/backends/hyperlight/common/mod.rs @@ -89,9 +89,10 @@ //! ## Exit codes //! //! Guest exit code on clean completion (0 for normal exit, non-zero for -//! `sys.exit(N)` or unhandled exceptions), -1 on any runner error -//! (preflight, install, runtime, guest crash, timeout). The specific -//! failure mode is in `error_message`. +//! `sys.exit(N)` or unhandled exceptions). A refused request exits 1; a +//! host that cannot provide the backend, a failed install or boot, a guest +//! crash and a timeout all exit -1. The specific failure mode is in +//! `error_message`, and `failure_phase` says which side is at fault. use std::path::{Path, PathBuf}; use std::sync::mpsc::{self, RecvTimeoutError}; @@ -99,7 +100,9 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use crate::mxc_common::logger::Logger; -use crate::mxc_common::models::{ExecutionRequest, HyperlightRuntime, ScriptResponse}; +use crate::mxc_common::models::{ + ExecutionRequest, FailurePhase, HyperlightRuntime, ScriptResponse, +}; use crate::mxc_common::script_runner::ScriptRunner; use crate::mxc_common::validator::{ validate_common, validate_network_policy_support, NetworkPolicySupport, @@ -137,32 +140,53 @@ pub fn is_whp_available() -> bool { #[derive(Debug)] enum RunnerError { - /// Pre-spawn validation failures (missing image, unsupported policy). - Preflight(String), - /// Runtime construction, install, or execution failure. - Runtime(String), + /// The request asks for something the guest cannot do. + Policy(String), + /// This host cannot provide the backend: no usable guest image for the + /// requested runtime, and nothing to install one from. + Unavailable(String), + /// Host-side preparation of a run failed (mount staging, path resolution). + Setup(String), + /// Boot, install, or snapshot bring-up failed. + Launch(String), + /// The guest failed while running the call. + Exec(String), + /// The call outlived `scriptTimeout` and was cut short. + Timeout(String), } impl std::fmt::Display for RunnerError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - RunnerError::Preflight(msg) => write!(f, "hyperlight preflight error: {msg}"), - RunnerError::Runtime(msg) => write!(f, "hyperlight runtime error: {msg}"), + RunnerError::Policy(msg) => write!(f, "hyperlight policy error: {msg}"), + RunnerError::Unavailable(msg) => { + write!(f, "hyperlight is not available on this host: {msg}") + } + RunnerError::Setup(msg) => write!(f, "hyperlight setup error: {msg}"), + RunnerError::Launch(msg) | RunnerError::Exec(msg) | RunnerError::Timeout(msg) => { + write!(f, "hyperlight runtime error: {msg}") + } } } } impl RunnerError { - fn to_response(&self) -> ScriptResponse { - ScriptResponse { - exit_code: ERROR_EXIT_CODE, - error_message: self.to_string(), - ..Default::default() + /// Which side a failure is attributed to: the request, the host, or the + /// run itself. + fn failure_phase(&self) -> FailurePhase { + match self { + RunnerError::Policy(_) => FailurePhase::Rejected, + RunnerError::Unavailable(_) => FailurePhase::BackendUnavailable, + RunnerError::Setup(_) | RunnerError::Launch(_) => FailurePhase::LaunchFailed, + RunnerError::Exec(_) => FailurePhase::PostLaunchFailed, + RunnerError::Timeout(_) => FailurePhase::Timeout, } } -} -const ERROR_EXIT_CODE: i32 = -1; + fn to_response(&self) -> ScriptResponse { + ScriptResponse::failed(self.failure_phase(), &self.to_string()) + } +} /// Env var override for the Hyperlight image home. Set this to force a /// specific location; otherwise the runner uses a standard OS-local @@ -406,7 +430,7 @@ impl HyperlightScriptRunner { default.join(name) ), }; - Err(RunnerError::Preflight(format!( + Err(RunnerError::Unavailable(format!( "no hyperlight image found for the {name} runtime. searched ${HOME_ENV}, \ {default:?}, /{EXE_RELATIVE_HOME}/, /{CWD_RELATIVE_HOME}/, each under \ `{name}/`. {hint}" @@ -444,10 +468,10 @@ impl HyperlightScriptRunner { /// Reject policies the guest cannot enforce before any sandbox is booted. fn validate_policies(request: &ExecutionRequest) -> Result<(), RunnerError> { if request.policy.network_proxy.is_enabled() { - return Err(RunnerError::Preflight(ERR_PROXY_POLICY.to_string())); + return Err(RunnerError::Policy(ERR_PROXY_POLICY.to_string())); } if !request.working_directory.is_empty() { - return Err(RunnerError::Preflight(ERR_WORKDIR.to_string())); + return Err(RunnerError::Policy(ERR_WORKDIR.to_string())); } // Denied paths: block early if any appears in the allow lists. for denied in &request.policy.denied_paths { @@ -459,15 +483,32 @@ impl HyperlightScriptRunner { { match path_relationship(allowed, denied) { PathRelationship::Overlaps => { - return Err(RunnerError::Preflight(format!( + return Err(RunnerError::Policy(format!( "deniedPaths entry {denied:?} overlaps allow list path {allowed:?}; \ Hyperlight cannot safely expose overlapping allowed and denied \ host paths" ))); } PathRelationship::Separate => {} - PathRelationship::Indeterminate => { - return Err(RunnerError::Preflight(format!( + PathRelationship::Unresolvable(reason) => { + return Err(RunnerError::Setup(format!( + "Hyperlight: cannot verify deniedPaths against allow lists because \ + the host could not resolve {allowed:?} or {denied:?} ({reason}). \ + With deniedPaths present, MXC fails closed rather than risk an \ + unenforced deny." + ))); + } + PathRelationship::Inaccessible(reason) => { + return Err(RunnerError::Policy(format!( + "Hyperlight: cannot verify deniedPaths against allow lists because \ + the invoking user cannot access {allowed:?} or {denied:?} \ + ({reason}). Grant access to both paths, or remove the deniedPaths \ + entry." + ))); + } + #[cfg(target_os = "windows")] + PathRelationship::Ambiguous => { + return Err(RunnerError::Policy(format!( "Hyperlight: cannot verify deniedPaths against allow lists because \ {allowed:?} or {denied:?} could not be resolved to a canonical \ location. With deniedPaths present, MXC fails closed rather than \ @@ -511,13 +552,13 @@ impl HyperlightScriptRunner { .map(|p| p.as_os_str().is_empty() || p.exists()) .unwrap_or(false); if !parent_ok { - return Err(RunnerError::Preflight(format!( + return Err(RunnerError::Policy(format!( "mount path {host:?} does not exist and its parent doesn't either; \ refusing to auto-create (fix the path or `mkdir -p` manually)" ))); } std::fs::create_dir_all(&host_path).map_err(|e| { - RunnerError::Preflight(format!("auto-create mount dir {host:?}: {e}")) + RunnerError::Setup(format!("auto-create mount dir {host:?}: {e}")) })?; } @@ -525,12 +566,12 @@ impl HyperlightScriptRunner { // so a relative path keeps meaning the same directory after a // cwd change. let host_path = std::fs::canonicalize(&host_path) - .map_err(|e| RunnerError::Preflight(format!("resolve mount path {host:?}: {e}")))?; + .map_err(|e| RunnerError::Setup(format!("resolve mount path {host:?}: {e}")))?; let basename = host_path .file_name() .and_then(|n| n.to_str()) .ok_or_else(|| { - RunnerError::Preflight(format!("mount path {host:?} has no filename component")) + RunnerError::Policy(format!("mount path {host:?} has no filename component")) })?; // The guest path travels to the kernel in its `vfs.fstab` // list, which these characters would break; the library @@ -539,14 +580,14 @@ impl HyperlightScriptRunner { .chars() .any(|c| c.is_whitespace() || matches!(c, ':' | '[' | ']')) { - return Err(RunnerError::Preflight(format!( + return Err(RunnerError::Policy(format!( "mount path {host:?}: the directory name may not contain whitespace, \ ':' or brackets (it names the guest mount point)" ))); } let guest_path = format!("{GUEST_MOUNT_ROOT}/{basename}"); if !seen_guest_paths.insert(guest_path.clone()) { - return Err(RunnerError::Preflight(format!( + return Err(RunnerError::Policy(format!( "two mount paths collide on guest path {guest_path:?}; \ rename one of the host directories" ))); @@ -616,7 +657,7 @@ impl HyperlightScriptRunner { let builder = SandboxBuilder::from_snapshot(rewind).mounts(mounts.iter().cloned()); let sandbox = builder .boot() - .map_err(|e| RunnerError::Runtime(format!("restore hyperlight snapshot: {e}")))?; + .map_err(|e| RunnerError::Launch(format!("restore hyperlight snapshot: {e}")))?; Ok(Guest { sandbox, fresh: true, @@ -724,11 +765,11 @@ impl HyperlightScriptRunner { // next call boots another from the rewind point. self.guest = None; let err = match failure { - RunError::TimedOut(timeout) => RunnerError::Runtime(format!( + RunError::TimedOut(timeout) => RunnerError::Timeout(format!( "execution timed out after {:.1}s", timeout.as_secs_f64() )), - RunError::Failed(msg) => RunnerError::Runtime(format!("run: {msg}")), + RunError::Failed(msg) => RunnerError::Exec(format!("run: {msg}")), }; logger.log_line(&err.to_string()); err.to_response() @@ -873,7 +914,7 @@ fn warm_snapshot( configure_surrogates(); let mut sandbox = rootfs_builder(home, runtime) .boot() - .map_err(|e| RunnerError::Runtime(format!("boot hyperlight rootfs: {e}")))?; + .map_err(|e| RunnerError::Launch(format!("boot hyperlight rootfs: {e}")))?; // Saved beside the snapshot directory, so a failed save leaves whatever // is there standing, then published by rename. let staged = home.join(format!(".{SNAPSHOT_DIR}.{}.part", std::process::id())); @@ -882,7 +923,7 @@ fn warm_snapshot( Ok(snapshot) => snapshot, Err(e) => { let _ = std::fs::remove_dir_all(&staged); - return Err(RunnerError::Runtime(format!( + return Err(RunnerError::Launch(format!( "save snapshot to {staged:?}: {e}" ))); } @@ -975,7 +1016,7 @@ fn load_persisted_snapshot( Err(e) => e, }; if !has_install_source(home, runtime) { - return Err(RunnerError::Preflight(no_install_source(runtime))); + return Err(RunnerError::Unavailable(no_install_source(runtime))); } logger.log_line(&match unloadable { hyperlight_unikraft::Error::SnapshotRelease { saved_by, .. } => format!( @@ -1280,11 +1321,13 @@ fn stamp_matches(home: &Path, runtime: HyperlightRuntime) -> bool { /// Whether an allowed host path overlaps a denied path after canonicalization, /// with a platform-aware lexical fallback for paths that do not exist yet. fn path_relationship(allowed: &str, denied: &str) -> PathRelationship { - let Ok(allowed) = anchor_policy_path(allowed) else { - return PathRelationship::Indeterminate; - }; - let Ok(denied) = anchor_policy_path(denied) else { - return PathRelationship::Indeterminate; + let (allowed, denied) = match (anchor_policy_path(allowed), anchor_policy_path(denied)) { + (Ok(allowed), Ok(denied)) => (allowed, denied), + (Err(e), _) | (_, Err(e)) => { + return PathRelationship::Unresolvable(format!( + "could not read the current directory: {e}" + )); + } }; match ( std::fs::canonicalize(&allowed), @@ -1293,7 +1336,27 @@ fn path_relationship(allowed: &str, denied: &str) -> PathRelationship { (Ok(allowed), Ok(denied)) => PathRelationship::from_overlaps( denied.starts_with(&allowed) || allowed.starts_with(&denied), ), - _ => path_relationship_fallback(&allowed.to_string_lossy(), &denied.to_string_lossy()), + (allowed_lookup, denied_lookup) => classify_unresolvable( + path_relationship_fallback(&allowed.to_string_lossy(), &denied.to_string_lossy()), + [&allowed_lookup, &denied_lookup], + ), + } +} + +/// A path the invoking user may not access is the caller's to correct; any +/// other failure to resolve it is a host fault. +fn classify_unresolvable( + relationship: PathRelationship, + lookups: [&std::io::Result; 2], +) -> PathRelationship { + let access_denied = lookups + .iter() + .any(|lookup| matches!(lookup, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied)); + match relationship { + PathRelationship::Unresolvable(reason) if access_denied => { + PathRelationship::Inaccessible(reason) + } + other => other, } } @@ -1310,7 +1373,13 @@ fn anchor_policy_path(path: &str) -> Result { enum PathRelationship { Overlaps, Separate, - Indeterminate, + /// The path spelling cannot be compared safely. + #[cfg(target_os = "windows")] + Ambiguous, + /// The host could not resolve a path. + Unresolvable(String), + /// The invoking user may not access a path. + Inaccessible(String), } impl PathRelationship { @@ -1343,16 +1412,19 @@ fn path_relationship_fallback_with( let Some(allowed_norm) = WindowsFallbackPath::parse_canonical(allowed, &allowed_resolved) else { - return PathRelationship::Indeterminate; + return PathRelationship::Ambiguous; }; let Some(denied_norm) = WindowsFallbackPath::parse_canonical(denied, &denied_resolved) else { - return PathRelationship::Indeterminate; + return PathRelationship::Ambiguous; }; return PathRelationship::from_overlaps(allowed_norm.overlaps(&denied_norm)); } - (PathCanonical::Unknown, _) | (_, PathCanonical::Unknown) => { - return PathRelationship::Indeterminate; + (PathCanonical::Unknown, _) => { + return PathRelationship::Unresolvable(format!("{allowed:?} could not be examined")); + } + (_, PathCanonical::Unknown) => { + return PathRelationship::Unresolvable(format!("{denied:?} could not be examined")); } _ => {} } @@ -1470,11 +1542,13 @@ fn has_trim_sensitive_component(path: &str) -> bool { #[cfg(not(target_os = "windows"))] fn path_relationship_fallback(allowed: &str, denied: &str) -> PathRelationship { - let Ok(allowed) = resolve_non_windows_fallback_path(Path::new(allowed)) else { - return PathRelationship::Indeterminate; + let resolve = |path: &str| { + resolve_non_windows_fallback_path(Path::new(path)) + .map_err(|e| PathRelationship::Unresolvable(format!("{path:?}: {e}"))) }; - let Ok(denied) = resolve_non_windows_fallback_path(Path::new(denied)) else { - return PathRelationship::Indeterminate; + let (allowed, denied) = match (resolve(allowed), resolve(denied)) { + (Ok(allowed), Ok(denied)) => (allowed, denied), + (Err(unresolvable), _) | (_, Err(unresolvable)) => return unresolvable, }; PathRelationship::from_overlaps(denied.starts_with(&allowed) || allowed.starts_with(&denied)) } @@ -1851,7 +1925,8 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = r.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert_denied_overlap_message(&resp.error_message); } @@ -1918,6 +1993,67 @@ mod tests { let _ = std::fs::remove_dir_all(&tmp); } + #[cfg(unix)] + #[test] + fn an_inaccessible_host_path_is_a_policy_rejection() { + use std::os::unix::fs::PermissionsExt; + + let tmp = fresh_tmp("deny-unresolvable"); + let locked = tmp.join("locked"); + std::fs::create_dir_all(locked.join("inner")).unwrap(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).unwrap(); + let restore = || { + let _ = std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o755)); + let _ = std::fs::remove_dir_all(&tmp); + }; + // Root ignores directory permissions. + if std::fs::canonicalize(locked.join("inner")).is_ok() { + restore(); + return; + } + + let request = ExecutionRequest { + script_code: "print('x')".to_string(), + policy: ContainerPolicy { + readwrite_paths: vec![locked.join("inner").to_string_lossy().to_string()], + denied_paths: vec![tmp.join("secret").to_string_lossy().to_string()], + ..Default::default() + }, + ..Default::default() + }; + let resp = runner().run(&request, &mut Logger::new(Mode::Buffer)); + restore(); + + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); + assert!( + resp.error_message + .contains("the invoking user cannot access"), + "got: {}", + resp.error_message + ); + } + + #[test] + fn only_permission_denied_makes_an_unresolvable_path_a_policy_fault() { + use std::io::{Error, ErrorKind}; + + let classify = |kind: ErrorKind| { + classify_unresolvable( + PathRelationship::Unresolvable("x".to_string()), + [&Ok(PathBuf::new()), &Err(Error::from(kind))], + ) + }; + assert_eq!( + classify(ErrorKind::PermissionDenied), + PathRelationship::Inaccessible("x".to_string()) + ); + assert_eq!( + classify(ErrorKind::Other), + PathRelationship::Unresolvable("x".to_string()) + ); + } + #[test] fn policy_rejects_denied_absolute_descendant_of_missing_relative_mount() { let relative_mount = format!("hyperlight-relative-{}", std::process::id()); @@ -1955,7 +2091,8 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = r.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert_denied_overlap_message(&resp.error_message); } @@ -1976,7 +2113,8 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = r.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert_denied_overlap_message(&resp.error_message); } @@ -2017,17 +2155,17 @@ mod tests { #[cfg(target_os = "windows")] #[test] - fn fallback_reports_indeterminate_for_unresolved_windows_paths() { + fn fallback_reports_unresolved_windows_paths_as_host_failures() { use crate::mxc_common::filesystem_canonical::PathCanonical; - assert_eq!( + assert!(matches!( path_relationship_fallback_with( "C:\\MXC\\HyperlightUnknown\\allow", "C:\\MXC\\HyperlightUnknown\\deny", |_| PathCanonical::Unknown, ), - PathRelationship::Indeterminate - ); + PathRelationship::Unresolvable(_) + )); } #[cfg(target_os = "windows")] @@ -2071,7 +2209,7 @@ mod tests { assert!( matches!( relationship, - PathRelationship::Overlaps | PathRelationship::Indeterminate + PathRelationship::Overlaps | PathRelationship::Ambiguous ), "same resolved tree must overlap or fail closed, got {relationship:?}" ); @@ -2093,7 +2231,7 @@ mod tests { _ => PathCanonical::Absent, }, ), - PathRelationship::Indeterminate + PathRelationship::Ambiguous ); } @@ -2173,7 +2311,43 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = r.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert!(resp.error_message.contains(ERR_WORKDIR)); } + + #[test] + fn a_host_without_an_image_blames_the_host() { + let resp = + RunnerError::Unavailable(no_install_source(HyperlightRuntime::Agent)).to_response(); + assert_eq!(resp.failure_phase, FailurePhase::BackendUnavailable); + assert_eq!(resp.exit_code, -1); + assert!(resp.error_message.contains("not available on this host")); + } + + #[test] + fn run_faults_keep_the_generic_backend_error_code() { + for (err, phase) in [ + ( + RunnerError::Setup("mount".to_string()), + FailurePhase::LaunchFailed, + ), + ( + RunnerError::Launch("boot".to_string()), + FailurePhase::LaunchFailed, + ), + ( + RunnerError::Exec("run".to_string()), + FailurePhase::PostLaunchFailed, + ), + ( + RunnerError::Timeout("deadline".to_string()), + FailurePhase::Timeout, + ), + ] { + let resp = err.to_response(); + assert_eq!(resp.failure_phase, phase, "{err}"); + assert_eq!(resp.exit_code, -1, "{err}"); + } + } } diff --git a/src/mxc-sdk/src/backends/isolation_session/common/error.rs b/src/mxc-sdk/src/backends/isolation_session/common/error.rs index ad6e3c9bfe..7c3b6b8b6f 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/error.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/error.rs @@ -249,6 +249,8 @@ impl std::fmt::Display for IsolationSessionError { } } +// SDK v2: Policy -> rejected (policy_validation), ServiceUnavailable -> +// unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. impl From for ScriptResponse { fn from(err: IsolationSessionError) -> Self { ScriptResponse::error(&err.to_string()) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs index 7ec4c8b798..c17df26810 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs @@ -37,7 +37,9 @@ use super::owned_thread::{self, Impersonation}; use super::pipe_relay::{ create_relay_thread, create_relay_thread_with_stop, duplicate_handle, PipeRelayWithStopParams, }; -use super::process_options::{build_iso_process_options, ProcessOptions}; +use super::process_options::{ + build_iso_process_options, with_service_timeout_grace, ProcessOptions, +}; // The OS account name is lifecycle addressing data, not diagnostic identity. const AGENT_IDENTITY_MARKER: &str = "isolation-session-agent"; @@ -398,16 +400,22 @@ impl IsolationSessionManager { /// Step 3: Create a process inside the started isolation session and run it /// to completion. Output is streamed live to the calling process's stdio via - /// internal relay threads; only the exit code is returned. + /// internal relay threads; only the exit code or timeout is returned. + /// + /// `options.timeout_ms` is enforced locally; the service-side timer is armed + /// with [`with_service_timeout_grace`] so a timeout is never reported as an + /// ordinary exit. /// /// This is the **relay** path, reached by `wxc-exec` and by an in-process /// caller that asked to attach. A caller that wants the streams handed back /// uses [`Self::piped_exec_handle`] instead. + /// + /// [`with_service_timeout_grace`]: super::process_options::with_service_timeout_grace pub(super) fn create_process( &self, options: &ProcessOptions, logger: Option<&mut Logger>, - ) -> Result { + ) -> Result { owned_thread::call(&self.impersonation, || { // Everything fallible that does not need the workload runs first, so no // failure can strand a running one. @@ -448,7 +456,8 @@ impl IsolationSessionManager { // Lifetime: each relay's param struct is moved to the heap and owned // by its thread, which frees it on exit, so joining is not required // for memory safety. - let started = self.start_process(options, logger.as_deref())?; + let service_options = with_service_timeout_grace(options.clone()); + let started = self.start_process(&service_options, logger.as_deref())?; let process = &started.process; let stdout_handle_val = started.stdout; let stderr_handle_val = started.stderr; @@ -583,10 +592,7 @@ impl IsolationSessionManager { scope.stop_stdin(); scope.finish(); - Ok(match outcome { - ExecOutcome::Exited(exit_code) => exit_code, - ExecOutcome::TimedOut => WAIT_FOR_EXIT_TIMEOUT, - }) + Ok(outcome) }) } @@ -1219,7 +1225,7 @@ const STILL_ACTIVE: i32 = windows::Win32::Foundation::STATUS_PENDING.0; /// What `WaitForExit` returns when the deadline elapses before the process /// exits. Like [`STILL_ACTIVE`] this is a legal exit code in its own right, so /// it is never used alone to conclude a timeout. -const WAIT_FOR_EXIT_TIMEOUT: i32 = -1; +pub(super) const WAIT_FOR_EXIT_TIMEOUT: i32 = -1; /// How long [`StartedProcess::terminate`] waits for a kill to land before /// reporting success anyway. Bounded so a failed `Terminate` cannot wedge that diff --git a/src/mxc-sdk/src/backends/isolation_session/common/one_shot.rs b/src/mxc-sdk/src/backends/isolation_session/common/one_shot.rs index 164fdae5e2..af851627ae 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/one_shot.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/one_shot.rs @@ -9,8 +9,9 @@ use std::fmt::Write; use std::io::IsTerminal; use crate::mxc_common::logger::Logger; -use crate::mxc_common::models::{ExecutionRequest, ScriptResponse}; +use crate::mxc_common::models::{ExecutionRequest, FailurePhase, ScriptResponse}; use crate::mxc_common::script_runner::ScriptRunner; +use crate::mxc_common::state_aware_backend::ExecOutcome; use crate::mxc_common::validator::{validate_network_policy_support, NetworkPolicySupport}; use super::manager::{log_sandbox_torn_down, IsolationSessionManager, TeardownOutcome}; @@ -34,12 +35,14 @@ use super::IsolationSessionRunner; /// anything applied — so there is nothing to retain. fn reject_unsupported_lifecycle(request: &ExecutionRequest) -> Result<(), ScriptResponse> { if !request.lifecycle.destroy_on_exit { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "lifecycle.destroyOnExit=false is not supported by the isolation session backend; \ the session is always stopped and the agent user removed", )); } if request.lifecycle.preserve_policy { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "lifecycle.preservePolicy=true is not supported by the isolation session backend; \ it installs no persistent filesystem or network enforcement, so there is none \ @@ -120,8 +123,8 @@ impl ScriptRunner for IsolationSessionRunner { return e.into(); } - let exit_code = match manager.create_process(&options, Some(logger)) { - Ok(code) => code, + let exec_outcome = match manager.create_process(&options, Some(logger)) { + Ok(exec_outcome) => exec_outcome, Err(e) => { let stopped = manager.stop_session().is_ok(); let deprovisioned = manager.deprovision_agent_user().is_ok(); @@ -157,13 +160,27 @@ impl ScriptRunner for IsolationSessionRunner { // Output already streamed live to wxc-exec's stdio via relay // threads in `create_process` — captured fields intentionally // empty (same pattern as AppContainer). - ScriptResponse { + completed_response(exec_outcome, options.timeout_ms) + } +} + +/// The response for a completed run, so a workload that failed or timed out is +/// told apart from the sandbox failing around it. +fn completed_response(outcome: ExecOutcome, timeout_ms: u32) -> ScriptResponse { + match outcome { + ExecOutcome::Exited(0) => ScriptResponse { + exit_code: 0, + ..Default::default() + }, + ExecOutcome::Exited(exit_code) => ScriptResponse { exit_code, - standard_out: String::new(), - standard_err: String::new(), - error_message: String::new(), + failure_phase: FailurePhase::ProcessExited, ..Default::default() - } + }, + ExecOutcome::TimedOut => ScriptResponse::failed( + FailurePhase::Timeout, + &format!("script timed out after {timeout_ms}ms"), + ), } } @@ -174,6 +191,22 @@ mod tests { ContainerPolicy, LifecycleConfig, NetworkAction, NetworkEgressPolicy, NetworkIngressPolicy, }; + #[test] + fn completed_runs_distinguish_exit_from_timeout() { + let ok = completed_response(ExecOutcome::Exited(0), 5_000); + assert_eq!((ok.exit_code, ok.failure_phase), (0, FailurePhase::None)); + + let exited = completed_response(ExecOutcome::Exited(-1), 5_000); + assert_eq!( + (exited.exit_code, exited.failure_phase), + (-1, FailurePhase::ProcessExited) + ); + + let timed_out = completed_response(ExecOutcome::TimedOut, 5_000); + assert_eq!(timed_out.failure_phase, FailurePhase::Timeout); + assert_eq!(timed_out.error_message, "script timed out after 5000ms"); + } + #[test] fn validate_runner_one_shot_rejects_default_network() { // No user, but the default (absent → `Block`) network is not the @@ -249,6 +282,28 @@ mod tests { ); } + #[test] + fn validate_runner_refuses_an_unsupported_one_shot_lifecycle() { + let runner = IsolationSessionRunner::new(); + for lifecycle in [ + LifecycleConfig { + destroy_on_exit: false, + preserve_policy: false, + }, + LifecycleConfig { + destroy_on_exit: true, + preserve_policy: true, + }, + ] { + let req = ExecutionRequest { + lifecycle, + ..canonical_request() + }; + let resp = runner.validate_runner(&req).unwrap_err(); + assert_eq!(resp.exit_code, -1); + } + } + #[test] fn validate_runner_one_shot_accepts_default_lifecycle() { // `destroyOnExit: true` (the default) is exactly what the backend does, diff --git a/src/mxc-sdk/src/backends/isolation_session/common/process_options.rs b/src/mxc-sdk/src/backends/isolation_session/common/process_options.rs index d40558ca99..e0968cc39f 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/process_options.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/process_options.rs @@ -135,10 +135,8 @@ pub(super) const SERVICE_TIMEOUT_GRACE_MS: u32 = 5_000; /// Relaxes the service-side timer so the caller's deadline is enforced locally. /// -/// Only for the streaming path, which has somewhere to report a timeout. The -/// run-to-completion path keeps the service timer exactly as it was: it has no -/// timeout channel, and its observable behaviour (exit code 1 on an OS-side -/// timeout) is pinned by the host suite. +/// Without the margin the two deadlines coincide, and the service's kill (an +/// ordinary exit code 1) could win the race and hide the timeout. /// /// Two deadlines are left alone rather than shifted: /// diff --git a/src/mxc-sdk/src/backends/isolation_session/common/sandbox.rs b/src/mxc-sdk/src/backends/isolation_session/common/sandbox.rs index 31166252a2..c958e1d793 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/sandbox.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/sandbox.rs @@ -75,23 +75,26 @@ pub fn spawn_one_shot( /// workload exits, and whose relay scope borrows the process — into a /// non-blocking start with detached relays. fn inherit_not_served() -> ScriptResponse { - ScriptResponse { - failure_phase: FailurePhase::Rejected, - ..ScriptResponse::error( - "the isolation session backend does not yet support inherited stdio; it serves piped \ - stdio, which is what an in-process caller receives. Run it through wxc-exec to have \ - the workload relayed onto this process's own stdio.", - ) - } + ScriptResponse::rejected( + "the isolation session backend does not yet support inherited stdio; it serves piped \ + stdio, which is what an in-process caller receives. Run it through wxc-exec to have \ + the workload relayed onto this process's own stdio.", + ) +} + +/// Flattens a launch failure onto a [`ScriptResponse`]. +// SDK v2: BackendUnavailable -> unavailable, PolicyValidation -> rejected. +// See docs/development/plans/sdk-v2-error-codes.md. +fn launch_failure_response(error: &MxcError) -> ScriptResponse { + ScriptResponse::failed(FailurePhase::LaunchFailed, &error.message) } impl SandboxBackend for IsolationSessionRunner { fn validate(&self, request: &ExecutionRequest) -> Result<(), ScriptResponse> { // One-shot runs the whole lifecycle, so provision-phase rules apply to // the whole call — the same check the run-to-completion path makes. - // - // Everything it refuses is a caller-fixable request problem, and the - // phase is what the dispatcher reads back to classify it. + // SDK v2: keep the phase from validate_runner so an unavailable service + // reports backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. ScriptRunner::validate_runner(self, request).map_err(|resp| ScriptResponse { failure_phase: FailurePhase::Rejected, ..resp @@ -114,19 +117,13 @@ impl SandboxBackend for IsolationSessionRunner { StdioMode::Pty(size) => { spawn_one_shot_pty(request, logger, size).map_err(|error| match error { OneShotSpawnFailure::Refused(response) => response, - OneShotSpawnFailure::Launch(error) => ScriptResponse { - failure_phase: FailurePhase::LaunchFailed, - ..ScriptResponse::error(&error.message) - }, + OneShotSpawnFailure::Launch(error) => launch_failure_response(&error), }) } // Delegated so both entry points share one validation prologue. StdioMode::Pipes => spawn_one_shot(request, logger).map_err(|e| match e { OneShotSpawnFailure::Refused(resp) => resp, - OneShotSpawnFailure::Launch(err) => ScriptResponse { - failure_phase: FailurePhase::LaunchFailed, - ..ScriptResponse::error(&err.message) - }, + OneShotSpawnFailure::Launch(err) => launch_failure_response(&err), }), } } @@ -837,4 +834,11 @@ mod tests { let after = with_cleanup_failures(launch, &[]).to_string(); assert_eq!(before, after); } + + #[test] + fn inherited_stdio_is_refused_as_a_request_problem() { + let refused = inherit_not_served(); + assert_eq!(refused.failure_phase, FailurePhase::Rejected); + assert_eq!(refused.exit_code, 1); + } } diff --git a/src/mxc-sdk/src/backends/isolation_session/common/state_aware.rs b/src/mxc-sdk/src/backends/isolation_session/common/state_aware.rs index b31c8a9b86..a80f6c8126 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/state_aware.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/state_aware.rs @@ -25,7 +25,9 @@ use crate::mxc_common::validator::{ use windows::Win32::Foundation::HANDLE; use super::error::map_lifecycle_error; -use super::manager::{log_sandbox_torn_down, IsolationSessionManager, TeardownOutcome}; +use super::manager::{ + log_sandbox_torn_down, IsolationSessionManager, TeardownOutcome, WAIT_FOR_EXIT_TIMEOUT, +}; use super::policy::{ reject_unhonorable_environment, validate_post_provision_policy, validate_provision_policy, }; @@ -316,9 +318,10 @@ impl StatefulSandboxBackend for IsolationSessionRunner { ); let mut logger = Logger::inherit_thread_diagnostic_sink(); - let exit_code = manager + let outcome = manager .create_process(&options, Some(&mut logger)) .map_err(map_lifecycle_error)?; + let exit_code = relayed_exit_code(outcome); // The output relay completed inside `create_process`. The // dispatcher sees zero pipe handles, skips its own relay setup, @@ -367,6 +370,16 @@ impl StatefulSandboxBackend for IsolationSessionRunner { } } +// SDK v2: a timeout returns backend_error "script timed out after {timeout_ms}ms" +// instead of an exit code a workload could also return. +// See docs/development/plans/sdk-v2-error-codes.md. +fn relayed_exit_code(outcome: ExecOutcome) -> i32 { + match outcome { + ExecOutcome::Exited(exit_code) => exit_code, + ExecOutcome::TimedOut => WAIT_FOR_EXIT_TIMEOUT, + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/mxc-sdk/src/backends/lxc/common/availability.rs b/src/mxc-sdk/src/backends/lxc/common/availability.rs index fbca924c7a..8d69b787bf 100644 --- a/src/mxc-sdk/src/backends/lxc/common/availability.rs +++ b/src/mxc-sdk/src/backends/lxc/common/availability.rs @@ -1,7 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +use std::io::ErrorKind; use std::process::{Child, Command, Stdio}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::OnceLock; use std::time::{Duration, Instant}; @@ -14,16 +16,43 @@ const POLL_INTERVAL: Duration = Duration::from_millis(25); enum LxcLsOutcome { ExitedSuccess, ExitedFailure, + NotFound, SpawnFailed, TimedOut, } +/// What execution should conclude from probing the LXC tooling. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LxcTooling { + Available, + /// `lxc-ls` is absent or reports itself broken. + Unavailable, + /// The probe itself failed (timeout, spawn error); worth retrying. + ProbeFailed, +} + /// Runs `lxc-ls --version`; only a clean exit counts as available. pub fn is_lxc_available() -> bool { static AVAILABLE: OnceLock = OnceLock::new(); *AVAILABLE.get_or_init(|| available_from(probe_lxc_ls())) } +/// The tooling verdict for execution, separate from the discovery cache. +/// +/// Only success is cached: a host that gains the LXC tools later, or whose +/// probe was transiently slow, is re-probed on the next run. +pub fn lxc_tooling_for_execution() -> LxcTooling { + static CONFIRMED: AtomicBool = AtomicBool::new(false); + if CONFIRMED.load(Ordering::Relaxed) { + return LxcTooling::Available; + } + let tooling = tooling_from(probe_lxc_ls()); + if tooling == LxcTooling::Available { + CONFIRMED.store(true, Ordering::Relaxed); + } + tooling +} + fn probe_lxc_ls() -> LxcLsOutcome { let child = Command::new("lxc-ls") .arg("--version") @@ -33,6 +62,7 @@ fn probe_lxc_ls() -> LxcLsOutcome { .spawn(); match child { Ok(mut child) => wait_bounded(&mut child, PROBE_TIMEOUT), + Err(error) if error.kind() == ErrorKind::NotFound => LxcLsOutcome::NotFound, Err(_) => LxcLsOutcome::SpawnFailed, } } @@ -60,6 +90,14 @@ fn available_from(outcome: LxcLsOutcome) -> bool { matches!(outcome, LxcLsOutcome::ExitedSuccess) } +fn tooling_from(outcome: LxcLsOutcome) -> LxcTooling { + match outcome { + LxcLsOutcome::ExitedSuccess => LxcTooling::Available, + LxcLsOutcome::NotFound | LxcLsOutcome::ExitedFailure => LxcTooling::Unavailable, + LxcLsOutcome::SpawnFailed | LxcLsOutcome::TimedOut => LxcTooling::ProbeFailed, + } +} + #[cfg(test)] mod tests { use super::*; @@ -68,7 +106,32 @@ mod tests { fn only_a_clean_exit_means_available() { assert!(available_from(LxcLsOutcome::ExitedSuccess)); assert!(!available_from(LxcLsOutcome::ExitedFailure)); + assert!(!available_from(LxcLsOutcome::NotFound)); assert!(!available_from(LxcLsOutcome::SpawnFailed)); assert!(!available_from(LxcLsOutcome::TimedOut)); } + + #[test] + fn a_missing_tool_is_unavailable_but_a_failed_probe_is_not() { + assert_eq!( + tooling_from(LxcLsOutcome::ExitedSuccess), + LxcTooling::Available + ); + assert_eq!( + tooling_from(LxcLsOutcome::NotFound), + LxcTooling::Unavailable + ); + assert_eq!( + tooling_from(LxcLsOutcome::ExitedFailure), + LxcTooling::Unavailable + ); + assert_eq!( + tooling_from(LxcLsOutcome::TimedOut), + LxcTooling::ProbeFailed + ); + assert_eq!( + tooling_from(LxcLsOutcome::SpawnFailed), + LxcTooling::ProbeFailed + ); + } } diff --git a/src/mxc-sdk/src/backends/lxc/common/lxc_runner.rs b/src/mxc-sdk/src/backends/lxc/common/lxc_runner.rs index e587e9cecd..00061b7a88 100644 --- a/src/mxc-sdk/src/backends/lxc/common/lxc_runner.rs +++ b/src/mxc-sdk/src/backends/lxc/common/lxc_runner.rs @@ -11,7 +11,8 @@ use std::time::{Duration, Instant}; use crate::mxc_common::host_is_canonical_loopback; use crate::mxc_common::logger::Logger; use crate::mxc_common::models::{ - ContainerPolicy, ExecutionRequest, LifecycleConfig, LxcConfig, NetworkAction, ScriptResponse, + ContainerPolicy, ExecutionRequest, FailurePhase, LifecycleConfig, LxcConfig, NetworkAction, + ScriptResponse, }; use crate::mxc_common::proxy_env::{apply_proxy_env, proxy_url_has_credentials, redact_proxy_url}; use crate::mxc_common::sandbox_process::{SandboxBackend, SandboxProcess, StdioMode}; @@ -31,6 +32,7 @@ use crate::mxc_common::sandbox_process::{ #[cfg(target_os = "linux")] use crate::mxc_pty::{LivePty, PtySize as UnixPtySize}; +use crate::lxc_common::availability::LxcTooling; use crate::lxc_common::filesystem_mounts; use crate::lxc_common::loopback_relay::LoopbackRelay; use crate::lxc_common::lxc_bindings::{ContainerFirewall, LxcContainer, StartNetwork}; @@ -207,13 +209,16 @@ impl LxcScriptRunner { return self.claim_generated_name(); } ContainerNameClaim::acquire(&self.container_id).ok_or_else(|| { - ScriptResponse::error(&format!( - "LXC: container '{}' is already in use by a run in this process. LXC reads a \ - run's network section only when the container starts, so a second run would \ - stop the first one's workload to restart it under its own policy. Let the \ - first run finish, or give this one its own containerId.", - self.container_id - )) + ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!( + "LXC: container '{}' is already in use by a run in this process. LXC reads a \ + run's network section only when the container starts, so a second run would \ + stop the first one's workload to restart it under its own policy. Let the \ + first run finish, or give this one its own containerId.", + self.container_id + ), + ) }) } @@ -225,7 +230,8 @@ impl LxcScriptRunner { return Ok(claim); } } - Err(ScriptResponse::error( + Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, "LXC: could not find a free container name for this run; every generated name was \ already in use by another run in this process. Retry, or set containerId to a name \ of your own.", @@ -398,13 +404,16 @@ impl LxcScriptRunner { let release = self.release_kind(container_created); Self::report_release_failure(release, release_container(release), logger); - Some(ScriptResponse::error(&format!( - "Container did not receive an IPv4 address within {:.0}s; \ - check that lxc-net/dnsmasq is running and able to assign one. \ - A bridge offering only IPv6 leaves the container unable to reach \ - the IPv4 destinations its policy names.", - timeout.as_secs_f64() - ))) + Some(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &format!( + "Container did not receive an IPv4 address within {:.0}s; \ + check that lxc-net/dnsmasq is running and able to assign one. \ + A bridge offering only IPv6 leaves the container unable to reach \ + the IPv4 destinations its policy names.", + timeout.as_secs_f64() + ), + )) } /// Everything that must succeed before a workload can launch; a failure @@ -428,13 +437,13 @@ impl LxcScriptRunner { &normalized } Ok(None) => request, - Err(msg) => return Err(ScriptResponse::error(&msg)), + Err(error) => return Err(error.into_response()), }; - if let Err(msg) = crate::mxc_common::filesystem_access::check_delegation(&request.policy) { - return Err(ScriptResponse::error(&msg)); - } + crate::mxc_common::filesystem_access::check_delegation(&request.policy) + .map_err(|error| error.into_response())?; if self.config.distribution.is_empty() || self.config.release.is_empty() { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "LXC distribution and release are required \ (e.g., \"distribution\": \"alpine\", \"release\": \"3.23\")", @@ -447,9 +456,25 @@ impl LxcScriptRunner { // The policy lowers to the same rules with or without a container, so // one that cannot be programmed is refused before a container exists. if let Err(msg) = NetworkIptablesManager::validate_egress_lowering(&request.policy) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&msg)); } + match crate::lxc_common::availability::lxc_tooling_for_execution() { + LxcTooling::Available => {} + LxcTooling::Unavailable => { + // SDK v2: unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. + return Err(ScriptResponse::error(LXC_TOOLING_UNAVAILABLE)); + } + LxcTooling::ProbeFailed => { + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + "LXC: probing the LXC tooling (`lxc-ls --version`) timed out or \ + could not run; retry once the host is responsive", + )); + } + } + if self.destroy_on_exit { signal_cleanup::set_active(&container_name); } @@ -476,10 +501,10 @@ impl LxcScriptRunner { if !container.is_defined() { let _ = writeln!(logger, "Creating LXC container..."); if let Err(e) = container.create(&self.config.distribution, &self.config.release) { - return Err(ScriptResponse::error(&format!( - "Failed to create container: {}", - e - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!("Failed to create container: {}", e), + )); } let _ = writeln!(logger, "Container created successfully."); container_created = true; @@ -493,10 +518,10 @@ impl LxcScriptRunner { if self.destroy_on_exit || container_created { let _ = container.destroy(); } - return Err(ScriptResponse::error(&format!( - "Failed to configure filesystem: {}", - e - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!("Failed to configure filesystem: {}", e), + )); } // LXC reads the network section only at start. A container an earlier @@ -510,11 +535,14 @@ impl LxcScriptRunner { if self.destroy_on_exit || container_created { let _ = container.destroy(); } - return Err(ScriptResponse::error(&format!( - "Failed to stop a container left running by an earlier run: {}. \ - Its network policy is the earlier run's, so the script was not run.", - e - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!( + "Failed to stop a container left running by an earlier run: {}. \ + Its network policy is the earlier run's, so the script was not run.", + e + ), + )); } } @@ -535,10 +563,10 @@ impl LxcScriptRunner { if self.destroy_on_exit || container_created { let _ = container.destroy(); } - return Err(ScriptResponse::error(&format!( - "Failed to start container: {}", - e - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &format!("Failed to start container: {}", e), + )); } let _ = writeln!(logger, "Container started successfully."); @@ -567,7 +595,8 @@ impl LxcScriptRunner { let netns_pid = container.init_pid(); let Some(hook_point) = egress_hook_point(netns_pid, plan.installs_firewall()) else { self.release_after_failure(&container, container_created, logger); - return Err(ScriptResponse::error( + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, "Failed to discover the container init PID; cannot enter the container \ network namespace to install its firewall. Aborting rather than \ running without it.", @@ -593,7 +622,7 @@ impl LxcScriptRunner { Ok(managers) => managers, Err(e) => { self.release_after_failure(&container, container_created, logger); - return Err(ScriptResponse::error(&e)); + return Err(ScriptResponse::failed(FailurePhase::PostLaunchFailed, &e)); } }; @@ -613,7 +642,7 @@ impl LxcScriptRunner { if is_firewall_installed == ContainerFirewall::Installed { if let Err(e) = fw_manager.allow_ping_sockets(logger) { self.release_after_failure(&container, container_created, logger); - return Err(ScriptResponse::error(&e)); + return Err(ScriptResponse::failed(FailurePhase::PostLaunchFailed, &e)); } } @@ -632,12 +661,15 @@ impl LxcScriptRunner { }); if let Err(reason) = stale_pin_error { self.release_after_failure(&container, container_created, logger); - return Err(ScriptResponse::error(&format!( - "Failed to clear a stale network proxy pin from the container's \ - /etc/hosts: {}. The script was not run, because it could have resolved \ - the pinned hostname to an address this policy did not authorize.", - reason - ))); + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &format!( + "Failed to clear a stale network proxy pin from the container's \ + /etc/hosts: {}. The script was not run, because it could have resolved \ + the pinned hostname to an address this policy did not authorize.", + reason + ), + )); } // `script_timeout == 0` means "no timeout" per the SDK contract. @@ -659,11 +691,14 @@ impl LxcScriptRunner { Ok(relay) => Some(relay), Err(e) => { self.release_after_failure(&container, container_created, logger); - return Err(ScriptResponse::error(&format!( - "LXC: cannot relay runtimeConfig.networkProxy into the container: \ + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &format!( + "LXC: cannot relay runtimeConfig.networkProxy into the container: \ {}. The script was not run.", - e - ))); + e + ), + )); } } } @@ -975,6 +1010,11 @@ pub const LXC_INHERIT_STDIO_UNSUPPORTED: &str = pub const LXC_STREAMING_LINUX_ONLY: &str = "LXC: sandboxes can only be launched on Linux."; +pub const LXC_TOOLING_UNAVAILABLE: &str = + "LXC: the LXC runtime is not available on this host — `lxc-ls --version` did not run. \ + Install the LXC tools (e.g. apt install lxc) and make sure they are on PATH, or select \ + a backend this host can provide."; + fn unsupported_network_proxy(policy: &ContainerPolicy) -> Option<&'static str> { if policy.network_proxy.is_enabled() && !policy.runtime_network_proxy_specified { Some("network.proxy") @@ -995,6 +1035,7 @@ impl LxcScriptRunner { fn validate_runtime_proxy(&self, policy: &ContainerPolicy) -> Result<(), ScriptResponse> { if let Some(address) = &policy.network_proxy.address { if !host_is_canonical_loopback(address.host()) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&format!( "LXC: runtimeConfig.networkProxy host '{}' must be localhost, 127.0.0.1, or \ ::1. The proxy is relayed into the container at the same loopback address \ @@ -1004,6 +1045,7 @@ impl LxcScriptRunner { } let url = address.to_url(); if proxy_url_has_credentials(&url) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&format!( "LXC: runtimeConfig.networkProxy must not carry credentials ('{}'). LXC \ passes the proxy URL to lxc-attach as a --set-var command-line argument, \ @@ -1017,6 +1059,7 @@ impl LxcScriptRunner { || !egress.allow.is_empty() || !egress.deny.is_empty() }) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "LXC: runtimeConfig.networkProxy cannot be combined with network.egress rules or \ default='allow'. The relayed proxy port is the container's only outbound path. \ @@ -1033,6 +1076,7 @@ impl ScriptRunner for LxcScriptRunner { self.validate_runtime_proxy(&request.policy)?; } if let Some(field) = unsupported_network_proxy(&request.policy) { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(&format!( "LXC: {field} is not supported and cannot be enforced; \ remove the proxy request." @@ -1092,6 +1136,7 @@ impl LxcScriptRunner { ) -> Result, ScriptResponse> { match stdio { StdioMode::Inherit => { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error(LXC_INHERIT_STDIO_UNSUPPORTED)); } StdioMode::Pty(_) | StdioMode::Pipes => {} @@ -1150,10 +1195,10 @@ impl LxcScriptRunner { Ok(launched) => launched, Err(e) => { let cleanup = self.abandon_prepared(&mut prepared, logger); - return Err(ScriptResponse::error(&launch_failure_message( - &format!("Execution failed: {}", e), - &cleanup, - ))); + return Err(ScriptResponse::failed( + FailurePhase::LaunchFailed, + &launch_failure_message(&format!("Execution failed: {}", e), &cleanup), + )); } }; @@ -1180,13 +1225,16 @@ impl LxcScriptRunner { let cleanup = self.abandon_prepared(&mut prepared, logger); let error = out_result.err().or(err_result.err()); - return Err(ScriptResponse::error(&launch_failure_message( - &format!( - "Failed to wrap the lxc-attach stdio pipes: {}", - error.map_or_else(|| "unknown error".to_string(), |e| e.to_string()) + return Err(ScriptResponse::failed( + FailurePhase::PostLaunchFailed, + &launch_failure_message( + &format!( + "Failed to wrap the lxc-attach stdio pipes: {}", + error.map_or_else(|| "unknown error".to_string(), |e| e.to_string()) + ), + &cleanup, ), - &cleanup, - ))); + )); } }; @@ -1216,6 +1264,7 @@ impl LxcScriptRunner { _logger: &mut Logger, _stdio: StdioMode, ) -> Result, ScriptResponse> { + // SDK v2: unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. Err(ScriptResponse::error(LXC_STREAMING_LINUX_ONLY)) } } @@ -2976,9 +3025,9 @@ mod tests { "the fail-closed readiness timeout message changed unexpectedly: {}", response.error_message ); - assert_eq!( - response.standard_err, response.error_message, - "error responses should carry the message in stderr as well" + assert!( + response.standard_err.is_empty(), + "the message belongs in the error envelope, not repeated in stderr" ); } @@ -3179,6 +3228,94 @@ mod tests { ); } + // SDK v2: refusals report policy_validation. See docs/development/plans/sdk-v2-error-codes.md. + #[test] + fn a_policy_this_backend_cannot_enforce_reports_backend_error() { + use crate::mxc_common::mxc_error::MxcErrorCode; + + let mut runtime_proxy = runtime_proxy_request("10.0.0.1"); + runtime_proxy.script_code = "echo hello".to_string(); + + for (what, runner, request) in [ + ( + "runtimeConfig.networkProxy", + runner_for_guard_tests("refusal-phase"), + runtime_proxy, + ), + ( + "network.proxy", + runner_for_guard_tests("refusal-phase"), + request_with_proxy_url("http://proxy.example.com:8080"), + ), + ( + "unlowerable network.egress", + runner_for_guard_tests("refusal-phase"), + request_with_unlowerable_egress(), + ), + ( + "no distribution or release", + validating_runner(), + ExecutionRequest::default(), + ), + ] { + let refusal = runner + .prepare(&request, &mut Logger::new(Mode::Buffer)) + .err() + .unwrap_or_else(|| panic!("input={what}; expected a refusal")); + + assert_eq!( + refusal.failure_phase.error_code(), + MxcErrorCode::BackendError, + "input={what}; output={refusal:?}" + ); + } + } + /// A container another live sandbox holds is contention, not a refusal: the + /// same request succeeds once the first run finishes. + #[test] + fn a_container_another_run_holds_is_a_launch_fault() { + let runner = runner_named("mxc-claim-phase-test"); + let _held = runner + .claim_container_name() + .expect("the first sandbox takes a container no other one is using"); + + let refusal = runner + .claim_container_name() + .expect_err("a second sandbox on a live container must be refused"); + + assert_eq!(refusal.failure_phase, FailurePhase::LaunchFailed); + assert_eq!(refusal.exit_code, -1); + } + + #[test] + fn a_container_that_never_got_an_address_is_a_post_launch_fault() { + let runner = runner_for_network_readiness_tests(false); + let response = fail_network_readiness(&runner, false).response; + + assert_eq!( + response.failure_phase, + FailurePhase::PostLaunchFailed, + "the container started; it was the sandbox network that never came up" + ); + } + + /// Only reachable off Linux, where the backend does not exist at all. + #[cfg(not(target_os = "linux"))] + #[test] + fn a_host_that_cannot_run_lxc_is_blamed_instead_of_the_request() { + let refusal = validating_runner() + .launch_streaming( + &streamable_request(), + &mut Logger::new(Mode::Buffer), + StdioMode::Pipes, + ) + .err() + .expect("LXC sandboxes only launch on Linux"); + + assert_eq!(refusal.exit_code, -1); + assert_eq!(refusal.error_message, LXC_STREAMING_LINUX_ONLY); + } + #[test] fn both_runner_personalities_declare_the_same_network_support() { let runner = validating_runner(); diff --git a/src/mxc-sdk/src/backends/nanvix/runner/mod.rs b/src/mxc-sdk/src/backends/nanvix/runner/mod.rs index aa1dc3bfc4..950f06aaf9 100644 --- a/src/mxc-sdk/src/backends/nanvix/runner/mod.rs +++ b/src/mxc-sdk/src/backends/nanvix/runner/mod.rs @@ -56,7 +56,7 @@ use std::thread::JoinHandle; use std::time::{Duration, Instant}; use crate::mxc_common::logger::Logger; -use crate::mxc_common::models::{ExecutionRequest, NetworkAction, ScriptResponse}; +use crate::mxc_common::models::{ExecutionRequest, FailurePhase, NetworkAction, ScriptResponse}; use crate::mxc_common::script_runner::ScriptRunner; use crate::mxc_common::validator::{validate_network_policy_support, NetworkPolicySupport}; @@ -132,8 +132,13 @@ fn exit_code_from_status(status: &std::process::ExitStatus) -> i32 { /// Classifies NanVix runner errors for structured error handling. #[derive(Debug)] enum NanVixError { - /// Pre-spawn validation failures (missing artifacts, invalid config, unsupported policy). - Preflight(String), + /// The request asks for something the NanVix guest will not enforce. + Policy(String), + /// A NanVix runtime artifact the host must supply is missing. + Unavailable(String), + /// Host-side preparation failed before the micro-VM was spawned + /// (snapshot generation, staging, locating the installation). + Setup(String), /// OS/platform failures while spawning/managing the NanVix process (WHP/spawn/handles). Platform(String), /// Stdin broken pipe, VM crash. @@ -148,7 +153,11 @@ enum NanVixError { impl std::fmt::Display for NanVixError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - NanVixError::Preflight(msg) => write!(f, "NanVix preflight error: {}", msg), + NanVixError::Policy(msg) => write!(f, "NanVix policy error: {}", msg), + NanVixError::Unavailable(msg) => { + write!(f, "NanVix is not available on this host: {}", msg) + } + NanVixError::Setup(msg) => write!(f, "NanVix setup error: {}", msg), NanVixError::Platform(msg) => write!(f, "NanVix platform error: {}", msg), NanVixError::Runtime(msg) => write!(f, "NanVix runtime error: {}", msg), NanVixError::Timeout { @@ -165,13 +174,21 @@ impl std::fmt::Display for NanVixError { } impl NanVixError { - fn to_response(&self) -> ScriptResponse { - ScriptResponse { - exit_code: ERROR_EXIT_CODE, - error_message: self.to_string(), - ..Default::default() + /// Which side a failure is attributed to: the request, the host, or the + /// run itself. + fn failure_phase(&self) -> FailurePhase { + match self { + NanVixError::Policy(_) => FailurePhase::Rejected, + NanVixError::Unavailable(_) => FailurePhase::BackendUnavailable, + NanVixError::Setup(_) | NanVixError::Platform(_) => FailurePhase::LaunchFailed, + NanVixError::Runtime(_) => FailurePhase::PostLaunchFailed, + NanVixError::Timeout { .. } => FailurePhase::Timeout, } } + + fn to_response(&self) -> ScriptResponse { + ScriptResponse::failed(self.failure_phase(), &self.to_string()) + } } /// Returns the directory containing the current executable. @@ -180,11 +197,11 @@ impl NanVixError { /// `process_util` is gated to `target_os = "windows"`. fn exe_dir() -> Result { std::env::current_exe() - .map_err(|e| NanVixError::Preflight(format!("cannot determine exe path: {}", e))) + .map_err(|e| NanVixError::Setup(format!("cannot determine exe path: {}", e))) .and_then(|exe| { exe.parent() .map(|p| p.to_path_buf()) - .ok_or_else(|| NanVixError::Preflight("exe has no parent directory".to_string())) + .ok_or_else(|| NanVixError::Setup("exe has no parent directory".to_string())) }) } @@ -321,7 +338,7 @@ impl NanVixScriptRunner { let nanvixd = dir.join(NANVIXD_BINARY); if !nanvixd.exists() { - return Err(NanVixError::Preflight(format!( + return Err(NanVixError::Unavailable(format!( "{} not found in {:?}", NANVIXD_BINARY, dir ))); @@ -329,7 +346,7 @@ impl NanVixScriptRunner { let ramfs = dir.join(RAMFS_IMAGE); if !ramfs.exists() { - return Err(NanVixError::Preflight(format!( + return Err(NanVixError::Unavailable(format!( "{} not found in {:?}", RAMFS_IMAGE, dir ))); @@ -337,7 +354,7 @@ impl NanVixScriptRunner { let initrd = dir.join(INITRD_BINARY); if !initrd.exists() { - return Err(NanVixError::Preflight(format!( + return Err(NanVixError::Unavailable(format!( "{} not found in {:?}", INITRD_BINARY, dir ))); @@ -350,7 +367,7 @@ impl NanVixScriptRunner { for name in crate::nanvix_common::BIN_SUBDIR_FILES { let path = bin_subdir.join(name); if !path.exists() { - return Err(NanVixError::Preflight(format!( + return Err(NanVixError::Unavailable(format!( "{}/{} not found in {:?}", BIN_DIR, name, dir ))); @@ -405,7 +422,7 @@ impl NanVixScriptRunner { let p = PathBuf::from(val); if !p.as_os_str().is_empty() { std::fs::create_dir_all(&p).map_err(|e| { - NanVixError::Preflight(format!( + NanVixError::Setup(format!( "cannot create ${} directory {:?}: {}", NANVIX_HOME_ENV, p, e )) @@ -457,9 +474,8 @@ impl NanVixScriptRunner { ramfs: &Path, initrd: &Path, ) -> Result<(), NanVixError> { - std::fs::create_dir_all(snapshot_home).map_err(|e| { - NanVixError::Preflight(format!("failed to create snapshot home: {}", e)) - })?; + std::fs::create_dir_all(snapshot_home) + .map_err(|e| NanVixError::Setup(format!("failed to create snapshot home: {}", e)))?; eprintln!("nanvix: no snapshot found — generating via cold boot (one-time cost)..."); @@ -471,7 +487,7 @@ impl NanVixScriptRunner { ramfs, initrd, ) - .map_err(NanVixError::Preflight)?; + .map_err(NanVixError::Setup)?; eprintln!( "nanvix: snapshot generated in {:.0?} — subsequent runs will use warm start", @@ -501,7 +517,7 @@ impl NanVixScriptRunner { .as_ref() .is_some_and(|egress| !egress.allow.is_empty() || !egress.deny.is_empty()) { - return Err(NanVixError::Preflight(ERR_DIRECTIONAL_FILTERS.to_string())); + return Err(NanVixError::Policy(ERR_DIRECTIONAL_FILTERS.to_string())); } let egress = policy .network_egress @@ -519,7 +535,7 @@ impl NanVixScriptRunner { .map(|ingress| ingress.host_loopback) .unwrap_or(NetworkAction::Deny); if egress != ingress || ingress != host_loopback { - return Err(NanVixError::Preflight(ERR_DIRECTIONAL_NETWORK.to_string())); + return Err(NanVixError::Policy(ERR_DIRECTIONAL_NETWORK.to_string())); } Ok(egress == NetworkAction::Allow) } @@ -527,13 +543,13 @@ impl NanVixScriptRunner { fn validate_policies(request: &ExecutionRequest) -> Result<(), NanVixError> { // denied_paths is explicitly rejected — microvm has no host visibility. if !request.policy.denied_paths.is_empty() { - return Err(NanVixError::Preflight(ERR_DENIED_PATHS.to_string())); + return Err(NanVixError::Policy(ERR_DENIED_PATHS.to_string())); } if request.policy.network_proxy.is_enabled() { - return Err(NanVixError::Preflight(ERR_PROXY_POLICY.to_string())); + return Err(NanVixError::Policy(ERR_PROXY_POLICY.to_string())); } if !request.working_directory.is_empty() { - return Err(NanVixError::Preflight(ERR_WORKDIR.to_string())); + return Err(NanVixError::Policy(ERR_WORKDIR.to_string())); } Self::resolve_networking_mode(request)?; @@ -806,8 +822,9 @@ impl NanVixScriptRunner { } /// Returns `true` when filesystem copyback should run. /// Copyback runs on any normal process exit (including non-zero exit codes). - /// It is skipped for preflight, spawn, runtime, and timeout errors, and for - /// OS crashes (negative exit codes from NTSTATUS values). + /// It is skipped for every runner error (policy, availability, setup, spawn, + /// runtime, timeout) and for OS crashes (negative exit codes from NTSTATUS + /// values). fn should_copy_back(response: &ScriptResponse) -> bool { response.error_message.is_empty() && response.exit_code >= 0 } @@ -853,7 +870,7 @@ impl ScriptRunner for NanVixScriptRunner { ) { Ok(s) => s, Err(e) => { - let err = NanVixError::Preflight(e.to_string()); + let err = NanVixError::Setup(e.to_string()); let _ = writeln!(logger, "{}", err); return err.to_response(); } @@ -1184,7 +1201,8 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = runner.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert!(resp.error_message.contains(ERR_WORKDIR)); } @@ -1225,7 +1243,8 @@ mod tests { }; let mut logger = Logger::new(Mode::Buffer); let resp = runner.run(&request, &mut logger); - assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, FailurePhase::Rejected.failure_exit_code()); assert!(resp.error_message.contains(ERR_PROXY_POLICY)); } @@ -1338,12 +1357,26 @@ mod tests { // -- NanVixError display tests --------------------------------------------- #[test] - fn error_display_preflight() { - let err = NanVixError::Preflight("missing binary".to_string()); - assert!(err.to_string().contains("preflight")); + fn error_display_policy() { + let err = NanVixError::Policy("workingDirectory".to_string()); + assert!(err.to_string().contains("policy")); + assert!(err.to_string().contains("workingDirectory")); + } + + #[test] + fn error_display_unavailable() { + let err = NanVixError::Unavailable("missing binary".to_string()); + assert!(err.to_string().contains("not available on this host")); assert!(err.to_string().contains("missing binary")); } + #[test] + fn error_display_setup() { + let err = NanVixError::Setup("snapshot home".to_string()); + assert!(err.to_string().contains("setup")); + assert!(err.to_string().contains("snapshot home")); + } + #[test] fn error_display_platform() { let err = NanVixError::Platform("spawn failed".to_string()); @@ -1370,9 +1403,53 @@ mod tests { assert!(msg.contains("5000")); } + #[test] + fn refused_policy_is_reported_as_a_rejection() { + let resp = NanVixError::Policy(ERR_WORKDIR.to_string()).to_response(); + assert_eq!(resp.failure_phase, FailurePhase::Rejected); + assert_eq!(resp.exit_code, 1); + assert!(!resp.error_message.is_empty()); + } + + #[test] + fn missing_runtime_artifacts_blame_the_host() { + let resp = NanVixError::Unavailable("nanvixd not found".to_string()).to_response(); + assert_eq!(resp.failure_phase, FailurePhase::BackendUnavailable); + assert_eq!(resp.exit_code, ERROR_EXIT_CODE); + } + + #[test] + fn run_faults_keep_the_generic_backend_error_code() { + for (err, phase) in [ + ( + NanVixError::Setup("staging".to_string()), + FailurePhase::LaunchFailed, + ), + ( + NanVixError::Platform("spawn".to_string()), + FailurePhase::LaunchFailed, + ), + ( + NanVixError::Runtime("crash".to_string()), + FailurePhase::PostLaunchFailed, + ), + ( + NanVixError::Timeout { + script_timeout_ms: 1, + total_ms: 2, + }, + FailurePhase::Timeout, + ), + ] { + let resp = err.to_response(); + assert_eq!(resp.failure_phase, phase, "{err}"); + assert_eq!(resp.exit_code, ERROR_EXIT_CODE, "{err}"); + } + } + #[test] fn error_to_response_has_error_exit_code() { - let err = NanVixError::Preflight("test".to_string()); + let err = NanVixError::Setup("test".to_string()); let resp = err.to_response(); assert_eq!(resp.exit_code, ERROR_EXIT_CODE); assert!(!resp.error_message.is_empty()); diff --git a/src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs b/src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs index 47e1da14b5..7e4b5a1640 100644 --- a/src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs +++ b/src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs @@ -43,6 +43,7 @@ use crate::mxc_common::models::{ CaptureDenialsErrorOutput, CaptureDenialsOutput, ContainmentBackend, ExecutionRequest, FailurePhase, SandboxOutputMetadata, ScriptResponse, }; +use crate::mxc_common::mxc_error::MxcError; use crate::mxc_common::process_util::{ create_std_pipes, InterruptiblePipeReader, OwnedHandle, PipeReadCanceller, PipeWriter, SendOwnedHandle, @@ -146,6 +147,9 @@ const PSEC_ENUMERATE_PATHS_UNSUPPORTED_MSG: &str = and QueryProcessSecurityEnvironmentSupport to advertise PSE_SUPPORT_FS_ENUMERATE; this OS \ build does not support that policy, and enumeration-only access cannot fall back to \ AppContainer enforcement"; +const PSEC_BASELINE_UNSUPPORTED_MSG: &str = + "this host does not support the baseline Process Security Environment contract version, so \ + the BaseContainer backend cannot be provided"; const CREATE_PROCESS_IN_SECURITY_ENVIRONMENT_API: &str = "CreateProcessW(PROC_THREAD_ATTRIBUTE_SECURITY_ENVIRONMENT)"; @@ -276,6 +280,14 @@ impl BaseContainerRequestDecision { ), } } + + /// The typed error for a request native PSEC cannot service. + // SDK v2: PolicyIncompatible, DeniedPathsUnsupported and + // EnumeratePathsUnsupported report policy_validation. + // See docs/development/plans/sdk-v2-error-codes.md. + pub(crate) fn rejection_error(self) -> Option { + self.rejection_message().map(MxcError::backend_unavailable) + } } /// Script runner that launches through a process security environment. @@ -301,9 +313,9 @@ impl BaseContainerRunner { /// Construct a runner for `request`, or return why native PSEC cannot fully /// enforce it. The serviceability decision is made once here, so /// validation during execution does not repeat the OS capability check. - pub(crate) fn for_request(request: &ExecutionRequest) -> Result { - match Self::can_backend_service_request(request).rejection_message() { - Some(reason) => Err(reason), + pub(crate) fn for_request(request: &ExecutionRequest) -> Result { + match Self::can_backend_service_request(request).rejection_error() { + Some(error) => Err(error), None => Ok(Self { request_serviceability_confirmed: true, ..Self::default() @@ -427,6 +439,24 @@ impl BaseContainerRunner { version } + /// Attribute an unsupported PSEC version: a version raised by a requested + /// policy feature is the request's fault, while an unsupported baseline + /// means the host does not implement the contract at all. + fn unsupported_psec_version_response( + request: &ExecutionRequest, + psec_version: SecurityEnvironmentVersion, + ) -> ScriptResponse { + if psec_version == SecurityEnvironmentVersion::V1_0 { + // SDK v2: unavailable (backend_unavailable). See docs/development/plans/sdk-v2-error-codes.md. + return ScriptResponse::rejected(PSEC_BASELINE_UNSUPPORTED_MSG); + } + ScriptResponse::rejected(if !request.policy.enumerate_paths.is_empty() { + PSEC_ENUMERATE_PATHS_UNSUPPORTED_MSG + } else { + "the required Process Security Environment schema version is not supported" + }) + } + /// The single BaseContainer serviceability decision point: return the first /// reason the request cannot be enforced, or /// [`BaseContainerRequestDecision::Serviceable`] when every check passes. @@ -494,20 +524,15 @@ impl BaseContainerRunner { ); let psec_version = Self::choose_min_required_psec_version_for_request(request); - let version_supported = - secenv::supports_version(psec_version).map_err(|error| ScriptResponse { - failure_phase: FailurePhase::BackendUnavailable, - ..ScriptResponse::error(&format!( - "failed to query Process Security Environment contract support: {error}" - )) - })?; + let version_supported = secenv::supports_version(psec_version).map_err(|error| { + ScriptResponse::unavailable(&format!( + "failed to query Process Security Environment contract support: {error}" + )) + })?; if !version_supported { - return Err(ScriptResponse::rejected( - if !request.policy.enumerate_paths.is_empty() { - PSEC_ENUMERATE_PATHS_UNSUPPORTED_MSG - } else { - "the required Process Security Environment schema version is not supported" - }, + return Err(Self::unsupported_psec_version_response( + request, + psec_version, )); } @@ -779,13 +804,7 @@ impl BaseContainerRunner { } else { FailurePhase::LaunchFailed }; - return Err(ScriptResponse { - exit_code: -1, - error_message: msg.clone(), - standard_err: msg, - failure_phase, - ..Default::default() - }); + return Err(ScriptResponse::failed(failure_phase, &msg)); } } } else { @@ -813,13 +832,7 @@ impl BaseContainerRunner { } else { FailurePhase::LaunchFailed }; - return Err(ScriptResponse { - exit_code: -1, - error_message: msg.clone(), - standard_err: msg, - failure_phase, - ..Default::default() - }); + return Err(ScriptResponse::failed(failure_phase, &msg)); } } } @@ -872,13 +885,7 @@ impl BaseContainerRunner { } else { FailurePhase::LaunchFailed }; - return Err(ScriptResponse { - exit_code: -1, - error_message: msg.clone(), - standard_err: msg, - failure_phase, - ..Default::default() - }); + return Err(ScriptResponse::failed(failure_phase, &msg)); } }; let environment = (!current_env_ptr.is_null()).then_some(current_env_ptr); @@ -960,12 +967,8 @@ impl BaseContainerRunner { }; return Err(ScriptResponse { - exit_code: -1, - error_message: diag.message.clone(), - standard_err: diag.message, extended_error, - failure_phase, - ..Default::default() + ..ScriptResponse::failed(failure_phase, &diag.message) }); } @@ -1037,12 +1040,8 @@ impl BaseContainerRunner { ); } return Err(ScriptResponse { - exit_code: -1, - error_message: JOB_SETUP_FAILED_MSG.to_string(), - standard_err: JOB_SETUP_FAILED_MSG.to_string(), extended_error, - failure_phase: FailurePhase::LaunchFailed, - ..Default::default() + ..ScriptResponse::failed(FailurePhase::LaunchFailed, JOB_SETUP_FAILED_MSG) }); } }; @@ -1076,10 +1075,7 @@ impl BaseContainerRunner { "; additionally failed to discard the learning-mode trace: {error}" ); } - return Err(ScriptResponse { - failure_phase: FailurePhase::LaunchFailed, - ..ScriptResponse::error(&message) - }); + return Err(ScriptResponse::failed(FailurePhase::LaunchFailed, &message)); } crate::mxc_common::telemetry::log_network_policy_applied( @@ -1183,13 +1179,10 @@ impl SandboxBackend for BaseContainerRunner { if !self.request_serviceability_confirmed && !Self::can_backend_service_request(request).can_service_request() { - return Err(ScriptResponse { - failure_phase: FailurePhase::BackendUnavailable, - ..ScriptResponse::error( - "the request cannot be represented by the process security environment \ - available on this host", - ) - }); + return Err(ScriptResponse::unavailable( + "the request cannot be represented by the process security environment \ + available on this host", + )); } if request.policy.least_privilege_mode { return Err(ScriptResponse::rejected( @@ -2052,6 +2045,7 @@ mod tests { NetworkCidr, NetworkEgressPolicy, NetworkIngressPolicy, NetworkPeer, NetworkPort, NetworkProtocol, NetworkRule, ProxyAddress, ProxyConfig, UiPolicy, }; + use crate::mxc_common::mxc_error::MxcErrorCode; use crate::mxc_common::network_parser::{parse_network_policy, NetworkSections}; use crate::mxc_common::ui_policy::EffectiveUiRestrictions; use crate::mxc_common::wire; @@ -2059,6 +2053,33 @@ mod tests { use crate::process_security_environment_spec::process_security_environment_layout as psec_layout; use std::sync::atomic::{AtomicUsize, Ordering}; + /// A host that does not implement the baseline contract cannot be fixed by + /// rewriting the request; a version raised by a requested feature can. + #[test] + fn an_unsupported_psec_version_is_rejected() { + let plain = ExecutionRequest::default(); + let baseline = BaseContainerRunner::unsupported_psec_version_response( + &plain, + SecurityEnvironmentVersion::V1_0, + ); + assert_eq!(baseline.failure_phase, FailurePhase::Rejected); + assert_eq!(baseline.error_message, PSEC_BASELINE_UNSUPPORTED_MSG); + + let enumerate = ExecutionRequest { + policy: ContainerPolicy { + enumerate_paths: vec![r"C:\project".to_string()], + ..Default::default() + }, + ..Default::default() + }; + let raised = BaseContainerRunner::unsupported_psec_version_response( + &enumerate, + SecurityEnvironmentVersion::V1_1, + ); + assert_eq!(raised.failure_phase, FailurePhase::Rejected); + assert_eq!(raised.error_message, PSEC_ENUMERATE_PATHS_UNSUPPORTED_MSG); + } + #[test] fn native_capture_probe_requires_base_container_and_complete_learning_mode_abi() { assert!(!BaseContainerRunner::native_capture_available_with( @@ -3725,11 +3746,13 @@ mod tests { crate::process_container_common::test_env::CaptureCapabilityGuard::set(true, true); let mut request = ExecutionRequest::default(); request.policy.least_privilege_mode = true; + let error = BaseContainerRunner::for_request(&request) + .err() + .expect("LPAC must be rejected"); + assert_eq!(error.code, MxcErrorCode::BackendUnavailable); assert_eq!( - BaseContainerRunner::for_request(&request).err(), - Some( - "Windows ProcessContainer cannot enforce the requested LPAC or legacy network.proxy policy" - ) + error.message, + "Windows ProcessContainer cannot enforce the requested LPAC or legacy network.proxy policy" ); } @@ -3741,18 +3764,21 @@ mod tests { DeniedPathsUnsupported, EnumeratePathsUnsupported, NativeCaptureUnavailable, }; - for (decision, message) in [ + for (decision, message, code) in [ ( DeniedPathsUnsupported, "Windows ProcessContainer requires native PSEC denied-path support for this request", + MxcErrorCode::BackendUnavailable, ), ( EnumeratePathsUnsupported, "Windows ProcessContainer requires native PSEC enumeration-only access support for this request", + MxcErrorCode::BackendUnavailable, ), ( NativeCaptureUnavailable, "Windows ProcessContainer requires native Learning Mode capture support for this request", + MxcErrorCode::BackendUnavailable, ), ] { let _guard = PolicyCapabilityGuard::set( @@ -3771,7 +3797,10 @@ mod tests { BaseContainerRunner::can_backend_service_request(&request), decision ); - assert_eq!(BaseContainerRunner::for_request(&request).err(), Some(message)); + let error = BaseContainerRunner::for_request(&request) + .err() + .expect("an unsupported capability must be rejected"); + assert_eq!((error.code, error.message.as_str()), (code, message)); let probe = run_probe(&request); assert_eq!(probe.tier, None); assert_eq!(probe.needs_dacl_augmentation, None); diff --git a/src/mxc-sdk/src/backends/seatbelt/common/profile_builder.rs b/src/mxc-sdk/src/backends/seatbelt/common/profile_builder.rs index e02792781c..e0e2463030 100644 --- a/src/mxc-sdk/src/backends/seatbelt/common/profile_builder.rs +++ b/src/mxc-sdk/src/backends/seatbelt/common/profile_builder.rs @@ -42,6 +42,23 @@ pub struct ProfileInputs { pub user_keychains: Option, } +/// Why a profile could not be built: the request, or the executor's host. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ProfileError { + /// The request names something the profile cannot express. + Policy(String), + /// The executor's environment lacks something the profile needs. + Host(String), +} + +impl std::fmt::Display for ProfileError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Policy(message) | Self::Host(message) => f.write_str(message), + } + } +} + /// Build a complete Seatbelt sandbox profile, scoping cooperative proxy /// reachability to the resolved address supplied by the runner. /// @@ -52,7 +69,7 @@ pub fn build_profile_with_proxy( request: &ExecutionRequest, proxy_address: Option<&ProxyAddress>, inputs: &ProfileInputs, -) -> Result { +) -> Result { build_profile_with_terminals(request, proxy_address, inputs, &[]) } @@ -62,7 +79,7 @@ pub(crate) fn build_profile_with_terminals( proxy_address: Option<&ProxyAddress>, inputs: &ProfileInputs, terminal_paths: &[PathBuf], -) -> Result { +) -> Result { if let Some(override_profile) = request .seatbelt .as_ref() @@ -90,9 +107,9 @@ pub(crate) fn build_profile_with_terminals( // Keep the controlling terminal usable without granting other host PTYs. out.push_str(TTY_ALLOW); for path in terminal_paths { - let path = path - .to_str() - .ok_or_else(|| "Seatbelt: assigned terminal path is not valid UTF-8".to_string())?; + let path = path.to_str().ok_or_else(|| { + ProfileError::Host("Seatbelt: assigned terminal path is not valid UTF-8".into()) + })?; let _ = writeln!( out, "(allow file-read* file-write* file-ioctl (literal {}))", @@ -544,13 +561,16 @@ fn write_keychain_rules( out: &mut String, request: &ExecutionRequest, user_keychains: Option<&str>, -) -> Result<(), String> { +) -> Result<(), ProfileError> { let enabled = request.seatbelt.as_ref().is_some_and(|c| c.keychain_access); if !enabled { return Ok(()); } - let user_keychains = user_keychains - .ok_or_else(|| "User keychain directory was not prepared for keychainAccess".to_string())?; + let user_keychains = user_keychains.ok_or_else(|| { + ProfileError::Host( + "User keychain directory was not prepared for keychainAccess".to_string(), + ) + })?; out.push_str(";; --- keychainAccess: Mach IPC for Keychain (securityd, prefs, XPC, LS) ---\n"); out.push_str("(allow mach-lookup\n"); @@ -628,14 +648,14 @@ mod tests { use super::*; use crate::mxc_common::models::{NetworkAction, SeatbeltConfig, UiPolicy}; - fn build_profile(request: &ExecutionRequest) -> Result { + fn build_profile(request: &ExecutionRequest) -> Result { build_profile_with_proxy(request, request.policy.network_proxy.address.as_ref()) } fn build_profile_with_proxy( request: &ExecutionRequest, proxy_address: Option<&ProxyAddress>, - ) -> Result { + ) -> Result { let inputs = ProfileInputs { readonly_paths: request.policy.readonly_paths.clone(), readwrite_paths: request.policy.readwrite_paths.clone(), @@ -1710,6 +1730,11 @@ mod tests { }); let error = super::build_profile_with_proxy(&request, None, &ProfileInputs::default()).unwrap_err(); - assert!(error.contains("not prepared for keychainAccess"), "{error}"); + assert!( + error + .to_string() + .contains("not prepared for keychainAccess"), + "{error}" + ); } } diff --git a/src/mxc-sdk/src/backends/seatbelt/common/profile_preparation.rs b/src/mxc-sdk/src/backends/seatbelt/common/profile_preparation.rs index 89b71e490b..5429acf112 100644 --- a/src/mxc-sdk/src/backends/seatbelt/common/profile_preparation.rs +++ b/src/mxc-sdk/src/backends/seatbelt/common/profile_preparation.rs @@ -7,14 +7,14 @@ use std::fs; use std::path::{Path, PathBuf}; use crate::mxc_common::models::ExecutionRequest; -use crate::seatbelt_common::profile_builder::ProfileInputs; +use crate::seatbelt_common::profile_builder::{ProfileError, ProfileInputs}; const DEVELOPER_DIR_LINKS: [&str; 2] = [ "/private/var/db/xcode_select_link", "/private/var/select/developer_dir", ]; -pub(crate) fn prepare_profile(request: &ExecutionRequest) -> Result { +pub(crate) fn prepare_profile(request: &ExecutionRequest) -> Result { if request .seatbelt .as_ref() @@ -31,8 +31,11 @@ pub(crate) fn prepare_profile(request: &ExecutionRequest) -> Result Result Result, String> { +fn resolve_all(paths: &[String]) -> Result, ProfileError> { paths.iter().map(|p| resolve_policy_path(p)).collect() } /// Seatbelt matches real paths, so a symlinked HOME must be resolved. -fn user_keychains_dir(home: &str) -> Result { +fn user_keychains_dir(home: &str) -> Result { resolve_policy_path(&format!("{home}/Library/Keychains")) } /// Follow symlinks before applying `..`, retaining missing trailing names. -pub(crate) fn resolve_policy_path(path: &str) -> Result { - let expanded = expand_tilde(path)?; - let resolution_error = - |error| format!("Filesystem path '{path}' could not be resolved: {error}"); - let absolute = std::path::absolute(&expanded).map_err(resolution_error)?; +pub(crate) fn resolve_policy_path(path: &str) -> Result { + let expanded = expand_tilde(path).map_err(ProfileError::Host)?; + let absolute = std::path::absolute(&expanded).map_err(|error| { + ProfileError::Host(format!( + "Filesystem path '{path}' could not be resolved: {error}" + )) + })?; let resolved = crate::mxc_common::filesystem_symlink::resolve_through_symlinks(&absolute) - .ok_or_else(|| format!("Filesystem path '{path}' has no resolved target"))?; + .ok_or_else(|| { + ProfileError::Policy(format!("Filesystem path '{path}' has no resolved target")) + })?; if let Some(error) = resolved.error { - return Err(resolution_error(error)); + return Err(classify_io_error( + format!("Filesystem path '{path}' could not be resolved: {error}"), + &error, + )); + } + resolved.path.into_os_string().into_string().map_err(|_| { + ProfileError::Policy(format!( + "Filesystem path '{path}' resolves to a non-UTF-8 path" + )) + }) +} + +/// Errors a different path would avoid blame the request; anything else is a host fault. +pub(crate) fn classify_io_error(message: String, error: &std::io::Error) -> ProfileError { + if crate::mxc_common::filesystem_symlink::is_policy_path_error(error) { + ProfileError::Policy(message) + } else { + ProfileError::Host(message) } - resolved - .path - .into_os_string() - .into_string() - .map_err(|_| format!("Filesystem path '{path}' resolves to a non-UTF-8 path")) } pub(crate) fn expand_tilde(path: &str) -> Result { @@ -213,7 +232,10 @@ mod tests { for suffix in ["", "child", "../secret"] { let path = link.join(suffix); let error = resolve_policy_path(path.to_str().unwrap()).unwrap_err(); - assert!(error.contains("could not be resolved"), "{error}"); + assert!( + matches!(&error, ProfileError::Policy(message) if message.contains("could not be resolved")), + "{error:?}" + ); } } } diff --git a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs index c276a8a66f..2b2fb05cab 100644 --- a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs +++ b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs @@ -27,7 +27,7 @@ use std::time::{Duration, Instant}; use crate::mxc_common::interruptible_reader::{wrap_pipe, InterruptibleReader, ReadCanceller}; use crate::mxc_common::logger::Logger; -use crate::mxc_common::models::{ExecutionRequest, ProxyAddress, ScriptResponse}; +use crate::mxc_common::models::{ExecutionRequest, FailurePhase, ProxyAddress, ScriptResponse}; use crate::mxc_common::sandbox_process::{ boxed_closer, cancel_and_join_discard, duplicate_and_take_native_stdio, group_kill, spawn_discard, take_boxed_read, take_boxed_write, wait_with_timeout, NativeStdio, PtySize, @@ -43,9 +43,9 @@ use crate::seatbelt_common::default_env::DEFAULT_SANDBOX_PATH; use crate::seatbelt_common::default_env::{env_pairs, resolved_env}; #[cfg(test)] use crate::seatbelt_common::profile_builder::build_profile_with_proxy; -use crate::seatbelt_common::profile_builder::build_profile_with_terminals; +use crate::seatbelt_common::profile_builder::{build_profile_with_terminals, ProfileError}; use crate::seatbelt_common::profile_preparation::{ - expand_tilde, prepare_profile, resolve_policy_path, + classify_io_error, expand_tilde, prepare_profile, resolve_policy_path, }; #[link(name = "proc")] @@ -285,12 +285,12 @@ impl SandboxBackend for SeatbeltScriptRunner { // Seatbelt's own invariants — the only home for them, so a caller that // builds an ExecutionRequest directly gets the same rules. crate::seatbelt_common::seatbelt_policy::validate_seatbelt_network_policy(&request.policy) - .map_err(error_response)?; + .map_err(rejected_response)?; crate::seatbelt_common::seatbelt_policy::validate_seatbelt_ui_policy(request) - .map_err(error_response)?; + .map_err(rejected_response)?; // Dry runs never reach spawn, so reject unresolvable paths here too. - prepare_profile(request).map_err(error_response)?; - resolved_working_directory_opt(request).map_err(error_response)?; + prepare_profile(request).map_err(profile_error_response)?; + resolved_working_directory_opt(request).map_err(profile_error_response)?; Ok(()) } @@ -346,7 +346,7 @@ fn spawn_exec( } else { "a caller-owned PTY" }; - return Err(error_response(format!( + return Err(rejected_response(format!( "Seatbelt guiAccess requires inherited stdio and cannot use {mode}" ))); } @@ -380,7 +380,7 @@ fn spawn_exec( // // The cwd is anchored and resolved to its physical path so `PWD` and the // default `HOME` match the directory the child actually starts in. - let resolved_cwd = resolved_working_directory_opt(request).map_err(error_response)?; + let resolved_cwd = resolved_working_directory_opt(request).map_err(profile_error_response)?; let cwd = resolved_cwd .as_deref() .unwrap_or(UNRESOLVED_WORKING_DIRECTORY); @@ -414,7 +414,7 @@ fn spawn_exec( let mut child = command .spawn() - .map_err(|error| error_response(spawn_error(&error)))?; + .map_err(|error| launch_failed_response(spawn_error(&error)))?; let (stdin, stdout, stderr) = match stdio { StdioMode::Pipes => (child.stdin.take(), child.stdout.take(), child.stderr.take()), @@ -435,7 +435,7 @@ fn spawn_exec( let _ = child.kill(); let _ = child.wait(); let error = out_result.err().or(err_result.err()); - return Err(error_response(format!( + return Err(launch_failed_response(format!( "Seatbelt: failed to wrap stdio pipes: {}", error.map_or_else(|| "unknown error".to_string(), |e| e.to_string()), ))); @@ -831,25 +831,28 @@ fn build_sandbox_command( }, &[], ) - .map_err(|error| error_response(format!("Seatbelt: failed to allocate PTY: {error}"))) + .map_err(|error| { + launch_failed_response(format!("Seatbelt: failed to allocate PTY: {error}")) + }) }) .transpose()?; let terminal_paths = match &pty { Some(pty) => vec![pty.secondary_path().to_path_buf()], None if !new_session => inherited_terminal_paths().map_err(|error| { - error_response(format!( + launch_failed_response(format!( "Seatbelt: failed to resolve inherited terminals: {error}" )) })?, None => Vec::new(), }; - let inputs = prepare_profile(request).map_err(error_response)?; + let inputs = prepare_profile(request).map_err(profile_error_response)?; let profile = build_profile_with_terminals(request, proxy_address, &inputs, &terminal_paths) - .map_err(error_response)?; + .map_err(profile_error_response)?; log_generated_profile(&profile, logger); - let profile_cstr = CString::new(profile) - .map_err(|e| error_response(format!("seatbelt profile contains embedded NUL byte: {e}")))?; + let profile_cstr = CString::new(profile).map_err(|e| { + rejected_response(format!("seatbelt profile contains embedded NUL byte: {e}")) + })?; let _ = writeln!(logger, "Seatbelt: applying sandbox via sandbox_init"); // SAFETY: The closure runs after fork(), before exec(). We only call @@ -908,11 +911,22 @@ fn log_generated_profile(profile: &str, logger: &mut Logger) { let _ = writeln!(logger, "{PROFILE_LOG_END}"); } -fn error_response(message: String) -> ScriptResponse { - ScriptResponse { - exit_code: -1, - error_message: message, - ..Default::default() +/// A request this backend refuses. +// SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. +fn rejected_response(message: String) -> ScriptResponse { + ScriptResponse::failed(FailurePhase::None, &message) +} + +/// A fault while bringing the sandboxed child up, after the request was +/// accepted. +fn launch_failed_response(message: String) -> ScriptResponse { + ScriptResponse::failed(FailurePhase::LaunchFailed, &message) +} + +fn profile_error_response(error: ProfileError) -> ScriptResponse { + match error { + ProfileError::Policy(message) => rejected_response(message), + ProfileError::Host(message) => launch_failed_response(message), } } @@ -951,7 +965,9 @@ const UNRESOLVED_WORKING_DIRECTORY: &str = "/"; /// /// The same value feeds the environment, so the default `HOME` cannot name a /// directory the child never entered. -fn resolved_working_directory_opt(request: &ExecutionRequest) -> Result, String> { +fn resolved_working_directory_opt( + request: &ExecutionRequest, +) -> Result, ProfileError> { let expand = |path: &str| expand_tilde(path).unwrap_or_else(|_| path.to_string()); let Some(selected) = request .resolved_working_directory_with(|path| Path::new(&expand(path)).is_dir()) @@ -960,27 +976,33 @@ fn resolved_working_directory_opt(request: &ExecutionRequest) -> Result {} Ok(_) => { - return Err(format!( + return Err(ProfileError::Policy(format!( "Seatbelt working directory '{absolute}' is not a directory" - )) + ))) } Err(e) => { - return Err(format!( - "Seatbelt working directory '{absolute}' is not usable: {e}" + return Err(classify_io_error( + format!("Seatbelt working directory '{absolute}' is not usable: {e}"), + &e, )) } } resolve_policy_path(&absolute) .map(Some) - .map_err(|e| format!("Seatbelt working directory: {e}")) + .map_err(|error| match error { + ProfileError::Policy(e) => { + ProfileError::Policy(format!("Seatbelt working directory: {e}")) + } + ProfileError::Host(e) => ProfileError::Host(format!("Seatbelt working directory: {e}")), + }) } /// Anchor a resolved working directory to the launcher's directory when it is @@ -1239,6 +1261,19 @@ os.close(fd) .err() .expect("guiAccess PTY must be rejected"); assert!(error.error_message.contains("caller-owned PTY")); + assert_eq!(error.failure_phase, FailurePhase::None); + assert_eq!(error.exit_code, FailurePhase::None.failure_exit_code()); + } + + #[test] + fn a_failed_launch_keeps_the_generic_backend_error_code() { + let response = launch_failed_response("failed to spawn sandboxed process".into()); + assert_eq!(response.failure_phase, FailurePhase::LaunchFailed); + assert_eq!( + response.failure_phase.error_code(), + crate::mxc_common::mxc_error::MxcErrorCode::BackendError + ); + assert_eq!(response.exit_code, -1); } #[test] @@ -1422,6 +1457,8 @@ os.close(fd) assert!(response.error_message.contains("hostLoopback='allow'")); assert!(response.error_message.contains("default='deny'")); assert!(!response.error_message.contains("inherit 'default'")); + assert_eq!(response.failure_phase, FailurePhase::None); + assert_eq!(response.exit_code, FailurePhase::None.failure_exit_code()); } /// The other divergent pair is expressible and must be accepted: it is the @@ -1622,10 +1659,11 @@ os.close(fd) for cwd in ["dangling", "loop", "missing", "missing/../project", "file"] { let mut request = base_request(); request.working_directory = dir.path().join(cwd).to_str().unwrap().into(); - let error = SeatbeltScriptRunner::new() + let response = SeatbeltScriptRunner::new() .validate(&request) - .expect_err("dry runs must reject an unusable cwd") - .error_message; + .expect_err("dry runs must reject an unusable cwd"); + assert_eq!(response.failure_phase, FailurePhase::None, "{cwd}"); + let error = response.error_message; assert!( error.contains("Seatbelt working directory"), "{cwd}: {error}" diff --git a/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/error.rs b/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/error.rs index cbe9d4186b..227888e092 100644 --- a/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/error.rs +++ b/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/error.rs @@ -59,12 +59,9 @@ impl OneShotError { }; ScriptResponse { - exit_code: -1, standard_err: summary.to_string(), - error_message: summary.to_string(), extended_error: detail, - failure_phase, - ..Default::default() + ..ScriptResponse::failed(failure_phase, summary) } } @@ -72,10 +69,11 @@ impl OneShotError { /// whether a retry could ever succeed. fn failure_phase(&self) -> FailurePhase { match self { - // Non-retryable preflight: the request/config cannot be honored or a - // required host prerequisite is missing. Retrying the same input on - // the same host will not succeed. - OneShotError::SandboxUnavailable(_) | OneShotError::Policy(_) => FailurePhase::Rejected, + // The optional OS feature is absent or disabled, so the host has to + // be provisioned; no change to the request reaches it. + OneShotError::SandboxUnavailable(_) => FailurePhase::BackendUnavailable, + // The request asks for something this backend will not do. + OneShotError::Policy(_) => FailurePhase::Rejected, // Launch attempt failed (incl. transient single-instance contention, // async-runtime setup, capture-proof, rendezvous wait, and the // initial guest connect) — generally worth retrying. @@ -91,6 +89,7 @@ impl OneShotError { #[cfg(test)] mod tests { use super::*; + use crate::mxc_common::mxc_error::MxcErrorCode; #[test] fn busy_maps_to_launch_failed_with_detail() { @@ -102,18 +101,23 @@ mod tests { } #[test] - fn policy_and_prereq_errors_map_to_rejected() { - for err in [ - OneShotError::Policy("denied path in share".to_string()), - OneShotError::SandboxUnavailable("feature off".to_string()), - ] { - let resp = err.into_response(); - assert_eq!( - resp.failure_phase, - FailurePhase::Rejected, - "non-retryable preflight should map to Rejected" - ); - } + fn a_refused_policy_is_told_apart_from_a_host_that_cannot_run_it() { + let refused = OneShotError::Policy("denied path in share".to_string()).into_response(); + assert_eq!(refused.failure_phase, FailurePhase::Rejected); + assert_eq!(refused.exit_code, 1); + assert_eq!( + refused.failure_phase.error_code(), + MxcErrorCode::PolicyValidation + ); + + let unavailable = + OneShotError::SandboxUnavailable("feature off".to_string()).into_response(); + assert_eq!(unavailable.failure_phase, FailurePhase::BackendUnavailable); + assert_eq!(unavailable.exit_code, -1); + assert_eq!( + unavailable.failure_phase.error_code(), + MxcErrorCode::BackendUnavailable + ); } #[test] diff --git a/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/state_aware.rs b/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/state_aware.rs index 8ca669f7df..0e106ee8de 100644 --- a/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/state_aware.rs +++ b/src/mxc-sdk/src/backends/windows_sandbox/lifecycle/state_aware.rs @@ -210,10 +210,12 @@ fn is_valid_sandbox_token(token: &str) -> bool { } /// Map a [`OneShotError`] from policy planning to the wire error model. Policy -/// rejections become `policy_validation`; anything else is a backend error. +/// rejections become `policy_validation`, a missing host feature becomes +/// `backend_unavailable`, and anything else is a backend error. fn map_policy_error(e: OneShotError) -> MxcError { match e { OneShotError::Policy(msg) => MxcError::policy_validation(msg), + OneShotError::SandboxUnavailable(msg) => MxcError::backend_unavailable(msg), other => MxcError::backend_error(format!("{other:?}")), } } @@ -1325,6 +1327,12 @@ mod tests { assert_eq!(err.code, MxcErrorCode::PolicyValidation); } + #[test] + fn map_policy_error_blames_the_host_for_a_missing_sandbox_feature() { + let err = map_policy_error(OneShotError::SandboxUnavailable("feature off".into())); + assert_eq!(err.code, MxcErrorCode::BackendUnavailable); + } + #[test] fn validate_start_rejects_non_default_network() { let backend = WindowsSandboxRunner::new(); diff --git a/src/mxc-sdk/src/backends/wslc/common/container_steps.rs b/src/mxc-sdk/src/backends/wslc/common/container_steps.rs index f36b13783e..7cc10cda50 100644 --- a/src/mxc-sdk/src/backends/wslc/common/container_steps.rs +++ b/src/mxc-sdk/src/backends/wslc/common/container_steps.rs @@ -806,14 +806,15 @@ pub unsafe fn load_sdk_checked(logger: &mut Logger) -> Result, pub store_may_be_held: bool, } impl From for SessionCreateFailure { fn from(response: ScriptResponse) -> Self { Self { - response, + response: Box::new(response), store_may_be_held: false, } } @@ -889,7 +890,7 @@ pub unsafe fn create_daemon_session( } }; return Err(SessionCreateFailure { - response, + response: Box::new(response), store_may_be_held, }); } diff --git a/src/mxc-sdk/src/backends/wslc/common/error.rs b/src/mxc-sdk/src/backends/wslc/common/error.rs index 5a3c9f5b22..4e284b6762 100644 --- a/src/mxc-sdk/src/backends/wslc/common/error.rs +++ b/src/mxc-sdk/src/backends/wslc/common/error.rs @@ -11,10 +11,8 @@ //! the message. Each variant here attributes the failure to a lifecycle phase //! instead. //! -//! `Display` reproduces the pre-existing message verbatim and -//! [`WslcError::into_response`] fills the same fields `ScriptResponse::error` -//! did, so the text a user sees is unchanged; `failure_phase` is the only -//! addition. +//! `Display` reproduces the pre-existing message verbatim, so the text a user +//! sees is unchanged. use std::fmt; @@ -68,18 +66,9 @@ impl WslcError { } } - /// Render as a [`ScriptResponse`], preserving the exact message text the - /// untyped construction produced. + /// Render as a [`ScriptResponse`] carrying the message verbatim. pub(crate) fn into_response(self) -> ScriptResponse { - let failure_phase = self.failure_phase(); - let msg = self.to_string(); - ScriptResponse { - exit_code: -1, - standard_err: msg.clone(), - error_message: msg, - failure_phase, - ..Default::default() - } + ScriptResponse::failed(self.failure_phase(), &self.to_string()) } } @@ -161,16 +150,34 @@ mod tests { } #[test] - fn response_matches_the_untyped_shape_apart_from_the_phase() { - let typed = WslcError::Rejected("bad path".to_string()).into_response(); - let untyped = ScriptResponse::error("bad path"); - - assert_eq!(typed.exit_code, untyped.exit_code); - assert_eq!(typed.standard_err, untyped.standard_err); - assert_eq!(typed.error_message, untyped.error_message); - // The untyped helper left this at the default, which is the bug. - assert_eq!(untyped.failure_phase, FailurePhase::None); - assert_eq!(typed.failure_phase, FailurePhase::Rejected); + fn response_carries_the_message_and_the_phase() { + let typed = WslcError::Host("bring-up failed".to_string()).into_response(); + + assert_eq!(typed.exit_code, -1); + assert_eq!(typed.error_message, "bring-up failed"); + assert_eq!(typed.failure_phase, FailurePhase::LaunchFailed); + } + + /// A WSLc rejection has to cost the caller the same exit code a rejection + /// from any other backend does. + #[test] + fn exit_codes_follow_the_shared_phase_contract() { + let s = || "x".to_string(); + assert_eq!( + WslcError::Rejected(s()).into_response().exit_code, + ScriptResponse::rejected("x").exit_code + ); + assert_eq!( + WslcError::Unavailable(s()).into_response().exit_code, + ScriptResponse::unavailable("x").exit_code + ); + for err in [ + WslcError::Host(s()), + WslcError::Runtime(s()), + WslcError::sdk("op", -1, ""), + ] { + assert_eq!(err.into_response().exit_code, -1); + } } #[test] diff --git a/src/mxc-sdk/src/backends/wslc/common/policy_mapping.rs b/src/mxc-sdk/src/backends/wslc/common/policy_mapping.rs index d26035b9ee..036819095b 100644 --- a/src/mxc-sdk/src/backends/wslc/common/policy_mapping.rs +++ b/src/mxc-sdk/src/backends/wslc/common/policy_mapping.rs @@ -10,6 +10,7 @@ //! injected, so its logic stays testable without touching disk. use crate::mxc_common::filesystem_canonical::{canonicalize_allowing_absent_tail, PathCanonical}; +use crate::mxc_common::filesystem_object::FilesystemPolicyError; use crate::mxc_common::logger::Logger; use crate::mxc_common::models::{ContainerPolicy, ExecutionRequest}; use crate::wslc_common::wslc_bindings::WslcContainerNetworkingMode; @@ -331,13 +332,13 @@ pub fn validate_denied_path_overlap( /// /// Returns the tightened policy when normalization changed something (the caller /// rebuilds the request around it) or `None` when the policy was already -/// conflict-free. Any failure is returned as a `String`; callers map it to their -/// own error envelope (`ScriptResponse` / `MxcError`). Normalization diagnostics +/// conflict-free. Callers map a failure to their own error envelope +/// (`ScriptResponse` / `MxcError`). Normalization diagnostics /// are written to `logger`; callers decide how to surface them. pub fn apply_provision_policy_gate( request: &ExecutionRequest, logger: &mut Logger, -) -> Result, String> { +) -> Result, FilesystemPolicyError> { let normalized = crate::mxc_common::filesystem_object::normalize_object_conflicts(&request.policy, logger)?; let effective = normalized.as_ref().unwrap_or(&request.policy); @@ -346,7 +347,8 @@ pub fn apply_provision_policy_gate( &effective.readwrite_paths, &effective.readonly_paths, &effective.denied_paths, - )?; + ) + .map_err(FilesystemPolicyError::Policy)?; Ok(normalized) } diff --git a/src/mxc-sdk/src/backends/wslc/common/state_aware.rs b/src/mxc-sdk/src/backends/wslc/common/state_aware.rs index 19eb2bbc0e..1d605e5dff 100644 --- a/src/mxc-sdk/src/backends/wslc/common/state_aware.rs +++ b/src/mxc-sdk/src/backends/wslc/common/state_aware.rs @@ -876,9 +876,10 @@ fn build_provision_config( /// ([`crate::wslc_common::policy_mapping::apply_provision_policy_gate`]): runs the full /// three-step gate (D6 normalization → D3 delegation → denied-path overlap), /// buffering normalization diagnostics and surfacing them on stderr (stdout -/// carries the phase envelope), and maps the gate's `String` error to a -/// `policy_validation` [`MxcError`]. Returns the tightened policy when -/// normalization changed something, else `None`. +/// carries the phase envelope), and maps a gate failure to a `policy_validation` +/// [`MxcError`]. +/// Returns the tightened policy when normalization changed something, else +/// `None`. fn normalize_and_check_delegation( request: &ExecutionRequest, ) -> Result, MxcError> { @@ -891,7 +892,9 @@ fn normalize_and_check_delegation( if !notes.is_empty() { eprint!("{notes}"); } - result.map_err(MxcError::policy_validation) + // SDK v2: FilesystemPolicyError::Host reports backend_error. + // See docs/development/plans/sdk-v2-error-codes.md. + result.map_err(|error| MxcError::policy_validation(error.message())) } /// Build daemon volume mounts from the request's filesystem policy. Overlapping diff --git a/src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs b/src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs index bbd36d482e..4a65f4619c 100644 --- a/src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs +++ b/src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs @@ -882,9 +882,11 @@ impl WSLContainerRunner { &normalized } Ok(None) => request, - Err(msg) => { - let _ = writeln!(logger, "[WSLC] {}", msg); - return Err(WslcError::Rejected(msg).into_response()); + Err(error) => { + let _ = writeln!(logger, "[WSLC] {}", error.message()); + // SDK v2: FilesystemPolicyError::Host maps to WslcError::Host (backend_error). + // See docs/development/plans/sdk-v2-error-codes.md. + return Err(WslcError::Rejected(error.message().to_string()).into_response()); } }; @@ -2031,7 +2033,15 @@ mod tests { let mut runner = WSLContainerRunner::new(&WslcConfig::default()); let response = runner.execute(&request, &mut logger); - assert_eq!(response.exit_code, -1, "overlap must fail the run"); + assert_eq!( + response.exit_code, + crate::mxc_common::models::FailurePhase::Rejected.failure_exit_code(), + "overlap must fail the run" + ); + assert_eq!( + response.failure_phase, + crate::mxc_common::models::FailurePhase::Rejected + ); assert!( response.error_message.contains("cannot be enforced"), "expected the overlap error, got: {}", @@ -2080,9 +2090,14 @@ mod tests { let response = runner.execute(&request, &mut logger); assert_eq!( - response.exit_code, -1, + response.exit_code, + crate::mxc_common::models::FailurePhase::Rejected.failure_exit_code(), "junction-aliased deny must fail the run" ); + assert_eq!( + response.failure_phase, + crate::mxc_common::models::FailurePhase::Rejected + ); assert!( response.error_message.contains("cannot be enforced"), "expected the overlap error, got: {}", @@ -2130,9 +2145,14 @@ mod tests { let response = runner.execute(&request, &mut logger); assert_eq!( - response.exit_code, -1, + response.exit_code, + crate::mxc_common::models::FailurePhase::Rejected.failure_exit_code(), "absent junction-aliased deny must fail the run" ); + assert_eq!( + response.failure_phase, + crate::mxc_common::models::FailurePhase::Rejected + ); assert!( response.error_message.contains("cannot be enforced"), "expected the overlap error, got: {}", diff --git a/src/mxc-sdk/src/bin/wslc_daemon/session_manager.rs b/src/mxc-sdk/src/bin/wslc_daemon/session_manager.rs index 8d488c9056..807f853387 100644 --- a/src/mxc-sdk/src/bin/wslc_daemon/session_manager.rs +++ b/src/mxc-sdk/src/bin/wslc_daemon/session_manager.rs @@ -1157,7 +1157,7 @@ fn start_session_boot( } .map(BootedSession) .map_err(|failure| BootFailure { - error: sr_err(failure.response), + error: sr_err(*failure.response), store_may_be_held: failure.store_may_be_held, }) } diff --git a/src/mxc-sdk/src/core/mxc_common/config_parser.rs b/src/mxc-sdk/src/core/mxc_common/config_parser.rs index 30daff4910..4d3eaac359 100644 --- a/src/mxc-sdk/src/core/mxc_common/config_parser.rs +++ b/src/mxc-sdk/src/core/mxc_common/config_parser.rs @@ -24,9 +24,10 @@ use crate::mxc_contract::{probe_version, supported_versions, ContractVersion, Ve use std::fs; /// Categorised error from `load_mxc_request`. The `wxc-exec` driver uses the -/// variant to choose the failure-output convention: state-aware failures -/// emit a JSON `{"error": ...}` envelope on stdout, while one-shot and -/// pre-discrimination failures keep the existing diagnostic-on-stderr path. +/// variant to choose where the failure is written: state-aware failures emit a +/// JSON `{"error": ...}` envelope on stdout, because there the envelope *is* +/// the operation's response document, while one-shot and pre-discrimination +/// failures emit theirs on stderr alongside the diagnostics. #[derive(Debug)] pub enum ParseError { /// I/O, base64-decode, or top-level JSON parse failure — the input could @@ -44,6 +45,23 @@ pub enum ParseError { StateAware(MxcError), } +impl ParseError { + /// The typed error this failure is reported to the caller as. + /// + /// Every variant resolves to a code, so a request that never reached a + /// backend is still classified rather than being reported as an + /// unexplained non-zero exit. + pub fn to_mxc_error(&self) -> MxcError { + match self { + ParseError::Decode(error) + | ParseError::Version(error) + | ParseError::OneShot(error) + | ParseError::OneShotMalformed(error) => error.to_mxc_error(), + ParseError::StateAware(error) => error.clone(), + } + } +} + #[derive(Debug, Clone, Copy)] enum ErrorOutput { Primary, diff --git a/src/mxc-sdk/src/core/mxc_common/error.rs b/src/mxc-sdk/src/core/mxc_common/error.rs index e465fded1c..9da7864c06 100644 --- a/src/mxc-sdk/src/core/mxc_common/error.rs +++ b/src/mxc-sdk/src/core/mxc_common/error.rs @@ -3,7 +3,7 @@ use thiserror::Error; -use crate::mxc_common::mxc_error::MxcError; +use crate::mxc_common::mxc_error::{MxcError, MxcErrorCode}; #[derive(Debug, Error)] pub enum WxcError { @@ -59,6 +59,36 @@ impl From for WxcError { } } +impl WxcError { + /// The wire code this failure is reported under, grouped by who can act on + /// it: the request, the host, or neither. + pub fn error_code(&self) -> MxcErrorCode { + match self { + WxcError::StateAware(error) => error.code, + WxcError::ConfigParse(_) | WxcError::StringConversion(_) => { + MxcErrorCode::MalformedRequest + } + WxcError::Validation(_) => MxcErrorCode::PolicyValidation, + WxcError::Process(_) + | WxcError::Io(_) + | WxcError::Firewall(_) + | WxcError::FilesystemPolicy(_) + | WxcError::Initialization(_) + | WxcError::NetworkProxy(_) => MxcErrorCode::BackendError, + } + } + + /// Convert to the typed error carried on the wire, keeping the `Display` + /// text as the message so the variant's prefix survives. A wrapped + /// [`MxcError`] is returned unchanged. + pub fn to_mxc_error(&self) -> MxcError { + match self { + WxcError::StateAware(error) => (**error).clone(), + error => MxcError::new(error.error_code(), error.to_string()), + } + } +} + impl From for WxcError { fn from(err: std::io::Error) -> Self { WxcError::Io(err.to_string()) @@ -71,3 +101,35 @@ pub const DENIED_PATHS_FEATURE_DISABLED_MSG: &str = OS build: it does not advertise PSE_SUPPORT_FS_DENY through \ QueryProcessSecurityEnvironmentSupport. Run on a build with \ BaseContainer deny support or remove filesystem.deniedPaths."; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_refused_policy_is_told_apart_from_a_host_fault() { + assert_eq!( + WxcError::Validation("denied".into()).error_code(), + MxcErrorCode::PolicyValidation + ); + assert_eq!( + WxcError::Io("disk gone".into()).error_code(), + MxcErrorCode::BackendError + ); + } + + #[test] + fn an_unreadable_request_is_malformed() { + assert_eq!( + WxcError::ConfigParse("bad json".into()).error_code(), + MxcErrorCode::MalformedRequest + ); + } + + #[test] + fn conversion_keeps_the_variants_prefix_in_the_message() { + let error = WxcError::Validation("portRange is empty".into()).to_mxc_error(); + assert_eq!(error.code, MxcErrorCode::PolicyValidation); + assert_eq!(error.message, "Validation error: portRange is empty"); + } +} diff --git a/src/mxc-sdk/src/core/mxc_common/filesystem_access.rs b/src/mxc-sdk/src/core/mxc_common/filesystem_access.rs index 46e528b43b..2faa11d176 100644 --- a/src/mxc-sdk/src/core/mxc_common/filesystem_access.rs +++ b/src/mxc-sdk/src/core/mxc_common/filesystem_access.rs @@ -26,6 +26,7 @@ //! **first**, so delegation is checked against the already-tightened intents (a //! path moved `rw → denied` must not then be required to have write access). +use crate::mxc_common::filesystem_object::FilesystemPolicyError; use crate::mxc_common::models::ContainerPolicy; /// The access the invoking user must hold to delegate a path to the sandbox. @@ -55,12 +56,20 @@ impl AccessMode { } } +/// What probing the invoking user's access to a path found. +#[derive(Clone, PartialEq, Eq, Debug)] +enum Access { + Granted, + /// The path does not exist; the existence warning reports it instead. + Missing, + /// The user lacks the access, or the path cannot be named on this host. + Denied, + /// The path could not be examined (e.g. a stale mount or I/O error). + Uninspectable(String), +} + /// Checks whether the invoking user holds the requested access to `path`. /// -/// Returns `Some(true)` / `Some(false)` when the result is determinable, or -/// `None` when it cannot be determined (e.g. the path does not exist — that case -/// is surfaced separately by the existence warning, so delegation skips it). -/// /// On Unix this uses `access(2)`, which checks the requested permissions /// against the process's **real** UID/GID (the invoking user) and covers both /// files and directories. This has full effect for **Bubblewrap**, which runs @@ -77,19 +86,16 @@ impl AccessMode { /// On Windows it probes the caller's effective access with `CreateFileW` /// (requesting `GENERIC_READ` / `GENERIC_READ | GENERIC_WRITE`, opened with /// `FILE_FLAG_BACKUP_SEMANTICS` so directories are covered too). A successful -/// open means the access is granted. Matching the Unix posture, the check -/// **fails closed**: only a genuinely missing path (`ERROR_FILE_NOT_FOUND` / -/// `ERROR_PATH_NOT_FOUND` — surfaced separately by the existence warning) is -/// skipped (`None`); every other failure, including `ERROR_ACCESS_DENIED` and -/// ambiguous errors (sharing/lock violation, transient I/O), is treated as -/// "cannot confirm access" and rejected (`Some(false)`) rather than silently -/// allowed. This covers files *and* directories — including the common WSLC -/// `readwritePaths` directory case — implementing spec D3 for the WSLC backend. +/// open means the access is granted. On both platforms only a missing path is +/// skipped; every other failure is refused. This covers files *and* directories, +/// including the common WSLC `readwritePaths` directory case. #[cfg(unix)] -fn user_can_access(path: &str, mode: AccessMode) -> Option { +fn user_can_access(path: &str, mode: AccessMode) -> Access { use std::ffi::CString; - let c_path = CString::new(path).ok()?; + let Ok(c_path) = CString::new(path) else { + return Access::Denied; + }; let mask = match mode { AccessMode::Read => libc::R_OK, AccessMode::ReadWrite => libc::R_OK | libc::W_OK, @@ -97,26 +103,25 @@ fn user_can_access(path: &str, mode: AccessMode) -> Option { // SAFETY: `c_path` is a valid NUL-terminated C string for the duration of the call. let rc = unsafe { libc::access(c_path.as_ptr(), mask) }; if rc == 0 { - return Some(true); + return Access::Granted; } - // access(2) failed. Distinguish genuine non-existence — which is skipped - // here and surfaced separately by the existence warning — from an access - // denial, which is the exact case delegation must catch (including a parent - // directory the caller cannot traverse, reported as EACCES). Only a missing - // path yields `None`; every other errno is treated as "not accessible". - match std::io::Error::last_os_error().raw_os_error() { - Some(libc::ENOENT) | Some(libc::ENOTDIR) => None, - _ => Some(false), + classify_access_error(std::io::Error::last_os_error()) +} + +/// An untraversable parent reports `EACCES`, so it counts as a denial. +#[cfg(unix)] +fn classify_access_error(error: std::io::Error) -> Access { + match error.raw_os_error() { + Some(libc::ENOENT) | Some(libc::ENOTDIR) => Access::Missing, + Some(libc::EACCES) | Some(libc::EPERM) | Some(libc::EROFS) => Access::Denied, + _ => Access::Uninspectable(error.to_string()), } } #[cfg(windows)] -fn user_can_access(path: &str, mode: AccessMode) -> Option { +fn user_can_access(path: &str, mode: AccessMode) -> Access { use windows::core::PCWSTR; - use windows::Win32::Foundation::{ - CloseHandle, GetLastError, ERROR_FILE_NOT_FOUND, ERROR_PATH_NOT_FOUND, GENERIC_READ, - GENERIC_WRITE, - }; + use windows::Win32::Foundation::{CloseHandle, GetLastError, GENERIC_READ, GENERIC_WRITE}; use windows::Win32::Storage::FileSystem::{ CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_EXISTING, @@ -152,53 +157,66 @@ fn user_can_access(path: &str, mode: AccessMode) -> Option { unsafe { let _ = CloseHandle(h); } - Some(true) - } - _ => { - // Fail closed, mirroring the Unix posture: skip ONLY a genuinely - // missing path (surfaced separately by the existence warning). Every - // other failure — access denied, sharing/lock violation, transient - // I/O — means we cannot confirm the caller has the access, which for - // a security delegation gate must reject rather than silently allow. - // SAFETY: reads the thread-local last error set by the failed call above. - let err = unsafe { GetLastError() }; - if err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND { - None - } else { - Some(false) - } + Access::Granted } + // SAFETY: reads the thread-local last error set by the failed call above. + _ => classify_access_error(unsafe { GetLastError() }), + } +} + +#[cfg(windows)] +fn classify_access_error(error: windows::Win32::Foundation::WIN32_ERROR) -> Access { + use windows::Win32::Foundation::{ + ERROR_ACCESS_DENIED, ERROR_FILE_NOT_FOUND, ERROR_INVALID_NAME, ERROR_PATH_NOT_FOUND, + ERROR_WRITE_PROTECT, + }; + + match error { + ERROR_FILE_NOT_FOUND | ERROR_PATH_NOT_FOUND => Access::Missing, + ERROR_ACCESS_DENIED | ERROR_INVALID_NAME | ERROR_WRITE_PROTECT => Access::Denied, + _ => Access::Uninspectable(std::io::Error::from_raw_os_error(error.0 as i32).to_string()), } } #[cfg(not(any(unix, windows)))] -fn user_can_access(_path: &str, _mode: AccessMode) -> Option { - None +fn user_can_access(_path: &str, _mode: AccessMode) -> Access { + Access::Missing } /// Validates the delegation constraint (spec D3): the sandbox receives no more /// access than the invoking user holds. `readwritePaths` require the user to /// have read+write access and `readonlyPaths` require read access; `deniedPaths` -/// are unbounded and not checked. Paths whose access cannot be determined (e.g. -/// non-existent paths) are skipped rather than rejected. +/// are unbounded and not checked. Non-existent paths are skipped. /// -/// Returns the rejection message for the first path that fails, or `Ok(())` when -/// every checkable path is within the caller's access. Callers surface the -/// message as their backend-appropriate error (e.g. `ScriptResponse::error`). -pub fn check_delegation(policy: &ContainerPolicy) -> Result<(), String> { +/// Fails closed on the first path that fails: a path the user cannot access is +/// [`FilesystemPolicyError::Policy`], and a path that could not be examined is +/// [`FilesystemPolicyError::Host`]. +pub fn check_delegation(policy: &ContainerPolicy) -> Result<(), FilesystemPolicyError> { for (paths, mode) in [ (&policy.readonly_paths, AccessMode::Read), (&policy.readwrite_paths, AccessMode::ReadWrite), ] { for path in paths { - if user_can_access(path, mode) == Some(false) { - return Err(format!( - "Filesystem path '{}' ({}): the invoking user does not have {} access, \ - so it cannot be delegated to the sandbox", - path, - mode.list_name(), - mode.access_phrase(), - )); + match user_can_access(path, mode) { + Access::Granted | Access::Missing => {} + Access::Denied => { + return Err(FilesystemPolicyError::Policy(format!( + "Filesystem path '{}' ({}): the invoking user does not have {} access, \ + so it cannot be delegated to the sandbox", + path, + mode.list_name(), + mode.access_phrase(), + ))); + } + Access::Uninspectable(reason) => { + return Err(FilesystemPolicyError::Host(format!( + "Filesystem path '{}' ({}): could not check the invoking user's {} \ + access ({reason}), so it was not delegated to the sandbox", + path, + mode.list_name(), + mode.access_phrase(), + ))); + } } } } @@ -224,8 +242,8 @@ mod tests { std::fs::write(&file, b"test").unwrap(); let f = file.to_str().unwrap(); - assert_eq!(user_can_access(f, AccessMode::Read), Some(true)); - assert_eq!(user_can_access(f, AccessMode::ReadWrite), Some(true)); + assert_eq!(user_can_access(f, AccessMode::Read), Access::Granted); + assert_eq!(user_can_access(f, AccessMode::ReadWrite), Access::Granted); assert!(check_delegation(&policy(&[f], &[])).is_ok()); } @@ -236,8 +254,8 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let d = dir.path().to_str().unwrap(); - assert_eq!(user_can_access(d, AccessMode::Read), Some(true)); - assert_eq!(user_can_access(d, AccessMode::ReadWrite), Some(true)); + assert_eq!(user_can_access(d, AccessMode::Read), Access::Granted); + assert_eq!(user_can_access(d, AccessMode::ReadWrite), Access::Granted); assert!(check_delegation(&policy(&[d], &[])).is_ok()); } @@ -247,7 +265,7 @@ mod tests { // (existence is surfaced separately as a warning, not a delegation error). assert_eq!( user_can_access("/definitely/not/here/mxc-xyz", AccessMode::Read), - None + Access::Missing ); assert!(check_delegation(&policy(&["/definitely/not/here/mxc-xyz"], &[])).is_ok()); } @@ -259,16 +277,34 @@ mod tests { #[cfg(windows)] #[test] - fn ambiguous_error_fails_closed() { - // A path Windows rejects for a reason OTHER than not-found or - // access-denied (here an invalid name containing an illegal `<` - // character → ERROR_INVALID_NAME) must fail closed — `Some(false)` → - // rejected — not be silently skipped. This locks the Unix/Windows - // posture alignment: only a genuinely-missing path is skipped; every - // other failure is treated as "cannot confirm access" and rejected. + fn an_invalid_path_name_is_refused_as_a_policy_error() { let invalid = "C:\\mxc_invalid &str { + match self { + Self::Policy(message) | Self::Host(message) => message, + } + } + + // SDK v2: Policy -> rejected (policy_validation), Host -> failed(LaunchFailed). + // See docs/development/plans/sdk-v2-error-codes.md. + pub fn into_response(self) -> ScriptResponse { + ScriptResponse::error(self.message()) + } +} + /// Result of comparing two paths by filesystem-object identity. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum ExistingObjectComparison { @@ -128,6 +154,7 @@ fn resolve_object(path: &Path) -> PathResolution { // could not examine the path. Err(e) => match e.raw_os_error() { Some(libc::ENOENT) | Some(libc::ENOTDIR) => PathResolution::Absent, + Some(libc::EACCES) | Some(libc::EPERM) => PathResolution::Inaccessible, _ => PathResolution::Unknown, }, } @@ -138,7 +165,7 @@ fn resolve_object(path: &Path) -> PathResolution { use std::os::windows::ffi::OsStrExt; use windows::core::PCWSTR; use windows::Win32::Foundation::{ - CloseHandle, GetLastError, ERROR_FILE_NOT_FOUND, ERROR_PATH_NOT_FOUND, + CloseHandle, GetLastError, ERROR_ACCESS_DENIED, ERROR_FILE_NOT_FOUND, ERROR_PATH_NOT_FOUND, }; use windows::Win32::Storage::FileSystem::{ CreateFileW, FileIdInfo, GetFileInformationByHandleEx, FILE_FLAG_BACKUP_SEMANTICS, @@ -178,6 +205,8 @@ fn resolve_object(path: &Path) -> PathResolution { let err = unsafe { GetLastError() }; return if err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND { classify_not_found_leaf_windows(path) + } else if err == ERROR_ACCESS_DENIED { + PathResolution::Inaccessible } else { PathResolution::Unknown }; @@ -253,7 +282,8 @@ fn compare_resolved_filesystem_objects( // Unknown must take precedence over Absent on every platform. Treating // an unexaminable path as merely different would let callers proceed // without proving that the policy paths refer to distinct objects. - (PathResolution::Unknown, _) | (_, PathResolution::Unknown) => { + (PathResolution::Unknown | PathResolution::Inaccessible, _) + | (_, PathResolution::Unknown | PathResolution::Inaccessible) => { ExistingObjectComparison::Unknown } (PathResolution::Absent, _) | (_, PathResolution::Absent) => { @@ -277,18 +307,17 @@ fn compare_resolved_filesystem_objects( /// - `Ok(None)` when there is nothing to change (the common case — no /// symlink / hard-link / bind-mount aliases), so callers can avoid cloning the /// request entirely. -/// - `Err(message)` when the config must be **rejected**: a path could not be -/// resolved to an object (permission denied, unreachable mount, I/O error — -/// *not* cleanly missing) while `deniedPaths` are present, so an alias that -/// would bypass a deny cannot be ruled out (fail closed). The message is -/// suitable for surfacing as a backend error. +/// - `Err(_)` when the run must be **refused**: a path could not be resolved to +/// an object (permission denied, unreachable mount, I/O error — *not* cleanly +/// missing) while `deniedPaths` are present, so an alias that would bypass a +/// deny cannot be ruled out (fail closed). /// /// Run this *after* the string-level normalization in `config_parser`, close to /// where the backend builds its mounts. pub fn normalize_object_conflicts( policy: &ContainerPolicy, logger: &mut Logger, -) -> Result, String> { +) -> Result, FilesystemPolicyError> { let total = policy.readwrite_paths.len() + policy.readonly_paths.len() + policy.denied_paths.len(); // A conflict needs at least two paths resolving to the same object, so a @@ -314,10 +343,11 @@ pub fn normalize_object_conflicts( // Group entry indices by object identity. Resolution has three outcomes: // - `Object`: grouped for conflict detection. // - `Absent` (cleanly missing): skipped — nothing to alias. - // - `Unknown` (exists/maybe-exists but unexaminable): when `deniedPaths` are - // present we cannot prove this path is not an undetectable alias that would - // bypass a deny, so we FAIL CLOSED and reject. Without `deniedPaths` there - // is no deny to bypass, so we log it and leave it in place. + // - `Inaccessible` / `Unknown` (exists/maybe-exists but unexaminable): when + // `deniedPaths` are present we cannot prove this path is not an + // undetectable alias that would bypass a deny, so we FAIL CLOSED. Without + // `deniedPaths` there is no deny to bypass, so we log it and leave it in + // place. let has_denied = !policy.denied_paths.is_empty(); let mut groups: HashMap> = HashMap::new(); for (i, (path, intent)) in entries.iter().enumerate() { @@ -326,18 +356,23 @@ pub fn normalize_object_conflicts( groups.entry(id).or_default().push(i); } PathResolution::Absent => {} + PathResolution::Inaccessible if has_denied => { + return Err(FilesystemPolicyError::Policy(unresolvable_message( + path, + *intent, + "permission denied", + "Grant access to the path, or remove the offending entry.", + ))); + } PathResolution::Unknown if has_denied => { - return Err(format!( - "Filesystem path '{}' ({}) could not be resolved to a filesystem object \ - (permission denied, unreachable mount, or I/O error). Because deniedPaths \ - are present, MXC cannot verify it does not alias a denied object and rejects \ - the config rather than risk bypassing the deny. Ensure the path is reachable, \ - or remove the offending entry.", + return Err(FilesystemPolicyError::Host(unresolvable_message( path, - intent.list_name(), - )); + *intent, + "unreachable mount or I/O error", + "Retry once the path is reachable, or remove the offending entry.", + ))); } - PathResolution::Unknown => { + PathResolution::Inaccessible | PathResolution::Unknown => { logger.log_line(&format!( "WARNING: filesystem path '{}' ({}) could not be resolved to a filesystem \ object and was not checked for aliasing", @@ -430,6 +465,15 @@ pub fn normalize_object_conflicts( Ok(Some(new_policy)) } +fn unresolvable_message(path: &str, intent: Intent, cause: &str, remedy: &str) -> String { + format!( + "Filesystem path '{path}' ({}) could not be resolved to a filesystem object ({cause}). \ + Because deniedPaths are present, MXC cannot verify it does not alias a denied object \ + and refuses to run rather than risk bypassing the deny. {remedy}", + intent.list_name(), + ) +} + #[cfg(test)] mod tests { use super::*; @@ -675,8 +719,9 @@ mod tests { let err = normalize_object_conflicts(&p, &mut logger) .expect_err("an unexaminable path with deniedPaths present must fail closed"); assert!( - err.contains("could not be resolved") && err.contains("deniedPaths"), - "expected a fail-closed rejection message, got: {err}" + matches!(&err, FilesystemPolicyError::Policy(message) + if message.contains("could not be resolved") && message.contains("deniedPaths")), + "expected a fail-closed policy rejection, got: {err:?}" ); // Restore traversal so the tempdir can recurse in during cleanup. @@ -742,8 +787,9 @@ mod tests { let err = normalize_object_conflicts(&p, &mut logger) .expect_err("an unexaminable path with deniedPaths present must fail closed"); assert!( - err.contains("could not be resolved") && err.contains("deniedPaths"), - "expected a fail-closed rejection message, got: {err}" + matches!(&err, FilesystemPolicyError::Policy(message) + if message.contains("could not be resolved") && message.contains("deniedPaths")), + "expected a fail-closed policy rejection, got: {err:?}" ); // Remove the deny ACE so the tempdir can clean up. diff --git a/src/mxc-sdk/src/core/mxc_common/filesystem_symlink.rs b/src/mxc-sdk/src/core/mxc_common/filesystem_symlink.rs index b0086c60b7..ba915fc81a 100644 --- a/src/mxc-sdk/src/core/mxc_common/filesystem_symlink.rs +++ b/src/mxc-sdk/src/core/mxc_common/filesystem_symlink.rs @@ -88,10 +88,36 @@ pub(crate) fn resolve_through_symlinks(path: &Path) -> Option } } +/// Whether a resolution error is a fault in the policy path itself (missing, +/// loop, not a directory, inaccessible) rather than a host I/O failure. +pub(crate) fn is_policy_path_error(error: &std::io::Error) -> bool { + use std::io::ErrorKind; + + matches!( + error.kind(), + ErrorKind::NotFound + | ErrorKind::NotADirectory + | ErrorKind::PermissionDenied + | ErrorKind::InvalidFilename + ) || error.raw_os_error() == Some(libc::ELOOP) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn only_path_problems_blame_the_request() { + for errno in [libc::ENOENT, libc::ENOTDIR, libc::ELOOP, libc::EACCES] { + let error = std::io::Error::from_raw_os_error(errno); + assert!(is_policy_path_error(&error), "{error}"); + } + for errno in [libc::EIO, libc::ESTALE] { + let error = std::io::Error::from_raw_os_error(errno); + assert!(!is_policy_path_error(&error), "{error}"); + } + } + /// Canonicalized up front so a symlinked `TMPDIR` does not make every /// expectation below differ from what the walk returns. fn temp_root(dir: &tempfile::TempDir) -> PathBuf { diff --git a/src/mxc-sdk/src/core/mxc_common/models.rs b/src/mxc-sdk/src/core/mxc_common/models.rs index d9524696af..66524216c7 100644 --- a/src/mxc-sdk/src/core/mxc_common/models.rs +++ b/src/mxc-sdk/src/core/mxc_common/models.rs @@ -1110,9 +1110,8 @@ pub enum FailurePhase { /// the VM/sandbox bring-up, or a transient resource contention (e.g. a /// single-instance backend already running). Generally worth retrying. LaunchFailed, - /// The request cannot be honored and will not succeed on a blind retry - /// without changing the input or host: a policy rejection, or a missing - /// host prerequisite (backend/runtime not installed). + /// The request itself is refusable: the policy asks for something the + /// selected backend cannot enforce. Only a changed request can succeed. Rejected, /// The launch command succeeded but the guest/sandbox infrastructure failed /// before or while running user code (agent rendezvous, channel connect, or @@ -1125,26 +1124,38 @@ pub enum FailurePhase { /// can detect a timeout uniformly across backends rather than inferring it /// from `exit_code == -1` (which collides with other failures). Timeout, - /// The selected containment backend is unavailable on this host: the API is - /// missing, or present but not usable (e.g. feature-disabled). Distinct from - /// [`LaunchFailed`] so callers can fall back to a lower tier rather than - /// hard-fail. + /// The host cannot provide the selected containment backend at all: the API + /// is missing, the runtime is not installed, or an OS feature it depends on + /// is disabled. No change to the request can satisfy it — the host has to + /// be provisioned, or a different backend selected. Distinct from + /// [`LaunchFailed`], which is a backend that is present and failed. BackendUnavailable, } impl FailurePhase { /// The wire error code a failure in this phase is reported as. /// - /// [`Rejected`](FailurePhase::Rejected) is the one phase a caller can act on - /// by changing the request, so it is the only one that earns a code of its - /// own; every other failure is an infrastructure problem the caller cannot - /// distinguish usefully and stays `backend_error`. + /// Two phases earn a code of their own because they tell the caller which + /// side is at fault: [`Rejected`](FailurePhase::Rejected) blames the + /// request, and [`BackendUnavailable`](FailurePhase::BackendUnavailable) + /// blames the host. Every other failure is a runtime fault the caller + /// cannot distinguish usefully and stays `backend_error`. pub fn error_code(&self) -> MxcErrorCode { match self { FailurePhase::Rejected => MxcErrorCode::PolicyValidation, + FailurePhase::BackendUnavailable => MxcErrorCode::BackendUnavailable, _ => MxcErrorCode::BackendError, } } + + /// The process exit status a failure in this phase terminates with. + /// + /// Derived from [`error_code`](FailurePhase::error_code) so a response and + /// an entry-point failure carrying the same code always exit alike: a + /// refusal exits 1, and a host or runtime fault exits -1. + pub fn failure_exit_code(&self) -> i32 { + self.error_code().exit_code() + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -1162,6 +1173,17 @@ pub struct ScriptResponse { /// Indicates at what phase the failure occurred. #[serde(default)] pub failure_phase: FailurePhase, + /// Non-fatal diagnostics raised while building or running the sandbox, + /// carried on the response so a programmatic caller can filter them + /// instead of scraping stderr. + /// + /// Boxed, like `output_metadata`, to keep `ScriptResponse` small enough to + /// serve as a `Result` error type. Reach for it through + /// [`warnings`](ScriptResponse::warnings) and + /// [`extend_warnings`](ScriptResponse::extend_warnings) rather than the + /// field, so the "absent" and "empty" spellings cannot drift apart. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub warnings: Option>>, /// Structured metadata produced after the sandboxed process exits. #[serde(default, skip_serializing_if = "Option::is_none")] pub output_metadata: Option>, @@ -1176,6 +1198,7 @@ impl Default for ScriptResponse { error_message: String::new(), extended_error: String::new(), failure_phase: FailurePhase::None, + warnings: None, output_metadata: None, } } @@ -1250,13 +1273,47 @@ impl ScriptResponse { /// copying it would print the message bare and then again inside the JSON /// envelope. pub fn rejected(msg: &str) -> Self { + Self::failed(FailurePhase::Rejected, msg) + } + + /// Create a response for a backend this host cannot provide. + /// + /// Reports `backend_unavailable`, which tells the caller the host is at + /// fault: no change to the request will satisfy it. Like + /// [`ScriptResponse::rejected`] nothing ran, so the message is left to the + /// envelope rather than copied into `standard_err`. + pub fn unavailable(msg: &str) -> Self { + Self::failed(FailurePhase::BackendUnavailable, msg) + } + + /// Create a response for a failure attributed to `phase`. + /// + /// The exit code follows the phase, and the message is left to the error + /// envelope rather than copied into `standard_err`. + pub fn failed(phase: FailurePhase, msg: &str) -> Self { ScriptResponse { - exit_code: 1, + exit_code: phase.failure_exit_code(), error_message: msg.to_string(), - failure_phase: FailurePhase::Rejected, + failure_phase: phase, ..Default::default() } } + + /// The non-fatal diagnostics this run recorded. + pub fn warnings(&self) -> &[String] { + self.warnings.as_deref().map_or(&[], Vec::as_slice) + } + + /// Append diagnostics, allocating only once there is something to record. + pub fn extend_warnings(&mut self, warnings: impl IntoIterator) { + let mut incoming = warnings.into_iter().peekable(); + if incoming.peek().is_none() { + return; + } + self.warnings + .get_or_insert_with(Box::default) + .extend(incoming); + } } #[cfg(test)] @@ -1264,18 +1321,21 @@ mod tests { use super::*; #[test] - fn only_rejection_earns_a_caller_actionable_error_code() { + fn each_caller_actionable_phase_earns_its_own_error_code() { assert_eq!( FailurePhase::Rejected.error_code(), MxcErrorCode::PolicyValidation ); + assert_eq!( + FailurePhase::BackendUnavailable.error_code(), + MxcErrorCode::BackendUnavailable + ); for phase in [ FailurePhase::None, FailurePhase::LaunchFailed, FailurePhase::PostLaunchFailed, FailurePhase::ProcessExited, FailurePhase::Timeout, - FailurePhase::BackendUnavailable, ] { assert_eq!( phase.error_code(), @@ -1285,6 +1345,63 @@ mod tests { } } + #[test] + fn only_a_refusal_exits_one() { + assert_eq!(FailurePhase::Rejected.failure_exit_code(), 1); + for phase in [ + FailurePhase::None, + FailurePhase::LaunchFailed, + FailurePhase::PostLaunchFailed, + FailurePhase::ProcessExited, + FailurePhase::Timeout, + FailurePhase::BackendUnavailable, + ] { + assert_eq!(phase.failure_exit_code(), -1, "phase {phase:?}"); + } + } + + #[test] + fn an_unavailable_host_is_told_apart_from_a_refusal_and_a_crash() { + let unavailable = ScriptResponse::unavailable("WSL is not installed"); + + assert_eq!(unavailable.exit_code, -1); + assert_eq!( + unavailable.failure_phase.error_code(), + MxcErrorCode::BackendUnavailable + ); + assert!(unavailable.standard_err.is_empty()); + } + + #[test] + fn warnings_are_omitted_from_the_wire_when_there_are_none() { + let quiet = serde_json::to_string(&ScriptResponse::default()).unwrap(); + assert!(!quiet.contains("warnings"), "{quiet}"); + + let mut noisy = ScriptResponse::default(); + noisy.extend_warnings(["dacl augmentation fell back".to_string()]); + let noisy = serde_json::to_string(&noisy).unwrap(); + assert!(noisy.contains("dacl augmentation fell back"), "{noisy}"); + } + + #[test] + fn recording_nothing_allocates_nothing() { + let mut response = ScriptResponse::default(); + response.extend_warnings(Vec::new()); + assert!(response.warnings.is_none()); + assert!(response.warnings().is_empty()); + } + + // `ScriptResponse` is used as a `Result` error type, so it has to stay + // under clippy's `result_large_err` threshold. + #[test] + fn the_response_stays_small_enough_to_be_an_error_type() { + assert!( + std::mem::size_of::() <= 128, + "{} bytes", + std::mem::size_of::() + ); + } + #[test] fn rejection_exits_one_and_leaves_stderr_to_the_envelope() { let rejected = ScriptResponse::rejected("policy not supported"); @@ -1297,6 +1414,16 @@ mod tests { assert!(rejected.standard_err.is_empty()); } + #[test] + fn a_phased_failure_takes_its_exit_code_from_the_phase() { + let launch = ScriptResponse::failed(FailurePhase::LaunchFailed, "spawn failed"); + + assert_eq!(launch.exit_code, -1); + assert_eq!(launch.failure_phase, FailurePhase::LaunchFailed); + assert_eq!(launch.error_message, "spawn failed"); + assert!(launch.standard_err.is_empty()); + } + #[test] fn sandbox_paths_normalize_against_the_root() { for (input, expected) in [ diff --git a/src/mxc-sdk/src/core/mxc_common/mxc_error.rs b/src/mxc-sdk/src/core/mxc_common/mxc_error.rs index 756e65df2d..928ee11c28 100644 --- a/src/mxc-sdk/src/core/mxc_common/mxc_error.rs +++ b/src/mxc-sdk/src/core/mxc_common/mxc_error.rs @@ -49,6 +49,28 @@ impl MxcErrorCode { Self::BackendError => "backend_error", } } + + /// The process exit status a failure with this code terminates with. + /// + /// Every caller-fault code exits 1: the request asked for something that + /// will not be done, and only a changed request can succeed. A host fault + /// (`backend_unavailable`) and a runtime fault (`backend_error`) exit -1, + /// and are told apart by the code rather than by the status. + pub fn exit_code(self) -> i32 { + match self { + Self::BackendUnavailable | Self::BackendError => -1, + Self::MalformedRequest + | Self::UnsupportedContainment + | Self::UnsupportedPhase + | Self::MalformedId + | Self::StaleId + | Self::NotProvisioned + | Self::NotStarted + | Self::AlreadyStarted + | Self::AlreadyStopped + | Self::PolicyValidation => 1, + } + } } impl std::fmt::Display for MxcErrorCode { @@ -187,6 +209,18 @@ impl MxcError { self.api_failure.as_ref().map(|f| f.operation.as_str()) } + /// Serialise to the JSON response envelope callers parse. + /// + /// Falls back to a minimal hand-written envelope if serialisation itself + /// fails, so a consumer that requires `error.code` still parses something. + pub fn to_envelope_string(&self) -> String { + let envelope: ResponseEnvelope<()> = ResponseEnvelope::from_error(self); + serde_json::to_string(&envelope).unwrap_or_else(|_| { + r#"{"error":{"code":"backend_error","message":"failed to serialise error envelope"}}"# + .to_string() + }) + } + /// The underlying platform status, when known. Never present without /// [`MxcError::operation`]. pub fn native_code(&self) -> Option<&str> { @@ -322,6 +356,27 @@ mod tests { } } + #[test] + fn only_host_and_runtime_faults_exit_minus_one() { + let cases = [ + (MxcErrorCode::MalformedRequest, 1), + (MxcErrorCode::UnsupportedContainment, 1), + (MxcErrorCode::UnsupportedPhase, 1), + (MxcErrorCode::BackendUnavailable, -1), + (MxcErrorCode::MalformedId, 1), + (MxcErrorCode::StaleId, 1), + (MxcErrorCode::NotProvisioned, 1), + (MxcErrorCode::NotStarted, 1), + (MxcErrorCode::AlreadyStarted, 1), + (MxcErrorCode::AlreadyStopped, 1), + (MxcErrorCode::PolicyValidation, 1), + (MxcErrorCode::BackendError, -1), + ]; + for (code, exit) in cases { + assert_eq!(code.exit_code(), exit, "{code}"); + } + } + #[test] fn convenience_constructors_set_correct_codes() { assert_eq!( diff --git a/src/mxc-sdk/src/core/mxc_common/script_runner.rs b/src/mxc-sdk/src/core/mxc_common/script_runner.rs index 822740959d..2dec092185 100644 --- a/src/mxc-sdk/src/core/mxc_common/script_runner.rs +++ b/src/mxc-sdk/src/core/mxc_common/script_runner.rs @@ -128,10 +128,12 @@ fn dry_run_summary(response: &ScriptResponse) -> String { } /// Print a dry-run result message to the logger, flush, and exit the process. +/// A failed dry run also emits its typed error envelope on stderr. pub fn handle_dry_run_exit(response: &ScriptResponse, logger: &mut Logger) -> ! { use std::fmt::Write; let _ = writeln!(logger, "{}", dry_run_summary(response)); print!("{}", logger.get_buffer()); + emit_backend_error_envelope(response); std::process::exit(response.exit_code); } @@ -150,11 +152,20 @@ pub fn handle_dry_run_exit(response: &ScriptResponse, logger: &mut Logger) -> ! /// /// The `code` comes from the response's /// [`FailurePhase`](crate::mxc_common::models::FailurePhase), so a refused policy is -/// reported as `policy_validation` rather than being flattened into the generic -/// `backend_error` every other failure uses. +/// reported as `policy_validation` and a host that cannot provide the backend +/// as `backend_unavailable`, rather than both being flattened into the generic +/// `backend_error` every other failure uses. The envelope starts on a fresh line +/// because the workload's stderr may not end in a newline. pub fn emit_backend_error_envelope(response: &ScriptResponse) { + if let Some(line) = backend_error_envelope_line(response) { + eprintln!("\n{line}"); + } +} + +/// The stderr line [`emit_backend_error_envelope`] writes, if any. +fn backend_error_envelope_line(response: &ScriptResponse) -> Option { if response.exit_code == 0 || response.error_message.is_empty() { - return; + return None; } let mut envelope = serde_json::json!({ @@ -167,11 +178,48 @@ pub fn emit_backend_error_envelope(response: &ScriptResponse) { envelope["error"]["extended_error"] = serde_json::Value::String(response.extended_error.clone()); } - if let Ok(json) = serde_json::to_string(&envelope) { - eprintln!("{json}"); + serde_json::to_string(&envelope).ok() +} + +/// The stderr lines a run's warnings are emitted as, one JSON object each. +/// +/// Kept separate from [`emit_warning_envelopes`] so the exact bytes a caller +/// has to parse are testable without capturing the process's stderr. +fn warning_envelope_lines(warnings: &[String]) -> Vec { + warnings + .iter() + .filter(|warning| !warning.trim().is_empty()) + .filter_map(|warning| { + serde_json::to_string(&serde_json::json!({ "warning": { "message": warning } })).ok() + }) + .collect() +} + +/// Emit each warning as its own structured JSON line on stderr. +/// +/// Warnings are reported on success as well as on failure, so this is separate +/// from [`emit_backend_error_envelope`] rather than folded into it. One object +/// per line keeps the stream parseable a line at a time, matching the error +/// envelope. The batch starts on a fresh line because the workload's stderr may +/// not end in a newline. +pub fn emit_warning_envelopes(warnings: &[String]) { + let lines = warning_envelope_lines(warnings); + if !lines.is_empty() { + eprintln!("\n{}", lines.join("\n")); } } +/// Emit a typed JSON error envelope on stderr for a failure raised before any +/// [`ScriptResponse`] exists, then exit with the status its code implies. +/// +/// Shared by every executor entry point. Without it such a failure prints a +/// bare line and exits 1, which a caller cannot tell from a typed policy +/// rejection. +pub fn emit_mxc_error_exit(error: &MxcError) -> ! { + eprintln!("{}", error.to_envelope_string()); + std::process::exit(error.code.exit_code()); +} + #[cfg(test)] mod tests { use super::get_timeout_milliseconds; @@ -193,28 +241,73 @@ mod tests { fn error_envelope_is_noop_without_error() { use crate::mxc_common::models::ScriptResponse; // exit 0 => no-op; non-zero but empty message (clean sandbox exit) => no-op. - super::emit_backend_error_envelope(&ScriptResponse { + assert!(super::backend_error_envelope_line(&ScriptResponse { exit_code: 0, error_message: "ignored on success".to_string(), ..Default::default() - }); - super::emit_backend_error_envelope(&ScriptResponse { + }) + .is_none()); + assert!(super::backend_error_envelope_line(&ScriptResponse { exit_code: 1, error_message: String::new(), ..Default::default() - }); + }) + .is_none()); } #[test] - fn error_envelope_emits_on_infra_failure() { + fn error_envelope_carries_the_code_for_each_failure_kind() { use crate::mxc_common::models::ScriptResponse; - // Exercises the serialization branch (writes to stderr); must not panic. - super::emit_backend_error_envelope(&ScriptResponse { - exit_code: 1, - error_message: "backend unavailable".to_string(), + let cases = [ + (ScriptResponse::rejected("refused"), "policy_validation"), + ( + ScriptResponse::unavailable("missing"), + "backend_unavailable", + ), + (ScriptResponse::error("broke"), "backend_error"), + ]; + for (response, code) in cases { + let line = super::backend_error_envelope_line(&response).expect(code); + let parsed: serde_json::Value = serde_json::from_str(&line).unwrap(); + assert_eq!(parsed["error"]["code"], code, "{line}"); + assert_eq!(parsed["error"]["message"], response.error_message); + } + } + + #[test] + fn error_envelope_includes_extended_error_when_present() { + use crate::mxc_common::models::ScriptResponse; + let line = super::backend_error_envelope_line(&ScriptResponse { extended_error: "WIN32_ERROR(1920)".to_string(), - ..Default::default() - }); + ..ScriptResponse::unavailable("backend unavailable") + }) + .unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&line).unwrap(); + assert_eq!(parsed["error"]["extended_error"], "WIN32_ERROR(1920)"); + } + + #[test] + fn warning_lines_are_one_parseable_object_each() { + let lines = super::warning_envelope_lines(&[ + "dacl augmentation fell back".to_string(), + "quoted \"name\" and \\ backslash".to_string(), + ]); + + assert_eq!( + lines[0], + r#"{"warning":{"message":"dacl augmentation fell back"}}"# + ); + // A warning is free-form text, so it has to survive JSON escaping. + let parsed: serde_json::Value = serde_json::from_str(&lines[1]).unwrap(); + assert_eq!( + parsed["warning"]["message"], + "quoted \"name\" and \\ backslash" + ); + } + + #[test] + fn blank_warnings_do_not_reach_the_stream() { + assert!(super::warning_envelope_lines(&[String::new(), " ".to_string()]).is_empty()); } #[test] diff --git a/src/mxc-sdk/src/core/mxc_common/validator.rs b/src/mxc-sdk/src/core/mxc_common/validator.rs index b3b359d17b..8be4aef722 100644 --- a/src/mxc-sdk/src/core/mxc_common/validator.rs +++ b/src/mxc-sdk/src/core/mxc_common/validator.rs @@ -147,12 +147,14 @@ pub fn validate_state_aware_network_policy_support( /// Validates non-backend-specific parts of the request (e.g. non-empty script). pub fn validate_common(request: &ExecutionRequest) -> Result<(), ScriptResponse> { if request.script_code.is_empty() { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error("Script content must not be empty.")); } if !request.policy.enumerate_paths.is_empty() && request.containment != crate::mxc_common::models::ContainmentBackend::ProcessContainer { + // SDK v2: rejected (policy_validation). See docs/development/plans/sdk-v2-error-codes.md. return Err(ScriptResponse::error( "processContainer.filesystem.enumeratePaths is supported only by the Windows \ ProcessContainer backend", diff --git a/src/mxc-sdk/src/core/mxc_engine/dispatch.rs b/src/mxc-sdk/src/core/mxc_engine/dispatch.rs index 0c2abe82e6..290e282d8a 100644 --- a/src/mxc-sdk/src/core/mxc_engine/dispatch.rs +++ b/src/mxc-sdk/src/core/mxc_engine/dispatch.rs @@ -280,8 +280,7 @@ fn spawn_process_container( use crate::mxc_common::sandbox_process::{SandboxBackend, StdioMode}; use crate::process_container_common::base_container_runner::BaseContainerRunner; - let mut backend = - BaseContainerRunner::for_request(request).map_err(MxcError::backend_unavailable)?; + let mut backend = BaseContainerRunner::for_request(request)?; backend .spawn(request, logger, StdioMode::Pipes) .map_err(map_spawn_error) @@ -325,6 +324,7 @@ fn spawn_wslc( ) -> Result, MxcError> { #[cfg(target_os = "windows")] { + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( "WSLC backend not compiled. Rebuild with --features wslc.", )) @@ -379,6 +379,7 @@ fn spawn_isolation_session( ) -> Result, MxcError> { #[cfg(target_os = "windows")] { + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( "IsolationSession backend not compiled. Rebuild with --features isolation_session.", )) @@ -399,6 +400,7 @@ fn spawn_isolation_session_pty( ) -> Result, MxcError> { #[cfg(target_os = "windows")] { + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( "IsolationSession backend not compiled. Rebuild with --features isolation_session.", )) @@ -561,6 +563,19 @@ mod tests { assert!(err.message.contains("Windows"), "got: {}", err.message); } + #[cfg(all(target_os = "windows", not(feature = "wslc")))] + #[test] + fn streaming_matches_run_to_completion_for_a_backend_this_build_omits() { + let mut request = minimal_request(); + request.containment = ContainmentBackend::Wslc; + let mut logger = Logger::new(Mode::Buffer); + let err = match spawn_runner(&request, &mut logger, false) { + Ok(_) => panic!("a backend this build omits must not spawn"), + Err(e) => e, + }; + assert_eq!(err.code, MxcErrorCode::UnsupportedContainment); + } + #[cfg(not(target_os = "linux"))] #[test] fn streaming_rejects_lxc_off_linux() { diff --git a/src/mxc-sdk/src/core/mxc_engine/error.rs b/src/mxc-sdk/src/core/mxc_engine/error.rs index 9135ecb224..bfcb3e9043 100644 --- a/src/mxc-sdk/src/core/mxc_engine/error.rs +++ b/src/mxc-sdk/src/core/mxc_engine/error.rs @@ -4,7 +4,7 @@ //! The SDK's own error type — a crate-owned facade over the internal //! `mxc_common` error, so the public API never exposes the foundation crate. -use crate::mxc_common::mxc_error::{MxcError, MxcErrorCode}; +use crate::mxc_common::mxc_error::{ApiFailure, MxcError, MxcErrorCode}; /// Error codes returned by the SDK, corresponding one-for-one to the /// snake_case codes reported by MXC. @@ -50,6 +50,26 @@ impl std::fmt::Display for ErrorCode { } } +impl ErrorCode { + /// The internal code this facade code stands for. + fn to_internal(self) -> MxcErrorCode { + match self { + Self::MalformedRequest => MxcErrorCode::MalformedRequest, + Self::UnsupportedContainment => MxcErrorCode::UnsupportedContainment, + Self::UnsupportedPhase => MxcErrorCode::UnsupportedPhase, + Self::BackendUnavailable => MxcErrorCode::BackendUnavailable, + Self::MalformedId => MxcErrorCode::MalformedId, + Self::StaleId => MxcErrorCode::StaleId, + Self::NotProvisioned => MxcErrorCode::NotProvisioned, + Self::NotStarted => MxcErrorCode::NotStarted, + Self::AlreadyStarted => MxcErrorCode::AlreadyStarted, + Self::AlreadyStopped => MxcErrorCode::AlreadyStopped, + Self::PolicyValidation => MxcErrorCode::PolicyValidation, + Self::BackendError => MxcErrorCode::BackendError, + } + } +} + impl From for ErrorCode { fn from(code: MxcErrorCode) -> Self { match code { @@ -147,11 +167,36 @@ impl From for Error { } } +impl Error { + /// Recover the internal error this facade error stands for, so an + /// in-repo binary can emit the typed envelope and exit status its code + /// implies. + /// + /// Deliberately an inherent method rather than `impl From for + /// MxcError`: a second `MxcError: From<_>` impl makes `?` ambiguous + /// wherever the error type is inferred. + /// + /// `MxcError::details` has no facade counterpart and does not survive the + /// round trip. + #[doc(hidden)] + pub fn into_mxc_error(self) -> MxcError { + let mut error = MxcError::new(self.code.to_internal(), self.message); + if let Some(operation) = self.operation { + let mut failure = ApiFailure::new(operation); + failure.native_code = self.native_code; + error = error.with_api_failure(failure); + } + if let Some(remediation) = self.remediation { + error = error.with_remediation(remediation); + } + error + } +} + #[cfg(test)] mod tests { use super::*; use crate::mxc_common::mxc_error::{ApiFailure as InnerFailure, MxcError}; - /// The conversion carries the API detail across, rather than keeping only /// the code and message. /// diff --git a/src/mxc-sdk/src/core/mxc_engine/run.rs b/src/mxc-sdk/src/core/mxc_engine/run.rs index ceceacc1db..03081f93cd 100644 --- a/src/mxc-sdk/src/core/mxc_engine/run.rs +++ b/src/mxc-sdk/src/core/mxc_engine/run.rs @@ -161,11 +161,21 @@ pub fn resolve_runner_for_audit( /// Resolve `request`'s backend and run it to completion. /// /// Convenience over [`resolve_runner`] that runs the resolved backend to -/// completion. +/// completion. Warnings the logger recorded are moved onto the response; +/// nothing is written to the terminal. pub fn run(request: &ExecutionRequest, logger: &mut Logger) -> Result { - let mut resolved = resolve_runner(request, logger)?; - let response = resolved.runner.run(request, logger); - Ok(response) + let resolved = resolve_runner(request, logger)?; + Ok(run_resolved(resolved, request, logger)) +} + +fn run_resolved( + mut resolved: ResolvedRunner, + request: &ExecutionRequest, + logger: &mut Logger, +) -> ScriptResponse { + let mut response = resolved.runner.run(request, logger); + response.extend_warnings(logger.take_warnings()); + response } // --------------------------------------------------------------------------- @@ -177,22 +187,20 @@ fn resolve_runner_inner_windows( request: &ExecutionRequest, logger: &mut Logger, ) -> Result { - use std::fmt::Write; - match request.containment { ContainmentBackend::ProcessContainer => { use crate::mxc_common::sandbox_process::Runner; use crate::process_container_common::base_container_runner::BaseContainerRunner; - BaseContainerRunner::for_request(request) - .map(|backend| ResolvedRunner { - runner: Box::new(Runner::new(backend)), - }) - .map_err(MxcError::backend_unavailable) + BaseContainerRunner::for_request(request).map(|backend| ResolvedRunner { + runner: Box::new(Runner::new(backend)), + }) } ContainmentBackend::Wslc => { #[cfg(feature = "wslc")] { + use std::fmt::Write; + let _ = writeln!(logger, "Using WSLContainer runner"); let wslc_config = request.wslc.as_ref().cloned().unwrap_or_default(); Ok(ResolvedRunner::without_guard(Box::new( @@ -202,6 +210,7 @@ fn resolve_runner_inner_windows( #[cfg(not(feature = "wslc"))] { let _ = logger; + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( "WSLC backend not compiled. Rebuild with --features wslc.", )) @@ -240,11 +249,9 @@ fn resolve_runner_inner_windows( if ws.idle_timeout_ms != default.idle_timeout_ms || ws.daemon_pipe_name != default.daemon_pipe_name { - let _ = writeln!( - logger, - "warning: windowsSandbox.idleTimeoutMs and daemonPipeName \ - are ignored by the one-shot backend; each invocation launches and tears \ - down a fresh VM" + logger.warning_line( + "windowsSandbox.idleTimeoutMs and daemonPipeName are ignored by the \ + one-shot backend; each invocation launches and tears down a fresh VM", ); } } @@ -261,6 +268,7 @@ fn resolve_runner_inner_windows( } #[cfg(not(feature = "isolation_session"))] { + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( "IsolationSession backend not compiled. Rebuild with --features isolation_session.", )) @@ -423,8 +431,10 @@ fn resolve_hyperlight() -> Result { } #[cfg(not(all(feature = "hyperlight", target_arch = "x86_64")))] { + // SDK v2: backend_unavailable. See docs/development/plans/sdk-v2-error-codes.md. Err(MxcError::unsupported_containment( - "Hyperlight backend requires x86_64 (Hyperlight needs KVM or WHP)", + "Hyperlight backend not compiled in; it requires x86_64 and --features hyperlight \ + (Hyperlight needs KVM or WHP).", )) } } @@ -488,6 +498,7 @@ mod tests { let mut logger = Logger::new(Mode::Buffer); resolve_runner_inner_windows(&request, &mut logger).unwrap(); assert!(logger.get_buffer().is_empty()); + assert!(logger.warnings().is_empty()); } } @@ -502,7 +513,7 @@ mod tests { resolve_runner_inner_windows(&request, &mut logger).unwrap(); - let warning = logger.get_buffer(); + let warning = logger.warnings().join("\n"); assert!(warning.contains("idleTimeoutMs")); assert!(warning.contains("daemonPipeName")); assert!(warning.contains("fresh VM")); @@ -522,4 +533,76 @@ mod tests { assert!(!logger.get_buffer().contains("experimental")); } + + fn resolve_error(request: &ExecutionRequest) -> Error { + let mut logger = Logger::new(Mode::Buffer); + match resolve_runner(request, &mut logger) { + Ok(_) => panic!("{:?} must not resolve", request.containment), + Err(error) => error, + } + } + + #[test] + fn an_unenforceable_process_container_field_is_backend_unavailable() { + let _guard = crate::process_container_common::test_env::PolicyCapabilityGuard::set( + true, false, true, + ); + let mut request = ExecutionRequest { + containment: ContainmentBackend::ProcessContainer, + ..Default::default() + }; + request.policy.enumerate_paths = vec!["C:\\tools".to_string()]; + + let error = resolve_error(&request); + + assert_eq!(error.code, crate::ErrorCode::BackendUnavailable, "{error}"); + } + + #[cfg(not(all(feature = "hyperlight", target_arch = "x86_64")))] + #[test] + fn a_backend_not_compiled_in_is_unsupported() { + let request = ExecutionRequest { + containment: ContainmentBackend::Hyperlight, + experimental_enabled: true, + ..Default::default() + }; + + let error = resolve_error(&request); + + assert_eq!( + error.code, + crate::ErrorCode::UnsupportedContainment, + "{error}" + ); + } +} + +#[cfg(test)] +mod run_tests { + use super::*; + use crate::mxc_common::logger::Mode; + + struct WarningRunner; + + impl ScriptRunner for WarningRunner { + fn execute(&mut self, _request: &ExecutionRequest, logger: &mut Logger) -> ScriptResponse { + logger.warning_line("advisory"); + ScriptResponse::default() + } + } + + #[test] + fn run_moves_logger_warnings_onto_the_response() { + let mut logger = Logger::new(Mode::Buffer); + let response = run_resolved( + ResolvedRunner::without_guard(Box::new(WarningRunner)), + &ExecutionRequest { + script_code: "echo hi".to_string(), + ..Default::default() + }, + &mut logger, + ); + assert_eq!(response.warnings(), ["advisory"]); + assert!(logger.warnings().is_empty()); + } } diff --git a/src/tools/lxc/src/linux_executor_arguments.rs b/src/tools/lxc/src/linux_executor_arguments.rs index bbe5fcdc17..244c3e9c37 100644 --- a/src/tools/lxc/src/linux_executor_arguments.rs +++ b/src/tools/lxc/src/linux_executor_arguments.rs @@ -5,7 +5,8 @@ use clap::Parser; use mxc_sdk::mxc_common::config_parser::load_one_shot_request; use mxc_sdk::mxc_common::logger::Logger; use mxc_sdk::mxc_common::models::ExecutionRequest; -use std::process; +use mxc_sdk::mxc_common::mxc_error::MxcError; +use mxc_sdk::mxc_common::script_runner::emit_mxc_error_exit; #[derive(Parser)] #[command(name = "lxc-exec", about = "Linux Container Executor")] @@ -99,19 +100,17 @@ impl LinuxExecutorArguments { } else if let Some(ref path) = self.config_path { (path.clone(), false) } else { - eprintln!( - "Error: No config provided. Use a positional path, --config, or --config-base64" - ); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "No config provided. Use a positional path, --config, or --config-base64", + )); }; let mut request = match load_one_shot_request(&config_data, logger, is_base64) { Ok(r) => r, - Err(_) => { - eprintln!("Request error"); + Err(error) => { crate::emit_warnings(logger); eprint!("{}", logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&error.to_mxc_error()); } }; @@ -125,20 +124,19 @@ impl LinuxExecutorArguments { self.setup_hyperlight.is_some() } - pub fn setup_hyperlight(&self) -> Result { + pub fn setup_hyperlight(&self) -> Result { if let Some(config_path) = &self.config_path { - return Err(format!( - "Error: --setup-hyperlight takes no config path; name runtimes with \ + return Err(MxcError::malformed_request(format!( + "--setup-hyperlight takes no config path; name runtimes with \ --setup-hyperlight={config_path}" - )); + ))); } #[cfg(not(all(feature = "hyperlight", target_arch = "x86_64")))] { - Err( - "Error: --setup-hyperlight requires x86_64 (Hyperlight needs KVM or WHP)" - .to_string(), - ) + Err(MxcError::unsupported_containment( + "--setup-hyperlight requires x86_64 (Hyperlight needs KVM or WHP)", + )) } #[cfg(all(feature = "hyperlight", target_arch = "x86_64"))] @@ -146,35 +144,35 @@ impl LinuxExecutorArguments { // WHP is delay-loaded; check before setup boots a VM. #[cfg(target_os = "windows")] if !mxc_sdk::hyperlight_common::is_whp_available() { - return Err( - "Error: --setup-hyperlight requires Windows Hypervisor Platform (WHP). \ - Enable the HypervisorPlatform optional feature and reboot." - .to_string(), - ); + return Err(MxcError::backend_unavailable( + "--setup-hyperlight requires Windows Hypervisor Platform (WHP). \ + Enable the HypervisorPlatform optional feature and reboot.", + )); } //KVM is checked before anything is pulled. #[cfg(target_os = "linux")] if !mxc_sdk::hyperlight_common::is_kvm_available() { - return Err( - "Error: --setup-hyperlight requires KVM: /dev/kvm must be readable and \ - writable by this user." - .to_string(), - ); + return Err(MxcError::backend_unavailable( + "--setup-hyperlight requires KVM: /dev/kvm must be readable and \ + writable by this user.", + )); } // Setup is an interactive install: the pull and the warm-up // report progress as they go. let runtimes = mxc_sdk::hyperlight_common::parse_runtimes(&self.setup_hyperlight.clone().unwrap()) - .map_err(|message| format!("Error: {message}"))?; + .map_err(MxcError::malformed_request)?; let mut logger = mxc_sdk::mxc_common::logger::Logger::new( mxc_sdk::mxc_common::logger::Mode::Console, ); match mxc_sdk::hyperlight_common::setup(self.force, &runtimes, &mut logger) { Ok(home) => Ok(format!("hyperlight setup: image home ready at {home:?}")), - Err(msg) => Err(format!("hyperlight setup failed: {msg}")), + Err(msg) => Err(MxcError::backend_error(format!( + "hyperlight setup failed: {msg}" + ))), } } } @@ -187,8 +185,9 @@ impl LinuxExecutorArguments { let name = match self.containername { Some(ref n) => n, None => { - eprintln!("Error: --containername is required with --delete"); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "--containername is required with --delete", + )); } }; diff --git a/src/tools/lxc/src/main.rs b/src/tools/lxc/src/main.rs index 1b482ad86c..9b1188a448 100644 --- a/src/tools/lxc/src/main.rs +++ b/src/tools/lxc/src/main.rs @@ -10,7 +10,8 @@ use std::{eprint, process}; use mxc_sdk::mxc_common::logger::{Logger, Mode}; use mxc_sdk::mxc_common::models::{ExecutionRequest, ScriptResponse}; -use mxc_sdk::mxc_common::script_runner; +use mxc_sdk::mxc_common::mxc_error::MxcError; +use mxc_sdk::mxc_common::script_runner::{self, emit_mxc_error_exit, emit_warning_envelopes}; use mxc_sdk::mxc_common::telemetry; use mxc_sdk::lxc_common::signal_cleanup; @@ -28,7 +29,7 @@ fn main() { // Enable the file sink if needed if let Some(ref log_path) = arguments.get_log_path() { if let Err(e) = logger.enable_file_sink(std::path::Path::new(log_path)) { - eprintln!("Warning: could not open log file '{}': {}", log_path, e); + emit_warning_envelopes(&[format!("could not open log file '{log_path}': {e}")]); } } @@ -40,8 +41,9 @@ fn main() { process::exit(0); } Err(e) => { - eprintln!("Error: probe serialization failed: {e}"); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_error(format!( + "probe serialization failed: {e}" + ))); } } } else if arguments.does_user_want_hyperlight_setup() { @@ -50,10 +52,7 @@ fn main() { eprintln!("{message}"); process::exit(0); } - Err(message) => { - eprintln!("{message}"); - process::exit(1); - } + Err(error) => emit_mxc_error_exit(&error), } } else if arguments.does_user_want_to_delete_a_container() { let success = arguments.delete_container(&mut logger); @@ -85,8 +84,9 @@ fn main() { // Setup watchdog for unexpected shutdowns if let Err(e) = signal_cleanup::install() { - eprintln!("Error: failed to install signal cleanup handler: {e}"); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_error(format!( + "failed to install signal cleanup handler: {e}" + ))); } let run_start = Instant::now(); @@ -95,7 +95,6 @@ fn main() { let response = match mxc_sdk::mxc_engine::run(&request, &mut logger) { Ok(response) => response, Err(e) => { - eprintln!("error: {}", e.message); emit_warnings(&logger); eprint!("{}", logger.get_buffer()); telemetry::emit_early_exit_with_kind( @@ -104,14 +103,15 @@ fn main() { requested_sandbox_kind, telemetry::FailureReason::InitError, ); - process::exit(1); + emit_mxc_error_exit(&e.into_mxc_error()); } }; let run_elapsed = run_start.elapsed(); let _ = writeln!(logger, "Runner completed in {}ms", run_elapsed.as_millis()); - emit_warnings(&logger); + // Emitted before the dry-run branch below, which exits the process. + emit_warning_envelopes(response.warnings()); // Handle dry_run request. if arguments.parse_and_validate() { @@ -145,9 +145,7 @@ fn log_request(request: &ExecutionRequest, logger: &mut Logger) { } fn emit_warnings(logger: &Logger) { - for warning in logger.warnings() { - eprintln!("{warning}"); - } + emit_warning_envelopes(logger.warnings()); } fn display_script_results(response: &ScriptResponse, logger: &mut Logger) { diff --git a/src/tools/lxc/tests/cli.rs b/src/tools/lxc/tests/cli.rs index 05c45571f8..bb9d675c18 100644 --- a/src/tools/lxc/tests/cli.rs +++ b/src/tools/lxc/tests/cli.rs @@ -27,3 +27,20 @@ fn writes_config_parse_error_to_log_file() { String::from_utf8_lossy(&output.stderr) ); } + +#[test] +fn a_setup_usage_error_is_a_typed_envelope() { + let output = Command::new(env!("CARGO_BIN_EXE_lxc-exec")) + .args(["--setup-hyperlight", "python"]) + .output() + .expect("run lxc-exec"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr + .lines() + .any(|line| line.starts_with('{') && line.contains(r#""code":"malformed_request""#)), + "no malformed_request envelope on stderr:\n{stderr}" + ); + assert_eq!(output.status.code(), Some(1)); +} diff --git a/src/tools/mxc_darwin/src/main.rs b/src/tools/mxc_darwin/src/main.rs index f017a763aa..72a83979e2 100644 --- a/src/tools/mxc_darwin/src/main.rs +++ b/src/tools/mxc_darwin/src/main.rs @@ -10,12 +10,13 @@ //! use still requires macOS. use std::fmt::Write; -use std::process; use clap::Parser; use mxc_sdk::mxc_common::config_parser::load_one_shot_request; use mxc_sdk::mxc_common::logger::{Logger, Mode}; use mxc_sdk::mxc_common::models::ExecutionRequest; +use mxc_sdk::mxc_common::mxc_error::MxcError; +use mxc_sdk::mxc_common::script_runner::{emit_mxc_error_exit, emit_warning_envelopes}; #[cfg(target_os = "macos")] use mxc_sdk::mxc_common::models::ScriptResponse; @@ -84,8 +85,9 @@ fn main() { } else if let Some(ref path) = cli.config_path { (path.clone(), false) } else { - eprintln!("Error: No config provided. Use a positional path, --config, or --config-base64"); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "No config provided. Use a positional path, --config, or --config-base64", + )); }; let mut logger = Logger::new(if cli.debug { @@ -96,15 +98,15 @@ fn main() { if let Some(ref log_path) = cli.log_file { if let Err(e) = logger.enable_file_sink(std::path::Path::new(log_path)) { - eprintln!("Warning: could not open log file '{}': {}", log_path, e); + emit_warning_envelopes(&[format!("could not open log file '{log_path}': {e}")]); } } let mut request = match load_one_shot_request(&config_data, &mut logger, is_base64) { Ok(r) => r, - Err(_) => { - eprint!("Request error\n{}", logger.get_buffer()); - process::exit(1); + Err(error) => { + eprint!("{}", logger.get_buffer()); + emit_mxc_error_exit(&error.to_mxc_error()); } }; @@ -150,14 +152,17 @@ fn run_seatbelt(request: &ExecutionRequest, logger: &mut Logger) -> ! { let response = match mxc_sdk::mxc_engine::run(request, logger) { Ok(response) => response, Err(e) => { - eprintln!("error: {}", e.message); + emit_warning_envelopes(logger.warnings()); eprint!("{}", logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&e.into_mxc_error()); } }; let run_elapsed = run_start.elapsed(); let _ = writeln!(logger, "Runner completed in {}ms", run_elapsed.as_millis()); + // Emitted before the dry-run branch below, which exits the process. + emit_warning_envelopes(response.warnings()); + if request.dry_run { handle_dry_run_exit(&response, logger); } @@ -182,15 +187,14 @@ fn run_seatbelt(request: &ExecutionRequest, logger: &mut Logger) -> ! { // with lxc-exec / wxc-exec (issue #564). mxc_sdk::mxc_common::script_runner::emit_backend_error_envelope(&response); - process::exit(response.exit_code); + std::process::exit(response.exit_code); } #[cfg(not(target_os = "macos"))] fn run_seatbelt(_request: &ExecutionRequest, logger: &mut Logger) -> ! { - eprintln!( - "mxc-exec-mac: the macOS sandbox backend is only available on macOS. \ - This binary was built for a non-Darwin target and cannot execute scripts." - ); print!("{}", logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_unavailable( + "mxc-exec-mac: the macOS sandbox backend is only available on macOS. \ + This binary was built for a non-Darwin target and cannot execute scripts.", + )); } diff --git a/src/tools/wxc/src/main.rs b/src/tools/wxc/src/main.rs index d2979faf69..91b5aed86f 100644 --- a/src/tools/wxc/src/main.rs +++ b/src/tools/wxc/src/main.rs @@ -18,10 +18,12 @@ use mxc_sdk::mxc_common::diagnostic::DiagnosticConfig; use mxc_sdk::mxc_common::error::WxcError; use mxc_sdk::mxc_common::logger::{Logger, Mode}; use mxc_sdk::mxc_common::models::{ContainmentBackend, ExecutionRequest, ScriptResponse}; -use mxc_sdk::mxc_common::mxc_error::{MxcError, MxcErrorCode, ResponseEnvelope}; +use mxc_sdk::mxc_common::mxc_error::{MxcError, MxcErrorCode}; #[cfg(target_os = "windows")] use mxc_sdk::mxc_common::script_runner::RunCancellation; -use mxc_sdk::mxc_common::script_runner::{handle_dry_run_exit, ScriptRunner}; +use mxc_sdk::mxc_common::script_runner::{ + emit_mxc_error_exit, emit_warning_envelopes, handle_dry_run_exit, ScriptRunner, +}; use mxc_sdk::mxc_common::state_aware_dispatch::{resolve_backend, DispatchOutcome}; use mxc_sdk::mxc_common::state_aware_request::{MxcRequest, ParsedStateAwareRequest, Phase}; use mxc_sdk::mxc_common::telemetry; @@ -370,7 +372,7 @@ fn logger_for_cli(cli: &Cli) -> Logger { if let Some(ref log_path) = cli.log_file { if let Err(error) = logger.enable_file_sink(std::path::Path::new(log_path)) { - eprintln!("Warning: could not open log file '{log_path}': {error}"); + emit_warning_envelopes(&[format!("could not open log file '{log_path}': {error}")]); } } @@ -588,9 +590,7 @@ fn run_state_aware_main( diagnostic_logger.log_audit_event(&record); } } - for warning in logger.take_warnings() { - eprintln!("{warning}"); - } + mxc_sdk::mxc_common::script_runner::emit_warning_envelopes(&logger.take_warnings()); let buffered = logger.get_buffer().to_string(); if !buffered.is_empty() { eprint!("{}", buffered); @@ -672,15 +672,15 @@ fn run_state_aware_main( process::exit(0); } StateAwareExit::ExecCode(exit_code) => process::exit(exit_code), - StateAwareExit::Error(json) => { + StateAwareExit::Error(json, exit_code) => { println!("{}", json); - process::exit(1); + process::exit(exit_code); } - StateAwareExit::ExecError(json) => { + StateAwareExit::ExecError(json, exit_code) => { // The script's own stderr may not end in a newline, and gluing the // envelope onto those bytes would leave it unparseable. eprintln!("\n{}", json); - process::exit(1); + process::exit(exit_code); } } } @@ -694,10 +694,10 @@ enum StateAwareExit { Envelope(String), /// Exit with this script exit code (its output was already streamed). ExecCode(i32), - /// Print this JSON error envelope to stdout, then exit 1. - Error(String), - /// Print this JSON error envelope to stderr, then exit 1. - ExecError(String), + /// Print this JSON error envelope to stdout, then exit with the code's status. + Error(String, i32), + /// Print this JSON error envelope to stderr, then exit with the code's status. + ExecError(String, i32), } /// Map a dispatch outcome to its terminal exit action. Pure — it neither prints @@ -713,22 +713,28 @@ fn finalize_state_aware_outcome( Ok(DispatchOutcome::ExecCompleted { exit_code }) => StateAwareExit::ExecCode(exit_code), Err(e) => { let json = error_envelope_string(&e); + let exit_code = e.code.exit_code(); // A failed exec has already streamed the script's output to stdout, // so an envelope appended there would corrupt both. A dry run // streams nothing and keeps its envelope on stdout. if phase == "exec" && !dry_run { - StateAwareExit::ExecError(json) + StateAwareExit::ExecError(json, exit_code) } else { - StateAwareExit::Error(json) + StateAwareExit::Error(json, exit_code) } } } } +/// Where a request failure is written, which differs by request shape. enum RequestErrorRoute<'a> { - Diagnostic, - Envelope(&'a MxcError), + /// One-shot: a typed envelope on stderr, beside the diagnostics, because + /// stdout belongs to the script's own output. + Stderr(MxcError), + /// State-aware: the envelope *is* the operation's response document, so it + /// goes to stdout where the caller parses the result. + Stdout(&'a MxcError), } fn request_error_route(error: &ParseError) -> RequestErrorRoute<'_> { @@ -736,8 +742,8 @@ fn request_error_route(error: &ParseError) -> RequestErrorRoute<'_> { ParseError::Decode(_) | ParseError::Version(_) | ParseError::OneShot(_) - | ParseError::OneShotMalformed(_) => RequestErrorRoute::Diagnostic, - ParseError::StateAware(e) => RequestErrorRoute::Envelope(e), + | ParseError::OneShotMalformed(_) => RequestErrorRoute::Stderr(error.to_mxc_error()), + ParseError::StateAware(e) => RequestErrorRoute::Stdout(e), } } @@ -746,10 +752,8 @@ fn reject_state_aware_without_operation(logger: &mut Logger) -> ! { remove 'phase' and 'sandboxId' from the config JSON"; let error = ParseError::OneShot(WxcError::ConfigParse(message.to_string())); log_request_parse_rejection(logger, &error, None); - eprintln!("Request error"); - eprintln!("{message}"); eprint!("{}", logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request(message)); } fn log_request_parse_rejection(logger: &mut Logger, error: &ParseError, phase: Option) { @@ -802,18 +806,9 @@ fn print_error_envelope(error: &MxcError) { /// Serialise `error` to its JSON response-envelope string. Split out of /// [`print_error_envelope`] so [`finalize_state_aware_outcome`] can build the -/// error-exit envelope without printing, and so both the print path and the -/// state-aware exit path share one serialisation (including the last-resort -/// fallback when the envelope itself fails to serialise). +/// error-exit envelope without printing. fn error_envelope_string(error: &MxcError) -> String { - let envelope: ResponseEnvelope<()> = ResponseEnvelope::from_error(error); - serde_json::to_string(&envelope).unwrap_or_else(|_| { - // Last-resort path: serialisation of the envelope itself failed — emit a - // minimal structurally-valid envelope so consumers that require - // `error.code` still parse something. - r#"{"error":{"code":"backend_error","message":"failed to serialise error envelope"}}"# - .to_string() - }) + error.to_envelope_string() } fn config_input(cli: &Cli) -> Option<(String, bool)> { @@ -965,15 +960,17 @@ fn main() { ) { Ok(MxcRequest::OneShot(request)) => request, Ok(MxcRequest::StateAware(_)) | Err(_) => { - eprintln!("Error: failed to load probe config"); eprint!("{}", probe_logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "failed to load probe config", + )); } }, Err(_) => { - eprintln!("Error: failed to load probe config"); eprint!("{}", probe_logger.get_buffer()); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "failed to load probe config", + )); } } } else { @@ -981,17 +978,13 @@ fn main() { }; let output = match mxc_sdk::mxc_engine::probe_execution_request(Some(&request)) { Ok(output) => output, - Err(error) => { - eprintln!("Error: {}", error.message); - process::exit(1); - } + Err(error) => emit_mxc_error_exit(&error.into_mxc_error()), }; match mxc_sdk::process_container_common::probe::to_json_pretty(&output) { Ok(s) => println!("{s}"), - Err(e) => { - eprintln!("Error: probe serialization failed: {e}"); - process::exit(1); - } + Err(e) => emit_mxc_error_exit(&MxcError::backend_error(format!( + "probe serialization failed: {e}" + ))), } if decoded_config.is_some() && output.error.is_some() { process::exit(1); @@ -1015,8 +1008,9 @@ fn main() { #[cfg(target_os = "windows")] if let Err(error) = install_executor_ctrl_handler() { - eprintln!("cannot install executor console-control handler: {error}"); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_error(format!( + "cannot install executor console-control handler: {error}" + ))); } // --setup-hyperlight: warm up the snapshot and exit. Runs before @@ -1026,50 +1020,43 @@ fn main() { // Setup exits before any config is read, so a positional here is // most likely a runtime name given with a space instead of `=`. if let Some(stray) = &cli.config_path { - eprintln!( - "Error: --setup-hyperlight takes no config path; name runtimes with \ + emit_mxc_error_exit(&MxcError::malformed_request(format!( + "--setup-hyperlight takes no config path; name runtimes with \ --setup-hyperlight={stray}" - ); - process::exit(1); + ))); } #[cfg(all(feature = "hyperlight", target_arch = "x86_64"))] { // WHP is delay-loaded; check before setup boots a VM. #[cfg(target_os = "windows")] if !mxc_sdk::hyperlight_common::is_whp_available() { - eprintln!( - "Error: --setup-hyperlight requires Windows Hypervisor Platform (WHP). \ - Enable the HypervisorPlatform optional feature and reboot." - ); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_unavailable( + "--setup-hyperlight requires Windows Hypervisor Platform (WHP). \ + Enable the HypervisorPlatform optional feature and reboot.", + )); } // Setup is an interactive install: the pull and the warm-up // report progress as they go. let mut logger = Logger::new(Mode::Console); - let runtimes = match mxc_sdk::hyperlight_common::parse_runtimes(runtime_names) { - Ok(runtimes) => runtimes, - Err(msg) => { - eprintln!("Error: {msg}"); - process::exit(1); - } - }; + let runtimes = mxc_sdk::hyperlight_common::parse_runtimes(runtime_names) + .unwrap_or_else(|msg| emit_mxc_error_exit(&MxcError::malformed_request(msg))); match mxc_sdk::hyperlight_common::setup(cli.force, &runtimes, &mut logger) { Ok(home) => { eprintln!("hyperlight setup: image home ready at {:?}", home); process::exit(0); } - Err(msg) => { - eprintln!("hyperlight setup failed: {msg}"); - process::exit(1); - } + Err(msg) => emit_mxc_error_exit(&MxcError::backend_error(format!( + "hyperlight setup failed: {msg}" + ))), } } #[cfg(not(all(feature = "hyperlight", target_arch = "x86_64")))] { let _ = runtime_names; - eprintln!("Error: --setup-hyperlight requires x86_64 (Hyperlight needs KVM or WHP)"); - process::exit(1); + emit_mxc_error_exit(&MxcError::unsupported_containment( + "--setup-hyperlight requires x86_64 (Hyperlight needs KVM or WHP)", + )); } } @@ -1086,8 +1073,9 @@ fn main() { .filter(|name| !name.is_empty()) .collect(); if images.is_empty() { - eprintln!("Error: --setup-wslc requires --image "); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "--setup-wslc requires --image ", + )); } let mut logger = Logger::new(if cli.debug { Mode::Console @@ -1114,16 +1102,16 @@ fn main() { } match result { Ok(()) => process::exit(0), - Err(msg) => { - eprintln!("wslc setup failed: {msg}"); - process::exit(1); - } + Err(msg) => emit_mxc_error_exit(&MxcError::backend_error(format!( + "wslc setup failed: {msg}" + ))), } } #[cfg(not(feature = "wslc"))] { - eprintln!("Error: WSLC backend not compiled. Rebuild with --features wslc."); - process::exit(1); + emit_mxc_error_exit(&MxcError::unsupported_containment( + "WSLC backend not compiled. Rebuild with --features wslc.", + )); } } @@ -1153,9 +1141,7 @@ fn main() { process::exit(1); } None => { - eprintln!("Request error"); - eprintln!("{}", error.into_error()); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request(error.into_error().to_string())); } }, None => { @@ -1170,8 +1156,7 @@ fn main() { eprint!("{}", logger.get_buffer()); process::exit(1); } - eprintln!("Error: {message}"); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request(message)); } }; if let Some(operation) = cli.operation { @@ -1246,15 +1231,16 @@ fn main() { Err(error) => { log_request_parse_rejection(&mut logger, &error, None); match request_error_route(&error) { - RequestErrorRoute::Diagnostic => { - eprint!("Request error\n{}", logger.get_buffer()); + RequestErrorRoute::Stderr(typed) => { + eprint!("{}", logger.get_buffer()); + emit_mxc_error_exit(&typed); } - RequestErrorRoute::Envelope(e) => { + RequestErrorRoute::Stdout(e) => { print_error_envelope(e); eprint!("{}", logger.get_buffer()); + process::exit(e.code.exit_code()); } } - process::exit(1); } }; @@ -1305,35 +1291,34 @@ fn main() { "containment", "", ); - eprintln!("Error: {message}"); telemetry::emit_early_exit_with_kind( telemetry_active, &request.containment, requested_sandbox_kind, telemetry::FailureReason::ConfigError, ); - process::exit(1); + emit_mxc_error_exit(&MxcError::policy_validation(message)); } let injected = apply_permissive_learning_mode(&mut request.policy.capabilities); let context = match audit::prepare_request(&mut request, config_file_path(&cli)) { Ok(context) => context, Err(message) => { - eprintln!("Error: {message}"); telemetry::emit_early_exit_with_kind( telemetry_active, &request.containment, requested_sandbox_kind, telemetry::FailureReason::ConfigError, ); - process::exit(1); + emit_mxc_error_exit(&MxcError::backend_error(message)); } }; audit_context = Some(context); logger.log("WARNING: --audit enabled - AppContainer restrictions will NOT be enforced\n"); if injected && cli.audit_verbose { - eprintln!( - "[mxc] permissiveLearningMode injected via --audit - AppContainer restrictions are NOT enforced" - ); + emit_warning_envelopes(&[ + "permissiveLearningMode injected via --audit - AppContainer restrictions are NOT enforced" + .to_string(), + ]); } } @@ -1347,9 +1332,6 @@ fn main() { "process.commandLine", "", ); - eprintln!( - "Error: no command to run. Provide `process.commandLine` in the policy or pass the command as arguments after the config path." - ); eprint!("{}", logger.get_buffer()); telemetry::emit_early_exit_with_kind( telemetry_active, @@ -1357,7 +1339,9 @@ fn main() { requested_sandbox_kind, telemetry::FailureReason::ConfigError, ); - process::exit(1); + emit_mxc_error_exit(&MxcError::malformed_request( + "no command to run. Provide `process.commandLine` in the policy or pass the command as arguments after the config path.", + )); } // Inject learningModeLogging capability when diagnostic console is enabled. @@ -1456,22 +1440,23 @@ fn main() { let mut runner: Box = match resolved_runner { Ok(resolved) => resolved.runner, Err(e) => { + let error = e.into_mxc_error(); log_config_rejected( &mut logger, - RejectionReason::RunnerUnavailable, + rejection_reason_for(&error), request.containment.wire_name(), "containment", "", ); - eprintln!("error: {}", e.message); eprint!("{}", logger.get_buffer()); + emit_warning_envelopes(&logger.take_warnings()); telemetry::emit_early_exit_with_kind( telemetry_active, &request.containment, requested_sandbox_kind, telemetry::FailureReason::InitError, ); - process::exit(1); + emit_mxc_error_exit(&error); } }; @@ -1493,22 +1478,28 @@ fn main() { if !cli.dry_run { match audit::finalize(&mut response, context, &exe_dir, cli.audit_verbose) { Ok(cleanup_warnings) => { - for warning in cleanup_warnings { - let message = format!("[audit] warning: {warning}"); - let _ = writeln!(logger, "{message}"); - eprintln!("{message}"); + for warning in &cleanup_warnings { + let _ = writeln!(logger, "[audit] warning: {warning}"); } + response.extend_warnings( + cleanup_warnings + .into_iter() + .map(|warning| format!("[audit] {warning}")), + ); eprintln!("[audit] artifacts written to {}", context.log_dir.display()); } Err(error) => { let message = format!("audit finalization failed: {error}"); let _ = writeln!(logger, "[audit] {message}"); - eprintln!("error: {message}"); - response.exit_code = -1; response.error_message = match response.error_message.is_empty() { true => message, false => format!("{}; {message}", response.error_message), }; + // Derived rather than hard-coded so the status keeps + // agreeing with the code the envelope reports: an + // otherwise-clean run carries `backend_error`, and a run + // the backend already refused stays a rejection. + response.exit_code = response.failure_phase.failure_exit_code(); } } } @@ -1524,11 +1515,11 @@ fn main() { // deny-by-default under --audit). The logger only records these rather than // writing them itself, so that `mxc_engine` embedders don't get unannounced // writes to a terminal they own. wxc-exec *does* own its terminal, so it - // opts in here. Messages carry their own banner; print them verbatim. + // opts in here. Warnings travel on the response so a programmatic caller + // reads them from one structured place rather than scraping stderr. // Emitted before the dry-run branch below, which exits the process. - for warning in logger.warnings() { - eprintln!("{warning}"); - } + response.extend_warnings(logger.take_warnings()); + emit_warning_envelopes(response.warnings()); #[cfg(target_os = "windows")] if let Err(error) = mxc_sdk::mxc_engine::emit_verbose_telemetry( @@ -1858,7 +1849,7 @@ mod tests { impl ResolveError { fn from_parse(e: ParseError) -> Self { - let envelope_routed = matches!(request_error_route(&e), RequestErrorRoute::Envelope(_)); + let envelope_routed = matches!(request_error_route(&e), RequestErrorRoute::Stdout(_)); let message = match e { ParseError::Decode(err) @@ -1948,28 +1939,33 @@ mod tests { #[test] fn request_error_route_matches_the_output_conventions() { - let decode = ParseError::Decode(WxcError::ConfigParse("decode".to_string())); - assert!(matches!( - request_error_route(&decode), - RequestErrorRoute::Diagnostic - )); - - let version = ParseError::Version(WxcError::ConfigParse("version".to_string())); - assert!(matches!( - request_error_route(&version), - RequestErrorRoute::Diagnostic - )); - - let oneshot = ParseError::OneShot(WxcError::ConfigParse("oneshot".to_string())); - assert!(matches!( - request_error_route(&oneshot), - RequestErrorRoute::Diagnostic - )); + // The stderr route still classifies: a one-shot parse failure blames + // the request, so it reports a code rather than an untyped exit. + for (case, error) in [ + ( + "decode", + ParseError::Decode(WxcError::ConfigParse("decode".to_string())), + ), + ( + "version", + ParseError::Version(WxcError::ConfigParse("version".to_string())), + ), + ( + "oneshot", + ParseError::OneShot(WxcError::ConfigParse("oneshot".to_string())), + ), + ] { + let RequestErrorRoute::Stderr(typed) = request_error_route(&error) else { + panic!("{case}: one-shot failures belong on stderr"); + }; + assert_eq!(typed.code, MxcErrorCode::MalformedRequest, "{case}"); + assert_eq!(typed.code.exit_code(), 1, "{case}"); + } let stateaware = ParseError::StateAware(MxcError::malformed_request("stateaware")); assert!(matches!( request_error_route(&stateaware), - RequestErrorRoute::Envelope(_) + RequestErrorRoute::Stdout(_) )); } @@ -2203,7 +2199,7 @@ mod tests { ) .unwrap_err(); assert!( - matches!(request_error_route(&error), RequestErrorRoute::Diagnostic), + matches!(request_error_route(&error), RequestErrorRoute::Stderr(_)), "{case}: pre-discrimination failures must remain stderr diagnostics" ); @@ -2242,7 +2238,7 @@ mod tests { &mut logger, ) .unwrap_err(); - let RequestErrorRoute::Envelope(error) = request_error_route(&error) else { + let RequestErrorRoute::Stdout(error) = request_error_route(&error) else { panic!("identified lifecycle requests must retain their JSON error envelope"); }; let envelope: serde_json::Value = @@ -2263,7 +2259,7 @@ mod tests { .unwrap_err(); assert!(matches!( request_error_route(&error), - RequestErrorRoute::Diagnostic + RequestErrorRoute::Stderr(_) )); assert!(logger.get_buffer().contains("duplicate field `telemetry`")); } @@ -3050,11 +3046,12 @@ mod tests { let outcome: Result = Err(MxcError::malformed_request("boom".to_string())); match finalize_state_aware_outcome(outcome, "provision", false) { - StateAwareExit::Error(json) => { + StateAwareExit::Error(json, exit_code) => { // Must be a parseable envelope carrying the error code so consumers // that key off `error.code` still work. let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); assert!(parsed["error"]["code"].is_string(), "{json}"); + assert_eq!(exit_code, 1); } other => panic!("expected Error, got {other:?}"), } @@ -3065,10 +3062,11 @@ mod tests { let outcome: Result = Err(MxcError::backend_error("output was truncated".to_string())); match finalize_state_aware_outcome(outcome, "exec", false) { - StateAwareExit::ExecError(json) => { + StateAwareExit::ExecError(json, exit_code) => { // Still a parseable envelope; only the stream it lands on changes. let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); assert!(parsed["error"]["code"].is_string(), "{json}"); + assert_eq!(exit_code, -1); } other => panic!("expected ExecError, got {other:?}"), } @@ -3080,7 +3078,7 @@ mod tests { Err(MxcError::malformed_request("boom".to_string())); assert!(matches!( finalize_state_aware_outcome(outcome, "exec", true), - StateAwareExit::Error(_) + StateAwareExit::Error(_, 1) )); } } diff --git a/tests/scripts/run_bwrap_cwd_advisory_test.sh b/tests/scripts/run_bwrap_cwd_advisory_test.sh index 9e974eaf21..a5eab23dd4 100644 --- a/tests/scripts/run_bwrap_cwd_advisory_test.sh +++ b/tests/scripts/run_bwrap_cwd_advisory_test.sh @@ -37,10 +37,21 @@ EOF for case in uncovered unrelated; do rc=0 output=$("$LXC_EXEC" --experimental "$WORK_DIR/$case.json" 2>"$WORK_DIR/$case.stderr") || rc=$? + # One warning envelope must carry the whole advisory, path included. if [ "$rc" -eq 0 ] \ - || ! grep -Fq "WARNING: Bubblewrap: process.cwd \"$CWD\"" "$WORK_DIR/$case.stderr" \ - || ! grep -Fq 'filesystem.readonlyPaths' "$WORK_DIR/$case.stderr" \ + || ! python3 - "$WORK_DIR/$case.stderr" "$CWD" <<'PY' \ || ! grep -Eq '(^|[[:space:]])bwrap:' "$WORK_DIR/$case.stderr"; then +import json, sys +path, cwd = sys.argv[1], sys.argv[2] +for line in open(path, encoding="utf-8"): + try: + message = json.loads(line)["warning"]["message"] + except (ValueError, KeyError, TypeError): + continue + if all(s in message for s in ("Bubblewrap: process.cwd", cwd, "filesystem.readonlyPaths")): + sys.exit(0) +sys.exit(1) +PY echo "$case cwd did not report the hint and Bubblewrap's original error: $output" cat "$WORK_DIR/$case.stderr" exit 1 @@ -48,7 +59,7 @@ for case in uncovered unrelated; do done output=$("$LXC_EXEC" --experimental "$WORK_DIR/covered.json" 2>"$WORK_DIR/covered.stderr") if ! grep -qFx "$CWD" <<<"$output" \ - || grep -Fq 'WARNING: Bubblewrap: process.cwd ' "$WORK_DIR/covered.stderr"; then + || grep -Fq 'Bubblewrap: process.cwd' "$WORK_DIR/covered.stderr"; then echo "Covered cwd did not run without an advisory: $output" cat "$WORK_DIR/covered.stderr" exit 1 diff --git a/tests/scripts/run_isolation_session_tests.ps1 b/tests/scripts/run_isolation_session_tests.ps1 index 6440ffde2c..80e8ea78c6 100644 --- a/tests/scripts/run_isolation_session_tests.ps1 +++ b/tests/scripts/run_isolation_session_tests.ps1 @@ -228,9 +228,10 @@ function Run-IsolationSessionTest { param( [string]$ConfigFile, [int]$ExpectedExit = 0, - [int[]]$ExpectedExitAnyOf = @(), [string[]]$OutputContains = @(), [string[]]$OutputLineNotEqual = @(), + [string]$ExpectedErrorCode, + [string]$ExpectedErrorMessageContains, [hashtable]$Request, [switch]$DryRun ) @@ -276,12 +277,7 @@ function Run-IsolationSessionTest { $pass = $true $reason = "" - if ($ExpectedExitAnyOf.Count -gt 0) { - if ($ExpectedExitAnyOf -notcontains $exitCode) { - $pass = $false - $reason = "Expected exit one of $($ExpectedExitAnyOf -join ', '), got $exitCode" - } - } elseif ($exitCode -ne $ExpectedExit) { + if ($exitCode -ne $ExpectedExit) { $pass = $false $reason = "Expected exit $ExpectedExit, got $exitCode" } @@ -309,6 +305,25 @@ function Run-IsolationSessionTest { } } + if ($pass -and $ExpectedErrorCode) { + $envelope = $null + foreach ($line in ($output -split "`r?`n")) { + if (-not $line.Trim().StartsWith('{"error"')) { continue } + try { $envelope = $line | ConvertFrom-Json } catch { $envelope = $null } + } + if ($null -eq $envelope) { + $pass = $false + $reason = "No error envelope in output" + } elseif ($envelope.error.code -ne $ExpectedErrorCode) { + $pass = $false + $reason = "Expected error code '$ExpectedErrorCode', got '$($envelope.error.code)'" + } elseif ($ExpectedErrorMessageContains -and + -not ([string]$envelope.error.message).Contains($ExpectedErrorMessageContains)) { + $pass = $false + $reason = "Error message missing '$ExpectedErrorMessageContains': $($envelope.error.message)" + } + } + if ($pass) { Write-Host "PASS" -ForegroundColor Green } else { @@ -361,11 +376,12 @@ $null = $results.Add((Run-IsolationSessionTest "isolation_session_stderr.json" ` # must appear in the captured output (proves streams aren't crossed or dropped mid-run). $null = $results.Add((Run-IsolationSessionTest "isolation_session_stdout_stderr_interleaved.json" ` -OutputContains @("OUT_A", "ERR_A", "OUT_B", "ERR_B", "OUT_C"))) -# Timeout: ping runs ~30s against a 1500ms deadline. The service-side timer and -# the local wait report different codes, and either can win the race to end the -# run. +# Timeout: ping runs ~30s against a 1500ms deadline. The service timer is armed +# with a grace margin, so the local wait ends the run and reports the timeout. $null = $results.Add((Run-IsolationSessionTest "isolation_session_timeout.json" ` - -ExpectedExitAnyOf 1, -1)) + -ExpectedExit -1 ` + -ExpectedErrorCode "backend_error" ` + -ExpectedErrorMessageContains "script timed out after 1500ms")) # Any IsolationSession-specific object is rejected at the same closed exact # contract boundary, before the command can run. diff --git a/tests/scripts/run_wslc_all_tests.ps1 b/tests/scripts/run_wslc_all_tests.ps1 index 2234dfb9eb..0f8a12fef9 100644 --- a/tests/scripts/run_wslc_all_tests.ps1 +++ b/tests/scripts/run_wslc_all_tests.ps1 @@ -365,13 +365,13 @@ $null = $results.Add((Run-WslcTest "wslc_network_bridged.json" ` # rules, so a per-destination egress rule must be refused rather than quietly # widened to the posture's default. foreach ($ruleConfig in @("wslc_network_egress_rules_rejected.json", "wslc_network_egress_deny_rules_rejected.json")) { - $null = $results.Add((Run-WslcTest $ruleConfig -ExpectedExit -1 ` + $null = $results.Add((Run-WslcTest $ruleConfig -ExpectedExit 1 ` -OutputContains "network.egress allow/deny rules" ` -OutputNotContains "WSLC_EGRESS_RULES_SHOULD_NOT_RUN")) } # `process.env` without `inheritDefaultEnv` is launched through `env -i NAME=VALUE`, # which puts the injected proxy URL in argv and therefore in /proc//cmdline. -$null = $results.Add((Run-WslcTest "wslc_network_proxy_credentials_rejected.json" -ExpectedExit -1 ` +$null = $results.Add((Run-WslcTest "wslc_network_proxy_credentials_rejected.json" -ExpectedExit 1 ` -OutputContains "must not carry credentials" ` -OutputNotContains "WSLC_PROXY_CREDENTIALS_SHOULD_NOT_RUN")) # Delegate the cooperative proxy fixture to its owning script, which asserts @@ -431,11 +431,11 @@ $null = $results.Add((Run-WslcTest "wslc_cold_cache_pull.json" -ForceDebug ` -OutputContains "Image 'busybox:latest' found" -OutputNotContains "Pulling image")) # The store is warm now, so a miss here can only come from the denied posture. Clear-ColdCacheStore -$null = $results.Add((Run-WslcTest "wslc_cold_cache_denied_egress.json" -ExpectedExit -1 ` +$null = $results.Add((Run-WslcTest "wslc_cold_cache_denied_egress.json" -ExpectedExit 1 ` -OutputContains "declares no egress" -OutputNotContains "SHOULD_NOT_RUN")) # `Check the image name and tag` is unique to the non-retryable arm; every arm # says `could not be pulled`. -$null = $results.Add((Run-WslcTest "wslc_cold_cache_unresolvable.json" -ExpectedExit -1 ` +$null = $results.Add((Run-WslcTest "wslc_cold_cache_unresolvable.json" -ExpectedExit 1 ` -OutputContains "Check the image name and tag")) Clear-ColdCacheStore @@ -465,7 +465,7 @@ $null = $results.Add((Run-WslcTest "wslc_destroy_on_exit_true.json" ` # deliver. It is refused before any container is created, so the payload must # never run. $null = $results.Add((Run-WslcTest "wslc_destroy_on_exit_false_rejected.json" ` - -ExpectedExit -1 ` + -ExpectedExit 1 ` -OutputContains "destroyOnExit=false")) Write-Host "`n--- Stdin Tests ---" -ForegroundColor Cyan