[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v10] ssh: switch from libssh2 to libssh
From: |
Pino Toscano |
Subject: |
Re: [Qemu-block] [PATCH v10] ssh: switch from libssh2 to libssh |
Date: |
Thu, 20 Jun 2019 11:49:42 +0200 |
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.