Re: Should ssh_userauth_none() produce an error?

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

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