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

[Qemu-devel] [PATCH 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.

Based-on: <20180615154203.11347-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        | 14 ++++++++---
 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          | 12 +++++----
 qapi/block.json       |  4 ++-
 qapi/char.json        |  2 ++
 qapi/migration.json   | 12 ++++++++-
 qemu-doc.texi         | 19 ++++++++------
 qemu-nbd.c            | 13 +++++++++-
 qemu-nbd.texi         |  4 +++
 ui/vnc.c              | 58 ++++++++++++++++++++++++++++++++++++-------
 16 files changed, 167 insertions(+), 33 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 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 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 <berrange@redhat.com>
---
 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(&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,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_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..8f16bfb513 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 users who
+connect against their x509 distinguished name.
 @item -v, --verbose
 Display extra debugging information
 @item -h, --help
-- 
2.17.0

[Qemu-devel] [PATCH 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-simple',
       '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      | 14 +++++++++++---
 hmp.c               |  2 +-
 include/block/nbd.h |  2 +-
 qapi/block.json     |  4 +++-
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 65a84739ed..1ef2989118 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,10 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
         }
     }
 
+    if (tls_authz) {
+        nbd_server->tlsauthz = g_strdup(tls_authz);
+    }
+
     qio_net_listener_set_client_func(nbd_server->listener,
                                      nbd_accept,
                                      NULL,
@@ -132,11 +139,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 ef93f4878b..74e18db103 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2214,7 +2214,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..8c7cc9b798 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -197,6 +197,7 @@
 #
 # @addr: Address on which to listen.
 # @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
+# @tls-authz: (optional) ID of the QAuthZ authorization object. Since 2.13
 #
 # Returns: error if the server is already running.
 #
@@ -204,7 +205,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 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.

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

diff --git a/hmp.c b/hmp.c
index 74e18db103..bef8ea2531 100644
--- a/hmp.c
+++ b/hmp.c
@@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
+        monitor_printf(mon, " %s: '%s'\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
+            params->has_tls_authz ? params->tls_authz : "");
     }
 
     qapi_free_MigrationParameters(params);
@@ -1632,6 +1635,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 1e99ec9b7e..d14c8d7003 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -645,6 +645,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;
@@ -1106,6 +1108,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 f7e10ee90f..b9ba34e3a6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -488,6 +488,10 @@
 #                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. (Since
+#             2.13)
+#
 # @max-bandwidth: to set maximum speed for migration. maximum speed in
 #                 bytes per second. (Since 2.8)
 #
@@ -522,7 +526,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' ] }
@@ -605,6 +609,7 @@
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
+            '*tls-authz': 'StrOrNull',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
             '*x-checkpoint-delay': 'int',
@@ -667,6 +672,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
+#             2.13)
+#
 # @max-bandwidth: to set maximum speed for migration. maximum speed in
 #                 bytes per second. (Since 2.8)
 #
@@ -704,6 +713,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 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,policy=deny,\
                rules.0.match=\*CN=fred,rules.0.policy=allow \
        -chardev socket,host=127.0.0.1,port=9000,server,\
	         tls-creds=tls0,tls-authz=authz0 \
        ...other qemud args...

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 chardev/char-socket.c | 11 ++++++++++-
 chardev/char.c        |  3 +++
 qapi/char.json        |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

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..284f3ee4f1 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -242,6 +242,7 @@
 # @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 (since 2.13)
 # @server: create server socket (default: true)
 # @wait: wait for incoming connection on server
 #        sockets (default: false).
@@ -259,6 +260,7 @@
 ##
 { 'struct': 'ChardevSocket', 'data': { 'addr'       : 'SocketAddressLegacy',
                                      '*tls-creds'  : 'str',
+                                     '*tls-authz'  : 'str',
                                      '*server'    : 'bool',
                                      '*wait'      : 'bool',
                                      '*nodelay'   : 'bool',
-- 
2.17.0

[Qemu-devel] [PATCH 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 | 11 +++-------
 ui/vnc.c      | 58 +++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index cd05760cac..5b7e3faab2 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2917,15 +2917,10 @@ The @code{-localtime} option has been replaced by @code{-rtc base=localtime}.
 
 The @code{-startdate} option has been replaced by @code{-rtc base=@var{date}}.
 
-@subsection -virtioconsole (since 3.0.0)
+@subsection -vnc acl (since 3.0.0)
 
-Option @option{-virtioconsole} has been replaced by
-@option{-device virtconsole}.
-
-@subsection -clock (since 3.0.0)
-
-The @code{-clock} option is ignored since QEMU version 1.7.0. There is no
-replacement since it is not needed anymore.
+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
 
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 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.

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 67c63013bd..c4a9ae5c85 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2089,6 +2089,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");
@@ -2096,6 +2109,8 @@ static void hmp_acl_show(Monitor *mon, const QDict *qdict)
     QAuthZListRuleList *rules;
     size_t i = 0;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2119,6 +2134,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;
     }
@@ -2137,6 +2154,8 @@ static void hmp_acl_policy(Monitor *mon, const QDict *qdict)
     int val;
     Error *err = NULL;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2172,6 +2191,8 @@ static void hmp_acl_add(Monitor *mon, const QDict *qdict)
     QAuthZListFormat format;
     size_t i = 0;
 
+    hmp_warn_acl();
+
     if (!auth) {
         return;
     }
@@ -2227,6 +2248,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 5b7e3faab2..c6aad94015 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2938,6 +2938,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 3/6] migration: add support for a "tls-authz" migration parameter

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> 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.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

I'd appreciate an example of using it, either in the migration docs or
the commit message.

> ---
>  hmp.c                 |  9 +++++++++
>  migration/migration.c |  8 ++++++++
>  migration/tls.c       |  2 +-
>  qapi/migration.json   | 12 +++++++++++-
>  4 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 74e18db103..bef8ea2531 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "%s: %" PRIu64 "\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
>              params->xbzrle_cache_size);
> +        monitor_printf(mon, " %s: '%s'\n",
> +            MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> +            params->has_tls_authz ? params->tls_authz : "");
>      }
>  
>      qapi_free_MigrationParameters(params);
> @@ -1632,6 +1635,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 1e99ec9b7e..d14c8d7003 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -645,6 +645,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;
> @@ -1106,6 +1108,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 f7e10ee90f..b9ba34e3a6 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -488,6 +488,10 @@
>  #                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. (Since
> +#             2.13)
> +#

