fix(ssh): wrap agent signers to continue to next identity on signing failure#3401
fix(ssh): wrap agent signers to continue to next identity on signing failure#3401Jason-Shen2 wants to merge 1 commit into
Conversation
…failure When an SSH agent signer fails to sign (e.g. user cancels a FIDO2/security-key user-presence prompt), golang.org/x/crypto/ssh treats the error as fatal and aborts the entire publickey authentication, preventing remaining identities from being tried. This deviates from OpenSSH behavior where a declined signature simply moves on to the next key. This change wraps agent-backed signers in a failoverSigner that returns a deliberately invalid signature on signing failure instead of propagating the error. The server rejects the invalid signature as a normal auth failure, allowing RetryableAuthMethod to continue with the next identity. Also adds conndebug logging for: - Agent socket dial failures - Agent key listing errors - Number of identities provided by the agent - Each agent identity being attempted - Per-key signing failures Fixes wavetermdev#3365
|
zhenxing.shen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughA new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/remote/sshsigners.go`:
- Around line 39-49: The failover path in failoverSigner.SignWithAlgorithm is
returning a signature whose Format is based on the key type instead of the
requested signing algorithm. Update invalidSignature (and its call site in
SignWithAlgorithm) so the fallback signature preserves the algorithm passed into
SignWithAlgorithm, using that value for Signature.Format rather than
signer.PublicKey().Type().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e89d98ce-5add-43a8-86ec-6f94637a33b3
📒 Files selected for processing (2)
pkg/remote/sshclient.gopkg/remote/sshsigners.go
| blocklogger.Infof(f.connCtx, "[conndebug] agent signing failed for key %s %s (%v); continuing with next identity\n", | ||
| f.signer.PublicKey().Type(), ssh.FingerprintSHA256(f.signer.PublicKey()), err) | ||
| return f.invalidSignature(), nil | ||
| } | ||
| return f.Sign(rand, data) | ||
| } | ||
|
|
||
| func (f failoverSigner) invalidSignature() *ssh.Signature { | ||
| return &ssh.Signature{ | ||
| Format: f.signer.PublicKey().Type(), | ||
| Blob: []byte("invalid-signature-identity-skipped"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline pkg/remote/sshsigners.go --view expanded || true
printf '\n== relevant file lines ==\n'
cat -n pkg/remote/sshsigners.go | sed -n '1,220p'
printf '\n== search for invalidSignature and SignWithAlgorithm usages ==\n'
rg -n "invalidSignature|SignWithAlgorithm|Sign\(" pkg/remote -S || trueRepository: wavetermdev/waveterm
Length of output: 3219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== go env GOPATH GOMODCACHE ==\n'
go env GOPATH GOMODCACHE 2>/dev/null || true
printf '\n== locate ssh package source in module cache ==\n'
GOMODCACHE="$(go env GOMODCACHE 2>/dev/null || true)"
if [ -n "${GOMODCACHE:-}" ] && [ -d "$GOMODCACHE" ]; then
fd -a "signature.go" "$GOMODCACHE" | rg "golang.org/x/crypto/ssh|crypto/ssh" || true
fd -a "signer.go" "$GOMODCACHE" | rg "golang.org/x/crypto/ssh|crypto/ssh" || true
fi
printf '\n== grep for Signature.Format docs and SignWithAlgorithm implementations ==\n'
if [ -n "${GOMODCACHE:-}" ] && [ -d "$GOMODCACHE" ]; then
rg -n "type Signature|Format.*algorithm|SignWithAlgorithm|AlgorithmSigner" "$GOMODCACHE"/**/golang.org/x/crypto@*/ssh 2>/dev/null || true
fiRepository: wavetermdev/waveterm
Length of output: 359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for failoverSigner usage ==\n'
rg -n "failoverSigner|invalidSignature\(" -S .
printf '\n== search for ssh signer/auth flow nearby ==\n'
rg -n "AlgorithmSigner|PublicKeys|PublicKeyCallback|ssh.NewSigner|ssh.Signer" pkg -SRepository: wavetermdev/waveterm
Length of output: 2338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant sshclient lines around public key callback ==\n'
cat -n pkg/remote/sshclient.go | sed -n '276,360p'
printf '\n== relevant sshclient lines around authSockSigner wiring ==\n'
cat -n pkg/remote/sshclient.go | sed -n '760,820p'Repository: wavetermdev/waveterm
Length of output: 7633
🌐 Web query:
golang.org/x/crypto/ssh Signature.Format SignWithAlgorithm algorithm string RSA rsa-sha2-256 documentation
💡 Result:
In the Go golang.org/x/crypto/ssh package, the Signature struct holds a Format field of type string, which identifies the signature algorithm used (e.g., "rsa-sha2-256" or "rsa-sha2-512") [1][2]. For custom signing implementations, the AlgorithmSigner interface allows you to specify a signing algorithm through the SignWithAlgorithm method [3][4]: type AlgorithmSigner interface { Signer // SignWithAlgorithm is like Signer.Sign, but allows specifying a desired // signing algorithm. SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error) } When implementing SignWithAlgorithm for RSA keys, you can pass "rsa-sha2-256" or "rsa-sha2-512" as the algorithm argument [3][5]. The implementation is expected to perform the following steps [3]: 1. Validate that the requested algorithm is supported for the key type (e.g., using ssh.KeyAlgoRSASHA256 or ssh.KeyAlgoRSASHA512) [3][2]. 2. Use the corresponding hash function to hash the input data. 3. Perform the cryptographic signature operation using the underlying RSA key. 4. Construct and return a Signature struct where the Format field is set to the algorithm string provided, and the Blob contains the signature data [3]. Note that rsa-sha2-256 and rsa-sha2-512 are recognized as signature algorithm identifiers in this package [2]. While the underlying public key format for both is still ssh-rsa, these identifiers allow the SSH protocol to negotiate stronger hashing algorithms than the legacy ssh-rsa (which uses SHA-1) [2]. When verifying or signing, the ssh package uses these strings to select the appropriate hash function for the operation [3][2].
Citations:
- 1: https://github.com/golang/crypto/blob/master/ssh/certs.go
- 2: https://go.googlesource.com/crypto/+/refs/tags/v0.23.0/ssh/keys.go
- 3: https://github.com/golang/crypto/blob/master/ssh/keys.go
- 4: https://go.googlesource.com/crypto/+/master/ssh/keys.go
- 5: golang/crypto@5d542ad
Preserve the requested signing algorithm in the failover signature. SignWithAlgorithm should return a Signature.Format matching algorithm (for example, rsa-sha2-256), not the key type, so the fallback path doesn’t advertise the wrong signature algorithm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/remote/sshsigners.go` around lines 39 - 49, The failover path in
failoverSigner.SignWithAlgorithm is returning a signature whose Format is based
on the key type instead of the requested signing algorithm. Update
invalidSignature (and its call site in SignWithAlgorithm) so the fallback
signature preserves the algorithm passed into SignWithAlgorithm, using that
value for Signature.Format rather than signer.PublicKey().Type().
2 participants
Fixes #3365
Problem
When an SSH agent-backed key fails to sign (e.g. user cancels a FIDO2 / security-key user-presence prompt), the
crypto/sshpublickey callback returns an error. The SSH library treats any callback error as fatal and aborts the entire authentication flow — it does not try any remaining agent identities or keyfiles. This differs from OpenSSH, which moves on to the next key when a signing request is declined.For users with multiple identities in their agent (e.g. a YubiKey/FIDO2 key first, then a regular ed25519 key), tapping "decline" on the hardware key prompt prevents any subsequent key from being attempted, even when the server would accept it.
Root cause
agentClient.Signers()returns[]ssh.Signerthat call through to the agent. IfSign()returns an error (e.g.agent.errNoMoreKeys/user cancellation),ssh.PublicKeysCallbackpropagates that error andRetryableAuthMethodstops retrying because the method itself failed, not because authentication was rejected.Fix
Wrap each agent-backed signer in a new
failoverSigner(sshsigners.go) that:ssh.Signerandssh.AlgorithmSigner.Sign/SignWithAlgorithmfailure, logs a[conndebug]message and returns a deliberately invalid signature (a blob that cannot verify) instead of an error.sshtreats as a normal per-key failure, allowingRetryableAuthMethodto continue with the next identity — exactly matching OpenSSH's behavior.Also adds [conndebug] logging to aid troubleshooting:
Testing
go build ./...passesgo vet ./pkg/remote/passesgo test ./pkg/remote/passes (includesTestDialIdentityAgentUnix)No behavioral change when signers succeed; only the error path changes so that a rejected hardware-key prompt no longer blocks fallback to other identities.