[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode |
Date: |
Mon, 19 Nov 2018 10:36:43 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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
--
|: 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, (continued)
Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file, Eric Blake, 2018/11/17
[Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode, Daniel P . Berrangé, 2018/11/16
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode,
Daniel P . Berrangé <=
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode, Eric Blake, 2018/11/17
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode, Daniel P . Berrangé, 2018/11/19
[Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS, Eric Blake, 2018/11/17
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode, Eric Blake, 2018/11/19
Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode, Kevin Wolf, 2018/11/20