Oops, 2.13 strikes again :-)

Other than that, OK from migration and HMP.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  # @max-bandwidth: to set maximum speed for migration. maximum speed in
>  #                 bytes per second. (Since 2.8)
>  #
> @@ -522,7 +526,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' ] }
> @@ -605,6 +609,7 @@
>              '*cpu-throttle-increment': 'int',
>              '*tls-creds': 'StrOrNull',
>              '*tls-hostname': 'StrOrNull',
> +            '*tls-authz': 'StrOrNull',
>              '*max-bandwidth': 'int',
>              '*downtime-limit': 'int',
>              '*x-checkpoint-delay': 'int',
> @@ -667,6 +672,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
> +#             2.13)
> +#
>  # @max-bandwidth: to set maximum speed for migration. maximum speed in
>  #                 bytes per second. (Since 2.8)
>  #
> @@ -704,6 +713,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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Fri, Jun 15, 2018 at 06:54:23PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > 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.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> 
> I'd appreciate an example of using it, either in the migration docs or
> the commit message.

Hmm, yes, it's an oversight to have missed an example in this commit
message.

> 
> > ---
> >  hmp.c                 |  9 +++++++++
> >  migration/migration.c |  8 ++++++++
> >  migration/tls.c       |  2 +-
> >  qapi/migration.json   | 12 +++++++++++-
> >  4 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 74e18db103..bef8ea2531 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> >          monitor_printf(mon, "%s: %" PRIu64 "\n",
> >              MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
> >              params->xbzrle_cache_size);
> > +        monitor_printf(mon, " %s: '%s'\n",
> > +            MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> > +            params->has_tls_authz ? params->tls_authz : "");
> >      }
> >  
> >      qapi_free_MigrationParameters(params);
> > @@ -1632,6 +1635,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 1e99ec9b7e..d14c8d7003 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -645,6 +645,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;
> > @@ -1106,6 +1108,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 f7e10ee90f..b9ba34e3a6 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -488,6 +488,10 @@
> >  #                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. (Since
> > +#             2.13)
> > +#
> 
> Oops, 2.13 strikes again :-)
> 
> Other than that, OK from migration and HMP.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> >  # @max-bandwidth: to set maximum speed for migration. maximum speed in
> >  #                 bytes per second. (Since 2.8)
> >  #
> > @@ -522,7 +526,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' ] }
> > @@ -605,6 +609,7 @@
> >              '*cpu-throttle-increment': 'int',
> >              '*tls-creds': 'StrOrNull',
> >              '*tls-hostname': 'StrOrNull',
> > +            '*tls-authz': 'StrOrNull',
> >              '*max-bandwidth': 'int',
> >              '*downtime-limit': 'int',
> >              '*x-checkpoint-delay': 'int',
> > @@ -667,6 +672,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
> > +#             2.13)
> > +#
> >  # @max-bandwidth: to set maximum speed for migration. maximum speed in
> >  #                 bytes per second. (Since 2.8)
> >  #
> > @@ -704,6 +713,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
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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 6/6] monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> 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.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

