[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Should ssh_userauth_none() produce an error?
- Subject: Re: Should ssh_userauth_none() produce an error?
- From: Andreas Schneider <asn@xxxxxxxxxxxxxx>
- Reply-to: libssh@xxxxxxxxxx
- Date: Mon, 27 Aug 2018 12:09:06 +0200
- To: Jakub Jelen <jjelen@xxxxxxxxxx>
- Cc: libssh@xxxxxxxxxx
On Monday, 27 August 2018 10:03:38 CEST Jakub Jelen wrote: > On Mon, 2018-08-27 at 09:32 +0200, Andreas Schneider wrote: > > On Sunday, 26 August 2018 22:30:29 CEST Jakub Jelen wrote: > > > Hello, > > > > Hi Jakub, > > > > > at this moment, I am debugging application, that is using the above > > > method to figure out the list of supported authentication > > > mechanisms > > > and is failing later without any specific error message (my other > > > email > > > from today). Then we ask for the last error, we are getting the > > > error > > > from the above authentication method, which should not be an error > > > at > > > all (actually confirmed from the server log -- the only failed > > > method > > > was the "none" one probing for the supported authentication > > > methods): > > > > > > read failed: Access denied. Authentication that can continue: > > > publickey,gssapi-keyex,gssapi-with-mic,password (libssh error code: > > > 1, > > > sftp error code: 0) > > > > > > Reading through the tests, they make sure this function returns > > > this > > > error, but I don't think this is a very good idea, because almost > > > any > > > other failure, which will not set its own error will see this error > > > message. > > > > > > It is also confusing, that this type of message does not list what > > > authentication method was used (none) so this is something that > > > should > > > get fixed at least. > > > > > > I understand, this might be harder to change, since this might be > > > considered as part of API, but this email is mostly to start a > > > discussion if others might see this as a problem as well or what > > > can be > > > done about such lurking errors in the code. > > > > Thanks for bringing this up, what do you think about the following > > branch? > > > > https://gitlab.com/cryptomilk/libssh-mirror/commits/master-auth > > Putting authentication-related variables in the session struct makes it > more clear what the members do. The function for resetting error was > something I was already searching for so I believe this will come handy > also in other cases. Yes, I think there are more places where the function should be called. > Resetting the error on the successful > authentication sounds good place to me. As well as the improved logging > makes it more clear what failed. This looks like you managed to handle > this without API change which is indeed good. > > I noticed only a copy&paste error in ssh_userauth_password(), which > says SSH_AUTH_METHOD_PUBLICKEY instead of SSH_AUTH_METHOD_PASSWORD. Oh, the function uses the wrong pending_call_state, that's why I had PUBLICKEY there, well spotted. I've fixed both issues. > Additionally, what confused me is also that doc/mainpage.dox lists > hostbased authentication as supported, while there is no such method > implemented in auth.c -- the documentation should be probably updated > (attached trivial patch). RB+ -- Andreas Schneider asn@xxxxxxxxxxxxxx GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D