Merge pull request #827 from l0ng-ai/fix/ssh-repeating-password-prompt-820

fix(ssh): stop the password prompt coming back after a reconnect (#820)
This commit is contained in:
l0ng-ai
2026-09-10 13:58:52 +08:00
committed by GitHub
4 changed files with 485 additions and 19 deletions
+157 -2
View File
@@ -49,6 +49,13 @@ pub async fn authenticate(
};
match outcome {
Outcome::Authenticated => return Ok(()),
// The user turned the question down. `password` and
// `keyboard-interactive` are one question asked two ways — a
// server offering both wants the same secret either way — so
// walking on to the next of them put the sheet the user had just
// closed straight back on screen, and on a link that reconnects by
// itself it kept coming back (#820). Nobody declined a *method*.
Outcome::Declined => return Err(AUTH_DECLINED.to_string()),
Outcome::Failed {
remaining_methods,
reason,
@@ -121,8 +128,28 @@ fn method_order(mode: SshAuthMode) -> Vec<MethodKind> {
}
}
/// What the whole attempt failed with when the person at the keyboard closed
/// the prompt. Distinct wording on purpose: a caller that retries — the
/// workspace supervisor reconnects on a clock — can tell a refusal it should
/// stop repeating from a credential that was merely wrong.
pub const AUTH_DECLINED: &str = "authentication cancelled";
/// Whether a failure is the one above, seen from wherever it ended up.
///
/// The reason travels a long way — route ack, `io::Error`, and a localised
/// "could not reach {machine}: {error}" around the outside — so this asks
/// whether the message *carries* the refusal rather than whether it is one,
/// exactly as `control::is_dialect_refusal` does with its own marker.
pub fn is_auth_declined(message: &str) -> bool {
message.contains(AUTH_DECLINED)
}
enum Outcome {
Authenticated,
/// Nobody answered the prompt this method raised: the user closed it, or
/// no window was there to show it. Either way the attempt is over — see
/// the arm in [`authenticate`].
Declined,
Failed {
remaining_methods: Option<MethodSet>,
reason: Option<String>,
@@ -456,6 +483,7 @@ async fn try_publickeys(
};
match outcome {
Outcome::Authenticated => return Outcome::Authenticated,
Outcome::Declined => return Outcome::Declined,
Outcome::Failed {
remaining_methods, ..
} => {
@@ -716,6 +744,7 @@ async fn try_identity_files(
for (path, source) in files {
match try_identity_file(handle, spec, broker, path, *source, round).await {
Outcome::Authenticated => return Outcome::Authenticated,
Outcome::Declined => return Outcome::Declined,
Outcome::Failed {
remaining_methods, ..
} => {
@@ -788,6 +817,12 @@ async fn try_identity_file(
rejected,
})
.await;
// Skipped, not `Declined`, and on purpose. Closing this sheet
// declines *this key*, and the methods still to come ask a
// different question — "your password" is not "the passphrase for
// id_rsa", and someone who cannot remember the passphrase is
// usually closing it precisely to be asked the other one. What
// #820 is about is the two prompts that ask the same thing.
let AuthResponse::Secret(passphrase) = resp else {
return Outcome::Skipped;
};
@@ -938,7 +973,7 @@ async fn try_password(
.await;
let pw = match resp {
AuthResponse::Secret(p) => p,
_ => return failed("password entry cancelled"),
_ => return Outcome::Declined,
};
match handle.authenticate_password(&spec.user, pw).await {
Ok(AuthResult::Success) => Outcome::Authenticated,
@@ -1037,7 +1072,7 @@ async fn try_keyboard_interactive(
.await
{
Some(a) => a,
None => return failed("keyboard-interactive cancelled"),
None => return Outcome::Declined,
};
// Only a round that actually sent the stored password spends
// it. Marking it spent for every round refused it to an
@@ -1167,6 +1202,126 @@ fn rsa_hash_alg(algorithm: &Algorithm) -> Option<HashAlg> {
#[cfg(test)]
mod tests {
use super::*;
use crate::daemon::ssh::test_support::PasswordFake;
use std::sync::{Mutex, OnceLock};
/// A broker standing in for the window: it records the kind of every
/// prompt that reaches it and answers each from a script, at once.
///
/// Answering from inside the emit closure works for the same reason
/// `declining_broker` does — `PromptBroker::prompt` files the waiting
/// sender before it emits — and it keeps these tests off the two-minute
/// prompt timeout.
fn scripted_broker(script: Vec<AuthResponse>) -> (Arc<Mutex<Vec<String>>>, Arc<PromptBroker>) {
let seen: Arc<Mutex<Vec<String>>> = Arc::new(Mutex::new(Vec::new()));
let script = Arc::new(Mutex::new(std::collections::VecDeque::from(script)));
let back: Arc<OnceLock<std::sync::Weak<PromptBroker>>> = Arc::new(OnceLock::new());
let asked = Arc::clone(&seen);
let emit_back = Arc::clone(&back);
let broker = PromptBroker::new(Box::new(move |msg| {
let crate::daemon::protocol::DaemonMsg::AuthPrompt { request_id, prompt } = msg else {
return true;
};
let label = match prompt {
AuthPromptKind::Password { .. } => "password",
AuthPromptKind::KeyboardInteractive { .. } => "keyboard-interactive",
AuthPromptKind::KeyPassphrase { .. } => "key-passphrase",
AuthPromptKind::Banner { .. } => return true,
_ => "host-key",
};
asked.lock().unwrap().push(label.to_string());
let answer = script
.lock()
.unwrap()
.pop_front()
.unwrap_or(AuthResponse::Cancelled);
if let Some(broker) = emit_back.get().and_then(std::sync::Weak::upgrade) {
broker.deliver(request_id, answer);
}
true
}));
let _ = back.set(Arc::downgrade(&broker));
(seen, broker)
}
fn runtime() -> tokio::runtime::Runtime {
tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.expect("build test runtime")
}
/// #820. A server that offers `password` *and* `keyboard-interactive` is
/// offering two ways to hand over one secret. Closing the sheet used to
/// fail only the method that raised it, so the very next thing the user
/// saw was the other method asking for the same password — which, on a
/// link that redials by itself, is a window that cannot be closed.
#[test]
fn closing_the_password_sheet_ends_the_attempt_rather_than_asking_again() {
runtime().block_on(async {
let mut fake = PasswordFake::connect("hunter2").await;
let (seen, broker) = scripted_broker(vec![AuthResponse::Cancelled]);
let err = authenticate(&mut fake.handle, &fake.spec, &broker)
.await
.expect_err("a declined prompt cannot authenticate");
assert_eq!(
seen.lock().unwrap().as_slice(),
["password"],
"one question was declined, so no second question is asked"
);
assert!(err.contains(AUTH_DECLINED), "{err}");
assert_eq!(
fake.kbdint_attempts(),
0,
"keyboard-interactive must not even reach the wire"
);
});
}
/// The other half of the rule: it is the *decline* that ends the attempt,
/// not a prompt having happened. A password the server turns down is a
/// wrong answer, and the method behind it is still worth trying.
#[test]
fn a_password_the_server_rejects_still_falls_through_to_the_next_method() {
runtime().block_on(async {
let mut fake = PasswordFake::connect("hunter2").await;
let (seen, broker) = scripted_broker(vec![
AuthResponse::Secret("wrong".into()),
AuthResponse::Cancelled,
]);
let err = authenticate(&mut fake.handle, &fake.spec, &broker)
.await
.expect_err("neither answer was the password");
assert_eq!(
seen.lock().unwrap().as_slice(),
["password", "keyboard-interactive"],
"a rejected answer is not a refusal to answer"
);
assert!(err.contains(AUTH_DECLINED), "{err}");
assert_eq!(fake.password_attempts(), 1);
});
}
#[test]
fn the_password_the_user_types_is_asked_for_once_and_authenticates() {
runtime().block_on(async {
let mut fake = PasswordFake::connect("hunter2").await;
let (seen, broker) = scripted_broker(vec![AuthResponse::Secret("hunter2".into())]);
authenticate(&mut fake.handle, &fake.spec, &broker)
.await
.expect("the right password authenticates");
assert_eq!(seen.lock().unwrap().as_slice(), ["password"]);
assert_eq!(fake.password_attempts(), 1);
assert_eq!(fake.kbdint_attempts(), 0);
});
}
#[test]
fn a_round_with_no_attempt_says_so_instead_of_saying_it_failed() {
+87 -16
View File
@@ -14,6 +14,7 @@ pub(crate) mod test_support;
pub use connect::ProcessStream;
pub use auth::{AUTH_DECLINED, is_auth_declined};
pub use broker::PromptBroker;
pub use forward::SshForwardRegistry;
pub use session::{ChannelCmd, SharedConnection, SshConnection, SshSessionHandle};
@@ -246,6 +247,12 @@ impl SshManager {
);
conn.mark_dead();
self.evict_connection(conn.key());
// Back through `open_connection`, which takes this key's slot
// again. Every other pane that was riding the same dead link
// is arriving here at the same moment — one dropped TCP
// connection kills all of them together — and the slot is the
// only thing that stops each of them dialling, and prompting,
// on its own.
let (fresh, _) = self
.open_connection(spec, broker)
.await
@@ -407,8 +414,36 @@ impl SshManager {
.block_on(self.open_remote_link(spec, setup, server_command))
}
/// Forget the connection this key was serving, keeping the slot that
/// serves it.
///
/// The slot is not bookkeeping: it is the mutual exclusion every dial to
/// this host queues on, and it is what makes one reconnect ask for a
/// password once instead of once per pane. Removing the entry threw that
/// away. A dropped link takes every pane on it down together, so all of
/// them reach the dead-reuse branch in `run_session` at the same moment;
/// the first to evict left the map empty, the next `open_connection`
/// inserted a brand-new mutex, and the pane behind it evicted *that* one —
/// the one a dial was already holding — and inserted another. Each pane
/// ended up queueing on a mutex of its own, so each ran its own handshake
/// and raised its own password prompt: answer one, the connection comes up,
/// and the next prompt is still on screen with more behind it (#820).
///
/// So the entry stays for the life of the process and only what it points
/// at is dropped. That is what the map already looks like in the ordinary
/// case — nothing else has ever removed an entry, and `routes()` reports a
/// slot whose connection is gone as disconnected rather than omitting it.
///
/// A slot somebody is dialling on is left completely alone: that dial is
/// about to overwrite the connection anyway, and the point of this function
/// is to not disturb it.
fn evict_connection(&self, key: &ConnectionKey) {
self.conns.lock().unwrap().remove(key);
let slot = self.conns.lock().unwrap().get(key).cloned();
if let Some(slot) = slot
&& let Ok(mut held) = slot.try_lock()
{
*held = Weak::new();
}
}
pub fn routes(&self) -> Vec<crate::daemon::control::RouteInfo> {
@@ -755,28 +790,64 @@ mod tests {
);
}
#[test]
fn evict_connection_clears_the_registry_slot() {
let runtime = tokio::runtime::Builder::new_current_thread()
.build()
.expect("build test runtime");
let mgr = SshManager {
runtime,
fn bare_manager() -> SshManager {
SshManager {
runtime: tokio::runtime::Builder::new_current_thread()
.build()
.expect("build test runtime"),
conns: Mutex::new(HashMap::new()),
forwards: SshForwardRegistry::default(),
probes: Mutex::new(HashMap::new()),
};
}
}
/// #820. Eviction drops the connection and keeps the slot, because the
/// slot is what every dial to this host queues on. Handing the next dial a
/// mutex of its own is what turned one reconnect into one password prompt
/// per pane.
#[test]
fn evict_connection_empties_the_slot_without_replacing_it() {
let mgr = bare_manager();
let key = ConnectionKey::from_spec(&base_spec());
mgr.conns
.lock()
.unwrap()
.insert(key.clone(), Arc::new(tokio::sync::Mutex::new(Weak::new())));
assert!(mgr.conns.lock().unwrap().contains_key(&key));
let slot: ConnSlot = Arc::new(tokio::sync::Mutex::new(Weak::new()));
mgr.conns.lock().unwrap().insert(key.clone(), slot.clone());
mgr.evict_connection(&key);
let held = mgr.conns.lock().unwrap().get(&key).cloned();
let held = held.expect("the slot every dial queues on must survive an eviction");
assert!(
!mgr.conns.lock().unwrap().contains_key(&key),
"evicted key must be gone so the next open creates a new entry"
Arc::ptr_eq(&held, &slot),
"the next dial has to wait on the same mutex the last one used, \
or two panes coming back from one dropped link each raise their \
own password prompt"
);
assert!(
held.try_lock()
.expect("nobody holds it here")
.upgrade()
.is_none(),
"what the slot pointed at is gone, so the next dial does not reuse it"
);
}
/// A slot somebody is dialling on is not eviction's business: that dial is
/// about to store its own connection there, and reaching into it is
/// exactly the interference this function exists to avoid.
#[test]
fn evict_connection_leaves_a_slot_that_is_being_dialled_on_alone() {
let mgr = bare_manager();
let key = ConnectionKey::from_spec(&base_spec());
let slot: ConnSlot = Arc::new(tokio::sync::Mutex::new(Weak::new()));
mgr.conns.lock().unwrap().insert(key.clone(), slot.clone());
let _dialling = slot.try_lock().expect("nobody else holds it in this test");
mgr.evict_connection(&key);
let held = mgr.conns.lock().unwrap().get(&key).cloned();
assert!(
held.is_some_and(|h| Arc::ptr_eq(&h, &slot)),
"a dial in flight keeps its slot"
);
}
@@ -233,3 +233,138 @@ fn spec_for(port: u16) -> NativeSshSpec {
spec.verify_host_keys = false;
spec
}
/// What a password server was asked for, so a test can say not only what the
/// user was shown but what actually went out on the wire.
#[derive(Default)]
struct AuthCounts {
passwords: AtomicUsize,
kbdint: AtomicUsize,
}
/// A server that wants a password and offers `keyboard-interactive` as the
/// other way to hand it one, counting both.
///
/// This is the shape of the host in #820: OpenSSH with `PasswordAuthentication
/// yes` advertises both methods, and a client that treats them as two separate
/// questions asks the same person the same thing twice.
struct PasswordSshd {
secret: String,
counts: Arc<AuthCounts>,
}
impl PasswordSshd {
fn reject() -> Auth {
Auth::Reject {
proceed_with_methods: Some(russh::MethodSet::from(
&[
russh::MethodKind::Password,
russh::MethodKind::KeyboardInteractive,
][..],
)),
partial_success: false,
}
}
}
impl server::Handler for PasswordSshd {
type Error = russh::Error;
async fn auth_none(&mut self, _user: &str) -> Result<Auth, Self::Error> {
Ok(Self::reject())
}
async fn auth_password(&mut self, _user: &str, password: &str) -> Result<Auth, Self::Error> {
self.counts.passwords.fetch_add(1, Ordering::SeqCst);
match password == self.secret {
true => Ok(Auth::Accept),
false => Ok(Self::reject()),
}
}
async fn auth_keyboard_interactive<'a>(
&'a mut self,
_user: &str,
_submethods: &str,
response: Option<russh::server::Response<'a>>,
) -> Result<Auth, Self::Error> {
self.counts.kbdint.fetch_add(1, Ordering::SeqCst);
match response {
// The opening request asks the question; only a client that got an
// answer out of somebody comes back carrying one.
None => Ok(Auth::Partial {
name: "".into(),
instructions: "".into(),
prompts: vec![("Password: ".into(), false)].into(),
}),
Some(_) => Ok(Self::reject()),
}
}
}
/// A client handle parked on a password server with not one authentication
/// method run yet, so a test can drive [`super::auth::authenticate`] itself.
pub(crate) struct PasswordFake {
pub(crate) handle: russh::client::Handle<ClientHandler>,
pub(crate) spec: NativeSshSpec,
counts: Arc<AuthCounts>,
}
impl PasswordFake {
pub(crate) async fn connect(secret: &str) -> PasswordFake {
let counts = Arc::new(AuthCounts::default());
let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
.await
.expect("bind a loopback port");
let addr = listener.local_addr().expect("bound address");
let mut config = server::Config::default();
config.inactivity_timeout = None;
// Rejections are deliberately slow in the default config, and these
// tests walk through several.
config.auth_rejection_time = Duration::from_millis(0);
config.auth_rejection_time_initial = Some(Duration::from_millis(0));
config
.keys
.push(PrivateKey::from(Ed25519Keypair::from_seed(&[9; 32])));
let handler = PasswordSshd {
secret: secret.to_string(),
counts: Arc::clone(&counts),
};
tokio::spawn(async move {
let (socket, _) = listener.accept().await.expect("accept the test client");
let running = server::run_stream(Arc::new(config), socket, handler)
.await
.expect("server handshake");
let _ = running.await;
});
let spec = spec_for(addr.port());
let handler = ClientHandler {
host: spec.host.clone(),
port: spec.port,
verify_host_keys: false,
skip_banner: true,
broker: PromptBroker::new(Box::new(|_| true)),
remote_forwards: RemoteForwardTable::default(),
};
let handle =
russh::client::connect(Arc::new(russh::client::Config::default()), addr, handler)
.await
.expect("client handshake");
PasswordFake {
handle,
spec,
counts,
}
}
/// `userauth` requests the server saw, per method.
pub(crate) fn password_attempts(&self) -> usize {
self.counts.passwords.load(Ordering::SeqCst)
}
pub(crate) fn kbdint_attempts(&self) -> usize {
self.counts.kbdint.load(Ordering::SeqCst)
}
}
+106 -1
View File
@@ -1276,6 +1276,9 @@ pub(crate) struct RemoteLinks {
/// times a second and the request's deadline is ten seconds, so without
/// this one reclaim would be sent forty times.
attaching: std::collections::HashSet<WorkspaceId>,
/// Machines the pump leaves alone until a person asks for them again:
/// disconnected from the switcher, or — since #820 — an authentication
/// the user closed the sheet on rather than answered.
suspended: std::collections::HashSet<HostId>,
instances: std::collections::HashMap<HostId, String>,
#[allow(
@@ -2020,8 +2023,17 @@ fn finish_attempt(
// for the rest of the session. Park it and give the user the move
// that actually changes the answer.
let parked = crate::daemon::control::is_dialect_refusal(&e);
// Somebody closed the password sheet. Retrying that is retrying a
// question the user has already answered, and the backoff answers
// it again a second later and every thirty seconds after that —
// which is the half of #820 where the window "cannot be closed".
// Suspend the machine instead: the strip says why and offers
// Retry, which is the user asking to be asked again.
let declined = !parked && crate::daemon::ssh::is_auth_declined(&e);
if parked {
log::warn!("{label} is served by a build this one cannot speak to: {e}");
} else if declined {
log::info!("not reconnecting to {label}: {e}");
} else {
log::warn!("reconnect to {label} failed: {e}");
}
@@ -2029,10 +2041,12 @@ fn finish_attempt(
link.attempting = false;
link.state = if parked {
LinkState::Mismatched(e.clone())
} else if declined {
LinkState::Failed(e.clone())
} else {
LinkState::Reconnecting
};
if !parked {
if !parked && !declined {
// The counter is the number of attempts that came back
// wrong. It moves here, not when the pump schedules one:
// a first try still in flight is attempt 1 on the strip,
@@ -2042,6 +2056,13 @@ fn finish_attempt(
link.next_attempt = None;
link.last_error = Some(e.clone());
});
// `Failed` on its own is not a park — the pump rewrites every
// state but `Mismatched` back to `Reconnecting` on its next tick.
// This is what actually stops the clock, and `retry_now` and the
// switcher's own connect are what start it again.
if declined {
cx.default_global::<RemoteLinks>().suspended.insert(host);
}
}
}
cx.refresh_windows();
@@ -3207,6 +3228,90 @@ mod tests {
});
}
/// What `connect_blocking` hands back when the person at the keyboard
/// closed the password sheet instead of filling it in, localised wrapper
/// and all.
fn a_decline() -> String {
t_fmt(
L10nKey::RemoteHostUnreachable,
&[
("machine", "build-box"),
("error", crate::daemon::ssh::AUTH_DECLINED),
],
)
}
/// #820. Closing the sheet is an answer. The backoff put the same question
/// back a second later and every thirty seconds after that, so the window
/// could not be got rid of — which is what the report calls "无法关闭".
#[gpui::test]
fn a_declined_password_stops_the_reconnect_clock(cx: &mut gpui::TestAppContext) {
cx.update(|cx| {
crate::core::config::pin_test_config_dir();
cx.set_global(crate::core::config::Config::default());
crate::ui::windows::WindowRegistry::init(cx);
let (host, target) = resolvable_machine("build-box");
let mut entry = crate::core::session::WindowView::on_remote(RemoteRef::new(
target.clone(),
WorkspaceId::new(),
));
entry.open = true;
let id = entry.id;
WorkspaceStore::install_for_test(
cx,
crate::core::session::WindowViews {
views: vec![entry],
active: None,
},
);
finish_attempt(cx, host, &target, Err(a_decline()));
let after = RemoteLinks::status_of(cx, id);
assert!(
matches!(after, Some(RemoteStatus::Failed(_))),
"a refusal to authenticate is not a machine that could not be \
reached: {after:?}"
);
for _ in 0..4 {
pump_tick(cx);
}
let link = cx.default_global::<RemoteLinks>().machines.get(&host);
let link = link.expect("the machine is still known");
assert!(
matches!(link.state, LinkState::Failed(_)),
"four ticks later the sheet has not been raised again"
);
assert!(!link.attempting, "and nothing is dialling behind it");
assert_eq!(
link.backoff.attempt(),
0,
"declining is not a failed attempt, so nothing counted one"
);
// Retry is the user asking to be asked again, and it is the action
// the strip already offers on a `Failed` link.
assert_eq!(
RemoteStatus::Failed(a_decline()).action_label(),
Some(t(L10nKey::RemoteActionRetry))
);
RemoteLinks::retry_now(cx, id);
assert!(
!cx.default_global::<RemoteLinks>().suspended.contains(&host),
"asking by hand puts the machine back on the clock"
);
pump_tick(cx);
assert!(
cx.default_global::<RemoteLinks>()
.machines
.get(&host)
.is_some_and(|l| l.attempting || l.state == LinkState::Reconnecting),
"and the pump picks it up again"
);
});
}
#[gpui::test]
fn a_parked_link_looks_again_when_the_slow_clock_runs_out(cx: &mut gpui::TestAppContext) {
cx.update(|cx| {