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

[Philippe Mathieu-Daudé]

On 6/14/19 4:30 PM, Max Reitz wrote:
> 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.

Aha! Good catch :) On v6 review I asked the opposite, no default for
0.0.8 enum, and default for pre-0.8:

https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg02466.html

Pino: can you fix this please?

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

Awesome, thanks!

signature.asc
Description: OpenPGP digital signature