Re: [Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh

[Pino Toscano]

On Tuesday, 18 June 2019 15:14:30 CEST Max Reitz wrote:
> On 18.06.19 11:24, Pino Toscano wrote:
> > Rewrite the implementation of the ssh block driver to use libssh instead
> > of libssh2.  The libssh library has various advantages over libssh2:
> > - easier API for authentication (for example for using ssh-agent)
> > - easier API for known_hosts handling
> > - supports newer types of keys in known_hosts
> > 
> > Use APIs/features available in libssh 0.8 conditionally, to support
> > older versions (which are not recommended though).
> > 
> > Adjust the tests according to the different error message, and to the
> > newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
> > and libssh >= 0.7.0.
> > 
> > Adjust the various Docker/Travis scripts to use libssh when available
> > instead of libssh2. The mingw/mxe testing is dropped for now, as there
> > are no packages for it.
> > 
> > Signed-off-by: Pino Toscano <address@hidden>
> > Tested-by: Philippe Mathieu-Daudé <address@hidden>
> > Acked-by: Alex Bennée <address@hidden>
> > ---
> > 
> > Changes from v9:
> > - restored "default" case in the server status switch for libssh < 0.8.0
> > - print the host key type & fingerprint on mismatch with known_hosts
> > - improve/fix message for failed socket_set_nodelay()
> > - reset s->sock properly
> > 
> > Changes from v8:
> > - use a newer key type in iotest 207
> > - improve the commit message
> > 
> > Changes from v7:
> > - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> > - ptrdiff_t -> size_t
> > 
> > Changes from v6:
> > - fixed few checkpatch style issues
> > - detect libssh 0.8 via symbol detection
> > - adjust travis/docker test material
> > - remove dead "default" case in a switch
> > - use variables for storing MIN() results
> > - adapt a documentation bit
> > 
> > Changes from v5:
> > - adapt to newer tracing APIs
> > - disable ssh compression (mimic what libssh2 does by default)
> > - use build time checks for libssh 0.8, and use newer APIs directly
> > 
> > Changes from v4:
> > - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> > - fix few return code checks
> > - remove now-unused parameters in few internal functions
> > - allow authentication with "none" method
> > - switch to unsigned int for the port number
> > - enable TCP_NODELAY on the socket
> > - fix one reference error message in iotest 207
> > 
> > Changes from v3:
> > - fix socket cleanup in connect_to_ssh()
> > - add comments about the socket cleanup
> > - improve the error reporting (closer to what was with libssh2)
> > - improve EOF detection on sftp_read()
> > 
> > Changes from v2:
> > - used again an own fd
> > - fixed co_yield() implementation
> > 
> > Changes from v1:
> > - fixed jumbo packets writing
> > - fixed missing 'err' assignment
> > - fixed commit message
> > 
> >  .travis.yml                                   |   4 +-
> >  block/Makefile.objs                           |   6 +-
> >  block/ssh.c                                   | 665 ++++++++++--------
> >  block/trace-events                            |  14 +-
> >  configure                                     |  65 +-
> >  docs/qemu-block-drivers.texi                  |   2 +-
> >  .../dockerfiles/debian-win32-cross.docker     |   1 -
> >  .../dockerfiles/debian-win64-cross.docker     |   1 -
> >  tests/docker/dockerfiles/fedora.docker        |   4 +-
> >  tests/docker/dockerfiles/ubuntu.docker        |   2 +-
> >  tests/docker/dockerfiles/ubuntu1804.docker    |   2 +-
> >  tests/qemu-iotests/207                        |   4 +-
> >  tests/qemu-iotests/207.out                    |   2 +-
> >  13 files changed, 423 insertions(+), 349 deletions(-)
> 
> [...]
> 
> > diff --git a/block/ssh.c b/block/ssh.c
> > index 6da7b9cbfe..644ae8b82c 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
> 
> [...]
> 
> > +    case SSH_SERVER_KNOWN_CHANGED:
> > +        ret = -EINVAL;
> > +        r = ssh_get_publickey(s->session, &pubkey);
> > +        if (r == 0) {
> > +            r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA1,
> > +                                       &server_hash, &server_hash_len);
> > +            pubkey_type = ssh_key_type(pubkey);
> > +            ssh_key_free(pubkey);
> > +        }
> > +        if (r == 0) {
> > +            fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
> > +                                                   server_hash,
> > +                                                   server_hash_len);
> > +            ssh_clean_pubkey_hash(&server_hash);
> > +        }
> > +        if (fingerprint) {
> > +            error_setg(errp,
> > +                       "host key (%s key with fingerprint %s) does not 
> > match "
> > +                       "the one in known_hosts",
> > +                       ssh_key_type_to_char(pubkey_type), fingerprint);
> > +            ssh_string_free_char(fingerprint);
> > +        } else  {
> > +            error_setg(errp, "host key does not match the one in 
> > known_hosts");
> > +        }
> 
> Usually I’d say that more information can’t be bad.  But here I don’t
> see how this additional information is useful.  known_hosts contains
> base64-encoded full public keys.  This only prints the SHA-1 digest.

Note that SHA-1 is printed with libssh < 0.8; with libssh >= 0.8 SHA-256
is used instead.

> The user cannot add that to known_hosts, or easily scan known_hosts to
> see whether it perhaps belongs to some other hosts (maybe because DHCP
> scrambled something).
> 
> Actually, even if this printed the base64 representation of the full
> key, the user probably wouldn’t just add that to known_hosts or do
> anything with it.  They’ll debug the problem with other tools.
> 
> So I don’t think this new information is really useful...?

When this situation happens with openssh, the big scary message prints
three things:
1) the fingerprint of the server
2) the line of the server in the known_hosts file
3) how to remove the keys of the server from known_hosts, i.e. a
   copy-paste'able `ssh-keygen -R host`