OK, so I can do all these by using object_add/object_del with the right
type and parameters?

but looks OK:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> ---
>  monitor.c     | 23 +++++++++++++++++++++++
>  qemu-doc.texi |  8 ++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 67c63013bd..c4a9ae5c85 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2089,6 +2089,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");
> @@ -2096,6 +2109,8 @@ static void hmp_acl_show(Monitor *mon, const QDict *qdict)
>      QAuthZListRuleList *rules;
>      size_t i = 0;
>  
> +    hmp_warn_acl();
> +
>      if (!auth) {
>          return;
>      }
> @@ -2119,6 +2134,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;
>      }
> @@ -2137,6 +2154,8 @@ static void hmp_acl_policy(Monitor *mon, const QDict *qdict)
>      int val;
>      Error *err = NULL;
>  
> +    hmp_warn_acl();
> +
>      if (!auth) {
>          return;
>      }
> @@ -2172,6 +2191,8 @@ static void hmp_acl_add(Monitor *mon, const QDict *qdict)
>      QAuthZListFormat format;
>      size_t i = 0;
>  
> +    hmp_warn_acl();
> +
>      if (!auth) {
>          return;
>      }
> @@ -2227,6 +2248,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 5b7e3faab2..c6aad94015 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2938,6 +2938,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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Tue, Jun 19, 2018 at 01:31:40PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > 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.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> OK, so I can do all these by using object_add/object_del with the right
> type and parameters?

It is a different paradigm for the way you manage it, but the end result
allows the same thing to be achieved, in a more flexible way.

With the old way, we precreated an ACL object for VNC, and then you
had to use these commands to add/remove individual  match rules and
or change the policy, etc. You could never create/delete the ACL itself.

With the new way, we have 4 different ACL implementations (so far)
and you can choose which to use. So you create the entire ACL with
all its rules populated atomically with object_add. There's no
create/delete of individual rules within the ACL, so if you want to
change rules you just delete the entire ACL & create it again. It
has failsafe to reject in case a client connects between the time
you delete and recreate.

One of the ACL impls allows storing the rules in a standalone text
file which we monitor with inotify. So in fact using that you can
update rules on the fly without needing QEMU interaction - just
change the content whenever needed.

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 5/6] vnc: allow specifying a custom authorization object name

[Daniel P. Berrangé]

On Fri, Jun 15, 2018 at 04:51:02PM +0100, Daniel P. Berrangé wrote:
> 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 \

