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

[Philippe Mathieu-Daudé]

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 <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?

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.

> 
>> +    }
>> +#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.

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 :(

Regards,

Phil.