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

[Max Reitz]

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

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