diff mbox series

[v2,2/6] nbd: allow authorization with nbd-server-start QMP command

Message ID 20180620121423.16979-3-berrange@redhat.com
State New
Headers show
Series Add authorization support to all network services | expand

Commit Message

Daniel P. Berrangé June 20, 2018, 12:14 p.m. UTC 
From: "Daniel P. Berrange" <berrange@redhat.com>

As with the previous patch to qemu-nbd, the nbd-server-start QMP command
also needs to be able to specify authorization when enabling TLS encryption.

First the client must create a QAuthZ object instance using the
'object-add' command:

   {
     'execute': 'object-add',
     'arguments': {
       'qom-type': 'authz-list',
       'id': 'authz0',
       'parameters': {
         'policy': 'deny',
         'rules': [
           {
             'match': '*CN=fred',
             'policy': 'allow'
           }
         ]
       }
     }
   }

They can then reference this in the new 'tls-authz' parameter when
executing the 'nbd-server-start' command:

   {
     'execute': 'nbd-server-start',
     'arguments': {
       'addr': {
           'type': 'inet',
           'host': '127.0.0.1',
           'port': '9000'
       },
       'tls-creds': 'tls0',
       'tls-authz': 'authz0'
     }
   }

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 blockdev-nbd.c      | 12 +++++++++---
 hmp.c               |  2 +-
 include/block/nbd.h |  2 +-
 qapi/block.json     |  8 +++++++-
 4 files changed, 18 insertions(+), 6 deletions(-)

Comments

Eric Blake June 20, 2018, 2:05 p.m. UTC | #1 
On 06/20/2018 07:14 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>

I thought you preferred the UTF-8 accent in your Author lines these 
days?  Or is this because this patch has been sitting around in your 
local repo prior to the point where you switched your git config author 
spelling? (Also applies to S-o-b in the series)

> 
> As with the previous patch to qemu-nbd, the nbd-server-start QMP command
> also needs to be able to specify authorization when enabling TLS encryption.
> 
> First the client must create a QAuthZ object instance using the
> 'object-add' command:
> 

> They can then reference this in the new 'tls-authz' parameter when
> executing the 'nbd-server-start' command:
> 

> @@ -132,11 +137,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
>   
>   void qmp_nbd_server_start(SocketAddressLegacy *addr,
>                             bool has_tls_creds, const char *tls_creds,
> +                          bool has_tls_authz, const char *tls_authz,
>                             Error **errp)
>   {
>       SocketAddress *addr_flat = socket_address_flatten(addr);
>   
> -    nbd_server_start(addr_flat, tls_creds, errp);
> +    nbd_server_start(addr_flat, tls_creds, tls_authz, errp);

Relies on QMP generated code setting tls_authz = NULL if has_tls_authz 
is false (but no different than the fact that we already relied on it 
for tls_creds).  Someday it would be nice to get rid of the has_FOO for 
optional strings, but that's not your problem.

> +++ b/qapi/block.json
> @@ -197,6 +197,11 @@
>   #
>   # @addr: Address on which to listen.
>   # @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
> +# @tls-authz: ID of the QAuthZ authorization object used to validate
> +#             the client's x509 distinguished name. This object is
> +#             is only resolved at time of use, so can be deleted and
> +#             recreated on the fly while the NBD server is active.
> +#             If missing, it will default to denying access. Since 3.0
>   #
>   # Returns: error if the server is already running.
>   #
> @@ -204,7 +209,8 @@
>   ##
>   { 'command': 'nbd-server-start',
>     'data': { 'addr': 'SocketAddressLegacy',
> -            '*tls-creds': 'str'} }
> +            '*tls-creds': 'str',
> +            '*tls-authz': 'str'} }

Reviewed-by: Eric Blake <eblake@redhat.com>

Although patch 1 and 2 touch NBD, I'm happy for Dan to be the one that 
merges it as part of the larger series.
Daniel P. Berrangé June 20, 2018, 2:07 p.m. UTC | #2 
On Wed, Jun 20, 2018 at 09:05:32AM -0500, Eric Blake wrote:
> On 06/20/2018 07:14 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> I thought you preferred the UTF-8 accent in your Author lines these days?
> Or is this because this patch has been sitting around in your local repo
> prior to the point where you switched your git config author spelling? (Also
> applies to S-o-b in the series)

Yeah its "only" been sitting in my tree since late 2016 :-)

