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`.
This commit is contained in:
l0ng-ai
2026-08-11 20:19:19 +08:00
parent d04d104559
commit 82e28784ce
5 changed files with 348 additions and 29 deletions
+38
View File
@@ -559,6 +559,11 @@ pub enum AuthPromptKind {
name: String,
instructions: String,
prompts: Vec<KiPrompt>,
/// 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::<AuthPromptKind>(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::<LegacyPromptKind>(&new).unwrap();
assert_eq!(name, "2FA");
assert_eq!(instructions, "code");
}
#[test]
fn native_ssh_spawn_uses_new_kind_byte() {
let msg = ClientMsg::SpawnNativeSsh {
+143 -13
View File
@@ -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<String>,
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<PromptBroker>,
@@ -743,13 +826,17 @@ async fn collect_ki_answers(
instructions: &str,
prompts: &[russh::client::Prompt],
allow_stored: bool,
) -> Option<Vec<String>> {
stored_rejected: bool,
) -> Option<KiRound> {
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<HashAlg> {
@@ -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!(
+11
View File
@@ -179,6 +179,10 @@ pub struct RemoteTerminal {
auth_prompts: Arc<Mutex<VecDeque<(u64, AuthPromptKind)>>>,
ssh_phase: Arc<Mutex<Option<SshPhase>>>,
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<String>,
auto_supplied_password: bool,
agent: Arc<Mutex<Option<CLIAgent>>>,
agent_session: Arc<Mutex<Option<AgentSessionState>>>,
@@ -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<String> {
self.ssh_user.clone()
}
pub fn auto_supplied_password(&self) -> bool {
self.auto_supplied_password
}
+25
View File
@@ -551,6 +551,18 @@ pub fn take_pending_install() -> Option<PendingInstall> {
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<crate::ui::ssh_prompt::PromptEndpoint>,
/// 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<AuthResponse>,
}
@@ -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,
});
}
+131 -16
View File
@@ -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<KiRow>,
/// `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<PromptEndpoint>,
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<PromptEndpoint>,
auto_supplied_password: bool,
) -> Option<PromptModel> {
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<String>) -> 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<String>,
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!(