[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/2] qemu-nbd: add support for authorization
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/2] qemu-nbd: add support for authorization of TLS clients |
Date: |
Wed, 27 Feb 2019 15:33:31 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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" <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.
> > + } 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.
> > 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.
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 :|