[Qemu-devel] [PATCH v2 0/6] Add authorization support to all network services

[Qemu-devel] [PATCH v2 0/6] Add authorization support to all network services

[Daniel P. Berrangé]

This series builds on the core authorization framework:

  https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04469.html

enabling its use with the VNC, chardev, NBD and migration network servers.

In combination with TLS x509 client certificates, this allows these
services to whitelist specific clients, which avoids the need to setup
restricted child certificate authorities.

In VNC it also allows whitelisting based on SASL user names.

Changed in v2:

 - Document that authz objects are resolved at time of use, not
   time of network service activation
 - Improve docs for tls-authz parameters on services
 - Fix 2.13 -> 3.0 version tags
 - Remove redundant conditionals around g_strdup
 - Fix arg syntax for qemu-nbd  s/-/--/
 - Remove QAPI (optional) annotation
 - Fix some outdated usage example

Based-on: <20180620103555.1342-1-berrange@redhat.com>

Daniel P. Berrangé (6):
  qemu-nbd: add support for authorization of TLS clients
  nbd: allow authorization with nbd-server-start QMP command
  migration: add support for a "tls-authz" migration parameter
  chardev: add support for authorization for TLS clients
  vnc: allow specifying a custom authorization object name
  monitor: deprecate acl_show, acl_reset, acl_policy, acl_add,
    acl_remove

 blockdev-nbd.c        | 12 ++++++---
 chardev/char-socket.c | 11 +++++++-
 chardev/char.c        |  3 +++
 hmp.c                 | 11 +++++++-
 include/block/nbd.h   |  4 +--
 migration/migration.c |  8 ++++++
 migration/tls.c       |  2 +-
 monitor.c             | 23 +++++++++++++++++
 nbd/server.c          | 10 ++++----
 qapi/block.json       |  8 +++++-
 qapi/char.json        |  6 +++++
 qapi/migration.json   | 14 ++++++++++-
 qemu-doc.texi         | 13 ++++++++++
 qemu-nbd.c            | 13 +++++++++-
 qemu-nbd.texi         |  4 +++
 qemu-options.hx       | 44 +++++++++++++++++++++++---------
 ui/vnc.c              | 58 ++++++++++++++++++++++++++++++++++++-------
 17 files changed, 207 insertions(+), 37 deletions(-)

-- 
2.17.0

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

[Daniel P. Berrangé]

From: "Daniel P. Berrange" <berrange@redhat.com>

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

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/block/nbd.h |  2 +-
 nbd/server.c        | 10 +++++-----
 qemu-nbd.c          | 13 ++++++++++++-
 qemu-nbd.texi       |  4 ++++
 4 files changed, 22 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..4f10f08dc0 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(&client->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,7 @@ void nbd_client_new(NBDExport *exp,
     if (tlscreds) {
         object_ref(OBJECT(client->tlscreds));
     }
-    client->tlsaclname = g_strdup(tlsaclname);
+    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_OPTS    262
 #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", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -755,6 +758,9 @@ int main(int argc, char **argv)
             g_free(trace_file);
             trace_file = trace_opt_parse(optarg);
             break;
+        case QEMU_NBD_OPT_TLSAUTHZ:
+            tlsauthz = optarg;
+            break;
         case QEMU_NBD_OPT_FORK:
             fork_process = true;
             break;
@@ -819,6 +825,11 @@ int main(int argc, char **argv)
                          error_get_pretty(local_err));
             exit(EXIT_FAILURE);
         }
+    } else {
+        if (tlsauthz) {
+            error_report("--tls-authz is not permitted without --tls-creds");
+            exit(EXIT_FAILURE);
+        }
     }
 
     if (disconnect) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed..7f9503cf05 100644
--- a/qemu-nbd.texi
+++ 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
+--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
-- 
2.17.0

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

[Daniel P. Berrangé]

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(-)

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

[Qemu-devel] [PATCH v2 3/6] migration: add support for a "tls-authz" migration parameter

[Daniel P. Berrangé]

From: "Daniel P. Berrange" <berrange@redhat.com>

