From fca37fc93b7cd1c04b54bebcf4db9f7a115432ce Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 07:39:15 +0000 Subject: [PATCH 1/6] Dial each resolved address with its own timeout, and disable Nagle russh's connect tries the resolved addresses one after another with no per-address bound, so a host whose first address is a black-holed IPv6 route spent the whole 20 s connect timeout there and never reached its working IPv4 address. Resolve the name ourselves, give each address 5 s, keep the overall cap, and hand the stream to connect_stream. A failed dial now says which addresses were tried and why each failed. The dialed socket also gets TCP_NODELAY, as OpenSSH sets for interactive sessions; with Nagle on, keystrokes waited on the server's delayed ACK and typing lagged noticeably on high-latency links. --- core/src/connection.rs | 97 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 92 insertions(+), 5 deletions(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 7437f0e9..b37a5440 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -11,8 +11,10 @@ use russh::keys::{ use russh::{mac, Preferred}; use std::borrow::Cow; use std::collections::HashSet; +use std::net::SocketAddr; use std::sync::{Arc, OnceLock, PoisonError}; use std::time::Duration; +use tokio::net::TcpStream; use tokio::sync::Mutex; /// Upper bound on opening the transport and on completing authentication. @@ -20,6 +22,13 @@ use tokio::sync::Mutex; /// leaves both futures pending forever. const CONNECT_TIMEOUT: Duration = Duration::from_secs(20); +/// Upper bound on the TCP handshake with one resolved address. A name that +/// resolves to an unreachable address first — typically an IPv6 address on a +/// network without IPv6 routing, where the SYN is silently dropped — would +/// otherwise use up all of [`CONNECT_TIMEOUT`] before a working address is +/// tried. +const DIAL_ATTEMPT_TIMEOUT: Duration = Duration::from_secs(5); + /// Client defaults shared by every session. /// /// russh's own defaults leave `keepalive_interval` and `inactivity_timeout` @@ -91,6 +100,37 @@ const MAC_ALGORITHMS: &[mac::Name] = &[ mac::HMAC_SHA1, ]; +/// Opens a TCP connection to the first of `addrs` that answers, trying them in +/// resolver order, each for at most [`DIAL_ATTEMPT_TIMEOUT`]. On failure the +/// error names every address with its own reason. +/// +/// The stream has `TCP_NODELAY` set, as OpenSSH does for interactive sessions: +/// with Nagle's algorithm on, a keystroke sent while the previous one is still +/// unacknowledged waits for the server's delayed ACK, which adds tens to +/// hundreds of milliseconds of lag on a high-latency link. +async fn dial(addrs: &[SocketAddr]) -> std::result::Result { + let mut failures = Vec::with_capacity(addrs.len()); + for addr in addrs { + match tokio::time::timeout(DIAL_ATTEMPT_TIMEOUT, TcpStream::connect(addr)).await { + Ok(Ok(stream)) => { + if let Err(e) = stream.set_nodelay(true) { + tracing::warn!("set_nodelay on {addr} failed: {e}"); + } + return Ok(stream); + } + Ok(Err(e)) => failures.push(format!("{addr}: {e}")), + Err(_) => failures.push(format!( + "{addr}: timed out after {}s", + DIAL_ATTEMPT_TIMEOUT.as_secs() + )), + } + } + if failures.is_empty() { + return Err("the name resolved to no addresses".into()); + } + Err(failures.join("; ")) +} + fn timed_out(stage: &str, addr: &str) -> CoreError { CoreError::Ssh(format!( "{stage} {addr}: timed out after {}s", @@ -301,10 +341,18 @@ pub async fn connect( // connect failure would hide the two fingerprints that make the change // legible. `CoreError::Ssh` only reaches here from `check_server_key`, which // already names the address, so it is passed through unwrapped too. - let mut session = tokio::time::timeout( - CONNECT_TIMEOUT, - client::connect(client_config(), &addr, handler), - ) + // `CONNECT_TIMEOUT` caps the whole stage, so a name with many dead + // addresses still gives up in time even though each one gets its own bound. + let mut session = tokio::time::timeout(CONNECT_TIMEOUT, async { + let resolved: Vec = tokio::net::lookup_host(&addr) + .await + .map_err(|e| CoreError::Ssh(format!("connect {addr}: {e}")))? + .collect(); + let stream = dial(&resolved) + .await + .map_err(|e| CoreError::Ssh(format!("connect {addr}: {e}")))?; + client::connect_stream(client_config(), stream, handler).await + }) .await .map_err(|_| timed_out("connect", &addr))? .map_err(|e| match e { @@ -316,7 +364,7 @@ pub async fn connect( })?; // Authentication is bounded separately, and the two bounds cover disjoint - // stages. `client::connect` does not return until russh has read the + // stages. `client::connect_stream` does not return until russh has read the // server's banner and the key exchange has completed, so a peer that // answers the TCP handshake and then goes silent trips the connect timeout // above and never reaches this point. What is left for this bound is a @@ -500,6 +548,45 @@ mod tests { assert_eq!(rsa_hash_for(Some(Some(HashAlg::Sha256))), HashAlg::Sha256); } + /// An address that never answers the handshake. Packets to it are dropped + /// rather than refused wherever there is a default route. + const BLACK_HOLE: &str = "10.255.255.1:22"; + + #[tokio::test] + async fn a_silent_address_does_not_hold_up_the_next_one() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let live = listener.local_addr().unwrap(); + let addrs = [BLACK_HOLE.parse().unwrap(), live]; + let stream = tokio::time::timeout(CONNECT_TIMEOUT / 2, dial(&addrs)) + .await + .expect("the dead address used up the connect timeout") + .expect("dial the live address"); + assert_eq!(stream.peer_addr().unwrap(), live); + } + + #[tokio::test] + async fn dialed_streams_disable_nagle() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let stream = dial(&[listener.local_addr().unwrap()]).await.unwrap(); + assert!(stream.nodelay().unwrap()); + } + + #[tokio::test] + async fn a_failed_dial_names_every_address() { + let closed = { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + listener.local_addr().unwrap() + }; + let other = { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + listener.local_addr().unwrap() + }; + let error = dial(&[closed, other]).await.unwrap_err(); + assert!(error.contains(&closed.to_string()), "{error}"); + assert!(error.contains(&other.to_string()), "{error}"); + assert!(dial(&[]).await.is_err()); + } + #[test] fn rsa_never_falls_back_to_sha1() { // The server lists only `ssh-rsa`. From de223c27af45266e42e5f66711e46f1549ce5bc8 Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 07:44:47 +0000 Subject: [PATCH 2/6] Sign in through keyboard-interactive, and say why a sign-in was rejected Password hosts only ever tried the password method, so servers that disable it and take the password through keyboard-interactive (FreeBSD's default sshd, many PAM setups) refused them outright. Probe with "none" first like OpenSSH, then use password and/or keyboard-interactive as the server offers them. The password is only given to a single hidden prompt, once; prompts for anything more, such as a one-time code, are refused with an explanation instead of being sent the password. A rejection used to read "authentication rejected by server" and nothing else. It now names the methods the server still accepts, says when the server does not offer password or publickey sign-in at all, and reports a partial success as a further step Clavyn cannot complete yet. --- core/src/connection.rs | 179 ++++++++++++++++++++++++++++++-- core/tests/ssh_roundtrip.rs | 200 +++++++++++++++++++++++++++++++++++- 2 files changed, 368 insertions(+), 11 deletions(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index b37a5440..16a41aed 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -3,12 +3,12 @@ use crate::identity::Identity; use crate::known_hosts::KnownHosts; use crate::vault::Vault; use crate::{CoreError, Result}; -use russh::client::{self, Config, Handle}; +use russh::client::{self, AuthResult, Config, Handle, KeyboardInteractiveAuthResponse}; use russh::keys::{ decode_secret_key, Algorithm, EcdsaCurve, HashAlg, PrivateKeyWithHashAlg, PublicKeyOrCertificate, }; -use russh::{mac, Preferred}; +use russh::{mac, MethodKind, MethodSet, Preferred}; use std::borrow::Cow; use std::collections::HashSet; use std::net::SocketAddr; @@ -292,6 +292,150 @@ fn rsa_hash_for(advertised: Option>) -> HashAlg { } } +/// Upper bound on keyboard-interactive exchanges in one sign-in. A server +/// asking for the password needs one or two; one that keeps sending empty +/// rounds is not going to finish. +const MAX_KEYBOARD_INTERACTIVE_ROUNDS: usize = 5; + +fn auth_error(e: russh::Error) -> CoreError { + CoreError::Ssh(format!("auth: {e}")) +} + +/// Signs in with a password the way OpenSSH does: a `none` request first to +/// learn which methods the server accepts, then `password` if it is offered, +/// and `keyboard-interactive` if it is offered and `password` is not, or did +/// not succeed. Servers that leave password checks to PAM often disable +/// `password` and ask for the password through `keyboard-interactive`. +async fn authenticate_with_password( + session: &mut Handle, + username: &str, + password: &str, +) -> Result { + let methods = match session + .authenticate_none(username) + .await + .map_err(auth_error)? + { + AuthResult::Success => return Ok(AuthResult::Success), + AuthResult::Failure { + remaining_methods, .. + } => remaining_methods, + }; + let mut outcome = AuthResult::Failure { + remaining_methods: methods.clone(), + partial_success: false, + }; + if methods.contains(&MethodKind::Password) { + outcome = session + .authenticate_password(username, password) + .await + .map_err(auth_error)?; + match &outcome { + AuthResult::Failure { + remaining_methods, + partial_success: false, + } if remaining_methods.contains(&MethodKind::KeyboardInteractive) => {} + _ => return Ok(outcome), + } + } + if methods.contains(&MethodKind::KeyboardInteractive) { + outcome = keyboard_interactive_password(session, username, password).await?; + } + Ok(outcome) +} + +/// Answers a keyboard-interactive exchange with the password, as OpenSSH does +/// for a PAM password prompt: the password goes only to a round with a single +/// prompt whose input is not echoed, and only once. A round with several +/// prompts, an echoed prompt, or a second password prompt asks for something +/// else — usually a one-time code — so it is refused rather than answered with +/// the password. Rounds with no prompts only carry text and are acknowledged +/// with an empty reply. +async fn keyboard_interactive_password( + session: &mut Handle, + username: &str, + password: &str, +) -> Result { + let mut reply = session + .authenticate_keyboard_interactive_start(username, None) + .await + .map_err(auth_error)?; + let mut answered = false; + for _ in 0..MAX_KEYBOARD_INTERACTIVE_ROUNDS { + let prompts = match reply { + KeyboardInteractiveAuthResponse::Success => return Ok(AuthResult::Success), + KeyboardInteractiveAuthResponse::Failure { + remaining_methods, + partial_success, + } => { + return Ok(AuthResult::Failure { + remaining_methods, + partial_success, + }) + } + KeyboardInteractiveAuthResponse::InfoRequest { prompts, .. } => prompts, + }; + let responses = match prompts.as_slice() { + [] => Vec::new(), + [prompt] if !prompt.echo && !answered => { + answered = true; + vec![password.to_owned()] + } + _ => { + let asked: Vec = prompts + .iter() + .map(|p| format!("{:?}", p.prompt.trim())) + .collect(); + return Err(CoreError::Ssh(format!( + "auth: the server asks for more than the password ({}); Clavyn cannot \ + answer additional sign-in prompts such as one-time codes yet", + asked.join(", ") + ))); + } + }; + reply = session + .authenticate_keyboard_interactive_respond(responses) + .await + .map_err(auth_error)?; + } + Err(CoreError::Ssh(format!( + "auth: the server sent more than {MAX_KEYBOARD_INTERACTIVE_ROUNDS} keyboard-interactive \ + rounds without finishing sign-in" + ))) +} + +/// Explains a failed sign-in from what the server said it still accepts. +/// `usable` lists the methods Clavyn can drive for this host's auth setting; +/// `credential` names what was offered. +fn rejection( + remaining: &MethodSet, + partial_success: bool, + usable: &[MethodKind], + method: &str, + credential: &str, +) -> CoreError { + let accepted = if remaining.is_empty() { + "nothing it names".to_owned() + } else { + remaining + .iter() + .map(<&str>::from) + .collect::>() + .join(", ") + }; + let reason = if partial_success { + format!( + "the server accepted this step but requires another: {accepted}; Clavyn cannot \ + complete multi-step sign-in yet" + ) + } else if !remaining.is_empty() && !usable.iter().any(|m| remaining.contains(m)) { + format!("the server does not accept {method} sign-in (it accepts: {accepted})") + } else { + format!("the server rejected the {credential} (it accepts: {accepted})") + }; + CoreError::Ssh(format!("authentication rejected by server: {reason}")) +} + /// Open an authenticated SSH session and return the handle. /// The caller is responsible for opening a channel and starting the shell. /// @@ -371,7 +515,7 @@ pub async fn connect( // server that finishes the key exchange and then stalls while answering an // authentication request. Neither timeout subsumes the other. let auth_ok = tokio::time::timeout(CONNECT_TIMEOUT, async { - let outcome = match auth { + match auth { AuthMethod::Agent => { unreachable!("agent auth is rejected before network connection") } @@ -379,7 +523,7 @@ pub async fn connect( let pw = password.ok_or_else(|| { CoreError::InvalidInput("password required but not provided".into()) })?; - session.authenticate_password(username, pw).await + authenticate_with_password(&mut session, username, pw).await } AuthMethod::PublicKey => { let key_id = key_id.ok_or_else(|| { @@ -418,15 +562,34 @@ pub async fn connect( PrivateKeyWithHashAlg::new(Arc::new(pair), hash_alg), ) .await + .map_err(auth_error) } - }; - outcome.map_err(|e| CoreError::Ssh(format!("auth: {e}"))) + } }) .await .map_err(|_| timed_out("auth", &addr))??; - if !auth_ok.success() { - return Err(CoreError::Ssh("authentication rejected by server".into())); + if let AuthResult::Failure { + remaining_methods, + partial_success, + } = auth_ok + { + return Err(match auth { + AuthMethod::PublicKey => rejection( + &remaining_methods, + partial_success, + &[MethodKind::PublicKey], + "publickey", + "key", + ), + _ => rejection( + &remaining_methods, + partial_success, + &[MethodKind::Password, MethodKind::KeyboardInteractive], + "password", + "password", + ), + }); } Ok(session) } diff --git a/core/tests/ssh_roundtrip.rs b/core/tests/ssh_roundtrip.rs index 2f5160a6..c59b4a29 100644 --- a/core/tests/ssh_roundtrip.rs +++ b/core/tests/ssh_roundtrip.rs @@ -17,8 +17,8 @@ use clavyn_core::vault::Vault; use clavyn_core::CoreError; use common::{copy_fixture, passphrase_protected_ed25519, PASSPHRASE}; use russh::keys::{decode_secret_key, parse_public_key_base64, Algorithm, PrivateKey, PublicKey}; -use russh::server::{self, Auth, ChannelOpenHandle, Msg, Session}; -use russh::{Channel, ChannelId, ChannelMsg, Preferred}; +use russh::server::{self, Auth, ChannelOpenHandle, Msg, Response, Session}; +use russh::{Channel, ChannelId, ChannelMsg, MethodKind, MethodSet, Preferred}; use std::borrow::Cow; use std::sync::Arc; use std::time::Duration; @@ -114,7 +114,10 @@ async fn start_server_offering( serve(config, handler).await } -async fn serve(config: server::Config, handler: TestServer) -> u16 { +async fn serve(config: server::Config, handler: H) -> u16 +where + H: server::Handler + Clone + Send + 'static, +{ let config = Arc::new(config); let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); let port = listener.local_addr().expect("addr").port(); @@ -389,3 +392,194 @@ async fn a_host_key_signed_only_with_sha1_is_refused() { assert!(pins.lock().await.list().is_empty()); } + +/// How [`SignInServer`] treats a sign-in. +#[derive(Clone, Copy)] +enum SignIn { + /// `keyboard-interactive` only: a round with no prompts, then a single + /// hidden `Password:` prompt, as a PAM-backed sshd with + /// `PasswordAuthentication no` sends. + KeyboardInteractive, + /// `keyboard-interactive` only, asking for the password and a one-time code + /// in one round. + PasswordAndCode, + /// `password`, rejecting every attempt while still listing `password` and + /// `publickey`, as sshd does. + PasswordOrKey, + /// The password through `keyboard-interactive` is only the first of two + /// steps, as with sshd's `AuthenticationMethods keyboard-interactive,publickey`. + PasswordThenKey, +} + +#[derive(Clone)] +struct SignInServer { + mode: SignIn, + rounds: usize, + responses: Arc>>, +} + +impl SignInServer { + fn new(mode: SignIn) -> Self { + Self { + mode, + rounds: 0, + responses: Arc::default(), + } + } +} + +impl server::Handler for SignInServer { + type Error = russh::Error; + + async fn auth_password(&mut self, _user: &str, _password: &str) -> Result { + Ok(Auth::Reject { + proceed_with_methods: Some(MethodSet::from( + &[MethodKind::Password, MethodKind::PublicKey][..], + )), + partial_success: false, + }) + } + + async fn auth_keyboard_interactive<'a>( + &'a mut self, + user: &str, + _submethods: &str, + response: Option>, + ) -> Result { + let answers: Vec = response + .into_iter() + .flatten() + .map(|b| String::from_utf8_lossy(&b).into_owned()) + .collect(); + self.responses + .lock() + .unwrap() + .extend(answers.iter().cloned()); + self.rounds += 1; + let prompt = |prompts: &'static [(Cow<'static, str>, bool)]| Auth::Partial { + name: "".into(), + instructions: "".into(), + prompts: Cow::Borrowed(prompts), + }; + Ok(match (self.mode, self.rounds) { + (SignIn::KeyboardInteractive, 1) => prompt(&[]), + (SignIn::KeyboardInteractive, 2) => prompt(&[(Cow::Borrowed("Password: "), false)]), + (SignIn::KeyboardInteractive, 3) if user == USER && answers == [PASSWORD] => { + Auth::Accept + } + (SignIn::PasswordThenKey, 1) => prompt(&[(Cow::Borrowed("Password: "), false)]), + (SignIn::PasswordThenKey, 2) if user == USER && answers == [PASSWORD] => Auth::Reject { + proceed_with_methods: Some(MethodSet::from(&[MethodKind::PublicKey][..])), + partial_success: true, + }, + (SignIn::PasswordAndCode, 1) => prompt(&[ + (Cow::Borrowed("Password: "), false), + (Cow::Borrowed("Verification code: "), false), + ]), + _ => Auth::reject(), + }) + } +} + +async fn start_sign_in_server(methods: &[MethodKind], handler: SignInServer) -> u16 { + let mut config = server_config(random_host_key()); + config.methods = MethodSet::from(methods); + serve(config, handler).await +} + +async fn password_error(port: u16, password: &str) -> String { + let dir = tempfile::tempdir().expect("temp dir"); + match connect( + &password_host(port), + None, + known_hosts(&dir), + None, + None, + Some(password), + ) + .await + { + Ok(_) => panic!("sign-in unexpectedly succeeded"), + Err(error) => error.to_string(), + } +} + +/// A server that takes the password only through `keyboard-interactive` — the +/// default for sshd on FreeBSD and many PAM setups — accepts a Password host. +#[tokio::test] +async fn a_password_is_answered_through_keyboard_interactive() { + let server = SignInServer::new(SignIn::KeyboardInteractive); + let responses = server.responses.clone(); + let port = start_sign_in_server(&[MethodKind::KeyboardInteractive], server).await; + let dir = tempfile::tempdir().expect("temp dir"); + + connect( + &password_host(port), + None, + known_hosts(&dir), + None, + None, + Some(PASSWORD), + ) + .await + .expect("keyboard-interactive password auth"); + assert_eq!(*responses.lock().unwrap(), [PASSWORD]); +} + +/// The password is never handed to a round that also asks for something else. +#[tokio::test] +async fn a_second_factor_prompt_is_refused_without_sending_the_password() { + let server = SignInServer::new(SignIn::PasswordAndCode); + let responses = server.responses.clone(); + let port = start_sign_in_server(&[MethodKind::KeyboardInteractive], server).await; + + let error = password_error(port, PASSWORD).await; + assert!(error.contains("Verification code"), "{error}"); + assert!(error.contains("cannot"), "{error}"); + assert!(responses.lock().unwrap().is_empty()); +} + +#[tokio::test] +async fn a_rejected_password_names_the_methods_the_server_accepts() { + let port = start_sign_in_server( + &[MethodKind::Password, MethodKind::PublicKey], + SignInServer::new(SignIn::PasswordOrKey), + ) + .await; + + let error = password_error(port, "wrong password").await; + assert!(error.contains("authentication rejected"), "{error}"); + assert!(error.contains("rejected the password"), "{error}"); + assert!(error.contains("it accepts: password, publickey"), "{error}"); +} + +#[tokio::test] +async fn a_server_without_password_sign_in_says_so() { + let port = start_sign_in_server( + &[MethodKind::PublicKey], + SignInServer::new(SignIn::PasswordOrKey), + ) + .await; + + let error = password_error(port, PASSWORD).await; + assert!( + error.contains("does not accept password sign-in (it accepts: publickey)"), + "{error}" + ); +} + +#[tokio::test] +async fn a_partial_success_is_reported_as_a_further_step() { + let port = start_sign_in_server( + &[MethodKind::KeyboardInteractive, MethodKind::PublicKey], + SignInServer::new(SignIn::PasswordThenKey), + ) + .await; + + let error = password_error(port, PASSWORD).await; + assert!( + error.contains("accepted this step but requires another: publickey"), + "{error}" + ); + assert!(error.contains("multi-step"), "{error}"); +} From 9f585634206fea7e3f771e57af17a9ffb0ade53f Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 07:47:50 +0000 Subject: [PATCH 3/6] Don't resend a rejected password through keyboard-interactive A server that offers password and rejects it would check the same credential through keyboard-interactive, so the retry could not succeed and only counted twice against MaxAuthTries and fail2ban. --- core/src/connection.rs | 35 +++++++++++++++-------------------- core/tests/ssh_roundtrip.rs | 25 ++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 16a41aed..6cacca32 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -301,11 +301,14 @@ fn auth_error(e: russh::Error) -> CoreError { CoreError::Ssh(format!("auth: {e}")) } -/// Signs in with a password the way OpenSSH does: a `none` request first to -/// learn which methods the server accepts, then `password` if it is offered, -/// and `keyboard-interactive` if it is offered and `password` is not, or did -/// not succeed. Servers that leave password checks to PAM often disable -/// `password` and ask for the password through `keyboard-interactive`. +/// Signs in with a password: a `none` request first to learn which methods the +/// server accepts, then `password` if it is offered, otherwise +/// `keyboard-interactive`. Servers that leave password checks to PAM often +/// disable `password` and ask for the password through `keyboard-interactive`. +/// +/// A password the server rejected through `password` is not sent again through +/// `keyboard-interactive`: both check the same credential, and a second attempt +/// only counts twice against `MaxAuthTries` and tools such as fail2ban. async fn authenticate_with_password( session: &mut Handle, username: &str, @@ -321,27 +324,19 @@ async fn authenticate_with_password( remaining_methods, .. } => remaining_methods, }; - let mut outcome = AuthResult::Failure { - remaining_methods: methods.clone(), - partial_success: false, - }; if methods.contains(&MethodKind::Password) { - outcome = session + return session .authenticate_password(username, password) .await - .map_err(auth_error)?; - match &outcome { - AuthResult::Failure { - remaining_methods, - partial_success: false, - } if remaining_methods.contains(&MethodKind::KeyboardInteractive) => {} - _ => return Ok(outcome), - } + .map_err(auth_error); } if methods.contains(&MethodKind::KeyboardInteractive) { - outcome = keyboard_interactive_password(session, username, password).await?; + return keyboard_interactive_password(session, username, password).await; } - Ok(outcome) + Ok(AuthResult::Failure { + remaining_methods: methods, + partial_success: false, + }) } /// Answers a keyboard-interactive exchange with the password, as OpenSSH does diff --git a/core/tests/ssh_roundtrip.rs b/core/tests/ssh_roundtrip.rs index c59b4a29..8d239ece 100644 --- a/core/tests/ssh_roundtrip.rs +++ b/core/tests/ssh_roundtrip.rs @@ -432,10 +432,12 @@ impl server::Handler for SignInServer { type Error = russh::Error; async fn auth_password(&mut self, _user: &str, _password: &str) -> Result { + let methods: &[MethodKind] = match self.mode { + SignIn::KeyboardInteractive => &[MethodKind::Password, MethodKind::KeyboardInteractive], + _ => &[MethodKind::Password, MethodKind::PublicKey], + }; Ok(Auth::Reject { - proceed_with_methods: Some(MethodSet::from( - &[MethodKind::Password, MethodKind::PublicKey][..], - )), + proceed_with_methods: Some(MethodSet::from(methods)), partial_success: false, }) } @@ -526,6 +528,23 @@ async fn a_password_is_answered_through_keyboard_interactive() { assert_eq!(*responses.lock().unwrap(), [PASSWORD]); } +/// A password rejected through `password` is not tried again through +/// `keyboard-interactive`, which checks the same credential. +#[tokio::test] +async fn a_rejected_password_is_not_sent_a_second_time() { + let server = SignInServer::new(SignIn::KeyboardInteractive); + let responses = server.responses.clone(); + let port = start_sign_in_server( + &[MethodKind::Password, MethodKind::KeyboardInteractive], + server, + ) + .await; + + let error = password_error(port, PASSWORD).await; + assert!(error.contains("rejected the password"), "{error}"); + assert!(responses.lock().unwrap().is_empty()); +} + /// The password is never handed to a round that also asks for something else. #[tokio::test] async fn a_second_factor_prompt_is_refused_without_sending_the_password() { From 811f0caa8fa40703570254a49651db29f365858f Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 08:14:52 +0000 Subject: [PATCH 4/6] Read the server's reply to the last keyboard-interactive round The loop answered five rounds but never looked at the reply to the fifth, so a server that accepted there was reported as asking for too many rounds. --- core/src/connection.rs | 9 ++++++++- core/tests/ssh_roundtrip.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 6cacca32..593fc31e 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -356,7 +356,9 @@ async fn keyboard_interactive_password( .await .map_err(auth_error)?; let mut answered = false; - for _ in 0..MAX_KEYBOARD_INTERACTIVE_ROUNDS { + // One pass more than the rounds answered, so the server's reply to the + // last allowed round is still read rather than reported as one too many. + for round in 0..=MAX_KEYBOARD_INTERACTIVE_ROUNDS { let prompts = match reply { KeyboardInteractiveAuthResponse::Success => return Ok(AuthResult::Success), KeyboardInteractiveAuthResponse::Failure { @@ -368,6 +370,11 @@ async fn keyboard_interactive_password( partial_success, }) } + KeyboardInteractiveAuthResponse::InfoRequest { .. } + if round == MAX_KEYBOARD_INTERACTIVE_ROUNDS => + { + break + } KeyboardInteractiveAuthResponse::InfoRequest { prompts, .. } => prompts, }; let responses = match prompts.as_slice() { diff --git a/core/tests/ssh_roundtrip.rs b/core/tests/ssh_roundtrip.rs index 8d239ece..3bc4e3ff 100644 --- a/core/tests/ssh_roundtrip.rs +++ b/core/tests/ssh_roundtrip.rs @@ -400,6 +400,9 @@ enum SignIn { /// hidden `Password:` prompt, as a PAM-backed sshd with /// `PasswordAuthentication no` sends. KeyboardInteractive, + /// `keyboard-interactive` only: four rounds with no prompts, the password in + /// the fifth, and the acceptance in reply to that fifth answer. + PasswordInFifthRound, /// `keyboard-interactive` only, asking for the password and a one-time code /// in one round. PasswordAndCode, @@ -469,6 +472,11 @@ impl server::Handler for SignInServer { (SignIn::KeyboardInteractive, 3) if user == USER && answers == [PASSWORD] => { Auth::Accept } + (SignIn::PasswordInFifthRound, 1..=4) => prompt(&[]), + (SignIn::PasswordInFifthRound, 5) => prompt(&[(Cow::Borrowed("Password: "), false)]), + (SignIn::PasswordInFifthRound, 6) if user == USER && answers == [PASSWORD] => { + Auth::Accept + } (SignIn::PasswordThenKey, 1) => prompt(&[(Cow::Borrowed("Password: "), false)]), (SignIn::PasswordThenKey, 2) if user == USER && answers == [PASSWORD] => Auth::Reject { proceed_with_methods: Some(MethodSet::from(&[MethodKind::PublicKey][..])), @@ -528,6 +536,26 @@ async fn a_password_is_answered_through_keyboard_interactive() { assert_eq!(*responses.lock().unwrap(), [PASSWORD]); } +/// The server's reply to the last round Clavyn answers is still read, so an +/// acceptance there signs in. +#[tokio::test] +async fn an_acceptance_after_the_last_allowed_round_signs_in() { + let server = SignInServer::new(SignIn::PasswordInFifthRound); + let port = start_sign_in_server(&[MethodKind::KeyboardInteractive], server).await; + let dir = tempfile::tempdir().expect("temp dir"); + + connect( + &password_host(port), + None, + known_hosts(&dir), + None, + None, + Some(PASSWORD), + ) + .await + .expect("sign-in accepted in reply to the fifth round"); +} + /// A password rejected through `password` is not tried again through /// `keyboard-interactive`, which checks the same credential. #[tokio::test] From d2a17a445a305e19c4a56145f2effe8ae7147028 Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 08:27:38 +0000 Subject: [PATCH 5/6] Give the last address the rest of the connect budget, and don't blame a credential nobody sent Only an address with a fallback after it needs the per-address cap; the last or only one was cut off at 5 s where it used to get the full 20. A server that named no method at all was reported as having rejected the password, even when the password was never sent. --- core/src/connection.rs | 54 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 593fc31e..1f8c1e6b 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -101,17 +101,31 @@ const MAC_ALGORITHMS: &[mac::Name] = &[ ]; /// Opens a TCP connection to the first of `addrs` that answers, trying them in -/// resolver order, each for at most [`DIAL_ATTEMPT_TIMEOUT`]. On failure the -/// error names every address with its own reason. +/// resolver order. Every address but the last gets at most +/// [`DIAL_ATTEMPT_TIMEOUT`]; the last has nothing left to fall back to, so it +/// keeps whatever remains of the caller's own timeout, as a single-address host +/// always did. On failure the error names every address with its own reason. /// /// The stream has `TCP_NODELAY` set, as OpenSSH does for interactive sessions: /// with Nagle's algorithm on, a keystroke sent while the previous one is still /// unacknowledged waits for the server's delayed ACK, which adds tens to /// hundreds of milliseconds of lag on a high-latency link. async fn dial(addrs: &[SocketAddr]) -> std::result::Result { + dial_with(addrs, DIAL_ATTEMPT_TIMEOUT).await +} + +async fn dial_with( + addrs: &[SocketAddr], + attempt_timeout: Duration, +) -> std::result::Result { let mut failures = Vec::with_capacity(addrs.len()); - for addr in addrs { - match tokio::time::timeout(DIAL_ATTEMPT_TIMEOUT, TcpStream::connect(addr)).await { + for (index, addr) in addrs.iter().enumerate() { + let attempt = if index + 1 == addrs.len() { + Ok(TcpStream::connect(addr).await) + } else { + tokio::time::timeout(attempt_timeout, TcpStream::connect(addr)).await + }; + match attempt { Ok(Ok(stream)) => { if let Err(e) = stream.set_nodelay(true) { tracing::warn!("set_nodelay on {addr} failed: {e}"); @@ -121,7 +135,7 @@ async fn dial(addrs: &[SocketAddr]) -> std::result::Result { Ok(Err(e)) => failures.push(format!("{addr}: {e}")), Err(_) => failures.push(format!( "{addr}: timed out after {}s", - DIAL_ATTEMPT_TIMEOUT.as_secs() + attempt_timeout.as_secs() )), } } @@ -425,7 +439,9 @@ fn rejection( .collect::>() .join(", ") }; - let reason = if partial_success { + let reason = if remaining.is_empty() && !partial_success { + "the server refused and named no sign-in method it would accept".to_owned() + } else if partial_success { format!( "the server accepted this step but requires another: {accepted}; Clavyn cannot \ complete multi-step sign-in yet" @@ -729,6 +745,32 @@ mod tests { assert_eq!(stream.peer_addr().unwrap(), live); } + #[test] + fn a_server_naming_no_methods_is_not_said_to_reject_the_credential() { + let error = rejection( + &MethodSet::from(&[][..]), + false, + &[MethodKind::Password, MethodKind::KeyboardInteractive], + "password", + "password", + ) + .to_string(); + assert!(!error.contains("rejected the password"), "{error}"); + assert!(error.contains("named no sign-in method"), "{error}"); + } + + #[tokio::test] + async fn the_last_address_is_not_cut_short() { + // With nothing after it, a slow address keeps the caller's whole + // budget: the outer timeout must be what ends this dial. + let outcome = tokio::time::timeout( + Duration::from_millis(500), + dial_with(&[BLACK_HOLE.parse().unwrap()], Duration::from_millis(50)), + ) + .await; + assert!(outcome.is_err(), "dial gave up on its own: {outcome:?}"); + } + #[tokio::test] async fn dialed_streams_disable_nagle() { let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); From 3f5cac46f3f74b04f54025d5fa7f0697119e3d2d Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 08:39:43 +0000 Subject: [PATCH 6/6] Skip the dial tests where the black-hole address is rejected at once They rely on an address that swallows the handshake; a network that refuses it quickly would make them pass or fail for the wrong reason. --- core/src/connection.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/src/connection.rs b/core/src/connection.rs index 1f8c1e6b..b00531b4 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -733,8 +733,25 @@ mod tests { /// rather than refused wherever there is a default route. const BLACK_HOLE: &str = "10.255.255.1:22"; + /// Whether [`BLACK_HOLE`] really swallows the handshake here. A network + /// that rejects it at once would make the dial tests pass or fail for the + /// wrong reason, so they skip themselves there. + async fn black_hole_is_silent() -> bool { + let silent = + tokio::time::timeout(Duration::from_millis(300), TcpStream::connect(BLACK_HOLE)) + .await + .is_err(); + if !silent { + eprintln!("skipped: {BLACK_HOLE} does not swallow connections on this network"); + } + silent + } + #[tokio::test] async fn a_silent_address_does_not_hold_up_the_next_one() { + if !black_hole_is_silent().await { + return; + } let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let live = listener.local_addr().unwrap(); let addrs = [BLACK_HOLE.parse().unwrap(), live]; @@ -761,6 +778,9 @@ mod tests { #[tokio::test] async fn the_last_address_is_not_cut_short() { + if !black_hole_is_silent().await { + return; + } // With nothing after it, a slow address keeps the caller's whole // budget: the outer timeout must be what ends this dial. let outcome = tokio::time::timeout(