Here I'm doing only (1).  Also, the current libssh2 driver does
something else, i.e. print the base64/printable representation of the
server key in known_hosts.

> (Hmm, I don’t know if this is a response to my “But the specification
> requires a warning!!1!”, but if so, I was actually not referring to more
> information but to this:

You mentioned this few times: can you please point me to this bit?
I read few RFCs related to ssh, and I did not find this information...

> $ ssh 192.168.0.12
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
> @    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
> IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
> Someone could be eavesdropping on you right now (man-in-the-middle attack)!
> It is also possible that a host key has just been changed.
> 
> 
> I mean, I was also only half-serious.  I should be serious because the
> libssh specification requires us to print some warning like that, but,
> well.  Yes, I’ll stop mumbling about this stuff now.)

To be on the explic side: are you asking to basically put the first 6
lines of the openssh error message (as you quoted them above) as error
message in the ssh driver?
As data point (I know it is not a strong argument), I'll mention that
the current libssh2-based driver does not do that, and (according to my
possibly limited knowledge) there were no problems/complaints about
that.  Sure, improvements are good, I get that, although I do not see
why just changing the underlying implementation of a driver makes an
error message for the same situation immediately no more acceptable.
I'm perfectly fine in improving this in sequent patches, for example --
as I mentioned, the API for this in libssh is sadly not usable, and it
will be usable with the future libssh 0.9.0 (kudos to the libssh guys
for the fast fix).

> 
> [...]
> 
> > diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> > index b3816136f7..774eb5f2a9 100755
> > --- a/tests/qemu-iotests/207
> > +++ b/tests/qemu-iotests/207
> > @@ -111,7 +111,7 @@ with iotests.FilePath('t.img') as disk_path, \
> >      iotests.img_info_log(remote_path)
> >  
> >      md5_key = subprocess.check_output(
> > -        'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
> > +        'ssh-keyscan -t ssh-ed25519 127.0.0.1 2>/dev/null | grep -v "\\^#" 
> > | ' +
> >          'cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1',
> >          shell=True).rstrip().decode('ascii')
> >  
> > @@ -149,7 +149,7 @@ with iotests.FilePath('t.img') as disk_path, \
> >      iotests.img_info_log(remote_path)
> >  
> >      sha1_key = subprocess.check_output(
> > -        'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
> > +        'ssh-keyscan -t ssh-ed25519 127.0.0.1 2>/dev/null | grep -v "\\^#" 
> > | ' +
> >          'cut -d" " -f3 | base64 -d | sha1sum -b | cut -d" " -f1',
> >          shell=True).rstrip().decode('ascii')
> 
> I’ve attached a diff that makes this test pass for me.  Maybe also for
> you and Philippe.

It works for me as well, thanks!
I squashed it locally, and amended the commit message to mention this
with credits for you.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.