From 1e409771bb62ff0235078c54111786786a59f27f Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 16 Oct 2026 22:19:51 +0700 Subject: [PATCH 1/2] fix(linux-ssh): offer no key replace under StrictHostKeyChecking yes --- linux/crates/app/src/ui/error_text/tests.rs | 1 + linux/crates/app/src/ui/host_key_failure.rs | 10 +- linux/crates/app/src/ui/host_key_review.rs | 62 ++++++- .../app/src/ui/workspace/health_banner.rs | 53 +++++- linux/crates/core/src/ssh_failure.rs | 5 + .../session/src/health/probe_outcome.rs | 1 + linux/crates/ssh/src/forward.rs | 1 + linux/crates/ssh/src/hop_config.rs | 1 + linux/crates/ssh/src/route.rs | 5 + linux/crates/ssh/src/session.rs | 65 ++++++- linux/crates/ssh/src/stderr_classify.rs | 33 ++++ linux/crates/ssh/tests/openssh_session.rs | 174 +++++++++++++++--- .../0013-openssh-client-transport.md | 1 + 13 files changed, 373 insertions(+), 39 deletions(-) diff --git a/linux/crates/app/src/ui/error_text/tests.rs b/linux/crates/app/src/ui/error_text/tests.rs index 59d1b8da80..3125bd5f20 100644 --- a/linux/crates/app/src/ui/error_text/tests.rs +++ b/linux/crates/app/src/ui/error_text/tests.rs @@ -265,6 +265,7 @@ fn ssh_failures_say_what_to_change() { sha256_fingerprint: "SHA256:new".to_owned(), known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), line: 7, + trust_allowed: true, }, ))); assert!(changed.contains("known_hosts"), "{changed}"); diff --git a/linux/crates/app/src/ui/host_key_failure.rs b/linux/crates/app/src/ui/host_key_failure.rs index dbad5a5fa3..4b80e5d6b0 100644 --- a/linux/crates/app/src/ui/host_key_failure.rs +++ b/linux/crates/app/src/ui/host_key_failure.rs @@ -23,10 +23,12 @@ pub(crate) enum HostKeyFailureKind { Unknown { trust_allowed: bool, }, - /// The old key is at `line` of `known_hosts`. + /// The old key is at `line` of `known_hosts`. `trust_allowed` is as + /// for `Unknown`, for the new key once the old one is removed. Changed { known_hosts: PathBuf, line: usize, + trust_allowed: bool, }, Revoked { detail: String, @@ -63,6 +65,7 @@ impl HostKeyFailure { sha256_fingerprint, known_hosts, line, + trust_allowed, } => Some(Self { host: host.clone(), algorithm: algorithm.clone(), @@ -70,6 +73,7 @@ impl HostKeyFailure { kind: HostKeyFailureKind::Changed { known_hosts: known_hosts.clone(), line: *line, + trust_allowed: *trust_allowed, }, }), SshFailure::HostKeyRevoked { host, detail } => Some(Self { @@ -108,6 +112,7 @@ mod tests { sha256_fingerprint: "SHA256:new".to_owned(), known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), line: 3, + trust_allowed: false, }); let revoked = ssh(SshFailure::HostKeyRevoked { host: "bastion".to_owned(), @@ -125,7 +130,8 @@ mod tests { changed.kind, HostKeyFailureKind::Changed { known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), - line: 3 + line: 3, + trust_allowed: false, } ); assert!(matches!( diff --git a/linux/crates/app/src/ui/host_key_review.rs b/linux/crates/app/src/ui/host_key_review.rs index 54ba838aa9..071e5e9b33 100644 --- a/linux/crates/app/src/ui/host_key_review.rs +++ b/linux/crates/app/src/ui/host_key_review.rs @@ -101,6 +101,29 @@ pub(crate) fn host_key_dialog_spec(failure: &HostKeyFailure) -> HostKeyDialogSpe &[("host", host)], ), ), + // ssh would refuse the new key without asking once the old one is + // gone, so a replace would only lose the old key. + HostKeyFailureKind::Changed { + known_hosts, + line, + trust_allowed: false, + } => fixed_elsewhere( + crate::i18n::gettext("SSH Host Key Changed"), + match known_hosts.as_os_str().is_empty() { + true => crate::i18n::gettext_f( + "The key “{host}” sent is not the one trusted in known_hosts. Someone may be intercepting the connection, or the server was reinstalled. The SSH configuration for this host accepts only keys already in known_hosts, so if your server administrator confirms the new fingerprint, replace the key there yourself, then retry.", + &[("host", host)], + ), + false => crate::i18n::gettext_f( + "The key “{host}” sent is not the one trusted in {file}, line {line}. Someone may be intercepting the connection, or the server was reinstalled. The SSH configuration for this host accepts only keys already in known_hosts, so if your server administrator confirms the new fingerprint, replace the key in that file yourself, then retry.", + &[ + ("host", host), + ("file", &known_hosts.display().to_string()), + ("line", &line.to_string()), + ], + ), + }, + ), // Without the file ssh named, there is no key to replace. HostKeyFailureKind::Changed { known_hosts, .. } if known_hosts.as_os_str().is_empty() => fixed_elsewhere( crate::i18n::gettext("SSH Host Key Changed"), @@ -109,7 +132,7 @@ pub(crate) fn host_key_dialog_spec(failure: &HostKeyFailure) -> HostKeyDialogSpe &[("host", host)], ), ), - HostKeyFailureKind::Changed { known_hosts, line } => HostKeyDialogSpec { + HostKeyFailureKind::Changed { known_hosts, line, .. } => HostKeyDialogSpec { heading: crate::i18n::gettext("SSH Host Key Changed"), body: crate::i18n::gettext_f( "The key “{host}” sent is not the one trusted in {file}, line {line}. Someone may be intercepting the connection, or the server was reinstalled. Replace the key only if your server administrator confirms the new fingerprint.", @@ -237,6 +260,23 @@ mod tests { HostKeyFailureKind::Changed { known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), line: 3, + trust_allowed: true, + }, + "SHA256:new", + )); + let strict_changed = host_key_dialog_spec(&failure( + HostKeyFailureKind::Changed { + known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), + line: 3, + trust_allowed: false, + }, + "SHA256:new", + )); + let strict_unplaced = host_key_dialog_spec(&failure( + HostKeyFailureKind::Changed { + known_hosts: PathBuf::new(), + line: 0, + trust_allowed: false, }, "SHA256:new", )); @@ -250,6 +290,7 @@ mod tests { HostKeyFailureKind::Changed { known_hosts: PathBuf::new(), line: 0, + trust_allowed: true, }, "SHA256:new", )); @@ -266,7 +307,24 @@ mod tests { assert!(revoked.body.contains("/etc/ssh/krl"), "{}", revoked.body); assert_eq!(ids(&unplaced), [CLOSE, RETRY]); assert!(!unplaced.body.contains(", line 0"), "{}", unplaced.body); - for spec in [&unknown, &unseen, &strict, &changed, &revoked, &unplaced] { + assert_eq!(ids(&strict_changed), [CLOSE, RETRY]); + assert!( + strict_changed.body.contains("/home/u/.ssh/known_hosts, line 3"), + "{}", + strict_changed.body + ); + assert_eq!(ids(&strict_unplaced), [CLOSE, RETRY]); + assert!(!strict_unplaced.body.contains(", line 0"), "{}", strict_unplaced.body); + for spec in [ + &unknown, + &unseen, + &strict, + &changed, + &revoked, + &unplaced, + &strict_changed, + &strict_unplaced, + ] { assert!(spec.body.contains("bastion"), "{}", spec.body); assert_eq!(spec.default, spec.close, "{spec:?}"); let default = spec.responses.iter().find(|response| response.id == spec.default); diff --git a/linux/crates/app/src/ui/workspace/health_banner.rs b/linux/crates/app/src/ui/workspace/health_banner.rs index e1d3100f2e..d4429efd08 100644 --- a/linux/crates/app/src/ui/workspace/health_banner.rs +++ b/linux/crates/app/src/ui/workspace/health_banner.rs @@ -297,6 +297,7 @@ mod tests { HostKeyFailureKind::Changed { known_hosts: PathBuf::from("/home/u/.ssh/known_hosts"), line: 1, + trust_allowed: true, }, HostKeyFailureKind::Revoked { detail: "revoked".to_owned(), @@ -403,12 +404,19 @@ mod tests { /// Open the review from the banner and click `response`, the way a /// user does. fn review_and_answer(test: &TestWindow, response: &str) { + answer(test, &open_review(test), response); + } + + fn open_review(test: &TestWindow) -> adw::AlertDialog { gtk::prelude::WidgetExt::activate_action(&test.window, &format!("win.{REVIEW_HOST_KEY}"), None) .expect("the review action"); wait_until(WAIT, || alert(test).is_some()).expect("the review dialog"); - let dialog = alert(test).expect("the review dialog"); + alert(test).expect("the review dialog") + } + + fn answer(test: &TestWindow, dialog: &adw::AlertDialog, response: &str) { let label = dialog.response_label(response); - descendants(&dialog) + descendants(dialog) .into_iter() .filter_map(|widget| widget.downcast::().ok()) .find(|button| button.label().as_deref() == Some(label.as_str())) @@ -517,10 +525,10 @@ mod tests { } fn changed_key(known_hosts: &Path) -> FakeEstablish { - changed_key_of("[bastion]:2222", known_hosts) + changed_key_of("[bastion]:2222", known_hosts, true) } - fn changed_key_of(host: &str, known_hosts: &Path) -> FakeEstablish { + fn changed_key_of(host: &str, known_hosts: &Path, trust_allowed: bool) -> FakeEstablish { FakeEstablish::Fail(ConnectError::Driver(DriverError::Transport(TransportError::Ssh( SshFailure::HostKeyChanged { host: host.to_owned(), @@ -528,6 +536,7 @@ mod tests { sha256_fingerprint: FINGERPRINT.to_owned(), known_hosts: known_hosts.to_owned(), line: 1, + trust_allowed, }, )))) } @@ -651,7 +660,7 @@ mod tests { } let test = parked_with( name, - vec![changed_key_of(host, &reported)], + vec![changed_key_of(host, &reported, true)], Some(resolving_ssh(dir.path(), &known_hosts)), Some(tunnel()), ); @@ -666,6 +675,40 @@ mod tests { } } + /// Under StrictHostKeyChecking yes ssh would refuse the new key once + /// the old one is gone, so the review offers no replace and the old + /// key stays. + #[gtk4::test] + fn review_offers_no_replace_when_the_configuration_takes_only_known_keys() { + let dir = tempfile::tempdir().unwrap(); + let known_hosts = dir.path().join("known_hosts"); + let keys = format!("[bastion]:2222 {KEY}\n"); + std::fs::write(&known_hosts, &keys).unwrap(); + let test = parked_with( + "ReviewStrictChanged", + vec![ + changed_key_of("[bastion]:2222", &known_hosts, false), + FakeEstablish::Connect(healthy()), + ], + Some(resolving_ssh(dir.path(), &known_hosts)), + Some(tunnel()), + ); + wait_for_button(&test, "Review Key…"); + + let dialog = open_review(&test); + + assert!(!dialog.has_response("replace"), "a replace was offered"); + assert!(dialog.has_response("retry")); + assert!( + dialog.body().contains(&known_hosts.display().to_string()), + "{}", + dialog.body() + ); + answer(&test, &dialog, "retry"); + wait_until(WAIT, || health(&test) == SessionHealth::Online).expect("the session back online"); + assert_eq!(std::fs::read_to_string(&known_hosts).unwrap(), keys); + } + #[gtk4::test] fn trust_answers_only_for_the_host_it_was_reviewed_for() { let test = parked( diff --git a/linux/crates/core/src/ssh_failure.rs b/linux/crates/core/src/ssh_failure.rs index c7ae4e86dd..493130e8ed 100644 --- a/linux/crates/core/src/ssh_failure.rs +++ b/linux/crates/core/src/ssh_failure.rs @@ -44,6 +44,10 @@ pub enum SshFailure { sha256_fingerprint: String, known_hosts: PathBuf, line: usize, + /// False when the host's ssh configuration takes only keys already + /// in known_hosts, so ssh would refuse the new key without asking + /// once the old one is removed. + trust_allowed: bool, }, #[error("the host key for {host} is revoked: {detail}")] @@ -155,6 +159,7 @@ mod tests { sha256_fingerprint: "SHA256:abc".to_owned(), known_hosts: PathBuf::from("/home/user/.ssh/known_hosts"), line: 3, + trust_allowed: true, }, ErrorCategory::Authentication, ), diff --git a/linux/crates/session/src/health/probe_outcome.rs b/linux/crates/session/src/health/probe_outcome.rs index 639c0ef3ee..8522370378 100644 --- a/linux/crates/session/src/health/probe_outcome.rs +++ b/linux/crates/session/src/health/probe_outcome.rs @@ -59,6 +59,7 @@ mod tests { sha256_fingerprint: "SHA256:abc".to_owned(), known_hosts: "/home/u/.ssh/known_hosts".into(), line: 3, + trust_allowed: true, })); assert_eq!(ProbeOutcome::classify(Ok(())), ProbeOutcome::Healthy); diff --git a/linux/crates/ssh/src/forward.rs b/linux/crates/ssh/src/forward.rs index 11b5430a97..c7b4c353a6 100644 --- a/linux/crates/ssh/src/forward.rs +++ b/linux/crates/ssh/src/forward.rs @@ -58,6 +58,7 @@ impl SshSession { host: &self.host, forward_target: Some(&target_text), declined_host_key: None, + strict_hosts: &[], }; return Err(classify( output.status.code(), diff --git a/linux/crates/ssh/src/hop_config.rs b/linux/crates/ssh/src/hop_config.rs index 13a6823c5c..691e37b101 100644 --- a/linux/crates/ssh/src/hop_config.rs +++ b/linux/crates/ssh/src/hop_config.rs @@ -113,6 +113,7 @@ mod tests { JumpHop { destination: SshDestination::new(host, None, None).unwrap(), host_keys, + known_hosts_name: Some(host.to_owned()), } } diff --git a/linux/crates/ssh/src/route.rs b/linux/crates/ssh/src/route.rs index 9d772a1032..68e2329659 100644 --- a/linux/crates/ssh/src/route.rs +++ b/linux/crates/ssh/src/route.rs @@ -8,6 +8,7 @@ use tokio::process::Command; use crate::SshDestination; use crate::host_key_checking::HostKeyChecking; +use crate::known_hosts::known_hosts_lookup; /// `ssh -G` does no network I/O, but a `Match exec` line can run a /// command, so each lookup is still bounded. @@ -31,6 +32,8 @@ enum Resolved { pub(crate) struct JumpHop { pub(crate) destination: SshDestination, pub(crate) host_keys: Option, + /// The name ssh looks the hop's key up by in known_hosts. + pub(crate) known_hosts_name: Option, } /// The jump hosts between this machine and the destination, as far as @@ -75,6 +78,7 @@ pub(crate) async fn trace_route(ssh_program: &Path, config_file: Option<&Path>, route.hops.push(JumpHop { destination: chain[index].clone(), host_keys: HostKeyChecking::from_ssh_g(&resolved), + known_hosts_name: known_hosts_lookup(&resolved), }); output = resolved; } @@ -280,6 +284,7 @@ esac"#, [JumpHop { destination: hop("bastion"), host_keys: Some(HostKeyChecking::AcceptNew), + known_hosts_name: Some("[192.0.2.10]:2222".to_owned()), }] ); } diff --git a/linux/crates/ssh/src/session.rs b/linux/crates/ssh/src/session.rs index be1a5debd9..16a0ef92e9 100644 --- a/linux/crates/ssh/src/session.rs +++ b/linux/crates/ssh/src/session.rs @@ -20,7 +20,7 @@ use crate::argv::{ControlOp, master_args}; use crate::askpass_bridge::AskpassBridge; use crate::hop_config; use crate::host_key_checking::HostKeyChecking; -use crate::known_hosts::spawn_failure; +use crate::known_hosts::{known_hosts_lookup, spawn_failure}; use crate::password_owner::password_owner; use crate::route::{JumpHop, Route, lookup, trace_route}; use crate::stderr_classify::{ClassifyContext, classify}; @@ -67,6 +67,7 @@ struct MasterPlan { /// The `user@host` in the destination's password prompt. password_owner: Option, host_keys: Option, + strict_hosts: Vec, route: Route, master_config: Option, } @@ -101,6 +102,7 @@ impl MasterPlan { Ok(Self { password_owner, host_keys, + strict_hosts: strict_hosts(resolved.as_deref(), &route.hops), route, master_config, }) @@ -215,6 +217,7 @@ impl SshSession { host: config.destination.host(), forward_target: None, declined_host_key: bridge.declined_host_key(), + strict_hosts: &plan.strict_hosts, }; return Err(classify(status, &stderr, &context)); } @@ -327,6 +330,20 @@ impl Drop for SshSession { } } +/// The known_hosts names of the hosts whose configuration sets +/// StrictHostKeyChecking yes. The master's `-o` and the pinned file +/// raise only accept-new and no to ask, so yes is what ssh applies. +fn strict_hosts(destination: Option<&str>, hops: &[JumpHop]) -> Vec { + let destination = destination + .filter(|resolved| HostKeyChecking::from_ssh_g(resolved) == Some(HostKeyChecking::Yes)) + .and_then(known_hosts_lookup); + let hops = hops + .iter() + .filter(|hop| hop.host_keys == Some(HostKeyChecking::Yes)) + .filter_map(|hop| hop.known_hosts_name.clone()); + destination.into_iter().chain(hops).collect() +} + /// The master's ssh_config when a jump host's own configuration would /// trust a host key without asking, which the master's `-o` options do /// not reach. `None` when every hop already asks or refuses. @@ -514,6 +531,7 @@ fn protocol(detail: &str) -> TransportError { #[cfg(test)] mod tests { use super::*; + use crate::SshDestination; #[tokio::test(start_paused = true)] async fn handshake_deadline_paused_while_prompting() { @@ -525,6 +543,51 @@ mod tests { assert_eq!(deadline.deadline() - Instant::now(), Duration::from_secs(15)); } + /// Each host's own StrictHostKeyChecking decides, as the real + /// `ssh -G` resolves the destination and every jump host. + #[tokio::test] + async fn the_strict_hosts_are_those_whose_configuration_says_yes() { + let dir = tempfile::tempdir().unwrap(); + let config_file = dir.path().join("ssh_config"); + std::fs::write( + &config_file, + "Host db\n HostName 10.0.0.5\n Port 2222\n ProxyJump strict-jump,ask-jump\n StrictHostKeyChecking yes\n\ + Host strict-jump\n HostName 192.0.2.1\n StrictHostKeyChecking yes\n\ + Host ask-jump\n HostName 192.0.2.2\n HostKeyAlias bastion\n StrictHostKeyChecking ask\n\ + Host new-jump\n HostName 192.0.2.4\n StrictHostKeyChecking accept-new\n\ + Host open\n HostName 192.0.2.3\n StrictHostKeyChecking no\n\ + Host unset\n HostName 192.0.2.5\n", + ) + .unwrap(); + let services = SshServices { + runtime: Arc::new(crate::SshRuntimeCell::new(dir.path().join("runtime"))), + ssh_program: "ssh".into(), + ssh_keygen_program: "ssh-keygen".into(), + askpass_program: "tablepro-askpass".into(), + config_file: Some(config_file), + }; + let strict_hosts = async |host: &str, jumps: &[&str]| { + let master_dir = tempfile::tempdir_in(dir.path()).unwrap(); + let config = SshConfig { + destination: SshDestination::new(host, None, None).unwrap(), + jump_hosts: jumps + .iter() + .map(|jump| SshDestination::new(jump, None, None).unwrap()) + .collect(), + auth: SshAuth::Agent, + }; + let plan = MasterPlan::resolve(&config, &services, master_dir.path(), &CancellationToken::new()) + .await + .unwrap(); + plan.strict_hosts + }; + + assert_eq!(strict_hosts("db", &[]).await, ["[10.0.0.5]:2222", "192.0.2.1"]); + assert_eq!(strict_hosts("open", &["strict-jump"]).await, ["192.0.2.1"]); + assert!(strict_hosts("unset", &["ask-jump", "new-jump"]).await.is_empty()); + assert!(strict_hosts("open", &[]).await.is_empty()); + } + #[test] fn control_path_too_long_detected() { let long = PathBuf::from(format!("/tmp/{}", "a".repeat(100))).join("control"); diff --git a/linux/crates/ssh/src/stderr_classify.rs b/linux/crates/ssh/src/stderr_classify.rs index 8633916158..ae42e718c8 100644 --- a/linux/crates/ssh/src/stderr_classify.rs +++ b/linux/crates/ssh/src/stderr_classify.rs @@ -18,6 +18,10 @@ pub(crate) struct ClassifyContext<'a> { pub host: &'a str, pub forward_target: Option<&'a str>, pub declined_host_key: Option<&'a DeclinedHostKey>, + /// The known_hosts names of the hosts whose configuration sets + /// StrictHostKeyChecking yes. ssh prints the same changed-key error + /// for yes, ask and accept-new, so only `ssh -G` tells them apart. + pub strict_hosts: &'a [String], } pub(crate) fn classify(status: Option, stderr: &str, context: &ClassifyContext<'_>) -> TransportError { @@ -89,12 +93,17 @@ fn changed_host_key(stderr: &str, context: &ClassifyContext<'_>) -> SshFailure { (PathBuf::from(path), line.trim().parse().unwrap_or(0)) }); let host = between(stderr, "Host key for ", " has changed").unwrap_or(context.host); + let strict = context + .strict_hosts + .iter() + .any(|strict| strict.eq_ignore_ascii_case(host)); SshFailure::HostKeyChanged { host: host.to_owned(), algorithm: algorithm.to_owned(), sha256_fingerprint: fingerprint.to_owned(), known_hosts, line, + trust_allowed: !strict, } } @@ -208,6 +217,7 @@ mod tests { host: "bastion", forward_target: Some("db:5432"), declined_host_key: None, + strict_hosts: &[], }; fn fixture(stderr: &str) -> TransportError { @@ -298,6 +308,7 @@ mod tests { sha256_fingerprint, known_hosts, line: 1, + trust_allowed: true, }) if host == "[127.0.0.1]:32768" && algorithm == "ED25519" && sha256_fingerprint == "SHA256:hqQS4EIntVyXA99oGaFNB9YErcr85eUmEi96ZPSAEDg" @@ -305,6 +316,28 @@ mod tests { )); } + /// ssh prints the same lines under StrictHostKeyChecking yes as under + /// ask, so the changed host's own configuration decides. + #[test] + fn a_changed_key_of_a_strict_host_cannot_be_trusted() { + let stderr = include_str!("../tests/fixtures/stderr/host_key_changed.txt"); + let trust_allowed = |strict_hosts: &[String]| { + let context = ClassifyContext { + strict_hosts, + ..CONTEXT + }; + match classify(Some(255), stderr, &context) { + TransportError::Ssh(SshFailure::HostKeyChanged { trust_allowed, .. }) => trust_allowed, + other => panic!("{other:?}"), + } + }; + + assert!(!trust_allowed(&["[127.0.0.1]:32768".to_owned()])); + assert!(!trust_allowed(&["jump".to_owned(), "[127.0.0.1]:32768".to_owned()])); + assert!(trust_allowed(&["127.0.0.1".to_owned(), "[127.0.0.1]:22".to_owned()])); + assert!(trust_allowed(&[])); + } + #[test] fn stderr_classification_fixture_host_key_revoked() { assert!(matches!( diff --git a/linux/crates/ssh/tests/openssh_session.rs b/linux/crates/ssh/tests/openssh_session.rs index 295e7aaeb8..7eb888cdd7 100644 --- a/linux/crates/ssh/tests/openssh_session.rs +++ b/linux/crates/ssh/tests/openssh_session.rs @@ -349,49 +349,165 @@ async fn dropping_last_arc_ends_master() { assert!(!process_alive(pid)); } -#[tokio::test] -#[ignore = "requires docker"] -async fn changed_key_reports_file_and_line_then_forget_allows_new_key() { - let fixture = start().await.unwrap(); - let keys = tempfile::tempdir().unwrap(); +/// Puts a key the server does not have in known_hosts for the server +/// at `host`:`port`, the way a reinstalled server looks to ssh. +fn trust_a_stale_key(known_hosts: &Path, host: &str, port: u16) -> TestResult<()> { + let keys = tempfile::tempdir()?; let stale_key = keys.path().join("stale"); let generated = std::process::Command::new("ssh-keygen") .args(["-q", "-t", "ed25519", "-N", "", "-f"]) .arg(&stale_key) - .status() - .unwrap(); + .status()?; assert!(generated.success()); - let public = std::fs::read_to_string(stale_key.with_extension("pub")).unwrap(); + let public = std::fs::read_to_string(stale_key.with_extension("pub"))?; let mut fields = public.split_whitespace(); - let entry = format!( - "[{}]:{} {} {}\n", - fixture.host, - fixture.port, - fields.next().unwrap(), - fields.next().unwrap() - ); - std::fs::write(&fixture.known_hosts, entry).unwrap(); - - let auth = SshAuth::Password { - password: SecretString::from("s3cret"), + let (Some(algorithm), Some(key)) = (fields.next(), fields.next()) else { + return Err("ssh-keygen wrote no public key".into()); }; - let (session, _) = connect(&fixture, auth.clone(), Vec::new()).await; - let TransportError::Ssh(SshFailure::HostKeyChanged { known_hosts, line, .. }) = session.unwrap_err() else { - panic!("expected a changed host key"); - }; - assert_eq!(known_hosts, fixture.known_hosts); - assert_eq!(line, 1); + std::fs::write(known_hosts, format!("[{host}]:{port} {algorithm} {key}\n"))?; + Ok(()) +} +/// The master asks about a key ssh does not know under every +/// StrictHostKeyChecking but yes, so the new key can be trusted once the +/// old one is gone. +#[tokio::test] +#[ignore = "requires docker"] +async fn changed_key_reports_file_and_line_then_forget_allows_new_key() { + let fixture = start().await.unwrap(); + for strict in ["ask", "accept-new", "no"] { + write_config(&fixture.config, &fixture.known_hosts, strict).unwrap(); + trust_a_stale_key(&fixture.known_hosts, &fixture.host, fixture.port).unwrap(); + + let (session, _) = connect(&fixture, password_auth(), Vec::new()).await; + let Err(TransportError::Ssh(SshFailure::HostKeyChanged { + known_hosts, + line, + trust_allowed, + .. + })) = &session + else { + panic!("{strict}: expected a changed host key, got {session:?}"); + }; + assert_eq!(known_hosts, &fixture.known_hosts, "{strict}"); + assert_eq!(*line, 1, "{strict}"); + assert!(*trust_allowed, "{strict}: the new key could not be trusted"); + + forget_host_key( + &fixture.services, + &config(&fixture, password_auth()).unwrap(), + &fixture.known_hosts, + ) + .await + .unwrap(); + let (session, prompter) = connect(&fixture, password_auth(), vec![accept()]).await; + let session = session.unwrap_or_else(|error| panic!("{strict}: {error:?}")); + assert!( + matches!( + prompter.requests().as_slice(), + [request] if matches!(request.purpose, PromptPurpose::SshHostKeyConfirmation { .. }) + ), + "{strict}: {:?}", + prompter.requests() + ); + session.shutdown(Duration::from_secs(5)).await; + } +} + +/// ssh prints the same changed-key error under StrictHostKeyChecking yes +/// as under ask, but without the old key it refuses the new one rather +/// than asking, so the report says the app cannot trust it. +#[tokio::test] +#[ignore = "requires docker"] +async fn a_changed_key_under_strict_checking_cannot_be_trusted_from_the_app() { + let fixture = start().await.unwrap(); + write_config(&fixture.config, &fixture.known_hosts, "yes").unwrap(); + trust_a_stale_key(&fixture.known_hosts, &fixture.host, fixture.port).unwrap(); + + let (session, _) = connect(&fixture, password_auth(), Vec::new()).await; + + assert!( + matches!( + &session, + Err(TransportError::Ssh(SshFailure::HostKeyChanged { + trust_allowed: false, + .. + })) + ), + "{session:?}" + ); forget_host_key( &fixture.services, - &config(&fixture, auth.clone()).unwrap(), + &config(&fixture, password_auth()).unwrap(), &fixture.known_hosts, ) .await .unwrap(); - let (session, prompter) = connect(&fixture, auth, vec![accept()]).await; - assert!(session.is_ok()); - assert_eq!(prompter.requests().len(), 1); + let (session, prompter) = connect(&fixture, password_auth(), vec![accept()]).await; + assert!( + matches!( + &session, + Err(TransportError::Ssh(SshFailure::HostKeyUnknown { + trust_allowed: false, + .. + })) + ), + "{session:?}" + ); + assert!(prompter.requests().is_empty(), "{:?}", prompter.requests()); +} + +/// A jump host's key is checked under the jump host's own configuration, +/// not the destination's. +#[tokio::test] +#[ignore = "requires docker"] +async fn a_jump_hosts_changed_key_follows_its_own_strict_checking() { + let fixture = start().await.unwrap(); + let services = SshServices { + ssh_program: PathBuf::from("ssh"), + config_file: Some(fixture.config.clone()), + ..fixture.services.clone() + }; + for (jump_checking, destination_checking, expected) in [("yes", "ask", false), ("ask", "yes", true)] { + std::fs::write( + &fixture.config, + format!( + "Host tablepro-jump\n HostName {}\n Port {}\n User deploy\n StrictHostKeyChecking {jump_checking}\n\ + Host *\n UserKnownHostsFile {}\n GlobalKnownHostsFile /dev/null\n StrictHostKeyChecking {destination_checking}\n \ + IdentityAgent none\n IdentitiesOnly yes\n PubkeyAuthentication no\n", + fixture.host, + fixture.port, + fixture.known_hosts.display() + ), + ) + .unwrap(); + trust_a_stale_key(&fixture.known_hosts, &fixture.host, fixture.port).unwrap(); + let config = SshConfig { + destination: SshDestination::new("127.0.0.1", Some(2222), Some("deploy".to_owned())).unwrap(), + jump_hosts: vec![SshDestination::new("tablepro-jump", None, None).unwrap()], + auth: password_auth(), + }; + let prompter = Arc::new(FakePrompter::new(Vec::new())); + + let session = SshSession::connect( + &config, + &services, + &LivenessPolicy::DESKTOP, + CredentialInteraction::Attended(prompter), + CancellationToken::new(), + ) + .await; + + let jump = format!("[{}]:{}", fixture.host, fixture.port); + assert!( + matches!( + &session, + Err(TransportError::Ssh(SshFailure::HostKeyChanged { host, trust_allowed, .. })) + if *host == jump && *trust_allowed == expected + ), + "jump {jump_checking}, destination {destination_checking}: {session:?}" + ); + } } #[tokio::test] diff --git a/linux/docs/decisions/0013-openssh-client-transport.md b/linux/docs/decisions/0013-openssh-client-transport.md index d565ea8806..fa38e690d2 100644 --- a/linux/docs/decisions/0013-openssh-client-transport.md +++ b/linux/docs/decisions/0013-openssh-client-transport.md @@ -23,6 +23,7 @@ People reach their databases through bastions configured in `~/.ssh/config`: `Pr - openssh-client 8.7 or later is a runtime dependency: every master gets `-o ForkAfterAuthentication=no`, which OpenSSH 8.7 added, and an older ssh refuses the unknown option. The GNOME 50 Flatpak runtime ships a newer one. - The helper is installed to libexecdir next to the app. - A jump host runs in a second ssh that gets only `-l`, `-p`, `-J` and `-F` from the master (ssh.c turns ProxyJump into a ProxyCommand), never its `-o` options. When a hop's configuration sets `StrictHostKeyChecking accept-new` or `no`, the master gets a generated `-F` file that sets `ask` for that host, includes `~/.ssh/config` and then copies in `/etc/ssh/ssh_config`, so the hop asks through the same dialog. The system file is copied because OpenSSH holds an `Include` to an owner check it skips for that file, and in a Flatpak sandbox root's files belong to nobody. Copied in, a relative `Include` inside it resolves under `~/.ssh` rather than `/etc/ssh`. The `~` of `Include ~/.ssh/config` follows `HOME`, where ssh's own read of the user file follows the password database. A hop whose `ssh -G` lookup fails or takes over 5 seconds is not pinned, and neither is any hop before it. Every other connection reads the configuration as before. +- A StrictHostKeyChecking of yes is kept, for the destination and for each jump host as its own `ssh -G` lookup gives it. A key that changed under it is not offered for replacement, because ssh would then refuse the new key without asking; the host key review says to fix known_hosts outside the app, then retry. - Failures are classified from OpenSSH's stderr, pinned by captured fixtures. A change to those strings in a later OpenSSH release shows up as a failing fixture test. - russh, ssh-key and internal-russh-forked-ssh-key leave the dependency graph when the app moves to this transport. From d224ce001469987a18da9a062bd5f71eff41ebf0 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 16 Oct 2026 22:35:58 +0700 Subject: [PATCH 2/2] fix(linux-ssh): say to fix known_hosts for an unknown key under strict --- linux/crates/app/src/ui/error_text/ssh.rs | 18 +++++++++++++ linux/crates/app/src/ui/error_text/tests.rs | 28 +++++++++++++++++++++ linux/crates/ssh/src/stderr_classify.rs | 7 ++++-- 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/linux/crates/app/src/ui/error_text/ssh.rs b/linux/crates/app/src/ui/error_text/ssh.rs index 6f89d1579d..cef3751258 100644 --- a/linux/crates/app/src/ui/error_text/ssh.rs +++ b/linux/crates/app/src/ui/error_text/ssh.rs @@ -30,6 +30,24 @@ pub fn ssh_failure_message(failure: &SshFailure) -> String { "The host key of {host} was not trusted, so the connection was not made.", &[("host", host)], ), + // ssh refuses under StrictHostKeyChecking yes before it shows the + // key, so there is no fingerprint and nothing to review. + SshFailure::HostKeyUnknown { + host, + trust_allowed: false, + .. + } => crate::i18n::gettext_f( + "The SSH configuration for {host} accepts only keys already in known_hosts. Add the server's key to known_hosts outside TablePro, then retry.", + &[("host", host)], + ), + SshFailure::HostKeyUnknown { + host, + sha256_fingerprint, + .. + } if sha256_fingerprint.is_empty() => crate::i18n::gettext_f( + "{host} is not in known_hosts. Connect once to review and trust its key.", + &[("host", host)], + ), SshFailure::HostKeyUnknown { host, sha256_fingerprint, diff --git a/linux/crates/app/src/ui/error_text/tests.rs b/linux/crates/app/src/ui/error_text/tests.rs index 3125bd5f20..3146cf4b47 100644 --- a/linux/crates/app/src/ui/error_text/tests.rs +++ b/linux/crates/app/src/ui/error_text/tests.rs @@ -286,6 +286,34 @@ fn ssh_failures_say_what_to_change() { assert!(declined.contains("not trusted"), "{declined}"); } +/// Under StrictHostKeyChecking yes ssh refuses before it shows the key, +/// so there is no fingerprint to print and no review the user can do. +#[test] +fn an_unknown_key_says_what_the_configuration_allows() { + let unknown = |sha256_fingerprint: &str, trust_allowed: bool| { + ssh_failure_message(&SshFailure::HostKeyUnknown { + host: "[bastion]:2222".to_owned(), + algorithm: "ED25519".to_owned(), + sha256_fingerprint: sha256_fingerprint.to_owned(), + trust_allowed, + declined: false, + }) + }; + let strict = unknown("", false); + let unseen = unknown("", true); + let reviewable = unknown("SHA256:abc", true); + + for message in [&strict, &unseen] { + assert!(message.contains("[bastion]:2222"), "{message}"); + assert!(!message.contains("key )") && !message.contains("(key"), "{message}"); + } + assert!(strict.contains("known_hosts outside TablePro"), "{strict}"); + assert!(!strict.contains("review"), "{strict}"); + assert!(unseen.contains("review"), "{unseen}"); + assert!(reviewable.contains("(key SHA256:abc)"), "{reviewable}"); + assert!(reviewable.contains("review"), "{reviewable}"); +} + #[test] fn connect_error_text_covers_every_variant() { let errors = [ diff --git a/linux/crates/ssh/src/stderr_classify.rs b/linux/crates/ssh/src/stderr_classify.rs index ae42e718c8..f1d8d5d77e 100644 --- a/linux/crates/ssh/src/stderr_classify.rs +++ b/linux/crates/ssh/src/stderr_classify.rs @@ -276,16 +276,19 @@ mod tests { )); } + /// Under StrictHostKeyChecking yes ssh names the key type and host + /// but refuses before it prints a fingerprint. #[test] fn stderr_classification_fixture_host_key_unknown_strict() { assert!(matches!( fixture(include_str!("../tests/fixtures/stderr/host_key_unknown_strict.txt")), TransportError::Ssh(SshFailure::HostKeyUnknown { + host, algorithm, + sha256_fingerprint, trust_allowed: false, declined: false, - .. - }) if algorithm == "ED25519" + }) if host == "[127.0.0.1]:32768" && algorithm == "ED25519" && sha256_fingerprint.is_empty() )); }