Opps this msg is outdated, since we don't have ability to express
such nested properties with -object since I dropped my hacky impl
of that.

>           -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 | 11 +++-------
>  ui/vnc.c      | 58 +++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index cd05760cac..5b7e3faab2 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2917,15 +2917,10 @@ The @code{-localtime} option has been replaced by @code{-rtc base=localtime}.
>  
>  The @code{-startdate} option has been replaced by @code{-rtc base=@var{date}}.
>  
> -@subsection -virtioconsole (since 3.0.0)
> +@subsection -vnc acl (since 3.0.0)
>  
> -Option @option{-virtioconsole} has been replaced by
> -@option{-device virtconsole}.
> -
> -@subsection -clock (since 3.0.0)
> -
> -The @code{-clock} option is ignored since QEMU version 1.7.0. There is no
> -replacement since it is not needed anymore.
> +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
>  
> 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
> 

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

[Eric Blake]

On 06/15/2018 10:50 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 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 <berrange@redhat.com>
> ---
>   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

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

[Eric Blake]

On 06/15/2018 10:50 AM, Daniel P. Berrangé wrote:
> 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-simple',
>         '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'
>       }
>     }

Is it worth using a discriminated union (string vs. QAuthZ) so that one 
could specify the authz policy inline rather than as a separate object, 
for convenience?  But that would be fine as a followup patch, if we even 
want it.

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

> @@ -118,6 +121,10 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
>           }
>       }
>   
> +    if (tls_authz) {
> +        nbd_server->tlsauthz = g_strdup(tls_authz);
> +    }

Pointless 'if'; g_strdup() does the right thing.

> +++ b/qapi/block.json
> @@ -197,6 +197,7 @@
>   #
>   # @addr: Address on which to listen.
>   # @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
> +# @tls-authz: (optional) ID of the QAuthZ authorization object. Since 2.13

No need for the string '(optional)' (I thought we killed those uses when 
we automated the documentation generation - but obviously a few were 
left behind).

s/2.13/3.0/

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

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

[Daniel P. Berrangé]

On Tue, Jun 19, 2018 at 03:10:12PM -0500, Eric Blake wrote:
> On 06/15/2018 10:50 AM, Daniel P. Berrangé wrote:
> > 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-simple',
> >         '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'
> >       }
> >     }
> 
> Is it worth using a discriminated union (string vs. QAuthZ) so that one
> could specify the authz policy inline rather than as a separate object, for
> convenience?  But that would be fine as a followup patch, if we even want
> it.

QAuthZ isn't a QAPI type - its a QOM object interface, so you'd have to
allow the entire object_add arg set inline, and then validate the QOM type
you received after the fact actually implemented the interface.  Also for
migration at least it is likely the single authz impl will be shared for
both migration + nbd services. So I think its cleaner just to keep it
separate to avoid having 2 distinct codepaths for handling the same thing


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

[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" <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 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 3/6] migration: add support for a "tls-authz" migration parameter

[Juan Quintela]

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

.....


It is not just the fault of this patch, but as you are the one doing the
tls bits on migration...


> @@ -1106,6 +1108,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);

We really try  hard not to use assert() on migration code (yes, it is an
ongoing effort).  The code around this is something like:

static void migrate_params_test_apply(MigrateSetParameters *params,
                                      MigrationParameters *dest)
{
    [...]

    if (params->has_compress_level) {
        dest->compress_level = params->compress_level;
    }

    [...]

    if (params->has_tls_creds) {
        assert(params->tls_creds->type == QTYPE_QSTRING);
        dest->tls_creds = g_strdup(params->tls_creds->u.s);
    }
    [...]
}

Ok, tls code is the one with strings, but still.

static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
{
    [...]
    if (params->has_compress_level) {
        s->parameters.compress_level = params->compress_level;
    }
    [...]

    if (params->has_tls_creds) {
        g_free(s->parameters.tls_creds);
        assert(params->tls_creds->type == QTYPE_QSTRING);
        s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
    }
}

And this other function:

