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. Max >>> + } >>> +#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. Max
signature.asc
Description: OpenPGP digital signature