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

[Max Reitz]

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