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 fa952747..547b6615 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -540,11 +540,30 @@ 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, instructions: String, prompts: Vec, + /// The round that just failed was answered with the stored password + /// rather than by the user, so this prompt exists to replace it. Same + /// compatibility reasoning as `KeyPassphrase::rejected` above. + #[serde(default)] + stored_rejected: bool, }, HostKeyUnknown { host: String, @@ -2193,6 +2212,15 @@ mod tests { text: "Code:".into(), echo: true, }], + stored_rejected: true, + }, + }, + DaemonMsg::AuthPrompt { + request_id: 3, + prompt: AuthPromptKind::KeyPassphrase { + key_path: "~/.ssh/id_ed25519".into(), + comment: String::new(), + rejected: true, }, }, DaemonMsg::SshStatus { @@ -2211,6 +2239,74 @@ 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"); + } + + /// `stored_rejected` gets the same treatment as `rejected` above, for the + /// same reason and with the same `PROTOCOL_VERSION` left alone. + #[test] + fn a_stored_rejected_flag_decodes_from_a_peer_that_never_sends_it() { + let old = r#"{"KeyboardInteractive":{"name":"2FA","instructions":"","prompts":[]}}"#; + assert_eq!( + serde_json::from_str::(old).unwrap(), + AuthPromptKind::KeyboardInteractive { + name: "2FA".into(), + instructions: String::new(), + prompts: vec![], + stored_rejected: false, + } + ); + + let new = serde_json::to_string(&AuthPromptKind::KeyboardInteractive { + name: "2FA".into(), + instructions: "code".into(), + prompts: vec![], + stored_rejected: true, + }) + .unwrap(); + #[derive(Deserialize)] + enum LegacyPromptKind { + KeyboardInteractive { name: String, instructions: String }, + } + let LegacyPromptKind::KeyboardInteractive { name, instructions } = + serde_json::from_str::(&new).unwrap(); + assert_eq!(name, "2FA"); + assert_eq!(instructions, "code"); + } + #[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 873ad841..b031cd8f 100644 --- a/crates/tty7-core/src/daemon/ssh/auth.rs +++ b/crates/tty7-core/src/daemon/ssh/auth.rs @@ -626,7 +626,10 @@ impl KeyRound { /// point (#484 review): russh has no offer-without-signature probe, so /// trying an encrypted key means signing — i.e. prompting *before* the server /// has shown any interest in that key. An explicit key earns that prompt; a -/// discovered default with no cached passphrase does not. +/// discovered default never does — not with no cached passphrase, and not with +/// a cached one that turned out to be wrong (#486), which for an explicit key +/// reopens the prompt but here would mean a sheet per stale `~/.ssh` entry on +/// every connection. enum IdentityLoad { Ready(russh::keys::PrivateKey), /// Not worth an offer: a `.pub`, an undecodable file, or a discovered @@ -634,8 +637,13 @@ enum IdentityLoad { Skip, /// An explicit key the user should hear about. Unusable(String), - /// Explicit, encrypted, no cached passphrase — ask the user. - NeedsPassphrase, + /// Explicit and encrypted, and no passphrase to hand opened it — ask the + /// user. `rejected` says a cached passphrase was tried first and refused, + /// which the sheet has to admit to before asking again (#486); without one + /// this is simply the first time anybody has been asked. + NeedsPassphrase { + rejected: bool, + }, } fn load_identity( @@ -660,19 +668,25 @@ fn load_identity( Some(passphrase) => match russh::keys::decode_secret_key(contents, Some(passphrase)) { Ok(key) => IdentityLoad::Ready(key), Err(e) => { - log::warn!("could not decrypt identity file {raw_path}: {e}"); + log::warn!("the stored passphrase did not decrypt {raw_path}: {e}"); match source { - KeySource::Explicit => IdentityLoad::Unusable(format!( - "could not decrypt identity file {raw_path}" - )), + // Ending the attempt here is what locked an explicit + // key out for good once a wrong passphrase reached the + // keychain: no prompt, and no way to correct it from + // inside the app (#486). The secret is simply wrong, so + // ask — and say that is why. + KeySource::Explicit => IdentityLoad::NeedsPassphrase { rejected: true }, // A stale cached passphrase for a key the user never - // configured: skip, don't shout. + // configured: skip, don't shout — and above all do not + // prompt. #484's rule holds whatever the reason the + // passphrase failed; nobody asked for this key, so it + // must never be the thing that puts a sheet on screen. KeySource::Discovered => IdentityLoad::Skip, } } }, None => match source { - KeySource::Explicit => IdentityLoad::NeedsPassphrase, + KeySource::Explicit => IdentityLoad::NeedsPassphrase { rejected: false }, KeySource::Discovered => IdentityLoad::Skip, }, }, @@ -748,12 +762,12 @@ async fn try_identity_file( } }; - let cached = spec - .key_passphrases - .as_ref() - .and_then(|m| m.get(raw_path)) - .map(String::as_str); - let key = match load_identity(&contents, raw_path, source, cached) { + let key = match load_identity( + &contents, + raw_path, + source, + stored_passphrase(spec, raw_path), + ) { IdentityLoad::Ready(k) => k, IdentityLoad::Skip => return Outcome::Skipped, IdentityLoad::Unusable(reason) => { @@ -763,16 +777,23 @@ async fn try_identity_file( reason: None, }; } - IdentityLoad::NeedsPassphrase => { + // One prompt serves both ways of arriving here — no passphrase to try, + // or one that was tried and refused. `rejected` is the only difference, + // and it only changes what the sheet says (#486). + IdentityLoad::NeedsPassphrase { rejected } => { let resp = broker .prompt(AuthPromptKind::KeyPassphrase { key_path: raw_path.to_string(), comment: String::new(), + rejected, }) .await; let AuthResponse::Secret(passphrase) = resp else { 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 asking again forever. match russh::keys::decode_secret_key(&contents, Some(&passphrase)) { Ok(k) => k, Err(e) => { @@ -815,6 +836,20 @@ 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, the GUI files the keychain entry under, and +/// `default_identity_candidates` spells a discovered key with — so the lookup +/// uses the raw path, not the one `expand_identity_placeholders` built for the +/// filesystem. +fn stored_passphrase<'a>(spec: &'a NativeSshSpec, raw_path: &str) -> Option<&'a str> { + spec.key_passphrases + .as_ref()? + .get(raw_path) + .map(String::as_str) +} + async fn try_agent( handle: &mut Handle, spec: &NativeSshSpec, @@ -933,6 +968,8 @@ async fn try_keyboard_interactive( const MAX_ROUNDS: u32 = 16; let mut rounds = 0u32; let mut stored_password_used = false; + let mut stored_password_rejected = false; + let mut last_source = KiAnswerSource::Nothing; loop { rounds += 1; if rounds > MAX_ROUNDS { @@ -943,9 +980,29 @@ async fn try_keyboard_interactive( KeyboardInteractiveAuthResponse::Failure { remaining_methods, .. } => { + // OpenSSH ends a rejected kbdint request with a plain + // USERAUTH_FAILURE rather than another info request, so a + // round answered from the keychain used to end the method + // right here — the same stale password on every reconnect, + // and the user never once asked to type a different one. + // Start the request over instead, with the stored password + // now spent, so the next round reaches the prompt. + if should_retry_ki(last_source, &remaining_methods) { + stored_password_used = true; + stored_password_rejected = true; + last_source = KiAnswerSource::Nothing; + resp = match handle + .authenticate_keyboard_interactive_start(&spec.user, None) + .await + { + Ok(r) => r, + Err(e) => return failed(format!("keyboard-interactive start error: {e}")), + }; + continue; + } return Outcome::Failed { remaining_methods: Some(remaining_methods), - reason: Some("keyboard-interactive rejected".to_string()), + reason: Some(ki_rejection_reason(last_source, stored_password_rejected)), }; } KeyboardInteractiveAuthResponse::InfoRequest { @@ -964,23 +1021,32 @@ async fn try_keyboard_interactive( continue; } - let allow_stored = !stored_password_used; - stored_password_used = true; - let answers = match collect_ki_answers( + // A device that asks again inside the same request has already + // turned the stored password down, exactly as a failed request + // that had to be restarted has. + stored_password_rejected |= last_source == KiAnswerSource::Stored; + let round = match collect_ki_answers( spec, broker, &name, &instructions, &prompts, - allow_stored, + !stored_password_used, + stored_password_rejected, ) .await { Some(a) => a, None => return failed("keyboard-interactive cancelled"), }; + // Only a round that actually sent the stored password spends + // it. Marking it spent for every round refused it to an + // OTP-then-password flow, where the first round is the code + // and the password is not asked for until the second. + last_source = round.source; + stored_password_used |= round.source == KiAnswerSource::Stored; resp = match handle - .authenticate_keyboard_interactive_respond(answers) + .authenticate_keyboard_interactive_respond(round.answers) .await { Ok(r) => r, @@ -991,6 +1057,58 @@ async fn try_keyboard_interactive( } } +/// Where the answers of the keyboard-interactive round that just went out came +/// from. It decides both whether a rejection is worth starting over for and +/// what to tell the user the server turned down. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KiAnswerSource { + /// No round has answered yet — the server refused the method before it + /// asked anything. + Nothing, + Stored, + Typed, +} + +struct KiRound { + answers: Vec, + source: KiAnswerSource, +} + +/// A rejection is only worth a second request when the round the server turned +/// down was answered from the keychain: nobody has been asked anything yet, so +/// the attempt has not actually been spent. An answer the user typed is their +/// answer, and re-asking for it in a loop is what a rejecting server would +/// like us to do. +/// +/// An empty `remaining_methods` is read as "the server did not say" and left +/// retryable, which is how `try_gssapi` above reads it too. The retry cannot +/// run away: it is reached only from `KiAnswerSource::Stored`, and the restart +/// spends the stored password, so no second restart can ever qualify — and the +/// round counter it shares with the info-request loop caps the whole method +/// either way. +fn should_retry_ki(last_source: KiAnswerSource, remaining: &MethodSet) -> bool { + last_source == KiAnswerSource::Stored + && (remaining.is_empty() || remaining.contains(&MethodKind::KeyboardInteractive)) +} + +/// "keyboard-interactive rejected" answered for three different situations, +/// and the one worth naming is the stored password: the user typed nothing, so +/// a message about their answer sends them looking for a typo they never made. +/// `stored_rejected` carries that across a restarted request, where the round +/// that spent the stored password belongs to the request before this one. +fn ki_rejection_reason(last_source: KiAnswerSource, stored_rejected: bool) -> String { + match last_source { + KiAnswerSource::Typed => "keyboard-interactive: your answer was rejected".to_string(), + KiAnswerSource::Stored => { + "keyboard-interactive: the stored password was rejected".to_string() + } + KiAnswerSource::Nothing if stored_rejected => { + "keyboard-interactive: the stored password was rejected".to_string() + } + KiAnswerSource::Nothing => "keyboard-interactive rejected".to_string(), + } +} + async fn collect_ki_answers( spec: &NativeSshSpec, broker: &Arc, @@ -998,13 +1116,17 @@ async fn collect_ki_answers( instructions: &str, prompts: &[russh::client::Prompt], allow_stored: bool, -) -> Option> { + stored_rejected: bool, +) -> Option { let all_password_type = prompts .iter() .all(|p| !p.echo && p.prompt.to_lowercase().contains("password")); if all_password_type && allow_stored { if let Some(pw) = &spec.password { - return Some(prompts.iter().map(|_| pw.clone()).collect()); + return Some(KiRound { + answers: prompts.iter().map(|_| pw.clone()).collect(), + source: KiAnswerSource::Stored, + }); } } @@ -1020,13 +1142,18 @@ async fn collect_ki_answers( name: name.to_string(), instructions: instructions.to_string(), prompts: ki_prompts, + stored_rejected, }) .await; - match resp { - AuthResponse::Secrets(v) if v.len() == prompts.len() => Some(v), - AuthResponse::Secret(s) if prompts.len() == 1 => Some(vec![s]), - _ => None, - } + let answers = match resp { + AuthResponse::Secrets(v) if v.len() == prompts.len() => v, + AuthResponse::Secret(s) if prompts.len() == 1 => vec![s], + _ => return None, + }; + Some(KiRound { + answers, + source: KiAnswerSource::Typed, + }) } fn rsa_hash_alg(algorithm: &Algorithm) -> Option { @@ -1118,6 +1245,62 @@ 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"), 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 only_a_stored_answer_earns_a_second_keyboard_interactive_request() { + let offers = MethodSet::from(&[MethodKind::KeyboardInteractive][..]); + + // Nobody was asked anything, so nothing has been spent yet. + assert!(should_retry_ki(KiAnswerSource::Stored, &offers)); + + // The user answered and was turned down; asking them again in a loop + // is what a rejecting server would like us to do. + assert!(!should_retry_ki(KiAnswerSource::Typed, &offers)); + assert!(!should_retry_ki(KiAnswerSource::Nothing, &offers)); + + // A server that no longer offers the method cannot be restarted into + // it; one that said nothing about what is left still can. + let elsewhere = MethodSet::from(&[MethodKind::PublicKey][..]); + assert!(!should_retry_ki(KiAnswerSource::Stored, &elsewhere)); + assert!(should_retry_ki(KiAnswerSource::Stored, &MethodSet::empty())); + } + + #[test] + fn a_rejection_says_whose_answer_it_was() { + let stored = ki_rejection_reason(KiAnswerSource::Stored, true); + assert!(stored.contains("stored password"), "{stored}"); + + let typed = ki_rejection_reason(KiAnswerSource::Typed, true); + assert!(typed.contains("your answer"), "{typed}"); + + // The restarted request carries the stored rejection across, even + // though its own rounds never sent anything. + let carried = ki_rejection_reason(KiAnswerSource::Nothing, true); + assert!(carried.contains("stored password"), "{carried}"); + + // A server that refused the method outright blames neither. + let neither = ki_rejection_reason(KiAnswerSource::Nothing, false); + assert!(!neither.contains("stored password"), "{neither}"); + assert!(!neither.contains("your answer"), "{neither}"); + } + #[test] fn rsa_gets_sha256_others_none() { assert_eq!( @@ -1296,7 +1479,7 @@ mod tests { // rather than spending a prompt on a key the server may not want. assert!(matches!( load_identity(&encrypted_key(), "k", KeySource::Explicit, None), - IdentityLoad::NeedsPassphrase + IdentityLoad::NeedsPassphrase { rejected: false } )); assert!(matches!( load_identity(&encrypted_key(), "k", KeySource::Discovered, None), @@ -1318,17 +1501,44 @@ mod tests { } #[test] - fn load_identity_wrong_cached_passphrase_is_loud_only_for_explicit() { + fn load_identity_wrong_cached_passphrase_asks_again_only_for_explicit() { + // #486 inside #484's matrix. A wrong stored passphrase used to be the + // end of an explicit key: `Unusable`, so "could not decrypt identity + // file" with no way to correct the secret from inside the app. It now + // reopens the prompt, flagged so the sheet can say the saved one was + // refused. assert!(matches!( load_identity(&encrypted_key(), "k", KeySource::Explicit, Some("wrong")), - IdentityLoad::Unusable(_) + IdentityLoad::NeedsPassphrase { rejected: true } )); + // The discovered half is the one that must not move: a `~/.ssh` default + // nobody configured stays silent whether its cached passphrase is + // absent or stale, so a stale entry cannot turn every connection into a + // prompt for a key the user never asked to use. assert!(matches!( load_identity(&encrypted_key(), "k", KeySource::Discovered, Some("wrong")), IdentityLoad::Skip )); } + #[test] + fn no_discovered_key_ever_asks_for_a_passphrase() { + // The seam where #484 and #486 meet: the self-heal reopens a prompt on + // a refused passphrase, and the probe hands this function keys the user + // never named. Whatever a discovered candidate's state, it must never + // be the thing that puts a sheet on screen — several of them would + // otherwise queue up a prompt storm on every connection. + for cached in [None, Some("wrong"), Some(PASSPHRASE)] { + assert!( + !matches!( + load_identity(&encrypted_key(), "k", KeySource::Discovered, cached), + IdentityLoad::NeedsPassphrase { .. } + ), + "a discovered key must not prompt (cached: {cached:?})" + ); + } + } + #[test] fn reason_names_the_keys_the_server_rejected() { let mut round = KeyRound::default(); 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/terminal/remote.rs b/src/terminal/remote.rs index e57588e1..8f4d1e0e 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -179,6 +179,10 @@ pub struct RemoteTerminal { auth_prompts: Arc>>, ssh_phase: Arc>>, ssh_endpoint: Option<(String, u16)>, + /// The account the SSH connection authenticates as. `ssh_endpoint` is what + /// the disconnect strip and the forward sheet need; the keychain files a + /// password under the user as well, so the auth sheet needs this too. + ssh_user: Option, auto_supplied_password: bool, agent: Arc>>, agent_session: Arc>>, @@ -551,6 +555,7 @@ impl RemoteTerminal { auth_prompts, ssh_phase, ssh_endpoint: None, + ssh_user: None, auto_supplied_password: false, agent, agent_session, @@ -1325,6 +1330,7 @@ impl RemoteTerminal { let mut stream = connect()?; let win = win_size(size, cell_w, cell_h); let endpoint = (spec.host.clone(), spec.port); + let user = spec.user.clone(); let auto_supplied_password = spec.password.is_some(); ClientMsg::SpawnNativeSsh { @@ -1347,6 +1353,7 @@ impl RemoteTerminal { let mut term = Self::from_stream(stream, size)?; term.ssh_endpoint = Some(endpoint); + term.ssh_user = Some(user); term.auto_supplied_password = auto_supplied_password; Ok((term, pane_id)) } @@ -1383,6 +1390,10 @@ impl RemoteTerminal { self.ssh_endpoint.clone() } + pub fn ssh_user(&self) -> Option { + self.ssh_user.clone() + } + pub fn auto_supplied_password(&self) -> bool { self.auto_supplied_password } diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 0557be92..a2630d2b 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -45,6 +45,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 b433a9a0..b035b2a2 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -47,6 +47,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 a47b27a8..e41833ab 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -113,6 +113,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 3fbd3690..63584aa4 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -45,6 +45,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/remote_connect.rs b/src/ui/remote_connect.rs index 139004eb..082e10af 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -551,6 +551,18 @@ pub fn take_pending_install() -> Option { pub struct PendingAuth { pub host: HostId, pub prompt: AuthPromptKind, + /// Which connection is asking, when the route is an SSH hop. The sheet + /// files and forgets keychain entries under this; a route that is not SSH + /// (WSL, a local stdio server) has no endpoint to name and gets `None`. + /// + /// Without it the sheet fell back to port 22 and a hard-coded "not + /// auto-supplied", so a routed prompt for a non-22 endpoint wrote its + /// password under the wrong key and a rejected stored one was never + /// noticed, let alone cleared. + pub endpoint: Option, + /// The route already carried a stored password into this attempt, so a + /// password prompt arriving anyway means the server turned it down. + pub auto_supplied_password: bool, reply: std::sync::mpsc::SyncSender, } @@ -612,6 +624,17 @@ impl crate::daemon::router::RouteAuthResponder for GuiRouteAuth { ) -> AuthResponse { let key = machine.origin_key(); let host = origin_host(&key).unwrap_or_else(|| HostId::from_connection_key(&key)); + let (endpoint, auto_supplied_password) = match machine { + crate::daemon::router::RouteTarget::Ssh(spec) => ( + Some(crate::ui::ssh_prompt::PromptEndpoint { + user: spec.user.clone(), + host: spec.host.clone(), + port: spec.port, + }), + spec.password.is_some(), + ), + _ => (None, false), + }; let (tx, rx) = std::sync::mpsc::sync_channel(1); { let Ok(mut mailbox) = AUTH_MAILBOX.lock() else { @@ -620,6 +643,8 @@ impl crate::daemon::router::RouteAuthResponder for GuiRouteAuth { mailbox.push(PendingAuth { host, prompt: prompt.clone(), + endpoint, + auto_supplied_password, reply: tx, }); } diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 6ba56f50..96803083 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -3491,6 +3491,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 f637bb18..9dc31482 100644 --- a/src/ui/ssh_connect.rs +++ b/src/ui/ssh_connect.rs @@ -437,6 +437,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 ab3a7acf..9a31c98b 100644 --- a/src/ui/ssh_prompt.rs +++ b/src/ui/ssh_prompt.rs @@ -19,6 +19,18 @@ pub(crate) struct KiRow { pub echo: bool, } +/// The connection a prompt belongs to, which is also the key the keychain +/// files its password under. A password prompt names its own user and host, +/// but nothing on the wire carries the port and a keyboard-interactive prompt +/// names none of the three — so whoever raises the sheet has to supply what it +/// knows about the connection. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PromptEndpoint { + pub user: String, + pub host: String, + pub port: u16, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PromptModel { Password { @@ -30,11 +42,18 @@ pub(crate) enum PromptModel { KeyPassphrase { key_path: String, comment: String, + rejected: bool, }, KeyboardInteractive { name: String, instructions: String, prompts: Vec, + /// `None` when nothing told the sheet which connection is asking — a + /// route the GUI could not resolve to an SSH hop. Without it there is + /// no keychain entry to name, so a rejected stored password can only + /// be re-typed, not forgotten. + endpoint: Option, + stored_rejected: bool, }, HostKeyUnknown { host: String, @@ -72,15 +91,18 @@ pub(crate) enum KeychainWrite { key_path: String, secret: String, }, + DeleteKeyPassphrase { + key_path: String, + }, } impl PromptModel { pub(crate) fn from_prompt( kind: AuthPromptKind, - endpoint: Option<(String, u16)>, + endpoint: Option, auto_supplied_password: bool, ) -> Option { - let port = endpoint.as_ref().map(|(_, p)| *p).unwrap_or(22); + let port = endpoint.as_ref().map(|e| e.port).unwrap_or(22); Some(match kind { AuthPromptKind::Password { user, host } => PromptModel::Password { user, @@ -88,13 +110,20 @@ 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, prompts, + stored_rejected, } => PromptModel::KeyboardInteractive { name, instructions, @@ -105,6 +134,8 @@ impl PromptModel { echo: p.echo, }) .collect(), + endpoint, + stored_rejected, }, AuthPromptKind::HostKeyUnknown { host, @@ -177,20 +208,43 @@ 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 }; (AuthResponse::Secret(secret), write) } -pub(crate) fn ki_submit(answers: Vec) -> AuthResponse { - AuthResponse::Secrets(answers) +/// Keyboard-interactive answers are never saved — one of them is as likely to +/// be a one-time code as a password — so this side only ever *forgets*: the +/// stored password the daemon says the server just turned down. +pub(crate) fn ki_submit( + endpoint: Option<&PromptEndpoint>, + answers: Vec, + stored_rejected: bool, +) -> (AuthResponse, KeychainWrite) { + let write = match endpoint.filter(|_| stored_rejected) { + Some(e) => KeychainWrite::DeletePassword { + user: e.user.clone(), + host: e.host.clone(), + port: e.port, + }, + None => KeychainWrite::None, + }; + (AuthResponse::Secrets(answers), write) } pub(crate) fn host_key_unknown_decision(trust: bool) -> AuthResponse { @@ -295,8 +349,13 @@ impl Tty7App { } } } + let endpoint = term.ssh_endpoint().map(|(host, port)| PromptEndpoint { + user: term.ssh_user().unwrap_or_default(), + host, + port, + }); ( - term.ssh_endpoint(), + endpoint, term.auto_supplied_password(), term.ssh_phase(), banners, @@ -353,7 +412,11 @@ impl Tty7App { if self.ssh_prompt.model.is_some() { return SheetOutcome::GiveBack(pending); } - let Some(model) = PromptModel::from_prompt(pending.prompt.clone(), None, false) else { + let Some(model) = PromptModel::from_prompt( + pending.prompt.clone(), + pending.endpoint.clone(), + pending.auto_supplied_password, + ) else { if let AuthPromptKind::Banner { text } = &pending.prompt { self.ssh_prompt.banners.push(text.clone()); } @@ -426,11 +489,17 @@ 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::KeyboardInteractive { + endpoint, + stored_rejected, + .. + } => ki_submit(endpoint.as_ref(), values, *stored_rejected), PromptModel::HostKeyUnknown { .. } => { (host_key_unknown_decision(true), KeychainWrite::None) } @@ -559,6 +628,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}"), + } + } } } @@ -713,8 +800,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() @@ -733,9 +827,15 @@ impl Tty7App { PromptModel::KeyboardInteractive { instructions, prompts, + stored_rejected, .. } => { let mut c = card; + if *stored_rejected { + c = c.child(div().text_xs().text_color(danger).child(crate::ui::i18n::t( + crate::ui::i18n::L10nKey::StoredPasswordRejected, + ))); + } if !instructions.is_empty() { c = c.child(div().text_xs().child(instructions.clone())); } @@ -963,6 +1063,14 @@ fn build_inputs( mod tests { use super::*; + fn endpoint() -> PromptEndpoint { + PromptEndpoint { + user: "deploy".into(), + host: "10.0.0.5".into(), + port: 2222, + } + } + #[test] fn password_prompt_carries_port_from_endpoint_and_marks_rejection() { let m = PromptModel::from_prompt( @@ -970,7 +1078,7 @@ mod tests { user: "deploy".into(), host: "10.0.0.5".into(), }, - Some(("10.0.0.5".into(), 2222)), + Some(endpoint()), true, ) .unwrap(); @@ -1029,7 +1137,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 { @@ -1037,15 +1145,117 @@ 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 ki_submit_bundles_all_answers() { + let (resp, write) = ki_submit(Some(&endpoint()), vec!["a".into(), "b".into()], false); + assert_eq!(resp, AuthResponse::Secrets(vec!["a".into(), "b".into()])); + assert_eq!(write, KeychainWrite::None); + } + + /// Keyboard-interactive has no "remember" of its own — an answer may well + /// be a one-time code — so the only keychain move it makes is letting go + /// of the stored password the server just turned down. Without it that + /// password was replayed into every later connection, and the prompt the + /// user answered here never became the one the next attempt sent. + #[test] + fn ki_answer_forgets_the_stored_password_the_server_rejected() { + let (resp, write) = ki_submit(Some(&endpoint()), vec!["typed".into()], true); + assert_eq!(resp, AuthResponse::Secrets(vec!["typed".into()])); assert_eq!( - ki_submit(vec!["a".into(), "b".into()]), - AuthResponse::Secrets(vec!["a".into(), "b".into()]) + write, + KeychainWrite::DeletePassword { + user: "deploy".into(), + host: "10.0.0.5".into(), + port: 2222, + } + ); + + // A prompt nobody could tie to an endpoint has no entry to name, so it + // must not guess one — deleting the wrong account is worse than + // leaving the right one in place. + let (_r, w) = ki_submit(None, vec!["typed".into()], true); + assert_eq!(w, KeychainWrite::None); + } + + /// The port has to come from the connection, not from the default: a + /// prompt for `:2222` that files its keychain entry under `:22` writes a + /// secret nothing ever reads back. + #[test] + fn keyboard_interactive_carries_the_endpoint_and_the_rejection_through() { + let m = PromptModel::from_prompt( + AuthPromptKind::KeyboardInteractive { + name: "2FA".into(), + instructions: "code please".into(), + prompts: vec![], + stored_rejected: true, + }, + Some(endpoint()), + false, + ) + .unwrap(); + assert_eq!( + m, + PromptModel::KeyboardInteractive { + name: "2FA".into(), + instructions: "code please".into(), + prompts: vec![], + endpoint: Some(endpoint()), + stored_rejected: true, + } + ); + } + + #[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, + } ); }