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

[Eric Blake]

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 <address@hidden>

> +# creator
> address@hidden
> +
> +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?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org