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

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;
+  }
 
-  /* 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) {
+    if (method & SSH_AUTH_METHOD_PASSWORD) {
+      rc = authenticate_password (h->session, password);
+      if (rc == SSH_AUTH_SUCCESS) return 0;
+      else {
+        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");
-- 
2.37.3

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