| Message ID | 20181116155325.22428-7-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc fixes to NBD | expand |
On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > 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? Reviewed-by: Eric Blake <eblake@redhat.com> > +# creator > +owner=berrange@redhat.com > + > +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. > + > +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. > + > +$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 > + > +$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? > + > +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. > +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). > + > +== 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. > +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 :) > + > +== 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. > +== 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 > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd > index 61e9e90fee..0483ea7c55 100644 > --- a/tests/qemu-iotests/common.nbd > +++ b/tests/qemu-iotests/common.nbd > @@ -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. > nbd_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. > + 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?
On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > 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. :(
On 11/16/18 11:20 AM, Eric Blake wrote: > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: >> 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. >> --- >> +== 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. > 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. >> nbd_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. So, I'm planning to squash this in, to use ss instead of socat, as follows: diff --git i/tests/qemu-iotests/common.nbd w/tests/qemu-iotests/common.nbd index 0483ea7c55a..d73af285abd 100644 --- i/tests/qemu-iotests/common.nbd +++ w/tests/qemu-iotests/common.nbd @@ -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 diff --git i/tests/qemu-iotests/233.out w/tests/qemu-iotests/233.out index eaa410c2703..eb4077f9fd7 100644 --- i/tests/qemu-iotests/233.out +++ w/tests/qemu-iotests/233.out @@ -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.
On 11/17/18 2:49 PM, Eric Blake wrote: > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: >> 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. :( 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: Tested-by: Eric Blake <eblake@redhat.com>
On Fri, Nov 16, 2018 at 11:20:26AM -0600, Eric Blake wrote: > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > > 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? If you're happy to touch it up, that's fine with me. > > + > > +$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? I was mostly interested in the handshake/cert stuff, but yes, we could do some qemu-io testing too. > > + > > +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. 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. > > +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). > > > + > > +== 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. Again this was the socat polling. > > > +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 :) I'm unclear if this message comes from the server or the client since they are intermingled. > > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd > > index 61e9e90fee..0483ea7c55 100644 > > --- a/tests/qemu-iotests/common.nbd > > +++ b/tests/qemu-iotests/common.nbd > > @@ -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. Yeah, I wanted the earlier patch to be a plain refactor of existing functionality, not adding anything new. > > nbd_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. '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 > > + if test $? != 0 > > + then > > + nbd_tcp_port=$port Opps, a stray <tab> here > > + 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? Yes, we could simply skip. I figure 100 ports is fine, since tests are normally run on a largely idle system. Regards, Daniel
On Sat, Nov 17, 2018 at 03:31:34PM -0600, Eric Blake wrote: > On 11/16/18 11:20 AM, Eric Blake wrote: > > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > > > 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. > > > --- > > > > +== 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. > > > > 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. > > > > > nbd_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. > > 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. > diff --git i/tests/qemu-iotests/233.out w/tests/qemu-iotests/233.out > index eaa410c2703..eb4077f9fd7 100644 > --- i/tests/qemu-iotests/233.out > +++ w/tests/qemu-iotests/233.out > @@ -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. Opps, yes. Regards, Daniel
On Sat, Nov 17, 2018 at 02:49:22PM -0600, Eric Blake wrote: > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > > 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? Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > 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. :( 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
On 11/19/18 4:37 AM, Daniel P. Berrangé wrote: >> 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. >>> 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. >> >> 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.
On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > 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 > +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. :)
On Mon, Nov 19, 2018 at 11:00:38AM -0600, Eric Blake wrote: > On 11/19/18 4:37 AM, Daniel P. Berrangé wrote: > > > > 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. > > > > > 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. > > > > > > 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. Yes, that's ok with me. Regards, Daniel
Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben: > 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. > +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
On 11/20/18 11:27 AM, Kevin Wolf wrote: > Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben: >> 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. > >> +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...)
On Tue, Nov 20, 2018 at 11:45:54AM -0600, Eric Blake wrote: > On 11/20/18 11:27 AM, Kevin Wolf wrote: > > Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben: > > > 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. > > > > > +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...) FWIW certtool is part of gnutls-utils and is available on every platform that QEMU officially supports as a build target. Regards, Daniel
On 11/20/18 11:53 AM, Daniel P. Berrangé wrote: >>>> +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...) > > 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?)
Am 20.11.2018 um 19:22 hat Eric Blake geschrieben: > On 11/20/18 11:53 AM, Daniel P. Berrangé wrote: > > > > > > +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...) > > > > 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
On Tue, Nov 20, 2018 at 12:22:39PM -0600, Eric Blake wrote: > On 11/20/18 11:53 AM, Daniel P. Berrangé wrote: > > > > > > +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...) > > > > 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
diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233 new file mode 100755 index 0000000000..8f3d5bd013 --- /dev/null +++ b/tests/qemu-iotests/233 @@ -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 +owner=berrange@redhat.com + +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 diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out new file mode 100644 index 0000000000..eaa410c270 --- /dev/null +++ b/tests/qemu-iotests/233.out @@ -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 diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd index 61e9e90fee..0483ea7c55 100644 --- a/tests/qemu-iotests/common.nbd +++ b/tests/qemu-iotests/common.nbd @@ -20,6 +20,7 @@ # nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock" +nbd_tcp_addr="127.0.0.1" nbd_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 + 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 +} + +function nbd_server_wait_for_tcp_socket() +{ + pid=$1 + + 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 + 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" + exit 1 +} + +function nbd_server_start_tcp_socket() +{ + nbd_server_stop + $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port $@ & + nbd_server_wait_for_tcp_socket $! +} diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index ebe4fe78bc..e4327efa42 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -228,3 +228,4 @@ 229 auto quick 231 auto quick 232 auto quick +233 auto quick