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 <ptosc...@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> Acked-by: Alex Bennée <alex.ben...@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?
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.
