diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index f4055874..103577f3 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -559,6 +559,11 @@ pub enum AuthPromptKind { 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, @@ -2174,6 +2179,7 @@ mod tests { text: "Code:".into(), echo: true, }], + stored_rejected: true, }, }, DaemonMsg::AuthPrompt { @@ -2236,6 +2242,38 @@ mod tests { 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 9c118932..2db296c3 100644 --- a/crates/tty7-core/src/daemon/ssh/auth.rs +++ b/crates/tty7-core/src/daemon/ssh/auth.rs @@ -678,6 +678,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 { @@ -688,9 +690,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 { @@ -709,23 +731,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, @@ -736,6 +767,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, @@ -743,13 +826,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, + }); } } @@ -765,13 +852,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 { @@ -910,6 +1002,44 @@ mod tests { 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!( diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 6ce9ca85..eee2e509 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/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/ssh_prompt.rs b/src/ui/ssh_prompt.rs index a7ed39aa..34fe217b 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 { @@ -36,6 +48,12 @@ pub(crate) enum PromptModel { 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, @@ -78,10 +96,10 @@ pub(crate) enum KeychainWrite { 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, @@ -102,6 +120,7 @@ impl PromptModel { name, instructions, prompts, + stored_rejected, } => PromptModel::KeyboardInteractive { name, instructions, @@ -112,6 +131,8 @@ impl PromptModel { echo: p.echo, }) .collect(), + endpoint, + stored_rejected, }, AuthPromptKind::HostKeyUnknown { host, @@ -202,8 +223,23 @@ pub(crate) fn passphrase_submit( (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 { @@ -308,8 +344,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, @@ -362,7 +403,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()); } @@ -427,7 +472,11 @@ impl Tty7App { let secret = values.first().cloned().unwrap_or_default(); 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) } @@ -755,9 +804,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())); } @@ -943,6 +998,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( @@ -950,7 +1013,7 @@ mod tests { user: "deploy".into(), host: "10.0.0.5".into(), }, - Some(("10.0.0.5".into(), 2222)), + Some(endpoint()), true, ) .unwrap(); @@ -1049,6 +1112,66 @@ mod tests { ); } + #[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!( + 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( @@ -1071,14 +1194,6 @@ mod tests { ); } - #[test] - fn ki_submit_bundles_all_answers() { - assert_eq!( - ki_submit(vec!["a".into(), "b".into()]), - AuthResponse::Secrets(vec!["a".into(), "b".into()]) - ); - } - #[test] fn unknown_host_trust_accepts_and_remembers_abort_rejects() { assert_eq!(