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 > > Tested-by: Philippe Mathieu-Daudé > > Acked-by: Alex Bennée > > --- > > > > 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