> 
> > 
> > As with the previous patch to qemu-nbd, the nbd-server-start QMP command
> > also needs to be able to specify authorization when enabling TLS encryption.
> > 
> > First the client must create a QAuthZ object instance using the
> > 'object-add' command:
> > 
> 
> > They can then reference this in the new 'tls-authz' parameter when
> > executing the 'nbd-server-start' command:
> > 
> 
> > @@ -132,11 +137,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
> >   void qmp_nbd_server_start(SocketAddressLegacy *addr,
> >                             bool has_tls_creds, const char *tls_creds,
> > +                          bool has_tls_authz, const char *tls_authz,
> >                             Error **errp)
> >   {
> >       SocketAddress *addr_flat = socket_address_flatten(addr);
> > -    nbd_server_start(addr_flat, tls_creds, errp);
> > +    nbd_server_start(addr_flat, tls_creds, tls_authz, errp);
> 
> Relies on QMP generated code setting tls_authz = NULL if has_tls_authz is
> false (but no different than the fact that we already relied on it for
> tls_creds).  Someday it would be nice to get rid of the has_FOO for optional
> strings, but that's not your problem.
> 
> > +++ b/qapi/block.json
> > @@ -197,6 +197,11 @@
> >   #
> >   # @addr: Address on which to listen.
> >   # @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
> > +# @tls-authz: ID of the QAuthZ authorization object used to validate
> > +#             the client's x509 distinguished name. This object is
> > +#             is only resolved at time of use, so can be deleted and
> > +#             recreated on the fly while the NBD server is active.
> > +#             If missing, it will default to denying access. Since 3.0
> >   #
> >   # Returns: error if the server is already running.
> >   #
> > @@ -204,7 +209,8 @@
> >   ##
> >   { 'command': 'nbd-server-start',
> >     'data': { 'addr': 'SocketAddressLegacy',
> > -            '*tls-creds': 'str'} }
> > +            '*tls-creds': 'str',
> > +            '*tls-authz': 'str'} }
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Although patch 1 and 2 touch NBD, I'm happy for Dan to be the one that
> merges it as part of the larger series.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Regards,
Daniel
diff mbox series

Patch

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 65a84739ed..2627379ede 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -23,6 +23,7 @@ 
 typedef struct NBDServerData {
     QIONetListener *listener;
     QCryptoTLSCreds *tlscreds;
+    char *tlsauthz;
 } NBDServerData;
 
 static NBDServerData *nbd_server;
@@ -37,7 +38,8 @@  static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
 {
     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
     nbd_client_new(NULL, cioc,
-                   nbd_server->tlscreds, NULL,
+                   nbd_server->tlscreds,
+                   nbd_server->tlsauthz,
                    nbd_blockdev_client_closed);
 }
 
@@ -53,6 +55,7 @@  static void nbd_server_free(NBDServerData *server)
     if (server->tlscreds) {
         object_unref(OBJECT(server->tlscreds));
     }
+    g_free(server->tlsauthz);
 
     g_free(server);
 }
@@ -88,7 +91,7 @@  static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
 
 
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      Error **errp)
+                      const char *tls_authz, Error **errp)
 {
     if (nbd_server) {
         error_setg(errp, "NBD server already running");
@@ -118,6 +121,8 @@  void nbd_server_start(SocketAddress *addr, const char *tls_creds,
         }
     }
 
+    nbd_server->tlsauthz = g_strdup(tls_authz);
+
     qio_net_listener_set_client_func(nbd_server->listener,
                                      nbd_accept,
                                      NULL,
@@ -132,11 +137,12 @@  void nbd_server_start(SocketAddress *addr, const char *tls_creds,
 
 void qmp_nbd_server_start(SocketAddressLegacy *addr,
                           bool has_tls_creds, const char *tls_creds,
+                          bool has_tls_authz, const char *tls_authz,
                           Error **errp)
 {
     SocketAddress *addr_flat = socket_address_flatten(addr);
 
-    nbd_server_start(addr_flat, tls_creds, errp);
+    nbd_server_start(addr_flat, tls_creds, tls_authz, errp);
     qapi_free_SocketAddress(addr_flat);
 }
 
diff --git a/hmp.c b/hmp.c
index f40d8279cf..b93a871830 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2221,7 +2221,7 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }
 
-    nbd_server_start(addr, NULL, &local_err);
+    nbd_server_start(addr, NULL, NULL, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 80ea9d240c..8a8ae8c3a7 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -313,7 +313,7 @@  void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
 
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      Error **errp);
+                      const char *tls_authz, Error **errp);
 
 
 /* nbd_read
diff --git a/qapi/block.json b/qapi/block.json
index c694524002..9403d45d20 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -197,6 +197,11 @@ 
 #
 # @addr: Address on which to listen.
 # @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
+# @tls-authz: ID of the QAuthZ authorization object used to validate
+#             the client's x509 distinguished name. This object is
+#             is only resolved at time of use, so can be deleted and
+#             recreated on the fly while the NBD server is active.
+#             If missing, it will default to denying access. Since 3.0
 #
 # Returns: error if the server is already running.
 #
@@ -204,7 +209,8 @@ 
 ##
 { 'command': 'nbd-server-start',
   'data': { 'addr': 'SocketAddressLegacy',
-            '*tls-creds': 'str'} }
+            '*tls-creds': 'str',
+            '*tls-authz': 'str'} }
 
 ##
 # @nbd-server-add: