From: Max Reitz <mreitz@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: kwolf@redhat.com, fam@euphon.net, qemu-block@nongnu.org,
rjones@redhat.com, sw@weilnetz.de, qemu-devel@nongnu.org,
Pino Toscano <ptoscano@redhat.com>,
alex.bennee@linaro.org, philmd@redhat.com
Subject: Re: [Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh
Date: Thu, 20 Jun 2019 15:01:57 +0200 [thread overview]
Message-ID: <b7fb15db-fb16-b0c6-e099-3d0a2a6dd55b@redhat.com> (raw)
In-Reply-To: <20190620101113.GV25448@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 7486 bytes --]
On 20.06.19 12:11, Daniel P. Berrangé wrote:
> On Tue, Jun 18, 2019 at 03:14:30PM +0200, 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 <ptoscano@redhat.com>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Acked-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>
>>> 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.
>> 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...?
>>
>>
>> (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:
>>
>> $ 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.)
>
> Those giant messages are targetted at humans though so I'm not so
> convinced it is useful for apps managing QEMU. IMHO it is sufficient
> to just report a simple error that the host ident check failed as
> the patch does now. It gives enough info to then investigate further
> to identify the root cause problems.
I fully agree that that message is absolutely unsuitable for qemu. I
didn’t state that, which is my mistake, sorry.
The point I wanted to make is that I’m citing something that doesn’t
print any data, but just an attack warning. I’d be content with just a
single sentence to that effect (and please not in caps lock), like “It
is possible that someone is attempting a man-in-the-middle attack.”
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2019-06-20 13:27 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-18 9:24 [Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh Pino Toscano
2019-06-18 13:14 ` Max Reitz
2019-06-20 9:49 ` Pino Toscano
2019-06-20 12:58 ` Max Reitz
2019-06-20 20:03 ` Pino Toscano
2019-06-20 20:10 ` Max Reitz
2019-06-20 10:11 ` Daniel P. Berrangé
2019-06-20 13:01 ` Max Reitz [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b7fb15db-fb16-b0c6-e099-3d0a2a6dd55b@redhat.com \
--to=mreitz@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=berrange@redhat.com \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=philmd@redhat.com \
--cc=ptoscano@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=rjones@redhat.com \
--cc=sw@weilnetz.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).