The QEMU instance that runs as the server for the migration data
transport (ie the target QEMU) needs to be able to configure access
control so it can prevent unauthorized clients initiating an incoming
migration. This adds a new 'tls-authz' migration parameter that is used
to provide the QOM ID of a QAuthZ subclass instance that provides the
access control check. This is checked against the x509 certificate
obtained during the TLS handshake.

For example, when starting a QEMU for incoming migration, it is
possible to give an example identity of the source QEMU that is
intended to be connecting later:

  $QEMU \
     -monitor stdio \
     -incoming defer \
     ...other args...

  (qemu) object_add tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
             endpoint=server,verify-peer=yes \
  (qemu) object_add authz-simple,id=auth0,identity=CN=laptop.example.com,,\
             O=Example Org,,L=London,,ST=London,,C=GB \
  (qemu) migrate_incoming tcp:localhost:9000

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hmp.c                 |  9 +++++++++
 migration/migration.c |  8 ++++++++
 migration/tls.c       |  2 +-
 qapi/migration.json   | 14 +++++++++++++-
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index b93a871830..b0a7f3a2fb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -373,6 +373,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
             params->max_postcopy_bandwidth);
+        monitor_printf(mon, " %s: '%s'\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
+            params->has_tls_authz ? params->tls_authz : "");
     }
 
     qapi_free_MigrationParameters(params);
@@ -1635,6 +1638,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->tls_hostname->type = QTYPE_QSTRING;
         visit_type_str(v, param, &p->tls_hostname->u.s, &err);
         break;
+    case MIGRATION_PARAMETER_TLS_AUTHZ:
+        p->has_tls_authz = true;
+        p->tls_authz = g_new0(StrOrNull, 1);
+        p->tls_authz->type = QTYPE_QSTRING;
+        visit_type_str(v, param, &p->tls_authz->u.s, &err);
+        break;
     case MIGRATION_PARAMETER_MAX_BANDWIDTH:
         p->has_max_bandwidth = true;
         /*
diff --git a/migration/migration.c b/migration/migration.c
index e1eaa97df4..40df9a9bc3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -650,6 +650,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->has_tls_hostname = true;
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
+    params->has_tls_authz = true;
+    params->tls_authz = g_strdup(s->parameters.tls_authz);
     params->has_max_bandwidth = true;
     params->max_bandwidth = s->parameters.max_bandwidth;
     params->has_downtime_limit = true;
@@ -1116,6 +1118,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
     }
 
+    if (params->has_tls_authz) {
+        g_free(s->parameters.tls_authz);
+        assert(params->tls_authz->type == QTYPE_QSTRING);
+        s->parameters.tls_authz = g_strdup(params->tls_authz->u.s);
+    }
+
     if (params->has_max_bandwidth) {
         s->parameters.max_bandwidth = params->max_bandwidth;
         if (s->to_dst_file) {
diff --git a/migration/tls.c b/migration/tls.c
index 3b9e8c9263..5171afc6c4 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -94,7 +94,7 @@ void migration_tls_channel_process_incoming(MigrationState *s,
 
     tioc = qio_channel_tls_new_server(
         ioc, creds,
-        NULL, /* XXX pass ACL name */
+        s->parameters.tls_authz,
         errp);
     if (!tioc) {
         return;
diff --git a/qapi/migration.json b/qapi/migration.json
index 1b4c1db670..3fe0d0c949 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -488,6 +488,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @tls-authz: ID of the 'authz' object subclass that provides access control
+#             checking of the TLS x509 certificate distinguished name.
+#             This object is only resolved at time of use, so can be deleted
+#             and recreated on the fly while the migration server is active.
+#             If missing, it will default to denying access (Since 3.0)
+#
 # @max-bandwidth: to set maximum speed for migration. maximum speed in
 #                 bytes per second. (Since 2.8)
 #
@@ -525,7 +531,7 @@
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname', 'max-bandwidth',
+           'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
            'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
            'x-multifd-channels', 'x-multifd-page-count',
            'xbzrle-cache-size', 'max-postcopy-bandwidth' ] }
@@ -612,6 +618,7 @@
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
+            '*tls-authz': 'StrOrNull',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
             '*x-checkpoint-delay': 'int',
