On 14.06.19 16:29, Pino Toscano wrote: > On Thursday, 13 June 2019 21:31:40 CEST Max Reitz wrote: >> On 13.06.19 15:20, Pino Toscano wrote: >>> - 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:-) > > There is the API since libssh 0.8.0... which unfortunately is not > usable, as they forgot to properly export the symbol :-(
Actuall, I just meant adding some wording about that to the error message. >>> 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? > > This switch is for libssh < 0.8.0, so enumerating all the possible > values of the enum of the old API is enough. state is an integer. I feel very uneasy about not having a default clause for a plain integer, especially if it is supplied by an external library. >>> @@ -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? > > It should, although it will not make a difference. connect_to_ssh() is > used in only two places: > - in ssh_file_open, and s->sock is reset to -1 on error > - in ssh_co_create, which uses a local BDRVSSHState I meant: Why don’t you move this to connect_to_ssh()? >>> 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. > > The problem here is slightly more than this: > - libssh2 supports only rsa and dsa keys, so when connecting rsa keys > are usually used, and thus it is easy to pass a fingerprint for that > rsa key > - libssh supports more recent (and stronger) types of keys, which of > course are preferred by more modern (open)ssh servers. Thus it is not > possible to know which key will be used when connecting, unless forced > (which I'd rather not do) > A possible future improvement could be IMHO to add an extra option to > set the allowed key types for the connection, so you can set it to a > specific one and specify the right fingerprint for it. I suppose we can just try all fingerprints we have and then see whether any works. Max
signature.asc
Description: OpenPGP digital signature