diff --git a/crates/tty7-core/src/daemon/ssh/handler.rs b/crates/tty7-core/src/daemon/ssh/handler.rs index 82869df2..92496141 100644 --- a/crates/tty7-core/src/daemon/ssh/handler.rs +++ b/crates/tty7-core/src/daemon/ssh/handler.rs @@ -105,23 +105,13 @@ impl ClientHandler { if !accept { return false; } - if remember { - // The superseded line has to go before the new one lands. - // `known_hosts::check` answers `Known` on any same-algorithm match, - // so an override that only appended left the key the user had just - // rejected trusted for good. If it cannot be dropped, do not append - // either: being asked again next time is the better half of that - // trade. - match known_hosts::forget_superseded(&self.host, self.port, key) { - Ok(()) => { - if let Err(e) = known_hosts::append_trusted(&self.host, self.port, key) { - log::warn!("failed to record host key in known_hosts: {e}"); - } - } - Err(e) => log::warn!( - "not recording host key: the superseded known_hosts line could not be removed: {e}" - ), - } + // `record_trusted` drops the line this key supersedes before adding it, + // and adds nothing if that drop fails — see its comment for why the + // order is not optional. + if remember + && let Err(e) = known_hosts::record_trusted(&self.host, self.port, key) + { + log::warn!("not recording host key in known_hosts: {e}"); } true } @@ -263,6 +253,54 @@ mod tests { } } + /// Saying no to a host-key prompt refuses the connection. + /// + /// The most direct failure this code could have: a person is shown a + /// changed host key, declines, and connects anyway. `accepted_and_remembered` + /// is tested on its own, but nothing checked that `apply_decision` acts on + /// what it answers — deleting the `if !accept` guard left the whole suite + /// green. + /// + /// No filesystem is touched: every response here answers `remember: false` + /// once refused, so the recording branch is not reached. `remember: true` + /// alongside `accept: false` is the dialog's checkbox state, not consent, + /// and is here for exactly that reason. + #[test] + fn a_refused_host_key_prompt_refuses_the_connection() { + let handler = ClientHandler { + host: "example.com".into(), + port: 2222, + verify_host_keys: true, + skip_banner: false, + broker: crate::daemon::ssh::broker::PromptBroker::new(Box::new(|_| true)), + remote_forwards: crate::daemon::ssh::forward::RemoteForwardTable::default(), + }; + let key = russh::keys::PublicKey::from_openssh( + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPXO/kBX63iuiTczoR6uNdl3wAFK7tGWz70jCKkKlw5r", + ) + .expect("a key to offer"); + + for resp in [ + AuthResponse::HostKeyDecision { + accept: false, + remember: false, + }, + // The checkbox left ticked while answering no. + AuthResponse::HostKeyDecision { + accept: false, + remember: true, + }, + AuthResponse::Cancelled, + // A response of the wrong shape is not consent either. + AuthResponse::Secret("yes".into()), + ] { + assert!( + !handler.apply_decision(resp.clone(), &key), + "{resp:?} must not accept the key" + ); + } + } + #[test] fn an_unknown_host_is_asked_about_with_no_prior_algorithm() { let HostKeyAction::Ask(prompt) = action(HostKeyStatus::Unknown, true) else { diff --git a/crates/tty7-core/src/daemon/ssh/known_hosts.rs b/crates/tty7-core/src/daemon/ssh/known_hosts.rs index f2b11e87..8cee4a4b 100644 --- a/crates/tty7-core/src/daemon/ssh/known_hosts.rs +++ b/crates/tty7-core/src/daemon/ssh/known_hosts.rs @@ -177,11 +177,36 @@ pub(crate) fn known_algorithms_in_str(contents: &str, host: &str, port: u16) -> out } -pub(crate) fn append_trusted(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> { +/// Trust `key` for `host:port`, dropping the line it supersedes first. +/// +/// One function rather than two calls at the call site, because the order is +/// the whole point and a call site can get it wrong silently. `check` answers +/// `Known` on any same-algorithm match, so appending without dropping leaves +/// the key this one replaces trusted for good — which is how a host that +/// rotated away from a compromised key would go on accepting the old one. +/// +/// And if the drop fails, nothing is appended: being asked again next time is +/// the better half of that trade. +pub(crate) fn record_trusted(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> { let path = default_path().ok_or_else(|| { std::io::Error::new(std::io::ErrorKind::NotFound, "no home dir for known_hosts") })?; - append_trusted_to(&path, host, port, key) + record_trusted_in_file(&path, host, port, key) +} + +pub(crate) fn record_trusted_in_file( + path: &Path, + host: &str, + port: u16, + key: &PublicKey, +) -> std::io::Result<()> { + forget_superseded_in_file(path, host, port, key).map_err(|e| { + std::io::Error::new( + e.kind(), + format!("the superseded known_hosts line could not be removed: {e}"), + ) + })?; + append_trusted_to(path, host, port, key) } pub(crate) fn append_trusted_to( @@ -331,13 +356,6 @@ pub(crate) fn delete_in_str(contents: &str, id: &KnownHostId) -> (String, bool) /// /// A no-op for a host that was merely unknown, which by definition has no /// same-algorithm line to drop. -pub(crate) fn forget_superseded(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> { - match default_path() { - Some(path) => forget_superseded_in_file(&path, host, port, key), - None => Ok(()), - } -} - pub(crate) fn forget_superseded_in_file( path: &Path, host: &str, @@ -1032,9 +1050,12 @@ mod tests { let path = dir.join("known_hosts"); std::fs::write(&path, format!("example.com {KEY_A}\n")).unwrap(); + // Through `record_trusted_in_file`, which is what `apply_decision` + // calls: the two halves in the wrong order, or one of them missing, is + // the bug this test is named after, and calling them separately here + // would let the call site drop one without anything noticing. let kb = key(KEY_B); - forget_superseded_in_file(&path, "example.com", 22, &kb).unwrap(); - append_trusted_to(&path, "example.com", 22, &kb).unwrap(); + record_trusted_in_file(&path, "example.com", 22, &kb).unwrap(); let contents = std::fs::read_to_string(&path).unwrap(); assert!(