@@ -675,6 +682,10 @@
 #                associated with the migration URI, if any. (Since 2.9)
 #                Note: 2.8 reports this by omitting tls-hostname instead.
 #
+# @tls-authz: ID of the 'authz' object subclass that provides access control
+#             checking of the TLS x509 certificate distinguished name. (Since
+#             3.0)
+#
 # @max-bandwidth: to set maximum speed for migration. maximum speed in
 #                 bytes per second. (Since 2.8)
 #
@@ -716,6 +727,7 @@
             '*cpu-throttle-increment': 'uint8',
             '*tls-creds': 'str',
             '*tls-hostname': 'str',
+            '*tls-authz': 'str',
             '*max-bandwidth': 'size',
             '*downtime-limit': 'uint64',
             '*x-checkpoint-delay': 'uint32',
-- 
2.17.0

[Qemu-devel] [PATCH v2 4/6] chardev: add support for authorization for TLS clients

[Daniel P. Berrangé]

From: "Daniel P. Berrange" <berrange@redhat.com>

Currently any client which can complete the TLS handshake is able to use
a chardev 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 chardev server. This is
still a fairly low bar.

This adds a 'tls-authz=OBJECT-ID' option to the socket chardev backend
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 check will not be permitted to use the chardev 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 -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
                endpoint=server,verify-peer=yes \
        -object authz-simple,id=authz0,identity=CN=laptop.example.com,,\
                O=Example Org,,L=London,,ST=London,,C=GB \
        -chardev socket,host=127.0.0.1,port=9000,server,\
	         tls-creds=tls0,tls-authz=authz0 \
        ...other qemu args...

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 chardev/char-socket.c | 11 ++++++++++-
 chardev/char.c        |  3 +++
 qapi/char.json        |  6 ++++++
 qemu-options.hx       |  9 +++++++--
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 159e69c3b1..d0c9d26025 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -53,6 +53,7 @@ typedef struct {
     QIONetListener *listener;
     GSource *hup_source;
     QCryptoTLSCreds *tls_creds;
+    char *tls_authz;
     int connected;
     int max_size;
     int do_telnetopt;
@@ -729,7 +730,7 @@ static void tcp_chr_tls_init(Chardev *chr)
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
-            NULL, /* XXX Use an ACL */
+            s->tls_authz,
             &err);
     } else {
         tioc = qio_channel_tls_new_client(
@@ -881,6 +882,7 @@ static void char_socket_finalize(Object *obj)
     if (s->tls_creds) {
         object_unref(OBJECT(s->tls_creds));
     }
+    g_free(s->tls_authz);
 
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
@@ -986,6 +988,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
             }
         }
     }
+    s->tls_authz = g_strdup(sock->tls_authz);
 
     s->addr = addr = socket_address_flatten(sock->addr);
 
@@ -1066,6 +1069,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     const char *fd = qemu_opt_get(opts, "fd");
     const char *tls_creds = qemu_opt_get(opts, "tls-creds");
     SocketAddressLegacy *addr;
+    const char *tls_authz = qemu_opt_get(opts, "tls-authz");
     ChardevSocket *sock;
 
     if ((!!path + !!fd + !!host) != 1) {
@@ -1094,6 +1098,10 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     } else {
         g_assert_not_reached();
     }
+    if (tls_authz && !tls_creds) {
+        error_setg(errp, "Authorization can only be used when TLS is enabled");
+        return;
+    }
 
     sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
     qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
@@ -1111,6 +1119,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->has_reconnect = true;
     sock->reconnect = reconnect;
     sock->tls_creds = g_strdup(tls_creds);
