diff --git a/Cargo.lock b/Cargo.lock index 040222b..7aae952 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2609,7 +2609,7 @@ dependencies = [ [[package]] name = "webterm" -version = "0.3.11" +version = "0.3.12" dependencies = [ "actix", "actix-files", diff --git a/Cargo.toml b/Cargo.toml index e831583..ba4f320 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ documentation = "https://docs.rs/webterm" readme = "README.md" categories = ["web-programming", "web-programming::websocket", "web-programming::http-server", "command-line-utilities"] keywords = ["terminal", "xterm", "websocket", "terminus", "console"] -version = "0.3.11" +version = "0.3.12" authors = ["fabian.freyer@physik.tu-berlin.de","jasonpwitty+socktop@proton.me"] edition = "2021" license = "BSD-3-Clause" diff --git a/kubernetes/03-deployment.yaml b/kubernetes/03-deployment.yaml index 0864843..c19bc3e 100644 --- a/kubernetes/03-deployment.yaml +++ b/kubernetes/03-deployment.yaml @@ -85,7 +85,7 @@ spec: containers: - name: webterm - image: gt.wittyoneoff.com/jason/socktop-webterm:0.3.11 + image: gt.wittyoneoff.com/jason/socktop-webterm:0.3.12 imagePullPolicy: Always command: ["/docker-entrypoint.sh"] @@ -165,13 +165,13 @@ spec: - name: socktop-home mountPath: /var/lib/socktop - # webterm-server runs as in-container root holding ONLY - # CAP_SETUID/CAP_SETGID (everything else dropped, no privilege - # escalation), so it can drop each websocket session to the - # unprivileged `demo` user via session-shell.sh. The kernel then - # refuses any signal a session aims at the server, the agent - # (running as `socktop`), or another session's UID — the kill - # feature's UI gating stops being the only line of defense. + # webterm-server runs as in-container root holding only the caps + # listed below (everything else dropped, no privilege escalation), + # so it can drop each websocket session to the unprivileged `demo` + # user via session-shell.sh. The kernel then refuses any signal a + # session aims at the server, the agent (running as `socktop`), or + # another session's UID — the kill feature's UI gating stops being + # the only line of defense. securityContext: allowPrivilegeEscalation: false capabilities: @@ -180,6 +180,14 @@ spec: add: - SETUID - SETGID + # The server must be able to signal the sessions it spawned, + # which run as `demo` — a different uid — so root needs + # CAP_KILL for that. Without it every idle-timeout teardown + # got EPERM, the session lived on, and (0.3.11) the actix + # worker blocked in wait() on it: half of all requests hung. + # Sessions still cannot signal anything: setpriv drops every + # cap (including this one) before the restricted shell runs. + - KILL # prepare_demo_home (entrypoint.sh) writes into and re-owns # /home/demo, which the image ships as demo-owned 700. With # ALL dropped, uid 0 has no implicit file privilege, so the diff --git a/src/lib.rs b/src/lib.rs index c88e43d..afc5705 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -218,6 +218,113 @@ impl Handler for Websocket { } } +/// Grace the session gets to exit on SIGHUP before the reaper sends SIGKILL. +const CHILD_EXIT_GRACE: Duration = Duration::from_secs(2); +/// Upper bound on how long the reaper polls after SIGKILL before it logs the +/// survivor and falls back to a blocking wait on its own thread. +const CHILD_REAP_DEADLINE: Duration = Duration::from_secs(10); +const CHILD_POLL_INTERVAL: Duration = Duration::from_millis(100); + +/// Send `signal` to the child's whole process group. +/// +/// portable-pty spawns the child with `setsid()`, so its pid is also its pgid +/// and `kill(-pid)` reaches every process in the session — the shell *and* +/// whatever it is running. Signalling only the shell is not enough: a +/// non-interactive bash waiting on a foreground command defers signal handling +/// until that command exits, so the shell dies on SIGKILL and its child is +/// orphaned still holding the pty. +fn signal_process_group(pid: u32, signal: libc::c_int) -> std::io::Result<()> { + let pgid = -(pid as libc::pid_t); + // SAFETY: plain libc call with a pgid we own; no memory is touched. + if unsafe { libc::kill(pgid, signal) } == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +/// Terminate and reap a session's child process without blocking the caller. +/// +/// SIGHUP goes to the process group immediately; a detached thread then polls +/// for exit, escalates to SIGKILL after [`CHILD_EXIT_GRACE`], and only after +/// [`CHILD_REAP_DEADLINE`] gives up polling and parks in a blocking `wait()` — +/// on its own thread, so a session the server is not permitted to signal can +/// never wedge an actix worker again. Signal failures are logged, not +/// swallowed: `EPERM` here means the container lacks `CAP_KILL` while sessions +/// run as a different uid. +/// +/// Returns the reaper's `JoinHandle`; callers normally drop it. +pub fn reap_child(mut child: Box) -> std::thread::JoinHandle<()> { + let pid = child.process_id(); + + if let Some(pid) = pid { + match signal_process_group(pid, libc::SIGHUP) { + Ok(()) => log::debug!("Sent SIGHUP to session process group {}", pid), + Err(e) => log::error!( + "Cannot SIGHUP session process group {}: {} \ + (EPERM means webterm-server lacks CAP_KILL for cross-uid sessions)", + pid, + e + ), + } + } + + std::thread::Builder::new() + .name(format!("reap-{}", pid.unwrap_or(0))) + .spawn(move || { + let started = Instant::now(); + let mut killed = false; + + loop { + match child.try_wait() { + Ok(Some(status)) => { + log::info!( + "Session {} exited with {:?} after {:?}", + pid.unwrap_or(0), + status, + started.elapsed() + ); + return; + } + Ok(None) => {} + Err(e) => { + log::error!("try_wait on session {} failed: {}", pid.unwrap_or(0), e); + return; + } + } + + let elapsed = started.elapsed(); + if !killed && elapsed >= CHILD_EXIT_GRACE { + killed = true; + if let Some(pid) = pid { + match signal_process_group(pid, libc::SIGKILL) { + Ok(()) => log::warn!( + "Session {} ignored SIGHUP for {:?}; sent SIGKILL", + pid, + elapsed + ), + Err(e) => { + log::error!("Cannot SIGKILL session process group {}: {}", pid, e) + } + } + } + } + if elapsed >= CHILD_REAP_DEADLINE { + log::error!( + "Session {} still alive {:?} after SIGKILL; reaper parking in wait()", + pid.unwrap_or(0), + elapsed + ); + let _ = child.wait(); + return; + } + + std::thread::sleep(CHILD_POLL_INTERVAL); + } + }) + .expect("failed to spawn session reaper thread") +} + /// Represents a PTY backend with attached child pub struct Terminal { pty_master: Option>, @@ -367,9 +474,17 @@ impl Actor for Terminal { fn stopping(&mut self, _ctx: &mut Self::Context) -> Running { log::info!("Stopping Terminal"); - if let Some(mut child) = self.child.take() { - let _ = child.kill(); - let _ = child.wait(); + // Release our side of the pty first so the reader thread sees EOF once + // the session is gone, then hand the child to the reaper. This used to + // be `child.kill(); child.wait();` inline — a blocking waitpid on the + // actix worker thread. When the kill was refused (the session runs as + // another uid and the server had no CAP_KILL) the child lived on and + // the worker hung forever, taking half the server's connections with + // it. Nothing here may block. + self.pty_writer = None; + self.pty_master = None; + if let Some(child) = self.child.take() { + reap_child(child); } // Notify the websocket that the child died. diff --git a/tests/reaper_tests.rs b/tests/reaper_tests.rs new file mode 100644 index 0000000..0d38c0c --- /dev/null +++ b/tests/reaper_tests.rs @@ -0,0 +1,144 @@ +// Copyright (c) 2024 Jason Witty . +// All rights reserved. +// +// Regression tests for session teardown (`webterm::reap_child`). +// +// Background: `Terminal::stopping` used to call `child.kill()` then a blocking +// `child.wait()` on the actix worker thread. When the signal was refused +// (sessions run as another uid, server without CAP_KILL) or only reached the +// shell (a non-interactive bash defers signals while a foreground command +// runs), the child survived and the worker hung forever — every other request +// to the server then timed out. These tests pin the fixed behaviour: the whole +// process group is signalled, SIGHUP escalates to SIGKILL, and the caller is +// never blocked. + +use std::io::Read; +use std::time::{Duration, Instant}; + +use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + +/// True while `pid` exists and is not a zombie (kill(pid, 0) also succeeds on +/// zombies, so read /proc directly). +fn alive(pid: i32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/status")) { + Ok(s) => !s + .lines() + .any(|l| l.starts_with("State:") && l.contains("Z (zombie)")), + Err(_) => false, + } +} + +fn wait_gone(pid: i32, timeout: Duration) -> bool { + let start = Instant::now(); + while start.elapsed() < timeout { + if !alive(pid) { + return true; + } + std::thread::sleep(Duration::from_millis(50)); + } + !alive(pid) +} + +/// Spawn `script` under `sh -c` in a fresh pty. The script must print the pid +/// of its long-running grandchild as its first line; that pid is returned along +/// with the pty child handle. +fn spawn_in_pty(script: &str) -> (Box, i32) { + let pty = native_pty_system(); + let pair = pty + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty"); + + let mut cmd = CommandBuilder::new("sh"); + cmd.arg("-c"); + cmd.arg(script); + let child = pair.slave.spawn_command(cmd).expect("spawn"); + drop(pair.slave); + + let mut reader = pair.master.try_clone_reader().expect("reader"); + let mut buf = Vec::new(); + let mut byte = [0u8; 1]; + let deadline = Instant::now() + Duration::from_secs(5); + while Instant::now() < deadline { + match reader.read(&mut byte) { + Ok(1) if byte[0] == b'\n' => break, + Ok(1) => buf.push(byte[0]), + _ => break, + } + } + let grandchild: i32 = String::from_utf8_lossy(&buf) + .trim() + .parse() + .expect("grandchild pid line"); + assert!( + alive(grandchild), + "grandchild {grandchild} should be running" + ); + + // Keep the master alive for the duration of the test (mirrors the + // server's reader thread holding a cloned fd), so the kernel does not + // hang up the pty for us and the reaper alone has to do the work. + std::mem::forget(pair.master); + std::mem::forget(reader); + + (child, grandchild) +} + +#[test] +fn reap_child_returns_immediately_and_kills_whole_process_group() { + // sh waits on a foreground sleep: signalling sh alone would be deferred. + let (child, sleeper) = spawn_in_pty("sleep 30 & echo $!; wait"); + let shell = child.process_id().expect("pid") as i32; + + let t0 = Instant::now(); + let reaper = webterm::reap_child(child); + assert!( + t0.elapsed() < Duration::from_millis(500), + "reap_child must not block the caller (took {:?})", + t0.elapsed() + ); + + reaper.join().expect("reaper thread"); + assert!( + wait_gone(shell, Duration::from_secs(2)), + "shell {shell} survived" + ); + assert!( + wait_gone(sleeper, Duration::from_secs(2)), + "grandchild {sleeper} was orphaned instead of killed with its group" + ); +} + +#[test] +fn reap_child_escalates_to_sigkill_when_sighup_is_ignored() { + // `trap '' HUP` makes sh ignore SIGHUP and the ignored disposition is + // inherited across exec, so sleep ignores it too — exactly the shape of a + // TUI that swallows HUP. + let (child, sleeper) = spawn_in_pty("trap '' HUP; sleep 30 & echo $!; wait"); + let shell = child.process_id().expect("pid") as i32; + + let t0 = Instant::now(); + webterm::reap_child(child).join().expect("reaper thread"); + let took = t0.elapsed(); + + assert!( + took >= Duration::from_secs(2), + "should have waited out the SIGHUP grace period (took {took:?})" + ); + assert!( + took < Duration::from_secs(8), + "SIGKILL escalation should finish well inside the deadline (took {took:?})" + ); + assert!( + wait_gone(shell, Duration::from_secs(2)), + "shell {shell} survived SIGKILL" + ); + assert!( + wait_gone(sleeper, Duration::from_secs(2)), + "grandchild {sleeper} survived SIGKILL" + ); +}