From a6cfa1bd975b3fb460be66305002de7173581ff9 Mon Sep 17 00:00:00 2001 From: Shiju Date: Thu, 1 Oct 2026 13:50:50 +0530 Subject: [PATCH 1/3] feat(gateway): validate VM filesystem tools during preflight Check required local VM tools through config preflight and share executable resolution with VM image operations. Report selected paths and actionable errors without creating gateway or sandbox state. Bound probe output and execution time, and clean up probe descendants on interruption. Preserve pure static validation and skip local tool checks for remote driver endpoints and unrelated drivers. Fixes #3951 Related to #3955 Signed-off-by: Shiju --- crates/openshell-core/src/e2fsprogs.rs | 599 ++++++++++++++++++ crates/openshell-core/src/lib.rs | 2 + crates/openshell-driver-vm/src/rootfs.rs | 512 +++++++-------- crates/openshell-gateway/src/lib.rs | 9 + .../tests/config_preflight.rs | 273 ++++++++ crates/openshell-server/src/cli.rs | 76 ++- crates/openshell-server/src/lib.rs | 15 + deploy/man/openshell-gateway.8.md | 6 +- docs/how-it-works/gateways/configuration.mdx | 40 +- skills/debug-openshell-cluster/SKILL.md | 3 +- 10 files changed, 1209 insertions(+), 326 deletions(-) create mode 100644 crates/openshell-core/src/e2fsprogs.rs create mode 100644 crates/openshell-gateway/tests/config_preflight.rs diff --git a/crates/openshell-core/src/e2fsprogs.rs b/crates/openshell-core/src/e2fsprogs.rs new file mode 100644 index 0000000000..29869bb2e6 --- /dev/null +++ b/crates/openshell-core/src/e2fsprogs.rs @@ -0,0 +1,599 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Host filesystem tools shared by VM image operations and gateway preflight. +//! +//! The gateway launches the VM driver with its environment unchanged. Resolve +//! tools here so both binaries select the same installation without linking the +//! driver runtime into the gateway or starting it during preflight. + +use std::ffi::OsStr; +use std::fs; +use std::os::unix::fs::PermissionsExt as _; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::time::Duration; +use tokio::sync::watch; + +const PROBE_TIMEOUT: Duration = Duration::from_secs(5); +const INSTALL_GUIDANCE: &str = "Install e2fsprogs 1.43 or newer, or repair the selected installation and the gateway service PATH; rerun config preflight with the service account and environment"; + +/// Resolve a tool using the VM driver's inherited PATH and existing package prefixes. +/// +/// Names are ordered alternatives: the formatter tries `mke2fs` before +/// `mkfs.ext4`. A present but broken installation returns an error and stops +/// lookup. The returned absolute path is also used for +/// image operations; the caller's environment is not changed. +pub fn resolve(names: &[&str]) -> Result { + resolve_in(names, &search_dirs(std::env::var_os("PATH").as_deref())) +} + +fn search_dirs(path: Option<&OsStr>) -> Vec { + let mut dirs: Vec<_> = path + .map(std::env::split_paths) + .into_iter() + .flatten() + .collect(); + // Preserve the VM driver's existing package-prefix fallbacks. Additional + // installations, including Linux sbin directories, belong on service PATH. + for root in ["/opt/homebrew/opt/e2fsprogs", "/usr/local/opt/e2fsprogs"] { + dirs.push(Path::new(root).join("sbin")); + dirs.push(Path::new(root).join("bin")); + } + dirs +} + +fn resolve_in(names: &[&str], dirs: &[PathBuf]) -> Result { + for name in names { + for directory in dirs { + let candidate = directory.join(name); + let metadata = match fs::metadata(&candidate) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(format!( + "inspect {}: {error}. {INSTALL_GUIDANCE}", + candidate.display() + )); + } + }; + if !metadata.is_file() || metadata.permissions().mode() & 0o111 == 0 { + return Err(format!( + "{} is not an executable file. {INSTALL_GUIDANCE}", + candidate.display() + )); + } + // Canonicalization makes relative PATH entries unambiguous in the + // report and ensures execution cannot redo PATH lookup differently. + return candidate.canonicalize().map_err(|error| { + format!( + "resolve {}: {error}. {INSTALL_GUIDANCE}", + candidate.display() + ) + }); + } + } + Err(format!( + "{} not found in the VM driver's search directories: {}. {INSTALL_GUIDANCE}", + names.join(" or "), + dirs.iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(":") + )) +} + +/// Check a formatter, `debugfs`, and `e2fsck` without creating images or driver state. +/// +/// Each selected executable receives only `-V`, with null stdin, a five-second +/// deadline, and at most 8 KiB captured per stream. Errors retain the selected +/// path and the tool's diagnostics. Image health requires a separate check. +pub async fn preflight(cancellation: watch::Receiver) -> Result, String> { + preflight_in_with_cancellation( + &search_dirs(std::env::var_os("PATH").as_deref()), + cancellation, + ) + .await +} + +#[cfg(test)] +async fn preflight_in(dirs: &[PathBuf]) -> Result, String> { + let (_sender, cancellation) = watch::channel(false); + preflight_in_with_cancellation(dirs, cancellation).await +} + +async fn preflight_in_with_cancellation( + dirs: &[PathBuf], + mut cancellation: watch::Receiver, +) -> Result, String> { + let mut reports = Vec::new(); + for (names, identity) in [ + (&["mke2fs", "mkfs.ext4"][..], "mke2fs"), + (&["debugfs"][..], "debugfs"), + (&["e2fsck"][..], "e2fsck"), + ] { + let result = async { + let path = resolve_in(names, dirs)?; + let label = path.display(); + let output = run_version_probe(Command::new(&path), &mut cancellation) + .await + .map_err(|error| format!("run {label} -V: {error}. {INSTALL_GUIDANCE}"))?; + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + if !output.status.success() { + return Err(format!( + "{label} -V failed with status {}\nstdout: {stdout}\nstderr: {stderr}\n{INSTALL_GUIDANCE}", + output.status + )); + } + let version = supported_version(identity, &stdout) + .or_else(|| supported_version(identity, &stderr)) + .ok_or_else(|| format!( + "{label} is not a supported {identity} executable\nstdout: {stdout}\nstderr: {stderr}\n{INSTALL_GUIDANCE}" + ))?; + Ok(format!("VM host tool {identity}: {label} ({version})")) + }.await; + match result { + Ok(report) => reports.push(report), + Err(error) => { + // Retain earlier selected paths even when a later tool fails. + reports.push(error); + return Err(reports.join("\n")); + } + } + } + Ok(reports) +} + +// Keep the group leader unreaped until group cleanup so its PID cannot be +// reused while a descendant still holds a captured output pipe open. +struct ProbeProcess { + child: tokio::process::Child, + group: Option, +} + +impl ProbeProcess { + fn stop_group(&mut self) -> Result, String> { + let Some(group) = self.group.take() else { + return Ok(None); + }; + match nix::sys::signal::killpg(group, nix::sys::signal::Signal::SIGKILL) { + Ok(()) | Err(nix::errno::Errno::ESRCH) => Ok(None), + // Darwin can return EPERM for a group containing only our exited, + // unreaped leader. Defer judgment until after its owned reap. + #[cfg(target_os = "macos")] + Err(nix::errno::Errno::EPERM) => Ok(Some(group)), + Err(error) => Err(format!("terminate version probe process group: {error}")), + } + } +} + +impl Drop for ProbeProcess { + fn drop(&mut self) { + let _ = self.stop_group(); + // kill_on_drop schedules the direct child for reaping if the caller + // aborts its future. CLI signal cancellation instead awaits cleanup. + } +} + +async fn cancelled(cancellation: &mut watch::Receiver) { + loop { + if *cancellation.borrow_and_update() { + return; + } + if cancellation.changed().await.is_err() { + // A closed sender does not request cancellation. + std::future::pending::<()>().await; + } + } +} + +async fn observe_exit(group: nix::unistd::Pid) -> Result<(), String> { + use rustix::process::{Pid, WaitId, WaitIdOptions, waitid}; + + let pid = Pid::from_raw(group.as_raw()).ok_or("invalid version probe process ID")?; + loop { + let exited = waitid( + WaitId::Pid(pid), + WaitIdOptions::EXITED | WaitIdOptions::NOHANG | WaitIdOptions::NOWAIT, + ) + .map(|status| status.is_some()); + match exited { + Ok(false) | Err(rustix::io::Errno::INTR) => { + tokio::time::sleep(Duration::from_millis(10)).await; + } + Ok(true) => return Ok(()), + Err(error) => return Err(format!("observe version probe exit: {error}")), + } + } +} + +async fn run_version_probe( + command: Command, + cancellation: &mut watch::Receiver, +) -> Result { + use std::process::Stdio; + + if *cancellation.borrow() { + return Err("host tool checks cancelled".to_string()); + } + let child = tokio::process::Command::from(command) + .arg("-V") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .process_group(0) + .kill_on_drop(true) + .spawn() + .map_err(|error| error.to_string())?; + let group = child + .id() + .and_then(|id| i32::try_from(id).ok()) + .map(nix::unistd::Pid::from_raw) + .ok_or_else(|| "version probe has no valid process ID".to_string())?; + let mut probe = ProbeProcess { + child, + group: Some(group), + }; + let stdout = probe + .child + .stdout + .take() + .ok_or("version probe stdout is unavailable")?; + let stderr = probe + .child + .stderr + .take() + .ok_or("version probe stderr is unavailable")?; + let output = tokio::select! { + biased; + () = cancelled(cancellation) => Err("host tool checks cancelled".to_string()), + result = tokio::time::timeout(PROBE_TIMEOUT, async { + let (stdout, stderr, ()) = tokio::try_join!( + read_probe_output(stdout, "stdout"), + read_probe_output(stderr, "stderr"), + observe_exit(group), + )?; + Ok((stdout, stderr)) + }) => result.unwrap_or_else(|_| Err("timed out after 5 seconds".to_string())), + }; + // Signal the group before reaping its leader, then retire the stored group + // ID before awaiting anything else. No later drop can signal a reused PID. + let group_cleanup = probe.stop_group(); + let status = tokio::time::timeout(Duration::from_secs(1), probe.child.wait()) + .await + .map_err(|_| "version probe cleanup exceeded one second".to_string()) + .and_then(|result| result.map_err(|error| format!("reap version probe: {error}"))); + let status = match (group_cleanup, status) { + (Ok(Some(group)), Ok(status)) => { + // Signal 0 only queries existence; it never signals a process. + // ESRCH after the owned reap proves no group members remain. If + // the ID was reused, this check can only fail conservatively. + match nix::sys::signal::killpg(group, None) { + Err(nix::errno::Errno::ESRCH) => Ok(status), + result => Err(format!( + "version probe process group remains after cleanup: {result:?}" + )), + } + } + (Ok(None), status) | (Err(_), status @ Err(_)) => status, + (Err(error), Ok(_)) => Err(error), + (Ok(Some(_)), Err(error)) => Err(error), + }; + match (output, status) { + (Ok((stdout, stderr)), Ok(status)) => Ok(std::process::Output { + status, + stdout, + stderr, + }), + (Err(error), Ok(_)) | (Ok(_), Err(error)) => Err(error), + (Err(error), Err(cleanup)) => Err(format!("{error}; {cleanup}")), + } +} + +async fn read_probe_output( + reader: impl tokio::io::AsyncRead + Unpin, + stream: &str, +) -> Result, String> { + use tokio::io::AsyncReadExt as _; + // Read one extra byte to distinguish complete output from overflow. Stop + // on overflow instead of draining an unbounded writer until the deadline. + const MAX_BYTES: usize = 8 * 1024; + let mut output = Vec::new(); + reader + .take((MAX_BYTES + 1) as u64) + .read_to_end(&mut output) + .await + .map_err(|error| format!("read version probe {stream}: {error}"))?; + if output.len() > MAX_BYTES { + output.truncate(MAX_BYTES); + return Err(format!( + "version probe {stream} exceeded {MAX_BYTES} bytes: {} [truncated]", + String::from_utf8_lossy(&output) + )); + } + Ok(output) +} + +fn supported_version<'a>(identity: &str, output: &'a str) -> Option<&'a str> { + output.lines().find_map(|line| { + let mut words = line.split_whitespace(); + if words.next() != Some(identity) { + return None; + } + let version = words.next()?; + let mut parts = version.split('.'); + let major = parts.next()?.parse::().ok()?; + let minor = parts.next()?.parse::().ok()?; + // Reject malformed version tokens rather than accepting an arbitrary + // suffix after an otherwise valid major/minor pair. + if let Some(patch) = parts.next() + && (patch.parse::().is_err() || parts.next().is_some()) + { + return None; + } + // VM image preparation uses mke2fs -d and ext4 filesystem features. + // Keep all host tools on the supported e2fsprogs release baseline. + ((major, minor) >= (1, 43)).then_some(version) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn fake_e2fs_tool(directory: &Path, name: &str, body: &str) { + let path = directory.join(name); + fs::write(&path, format!("#!/bin/sh\n{body}\n")).expect("write tool"); + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).expect("make executable"); + } + + fn fake_e2fs_installation(directory: &Path) { + for name in ["mke2fs", "debugfs", "e2fsck"] { + fake_e2fs_tool( + directory, + name, + &format!("test \"$1\" = -V || exit 64\necho '{name} 1.47.4' >&2"), + ); + } + } + + #[tokio::test] + async fn filesystem_preflight_accepts_private_prefix_and_formatter_alias() { + let temp = tempfile::tempdir().expect("private prefix"); + fake_e2fs_installation(temp.path()); + fs::rename(temp.path().join("mke2fs"), temp.path().join("mkfs.ext4")) + .expect("use formatter alias"); + preflight_in(&[temp.path().to_path_buf()]) + .await + .expect("all required tools available"); + let selected = resolve_in(&["debugfs"], &[temp.path().to_path_buf()]) + .expect("execution uses same resolver"); + let expected = temp + .path() + .join("debugfs") + .canonicalize() + .expect("tool path"); + assert_eq!(selected, expected); + } + + #[tokio::test] + async fn filesystem_preflight_rejects_missing_tools() { + let temp = tempfile::tempdir().expect("clean host search path"); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("missing formatter must reject preparation"); + assert!(error.contains("mke2fs or mkfs.ext4 not found"), "{error}"); + assert!(error.contains("gateway service PATH"), "{error}"); + } + + #[tokio::test] + async fn filesystem_preflight_requires_debugfs_and_recovery_tool() { + for missing in ["debugfs", "e2fsck"] { + let temp = tempfile::tempdir().expect("partial installation"); + fake_e2fs_installation(temp.path()); + fs::remove_file(temp.path().join(missing)).expect("remove required tool"); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("missing tool"); + assert!(error.contains(&format!("{missing} not found")), "{error}"); + assert!( + error.contains("VM host tool mke2fs:"), + "earlier path lost: {error}" + ); + } + } + + #[tokio::test] + async fn filesystem_preflight_retains_missing_interpreter_error() { + let temp = tempfile::tempdir().expect("broken installation"); + let path = temp.path().join("mke2fs"); + fs::write(&path, "#!/nonexistent/e2fsprogs-interpreter\n").expect("broken executable"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("executable bit"); + fake_e2fs_tool(temp.path(), "mkfs.ext4", "echo 'mke2fs 1.47.4' >&2"); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("loader error"); + assert!(error.contains("mke2fs -V:"), "{error}"); + assert!(error.contains("No such file or directory"), "{error}"); + assert!(!error.contains("mke2fs or mkfs.ext4 not found"), "{error}"); + } + + #[tokio::test] + async fn filesystem_preflight_rejects_nonexecutable_before_another_installation() { + let first = tempfile::tempdir().expect("first installation"); + let second = tempfile::tempdir().expect("second installation"); + fs::write(first.path().join("mke2fs"), b"not executable").expect("write broken tool"); + fake_e2fs_installation(second.path()); + let path = std::env::join_paths([first.path(), second.path()]).expect("search path"); + let error = preflight_in(&std::env::split_paths(&path).collect::>()) + .await + .expect_err("broken installation must not fall back"); + assert!(error.contains("is not an executable file"), "{error}"); + assert!( + error.contains(&first.path().display().to_string()), + "{error}" + ); + } + + #[tokio::test] + async fn filesystem_preflight_preserves_failed_tool_output() { + let first = tempfile::tempdir().expect("first installation"); + let second = tempfile::tempdir().expect("second installation"); + fake_e2fs_tool(first.path(), "mke2fs", "echo 'loader failed' >&2\nexit 42"); + fake_e2fs_installation(second.path()); + let path = std::env::join_paths([first.path(), second.path()]).expect("search path"); + let error = preflight_in(&std::env::split_paths(&path).collect::>()) + .await + .expect_err("real execution failure must not fall back"); + assert!(error.contains("42"), "{error}"); + assert!(error.contains("loader failed"), "{error}"); + assert!( + error.contains(&first.path().display().to_string()), + "{error}" + ); + } + + #[tokio::test] + async fn filesystem_preflight_rejects_incompatible_tool() { + let temp = tempfile::tempdir().expect("old installation"); + fake_e2fs_installation(temp.path()); + fake_e2fs_tool(temp.path(), "debugfs", "echo 'debugfs 1.42.13' >&2"); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("old tool must reject preparation"); + assert!( + error.contains("not a supported debugfs executable"), + "{error}" + ); + assert!(error.contains("debugfs 1.42.13"), "{error}"); + } + + #[tokio::test] + async fn filesystem_preflight_stops_a_hung_version_probe() { + let temp = tempfile::tempdir().expect("hung installation"); + fake_e2fs_tool(temp.path(), "mke2fs", "exec /bin/sleep 30"); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("preflight must finish even if a tool hangs"); + assert!(error.contains("timed out after 5 seconds"), "{error}"); + } + + #[tokio::test] + async fn filesystem_preflight_bounds_each_output_stream() { + let temp = tempfile::tempdir().expect("noisy installation"); + for (redirect, stream) in [("", "stdout"), (">&2", "stderr")] { + fake_e2fs_tool( + temp.path(), + "mke2fs", + &format!("/bin/dd if=/dev/zero bs=16384 count=4 {redirect} 2>/dev/null\nexit 42"), + ); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("oversized probe output must fail early"); + assert!(error.contains(&format!("{stream} exceeded 8192 bytes"))); + assert!( + error.len() < 9000, + "diagnostic grew to {} bytes", + error.len() + ); + } + } + + #[tokio::test] + async fn filesystem_preflight_timeout_terminates_wrapper_descendants() { + let temp = tempfile::tempdir().expect("wrapper installation"); + let marker = temp.path().join("survived-timeout"); + fake_e2fs_tool( + temp.path(), + "mke2fs", + &format!( + "(/bin/sleep 6; echo survived > '{}') &\nwait", + marker.display() + ), + ); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("wrapper must time out"); + assert!(error.contains("timed out after 5 seconds"), "{error}"); + tokio::time::sleep(Duration::from_millis(1300)).await; + assert!( + !marker.exists(), + "wrapper descendant survived its probe timeout" + ); + } + + #[tokio::test] + async fn filesystem_preflight_retains_exited_leader_until_held_pipe_cleanup() { + let temp = tempfile::tempdir().expect("wrapper installation"); + let marker = temp.path().join("survived-exited-leader"); + fake_e2fs_tool( + temp.path(), + "mke2fs", + &format!( + "(/bin/sleep 6; echo survived > '{}') &\necho 'mke2fs 1.47.4'\nexit 0", + marker.display() + ), + ); + let error = preflight_in(&[temp.path().to_path_buf()]) + .await + .expect_err("descendant holds output pipe"); + assert!(error.contains("timed out after 5 seconds"), "{error}"); + tokio::time::sleep(Duration::from_millis(1300)).await; + assert!( + !marker.exists(), + "descendant of exited probe leader survived cleanup" + ); + } + + #[tokio::test] + async fn filesystem_preflight_cancellation_terminates_wrapper_descendants() { + let temp = tempfile::tempdir().expect("wrapper installation"); + let ready = temp.path().join("child-ready"); + let marker = temp.path().join("survived-cancellation"); + fake_e2fs_tool( + temp.path(), + "mke2fs", + &format!( + "(echo ready > '{}'; /bin/sleep 6; echo survived > '{}') &\nwait", + ready.display(), + marker.display() + ), + ); + let path = temp.path().to_path_buf(); + let probe = tokio::spawn(async move { preflight_in(&[path]).await }); + for _ in 0..400 { + if ready.exists() { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + if !ready.exists() { + probe.abort(); + let result = probe.await; + panic!("probe descendant did not start: {result:?}"); + } + probe.abort(); + assert!(probe.await.expect_err("cancelled task").is_cancelled()); + tokio::time::sleep(Duration::from_millis(6300)).await; + assert!(!marker.exists(), "wrapper descendant survived cancellation"); + } + + #[test] + fn filesystem_preflight_requires_expected_identity_and_version() { + assert_eq!( + supported_version("mke2fs", "mke2fs 1.43 (test)"), + Some("1.43") + ); + for output in [ + "other 1.47.4", + "mke2fs unknown", + "mke2fs 1.42.13", + "mke2fs 1.47.garbage", + "mke2fs 1.47.4.5", + "", + ] { + assert_eq!(supported_version("mke2fs", output), None, "{output}"); + } + } +} diff --git a/crates/openshell-core/src/lib.rs b/crates/openshell-core/src/lib.rs index 472175f511..6f7a050f1c 100644 --- a/crates/openshell-core/src/lib.rs +++ b/crates/openshell-core/src/lib.rs @@ -17,6 +17,8 @@ pub mod denial; pub mod driver_mounts; pub mod driver_utils; pub mod dynamic_string_allowlist; +#[cfg(unix)] +pub mod e2fsprogs; pub mod endpoint_path; pub mod endpoint_status; pub mod error; diff --git a/crates/openshell-driver-vm/src/rootfs.rs b/crates/openshell-driver-vm/src/rootfs.rs index e1a09bcd98..cd4ee6307e 100644 --- a/crates/openshell-driver-vm/src/rootfs.rs +++ b/crates/openshell-driver-vm/src/rootfs.rs @@ -370,36 +370,20 @@ pub fn set_rootfs_image_file_mode( /// Replay the ext4 journal and repair automatically correctable filesystem /// state before the driver mutates a preserved guest disk offline. pub fn recover_rootfs_image(image_path: &Path) -> Result<(), String> { - let mut failures = Vec::new(); - let mut unavailable = Vec::new(); - - for candidate in e2fs_tool_candidates("e2fsck") { - let label = candidate.display().to_string(); - match Command::new(&candidate) - .arg("-p") - .arg("-f") - .arg(image_path) - .output() - { - Ok(output) if matches!(output.status.code(), Some(0..=2)) => return Ok(()), - Ok(output) => failures.push(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - unavailable.push(format!("{label} not found")); - } - Err(error) => failures.push(format!("run {label}: {error}")), - } + let mut command = e2fs_command("e2fsck")?; + let path = PathBuf::from(command.get_program()); + let output = command.arg("-p").arg("-f").arg(image_path).output(); + match output { + Ok(output) if matches!(output.status.code(), Some(0..=2)) => Ok(()), + Ok(output) => Err(format!( + "{} failed with status {}\nstdout: {}\nstderr: {}", + path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )), + Err(error) => Err(format!("run {}: {error}", path.display())), } - - Err(if failures.is_empty() { - unavailable.join("\n") - } else { - failures.join("\n") - }) } #[cfg(target_os = "macos")] @@ -668,75 +652,33 @@ fn round_up_to_mib(bytes: u64) -> u64 { bytes.div_ceil(MIB) * MIB } -enum FormatterAttempt { - Succeeded, - Failed(String), - Unavailable(String), -} - fn format_ext4_image_from_dir(source: &Path, image_path: &Path) -> Result<(), String> { - let candidates = ["mke2fs", "mkfs.ext4"] - .into_iter() - .flat_map(e2fs_tool_candidates); - run_ext4_formatter_candidates(candidates, |candidate| { - let label = candidate.display().to_string(); - let output = Command::new(candidate) - .arg("-q") - .arg("-F") - .arg("-t") - .arg("ext4") - .arg("-E") - .arg("root_owner=0:0") - .arg("-d") - .arg(source) - .arg(image_path) - .output(); - match output { - Ok(output) if output.status.success() => FormatterAttempt::Succeeded, - Ok(output) => FormatterAttempt::Failed(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - FormatterAttempt::Unavailable(format!("{label} not found")) - } - Err(err) => FormatterAttempt::Failed(format!("run {label}: {err}")), - } - }) - .map_err(|details| { - format!( - "failed to create ext4 rootfs image from {}: {details}. Install e2fsprogs (mke2fs/mkfs.ext4) and retry", - source.display() - ) - }) -} - -fn run_ext4_formatter_candidates( - candidates: impl IntoIterator, - mut run: impl FnMut(&Path) -> FormatterAttempt, -) -> Result<(), String> { - let mut failures = Vec::new(); - let mut unavailable = Vec::new(); - - for candidate in candidates { - match run(&candidate) { - FormatterAttempt::Succeeded => return Ok(()), - FormatterAttempt::Failed(error) => failures.push(error), - FormatterAttempt::Unavailable(error) => unavailable.push(error), - } - } - - if failures.is_empty() { - Err(if unavailable.is_empty() { - "no ext4 formatter candidates configured".to_string() - } else { - unavailable.join("\n") - }) - } else { - Err(failures.join("\n")) + let path = openshell_core::e2fsprogs::resolve(&["mke2fs", "mkfs.ext4"])?; + let output = Command::new(&path) + .arg("-q") + .arg("-F") + .arg("-t") + .arg("ext4") + .arg("-E") + .arg("root_owner=0:0") + .arg("-d") + .arg(source) + .arg(image_path) + .output() + .map_err(|error| format!("run {}: {error}", path.display()))?; + if output.status.success() { + return Ok(()); } + // A selected formatter failure must retain its diagnostics. Retrying with + // another installation can overwrite a partial image and hide the cause. + Err(format!( + "failed to create ext4 rootfs image from {}: {} failed with status {}\nstdout: {}\nstderr: {}", + source.display(), + path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )) } fn ensure_rootfs_image_parent_dirs(image_path: &Path, guest_path: &str) { @@ -885,52 +827,40 @@ pub fn ext4_image_has_directory(image_path: &Path, guest_path: &str) -> Result { - // debugfs exits 0 whether or not the path exists; the answer - // is only in its output. - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - if stdout.contains("Type: directory") { - return Ok(true); - } - if stdout.contains("Type: ") || stderr.contains("File not found") { - return Ok(false); - } - return Err(format!( - "debugfs command '{command}' produced unrecognized output for {}\nstdout: {stdout}\nstderr: {stderr}", - image_path.display() - )); + let output = e2fs_command("debugfs")? + .arg("-R") + .arg(&command) + .arg(image_path) + .output(); + match output { + Ok(output) if output.status.success() => { + // debugfs exits 0 whether or not the path exists; the answer + // is only in its output. + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + if stdout.contains("Type: directory") { + return Ok(true); } - Ok(output) => { - last_error = Some(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )); + if stdout.contains("Type: ") || stderr.contains("File not found") { + return Ok(false); } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - last_error = Some(format!("{label} not found")); - } - Err(error) => last_error = Some(format!("run {label}: {error}")), + Err(format!( + "debugfs command '{command}' produced unrecognized output for {}\nstdout: {stdout}\nstderr: {stderr}", + image_path.display() + )) } + Ok(output) => Err(format!( + "debugfs command '{command}' failed for {}: debugfs failed with status {}\nstdout: {}\nstderr: {}. Install e2fsprogs (debugfs) and retry", + image_path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )), + Err(error) => Err(format!( + "debugfs command '{command}' failed for {}: {error}. Install e2fsprogs (debugfs) and retry", + image_path.display() + )), } - - Err(format!( - "debugfs command '{command}' failed for {}: {}. Install e2fsprogs (debugfs) and retry", - image_path.display(), - last_error.unwrap_or_else(|| "debugfs not found".to_string()) - )) } fn sandbox_guest_user_ids_from_image_path( @@ -949,45 +879,33 @@ fn sandbox_guest_user_ids_from_image_path( let quoted_path = debugfs_quote_absolute_path(guest_path) .expect("the static passwd path is a valid debugfs path"); let command = format!("cat {quoted_path}"); - let mut last_error = None; - - for candidate in e2fs_tool_candidates("debugfs") { - let label = candidate.display().to_string(); - match Command::new(&candidate) - .arg("-R") - .arg(&command) - .arg(image_path) - .output() - { - Ok(output) if output.status.success() => { - let passwd = String::from_utf8(output.stdout).map_err(|error| { - format!( - "read {guest_path} from {} as UTF-8: {error}", - image_path.display() - ) - })?; - return parse_sandbox_guest_user_ids(&passwd, &image_path.display().to_string()); - } - Ok(output) => { - last_error = Some(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )); - } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - last_error = Some(format!("{label} not found")); - } - Err(error) => last_error = Some(format!("run {label}: {error}")), + let output = e2fs_command("debugfs")? + .arg("-R") + .arg(&command) + .arg(image_path) + .output(); + match output { + Ok(output) if output.status.success() => { + let passwd = String::from_utf8(output.stdout).map_err(|error| { + format!( + "read {guest_path} from {} as UTF-8: {error}", + image_path.display() + ) + })?; + parse_sandbox_guest_user_ids(&passwd, &image_path.display().to_string()) } + Ok(output) => Err(format!( + "debugfs command '{command}' failed for {}: debugfs failed with status {}\nstdout: {}\nstderr: {}. Install e2fsprogs (debugfs) and retry", + image_path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )), + Err(error) => Err(format!( + "debugfs command '{command}' failed for {}: {error}. Install e2fsprogs (debugfs) and retry", + image_path.display() + )), } - - Err(format!( - "debugfs command '{command}' failed for {}: {}. Install e2fsprogs (debugfs) and retry", - image_path.display(), - last_error.unwrap_or_else(|| "debugfs not found".to_string()) - )) } fn sandbox_guest_user_ids(rootfs: &Path) -> Result, String> { @@ -1037,83 +955,57 @@ fn run_debugfs_batch(image_path: &Path, commands: &[String]) -> Result<(), Strin } fn run_debugfs_batch_file(image_path: &Path, command_path: &Path) -> Result<(), String> { - let mut last_error = None; - for candidate in e2fs_tool_candidates("debugfs") { - let label = candidate.display().to_string(); - let output = Command::new(&candidate) - .arg("-w") - .arg("-f") - .arg(command_path) - .arg(image_path) - .output(); - match output { - Ok(output) if output.status.success() => return Ok(()), - Ok(output) => { - last_error = Some(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )); - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - last_error = Some(format!("{label} not found")); - } - Err(err) => { - last_error = Some(format!("run {label}: {err}")); - } - } + let output = e2fs_command("debugfs")? + .arg("-w") + .arg("-f") + .arg(command_path) + .arg(image_path) + .output(); + match output { + Ok(output) if output.status.success() => Ok(()), + Ok(output) => Err(format!( + "debugfs batch {} failed for {}: debugfs failed with status {}\nstdout: {}\nstderr: {}. Install e2fsprogs (debugfs) and retry", + command_path.display(), + image_path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )), + Err(error) => Err(format!( + "debugfs batch {} failed for {}: {error}. Install e2fsprogs (debugfs) and retry", + command_path.display(), + image_path.display() + )), } - Err(format!( - "debugfs batch {} failed for {}: {}. Install e2fsprogs (debugfs) and retry", - command_path.display(), - image_path.display(), - last_error.unwrap_or_else(|| "debugfs not found".to_string()) - )) } fn run_debugfs(image_path: &Path, command: &str) -> Result<(), String> { - let mut last_error = None; - for candidate in e2fs_tool_candidates("debugfs") { - let label = candidate.display().to_string(); - let output = Command::new(&candidate) - .arg("-w") - .arg("-R") - .arg(command) - .arg(image_path) - .output(); - match output { - Ok(output) if output.status.success() => return Ok(()), - Ok(output) => { - last_error = Some(format!( - "{label} failed with status {}\nstdout: {}\nstderr: {}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )); - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - last_error = Some(format!("{label} not found")); - } - Err(err) => { - last_error = Some(format!("run {label}: {err}")); - } - } + let output = e2fs_command("debugfs")? + .arg("-w") + .arg("-R") + .arg(command) + .arg(image_path) + .output(); + match output { + Ok(output) if output.status.success() => Ok(()), + Ok(output) => Err(format!( + "debugfs command '{command}' failed for {}: debugfs failed with status {}\nstdout: {}\nstderr: {}. Install e2fsprogs (debugfs) and retry", + image_path.display(), + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )), + Err(error) => Err(format!( + "debugfs command '{command}' failed for {}: {error}. Install e2fsprogs (debugfs) and retry", + image_path.display() + )), } - Err(format!( - "debugfs command '{command}' failed for {}: {}. Install e2fsprogs (debugfs) and retry", - image_path.display(), - last_error.unwrap_or_else(|| "debugfs not found".to_string()) - )) } -fn e2fs_tool_candidates(tool: &str) -> Vec { - let mut candidates = vec![PathBuf::from(tool)]; - for root in ["/opt/homebrew/opt/e2fsprogs", "/usr/local/opt/e2fsprogs"] { - candidates.push(Path::new(root).join("sbin").join(tool)); - candidates.push(Path::new(root).join("bin").join(tool)); - } - candidates +fn e2fs_command(tool: &str) -> Result { + // Preflight and image operations must execute the same selected file with + // the same inherited environment, including when PATH is restricted. + Ok(Command::new(openshell_core::e2fsprogs::resolve(&[tool])?)) } fn temporary_injection_path(image_path: &Path) -> PathBuf { @@ -1558,10 +1450,7 @@ mod tests { #[test] fn recover_rootfs_image_accepts_clean_ext4_image() { - if !e2fs_tool_candidates("e2fsck") - .iter() - .any(|candidate| Command::new(candidate).arg("-V").output().is_ok()) - { + if e2fs_command("e2fsck").is_err() { return; } @@ -1580,10 +1469,7 @@ mod tests { #[test] fn ext4_image_has_directory_distinguishes_directories_files_and_missing_paths() { - if !e2fs_tool_candidates("debugfs") - .iter() - .any(|candidate| Command::new(candidate).arg("-V").output().is_ok()) - { + if e2fs_command("debugfs").is_err() { return; } @@ -1668,58 +1554,90 @@ mod tests { assert_eq!(debugfs_quote_argument("/tmp/bad\npath"), None); } - #[test] - fn formatter_candidates_preserve_executed_failure_over_missing_fallback() { - let candidates = vec![PathBuf::from("mke2fs"), PathBuf::from("missing")]; - - let err = run_ext4_formatter_candidates(candidates, |candidate| { - if candidate == Path::new("mke2fs") { - FormatterAttempt::Failed( - "mke2fs failed with status 1\nstdout: formatter output\nstderr: no space left" - .to_string(), - ) - } else { - FormatterAttempt::Unavailable("missing not found".to_string()) - } - }) - .expect_err("formatter should fail"); - - assert!(err.contains("mke2fs failed with status 1")); - assert!(err.contains("no space left")); - assert!(!err.contains("missing not found")); + fn run_tool_fixture_test(test: &str, directory: &Path) { + // Isolate PATH in a child test process. Other tests may prepare images + // concurrently and must never observe this deliberately broken tool. + let output = Command::new(std::env::current_exe().expect("test executable")) + .args(["--exact", test, "--nocapture"]) + .env("PATH", directory) + .env("OPENSHELL_ROOTFS_TOOL_TEST_ROOT", directory) + .output() + .expect("isolated tool test"); + assert!( + output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); } #[test] - fn formatter_candidates_report_all_missing_tools() { - let candidates = vec![PathBuf::from("mke2fs"), PathBuf::from("mkfs.ext4")]; - - let err = run_ext4_formatter_candidates(candidates, |candidate| { - FormatterAttempt::Unavailable(format!("{} not found", candidate.display())) - }) - .expect_err("formatter should be unavailable"); + fn formatter_preserves_selected_tool_failure_without_retry() { + use std::os::unix::fs::PermissionsExt as _; - assert!(err.contains("mke2fs not found")); - assert!(err.contains("mkfs.ext4 not found")); + if let Some(directory) = std::env::var_os("OPENSHELL_ROOTFS_TOOL_TEST_ROOT") { + let directory = PathBuf::from(directory); + let image = directory.join("rootfs.ext4"); + File::create(&image) + .expect("image") + .set_len(16 * 1024 * 1024) + .expect("image size"); + let source = directory.join("source"); + fs::create_dir(&source).expect("source directory"); + let error = format_ext4_image_from_dir(&source, &image) + .expect_err("the selected formatter failure must not select another installation"); + assert!(error.contains("selected-formatter-failed"), "{error}"); + assert!(error.contains("42"), "{error}"); + return; + } + let directory = tempfile::tempdir().expect("tool installation"); + for (name, body) in [ + ("mke2fs", "echo selected-formatter-failed >&2\nexit 42"), + ("mkfs.ext4", "exit 0"), + ] { + let path = directory.path().join(name); + fs::write(&path, format!("#!/bin/sh\n{body}\n")).expect("tool fixture"); + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).expect("executable"); + } + run_tool_fixture_test( + "rootfs::tests::formatter_preserves_selected_tool_failure_without_retry", + directory.path(), + ); } #[test] - fn formatter_candidates_accept_successful_fallback() { - let candidates = vec![PathBuf::from("first"), PathBuf::from("second")]; - let mut attempted = Vec::new(); - - run_ext4_formatter_candidates(candidates, |candidate| { - attempted.push(candidate.to_path_buf()); - if candidate == Path::new("second") { - FormatterAttempt::Succeeded - } else { - FormatterAttempt::Failed("first failed".to_string()) - } - }) - .expect("fallback should succeed"); + fn recovery_preserves_selected_path_and_execution_errors() { + use std::os::unix::fs::PermissionsExt as _; - assert_eq!( - attempted, - vec![PathBuf::from("first"), PathBuf::from("second")] + if let Some(directory) = std::env::var_os("OPENSHELL_ROOTFS_TOOL_TEST_ROOT") { + let directory = PathBuf::from(directory); + let path = directory.join("e2fsck"); + for (script, expected) in [ + ( + "#!/nonexistent/e2fsprogs-interpreter\n", + "No such file or directory", + ), + ( + "#!/bin/sh\necho recovery-failed >&2\nexit 4\n", + "recovery-failed", + ), + ] { + fs::write(&path, script).expect("recovery tool"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("executable"); + let error = recover_rootfs_image(&directory.join("overlay.ext4")) + .expect_err("recovery failure"); + assert!( + error.contains(path.canonicalize().unwrap().to_str().unwrap()), + "{error}" + ); + assert!(error.contains(expected), "{error}"); + } + return; + } + let directory = tempfile::tempdir().expect("tool installation"); + run_tool_fixture_test( + "rootfs::tests::recovery_preserves_selected_path_and_execution_errors", + directory.path(), ); } diff --git a/crates/openshell-gateway/src/lib.rs b/crates/openshell-gateway/src/lib.rs index 8864e85ede..dbdf46c2b2 100644 --- a/crates/openshell-gateway/src/lib.rs +++ b/crates/openshell-gateway/src/lib.rs @@ -398,6 +398,15 @@ impl openshell_server::ComputeDriverFactory for VmFactory { true } + async fn preflight_host_tools( + &self, + cancellation: tokio::sync::watch::Receiver, + ) -> openshell_core::Result> { + openshell_core::e2fsprogs::preflight(cancellation) + .await + .map_err(openshell_core::Error::config) + } + fn validate_config( &self, context: openshell_server::ComputeDriverConfigContext<'_>, diff --git a/crates/openshell-gateway/tests/config_preflight.rs b/crates/openshell-gateway/tests/config_preflight.rs new file mode 100644 index 0000000000..3c404c672d --- /dev/null +++ b/crates/openshell-gateway/tests/config_preflight.rs @@ -0,0 +1,273 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#![cfg(all(unix, feature = "compute-driver-vm"))] + +use std::fs; +use std::os::unix::fs::PermissionsExt as _; +use std::path::{Path, PathBuf}; +use std::process::Output; +use std::time::Duration; + +struct Fixture { + root: tempfile::TempDir, + tools: PathBuf, + config: PathBuf, +} + +impl Fixture { + fn new() -> Self { + let root = tempfile::tempdir().expect("fixture root"); + let tools = root.path().join("tools"); + fs::create_dir(&tools).expect("tools directory"); + let config = root.path().join("gateway.toml"); + let fixture = Self { + root, + tools, + config, + }; + fixture.write_config(Some("vm")); + for name in ["mke2fs", "debugfs", "e2fsck"] { + fixture.tool(name, &format!( + "test \"$1\" = -V || exit 64\ntest \"$#\" = 1 || exit 65\nread ignored && exit 66\ntest \"$PREFLIGHT_TEST_ENV\" = inherited || exit 67\necho '{name} 1.47.4' >&2" + )); + } + fixture + } + + fn write_config(&self, driver: Option<&str>) { + let selector = + driver.map_or_else(String::new, |name| format!("compute_driver = {name:?}\n")); + fs::write(&self.config, format!( + "[openshell]\nversion = 2\n[openshell.gateway]\ndisable_tls = true\n{selector}[openshell.drivers.vm]\nbootstrap_image = 'unreachable.invalid/vm:must-not-pull'\nstate_dir = {:?}\n", + self.root.path().join("vm-state") + )).expect("gateway configuration"); + } + + fn tool(&self, name: &str, body: &str) { + let path = self.tools.join(name); + fs::write(&path, format!("#!/bin/sh\n{body}\n")).expect("tool fixture"); + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).expect("executable fixture"); + } + + fn command(&self, replay: &[&str]) -> tokio::process::Command { + let mut command = tokio::process::Command::new(env!("CARGO_BIN_EXE_openshell-gateway")); + command + .env_clear() + .env("HOME", self.root.path()) + .env("PATH", &self.tools) + .env("XDG_CONFIG_HOME", self.root.path().join("config-home")) + .env("XDG_STATE_HOME", self.root.path().join("state-home")) + .env("PREFLIGHT_TEST_ENV", "inherited") + .args(["config", "preflight"]) + .kill_on_drop(true); + if replay.is_empty() { + command.arg("--path").arg(&self.config); + } else { + command + .arg("--") + .arg("--config") + .arg(&self.config) + .args(replay); + } + command + } + + async fn run(&self, replay: &[&str]) -> Output { + let original_config = fs::read(&self.config).expect("original config"); + let mut command = self.command(replay); + let output = tokio::time::timeout(Duration::from_secs(15), command.output()) + .await + .expect("preflight must finish") + .expect("run gateway preflight"); + assert_eq!( + fs::read(&self.config).expect("unchanged config"), + original_config + ); + for name in ["vm-state", "state-home", "config-home", ".local"] { + assert!( + !self.root.path().join(name).exists(), + "preflight created {name}" + ); + } + output + } +} + +fn combined(output: &Output) -> String { + format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ) +} + +#[tokio::test] +async fn local_vm_reports_tools_without_starting_driver_or_creating_state() { + let fixture = Fixture::new(); + let output = fixture.run(&[]).await; + assert!(output.status.success(), "{}", combined(&output)); + let report = String::from_utf8_lossy(&output.stdout); + for name in ["mke2fs", "debugfs", "e2fsck"] { + let path = fixture + .tools + .join(name) + .canonicalize() + .expect("selected path"); + assert!( + report.contains(path.to_str().expect("UTF-8 path")), + "{report}" + ); + } + // PATH contains only the filesystem fixtures, never a VM driver binary. + assert!(!fixture.tools.join("openshell-driver-vm").exists()); +} + +#[tokio::test] +async fn config_file_retains_selected_tool_error_and_corrective_guidance() { + let fixture = Fixture::new(); + fixture.tool("debugfs", "echo 'fixture loader failure' >&2\nexit 42"); + let output = fixture.run(&[]).await; + assert!(!output.status.success()); + let report = combined(&output); + for expected in [ + "debugfs", + "fixture loader failure", + "42", + "gateway service PATH", + "mke2fs", + ] { + assert!(report.contains(expected), "missing {expected}: {report}"); + } + assert!( + report.contains(fixture.tools.to_str().expect("tool path")), + "{report}" + ); +} + +#[tokio::test] +async fn local_vm_rejects_nonexecutable_and_unsupported_tools() { + let fixture = Fixture::new(); + fs::set_permissions( + fixture.tools.join("mke2fs"), + fs::Permissions::from_mode(0o644), + ) + .unwrap(); + let output = fixture.run(&[]).await; + assert!(!output.status.success()); + assert!( + combined(&output).contains("not an executable file"), + "{}", + combined(&output) + ); + + fixture.tool("mke2fs", "echo 'mke2fs 1.42.13' >&2"); + let output = fixture.run(&[]).await; + assert!(!output.status.success()); + // The diagnostic renderer wraps long selected paths and error text. + let report = combined(&output) + .split_whitespace() + .filter(|word| *word != "│") + .collect::>() + .join(" "); + assert!(report.contains("not a supported mke2fs"), "{report}"); +} + +#[tokio::test] +async fn local_vm_rejects_hanging_tool_with_deadline() { + let fixture = Fixture::new(); + fixture.tool("mke2fs", "exec /bin/sleep 30"); + let start = std::time::Instant::now(); + let output = fixture.run(&[]).await; + assert!(!output.status.success()); + assert!( + combined(&output).contains("timed out after 5 seconds"), + "{}", + combined(&output) + ); + assert!(start.elapsed() < Duration::from_secs(12)); +} + +#[tokio::test] +async fn signals_stop_probe_wrapper_and_descendants_before_cli_exit() { + use nix::sys::signal::{Signal, kill}; + use nix::unistd::Pid; + use std::process::Stdio; + + for signal in [Signal::SIGINT, Signal::SIGTERM] { + let fixture = Fixture::new(); + let ready = fixture.root.path().join("probe-ready"); + let survived = fixture.root.path().join("survived-signal"); + fixture.tool( + "mke2fs", + &format!( + "(echo ready > '{}'; /bin/sleep 2; echo survived > '{}') &\nwait", + ready.display(), + survived.display() + ), + ); + let child = fixture + .command(&[]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("preflight process"); + let pid = + Pid::from_raw(i32::try_from(child.id().expect("gateway PID")).expect("valid PID")); + tokio::time::timeout(Duration::from_secs(5), async { + while !ready.exists() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("probe descendant ready"); + kill(pid, signal).expect("signal gateway"); + let output = tokio::time::timeout(Duration::from_secs(5), child.wait_with_output()) + .await + .expect("signal cleanup deadline") + .expect("preflight exit"); + assert!(!output.status.success()); + tokio::time::sleep(Duration::from_millis(2200)).await; + assert!(!survived.exists(), "probe descendant survived {signal}"); + assert!( + combined(&output).contains(&format!("interrupted by {signal}")), + "{}", + combined(&output) + ); + assert!(!fixture.root.path().join("vm-state").exists()); + } +} + +#[tokio::test] +async fn remote_vm_reports_unperformed_checks_without_running_local_tools() { + let fixture = Fixture::new(); + fixture.tool("mke2fs", "echo 'local tool must not run' >&2\nexit 42"); + let socket = fixture.root.path().join("absent-remote.sock"); + let output = fixture + .run(&["--compute-driver-socket", socket.to_str().unwrap()]) + .await; + assert!(output.status.success(), "{}", combined(&output)); + assert!(combined(&output).contains("host tool checks not performed for a remote endpoint")); + assert!(!combined(&output).contains("local tool must not run")); + assert!(!Path::new(&socket).exists()); +} + +#[tokio::test] +async fn vm_table_without_selection_does_not_probe_tools() { + let fixture = Fixture::new(); + fixture.write_config(None); + fixture.tool("mke2fs", "exit 42"); + let output = fixture.run(&[]).await; + assert!(output.status.success(), "{}", combined(&output)); + assert!(!combined(&output).contains("VM host tool")); +} + +#[cfg(feature = "compute-driver-docker")] +#[tokio::test] +async fn unrelated_driver_does_not_require_vm_tools() { + let fixture = Fixture::new(); + fixture.tool("mke2fs", "exit 42"); + let output = fixture.run(&["--compute-driver", "docker"]).await; + assert!(output.status.success(), "{}", combined(&output)); + assert!(!combined(&output).contains("VM host tool")); +} diff --git a/crates/openshell-server/src/cli.rs b/crates/openshell-server/src/cli.rs index 9000ab1324..6315c35f9a 100644 --- a/crates/openshell-server/src/cli.rs +++ b/crates/openshell-server/src/cli.rs @@ -287,7 +287,12 @@ pub async fn run_cli_with_compute_drivers(compute_drivers: ComputeDriverRegistry Some(Commands::GenerateCerts(args)) => certgen::run(args).await, Some(Commands::Config(args)) => match args.command { ConfigCommand::Preflight(args) => { - run_config_preflight_with_drivers(args, cli.run, &matches, &compute_drivers) + let driver = + run_config_preflight_with_drivers(args, cli.run, &matches, &compute_drivers)?; + for report in preflight_host_tools(driver).await? { + println!("{report}"); + } + Ok(()) } }, None => Box::pin(run_from_args(cli.run, matches, compute_drivers)).await, @@ -760,7 +765,7 @@ fn run_config_preflight( Some(detect_preflight_test_driver), PreflightTestFactory, )?)?; - run_config_preflight_with_drivers(args, run, matches, ®istry) + run_config_preflight_with_drivers(args, run, matches, ®istry).map(|_| ()) } fn run_config_preflight_with_drivers( @@ -768,7 +773,7 @@ fn run_config_preflight_with_drivers( run: RunArgs, matches: &ArgMatches, compute_drivers: &ComputeDriverRegistry, -) -> Result<()> { +) -> Result> { if args.gateway_args.is_empty() { return run_effective_config_preflight(args.path, run, matches, compute_drivers); } @@ -783,7 +788,7 @@ fn run_config_preflight_with_drivers( clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion ) => { - return Ok(()); + return Ok(None); } Err(error) => return Err(miette::miette!("{error}")), }; @@ -792,7 +797,7 @@ fn run_config_preflight_with_drivers( if replay.command.is_some() { // A valid non-daemon action does not consume gateway startup // configuration. Let the immediately following invocation perform it. - return Ok(()); + return Ok(None); } run_effective_config_preflight(None, replay.run, &replay_matches, compute_drivers) } @@ -802,7 +807,7 @@ fn run_effective_config_preflight( mut run: RunArgs, matches: &ArgMatches, compute_drivers: &ComputeDriverRegistry, -) -> Result<()> { +) -> Result> { let path = if path_override.is_some() { path_override } else { @@ -850,8 +855,9 @@ fn run_effective_config_preflight( gateway_tls_enabled: !run.disable_tls, endpoint_overrides: &endpoint_overrides, }; + let mut selected_driver = None; if let Some(selection) = selection.as_ref() { - crate::validate_compute_driver_config( + selected_driver = Some(crate::validate_compute_driver_config( compute_drivers, selection.name(), run.name.trim(), @@ -859,7 +865,7 @@ fn run_effective_config_preflight( &run.log_level, driver_startup, true, - )?; + )?); } else if file.is_some() { // Runtime auto-detection may connect local API sockets or launch a // bounded discovery command. Preflight must not perform those @@ -881,11 +887,11 @@ fn run_effective_config_preflight( )?; } } - Ok(()) + Ok(selected_driver) })(); match (validation, path.as_ref()) { - (Ok(()), _) => Ok(()), + (Ok(driver), _) => Ok(driver), (Err(_), Some(path)) => Err(miette::miette!( "{}", config_file::ConfigPreflightError::invalid_current(path) @@ -894,6 +900,56 @@ fn run_effective_config_preflight( } } +/// Run executable probes outside the pure configuration-validation context. +async fn preflight_host_tools( + driver: Option, +) -> Result> { + match driver { + Some(crate::ConfiguredComputeDriver::Registered(registration)) => { + let (cancellation_tx, cancellation_rx) = tokio::sync::watch::channel(false); + #[cfg(unix)] + { + use tokio::signal::unix::{SignalKind, signal}; + + // Register before polling the hook: a probe owns a separate + // process group, so default CLI termination cannot clean it up. + let mut interrupt = signal(SignalKind::interrupt()) + .map_err(|error| miette::miette!("register preflight SIGINT: {error}"))?; + let mut terminate = signal(SignalKind::terminate()) + .map_err(|error| miette::miette!("register preflight SIGTERM: {error}"))?; + let check = registration.factory.preflight_host_tools(cancellation_rx); + tokio::pin!(check); + let reason = tokio::select! { + biased; + _ = interrupt.recv() => "SIGINT", + _ = terminate.recv() => "SIGTERM", + result = &mut check => return result.map_err(|error| miette::miette!("{error}")), + }; + cancellation_tx.send_replace(true); + // The hook owns its children. Await its cancellation cleanup + // before the short-lived CLI shuts down the Tokio runtime. + let _ = check.await; + Err(miette::miette!( + "host tool preflight interrupted by {reason}" + )) + } + #[cfg(not(unix))] + { + let _cancellation_tx = cancellation_tx; + registration + .factory + .preflight_host_tools(cancellation_rx) + .await + .map_err(|error| miette::miette!("{error}")) + } + } + Some(crate::ConfiguredComputeDriver::Remote { name }) => Ok(vec![format!( + "compute driver '{name}': host tool checks not performed for a remote endpoint; run preflight on the driver host with its service account and environment" + )]), + None => Ok(Vec::new()), + } +} + fn validate_preflight_semantics( args: &RunArgs, matches: &ArgMatches, diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index d6d35f6ba3..e33502cb69 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -1268,6 +1268,21 @@ pub trait ComputeDriverFactory: Send + Sync { false } + /// Check locally installed host tools after configuration validation. + /// + /// Only the explicit `config preflight` command calls this hook. Probes + /// must bound time and output, clean up on cancellation, and avoid driver + /// startup, transport connections, images, and runtime state. Return + /// operator-readable results including the selected executable paths. + /// The process inherits the gateway's account and environment. When + /// `cancellation` becomes true, finish process cleanup before returning. + async fn preflight_host_tools( + &self, + _cancellation: watch::Receiver, + ) -> Result> { + Ok(Vec::new()) + } + async fn build(&self, context: ComputeDriverBuildContext<'_>) -> Result; } diff --git a/deploy/man/openshell-gateway.8.md b/deploy/man/openshell-gateway.8.md index 9be010095a..4bb00717ed 100644 --- a/deploy/man/openshell-gateway.8.md +++ b/deploy/man/openshell-gateway.8.md @@ -120,7 +120,7 @@ Validate a gateway configuration before starting the daemon: With no path, preflight validates a nonempty OPENSHELL_GATEWAY_CONFIG. If that variable is unset, it optionally validates an auto-discovered XDG config. The -absence of either config succeeds. An explicit missing path, legacy schema-v1 +absence of either config still validates the effective daemon arguments. An explicit missing path, legacy schema-v1 file, invalid TOML, symlink, or nonregular file fails with a nonzero status. Preflight merges file and environment values and applies read-only startup checks for selector and socket normalization, registered compute-driver configuration, @@ -134,6 +134,10 @@ Arguments after **--** replace **--path** mode and are parsed as the exact gatew daemon invocation. Package wrappers use this form so command-line overrides are validated before the same arguments reach startup. +An explicitly selected local **vm** driver also checks **mke2fs** or **mkfs.ext4**, **debugfs**, and **e2fsck**. Install e2fsprogs 1.43 or newer with the operating system's package manager, then run preflight with the gateway service's account, working directory, configuration, and environment. The command reports selected executable paths and versions. A restricted service **PATH** can select different tools from an interactive shell; include the installation's bin and sbin directories in that service's environment. + +Each executable receives only **-V**, with a five-second deadline and an 8 KiB output limit per stream. Missing, non-executable, unsupported, or failing tools return nonzero status with installation or repair guidance. Preflight creates no images or runtime state and does not start the VM driver. Other drivers do not require these tools. A remote driver endpoint reports that host tool checks were not performed; check a local VM configuration on that host in the driver service's environment. + The Debian and Ubuntu systemd user unit runs preflight before certificate generation, while retaining its EnvironmentFile and bare ExecStart behavior. The Snap wrapper replays its effective daemon arguments through preflight. It first diff --git a/docs/how-it-works/gateways/configuration.mdx b/docs/how-it-works/gateways/configuration.mdx index 3b3c3aaaf0..f0e9ec0bac 100644 --- a/docs/how-it-works/gateways/configuration.mdx +++ b/docs/how-it-works/gateways/configuration.mdx @@ -1248,22 +1248,7 @@ changing it: openshell-gateway config preflight --path ~/.config/openshell/gateway.toml ``` -Without `--path`, the command validates a nonempty `OPENSHELL_GATEWAY_CONFIG`. -Otherwise, it validates an existing XDG gateway config when one is discovered. -When neither source selects a config, preflight succeeds. An explicit missing path, -a legacy schema-v1 file, invalid TOML, a symlink, or any nonregular file fails. -Preflight merges the selected file with the current `OPENSHELL_*` environment and -applies the daemon's read-only startup checks. These checks include selector and -socket normalization, registered-driver selection and configuration, rate-limit -pairs, TLS and mTLS relationships, interceptor registrations, and supervisor -middleware registrations. When a selected file omits `compute_driver`, preflight -validates each configured table for an auto-detectable driver without running the -runtime detection probes, which can connect local sockets or launch discovery -commands. It validates complete guest TLS path sets without requiring -package-generated certificates to exist before certificate generation. It does -not construct a compute driver or connect to a transport. A failed -preflight always preserves the file; it never migrates, replaces, or rewrites -configuration. +Without `--path`, the command validates a nonempty `OPENSHELL_GATEWAY_CONFIG`. Otherwise, it validates an existing XDG gateway config when one is discovered. When neither source selects a config, preflight validates the effective daemon arguments. An explicit missing path, a legacy schema-v1 file, invalid TOML, a symlink, or any nonregular file fails. Preflight merges the selected file with the current `OPENSHELL_*` environment and applies the daemon's read-only startup checks. These checks include selector and socket normalization, registered-driver selection and configuration, rate-limit pairs, TLS and mTLS relationships, interceptor registrations, and supervisor middleware registrations. When a selected file omits `compute_driver`, preflight validates each configured table for an auto-detectable driver without running the runtime detection probes, which can connect local sockets or launch discovery commands. It validates complete guest TLS path sets without requiring package-generated certificates to exist before certificate generation. It does not construct a compute driver or connect to a transport. A failed preflight always preserves the file; it never migrates, replaces, or rewrites configuration. To validate the exact daemon arguments that a wrapper will pass, place them after `--` instead of using `--path`: @@ -1290,3 +1275,26 @@ $EDITOR ~/.config/openshell/gateway.toml openshell-gateway config preflight --path ~/.config/openshell/gateway.toml systemctl --user restart openshell-gateway ``` + +### Check local VM host tools + +For an explicitly selected local `vm` driver, preflight also checks a formatter (`mke2fs` or `mkfs.ext4`), `debugfs`, and `e2fsck`. Install e2fsprogs 1.43 or newer with your operating system's package manager. OpenShell does not install or manage these host dependencies. + +For example, on macOS: + +```shell +brew install e2fsprogs +openshell-gateway config preflight -- --config ~/.config/openshell/gateway.toml --compute-driver vm +``` + +Run preflight with the account, configuration, working directory, and environment intended for the gateway. A successful check in an interactive shell does not check a service with a different `PATH`. For a service whose environment uses a private installation, reproduce its configured path explicitly: + +```shell +env PATH=/opt/company/e2fsprogs/sbin:/opt/company/e2fsprogs/bin:/usr/bin:/bin /usr/local/bin/openshell-gateway config preflight -- --config ~/.config/openshell/gateway.toml --compute-driver vm +``` + +Replace the executable and configuration paths with your installation's paths, and run this command as the service account. Preflight and VM image operations search inherited `PATH` entries, then the existing e2fsprogs `sbin` and `bin` directories under `/opt/homebrew/opt/e2fsprogs` and `/usr/local/opt/e2fsprogs`. They prefer `mke2fs` and try `mkfs.ext4` only when `mke2fs` is absent. A selected file that cannot run is an error; fix that installation or the service's `PATH` before rerunning the command. + +The command reports each selected executable path and version. Each tool receives only `-V`, with a five-second deadline and an 8 KiB output limit per stream. Missing, non-executable, outdated, or failing tools return a nonzero exit status with corrective guidance and the selected tool's error. The checks create no gateway state or images and do not start a driver or VM. Version checks confirm the installation; they do not test image creation or filesystem recovery. + +Other drivers do not require these VM tools. A VM configuration table alone does not select the VM driver. When a remote driver socket is configured, preflight reports that host tool checks were not performed; run the check on that host in the driver service's environment with a local VM configuration. diff --git a/skills/debug-openshell-cluster/SKILL.md b/skills/debug-openshell-cluster/SKILL.md index 6382428946..dd65cd4d00 100644 --- a/skills/debug-openshell-cluster/SKILL.md +++ b/skills/debug-openshell-cluster/SKILL.md @@ -896,8 +896,7 @@ Use the VM driver logs and host diagnostics available in the user's environment. - The VM driver process is running and reachable by the gateway. - The runtime rootfs exists and matches the expected architecture. -- `mke2fs` or `mkfs.ext4` and `debugfs` from e2fsprogs are installed; explicit - `sandbox_uid`/`sandbox_gid` does not remove this prerequisite. +- `mke2fs` or `mkfs.ext4`, `debugfs`, and `e2fsck` from e2fsprogs are installed. Run `openshell-gateway config preflight` with the intended local VM configuration, service account, and environment to check the selected paths and versions before startup. A remote endpoint reports that host checks were not performed. Explicit `sandbox_uid`/`sandbox_gid` does not remove this prerequisite. - A persisted overlay identity error is resolved from its owner marker, overlay upper layer, prepared rootfs, explicit config, or current image. Do not assign `10001:10001` unless the persisted state reports that legacy identity. From ee75f43c9f0dc5be1d817840ee24f926e12ff9a7 Mon Sep 17 00:00:00 2001 From: Shiju Date: Thu, 1 Oct 2026 14:37:38 +0530 Subject: [PATCH 2/3] fix(gateway): stabilize filesystem preflight checks Combine identical filesystem-tool error arms and normalize rendered diagnostics in command tests so terminal wrapping preserves assertions. Describe driver TLS validation without depending on removed guest fields. Signed-off-by: Shiju --- crates/openshell-core/src/e2fsprogs.rs | 3 +-- .../tests/config_preflight.rs | 23 ++++++++++--------- docs/how-it-works/gateways/configuration.mdx | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/openshell-core/src/e2fsprogs.rs b/crates/openshell-core/src/e2fsprogs.rs index 29869bb2e6..d6f4c5ddc4 100644 --- a/crates/openshell-core/src/e2fsprogs.rs +++ b/crates/openshell-core/src/e2fsprogs.rs @@ -277,8 +277,7 @@ async fn run_version_probe( } } (Ok(None), status) | (Err(_), status @ Err(_)) => status, - (Err(error), Ok(_)) => Err(error), - (Ok(Some(_)), Err(error)) => Err(error), + (Err(error), Ok(_)) | (Ok(Some(_)), Err(error)) => Err(error), }; match (output, status) { (Ok((stdout, stderr)), Ok(status)) => Ok(std::process::Output { diff --git a/crates/openshell-gateway/tests/config_preflight.rs b/crates/openshell-gateway/tests/config_preflight.rs index 3c404c672d..7c2a6727d4 100644 --- a/crates/openshell-gateway/tests/config_preflight.rs +++ b/crates/openshell-gateway/tests/config_preflight.rs @@ -102,6 +102,15 @@ fn combined(output: &Output) -> String { ) } +fn normalized_diagnostic(output: &Output) -> String { + // Terminal line wrapping can split an error sentence after a long path. + combined(output) + .split_whitespace() + .filter(|word| *word != "│") + .collect::>() + .join(" ") +} + #[tokio::test] async fn local_vm_reports_tools_without_starting_driver_or_creating_state() { let fixture = Fixture::new(); @@ -155,21 +164,13 @@ async fn local_vm_rejects_nonexecutable_and_unsupported_tools() { .unwrap(); let output = fixture.run(&[]).await; assert!(!output.status.success()); - assert!( - combined(&output).contains("not an executable file"), - "{}", - combined(&output) - ); + let report = normalized_diagnostic(&output); + assert!(report.contains("not an executable file"), "{report}"); fixture.tool("mke2fs", "echo 'mke2fs 1.42.13' >&2"); let output = fixture.run(&[]).await; assert!(!output.status.success()); - // The diagnostic renderer wraps long selected paths and error text. - let report = combined(&output) - .split_whitespace() - .filter(|word| *word != "│") - .collect::>() - .join(" "); + let report = normalized_diagnostic(&output); assert!(report.contains("not a supported mke2fs"), "{report}"); } diff --git a/docs/how-it-works/gateways/configuration.mdx b/docs/how-it-works/gateways/configuration.mdx index f0e9ec0bac..2a277ce623 100644 --- a/docs/how-it-works/gateways/configuration.mdx +++ b/docs/how-it-works/gateways/configuration.mdx @@ -1248,7 +1248,7 @@ changing it: openshell-gateway config preflight --path ~/.config/openshell/gateway.toml ``` -Without `--path`, the command validates a nonempty `OPENSHELL_GATEWAY_CONFIG`. Otherwise, it validates an existing XDG gateway config when one is discovered. When neither source selects a config, preflight validates the effective daemon arguments. An explicit missing path, a legacy schema-v1 file, invalid TOML, a symlink, or any nonregular file fails. Preflight merges the selected file with the current `OPENSHELL_*` environment and applies the daemon's read-only startup checks. These checks include selector and socket normalization, registered-driver selection and configuration, rate-limit pairs, TLS and mTLS relationships, interceptor registrations, and supervisor middleware registrations. When a selected file omits `compute_driver`, preflight validates each configured table for an auto-detectable driver without running the runtime detection probes, which can connect local sockets or launch discovery commands. It validates complete guest TLS path sets without requiring package-generated certificates to exist before certificate generation. It does not construct a compute driver or connect to a transport. A failed preflight always preserves the file; it never migrates, replaces, or rewrites configuration. +Without `--path`, the command validates a nonempty `OPENSHELL_GATEWAY_CONFIG`. Otherwise, it validates an existing XDG gateway config when one is discovered. When neither source selects a config, preflight validates the effective daemon arguments. An explicit missing path, a legacy schema-v1 file, invalid TOML, a symlink, or any nonregular file fails. Preflight merges the selected file with the current `OPENSHELL_*` environment and applies the daemon's read-only startup checks. These checks include selector and socket normalization, registered-driver selection and configuration, rate-limit pairs, TLS and mTLS relationships, interceptor registrations, and supervisor middleware registrations. When a selected file omits `compute_driver`, preflight validates each configured table for an auto-detectable driver without running the runtime detection probes, which can connect local sockets or launch discovery commands. It validates configured driver TLS requirements, including the gateway CA, without requiring package-generated certificate files to exist before certificate generation. It does not construct a compute driver or connect to a transport. A failed preflight always preserves the file; it never migrates, replaces, or rewrites configuration. To validate the exact daemon arguments that a wrapper will pass, place them after `--` instead of using `--path`: From 8bd564bf637e1bc9250289258cd10be40354cb69 Mon Sep 17 00:00:00 2001 From: Shiju Date: Thu, 1 Oct 2026 14:58:01 +0530 Subject: [PATCH 3/3] test(gateway): serialize preflight fixture paths as TOML Keep temporary paths quoted and escaped through the TOML serializer instead of relying on Rust Debug formatting. Signed-off-by: Shiju --- crates/openshell-gateway/tests/config_preflight.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/openshell-gateway/tests/config_preflight.rs b/crates/openshell-gateway/tests/config_preflight.rs index 7c2a6727d4..afdf956b72 100644 --- a/crates/openshell-gateway/tests/config_preflight.rs +++ b/crates/openshell-gateway/tests/config_preflight.rs @@ -38,9 +38,11 @@ impl Fixture { fn write_config(&self, driver: Option<&str>) { let selector = driver.map_or_else(String::new, |name| format!("compute_driver = {name:?}\n")); + // TOML serialization preserves quotes and escapes in the fixture path. + let state_dir = toml::Value::try_from(self.root.path().join("vm-state")) + .expect("serialize VM state directory"); fs::write(&self.config, format!( - "[openshell]\nversion = 2\n[openshell.gateway]\ndisable_tls = true\n{selector}[openshell.drivers.vm]\nbootstrap_image = 'unreachable.invalid/vm:must-not-pull'\nstate_dir = {:?}\n", - self.root.path().join("vm-state") + "[openshell]\nversion = 2\n[openshell.gateway]\ndisable_tls = true\n{selector}[openshell.drivers.vm]\nbootstrap_image = 'unreachable.invalid/vm:must-not-pull'\nstate_dir = {state_dir}\n" )).expect("gateway configuration"); }