fix(security): implement real TOFU for accept-new host key mode

CHANGELOG.md

@@ -7,6 +7,12 @@ 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` 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 <shared key>` line beside a per-node `node2 <node2's own key>` 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} <key>` (or `[{host}]:{port} <key>`) 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 <host>` 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.

docs/architecture/ssh-client.md

@@ -37,11 +37,14 @@
 
 **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: 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)
+- 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

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(

src/commands/interactive/connection.rs

@@ -613,6 +613,37 @@ 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(),
+            port: 22,
+            line: 3,
+        };
+        assert!(
+            !is_auth_error_for_password_fallback(&error),
+            "HostKeyChanged should NOT trigger password fallback (host key issue)"
+        );
+    }
+
+    #[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(

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()