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? > + } > +#endif /* !HAVE_LIBSSH_0_8 */ > > /* known_hosts checking successful. */ > ret = 0; > > out: > - if (knh != NULL) { > - libssh2_knownhost_free(knh); > - } > - g_free(knh_file); > return ret; > } [...] > @@ -657,71 +644,147 @@ static int connect_to_ssh(BDRVSSHState *s, > BlockdevOptionsSsh *opts, [...] > + /* > + * Try to disable the Nagle algorithm on TCP sockets to reduce latency, > + * but do not fail if it cannot be disabled. > + */ > + r = socket_set_nodelay(new_sock); > + if (r < 0) { > + warn_report("setting NODELAY for the ssh server %s failed: %m", TIL about %m. > + s->inet->host); > + } > + [...] > @@ -775,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict > *options, int bdrv_flags, > } > > /* Go non-blocking. */ > - libssh2_session_set_blocking(s->session, 0); > + ssh_set_blocking(s->session, 0); > > qapi_free_BlockdevOptionsSsh(opts); > > return 0; > > err: > - if (s->sock >= 0) { > - close(s->sock); > - } > s->sock = -1; Shouldn’t connect_to_ssh() set this to -1 after closing the session? > > qapi_free_BlockdevOptionsSsh(opts); [...] > 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. Max
signature.asc
Description: OpenPGP digital signature