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

[Eric Blake]

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.

> @@ -142,13 +146,16 @@ qemu-nbd -f qcow2 file.qcow2
>  @end example
>  
>  Start a long-running server listening with encryption on port 10810,
> -and require clients to have a correct X.509 certificate to connect to
> +and whitelist clients with a specific X.509 certificate to connect to
>  a 1 megabyte subset of a raw file, using the export name 'subset':
>  
>  @example
>  qemu-nbd \
>    --object tls-creds-x509,id=tls0,endpoint=server,dir=/path/to/qemutls \
> -  --tls-creds tls0 -t -x subset -p 10810 \
> +  --object 'authz-simple,id=auth0,identity=CN=laptop.example.com,,\
> +            O=Example Org,,L=London,,ST=London,,C=GB' \

A long line may be necessary here, unless the whitespace in the
identity= parameter inserted by the line continuation is harmless.  Long
lines in man pages are annoying, but even worse is an example that
copies-and-pastes incorrectly.  I may just s/^ *O/O/.


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

Reviewed-by: Eric Blake <address@hidden>

I can make the minor changes as part of staging through my NBD tree
without needing a v7.

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