Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 6/14/19 4:30 PM, Max Reitz wrote:
> On 14.06.19 16:26, Philippe Mathieu-Daudé wrote:
>> On 6/13/19 9:31 PM, Max Reitz wrote:
>>> On 13.06.19 15:20, 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
Tested-by: Philippe Mathieu-Daudé
Acked-by: Alex Bennée
---
>>>
>>> Can confirm that this runs much faster than the last version I tested.
>>> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
>>> with me. :-)
>>>
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 | 622 +-
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, 376 insertions(+), 353 deletions(-)
>>>
>>> Surprisingly little has changed since v4. Good, good, that makes my
>>> reviewing job much easier because I can thus rely on past me. :-)
>>>
diff --git a/block/ssh.c b/block/ssh.c
index 6da7b9cbfe..fb458d4548 100644
--- a/block/ssh.c
+++ b/block/ssh.c
>>>
>>> [...]
>>>
@@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename,
QDict *options,
parse_uri(filename, options, errp);
}
-static int check_host_key_knownhosts(BDRVSSHState *s,
- const char *host, int port, Error
**errp)
+static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
{
>>>
>>> [...]
>>>
-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);
+
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 6/14/19 4:34 PM, Max Reitz wrote:
> 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:
[...]
-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.
Agreed. What's odd is I tested it on Ubuntu Xenial which is 0.6.3 and no
got no cpp warning. I wonder if it is using a backported patch adding
ssh_session_is_known_server(), like 0.7.1 on Ubuntu Bionic. Anyway,
better add a default.
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 6/13/19 9:31 PM, Max Reitz wrote:
> On 13.06.19 15:20, 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
>> Tested-by: Philippe Mathieu-Daudé
>> Acked-by: Alex Bennée
>> ---
>
> Can confirm that this runs much faster than the last version I tested.
> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
> with me. :-)
>
>> 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 | 622 +-
>> 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, 376 insertions(+), 353 deletions(-)
>
> Surprisingly little has changed since v4. Good, good, that makes my
> reviewing job much easier because I can thus rely on past me. :-)
>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 6da7b9cbfe..fb458d4548 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>
> [...]
>
>> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename,
>> QDict *options,
>> parse_uri(filename, options, errp);
>> }
>>
>> -static int check_host_key_knownhosts(BDRVSSHState *s,
>> - const char *host, int port, Error
>> **errp)
>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>> {
>
> [...]
>
>> -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:-)
>
>> goto out;
>> -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
>> +case SSH_KNOWN_HOSTS_OTHER:
>> ret = -EINVAL;
>> -
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
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
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
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
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 14.06.19 16:26, Philippe Mathieu-Daudé wrote:
> On 6/13/19 9:31 PM, Max Reitz wrote:
>> On 13.06.19 15:20, 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
>>> Tested-by: Philippe Mathieu-Daudé
>>> Acked-by: Alex Bennée
>>> ---
>>
>> Can confirm that this runs much faster than the last version I tested.
>> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
>> with me. :-)
>>
>>> 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 | 622 +-
>>> 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, 376 insertions(+), 353 deletions(-)
>>
>> Surprisingly little has changed since v4. Good, good, that makes my
>> reviewing job much easier because I can thus rely on past me. :-)
>>
>>> diff --git a/block/ssh.c b/block/ssh.c
>>> index 6da7b9cbfe..fb458d4548 100644
>>> --- a/block/ssh.c
>>> +++ b/block/ssh.c
>>
>> [...]
>>
>>> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename,
>>> QDict *options,
>>> parse_uri(filename, options, errp);
>>> }
>>>
>>> -static int check_host_key_knownhosts(BDRVSSHState *s,
>>> - const char *host, int port, Error
>>> **errp)
>>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>>> {
>>
>> [...]
>>
>>> -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
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 6/13/19 2:31 PM, Max Reitz wrote:
>> @@ -657,71 +644,147 @@ static int connect_to_ssh(BDRVSSHState *s,
>> BlockdevOptionsSsh *opts,
>
> [...]
>
>> +/*
>> + * Try to disable the Nagle algorithm on TCP sockets to reduce latency,
>> + * but do not fail if it cannot be disabled.
>> + */
>> +r = socket_set_nodelay(new_sock);
>> +if (r < 0) {
>> +warn_report("setting NODELAY for the ssh server %s failed: %m",
>
> TIL about %m.
printf("%m") is not portable to non-glibc. Spell it out as
"%s",strerror(errno). (It _is_ portable for syslog(), but warn_report()
is not wired to syslog() semantics).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 13.06.19 15:20, 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
> Tested-by: Philippe Mathieu-Daudé
> Acked-by: Alex Bennée
> ---
Can confirm that this runs much faster than the last version I tested.
197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
with me. :-)
> 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 | 622 +-
> 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, 376 insertions(+), 353 deletions(-)
Surprisingly little has changed since v4. Good, good, that makes my
reviewing job much easier because I can thus rely on past me. :-)
> diff --git a/block/ssh.c b/block/ssh.c
> index 6da7b9cbfe..fb458d4548 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
[...]
> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename,
> QDict *options,
> parse_uri(filename, options, errp);
> }
>
> -static int check_host_key_knownhosts(BDRVSSHState *s,
> - const char *host, int port, Error
> **errp)
> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
> {
[...]
> -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:-)
> 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");
>
Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
On 6/13/19 3:20 PM, 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 > Tested-by: Philippe Mathieu-Daudé > Acked-by: Alex Bennée > --- > > 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 | 622 +- > 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, 376 insertions(+), 353 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index 08502c0aa2..75f47df3d2 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -31,7 +31,7 @@ addons: >- libseccomp-dev >- libspice-protocol-dev >- libspice-server-dev > - - libssh2-1-dev > + - libssh-dev >- liburcu-dev >- libusb-1.0-0-dev >- libvte-2.91-dev > @@ -268,7 +268,7 @@ matrix: > - libseccomp-dev > - libspice-protocol-dev > - libspice-server-dev > -- libssh2-1-dev > +- libssh-dev > - liburcu-dev > - libusb-1.0-0-dev > - libvte-2.91-dev > diff --git a/block/Makefile.objs b/block/Makefile.objs > index ae11605c9f..bf01429dd5 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o > block-obj-$(CONFIG_RBD) += rbd.o > block-obj-$(CONFIG_GLUSTERFS) += gluster.o > block-obj-$(CONFIG_VXHS) += vxhs.o > -block-obj-$(CONFIG_LIBSSH2) += ssh.o > +block-obj-$(CONFIG_LIBSSH) += ssh.o > block-obj-y += accounting.o dirty-bitmap.o > block-obj-y += write-threshold.o > block-obj-y += backup.o > @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS) > gluster.o-cflags := $(GLUSTERFS_CFLAGS) > gluster.o-libs := $(GLUSTERFS_LIBS) > vxhs.o-libs:= $(VXHS_LIBS) > -ssh.o-cflags := $(LIBSSH2_CFLAGS) > -ssh.o-libs := $(LIBSSH2_LIBS) > +ssh.o-cflags := $(LIBSSH_CFLAGS) > +ssh.o-libs := $(LIBSSH_LIBS) > block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o > block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y) > dmg-bz2.o-libs := $(BZIP2_LIBS) > diff --git a/block/ssh.c b/block/ssh.c > index 6da7b9cbfe..fb458d4548 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -24,8 +24,8 @@ > > #include "qemu/osdep.h" > >
[Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh
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 Tested-by: Philippe Mathieu-Daudé Acked-by: Alex Bennée --- 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 | 622 +- 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, 376 insertions(+), 353 deletions(-) diff --git a/.travis.yml b/.travis.yml index 08502c0aa2..75f47df3d2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -31,7 +31,7 @@ addons: - libseccomp-dev - libspice-protocol-dev - libspice-server-dev - - libssh2-1-dev + - libssh-dev - liburcu-dev - libusb-1.0-0-dev - libvte-2.91-dev @@ -268,7 +268,7 @@ matrix: - libseccomp-dev - libspice-protocol-dev - libspice-server-dev -- libssh2-1-dev +- libssh-dev - liburcu-dev - libusb-1.0-0-dev - libvte-2.91-dev diff --git a/block/Makefile.objs b/block/Makefile.objs index ae11605c9f..bf01429dd5 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o block-obj-$(CONFIG_RBD) += rbd.o block-obj-$(CONFIG_GLUSTERFS) += gluster.o block-obj-$(CONFIG_VXHS) += vxhs.o -block-obj-$(CONFIG_LIBSSH2) += ssh.o +block-obj-$(CONFIG_LIBSSH) += ssh.o block-obj-y += accounting.o dirty-bitmap.o block-obj-y += write-threshold.o block-obj-y += backup.o @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS) gluster.o-cflags := $(GLUSTERFS_CFLAGS) gluster.o-libs := $(GLUSTERFS_LIBS) vxhs.o-libs:= $(VXHS_LIBS) -ssh.o-cflags := $(LIBSSH2_CFLAGS) -ssh.o-libs := $(LIBSSH2_LIBS) +ssh.o-cflags := $(LIBSSH_CFLAGS) +ssh.o-libs := $(LIBSSH_LIBS) block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y) dmg-bz2.o-libs := $(BZIP2_LIBS) diff --git a/block/ssh.c b/block/ssh.c index 6da7b9cbfe..fb458d4548 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" -#include -#include +#include +#include #include "block/block_int.h" #include "block/qdict.h" @@ -46,13 +46,11 @@ #include "trace.h" /* - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note - * that this requires that libssh2 was specially compiled with the - * `./configure --enable-debug' option,