+    sock->tls_authz = g_strdup(tls_authz);
 
     addr = g_new0(SocketAddressLegacy, 1);
     if (path) {
diff --git a/chardev/char.c b/chardev/char.c
index 76d866e6fe..d28c16ccca 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -837,6 +837,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "tls-creds",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "tls-authz",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "width",
             .type = QEMU_OPT_NUMBER,
diff --git a/qapi/char.json b/qapi/char.json
index ae19dcd1ed..a4eeecce88 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -242,6 +242,11 @@
 # @addr: socket address to listen on (server=true)
 #        or connect to (server=false)
 # @tls-creds: the ID of the TLS credentials object (since 2.6)
+# @tls-authz: the ID of the QAuthZ authorization object against which
+#             the client's x509 distinguished name will validated. This
+#             object is only resolved at time of use, so can be deleted
+#             and recreated on the fly while the chardev server is active.
+#             If missing, it will default to denying access (since 3.0)
 # @server: create server socket (default: true)
 # @wait: wait for incoming connection on server
 #        sockets (default: false).
@@ -259,6 +264,7 @@
 ##
 { 'struct': 'ChardevSocket', 'data': { 'addr'       : 'SocketAddressLegacy',
                                      '*tls-creds'  : 'str',
+                                     '*tls-authz'  : 'str',
                                      '*server'    : 'bool',
                                      '*wait'      : 'bool',
                                      '*nodelay'   : 'bool',
diff --git a/qemu-options.hx b/qemu-options.hx
index fb7ad0740c..b7337da782 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2487,7 +2487,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
     "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay][,reconnect=seconds]\n"
     "         [,server][,nowait][,telnet][,reconnect=seconds][,mux=on|off]\n"
-    "         [,logfile=PATH][,logappend=on|off][,tls-creds=ID] (tcp)\n"
+    "         [,logfile=PATH][,logappend=on|off][,tls-creds=ID][,tls-authz=ID], (tcp)\n"
     "-chardev socket,id=id,path=path[,server][,nowait][,telnet][,reconnect=seconds]\n"
     "         [,mux=on|off][,logfile=PATH][,logappend=on|off] (unix)\n"
     "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
@@ -2616,7 +2616,7 @@ The available backends are:
 A void device. This device will not emit any data, and will drop any data it
 receives. The null backend does not take any options.
 
-@item -chardev socket,id=@var{id}[,@var{TCP options} or @var{unix options}][,server][,nowait][,telnet][,reconnect=@var{seconds}][,tls-creds=@var{id}]
+@item -chardev socket,id=@var{id}[,@var{TCP options} or @var{unix options}][,server][,nowait][,telnet][,reconnect=@var{seconds}][,tls-creds=@var{id}][,tls-authz=@var{id}]
 
 Create a two-way stream socket, which can be either a TCP or a unix socket. A
 unix socket will be created if @option{path} is specified. Behaviour is
@@ -2639,6 +2639,11 @@ and specifies the id of the TLS credentials to use for the handshake. The
 credentials must be previously created with the @option{-object tls-creds}
 argument.
 
+@option{tls-auth} provides the ID of the QAuthZ authorization object against
+which the client's x509 distinguished name will validated. This object is only
+resolved at time of use, so can be deleted and recreated on the fly while the
+chardev server is active. If missing, it will default to denying access.
+
 TCP and unix socket options are given below:
 
 @table @option
-- 
2.17.0

[Qemu-devel] [PATCH v2 5/6] vnc: allow specifying a custom authorization object name

[Daniel P. Berrangé]

From: "Daniel P. Berrange" <berrange@redhat.com>

The VNC server has historically had support for ACLs to check both the
SASL username and the TLS x509 distinguished name. The VNC server was
responsible for creating the initial ACL, and the client app was then
responsible for populating it with rules using the HMP 'acl_add' command.

This is not satisfactory for a variety of reasons. There is no way to
populate the ACLs from the command line, users are forced to use the
HMP. With multiple network services all supporting TLS and ACLs now, it
is desirable to be able to define a single ACL that is referenced by all
services.

To address these limitations, two new options are added to the VNC
server CLI. The 'tls-authz' option takes the ID of a QAuthZ object to
use for checking TLS x509 distinguished names, and the 'sasl-authz'
option takes the ID of another object to use for checking SASL usernames.

In this example, we setup two authorization rules. The first allows any
client with a certificate issued by the 'RedHat' organization in the
'London' locality. The second ACL allows clients with either the
'joe@REDHAT.COM' or  'fred@REDHAT.COM' kerberos usernames. Both checks
must pass for the user to be allowed.

    $QEMU -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=O=RedHat,,L=London,rules.0.policy=allow \
          -object authz-simple,id=authz1,policy=deny,\
                  rules.0.match=fred@REDHAT.COM,rules.0.policy=allow \
                  rules.0.match=joe@REDHAT.COM,rules.0.policy=allow \
          -vnc 0.0.0.0:1,tls-creds=tls0,tls-authz=authz0,
	       sasl,sasl-authz=authz1 \
          ...other QEMU args...

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-doc.texi   |  5 +++++
 qemu-options.hx | 35 ++++++++++++++++++++---------
 ui/vnc.c        | 58 +++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 282bc3dc35..5639b7ecdb 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2912,6 +2912,11 @@ Option @option{-virtioconsole} has been replaced by
 The @code{-clock} option is ignored since QEMU version 1.7.0. There is no
 replacement since it is not needed anymore.
 
+@subsection -vnc acl (since 3.0.0)
+
+The @code{acl} option to the @code{-vnc} argument has been replaced
+by the @code{tls-authz} and @code{sasl-authz} options.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index b7337da782..d7a87e8b1e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1647,6 +1647,14 @@ The @option{tls-creds} parameter obsoletes the @option{tls},
 it is not permitted to set both new and old type options at
 the same time.
 
+@item tls-authz=@var{ID}
+
+Provides the ID of the QAuthZ authorization object against which
+the client's x509 distinguished name will validated. This object is
+only resolved at time of use, so can be deleted and recreated on the
+fly while the VNC server is active. If missing, it will default
+to denying access.
+
 @item tls
 
 Require that client use TLS when communicating with the VNC server. This
@@ -1700,18 +1708,25 @@ ensures a data encryption preventing compromise of authentication
 credentials. See the @ref{vnc_security} section for details on using
 SASL authentication.
 
+@item sasl-authz=@var{ID}
+
+Provides the ID of the QAuthZ authorization object against which
+the client's SASL username will validated. This object is
+only resolved at time of use, so can be deleted and recreated on the
+fly while the VNC server is active. If missing, it will default
+to denying access.
+
 @item acl
 
-Turn on access control lists for checking of the x509 client certificate
-and SASL party. For x509 certs, the ACL check is made against the
-certificate's distinguished name. This is something that looks like
-@code{C=GB,O=ACME,L=Boston,CN=bob}. For SASL party, the ACL check is
-made against the username, which depending on the SASL plugin, may
-include a realm component, eg @code{bob} or @code{bob@@EXAMPLE.COM}.
-When the @option{acl} flag is set, the initial access list will be
-empty, with a @code{deny} policy. Thus no one will be allowed to
-use the VNC server until the ACLs have been loaded. This can be
-achieved using the @code{acl} monitor command.
+Legacy method for enabling authorization of clients against the
+x509 distinguished name and SASL username. It results in the creation
+of two @code{authz-list} objects with IDs of @code{vnc.username} and
+@code{vnc.x509dname}. The rules for these objects must be configured
+with the HMP ACL commands.
+
+This option is deprecated and should no longer be used. The new
+@option{sasl-authz} and @option{tls-authz} options are a
+replacement.
 
 @item lossy
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 9fb8430c35..2da9433ca7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3407,6 +3407,12 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "acl",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "tls-authz",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "sasl-authz",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "lossy",
             .type = QEMU_OPT_BOOL,
@@ -3894,6 +3900,8 @@ void vnc_display_open(const char *id, Error **errp)
     int saslErr;
 #endif
     int acl = 0;
+    const char *tlsauthz;
+    const char *saslauthz;
     int lock_key_sync = 1;
     int key_delay_ms;
 
@@ -3999,7 +4007,33 @@ void vnc_display_open(const char *id, Error **errp)
             }
         }
     }
