From e2433e8834d58d9314f8c5b377eaf2fc642d8e03 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 13:12:37 +0900 Subject: [PATCH 1/6] fix(security): implement real TOFU for accept-new host key mode The documented and recommended default `--strict-host-key-checking accept-new` mapped to `ServerCheckMethod::NoCheck`, so `check_server_key` accepted any server key unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, giving zero MITM protection contrary to the help text. Strict (`yes`) mode had the same defect class: a missing known_hosts file or an undeterminable path silently downgraded to `NoCheck`. Every known_hosts check error also collapsed into the generic `ServerCheckFailed`, making a changed key indistinguishable from an unreadable file. accept-new now maps to a new path-carrying `ServerCheckMethod::AcceptNewKnownHostsFile` variant handled by a new `ssh::tokio_client::host_verification` module: a key matching its known_hosts entry is accepted, an unknown host is recorded via russh's `learn_known_hosts_path` (which handles entry formatting including the `[host]:port` form for non-22 ports) and accepted with the OpenSSH-style "Permanently added" notice, and a conflicting key is rejected with the OpenSSH warning banner carrying the offered key's SHA256 fingerprint, the offending file and line, and an `ssh-keygen -R` remediation hint, without overwriting the entry. The whole check-then-record sequence runs under a process-wide `tokio::sync::Mutex` so parallel first-time connects to the same new host record exactly one entry and never observe torn writes; the critical section is purely synchronous file I/O, and a tokio mutex keeps the guard sound inside the async handler. When the recording creates `~/.ssh` or `known_hosts`, they get modes 0700 and 0600 respectively (Unix only; pre-existing files keep their modes). Strict mode now returns `KnownHostsFile(path)` unconditionally: russh treats a missing file as empty, so unknown hosts are rejected instead of unverified, and an undeterminable home directory falls back to `DefaultKnownHostsFile`, which fails closed. Changed keys surface as a dedicated `Error::HostKeyChanged { host, line }` in the `KnownHostsFile`, `DefaultKnownHostsFile`, and accept-new arms alike, with guidance arms added to `connect_error_message` and `format_ssh_error` that follow the #238 invariants (no cause restatement, no trailing period), and interactive password fallback explicitly never triggers on it. `get_check_method` is now a pure mapping with no filesystem side effects, the redundant strict-mode match in `jump/chain/tunnel.rs` is removed, the stale async-ssh2-tokio comments are deleted, and the hand-rolled mapping in the exec port-forwarding path (which sent accept-new through strict checking) is routed through `get_check_method` so TOFU covers every connection path: direct, first jump hop, intermediate hops, tunnel destination, SFTP, interactive, and port forwarding. Validated with cargo test --lib for ssh::tokio_client, ssh::known_hosts, ssh::client, jump::chain, and commands::exec (all passing, including 13 new tests covering first-use recording, dedup under 8-way concurrency, non-standard port round-trip, changed-key rejection, permission handling, and strict-mode rejection with a missing file), plus cargo clippy --lib --tests -D warnings and cargo fmt. Tests inject temp known_hosts paths through the new variant instead of mutating HOME. Refs #239 --- CHANGELOG.md | 3 + docs/architecture/ssh-client.md | 7 +- src/commands/exec.rs | 15 +- src/commands/interactive/connection.rs | 15 + src/jump/chain/tunnel.rs | 5 +- src/ssh/client/connection.rs | 44 ++ src/ssh/client/file_transfer.rs | 39 ++ src/ssh/known_hosts.rs | 133 +++--- src/ssh/tokio_client/authentication.rs | 9 + src/ssh/tokio_client/connection.rs | 31 +- src/ssh/tokio_client/error.rs | 4 + src/ssh/tokio_client/host_verification.rs | 533 ++++++++++++++++++++++ src/ssh/tokio_client/mod.rs | 1 + 13 files changed, 754 insertions(+), 85 deletions(-) create mode 100644 src/ssh/tokio_client/host_verification.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f2cd932..33481f5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security +- **Implement real TOFU verification for the default `accept-new` host key mode, which previously performed no verification at all** (#239). `--strict-host-key-checking accept-new` (the documented, recommended default) mapped to `NoCheck`: any server key was accepted unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, contrary to the help text's "Accept new hosts, reject changed keys". accept-new now checks the offered key against known_hosts, records unknown hosts (creating `~/.ssh` with mode 0700 and `known_hosts` with 0600 when it creates them, printing the OpenSSH-style "Permanently added" notice, and serializing parallel first-time connects behind a process-wide lock so the same new host is recorded exactly once), and rejects changed keys with the OpenSSH-style warning banner including the offered key's SHA256 fingerprint, the offending file and line, and an `ssh-keygen -R` remediation hint, without overwriting the recorded entry. Strict (`yes`) mode no longer downgrades to `NoCheck` when the known_hosts file is missing or its path cannot be determined; a missing file now behaves as an empty one, so unknown hosts are rejected. Changed keys are reported through a dedicated `HostKeyChanged` error carrying the host and conflicting line in every known_hosts-based mode instead of collapsing into the generic "Server check failed", and `no` mode is unchanged. The fix applies to every connection path since they all flow through the same handler: direct, jump-chain (first hop, intermediate hops, and the destination through the tunnel), SFTP transfers, and interactive mode. Entries use the `[host]:port` form for non-standard ports and round-trip through subsequent checks. + ### Fixed - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. - **Fix three pre-existing error-message defects that the #238 chain rendering would otherwise have made user-visible**. `src/jump/chain.rs`'s intermediate jump-hop context interpolated the jump host twice, printing `Failed to connect to jump host bastion (hop 2): bastion` instead of `Failed to connect to jump host bastion (hop 2)`. The direct-connect error in `src/ssh/client/connection.rs` and the file-transfer connect error in `src/ssh/client/file_transfer.rs` both built their anyhow chain backwards as `anyhow!(friendly_message).context(e)`, which makes `.context()`'s argument the *outer* layer, so the raw SSH error rendered first and the friendly message rendered underneath it as a near-duplicate cause; both now build `anyhow::Error::new(e).context(friendly_message)` so the friendly message renders first and `e` renders as the cause. `src/commands/ping.rs`'s error-chain print now reuses the shared `format_connection_error` helper instead of its own inline `format!("{e:#}")` for consistency; its per-line indentation is unchanged. diff --git a/docs/architecture/ssh-client.md b/docs/architecture/ssh-client.md index dea0fbe1..e5cc167e 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -37,11 +37,12 @@ **Security Implementation:** - Host key verification with three modes: - - `StrictHostKeyChecking::Yes` - Strict verification using known_hosts + - `StrictHostKeyChecking::Yes` - Strict verification using known_hosts; a missing file behaves as an empty one, so unknown hosts are rejected - `StrictHostKeyChecking::No` - Skip all verification - - `StrictHostKeyChecking::AcceptNew` - TOFU mode + - `StrictHostKeyChecking::AcceptNew` - TOFU mode: keys matching their known_hosts entry are accepted, unknown hosts are recorded in known_hosts and accepted, changed keys are rejected with an OpenSSH-style warning - CLI flag `--strict-host-key-checking` with default "accept-new" -- Uses system known_hosts file (~/.ssh/known_hosts) +- Uses system known_hosts file (~/.ssh/known_hosts); accept-new creates it on first recording (directory 0700, file 0600) and serializes concurrent first-time recordings behind a process-wide lock so parallel connects to the same new host produce a single entry +- Changed keys surface as a dedicated `HostKeyChanged` error (host, offending line) instead of a generic check failure, in strict and accept-new modes alike - SSH agent authentication with auto-detection ### 4.0.1 Command Output Streaming Infrastructure diff --git a/src/commands/exec.rs b/src/commands/exec.rs index 6e7b49a3..2bad3c7d 100644 --- a/src/commands/exec.rs +++ b/src/commands/exec.rs @@ -99,8 +99,7 @@ async fn execute_command_with_forwarding(params: ExecuteCommandParams<'_>) -> Re let mut manager = ForwardingManager::new(forwarding_config); // Create SSH client for forwarding - use crate::ssh::known_hosts::StrictHostKeyChecking; - use crate::ssh::tokio_client::{AuthMethod, Client, ServerCheckMethod}; + use crate::ssh::tokio_client::{AuthMethod, Client}; // Determine authentication method let auth_method = if params.use_agent { @@ -154,12 +153,12 @@ async fn execute_command_with_forwarding(params: ExecuteCommandParams<'_>) -> Re AuthMethod::with_key_file(key_path, None) }; - // Determine server check method - let server_check = match params.strict_mode { - StrictHostKeyChecking::Yes => ServerCheckMethod::DefaultKnownHostsFile, - StrictHostKeyChecking::No => ServerCheckMethod::NoCheck, - StrictHostKeyChecking::AcceptNew => ServerCheckMethod::DefaultKnownHostsFile, // Could be enhanced - }; + // Determine server check method through the shared mapping so the + // port-forwarding path gets the same TOFU behavior as every other + // connection path (#239). The previous hand-rolled match sent accept-new + // through strict default-known_hosts checking, which rejected unknown + // hosts instead of recording them. + let server_check = crate::ssh::known_hosts::get_check_method(params.strict_mode); // Create SSH client let ssh_client = Arc::new( diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index d732d6b5..199a6beb 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -613,6 +613,21 @@ mod tests { ); } + #[test] + fn test_host_key_changed_does_not_trigger_fallback() { + // A changed host key is a possible man-in-the-middle, not an auth + // problem; retrying with a password would hand credentials to the + // untrusted endpoint (#239). + let error = SshError::HostKeyChanged { + host: "node1.example.com".to_string(), + line: 3, + }; + assert!( + !is_auth_error_for_password_fallback(&error), + "HostKeyChanged should NOT trigger password fallback (host key issue)" + ); + } + #[test] fn test_io_error_does_not_trigger_fallback() { let error = SshError::IoError(std::io::Error::new( diff --git a/src/jump/chain/tunnel.rs b/src/jump/chain/tunnel.rs index 1e9b4914..c62cab0b 100644 --- a/src/jump/chain/tunnel.rs +++ b/src/jump/chain/tunnel.rs @@ -214,10 +214,7 @@ pub(super) async fn connect_to_destination( // Create SSH client over the tunnel stream with keepalive settings let config = Arc::new(ssh_connection_config.to_russh_config()); - let check_method = match strict_mode { - StrictHostKeyChecking::No => crate::ssh::tokio_client::ServerCheckMethod::NoCheck, - _ => crate::ssh::known_hosts::get_check_method(strict_mode), - }; + let check_method = crate::ssh::known_hosts::get_check_method(strict_mode); let socket_addr: SocketAddr = format!("{destination_host}:{destination_port}") .to_socket_addrs() diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index 2cbe1a47..ecc0f41c 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -49,6 +49,13 @@ fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> Option "Host key verification failed: the server's host key was not recognized or has changed" .to_string(), ), + crate::ssh::tokio_client::Error::HostKeyChanged { host, .. } => Some(format!( + "Possible man-in-the-middle attack: verify the server's new key out of band, or remove the old entry with 'ssh-keygen -R {host}' if the change is expected" + )), + crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey) => Some( + "The host is not in known_hosts and strict host key checking is enabled; connect once with '--strict-host-key-checking accept-new' or add the key manually" + .to_string(), + ), crate::ssh::tokio_client::Error::KeyInvalid(_) => { Some("Check the key file format and passphrase".to_string()) } @@ -562,6 +569,11 @@ mod tests { crate::ssh::tokio_client::Error::ServerCheckFailed, crate::ssh::tokio_client::Error::AgentConnectionFailed, crate::ssh::tokio_client::Error::AgentNoIdentities, + crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + line: 3, + }, + crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey), ] { let message = connect_error_message(&e).expect("variant adds guidance"); assert!( @@ -570,4 +582,36 @@ mod tests { ); } } + + #[test] + fn test_connect_error_message_host_key_changed_adds_guidance_without_echo() { + // The changed-key context layer must add remediation guidance (which + // host entry to remove) without restating the cause's own wording, + // which would render twice through anyhow's `{:#}` form (#239, #238). + let e = crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + line: 7, + }; + let cause_text = e.to_string(); + let message = connect_error_message(&e).expect("HostKeyChanged adds guidance"); + assert!( + message.contains("ssh-keygen -R node1.example.com"), + "guidance must include the removal command, got: {message}" + ); + assert!( + !message.contains("has changed and no longer matches"), + "context must not restate the cause, got: {message}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(message)); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + assert!( + rendered.contains("line 7"), + "the conflicting line number must survive into the chain, got: {rendered}" + ); + } } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index a7cf001a..6a5eb435 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -746,6 +746,11 @@ fn format_ssh_error(context: &str, e: &crate::ssh::tokio_client::Error) -> Strin "{context} failed: Host key verification failed, the server's host key is not trusted" ) } + crate::ssh::tokio_client::Error::HostKeyChanged { host, .. } => { + format!( + "{context} failed: Possible man-in-the-middle attack, remove the old known_hosts entry with 'ssh-keygen -R {host}' only if the key change is expected" + ) + } crate::ssh::tokio_client::Error::PasswordWrong => { format!("{context} failed: Password authentication rejected") } @@ -822,6 +827,10 @@ mod tests { crate::ssh::tokio_client::Error::AgentConnectionFailed, crate::ssh::tokio_client::Error::AgentNoIdentities, crate::ssh::tokio_client::Error::AgentAuthenticationFailed, + crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + line: 3, + }, ] { let detailed = format_ssh_error(&context, &e); assert!( @@ -831,6 +840,36 @@ mod tests { } } + #[test] + fn test_format_ssh_error_host_key_changed_adds_guidance_without_echo() { + // The changed-key context layer must point at the offending entry's + // removal command without restating the cause's own wording, which + // would render twice through anyhow's `{:#}` form (#239, #238). + let e = crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + line: 7, + }; + let cause_text = e.to_string(); + let context = "SFTP upload to host:22".to_string(); + + let detailed = format_ssh_error(&context, &e); + assert!( + detailed.contains("ssh-keygen -R node1.example.com"), + "guidance must include the removal command, got: {detailed}" + ); + assert!( + !detailed.contains("has changed and no longer matches"), + "context must not restate the cause, got: {detailed}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(detailed)); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + } + #[test] fn test_format_ssh_error_catch_all_does_not_duplicate_cause() { // The catch-all arm used to interpolate `{e}` directly into the diff --git a/src/ssh/known_hosts.rs b/src/ssh/known_hosts.rs index be45f238..fa63fbe8 100644 --- a/src/ssh/known_hosts.rs +++ b/src/ssh/known_hosts.rs @@ -21,69 +21,61 @@ pub fn get_default_known_hosts_path() -> Option { dirs::home_dir().map(|home| home.join(".ssh").join("known_hosts")) } -/// Create a ServerCheckMethod based on strict host key checking mode +/// Create a ServerCheckMethod based on strict host key checking mode. +/// +/// This is a pure mapping with no filesystem side effects. A missing +/// known_hosts file is handled at verification time: russh treats an +/// unreadable file as empty, so strict mode rejects unknown hosts and +/// accept-new mode records them, creating the file on first recording. pub fn get_check_method(strict_mode: StrictHostKeyChecking) -> ServerCheckMethod { match strict_mode { - StrictHostKeyChecking::Yes => { - // Use the default known_hosts file in strict mode - if let Some(known_hosts_path) = get_default_known_hosts_path() { - if known_hosts_path.exists() { - tracing::debug!( - "Using known_hosts file: {:?} (strict mode)", - known_hosts_path - ); - ServerCheckMethod::DefaultKnownHostsFile - } else { - tracing::warn!( - "Known hosts file not found at {:?}, using NoCheck", - known_hosts_path - ); - eprintln!( - "WARNING: Known hosts file not found. Host key verification disabled." - ); - ServerCheckMethod::NoCheck - } - } else { - tracing::warn!("Could not determine known_hosts path, using NoCheck"); - ServerCheckMethod::NoCheck + StrictHostKeyChecking::Yes => match get_default_known_hosts_path() { + Some(known_hosts_path) => { + tracing::debug!( + "Using known_hosts file: {:?} (strict mode)", + known_hosts_path + ); + ServerCheckMethod::KnownHostsFile(known_hosts_path.to_string_lossy().into_owned()) } - } + None => { + // Verification must never be disabled in strict mode (#239). + // Fall back to russh's own default path resolution; when no + // home directory exists that resolution errors out and the + // connection is rejected, so this fails closed. + tracing::warn!( + "Could not determine known_hosts path; strict host key checking will fail closed" + ); + ServerCheckMethod::DefaultKnownHostsFile + } + }, StrictHostKeyChecking::No => { tracing::debug!("Host key checking disabled (strict mode = no)"); ServerCheckMethod::NoCheck } - StrictHostKeyChecking::AcceptNew => { - // Use known_hosts but don't fail on new hosts - // Note: async-ssh2-tokio doesn't support TOFU mode directly, - // so we use the known_hosts file if it exists, otherwise NoCheck - if let Some(known_hosts_path) = get_default_known_hosts_path() { - if known_hosts_path.exists() { - tracing::debug!( - "Using known_hosts file: {:?} (accept-new mode)", - known_hosts_path - ); - // Unfortunately, the library doesn't support accept-new mode directly - // We'll use the known_hosts file, but it will fail on unknown hosts - // For now, we'll use NoCheck for accept-new mode - tracing::info!( - "Note: accept-new mode not fully supported, using relaxed checking" - ); - ServerCheckMethod::NoCheck - } else { - // Create the .ssh directory if it doesn't exist - if let Some(ssh_dir) = known_hosts_path.parent() { - let _ = std::fs::create_dir_all(ssh_dir); - } - // Create an empty known_hosts file - let _ = std::fs::File::create(&known_hosts_path); - tracing::debug!("Created empty known_hosts file at {:?}", known_hosts_path); - ServerCheckMethod::NoCheck - } - } else { - tracing::warn!("Could not determine known_hosts path, using NoCheck"); + StrictHostKeyChecking::AcceptNew => match get_default_known_hosts_path() { + Some(known_hosts_path) => { + tracing::debug!( + "Using known_hosts file: {:?} (accept-new/TOFU mode)", + known_hosts_path + ); + ServerCheckMethod::AcceptNewKnownHostsFile( + known_hosts_path.to_string_lossy().into_owned(), + ) + } + None => { + // Without a home directory there is no persistent trust state, + // so first-use recording and change detection are impossible: + // every host is "new" and accept-new semantics accept it. Warn + // loudly that nothing can be recorded or verified. + tracing::warn!( + "Could not determine known_hosts path; host keys cannot be recorded or verified" + ); + eprintln!( + "Warning: could not determine the known_hosts path; host keys cannot be recorded or verified in accept-new mode" + ); ServerCheckMethod::NoCheck } - } + }, } } @@ -184,16 +176,33 @@ mod tests { let method = get_check_method(StrictHostKeyChecking::No); assert!(matches!(method, ServerCheckMethod::NoCheck)); - // Test with AcceptNew mode (should use NoCheck since library doesn't support TOFU) + // AcceptNew must map to the TOFU variant carrying the default + // known_hosts path, whether or not the file exists yet (#239). let method = get_check_method(StrictHostKeyChecking::AcceptNew); - assert!(matches!(method, ServerCheckMethod::NoCheck)); + match method { + ServerCheckMethod::AcceptNewKnownHostsFile(path) => { + assert!( + path.ends_with("known_hosts"), + "expected the default known_hosts path, got: {path}" + ); + } + other => panic!("accept-new must map to AcceptNewKnownHostsFile, got {other:?}"), + } - // Test with Yes mode + // Yes must never disable verification, even when the known_hosts + // file is missing: a missing file behaves as an empty one (#239). let method = get_check_method(StrictHostKeyChecking::Yes); - // Result depends on whether known_hosts file exists - assert!(matches!( - method, - ServerCheckMethod::DefaultKnownHostsFile | ServerCheckMethod::NoCheck - )); + match method { + ServerCheckMethod::KnownHostsFile(path) => { + assert!( + path.ends_with("known_hosts"), + "expected the default known_hosts path, got: {path}" + ); + } + // Only reachable when no home directory can be determined; still + // a verifying method that fails closed. + ServerCheckMethod::DefaultKnownHostsFile => {} + other => panic!("strict mode must keep verification enabled, got {other:?}"), + } } } diff --git a/src/ssh/tokio_client/authentication.rs b/src/ssh/tokio_client/authentication.rs index ad1f7868..20f84fbc 100644 --- a/src/ssh/tokio_client/authentication.rs +++ b/src/ssh/tokio_client/authentication.rs @@ -196,6 +196,10 @@ pub enum ServerCheckMethod { DefaultKnownHostsFile, /// 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, + /// changed keys are rejected (OpenSSH `StrictHostKeyChecking=accept-new`) + AcceptNewKnownHostsFile(String), } impl ServerCheckMethod { @@ -213,6 +217,11 @@ impl ServerCheckMethod { pub fn with_known_hosts_file(known_hosts_file: &str) -> Self { Self::KnownHostsFile(known_hosts_file.to_string()) } + + /// Convenience method to create a [`ServerCheckMethod`] from a string literal. + pub fn with_accept_new_known_hosts_file(known_hosts_file: &str) -> Self { + Self::AcceptNewKnownHostsFile(known_hosts_file.to_string()) + } } /// This takes a handle and performs authentification with the given method. diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index 900e9439..d3c17a8f 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -541,25 +541,40 @@ impl Handler for ClientHandler { Ok(pk == *server_public_key) } ServerCheckMethod::KnownHostsFile(known_hosts_path) => { - let result = russh::keys::check_known_hosts_path( + russh::keys::check_known_hosts_path( &self.hostname, self.host.port(), server_public_key, known_hosts_path, ) - .map_err(|_| super::Error::ServerCheckFailed)?; - - Ok(result) + .map_err(|e| { + super::host_verification::map_known_hosts_error( + &self.hostname, + server_public_key, + known_hosts_path, + e, + ) + }) } ServerCheckMethod::DefaultKnownHostsFile => { - let result = russh::keys::check_known_hosts( + russh::keys::check_known_hosts(&self.hostname, self.host.port(), server_public_key) + .map_err(|e| { + super::host_verification::map_known_hosts_error( + &self.hostname, + server_public_key, + "~/.ssh/known_hosts", + e, + ) + }) + } + ServerCheckMethod::AcceptNewKnownHostsFile(known_hosts_path) => { + super::host_verification::verify_accept_new( &self.hostname, self.host.port(), server_public_key, + known_hosts_path, ) - .map_err(|_| super::Error::ServerCheckFailed)?; - - Ok(result) + .await } } } diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index a47765b4..769621ef 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -23,6 +23,10 @@ pub enum Error { CommandDidntExit, #[error("Server check failed")] ServerCheckFailed, + #[error( + "Host key for '{host}' has changed and no longer matches the known_hosts entry at line {line}" + )] + HostKeyChanged { host: String, line: usize }, #[error("SSH error occurred: {0}")] SshError(#[from] russh::Error), #[error("Send error")] diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs new file mode 100644 index 00000000..7e3ca1c5 --- /dev/null +++ b/src/ssh/tokio_client/host_verification.rs @@ -0,0 +1,533 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Trust On First Use (TOFU) host key verification. +//! +//! Implements the `accept-new` host key checking mode (#239): a host key that +//! matches its known_hosts entry is accepted, an unknown host is recorded in +//! the known_hosts file and accepted, and a key that conflicts with an existing +//! entry is rejected with an OpenSSH-style warning. The recording itself is +//! delegated to `russh::keys::known_hosts::learn_known_hosts_path`, which +//! handles entry formatting (including the `[host]:port` form for non-22 +//! ports) and file creation; this module adds the trust decision, restrictive +//! permissions on newly created files, and serialization of concurrent +//! first-time connections. + +use russh::keys::{Algorithm, HashAlg, PublicKey}; +use std::path::Path; +use tokio::sync::Mutex; + +/// Serializes every known_hosts check-then-record sequence in this process. +/// +/// bssh connects to many nodes in parallel, so several first-time connections +/// to the same new host can run concurrently. Without serialization each of +/// them would observe "unknown host" and append its own entry, and a reader +/// could also observe a half-written line. Holding one lock across the whole +/// check+record critical section makes the sequence atomic within the +/// process: whichever connection wins the race records the key, and the rest +/// re-run the check under the lock and see the entry as already known. +/// +/// This is a `tokio::sync::Mutex` rather than `std::sync::Mutex`. The guard +/// lives inside an async fn, and while the critical section is purely +/// synchronous file I/O today, a tokio mutex keeps the guard sound if an +/// await point is ever introduced and parks contending tasks instead of +/// blocking runtime worker threads. +static KNOWN_HOSTS_LOCK: Mutex<()> = Mutex::const_new(()); + +/// Verify a server key in accept-new (TOFU) mode against `known_hosts_path`. +/// +/// Returns `Ok(true)` when the key matches an existing entry or the host was +/// unknown (in which case the key is recorded first). Returns +/// [`super::Error::HostKeyChanged`] when an entry for the host exists with a +/// different key, without modifying the file. +pub(super) async fn verify_accept_new( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) -> Result { + let _guard = KNOWN_HOSTS_LOCK.lock().await; + + match russh::keys::check_known_hosts_path(hostname, port, server_public_key, known_hosts_path) { + Ok(true) => Ok(true), + Ok(false) => { + // Unknown host: trust on first use. A missing known_hosts file + // also lands here because russh treats an unreadable file as + // empty; `learn_known_hosts_path` creates it on demand. + record_host_key(hostname, port, server_public_key, known_hosts_path); + Ok(true) + } + Err(e) => Err(map_known_hosts_error( + hostname, + server_public_key, + known_hosts_path, + e, + )), + } +} + +/// Convert a known_hosts check failure into the crate error type. +/// +/// A [`russh::keys::Error::KeyChanged`] becomes the dedicated +/// [`super::Error::HostKeyChanged`] variant after printing an OpenSSH-style +/// warning, so a changed key is distinguishable from an unreadable or +/// malformed known_hosts file (which stays the generic `ServerCheckFailed`). +/// Shared by the accept-new, `KnownHostsFile`, and `DefaultKnownHostsFile` +/// check methods so strict mode reports changed keys just as clearly. +pub(super) fn map_known_hosts_error( + hostname: &str, + server_public_key: &PublicKey, + known_hosts_display: &str, + err: russh::keys::Error, +) -> super::Error { + match err { + russh::keys::Error::KeyChanged { line } => { + print_host_key_changed_warning(hostname, server_public_key, known_hosts_display, line); + super::Error::HostKeyChanged { + host: hostname.to_string(), + line, + } + } + e => { + tracing::error!("Host key verification failed for '{hostname}': {e}"); + super::Error::ServerCheckFailed + } + } +} + +/// Record `server_public_key` in `known_hosts_path` and print the OpenSSH +/// "Permanently added" notice. +/// +/// Recording is best effort, matching OpenSSH: in accept-new mode an unknown +/// host is accepted either way, so a failure to persist the key (read-only +/// filesystem, permission problem) is reported as a warning instead of +/// failing the connection. +fn record_host_key( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) { + let path = Path::new(known_hosts_path); + // Only files and directories created by this recording get their + // permissions tightened; pre-existing ones are left untouched. + let dir_preexisted = path.parent().is_none_or(Path::exists); + let file_preexisted = path.exists(); + + if let Err(e) = + russh::keys::known_hosts::learn_known_hosts_path(hostname, port, server_public_key, path) + { + tracing::warn!("Failed to record host key for '{hostname}' in {known_hosts_path}: {e}"); + eprintln!( + "Warning: failed to add '{}' to the list of known hosts ({known_hosts_path}): {e}", + known_hosts_entry_name(hostname, port) + ); + return; + } + + #[cfg(unix)] + restrict_created_permissions(path, dir_preexisted, file_preexisted); + #[cfg(not(unix))] + let _ = (dir_preexisted, file_preexisted); + + eprintln!( + "Permanently added '{}' ({}) to the list of known hosts.", + known_hosts_entry_name(hostname, port), + algorithm_display_name(server_public_key) + ); +} + +/// 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. +fn known_hosts_entry_name(hostname: &str, port: u16) -> String { + if port == 22 { + hostname.to_string() + } else { + format!("[{hostname}]:{port}") + } +} + +/// Tighten permissions on the `.ssh` directory (0700) and known_hosts file +/// (0600), but only when this recording created them. +/// `learn_known_hosts_path` creates both with the process umask, so a fresh +/// directory could otherwise be group/world readable. +#[cfg(unix)] +fn restrict_created_permissions(path: &Path, dir_preexisted: bool, file_preexisted: bool) { + use std::os::unix::fs::PermissionsExt; + + if !dir_preexisted + && let Some(parent) = path.parent() + && let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) + { + tracing::warn!("Failed to set mode 0700 on {}: {e}", parent.display()); + } + if !file_preexisted + && let Err(e) = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + { + tracing::warn!("Failed to set mode 0600 on {}: {e}", path.display()); + } +} + +/// OpenSSH-style short algorithm name for user-facing messages +/// (e.g. `ED25519` rather than `ssh-ed25519`). +fn algorithm_display_name(key: &PublicKey) -> String { + match key.algorithm() { + Algorithm::Ed25519 => "ED25519".to_string(), + Algorithm::Rsa { .. } => "RSA".to_string(), + Algorithm::Ecdsa { .. } => "ECDSA".to_string(), + Algorithm::Dsa => "DSA".to_string(), + Algorithm::SkEd25519 => "ED25519-SK".to_string(), + Algorithm::SkEcdsaSha2NistP256 => "ECDSA-SK".to_string(), + other => other.as_str().to_uppercase(), + } +} + +/// Print the loud OpenSSH-style warning for a changed host key, including the +/// SHA256 fingerprint of the offered key, the conflicting file and line, and +/// a remediation hint. The conflicting entry is never modified. +fn print_host_key_changed_warning( + hostname: &str, + server_public_key: &PublicKey, + known_hosts_display: &str, + line: usize, +) { + let algo = algorithm_display_name(server_public_key); + let fingerprint = server_public_key.fingerprint(HashAlg::Sha256); + eprintln!( + "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n\ + @ WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED! @\n\ + @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n\ + IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!\n\ + Someone could be eavesdropping on you right now (man-in-the-middle attack)!\n\ + It is also possible that a host key has just been changed.\n\ + The fingerprint for the {algo} key sent by the remote host is\n\ + {fingerprint}.\n\ + Please contact your system administrator.\n\ + Add correct host key in {known_hosts_display} to get rid of this message.\n\ + Offending key in {known_hosts_display}:{line}\n\ + If the key change is expected, remove the old entry with:\n\ + \x20 ssh-keygen -R '{hostname}'" + ); +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ssh::tokio_client::{ClientHandler, Error, ServerCheckMethod}; + use russh::client::Handler; + use russh::keys::PrivateKey; + use std::path::PathBuf; + use tempfile::TempDir; + + fn generate_key() -> PrivateKey { + PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519) + .expect("ed25519 key generation should not fail") + } + + /// Non-empty known_hosts lines. `learn_known_hosts_path` prefixes its + /// first append to a fresh file with a newline, so blank lines are not + /// entries. + fn entry_lines(path: &Path) -> Vec { + std::fs::read_to_string(path) + .unwrap_or_default() + .lines() + .filter(|l| !l.trim().is_empty()) + .map(str::to_string) + .collect() + } + + fn temp_known_hosts() -> (TempDir, PathBuf, String) { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("known_hosts"); + let path_str = path.to_str().unwrap().to_string(); + (dir, path, path_str) + } + + #[tokio::test] + async fn test_accept_new_records_unknown_host_and_accepts() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + // First connection: file does not exist yet, host is unknown. + let result = verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true)), "unknown host must be accepted"); + + let lines = entry_lines(&path); + assert_eq!(lines.len(), 1, "exactly one entry must be recorded"); + assert!( + lines[0].starts_with("node1.example.com "), + "port 22 must be recorded as the bare hostname, got: {}", + lines[0] + ); + assert!(lines[0].contains("ssh-ed25519")); + } + + #[tokio::test] + async fn test_accept_new_second_connection_does_not_duplicate() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + for _ in 0..3 { + let result = + verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + } + + assert_eq!( + entry_lines(&path).len(), + 1, + "repeat connections with the same key must not append duplicates" + ); + } + + #[tokio::test] + async fn test_accept_new_rejects_changed_key_and_keeps_entry() { + let (_dir, path, path_str) = temp_known_hosts(); + let original = generate_key(); + let imposter = generate_key(); + + let result = + verify_accept_new("node1.example.com", 22, original.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + let recorded = entry_lines(&path); + + // Same host now presents a different key: must be rejected with the + // dedicated changed-key error, not accepted and not re-recorded. + let result = + verify_accept_new("node1.example.com", 22, imposter.public_key(), &path_str).await; + match result { + Err(Error::HostKeyChanged { host, line }) => { + assert_eq!(host, "node1.example.com"); + assert!(line > 0); + } + other => panic!("expected HostKeyChanged, got {other:?}"), + } + + assert_eq!( + entry_lines(&path), + recorded, + "the conflicting entry must not be overwritten or appended to" + ); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn test_accept_new_concurrent_first_connections_record_one_entry() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = std::sync::Arc::new(generate_key()); + + let mut handles = Vec::new(); + for _ in 0..8 { + let key = std::sync::Arc::clone(&key); + let path_str = path_str.clone(); + handles.push(tokio::spawn(async move { + verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await + })); + } + for handle in handles { + let result = handle.await.unwrap(); + assert!(matches!(result, Ok(true)), "every racer must be accepted"); + } + + assert_eq!( + entry_lines(&path).len(), + 1, + "parallel first-time connections must record exactly one entry" + ); + } + + #[tokio::test] + async fn test_accept_new_non_standard_port_round_trips() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + let result = + verify_accept_new("node1.example.com", 2222, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + + let lines = entry_lines(&path); + assert_eq!(lines.len(), 1); + assert!( + lines[0].starts_with("[node1.example.com]:2222 "), + "non-standard ports must use the [host]:port form, got: {}", + lines[0] + ); + + // The recorded entry must round-trip through the checker. + let result = + verify_accept_new("node1.example.com", 2222, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + assert_eq!(entry_lines(&path).len(), 1, "round trip must not duplicate"); + + // A different key on the same host:port is a changed key. + let imposter = generate_key(); + let result = + verify_accept_new("node1.example.com", 2222, imposter.public_key(), &path_str).await; + assert!(matches!(result, Err(Error::HostKeyChanged { .. }))); + } + + #[cfg(unix)] + #[tokio::test] + async fn test_accept_new_created_dir_and_file_get_restrictive_modes() { + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + // Both the .ssh directory and the file are created by the recording. + let ssh_dir = dir.path().join(".ssh"); + let path = ssh_dir.join("known_hosts"); + let path_str = path.to_str().unwrap().to_string(); + 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 dir_mode = std::fs::metadata(&ssh_dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700, "created .ssh directory must be 0700"); + let file_mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600, "created known_hosts must be 0600"); + } + + #[cfg(unix)] + #[tokio::test] + async fn test_accept_new_preserves_preexisting_dir_and_file_modes() { + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let ssh_dir = dir.path().join(".ssh"); + std::fs::create_dir(&ssh_dir).unwrap(); + std::fs::set_permissions(&ssh_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let path = ssh_dir.join("known_hosts"); + std::fs::write(&path, "").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + let path_str = path.to_str().unwrap().to_string(); + 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 dir_mode = std::fs::metadata(&ssh_dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o755, "pre-existing directory mode must be kept"); + let file_mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o644, "pre-existing file mode must be kept"); + } + + #[test] + fn test_map_known_hosts_error_distinguishes_key_changed() { + let key = generate_key(); + + let err = map_known_hosts_error( + "node1.example.com", + key.public_key(), + "/tmp/known_hosts", + russh::keys::Error::KeyChanged { line: 7 }, + ); + match err { + Error::HostKeyChanged { host, line } => { + assert_eq!(host, "node1.example.com"); + assert_eq!(line, 7); + } + other => panic!("expected HostKeyChanged, got {other:?}"), + } + + let err = map_known_hosts_error( + "node1.example.com", + key.public_key(), + "/tmp/known_hosts", + russh::keys::Error::KeyIsCorrupt, + ); + assert!(matches!(err, Error::ServerCheckFailed)); + } + + #[test] + fn test_known_hosts_entry_name_forms() { + assert_eq!(known_hosts_entry_name("host", 22), "host"); + assert_eq!(known_hosts_entry_name("host", 2222), "[host]:2222"); + } + + // Strict (`yes`) mode behavior through the real handler entry point. + + fn handler_for(check: ServerCheckMethod) -> ClientHandler { + ClientHandler::new( + "node1.example.com".to_string(), + "127.0.0.1:22".parse().unwrap(), + check, + ) + } + + #[tokio::test] + async fn test_strict_mode_missing_known_hosts_rejects_unknown_host() { + // Regression test for #239's second defect: a missing known_hosts + // file in strict mode must behave as an empty file (unknown host + // rejected), never as disabled verification. + let (_dir, _path, path_str) = temp_known_hosts(); + let key = generate_key(); + + let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); + let result = handler.check_server_key(key.public_key()).await; + assert!( + matches!(result, Ok(false)), + "unknown host must be rejected in strict mode, got {result:?}" + ); + } + + #[tokio::test] + async fn test_strict_mode_reports_changed_key_specifically() { + let (_dir, _path, path_str) = temp_known_hosts(); + let original = generate_key(); + let imposter = generate_key(); + + russh::keys::known_hosts::learn_known_hosts_path( + "node1.example.com", + 22, + original.public_key(), + &path_str, + ) + .unwrap(); + + let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str.clone())); + let result = handler.check_server_key(imposter.public_key()).await; + assert!( + matches!(result, Err(Error::HostKeyChanged { .. })), + "strict mode must report a changed key specifically, got {result:?}" + ); + + // The original key still verifies. + let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); + let result = handler.check_server_key(original.public_key()).await; + assert!(matches!(result, Ok(true))); + } + + #[tokio::test] + async fn test_no_check_accepts_without_touching_filesystem() { + let (_dir, path, _path_str) = temp_known_hosts(); + let key = generate_key(); + + let mut handler = handler_for(ServerCheckMethod::NoCheck); + let result = handler.check_server_key(key.public_key()).await; + assert!(matches!(result, Ok(true))); + assert!(!path.exists(), "NoCheck must not create a known_hosts file"); + } + + #[tokio::test] + async fn test_accept_new_through_client_handler() { + // The handler arm must reach the TOFU path end to end. + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + let mut handler = handler_for(ServerCheckMethod::AcceptNewKnownHostsFile(path_str)); + let result = handler.check_server_key(key.public_key()).await; + assert!(matches!(result, Ok(true))); + assert_eq!(entry_lines(&path).len(), 1); + } +} diff --git a/src/ssh/tokio_client/mod.rs b/src/ssh/tokio_client/mod.rs index 971b0f9e..377bb004 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -21,6 +21,7 @@ pub mod connection; mod connection_tests; pub mod error; pub mod file_transfer; +mod host_verification; mod to_socket_addrs_with_hostname; // Re-export public API types for backward compatibility From 328515a343347c76c130a7afdfa993b8ffc11239 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 13:47:47 +0900 Subject: [PATCH 2/6] fix(security): reject alternate-algorithm host keys and fix removal hint Two defects found reviewing #239's TOFU implementation. `russh::keys::check_known_hosts_path` reports `Err(KeyChanged)` only when an entry for the host exists with the same key algorithm, and `Ok(false)` both for a hostname with no entry at all and for a hostname whose entries are all of a different algorithm. accept-new treated the second case as a first use, appending the offered key next to the existing entry and accepting the connection, so a man-in-the-middle that advertises support for only an algorithm the host has not used yet bypassed pinning entirely: SSH negotiation picks the first client algorithm the server also supports, so the attacker chooses it. OpenSSH resists this by reordering its per-host key algorithm proposal to prefer already-known types, which russh cannot do per host, so `verify_accept_new` now consults `known_host_keys_path` before recording and treats any existing entry for the host as a changed key. The printed removal command did not work for non-standard ports. known_hosts records those as `[host]:port`, but the warning banner and both client-facing guidance messages emitted `ssh-keygen -R `, which matches nothing, and `Error::HostKeyChanged` did not carry the port for the downstream messages to use. The variant now carries the port, the banner follows OpenSSH's `ssh-keygen -f "" -R ""` form with the resolved default known_hosts path, and every printed argument is double quoted so an unmatched `[...]` glob does not make zsh reject the command. Both #240 invariants are preserved: no context layer restates its own cause, and no context message ends with a period. Refs #239 --- CHANGELOG.md | 2 +- src/commands/interactive/connection.rs | 1 + src/ssh/client/connection.rs | 47 +++++- src/ssh/client/file_transfer.rs | 44 +++++- src/ssh/tokio_client/connection.rs | 12 +- src/ssh/tokio_client/error.rs | 10 +- src/ssh/tokio_client/host_verification.rs | 174 ++++++++++++++++++++-- src/ssh/tokio_client/mod.rs | 4 +- 8 files changed, 269 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33481f5d..cc4308a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Security -- **Implement real TOFU verification for the default `accept-new` host key mode, which previously performed no verification at all** (#239). `--strict-host-key-checking accept-new` (the documented, recommended default) mapped to `NoCheck`: any server key was accepted unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, contrary to the help text's "Accept new hosts, reject changed keys". accept-new now checks the offered key against known_hosts, records unknown hosts (creating `~/.ssh` with mode 0700 and `known_hosts` with 0600 when it creates them, printing the OpenSSH-style "Permanently added" notice, and serializing parallel first-time connects behind a process-wide lock so the same new host is recorded exactly once), and rejects changed keys with the OpenSSH-style warning banner including the offered key's SHA256 fingerprint, the offending file and line, and an `ssh-keygen -R` remediation hint, without overwriting the recorded entry. Strict (`yes`) mode no longer downgrades to `NoCheck` when the known_hosts file is missing or its path cannot be determined; a missing file now behaves as an empty one, so unknown hosts are rejected. Changed keys are reported through a dedicated `HostKeyChanged` error carrying the host and conflicting line in every known_hosts-based mode instead of collapsing into the generic "Server check failed", and `no` mode is unchanged. The fix applies to every connection path since they all flow through the same handler: direct, jump-chain (first hop, intermediate hops, and the destination through the tunnel), SFTP transfers, and interactive mode. Entries use the `[host]:port` form for non-standard ports and round-trip through subsequent checks. +- **Implement real TOFU verification for the default `accept-new` host key mode, which previously performed no verification at all** (#239). `--strict-host-key-checking accept-new` (the documented, recommended default) mapped to `NoCheck`: any server key was accepted unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, contrary to the help text's "Accept new hosts, reject changed keys". accept-new now checks the offered key against known_hosts, records unknown hosts (creating `~/.ssh` with mode 0700 and `known_hosts` with 0600 when it creates them, printing the OpenSSH-style "Permanently added" notice, and serializing parallel first-time connects behind a process-wide lock so the same new host is recorded exactly once), and rejects changed keys with the OpenSSH-style warning banner including the offered key's SHA256 fingerprint, the offending file and line, and an `ssh-keygen -R` remediation hint, without overwriting the recorded entry. Strict (`yes`) mode no longer downgrades to `NoCheck` when the known_hosts file is missing or its path cannot be determined; a missing file now behaves as an empty one, so unknown hosts are rejected. Changed keys are reported through a dedicated `HostKeyChanged` error carrying the host and conflicting line in every known_hosts-based mode instead of collapsing into the generic "Server check failed", and `no` mode is unchanged. The fix applies to every connection path since they all flow through the same handler: direct, jump-chain (first hop, intermediate hops, and the destination through the tunnel), SFTP transfers, and interactive mode. Entries use the `[host]:port` form for non-standard ports and round-trip through subsequent checks. accept-new also rejects a key whose algorithm differs from an already-recorded entry for the same host instead of recording it alongside: russh's `check_known_hosts_path` reports a changed key only when an entry of the *same* algorithm holds a different key, so a host pinned only under other algorithms was indistinguishable from a first use, and a man-in-the-middle that advertised support for just an algorithm the real host had not used yet would have been trusted, since SSH negotiation picks the first client algorithm the server also supports. OpenSSH resists this by reordering its per-host key algorithm proposal to prefer already-known types, which russh cannot do per host, so accept-new now consults `known_host_keys_path` before recording and treats any existing entry for the host as a changed key. The changed-key remediation command also names the port-qualified `[host]:port` entry and, in the warning banner, the resolved known_hosts file (`ssh-keygen -f "" -R ""`, OpenSSH's own form), so it works for non-standard ports where the previous `ssh-keygen -R ` matched nothing; every printed argument is double quoted so an unmatched `[...]` glob does not make zsh reject the command. ### Fixed - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index 199a6beb..cc1354e9 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -620,6 +620,7 @@ mod tests { // untrusted endpoint (#239). let error = SshError::HostKeyChanged { host: "node1.example.com".to_string(), + port: 22, line: 3, }; assert!( diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index ecc0f41c..988fad00 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -49,9 +49,19 @@ fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> Option "Host key verification failed: the server's host key was not recognized or has changed" .to_string(), ), - crate::ssh::tokio_client::Error::HostKeyChanged { host, .. } => Some(format!( - "Possible man-in-the-middle attack: verify the server's new key out of band, or remove the old entry with 'ssh-keygen -R {host}' if the change is expected" - )), + crate::ssh::tokio_client::Error::HostKeyChanged { host, port, .. } => { + // Name the entry as known_hosts records it, so the command also + // works for non-standard ports, and double quote it so zsh does not + // reject the unmatched `[...]` glob. No `-f` here: this path has no + // known_hosts path to hand, and production always uses the default + // file, which `ssh-keygen` picks itself. + let entry = crate::ssh::tokio_client::host_verification::known_hosts_entry_name( + host, *port, + ); + Some(format!( + "Possible man-in-the-middle attack: verify the server's new key out of band, or remove the old entry with 'ssh-keygen -R \"{entry}\"' if the change is expected" + )) + } crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey) => Some( "The host is not in known_hosts and strict host key checking is enabled; connect once with '--strict-host-key-checking accept-new' or add the key manually" .to_string(), @@ -571,6 +581,12 @@ mod tests { crate::ssh::tokio_client::Error::AgentNoIdentities, crate::ssh::tokio_client::Error::HostKeyChanged { host: "node1.example.com".to_string(), + port: 22, + line: 3, + }, + crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + port: 2222, line: 3, }, crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey), @@ -590,12 +606,13 @@ mod tests { // which would render twice through anyhow's `{:#}` form (#239, #238). let e = crate::ssh::tokio_client::Error::HostKeyChanged { host: "node1.example.com".to_string(), + port: 22, line: 7, }; let cause_text = e.to_string(); let message = connect_error_message(&e).expect("HostKeyChanged adds guidance"); assert!( - message.contains("ssh-keygen -R node1.example.com"), + message.contains("ssh-keygen -R \"node1.example.com\""), "guidance must include the removal command, got: {message}" ); assert!( @@ -614,4 +631,26 @@ mod tests { "the conflicting line number must survive into the chain, got: {rendered}" ); } + + #[test] + fn test_connect_error_message_host_key_changed_names_port_qualified_entry() { + // known_hosts records a non-standard port as `[host]:port`, so + // `ssh-keygen -R host` would remove nothing and leave the user stuck. + // The guidance must name the entry that actually exists, quoted so the + // unmatched `[...]` glob does not make zsh reject the command. + let e = crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + port: 2222, + line: 7, + }; + let message = connect_error_message(&e).expect("HostKeyChanged adds guidance"); + assert!( + message.contains("ssh-keygen -R \"[node1.example.com]:2222\""), + "guidance must name the port-qualified entry, got: {message}" + ); + assert!( + !message.contains("has changed and no longer matches"), + "context must not restate the cause, got: {message}" + ); + } } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index 6a5eb435..b0570c68 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -746,9 +746,16 @@ fn format_ssh_error(context: &str, e: &crate::ssh::tokio_client::Error) -> Strin "{context} failed: Host key verification failed, the server's host key is not trusted" ) } - crate::ssh::tokio_client::Error::HostKeyChanged { host, .. } => { + crate::ssh::tokio_client::Error::HostKeyChanged { host, port, .. } => { + // Name the entry as known_hosts records it, so the command also + // works for non-standard ports, and double quote it so zsh does not + // reject the unmatched `[...]` glob. No `-f` here: this path has no + // known_hosts path to hand, and production always uses the default + // file, which `ssh-keygen` picks itself. + let entry = + crate::ssh::tokio_client::host_verification::known_hosts_entry_name(host, *port); format!( - "{context} failed: Possible man-in-the-middle attack, remove the old known_hosts entry with 'ssh-keygen -R {host}' only if the key change is expected" + "{context} failed: Possible man-in-the-middle attack, remove the old known_hosts entry with 'ssh-keygen -R \"{entry}\"' only if the key change is expected" ) } crate::ssh::tokio_client::Error::PasswordWrong => { @@ -829,6 +836,12 @@ mod tests { crate::ssh::tokio_client::Error::AgentAuthenticationFailed, crate::ssh::tokio_client::Error::HostKeyChanged { host: "node1.example.com".to_string(), + port: 22, + line: 3, + }, + crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + port: 2222, line: 3, }, ] { @@ -847,6 +860,7 @@ mod tests { // would render twice through anyhow's `{:#}` form (#239, #238). let e = crate::ssh::tokio_client::Error::HostKeyChanged { host: "node1.example.com".to_string(), + port: 22, line: 7, }; let cause_text = e.to_string(); @@ -854,7 +868,7 @@ mod tests { let detailed = format_ssh_error(&context, &e); assert!( - detailed.contains("ssh-keygen -R node1.example.com"), + detailed.contains("ssh-keygen -R \"node1.example.com\""), "guidance must include the removal command, got: {detailed}" ); assert!( @@ -870,6 +884,30 @@ mod tests { ); } + #[test] + fn test_format_ssh_error_host_key_changed_names_port_qualified_entry() { + // known_hosts records a non-standard port as `[host]:port`, so + // `ssh-keygen -R host` would remove nothing and leave the user stuck. + // The guidance must name the entry that actually exists, quoted so the + // unmatched `[...]` glob does not make zsh reject the command. + let e = crate::ssh::tokio_client::Error::HostKeyChanged { + host: "node1.example.com".to_string(), + port: 2222, + line: 7, + }; + let context = "SFTP upload to host:2222".to_string(); + + let detailed = format_ssh_error(&context, &e); + assert!( + detailed.contains("ssh-keygen -R \"[node1.example.com]:2222\""), + "guidance must name the port-qualified entry, got: {detailed}" + ); + assert!( + !detailed.contains("has changed and no longer matches"), + "context must not restate the cause, got: {detailed}" + ); + } + #[test] fn test_format_ssh_error_catch_all_does_not_duplicate_cause() { // The catch-all arm used to interpolate `{e}` directly into the diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index d3c17a8f..69f86bf1 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -550,6 +550,7 @@ impl Handler for ClientHandler { .map_err(|e| { super::host_verification::map_known_hosts_error( &self.hostname, + self.host.port(), server_public_key, known_hosts_path, e, @@ -559,10 +560,19 @@ impl Handler for ClientHandler { ServerCheckMethod::DefaultKnownHostsFile => { russh::keys::check_known_hosts(&self.hostname, self.host.port(), server_public_key) .map_err(|e| { + // The changed-key banner prints an `ssh-keygen -f + // ""` hint, so it needs the resolved path: a + // literal `~/.ssh/known_hosts` is not expanded inside + // the quotes the command requires. + let known_hosts_display = + crate::ssh::known_hosts::get_default_known_hosts_path() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_else(|| "~/.ssh/known_hosts".to_string()); super::host_verification::map_known_hosts_error( &self.hostname, + self.host.port(), server_public_key, - "~/.ssh/known_hosts", + &known_hosts_display, e, ) }) diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index 769621ef..8de29014 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -23,10 +23,18 @@ pub enum Error { CommandDidntExit, #[error("Server check failed")] ServerCheckFailed, + /// `port` is not interpolated into the `Display` text, but carrying it is + /// what lets the client-facing messages name the actual known_hosts entry + /// (`[host]:port` for non-standard ports) in their `ssh-keygen -R` + /// remediation hint. #[error( "Host key for '{host}' has changed and no longer matches the known_hosts entry at line {line}" )] - HostKeyChanged { host: String, line: usize }, + HostKeyChanged { + host: String, + port: u16, + line: usize, + }, #[error("SSH error occurred: {0}")] SshError(#[from] russh::Error), #[error("Send error")] diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index 7e3ca1c5..99d66767 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -47,10 +47,10 @@ static KNOWN_HOSTS_LOCK: Mutex<()> = Mutex::const_new(()); /// Verify a server key in accept-new (TOFU) mode against `known_hosts_path`. /// -/// Returns `Ok(true)` when the key matches an existing entry or the host was -/// unknown (in which case the key is recorded first). Returns -/// [`super::Error::HostKeyChanged`] when an entry for the host exists with a -/// different key, without modifying the file. +/// Returns `Ok(true)` when the key matches an existing entry or the host had no +/// entry at all (in which case the key is recorded first). Returns +/// [`super::Error::HostKeyChanged`] when any entry for the host already exists +/// and does not match the offered key, without modifying the file. pub(super) async fn verify_accept_new( hostname: &str, port: u16, @@ -61,15 +61,60 @@ pub(super) async fn verify_accept_new( match russh::keys::check_known_hosts_path(hostname, port, server_public_key, known_hosts_path) { Ok(true) => Ok(true), + // `check_known_hosts_path` reports `Err(KeyChanged)` only when an entry + // for the host exists *with the same key algorithm* but a different + // key. A hostname whose entries are all of some other algorithm + // therefore also lands here, indistinguishable from a hostname with no + // entry at all, so "unknown host" cannot be inferred from this arm + // alone; the entries for the host have to be looked at directly. Ok(false) => { - // Unknown host: trust on first use. A missing known_hosts file - // also lands here because russh treats an unreadable file as - // empty; `learn_known_hosts_path` creates it on demand. - record_host_key(hostname, port, server_public_key, known_hosts_path); - Ok(true) + match russh::keys::known_hosts::known_host_keys_path(hostname, port, known_hosts_path) { + // Unreadable or malformed known_hosts. `check_known_hosts_path` + // parses the same lines, so it would normally have failed first; + // failing closed here keeps the fallthrough from recording. + Err(e) => Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + e, + )), + Ok(existing) => match existing.first() { + // The host is pinned, just under a different key algorithm. + // Recording the offered key alongside the existing entry would + // hand an active man-in-the-middle a complete bypass of + // pinning: SSH negotiation selects the first host key + // algorithm on the client's list that the server also + // supports, so an attacker who advertises support for only an + // algorithm the real host has not used yet gets to choose that + // algorithm and would then be trusted on "first" use. OpenSSH + // resists this by reordering its host key algorithm proposal + // per host to prefer already-known types + // (`order_hostkeyalgs`), which russh cannot do per host, so the + // protection has to live in this check: an existing entry that + // does not match is treated as a changed key, reported against + // the first conflicting line. + Some(&(line, _)) => Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + russh::keys::Error::KeyChanged { line }, + )), + // Genuinely unknown host: trust on first use. A missing + // known_hosts file also lands here because russh treats an + // unreadable file as empty; `learn_known_hosts_path` creates it + // on demand. + None => { + record_host_key(hostname, port, server_public_key, known_hosts_path); + Ok(true) + } + }, + } } Err(e) => Err(map_known_hosts_error( hostname, + port, server_public_key, known_hosts_path, e, @@ -87,15 +132,23 @@ pub(super) async fn verify_accept_new( /// check methods so strict mode reports changed keys just as clearly. pub(super) fn map_known_hosts_error( hostname: &str, + port: u16, server_public_key: &PublicKey, known_hosts_display: &str, err: russh::keys::Error, ) -> super::Error { match err { russh::keys::Error::KeyChanged { line } => { - print_host_key_changed_warning(hostname, server_public_key, known_hosts_display, line); + print_host_key_changed_warning( + hostname, + port, + server_public_key, + known_hosts_display, + line, + ); super::Error::HostKeyChanged { host: hostname.to_string(), + port, line, } } @@ -151,7 +204,7 @@ fn record_host_key( /// 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. -fn known_hosts_entry_name(hostname: &str, port: u16) -> String { +pub(crate) fn known_hosts_entry_name(hostname: &str, port: u16) -> String { if port == 22 { hostname.to_string() } else { @@ -199,12 +252,20 @@ fn algorithm_display_name(key: &PublicKey) -> String { /// a remediation hint. The conflicting entry is never modified. fn print_host_key_changed_warning( hostname: &str, + port: u16, server_public_key: &PublicKey, known_hosts_display: &str, line: usize, ) { let algo = algorithm_display_name(server_public_key); let fingerprint = server_public_key.fingerprint(HashAlg::Sha256); + // The removal command names the entry as known_hosts actually records it, + // so it works for non-standard ports: `ssh-keygen -R hostname` matches + // nothing when the entry is `[hostname]:port`. Both arguments are double + // quoted because an unquoted `[host]:port` is an unmatched glob that zsh + // refuses to run ("no matches found"), and because the resolved + // known_hosts path may contain spaces. + let entry_name = known_hosts_entry_name(hostname, port); eprintln!( "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n\ @ WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED! @\n\ @@ -218,7 +279,7 @@ fn print_host_key_changed_warning( Add correct host key in {known_hosts_display} to get rid of this message.\n\ Offending key in {known_hosts_display}:{line}\n\ If the key change is expected, remove the old entry with:\n\ - \x20 ssh-keygen -R '{hostname}'" + \x20 ssh-keygen -f \"{known_hosts_display}\" -R \"{entry_name}\"" ); } @@ -236,6 +297,26 @@ mod tests { .expect("ed25519 key generation should not fail") } + /// A fixed RSA public key, used wherever a test needs a key of a *different* + /// algorithm than [`generate_key`]'s ED25519. + /// + /// This is a checked-in fixture rather than `PrivateKey::random(.., + /// Algorithm::Rsa { .. })` because ssh-key always generates 4096-bit RSA and + /// the pure-Rust prime search costs minutes in an unoptimized test build. + /// Only the public half is ever needed: both `verify_accept_new` and + /// `learn_known_hosts_path` take a `&PublicKey`. + const RSA_PUBLIC_KEY_FIXTURE: &str = "AAAAB3NzaC1yc2EAAAADAQABAAABAQDHQLu1Tz0J6aMlXcWUot3RKzgkfGen5V0tlCTDCmvUsqdkNZyKjbXLz725KrF8D4KZadci68LKgJ1oqyMKnjFRH40l3JMlNUQaWSo7wROStyax3cyJB+h//z9l8BB/6diq2JZk1UOl0DflsFtKc1p0KgmUhG6hY/Gu8CZQx8L1Y0N2SC1L4LRgx0gYvGt3MisAyvjl5Hah2d3GVi+PS9Jb2Ckmfrr4JQ3BEO0x4vhJWUGn2D1Nh5asTIvW/7v5k6DfkUWY8unQv5Wu/aEOC9NfuIWX8dS5mClvm8g8HVZ7gXW7zwvCq5a7cKn3IggMehzdTG1nN/dtLUCh3FTJt7iV"; + + fn rsa_public_key() -> PublicKey { + let key = russh::keys::parse_public_key_base64(RSA_PUBLIC_KEY_FIXTURE) + .expect("the RSA fixture must parse"); + assert!( + matches!(key.algorithm(), Algorithm::Rsa { .. }), + "the fixture must be an RSA key so it differs from generate_key()" + ); + key + } + /// Non-empty known_hosts lines. `learn_known_hosts_path` prefixes its /// first append to a fresh file with a newline, so blank lines are not /// entries. @@ -308,8 +389,9 @@ mod tests { let result = verify_accept_new("node1.example.com", 22, imposter.public_key(), &path_str).await; match result { - Err(Error::HostKeyChanged { host, line }) => { + Err(Error::HostKeyChanged { host, port, line }) => { assert_eq!(host, "node1.example.com"); + assert_eq!(port, 22); assert!(line > 0); } other => panic!("expected HostKeyChanged, got {other:?}"), @@ -322,6 +404,67 @@ mod tests { ); } + #[tokio::test] + async fn test_accept_new_rejects_alternate_algorithm_key_for_known_host() { + let (_dir, path, path_str) = temp_known_hosts(); + let pinned = generate_key(); + + let result = + verify_accept_new("node1.example.com", 22, pinned.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + let recorded = entry_lines(&path); + assert_eq!(recorded.len(), 1, "the ED25519 key must be pinned first"); + + // The host is pinned, just not with an RSA key, and russh's checker + // reports a changed key only when the algorithms match. This case used + // to be indistinguishable from a first use, so the offered key was + // appended next to the existing entry and accepted, which let an + // attacker who advertises support for only an algorithm the real host + // has not used yet bypass pinning entirely. + let result = verify_accept_new("node1.example.com", 22, &rsa_public_key(), &path_str).await; + match result { + Err(Error::HostKeyChanged { host, port, line }) => { + assert_eq!(host, "node1.example.com"); + assert_eq!(port, 22); + assert!(line > 0); + } + other => { + panic!("expected HostKeyChanged for an alternate-algorithm key, got {other:?}") + } + } + + assert_eq!( + entry_lines(&path), + recorded, + "no second entry may be appended for an already-pinned host" + ); + } + + #[tokio::test] + async fn test_changed_key_error_carries_connection_port() { + // The client-facing guidance messages build `ssh-keygen -R + // "[host]:port"` out of this port, so it has to be the port the + // connection actually used rather than a default 22. + let (_dir, _path, path_str) = temp_known_hosts(); + let original = generate_key(); + let imposter = generate_key(); + + let result = + verify_accept_new("node1.example.com", 2222, original.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + + let result = + verify_accept_new("node1.example.com", 2222, imposter.public_key(), &path_str).await; + match result { + Err(Error::HostKeyChanged { host, port, line }) => { + assert_eq!(host, "node1.example.com"); + assert_eq!(port, 2222, "the error must carry the connection port"); + assert!(line > 0); + } + other => panic!("expected HostKeyChanged, got {other:?}"), + } + } + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_accept_new_concurrent_first_connections_record_one_entry() { let (_dir, path, path_str) = temp_known_hosts(); @@ -428,13 +571,15 @@ mod tests { let err = map_known_hosts_error( "node1.example.com", + 22, key.public_key(), "/tmp/known_hosts", russh::keys::Error::KeyChanged { line: 7 }, ); match err { - Error::HostKeyChanged { host, line } => { + Error::HostKeyChanged { host, port, line } => { assert_eq!(host, "node1.example.com"); + assert_eq!(port, 22); assert_eq!(line, 7); } other => panic!("expected HostKeyChanged, got {other:?}"), @@ -442,6 +587,7 @@ mod tests { let err = map_known_hosts_error( "node1.example.com", + 22, key.public_key(), "/tmp/known_hosts", russh::keys::Error::KeyIsCorrupt, diff --git a/src/ssh/tokio_client/mod.rs b/src/ssh/tokio_client/mod.rs index 377bb004..0ca040a1 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -21,7 +21,9 @@ pub mod connection; mod connection_tests; pub mod error; pub mod file_transfer; -mod host_verification; +// `pub(crate)` so the client-facing error message builders in `ssh::client` can +// reuse `known_hosts_entry_name` instead of duplicating the `[host]:port` rule. +pub(crate) mod host_verification; mod to_socket_addrs_with_hostname; // Re-export public API types for backward compatibility From ca6425a119b20234a870381c0a715db757fbba8f Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 14:11:03 +0900 Subject: [PATCH 3/6] fix(security): accept a host key matching any recorded entry `russh::keys::check_known_hosts_path` maps every known_hosts line that matches the host and then gathers the per-line results with `collect::, _>>()`, which short-circuits on the first `Err(KeyChanged)`. A host with two or more entries of the same key algorithm therefore fails the check as soon as one of them is not the offered key, even when another entry holds exactly that key. Position does not matter: a correct entry on line 1 with a stale entry on line 2 errors just the same. OpenSSH's `check_key_in_hostkeys` does the opposite, returning HOST_OK if any recorded key for the host matches, and reporting HOST_CHANGED only when the host has keys of the same type and none of them match. Now that `accept-new` is the CLI default, that mismatch rejected legitimate, OpenSSH-accepted known_hosts layouts with the full "REMOTE HOST IDENTIFICATION HAS CHANGED" banner. Two realistic layouts trigger it: a stale host key kept beside a rotated one for the same host and algorithm, and a cluster-style file with a comma-separated `node1,node2,node3 ssh-ed25519 ` line plus a per-node `node2 ssh-ed25519 ` line. In the second case, connecting to node2 while it offered its own genuine key was rejected as a changed key, and the banner pointed at the shared line rather than at any entry that actually conflicted. A false man-in-the-middle alarm is a security problem in its own right: the banner's own remediation advice tells the user to run `ssh-keygen -R `, which deletes the legitimate pin and turns the next connection into a fresh trust-on-first-use that accepts whatever key is offered. `lookup_known_host` now reads the host's entries directly and applies OpenSSH's rule, so any recorded key equal to the offered key is a match and only a host with recorded keys none of which match counts as changed. Strict (`yes`) mode and `DefaultKnownHostsFile` route through the new `verify_known_hosts_file`, which shares that lookup, so every mode agrees on which keys verify and which count as changed. `DefaultKnownHostsFile` also resolves the path itself rather than letting russh resolve it with `std::env::home_dir()` while the banner text was built from `dirs::home_dir()`, closing a latent inconsistency where the check and the `ssh-keygen -f ""` hint could name different files. A missing home directory still fails closed, now as `ServerCheckFailed` directly instead of via russh's `NoHomeDir`. The offending line reported in the banner now prefers an entry with the same key algorithm as the offered key, since that is the entry OpenSSH would name, and falls back to the host's first entry so an algorithm-only conflict (the alternate-algorithm pinning bypass) still points at a real line. The unknown-host path also parses known_hosts once instead of twice while holding the process-wide lock, because the entries it needs in order to tell "unknown" from "changed" are the same ones the check already read. Four regression tests cover the layouts OpenSSH accepts (a key matching either of two same-algorithm entries, and a per-host key beside a shared cluster line), the same-algorithm offending line, and strict mode agreeing with accept-new. Validation: - cargo check --lib --tests (clean) - cargo test --lib ssh::tokio_client (24 passed, including 4 new) - cargo test --lib ssh::known_hosts (5 passed) - cargo test --lib ssh::client (20 passed) - cargo clippy --lib --tests -- -D warnings (clean) - cargo fmt --all --check (clean) Refs #239 --- src/ssh/tokio_client/connection.rs | 49 ++- src/ssh/tokio_client/host_verification.rs | 348 ++++++++++++++++++---- 2 files changed, 305 insertions(+), 92 deletions(-) diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index 69f86bf1..7df35319 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -541,41 +541,34 @@ impl Handler for ClientHandler { Ok(pk == *server_public_key) } ServerCheckMethod::KnownHostsFile(known_hosts_path) => { - russh::keys::check_known_hosts_path( + super::host_verification::verify_known_hosts_file( &self.hostname, self.host.port(), server_public_key, known_hosts_path, ) - .map_err(|e| { - super::host_verification::map_known_hosts_error( - &self.hostname, - self.host.port(), - server_public_key, - known_hosts_path, - e, - ) - }) } ServerCheckMethod::DefaultKnownHostsFile => { - russh::keys::check_known_hosts(&self.hostname, self.host.port(), server_public_key) - .map_err(|e| { - // The changed-key banner prints an `ssh-keygen -f - // ""` hint, so it needs the resolved path: a - // literal `~/.ssh/known_hosts` is not expanded inside - // the quotes the command requires. - let known_hosts_display = - crate::ssh::known_hosts::get_default_known_hosts_path() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_else(|| "~/.ssh/known_hosts".to_string()); - super::host_verification::map_known_hosts_error( - &self.hostname, - self.host.port(), - server_public_key, - &known_hosts_display, - e, - ) - }) + // Resolve the default path here rather than letting russh + // resolve it internally, so the check and the changed-key + // banner's `ssh-keygen -f ""` hint agree on one path and + // both modes share `verify_known_hosts_file`. No home + // directory means no trust state at all, so fail closed. + let Some(known_hosts_path) = + crate::ssh::known_hosts::get_default_known_hosts_path() + else { + tracing::error!( + "Cannot determine the known_hosts path; rejecting host key for '{}'", + self.hostname + ); + return Err(super::Error::ServerCheckFailed); + }; + super::host_verification::verify_known_hosts_file( + &self.hostname, + self.host.port(), + server_public_key, + &known_hosts_path.to_string_lossy(), + ) } ServerCheckMethod::AcceptNewKnownHostsFile(known_hosts_path) => { super::host_verification::verify_accept_new( diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index 99d66767..bf58a9c6 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -15,14 +15,18 @@ //! Trust On First Use (TOFU) host key verification. //! //! Implements the `accept-new` host key checking mode (#239): a host key that -//! matches its known_hosts entry is accepted, an unknown host is recorded in -//! the known_hosts file and accepted, and a key that conflicts with an existing -//! entry is rejected with an OpenSSH-style warning. The recording itself is -//! delegated to `russh::keys::known_hosts::learn_known_hosts_path`, which -//! handles entry formatting (including the `[host]:port` form for non-22 -//! ports) and file creation; this module adds the trust decision, restrictive -//! permissions on newly created files, and serialization of concurrent -//! first-time connections. +//! matches any key recorded for the host is accepted, an unknown host is +//! recorded in the known_hosts file and accepted, and a key offered for a host +//! that has recorded keys, none of which match, is rejected with an +//! OpenSSH-style warning. The recording itself is delegated to +//! `russh::keys::known_hosts::learn_known_hosts_path`, which handles entry +//! formatting (including the `[host]:port` form for non-22 ports) and file +//! creation; this module adds the trust decision, restrictive permissions on +//! newly created files, and serialization of concurrent first-time connections. +//! +//! The known_hosts lookup itself ([`lookup_known_host`]) also backs strict +//! (`yes`) mode through [`verify_known_hosts_file`], so both modes agree on +//! which keys verify and which count as changed. use russh::keys::{Algorithm, HashAlg, PublicKey}; use std::path::Path; @@ -45,12 +49,66 @@ use tokio::sync::Mutex; /// blocking runtime worker threads. static KNOWN_HOSTS_LOCK: Mutex<()> = Mutex::const_new(()); +/// The outcome of looking a host up in a known_hosts file. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KnownHostLookup { + /// One of the host's recorded keys is the offered key. + Match, + /// The host has recorded keys and none of them is the offered key. + /// `line` is the entry to report as the offending one. + Conflict { line: usize }, + /// The host has no recorded keys at all. + Unknown, +} + +/// Look `hostname`/`port` up in `known_hosts_path` using OpenSSH's matching +/// rule: the host is trusted when *any* key recorded for it equals the offered +/// key, and only a host that has recorded keys with none matching counts as +/// changed. +/// +/// This replaces `russh::keys::check_known_hosts_path`, which collects its +/// per-line results with `collect::, _>>()` and so reports +/// `Err(KeyChanged)` as soon as *any* same-algorithm entry for the host +/// differs, even when another entry holds exactly the offered key. A host that +/// legitimately carries several keys of one algorithm (a stale entry kept +/// beside a rotated one, or a comma-separated cluster line beside a per-node +/// line) would otherwise be rejected with the man-in-the-middle banner where +/// OpenSSH's `check_key_in_hostkeys` accepts. Reading the entries directly also +/// parses the file once instead of twice on the unknown-host path. +fn lookup_known_host( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) -> Result { + let recorded = + russh::keys::known_hosts::known_host_keys_path(hostname, port, known_hosts_path)?; + + if recorded.iter().any(|(_, key)| key == server_public_key) { + return Ok(KnownHostLookup::Match); + } + + // Report the line of an entry with the same key algorithm when there is + // one, since that is the entry OpenSSH would call the offending key. Fall + // back to the host's first entry so an algorithm-only conflict (the + // alternate-algorithm pinning bypass) still points at a real line. + let offending = recorded + .iter() + .find(|(_, key)| key.algorithm() == server_public_key.algorithm()) + .or_else(|| recorded.first()); + + Ok(match offending { + Some(&(line, _)) => KnownHostLookup::Conflict { line }, + None => KnownHostLookup::Unknown, + }) +} + /// Verify a server key in accept-new (TOFU) mode against `known_hosts_path`. /// -/// Returns `Ok(true)` when the key matches an existing entry or the host had no -/// entry at all (in which case the key is recorded first). Returns -/// [`super::Error::HostKeyChanged`] when any entry for the host already exists -/// and does not match the offered key, without modifying the file. +/// Returns `Ok(true)` when the key matches any of the host's recorded keys, or +/// when the host has no recorded keys at all (in which case the key is recorded +/// first). Returns [`super::Error::HostKeyChanged`] when the host has recorded +/// keys and none of them matches the offered key, without modifying the file. pub(super) async fn verify_accept_new( hostname: &str, port: u16, @@ -59,59 +117,65 @@ pub(super) async fn verify_accept_new( ) -> Result { let _guard = KNOWN_HOSTS_LOCK.lock().await; - match russh::keys::check_known_hosts_path(hostname, port, server_public_key, known_hosts_path) { - Ok(true) => Ok(true), - // `check_known_hosts_path` reports `Err(KeyChanged)` only when an entry - // for the host exists *with the same key algorithm* but a different - // key. A hostname whose entries are all of some other algorithm - // therefore also lands here, indistinguishable from a hostname with no - // entry at all, so "unknown host" cannot be inferred from this arm - // alone; the entries for the host have to be looked at directly. - Ok(false) => { - match russh::keys::known_hosts::known_host_keys_path(hostname, port, known_hosts_path) { - // Unreadable or malformed known_hosts. `check_known_hosts_path` - // parses the same lines, so it would normally have failed first; - // failing closed here keeps the fallthrough from recording. - Err(e) => Err(map_known_hosts_error( - hostname, - port, - server_public_key, - known_hosts_path, - e, - )), - Ok(existing) => match existing.first() { - // The host is pinned, just under a different key algorithm. - // Recording the offered key alongside the existing entry would - // hand an active man-in-the-middle a complete bypass of - // pinning: SSH negotiation selects the first host key - // algorithm on the client's list that the server also - // supports, so an attacker who advertises support for only an - // algorithm the real host has not used yet gets to choose that - // algorithm and would then be trusted on "first" use. OpenSSH - // resists this by reordering its host key algorithm proposal - // per host to prefer already-known types - // (`order_hostkeyalgs`), which russh cannot do per host, so the - // protection has to live in this check: an existing entry that - // does not match is treated as a changed key, reported against - // the first conflicting line. - Some(&(line, _)) => Err(map_known_hosts_error( - hostname, - port, - server_public_key, - known_hosts_path, - russh::keys::Error::KeyChanged { line }, - )), - // Genuinely unknown host: trust on first use. A missing - // known_hosts file also lands here because russh treats an - // unreadable file as empty; `learn_known_hosts_path` creates it - // on demand. - None => { - record_host_key(hostname, port, server_public_key, known_hosts_path); - Ok(true) - } - }, - } + match lookup_known_host(hostname, port, server_public_key, known_hosts_path) { + Ok(KnownHostLookup::Match) => Ok(true), + // The host is pinned and the offered key is not one of its recorded + // keys. That covers both a genuine key change and a key offered under + // an algorithm the host has not used yet: recording the latter + // alongside the existing entry would hand an active man-in-the-middle a + // complete bypass of pinning, because SSH negotiation lets the server + // steer the choice of host key algorithm and OpenSSH's per-host + // reordering of that proposal (`order_hostkeyalgs`) is not available + // through russh. + Ok(KnownHostLookup::Conflict { line }) => Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + russh::keys::Error::KeyChanged { line }, + )), + // Genuinely unknown host: trust on first use. A missing known_hosts + // file also lands here because russh treats an unopenable file as + // empty; `learn_known_hosts_path` creates it on demand. + Ok(KnownHostLookup::Unknown) => { + record_host_key(hostname, port, server_public_key, known_hosts_path); + Ok(true) } + // Unreadable or malformed known_hosts: fail closed rather than record. + Err(e) => Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + e, + )), + } +} + +/// Verify a server key against `known_hosts_path` without recording anything, +/// for strict (`yes`) mode. +/// +/// Returns `Ok(false)` for a host with no recorded keys so the caller's own +/// unknown-host rejection applies, `Ok(true)` when one of the host's recorded +/// keys is the offered key, and [`super::Error::HostKeyChanged`] when the host +/// is pinned and none of its keys match. Shares [`lookup_known_host`] with +/// accept-new so both modes agree on what "changed" means. +pub(super) fn verify_known_hosts_file( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) -> Result { + match lookup_known_host(hostname, port, server_public_key, known_hosts_path) { + Ok(KnownHostLookup::Match) => Ok(true), + Ok(KnownHostLookup::Unknown) => Ok(false), + Ok(KnownHostLookup::Conflict { line }) => Err(map_known_hosts_error( + hostname, + port, + server_public_key, + known_hosts_path, + russh::keys::Error::KeyChanged { line }, + )), Err(e) => Err(map_known_hosts_error( hostname, port, @@ -440,6 +504,133 @@ mod tests { ); } + #[tokio::test] + async fn test_accept_new_accepts_key_matching_any_recorded_entry() { + let (_dir, path, path_str) = temp_known_hosts(); + let stale = generate_key(); + let current = generate_key(); + + // Two ED25519 entries for one host: a stale key kept beside the rotated + // one. OpenSSH's `check_key_in_hostkeys` returns HOST_OK as soon as + // *any* key recorded for the host matches, so both of these keys + // verify. russh's `check_known_hosts_path` instead collects its + // per-line results with `collect::, _>>()`, so a + // non-matching same-algorithm entry short-circuits the whole check into + // `Err(KeyChanged)` no matter which line holds the matching key. That + // is why the entries are looked up directly here. + std::fs::write( + &path, + format!( + "node1.example.com {}\nnode1.example.com {}\n", + stale.public_key().to_openssh().unwrap(), + current.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + let recorded = entry_lines(&path); + assert_eq!(recorded.len(), 2, "the fixture must hold two entries"); + + let result = + verify_accept_new("node1.example.com", 22, current.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "a key matching the second entry must be accepted, got {result:?}" + ); + assert_eq!( + entry_lines(&path), + recorded, + "an already-recorded key must not be appended again" + ); + + // The stale key is still a recorded key for the host, so OpenSSH keeps + // accepting it until the operator removes that entry. + let result = + verify_accept_new("node1.example.com", 22, stale.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "a key matching the first entry must be accepted, got {result:?}" + ); + assert_eq!(entry_lines(&path), recorded); + } + + #[tokio::test] + async fn test_accept_new_accepts_per_host_key_beside_shared_cluster_entry() { + let (_dir, path, path_str) = temp_known_hosts(); + let shared = generate_key(); + let own = generate_key(); + + // Cluster-style known_hosts: one comma-separated line pins a shared key + // for every node, and a later line pins node2's own key. OpenSSH + // matches the host against both lines and accepts either key. The old + // `check_known_hosts_path` call rejected this layout with the + // man-in-the-middle banner, and pointed at the shared line rather than + // at any entry that actually conflicted. + std::fs::write( + &path, + format!( + "node1,node2,node3 {}\nnode2 {}\n", + shared.public_key().to_openssh().unwrap(), + own.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + let recorded = entry_lines(&path); + + let result = verify_accept_new("node2", 22, own.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "node2's own pinned key must be accepted, got {result:?}" + ); + assert_eq!( + entry_lines(&path), + recorded, + "nothing may be appended for an already-pinned host" + ); + } + + #[tokio::test] + async fn test_accept_new_conflict_reports_same_algorithm_line() { + let (_dir, path, path_str) = temp_known_hosts(); + let stale = generate_key(); + let imposter = generate_key(); + + // An RSA entry precedes the stale ED25519 one. OpenSSH names the key of + // the same type as the offending one, so the banner must point at line + // 2 rather than at the host's first entry, which is a key the operator + // would then be told to delete for no reason. + std::fs::write( + &path, + format!( + "node1.example.com {}\nnode1.example.com {}\n", + rsa_public_key().to_openssh().unwrap(), + stale.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + let recorded = entry_lines(&path); + assert_eq!(recorded.len(), 2, "the fixture must hold two entries"); + + let result = + verify_accept_new("node1.example.com", 22, imposter.public_key(), &path_str).await; + match result { + Err(Error::HostKeyChanged { host, port, line }) => { + assert_eq!(host, "node1.example.com"); + assert_eq!(port, 22); + assert_eq!( + line, 2, + "the same-algorithm entry must be reported as the offending one" + ); + } + other => panic!("expected HostKeyChanged, got {other:?}"), + } + + assert_eq!( + entry_lines(&path), + recorded, + "a conflicting key must not be recorded" + ); + } + #[tokio::test] async fn test_changed_key_error_carries_connection_port() { // The client-facing guidance messages build `ssh-keygen -R @@ -654,6 +845,35 @@ mod tests { assert!(matches!(result, Ok(true))); } + #[tokio::test] + async fn test_strict_mode_accepts_key_matching_any_recorded_entry() { + let (_dir, path, path_str) = temp_known_hosts(); + let stale = generate_key(); + let current = generate_key(); + + // Strict mode shares the lookup with accept-new, so both modes agree on + // what "changed" means. A stale ED25519 entry kept beside the rotated + // one is a layout OpenSSH accepts, and short-circuiting on the + // non-matching same-algorithm line would reject the connection here just + // as it did in accept-new. + std::fs::write( + &path, + format!( + "node1.example.com {}\nnode1.example.com {}\n", + stale.public_key().to_openssh().unwrap(), + current.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); + let result = handler.check_server_key(current.public_key()).await; + assert!( + matches!(result, Ok(true)), + "strict mode must accept a key matching any recorded entry, got {result:?}" + ); + } + #[tokio::test] async fn test_no_check_accepts_without_touching_filesystem() { let (_dir, path, _path_str) = temp_known_hosts(); From 5590918dedca4e8ab0c3c08a91b33bca8c8ae719 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 14:18:03 +0900 Subject: [PATCH 4/6] fix(security): reject hostnames unrepresentable in known_hosts `russh::keys::known_hosts::learn_known_hosts_path` appends the entry as `{host} ` (or `[{host}]:{port} `) applying no escaping and no validation to the hostname, while russh's own parser splits each line on `' '`, treats `,` as the host-list separator, skips any line whose first byte is `#`, and reads a leading `|1|` as a hashed host field. Before this branch nothing was ever written to known_hosts, so the hostname string never reached a file; now that accept-new records it, a hostname carrying any of those characters no longer round-trips. Nothing on the way to `ClientHandler` validates the hostname. `security::validate_hostname` is the only validator that rejects whitespace, newlines and `#`, it is reached only from the `-H/--hosts` branch, and its result is then discarded as the connect hostname anyway because `app::nodes` overwrites it with the unvalidated `~/.ssh/config` `HostName`. `Config::from_backendai_env` builds the node list from the platform-supplied `BACKENDAI_CLUSTER_HOSTS` variable with only `str::trim()` and a non-empty check, which leaves interior spaces, tabs, newlines and `#` intact, and that source outranks every config file with no `-c` flag. YAML cluster hosts, `^hostfile` lines and `expand_env_vars` splicing get no character validation either. `AuthContext::new` is the only check on the exec, SFTP and interactive paths and rejects only NUL, LF and CR, not space, tab, other control characters, `#` or `,`. The port-forwarding path in `commands::exec` builds its `AuthMethod` inline without `AuthContext` at all, so it has no hostname check whatsoever, and that is exactly the path this branch newly gave recording powers. Four concrete consequences, all reproduced end to end by driving `ClientHandler::check_server_key` with `AcceptNewKnownHostsFile`: a hostname containing a newline writes two lines, so the second one pins the key the server just offered for a host the user never named, and every later connection to that host either accepts the attacker's key or is refused as a changed key; a hostname containing a space writes a key field that `parse_public_key_base64` cannot parse, so the lookup returns `Err` forever and the host becomes permanently unconnectable behind a generic "Server check failed"; a hostname starting with `#` writes a line the parser skips, so every connection is a fresh first use that accepts whatever key is offered and appends another entry, growing the file without bound; and a hostname containing `,` writes a host-list entry that silently pins one server's key for every name in the list. The guard `ensure_recordable_hostname` therefore lives in `ssh::tokio_client::host_verification` at the point of use rather than in any caller, so it covers every connection path regardless of which caller skipped validation. It rejects an empty or over-long hostname, any whitespace or control character, and `#`, `,`, `|`, `*`, `?`, `!` and `\` (the last four because the file is shared with OpenSSH, whose pattern matching would read such an entry as covering hosts it was never meant to). Both `verify_accept_new` and `verify_known_hosts_file` call it first, so strict mode reports the real reason instead of a misleading "unknown host" and the two modes stay consistent. It fails closed with `ServerCheckFailed`: a hostname that cannot be recorded cannot be verified on any later connection either, so connecting unverified would be the worse outcome. Tests cover the newline, space, tab, `#`, `,`, `|`, `*`, empty and over-long cases (asserting nothing is written), and a regression guard proves ordinary names, IPv4, IPv6 literals, zone identifiers and the bracketed `[::1]` form still record exactly one entry. Verified with `cargo check --lib --tests`, `cargo clippy --lib --tests -- -D warnings`, `cargo fmt --all --check`, and the `ssh::tokio_client` (27), `ssh::known_hosts` (5), `ssh::client` (20), `jump::chain` (17) and `commands::interactive` (19) library test suites. Refs #239 --- src/ssh/tokio_client/host_verification.rs | 183 +++++++++++++++++++++- 1 file changed, 182 insertions(+), 1 deletion(-) diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index bf58a9c6..3099cd81 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -103,18 +103,86 @@ fn lookup_known_host( }) } +/// Longest hostname accepted in a known_hosts entry. A DNS name cannot exceed +/// 253 characters, so this only bounds the line length against a hostname that +/// could never have resolved anyway. +const MAX_KNOWN_HOSTS_HOSTNAME_LEN: usize = 255; + +/// Reject a hostname that cannot be represented as a known_hosts entry. +/// +/// `learn_known_hosts_path` writes `{host} ` (or `[{host}]:{port} `) +/// with no escaping, and russh's parser splits entries on `' '`, reads `,` as +/// the host-list separator, skips any line whose first byte is `#`, and reads a +/// leading `|1|` as a hashed host field. A hostname carrying any of those, or +/// any whitespace or control character, therefore does not round-trip: it can +/// forge a second entry that pins the offered key for a host the user never +/// named, silently pin one key for a whole list of hosts, or write a line that +/// can never match again, in which case every later connection is a fresh first +/// use that accepts any key and appends yet another entry. +/// +/// Nothing validates the hostname on the way here. `validate_hostname` is +/// reached only from `-H/--hosts` and is then overwritten by the unvalidated +/// `~/.ssh/config` `HostName`; `Config::from_backendai_env` only trims the +/// platform-supplied `BACKENDAI_CLUSTER_HOSTS` value; `AuthContext::new` +/// rejects only NUL, LF and CR; and the port-forwarding path in +/// `commands::exec` skips `AuthContext` entirely. So the check belongs at the +/// point of use, where every connection path passes through. +/// +/// A hostname that cannot be recorded also cannot be verified on any later +/// connection, so this fails closed rather than connecting unverified. +fn ensure_recordable_hostname(hostname: &str) -> Result<(), super::Error> { + // `*`, `?` and `!` are OpenSSH known_hosts pattern metacharacters. russh + // does not implement patterns, but the file is shared with OpenSSH, which + // would read such an entry as matching hosts it was never meant to. + const REJECTED: [char; 7] = ['#', ',', '|', '*', '?', '!', '\\']; + + let problem = if hostname.is_empty() { + Some("it is empty") + } else if hostname.len() > MAX_KNOWN_HOSTS_HOSTNAME_LEN { + Some("it is too long") + } else if hostname + .chars() + .any(|c| c.is_whitespace() || c.is_control()) + { + Some("it contains whitespace or a control character") + } else if hostname.contains(REJECTED) { + Some("it contains a known_hosts metacharacter") + } else { + None + }; + + match problem { + None => Ok(()), + Some(problem) => { + tracing::error!( + "Refusing to verify host key: hostname is not representable in known_hosts because {problem}" + ); + eprintln!( + "Host key verification failed: the hostname cannot be recorded in known_hosts because {problem}" + ); + Err(super::Error::ServerCheckFailed) + } + } +} + /// Verify a server key in accept-new (TOFU) mode against `known_hosts_path`. /// /// Returns `Ok(true)` when the key matches any of the host's recorded keys, or /// when the host has no recorded keys at all (in which case the key is recorded /// first). Returns [`super::Error::HostKeyChanged`] when the host has recorded /// keys and none of them matches the offered key, without modifying the file. +/// A hostname that cannot be represented in a known_hosts entry is rejected +/// outright, before anything is looked up or written. pub(super) async fn verify_accept_new( hostname: &str, port: u16, server_public_key: &PublicKey, known_hosts_path: &str, ) -> Result { + // Checked before the lock is taken: serializing a hostname that is already + // rejected would only make every other connection wait on it. + ensure_recordable_hostname(hostname)?; + let _guard = KNOWN_HOSTS_LOCK.lock().await; match lookup_known_host(hostname, port, server_public_key, known_hosts_path) { @@ -159,13 +227,19 @@ pub(super) async fn verify_accept_new( /// unknown-host rejection applies, `Ok(true)` when one of the host's recorded /// keys is the offered key, and [`super::Error::HostKeyChanged`] when the host /// is pinned and none of its keys match. Shares [`lookup_known_host`] with -/// accept-new so both modes agree on what "changed" means. +/// accept-new so both modes agree on what "changed" means. A hostname that +/// cannot be represented in a known_hosts entry is rejected outright: strict +/// mode never records, but such a hostname could not match a well-formed entry +/// either, so this only replaces a misleading "unknown host" with the real +/// reason. pub(super) fn verify_known_hosts_file( hostname: &str, port: u16, server_public_key: &PublicKey, known_hosts_path: &str, ) -> Result { + ensure_recordable_hostname(hostname)?; + match lookup_known_host(hostname, port, server_public_key, known_hosts_path) { Ok(KnownHostLookup::Match) => Ok(true), Ok(KnownHostLookup::Unknown) => Ok(false), @@ -896,4 +970,111 @@ mod tests { assert!(matches!(result, Ok(true))); assert_eq!(entry_lines(&path).len(), 1); } + + // Hostnames that cannot be written back out as a known_hosts entry. + + #[tokio::test] + async fn test_accept_new_rejects_hostnames_that_cannot_be_recorded() { + let key = generate_key(); + let over_long = "a".repeat(300); + + // `learn_known_hosts_path` writes the hostname into the entry with no + // escaping at all, and russh's parser splits on ' ', treats ',' as the + // host-list separator, skips any line starting with '#', and reads a + // leading '|1|' as a hashed host. None of these hostnames survives that + // round trip, so recording one is worse than refusing to connect. + let unrecordable = [ + // The dangerous one: the embedded newline ends the forged entry and + // starts a second line, so the server gets its own key pinned for + // `[node1]:2200`, a host the user never named. Every later + // connection to node1 then trusts the attacker's key or is refused + // as a changed key. + "victim]:2200 ssh-ed25519 AAAA\n[node1", + // A space makes russh read `plaintext` as the key field, which then + // fails to parse, so the host stays permanently unconnectable with + // only a generic "Server check failed". + "evil.example.com plaintext", + "evil.example.com\tplaintext", + // russh skips a line whose first byte is '#', so the entry can + // never match again: every connection is a fresh first use that + // accepts whatever key is offered and appends yet another entry, + // growing the file without bound. + "#node1.example.com", + // A comma writes a host list, silently pinning one server's key for + // every name in it. + "node1,node2", + // A leading '|' is how russh spells a hashed host field. + "node1|node2", + // OpenSSH reads this as a pattern and would match hosts the entry + // was never meant to cover. + "*.example.com", + "", + over_long.as_str(), + ]; + + for hostname in unrecordable { + let (_dir, path, path_str) = temp_known_hosts(); + let result = verify_accept_new(hostname, 2200, key.public_key(), &path_str).await; + assert!( + matches!(result, Err(Error::ServerCheckFailed)), + "hostname {hostname:?} must be refused, got {result:?}" + ); + assert!( + !path.exists(), + "hostname {hostname:?} must not cause anything to be written" + ); + } + } + + #[tokio::test] + async fn test_accept_new_still_records_normal_and_ipv6_hostnames() { + let key = generate_key(); + + // Regression guard for the rejection set above: IPv6 literals, zone + // identifiers, the bracketed form and ordinary DNS names all have to + // keep working, so the character set may not grow past what actually + // breaks the known_hosts round trip. + for hostname in [ + "node1.example.com", + "host-1.sub.example.com", + "10.0.0.1", + "::1", + "fe80::1%eth0", + "[::1]", + ] { + let (_dir, path, path_str) = temp_known_hosts(); + let result = verify_accept_new(hostname, 22, key.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "hostname {hostname:?} must still be accepted, got {result:?}" + ); + assert_eq!( + entry_lines(&path).len(), + 1, + "hostname {hostname:?} must record exactly one entry" + ); + } + } + + #[tokio::test] + async fn test_strict_mode_rejects_hostname_that_cannot_be_recorded() { + // Strict mode never records, so it cannot be poisoned, but it shares + // the guard so both modes fail for the same stated reason instead of + // reporting a misleading "not in known_hosts" for a hostname that + // could never have matched a well-formed entry anyway. + let (_dir, _path, path_str) = temp_known_hosts(); + let key = generate_key(); + + // `handler_for` hardcodes a valid hostname, so build the handler here. + let mut handler = ClientHandler::new( + "evil.example.com plaintext".to_string(), + "127.0.0.1:22".parse().unwrap(), + ServerCheckMethod::KnownHostsFile(path_str), + ); + let result = handler.check_server_key(key.public_key()).await; + assert!( + matches!(result, Err(Error::ServerCheckFailed)), + "strict mode must refuse an unrecordable hostname, got {result:?}" + ); + } } From 8aa462724f0ca55cf276d032011d475e464b3ad1 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 14:40:58 +0900 Subject: [PATCH 5/6] fix(security): honor known_hosts markers, case, and permission gaps Two review passes on the accept-new TOFU implementation reported three remaining gaps that were not yet fixed: known_hosts `@revoked`/`@cert-authority` marker lines were silently ignored, host matching was case-sensitive where OpenSSH is not, and known_hosts permission tightening happened after creation instead of at creation. russh's `known_host_keys_path` splits each line on `' '` and takes the first field as the host list, so on a marker line such as `@revoked node1 ssh-ed25519 ` that first field is the literal string `@revoked`, which never matches `node1`. The line was therefore invisible to the lookup, and a revoked key looked like an ordinary first use: recorded and accepted while printing the normal "Permanently added" notice. A new scan in `host_verification.rs` runs before the ordinary lookup in both accept-new and strict (`yes`)/`DefaultKnownHostsFile` mode: a `@revoked` line whose host matches (exact, comma-list, or `*`/`?` glob form, erring toward matching when in doubt since failing to honor a revocation is worse than an unnecessary rejection) and whose key equals the offered key is a hard rejection with a new `HostKeyRevoked` error, while a matching `@revoked` line naming a different key does not block the offered one. A `@cert-authority` match only warns and falls through to ordinary TOFU, since bssh has no CA signature validation path and failing closed would break every working CA setup with no workaround. Hostnames are now lowercased once at entry to `verify_accept_new` and `verify_known_hosts_file`, and the normalized value is used for both the lookup and the recorded entry. russh's `match_hostname` is a plain byte compare, so without this a differently-cased hostname bypassed an existing pin and re-TOFU'd instead of hitting the same entry OpenSSH would have written. `~/.ssh` and `known_hosts` are now created with mode 0700/0600 directly via `DirBuilder`/`OpenOptions` (`#[cfg(unix)]`, with a `#[cfg(not(unix))]` fallback) before `learn_known_hosts_path` runs, instead of letting it create them with the process umask and tightening the mode only afterward. A `mkdir`/`open` syscall that requests the final mode directly cannot be widened by any standard umask, since umask only clears requested bits and neither 0700 nor 0600 carries a group or other bit to clear, so this closes the window instead of narrowing it after the fact. Pre-existing files and directories are left untouched either way, and the prior post-creation chmod is kept as a fallback for the rare case where pre-creation could not run (for example a missing grandparent directory that only `learn_known_hosts_path`'s `create_dir_all` fills in). `HostKeyRevoked` gets its own guidance arms in `connect_error_message` and `format_ssh_error`, verified against both #240 invariants (no restating the cause, no trailing period), and a regression test confirms it does not trigger interactive password fallback, matching `HostKeyChanged`. Validation: - cargo check --lib --tests (clean) - cargo clippy --lib --tests -- -D warnings (clean) - cargo fmt --all -- --check (clean) - cargo test --lib ssh::tokio_client (36 passed, 9 new: revoked matching/non-matching, cert-authority, comma-list/glob/non-standard-port marker matching, case-insensitive hostname matching for accept-new and strict mode) - cargo test --lib ssh::client (22 passed, 2 new guidance-invariant tests) - cargo test --lib commands::interactive (20 passed, 1 new password-fallback exclusion test) - cargo test --lib ssh::known_hosts (5 passed) and cargo test --lib jump::chain (17 passed) Refs #239 --- src/commands/interactive/connection.rs | 15 + src/ssh/client/connection.rs | 42 ++ src/ssh/client/file_transfer.rs | 44 ++ src/ssh/tokio_client/error.rs | 13 + src/ssh/tokio_client/host_verification.rs | 573 +++++++++++++++++++++- 5 files changed, 677 insertions(+), 10 deletions(-) diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index cc1354e9..2e71f91f 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -629,6 +629,21 @@ mod tests { ); } + #[test] + fn test_host_key_revoked_does_not_trigger_fallback() { + // Same reasoning as HostKeyChanged: a revoked host key is a possible + // man-in-the-middle, not an auth problem (#239). + let error = SshError::HostKeyRevoked { + host: "node1.example.com".to_string(), + port: 22, + line: 3, + }; + assert!( + !is_auth_error_for_password_fallback(&error), + "HostKeyRevoked should NOT trigger password fallback (host key issue)" + ); + } + #[test] fn test_io_error_does_not_trigger_fallback() { let error = SshError::IoError(std::io::Error::new( diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index 988fad00..56be3344 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -62,6 +62,14 @@ fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> Option "Possible man-in-the-middle attack: verify the server's new key out of band, or remove the old entry with 'ssh-keygen -R \"{entry}\"' if the change is expected" )) } + // Unlike `HostKeyChanged`, there is no `ssh-keygen -R` remediation to + // offer: the marker was placed deliberately to blocklist this exact + // key, and the entry it points at is the `@revoked` line itself, not + // a stale pin to remove. + crate::ssh::tokio_client::Error::HostKeyRevoked { .. } => Some( + "The offered host key matches a key explicitly revoked in known_hosts; do not connect unless you can independently verify the server's identity out of band" + .to_string(), + ), crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey) => Some( "The host is not in known_hosts and strict host key checking is enabled; connect once with '--strict-host-key-checking accept-new' or add the key manually" .to_string(), @@ -589,6 +597,11 @@ mod tests { port: 2222, line: 3, }, + crate::ssh::tokio_client::Error::HostKeyRevoked { + host: "node1.example.com".to_string(), + port: 22, + line: 3, + }, crate::ssh::tokio_client::Error::SshError(russh::Error::UnknownKey), ] { let message = connect_error_message(&e).expect("variant adds guidance"); @@ -653,4 +666,33 @@ mod tests { "context must not restate the cause, got: {message}" ); } + + #[test] + fn test_connect_error_message_host_key_revoked_adds_guidance_without_echo() { + // Same invariant as HostKeyChanged: the context layer adds guidance + // without restating the cause's own wording, which would otherwise + // render twice through anyhow's `{:#}` form (#239, #238). + let e = crate::ssh::tokio_client::Error::HostKeyRevoked { + host: "node1.example.com".to_string(), + port: 22, + line: 7, + }; + let cause_text = e.to_string(); + let message = connect_error_message(&e).expect("HostKeyRevoked adds guidance"); + assert!( + message.contains("explicitly revoked"), + "guidance must warn about revocation, got: {message}" + ); + assert!( + !message.contains("is explicitly revoked by the known_hosts entry at line"), + "context must not restate the cause, got: {message}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(message)); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + } } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index b0570c68..7fea6a01 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -758,6 +758,14 @@ fn format_ssh_error(context: &str, e: &crate::ssh::tokio_client::Error) -> Strin "{context} failed: Possible man-in-the-middle attack, remove the old known_hosts entry with 'ssh-keygen -R \"{entry}\"' only if the key change is expected" ) } + // No `ssh-keygen -R` remediation here, unlike `HostKeyChanged`: the + // entry that matched is the `@revoked` marker line placed + // deliberately to blocklist this exact key, not a stale pin. + crate::ssh::tokio_client::Error::HostKeyRevoked { .. } => { + format!( + "{context} failed: The offered host key is explicitly revoked in known_hosts, refusing to connect" + ) + } crate::ssh::tokio_client::Error::PasswordWrong => { format!("{context} failed: Password authentication rejected") } @@ -844,6 +852,11 @@ mod tests { port: 2222, line: 3, }, + crate::ssh::tokio_client::Error::HostKeyRevoked { + host: "node1.example.com".to_string(), + port: 22, + line: 3, + }, ] { let detailed = format_ssh_error(&context, &e); assert!( @@ -908,6 +921,37 @@ mod tests { ); } + #[test] + fn test_format_ssh_error_host_key_revoked_adds_guidance_without_echo() { + // Same invariant as HostKeyChanged: guidance without restating the + // cause's own wording, which would render twice through anyhow's + // `{:#}` form (#239, #238). + let e = crate::ssh::tokio_client::Error::HostKeyRevoked { + host: "node1.example.com".to_string(), + port: 22, + line: 7, + }; + let cause_text = e.to_string(); + let context = "SFTP upload to host:22".to_string(); + + let detailed = format_ssh_error(&context, &e); + assert!( + detailed.contains("explicitly revoked"), + "guidance must warn about revocation, got: {detailed}" + ); + assert!( + !detailed.contains("is explicitly revoked by the known_hosts entry at line"), + "context must not restate the cause, got: {detailed}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(detailed)); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + } + #[test] fn test_format_ssh_error_catch_all_does_not_duplicate_cause() { // The catch-all arm used to interpolate `{e}` directly into the diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index 8de29014..ff7b65cf 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -35,6 +35,19 @@ pub enum Error { port: u16, line: usize, }, + /// The offered key exactly matches a key an operator explicitly + /// blocklisted with a known_hosts `@revoked` marker line. `port` is + /// carried for the same reason `HostKeyChanged` carries it (naming the + /// actual known_hosts entry in client-facing guidance), but unlike a + /// changed key there is no `ssh-keygen -R` remediation to suggest: the + /// marker was placed deliberately, and removing it would silence the + /// warning rather than fix anything. + #[error("Host key for '{host}' is explicitly revoked by the known_hosts entry at line {line}")] + HostKeyRevoked { + host: String, + port: u16, + line: usize, + }, #[error("SSH error occurred: {0}")] SshError(#[from] russh::Error), #[error("Send error")] diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index 3099cd81..16562940 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -21,12 +21,20 @@ //! OpenSSH-style warning. The recording itself is delegated to //! `russh::keys::known_hosts::learn_known_hosts_path`, which handles entry //! formatting (including the `[host]:port` form for non-22 ports) and file -//! creation; this module adds the trust decision, restrictive permissions on -//! newly created files, and serialization of concurrent first-time connections. +//! creation; this module adds the trust decision, restrictive permissions +//! created up front rather than tightened after the fact, and serialization +//! of concurrent first-time connections. //! //! The known_hosts lookup itself ([`lookup_known_host`]) also backs strict //! (`yes`) mode through [`verify_known_hosts_file`], so both modes agree on -//! which keys verify and which count as changed. +//! which keys verify and which count as changed. Both entry points also +//! lowercase the hostname once before it is used for either the lookup or the +//! recorded entry, matching OpenSSH's case-insensitive known_hosts matching +//! (russh's own matcher is a plain byte compare), and scan for +//! `@revoked`/`@cert-authority` marker lines before the ordinary lookup runs: +//! russh's parser reads the marker itself as the literal first field of the +//! host list, so a line like `@revoked node1 ssh-ed25519 ` never matches +//! `node1` and is otherwise invisible to [`lookup_known_host`]. use russh::keys::{Algorithm, HashAlg, PublicKey}; use std::path::Path; @@ -103,6 +111,203 @@ fn lookup_known_host( }) } +/// The result of scanning known_hosts for `@revoked`/`@cert-authority` +/// marker lines naming a host, ahead of the ordinary lookup. +/// +/// russh's `known_host_keys_path` splits each line on `' '` and takes the +/// *first* field as the host list, so on a marker line such as `@revoked +/// node1 ssh-ed25519 ` that first field is the literal string +/// `@revoked`, which never matches `node1`. The line is therefore invisible +/// to [`lookup_known_host`], and without this scan a revoked key would look +/// like an ordinary first use: recorded and accepted. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum MarkerScan { + /// A `@revoked` line names the host and its key field is exactly the + /// offered key: the caller must hard reject without recording or + /// accepting anything. + Revoked { line: usize }, + /// A `@cert-authority` line names the host. bssh has no CA signature + /// validation path to fall back to, so this does not fail closed; the + /// caller warns and falls through to ordinary TOFU. + CertAuthority { line: usize }, + /// No marker line applies to this host. + None, +} + +/// Scan `known_hosts_path` for `@revoked` and `@cert-authority` lines naming +/// `hostname`/`port`. `hostname` must already be normalized (lowercase) by +/// the caller, the same convention [`lookup_known_host`] relies on. +/// +/// An unreadable or missing file has nothing to scan and reports +/// [`MarkerScan::None`], the same as [`lookup_known_host`] treats it as an +/// unknown host: `record_host_key` is what actually creates the file. +fn scan_known_hosts_markers( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) -> MarkerScan { + let Ok(contents) = std::fs::read_to_string(known_hosts_path) else { + return MarkerScan::None; + }; + + let host_port = known_hosts_entry_name(hostname, port); + let mut cert_authority: Option = None; + + for (idx, raw_line) in contents.lines().enumerate() { + let line = idx + 1; + let text = raw_line.trim_start(); + if text.is_empty() || text.starts_with('#') { + continue; + } + + let mut fields = text.split(' ').filter(|f| !f.is_empty()); + let Some(marker) = fields.next() else { + continue; + }; + let is_revoked = marker == "@revoked"; + if !is_revoked && marker != "@cert-authority" { + continue; + } + // Fields after the marker mirror an ordinary known_hosts line: host + // list, key type, base64 key. The key type itself is not needed here + // (`parse_public_key_base64` recovers the algorithm from the key + // blob), only skipped over to reach the base64 field, the same way + // russh's own `known_host_keys_path` does for ordinary lines. + let (Some(host_field), Some(_key_type), Some(key_field)) = + (fields.next(), fields.next(), fields.next()) + else { + continue; + }; + + if !marker_host_matches(&host_port, hostname, host_field, is_revoked) { + continue; + } + + if is_revoked { + if let Ok(key) = russh::keys::parse_public_key_base64(key_field) + && key == *server_public_key + { + return MarkerScan::Revoked { line }; + } + // A `@revoked` line matching the host but holding a *different* + // key revokes some other key, not the one being offered now, so + // it must not block this one; keep scanning the rest of the file. + } else { + cert_authority.get_or_insert(line); + } + } + + match cert_authority { + Some(line) => MarkerScan::CertAuthority { line }, + None => MarkerScan::None, + } +} + +/// Whether a marker line's host field names `hostname`/`host_port`. +/// +/// Supports the exact form, the comma-separated list form, and simple `*` +/// and `?` globs, since marker lines commonly use them (`@cert-authority +/// *.example.com`). When `lenient` (used for `@revoked` only), a pattern that +/// does not match the port-qualified form is also tried against the bare +/// hostname: a `@revoked` line commonly omits the port, and failing to honor +/// a revocation is worse than an unnecessary rejection. +fn marker_host_matches( + host_port: &str, + hostname: &str, + pattern_field: &str, + lenient: bool, +) -> bool { + pattern_field.split(',').any(|pattern| { + let pattern = pattern.trim(); + if pattern.is_empty() { + return false; + } + glob_match(host_port, pattern) + || (lenient && host_port != hostname && glob_match(hostname, pattern)) + }) +} + +/// Minimal case-insensitive glob matcher supporting `*` (any sequence, +/// including empty) and `?` (any single character). known_hosts marker +/// patterns use only these two wildcards (no bracket classes), and matching +/// is case-insensitive to agree with the hostname normalization elsewhere in +/// this module. +fn glob_match(text: &str, pattern: &str) -> bool { + if !pattern.contains(['*', '?']) { + return text.eq_ignore_ascii_case(pattern); + } + let text: Vec = text.chars().map(|c| c.to_ascii_lowercase()).collect(); + let pattern: Vec = pattern.chars().map(|c| c.to_ascii_lowercase()).collect(); + glob_match_chars(&text, &pattern, 0, 0) +} + +/// Recursive helper for [`glob_match`]. +fn glob_match_chars(text: &[char], pattern: &[char], ti: usize, pi: usize) -> bool { + if pi == pattern.len() { + return ti == text.len(); + } + match pattern[pi] { + '*' => { + glob_match_chars(text, pattern, ti, pi + 1) + || (ti < text.len() && glob_match_chars(text, pattern, ti + 1, pi)) + } + '?' => ti < text.len() && glob_match_chars(text, pattern, ti + 1, pi + 1), + c => ti < text.len() && text[ti] == c && glob_match_chars(text, pattern, ti + 1, pi + 1), + } +} + +/// Act on any `@revoked`/`@cert-authority` marker line naming +/// `hostname`/`port`, ahead of the ordinary lookup. `hostname` must already +/// be normalized (lowercase). +/// +/// A `@revoked` match is a hard stop: `Err` with the dedicated +/// [`super::Error::HostKeyRevoked`], printed with a loud warning, and nothing +/// is recorded or accepted. A `@cert-authority` match only warns: bssh has no +/// CA signature validation to fall back to, so failing closed here would +/// break every working CA setup with no workaround, and the point of this +/// check is only to stop the marker being silently ignored, not to implement +/// CA validation. No marker at all is a silent `Ok(())`. +fn check_marker_lines( + hostname: &str, + port: u16, + server_public_key: &PublicKey, + known_hosts_path: &str, +) -> Result<(), super::Error> { + match scan_known_hosts_markers(hostname, port, server_public_key, known_hosts_path) { + MarkerScan::Revoked { line } => { + let entry = known_hosts_entry_name(hostname, port); + tracing::error!( + "Refusing host key for '{entry}': revoked by {known_hosts_path}:{line}" + ); + eprintln!( + "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n\ + @ WARNING: REVOKED HOST KEY! @\n\ + @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n\ + The key offered by '{entry}' matches a key explicitly revoked at {known_hosts_path}:{line}.\n\ + This most likely means the key has been compromised. Connecting is refused.\n\ + Do not remove the @revoked entry unless you are certain the revocation itself was a mistake." + ); + Err(super::Error::HostKeyRevoked { + host: hostname.to_string(), + port, + line, + }) + } + MarkerScan::CertAuthority { line } => { + let entry = known_hosts_entry_name(hostname, port); + tracing::warn!( + "'{entry}' has a @cert-authority entry at {known_hosts_path}:{line}, which bssh does not validate; falling back to trust-on-first-use for the offered key" + ); + eprintln!( + "Warning: {known_hosts_path}:{line} marks '{entry}' as a certificate authority (@cert-authority); bssh cannot validate CA-signed host keys, so the offered key is being trusted on first use instead" + ); + Ok(()) + } + MarkerScan::None => Ok(()), + } +} + /// Longest hostname accepted in a known_hosts entry. A DNS name cannot exceed /// 253 characters, so this only bounds the line length against a hostname that /// could never have resolved anyway. @@ -170,21 +375,34 @@ fn ensure_recordable_hostname(hostname: &str) -> Result<(), super::Error> { /// Returns `Ok(true)` when the key matches any of the host's recorded keys, or /// when the host has no recorded keys at all (in which case the key is recorded /// first). Returns [`super::Error::HostKeyChanged`] when the host has recorded -/// keys and none of them matches the offered key, without modifying the file. -/// A hostname that cannot be represented in a known_hosts entry is rejected -/// outright, before anything is looked up or written. +/// keys and none of them matches the offered key, without modifying the file, +/// and [`super::Error::HostKeyRevoked`] when the offered key matches a +/// known_hosts `@revoked` marker line. A hostname that cannot be represented +/// in a known_hosts entry is rejected outright, before anything is looked up +/// or written. pub(super) async fn verify_accept_new( hostname: &str, port: u16, server_public_key: &PublicKey, known_hosts_path: &str, ) -> Result { + // OpenSSH lowercases the hostname before every known_hosts comparison; + // russh's `match_hostname` is a plain byte compare. Normalizing once here + // and using the normalized value for both the lookup and the recorded + // entry keeps a later differently-cased connection hitting the same pin + // instead of bypassing it and re-recording, and matches what OpenSSH + // itself would have written to the file. + let hostname = hostname.to_ascii_lowercase(); + let hostname = hostname.as_str(); + // Checked before the lock is taken: serializing a hostname that is already // rejected would only make every other connection wait on it. ensure_recordable_hostname(hostname)?; let _guard = KNOWN_HOSTS_LOCK.lock().await; + 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) => Ok(true), // The host is pinned and the offered key is not one of its recorded @@ -225,9 +443,12 @@ pub(super) async fn verify_accept_new( /// /// Returns `Ok(false)` for a host with no recorded keys so the caller's own /// unknown-host rejection applies, `Ok(true)` when one of the host's recorded -/// keys is the offered key, and [`super::Error::HostKeyChanged`] when the host -/// is pinned and none of its keys match. Shares [`lookup_known_host`] with -/// accept-new so both modes agree on what "changed" means. A hostname that +/// keys is the offered key, [`super::Error::HostKeyChanged`] when the host is +/// pinned and none of its keys match, and [`super::Error::HostKeyRevoked`] +/// when the offered key matches a known_hosts `@revoked` marker line. Shares +/// [`lookup_known_host`] with accept-new so both modes agree on what +/// "changed" means, and the hostname is normalized (lowercase) the same way +/// so both modes hit the same pin regardless of spelling. A hostname that /// cannot be represented in a known_hosts entry is rejected outright: strict /// mode never records, but such a hostname could not match a well-formed entry /// either, so this only replaces a misleading "unknown host" with the real @@ -238,7 +459,11 @@ pub(super) fn verify_known_hosts_file( server_public_key: &PublicKey, known_hosts_path: &str, ) -> Result { + let hostname = hostname.to_ascii_lowercase(); + let hostname = hostname.as_str(); + ensure_recordable_hostname(hostname)?; + 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) => Ok(true), @@ -316,6 +541,14 @@ fn record_host_key( let dir_preexisted = path.parent().is_none_or(Path::exists); let file_preexisted = path.exists(); + // Create with the final restrictive mode up front, before + // `learn_known_hosts_path` gets a chance to create either one with the + // process umask. See `precreate_with_restrictive_permissions` for why + // this closes the window that `restrict_created_permissions` below can + // only narrow after the fact. + #[cfg(unix)] + precreate_with_restrictive_permissions(path, dir_preexisted, file_preexisted); + if let Err(e) = russh::keys::known_hosts::learn_known_hosts_path(hostname, port, server_public_key, path) { @@ -350,10 +583,69 @@ pub(crate) fn known_hosts_entry_name(hostname: &str, port: u16) -> String { } } +/// Pre-create the `.ssh` directory (0700) and known_hosts file (0600) with +/// their final permissions before `learn_known_hosts_path` touches either. +/// +/// `learn_known_hosts_path` creates both with `std::fs::create_dir_all` and +/// `OpenOptions::create(true)`, which apply the process umask, and the mode +/// used to be tightened by [`restrict_created_permissions`] only *after* +/// that call returned, leaving a window where a permissive umask could leave +/// either one group- or world-writable. A `mkdir`/`open` syscall that +/// requests 0o700/0o600 directly cannot be widened by any umask, because +/// umask only ever clears requested bits and neither mode carries a group or +/// other bit for a standard umask to clear, so creating with the final mode +/// up front closes the window instead of narrowing it afterward. +/// +/// Only whichever of the two does not already exist is touched, matching +/// [`restrict_created_permissions`]'s preservation of pre-existing modes. An +/// error other than "already exists" (a benign race with another creator, +/// since this runs under `KNOWN_HOSTS_LOCK` only within this process) is +/// logged and otherwise ignored: `learn_known_hosts_path` runs regardless, +/// and its own error handling in [`record_host_key`] is what decides whether +/// the connection fails. +#[cfg(unix)] +fn precreate_with_restrictive_permissions( + path: &Path, + dir_preexisted: bool, + file_preexisted: bool, +) { + use std::fs::{DirBuilder, OpenOptions}; + use std::io::ErrorKind; + use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; + + if !dir_preexisted + && let Some(parent) = path.parent() + && let Err(e) = DirBuilder::new().mode(0o700).create(parent) + && e.kind() != ErrorKind::AlreadyExists + { + tracing::warn!( + "Failed to pre-create {} with mode 0700: {e}", + parent.display() + ); + } + + if !file_preexisted + && let Err(e) = OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + && e.kind() != ErrorKind::AlreadyExists + { + tracing::warn!( + "Failed to pre-create {} with mode 0600: {e}", + path.display() + ); + } +} + /// Tighten permissions on the `.ssh` directory (0700) and known_hosts file /// (0600), but only when this recording created them. /// `learn_known_hosts_path` creates both with the process umask, so a fresh -/// directory could otherwise be group/world readable. +/// directory could otherwise be group/world readable. Kept as a fallback for +/// the rare case where [`precreate_with_restrictive_permissions`] could not +/// pre-create one of them (for example a missing grandparent directory that +/// only `learn_known_hosts_path`'s `create_dir_all` fills in). #[cfg(unix)] fn restrict_created_permissions(path: &Path, dir_preexisted: bool, file_preexisted: bool) { use std::os::unix::fs::PermissionsExt; @@ -705,6 +997,267 @@ mod tests { ); } + // `@revoked` / `@cert-authority` marker lines. + + #[tokio::test] + async fn test_accept_new_rejects_revoked_key() { + let (_dir, path, path_str) = temp_known_hosts(); + let revoked = generate_key(); + + std::fs::write( + &path, + format!( + "@revoked node1.example.com {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + // russh's own parser reads "@revoked" as the literal host list, so + // without marker handling this offered key would look like an + // ordinary first use and get recorded and accepted. + let result = + verify_accept_new("node1.example.com", 22, revoked.public_key(), &path_str).await; + match result { + Err(Error::HostKeyRevoked { host, port, line }) => { + assert_eq!(host, "node1.example.com"); + assert_eq!(port, 22); + assert_eq!(line, 1); + } + other => panic!("expected HostKeyRevoked, got {other:?}"), + } + assert_eq!( + entry_lines(&path).len(), + 1, + "the revoked key must not be recorded" + ); + } + + #[tokio::test] + async fn test_accept_new_revoked_entry_for_different_key_does_not_block() { + let (_dir, path, path_str) = temp_known_hosts(); + let revoked = generate_key(); + let genuine = generate_key(); + + std::fs::write( + &path, + format!( + "@revoked node1.example.com {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + // A `@revoked` line matching the host but holding a *different* key + // revokes that other key, not the one offered now, so it must not + // block this connection: as far as ordinary TOFU is concerned this + // host is still unknown and the genuine key is recorded normally. + let result = + verify_accept_new("node1.example.com", 22, genuine.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "a key that is not the revoked one must not be blocked, got {result:?}" + ); + assert_eq!( + entry_lines(&path).len(), + 2, + "the genuine key must be recorded alongside the @revoked marker" + ); + } + + #[tokio::test] + async fn test_accept_new_cert_authority_warns_and_falls_through_to_tofu() { + let (_dir, path, path_str) = temp_known_hosts(); + let key = generate_key(); + + std::fs::write( + &path, + "@cert-authority *.example.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJdD7y3aLq454yWBdwLWbieU1ebz9/cu7/QEXn9OIeZJ\n", + ) + .unwrap(); + + // bssh has no CA signature validation path, so the marker must not + // fail closed: the offered key still goes through ordinary TOFU and + // is recorded like any other unknown host. + let result = verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!( + matches!(result, Ok(true)), + "a @cert-authority marker must not block TOFU, got {result:?}" + ); + assert_eq!( + entry_lines(&path).len(), + 2, + "the offered key must still be recorded alongside the @cert-authority line" + ); + } + + #[tokio::test] + async fn test_accept_new_revoked_marker_matches_comma_list_and_glob() { + let revoked = generate_key(); + + let (_dir, path, path_str) = temp_known_hosts(); + std::fs::write( + &path, + format!( + "@revoked node2,node1.example.com,node3 {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + let result = + verify_accept_new("node1.example.com", 22, revoked.public_key(), &path_str).await; + assert!( + matches!(result, Err(Error::HostKeyRevoked { .. })), + "the comma-list form must match, got {result:?}" + ); + + let (_dir2, path2, path2_str) = temp_known_hosts(); + std::fs::write( + &path2, + format!( + "@revoked *.example.com {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + let result = + verify_accept_new("node1.example.com", 22, revoked.public_key(), &path2_str).await; + assert!( + matches!(result, Err(Error::HostKeyRevoked { .. })), + "the glob form must match, got {result:?}" + ); + } + + #[tokio::test] + async fn test_accept_new_revoked_marker_matches_non_standard_port_entry_form() { + let (_dir, path, path_str) = temp_known_hosts(); + let revoked = generate_key(); + std::fs::write( + &path, + format!( + "@revoked [node1.example.com]:2222 {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + let result = + verify_accept_new("node1.example.com", 2222, revoked.public_key(), &path_str).await; + assert!( + matches!(result, Err(Error::HostKeyRevoked { .. })), + "the [host]:port marker form must match, got {result:?}" + ); + } + + #[tokio::test] + async fn test_accept_new_revoked_marker_without_port_still_matches_non_standard_port() { + // `@revoked` errs toward matching: a marker line commonly omits the + // port, and failing to honor a revocation is worse than an + // unnecessary rejection. + let (_dir, path, path_str) = temp_known_hosts(); + let revoked = generate_key(); + std::fs::write( + &path, + format!( + "@revoked node1.example.com {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + let result = + verify_accept_new("node1.example.com", 2222, revoked.public_key(), &path_str).await; + assert!( + matches!(result, Err(Error::HostKeyRevoked { .. })), + "a port-less @revoked pattern must still match a non-standard port connection, got {result:?}" + ); + } + + #[tokio::test] + async fn test_strict_mode_rejects_revoked_key() { + let (_dir, path, path_str) = temp_known_hosts(); + let revoked = generate_key(); + std::fs::write( + &path, + format!( + "@revoked node1.example.com {}\n", + revoked.public_key().to_openssh().unwrap() + ), + ) + .unwrap(); + + let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); + let result = handler.check_server_key(revoked.public_key()).await; + assert!( + matches!(result, Err(Error::HostKeyRevoked { .. })), + "strict mode must also honor @revoked, got {result:?}" + ); + } + + // Hostname case normalization. + + #[tokio::test] + async fn test_accept_new_hostname_matching_is_case_insensitive() { + 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 recorded = entry_lines(&path); + assert_eq!(recorded.len(), 1); + assert!( + recorded[0].starts_with("node1.example.com "), + "the recorded entry must use the normalized (lowercase) hostname, got: {}", + recorded[0] + ); + + // A differently-cased spelling of the same host must hit the same + // pin instead of looking unknown and recording a second entry. + let result = verify_accept_new("node1.example.com", 22, key.public_key(), &path_str).await; + assert!(matches!(result, Ok(true))); + assert_eq!( + entry_lines(&path), + recorded, + "differently-cased hostnames must resolve to the same pin" + ); + + // A changed key offered under yet another casing must still be + // caught as a conflict, not treated as a fresh first use. + let imposter = generate_key(); + let result = + verify_accept_new("Node1.Example.Com", 22, imposter.public_key(), &path_str).await; + assert!( + matches!(result, Err(Error::HostKeyChanged { .. })), + "a changed key under a different casing must still be caught, got {result:?}" + ); + assert_eq!(entry_lines(&path), recorded); + } + + #[tokio::test] + async fn test_strict_mode_hostname_matching_is_case_insensitive() { + let (_dir, _path, path_str) = temp_known_hosts(); + let key = generate_key(); + + russh::keys::known_hosts::learn_known_hosts_path( + "node1.example.com", + 22, + key.public_key(), + &path_str, + ) + .unwrap(); + + let mut handler = ClientHandler::new( + "NODE1.example.com".to_string(), + "127.0.0.1:22".parse().unwrap(), + ServerCheckMethod::KnownHostsFile(path_str), + ); + let result = handler.check_server_key(key.public_key()).await; + assert!( + matches!(result, Ok(true)), + "strict mode must match an existing pin regardless of hostname casing, got {result:?}" + ); + } + #[tokio::test] async fn test_changed_key_error_carries_connection_port() { // The client-facing guidance messages build `ssh-keygen -R From 85a43246bfc440962a7107a895901a1433f36df2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 14:41:21 +0900 Subject: [PATCH 6/6] docs: reconcile CHANGELOG and architecture docs with final TOFU state CHANGELOG.md's `[Unreleased]` security entry only described the first commit on this branch (implementing accept-new TOFU) and had drifted from what the branch actually ships. It now has four bullets: the original accept-new/strict-mode/HostKeyChanged work, the OpenSSH-compatible any-matching-entry acceptance rule and alternate-algorithm bypass fix, hostname sanitization before recording, and this branch's last commit (known_hosts marker handling, case-insensitive hostname matching, and the closed permission-creation window). `docs/architecture/ssh-client.md`'s host key verification section is updated to match: the accept-new bullet now describes any-matching-entry acceptance instead of "keys matching their known_hosts entry", and new bullets cover `@revoked`/`HostKeyRevoked`, the `@cert-authority` warn-and-fall-through behavior, and hostname case normalization and rejection. The PR body was also updated (via the GitHub API, since `gh pr edit` fails on this repository's Projects-Classic GraphQL deprecation) to describe the final state across all four commits instead of only the first. Refs #239 --- CHANGELOG.md | 5 ++++- docs/architecture/ssh-client.md | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc4308a2..753e8795 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Security -- **Implement real TOFU verification for the default `accept-new` host key mode, which previously performed no verification at all** (#239). `--strict-host-key-checking accept-new` (the documented, recommended default) mapped to `NoCheck`: any server key was accepted unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, contrary to the help text's "Accept new hosts, reject changed keys". accept-new now checks the offered key against known_hosts, records unknown hosts (creating `~/.ssh` with mode 0700 and `known_hosts` with 0600 when it creates them, printing the OpenSSH-style "Permanently added" notice, and serializing parallel first-time connects behind a process-wide lock so the same new host is recorded exactly once), and rejects changed keys with the OpenSSH-style warning banner including the offered key's SHA256 fingerprint, the offending file and line, and an `ssh-keygen -R` remediation hint, without overwriting the recorded entry. Strict (`yes`) mode no longer downgrades to `NoCheck` when the known_hosts file is missing or its path cannot be determined; a missing file now behaves as an empty one, so unknown hosts are rejected. Changed keys are reported through a dedicated `HostKeyChanged` error carrying the host and conflicting line in every known_hosts-based mode instead of collapsing into the generic "Server check failed", and `no` mode is unchanged. The fix applies to every connection path since they all flow through the same handler: direct, jump-chain (first hop, intermediate hops, and the destination through the tunnel), SFTP transfers, and interactive mode. Entries use the `[host]:port` form for non-standard ports and round-trip through subsequent checks. accept-new also rejects a key whose algorithm differs from an already-recorded entry for the same host instead of recording it alongside: russh's `check_known_hosts_path` reports a changed key only when an entry of the *same* algorithm holds a different key, so a host pinned only under other algorithms was indistinguishable from a first use, and a man-in-the-middle that advertised support for just an algorithm the real host had not used yet would have been trusted, since SSH negotiation picks the first client algorithm the server also supports. OpenSSH resists this by reordering its per-host key algorithm proposal to prefer already-known types, which russh cannot do per host, so accept-new now consults `known_host_keys_path` before recording and treats any existing entry for the host as a changed key. The changed-key remediation command also names the port-qualified `[host]:port` entry and, in the warning banner, the resolved known_hosts file (`ssh-keygen -f "" -R ""`, OpenSSH's own form), so it works for non-standard ports where the previous `ssh-keygen -R ` matched nothing; every printed argument is double quoted so an unmatched `[...]` glob does not make zsh reject the command. +- **Implement real TOFU verification for the default `accept-new` host key mode, which previously performed no verification at all** (#239). `--strict-host-key-checking accept-new` (the documented, recommended default) mapped to `NoCheck`: any server key was accepted unconditionally, nothing was ever written to `~/.ssh/known_hosts`, and changed keys were never rejected, contrary to the help text's "Accept new hosts, reject changed keys". accept-new now checks the offered key against known_hosts, records unknown hosts (creating `~/.ssh` and `known_hosts` with their final restrictive permissions, printing the OpenSSH-style "Permanently added" notice, and serializing parallel first-time connects behind a process-wide lock so the same new host is recorded exactly once), and rejects changed keys with the OpenSSH-style warning banner including the offered key's SHA256 fingerprint, the offending file and line, and a remediation hint naming the actual `[host]:port`-qualified entry, without overwriting the recorded entry. Strict (`yes`) mode no longer downgrades to `NoCheck` when the known_hosts file is missing or its path cannot be determined; a missing file now behaves as an empty one, so unknown hosts are rejected. Changed keys are reported through a dedicated `HostKeyChanged` error carrying the host, port, and conflicting line in every known_hosts-based mode instead of collapsing into the generic "Server check failed", and `no` mode is unchanged. The fix applies to every connection path since they all flow through the same handler: direct, jump-chain (first hop, intermediate hops, and the destination through the tunnel), SFTP transfers, port forwarding, and interactive mode. Entries use the `[host]:port` form for non-standard ports and round-trip through subsequent checks. +- **Accept a host key that matches any of a host's recorded known_hosts entries, instead of rejecting as soon as one same-algorithm entry differs** (#239). `russh::keys::check_known_hosts_path` collects its per-line results and short-circuits on the first non-matching same-algorithm entry, so a host with two or more keys of one algorithm, a stale key kept beside a rotated one, or a cluster-style `node1,node2,node3 ` line beside a per-node `node2 ` line, was rejected with the full "REMOTE HOST IDENTIFICATION HAS CHANGED" banner even while offering a key that a later line in the same file held, a false man-in-the-middle alarm whose own remediation (`ssh-keygen -R`) would have deleted the legitimate pin and turned the next connection into an unguarded first use. accept-new and strict (`yes`)/`DefaultKnownHostsFile` mode now share one lookup that applies OpenSSH's rule instead: any recorded key equal to the offered key verifies, and only a host whose recorded keys none of them match counts as changed; that also still catches the alternate-algorithm bypass closed earlier in this branch, where a key offered under an algorithm the host had not used yet was recorded and accepted alongside the existing entry, since SSH negotiation lets the server steer which algorithm gets offered and russh cannot reorder its per-host proposal to prefer already-known types the way OpenSSH does. The changed-key banner now prefers the entry with the same algorithm as the offered key when naming the offending line, and `DefaultKnownHostsFile` resolves its own path once instead of letting russh and the banner text resolve it independently. +- **Reject a hostname that cannot round-trip through a known_hosts entry, instead of recording it verbatim** (#239). `learn_known_hosts_path` appends `{host} ` (or `[{host}]:{port} `) with no escaping, and russh's parser splits each line on `' '`, reads `,` as the host-list separator, skips a line starting with `#`, and reads a leading `|1|` as a hashed host field, none of which any hostname source reaching the connect handler actually validated against (`-H/--hosts` validation was discarded in favor of the unvalidated `~/.ssh/config` `HostName`, `BACKENDAI_CLUSTER_HOSTS` was only trimmed, and the port-forwarding path skipped hostname validation entirely). A hostname containing a newline could pin an attacker's key against a second host the user never named, a space made the key field unparsable so the host became permanently unconnectable, a leading `#` made the line invisible to every later lookup so it re-recorded on every connection, and a comma silently shared one key across a whole host list. accept-new and strict mode now both reject an empty, over-255-byte, whitespace/control-character, or `#`/`,`/`|`/`*`/`?`/`!`/`\`-containing hostname before anything is looked up or written, failing closed since a hostname that cannot be recorded cannot be verified on any later connection either; ordinary names, IPv4/IPv6 literals, zone identifiers, and the bracketed `[::1]` form are unaffected. +- **Honor known_hosts `@revoked`/`@cert-authority` marker lines, match hostnames case-insensitively, and close the window between creating `~/.ssh`/`known_hosts` and restricting their permissions** (#239). russh's known_hosts parser reads a marker line's own `@revoked`/`@cert-authority` token as the literal host field, so it never matched the real host and both accept-new and strict mode silently ignored these lines, meaning a key an operator had explicitly revoked was recorded and accepted like any ordinary first use. A dedicated scan now runs before the ordinary lookup in every known_hosts-based mode: a `@revoked` line whose host matches (the exact, comma-list, or `*`/`?` glob form, erring toward matching when in doubt since failing to honor a revocation is worse than an unnecessary rejection) and whose key equals the offered key is a hard rejection with a new `HostKeyRevoked` error, while a matching `@revoked` line naming a *different* key does not block the offered one; a `@cert-authority` match only prints a loud warning, since bssh has no CA signature validation to fall back to and failing closed there would break every working CA setup with no workaround, and falls through to ordinary TOFU. Hostname matching is also normalized to lowercase once at the point of verification, for both the lookup and the recorded entry, matching OpenSSH's case-insensitive known_hosts comparison (russh's own matcher is a plain byte compare), so `NODE1.example.com` and `node1.example.com` now hit the same pin instead of the differently-cased spelling looking unknown and re-recording. Finally, `~/.ssh` and `known_hosts` are created with mode 0700/0600 directly (`DirBuilder`/`OpenOptions` with an explicit mode) before `learn_known_hosts_path` runs, rather than letting it create them with the process umask and tightening the mode only afterward, closing the brief window in which an unusually permissive umask could leave either one group- or world-writable. ### Fixed - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. diff --git a/docs/architecture/ssh-client.md b/docs/architecture/ssh-client.md index e5cc167e..f72ee165 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -39,10 +39,12 @@ - Host key verification with three modes: - `StrictHostKeyChecking::Yes` - Strict verification using known_hosts; a missing file behaves as an empty one, so unknown hosts are rejected - `StrictHostKeyChecking::No` - Skip all verification - - `StrictHostKeyChecking::AcceptNew` - TOFU mode: keys matching their known_hosts entry are accepted, unknown hosts are recorded in known_hosts and accepted, changed keys are rejected with an OpenSSH-style warning + - `StrictHostKeyChecking::AcceptNew` - TOFU mode: a key matching any of a host's recorded known_hosts entries is accepted (not just the first one, matching OpenSSH rather than short-circuiting on the first same-algorithm mismatch), unknown hosts are recorded and accepted, and a key is rejected only when the host has recorded keys and none of them match - CLI flag `--strict-host-key-checking` with default "accept-new" -- Uses system known_hosts file (~/.ssh/known_hosts); accept-new creates it on first recording (directory 0700, file 0600) and serializes concurrent first-time recordings behind a process-wide lock so parallel connects to the same new host produce a single entry -- Changed keys surface as a dedicated `HostKeyChanged` error (host, offending line) instead of a generic check failure, in strict and accept-new modes alike +- Uses system known_hosts file (~/.ssh/known_hosts); accept-new creates the directory (0700) and file (0600) with their final permissions up front rather than tightening the mode after creation, and serializes concurrent first-time recordings behind a process-wide lock so parallel connects to the same new host produce a single entry +- Changed keys surface as a dedicated `HostKeyChanged` error (host, port, offending line) instead of a generic check failure, in strict and accept-new modes alike; a key matching a known_hosts `@revoked` marker line is rejected separately as `HostKeyRevoked`, since russh's parser otherwise reads the marker itself as the literal host field and never matches these lines against the real host, silently letting a revoked key through TOFU +- A `@cert-authority` marker line is not validated (bssh has no CA signature verification path); it only prints a warning and falls through to ordinary TOFU for the offered key, rather than failing closed +- The hostname is lowercased once before both the lookup and the recorded entry, matching OpenSSH's case-insensitive known_hosts comparison, and rejected outright if it cannot round-trip through a known_hosts line (contains whitespace, a control character, or a known_hosts/glob metacharacter), since an unrecordable hostname can never be verified on a later connection either - SSH agent authentication with auto-detection ### 4.0.1 Command Output Streaming Infrastructure