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

[Daniel P . Berrangé]

On Wed, Feb 27, 2019 at 10:43:40AM -0600, Eric Blake wrote:
> On 2/27/19 10:20 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.
> > 
> 
> > +++ b/qemu-nbd.c
> 
> > @@ -103,6 +105,7 @@ static void usage(const char *name)
> >  "  --object type,id=ID,...   define an object such as 'secret' for 
> > providing\n"
> >  "                            passwords and/or encryption keys\n"
> >  "  --tls-creds=ID            use id of an earlier --object to provide 
> > TLS\n"
> > +"  --tls-authz=ID            use id of an earlier --object to provide 
> > authorization\n"
> 
> Usage line exceeds 80 columns; I don't mind splitting the line.

Odd that checkpatch.pl didn't complain about this.

> > +== check TLS with authorization ==
> > +qemu-img: Could not open 
> > 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read 
> > option reply: Cannot read from TLS channel: Software caused connection abort
> > +qemu-img: Could not open 
> > 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read 
> > option reply: Cannot read from TLS channel: Software caused connection abort
> 
> A rather uninformative message for the client to figure out why it
> failed, but (as with all things security-related), giving too many
> details may in itself be an improper information leak. At any rate, I
> don't know that you could make it work any better, so it is not a
> problem with this patch.  It may be the sign of a server bug for closing
> the socket too soon (before the client has a chance to read an actual
> error message), where we could tweak things to provoke a nicer error
> than 'Software caused connection abort', but that would be a separate patch.

I'm not sure how we'd do anything better here. From NBD's pov the TLS
handshake failed to complete. At that point NBD can't assume that it is
able to successfully able to send anything over the connection, so has
little choice but to close it.

To some extent this is a result of the QIOChannelTLS API design, because
in acutal fact we've completed the TLS protocol handshake, so we do
actually have an established TLS connection, but we don't consider the
client authorized.

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