[Libguestfs] [PATCH nbdkit] ssh: Improve the error message when all authentication methods fail
Richard W.M. Jones
rjones at redhat.com
Thu Jan 5 16:02:15 UTC 2023
On Thu, Jan 05, 2023 at 04:49:16PM +0100, Laszlo Ersek wrote:
> On 1/5/23 12:34, Richard W.M. Jones wrote:
> > The current error message:
> >
> > nbdkit: ssh[1]: error: all possible authentication methods failed
> >
> > is confusing and non-actionable. It's hard even for experts to
> > understand the relationship between the authentication methods offered
> > by a server and what we require.
> >
> > Try to improve the error message in some common situations, especially
> > where password authentication on the server side is disabled but the
> > client supplied a password=... parameter. After this change, you will
> > see an actionable error:
> >
> > nbdkit: ssh[1]: error: the server does not offer password
> > authentication, but you tried to use a password; if you have root
> > access to the server, try editing 'sshd_config' and setting
> > 'PasswordAuthentication yes'; otherwise try using an SSH agent with
> > a passphrase
> >
> > Also remove an incidental comment left over when I copied the libssh
> > example code.
> >
> > See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2158300
> > ---
> > plugins/ssh/ssh.c | 35 ++++++++++++++++++++++++++++-------
> > 1 file changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
> > index 6cf40c26f..23c0b46f9 100644
> > --- a/plugins/ssh/ssh.c
> > +++ b/plugins/ssh/ssh.c
> > @@ -355,14 +355,35 @@ authenticate (struct ssh_handle *h)
> > rc = authenticate_pubkey (h->session);
> > if (rc == SSH_AUTH_SUCCESS) return 0;
> > }
> > + else if (password == NULL) {
> > + /* Because the password method below requires a password, we know
> > + * that it will fail, so print an actionable error message and
> > + * bail now.
> > + */
> > + nbdkit_error ("the server does not offer SSH agent authentication; "
> > + "try using a password=... parameter, see the "
> > + "nbdkit-ssh-plugin(1) manual page");
> > + return -1;
> > + }
>
> I think the "else" is wrong here.
Yeah, I think it looks wrong too. Part of the problem is that
previously we simply tried each authentication method in turn. In the
original libssh example code I was copying, there are more supported
methods. In this code there are only two.
Adding all these extra statements basically makes everything
interdependent.
> We can end up where the "else" starts for two reasons:
> - the server doesn't offer pubkey auth, or
> - we tried pubkey auth, but failed it.
>
> In either case, we need to proceed to password auth. If there is no
> password specified, we should bail out early, with the helpful error
> message, in *both* scenarios. But due to the "else", we're not going to
> do that if we try pubkey auth, and fail it.
>
> So I'd suggest just removing the "else" keyword.
>
> Also I think the message should not be tied to agent authentication, but
> pubkey authentication. The agent auth is useful if your private key is
> protected with a password. But if that's not the case, an agent is not
> required.
Hmm, I see. I hadn't really considered that case because who uses a
private key without a passphrase, but it's certainly possible, and
might happen in automated environments.
> > - /* Example code tries keyboard-interactive here, but we cannot use
> > - * that method from a server.
> > - */
> > -
> > - if (password != NULL && (method & SSH_AUTH_METHOD_PASSWORD)) {
> > - rc = authenticate_password (h->session, password);
> > - if (rc == SSH_AUTH_SUCCESS) return 0;
> > + if (password != NULL) {
>
> In turn, once you remove the "else" keyword from above, this (password
> != NULL) check becomes a tautology, and can be removed -- and the stuff
> in its scope can be unnested one level:
>
> > + if (method & SSH_AUTH_METHOD_PASSWORD) {
> > + rc = authenticate_password (h->session, password);
> > + if (rc == SSH_AUTH_SUCCESS) return 0;
> > + else {
>
> Side comment: I suggest not using "else" after a conditional "return";
> it only leads to needless nesting.
>
> > + nbdkit_error ("password authentication failed, "
> > + "is the username and password correct?");
> > + return -1;
> > + }
> > + }
> > + else {
> > + nbdkit_error ("the server does not offer password authentication, "
> > + "but you tried to use a password; if you have root access "
> > + "to the server, try editing 'sshd_config' and setting "
> > + "'PasswordAuthentication yes'; otherwise try using "
> > + "an SSH agent with a passphrase");
> > + return -1;
> > + }
> > }
> >
> > nbdkit_error ("all possible authentication methods failed");
>
> To simplify even further, you could swap around the outer if/else here,
> saving on even more nesting.
>
> Something like this, ultimately -- here I'm throwing in a reordering of
> steps as well (no need to ask the user for a password until the server
> offers password auth):
>
> diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
> index 5e314cd738ae..423c3f8f2046 100644
> --- a/plugins/ssh/ssh.c
> +++ b/plugins/ssh/ssh.c
> @@ -353,16 +353,20 @@ authenticate (struct ssh_handle *h)
> if (rc == SSH_AUTH_SUCCESS) return 0;
> }
>
> - /* Example code tries keyboard-interactive here, but we cannot use
> - * that method from a server.
> - */
> + if (!(method & SSH_AUTH_METHOD_PASSWORD)) {
> + nbdkit_error (... "server doesn't offer password auth, reconfig it" ...);
> + return -1;
> + }
>
> - if (password != NULL && (method & SSH_AUTH_METHOD_PASSWORD)) {
> - rc = authenticate_password (h->session, password);
> - if (rc == SSH_AUTH_SUCCESS) return 0;
> + if (password == NULL) {
> + nbdkit_error (... "provide a password" ...);
> + return -1;
> }
>
> - nbdkit_error ("all possible authentication methods failed");
> + rc = authenticate_password (h->session, password);
> + if (rc == SSH_AUTH_SUCCESS) return 0;
> +
> + nbdkit_error ("password authentication failed, user/pass correct?");
> return -1;
> }
>
> With this, I actually notice a tricky code path through your version:
> it's rooted again in that "else". Namely, if we try pubkey auth, and
> fail it, and *also* do not provide a password, then we'll just say at
> the end: "all possible authentication methods failed". Instead, we
> should say "try a password!" in this case -- and then the "all possible
> authentication methods failed" at the end becomes unreachable, and can
> be removed; like I'm trying to show above.
>
> Sorry if I missed something.
Let me have a rethink of this one.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
More information about the Libguestfs
mailing list