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