diff --git a/Cargo.lock b/Cargo.lock index 424a8f5..e52ba04 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -684,6 +684,16 @@ version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" +[[package]] +name = "socket2" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3d1e2c7f27f8d4cb10542a02c49005dbd6e93095799d6f3be745fae9f8fedd4" +dependencies = [ + "libc", + "windows-sys 0.61.2", +] + [[package]] name = "strsim" version = "0.11.1" @@ -929,6 +939,7 @@ dependencies = [ "mio", "pin-project-lite", "signal-hook-registry", + "socket2", "tokio-macros", "windows-sys 0.61.2", ] diff --git a/README.md b/README.md index 793f2ab..c3328c4 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,35 @@ Publish a port with `-p`: tinybox create --sandbox docker --image nginx -p 8080:80 ``` +## Something that keeps running + +`exec` waits for the command, which is right for a build and wrong for a server. +`spawn` starts one and hands back an identifier instead: + +```sh +tinybox create --sandbox docker --image nginx -p 8080:80 +pid=$(tinybox spawn box-0 -- nginx -g 'daemon off;') +tinybox ps box-0 "$pid" # -> running +tinybox kill box-0 "$pid" # -> stopped +``` + +The process survives between commands, which is what a sandbox declaring +`Detach` is promising — `tinybox inspect` says which ones do. `passthrough` and +`docker` do; `namespace` and `microvm` decline rather than background something +they could not find again. + +Publishing puts that port on the machine the box runs on. When that machine is +somewhere else, `forward` closes the gap: + +```sh +tinybox --host ssh://builder@example.com forward 8080 +# 127.0.0.1:54321 # ...and the tunnel lasts as long as this runs +``` + +Reach was always the `Host`'s question, so a tunnel is answered there too — see +[ADR 0007](docs/adr/0007-reach-includes-forwarding-and-detachment.md). Nothing +in `ssh` or `docker` knows about the other, here either. + ## Without a daemon `namespace` isolates a directory you already have, using Linux namespaces diff --git a/crates/tinybox-cli/src/command/mod.rs b/crates/tinybox-cli/src/command/mod.rs index afa4ea0..65ff47f 100644 --- a/crates/tinybox-cli/src/command/mod.rs +++ b/crates/tinybox-cli/src/command/mod.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use clap::{Parser, Subcommand, ValueEnum}; use tinybox_core::{ BoxId, BoxInfo, BoxSpec, Clock, Error, ExecRequest, Host, HostRef, NetworkPolicy, - PassthroughSandbox, Placement, PortMapping, Sandbox, SandboxRef, SnapshotId, Store, + PassthroughSandbox, Placement, PortMapping, ProcessId, Sandbox, SandboxRef, SnapshotId, Store, SystemClock, TemplateName, Templates, WorkspaceSource, passthrough, }; use tinybox_docker::DockerSandbox; @@ -177,6 +177,47 @@ enum Command { #[arg(trailing_var_arg = true, required = true, value_name = "COMMAND")] argv: Vec, }, + /// Start a command in a box and leave it running. + /// + /// Where `exec` waits, this returns a process id as soon as the command is + /// started. It is how a server gets into a box; `exec` would never return. + Spawn { + /// Which box to start it in. + id: String, + /// The command and its arguments. + #[arg(trailing_var_arg = true, required = true, value_name = "COMMAND")] + argv: Vec, + }, + /// Report whether a spawned process is still running. + Ps { + /// Which box it was started in. + id: String, + /// The process id `spawn` printed. + process: String, + }, + /// Stop a spawned process. + /// + /// Succeeds when it has already exited: stopping something already stopped + /// is the outcome the caller wanted. + Kill { + /// Which box it was started in. + id: String, + /// The process id `spawn` printed. + process: String, + }, + /// Make a port on the box's machine reachable from this one. + /// + /// Publishing a port (`create -p`) puts it on the machine the box runs on. + /// When that is somewhere else, this is what closes the gap. The tunnel + /// lasts as long as the command runs, so it holds until interrupted. + Forward { + /// The port on the box's machine. + port: u16, + /// The address to reach it at over there. Defaults to loopback, which + /// is where a published port lands. + #[arg(long, value_name = "IP", default_value = "127.0.0.1")] + address: std::net::IpAddr, + }, /// List every box. #[command(alias = "list")] Ls, @@ -350,12 +391,11 @@ impl Cli { )?; announce(&sandbox.create(&spec).await?, sandbox.as_ref(), out, err) } - Command::Exec { id, argv } => { - let id = BoxId::new(id)?; - let sandbox = build(sandbox_of(&store, &id)?)?; - let output = sandbox.exec(&id, &ExecRequest::new(argv)).await?; - report(&output, out, err) - } + Command::Exec { id, argv } => exec(&store, &backends, id, argv, out, err).await, + Command::Spawn { id, argv } => spawn(&store, &backends, id, argv, out).await, + Command::Ps { id, process } => probe(&store, &backends, id, &process, out).await, + Command::Kill { id, process } => kill(&store, &backends, id, &process, out).await, + Command::Forward { port, address } => forward(reach.as_ref(), address, port, out).await, // Listing is the store's business, not the sandbox's: the store is // what owns the set of records. Command::Ls => text(out, &render_listing(&store.list()?)), @@ -570,6 +610,37 @@ fn line(out: &mut dyn Write, value: &str) -> tinybox_core::Result { text(out, &format!("{value}\n")) } +/// Open a tunnel to `remote` and hold it until the process is interrupted. +/// +/// The forward is a guard, so it exists for exactly as long as this function +/// runs. There is no daemon to hand it to and no state file that could +/// describe a tunnel this process is no longer holding open, so blocking is +/// the honest shape: the command running *is* the forward existing. +/// +/// # Errors +/// +/// Returns whatever the host reports when the tunnel cannot be opened — +/// [`Error::Unsupported`] from a host that cannot tunnel at all. +async fn forward( + reach: &dyn Host, + address: std::net::IpAddr, + port: u16, + out: &mut dyn Write, +) -> tinybox_core::Result { + let forwarded = reach.forward((address, port).into()).await?; + line(out, &forwarded.local_addr().to_string())?; + + if forwarded.is_direct() { + // Nothing is being held open, so there is nothing to hold *for*. + // Blocking here would look like a working tunnel and be a hang. + return Ok(0); + } + // Park until the terminal interrupts us; dropping `forwarded` on the way + // out closes the tunnel. + std::future::pending::<()>().await; + Ok(0) +} + /// Forward a finished command's output and status to the caller. /// /// # Errors @@ -783,6 +854,94 @@ fn render_sync(outcome: &tinybox_sync::Sync) -> String { /// Returns [`Error::InvalidIdentifier`] when a Docker namespace is not a valid /// identifier. /// Destroy one box and print its identifier back. +/// Run a command in a box, mirroring its output and exit status. +/// +/// # Errors +/// +/// Returns whatever the backend reports when the command could not be started. +/// A command that runs and exits non-zero is **not** an error: its status +/// becomes this process's. +async fn exec( + store: &Arc, + backends: &Backends<'_>, + id: String, + argv: Vec, + out: &mut dyn Write, + err: &mut dyn Write, +) -> tinybox_core::Result { + let id = BoxId::new(id)?; + let sandbox = backends.get(sandbox_of(store, &id)?)?; + let output = sandbox.exec(&id, &ExecRequest::new(argv)).await?; + report(&output, out, err) +} + +/// Start a command in a box and print the identifier for asking about it. +/// +/// # Errors +/// +/// Returns [`Error::Unsupported`] when the box's sandbox cannot host a process +/// between commands, and whatever the backend reports when the command could +/// not be started. +async fn spawn( + store: &Arc, + backends: &Backends<'_>, + id: String, + argv: Vec, + out: &mut dyn Write, +) -> tinybox_core::Result { + let id = BoxId::new(id)?; + let sandbox = backends.get(sandbox_of(store, &id)?)?; + let process = sandbox.spawn(&id, &ExecRequest::new(argv)).await?; + line(out, process.as_ref()) +} + +/// Report whether a spawned process is still running. +/// +/// A process that has exited prints `gone` and exits zero: that it finished is +/// an answer, and reporting it as a failure would be indistinguishable from an +/// unreachable box. +/// +/// # Errors +/// +/// Returns [`Error::Unsupported`] when the box's sandbox does not track +/// detached processes, and a backend error when the box cannot be reached. +async fn probe( + store: &Arc, + backends: &Backends<'_>, + id: String, + process: &str, + out: &mut dyn Write, +) -> tinybox_core::Result { + let id = BoxId::new(id)?; + let sandbox = backends.get(sandbox_of(store, &id)?)?; + let running = sandbox + .is_running(&id, &ProcessId::new(process.to_owned())?) + .await?; + line(out, if running { "running" } else { "gone" }) +} + +/// Stop a spawned process. +/// +/// # Errors +/// +/// Returns [`Error::Unsupported`] when the box's sandbox does not track +/// detached processes, and a backend error when the box cannot be reached. A +/// process that had already exited is not an error. +async fn kill( + store: &Arc, + backends: &Backends<'_>, + id: String, + process: &str, + out: &mut dyn Write, +) -> tinybox_core::Result { + let id = BoxId::new(id)?; + let sandbox = backends.get(sandbox_of(store, &id)?)?; + sandbox + .stop(&id, &ProcessId::new(process.to_owned())?) + .await?; + line(out, "stopped") +} + async fn remove( store: &Arc, backends: &Backends<'_>, diff --git a/crates/tinybox-cli/src/command/test.rs b/crates/tinybox-cli/src/command/test.rs index e58d4c9..409c249 100644 --- a/crates/tinybox-cli/src/command/test.rs +++ b/crates/tinybox-cli/src/command/test.rs @@ -533,13 +533,16 @@ async fn inspect_lists_what_the_sandbox_declares() -> Result<()> { let inspected = invoke(dir.path(), &["inspect", "box-0"]).await; - // Passthrough declares nothing, and says that rather than printing an - // empty list the reader has to interpret. + // Passthrough declares detached processes and nothing else: a box here is + // an ordinary directory on this machine, so a backgrounded process really + // does survive between commands, but there is no filesystem boundary to + // snapshot and no limit it can apply. assert!( - inspected - .out - .contains("supports: nothing beyond running commands") + inspected.out.contains("supports: detached processes"), + "{}", + inspected.out ); + assert!(!inspected.out.contains("filesystem snapshots")); Ok(()) } @@ -1298,3 +1301,94 @@ fn write_boxes(dir: &Path, boxes: &[(&str, Option)]) -> R std::fs::write(dir.join("boxes.json"), format!("{{{}}}", records.join(","))) .map_err(|error| Error::io("write", &error)) } + +#[tokio::test] +async fn a_spawned_process_outlives_the_command_that_started_it() -> Result<()> { + // The whole point of `spawn` over `exec`: a separate invocation is standing + // in for a separate process, and the thing started by the first one is + // still there for the second to ask about. + let dir = temp_dir()?; + invoke(dir.path(), &["create", "--dir", "/tmp"]).await; + + let spawned = invoke(dir.path(), &["spawn", "box-0", "sleep", "30"]).await; + assert_eq!(spawned.code, 0); + let process = spawned.out.trim().to_owned(); + assert!(!process.is_empty(), "spawn prints an identifier"); + + let running = invoke(dir.path(), &["ps", "box-0", &process]).await; + assert_eq!(running.out.trim(), "running"); + + let killed = invoke(dir.path(), &["kill", "box-0", &process]).await; + assert_eq!(killed.code, 0); + + let gone = invoke(dir.path(), &["ps", "box-0", &process]).await; + // `gone` on stdout with a zero exit: the process finishing is an answer, + // not a failure, and reporting it as one would be indistinguishable from + // an unreachable box. + assert_eq!(gone.code, 0); + assert_eq!(gone.out.trim(), "gone"); + Ok(()) +} + +#[tokio::test] +async fn asking_about_a_process_that_was_never_started_answers_gone() -> Result<()> { + let dir = temp_dir()?; + invoke(dir.path(), &["create", "--dir", "/tmp"]).await; + + let answer = invoke(dir.path(), &["ps", "box-0", "p1-0"]).await; + + assert_eq!(answer.code, 0); + assert_eq!(answer.out.trim(), "gone"); + Ok(()) +} + +#[tokio::test] +async fn killing_a_process_that_has_already_exited_is_not_an_error() -> Result<()> { + // Stopping something already stopped is the outcome the caller wanted. + let dir = temp_dir()?; + invoke(dir.path(), &["create", "--dir", "/tmp"]).await; + + let killed = invoke(dir.path(), &["kill", "box-0", "p1-0"]).await; + + assert_eq!(killed.code, 0); + Ok(()) +} + +#[tokio::test] +async fn a_local_forward_reports_the_address_and_returns() -> Result<()> { + // Nothing is held open on a local host, so blocking would look like a + // working tunnel and be a hang. + let dir = temp_dir()?; + + let forwarded = invoke(dir.path(), &["forward", "7788"]).await; + + assert_eq!(forwarded.code, 0); + assert_eq!(forwarded.out.trim(), "127.0.0.1:7788"); + Ok(()) +} + +#[tokio::test] +async fn spawning_into_a_sandbox_that_cannot_detach_is_refused() -> Result<()> { + // A namespace box is a record and a bound directory rather than a running + // container, so a backgrounded process would not survive to be found. It + // says so instead. + let dir = temp_dir()?; + let created = invoke( + dir.path(), + &["create", "--sandbox", "namespace", "--dir", "/tmp"], + ) + .await; + if created.code != 0 { + return Ok(()); // No bubblewrap on this host. + } + + let spawned = invoke(dir.path(), &["spawn", "box-0", "sleep", "30"]).await; + + assert_eq!(spawned.code, EXIT_TINYBOX_ERROR); + assert!( + spawned.err.contains("detached processes"), + "{}", + spawned.err + ); + Ok(()) +} diff --git a/crates/tinybox-core/src/capability/mod.rs b/crates/tinybox-core/src/capability/mod.rs index 87d6e6a..1e3d470 100644 --- a/crates/tinybox-core/src/capability/mod.rs +++ b/crates/tinybox-core/src/capability/mod.rs @@ -106,6 +106,15 @@ impl SandboxCapabilities { self.with(Capability::ResourceLimits) } + /// Declare that a process can be left running in a box and found again. + /// + /// See [`Capability::Detach`] for what a backend is promising. A sandbox + /// that cannot later locate or stop such a process must not call this. + #[must_use] + pub const fn with_detach(self) -> Self { + self.with(Capability::Detach) + } + /// Add one capability to the set. /// /// Snapshot capabilities are not settable this way: what a sandbox can diff --git a/crates/tinybox-core/src/capability/test.rs b/crates/tinybox-core/src/capability/test.rs index f14851c..9bdc4d6 100644 --- a/crates/tinybox-core/src/capability/test.rs +++ b/crates/tinybox-core/src/capability/test.rs @@ -11,7 +11,8 @@ const MICROVM: SandboxCapabilities = SandboxCapabilities::new( .with_fork() .with_pause_resume() .with_port_forward() -.with_resource_limits(); +.with_resource_limits() +.with_detach(); /// A container sandbox: isolated and snapshottable, but no memory capture. const CONTAINER: SandboxCapabilities = @@ -33,6 +34,7 @@ fn passthrough_admits_it_isolates_nothing() { assert!(!caps.supports(Capability::FilesystemSnapshot)); assert!(!caps.supports(Capability::MemorySnapshot)); assert!(!caps.supports(Capability::ResourceLimits)); + assert!(!caps.supports(Capability::Detach)); } #[test] @@ -53,6 +55,7 @@ fn each_builder_method_adds_exactly_one_capability() { base.with_resource_limits().declared(), [Capability::ResourceLimits] ); + assert_eq!(base.with_detach().declared(), [Capability::Detach]); } #[test] @@ -129,6 +132,7 @@ fn capabilities_do_not_share_a_bit() { Capability::ResourceLimits, SandboxCapabilities::with_resource_limits, ), + (Capability::Detach, SandboxCapabilities::with_detach), ] { built = add(built); expected.push(capability); diff --git a/crates/tinybox-core/src/capability/types.rs b/crates/tinybox-core/src/capability/types.rs index ee2c9f1..12ef71c 100644 --- a/crates/tinybox-core/src/capability/types.rs +++ b/crates/tinybox-core/src/capability/types.rs @@ -106,19 +106,28 @@ pub enum Capability { PortForward, /// Applying the limits in [`Resources`](crate::spec::Resources). ResourceLimits, + /// Hosting a process that outlives the command that started it. + /// + /// A sandbox declares this when a caller can leave a server running in a + /// box and come back to it — see [`detach`](crate::detach). A sandbox whose + /// boxes do not persist writes between commands, or that returns only what + /// a command printed, must decline: a background process it cannot later + /// find or stop is worse than a refusal. + Detach, } impl Capability { /// Every capability, in declaration order. /// /// Used to render a declared set; keep it in step with the enum. - pub const ALL: [Self; 6] = [ + pub const ALL: [Self; 7] = [ Self::FilesystemSnapshot, Self::MemorySnapshot, Self::Fork, Self::PauseResume, Self::PortForward, Self::ResourceLimits, + Self::Detach, ]; /// This capability's bit within a [`SandboxCapabilities`] feature set. @@ -132,6 +141,7 @@ impl Capability { Self::PauseResume => 1 << 3, Self::PortForward => 1 << 4, Self::ResourceLimits => 1 << 5, + Self::Detach => 1 << 6, } } } @@ -145,6 +155,7 @@ impl fmt::Display for Capability { Self::PauseResume => "pause and resume", Self::PortForward => "port forwarding", Self::ResourceLimits => "resource limits", + Self::Detach => "detached processes", }; formatter.write_str(text) } diff --git a/crates/tinybox-core/src/detach/mod.rs b/crates/tinybox-core/src/detach/mod.rs new file mode 100644 index 0000000..e56878a --- /dev/null +++ b/crates/tinybox-core/src/detach/mod.rs @@ -0,0 +1,170 @@ +//! Leaving a process running in a box, and finding it again later. +//! +//! [`Sandbox::exec`](crate::runtime::Sandbox::exec) runs a command to +//! completion and collects its output. That is the right shape for the work +//! tinybox was built for — a build, a test run, an agent's command — and the +//! wrong shape for a server. Starting one through `exec` never returns. +//! +//! # Why this is one mechanism rather than one per backend +//! +//! Docker has `docker exec --detach`, and a local host could hold a +//! [`std::process::Child`]. Neither generalizes: `--detach` hands back nothing +//! a caller could name, and a child handle dies with the process holding it, +//! which is exactly the process a detached command is supposed to outlive. SSH +//! has neither. +//! +//! What every box tinybox can host a server in *does* have is a POSIX shell. +//! So the mechanism is the shell's own: background the command, record its pid +//! in a file named after a [`ProcessId`] tinybox minted, and answer later +//! questions by reading that file. One implementation, identical semantics +//! everywhere, and the backend contributes only its existing +//! [`exec`](crate::runtime::Sandbox::exec) path. +//! +//! # What a backend is promising +//! +//! A sandbox that declares [`Capability::Detach`](crate::Capability::Detach) +//! promises two things beyond running the command: that a write to +//! [`PID_DIR`] survives until the next command, and that the process itself +//! keeps running between commands. A sandbox where either is false — one whose +//! boxes are re-bound per command, or that returns only what the command +//! printed — must decline. A background process that cannot be found or +//! stopped is worse than a refusal, because it looks like it worked. +//! +//! ``` +//! use tinybox_core::detach; +//! use tinybox_core::runtime::ExecRequest; +//! +//! let process = detach::mint(); +//! let start = detach::start(&process, &ExecRequest::new(["sleep", "60"]))?; +//! +//! // A shell command, because that is what backgrounding requires. +//! assert_eq!(start.program(), Some("/bin/sh")); +//! # Ok::<(), tinybox_core::Error>(()) +//! ``` + +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::error::{Error, Result}; +use crate::identity::ProcessId; +use crate::runtime::ExecRequest; +use crate::shell; + +/// Where pid files are written inside a box. +/// +/// `/tmp` rather than the workspace: the workspace is the user's, may be a +/// read-only mount, and is often synced back out. Runtime bookkeeping does not +/// belong in it. +pub const PID_DIR: &str = "/tmp"; + +/// The shell every detached command is started through. +/// +/// Spelled absolutely so a box with an unusual `PATH` still resolves it, and +/// `sh` rather than `bash` because a minimal image often has only the former. +const SHELL: &str = "/bin/sh"; + +/// Distinguishes ids minted within one process. +static COUNTER: AtomicU64 = AtomicU64::new(0); + +/// Mint an identifier for a process about to be started. +/// +/// The value is opaque; callers should store it rather than parse it. It is +/// infallible because the text is built here from a fixed prefix and decimal +/// digits, which is always a valid identifier — a `Result` would hand callers +/// an error arm that can never happen. +#[must_use] +pub fn mint() -> ProcessId { + let ordinal = COUNTER.fetch_add(1, Ordering::Relaxed); + // Two sources so that two hosts, or two runs, do not collide on a shared + // box: a monotonic ordinal within this process, and the process's own pid. + ProcessId::from_generated(format!("p{}-{ordinal}", std::process::id())) +} + +/// The path of the file recording `process`'s real pid inside its box. +#[must_use] +pub fn pid_file(process: &ProcessId) -> String { + format!("{PID_DIR}/tinybox-{process}.pid") +} + +/// The command that starts `request` in the background and records its pid. +/// +/// The shell writes the pid *before* the outer shell exits, so a caller that +/// gets a successful [`ExecOutput`](crate::runtime::ExecOutput) back can +/// immediately ask whether the process is running and get a truthful answer. +/// Output is discarded: nothing is reading it, and a full pipe would eventually +/// block the very process this is trying to leave running. +/// +/// # Errors +/// +/// Returns [`Error::EmptyCommand`] when the request names no program. The +/// sandbox is named `detach` because the failure is in this construction, not +/// in any backend. +pub fn start(process: &ProcessId, request: &ExecRequest) -> Result { + if request.argv.is_empty() { + return Err(Error::EmptyCommand { + sandbox: "detach".to_owned(), + }); + } + + let inner = shell::script(&request.argv, request.cwd.as_deref(), &request.env); + let pid_file = shell::quote(&pid_file(process)); + // `$!` is the pid of the most recent background command, so it is captured + // before anything else can overwrite it. + let line = format!("{{ {inner} ; }} /dev/null 2>&1 & echo $! > {pid_file}"); + + let mut started = ExecRequest::new([SHELL, "-c", &line]); + // stdin belongs to the backgrounded command, which is already given + // /dev/null above; passing the caller's payload here would feed the + // wrapper instead. + started.stdin = None; + Ok(started) +} + +/// The command that reports whether `process` is still running. +/// +/// Prints `running` or `gone`, and exits zero either way: "the process has +/// finished" is an answer, not a failure, and conflating it with one would make +/// an unreachable box indistinguishable from a completed server. +/// +/// Signal `0` performs the kernel's permission and existence check without +/// delivering anything, which is the standard way to ask. +#[must_use] +pub fn probe(process: &ProcessId) -> ExecRequest { + let pid_file = shell::quote(&pid_file(process)); + let line = format!( + "if [ -f {pid_file} ] && kill -0 \"$(cat {pid_file})\" 2>/dev/null; \ + then echo running; else echo gone; fi" + ); + ExecRequest::new([SHELL, "-c", &line]) +} + +/// What [`probe`] prints when the process is still running. +pub const RUNNING: &str = "running"; + +/// The command that stops `process` and removes its pid file. +/// +/// `TERM` first so the process can shut down on its own terms, then `KILL` +/// after a grace period for one that will not. The pid file is removed either +/// way: leaving it behind would make a later [`probe`] answer about whatever +/// process inherits that pid next, which on a long-lived box is a real +/// possibility and a confusing bug. +/// +/// Exits zero when the process was already gone, because stopping something +/// that has already stopped is the outcome the caller wanted. +#[must_use] +pub fn stop(process: &ProcessId, grace: std::time::Duration) -> ExecRequest { + let pid_file = shell::quote(&pid_file(process)); + let seconds = grace.as_secs().max(1); + let line = format!( + "if [ -f {pid_file} ]; then pid=$(cat {pid_file}); \ + kill -TERM \"$pid\" 2>/dev/null; \ + for _ in $(seq {seconds}); do kill -0 \"$pid\" 2>/dev/null || break; sleep 1; done; \ + kill -KILL \"$pid\" 2>/dev/null; rm -f {pid_file}; fi; exit 0" + ); + ExecRequest::new([SHELL, "-c", &line]) +} + +/// How long [`stop`] waits for a graceful exit before killing. +pub const DEFAULT_GRACE: std::time::Duration = std::time::Duration::from_secs(5); + +#[cfg(test)] +mod test; diff --git a/crates/tinybox-core/src/detach/test.rs b/crates/tinybox-core/src/detach/test.rs new file mode 100644 index 0000000..fe70508 --- /dev/null +++ b/crates/tinybox-core/src/detach/test.rs @@ -0,0 +1,195 @@ +//! Tests for the detached-process mechanism. +//! +//! The command builders are pure, so the encoding is pinned here exactly. The +//! part that needs a real shell — that the wrapper actually backgrounds a +//! process and that the pid it records is the right one — is checked at the +//! bottom against `sh` itself, skipped where no shell exists. + +use std::collections::BTreeMap; +use std::path::Path; +use std::time::Duration; + +use super::{DEFAULT_GRACE, PID_DIR, RUNNING, mint, pid_file, probe, start, stop}; +use crate::error::{Error, Result}; +use crate::identity::ProcessId; +use crate::runtime::ExecRequest; + +fn process() -> Result { + ProcessId::new("p1-0") +} + +#[test] +fn a_minted_id_is_valid_and_distinct() -> Result<()> { + let first = mint(); + let second = mint(); + + assert_ne!(first, second); + // Round-trips through the validating constructor, which is what makes the + // fallback in `mint` unreachable rather than merely unlikely. + ProcessId::new(first.as_str())?; + Ok(()) +} + +#[test] +fn the_pid_file_lives_outside_the_workspace() -> Result<()> { + // Runtime bookkeeping in the workspace would be synced back out, or fail + // on a read-only mount. + assert_eq!(pid_file(&process()?), format!("{PID_DIR}/tinybox-p1-0.pid")); + Ok(()) +} + +#[test] +fn starting_runs_through_a_shell_because_backgrounding_needs_one() -> Result<()> { + let started = start(&process()?, &ExecRequest::new(["sleep", "60"]))?; + + assert_eq!(started.program(), Some("/bin/sh")); + assert_eq!(started.argv[1], "-c"); + Ok(()) +} + +#[test] +fn the_pid_is_recorded_before_the_wrapper_exits() -> Result<()> { + // Otherwise a caller could ask "is it running" and be told "gone" about a + // process that had started perfectly well. + let started = start(&process()?, &ExecRequest::new(["sleep", "60"]))?; + let line = &started.argv[2]; + + assert!(line.contains("& echo $! >"), "{line:?}"); + assert!( + line.ends_with(&format!("'{PID_DIR}/tinybox-p1-0.pid'")), + "{line:?}" + ); + Ok(()) +} + +#[test] +fn output_is_discarded_so_a_full_pipe_cannot_block_the_process() -> Result<()> { + let started = start(&process()?, &ExecRequest::new(["server"]))?; + + assert!(started.argv[2].contains("/dev/null 2>&1")); + Ok(()) +} + +#[test] +fn the_command_is_quoted_so_a_filename_cannot_inject() -> Result<()> { + let started = start(&process()?, &ExecRequest::new(["echo", "; rm -rf /"]))?; + + // One quoted word, so the semicolon is data. + assert!( + started.argv[2].contains(r"'echo' '; rm -rf /'"), + "{:?}", + started.argv[2] + ); + Ok(()) +} + +#[test] +fn the_working_directory_and_environment_reach_the_backgrounded_command() -> Result<()> { + let mut request = ExecRequest::new(["server"]).with_cwd(Path::new("/srv/work")); + request.env = BTreeMap::from([("PORT".to_owned(), "7788".to_owned())]); + + let started = start(&process()?, &request)?; + + assert!(started.argv[2].contains("cd '/srv/work' &&")); + assert!(started.argv[2].contains("env 'PORT=7788'")); + Ok(()) +} + +#[test] +fn a_caller_payload_does_not_reach_the_wrapper() -> Result<()> { + // The backgrounded command already gets /dev/null; a payload here would + // feed the wrapping shell instead, which is never what a caller meant. + let request = ExecRequest::new(["server"]).with_stdin(b"payload".to_vec()); + + let started = start(&process()?, &request)?; + + assert_eq!(started.stdin, None); + Ok(()) +} + +#[test] +fn an_empty_command_is_refused_here_rather_than_by_a_backend() -> Result<()> { + let outcome = start(&process()?, &ExecRequest::new(Vec::::new())); + + assert_eq!( + outcome.err(), + Some(Error::EmptyCommand { + sandbox: "detach".to_owned() + }) + ); + Ok(()) +} + +#[test] +fn probing_asks_the_kernel_rather_than_trusting_the_file() -> Result<()> { + // A pid file outlives its process; signal 0 is the existence check. + let request = probe(&process()?); + + assert!(request.argv[2].contains("kill -0")); + assert!(request.argv[2].contains(RUNNING)); + Ok(()) +} + +#[test] +fn stopping_escalates_and_always_clears_the_pid_file() -> Result<()> { + let request = stop(&process()?, DEFAULT_GRACE); + let line = &request.argv[2]; + + assert!(line.contains("kill -TERM"), "{line:?}"); + assert!(line.contains("kill -KILL"), "{line:?}"); + // Left behind, a stale file would make a later probe answer about whatever + // process inherits that pid next. + assert!(line.contains("rm -f"), "{line:?}"); + // Stopping something already stopped is the outcome the caller wanted. + assert!(line.contains("exit 0"), "{line:?}"); + Ok(()) +} + +#[test] +fn a_sub_second_grace_still_waits_a_whole_second() -> Result<()> { + // `seq 0` would produce no iterations, so TERM and KILL would land back to + // back and the graceful path would never happen. + let request = stop(&process()?, Duration::from_millis(10)); + + assert!(request.argv[2].contains("seq 1"), "{:?}", request.argv[2]); + Ok(()) +} + +/// Run one of these command builders through a real `sh`, returning stdout. +/// +/// Returns `None` where no shell exists, so the encoding tests above remain +/// the guarantee on such a host. +fn run(request: &ExecRequest) -> Option { + let output = std::process::Command::new(&request.argv[0]) + .args(&request.argv[1..]) + .output() + .ok()?; + Some(String::from_utf8_lossy(&output.stdout).trim().to_owned()) +} + +#[test] +fn a_started_process_is_reported_running_and_then_stops() -> Result<()> { + // The property the encoding tests cannot check: that this really does + // background something, and that the recorded pid is that something's. + let id = mint(); + let started = start(&id, &ExecRequest::new(["sleep", "30"]))?; + + if run(&started).is_none() { + return Ok(()); // No shell on this host. + } + + assert_eq!(run(&probe(&id)).as_deref(), Some(RUNNING)); + + run(&stop(&id, Duration::from_secs(1))); + assert_eq!(run(&probe(&id)).as_deref(), Some("gone")); + Ok(()) +} + +#[test] +fn probing_a_process_that_was_never_started_answers_gone() { + let id = mint(); + + if let Some(answer) = run(&probe(&id)) { + assert_eq!(answer, "gone"); + } +} diff --git a/crates/tinybox-core/src/identity/mod.rs b/crates/tinybox-core/src/identity/mod.rs index 23bab88..19dfb1a 100644 --- a/crates/tinybox-core/src/identity/mod.rs +++ b/crates/tinybox-core/src/identity/mod.rs @@ -21,7 +21,7 @@ use crate::error::{Error, Result}; mod types; -pub use types::{BoxId, HostRef, SandboxRef, SnapshotId, TemplateName}; +pub use types::{BoxId, HostRef, ProcessId, SandboxRef, SnapshotId, TemplateName}; /// Whether `value` would be accepted as a tinybox identifier. /// diff --git a/crates/tinybox-core/src/identity/types.rs b/crates/tinybox-core/src/identity/types.rs index 36c79ab..c3ea95c 100644 --- a/crates/tinybox-core/src/identity/types.rs +++ b/crates/tinybox-core/src/identity/types.rs @@ -51,6 +51,28 @@ macro_rules! identifier { Ok(Self(value)) } + #[doc = concat!("Wrap a ", $kind, " this crate generated itself.")] + /// + /// Skips validation, which is sound only because the caller built + /// the text from a fixed pattern. It exists so that an internally + /// minted identifier has no impossible error arm: an + /// `unwrap_or_else` there would be a branch no test could reach, + /// and an unreachable branch in a coverage-gated crate gets + /// "covered" by something meaningless. + /// + /// The macro emits this for all six identifiers and only + /// `ProcessId` mints its own today, so it is allowed to go unused + /// rather than complicating the macro with a flag for one caller. + #[allow(dead_code, reason = "generated for six types, minted by one")] + pub(crate) fn from_generated(value: String) -> Self { + debug_assert!( + validate($kind, &value).is_ok(), + "generated {} is not valid: {value:?}", + $kind, + ); + Self(value) + } + #[doc = concat!("Borrow this ", $kind, " as a string slice.")] #[must_use] pub fn as_str(&self) -> &str { @@ -123,3 +145,17 @@ identifier!( SandboxRef, "sandbox reference" ); + +identifier!( + /// Identifies one detached process inside a box. + /// + /// Deliberately *not* an operating-system pid. None of the transports + /// tinybox speaks hands back a process handle a caller could hold — `docker + /// exec` and `ssh` both return only what the command printed — so tinybox + /// mints this itself and [`detach`](crate::detach) records the real pid + /// beside it, inside the box. The identifier is therefore stable across + /// reconnects and meaningful on the caller's side, which a pid from a + /// foreign process table is not. + ProcessId, + "process id" +); diff --git a/crates/tinybox-core/src/lib.rs b/crates/tinybox-core/src/lib.rs index 1265027..1faf0c6 100644 --- a/crates/tinybox-core/src/lib.rs +++ b/crates/tinybox-core/src/lib.rs @@ -63,10 +63,12 @@ pub mod capability; pub mod clock; +pub mod detach; pub mod error; pub mod identity; pub mod passthrough; pub mod runtime; +pub mod shell; pub mod spec; pub mod store; pub mod template; @@ -74,9 +76,11 @@ pub mod template; pub use capability::{Capability, IsolationLevel, SandboxCapabilities, SnapshotSupport}; pub use clock::{Clock, SystemClock}; pub use error::{Error, Result}; -pub use identity::{BoxId, HostRef, SandboxRef, SnapshotId, TemplateName}; +pub use identity::{BoxId, HostRef, ProcessId, SandboxRef, SnapshotId, TemplateName}; pub use passthrough::PassthroughSandbox; -pub use runtime::{BoxInfo, BoxState, ExecOutput, ExecRequest, Host, Sandbox}; +pub use runtime::{ + BoxInfo, BoxState, ExecOutput, ExecRequest, Forward, ForwardGuard, Host, Sandbox, +}; pub use spec::{ BoxSpec, Lifecycle, NetworkPolicy, Placement, PortMapping, Resources, WorkspaceSource, }; diff --git a/crates/tinybox-core/src/passthrough/mod.rs b/crates/tinybox-core/src/passthrough/mod.rs index ef8a3f1..bb137a8 100644 --- a/crates/tinybox-core/src/passthrough/mod.rs +++ b/crates/tinybox-core/src/passthrough/mod.rs @@ -35,8 +35,9 @@ use async_trait::async_trait; use crate::capability::{Capability, SandboxCapabilities}; use crate::clock::{Clock, SystemClock}; +use crate::detach; use crate::error::{Error, Result}; -use crate::identity::{BoxId, SnapshotId}; +use crate::identity::{BoxId, ProcessId, SnapshotId}; use crate::runtime::{BoxInfo, BoxState, ExecOutput, ExecRequest, Host, Sandbox}; use crate::spec::{BoxSpec, WorkspaceSource}; use crate::store::Store; @@ -116,6 +117,30 @@ impl PassthroughSandbox { }; Ok(resolved) } + + /// Look `id` up, check it accepts commands, and resolve `request` + /// against its spec. + /// + /// Shared by `exec` and the detach trio so that a backgrounded command + /// sees the same working directory, environment, and state check a + /// foreground one does. Having two paths here is how they drift. + /// + /// # Errors + /// + /// Returns [`Error::UnknownBox`] when `id` does not resolve, + /// [`Error::InvalidState`] when the box is not accepting commands, and + /// [`Error::EmptyCommand`] when the request names no program. + fn resolved_for(&self, id: &BoxId, request: &ExecRequest) -> Result { + let info = self.store.get(id)?; + if !info.state.accepts_commands() { + return Err(Error::InvalidState { + id: id.as_str().to_owned(), + actual: info.state, + expected: BoxState::Ready, + }); + } + Self::resolve(&info.spec, request) + } } #[async_trait] @@ -125,7 +150,12 @@ impl Sandbox for PassthroughSandbox { } fn capabilities(&self) -> SandboxCapabilities { - SandboxCapabilities::PASSTHROUGH + // Detach, and nothing else. A passthrough box is an ordinary directory + // on an ordinary machine, so a backgrounded process keeps running and + // its pid file is still there next time — which is the whole of what + // `Capability::Detach` promises. The refusals below are unaffected: + // this sandbox still has no filesystem boundary to snapshot. + SandboxCapabilities::PASSTHROUGH.with_detach() } async fn create(&self, spec: &BoxSpec) -> Result { @@ -138,16 +168,7 @@ impl Sandbox for PassthroughSandbox { } async fn exec(&self, id: &BoxId, request: &ExecRequest) -> Result { - let info = self.store.get(id)?; - if !info.state.accepts_commands() { - return Err(Error::InvalidState { - id: id.as_str().to_owned(), - actual: info.state, - expected: BoxState::Ready, - }); - } - - let resolved = Self::resolve(&info.spec, request)?; + let resolved = self.resolved_for(id, request)?; self.host.run(&resolved).await } @@ -172,6 +193,35 @@ impl Sandbox for PassthroughSandbox { async fn destroy(&self, id: &BoxId) -> Result<()> { self.store.remove(id) } + + async fn spawn(&self, id: &BoxId, request: &ExecRequest) -> Result { + let process = detach::mint(); + // Resolved first, so the box's own cwd and environment reach the + // backgrounded command exactly as they would a foreground one. + let resolved = self.resolved_for(id, request)?; + let started = detach::start(&process, &resolved)?; + let output = self.host.run(&started).await?; + if !output.succeeded() { + return Err(Error::Backend { + sandbox: NAME.to_owned(), + operation: "start a detached process", + message: output.stderr_lossy().trim().to_owned(), + }); + } + Ok(process) + } + + async fn is_running(&self, id: &BoxId, process: &ProcessId) -> Result { + let resolved = self.resolved_for(id, &detach::probe(process))?; + let output = self.host.run(&resolved).await?; + Ok(output.stdout_lossy().trim() == detach::RUNNING) + } + + async fn stop(&self, id: &BoxId, process: &ProcessId) -> Result<()> { + let resolved = self.resolved_for(id, &detach::stop(process, detach::DEFAULT_GRACE))?; + self.host.run(&resolved).await?; + Ok(()) + } } #[cfg(test)] diff --git a/crates/tinybox-core/src/passthrough/test.rs b/crates/tinybox-core/src/passthrough/test.rs index 25d2cb7..866acd0 100644 --- a/crates/tinybox-core/src/passthrough/test.rs +++ b/crates/tinybox-core/src/passthrough/test.rs @@ -74,7 +74,10 @@ fn it_admits_it_confines_nothing() { assert!(!caps.is_suitable_for_untrusted_code()); // Limits are declined rather than accepted and quietly ignored. assert!(!caps.supports(Capability::ResourceLimits)); - assert!(caps.declared().is_empty()); + // Detach is the one thing it does declare, and honestly: a box here is a + // directory on this machine, so a backgrounded process really does outlive + // the command that started it. + assert_eq!(caps.declared(), [Capability::Detach]); } #[tokio::test] @@ -339,3 +342,72 @@ async fn a_new_box_records_when_it_was_created() -> Result<()> { ); Ok(()) } + +#[tokio::test] +async fn a_spawned_process_goes_through_the_detach_wrapper() -> Result<()> { + let (sandbox, host) = sandbox(); + let created = sandbox.create(&spec()?).await?; + + let process = sandbox + .spawn(&created.id, &ExecRequest::new(["server", "--port", "7788"])) + .await?; + + let ran = host.last().ok_or(Error::EmptyCommand { + sandbox: NAME.to_owned(), + })?; + // A shell, because backgrounding is a shell's job — and the box's own + // workspace directory, because a detached command must resolve exactly the + // way a foreground one does. + assert_eq!(ran.program(), Some("/bin/sh")); + assert!(ran.argv[2].contains("'server' '--port' '7788'"), "{ran:?}"); + assert!(ran.argv[2].contains(process.as_str()), "{ran:?}"); + Ok(()) +} + +#[tokio::test] +async fn spawning_into_an_unknown_box_fails_before_anything_runs() -> Result<()> { + let (sandbox, host) = sandbox(); + + let outcome = sandbox + .spawn(&BoxId::new("box-0")?, &ExecRequest::new(["server"])) + .await; + + assert!(outcome.is_err()); + assert!(host.seen().is_empty(), "nothing should have been run"); + Ok(()) +} + +#[tokio::test] +async fn a_probe_reports_running_only_when_the_box_says_so() -> Result<()> { + // `RecordingHost` always answers "ran", which is not the marker `probe` + // looks for — so a host that says something unexpected reads as "gone" + // rather than as "running". Guessing the other way would report a live + // server that had actually died. + let (sandbox, _host) = sandbox(); + let created = sandbox.create(&spec()?).await?; + let process = sandbox + .spawn(&created.id, &ExecRequest::new(["server"])) + .await?; + + assert!(!sandbox.is_running(&created.id, &process).await?); + Ok(()) +} + +#[tokio::test] +async fn stopping_succeeds_even_though_nothing_was_really_started() -> Result<()> { + // Stopping something already gone is the outcome the caller wanted, so it + // is not an error. + let (sandbox, host) = sandbox(); + let created = sandbox.create(&spec()?).await?; + let process = sandbox + .spawn(&created.id, &ExecRequest::new(["server"])) + .await?; + + sandbox.stop(&created.id, &process).await?; + + let ran = host.last().ok_or(Error::EmptyCommand { + sandbox: NAME.to_owned(), + })?; + assert!(ran.argv[2].contains("kill -TERM"), "{ran:?}"); + Ok(()) +} diff --git a/crates/tinybox-core/src/runtime/forward.rs b/crates/tinybox-core/src/runtime/forward.rs new file mode 100644 index 0000000..98f8f79 --- /dev/null +++ b/crates/tinybox-core/src/runtime/forward.rs @@ -0,0 +1,82 @@ +//! A live path from this machine to a port somewhere else. + +use std::fmt; +use std::net::SocketAddr; + +/// The far side of a [`Forward`], held open for as long as the forward is. +/// +/// An `ssh -L` tunnel is a child process; a local forward is nothing at all. +/// The trait exists so that core can own the *guarantee* — that dropping a +/// [`Forward`] closes it — without owning any of the machinery, which belongs +/// to whichever host crate created it. +pub trait ForwardGuard: fmt::Debug + Send + Sync { + /// Tear the forward down. Called at most once, from [`Forward`]'s `Drop`. + /// + /// Implementations must not block for long and must not panic: this runs + /// during unwinding as often as not. + fn close(&mut self); +} + +/// A port on another machine, reachable at a local address. +/// +/// [`Host::forward`](crate::runtime::Host::forward) returns one of these, and +/// it is a guard: the path exists for exactly as long as the value does. That +/// is why it is not `Clone` and why [`Forward::local_addr`] borrows rather than +/// handing out an address that could outlive the tunnel carrying it. +/// +/// # Why reach includes this +/// +/// A [`Sandbox`](crate::runtime::Sandbox) publishes a guest port to *its +/// host's* address space — that is what +/// [`PortMapping`](crate::spec::PortMapping) means. When the host is remote, +/// the caller still cannot reach it, and no amount of sandbox-side +/// configuration changes that. Closing the gap is a reach question, so it is +/// the [`Host`](crate::runtime::Host)'s to answer. +#[derive(Debug)] +pub struct Forward { + local: SocketAddr, + guard: Option>, +} + +impl Forward { + /// A forward that needs nothing held open. + /// + /// A local host returns this: the address is already reachable, so there is + /// no tunnel and nothing to tear down. + #[must_use] + pub const fn direct(local: SocketAddr) -> Self { + Self { local, guard: None } + } + + /// A forward that lives for as long as `guard` is held. + #[must_use] + pub fn guarded(local: SocketAddr, guard: Box) -> Self { + Self { + local, + guard: Some(guard), + } + } + + /// Where to connect on this machine. + #[must_use] + pub const fn local_addr(&self) -> SocketAddr { + self.local + } + + /// Whether anything is being held open on this forward's behalf. + /// + /// A caller has no reason to branch on this; it is here so that a host's + /// tests can tell a real tunnel from a direct answer. + #[must_use] + pub const fn is_direct(&self) -> bool { + self.guard.is_none() + } +} + +impl Drop for Forward { + fn drop(&mut self) { + if let Some(guard) = self.guard.as_mut() { + guard.close(); + } + } +} diff --git a/crates/tinybox-core/src/runtime/forward_test.rs b/crates/tinybox-core/src/runtime/forward_test.rs new file mode 100644 index 0000000..34a7f3b --- /dev/null +++ b/crates/tinybox-core/src/runtime/forward_test.rs @@ -0,0 +1,51 @@ +//! Tests for the [`Forward`](super::Forward) guard. +//! +//! What matters here is the guarantee the type exists to make: the tunnel +//! behind a forward is closed when the forward is dropped, exactly once, even +//! though core owns none of the machinery doing the closing. + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use super::{Forward, ForwardGuard}; + +/// A guard that counts how many times it was closed. +#[derive(Debug)] +struct Counted(Arc); + +impl ForwardGuard for Counted { + fn close(&mut self) { + self.0.fetch_add(1, Ordering::Relaxed); + } +} + +#[test] +fn a_direct_forward_holds_nothing_open() { + // A local host answers this way: the address is already reachable, so + // there is no tunnel and nothing to tear down. + let forward = Forward::direct(([127, 0, 0, 1], 7788).into()); + + assert!(forward.is_direct()); + assert_eq!(forward.local_addr().port(), 7788); +} + +#[test] +fn dropping_a_guarded_forward_closes_it_exactly_once() { + let closes = Arc::new(AtomicUsize::new(0)); + { + let forward = Forward::guarded( + ([127, 0, 0, 1], 1234).into(), + Box::new(Counted(closes.clone())), + ); + assert!(!forward.is_direct()); + assert_eq!(closes.load(Ordering::Relaxed), 0, "not closed while held"); + } + + assert_eq!(closes.load(Ordering::Relaxed), 1); +} + +#[test] +fn dropping_a_direct_forward_is_harmless() { + // Nothing to close, and `Drop` must not assume there is. + drop(Forward::direct(([127, 0, 0, 1], 1).into())); +} diff --git a/crates/tinybox-core/src/runtime/mod.rs b/crates/tinybox-core/src/runtime/mod.rs index 4df50a9..73f32ac 100644 --- a/crates/tinybox-core/src/runtime/mod.rs +++ b/crates/tinybox-core/src/runtime/mod.rs @@ -15,19 +15,25 @@ //! //! [`Sandbox::capabilities`] must describe what the backend really does. Core //! checks the declaration before dispatching and surfaces -//! [`Error::Unsupported`](crate::error::Error::Unsupported), so a backend should +//! [`Error::Unsupported`], so a backend should //! return an accurate [`SandboxCapabilities`] and let the check fail rather //! than emulate something it cannot deliver. use async_trait::async_trait; -use crate::capability::SandboxCapabilities; -use crate::error::Result; -use crate::identity::{BoxId, SnapshotId}; +use std::net::SocketAddr; + +use crate::capability::{Capability, SandboxCapabilities}; +use crate::error::{Error, Result}; +use crate::identity::{BoxId, ProcessId, SnapshotId}; use crate::spec::BoxSpec; +mod forward; +#[cfg(test)] +mod forward_test; mod types; +pub use forward::{Forward, ForwardGuard}; pub use types::{BoxInfo, BoxState, ExecOutput, ExecRequest}; /// A machine tinybox can reach and run commands on. @@ -51,6 +57,32 @@ pub trait Host: std::fmt::Debug + Send + Sync + 'static { /// [`ExecOutput::exit_code`], because a failing command is a result, not a /// transport fault. async fn run(&self, request: &ExecRequest) -> Result; + + /// Make `remote` — an address in *this host's* address space — reachable + /// from the machine tinybox is running on. + /// + /// A sandbox publishing a guest port + /// ([`PortMapping`](crate::spec::PortMapping)) puts it on its host. When + /// that host is another machine, the caller still cannot connect, and no + /// sandbox-side configuration fixes it — closing that gap is a question + /// about reach, which is this trait's subject. A local host answers by + /// handing the address straight back. + /// + /// The returned [`Forward`] is a guard: the path lasts exactly as long as + /// it is held. + /// + /// # Errors + /// + /// Returns [`Error::Unsupported`] by default, so a host that cannot tunnel + /// says so rather than returning an address nothing is listening on. Also + /// returns an error when the tunnel cannot be established. + async fn forward(&self, remote: SocketAddr) -> Result { + let _ = remote; + Err(Error::Unsupported { + sandbox: self.name().to_owned(), + capability: Capability::PortForward, + }) + } } /// A confinement that boxes are created inside. @@ -81,8 +113,8 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static { /// /// # Errors /// - /// Returns [`Error::UnknownBox`](crate::error::Error::UnknownBox) when `id` does - /// not resolve, [`Error::InvalidState`](crate::error::Error::InvalidState) when + /// Returns [`Error::UnknownBox`] when `id` does + /// not resolve, [`Error::InvalidState`] when /// the box is not running, or a backend error when the command cannot be /// started. async fn exec(&self, id: &BoxId, request: &ExecRequest) -> Result; @@ -95,9 +127,9 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static { /// /// # Errors /// - /// Returns [`Error::Unsupported`](crate::error::Error::Unsupported) when this + /// Returns [`Error::Unsupported`] when this /// sandbox does not snapshot, or - /// [`Error::UnknownBox`](crate::error::Error::UnknownBox) when `id` does not + /// [`Error::UnknownBox`] when `id` does not /// resolve. async fn snapshot(&self, id: &BoxId) -> Result; @@ -108,9 +140,9 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static { /// /// # Errors /// - /// Returns [`Error::Unsupported`](crate::error::Error::Unsupported) when this + /// Returns [`Error::Unsupported`] when this /// sandbox cannot fork, or - /// [`Error::UnknownSnapshot`](crate::error::Error::UnknownSnapshot) when + /// [`Error::UnknownSnapshot`] when /// `snapshot` does not resolve. async fn fork(&self, snapshot: &SnapshotId, spec: &BoxSpec) -> Result; @@ -118,7 +150,7 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static { /// /// # Errors /// - /// Returns [`Error::UnknownBox`](crate::error::Error::UnknownBox) when `id` does + /// Returns [`Error::UnknownBox`] when `id` does /// not resolve. async fn inspect(&self, id: &BoxId) -> Result; @@ -126,9 +158,68 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static { /// /// # Errors /// - /// Returns [`Error::UnknownBox`](crate::error::Error::UnknownBox) when `id` does + /// Returns [`Error::UnknownBox`] when `id` does /// not resolve. async fn destroy(&self, id: &BoxId) -> Result<()>; + + /// Start a command in a box and leave it running. + /// + /// Where [`Sandbox::exec`] waits, this returns as soon as the process is + /// started, handing back an identifier for asking about it later. It is how + /// a server gets into a box; `exec` would never return. + /// + /// See [`detach`](crate::detach) for the mechanism, and for what a backend + /// is promising by declaring + /// [`Capability::Detach`]. + /// + /// # Errors + /// + /// Returns [`Error::Unsupported`] by default. A sandbox that cannot host a + /// process between commands must leave it that way: a background process + /// that cannot be found or stopped is worse than a refusal, because it + /// looks like it worked. + async fn spawn(&self, id: &BoxId, request: &ExecRequest) -> Result { + let (_, _) = (id, request); + Err(Error::Unsupported { + sandbox: self.name().to_owned(), + capability: Capability::Detach, + }) + } + + /// Whether a process started by [`Sandbox::spawn`] is still running. + /// + /// A process that has finished is `false`, not an error: "it exited" is an + /// answer, and conflating it with an unreachable box would hide a real + /// failure behind an ordinary one. + /// + /// # Errors + /// + /// Returns [`Error::Unsupported`] by default, and a backend error when the + /// box cannot be reached to ask. + async fn is_running(&self, id: &BoxId, process: &ProcessId) -> Result { + let (_, _) = (id, process); + Err(Error::Unsupported { + sandbox: self.name().to_owned(), + capability: Capability::Detach, + }) + } + + /// Stop a process started by [`Sandbox::spawn`]. + /// + /// Succeeds when the process was already gone: stopping something that has + /// already stopped is the outcome the caller wanted. + /// + /// # Errors + /// + /// Returns [`Error::Unsupported`] by default, and a backend error when the + /// box cannot be reached. + async fn stop(&self, id: &BoxId, process: &ProcessId) -> Result<()> { + let (_, _) = (id, process); + Err(Error::Unsupported { + sandbox: self.name().to_owned(), + capability: Capability::Detach, + }) + } } #[cfg(test)] diff --git a/crates/tinybox-core/src/runtime/test.rs b/crates/tinybox-core/src/runtime/test.rs index 12216aa..c18e9ac 100644 --- a/crates/tinybox-core/src/runtime/test.rs +++ b/crates/tinybox-core/src/runtime/test.rs @@ -449,3 +449,80 @@ fn a_box_with_no_recorded_creation_time_never_expires() -> Result<()> { assert!(!info.is_expired(SystemTime::UNIX_EPOCH + Duration::from_secs(86_400))); Ok(()) } + +/// The defaults exist so a backend opts *in* to the two operations added for +/// services. Neither `FakeSandbox` nor `FakeHost` overrides them, which is +/// exactly the case these check. +mod defaults { + use super::{CONTAINER, FakeHost, FakeSandbox, spec}; + use crate::capability::Capability; + use crate::error::{Error, Result}; + use crate::identity::{BoxId, ProcessId}; + use crate::runtime::{ExecRequest, Host, Sandbox}; + + fn process() -> Result { + ProcessId::new("p1-0") + } + + #[tokio::test] + async fn a_sandbox_that_does_not_override_them_refuses_all_three() -> Result<()> { + // Silence is not an option here: a background process a sandbox cannot + // find or stop again is worse than a refusal, because it looks like it + // worked. + let sandbox = FakeSandbox::new(CONTAINER); + let created = sandbox.create(&spec()?).await?; + let expected = Some(Error::Unsupported { + sandbox: "fake".to_owned(), + capability: Capability::Detach, + }); + + assert_eq!( + sandbox + .spawn(&created.id, &ExecRequest::new(["server"])) + .await + .err(), + expected + ); + assert_eq!( + sandbox.is_running(&created.id, &process()?).await.err(), + expected + ); + assert_eq!(sandbox.stop(&created.id, &process()?).await.err(), expected); + Ok(()) + } + + #[tokio::test] + async fn the_refusal_names_the_sandbox_that_refused() -> Result<()> { + // Two sandboxes in one process both refusing "detached processes" is + // useless if neither says which box the caller was talking to. + let sandbox = FakeSandbox::new(CONTAINER); + + let outcome = sandbox + .spawn(&BoxId::new("box-0")?, &ExecRequest::new(["server"])) + .await; + + assert!( + outcome + .err() + .is_some_and(|error| error.to_string().contains("fake")), + ); + Ok(()) + } + + #[tokio::test] + async fn a_host_that_cannot_tunnel_says_so_rather_than_answering() -> Result<()> { + // Returning the address unchanged would be the tempting default and the + // wrong one: the caller would connect to a port on their own machine + // that nothing is listening on. + let outcome = FakeHost.forward(([127, 0, 0, 1], 7788).into()).await; + + assert_eq!( + outcome.err(), + Some(Error::Unsupported { + sandbox: "fake-host".to_owned(), + capability: Capability::PortForward, + }) + ); + Ok(()) + } +} diff --git a/crates/tinybox-ssh/src/host/quote.rs b/crates/tinybox-core/src/shell/mod.rs similarity index 62% rename from crates/tinybox-ssh/src/host/quote.rs rename to crates/tinybox-core/src/shell/mod.rs index 47ab223..3688e1c 100644 --- a/crates/tinybox-ssh/src/host/quote.rs +++ b/crates/tinybox-core/src/shell/mod.rs @@ -1,17 +1,24 @@ -//! Turning an argument vector into something a remote shell will not mangle. +//! Turning an argument vector into something a POSIX shell will not mangle. //! //! # Why this exists at all //! //! tinybox passes commands as argument vectors precisely so that no backend has -//! to quote and no caller can inject through a filename. SSH breaks that -//! guarantee: its exec channel carries a command *string*, which the remote -//! login shell then parses. That is true of the protocol, not of shelling out — -//! an SSH library would face exactly the same problem. +//! to quote and no caller can inject through a filename. Two things break that +//! guarantee, and both are properties of a protocol rather than of shelling +//! out: +//! +//! - **SSH** carries a command *string* on its exec channel, which the remote +//! login shell then parses. An embedded SSH client would face this too. +//! - **A detached process** ([`crate::detach`]) needs a shell on the far side to +//! background the command and record its pid, because no transport tinybox +//! speaks returns a process handle a caller could hold. //! //! So this is the one place in tinybox where the no-quoting property has to be //! re-established by hand, which makes it the one place where a bug is a -//! command-injection bug. It is a pure function for that reason: every case can -//! be pinned in a test. +//! command-injection bug. It lives in core, and is public, so that it stays +//! *one* place: a second copy is a second chance to get it wrong, and the two +//! callers are in different crates. Every function here is pure, so every case +//! can be pinned in a test. /// Wrap one argument so a POSIX shell reproduces it exactly. /// @@ -22,7 +29,8 @@ /// /// An empty argument still needs quoting, or it would vanish from the command /// line rather than arriving as an empty string. -fn quote(argument: &str) -> String { +#[must_use] +pub fn quote(argument: &str) -> String { let mut quoted = String::with_capacity(argument.len() + 2); quoted.push('\''); for character in argument.chars() { @@ -42,7 +50,8 @@ fn quote(argument: &str) -> String { /// Every argument is quoted, including the program name: a program path /// containing a space is unusual but not invalid, and treating the first /// argument specially is how that becomes a bug. -pub(super) fn command_line(argv: I) -> String +#[must_use] +pub fn command_line(argv: I) -> String where I: IntoIterator, S: AsRef, @@ -53,16 +62,18 @@ where .join(" ") } -/// Build the full remote command, including working directory and environment. +/// Build a full command, including working directory and environment. /// -/// SSH does not carry the caller's environment or working directory, so both -/// are applied by the remote shell. `cd` runs first and is chained with `&&`, -/// so a missing directory fails the command rather than silently running it +/// A shell does not inherit the caller's working directory or environment +/// across any of the transports tinybox uses, so both are applied by the shell +/// that runs the command. `cd` runs first and is chained with `&&`, so a +/// missing directory fails the command rather than silently running it /// somewhere else — which for a build command would be worse than an error. /// /// `env` is used rather than `KEY=value command` prefixes because it applies /// cleanly whatever the command is, including a shell builtin. -pub(super) fn remote_command( +#[must_use] +pub fn script( argv: &[String], cwd: Option<&std::path::Path>, env: &std::collections::BTreeMap, diff --git a/crates/tinybox-ssh/src/host/quote/test.rs b/crates/tinybox-core/src/shell/test.rs similarity index 88% rename from crates/tinybox-ssh/src/host/quote/test.rs rename to crates/tinybox-core/src/shell/test.rs index 00d839a..38e2d2f 100644 --- a/crates/tinybox-ssh/src/host/quote/test.rs +++ b/crates/tinybox-core/src/shell/test.rs @@ -1,4 +1,4 @@ -//! Tests for remote shell quoting. +//! Tests for POSIX shell quoting. //! //! A bug here is a command-injection bug, so these are exhaustive about the //! metacharacters a shell acts on rather than sampling a few. `live_ssh.rs` @@ -8,7 +8,7 @@ use std::collections::BTreeMap; use std::path::Path; -use super::{command_line, remote_command}; +use super::{command_line, script}; /// Every construct a POSIX shell would otherwise act on. const DANGEROUS: [&str; 16] = [ @@ -104,7 +104,7 @@ fn a_quoted_argument_round_trips_through_a_real_shell() { fn a_bare_command_has_no_prefix() { let argv = vec!["ls".to_owned(), "-la".to_owned()]; - assert_eq!(remote_command(&argv, None, &BTreeMap::new()), "'ls' '-la'"); + assert_eq!(script(&argv, None, &BTreeMap::new()), "'ls' '-la'"); } #[test] @@ -114,7 +114,7 @@ fn a_working_directory_is_entered_first_and_chained_with_and() { // `&&` rather than `;` so a missing directory fails the command instead of // running it somewhere unexpected. assert_eq!( - remote_command(&argv, Some(Path::new("/srv/work")), &BTreeMap::new()), + script(&argv, Some(Path::new("/srv/work")), &BTreeMap::new()), "cd '/srv/work' && 'ls'" ); } @@ -124,11 +124,11 @@ fn a_working_directory_with_a_space_or_quote_is_quoted() { let argv = vec!["pwd".to_owned()]; assert_eq!( - remote_command(&argv, Some(Path::new("/srv/my work")), &BTreeMap::new()), + script(&argv, Some(Path::new("/srv/my work")), &BTreeMap::new()), "cd '/srv/my work' && 'pwd'" ); assert_eq!( - remote_command(&argv, Some(Path::new("/srv/it's")), &BTreeMap::new()), + script(&argv, Some(Path::new("/srv/it's")), &BTreeMap::new()), r"cd '/srv/it'\''s' && 'pwd'" ); } @@ -139,10 +139,7 @@ fn environment_is_applied_with_env_and_fully_quoted() { let mut env = BTreeMap::new(); env.insert("SIMPLE".to_owned(), "value".to_owned()); - assert_eq!( - remote_command(&argv, None, &env), - "env 'SIMPLE=value' 'printenv'" - ); + assert_eq!(script(&argv, None, &env), "env 'SIMPLE=value' 'printenv'"); } #[test] @@ -153,7 +150,7 @@ fn a_value_that_looks_like_a_command_stays_a_value() { // The whole `KEY=value` pair is one quoted word, so the semicolon is data. assert_eq!( - remote_command(&argv, None, &env), + script(&argv, None, &env), "env 'EVIL=; rm -rf /' 'printenv'" ); } @@ -168,7 +165,7 @@ fn a_directory_and_an_environment_compose() { // Ordered, because the environment is a BTreeMap: two requests differing // only in insertion order produce the same command. assert_eq!( - remote_command(&argv, Some(Path::new("/w")), &env), + script(&argv, Some(Path::new("/w")), &env), "cd '/w' && env 'A=1' 'B=2' 'make'" ); } diff --git a/crates/tinybox-docker/src/sandbox/mod.rs b/crates/tinybox-docker/src/sandbox/mod.rs index 5f4ca0e..119e2f0 100644 --- a/crates/tinybox-docker/src/sandbox/mod.rs +++ b/crates/tinybox-docker/src/sandbox/mod.rs @@ -5,8 +5,8 @@ use std::sync::Arc; use async_trait::async_trait; use tinybox_core::{ BoxId, BoxInfo, BoxSpec, BoxState, Capability, Clock, Error, ExecOutput, ExecRequest, Host, - IsolationLevel, Result, Sandbox, SandboxCapabilities, SnapshotId, SnapshotSupport, Store, - SystemClock, + IsolationLevel, ProcessId, Result, Sandbox, SandboxCapabilities, SnapshotId, SnapshotSupport, + Store, SystemClock, detach, }; mod args; @@ -116,12 +116,18 @@ impl DockerSandbox { /// `PortForward` **is** declared, because ports are named in the /// [`BoxSpec`] and applied at creation — which is the only moment a /// container can gain one. + /// + /// `Detach` is declared because a container is a running machine between + /// commands: `args::run` starts it with `--detach` and a keepalive, so a + /// backgrounded process and the pid file naming it are both still there on + /// the next `docker exec`. #[must_use] pub const fn declared_capabilities() -> SandboxCapabilities { SandboxCapabilities::new(IsolationLevel::Kernel, SnapshotSupport::Filesystem) .with_fork() .with_resource_limits() .with_port_forward() + .with_detach() } /// Run a `docker` command, treating a non-zero exit as a failure. @@ -179,6 +185,30 @@ impl Sandbox for DockerSandbox { Ok(info) } + async fn spawn(&self, id: &BoxId, request: &ExecRequest) -> Result { + let process = detach::mint(); + let output = self.exec(id, &detach::start(&process, request)?).await?; + if !output.succeeded() { + return Err(Error::Backend { + sandbox: NAME.to_owned(), + operation: "start a detached process", + message: output.stderr_lossy().trim().to_owned(), + }); + } + Ok(process) + } + + async fn is_running(&self, id: &BoxId, process: &ProcessId) -> Result { + let output = self.exec(id, &detach::probe(process)).await?; + Ok(output.stdout_lossy().trim() == detach::RUNNING) + } + + async fn stop(&self, id: &BoxId, process: &ProcessId) -> Result<()> { + self.exec(id, &detach::stop(process, detach::DEFAULT_GRACE)) + .await?; + Ok(()) + } + async fn exec(&self, id: &BoxId, request: &ExecRequest) -> Result { let info = self.inspect(id).await?; if !info.state.accepts_commands() { diff --git a/crates/tinybox-docker/src/sandbox/test.rs b/crates/tinybox-docker/src/sandbox/test.rs index 1ce8a0d..85605ed 100644 --- a/crates/tinybox-docker/src/sandbox/test.rs +++ b/crates/tinybox-docker/src/sandbox/test.rs @@ -747,3 +747,107 @@ async fn a_new_container_records_when_it_was_created() -> Result<()> { ); Ok(()) } + +#[tokio::test] +async fn a_detached_process_is_started_through_docker_exec() -> Result<()> { + let (sandbox, host, _store) = sandbox(); + let info = sandbox.create(&spec()?).await?; + host.push_ok("running"); // inspect + + let process = sandbox + .spawn(&info.id, &ExecRequest::new(["openhuman-core", "serve"])) + .await?; + + let argv = host.command(2).unwrap_or_default(); + assert_eq!(argv[0..2], ["docker", "exec"]); + // No `--detach`: the wrapper's own `&` is what backgrounds the process, + // which is also what makes the pid recoverable. `docker exec --detach` + // hands back nothing a caller could name. + assert!(!argv.contains(&"--detach".to_owned()), "{argv:?}"); + let line = argv.last().map(String::as_str).unwrap_or_default(); + assert!(line.contains("'openhuman-core' 'serve'"), "{line:?}"); + assert!(line.contains(process.as_str()), "{line:?}"); + Ok(()) +} + +#[tokio::test] +async fn the_detach_wrapper_carries_cwd_and_env_rather_than_docker_flags() -> Result<()> { + // Both would work, but only one of them survives the shell that has to + // background the command, so the wrapper owns them and `args::exec` sees a + // request with neither. + let (sandbox, host, _store) = sandbox(); + let info = sandbox.create(&spec()?).await?; + host.push_ok("running"); // inspect + + sandbox + .spawn( + &info.id, + &ExecRequest::new(["server"]) + .with_cwd("/srv/work") + .with_env("PORT", "7788"), + ) + .await?; + + let argv = host.command(2).unwrap_or_default(); + assert!(!argv.contains(&"--workdir".to_owned()), "{argv:?}"); + assert!(!argv.contains(&"--env".to_owned()), "{argv:?}"); + let line = argv.last().map(String::as_str).unwrap_or_default(); + assert!(line.contains("cd '/srv/work' &&"), "{line:?}"); + assert!(line.contains("env 'PORT=7788'"), "{line:?}"); + Ok(()) +} + +#[tokio::test] +async fn a_probe_answers_from_what_the_container_printed() -> Result<()> { + let (sandbox, host, _store) = sandbox(); + let info = sandbox.create(&spec()?).await?; + host.push_ok("running"); // inspect + host.push_ok("running"); // the probe itself + + assert!( + sandbox + .is_running(&info.id, &tinybox_core::ProcessId::new("p1-0")?) + .await? + ); + + host.push_ok("running"); // inspect + host.push_ok("gone"); + assert!( + !sandbox + .is_running(&info.id, &tinybox_core::ProcessId::new("p1-0")?) + .await? + ); + Ok(()) +} + +#[tokio::test] +async fn a_failed_start_carries_the_containers_diagnostic() -> Result<()> { + // Unlike `exec`, where a non-zero status is a result, a detached start that + // fails means no process exists — so it is an error, not an empty success + // the caller would later probe and find "gone" for no stated reason. + let (sandbox, host, _store) = sandbox(); + let info = sandbox.create(&spec()?).await?; + host.push_ok("running"); // inspect + host.push_failure("/bin/sh: openhuman-core: not found"); + + let outcome = sandbox + .spawn(&info.id, &ExecRequest::new(["openhuman-core"])) + .await; + + assert_eq!( + outcome.err(), + Some(Error::Backend { + sandbox: NAME.to_owned(), + operation: "start a detached process", + message: "/bin/sh: openhuman-core: not found".to_owned(), + }) + ); + Ok(()) +} + +#[test] +fn detach_is_declared_because_a_container_persists_between_commands() { + let declared = DockerSandbox::declared_capabilities(); + + assert!(declared.supports(Capability::Detach)); +} diff --git a/crates/tinybox-host/src/local/mod.rs b/crates/tinybox-host/src/local/mod.rs index ac19bc5..e0b1dcb 100644 --- a/crates/tinybox-host/src/local/mod.rs +++ b/crates/tinybox-host/src/local/mod.rs @@ -1,7 +1,7 @@ //! Running commands on the machine tinybox is running on. use async_trait::async_trait; -use tinybox_core::{Error, ExecOutput, ExecRequest, Host, Result}; +use tinybox_core::{Error, ExecOutput, ExecRequest, Forward, Host, Result}; use tokio::io::AsyncWriteExt as _; use tokio::process::Command; @@ -112,6 +112,21 @@ impl Host for LocalHost { .map_err(|error| Error::io("wait", &error))?; Ok(Self::collect(&output)) } + + /// Hand the address straight back. + /// + /// A port published on this machine is already reachable from this + /// machine, so there is nothing to tunnel and nothing to hold open. The + /// method exists so that a caller can ask any host for reach without first + /// asking which kind of host it has — the difference between `local` and + /// `ssh` should not leak into code that only wants somewhere to connect. + /// + /// # Errors + /// + /// Never. The signature is fallible because other hosts' forwards are. + async fn forward(&self, remote: std::net::SocketAddr) -> Result { + Ok(Forward::direct(remote)) + } } impl LocalHost { diff --git a/crates/tinybox-host/src/local/test.rs b/crates/tinybox-host/src/local/test.rs index c40e78c..b34578e 100644 --- a/crates/tinybox-host/src/local/test.rs +++ b/crates/tinybox-host/src/local/test.rs @@ -266,3 +266,17 @@ async fn a_command_that_ignores_its_input_does_not_fail_the_write() -> Result<() assert!(outcome.is_ok() || matches!(outcome, Err(Error::Io { .. }))); Ok(()) } + +#[tokio::test] +async fn a_local_forward_is_the_address_itself() -> Result<()> { + // Nothing to tunnel: a port published on this machine is already reachable + // from it. The method exists so a caller can ask any host for reach without + // first asking which kind of host it has. + let forwarded = LocalHost::new() + .forward(([127, 0, 0, 1], 7788).into()) + .await?; + + assert_eq!(forwarded.local_addr(), ([127, 0, 0, 1], 7788).into()); + assert!(forwarded.is_direct()); + Ok(()) +} diff --git a/crates/tinybox-microvm/src/sandbox/guest/test.rs b/crates/tinybox-microvm/src/sandbox/guest/test.rs index 238c28b..593628e 100644 --- a/crates/tinybox-microvm/src/sandbox/guest/test.rs +++ b/crates/tinybox-microvm/src/sandbox/guest/test.rs @@ -257,8 +257,14 @@ fn decode(encoded: &str) -> String { } } + // `as_chunks` rather than `chunks_exact(8)`: the size is a constant, so the + // array form is what clippy asks for and it drops the trailing partial + // chunk the same way. Unrelated to this test's subject; the lint arrived + // with a newer toolchain. let bytes = bits - .chunks_exact(8) + .as_chunks::<8>() + .0 + .iter() .map(|chunk| chunk.iter().fold(0u8, |acc, bit| (acc << 1) | *bit)) .collect::>(); String::from_utf8_lossy(&bytes).into_owned() diff --git a/crates/tinybox-ssh/Cargo.toml b/crates/tinybox-ssh/Cargo.toml index c33cccd..167b0fb 100644 --- a/crates/tinybox-ssh/Cargo.toml +++ b/crates/tinybox-ssh/Cargo.toml @@ -15,6 +15,11 @@ publish = false [dependencies] tinybox-core.workspace = true async-trait.workspace = true +# A port forward is a process that outlives the call creating it, so unlike +# every other operation here it cannot be handed to the inner host. `net` and +# `time` are what waiting for the tunnel's local listener needs without +# blocking a runtime worker for the whole timeout. +tokio = { workspace = true, features = ["net", "time"] } [dev-dependencies] # `sync` for the OnceCell that shares one test server across the suite. diff --git a/crates/tinybox-ssh/src/host/forward.rs b/crates/tinybox-ssh/src/host/forward.rs new file mode 100644 index 0000000..9a6b5b0 --- /dev/null +++ b/crates/tinybox-ssh/src/host/forward.rs @@ -0,0 +1,213 @@ +//! Making a port on the far machine reachable from this one. +//! +//! Everything else in this crate builds a command line and hands it to an inner +//! [`Host`](tinybox_core::Host) to run to completion. A tunnel cannot work that +//! way: it *is* the running process, and it has to outlive the call that +//! created it. So this module spawns `ssh -N -L` directly and hands the child +//! to a [`Forward`] guard, which kills it on drop. + +use std::net::{SocketAddr, TcpListener}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +use tinybox_core::{Error, Forward, ForwardGuard, Result}; + +use super::target::SshTarget; + +/// How long to wait for the tunnel's local listener to start accepting. +/// +/// `ssh` binds the local side early — before authenticating, and before the far +/// side has agreed to anything — so this waits on the listener appearing and +/// nothing more. See [`open`] for what that does and does not prove. +const LISTEN_TIMEOUT: Duration = Duration::from_secs(10); + +/// How often to retry the local connect while waiting. +const POLL_INTERVAL: Duration = Duration::from_millis(50); + +/// The `ssh` command that carries a forward and nothing else. +/// +/// Pure, and separate from spawning it, for the reason ADR 0004 gives for +/// `tinybox-docker`'s `args` module: which flags a backend chooses is the +/// interesting part, and it should be assertable as a value rather than only +/// observable by running the tool. +fn tunnel_command(target: &SshTarget, local_port: u16, remote: SocketAddr) -> Vec { + let mut argv = vec!["ssh".to_owned()]; + argv.extend(target.connection_flags()); + // Do not run a remote command: this connection exists only to carry the + // forward, and a login shell on the far side would be one more thing to + // fail. + argv.push("-N".to_owned()); + // Fail loudly rather than sitting there connected with no forward, which + // would look identical to success until the first connection attempt. + argv.push("-o".to_owned()); + argv.push("ExitOnForwardFailure=yes".to_owned()); + // Notice a dead peer instead of holding a tunnel that stopped working. + argv.push("-o".to_owned()); + argv.push("ServerAliveInterval=15".to_owned()); + argv.push("-L".to_owned()); + argv.push(format!( + "127.0.0.1:{local_port}:{}:{}", + remote.ip(), + remote.port() + )); + argv.push(target.destination().to_owned()); + argv +} + +/// A child process holding a forward open, killed when the [`Forward`] that +/// owns it is dropped. +#[derive(Debug)] +struct SshTunnel { + child: Child, +} + +impl SshTunnel { + /// Start `argv`, with every stream detached except the stderr a failure + /// diagnostic is read from. + /// + /// # Errors + /// + /// Returns [`Error::Io`] when the program cannot be started at all — no + /// `ssh` on `PATH` being the usual reason. + fn spawn(argv: &[String]) -> Result { + let mut command = Command::new(&argv[0]); + command.args(&argv[1..]); + command.stdin(Stdio::null()); + command.stdout(Stdio::null()); + command.stderr(Stdio::piped()); + + let child = command + .spawn() + .map_err(|error| Error::io("spawn ssh for a port forward", &error))?; + Ok(Self { child }) + } +} + +impl ForwardGuard for SshTunnel { + fn close(&mut self) { + // Both results are deliberately ignored: a tunnel whose `ssh` already + // exited is closed, which is the state this method exists to reach. + // `wait` follows `kill` so the child is reaped rather than left a + // zombie for the lifetime of the host process. + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +/// Reserve a free port on the loopback interface. +/// +/// Binding and immediately closing is the portable way to have the operating +/// system choose; `ssh -L` cannot report back a port it chose itself, so the +/// choice has to be made here. The gap between closing and `ssh` binding is a +/// race in principle. In practice nothing else is handing out ephemeral ports +/// in that window, and `ExitOnForwardFailure` turns a lost race into an +/// immediate failure rather than a tunnel to nowhere. +fn reserve_local_port() -> Result { + TcpListener::bind(("127.0.0.1", 0)) + .and_then(|listener| listener.local_addr()) + .map(|address| address.port()) + .map_err(|error| Error::io("reserve a local port", &error)) +} + +/// Open a tunnel from a local loopback port to `remote` on `target`. +/// +/// # What a successful return proves, and what it does not +/// +/// It proves a local listener exists. It does **not** prove the far side is +/// reachable: `ssh` binds the local port before it authenticates, so a +/// destination that will be refused can still produce a working listener for a +/// moment, and a connection through it then fails. `ExitOnForwardFailure=yes` +/// and the child-death check below narrow that window rather than closing it, +/// because it cannot be closed from here — only the far side knows. +/// +/// So a caller that needs a *working* endpoint must check the endpoint. That is +/// not a shortcoming of this function: whatever is listening over there has its +/// own readiness, later than the tunnel's, and only the caller knows how to ask +/// about it. +/// +/// # Errors +/// +/// Returns [`Error::Io`] when a local port cannot be reserved or `ssh` cannot +/// be started, and [`Error::Backend`] when `ssh` exits before the listener +/// appears, or when it never appears within [`LISTEN_TIMEOUT`]. +pub(super) async fn open(target: &SshTarget, remote: SocketAddr) -> Result { + let local_port = reserve_local_port()?; + let local: SocketAddr = ([127, 0, 0, 1], local_port).into(); + + let mut tunnel = SshTunnel::spawn(&tunnel_command(target, local_port, remote))?; + + match wait_until_listening(&mut tunnel, local, LISTEN_TIMEOUT).await { + Ok(()) => Ok(Forward::guarded(local, Box::new(tunnel))), + Err(error) => { + // Do not leave an `ssh` behind for a forward the caller will never + // be handed. + tunnel.close(); + Err(error) + } + } +} + +/// Wait until something accepts on `local`, or the tunnel dies, or `timeout` +/// runs out. +/// +/// The deadline is a parameter rather than a constant read here so the +/// giving-up path is reachable in a test without waiting out +/// [`LISTEN_TIMEOUT`]; `open` supplies that constant. +/// +/// Asynchronous throughout: `Host::forward` is called from a runtime worker, +/// and a ten-second blocking poll there would stall every other task sharing +/// that thread. +async fn wait_until_listening( + tunnel: &mut SshTunnel, + local: SocketAddr, + timeout: Duration, +) -> Result<()> { + let deadline = Instant::now() + timeout; + loop { + if tokio::net::TcpStream::connect(local).await.is_ok() { + return Ok(()); + } + // An `ssh` that has already exited is never going to start listening, + // so report its own diagnostic instead of waiting out the deadline. + if let Ok(Some(_)) = tunnel.child.try_wait() { + return Err(Error::Backend { + sandbox: super::NAME.to_owned(), + operation: "open a port forward", + message: exit_diagnostic(tunnel), + }); + } + if Instant::now() >= deadline { + return Err(Error::Backend { + sandbox: super::NAME.to_owned(), + operation: "open a port forward", + message: format!( + "the forward did not start accepting on {local} within {}ms", + timeout.as_millis() + ), + }); + } + tokio::time::sleep(POLL_INTERVAL).await; + } +} + +/// Whatever `ssh` said on its way out. +/// +/// Falls back to a description rather than an empty string: an error with no +/// message is the least useful thing this could report. +fn exit_diagnostic(tunnel: &mut SshTunnel) -> String { + use std::io::Read as _; + + let mut text = String::new(); + if let Some(stderr) = tunnel.child.stderr.as_mut() { + let _ = stderr.read_to_string(&mut text); + } + let trimmed = text.trim(); + if trimmed.is_empty() { + "ssh exited before the forward was established".to_owned() + } else { + trimmed.to_owned() + } +} + +#[cfg(test)] +mod test; diff --git a/crates/tinybox-ssh/src/host/forward/test.rs b/crates/tinybox-ssh/src/host/forward/test.rs new file mode 100644 index 0000000..afa23fa --- /dev/null +++ b/crates/tinybox-ssh/src/host/forward/test.rs @@ -0,0 +1,230 @@ +//! Tests for the SSH port forward. +//! +//! Driving a real tunnel needs a real sshd, which is `live_ssh.rs`'s job. What +//! is checked here is everything that does not: the flags chosen, the refusal a +//! chained host gets, and — by standing an ordinary child process in for `ssh` +//! — that waiting really does resolve when a listener appears and really does +//! give up when the child dies. + +use std::net::{IpAddr, Ipv4Addr, SocketAddr, TcpListener}; +use std::sync::Arc; +use std::time::Duration; + +use async_trait::async_trait; +use tinybox_core::{Capability, Error, ExecOutput, ExecRequest, ForwardGuard as _, Host, Result}; + +use super::super::{SshHost, SshTarget}; +use super::{LISTEN_TIMEOUT, SshTunnel, exit_diagnostic, tunnel_command, wait_until_listening}; + +/// A host that is not `local`, so an [`SshHost`] wrapping it is a chain. +#[derive(Debug)] +struct NotLocal; + +#[async_trait] +impl Host for NotLocal { + fn name(&self) -> &'static str { + "ssh" + } + + async fn run(&self, _request: &ExecRequest) -> Result { + Ok(ExecOutput::new(0, Vec::new(), Vec::new())) + } +} + +/// A destination in a reserved TLD, so no test can reach a real machine. +fn target() -> Result { + SshTarget::new("builder@example.invalid") +} + +/// An address nothing can ever accept on. +/// +/// Port 0 is not a connectable port — it means "let the OS choose" when +/// binding, and connecting to it fails immediately. That makes it the one +/// address these tests can rely on, where binding an ephemeral port and closing +/// it cannot: the port is free the moment it is released, so a sibling test +/// binding its own listener can land on exactly that number and the connect +/// unexpectedly succeeds. That is a race these tests lost on CI and won +/// locally, which is the worst way round. +const NEVER_ACCEPTS: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0); + +/// Start `argv` as a stand-in for the `ssh` that would carry a tunnel. +fn stand_in(argv: &[&str]) -> Result { + SshTunnel::spawn(&argv.iter().map(|a| (*a).to_owned()).collect::>()) +} + +#[test] +fn the_tunnel_carries_only_the_forward() -> Result<()> { + let argv = tunnel_command(&target()?, 54321, ([10, 0, 0, 5], 7788).into()); + + // No remote command: a login shell on the far side is one more thing that + // can fail, and this connection has no use for one. + assert!(argv.contains(&"-N".to_owned()), "{argv:?}"); + // Without this, a refused forward leaves `ssh` connected and idle, which + // looks exactly like success until the first connection attempt. + assert!( + argv.contains(&"ExitOnForwardFailure=yes".to_owned()), + "{argv:?}" + ); + // The local side is loopback-only: a forward reachable from the network + // would republish the far machine's port to anyone who can reach this one. + let spec = argv + .iter() + .position(|part| part == "-L") + .map(|at| &argv[at + 1]); + assert_eq!( + spec.map(String::as_str), + Some("127.0.0.1:54321:10.0.0.5:7788") + ); + // The destination is last, so nothing after it can be read as a flag. + assert_eq!( + argv.last().map(String::as_str), + Some("builder@example.invalid") + ); + Ok(()) +} + +#[test] +fn the_tunnel_inherits_the_targets_connection_settings() -> Result<()> { + // A forward that ignored `--ssh-port` or `BatchMode` would behave + // differently from every other command against the same target. + let argv = tunnel_command(&target()?.with_port(2222), 1, ([127, 0, 0, 1], 2).into()); + + assert!(argv.contains(&"BatchMode=yes".to_owned()), "{argv:?}"); + assert!(argv.contains(&"2222".to_owned()), "{argv:?}"); + Ok(()) +} + +#[tokio::test] +async fn waiting_resolves_as_soon_as_something_accepts() -> Result<()> { + // A listener already bound stands in for the far side being reachable. + let listener = TcpListener::bind(("127.0.0.1", 0)).map_err(|e| Error::io("bind", &e))?; + let local: SocketAddr = listener.local_addr().map_err(|e| Error::io("addr", &e))?; + let mut tunnel = stand_in(&["sleep", "30"])?; + + let outcome = wait_until_listening(&mut tunnel, local, LISTEN_TIMEOUT).await; + + tunnel.close(); + assert!(outcome.is_ok(), "{outcome:?}"); + Ok(()) +} + +#[tokio::test] +async fn a_tunnel_that_dies_is_reported_with_its_own_diagnostic() -> Result<()> { + // Waiting out the full timeout for a process that has already exited would + // turn a rejected key into a ten-second hang and then a message saying + // nothing about why. + let mut tunnel = stand_in(&["/bin/sh", "-c", "echo 'Permission denied' >&2; exit 255"])?; + + let outcome = wait_until_listening(&mut tunnel, NEVER_ACCEPTS, LISTEN_TIMEOUT).await; + + match outcome.err() { + Some(Error::Backend { message, .. }) => { + assert!(message.contains("Permission denied"), "{message:?}"); + } + other => assert_eq!(format!("{other:?}"), "a backend error"), + } + Ok(()) +} + +#[tokio::test] +async fn waiting_gives_up_rather_than_holding_a_tunnel_that_never_works() -> Result<()> { + // A tunnel whose `ssh` is alive but never binds — a forward the server + // silently dropped — has no event to wait for, so only the deadline ends + // it. Reported with the deadline in it, because "it did not work" without + // "and I waited this long" tells an operator nothing. + let mut tunnel = stand_in(&["sleep", "30"])?; + + let outcome = wait_until_listening(&mut tunnel, NEVER_ACCEPTS, Duration::from_millis(1)).await; + + tunnel.close(); + match outcome.err() { + Some(Error::Backend { message, .. }) => { + assert!(message.contains("did not start accepting"), "{message:?}"); + assert!(message.contains("1ms"), "{message:?}"); + } + other => assert_eq!(format!("{other:?}"), "a backend error"), + } + Ok(()) +} + +#[test] +fn a_silent_exit_still_says_something() -> Result<()> { + // An error with no message is the least useful thing this could report. + let mut tunnel = stand_in(&["/bin/sh", "-c", "exit 1"])?; + let _ = tunnel.child.wait(); + + assert_eq!( + exit_diagnostic(&mut tunnel), + "ssh exited before the forward was established" + ); + Ok(()) +} + +#[test] +fn closing_a_tunnel_twice_is_harmless() -> Result<()> { + // `Forward`'s `Drop` calls this, and a test may have called it already. + let mut tunnel = stand_in(&["sleep", "30"])?; + + tunnel.close(); + tunnel.close(); + Ok(()) +} + +#[test] +fn a_missing_program_is_reported_rather_than_silently_absent() { + let outcome = stand_in(&["tinybox-no-such-program-exists"]); + + assert!(matches!(outcome.err(), Some(Error::Io { .. }))); +} + +#[tokio::test] +async fn a_chained_host_refuses_rather_than_tunnelling_from_the_wrong_machine() -> Result<()> { + // Every other operation composes, because it is a command line the inner + // host runs. A tunnel is a process that has to keep running, so opening it + // here would put it on this machine and report an address leading nowhere. + let chained = SshHost::new(Arc::new(NotLocal), target()?); + + let outcome = chained.forward(([127, 0, 0, 1], 7788).into()).await; + + assert_eq!( + outcome.err(), + Some(Error::Unsupported { + sandbox: "ssh".to_owned(), + capability: Capability::PortForward, + }) + ); + Ok(()) +} + +#[tokio::test] +async fn an_unreachable_destination_settles_quickly_instead_of_hanging() -> Result<()> { + // `BatchMode=yes` is what makes this settle at all: without it `ssh` would + // prompt for a password nobody is there to answer, and the call would hang + // rather than fail. + // + // Which way it settles is deliberately not asserted. `ssh` binds the local + // port before it authenticates, so an unreachable destination can produce a + // listener for a moment before dying — see `open`'s documentation. Pinning + // one outcome here would be pinning a race, and the property that matters + // is that neither outcome takes the full `LISTEN_TIMEOUT`. + let host = SshHost::new(Arc::new(tinybox_host::LocalHost::new()), target()?); + + let started = std::time::Instant::now(); + let outcome = host.forward(([127, 0, 0, 1], 7788).into()).await; + let elapsed = started.elapsed(); + + assert!(elapsed < LISTEN_TIMEOUT, "took {elapsed:?}"); + if let Err(error) = outcome { + assert!( + matches!( + error, + Error::Backend { + operation: "open a port forward", + .. + } | Error::Io { .. } // No `ssh` binary on this host. + ), + "unexpected error: {error:?}" + ); + } + Ok(()) +} diff --git a/crates/tinybox-ssh/src/host/mod.rs b/crates/tinybox-ssh/src/host/mod.rs index efdfaf7..ea06173 100644 --- a/crates/tinybox-ssh/src/host/mod.rs +++ b/crates/tinybox-ssh/src/host/mod.rs @@ -3,9 +3,9 @@ use std::sync::Arc; use async_trait::async_trait; -use tinybox_core::{Error, ExecOutput, ExecRequest, Host, Result}; +use tinybox_core::{Error, ExecOutput, ExecRequest, Forward, Host, Result}; -mod quote; +mod forward; mod target; pub use target::SshTarget; @@ -70,7 +70,7 @@ impl SshHost { // `--` separates ssh's own options from the remote command, so a // command starting with a dash cannot be read as an ssh flag. argv.push("--".to_owned()); - argv.push(quote::remote_command( + argv.push(tinybox_core::shell::script( &request.argv, request.cwd.as_deref(), &request.env, @@ -113,7 +113,48 @@ impl Host for SshHost { } self.inner.run(&forwarded).await } + + /// Open a tunnel from this machine to `remote` on the far machine. + /// + /// This is the half of reach that command dispatch cannot cover. A sandbox + /// publishes a guest port to *its host*, and when that host is over there, + /// publishing is all it can do — the caller still has no route. `ssh -L` + /// is the route, so it belongs here rather than in any sandbox. + /// + /// # Only from a local inner host + /// + /// Every other operation on this type composes freely, because it builds a + /// command line and lets the inner host decide where it runs. A tunnel + /// cannot: it is a process that has to keep running, which + /// [`Host::run`] has no way to express. So a chained `SshHost` — reaching + /// one machine through another — refuses rather than opening a tunnel on + /// the wrong machine and reporting an address that leads nowhere. + /// `ProxyJump` in the user's SSH config is the supported way to do that, + /// and it needs no code here. + /// + /// # Errors + /// + /// Returns [`Error::Unsupported`] when the inner host is not the local + /// machine, [`Error::Io`] when `ssh` cannot be started, and + /// [`Error::Backend`] when the forward is refused or never starts + /// accepting. + async fn forward(&self, remote: std::net::SocketAddr) -> Result { + if self.inner.name() != LOCAL_HOST_NAME { + return Err(Error::Unsupported { + sandbox: NAME.to_owned(), + capability: tinybox_core::Capability::PortForward, + }); + } + forward::open(&self.target, remote).await + } } +/// The inner host a tunnel can be opened from. +/// +/// Matched by name rather than by type so that this crate keeps its +/// dependency-free relationship with `tinybox-host`; the name is the same +/// registry key [`HostRef`](tinybox_core::HostRef) uses. +const LOCAL_HOST_NAME: &str = "local"; + #[cfg(test)] mod test; diff --git a/docs/adr/0007-reach-includes-forwarding-and-detachment.md b/docs/adr/0007-reach-includes-forwarding-and-detachment.md new file mode 100644 index 0000000..233756a --- /dev/null +++ b/docs/adr/0007-reach-includes-forwarding-and-detachment.md @@ -0,0 +1,89 @@ +# 7. Reach includes forwarding, and detachment is one shell mechanism + +- **Status:** Accepted +- **Date:** 2026-08-22 + +## Context + +Both traits were shaped around one workload: run a command, wait, collect what +it produced. That is the right shape for a build, a test run, or an agent's +command, and it is the wrong shape for a *service*. + +Putting a server in a box needs two things tinybox could not express. + +**Nothing could outlive its own command.** `Sandbox::exec` returns an +`ExecOutput`, which means it waits. Starting `openhuman-core serve` through it +never returns, and there is no other way in. + +**A published port was not necessarily reachable.** `PortMapping` publishes a +guest port to *its host's* address space, which is exactly right — and when the +host is another machine, the caller who asked for it still has no route to it. +No amount of sandbox-side configuration changes that, because the gap is not in +the confinement, it is in the reach. + +The second one is the more interesting mistake, because it was invisible. Every +piece worked: `ssh` + `docker` composed as ADR 0002 promised, `--publish` was +applied, `inspect` reported the mapping. The port was simply on the wrong +machine, and nothing in the model said so. + +## Decision + +**Forwarding is a `Host` operation.** `Host::forward(SocketAddr) -> Forward` +answers "make that address reachable from here". `LocalHost` hands the address +back; `SshHost` holds an `ssh -N -L` child. The returned `Forward` is a guard: +the path exists for exactly as long as the value does. + +**Detachment is one mechanism in core, not one per backend.** +`Sandbox::{spawn, is_running, stop}` are declared alongside +`Capability::Detach`, and every implementation dispatches through +`tinybox_core::detach`, which builds a shell command that backgrounds the +command and records its pid in a file named after a tinybox-minted `ProcessId`. + +Both trait methods default to `Error::Unsupported`, so a backend opts in. + +## Consequences + +- **`ssh` + `docker` now reaches all the way.** A container on another machine + publishes to that machine, and the forward closes the remaining gap — with no + code naming that pairing, which is the same property ADR 0002 bought for + command dispatch, extended to connections. +- **The detach mechanism is deliberately *not* `docker exec --detach`.** That + flag exists and would have been the obvious choice for the Docker backend + alone. It hands back nothing a caller could name, so there would be no way to + ask whether the process is still running or to stop it — and `ssh` and the + local host have no equivalent flag at all. The shell is the one thing every + box that can host a server already has, so it is the one mechanism. +- **A backend declaring `Detach` promises more than "it ran".** It promises the + pid file survives to the next command and the process keeps running between + commands. `namespace` and `microvm` therefore decline: the first re-binds its + directory per command, the second returns only what the command printed. A + background process that cannot be found or stopped is worse than a refusal, + because it looks like it worked. +- **Shell quoting moved into core and became public** (`tinybox_core::shell`). + It was `tinybox-ssh`'s private module, written where the no-injection property + had to be re-established by hand. Detachment is the second such place, and a + second copy of a command-injection-critical function is a second chance to get + it wrong. +- **`SshHost::forward` refuses when its inner host is not local.** Every other + operation on that type composes freely, because it builds a command line and + lets the inner host decide where it runs. A tunnel cannot: it is a process + that has to keep running, which `Host::run` cannot express, so a chained host + would open it on the wrong machine and report an address leading nowhere. + `ProxyJump` in the user's SSH config does that case properly and needs no code + here. +- **The pid file is a real cost.** It lives in `/tmp` inside the box, so a box + whose `/tmp` is read-only or non-POSIX cannot detach, and a `ProcessId` + outlives the process it names until `stop` removes the file. `stop` therefore + removes it unconditionally — a stale file would make a later probe answer + about whatever process inherits that pid next. +- **`forward` blocks in the CLI.** There is no daemon to hand a guard to and no + honest way to record a tunnel this process is no longer holding open, so + `tinybox forward` running *is* the forward existing. On a local host, where + nothing is held open, it prints the address and returns rather than pretending. + +## Related + +- ADR 0002 — host and sandbox are orthogonal; this extends that split from + command dispatch to connections +- ADR 0004 — backends drive external tools through `Host`, which is why `ssh -L` + is a command line here too