From 06037a94f9f90554c3338b325ee6ff48f610b9eb Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Mon, 3 Aug 2026 19:34:40 +0900 Subject: [PATCH] chore: resolve host key verification follow-ups Serialize HOME-sensitive tests with a shared helper lock, remove the known_hosts fast-path serialization for definite matches, and keep fresh known_hosts files from starting with a blank line. Bring the shared ServerCheckMethod mirror back in sync with the client enum and make socket-address slice hostnames use one recordable address instead of a comma-separated host list. Validation: CARGO_TARGET_DIR=/home/inureyes/Development/backend.ai/bssh/target cargo test --lib ssh::tokio_client::host_verification; focused HOME tilde test passed 10 consecutive runs; CARGO_TARGET_DIR=/home/inureyes/Development/backend.ai/bssh/target cargo check --lib --tests; CARGO_TARGET_DIR=/home/inureyes/Development/backend.ai/bssh/target cargo clippy --lib --tests -- -D warnings. Closes #243 --- src/commands/interactive/utils.rs | 1 + src/shared/auth_types.rs | 82 ++++++++++++++ src/ssh/tokio_client/host_verification.rs | 101 ++++++++++++++++++ .../to_socket_addrs_with_hostname.rs | 28 ++++- src/test_helpers/env_guard.rs | 50 ++++++++- 5 files changed, 256 insertions(+), 6 deletions(-) diff --git a/src/commands/interactive/utils.rs b/src/commands/interactive/utils.rs index 0772613e..1609413d 100644 --- a/src/commands/interactive/utils.rs +++ b/src/commands/interactive/utils.rs @@ -74,6 +74,7 @@ mod tests { #[test] fn test_expand_path_with_tilde() { + let _home_lock = crate::test_helpers::EnvGuard::lock_home(); let cmd = InteractiveCommand { single_node: false, multiplex: true, diff --git a/src/shared/auth_types.rs b/src/shared/auth_types.rs index 58edc3ad..8584f33e 100644 --- a/src/shared/auth_types.rs +++ b/src/shared/auth_types.rs @@ -288,6 +288,15 @@ pub enum ServerCheckMethod { /// Use a specific known_hosts file path. KnownHostsFile(String), + + /// Trust On First Use against a specific known_hosts file path. + /// + /// Matching keys are accepted, unknown hosts are recorded and accepted, + /// and changed keys are rejected. + AcceptNewKnownHostsFile(String), + + /// Trust On First Use for the lifetime of this process only. + AcceptNewInMemory, } impl ServerCheckMethod { @@ -317,6 +326,53 @@ impl ServerCheckMethod { pub fn with_known_hosts_file(path: impl Into) -> Self { Self::KnownHostsFile(path.into()) } + + /// Create a ServerCheckMethod using accept-new semantics with a known_hosts file. + /// + /// # Arguments + /// + /// * `path` - Path to the known_hosts file + pub fn with_accept_new_known_hosts_file(path: impl Into) -> Self { + Self::AcceptNewKnownHostsFile(path.into()) + } +} + +impl From for ServerCheckMethod { + fn from(method: crate::ssh::tokio_client::ServerCheckMethod) -> Self { + match method { + crate::ssh::tokio_client::ServerCheckMethod::NoCheck => Self::NoCheck, + crate::ssh::tokio_client::ServerCheckMethod::PublicKey(key) => Self::PublicKey(key), + crate::ssh::tokio_client::ServerCheckMethod::PublicKeyFile(path) => { + Self::PublicKeyFile(path) + } + crate::ssh::tokio_client::ServerCheckMethod::DefaultKnownHostsFile => { + Self::DefaultKnownHostsFile + } + crate::ssh::tokio_client::ServerCheckMethod::KnownHostsFile(path) => { + Self::KnownHostsFile(path) + } + crate::ssh::tokio_client::ServerCheckMethod::AcceptNewKnownHostsFile(path) => { + Self::AcceptNewKnownHostsFile(path) + } + crate::ssh::tokio_client::ServerCheckMethod::AcceptNewInMemory => { + Self::AcceptNewInMemory + } + } + } +} + +impl From for crate::ssh::tokio_client::ServerCheckMethod { + fn from(method: ServerCheckMethod) -> Self { + match method { + ServerCheckMethod::NoCheck => Self::NoCheck, + ServerCheckMethod::PublicKey(key) => Self::PublicKey(key), + ServerCheckMethod::PublicKeyFile(path) => Self::PublicKeyFile(path), + ServerCheckMethod::DefaultKnownHostsFile => Self::DefaultKnownHostsFile, + ServerCheckMethod::KnownHostsFile(path) => Self::KnownHostsFile(path), + ServerCheckMethod::AcceptNewKnownHostsFile(path) => Self::AcceptNewKnownHostsFile(path), + ServerCheckMethod::AcceptNewInMemory => Self::AcceptNewInMemory, + } + } } #[cfg(test)] @@ -370,5 +426,31 @@ mod tests { let file = ServerCheckMethod::with_known_hosts_file("/path/to/known_hosts"); assert!(matches!(file, ServerCheckMethod::KnownHostsFile(_))); + + let accept_new = + ServerCheckMethod::with_accept_new_known_hosts_file("/path/to/known_hosts"); + assert!(matches!( + accept_new, + ServerCheckMethod::AcceptNewKnownHostsFile(_) + )); + } + + #[test] + fn test_server_check_method_converts_to_client_type() { + let shared = ServerCheckMethod::AcceptNewKnownHostsFile("/tmp/known_hosts".to_string()); + let client: crate::ssh::tokio_client::ServerCheckMethod = shared.clone().into(); + assert_eq!( + client, + crate::ssh::tokio_client::ServerCheckMethod::AcceptNewKnownHostsFile( + "/tmp/known_hosts".to_string() + ) + ); + + let round_trip: ServerCheckMethod = client.into(); + assert_eq!(round_trip, shared); + + let in_memory: ServerCheckMethod = + crate::ssh::tokio_client::ServerCheckMethod::AcceptNewInMemory.into(); + assert_eq!(in_memory, ServerCheckMethod::AcceptNewInMemory); } } diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index 5ba16881..0df258bf 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -37,6 +37,8 @@ //! `node1` and is otherwise invisible to [`lookup_known_host`]. use russh::keys::{Algorithm, HashAlg, PublicKey}; +#[cfg(test)] +use std::sync::Mutex as StdMutex; use std::{ collections::HashMap, fs::{File, OpenOptions}, @@ -73,6 +75,10 @@ static KNOWN_HOSTS_LOCK: Mutex<()> = Mutex::const_new(()); static PROCESS_HOST_PINS: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); +#[cfg(test)] +static FILE_LOCK_ACQUISITIONS: LazyLock>> = + LazyLock::new(|| StdMutex::new(Vec::new())); + #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ProcessPinKey { hostname: String, @@ -277,6 +283,11 @@ fn acquire_known_hosts_file_lock( ); super::Error::ServerCheckFailed })?; + #[cfg(test)] + FILE_LOCK_ACQUISITIONS + .lock() + .unwrap() + .push(known_hosts_path.to_string()); Ok(KnownHostsFileLock { file }) } @@ -673,6 +684,34 @@ pub(super) async fn verify_accept_new( // rejected would only make every other connection wait on it. ensure_recordable_hostname(hostname)?; + probe_known_hosts_path(known_hosts_path)?; + check_marker_lines(hostname, port, server_public_key, known_hosts_path)?; + match lookup_known_host(hostname, port, server_public_key, known_hosts_path) { + Ok(KnownHostLookup::Match) => { + verify_process_pin(hostname, port, Some(known_hosts_path), server_public_key).await?; + return Ok(true); + } + Ok(KnownHostLookup::Conflict { line }) => { + return Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + russh::keys::Error::KeyChanged { line }, + )); + } + Ok(KnownHostLookup::Unknown) => {} + Err(e) => { + return Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + e, + )); + } + } + let _guard = KNOWN_HOSTS_LOCK.lock().await; let _file_lock = acquire_known_hosts_file_lock(known_hosts_path)?; @@ -865,6 +904,10 @@ fn record_host_key( return; } + if !file_preexisted { + remove_leading_blank_line(path); + } + #[cfg(unix)] restrict_created_permissions(path, dir_preexisted, file_preexisted); #[cfg(not(unix))] @@ -877,6 +920,26 @@ fn record_host_key( ); } +fn remove_leading_blank_line(path: &Path) { + match std::fs::read_to_string(path) { + Ok(contents) if contents.starts_with('\n') => { + if let Err(e) = std::fs::write(path, contents.trim_start_matches('\n')) { + tracing::warn!( + "Failed to remove leading blank line from {}: {e}", + path.display() + ); + } + } + Ok(_) => {} + Err(e) => { + tracing::warn!( + "Failed to inspect {} for leading blank line cleanup: {e}", + path.display() + ); + } + } +} + /// The host as it appears in the known_hosts entry: `[host]:port` for /// non-standard ports, the bare hostname otherwise. Mirrors the convention /// `learn_known_hosts_path` writes and `check_known_hosts_path` matches. @@ -1105,6 +1168,21 @@ mod tests { assert!(lines[0].contains("ssh-ed25519")); } + #[tokio::test] + async fn test_accept_new_fresh_file_does_not_start_with_blank_line() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + let result = verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + + let contents = std::fs::read_to_string(&path).unwrap(); + assert!( + !contents.starts_with('\n'), + "fresh known_hosts must not start with a blank line, got {contents:?}" + ); + } + #[tokio::test] async fn test_accept_new_second_connection_does_not_duplicate() { let (_dir, path, path_str) = temp_known_hosts(); @@ -1239,6 +1317,29 @@ mod tests { assert_eq!(entry_lines(&path), recorded); } + #[tokio::test] + async fn test_accept_new_known_match_avoids_file_lock() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + std::fs::write( + &path, + format!( + "node1.example.com {}\n", + key.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + FILE_LOCK_ACQUISITIONS.lock().unwrap().clear(); + let result = verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + let acquisitions = FILE_LOCK_ACQUISITIONS.lock().unwrap(); + assert!( + !acquisitions.iter().any(|path| path == &path_str), + "a definite known-host match must return before acquiring the file lock" + ); + } + #[tokio::test] async fn test_accept_new_accepts_per_host_key_beside_shared_cluster_entry() { let (_dir, path, path_str) = temp_known_hosts(); diff --git a/src/ssh/tokio_client/to_socket_addrs_with_hostname.rs b/src/ssh/tokio_client/to_socket_addrs_with_hostname.rs index f1c97442..dd195eec 100644 --- a/src/ssh/tokio_client/to_socket_addrs_with_hostname.rs +++ b/src/ssh/tokio_client/to_socket_addrs_with_hostname.rs @@ -102,9 +102,31 @@ impl ToSocketAddrsWithHostname for &[SocketAddr] { } fn hostname(&self) -> String { - self.iter() + self.first() .map(|addr| addr.ip().to_string()) - .collect::>() - .join(",") + .unwrap_or_default() + } +} + +#[cfg(test)] +mod tests { + use super::ToSocketAddrsWithHostname; + use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; + + #[test] + fn socket_addr_slice_hostname_uses_first_address_only() { + let addrs = [ + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 22), + SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 22), + ]; + + assert_eq!(addrs.as_slice().hostname(), "127.0.0.1"); + } + + #[test] + fn empty_socket_addr_slice_hostname_is_empty() { + let addrs: [SocketAddr; 0] = []; + + assert_eq!(addrs.as_slice().hostname(), ""); } } diff --git a/src/test_helpers/env_guard.rs b/src/test_helpers/env_guard.rs index 6f96d77e..fb936339 100644 --- a/src/test_helpers/env_guard.rs +++ b/src/test_helpers/env_guard.rs @@ -67,7 +67,23 @@ #![cfg(test)] -use std::ffi::{OsStr, OsString}; +use std::{ + ffi::{OsStr, OsString}, + sync::{Mutex, MutexGuard}, +}; + +static HOME_ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// Guard for tests that read `HOME` without mutating it. +/// +/// `HOME` is process-global state and several tests temporarily repoint it. +/// Readers that call `dirs::home_dir()` or otherwise inspect `HOME` must hold +/// the same lock as `EnvGuard::set("HOME", ...)` so parallel test execution +/// cannot observe two different homes within one assertion. +#[must_use = "HomeEnvLock must be held for the whole HOME-sensitive assertion"] +pub struct HomeEnvLock { + _guard: MutexGuard<'static, ()>, +} /// RAII guard that sets or removes an environment variable on construction /// and restores the previous value (or unset state) on drop. @@ -79,6 +95,7 @@ use std::ffi::{OsStr, OsString}; pub struct EnvGuard { key: OsString, original: Option, + _home_lock: Option>, } // `#[allow(dead_code)]` is applied per-method so integration tests that only @@ -90,6 +107,7 @@ impl EnvGuard { #[allow(dead_code)] pub fn set(key: impl Into, value: impl AsRef) -> Self { let key = key.into(); + let home_lock = lock_home_if_needed(&key); let original = std::env::var_os(&key); // SAFETY: `#[serial]`-annotated tests that construct `EnvGuard` do not // run concurrently with each other, so cross-serial races on the env @@ -101,20 +119,46 @@ impl EnvGuard { unsafe { std::env::set_var(&key, value); } - Self { key, original } + Self { + key, + original, + _home_lock: home_lock, + } } /// Remove an environment variable, saving its prior value for restoration. #[allow(dead_code)] pub fn remove(key: impl Into) -> Self { let key = key.into(); + let home_lock = lock_home_if_needed(&key); let original = std::env::var_os(&key); // SAFETY: same rationale as `EnvGuard::set`; see the full comment // there and the module-level soundness contract. unsafe { std::env::remove_var(&key); } - Self { key, original } + Self { + key, + original, + _home_lock: home_lock, + } + } + + /// Lock the process-global `HOME` environment variable for tests that only + /// read it. + #[allow(dead_code)] + pub fn lock_home() -> HomeEnvLock { + HomeEnvLock { + _guard: HOME_ENV_LOCK.lock().unwrap(), + } + } +} + +fn lock_home_if_needed(key: &OsStr) -> Option> { + if key == OsStr::new("HOME") { + Some(HOME_ENV_LOCK.lock().unwrap()) + } else { + None } }