From 15451440354f4c4cfa7a073a315298347b2c6976 Mon Sep 17 00:00:00 2001 From: sebi Date: Thu, 2 Jul 2026 12:05:34 -0500 Subject: [PATCH] fix(config): empty allowed-hosts guard, fail-fast port, 0600 secrets, no request-path panics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - MCP_SSH_ALLOWED_HOSTS set-but-empty (or allowed_hosts = [] / blank entries in the file) fed rmcp an empty allowlist, which it reads as allow-ALL hosts — the DNS-rebinding guard silently OFF, worse than unset. Empty now behaves exactly like unset: loopback default + warn. - Invalid MCP_SSH_PORT (8080x, 70000) was silently ignored, leaving the server on the default port while the operator believed it moved. Now fails fast like an invalid MCP_SSH_BIND. - set-auth created the config file umask-default (0644) with the plaintext password and only chmodded after — and a chmod failure was silently discarded, leaving it world-readable forever. Now created 0600 from the first byte; the tighten-existing chmod propagates errors. - config::db_path() swallowed read/parse errors (unwrap_or_default), silently pointing admin commands at the DEFAULT database while the server used the configured one. Errors now propagate. - Db::call had expect() on the request path: one panicking closure poisoned the mutex and turned every later call (all bearer validation, all job writes) into a panic until restart. Poisoned locks are recovered; a closure panic returns an error to its caller alone. - Admin job kill cast the stored pgid i64 -> u32 with as, wrapping a corrupt/negative row into a real (wrong) process group to signal. Now rejected with a message. Co-Authored-By: Claude Fable 5 --- src/admin.rs | 13 +++- src/config.rs | 170 +++++++++++++++++++++++++++++++++++++++++--------- src/db.rs | 35 ++++++++++- src/main.rs | 34 ++++++++-- 4 files changed, 214 insertions(+), 38 deletions(-) diff --git a/src/admin.rs b/src/admin.rs index f3d5fea..6ffea15 100644 --- a/src/admin.rs +++ b/src/admin.rs @@ -10,7 +10,7 @@ use crate::db::{Db, now_unix}; /// Open the database the running server uses. Idempotent — applies the same schema /// and pragmas, safe to run alongside the daemon (SQLite WAL allows it). fn open_db() -> anyhow::Result { - Db::open(&crate::config::db_path()) + Db::open(&crate::config::db_path()?) } // ---- jobs ---- @@ -125,7 +125,14 @@ async fn kill_job(db: &Db, id: &str) -> anyhow::Result { )); }; - let killed = crate::jobs::kill_group(pgid as u32).await; + // A pgid outside u32 is a corrupt row; a raw `as` cast would wrap it into a + // real (wrong) process group and signal that instead. + let Ok(pgid) = u32::try_from(pgid) else { + return Ok(format!( + "job {id} has a corrupt pgid ({pgid}) — refusing to signal" + )); + }; + let killed = crate::jobs::kill_group(pgid).await; // Record the kill only if the row is still `running`: when the server owns the // job, its waiter may already have written the real exit as the group died. let lookup = id.to_string(); @@ -165,7 +172,7 @@ pub async fn sessions() -> anyhow::Result<()> { print!( "{}", render_sessions( - &crate::config::db_path().display().to_string(), + &crate::config::db_path()?.display().to_string(), &access, &refresh, now diff --git a/src/config.rs b/src/config.rs index e1d7ae5..999ba96 100644 --- a/src/config.rs +++ b/src/config.rs @@ -112,23 +112,28 @@ impl Config { "/var/lib/mcp-ssh/mcp-ssh.db", )); - let allowed_hosts = match env.get("MCP_SSH_ALLOWED_HOSTS") { - Some(v) => split_hosts(&v), - None => match file.allowed_hosts { - Some(hosts) => hosts, - // Only the real fallback — neither env nor file set hosts — risks - // DNS rebinding, so warn here rather than for any explicit config. - None => { - if !bind.ip().is_loopback() { - warn!( - "MCP_SSH_ALLOWED_HOSTS unset and bind is non-loopback ({}) — \ - DNS-rebinding attacks possible. Set MCP_SSH_ALLOWED_HOSTS explicitly.", - bind.ip() - ); - } - vec!["localhost".into(), "127.0.0.1".into()] + let explicit_hosts = match env.get("MCP_SSH_ALLOWED_HOSTS") { + Some(v) => Some(split_hosts(&v)), + None => file + .allowed_hosts + .map(|hosts| normalize_hosts(hosts.into_iter())), + }; + let allowed_hosts = match explicit_hosts { + Some(hosts) if !hosts.is_empty() => hosts, + // Unset, OR set but empty after trimming (`MCP_SSH_ALLOWED_HOSTS=`, + // `allowed_hosts = []`). Empty is treated exactly like unset because + // rmcp reads an empty allowlist as allow-ALL hosts — the guard would + // be silently OFF, worse than the default. + _ => { + if !bind.ip().is_loopback() { + warn!( + "MCP_SSH_ALLOWED_HOSTS unset or empty and bind is non-loopback ({}) — \ + DNS-rebinding attacks possible. Set MCP_SSH_ALLOWED_HOSTS explicitly.", + bind.ip() + ); } - }, + vec!["localhost".into(), "127.0.0.1".into()] + } }; let public_url = opt(env, "MCP_SSH_PUBLIC_URL", file.public_url); @@ -149,19 +154,21 @@ impl Config { /// Resolve just the SQLite path: env `MCP_SSH_DB`, else the file's `db_path`, else /// the default. The admin subcommands (`jobs`/`job kill`/`sessions`) touch only the /// database, so they resolve it this way instead of `Config::load`, which requires -/// auth credentials they don't need. -pub fn db_path() -> PathBuf { +/// auth credentials they don't need. A config file that exists but can't be read +/// or parsed is an error, not a silent fall-through to the default path — that +/// made the admin CLI inspect (or kill in!) a different database than the server. +pub fn db_path() -> Result { db_path_in(&ProcessEnv) } -fn db_path_in(env: &dyn EnvSource) -> PathBuf { - let file = load_file(&config_path(env)).unwrap_or_default(); - PathBuf::from(pick( +fn db_path_in(env: &dyn EnvSource) -> Result { + let file = load_file(&config_path(env))?; + Ok(PathBuf::from(pick( env, "MCP_SSH_DB", file.db_path, "/var/lib/mcp-ssh/mcp-ssh.db", - )) + ))) } /// Write `user`/`pass` into the config file, preserving other fields. Chmod 600. @@ -179,11 +186,30 @@ fn set_auth_in(env: &dyn EnvSource, user: &str, pass: &str) -> Result std::io::Result<()> { + use std::io::Write; + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create(true).truncate(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + let mut f = opts.open(path)?; + f.write_all(contents.as_bytes()) +} + fn load_file(path: &std::path::Path) -> Result { match std::fs::read_to_string(path) { Ok(s) => toml::from_str(&s).map_err(|e| ConfigError::Parse(e.to_string())), @@ -193,15 +219,23 @@ fn load_file(path: &std::path::Path) -> Result { } #[cfg(unix)] -fn chmod_600(path: &std::path::Path) { +fn chmod_600(path: &std::path::Path) -> std::io::Result<()> { use std::os::unix::fs::PermissionsExt; - let _ = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) } #[cfg(not(unix))] -fn chmod_600(_path: &std::path::Path) {} +fn chmod_600(_path: &std::path::Path) -> std::io::Result<()> { + Ok(()) +} fn split_hosts(v: &str) -> Vec { - v.split(',') + normalize_hosts(v.split(',').map(str::to_string)) +} + +/// Trim and drop empty entries so `""` or `" , "` can't smuggle an empty list +/// (or empty strings) into the allowlist. +fn normalize_hosts(hosts: impl Iterator) -> Vec { + hosts .map(|s| s.trim().to_string()) .filter(|s| !s.is_empty()) .collect() @@ -326,6 +360,86 @@ mod tests { } } + #[test] + fn empty_allowed_hosts_falls_back_to_loopback_default() { + // rmcp treats an empty allowlist as allow-ALL hosts, so set-but-empty + // (`MCP_SSH_ALLOWED_HOSTS=`, whitespace/commas, or `allowed_hosts = []` + // in the file) must behave exactly like unset — loopback default, never + // an empty vec. + let dir = tempdir().unwrap(); + let cfg_path = dir.path().join("config.toml"); + for empty in ["", " , "] { + let env = MapEnv::new(&[ + ("MCP_SSH_CONFIG", cfg_path.to_str().unwrap()), + ("MCP_SSH_ALLOWED_HOSTS", empty), + ("MCP_SSH_USER", "test"), + ("MCP_SSH_PASS", "test"), + ]); + let cfg = Config::from_env(&env).expect("load"); + assert_eq!( + cfg.allowed_hosts, + vec!["localhost", "127.0.0.1"], + "env {empty:?} must not disable the host guard" + ); + } + + // Same for an explicit empty list in the file. + std::fs::write(&cfg_path, "allowed_hosts = []\n").unwrap(); + let env = MapEnv::new(&[ + ("MCP_SSH_CONFIG", cfg_path.to_str().unwrap()), + ("MCP_SSH_USER", "test"), + ("MCP_SSH_PASS", "test"), + ]); + let cfg = Config::from_env(&env).expect("load"); + assert_eq!(cfg.allowed_hosts, vec!["localhost", "127.0.0.1"]); + } + + #[test] + fn db_path_propagates_a_broken_config_file() { + // A config file that exists but doesn't parse must be an error — the old + // unwrap_or_default() sent admin commands to the DEFAULT db path while + // the server (whose Config::load errors loudly) used the configured one. + let dir = tempdir().unwrap(); + let cfg_path = dir.path().join("config.toml"); + std::fs::write(&cfg_path, "this is { not toml").unwrap(); + let env = MapEnv::new(&[("MCP_SSH_CONFIG", cfg_path.to_str().unwrap())]); + assert!( + matches!(db_path_in(&env), Err(ConfigError::Parse(_))), + "broken config must not silently resolve to the default db path" + ); + + // A missing file is still fine (defaults apply). + let env = MapEnv::new(&[( + "MCP_SSH_CONFIG", + dir.path().join("absent.toml").to_str().unwrap(), + )]); + assert_eq!( + db_path_in(&env).unwrap(), + PathBuf::from("/var/lib/mcp-ssh/mcp-ssh.db") + ); + } + + #[cfg(unix)] + #[test] + fn set_auth_tightens_a_preexisting_loose_config_file() { + use std::os::unix::fs::PermissionsExt; + let dir = tempdir().unwrap(); + let cfg_path = dir.path().join("config.toml"); + std::fs::write(&cfg_path, "").unwrap(); + std::fs::set_permissions(&cfg_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let env = MapEnv::new(&[("MCP_SSH_CONFIG", cfg_path.to_str().unwrap())]); + set_auth_in(&env, "bob", "s3cr3t").expect("set_auth"); + + use std::os::unix::fs::MetadataExt; + let mode = std::fs::metadata(&cfg_path).unwrap().mode(); + assert_eq!( + mode & 0o777, + 0o600, + "a pre-existing world-readable file must be tightened" + ); + } + #[test] fn non_loopback_bind_without_allowed_hosts_env_loads() { let dir = tempdir().unwrap(); diff --git a/src/db.rs b/src/db.rs index 1783530..4ba7f66 100644 --- a/src/db.rs +++ b/src/db.rs @@ -70,6 +70,12 @@ impl Db { /// Run `f` against the connection on the blocking pool. The guard lives only /// inside the closure (a separate thread), so it never crosses an `.await`. + /// + /// Never panics: this sits on the request path (every bearer validation and + /// job write), where a panic would poison the mutex and turn ALL later calls + /// into panics until restart. A poisoned lock is recovered (the connection + /// itself is fine — only the closure panicked mid-hold), and a panicking + /// closure comes back as an error to its caller alone. pub async fn call(&self, f: F) -> rusqlite::Result where F: FnOnce(&Connection) -> rusqlite::Result + Send + 'static, @@ -77,11 +83,18 @@ impl Db { { let conn = self.conn.clone(); tokio::task::spawn_blocking(move || { - let guard = conn.lock().expect("db connection mutex poisoned"); + let guard = conn + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); f(&guard) }) .await - .expect("db blocking task panicked") + .unwrap_or_else(|join_error| { + Err(rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_ERROR), + Some(format!("db task panicked: {join_error}")), + )) + }) } } @@ -141,6 +154,24 @@ mod tests { assert_eq!(exp, 123); } + #[tokio::test] + async fn panicking_closure_is_an_error_and_the_connection_survives() { + // A panic inside a db closure must come back as Err to its caller only — + // not panic the request path, and not poison the connection for every + // later call (which previously turned one bug into a full outage). + let db = Db::memory(); + let r = db + .call(|_conn| -> rusqlite::Result<()> { panic!("boom") }) + .await; + assert!(r.is_err(), "panicking closure must surface as an error"); + + let one: i64 = db + .call(|conn| conn.query_row("SELECT 1", [], |row| row.get(0))) + .await + .expect("connection must remain usable after a closure panic"); + assert_eq!(one, 1); + } + #[test] fn ensure_column_adds_missing_and_is_idempotent() { let conn = Connection::open_in_memory().unwrap(); diff --git a/src/main.rs b/src/main.rs index 04f1f08..4661d74 100644 --- a/src/main.rs +++ b/src/main.rs @@ -74,11 +74,10 @@ fn set_auth(user: String) -> anyhow::Result<()> { async fn serve(port: Option) -> anyhow::Result<()> { let mut cfg = config::Config::load()?; // `--port` (or MCP_SSH_PORT) overrides just the port of the bind address. - if let Some(p) = port.or_else(|| { - std::env::var("MCP_SSH_PORT") - .ok() - .and_then(|v| v.parse().ok()) - }) { + if let Some(p) = match port { + Some(p) => Some(p), + None => env_port(std::env::var("MCP_SSH_PORT").ok())?, + } { cfg.bind.set_port(p); } // The single SQLite database (OAuth tokens + job metadata) under the systemd @@ -114,6 +113,20 @@ async fn serve(port: Option) -> anyhow::Result<()> { Ok(()) } +/// Parse the `MCP_SSH_PORT` value. Unset/empty → no override; anything else must +/// be a valid port. Silently ignoring `8080x` or `70000` (the old `.ok()` chain) +/// left the server listening on the config/default port while the operator +/// believed they'd moved it — fail fast instead, like an invalid `MCP_SSH_BIND`. +fn env_port(raw: Option) -> anyhow::Result> { + match raw.as_deref() { + None | Some("") => Ok(None), + Some(v) => v + .parse::() + .map(Some) + .map_err(|e| anyhow::anyhow!("invalid MCP_SSH_PORT {v:?}: {e}")), + } +} + /// Resolves on Ctrl-C or (on Unix) SIGTERM, letting axum drain in-flight /// requests before exit. systemd sends SIGTERM on `stop`/`restart`. async fn shutdown_signal() { @@ -159,6 +172,17 @@ mod tests { use super::*; + #[test] + fn env_port_rejects_garbage_and_passes_valid_values() { + assert_eq!(env_port(None).unwrap(), None, "unset → no override"); + assert_eq!(env_port(Some("".into())).unwrap(), None, "empty → unset"); + assert_eq!(env_port(Some("8080".into())).unwrap(), Some(8080)); + // A typo'd or out-of-range port must fail fast, not silently bind the + // default port. + assert!(env_port(Some("8080x".into())).is_err()); + assert!(env_port(Some("70000".into())).is_err()); + } + // SIGTERM (what systemd sends on stop) must resolve `shutdown_signal` so // axum begins draining instead of the process being killed outright. #[tokio::test]