From 231c478d0bbe32d6542e4701ded90f20e7724a86 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Fri, 18 Sep 2026 10:28:34 +0200 Subject: [PATCH] put: surface known_hosts problems instead of silently degrading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unreadable or unparsable known_hosts file was swallowed with 'let _', silently downgrading to prompt-and-accept without telling the user why their configuration was ignored: warn naming the file, then continue. And when the pinned Launchpad fingerprint matches, a DIFFERENT key recorded for that host in known_hosts was silently bypassed: warn about the stale entry (diagnostic only — the published fingerprint stays authoritative). --- src/put/ssh.rs | 184 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 172 insertions(+), 12 deletions(-) diff --git a/src/put/ssh.rs b/src/put/ssh.rs index 99e2ab0..b7fc427 100644 --- a/src/put/ssh.rs +++ b/src/put/ssh.rs @@ -25,7 +25,7 @@ use lazy_static::lazy_static; use log::debug; use serde::Deserialize; use sha2::{Digest, Sha256}; -use ssh2::{CheckResult, HostKeyType, KnownHostFileKind, Session}; +use ssh2::{CheckResult, HostKeyType, KnownHostFileKind, KnownHosts, Session}; use crate::ui::prompt; @@ -346,7 +346,9 @@ pub fn connect( /// Check the server's host key, in decreasing order of trust: /// /// 1. matching one of the host's pinned (published, `host_keys.yml`) -/// fingerprints — accepted silently; +/// fingerprints — accepted (with a diagnostic warning when the user's +/// known_hosts records a *different* key for the host, so a stale +/// entry gets cleaned up; the pin stays authoritative); /// 2. matching `~/.ssh/known_hosts` — accepted; /// 3. known-and-different — refused loudly (possible man-in-the-middle); /// 4. unknown — fingerprint shown, explicit confirmation required, and on @@ -362,21 +364,16 @@ fn verify_host_key( if host_key_is_pinned(host, &fingerprint) { debug!("{host} host key matches the published fingerprint"); + // Purely diagnostic — the pin already decided — but a known_hosts + // entry disagreeing with the published key is worth surfacing + log_known_hosts_discrepancy(host, port, key); return Ok(()); } let mut known_hosts = Session::new()?.known_hosts()?; - for file in known_hosts_files() { - // Unreadable/unknown-format lines are skipped by libssh2; a missing - // file is fine - let _ = known_hosts.read_file(&file, KnownHostFileKind::OpenSSH); - } + load_known_hosts(&mut known_hosts, &known_hosts_files()); - let check = if port == 22 { - known_hosts.check(host, key) - } else { - known_hosts.check_port(host, port, key) - }; + let check = check_known_hosts(&known_hosts, host, port, key); match check { CheckResult::Match => Ok(()), @@ -414,6 +411,76 @@ fn verify_host_key( } } +/// Read `files` into `known_hosts`: silently skips missing files (the +/// normal first-use case — there is nothing to consult), warns about a +/// file that exists but cannot be read or parsed, naming the file and the +/// error — the user should know why their maintained entries are not being +/// consulted — and skips it. Unreadable/unknown-format *lines* are skipped +/// by libssh2 itself. Returns the emitted warnings, so the decision is +/// testable without capturing log output. +fn load_known_hosts(known_hosts: &mut KnownHosts, files: &[PathBuf]) -> Vec { + let mut warnings = Vec::new(); + for file in files { + if !file.exists() { + continue; + } + if let Err(e) = known_hosts.read_file(file, KnownHostFileKind::OpenSSH) { + let warning = format!( + "cannot read the known hosts file {}: {e} — its entries are ignored", + file.display() + ); + log::warn!("{warning}"); + warnings.push(warning); + } + } + warnings +} + +/// Look up `host`'s `key` in the loaded entries, using the `[host]:port` +/// spelling for non-standard ports (the known_hosts encoding) +fn check_known_hosts(known_hosts: &KnownHosts, host: &str, port: u16, key: &[u8]) -> CheckResult { + if port == 22 { + known_hosts.check(host, key) + } else { + known_hosts.check_port(host, port, key) + } +} + +/// What to tell the user when the server's key matched a pinned (published) +/// fingerprint while their known_hosts records a different key for the same +/// host — a stale entry, or worse an old compromise artifact they should +/// clean up; `None` for every other check result, since the pin is +/// authoritative and needs no corroboration. Pure so the decision and the +/// wording are testable without a server or a filesystem. +fn known_hosts_discrepancy(host: &str, check: CheckResult) -> Option { + match check { + CheckResult::Mismatch => Some(format!( + "the host key of {host} matches Launchpad's published \ + fingerprint, but your known_hosts file(s) record a DIFFERENT \ + key for it — proceeding on the published fingerprint; consider \ + removing the stale '{host}' entry from your known_hosts" + )), + CheckResult::Match | CheckResult::NotFound | CheckResult::Failure => None, + } +} + +/// Diagnostic-only companion to the pinned-fingerprint acceptance: load the +/// user's known hosts files and warn when they record a different key for +/// `host` than the pinned one about to be accepted. The pin stays +/// authoritative: any failure here is tolerated and the connection +/// proceeds regardless. +fn log_known_hosts_discrepancy(host: &str, port: u16, key: &[u8]) { + let Ok(mut known_hosts) = Session::new().and_then(|session| session.known_hosts()) else { + return; + }; + load_known_hosts(&mut known_hosts, &known_hosts_files()); + + let check = check_known_hosts(&known_hosts, host, port, key); + if let Some(warning) = known_hosts_discrepancy(host, check) { + log::warn!("{warning}"); + } +} + /// Known hosts files to consult, user file first (so that new keys are /// accepted because of the user file, mirroring ssh's own ordering) fn known_hosts_files() -> Vec { @@ -686,6 +753,99 @@ mod tests { )); } + /// A pinned-key host whose known_hosts entries record a DIFFERENT key + /// is flagged with a message naming the host; every other check result + /// stays silent — the published fingerprint remains authoritative. + #[test] + fn known_hosts_discrepancy_flags_mismatch_only() { + let message = known_hosts_discrepancy("ppa.launchpad.net", CheckResult::Mismatch) + .expect("a mismatch under a matching pin must be flagged"); + assert!( + message.contains("ppa.launchpad.net"), + "unexpected: {message}" + ); + assert!(message.contains("DIFFERENT"), "unexpected: {message}"); + assert!( + message.contains("published fingerprint"), + "unexpected: {message}" + ); + assert!( + message.contains("stale"), + "the user must be told to clean the entry up: {message}" + ); + + for quiet in [ + CheckResult::Match, + CheckResult::NotFound, + CheckResult::Failure, + ] { + assert!( + known_hosts_discrepancy("ppa.launchpad.net", quiet).is_none(), + "{quiet:?} must not be flagged" + ); + } + } + + /// Known hosts loading decisions: a missing file is the normal + /// first-use case and stays silent, a readable file is loaded and its + /// entries consulted, and an existing but unreadable file yields a + /// warning naming it — the user must learn why their maintained + /// entries are not being consulted. Offline: a bare session suffices + /// for the known-hosts store. + #[test] + fn load_known_hosts_warns_only_on_unreadable_files() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let mut known_hosts = Session::new().unwrap().known_hosts().unwrap(); + + // A missing file is silent... + let missing = dir.path().join("known_hosts"); + assert!(load_known_hosts(&mut known_hosts, &[missing]).is_empty()); + + // ...a readable one is loaded and consulted... + let key = b"a test host key"; + let loaded = dir.path().join("loaded_known_hosts"); + fs::write( + &loaded, + format!("host.example.com ssh-ed25519 {}\n", base64_nopad(key)), + ) + .unwrap(); + assert!( + load_known_hosts(&mut known_hosts, &[loaded]).is_empty(), + "a readable file must not warn" + ); + assert!( + matches!( + check_known_hosts(&known_hosts, "host.example.com", 22, key), + CheckResult::Match + ), + "the loaded entry must be consulted" + ); + + // ...and an existing but unreadable one is reported, naming the + // file. Environments that read through the mode bits (root) cannot + // exercise the unreadable case and are skipped. + let locked = dir.path().join("locked_known_hosts"); + fs::write(&locked, b"stale.example.com ssh-ed25519 c3RhbGU=").unwrap(); + let mut permissions = fs::metadata(&locked).unwrap().permissions(); + permissions.set_mode(0o000); + fs::set_permissions(&locked, permissions).unwrap(); + if fs::read(&locked).is_ok() { + return; + } + let warnings = load_known_hosts(&mut known_hosts, &[locked]); + assert_eq!(warnings.len(), 1, "unexpected: {warnings:?}"); + assert!( + warnings[0].contains("locked_known_hosts"), + "the warning must name the file: {warnings:?}" + ); + assert!( + warnings[0].contains("ignored"), + "the warning must say the entries are not consulted: {warnings:?}" + ); + } + /// Negated `Host` patterns exclude the host from the whole block (the /// OpenSSH rule): a block applies iff at least one positive pattern /// matches AND no negated pattern matches.