+    if (qemu_opt_get(opts, "acl")) {
+        error_report("The 'acl' option to -vnc is deprecated. "
+                     "Please use the 'tls-authz' and 'sasl-authz' "
+                     "options instead");
+    }
     acl = qemu_opt_get_bool(opts, "acl", false);
+    tlsauthz = qemu_opt_get(opts, "tls-authz");
+    if (acl && tlsauthz) {
+        error_setg(errp, "'acl' option is mutually exclusive with the "
+                   "'tls-authz' option");
+        goto fail;
+    }
+    if (tlsauthz && !vd->tlscreds) {
+        error_setg(errp, "'tls-authz' provided but TLS is not enabled");
+        goto fail;
+    }
+
+    saslauthz = qemu_opt_get(opts, "sasl-authz");
+    if (acl && saslauthz) {
+        error_setg(errp, "'acl' option is mutually exclusive with the "
+                   "'sasl-authz' option");
+        goto fail;
+    }
+    if (saslauthz && !sasl) {
+        error_setg(errp, "'sasl-authz' provided but SASL auth is not enabled");
+        goto fail;
+    }
 
     share = qemu_opt_get(opts, "share");
     if (share) {
@@ -4029,7 +4063,9 @@ void vnc_display_open(const char *id, Error **errp)
         vd->non_adaptive = true;
     }
 
