Re: [Qemu-block] [Qemu-devel] [PATCH v5 1/2] qemu-nbd: add support for authorization of TLS clients

[Eric Blake]

On 2/27/19 9:58 AM, Daniel P. Berrangé wrote:

>>>>  echo "== check TLS works =="
>>>> -obj=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
>>>> -$QEMU_IMG info --image-opts --object $obj \
>>>> +obj1=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
>>>> +obj2=tls-creds-x509,dir=${tls_dir}/client3,endpoint=client,id=tls0
>>>> +$QEMU_IMG info --image-opts --object $obj1 \
>>>>      driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
>>>>      2>&1 | sed "s/$nbd_tcp_port/PORT/g"
>>>> -$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj \
>>>> +$QEMU_IMG info --image-opts --object $obj2 \
>>>> +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
>>>> +    2>&1 | sed "s/$nbd_tcp_port/PORT/g"
>>>> +$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj1 \
>>>>      --tls-creds=tls0
>>>
>>> It looks like you have both a positive and negative test that a qemu-img
>>> client can only connect with the right auth, but only a positive test
>>> for a qemu-nbd --list client.  Should we also duplicate the
>>> QEMU_NBD_PROG line to use $obj2 for a negative test?
>>
>> I can look at that
> 
> On reflection I don't think that's needed. This is a server side feature we
> are validating. As such it is sufficient to validate with any client app
> supporting TLS. IOW, qemu-img is sufficient to prove the server side feature
> is working correctly.

There's two things being validated in this test: does the server
properly honor authorization (accepting/denying clients as appropriate),
and does the client properly expose a way to pass correct credentials.
You're right that once we've tested that the server can reject at least
one client lacking credentials, we don't need any more negative tests
(the server will reject any other client without credentials); and as
long as both clients that we care about (qemu-img, qemu-nbd --list) have
at least one positive test, then we've covered that each important
client has a way to provide the required credentials.

Okay, I can live with your claim that we have sufficient coverage, and
don't need anything more with --list (for this test, where the server is
accepting regardless of client certificate).

>>>> +echo "== check TLS with authorization =="
>>>> +
>>>> +nbd_server_stop
>>>> +
>>>> +nbd_server_start_tcp_socket \
>>>> +    --object 
>>>> tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes
>>>>  \
>>>> +    --object "authz-simple,identity=CN=localhost,,O=Cthulu Dark Lord 
>>>> Enterprises client1,,L=R'lyeh,,C=South Pacific,id=authz0" \
>>>> +    --tls-authz authz0 \
>>>> +    --tls-creds tls0 \
>>>> +    -f $IMGFMT "$TEST_IMG" 2>> "$TEST_DIR/server.log"
>>>> +
>>>> +$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
>>>> +
>>>> +$QEMU_IMG info --image-opts \
>>>> +    --object 
>>>> tls-creds-x509,dir=${tls_dir}/client3,endpoint=client,id=tls0 \
>>>> +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
>>>
>>> Similarly, do we want --list testing here?
>>
>> I'll have a look.
> 
> For same reason as above, we don't need to repeat with qemu-nbd --list

Here, it's a bit fuzzier - now that the server DOES require the client
to use a correct certificate (you're showing that the server accepts
client 1 but rejects client 2, because they used a different
certificate).  But, since client 1 is the same here as it was before,
and the earlier test showed how to let qemu-nbd --list as client give
the same credentials as before, I can also live with the notion that we
don't need a --list test here (the server being stricter about
certificates doesn't change that the client can still pass the correct
certificate).

Okay.

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