On Thursday, 20 June 2019 14:58:40 CEST Max Reitz wrote:
> 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 <ptosc...@redhat.com>
> >>> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> >>> Acked-by: Alex Bennée <alex.ben...@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.
> >
> > 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.Ah, now I see what you mean! I thought that "libssh specification" was any of the SSH RFCs, and that's why I did not find what you meant. I see you were talking about the libssh API documentation :-) (Never heard the API docs of a library called as "specification" before, TBH.) > 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. Makes sense -- I just asked to the libssh people, and appending "this may be a possible attack" should be enough, especially that this is not a UI message like the one written by the ssh client. > >> $ 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. Not a problem. I preferred to ask explicitly to make sure to get it right -- any amount of information shown is fine for me, I want to make sure to follow QEMU best practices (if any). -- Pino Toscano
signature.asc
Description: This is a digitally signed message part.
