Conversation
…ump hosts When SSH_AUTH_SOCK is set but the agent has no loaded identities, bssh now falls back to key file authentication instead of failing. This matches OpenSSH's behavior where ssh -J works by trying the agent first, then falling back to key files when the agent is empty. The fix adds an agent_has_identities() helper function that queries the agent before returning AuthMethod::Agent. If the agent has no identities or communication fails, the code falls through to try SSH key files. Closes #116
Adds test file to verify the authentication fallback chain when SSH agent is running but has no loaded identities. Tests verify: - Fallback to key files when SSH_AUTH_SOCK is not set - Agent socket scenario documentation - --use-agent flag fallback behavior - Authentication fallback chain order - Timeout behavior expectations
Addresses two HIGH severity issues identified by pr-reviewer:
1. Performance: Duplicate agent identity checks
- Previously, agent_has_identities() was called up to 2 times in
determine_auth_method(), and identities were queried again in
authenticate_connection()
- Now caches the result at the start of determine_auth_method()
- Reduces agent queries from 2-3 to just 1 per authentication
2. Error handling: Missing timeout on agent connection
- Added 5-second timeout to agent operations to prevent indefinite
hangs when agent is unresponsive (e.g., waiting for hardware token)
- Gracefully falls back to key file authentication on timeout
Adds 9 unit tests to verify the core authentication logic: - test_agent_timeout_constant: Verifies 5-second timeout constant - test_agent_available_false_when_no_socket: Tests SSH_AUTH_SOCK check - test_agent_has_identities_invalid_socket: Tests invalid socket handling - test_determine_auth_method_fallback_to_key_file: Tests key file fallback - test_determine_auth_method_prefers_agent_when_available: Tests agent priority - test_determine_auth_method_tries_default_keys: Tests ~/.ssh/ key discovery - test_determine_auth_method_fails_when_no_method_available: Tests error case - test_agent_caching_design: Documents caching behavior - test_timeout_design: Documents timeout behavior These tests verify the actual authentication fallback logic instead of just documentation/sanity tests, addressing the PR reviewer's MEDIUM issue about test quality.
inureyes
added
type:bug
Applies the same fix from jump host auth to the main SSH authentication module. When SSH agent is running but has no loaded identities, now correctly falls back to key file authentication instead of failing. This fixes the issue where jump host connection succeeds but destination host authentication fails because the main auth module was not checking agent identity availability. The fix: - Adds agent_has_identities() function with 5-second timeout - agent_auth() now verifies agent has identities before returning Agent method - Falls back to default key files (~/.ssh/id_rsa, etc.) when agent is empty
The test was failing because our fix now checks if the SSH agent has identities before returning AuthMethod::Agent. The fake Unix socket used in the test doesn't implement the SSH agent protocol. Updated the test to: - Create a fake SSH key file for fallback - Accept either Agent (real agent with keys) or PrivateKeyFile (fallback) as valid outcomes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ssh -Jworks by trying agent first, then falling back to key filesProblem
Jump host authentication fails when SSH agent is running but has no loaded identities, even though valid SSH key files exist on disk. This differs from OpenSSH behavior, which gracefully falls back to key files when the agent returns an empty identity list.
Solution
agent_has_identities()helper function that queries the agent before returningAuthMethod::Agentdetermine_auth_method()to avoid redundant queriesTest plan
Closes #116