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

2018-06-20 Thread Daniel P . Berrangé 
On Tue, Jun 19, 2018 at 03:06:06PM -0500, Eric Blake wrote:
> On 06/15/2018 10:50 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" 
> > 
> > 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 contains 'CN=fred', you would
> > use:
> > 
> >qemu-nbd -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
> > endpoint=server,verify-peer=yes \
> > -object authz-simple,id=authz0,policy=deny,\
> >rules.0.match=*CN=fred,rules.0.policy=allow \
> 
> s/-object/--object/g
> 
> > -tls-creds tls0 \
> > -tls-authz authz0
> 
> s/-tls/--tls/g
> 
> (qemu-nbd requires double-dash long-opts, -o means --offset except that
> 'bject' is not an offset; similarly for -t meaning --persistent)

Sigh, yes, just another reason for us to standardize on -- everywhere

> > +++ b/qemu-nbd.c
> 
> > @@ -533,6 +535,7 @@ int main(int argc, char **argv)
> >   { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
> >   { "trace", required_argument, NULL, 'T' },
> >   { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
> > +{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
> 
> Not your fault, but worth sorting these alphabetically?
> 
> Bummer that pre-patch, you could use '--tls' as an unambiguous abbreviation
> for --tls-creds; now it is an ambiguous prefix (you have to type --tls-c or
> --tls-a to get to the point of no ambiguity).  If we really cared, we could
> add:
> 
> { "t", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
> { "tl", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
> { "tls", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
> { "tls-", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
> 
> since getopt_long() no longer reports ambiguity if there is an exact match
> to what is otherwise the common prefix of two ambiguous options. But I don't
> think backwards-compatibility on this front is worth worrying about
> (generally, scripts don't rely on getopt_long()'s unambiguous prefix
> handling).

Personally I think this is not worth worrying about. We've never documented
ability to abbreviate nor ever promised they are stable. Abbreviations are
inherantly unstable as you illustrate, so if anything we should just document
that you should never abbreviate args.

> 
> > +++ b/qemu-nbd.texi
> > @@ -91,6 +91,10 @@ of the TLS credentials object previously created with 
> > the --object
> >   option.
> >   @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
> 
> s/qauthz/authz-simple/ ?

No, qauthz is a QOM interface, which is implemented by many subclasses,
of which authz-simple is just one example.

> > +--object option. This will be used to authorize users who
> > +connect against their x509 distinguished name.
> 
> Sounds like someone is "connecting against their name", rather than
> "authorizing against their name".  Better might be:
> 
> This will be used to authorize connecting users against their x509
> distinguished name.

Ok

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

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

2018-06-19 Thread Eric Blake 

On 06/15/2018 10:50 AM, Daniel P. Berrangé wrote:

From: "Daniel P. Berrange" 

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 contains 'CN=fred', you would
use:

   qemu-nbd -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
endpoint=server,verify-peer=yes \
-object authz-simple,id=authz0,policy=deny,\
   rules.0.match=*CN=fred,rules.0.policy=allow \


s/-object/--object/g


-tls-creds tls0 \
-tls-authz authz0


s/-tls/--tls/g

(qemu-nbd requires double-dash long-opts, -o means --offset except that 
'bject' is not an offset; similarly for -t meaning --persistent)



   other qemu-nbd args...

Signed-off-by: Daniel P. Berrange 
---
  include/block/nbd.h |  2 +-
  nbd/server.c| 12 +++-
  qemu-nbd.c  | 13 -
  qemu-nbd.texi   |  4 
  4 files changed, 24 insertions(+), 7 deletions(-)



+++ b/nbd/server.c



@@ -2153,7 +2153,9 @@ void nbd_client_new(NBDExport *exp,
  if (tlscreds) {
  object_ref(OBJECT(client->tlscreds));
  }
-client->tlsaclname = g_strdup(tlsaclname);
+if (tlsauthz) {
+client->tlsauthz = g_strdup(tlsauthz);
+}


The 'if' is pointless; g_strdup(NULL) is safe.



+++ b/qemu-nbd.c



@@ -533,6 +535,7 @@ int main(int argc, char **argv)
  { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
  { "trace", required_argument, NULL, 'T' },
  { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },


Not your fault, but worth sorting these alphabetically?

Bummer that pre-patch, you could use '--tls' as an unambiguous 
abbreviation for --tls-creds; now it is an ambiguous prefix (you have to 
type --tls-c or --tls-a to get to the point of no ambiguity).  If we 
really cared, we could add:


{ "t", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
{ "tl", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
{ "tls", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
{ "tls-", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },

since getopt_long() no longer reports ambiguity if there is an exact 
match to what is otherwise the common prefix of two ambiguous options. 
But I don't think backwards-compatibility on this front is worth 
worrying about (generally, scripts don't rely on getopt_long()'s 
unambiguous prefix handling).



+++ b/qemu-nbd.texi
@@ -91,6 +91,10 @@ of the TLS credentials object previously created with the 
--object
  option.
  @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


s/qauthz/authz-simple/ ?


+--object option. This will be used to authorize users who
+connect against their x509 distinguished name.


Sounds like someone is "connecting against their name", rather than 
"authorizing against their name".  Better might be:


This will be used to authorize connecting users against their x509 
distinguished name.


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

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

2018-06-15 Thread Daniel P . Berrangé 
From: "Daniel P. Berrange" 

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 contains 'CN=fred', you would
use:

  qemu-nbd -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
   endpoint=server,verify-peer=yes \
   -object authz-simple,id=authz0,policy=deny,\
   rules.0.match=*CN=fred,rules.0.policy=allow \
   -tls-creds tls0 \
   -tls-authz authz0
   other qemu-nbd args...

Signed-off-by: Daniel P. Berrange 
---
 include/block/nbd.h |  2 +-
 nbd/server.c| 12 +++-
 qemu-nbd.c  | 13 -
 qemu-nbd.texi   |  4 
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..80ea9d240c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -307,7 +307,7 @@ void nbd_export_close_all(void);
 void nbd_client_new(NBDExport *exp,
 QIOChannelSocket *sioc,
 QCryptoTLSCreds *tlscreds,
-const char *tlsaclname,
+const char *tlsauthz,
 void (*close_fn)(NBDClient *, bool));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
diff --git a/nbd/server.c b/nbd/server.c
index 9e1f227178..579fc828e1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -100,7 +100,7 @@ struct NBDClient {
 
 NBDExport *exp;
 QCryptoTLSCreds *tlscreds;
-char *tlsaclname;
+char *tlsauthz;
 QIOChannelSocket *sioc; /* The underlying data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
 
@@ -677,7 +677,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient 
*client,
 
 tioc = qio_channel_tls_new_server(ioc,
   client->tlscreds,
-  client->tlsaclname,
+  client->tlsauthz,
   errp);
 if (!tioc) {
 return NULL;
@@ -1250,7 +1250,7 @@ void nbd_client_put(NBDClient *client)
 if (client->tlscreds) {
 object_unref(OBJECT(client->tlscreds));
 }
-g_free(client->tlsaclname);
+g_free(client->tlsauthz);
 if (client->exp) {
 QTAILQ_REMOVE(>exp->clients, client, next);
 nbd_export_put(client->exp);
@@ -2140,7 +2140,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
 void nbd_client_new(NBDExport *exp,
 QIOChannelSocket *sioc,
 QCryptoTLSCreds *tlscreds,
-const char *tlsaclname,
+const char *tlsauthz,
 void (*close_fn)(NBDClient *, bool))
 {
 NBDClient *client;
@@ -2153,7 +2153,9 @@ void nbd_client_new(NBDExport *exp,
 if (tlscreds) {
 object_ref(OBJECT(client->tlscreds));
 }
-client->tlsaclname = g_strdup(tlsaclname);
+if (tlsauthz) {
+client->tlsauthz = g_strdup(tlsauthz);
+}
 client->sioc = sioc;
 object_ref(OBJECT(client->sioc));
 client->ioc = QIO_CHANNEL(sioc);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c72..c0c9c805c0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -52,6 +52,7 @@
 #define QEMU_NBD_OPT_TLSCREDS  261
 #define QEMU_NBD_OPT_IMAGE_OPTS262
 #define QEMU_NBD_OPT_FORK  263
+#define QEMU_NBD_OPT_TLSAUTHZ  264
 
 #define MBR_SIZE 512
 
@@ -66,6 +67,7 @@ static int shared = 1;
 static int nb_fds;
 static QIONetListener *server;
 static QCryptoTLSCreds *tlscreds;
+static const char *tlsauthz;
 
 static void usage(const char *name)
 {
@@ -355,7 +357,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,
 nb_fds++;
 nbd_update_server_watch();
 nbd_client_new(newproto ? NULL : exp, cioc,
-   tlscreds, NULL, nbd_client_closed);
+   tlscreds, tlsauthz, nbd_client_closed);
 }
 
 static void nbd_update_server_watch(void)
@@ -533,6 +535,7 @@ int main(int argc, char **argv)
 { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
 { "trace", required_argument, NULL, 'T' },
 { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+{ "tls-authz",