Skip to content

fix: skip SSH agent when no keys loaded to prevent auth failure#12

Merged
mcheemaa merged 2 commits intomainfrom
fix/ssh-agent-empty-signers
Apr 5, 2026
Merged

fix: skip SSH agent when no keys loaded to prevent auth failure#12
mcheemaa merged 2 commits intomainfrom
fix/ssh-agent-empty-signers

Conversation

@mcheemaa
Copy link
Copy Markdown
Member

@mcheemaa mcheemaa commented Apr 5, 2026

Summary

Fixes a bug introduced in #10 where an empty SSH agent poisons the publickey auth chain, breaking SSH for most macOS users.

Root cause: Go's x/crypto/ssh library, when PublicKeysCallback returns zero signers from an empty agent, reports the publickey method as failed with an empty allowed-methods list. This prevents subsequent publickey auth methods (like the key file fallback) from being tried.

Impact: On macOS, SSH_AUTH_SOCK is always set by the system. If no keys are explicitly loaded in the agent (common), PR #10's code fails to authenticate even when a valid key file exists at ~/.ssh/id_ed25519.

Changes

  • Check agent.Signers() before adding the agent as an auth method. Only use the agent if it actually has keys loaded.
  • Close the agent connection immediately after extracting signers (fixes FD leak from fix: support SSH agent and passphrase-protected keys #10).
  • Replace em dash with regular dash in comment.

Testing

Verified against live Hetzner VM (specter update murphy):

Scenario Before After
Agent running, no keys, key file present FAIL PASS
No agent, key file only PASS PASS
No auth available Clear error Clear error

Go's x/crypto/ssh library fails the entire publickey auth chain when
PublicKeysCallback returns zero signers from an empty agent. This
breaks SSH for macOS users where SSH_AUTH_SOCK is always set but no
keys are explicitly loaded in the agent.

Now checks agent.Signers() before adding the agent as an auth method.
Also closes the agent connection immediately after use (fixes FD leak
from #10) and removes em dash from comment.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: abd9674d40

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +303 to +304
authMethods = append(authMethods, ssh.PublicKeysCallback(agentClient.Signers))
conn.Close()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep agent socket open for PublicKeysCallback auth

ssh.PublicKeysCallback(agentClient.Signers) is lazy and runs during ssh.Dial, but this change closes conn immediately afterward, so the callback has no live agent transport when authentication actually starts. In environments that rely on agent-backed keys (for example passphrase-protected keys that cannot be parsed from ~/.ssh/id_*), agent auth now fails deterministically even when keys are loaded, which regresses the main SSH path this function is meant to support.

Useful? React with 👍 / 👎.

PublicKeysCallback calls Signers() lazily during the SSH handshake,
not when the auth method is added. The previous commit closed the
agent connection too early, which would break agent auth for users
with keys loaded. Now keeps the connection open through Dial and
closes it after, whether success or failure.
@mcheemaa mcheemaa merged commit 7c8dd1f into main Apr 5, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant