fix: Handle SshError(Disconnect) during auth for password fallback (issue #113)

[inureyes]

Summary

Fixes #113 - Password fallback not working when SSH key authentication fails

When SSH key authentication fails, the russh library may disconnect the connection before returning the authentication failure result. This causes the error to manifest as SshError(Disconnect) instead of KeyAuthFailed, which previously prevented the password fallback mechanism from triggering.

Changes

• Update is_auth_error_for_password_fallback() function to recognize:
  • SshError(Disconnect) - server disconnects after key auth rejection
  • SshError(RecvError) - server closes connection during auth
• Refactor establish_connection() to use the helper function for consistent error classification
• Add comprehensive unit tests for new error handling in connection.rs
• Add integration tests for issue Password fallback not working when SSH key authentication fails #113 scenarios in password_fallback_test.rs

Root Cause Analysis

The russh library behavior when key authentication fails:

userauth_failure -> drop handle -> disconnected SshError(Disconnect)

This means the password fallback condition matching was not receiving KeyAuthFailed error, but instead SshError(Disconnect).

Test plan

• All existing unit tests pass
• All existing integration tests pass
• New tests for SshError(Disconnect) handling pass
• New tests for SshError(RecvError) handling pass
• Verified that non-auth errors (HUP, ConnectionTimeout) do NOT trigger fallback
• Clippy passes without warnings
• Manual testing on SSH servers that disconnect after key auth rejection

[inureyes]

Security & Performance Review

Analysis Summary

• Scope: changed-files
• Languages: Rust
• Total Issues: 3 (0 Critical, 0 High, 1 Medium, 2 Low)
• Files Changed: 2
  • src/commands/interactive/connection.rs (+106, -15)
  • tests/password_fallback_test.rs (+56, -0)

Changes Overview

This PR fixes issue #113 where password fallback was not working when SSH key authentication fails. The root cause was that the russh library may disconnect the connection before returning KeyAuthFailed, instead returning SshError(Disconnect) or SshError(RecvError).

Key Changes:

1. Extended is_auth_error_for_password_fallback() to recognize Disconnect and RecvError as auth failures
2. Refactored establish_connection() to use the helper function consistently
3. Added comprehensive unit and integration tests

---

Prioritized Issues

MEDIUM - Potential False Positive Authentication Fallback

Location: src/commands/interactive/connection.rs lines 505-520

Issue: The PR treats Disconnect and RecvError as authentication failures. However, these errors can occur in non-authentication scenarios:

• Network interruption during SSH negotiation
• Server termination due to resource constraints
• Rate limiting or firewall intervention

Security Consideration: If a connection fails before authentication is attempted, the user might be prompted for a password unnecessarily. In an adversarial scenario, a MITM attacker could trigger this behavior.

Mitigating Factors:

1. Password fallback requires allow_password_fallback=true AND atty::is(atty::Stream::Stdin)
2. Host key verification still runs before auth attempts
3. Debug logging provides forensic context
4. OpenSSH exhibits similar heuristic behavior
5. Tests verify HUP, ConnectionTimeout, NotAuthenticated do NOT trigger fallback

Risk Assessment: The trade-off is acceptable because:

• Without this change, password fallback never works on servers that disconnect after auth rejection
• The alternative (broken password fallback) is worse than occasional false prompts
• Security-conscious users can disable password mode entirely

Status: Acceptable - No changes required

---

LOW - Test Coverage Enhancement Opportunity

Location: tests/password_fallback_test.rs

Issue: Tests cover individual error type classification well, but lack integration-level mock testing for the actual connection flow where:

1. TCP connection succeeds
2. SSH negotiation succeeds
3. Key auth is attempted
4. Server disconnects mid-auth
5. Password prompt appears

Recommendation: Consider adding a future integration test with a mock SSH server to validate the end-to-end flow.

Status: Acceptable - Current unit tests are sufficient for this fix

---

LOW - Documentation Enhancement

Location: src/commands/interactive/connection.rs lines 468-491

Issue: The documentation is thorough but could note that Disconnect/RecvError handling is a heuristic that prioritizes functionality over precision.

Recommendation: Consider adding a note like:

/// # Heuristics
/// The `Disconnect` and `RecvError` cases are heuristics - these errors
/// may occasionally occur outside of authentication contexts. This trade-off
/// prioritizes OpenSSH-compatible behavior over absolute precision.

Status: Acceptable - Current documentation is adequate

---

Positive Observations

1. Security Best Practices: The code correctly:

   • Does NOT trigger fallback for PasswordWrong (prevents infinite loops)
   • Does NOT trigger fallback for ServerCheckFailed (preserves host key verification)
   • Maintains rate limiting and timing attack mitigation in establish_connection()

2. Code Quality:

   • Well-documented function with clear explanation of behavior
   • Comprehensive test coverage for both positive and negative cases
   • Debug logging provides useful forensic information

3. Behavioral Correctness:

   • Matches OpenSSH's behavior for auth failure handling
   • Tests verify the distinction between auth failures and non-auth errors

---

Test Results

cargo test: All tests pass
cargo clippy: No warnings

---

Verdict

Approved - The changes correctly address issue #113 with appropriate security considerations. The heuristic approach to treating Disconnect/RecvError as potential auth failures is a reasonable trade-off that matches OpenSSH behavior.

Checklist

• Security implications reviewed
• Performance impact assessed (none - error handling only)
• Test coverage verified
• Code quality reviewed
• Documentation reviewed