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