-    if (acl) {
+    if (tlsauthz) {
+        vd->tlsauthzid = g_strdup(tlsauthz);
+    } else if (acl) {
         if (strcmp(vd->id, "default") == 0) {
             vd->tlsauthzid = g_strdup("vnc.x509dname");
         } else {
@@ -4040,15 +4076,19 @@ void vnc_display_open(const char *id, Error **errp)
                                               &error_abort));
     }
 #ifdef CONFIG_VNC_SASL
-    if (acl && sasl) {
-        if (strcmp(vd->id, "default") == 0) {
-            vd->sasl.authzid = g_strdup("vnc.username");
-        } else {
-            vd->sasl.authzid = g_strdup_printf("vnc.%s.username", vd->id);
+    if (sasl) {
+        if (saslauthz) {
+            vd->sasl.authzid = g_strdup(saslauthz);
+        } else if (acl) {
+            if (strcmp(vd->id, "default") == 0) {
+                vd->sasl.authzid = g_strdup("vnc.username");
+            } else {
+                vd->sasl.authzid = g_strdup_printf("vnc.%s.username", vd->id);
+            }
+            vd->sasl.authz = QAUTHZ(qauthz_list_new(vd->sasl.authzid,
+                                                    QAUTHZ_LIST_POLICY_DENY,
+                                                    &error_abort));
         }
-        vd->sasl.authz = QAUTHZ(qauthz_list_new(vd->sasl.authzid,
-                                                QAUTHZ_LIST_POLICY_DENY,
-                                                &error_abort));
     }
 #endif
 
-- 
2.17.0

[Qemu-devel] [PATCH v2 6/6] monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove

[Daniel P. Berrangé]

The various ACL related commands are obsolete now that the QAuthZ
framework for authorization is fully integrated throughout QEMU network
services. Mark it as deprecated with no replacement to be provided.

Authorization is now provided by using 'object_add' together with
the 'tls-authz' or 'sasl-authz' parameters to the VNC server, and
equivalent for other network services.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor.c     | 23 +++++++++++++++++++++++
 qemu-doc.texi |  8 ++++++++
 2 files changed, 31 insertions(+)

diff --git a/monitor.c b/monitor.c
index 07d14f53f9..cbcfbf912b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2112,6 +2112,19 @@ static QAuthZList *find_auth(Monitor *mon, const char *name)
     return QAUTHZ_LIST(obj);
 }
 
+static bool warn_acl;
+static void hmp_warn_acl(void)
+{
+    if (warn_acl) {
+        return;
+    }
+    error_report("The acl_show, acl_reset, acl_policy, acl_add, acl_remove "
+                 "commands are deprecated with no replacement. Authorization "
+                 "for VNC should be performed using the pluggable QAuthZ "
+                 "objects");
+    warn_acl = true;
+}
+
 static void hmp_acl_show(Monitor *mon, const QDict *qdict)
 {
     const char *aclname = qdict_get_str(qdict, "aclname");
@@ -2119,6 +2132,8 @@ static void hmp_acl_show(Monitor *mon, const QDict *qdict)
     QAuthZListRuleList *rules;
     size_t i = 0;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2142,6 +2157,8 @@ static void hmp_acl_reset(Monitor *mon, const QDict *qdict)
     const char *aclname = qdict_get_str(qdict, "aclname");
     QAuthZList *auth = find_auth(mon, aclname);
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2160,6 +2177,8 @@ static void hmp_acl_policy(Monitor *mon, const QDict *qdict)
     int val;
     Error *err = NULL;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2195,6 +2214,8 @@ static void hmp_acl_add(Monitor *mon, const QDict *qdict)
     QAuthZListFormat format;
     size_t i = 0;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2250,6 +2271,8 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
     QAuthZList *auth = find_auth(mon, aclname);
     ssize_t i = 0;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 5639b7ecdb..8327e56db9 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2933,6 +2933,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
 The ``arch'' output member of the ``query-cpus-fast'' command is
 replaced by the ``target'' output member.
 
+@section Human Monitor Protocol (HMP) commands
+
+@subsection acl_show, acl_reset, acl_policy, acl_add, acl_remove (since 3.0.0)
+
+The ``acl_show'', ``acl_reset'', ``acl_policy'', ``acl_add'', and
+``acl_remove'' commands are deprecated with no replacement. Authorization
+for VNC should be performed using the pluggable QAuthZ objects.
+
 @section System emulator devices
 
 @subsection ivshmem (since 2.6.0)
-- 
2.17.0

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

[Eric Blake]

On 06/20/2018 07:14 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> 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

Is the space in O= intentional?

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

you need shell quoting to preserve the space. Also, the indentation 
breaks the intent that these long lines are single arguments, not 
separate arguments.

>             --tls-creds tls0 \
>             --tls-authz authz0
> 	   ....other qemu-nbd args...

Is it also worth a sample command line using JSON syntax?

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>   include/block/nbd.h |  2 +-
>   nbd/server.c        | 10 +++++-----
>   qemu-nbd.c          | 13 ++++++++++++-
>   qemu-nbd.texi       |  4 ++++
>   4 files changed, 22 insertions(+), 7 deletions(-)
> 

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

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

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

[Daniel P. Berrangé]

On Wed, Jun 20, 2018 at 08:58:40AM -0500, Eric Blake wrote:
> On 06/20/2018 07:14 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > 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
> 
> Is the space in O= intentional?

Yes it really does have a space, though perhaps for sake of
illustration I'll remove the space.

> 
> > 
> > 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 \
> 
> you need shell quoting to preserve the space. Also, the indentation breaks
> the intent that these long lines are single arguments, not separate
> arguments.

Yeah I'll definitely remove the space, because it just makes this
more confusing than needed.

I know the \ isn't really supposed to separate args, but I don't
see a nicer way to format it for a commit message while respecting
line length. I figured people would understand what I meant, but
the fact that I've indented the second line to vertically align.

> 
> >             --tls-creds tls0 \
> >             --tls-authz authz0
> > 	   ....other qemu-nbd args...
> 
> Is it also worth a sample command line using JSON syntax?

Probably overkill for the commit message IMHO.

> 
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >   include/block/nbd.h |  2 +-
> >   nbd/server.c        | 10 +++++-----
> >   qemu-nbd.c          | 13 ++++++++++++-
> >   qemu-nbd.texi       |  4 ++++
> >   4 files changed, 22 insertions(+), 7 deletions(-)
> > 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

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 v2 2/6] nbd: allow authorization with nbd-server-start QMP command

[Eric Blake]

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.

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

Re: [Qemu-devel] [PATCH v2 2/6] nbd: allow authorization with nbd-server-start QMP command

[Daniel P. Berrangé]

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
-- 
|: 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 v2 1/6] qemu-nbd: add support for authorization of TLS clients

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> 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
> 
> 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 \

