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

[Pino Toscano <ptoscano@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5845 bytes --]

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 :-(

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

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

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

-- 
Pino Toscano

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]