fix: SSH does not try multiple authentication methods of the same type#264
Conversation
Signed-off-by: Christian Groschupp <christian@groschupp.org>
cgroschupp
force-pushed
the
fix/ssh-doesnt-tries-multiple-authmethods-from-the-same-type-v2
branch
from
836b675 to
764cbee
Compare
|
@kke can you please trigger again the workflows? |
|
@kke can you please review this PR? |
|
@twz123 Could you please review this PR? |
|
This seems to target the main branch which isn't currently used anywhere. The |
There was a problem hiding this comment.
Pull request overview
This PR fixes SSH authentication behavior where multiple publickey auth methods were configured separately, causing golang.org/x/crypto/ssh to try only the first one and skip later key-based methods (e.g., an explicitly configured key file when agent keys exist).
Changes:
- Replace caching of
ssh.AuthMethodper keyPath with caching ofssh.Signer. - Build a single
ssh.PublicKeys(signers...)auth method from agent signers + keyPath-derived signers to ensure multiple keys of the same method type are attempted. - Adjust helper functions to return
ssh.Signerinstead ofssh.AuthMethod.
Comments suppressed due to low confidence (1)
protocol/ssh/connection.go:345
config.Authis appended withssh.PublicKeys(signers...)unconditionally. Whensignersis empty (no agent + no usable keyPaths), this still adds an AuthMethod, making thelen(config.Auth) == 0check ineffective and shifting the failure to a later/less clear auth error. Suggest only appending thePublicKeysauth method whenlen(signers) > 0, so the existing "no usable authentication method found" error is preserved.
config.Auth = append(config.Auth, ssh.PublicKeys(signers...))
if len(config.Auth) == 0 {
return nil, fmt.Errorf("%w: no usable authentication method found", protocol.ErrAbort)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(c.AuthMethods) > 0 { | ||
| log.Trace(context.Background(), "using passed-in auth methods", "count", len(c.AuthMethods)) | ||
| config.Auth = c.AuthMethods | ||
| } else if len(signers) > 0 { | ||
| c.Log().Debug("using all keys from ssh agent", "count", len(signers)) | ||
| } |
There was a problem hiding this comment.
signers is populated from the SSH agent before keyPath signers are appended. This means explicitly configured identity files may be tried only after potentially many agent keys, which can still fail in practice due to server-side MaxAuthTries before the keyPath key is attempted. Consider collecting keyPath-derived signers first (and/or prioritizing Config.KeyPath), then appending agent signers after, and pass only the agent signers into pkeySigner when resolving .pub keys against the agent.
| case ssh.Signer: | ||
| log.Trace(context.Background(), "using cached auth method", log.FileAttr(keyPath)) | ||
| signers = append(signers, signerCacheItem) | ||
| case error: | ||
| log.Trace(context.Background(), "already discarded key", log.FileAttr(keyPath), log.ErrorAttr(signerCacheItem)) | ||
| default: | ||
| log.Trace(context.Background(), fmt.Sprintf("unexpected type %T for cached auth method for %s", am, keyPath)) |
There was a problem hiding this comment.
Trace messages in this cache branch still refer to a cached "auth method", but the cache now stores ssh.Signer values. Updating these log strings (and the "unexpected type" message) to refer to cached signers would avoid confusion during debugging.
|
Recreated in #297 because this fell too far behind |
See here: #265 |
The ssh does not try multiple authentication methods of the same type.
Fixes #237, k0sproject/k0sctl#674