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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Feb 27, 2019 at 08:25:20AM -0600, Eric Blake wrote:
> On 2/27/19 5:51 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > 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.

> > +    } 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?)

We should reject tls-creds when --list is set, since it only makes
sense on the server side, not client.

> > 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.
> > +@item --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.

Ok.

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

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

I'll have a look.

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

I'll do a v6 since there's still enough time before soft freeze.

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