[Libguestfs] [PATCH nbdkit] ssh: Improve the error message when all authentication methods fail

[Laszlo Ersek]

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.

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.

>
> -  /* 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.

Laszlo