I'm confused about how that gets parsed, what differentiates the ,s
that separate the arguments (e.g. ,id=  ,identity=) and the ,s that
separate the options within the identity string (e.g. the ,ST=London)
Would:
  --object authz-simple,identity=CN=laptop.example.com,,O=Example Org,,L=London,,ST=London,,C=GB,id=auth0

be equivalent?

Dave


>            --tls-creds tls0 \
>            --tls-authz authz0
> 	   ....other qemu-nbd args...
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/block/nbd.h |  2 +-
>  nbd/server.c        | 10 +++++-----
>  qemu-nbd.c          | 13 ++++++++++++-
>  qemu-nbd.texi       |  4 ++++
>  4 files changed, 22 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..4f10f08dc0 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(&client->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,7 @@ void nbd_client_new(NBDExport *exp,
>      if (tlscreds) {
>          object_ref(OBJECT(client->tlscreds));
>      }
> -    client->tlsaclname = g_strdup(tlsaclname);
> +    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_OPTS    262
>  #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", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
>          { NULL, 0, NULL, 0 }
>      };
>      int ch;
> @@ -755,6 +758,9 @@ int main(int argc, char **argv)
>              g_free(trace_file);
>              trace_file = trace_opt_parse(optarg);
>              break;
> +        case QEMU_NBD_OPT_TLSAUTHZ:
> +            tlsauthz = optarg;
> +            break;
>          case QEMU_NBD_OPT_FORK:
>              fork_process = true;
>              break;
> @@ -819,6 +825,11 @@ int main(int argc, char **argv)
>                           error_get_pretty(local_err));
>              exit(EXIT_FAILURE);
>          }
> +    } else {
> +        if (tlsauthz) {
> +            error_report("--tls-authz is not permitted without --tls-creds");
> +            exit(EXIT_FAILURE);
> +        }
>      }
>  
>      if (disconnect) {
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 9a84e81eed..7f9503cf05 100644
> --- a/qemu-nbd.texi
> +++ 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
> +--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
> -- 
> 2.17.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Wed, Jun 20, 2018 at 03:22:53PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > 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
> > 
> > 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 \
> 
> I'm confused about how that gets parsed, what differentiates the ,s
> that separate the arguments (e.g. ,id=  ,identity=) and the ,s that
> separate the options within the identity string (e.g. the ,ST=London)

That's why I've doubled up - eg ',,' must be used when you need to
include a literal ',' in a value without it being interpreted as
starting a new option

> Would:
>   --object authz-simple,identity=CN=laptop.example.com,,O=Example Org,,L=London,,ST=London,,C=GB,id=auth0
> 
> be equivalent?

Yes


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 v2 1/6] qemu-nbd: add support for authorization of TLS clients

