diff --git a/core/src/connection.rs b/core/src/connection.rs index 7437f0e9..b00531b4 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -3,16 +3,18 @@ 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; 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,51 @@ const MAC_ALGORITHMS: &[mac::Name] = &[ mac::HMAC_SHA1, ]; +/// Opens a TCP connection to the first of `addrs` that answers, trying them in +/// 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 (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}"); + } + return Ok(stream); + } + Ok(Err(e)) => failures.push(format!("{addr}: {e}")), + Err(_) => failures.push(format!( + "{addr}: timed out after {}s", + 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", @@ -252,6 +306,154 @@ 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: 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, + 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, + }; + if methods.contains(&MethodKind::Password) { + return session + .authenticate_password(username, password) + .await + .map_err(auth_error); + } + if methods.contains(&MethodKind::KeyboardInteractive) { + return keyboard_interactive_password(session, username, password).await; + } + Ok(AuthResult::Failure { + remaining_methods: methods, + partial_success: false, + }) +} + +/// 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; + // 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 { + remaining_methods, + partial_success, + } => { + return Ok(AuthResult::Failure { + remaining_methods, + partial_success, + }) + } + KeyboardInteractiveAuthResponse::InfoRequest { .. } + if round == MAX_KEYBOARD_INTERACTIVE_ROUNDS => + { + break + } + 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 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" + ) + } 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. /// @@ -301,10 +503,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,14 +526,14 @@ 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 // 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") } @@ -331,7 +541,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(|| { @@ -370,15 +580,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) } @@ -500,6 +729,91 @@ 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"; + + /// 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]; + 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); + } + + #[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() { + 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( + 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(); + 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`. diff --git a/core/tests/ssh_roundtrip.rs b/core/tests/ssh_roundtrip.rs index 2f5160a6..3bc4e3ff 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,241 @@ 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: 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, + /// `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 { + 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(methods)), + 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::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][..])), + 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 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] +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() { + 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}"); +}