[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh |
Date: |
Thu, 13 Jun 2019 21:31:40 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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 <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> Acked-by: Alex Bennée <address@hidden>
> ---
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