Misc fixes to NBD
cover Misc fixes to NBD
This does two minor fixes to the NBD code and adds significant coverage of the NBD TLS support to detect future problems.
The first two patches should be for 3.1.
The tests can wait till 4.0 if desired.
Daniel P. Berrangé (6):
nbd: fix whitespace in server error message
nbd: stop waiting for a NBD response with NBD_CMD_DISC
tests: pull qemu-nbd iotest helpers into common.nbd file
tests: check if qemu-nbd is still alive before waiting
tests: add iotests helpers for dealing with TLS certificates
tests: exercise NBD server in TLS mode
block/nbd-client.c | 1 + nbd/server.c | 2 +- tests/qemu-iotests/058 | 47 ++---------- tests/qemu-iotests/233 | 99 ++++++++++++++++++++++++ tests/qemu-iotests/233.out | 33 ++++++++ tests/qemu-iotests/common.nbd | 111 +++++++++++++++++++++++++++ tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 8 files changed, 393 insertions(+), 40 deletions(-) create mode 100755 tests/qemu-iotests/233 create mode 100644 tests/qemu-iotests/233.out create mode 100644 tests/qemu-iotests/common.nbd create mode 100644 tests/qemu-iotests/common.tls
Signature · 1 line
2.19.1
PATCH 1/6 nbd: fix whitespace in server error message
A space was missing after the option number was printed:
Option 0x8not permitted before TLS
becomes
Option 0x8 not permitted before TLS
This fixes
commit 3668328303429f3bc93ab3365c66331600b06a2d
Author: Eric Blake <[email protected]>
Date: Fri Oct 14 13:33:09 2016 -0500
nbd: Send message along with server NBD_REP_ERR errors
--- nbd/server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
nbd/server.c +1 -1
@@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,default : ret =nbd_opt_drop (client,NBD_REP_ERR_TLS_REQD , errp, -"Option 0x%" PRIx32 +"Option 0x%" PRIx32" " "not permitted before TLS" , option);/* Let the client keep trying, unless they asked to * quit. In this mode, we've already sent an error, so
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message
Quoted 26 lines · Daniel P. Berrangé
A space was missing after the option number was printed:
Option 0x8not permitted before TLSbecomes
Option
0x8not permitted before TLSThis fixes
commit
3668328303429f3bc93ab3365c66331600b06a2d
Author: Eric Blake <[email protected]>
Date: Fri Oct 14 13:33:09 2016 -0500nbd: Send message along with server
NBD_REP_ERRerrorsSigned-off-by Daniel P. Berrangé <[email protected]>--- nbd/server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)nbd/server.c +0
-0
nbd/server.c static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, · line 1139
Quoted 6 lines
nbd/server.c +1
-1@@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,default : ret =nbd_opt_drop (client,NBD_REP_ERR_TLS_REQD , errp, -"Option 0x%" PRIx32 +"Option 0x%" PRIx32" " "not permitted before TLS" , option);
Visually, I'd include the space in the next line instead of on its own (but that's aesthetics, not semantic). Agree that this is 3.1 material; I'll queue it up and send a PR on Monday.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Philippe Mathieu-Daudé
Reviewed-byRe: [Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message
Quoted 22 lines · Eric Blake
Quoted 21 lines · Daniel P. Berrangé
A space was missing after the option number was printed:
Option 0x8not permitted before TLS
becomes
Option
0x8not permitted before TLSThis fixes
commit
3668328303429f3bc93ab3365c66331600b06a2d
Author: Eric Blake <[email protected]> Date: Fri Oct 14 13:33:09 2016 -0500nbd: Send message along with server
NBD_REP_ERRerrorsSigned-off-by Daniel P. Berrangé <[email protected]>---
nbd/server.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 13 lines
Quoted 11 lines
nbd/server.c +0
-0@@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,default:
ret = nbd_opt_drop(client,NBD_REP_ERR_TLS_REQD, errp, - "Option 0x%" PRIx32
+ "Option 0x%" PRIx32 " " "not permitted before TLS", option);Visually, I'd include the space in the next line instead of on its own
Agreed :)
Quoted 4 lines
(but that's aesthetics, not semantic). Agree that this is 3.1 material; I'll queue it up and send a PR on Monday.
Reviewed-by Eric Blake <[email protected]>
PATCH 2/6 nbd: stop waiting for a NBD response with NBD_CMD_DISC
In reply to Daniel P. BerrangéWhen sending a NBD_CMD_DISC message there is no reply expected, however, the nbd_read_eof() coroutine is still waiting for a reply. In a plain NBD connection this doesn't matter as it will just get an EOF, however, on a TLS connection it will get an interrupted TLS data packet. The nbd_read_eof() will then print an error message on the console due to missing reply to NBD_CMD_DISC.
This can be seen with qemu-img
$ qemu-img info \
--object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
--image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
qemu-img: Cannot read from TLS channel: Input/output error
image: nbd://127.0.0.1:9000
file format: nbd
virtual size: 10M (10485760 bytes)
disk size: unavailableSimply setting the 'quit' flag after sending NBD_CMD_DISC is enough to get the coroutine to stop waiting for a reply and thus supress the error message. This fixes a regression introduced in
commit be41c100c0d67f6072ddd4910c4b6f7d239f025f
Author: Vladimir Sementsov-Ogievskiy <[email protected]>
Date: Fri May 26 14:09:13 2017 +0300
nbd/client.c: use errp instead of LOG
Move to modern errp scheme from just LOGging errors.
Message-Id: <[email protected]>
--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)
block/nbd-client.c +1 -0
@@ -961,6 +961,7 @@ void nbd_client_close(BlockDriverState *bs) }nbd_send_request (client->ioc, &request); + client->quit = true;nbd_teardown_connection (bs); }
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC
Quoted 5 lines · Daniel P. Berrangé
When sending a
NBD_CMD_DISCmessage there is no reply expected, however, thenbd_read_eof()coroutine is still waiting for a reply. In a plain NBD connection this doesn't matter as it will just get an EOF, however, on a TLS connection it will get an interrupted TLS data packet.
Does that depend on whether the server tried to gracefully close the TLS connection (and flushed enough packets to do so)? At any rate, while it's nice to wait for the server to cleanly end TLS, we can't control the behavior of all servers, and I agree with your analysis that having the server silently deal with an abrupt shutdown is nicer than polluting stderr, as it's not actually a data loss situation for either the client or the server.
Quoted 14 lines
The
nbd_read_eof()will then print an error message on the console due to missing reply toNBD_CMD_DISC.This can be seen with qemu-img
$ qemu-img info \ --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \ --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0 qemu-img: Cannot read from TLS channel: Input/output error image: nbd://127.0.0.1:9000 file format: nbd virtual size: 10M (10485760 bytes) disk size: unavailable
Quoted 2 lines
Simply setting the 'quit' flag after sending
NBD_CMD_DISCis enough to get the coroutine to stop waiting for a reply and thus supress the error
s/supress/suppress/
Quoted 10 lines
message. This fixes a regression introduced in
commit
be41c100c0d67f6072ddd4910c4b6f7d239f025f
Author: Vladimir Sementsov-Ogievskiy <[email protected]>
Date: Fri May 26 14:09:13 2017 +0300
nbd/client.c: use errp instead of LOGMove to modern errp scheme from just LOGging errors.
Quoted 8 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>Message-Id: <[email protected]>
Signed-off-by Paolo Bonzini <[email protected]>Signed-off-by Daniel P. Berrangé <[email protected]>--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)
Agree that this is 3.1 material; will queue.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC
In reply to Daniel P. BerrangéQuoted 18 lines · Daniel P. Berrangé
When sending a
NBD_CMD_DISCmessage there is no reply expected, however, thenbd_read_eof()coroutine is still waiting for a reply. In a plain NBD connection this doesn't matter as it will just get an EOF, however, on a TLS connection it will get an interrupted TLS data packet. Thenbd_read_eof()will then print an error message on the console due to missing reply toNBD_CMD_DISC.This can be seen with qemu-img
$ qemu-img info \ --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \ --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0 qemu-img: Cannot read from TLS channel: Input/output error image: nbd://127.0.0.1:9000 file format: nbd virtual size: 10M (10485760 bytes) disk size: unavailable
Quoted 3 lines
Simply setting the 'quit' flag after sending
NBD_CMD_DISCis enough to get the coroutine to stop waiting for a reply and thus supress the error message.
Actually, it's not quite enough - once you actually start performing I/O, enough coroutines are kicked off that the error still happens:
$ qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \ --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\ driver=nbd,host=localhost,port=10809,tls-creds=tls0 read 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec) wrote 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec) Cannot read from TLS channel: Input/output error
Squashing this in on top of your patch helps, though:
block/nbd-client.c +8 -1
@@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)assert (s->reply.handle ==0 ); ret =nbd_receive_reply (s->ioc, &s->reply, &local_err);if (local_err) { -error_report_err (local_err); +/* If we are already quitting, either another error has +* already been reported, or we requested NBD_CMD_DISC and +* don't need to report anything further. */ +if (!s->quit) { +error_report_err (local_err); + }else { +error_free (local_err); + } }if (ret <=0 ) {break ;
But I want to do more testing to make sure I'm not missing out on reporting an actual error if I add that.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC
Quoted 19 lines · Eric Blake
Quoted 18 lines · Daniel P. Berrangé
When sending a
NBD_CMD_DISCmessage there is no reply expected, however, thenbd_read_eof()coroutine is still waiting for a reply. In a plain NBD connection this doesn't matter as it will just get an EOF, however, on a TLS connection it will get an interrupted TLS data packet. Thenbd_read_eof()will then print an error message on the console due to missing reply toNBD_CMD_DISC.This can be seen with qemu-img
$ qemu-img info \ --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \ --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0 qemu-img: Cannot read from TLS channel: Input/output error image: nbd://127.0.0.1:9000 file format: nbd virtual size: 10M (10485760 bytes) disk size: unavailable
Quoted 6 lines
Quoted 3 lines
Simply setting the 'quit' flag after sending
NBD_CMD_DISCis enough to get the coroutine to stop waiting for a reply and thus supress the error message.Actually, it's not quite enough - once you actually start performing I/O, enough coroutines are kicked off that the error still happens:
Hmm, does the client not send requests synchronously ? I expected that any other I/O operations would have already received their reply by the time we sent the DISC command.
Quoted 34 lines
$ qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \ --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\ driver=nbd,host=localhost,port=10809,tls-creds=tls0 read 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec) wrote 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec) Cannot read from TLS channel: Input/output errorSquashing this in on top of your patch helps, though:
block/nbd-client.c +8
-1@@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)assert (s->reply.handle ==0 ); ret =nbd_receive_reply (s->ioc, &s->reply, &local_err);if (local_err) { -error_report_err (local_err); +/* If we are already quitting, either another error has +* already been reported, or we requested NBD_CMD_DISC and +* don't need to report anything further. */ +if (!s->quit) { +error_report_err (local_err); + }else { +error_free (local_err); + } }if (ret <=0 ) {break;
Quoted 2 lines
But I want to do more testing to make sure I'm not missing out on reporting an actual error if I add that.
Yes, I'd be a little concerned about missing reporting of an error from a command other than NBD_CMD_DISC if we did this.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC
Quoted 11 lines · Daniel P. Berrangé
Quoted 6 lines
Quoted 3 lines
Simply setting the 'quit' flag after sending
NBD_CMD_DISCis enough to get the coroutine to stop waiting for a reply and thus supress the error message.Actually, it's not quite enough - once you actually start performing I/O, enough coroutines are kicked off that the error still happens:
Hmm, does the client not send requests synchronously ? I expected that any other I/O operations would have already received their reply by the time we sent the DISC command.
During handshake phase (NBD_OPT_* messages), client sends requests synchronously (at most one pending read, in response to the most recent write). During transmission phase (NBD_CMD_* messages), the client is allowed to send messages asynchronously. However, the NBD protocol recommends (but not requires) that the client never send NBD_CMD_DISC to disconnect until AFTER the client has collected all responses to any earlier in-flight messages. And if I'm reading the block/nbd-client.c code correctly, in general we don't try to shutdown a client while there are any pending commands with unread replies, except when we are shutting down due to detecting a protocol error (at which point we are already out of sync with the server and waiting for replies is not going to be helpful).
Quoted 5 lines
Quoted 2 lines
But I want to do more testing to make sure I'm not missing out on reporting an actual error if I add that.
Yes, I'd be a little concerned about missing reporting of an error from a command other than
NBD_CMD_DISCif we did this.
But it looks like your approach of making the qio code behave more nicely is a smarter cleanup than my idea, so I'm willing to ditch my code in favor of yours. Testing your approach now :)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC
In reply to Eric BlakeQuoted 59 lines · Eric Blake
Quoted 18 lines · Daniel P. Berrangé
When sending a
NBD_CMD_DISCmessage there is no reply expected, however, thenbd_read_eof()coroutine is still waiting for a reply. In a plain NBD connection this doesn't matter as it will just get an EOF, however, on a TLS connection it will get an interrupted TLS data packet. Thenbd_read_eof()will then print an error message on the console due to missing reply toNBD_CMD_DISC.This can be seen with qemu-img
$ qemu-img info \ --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \ --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0 qemu-img: Cannot read from TLS channel: Input/output error image: nbd://127.0.0.1:9000 file format: nbd virtual size: 10M (10485760 bytes) disk size: unavailableQuoted 3 lines
Simply setting the 'quit' flag after sending
NBD_CMD_DISCis enough to get the coroutine to stop waiting for a reply and thus supress the error message.Actually, it's not quite enough - once you actually start performing I/O, enough coroutines are kicked off that the error still happens:
$ qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \ --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\ driver=nbd,host=localhost,port=10809,tls-creds=tls0 read 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec) wrote 1048576/1048576 bytes at offset 1048576 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec) Cannot read from TLS channel: Input/output errorSquashing this in on top of your patch helps, though:
block/nbd-client.c +8
-1@@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)assert (s->reply.handle ==0 ); ret =nbd_receive_reply (s->ioc, &s->reply, &local_err);if (local_err) { -error_report_err (local_err); +/* If we are already quitting, either another error has +* already been reported, or we requested NBD_CMD_DISC and +* don't need to report anything further. */ +if (!s->quit) { +error_report_err (local_err); + }else { +error_free (local_err); + } }if (ret <=0 ) {break;
Quoted 2 lines
But I want to do more testing to make sure I'm not missing out on reporting an actual error if I add that.
It occurred to me that it would be nice if the TLS channel behaved the same a normal channel when seeing EOF on the socket. Unfortunately the reason why GNUTLS returns an error in this case does in fact make sense, so it isn't quite as simple as just ignoring the error. Fortunately the NBD client has decided it wants to shutdown and crucially has called qio_channel_shutdown() at this point. Thus we can safely ignore the gnutls error code when the channel has been shutdown for read and just return 0 for EOF as with a plain socket.
Thus I've sent a patch which solves the problem in that way:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03616.html
With this applied, both my original patch and your extra chunk here should be redundant.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 3/6 tests: pull qemu-nbd iotest helpers into common.nbd file
In reply to Daniel P. BerrangéThe helpers for starting/stopping qemu-nbd in 058 will be useful in other test cases, so move them into a common.nbd file.
--- tests/qemu-iotests/058 | 47 +++++------------------------ tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 39 deletions(-) create mode 100644 tests/qemu-iotests/common.nbd
tests/qemu-iotests/058 +8 -39
@@ -29,55 +29,19 @@ echo "QA output created by $seq" here=`pwd` status=1 # failure is the default! -nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket -nbd_snapshot_img="nbd:unix:$nbd_unix_socket" -rm -f "${TEST_DIR}/qemu-nbd.pid" - -_cleanup_nbd() -{ - local NBD_SNAPSHOT_PID - if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then - read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" - rm -f "${TEST_DIR}/qemu-nbd.pid" - if [ -n "$NBD_SNAPSHOT_PID" ]; then - kill "$NBD_SNAPSHOT_PID" - fi - fi - rm -f "$nbd_unix_socket" -} - -_wait_for_nbd() -{ - for ((i = 0; i < 300; i++)) - do - if [ -r "$nbd_unix_socket" ]; then - return - fi - sleep 0.1 - done - echo "Failed in check of unix socket created by qemu-nbd" - exit 1 -} - -converted_image=$TEST_IMG.converted - _export_nbd_snapshot() { - _cleanup_nbd - $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 & - _wait_for_nbd + nbd_server_start_unix_socket "$TEST_IMG" -l $1 } _export_nbd_snapshot1() { - _cleanup_nbd - $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 & - _wait_for_nbd + nbd_server_start_unix_socket "$TEST_IMG" -l snapshot.name=$1 } _cleanup() { - _cleanup_nbd + nbd_server_stop _cleanup_test_img rm -f "$converted_image" }
@@ -87,6 +51,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter . ./common.pattern +. ./common.nbd _supported_fmt qcow2 _supported_proto file
@@ -95,6 +60,10 @@ _require_command QEMU_NBD # Internal snapshots are (currently) impossible with refcount_bits=1 _unsupported_imgopts 'refcount_bits=1[^0-9]' +nbd_snapshot_img="nbd:unix:$nbd_unix_socket" + +converted_image=$TEST_IMG.converted + # Use -f raw instead of -f $IMGFMT for the NBD connection QEMU_IO_NBD="$QEMU_IO -f raw --cache=$CACHEMODE"
tests/qemu-iotests/common.nbd new file +56 -0
56 lines not on this page · raw message
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
Quoted 2 lines · Daniel P. Berrangé
The helpers for starting/stopping qemu-nbd in 058 will be useful in other test cases, so move them into a common.nbd file.
In fact, I already have a proposed patch 228 that I'm trying to clean up for potential inclusion in 3.1 that could use this! [I'm still hammering on those patches, so I don't know if they will make 3.1 or be delayed to 4.0 - this patch is not needed in 3.1 unless either your later patches are 3.1 material or my patch ends up ready in time]
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg00308.html
Quoted 8 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/058 | 47 +++++------------------------ tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 39 deletions(-)create mode 100644 tests/qemu-iotests/common.nbd
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
In reply to Daniel P. BerrangéQuoted 20 lines · Daniel P. Berrangé
The helpers for starting/stopping qemu-nbd in 058 will be useful in other test cases, so move them into a common.nbd file.
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/058 | 47 +++++------------------------ tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 39 deletions(-)create mode 100644 tests/qemu-iotests/common.nbd
tests/qemu-iotests/058 +1
-5- -_cleanup_nbd() -{ -_wait_for_nbd() -{ +++ b/tests/qemu-iotests/common.nbd
tests/qemu-iotests/058 line 1
Quoted 1 line
tests/qemu-iotests/058 +1
-0@@ -0,0 +1,56 @@ +#!/bin/bash
I know we're using bash,
tests/qemu-iotests/common.nbd line 38
Quoted 5 lines
tests/qemu-iotests/common.nbd new file +4
-0+ +function nbd_server_stop() +{ +function nbd_server_wait_for_unix_socket()
and bash supports 'function', but it is an obsolete syntactic sugar thing that I don't recommend using. (In ksh, it actually makes a difference in behavior whether you use 'function' or not, and using it in 'bash' makes it harder to port code over to 'ksh' - and hence in bash it is obsolete because here it does NOT cause the change in behavior that ksh users expect)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
Quoted 4 lines · Eric Blake
Quoted 1 line
+#!/bin/bash
I know we're using bash,
Quoted 8 lines
tests/qemu-iotests/common.nbd line 38
Quoted 5 lines
tests/qemu-iotests/common.nbd new file +4
-0+ +function nbd_server_stop() +{ +function nbd_server_wait_for_unix_socket()and bash supports 'function', but it is an obsolete syntactic sugar thing that I don't recommend using. (In ksh, it actually makes a difference in behavior whether you use 'function' or not, and using it in 'bash' makes it harder to port code over to 'ksh' - and hence in bash it is obsolete because here it does NOT cause the change in behavior that ksh users expect)
Of course, I hit send too soon, before getting to my punchline:
Since we already have so many existing iotests that use 'function', it's better to clean that up as a separate patch.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
Quoted 20 lines · Eric Blake
Quoted 8 lines · Eric Blake
Quoted 1 line
+#!/bin/bash
I know we're using bash,
Quoted 3 lines
tests/qemu-iotests/common.nbd new file +3
-0+ +function nbd_server_stop() +{Quoted 9 lines
Quoted 1 line
+function
nbd_server_wait_for_unix_socket()and bash supports 'function', but it is an obsolete syntactic sugar thing that I don't recommend using. (In ksh, it actually makes a difference in behavior whether you use 'function' or not, and using it in 'bash' makes it harder to port code over to 'ksh' - and hence in bash it is obsolete because here it does NOT cause the change in behavior that ksh users expect)
Quoted 4 lines
Of course, I hit send too soon, before getting to my punchline:
Since we already have so many existing iotests that use 'function', it's better to clean that up as a separate patch.
Yeah, I actually thought 'function' was the preferred syntax and omitting it was bad since so many iotests used it :-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
In reply to Daniel P. Berrangétests/qemu-iotests/common.nbd line 54
Quoted 10 lines · Daniel P. Berrangé
The helpers for starting/stopping qemu-nbd in 058 will be useful in other test cases, so move them into a common.nbd file.
Signed-off-by Daniel P. Berrangé <[email protected]>---
tests/qemu-iotests/common.nbd new file +4
-0+function nbd_server_start_unix_socket() +{ + nbd_server_stop + $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
Needs to be "$@" to properly preserve whitespace and/or empty arguments (the latter if someone passes -x '' for a default-named export).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file
Quoted 7 lines · Eric Blake
Quoted 5 lines · Daniel P. Berrangé
The helpers for starting/stopping qemu-nbd in 058 will be useful in other test cases, so move them into a common.nbd file.
Signed-off-by Daniel P. Berrangé <[email protected]>---
Quoted 7 lines
tests/qemu-iotests/common.nbd line 54
Quoted 4 lines
tests/qemu-iotests/common.nbd new file +4
-0+function nbd_server_start_unix_socket() +{ + nbd_server_stop + $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &Needs to be "$@" to properly preserve whitespace and/or empty arguments (the latter if someone passes -x '' for a default-named export).
ok
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 4/6 tests: check if qemu-nbd is still alive before waiting
In reply to Daniel P. BerrangéIf the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit and then check again repeatedly for upto 30 seconds. This is pointless if the qemu-nbd process has quit due to an error, so check whether the pid is still alive before waiting and retrying.
--- tests/qemu-iotests/common.nbd | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
tests/qemu-iotests/common.nbd +9 -1
@@ -37,11 +37,19 @@ function nbd_server_stop() function nbd_server_wait_for_unix_socket() { + pid=$1 + for ((i = 0; i < 300; i++)) do if [ -r "$nbd_unix_socket" ]; then return fi + kill -s 0 $pid 2>/dev/null + if test $? != 0 + then + echo "qemu-nbd unexpectedly quit" + exit 1 + fi sleep 0.1 done echo "Failed in check of unix socket created by qemu-nbd"
@@ -52,5 +60,5 @@ function nbd_server_start_unix_socket() { nbd_server_stop $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ & - nbd_server_wait_for_unix_socket + nbd_server_wait_for_unix_socket $! }
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting
Quoted 2 lines · Daniel P. Berrangé
If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit and then check again repeatedly for upto 30 seconds. This is pointless
s/upto/up to/
Quoted 2 lines
if the qemu-nbd process has quit due to an error, so check whether the pid is still alive before waiting and retrying.
"But our tests are perfect and qemu-nbd never fails" :)
Yes, this makes sense. Not 3.1 material on its own (after all, our testsuite isn't showing such failures, so we aren't wasting that time at the moment) - but worth including if the later patches end up in 3.1.
Quoted 5 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/common.nbd | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Quoted 11 lines
tests/qemu-iotests/common.nbd +2
-0@@ -37,11 +37,19 @@ function nbd_server_stop() function nbd_server_wait_for_unix_socket() { + pid=$1 +
tests/qemu-iotests/common.nbd function nbd_server_stop() · line 51
Quoted 10 lines
tests/qemu-iotests/common.nbd +5
-0for ((i = 0; i < 300; i++)) do if [ -r "$nbd_unix_socket" ]; then return fi + kill -s 0 $pid 2>/dev/null + if test $? != 0 + then + echo "qemu-nbd unexpectedly quit" + exit 1
Maybe the echo should be redirected to stderr. But we aren't consistently doing that in other tests (_init_error does it, but other spots in check are not), so I'm not changing it.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting
Quoted 10 lines · Eric Blake
Quoted 2 lines · Daniel P. Berrangé
If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit and then check again repeatedly for upto 30 seconds. This is pointless
s/upto/up to/
Quoted 2 lines
if the qemu-nbd process has quit due to an error, so check whether the pid is still alive before waiting and retrying.
"But our tests are perfect and qemu-nbd never fails" :)
Well the key benefit for this is actually people writing new tests, like me with this series, who continually screw up the argv syntax to qemu-nbd and don't want to wait 30 seconds for it to fail :)
Quoted 28 lines
Yes, this makes sense. Not 3.1 material on its own (after all, our testsuite isn't showing such failures, so we aren't wasting that time at the moment) - but worth including if the later patches end up in 3.1.
Quoted 5 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/common.nbd | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)Reviewed-by Eric Blake <[email protected]>Quoted 16 lines
tests/qemu-iotests/common.nbd +3
-0@@ -37,11 +37,19 @@ function nbd_server_stop() function nbd_server_wait_for_unix_socket() { + pid=$1 + for ((i = 0; i < 300; i++)) do if [ -r "$nbd_unix_socket" ]; then return fi + kill -s 0 $pid 2>/dev/null
Quoted 8 lines
tests/qemu-iotests/common.nbd function nbd_server_stop() · line 51
Quoted 4 lines
tests/qemu-iotests/common.nbd +4
-0+ if test $? != 0 + then + echo "qemu-nbd unexpectedly quit" + exit 1Maybe the echo should be redirected to stderr. But we aren't consistently doing that in other tests (
_init_errordoes it, but other spots in check are not), so I'm not changing it.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 5/6 tests: add iotests helpers for dealing with TLS certificates
In reply to Daniel P. BerrangéAdd helpers to common.tls for creating TLS certificates for a CA, server and client.
--- tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 tests/qemu-iotests/common.tls
tests/qemu-iotests/common.tls new file +139 -0
@@ -0,0 +1,139 @@ +#!/bin/bash +# +# Helpers for TLS related config +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir} +} + + +function tls_x509_init() +{ + mkdir "${tls_dir}" + + # use a fixed key so we don't waste system entropy on + # each test run + cat > ${tls_dir}/key.pem <<EOF +-----BEGIN PRIVATE KEY----- +MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBALVcr +BL40Tm6yq88FBhJNw1aaoCjmtg0l4dWQZ/e9Fimx4ARxFpT+ji4FE +Cgl9s/SGqC+1nvlkm9ViSo0j7MKDbnDB+VRHDvMAzQhA2X7e8M0n9 +rPolUY2lIVC83q0BBaOBkCj2RSmT2xTEbbC2xLukSrg2WP/ihVOxc +kXRuyFtzAgMBAAECgYB7slBexDwXrtItAMIH6m/U+LUpNe0Xx48OL +IOn4a4whNgO/o84uIwygUK27ZGFZT0kAGAk8CdF9hA6ArcbQ62s1H +myxrUbF9/mrLsQw1NEqpuUk9Ay2Tx5U/wPx35S3W/X2AvR/ZpTnCn +2q/7ym9fyiSoj86drD7BTvmKXlOnOwQJBAPOFMp4mMa9NGpGuEssO +m3Uwbp6lhcP0cA9MK+iOmeANpoKWfBdk5O34VbmeXnGYWEkrnX+9J +bM4wVhnnBWtgBMCQQC+qAEmvwcfhauERKYznMVUVksyeuhxhCe7EK +mPh+U2+g0WwdKvGDgO0PPt1gq0ILEjspMDeMHVdTwkaVBo/uMhAkA +Z5SsZyCP2aTOPFDypXRdI4eqRcjaEPOUBq27r3uYb/jeboVb2weLa +L1MmVuHiIHoa5clswPdWVI2y0em2IGoDAkBPSp/v9VKJEZabk9Frd +a+7u4fanrM9QrEjY3KhduslSilXZZSxrWjjAJPyPiqFb3M8XXA26W +nz1KYGnqYKhLcBAkB7dt57n9xfrhDpuyVEv+Uv1D3VVAhZlsaZ5Pp +dcrhrkJn2sa/+O8OKvdrPSeeu/N5WwYhJf61+CPoenMp7IFci +-----END PRIVATE KEY----- +EOF +} + + +function tls_x509_create_root_ca() +{ + name=$1 + + test -z "$name" && name=ca-cert + + cat > ${tls_dir}/ca.info <<EOF +cn = Cthulu Dark Lord Enterprises $name +ca +cert_signing_key +EOF + + certtool --generate-self-signed \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/ca.info \ + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1 + + rm -f ${tls_dir}/ca.info +} + + +function tls_x509_create_server() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +organization = Cthulu Dark Lord Enterprises $name +cn = localhost +dns_name = localhost +dns_name = localhost.localdomain +ip_address = 127.0.0.1 +ip_address = ::1 +tls_www_server +encryption_key +signing_key +EOF + + certtool --generate-certificate \ + --load-ca-privkey ${tls_dir}/key.pem \ + --load-ca-certificate ${tls_dir}/$caname-cert.pem \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/cert.info \ + --outfile ${tls_dir}/$name/server-cert.pem 2>&1 | head -1 + ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem + ln -s ${tls_dir}/key.pem ${tls_dir}/$name/server-key.pem + + rm -f ${tls_dir}/cert.info +} + + +function tls_x509_create_client() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +country = South Pacific +locality = R'lyeh +organization = Cthulu Dark Lord Enterprises $name +cn = localhost +tls_www_client +encryption_key +signing_key +EOF + + certtool --generate-certificate \ + --load-ca-privkey ${tls_dir}/key.pem \ + --load-ca-certificate ${tls_dir}/$caname-cert.pem \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/cert.info \ + --outfile ${tls_dir}/$name/client-cert.pem 2>&1 | head -1 + ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem + ln -s ${tls_dir}/key.pem ${tls_dir}/$name/client-key.pem + + rm -f ${tls_dir}/cert.info +}
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates
Quoted 2 lines · Daniel P. Berrangé
Add helpers to common.tls for creating TLS certificates for a CA, server and client.
MUCH appreciated! We NEED this coverage, easily automated.
tests/qemu-iotests/common.tls
Quoted 9 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+)create mode 100644 tests/qemu-iotests/common.tls
tests/qemu-iotests/common.tls new file +0
-0
I was a bit surprised that this wasn't 100755, but this matches the fact that none of the other common.* are executable. And after thinking more, it makes sense - they aren't standalone scripts, but designed to be sourced, and 'source' doesn't care about execute bits.
tests/qemu-iotests/common.tls line 28
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir}
Why not just:
rm -rf $tls_dir
Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "".
tests/qemu-iotests/common.tls line 34
Quoted 6 lines
tests/qemu-iotests/common.tls new file +6
-0+} + + +function tls_x509_init() +{ + mkdir "${tls_dir}"
And this just highlights the quoting inconsistency. Should this use mkdir -p?
tests/qemu-iotests/common.tls line 65
Quoted 6 lines
tests/qemu-iotests/common.tls new file +6
-0+ +function tls_x509_create_root_ca() +{ + name=$1 + + test -z "$name" && name=ca-cert
Could also be shortened as:
name=${1:-ca-cert}
tests/qemu-iotests/common.tls line 68
Quoted 3 lines
tests/qemu-iotests/common.tls new file +3
-0+ + cat > ${tls_dir}/ca.info <<EOF +cn = Cthulu Dark Lord Enterprises $name
s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just because we botched the spelling of his name :)
tests/qemu-iotests/common.tls line 76
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+ca +cert_signing_key +EOF + + certtool --generate-self-signed \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/ca.info \ + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1
More missing ""
tests/qemu-iotests/common.tls line 89
Quoted 13 lines
tests/qemu-iotests/common.tls new file +13
-0+ + rm -f ${tls_dir}/ca.info +} + + +function tls_x509_create_server() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +organization = Cthulu Dark Lord Enterprises $name
Matched spelling
tests/qemu-iotests/common.tls line 122
Quoted 10 lines
tests/qemu-iotests/common.tls new file +10
-0+function tls_x509_create_client() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +country = South Pacific +locality = R'lyeh +organization = Cthulu Dark Lord Enterprises $name
And again
Needs several touch-ups, but the idea itself is sound.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates
Quoted 21 lines · Eric Blake
Quoted 2 lines · Daniel P. Berrangé
Add helpers to common.tls for creating TLS certificates for a CA, server and client.
MUCH appreciated! We NEED this coverage, easily automated.
tests/qemu-iotests/common.tls
Quoted 9 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+)create mode 100644 tests/qemu-iotests/common.tls
tests/qemu-iotests/common.tls new file +0
-0I was a bit surprised that this wasn't 100755, but this matches the fact that none of the other common.* are executable. And after thinking more, it makes sense - they aren't standalone scripts, but designed to be sourced, and 'source' doesn't care about execute bits.
Quoted 11 lines
tests/qemu-iotests/common.tls line 28
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir}Why not just:
rm -rf $tls_dir
Yeah, I guess we could do that for simplicity
Quoted 2 lines
Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "".
Hmm, yes.
Quoted 9 lines
tests/qemu-iotests/common.tls line 34
Quoted 6 lines
tests/qemu-iotests/common.tls new file +6
-0+} + + +function tls_x509_init() +{ + mkdir "${tls_dir}"And this just highlights the quoting inconsistency. Should this use mkdir -p?
I assume $TEST_DIR would already exist, so wouldn't need -p.
Quoted 10 lines
tests/qemu-iotests/common.tls line 65
Quoted 6 lines
tests/qemu-iotests/common.tls new file +6
-0+ +function tls_x509_create_root_ca() +{ + name=$1 + + test -z "$name" && name=ca-certCould also be shortened as:
name=${1:-ca-cert}
ok
Quoted 47 lines
tests/qemu-iotests/common.tls line 68
Quoted 3 lines
tests/qemu-iotests/common.tls new file +3
-0+ + cat > ${tls_dir}/ca.info <<EOF +cn = Cthulu Dark Lord Enterprises $names/Cthulu/Cthulhu/ - after all, we don't want him coming after us just because we botched the spelling of his name :)
tests/qemu-iotests/common.tls line 76
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+ca +cert_signing_key +EOF + + certtool --generate-self-signed \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/ca.info \ + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1More missing ""
tests/qemu-iotests/common.tls line 89
Quoted 13 lines
tests/qemu-iotests/common.tls new file +13
-0+ + rm -f ${tls_dir}/ca.info +} + + +function tls_x509_create_server() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +organization = Cthulu Dark Lord Enterprises $nameMatched spelling
tests/qemu-iotests/common.tls line 122
Quoted 10 lines
tests/qemu-iotests/common.tls new file +10
-0+function tls_x509_create_client() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +country = South Pacific +locality = R'lyeh +organization = Cthulu Dark Lord Enterprises $nameAnd again
Quoted 1 line
Needs several touch-ups, but the idea itself is sound.
Yes will fix
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates
Quoted 34 lines · Daniel P. Berrangé
Quoted 21 lines · Eric Blake
Quoted 2 lines · Daniel P. Berrangé
Add helpers to common.tls for creating TLS certificates for a CA, server and client.
MUCH appreciated! We NEED this coverage, easily automated.
tests/qemu-iotests/common.tls
Quoted 9 lines
Signed-off-by Daniel P. Berrangé <[email protected]>--- tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+)create mode 100644 tests/qemu-iotests/common.tls
tests/qemu-iotests/common.tls new file +0
-0I was a bit surprised that this wasn't 100755, but this matches the fact that none of the other common.* are executable. And after thinking more, it makes sense - they aren't standalone scripts, but designed to be sourced, and 'source' doesn't care about execute bits.
Quoted 11 lines
tests/qemu-iotests/common.tls line 28
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir}Why not just:
rm -rf $tls_dir
Quoted 6 lines
Yeah, I guess we could do that for simplicity
Quoted 2 lines
Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "".
Hmm, yes.
Which by the way is a very good reason *not* to blindly use "rm -r".
So far we only seem to have one instance of "rm -r" in the iotests (and that is on three files, so I don't even know why that has -r), and I'm glad about that.
I prefer for scripts to only delete what they have created and not blindly delete something. Wildcards are already kind of pushing it.
Maybe the user has created the tls dir beforehand, then I'd prefer for the iotests not to just delete it and everything in it. But the worst of course would be if we get escaping wrong somewhere (as demonstrated here) and suddenly we delete a completely unrelated directory by accident. Anyone remember Steam's 'rm -rf "$STEAMROOT"/*'?
Everyone knows they have to be careful with deleting things, but most of the time it is a bother if you're in an interactive shell and know your directory structure and all the arguments you're passing perfectly well.
But a script doesn't know either, and "it's a bother" is not really an
argument if you have to write the code just once.
Max
Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates
Quoted 12 lines · Max Reitz
Quoted 12 lines
Quoted 11 lines
tests/qemu-iotests/common.tls line 28
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir}Why not just:
rm -rf $tls_dir
Quoted 8 lines
Quoted 6 lines
Yeah, I guess we could do that for simplicity
Quoted 2 lines
Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "".
Hmm, yes.
Which by the way is a very good reason *not* to blindly use "rm -r".
If we ever revive Jeff Cody's patches to let each test run with its own temporary directory, then we don't need this function at all. With that in place, you either run the testsuite in debug mode (all temporary files preserved) or in normal mode (./check itself does rm -rf on the temporary directory).
Quoted 4 lines
So far we only seem to have one instance of "rm -r" in the iotests (and that is on three files, so I don't even know why that has -r), and I'm glad about that.
But until we have the global implementation of per-test temporary directories with cleanup relegated to the testsuite driver, I'm fine following your preference of explicit deletion of specific files rather than recursive deletion of the tree, even if it is more lines of code.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates
Quoted 21 lines · Eric Blake
Quoted 19 lines · Max Reitz
Quoted 18 lines
Quoted 11 lines
tests/qemu-iotests/common.tls line 28
Quoted 8 lines
tests/qemu-iotests/common.tls new file +8
-0+tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir}Why not just:
rm -rf $tls_dirYeah, I guess we could do that for simplicity
Quoted 2 lines
Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "".
Hmm, yes.
Quoted 7 lines
Quoted 1 line
Which by the way is a very good reason *not* to blindly use "rm -r".
If we ever revive Jeff Cody's patches to let each test run with its own temporary directory, then we don't need this function at all. With that in place, you either run the testsuite in debug mode (all temporary files preserved) or in normal mode (./check itself does rm -rf on the temporary directory).
That would be very nice - i'm often just commenting out the rm lines to preserve temp files.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 6/6 tests: exercise NBD server in TLS mode
In reply to Daniel P. BerrangéAdd tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
--- tests/qemu-iotests/233 | 99 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/233.out | 33 ++++++++++++ tests/qemu-iotests/common.nbd | 47 +++++++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 180 insertions(+) create mode 100755 tests/qemu-iotests/233 create mode 100644 tests/qemu-iotests/233.out
tests/qemu-iotests/233 new file +99 -0
@@ -0,0 +1,99 @@ +#!/bin/bash +# +# Test NBD TLS certificate / authorization integration +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +_cleanup() +{ + nbd_server_stop + _cleanup_test_img + tls_x509_cleanup +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.pattern +. ./common.tls +. ./common.nbd + +_supported_fmt raw qcow2 +_supported_proto file +_supported_os Linux +_require_command QEMU_NBD + +nbd_server_set_tcp_port +tls_x509_init + +echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2" + +echo +echo "== preparing image ==" +_make_test_img 64M + + +echo +echo "== check TLS client to plain server fails ==" +nbd_server_start_tcp_socket "$TEST_IMG" + +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +nbd_server_stop + +echo +echo "== check plain client to TLS server fails ==" + +nbd_server_start_tcp_socket --object tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes --tls-creds tls0 "$TEST_IMG" + +$QEMU_IMG info nbd://localhost:$nbd_tcp_port + +echo +echo "== check TLS works ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +echo +echo "== check TLS with different CA fails ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +# success, all done +echo "*** done" +rm -f $seq.full +status=0
tests/qemu-iotests/233.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 233 + +== preparing TLS creds == +Generating a self signed certificate... +Generating a self signed certificate... +Generating a signed certificate... +Generating a signed certificate... +Generating a signed certificate... + +== preparing image == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 + +== check TLS client to plain server fails == +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls) +server reported: TLS not configured + +== check plain client to TLS server fails == +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply) +server reported: Option 0x8 not permitted before TLS +write failed (error message): Unable to write to socket: Broken pipe + +== check TLS works == +image: nbd://127.0.0.1:10809 +file format: nbd +virtual size: 64M (67108864 bytes) +disk size: unavailable + +== check TLS with different CA fails == +option negotiation failed: Verify failed: No certificate was found. +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': The certificate hasn't got a known issuer +*** done
tests/qemu-iotests/common.nbd +47 -0
56 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -228,3 +228,4 @@ 229 auto quick 231 auto quick 232 auto quick +233 auto quick
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 11 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
--- tests/qemu-iotests/233 | 99 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/233.out | 33 ++++++++++++ tests/qemu-iotests/common.nbd | 47 +++++++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 180 insertions(+)create mode 100755 tests/qemu-iotests/233
create mode 100644 tests/qemu-iotests/233.out
Adding tests is non-invasive, so I have no objection to taking the entire series in 3.1. Do you want me to touch up the nits I found earlier, or should I wait for a v2?
tests/qemu-iotests/233 line 27
Quoted 7 lines
tests/qemu-iotests/233 new file +7
-0+# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd`
Dead code. Mao Zhongyi has a patch to remove it. I'm thinking of putting his patches in first, and then yours.
tests/qemu-iotests/233 line 69
Quoted 4 lines
tests/qemu-iotests/233 new file +4
-0+ +echo +echo "== check TLS client to plain server fails ==" +nbd_server_start_tcp_socket "$TEST_IMG"
The negative testing is just as important as positive testing, to prove TLS is adding the promised authorization.
tests/qemu-iotests/233 line 80
Quoted 11 lines
tests/qemu-iotests/233 new file +11
-0+ +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +nbd_server_stop + +echo +echo "== check plain client to TLS server fails ==" + +nbd_server_start_tcp_socket --object tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes --tls-creds tls0 "$TEST_IMG"
Long line
tests/qemu-iotests/233 line 88
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+ +$QEMU_IMG info nbd://localhost:$nbd_tcp_port + +echo +echo "== check TLS works ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
Is it also worth reading or even writing, to ensure that more than just the handshake is working?
tests/qemu-iotests/233
Quoted 11 lines
tests/qemu-iotests/233 new file +10
-0+ +echo +echo "== check TLS with different CA fails ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +# success, all done +== check TLS client to plain server fails == +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
Annoying message; I wonder if we can clean that up. But not this patch's problem.
tests/qemu-iotests/233.out line 16
Quoted 2 lines
tests/qemu-iotests/233.out new file +2
-0+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls) +server reported: TLS not configured
This 2-line message is better (and as such, explains why I think the message about early EOF should be silenced in this case).
tests/qemu-iotests/233.out line 21
Quoted 5 lines
tests/qemu-iotests/233.out new file +5
-0+ +== check plain client to TLS server fails == +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply) +server reported: Option 0x8 not permitted before TLS
Same story about a redundant message.
Quoted 1 line
+write failed (error message): Unable to write to socket: Broken pipe
Hmm - is this the client complaining that it can't send more to the server (that's a bug if the server hung up, since the protocol allows the client to change its mind and try TLS after all), or the server complaining that the client hung up abruptly without sending NBD_OPT_ABORT? Qemu as client either tries TLS right away, or knows that if the server requires TLS and the client has no TLS credentials then the client will never succeed, so the client gives up - but it SHOULD be giving up with NBD_OPT_ABORT rather than just disconnecting. I'll have to play with that. Doesn't impact your patch, but might spur a followup fix :)
tests/qemu-iotests/233.out line 29
Quoted 7 lines
tests/qemu-iotests/233.out new file +7
-0+ +== check TLS works == +image: nbd://127.0.0.1:10809 +file format: nbd +virtual size: 64M (67108864 bytes) +disk size: unavailable +
Again, also doing a read and/or write to ensure the encrypted data is handled correctly would be nice. Can be in a followup patch.
tests/qemu-iotests/common.nbd line 23
Quoted 13 lines
+== check TLS with different CA fails ==
tests/qemu-iotests/233.out new file +3
-0+option negotiation failed: Verify failed: No certificate was found. +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': The certificate hasn't got a known issuer +*** donetests/qemu-iotests/common.nbd +1
-0@@ -20,6 +20,7 @@ # nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock" +nbd_tcp_addr="127.0.0.1"
Should this be in your earlier patch that created common.nbd? Maybe not, as it doesn't get used until the functions you add here for tcp support. Still, I wonder if we should split the addition of tcp support separate from the new test.
tests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 71
Quoted 13 lines
tests/qemu-iotests/common.nbd +6
-0nbd_pid_file="${TEST_DIR}/qemu-nbd.pid" function nbd_server_stop()@@ -62,3 +63,49 @@ function nbd_server_start_unix_socket() $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ & nbd_server_wait_for_unix_socket $! } + +function nbd_server_set_tcp_port() +{ + for port in `seq 10809 10909` + do + socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas. nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.
tests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 80
Quoted 9 lines
tests/qemu-iotests/common.nbd +9
-0+ if test $? != 0 + then + nbd_tcp_port=$port + return + fi + done + + echo "Cannot find free TCP port for nbd in range 10809-10909" + exit 1
Should we skip instead of fail in this case? Would we do well picking a larger port range?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 6 lines · Eric Blake
Quoted 4 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---
Quoted 6 lines
Quoted 3 lines
+== check TLS client to plain server fails ==
+option negotiation failed: read failed: Unexpected end-of-file before all bytes were readAnnoying message; I wonder if we can clean that up. But not this patch's problem.
Actually, I tracked this message down to using socat (which actually connects and then abruptly exits) when probing whether the socket is up and listening. That is, the message is being produced as a side effect of nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG command we are interested in testing.
Quoted 6 lines
Quoted 6 lines
nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
functionnbd_server_stop()(quoted diff) +0
-0@@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()$QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
nbd_server_wait_for_unix_socket$!
}
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ +function nbd_server_set_tcp_port() +{ + for port in `seq 10809 10909` + do + socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.shgreps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.
So, I'm planning to squash this in, to use ss instead of socat, as follows:
tests/qemu-iotests/common.nbd +6 -8
@@ -66,12 +66,12 @@ function nbd_server_start_unix_socket() function nbd_server_set_tcp_port() { - for port in `seq 10809 10909` + (ss --help) >/dev/null 2>&1 || _notrun "ss utility not found, skipping test" + + for ((port = 10809; port <= 10909; port++)) do - socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1 - if test $? != 0 - then - nbd_tcp_port=$port + if ! ss -tln | grep -sqE ":$port\b"; then + nbd_tcp_port=$port return fi done
@@ -86,9 +86,7 @@ function nbd_server_wait_for_tcp_socket() for ((i = 0; i < 300; i++)) do - socat TCP:localhost:$nbd_tcp_port STDIO < /dev/null 1>/dev/null 2>&1 - if test $? == 0 - then + if ss -tln | grep -sqE ":$nbd_tcp_port\b"; then return fi kill -s 0 $pid 2>/dev/null
tests/qemu-iotests/233.out +0 -1
@@ -11,12 +11,10 @@ Generating a signed certificate... Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 == check TLS client to plain server fails == -option negotiation failed: read failed: Unexpected end-of-file before all bytes were read qemu-img: Could not open
'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls)
server reported: TLS not configured
== check plain client to TLS server fails ==
-option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation
required before option 8 (structured reply)
server reported: Option 0x8 not permitted before TLS
write failed (error message): Unable to write to socket: Broken pipe
Also, you have to sanitize 233.out to change 10809 into PORT, so the test can still pass when it picked a different port.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 40 lines · Eric Blake
Quoted 6 lines · Eric Blake
Quoted 4 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---Quoted 7 lines
Quoted 3 lines
+== check TLS client to plain server fails ==
+option negotiation failed: read failed: Unexpected end-of-file before all bytes were readAnnoying message; I wonder if we can clean that up. But not this patch's problem.
Actually, I tracked this message down to using socat (which actually connects and then abruptly exits) when probing whether the socket is up and listening. That is, the message is being produced as a side effect of
nbd_server_wait_for_tcp_socketrather than during the actual $QEMU_IMG command we are interested in testing.Quoted 9 lines
Quoted 9 lines
nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
functionnbd_server_stop()(quoted diff) +0
-0@@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()$QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
nbd_server_wait_for_unix_socket$!
}(quoted diff) +3
-0+ +function nbd_server_set_tcp_port() +{Quoted 8 lines
Quoted 3 lines
+ for port in
seq 10809 10909
+ do
+ socat TCP:$nbd_tcp_addr:$port STDIO </dev/null1>/dev/null 2>&1This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.shgreps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.
Quoted 1 line
So, I'm planning to squash this in, to use ss instead of socat, as follows:
Personally I prefer socat since it is more portable, per my previous message.
Quoted 24 lines
tests/qemu-iotests/233.out +0
-1@@ -11,12 +11,10 @@ Generating a signed certificate... Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 == check TLS client to plain server fails == -option negotiation failed: read failed: Unexpected end-of-file before allbytes were read
qemu-img: Could not open
'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls)
server reported: TLS not configured== check plain client to TLS server fails ==
-option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
before option 8 (structured reply)
server reported: Option0x8not permitted before TLS
write failed (error message): Unable to write to socket: Broken pipe
Quoted 2 lines
Also, you have to sanitize 233.out to change 10809 into PORT, so the test can still pass when it picked a different port.
Opps, yes.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 11 lines · Daniel P. Berrangé
Quoted 11 lines
Actually, I tracked this message down to using socat (which actually connects and then abruptly exits) when probing whether the socket is up and listening. That is, the message is being produced as a side effect of
nbd_server_wait_for_tcp_socketrather than during the actual $QEMU_IMG command we are interested in testing.Quoted 4 lines
This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.shgreps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.
Quoted 4 lines
Quoted 1 line
So, I'm planning to squash this in, to use ss instead of socat, as follows:
Personally I prefer socat since it is more portable, per my previous message.
socat is indeed probably more portable, but since tests 233 uses '_supported_os Linux', ss availability isn't a problem until a future user ports this test to non-Linux. I'd like to patch qemu-nbd to NOT warn about a user that connects but does not consume data (the socat case, as well as port probes), but as that patch does not exist yet and -rc2 is getting close, I'll go ahead and send the pull request with ss instead of socat.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 18 lines · Eric Blake
Quoted 11 lines · Daniel P. Berrangé
Quoted 11 lines
Actually, I tracked this message down to using socat (which actually connects and then abruptly exits) when probing whether the socket is up and listening. That is, the message is being produced as a side effect of
nbd_server_wait_for_tcp_socketrather than during the actual $QEMU_IMG command we are interested in testing.Quoted 4 lines
This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.shgreps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.Quoted 4 lines
Quoted 1 line
So, I'm planning to squash this in, to use ss instead of socat, as follows:
Personally I prefer socat since it is more portable, per my previous message.
Quoted 6 lines
socat is indeed probably more portable, but since tests 233 uses '
_supported_osLinux', ss availability isn't a problem until a future user ports this test to non-Linux. I'd like to patch qemu-nbd to NOT warn about a user that connects but does not consume data (the socat case, as well as port probes), but as that patch does not exist yet and -rc2 is getting close, I'll go ahead and send the pull request with ss instead of socat.
Yes, that's ok with me.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Eric BlakeQuoted 13 lines · Eric Blake
Quoted 11 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
--- tests/qemu-iotests/233 | 99 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/233.out | 33 ++++++++++++ tests/qemu-iotests/common.nbd | 47 +++++++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 180 insertions(+)create mode 100755 tests/qemu-iotests/233
create mode 100644 tests/qemu-iotests/233.out
Quoted 3 lines
Adding tests is non-invasive, so I have no objection to taking the entire series in 3.1. Do you want me to touch up the nits I found earlier, or should I wait for a v2?
If you're happy to touch it up, that's fine with me.
Quoted 11 lines
tests/qemu-iotests/233 line 88
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+ +$QEMU_IMG info nbd://localhost:$nbd_tcp_port + +echo +echo "== check TLS works ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0Is it also worth reading or even writing, to ensure that more than just the handshake is working?
I was mostly interested in the handshake/cert stuff, but yes, we could do some qemu-io testing too.
Quoted 9 lines
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+ +echo +echo "== check TLS with different CA fails ==" +$QEMU_IMG info --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 + +# success, all done
Quoted 5 lines
Quoted 2 lines
+== check TLS client to plain server fails ==
+option negotiation failed: read failed: Unexpected end-of-file before all bytes were readAnnoying message; I wonder if we can clean that up. But not this patch's problem.
This is a result of using the 'socat' check to poll for qemu-nbd readiness. When socat finally succeeds in connecting & then drops the conenction, qemu-nbd prints this message. Personally I don't think we need remove it unless we want to make qemu-nbd silently by default when clients do unusual things.
Quoted 6 lines
tests/qemu-iotests/233.out line 16
Quoted 2 lines
tests/qemu-iotests/233.out new file +2
-0+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls) +server reported: TLS not configuredThis 2-line message is better (and as such, explains why I think the message about early EOF should be silenced in this case).
Quoted 7 lines
tests/qemu-iotests/233.out line 21
Quoted 5 lines
tests/qemu-iotests/233.out new file +5
-0+ +== check plain client to TLS server fails == +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply) +server reported: Option 0x8 not permitted before TLSSame story about a redundant message.
Again this was the socat polling.
Quoted 12 lines
Quoted 2 lines
+write failed (error message): Unable to write to socket: Broken pipe
Hmm - is this the client complaining that it can't send more to the server (that's a bug if the server hung up, since the protocol allows the client to change its mind and try TLS after all), or the server complaining that the client hung up abruptly without sending
NBD_OPT_ABORT? Qemu as client either tries TLS right away, or knows that if the server requires TLS and the client has no TLS credentials then the client will never succeed, so the client gives up - but it SHOULD be giving up withNBD_OPT_ABORTrather than just disconnecting. I'll have to play with that. Doesn't impact your patch, but might spur a followup fix :)
I'm unclear if this message comes from the server or the client since they are intermingled.
Quoted 5 lines
Quoted 5 lines
tests/qemu-iotests/common.nbd +0
-0@@ -20,6 +20,7 @@
Quoted 8 lines
tests/qemu-iotests/common.nbd line 23
Quoted 3 lines
tests/qemu-iotests/common.nbd +1
-0# nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock" +nbd_tcp_addr="127.0.0.1"Should this be in your earlier patch that created common.nbd? Maybe not, as it doesn't get used until the functions you add here for tcp support. Still, I wonder if we should split the addition of tcp support separate from the new test.
Yeah, I wanted the earlier patch to be a plain refactor of existing functionality, not adding anything new.
Quoted 9 lines
Quoted 9 lines
tests/qemu-iotests/common.nbd +3
-0nbd_pid_file="${TEST_DIR}/qemu-nbd.pid" function nbd_server_stop()@@ -62,3 +63,49 @@ function nbd_server_start_unix_socket() $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ & nbd_server_wait_for_unix_socket $! } + +function nbd_server_set_tcp_port() +{
Quoted 8 lines
tests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 71
Quoted 3 lines
tests/qemu-iotests/common.nbd +3
-0+ for port in `seq 10809 10909` + do + socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1This is the first use of socat in iotests. Might not be the most portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.shgreps the output of 'ss -ltn' to locate free ports, but I don't know if ss is any better than socat.
'ss' is a Linux specific command IIUC since it is part of 'iproute', while 'socat' is in fact available more or less everywhere:
https://repology.org/metapackage/socat1/versions
Quoted 3 lines
tests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 74
Quoted 3 lines
tests/qemu-iotests/common.nbd +3
-0+ if test $? != 0 + then + nbd_tcp_port=$port
Opps, a stray <tab> here
Quoted 9 lines
tests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 80
Quoted 6 lines
tests/qemu-iotests/common.nbd +6
-0+ return + fi + done + + echo "Cannot find free TCP port for nbd in range 10809-10909" + exit 1Should we skip instead of fail in this case? Would we do well picking a larger port range?
Yes, we could simply skip. I figure 100 ports is fine, since tests are normally run on a largely idle system.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Daniel P. BerrangéQuoted 4 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---
Missing your Signed-off-by. Can you please supply that, so I can include this in my pull request?
Also, I'm getting failures when trying to test it:
@@ -17,9 +17,9 @@
== check plain client to TLS server fails ==
option negotiation failed: read failed: Unexpected end-of-file before
all bytes were read
+write failed (error message): Unable to write to socket: Broken pipe
qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation
required before option 8 (structured reply)
server reported: Option 0x8 not permitted before TLS
-write failed (error message): Unable to write to socket: Broken pipe
which looks like an output race. :(
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
Tested-byRe: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 22 lines · Eric Blake
Quoted 4 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---Missing your Signed-off-by. Can you please supply that, so I can include this in my pull request?
Also, I'm getting failures when trying to test it:
(quoted diff) +0
-0@@ -17,9 +17,9 @@ == check plain client to TLS server fails == option negotiation failed: read failed: Unexpected end-of-file beforeall bytes were read
+write failed (error message): Unable to write to socket: Broken pipe
qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation
required before option 8 (structured reply)
server reported: Option0x8not permitted before TLS
-write failed (error message): Unable to write to socket: Broken pipe
Quoted 1 line
which looks like an output race. :(
Found and squashed it - commit 37ec36f6 fixed plaintext servers to not be noisy for NBD_OPT_ABORT, but did not give equal treatment to TLS servers. Patch coming up separately.
So, with my fixes, I can add:
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 1/1 nbd/server: Ignore write errors when replying to NBD_OPT_ABORT
In reply to Eric BlakeCommit 37ec36f6 intentionally ignores errors when trying to reply to an NBD_OPT_ABORT request for plaintext clients, but did not make the same change for a TLS server. Since NBD_OPT_ABORT is documented as being a potential for an EPIPE when the client hangs up without waiting for our reply, we don't need to pollute the server's output with that failure.
--- nbd/server.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
nbd/server.c +8 -4
@@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,return -EINVAL ;default : - ret =nbd_opt_drop (client,NBD_REP_ERR_TLS_REQD , errp, +/* Let the client keep trying, unless they asked to +* quit. Always try to give an error back to the +* client; but when replying to OPT_ABORT, be aware +* that the client may hang up before receiving the +* error, in which case we are fine ignoring the +* resulting EPIPE. */ + ret =nbd_opt_drop (client,NBD_REP_ERR_TLS_REQD , + option ==NBD_OPT_ABORT ?NULL : errp,"Option 0x%" PRIx32" not permitted before TLS" , option); -/* Let the client keep trying, unless they asked to -* quit. In this mode, we've already sent an error, so -* we can't ack the abort. */ if (option ==NBD_OPT_ABORT ) {return 1 ; }
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH 1.5/6] nbd/server: Ignore write errors when replying to NBD_OPT_ABORT
Quoted 27 lines · Eric Blake
Commit
37ec36f6intentionally ignores errors when trying to reply to anNBD_OPT_ABORTrequest for plaintext clients, but did not make the same change for a TLS server. SinceNBD_OPT_ABORTis documented as being a potential for an EPIPE when the client hangs up without waiting for our reply, we don't need to pollute the server's output with that failure.Signed-off-by Eric Blake <[email protected]>--- nbd/server.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)nbd/server.c +6
-1@@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,return -EINVAL ;default : - ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp, +/* Let the client keep trying, unless they asked to +* quit. Always try to give an error back to the +* client; but when replying to OPT_ABORT, be aware +* that the client may hang up before receiving the +* error, in which case we are fine ignoring the +* resulting EPIPE. */
nbd/server.c static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, · line 1149
Quoted 10 lines
nbd/server.c +2
-3+ ret =nbd_opt_drop (client,NBD_REP_ERR_TLS_REQD , + option ==NBD_OPT_ABORT ?NULL : errp,"Option 0x%" PRIx32" not permitted before TLS" , option); -/* Let the client keep trying, unless they asked to -* quit. In this mode, we've already sent an error, so -* we can't ack the abort. */ if (option ==NBD_OPT_ABORT ) {return 1 ; }
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Eric BlakeQuoted 8 lines · Eric Blake
Quoted 4 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---Missing your Signed-off-by. Can you please supply that, so I can include this in my pull request?
Quoted 15 lines
Also, I'm getting failures when trying to test it:
(quoted diff) +0
-0@@ -17,9 +17,9 @@ == check plain client to TLS server fails == option negotiation failed: read failed: Unexpected end-of-file before allbytes were read
+write failed (error message): Unable to write to socket: Broken pipe
qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
before option 8 (structured reply)
server reported: Option0x8not permitted before TLS
-write failed (error message): Unable to write to socket: Broken pipe
Quoted 1 line
which looks like an output race. :(
Guess that shows the message is in fact coming from the server rather than the client then.
We could just silence the qemu-nbd process to null, since we're primarily interested in what the client displays in this test
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS
In reply to Daniel P. BerrangéEnhance test 233 to also perform I/O beyond the initial handshake.
---
Depends on my tweak to 2/6 to suppress an EIO error message on a failed read after NBD_CMD_DISC.
tests/qemu-iotests/233 | 12 +++++++++++- tests/qemu-iotests/233.out | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)
tests/qemu-iotests/233 +11 -1
@@ -61,7 +61,7 @@ tls_x509_create_client "ca2" "client2" echo echo "== preparing image ==" _make_test_img 64M - +$QEMU_IO -c 'w -P 0x11 1m 1m' "$TEST_IMG" | _filter_qemu_io echo echo "== check TLS client to plain server fails =="
@@ -95,6 +95,16 @@ $QEMU_IMG info --image-opts \ driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \ 2>&1 | sed "s/$nbd_tcp_port/PORT/g" +echo +echo "== perform I/O over TLS ==" +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT +$QEMU_IO -c 'r -P 0x11 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \ + --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \ + driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \ + 2>&1 | sed "s/$nbd_tcp_port/PORT/g" | _filter_qemu_io + +$QEMU_IO -f qcow2 -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io + # success, all done echo "*** done" rm -f $seq.full
tests/qemu-iotests/233.out +10 -0
@@ -9,6 +9,8 @@ Generating a signed certificate... == preparing image == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +wrote 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == check TLS client to plain server fails == qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Denied by server for option 5 (starttls)
@@ -27,4 +29,12 @@ disk size: unavailable == check TLS with different CA fails == option negotiation failed: Verify failed: No certificate was found. qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer + +== perform I/O over TLS == +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done
Signature · 1 line
2.17.2
Daniel P. Berrangé
Reviewed-byRe: [Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS
Quoted 11 lines · Eric Blake
Enhance test 233 to also perform I/O beyond the initial handshake.
Signed-off-by Eric Blake <[email protected]>---
Depends on my tweak to 2/6 to suppress an EIO error message on a failed read after
NBD_CMD_DISC.tests/qemu-iotests/233 | 12 +++++++++++- tests/qemu-iotests/233.out | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS
Quoted 6 lines · Daniel P. Berrangé
Quoted 5 lines · Eric Blake
Enhance test 233 to also perform I/O beyond the initial handshake.
Signed-off-by Eric Blake <[email protected]>---
Quoted 8 lines
Quoted 6 lines
Depends on my tweak to 2/6 to suppress an EIO error message on a failed read after
NBD_CMD_DISC.tests/qemu-iotests/233 | 12 +++++++++++- tests/qemu-iotests/233.out | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)Reviewed-by Daniel P. Berrangé <[email protected]>
Yay - after dropping patch 2/6 (both your work, and the code I was questioning whether to squash in), and instead using your patch for making qio return 0 on connection abort after shutdown, this test still passes without any extra lines from the server.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Daniel P. Berrangétests/qemu-iotests/common.nbd function nbd_server_start_unix_socket() · line 102
Quoted 13 lines · Daniel P. Berrangé
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
---+++ b/tests/qemu-iotests/common.nbd
tests/qemu-iotests/common.nbd +5
-0+function nbd_server_wait_for_tcp_socket() +{ + sleep 0.1 + done + echo "Failed in check of unix socket created by qemu-nbd"
Too much copy-and-paste. Touching up. :)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Daniel P. BerrangéAm 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
tests/qemu-iotests/233 line 60
Quoted 12 lines
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"
Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
@@ -1,30 +1,21 @@
QA output created by 233
== preparing TLS creds ==
-Generating a self signed certificate... -Generating a self signed certificate... -Generating a signed certificate... -Generating a signed certificate... -Generating a signed certificate... +./common.tls: line 71: certtool: command not found +./common.tls: line 71: certtool: command not found +./common.tls: line 98: certtool: command not found +./common.tls: line 127: certtool: command not found +./common.tls: line 127: certtool: command not found
Of course, after that everything else fails as well.
Kevin
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 5 lines · Kevin Wolf
Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
Quoted 3 lines
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
Quoted 11 lines
tests/qemu-iotests/233 line 60
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
Looks like we'll need a followup patch to skip the test if certtool is not found. (I already did the same in common.nbd if 'ss' was not found; so it should be easy to copy...)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 18 lines · Eric Blake
Quoted 5 lines · Kevin Wolf
Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
Quoted 3 lines
Add tests that validate it is possible to connect to an NBD server running TLS mode. Also test mis-matched TLS vs non-TLS connections correctly fail.
Quoted 11 lines
tests/qemu-iotests/233 line 60
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
Quoted 3 lines
Looks like we'll need a followup patch to skip the test if certtool is not found. (I already did the same in common.nbd if 'ss' was not found; so it should be easy to copy...)
FWIW certtool is part of gnutls-utils and is available on every platform that QEMU officially supports as a build target.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Quoted 12 lines · Daniel P. Berrangé
Quoted 12 lines
Quoted 11 lines
tests/qemu-iotests/233 line 60
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
Quoted 6 lines
Quoted 3 lines
Looks like we'll need a followup patch to skip the test if certtool is not found. (I already did the same in common.nbd if 'ss' was not found; so it should be easy to copy...)
FWIW certtool is part of gnutls-utils and is available on every platform that QEMU officially supports as a build target.
Ported to the build target != installed on the build machine. The failure is happening on machines that do not have all the build prerequisites (since it should still be possible to configure qemu to build without TLS, right?)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
Am 20.11.2018 um 19:22 hat Eric Blake geschrieben:
Quoted 18 lines · Daniel P. Berrangé
Quoted 17 lines
Quoted 16 lines
Quoted 12 lines
tests/qemu-iotests/233 line 60
Quoted 9 lines
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
Looks like we'll need a followup patch to skip the test if certtool is not found. (I already did the same in common.nbd if 'ss' was not found; so it should be easy to copy...)
Quoted 6 lines
Quoted 2 lines
FWIW certtool is part of gnutls-utils and is available on every platform that QEMU officially supports as a build target.
Ported to the build target != installed on the build machine. The failure is happening on machines that do not have all the build prerequisites (since it should still be possible to configure qemu to build without TLS, right?)
It happens on my laptop, and qemu builds just fine.
Kevin
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
In reply to Eric BlakeQuoted 18 lines · Eric Blake
Quoted 16 lines · Daniel P. Berrangé
Quoted 15 lines
Quoted 11 lines
tests/qemu-iotests/233 line 60
Quoted 8 lines
tests/qemu-iotests/233 new file +8
-0+echo +echo "== preparing TLS creds ==" + +tls_x509_create_root_ca "ca1" +tls_x509_create_root_ca "ca2" +tls_x509_create_server "ca1" "server1" +tls_x509_create_client "ca1" "client1" +tls_x509_create_client "ca2" "client2"Looks like we can't blindly assume that certtool exists. This test case fails for me, starting with the following diff:
Looks like we'll need a followup patch to skip the test if certtool is not found. (I already did the same in common.nbd if 'ss' was not found; so it should be easy to copy...)
Quoted 6 lines
Quoted 2 lines
FWIW certtool is part of gnutls-utils and is available on every platform that QEMU officially supports as a build target.
Ported to the build target != installed on the build machine. The failure is happening on machines that do not have all the build prerequisites (since it should still be possible to configure qemu to build without TLS, right?)
Yes, I just assumed that the iotests could expect a fully featured install
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 7 lines · Daniel P. Berrangé
This does two minor fixes to the NBD code and adds significant coverage of the NBD TLS support to detect future problems.
The first two patches should be for 3.1.
The tests can wait till 4.0 if desired.
Quoted 8 lines
Daniel P. Berrangé (6):
nbd: fix whitespace in server error message
nbd: stop waiting for a NBD response withNBD_CMD_DISC
tests: pull qemu-nbd iotest helpers into common.nbd file
tests: check if qemu-nbd is still alive before waiting
tests: add iotests helpers for dealing with TLS certificates
tests: exercise NBD server in TLS mode
I'm still missing your S-o-b on 6. I've posted a preliminary version of your series with my touchups incorporated, if you'd like to double check it, at:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/nbd
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 6 lines · Daniel P. Berrangé
This does two minor fixes to the NBD code and adds significant coverage of the NBD TLS support to detect future problems.
The first two patches should be for 3.1.
The tests can wait till 4.0 if desired.
Although this series is now in 3.1, I can think of further enhancements we should add for 4.0 (summarizing an IRC conversation with Dan). Capturing it here to remember things...
- we need iotests coverage of Pre-Shared Keys (PSK) as an alternative to certificates (either add on to 233, or a new test)
- add an optional QMP parameter for specifying the hostname to validate a certificate against when using a Unix socket with TLS (compare tls-hostname added to 'migrate'), rather than the current restriction that using TLS with an NBD client requires TCP
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org