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

[Eric Blake]

On 2/27/19 5:51 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <address@hidden>
> 
> Currently any client which can complete the TLS handshake is able to use
> the NBD server. The server admin can turn on the 'verify-peer' option
> for the x509 creds to require the client to provide a x509 certificate.
> This means the client will have to acquire a certificate from the CA
> before they are permitted to use the NBD server. This is still a fairly
> low bar to cross.
> 
> This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> takes the ID of a previously added 'QAuthZ' object instance. This will
> be used to validate the client's x509 distinguished name. Clients
> failing the authorization check will not be permitted to use the NBD
> server.
> 
> For example to setup authorization that only allows connection from a client
> whose x509 certificate distinguished name is
> 
>    CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> 
> escape the commas in the name and use:
> 
>   qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
>                     endpoint=server,verify-peer=yes \
>            --object 'authz-simple,id=auth0,identity=CN=laptop.example.com,,\
>                      O=Example Org,,L=London,,ST=London,,C=GB' \
>            --tls-creds tls0 \
>            --tls-authz authz0 \
>          ....other qemu-nbd args...
> 
> NB: a real shell command line would not have leading whitespace after
> the line continuation, it is just included here for clarity.

I'm half-tempted to propose a QemuOpts patch that eats whitespace after
comma, to make examples like this actually valid command lines.
(Actually, your example breaks line after an escaped comma, which would
result in a literal 'identity=CN=...,   O=Example...' option even if
space is eaten, and I don't know if that forms a valid identity).  But
doesn't hold up this patch.

> 
> Reviewed-by: Juan Quintela <address@hidden>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> +++ b/qemu-nbd.c

> +    } else {
> +        if (tlsauthz) {
> +            error_report("--tls-authz is not permitted without --tls-creds");
> +            exit(EXIT_FAILURE);
> +        }
>      }
>  
>      if (list) {

--list is new compared to when you originally proposed this patch. Does
anything need to be changed on that front?  (That is, does --tls-authz
make sense when qemu-nbd is used as a client to probe a remote server's
advertisements, or is it a server-only setting where we need to enhance
the --list mode to reject use of the option?)

> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index d0c5182814..d45ed9e0a0 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -117,6 +117,10 @@ option; or provide the credentials needed for connecting 
> as a client
>  in list mode.
>  @item --fork
>  Fork off the server process and exit the parent once the server is running.
> address@hidden --tls-authz=ID
> +Specify the ID of a qauthz object previously created with the
> +--object option. This will be used to authorize connecting users
> +against their x509 distinguished name.
>  @item -v, --verbose
>  Display extra debugging information.
>  @item -h, --help

qemu-nbd.texi now has examples (which it did not have when you first
wrote the patch).  I'd love it if your command line example from the
commit message were also included in the examples section of the man page.


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

>  
>  echo
> @@ -117,6 +122,26 @@ $QEMU_IO -c 'r -P 0x11 1m 1m' -c 'w -P 0x22 1m 1m' 
> --image-opts \
>  
>  $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
>  
> +echo
> +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?

Overall the patch looks good, but I'm wondering if we want a v6 to
address the items pointed out above, or if a followup patch is sufficient.

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