static bool migrate_params_check(MigrationParameters *params, Error **errp)
{
    if (params->has_compress_level &&
        (params->compress_level > 9)) {
        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
                   "is invalid, it should be in the range of 0 to 9");
        return false;
    }
    [...]
}

Where we don't check anything for tls.

Perhaps we can move the asserts here?

We can also try to merge migrate_params_check and
migrate_params_test_apply() into a single function, but it is not
completely trivial at the moment.

Wondering if we can do it better.

Later, Juan.

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

[Daniel P. Berrangé]

On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> .....
> 
> 
> It is not just the fault of this patch, but as you are the one doing the
> tls bits on migration...
> 
> 
> > @@ -1106,6 +1108,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);
> 
> We really try  hard not to use assert() on migration code (yes, it is an
> ongoing effort).  The code around this is something like:
> 
> static void migrate_params_test_apply(MigrateSetParameters *params,
>                                       MigrationParameters *dest)
> {
>     [...]
> 
>     if (params->has_compress_level) {
>         dest->compress_level = params->compress_level;
>     }
> 
>     [...]
> 
>     if (params->has_tls_creds) {
>         assert(params->tls_creds->type == QTYPE_QSTRING);
>         dest->tls_creds = g_strdup(params->tls_creds->u.s);
>     }
>     [...]
> }
> 
> Ok, tls code is the one with strings, but still.

My understanding is that because we declared this parameter to
have "str" type in the QAPI schema, the QAPI code should already
guarantee that  "params->tls_creds->type == QTYPE_QSTRING" is
true.

IOW, the assert should never fail, and if it does, that would
be a good thing as if QAPI wasn't validating input correctly
something very bad has gone wrong.


> static bool migrate_params_check(MigrationParameters *params, Error **errp)
> {
>     if (params->has_compress_level &&
>         (params->compress_level > 9)) {
>         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>                    "is invalid, it should be in the range of 0 to 9");
>         return false;

This is different because QAPI schema merely declares it to be an
int, so nothing in the QAPI input validation will do range checking.
So this codepath is definitely reachable by users feeding bad input.

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 3/6] migration: add support for a "tls-authz" migration parameter

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > From: "Daniel P. Berrange" <berrange@redhat.com>
>> 
>> .....
>> 
>> 
>> It is not just the fault of this patch, but as you are the one doing the
>> tls bits on migration...
>> 
>> 
>> > @@ -1106,6 +1108,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);
>> 
>> We really try  hard not to use assert() on migration code (yes, it is an
>> ongoing effort).  The code around this is something like:
>> 
>> static void migrate_params_test_apply(MigrateSetParameters *params,
>>                                       MigrationParameters *dest)
>> {
>>     [...]
>> 
>>     if (params->has_compress_level) {
>>         dest->compress_level = params->compress_level;
>>     }
>> 
>>     [...]
>> 
>>     if (params->has_tls_creds) {
>>         assert(params->tls_creds->type == QTYPE_QSTRING);
>>         dest->tls_creds = g_strdup(params->tls_creds->u.s);
>>     }
>>     [...]
>> }
>> 
>> Ok, tls code is the one with strings, but still.
>
> My understanding is that because we declared this parameter to
> have "str" type in the QAPI schema, the QAPI code should already
> guarantee that  "params->tls_creds->type == QTYPE_QSTRING" is
> true.
>
> IOW, the assert should never fail, and if it does, that would
> be a good thing as if QAPI wasn't validating input correctly
> something very bad has gone wrong.
>
>
>> static bool migrate_params_check(MigrationParameters *params, Error **errp)
>> {
>>     if (params->has_compress_level &&
>>         (params->compress_level > 9)) {
>>         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>>                    "is invalid, it should be in the range of 0 to 9");
>>         return false;
>
> This is different because QAPI schema merely declares it to be an
> int, so nothing in the QAPI input validation will do range checking.
> So this codepath is definitely reachable by users feeding bad input.

ok then.  Thanks for the explanation.

Later, Juan.