From d04d1045599052a374278685e4472094f5a30fbc Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:17:14 +0800 Subject: [PATCH] fix(ssh): let a rejected stored passphrase ask again (#486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Saving the wrong passphrase for an encrypted key locked that key out permanently. `passphrase_submit` wrote `SetKeyPassphrase` on the "remember" checkbox alone — before the daemon had tried the secret, since `apply_keychain_write` runs ahead of `respond_active` — and `try_identity_file` treated a stored passphrase as final: a decrypt failure with one went straight to "could not decrypt identity file", with no prompt and nothing in the UI that could let go of it. The daemon now says so. `AuthPromptKind::KeyPassphrase` grows a `rejected` flag, and a stored passphrase that does not open the file falls through to the interactive prompt carrying it, so the typed answer still gets its attempt. A passphrase the user typed this time keeps the hard failure — that is a wrong answer, not stale state. The sheet renders the warning line the password sheet already had, and a rejected prompt answered without "remember" now emits `DeleteKeyPassphrase`, mirroring the password idiom exactly. The flag is a `#[serde(default)]` field on a struct variant of an externally tagged enum, which is compatible in both directions: an older peer never sets it and serde ignores fields it does not know. So `PROTOCOL_VERSION` deliberately does not move — the remote-server handshake gates on it, and a bump would turn away older servers over a field they can safely ignore. `protocol.rs`'s compat test pins both directions. Also: deleting an SSH profile now drops the key-passphrase entries no other profile still references, which is what `delete_profile_confirmed`'s own comment already claimed to do but only ever did for the password. --- .github/scripts/check-host-boundary.sh | 1 + crates/tty7-core/src/daemon/protocol.rs | 58 +++++++++++++ crates/tty7-core/src/daemon/ssh/auth.rs | 77 ++++++++++++---- src/core/keychain.rs | 1 - src/ui/i18n/en.rs | 3 + src/ui/i18n/ja.rs | 3 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + src/ui/settings.rs | 27 ++++++ src/ui/ssh_connect.rs | 63 ++++++++++++++ src/ui/ssh_prompt.rs | 111 ++++++++++++++++++++++-- 11 files changed, 322 insertions(+), 24 deletions(-) diff --git a/.github/scripts/check-host-boundary.sh b/.github/scripts/check-host-boundary.sh index 642c173b..dd2dc108 100755 --- a/.github/scripts/check-host-boundary.sh +++ b/.github/scripts/check-host-boundary.sh @@ -58,6 +58,7 @@ src/ui/app.rs|std::fs::create_dir_all # sees the file, only the resulting auth. src/ui/ssh_prompt.rs|std::fs::read src/ui/ssh_connect.rs|std::fs::read +src/ui/settings.rs|std::fs::read # Shell history lives in the local user's home (`~/.zsh_history` &co.) and backs # this app's own history search. A remote pane's history is the remote shell's diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index 20dfe4ca..f4055874 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -540,6 +540,20 @@ pub enum AuthPromptKind { KeyPassphrase { key_path: String, comment: String, + /// The connection already had a passphrase for this key and the key + /// would not decrypt with it, so the one the user is about to type + /// replaces a stored secret rather than adding a first one. Only the + /// daemon can know this — the client sees a passphrase prompt and + /// cannot tell a first ask from a second — and without it a wrong + /// "remember" bricks the key for every later connection. + /// + /// `#[serde(default)]`, and deliberately not a `PROTOCOL_VERSION` + /// bump: an older peer on either side of this field simply never sets + /// it, and serde ignores fields it does not know, so the flag is + /// compatible in both directions across daemon↔GUI and GUI↔remote + /// `tty7-server`. + #[serde(default)] + rejected: bool, }, KeyboardInteractive { name: String, @@ -2162,6 +2176,14 @@ mod tests { }], }, }, + DaemonMsg::AuthPrompt { + request_id: 3, + prompt: AuthPromptKind::KeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + comment: String::new(), + rejected: true, + }, + }, DaemonMsg::SshStatus { phase: SshPhase::Failed { reason: "nope".into(), @@ -2178,6 +2200,42 @@ mod tests { } } + /// `rejected` rides on a struct variant of an externally tagged enum that + /// crosses both daemon↔GUI and GUI↔remote `tty7-server`, so it has to + /// survive a peer that predates it in *either* direction. That is why + /// `PROTOCOL_VERSION` did not move for it: the remote handshake gates on + /// it, and bumping would turn every older server away over a field it can + /// safely ignore. + #[test] + fn a_rejected_passphrase_flag_decodes_from_a_peer_that_never_sends_it() { + let old = r#"{"KeyPassphrase":{"key_path":"~/.ssh/id_ed25519","comment":""}}"#; + assert_eq!( + serde_json::from_str::(old).unwrap(), + AuthPromptKind::KeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + comment: String::new(), + rejected: false, + } + ); + + // The other direction: a peer that predates the flag is handed one + // set, and must read the prompt rather than reject the frame. + let new = serde_json::to_string(&AuthPromptKind::KeyPassphrase { + key_path: "/k".into(), + comment: "work laptop".into(), + rejected: true, + }) + .unwrap(); + #[derive(Deserialize)] + enum LegacyPromptKind { + KeyPassphrase { key_path: String, comment: String }, + } + let LegacyPromptKind::KeyPassphrase { key_path, comment } = + serde_json::from_str::(&new).unwrap(); + assert_eq!(key_path, "/k"); + assert_eq!(comment, "work laptop"); + } + #[test] fn native_ssh_spawn_uses_new_kind_byte() { let msg = ClientMsg::SpawnNativeSsh { diff --git a/crates/tty7-core/src/daemon/ssh/auth.rs b/crates/tty7-core/src/daemon/ssh/auth.rs index f8661e96..9c118932 100644 --- a/crates/tty7-core/src/daemon/ssh/auth.rs +++ b/crates/tty7-core/src/daemon/ssh/auth.rs @@ -496,32 +496,48 @@ async fn try_identity_file( let key = match russh::keys::decode_secret_key(&contents, None) { Ok(k) => k, Err(russh::keys::Error::KeyIsEncrypted) => { - let provided = spec - .key_passphrases - .as_ref() - .and_then(|m| m.get(raw_path)) - .cloned(); - let passphrase = match provided { - Some(p) => p, + // A passphrase the connection carried in from the keychain gets + // one silent attempt. If it does not open the file it is simply + // the wrong secret, and the only way forward is to ask — which is + // what this used to refuse to do: a passphrase saved by mistake + // ended every later connection here, with no prompt and no way to + // correct it from inside the app. + let stored = stored_passphrase(spec, raw_path); + let unlocked = match &stored { + Some(p) => match russh::keys::decode_secret_key(&contents, Some(p)) { + Ok(k) => Some(k), + Err(e) => { + log::warn!("the stored passphrase did not decrypt {path}: {e}"); + None + } + }, + None => None, + }; + match unlocked { + Some(k) => k, None => { let resp = broker .prompt(AuthPromptKind::KeyPassphrase { key_path: raw_path.to_string(), comment: String::new(), + rejected: stored.is_some(), }) .await; - match resp { + let typed = match resp { AuthResponse::Secret(p) => p, _ => return Outcome::Skipped, + }; + // The user just typed this one, so a failure here is not + // stale state to heal — it is the answer being wrong, and + // saying so beats silently asking again. + match russh::keys::decode_secret_key(&contents, Some(&typed)) { + Ok(k) => k, + Err(e) => { + log::warn!("could not decrypt identity file {path}: {e}"); + return failed(format!("could not decrypt identity file {path}")); + } } } - }; - match russh::keys::decode_secret_key(&contents, Some(&passphrase)) { - Ok(k) => k, - Err(e) => { - log::warn!("could not decrypt identity file {path}: {e}"); - return failed(format!("could not decrypt identity file {path}")); - } } } Err(e) => { @@ -544,6 +560,16 @@ async fn try_identity_file( } } +/// The passphrase this connection already carries for `raw_path`, if any. +/// +/// The map is keyed by the identity path exactly as the spec lists it — the +/// same string the prompt names and the GUI files the keychain entry under — +/// so the lookup uses the raw path, not the one `expand_identity_path` built +/// for the filesystem. +fn stored_passphrase(spec: &NativeSshSpec, raw_path: &str) -> Option { + spec.key_passphrases.as_ref()?.get(raw_path).cloned() +} + async fn try_agent(handle: &mut Handle, spec: &NativeSshSpec) -> Outcome { #[cfg(unix)] { @@ -863,6 +889,27 @@ mod tests { assert_eq!(hosts, vec!["10.0.0.1".to_string()]); } + fn spec_with(extra: &str) -> NativeSshSpec { + serde_json::from_str(&format!( + r#"{{"host":"h","port":22,"user":"u","auth_mode":"auto"{extra}}}"# + )) + .expect("the minimal spec shape is what the daemon already accepts on the wire") + } + + #[test] + fn a_stored_passphrase_is_found_by_the_path_the_spec_lists() { + // The GUI files the entry under the identity path it put in the spec, + // tilde and all, and `try_identity_file` has to look it up under the + // same string rather than under the filesystem path it expanded to. + let spec = spec_with(r#","key_passphrases":{"~/.ssh/id_ed25519":"pp"}"#); + assert_eq!( + stored_passphrase(&spec, "~/.ssh/id_ed25519").as_deref(), + Some("pp") + ); + assert_eq!(stored_passphrase(&spec, "/home/u/.ssh/id_ed25519"), None); + assert_eq!(stored_passphrase(&spec_with(""), "~/.ssh/id_ed25519"), None); + } + #[test] fn rsa_gets_sha256_others_none() { assert_eq!( diff --git a/src/core/keychain.rs b/src/core/keychain.rs index 7319518b..9308955e 100644 --- a/src/core/keychain.rs +++ b/src/core/keychain.rs @@ -63,7 +63,6 @@ pub trait CredentialStore: Send + Sync { Ok(CredentialRef::key_passphrase(key_sha512_hex.to_string())) } - #[allow(dead_code)] fn delete_key_passphrase(&self, key_sha512_hex: &str) -> CredentialResult<()> { self.delete(SERVICE_KEY_PASSPHRASE, key_sha512_hex) } diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 6914debd..88e2ce0b 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -44,6 +44,9 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::KeepMine => "Keep mine", L10nKey::Dismiss => "Dismiss", L10nKey::StoredPasswordRejected => "The stored password was rejected. Enter a new one.", + L10nKey::StoredPassphraseRejected => { + "The saved passphrase did not unlock this key. Enter the right one." + } L10nKey::Trust => "Trust", L10nKey::Abort => "Abort", L10nKey::HostKeyOverrideMessage => { diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index a8b2cccb..d92baeff 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -46,6 +46,9 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::StoredPasswordRejected => { "保存されたパスワードが拒否されました。新しいパスワードを入力してください" } + L10nKey::StoredPassphraseRejected => { + "保存されたパスフレーズではこの鍵を解除できませんでした。正しいものを入力してください" + } L10nKey::Trust => "信頼する", L10nKey::Abort => "中止", L10nKey::HostKeyOverrideMessage => { diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 489d29f7..86c4e815 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -112,6 +112,7 @@ l10n_keys! { KeepMine, Dismiss, StoredPasswordRejected, + StoredPassphraseRejected, Trust, Abort, HostKeyOverrideMessage, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index ec546971..8fa6a742 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -44,6 +44,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::KeepMine => "保留我的版本", L10nKey::Dismiss => "关闭", L10nKey::StoredPasswordRejected => "已存储的密码被拒绝，请输入新密码。", + L10nKey::StoredPassphraseRejected => "已保存的口令无法解锁该密钥，请输入正确的口令。", L10nKey::Trust => "信任", L10nKey::Abort => "中止", L10nKey::HostKeyOverrideMessage => "输入 yes 覆盖并信任新密钥，或按 Esc 中止。", diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 704e9f3e..7048c942 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -3233,6 +3233,33 @@ impl Tty7App { use crate::core::keychain::{CredentialStore, OsCredentialStore}; let _ = OsCredentialStore.delete_password(&user, &host, port); } + // The same argument for the key passphrases this profile taught the + // app about: the comment above says "the secret", but until now only + // the password was let go of, so a deleted profile stranded its + // passphrase entries with no UI left to reach them. A key is only + // forgotten when no surviving profile still lists it. + use crate::core::keychain::{CredentialStore as _, OsCredentialStore}; + let mine = cfg + .ssh_profiles + .iter() + .find(|p| p.id == id) + .map(|p| p.expanded_identity_files()) + .unwrap_or_default(); + let kept: std::collections::HashSet = cfg + .ssh_profiles + .iter() + .filter(|p| p.id != id) + .flat_map(|p| p.expanded_identity_files()) + .collect(); + for path in mine.iter().filter(|p| !kept.contains(*p)) { + // Keyed by the key file's contents, so a key already gone from + // disk cannot be looked up — and has no live passphrase to leak. + let Ok(bytes) = std::fs::read(path) else { + continue; + }; + let account = crate::core::keychain::key_account_from_contents(&bytes); + let _ = OsCredentialStore.delete_key_passphrase(&account); + } self.update_config(cx, |cfg| { cfg.ssh_profiles.retain(|p| p.id != id); cfg.ssh_profile_frecency.remove(&id); diff --git a/src/ui/ssh_connect.rs b/src/ui/ssh_connect.rs index 06c6699b..615a2919 100644 --- a/src/ui/ssh_connect.rs +++ b/src/ui/ssh_connect.rs @@ -422,6 +422,69 @@ mod tests { assert_eq!(spec.password, None); } + /// Both halves of "remember passphrase" key the keychain entry off the + /// *contents* of the key file, so the read-back here and the write in + /// `ui::ssh_prompt` only ever meet if both expand a leading `~/` first. + /// They do — `expanded_identity_files` runs the path through + /// `expand_tilde` — and this pins that down from the outside: whichever + /// way the profile spells the path, the spec lists it expanded and files + /// the passphrase under that same string, which is the key the daemon's + /// `spec.key_passphrases` lookup uses. + #[test] + fn a_tilde_and_an_absolute_identity_path_resolve_the_same_stored_passphrase() { + let home = crate::core::ssh_profile::expand_tilde("~"); + let dir = tempfile::Builder::new() + .prefix("tty7-key-test") + .tempdir_in(&home) + .expect("the home directory is writable"); + let key = dir.path().join("id_ed25519"); + std::fs::write(&key, b"-----BEGIN OPENSSH PRIVATE KEY-----\nencrypted\n") + .expect("the temp directory is writable"); + + let store = InMemoryCredentialStore::new(); + let account = key_account_from_contents(&std::fs::read(&key).unwrap()); + store.set_key_passphrase(&account, "pp").unwrap(); + + let leaf = dir + .path() + .file_name() + .unwrap() + .to_string_lossy() + .to_string(); + let mut p = profile("web", "10.0.0.5", "deploy"); + p.auth = AuthMode::PublicKey; + + for spelling in [ + format!("~/{leaf}/id_ed25519"), + key.to_string_lossy().to_string(), + ] { + p.identity_files = vec![spelling.clone()]; + let spec = build_native_ssh_spec(&p, &[], &store, true); + let listed = spec + .identity_files + .first() + .expect("the profile lists one key"); + assert!(!listed.starts_with('~'), "{spelling} was left unexpanded"); + assert_eq!( + spec.key_passphrases + .as_ref() + .and_then(|m| m.get(listed)) + .map(String::as_str), + Some("pp"), + "{spelling} should resolve its stored passphrase" + ); + } + + // A mode that will never offer the key does not go looking for its + // secret either. + p.auth = AuthMode::Password; + assert!( + build_native_ssh_spec(&p, &[], &store, true) + .key_passphrases + .is_none() + ); + } + #[test] fn resolves_jump_chain_into_nested_specs() { let bastion = profile("bastion", "bastion.example.com", "jump"); diff --git a/src/ui/ssh_prompt.rs b/src/ui/ssh_prompt.rs index 52346bd9..a7ed39aa 100644 --- a/src/ui/ssh_prompt.rs +++ b/src/ui/ssh_prompt.rs @@ -30,6 +30,7 @@ pub(crate) enum PromptModel { KeyPassphrase { key_path: String, comment: String, + rejected: bool, }, KeyboardInteractive { name: String, @@ -69,6 +70,9 @@ pub(crate) enum KeychainWrite { key_path: String, secret: String, }, + DeleteKeyPassphrase { + key_path: String, + }, } impl PromptModel { @@ -85,9 +89,15 @@ impl PromptModel { port, rejected: auto_supplied_password, }, - AuthPromptKind::KeyPassphrase { key_path, comment } => { - PromptModel::KeyPassphrase { key_path, comment } - } + AuthPromptKind::KeyPassphrase { + key_path, + comment, + rejected, + } => PromptModel::KeyPassphrase { + key_path, + comment, + rejected, + }, AuthPromptKind::KeyboardInteractive { name, instructions, @@ -172,12 +182,20 @@ pub(crate) fn passphrase_submit( key_path: &str, secret: String, remember: bool, + rejected: bool, ) -> (AuthResponse, KeychainWrite) { let write = if remember { KeychainWrite::SetKeyPassphrase { key_path: key_path.to_string(), secret: secret.clone(), } + } else if rejected { + // The stored passphrase is the reason this sheet is up, and the user + // has just declined to save the replacement. Leaving the old one + // behind would hand the same dead secret to the next connection. + KeychainWrite::DeleteKeyPassphrase { + key_path: key_path.to_string(), + } } else { KeychainWrite::None }; @@ -403,9 +421,11 @@ impl Tty7App { let secret = values.first().cloned().unwrap_or_default(); password_submit(user, host, *port, secret, remember, *rejected) } - PromptModel::KeyPassphrase { key_path, .. } => { + PromptModel::KeyPassphrase { + key_path, rejected, .. + } => { let secret = values.first().cloned().unwrap_or_default(); - passphrase_submit(key_path, secret, remember) + passphrase_submit(key_path, secret, remember, *rejected) } PromptModel::KeyboardInteractive { .. } => (ki_submit(values), KeychainWrite::None), PromptModel::HostKeyUnknown { .. } => { @@ -536,6 +556,24 @@ impl Tty7App { Err(e) => log::warn!("not remembering passphrase; cannot read {path}: {e}"), } } + KeychainWrite::DeleteKeyPassphrase { key_path } => { + // Keyed by the key file's contents, exactly as the set arm + // above is — the keychain account for a passphrase is a hash + // of the file, not its path. A delete that keeps failing + // leaves the rejected passphrase in place, and the key stays + // locked out on every later connection with nothing anywhere + // recording why. + let path = crate::core::ssh_profile::expand_tilde(&key_path); + match std::fs::read(&path) { + Ok(bytes) => { + let account = crate::core::keychain::key_account_from_contents(&bytes); + if let Err(e) = store.delete_key_passphrase(&account) { + log::warn!("could not forget key passphrase in keychain: {e}"); + } + } + Err(e) => log::warn!("not forgetting passphrase; cannot read {path}: {e}"), + } + } } } @@ -690,8 +728,15 @@ impl Tty7App { cx, )) } - PromptModel::KeyPassphrase { comment, .. } => { + PromptModel::KeyPassphrase { + comment, rejected, .. + } => { let mut c = card; + if *rejected { + c = c.child(div().text_xs().text_color(danger).child(crate::ui::i18n::t( + crate::ui::i18n::L10nKey::StoredPassphraseRejected, + ))); + } if !comment.is_empty() { c = c.child( div() @@ -964,7 +1009,7 @@ mod tests { #[test] fn passphrase_remember_stores_by_key_path() { - let (_resp, write) = passphrase_submit("/home/u/.ssh/id_ed25519", "pp".into(), true); + let (_resp, write) = passphrase_submit("/home/u/.ssh/id_ed25519", "pp".into(), true, false); assert_eq!( write, KeychainWrite::SetKeyPassphrase { @@ -972,10 +1017,60 @@ mod tests { secret: "pp".into(), } ); - let (_r, w) = passphrase_submit("/k", "pp".into(), false); + let (_r, w) = passphrase_submit("/k", "pp".into(), false, false); assert_eq!(w, KeychainWrite::None); } + /// The passphrase side of `fr_a6_rejected_without_remember_deletes_stale_entry` + /// above. It matters more here than it does for a password: a key + /// passphrase the daemon cannot use is not one wrong login, it is a key + /// that never opens again, and before this the sheet had no way at all to + /// let go of one. + #[test] + fn passphrase_rejected_without_remember_deletes_stale_entry() { + let (resp, write) = passphrase_submit("~/.ssh/id_ed25519", "right".into(), false, true); + assert!(matches!(resp, AuthResponse::Secret(_))); + assert_eq!( + write, + KeychainWrite::DeleteKeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + } + ); + + // Remembering still wins: the new secret replaces the old one, so + // there is nothing left to delete. + let (_r, w) = passphrase_submit("~/.ssh/id_ed25519", "right".into(), true, true); + assert_eq!( + w, + KeychainWrite::SetKeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + secret: "right".into(), + } + ); + } + + #[test] + fn passphrase_prompt_carries_the_daemons_rejection() { + let m = PromptModel::from_prompt( + AuthPromptKind::KeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + comment: String::new(), + rejected: true, + }, + None, + false, + ) + .unwrap(); + assert_eq!( + m, + PromptModel::KeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + comment: String::new(), + rejected: true, + } + ); + } + #[test] fn ki_submit_bundles_all_answers() { assert_eq!(