Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

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 :|