[Eric Blake]

On 06/20/2018 09:22 AM, Dr. David Alan Gilbert wrote:

>> 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
>>
>> 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 \
> 
> I'm confused about how that gets parsed, what differentiates the ,s
> that separate the arguments (e.g. ,id=  ,identity=) and the ,s that
> separate the options within the identity string (e.g. the ,ST=London)
> Would:
>    --object authz-simple,identity=CN=laptop.example.com,,O=Example Org,,L=London,,ST=London,,C=GB,id=auth0
> 
> be equivalent?

Yes, once you take care of quoting the space and unfolding indentation. 
Our standard QemuOpt parser treats ',,' as a literal comma, and all 
other ',' as separating args.  So either form is ultimately parsed as:

--object
   [type=]"authz-simple"
   id="auth0"
   identity="CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB"

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

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

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Wed, Jun 20, 2018 at 03:22:53PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > From: "Daniel P. Berrange" <berrange@redhat.com>
> > > 
> > > 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
> > > 
> > > 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 \
> > 
> > I'm confused about how that gets parsed, what differentiates the ,s
> > that separate the arguments (e.g. ,id=  ,identity=) and the ,s that
> > separate the options within the identity string (e.g. the ,ST=London)
> 
> That's why I've doubled up - eg ',,' must be used when you need to
> include a literal ',' in a value without it being interpreted as
> starting a new option

OK, yeh I forgot about the obscure double-comma rule.

> > Would:
> >   --object authz-simple,identity=CN=laptop.example.com,,O=Example Org,,L=London,,ST=London,,C=GB,id=auth0
> > 
> > be equivalent?
> 
> Yes

OK.

Dave

> 
> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK