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

[Max Reitz]

On 20.06.19 11:49, Pino Toscano wrote:
> On Tuesday, 18 June 2019 15:14:30 CEST Max Reitz wrote:
>> On 18.06.19 11:24, 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>
>>> ---
>>>
>>> Changes from v9:
>>> - restored "default" case in the server status switch for libssh < 0.8.0
>>> - print the host key type & fingerprint on mismatch with known_hosts
>>> - improve/fix message for failed socket_set_nodelay()
>>> - reset s->sock properly
>>>
>>> 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                                   | 665 ++++++++++--------
>>>  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, 423 insertions(+), 349 deletions(-)
>>
>> [...]
>>
>>> diff --git a/block/ssh.c b/block/ssh.c
>>> index 6da7b9cbfe..644ae8b82c 100644
>>> --- a/block/ssh.c
>>> +++ b/block/ssh.c
>>
>> [...]
>>
>>> +    case SSH_SERVER_KNOWN_CHANGED:
>>> +        ret = -EINVAL;
>>> +        r = ssh_get_publickey(s->session, &pubkey);
>>> +        if (r == 0) {
>>> +            r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA1,
>>> +                                       &server_hash, &server_hash_len);
>>> +            pubkey_type = ssh_key_type(pubkey);
>>> +            ssh_key_free(pubkey);
>>> +        }
>>> +        if (r == 0) {
>>> +            fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
>>> +                                                   server_hash,
>>> +                                                   server_hash_len);
>>> +            ssh_clean_pubkey_hash(&server_hash);
>>> +        }
>>> +        if (fingerprint) {
>>> +            error_setg(errp,
>>> +                       "host key (%s key with fingerprint %s) does not 
>>> match "
>>> +                       "the one in known_hosts",
>>> +                       ssh_key_type_to_char(pubkey_type), fingerprint);
>>> +            ssh_string_free_char(fingerprint);
>>> +        } else  {
>>> +            error_setg(errp, "host key does not match the one in 
>>> known_hosts");
>>> +        }
>>
>> Usually I’d say that more information can’t be bad.  But here I don’t
>> see how this additional information is useful.  known_hosts contains
>> base64-encoded full public keys.  This only prints the SHA-1 digest.
> 
> Note that SHA-1 is printed with libssh < 0.8; with libssh >= 0.8 SHA-256
> is used instead.
> 
>> The user cannot add that to known_hosts, or easily scan known_hosts to
>> see whether it perhaps belongs to some other hosts (maybe because DHCP
>> scrambled something).
>>
>> Actually, even if this printed the base64 representation of the full
>> key, the user probably wouldn’t just add that to known_hosts or do
>> anything with it.  They’ll debug the problem with other tools.
>>
>> So I don’t think this new information is really useful...?
> 
> When this situation happens with openssh, the big scary message prints
> three things:
> 1) the fingerprint of the server
> 2) the line of the server in the known_hosts file
> 3) how to remove the keys of the server from known_hosts, i.e. a
>    copy-paste'able `ssh-keygen -R host`
> 
> Here I'm doing only (1).  Also, the current libssh2 driver does
> something else, i.e. print the base64/printable representation of the
> server key in known_hosts.

Well, I don’t know whether it’s necessary to reproduce the exact message
with all the data it contains.  As I said, I think users can and will
investigate the exact root of the problem with tools outside of qemu.
(Such as openssh’s ssh itself.)

>> (Hmm, I don’t know if this is a response to my “But the specification
>> requires a warning!!1!”, but if so, I was actually not referring to more
>> information but to this:
> 
> You mentioned this few times: can you please point me to this bit?
> I read few RFCs related to ssh, and I did not find this information...

For example:
http://api.libssh.org/master/group__libssh__session.html#gacbc5d04fe66beee863a0c61a93fdf765
> SSH_KNOWN_HOSTS_CHANGED: The server key has changed. Either you are under 
> attack or the administrator changed the key. You HAVE to warn the user about 
> a possible attack.

This doesn’t require any specific formatting or data to be given to the
user.  All it requires is “to warn the user about a possible attack”.
That can be as simple as appending “This may be due to a
man-in-the-middle attack” to the error message.

>> $ ssh 192.168.0.12
>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> @    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
>> Someone could be eavesdropping on you right now (man-in-the-middle attack)!
>> It is also possible that a host key has just been changed.
>>
>>
>> I mean, I was also only half-serious.  I should be serious because the
>> libssh specification requires us to print some warning like that, but,
>> well.  Yes, I’ll stop mumbling about this stuff now.)
> 
> To be on the explic side: are you asking to basically put the first 6
> lines of the openssh error message (as you quoted them above) as error
> message in the ssh driver?

God forbid no.  I was just making an example of “Here is a warning about
a possible attack”.  I fully agree with Dan (and probably you) that this
is completely unsuitable to qemu’s interface.

Sorry if that came across in another way.

> As data point (I know it is not a strong argument), I'll mention that
> the current libssh2-based driver does not do that, and (according to my
> possibly limited knowledge) there were no problems/complaints about
> that.  Sure, improvements are good, I get that, although I do not see
> why just changing the underlying implementation of a driver makes an
> error message for the same situation immediately no more acceptable.
> I'm perfectly fine in improving this in sequent patches, for example --
> as I mentioned, the API for this in libssh is sadly not usable, and it
> will be usable with the future libssh 0.9.0 (kudos to the libssh guys
> for the fast fix).

Again, I’m only being half-serious.  I tried to bring that point across
every time I mention this thing.

If you just say “Let’s leave it as it is, I don’t see any benefit”,
sure, OK, I’ll accept that.

But so far you didn’t say that, so I kept pestering on.


I would have never mentioned this topic if I hadn’t read the
specification while reviewing your patch last year and noticed that it
explicitly states “You HAVE to warn the user about a possible attack.”

I never said “Please emit the exact message openssh’s ssh” emits.  Well,
I never meant to say that.  Maybe I accidentally did.  Sorry again.

My whole point is: The specification requires libssh users to emit
*some* warning.  If we want to adhere to the specification, we have to
do that.  Just one sentence is sufficient.

If we don’t want to adhere to the specification, sure, let’s go.  But
you’ll have to explicitly say that.  I’m not the patch author, I’m but a
humble reviewer, and as such I’ll point out all flaws I see.  You have
to give justifications, not me. O:-)

>> [...]
>>
>>> 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
>>> @@ -111,7 +111,7 @@ with iotests.FilePath('t.img') as disk_path, \
>>>      iotests.img_info_log(remote_path)
>>>  
>>>      md5_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 | md5sum -b | cut -d" " -f1',
>>>          shell=True).rstrip().decode('ascii')
>>>  
>>> @@ -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')
>>
>> I’ve attached a diff that makes this test pass for me.  Maybe also for
>> you and Philippe.
> 
> It works for me as well, thanks!

Great!

Max

signature.asc
Description: OpenPGP digital signature