Re: [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9320 bytes --]

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 <ptoscano@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Acked-by: Alex Bennée <alex.bennee@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]