diff --git a/crates/tty7-core/src/daemon/ssh/auth.rs b/crates/tty7-core/src/daemon/ssh/auth.rs index b031cd8f..de818459 100644 --- a/crates/tty7-core/src/daemon/ssh/auth.rs +++ b/crates/tty7-core/src/daemon/ssh/auth.rs @@ -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 { } } +/// 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, reason: Option, @@ -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 { #[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) -> (Arc>>, Arc) { + let seen: Arc>> = Arc::new(Mutex::new(Vec::new())); + let script = Arc::new(Mutex::new(std::collections::VecDeque::from(script))); + let back: Arc>> = 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() { diff --git a/crates/tty7-core/src/daemon/ssh/mod.rs b/crates/tty7-core/src/daemon/ssh/mod.rs index 44760b40..2e04b00d 100644 --- a/crates/tty7-core/src/daemon/ssh/mod.rs +++ b/crates/tty7-core/src/daemon/ssh/mod.rs @@ -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 { @@ -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" ); } diff --git a/crates/tty7-core/src/daemon/ssh/test_support.rs b/crates/tty7-core/src/daemon/ssh/test_support.rs index 946bd928..5faf26e6 100644 --- a/crates/tty7-core/src/daemon/ssh/test_support.rs +++ b/crates/tty7-core/src/daemon/ssh/test_support.rs @@ -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, +} + +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 { + Ok(Self::reject()) + } + + async fn auth_password(&mut self, _user: &str, password: &str) -> Result { + 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>, + ) -> Result { + 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, + pub(crate) spec: NativeSshSpec, + counts: Arc, +} + +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) + } +} diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 4f06d3c2..fbbcce21 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -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, + /// 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, instances: std::collections::HashMap, #[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::().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::().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::().suspended.contains(&host), + "asking by hand puts the machine back on the clock" + ); + pump_tick(cx); + assert!( + cx.default_global::() + .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| {