From bf9c57dec74df3068e7ce9194a9a11481d5c6c42 Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Tue, 11 Aug 2026 21:40:49 +0800 Subject: [PATCH] fix(ssh): let a rejected stored credential ask again (#519) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ssh): let a rejected stored passphrase ask again (#486) 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. * fix(ssh): stop replaying a stale password at keyboard-interactive (#487) `try_keyboard_interactive` answered a password-shaped round from the keychain, marked the stored password spent whether or not it had been used, and returned on the first `Failure` — so the `MAX_ROUNDS` loop never got a second pass with the stored password withheld. The same dead secret went out on every reconnect and the user was never once asked to type a different one; `ki_submit` always emitted `KeychainWrite::None`, so nothing could clear it either. `collect_ki_answers` now reports where its answers came from, and only a round that actually sent the stored password spends it — which also fixes an OTP-then-password flow that was refusing the stored password for no reason, its first round having burned the allowance on a code. On a rejection whose last round came from the keychain, and where the server still offers the method, the request is started over with the stored password withheld, so the next round reaches the prompt. That retry is bounded twice over: the restart spends the stored password, so no second restart can qualify, and the round counter it shares with the info-request loop caps the method either way. The failure text now says which of the two was turned down. Scope, honestly: the only live scenario is auth mode Auto against a server offering keyboard-interactive but not password, with a stored password for that endpoint — a profile pinned to KeyboardInteractive gets `password: None` and always prompts, and Password never tries KI. Whether the symptom shows also depends on the server: OpenSSH ends a rejected kbdint request with USERAUTH_FAILURE (symptom holds), while a device that re-issues an InfoRequest in the same request already reached the prompt. `AuthPromptKind::KeyboardInteractive` grows a `#[serde(default)]` `stored_rejected`, same both-directions compatibility as `KeyPassphrase`'s `rejected` and the same reason `PROTOCOL_VERSION` stays put. The sheet shows the warning line and, on submit, forgets the rejected password. That needed an endpoint the KI prompt does not carry, which also fixed a bug next door: `raise_routed_auth` called `from_prompt(.., None, false)`, so every routed password write was keyed to port 22 regardless of the real port and the rejected self-heal could never fire there. `PendingAuth` now carries the endpoint and the auto-supplied flag, read straight off the route's `NativeSshSpec`. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> --- .github/scripts/check-host-boundary.sh | 1 + crates/tty7-core/src/daemon/protocol.rs | 96 +++++++++ crates/tty7-core/src/daemon/ssh/auth.rs | 274 +++++++++++++++++++++--- src/core/keychain.rs | 1 - src/terminal/remote.rs | 11 + 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/remote_connect.rs | 25 +++ src/ui/settings.rs | 27 +++ src/ui/ssh_connect.rs | 63 ++++++ src/ui/ssh_prompt.rs | 246 +++++++++++++++++++-- 13 files changed, 701 insertions(+), 51 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 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, + } ); }