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

[Daniel P . Berrangé]

On Tue, Jun 18, 2019 at 03:14:30PM +0200, 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.
> 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...?
> 
> 
> (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:
> 
> $ 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.)

Those giant messages are targetted at humans though so I'm not so
convinced it is useful for apps managing QEMU. IMHO it is sufficient
to just report a simple error that the host ident check failed as
the patch does now. It gives enough info to then investigate further
to identify the root cause problems.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|