Adds support for multiple SSH auth mechanisms being used sequentially#5305
Conversation
|
Thanks for your PR! I'm not that knowledgeable around our SSH backend, but maybe @tiennou can chime in here? |
tiennou
left a comment
tiennou
left a comment
There was a problem hiding this comment.
That looks sensible — I'd expect something like that to happen with more than one authentication method enforced by the server, though I've only skimmed the RFC 😉. This is part of the problem I have with #5253 : we don't carry around a "complete" SSH protocol suite, so there's a bunch of testing needed, and I haven't got around to reading that much documentation yet… (For the record, libssh2 has a Docker-powered clar-run openssh server helper).
I only have a minor comment on styling, but many thanks @kas-luthor for the fix, and the time taken to debug it.
|
|
||
| error = _git_ssh_authenticate_session(session, cred); | ||
|
|
||
| if (error == GIT_EAUTH) { |
There was a problem hiding this comment.
You can clobber error here, so no need to declare sub_error.
/* refresh auth methods */
if (error == GIT_EAUTH &&
(error = list_auth_methods(…) < 0))
goto done;There was a problem hiding this comment.
I disagree, with that change error will no longer be GIT_EAUTH after the call to list_auth_methods() and cancel the loop.
There was a problem hiding this comment.
Actually… there's nothing in here that actually prevents a server from swamping you into a loop by repeatedly switching auth methods and continuously failing to authenticate, so maybe that's something that needs to happen… Not really related, but you've just mentioned a loop, and I happened to completely miss its condition 😉.
So; I'm okay with your initial version then. If you happen to add a auth counter or something, that would be lovely. I'm not sure how it would really work though…
There was a problem hiding this comment.
So; I'm okay with your initial version then. If you happen to add a auth counter or something, that would be lovely. I'm not sure how it would really work though…
Does the server even need to switch auth methods? He could just refuse the authentication repeatedly, which would cause us to loop infinitely here until one manages to send the correct credentials. Or am I missing something?
There was a problem hiding this comment.
@pks-t Authentication can be cancelled by the callback by returning an error code
|
|
||
| error = _git_ssh_authenticate_session(session, cred); | ||
|
|
||
| if (error == GIT_EAUTH) { |
There was a problem hiding this comment.
Actually… there's nothing in here that actually prevents a server from swamping you into a loop by repeatedly switching auth methods and continuously failing to authenticate, so maybe that's something that needs to happen… Not really related, but you've just mentioned a loop, and I happened to completely miss its condition 😉.
So; I'm okay with your initial version then. If you happen to add a auth counter or something, that would be lovely. I'm not sure how it would really work though…
|
Thanks a lot for your work, @kas-luthor.
I guess we can just wait for somebody to complain and add it later, if required at all. |
4 participants
Needed to make eg. 2FA work
Fixes the following problem:
pubkeyandkeyboard-interactive, both are required).pubkeyas an available mechanismcred_acquire_cbwithGIT_CREDTYPE_SSH_KEYset andGIT_CREDTYPE_SSH_INTERACTIVEunset.pubkeyauthentication is completed successfully, but the server responds with a failure code since not all mechanisms have successfully completed.keyboard-interactiveon the server side.cred_acquire_cbwithGIT_CREDTYPE_SSH_KEYset andGIT_CREDTYPE_SSH_INTERACTIVEunset.cred_acquire_cb