On 6/14/19 4:30 PM, Max Reitz wrote: > On 14.06.19 16:26, Philippe Mathieu-Daudé wrote: >> On 6/13/19 9:31 PM, Max Reitz wrote: >>> On 13.06.19 15:20, 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 <ptosc...@redhat.com> >>>> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> Acked-by: Alex Bennée <alex.ben...@linaro.org> >>>> --- >>> >>> Can confirm that this runs much faster than the last version I tested. >>> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine >>> with me. :-) >>> >>>> 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 | 622 +++++++++--------- >>>> 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, 376 insertions(+), 353 deletions(-) >>> >>> Surprisingly little has changed since v4. Good, good, that makes my >>> reviewing job much easier because I can thus rely on past me. :-) >>> >>>> diff --git a/block/ssh.c b/block/ssh.c >>>> index 6da7b9cbfe..fb458d4548 100644 >>>> --- a/block/ssh.c >>>> +++ b/block/ssh.c >>> >>> [...] >>> >>>> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename, >>>> QDict *options, >>>> parse_uri(filename, options, errp); >>>> } >>>> >>>> -static int check_host_key_knownhosts(BDRVSSHState *s, >>>> - const char *host, int port, Error >>>> **errp) >>>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp) >>>> { >>> >>> [...] >>> >>>> - switch (r) { >>>> - case LIBSSH2_KNOWNHOST_CHECK_MATCH: >>>> + switch (state) { >>>> + case SSH_KNOWN_HOSTS_OK: >>>> /* OK */ >>>> - trace_ssh_check_host_key_knownhosts(found->key); >>>> + trace_ssh_check_host_key_knownhosts(); >>>> break; >>>> - case LIBSSH2_KNOWNHOST_CHECK_MISMATCH: >>>> + case SSH_KNOWN_HOSTS_CHANGED: >>>> ret = -EINVAL; >>>> - session_error_setg(errp, s, >>>> - "host key does not match the one in known_hosts" >>>> - " (found key %s)", found->key); >>>> + error_setg(errp, "host key does not match the one in >>>> known_hosts"); >>> >>> So what about the possible attack warning that the specification >>> technically requires us to print? O:-) >>> >>>> goto out; >>>> - case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND: >>>> + case SSH_KNOWN_HOSTS_OTHER: >>>> ret = -EINVAL; >>>> - session_error_setg(errp, s, "no host key was found in >>>> known_hosts"); >>>> + error_setg(errp, >>>> + "host key for this server not found, another type >>>> exists"); >>>> goto out; >>>> - case LIBSSH2_KNOWNHOST_CHECK_FAILURE: >>>> + case SSH_KNOWN_HOSTS_UNKNOWN: >>>> ret = -EINVAL; >>>> - session_error_setg(errp, s, >>>> - "failure matching the host key with known_hosts"); >>>> + error_setg(errp, "no host key was found in known_hosts"); >>>> + goto out; >>>> + case SSH_KNOWN_HOSTS_NOT_FOUND: >>>> + ret = -ENOENT; >>>> + error_setg(errp, "known_hosts file not found"); >>>> + goto out; >>>> + case SSH_KNOWN_HOSTS_ERROR: >>>> + ret = -EINVAL; >>>> + error_setg(errp, "error while checking the host"); >>>> goto out; >>>> default: >>>> ret = -EINVAL; >>>> - session_error_setg(errp, s, "unknown error matching the host key" >>>> - " with known_hosts (%d)", r); >>>> + error_setg(errp, "error while checking for known server"); >>>> goto out; >>>> } >>>> +#else /* !HAVE_LIBSSH_0_8 */ >>>> + int state; >>>> + >>>> + state = ssh_is_server_known(s->session); >>>> + trace_ssh_server_status(state); >>>> + >>>> + switch (state) { >>>> + case SSH_SERVER_KNOWN_OK: >>>> + /* OK */ >>>> + trace_ssh_check_host_key_knownhosts(); >>>> + break; >>>> + case SSH_SERVER_KNOWN_CHANGED: >>>> + ret = -EINVAL; >>>> + error_setg(errp, "host key does not match the one in >>>> known_hosts"); >>>> + goto out; >>>> + case SSH_SERVER_FOUND_OTHER: >>>> + ret = -EINVAL; >>>> + error_setg(errp, >>>> + "host key for this server not found, another type >>>> exists"); >>>> + goto out; >>>> + case SSH_SERVER_FILE_NOT_FOUND: >>>> + ret = -ENOENT; >>>> + error_setg(errp, "known_hosts file not found"); >>>> + goto out; >>>> + case SSH_SERVER_NOT_KNOWN: >>>> + ret = -EINVAL; >>>> + error_setg(errp, "no host key was found in known_hosts"); >>>> + goto out; >>>> + case SSH_SERVER_ERROR: >>>> + ret = -EINVAL; >>>> + error_setg(errp, "server error"); >>>> + goto out; >>> >>> No default here? >> >> I explicitly suggested Pino to not use default here, since >> ssh_server_known_e is a finite enum. If upstream libssh adds more >> errors, and a distrib packages a new version, we'll get a build error. >> It looks quicker for us to react than adding a abort() here and wait an >> user to complain. But this is a personal preference, I won't object if >> you prefer we use a default here. > > Well, OK, but this is the pre-0.8 code path, "state" is an integer here.
Aha! Good catch :) On v6 review I asked the opposite, no default for 0.0.8 enum, and default for pre-0.8: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg02466.html Pino: can you fix this please? >>>> + } >>>> +#endif /* !HAVE_LIBSSH_0_8 */ > > [...] > >>>> 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 >>> >>> [...] >>> >>>> @@ -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') >>> >>> Hm. This is a pain. >>> >>> I suppose the best would be to drop the "-t" altogether and then check >>> whether any of the entries ssh-keyscan reports matches. >> >> This was my first approach, but then the 207.out file doesn't match. >> I don't know enough of iotests fu to help here :( > > I’ll see what I can do. Awesome, thanks!
signature.asc
Description: OpenPGP digital signature