diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index 4b229798..0884bce8 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -72,16 +72,16 @@ impl InteractiveCommand { // Check if authentication failed and password fallback is allowed // This matches SSH key failures as well as SSH agent authentication failures + // Also handles the case where russh disconnects during authentication failure + // (which returns SshError(Disconnect) instead of KeyAuthFailed) let result = match result { - Err( - SshError::KeyAuthFailed - | SshError::AgentAuthenticationFailed - | SshError::AgentNoIdentities - | SshError::AgentConnectionFailed - | SshError::AgentRequestIdentitiesFailed, - ) if allow_password_fallback && atty::is(atty::Stream::Stdin) => { + Err(ref err) + if allow_password_fallback + && atty::is(atty::Stream::Stdin) + && is_auth_error_for_password_fallback(err) => + { tracing::debug!( - "SSH authentication failed for {username}@{host}:{port}, attempting password fallback" + "SSH authentication failed for {username}@{host}:{port} ({err}), attempting password fallback" ); // Prompt for password (matching OpenSSH behavior) @@ -473,18 +473,56 @@ impl InteractiveCommand { /// - SSH agent has no identities loaded /// - SSH agent connection fails /// - SSH agent identity request fails +/// - SSH server disconnects during authentication (russh::Error::Disconnect) +/// This is a common behavior when the server rejects key authentication +/// and the russh library drops the connection before returning the auth result. /// /// These are all cases where falling back to password authentication makes sense, /// matching OpenSSH's behavior. +/// +/// # Important +/// The SshError(Disconnect) case is particularly important because russh may +/// disconnect the connection before returning the authentication failure result. +/// The log flow in this case is: +/// ```text +/// userauth_failure -> drop handle -> disconnected SshError(Disconnect) +/// ``` +/// Without handling this case, password fallback would never be triggered when +/// key authentication fails on servers that disconnect after auth failure. pub fn is_auth_error_for_password_fallback(error: &SshError) -> bool { - matches!( - error, + match error { + // Explicit authentication failures SshError::KeyAuthFailed - | SshError::AgentAuthenticationFailed - | SshError::AgentNoIdentities - | SshError::AgentConnectionFailed - | SshError::AgentRequestIdentitiesFailed - ) + | SshError::AgentAuthenticationFailed + | SshError::AgentNoIdentities + | SshError::AgentConnectionFailed + | SshError::AgentRequestIdentitiesFailed => true, + + // russh may disconnect after auth failure, which manifests as these errors + // This is a key fix for GitHub issue #113: the server may disconnect + // during authentication, and we should treat this as an auth failure + // that can be retried with password. + SshError::SshError(russh::Error::Disconnect) => { + tracing::debug!( + "Treating SshError(Disconnect) as auth failure - server likely \ + disconnected after key authentication rejection" + ); + true + } + + // RecvError can occur when the server closes the channel during auth + SshError::SshError(russh::Error::RecvError) => { + tracing::debug!( + "Treating SshError(RecvError) as auth failure - server likely \ + closed connection during authentication" + ); + true + } + + // All other errors should not trigger password fallback + // This includes: PasswordWrong, ServerCheckFailed, IoError, etc. + _ => false, + } } #[cfg(test)] @@ -574,4 +612,57 @@ mod tests { "KeyboardInteractiveAuthFailed should NOT trigger password fallback" ); } + + // Tests for issue #113: Handle SshError(Disconnect) during authentication + #[test] + fn test_ssh_disconnect_triggers_password_fallback() { + let error = SshError::SshError(russh::Error::Disconnect); + assert!( + is_auth_error_for_password_fallback(&error), + "SshError(Disconnect) should trigger password fallback - \ + server may disconnect after key auth rejection" + ); + } + + #[test] + fn test_ssh_recv_error_triggers_password_fallback() { + let error = SshError::SshError(russh::Error::RecvError); + assert!( + is_auth_error_for_password_fallback(&error), + "SshError(RecvError) should trigger password fallback - \ + server may close connection during authentication" + ); + } + + #[test] + fn test_ssh_hup_does_not_trigger_fallback() { + // HUP is a different type of disconnect that happens during normal operation + let error = SshError::SshError(russh::Error::HUP); + assert!( + !is_auth_error_for_password_fallback(&error), + "SshError(HUP) should NOT trigger password fallback - \ + this indicates remote closed connection, not auth failure" + ); + } + + #[test] + fn test_ssh_connection_timeout_does_not_trigger_fallback() { + let error = SshError::SshError(russh::Error::ConnectionTimeout); + assert!( + !is_auth_error_for_password_fallback(&error), + "SshError(ConnectionTimeout) should NOT trigger password fallback - \ + this is a network issue, not auth failure" + ); + } + + #[test] + fn test_ssh_not_authenticated_does_not_trigger_fallback() { + // NotAuthenticated means we haven't tried auth yet, not that auth failed + let error = SshError::SshError(russh::Error::NotAuthenticated); + assert!( + !is_auth_error_for_password_fallback(&error), + "SshError(NotAuthenticated) should NOT trigger password fallback - \ + this means auth hasn't been attempted yet" + ); + } } diff --git a/tests/password_fallback_test.rs b/tests/password_fallback_test.rs index fffb6a62..20026601 100644 --- a/tests/password_fallback_test.rs +++ b/tests/password_fallback_test.rs @@ -105,3 +105,59 @@ fn test_host_key_verification_not_bypassed() { "ServerCheckFailed must NOT trigger password fallback - host key verification is a security feature" ); } + +// Tests for issue #113: Handle SshError(Disconnect) during authentication +// The russh library may disconnect the connection before returning authentication failure, +// which manifests as SshError(Disconnect) instead of KeyAuthFailed. + +/// Test that SshError(Disconnect) triggers password fallback. +/// This is the key fix for GitHub issue #113. +#[test] +fn test_ssh_disconnect_during_auth_triggers_password_fallback() { + let error = SshError::SshError(russh::Error::Disconnect); + assert!( + is_auth_error_for_password_fallback(&error), + "SshError(Disconnect) should trigger password fallback - \ + server may disconnect after key authentication rejection (issue #113)" + ); +} + +/// Test that SshError(RecvError) triggers password fallback. +/// The server may close the channel during authentication, resulting in RecvError. +#[test] +fn test_ssh_recv_error_during_auth_triggers_password_fallback() { + let error = SshError::SshError(russh::Error::RecvError); + assert!( + is_auth_error_for_password_fallback(&error), + "SshError(RecvError) should trigger password fallback - \ + server may close connection during authentication" + ); +} + +/// Test that other SSH errors (HUP, ConnectionTimeout) do NOT trigger password fallback. +/// These indicate network issues, not authentication failures. +#[test] +fn test_other_ssh_errors_do_not_trigger_fallback() { + let non_auth_ssh_errors: Vec<(SshError, &str)> = vec![ + ( + SshError::SshError(russh::Error::HUP), + "HUP - remote closed connection", + ), + ( + SshError::SshError(russh::Error::ConnectionTimeout), + "ConnectionTimeout - network issue", + ), + ( + SshError::SshError(russh::Error::NotAuthenticated), + "NotAuthenticated - auth not attempted yet", + ), + ]; + + for (error, desc) in non_auth_ssh_errors { + assert!( + !is_auth_error_for_password_fallback(&error), + "{} should NOT trigger password fallback", + desc + ); + } +}