[Qemu-devel] [PATCH v6 0/3] nbd: support for authorization control on TLS connections

[Qemu-devel] [PATCH v6 0/3] nbd: support for authorization control on TLS connections

[Daniel P. Berrangé]

This series provides the NBD parts of the authorization control series
previously posted as:

  v1: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04482.html
  v2: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05727.html
  v3: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01639.html
  v4: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg04319.html

Then separated for merge:

  v5: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07347.html

The core authz framework is now merged & these patches have all had
positive review. Thus these NBD parts are ready to go into the NBD
maintainer's tree, should the maintainer consider them acceptable.

Changed in v6:

 - Fix qapi annotations
 - Add qemu-nbd example
 - Reject authz parameter with --list

Daniel P. Berrangé (3):
  qemu-nbd: add support for authorization of TLS clients
  nbd: allow authorization with nbd-server-start QMP command
  nbd: fix outdated qapi docs syntax for tls-creds

 blockdev-nbd.c             | 11 ++++++++---
 hmp.c                      |  2 +-
 include/block/nbd.h        |  4 ++--
 nbd/server.c               | 10 +++++-----
 qapi/block.json            | 10 ++++++++--
 qemu-nbd.c                 | 18 +++++++++++++++++-
 qemu-nbd.texi              | 11 +++++++++--
 tests/qemu-iotests/233     | 31 ++++++++++++++++++++++++++++---
 tests/qemu-iotests/233.out | 11 +++++++++++
 9 files changed, 89 insertions(+), 19 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v6 1/3] 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

escape the commas in the name and 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...

NB: a real shell command line would not have leading whitespace after
the line continuation, it is just included here for clarity.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/block/nbd.h        |  2 +-
 nbd/server.c               | 10 +++++-----
 qemu-nbd.c                 | 18 +++++++++++++++++-
 qemu-nbd.texi              | 11 +++++++++--
 tests/qemu-iotests/233     | 31 ++++++++++++++++++++++++++++---
 tests/qemu-iotests/233.out | 11 +++++++++++
 6 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 96cfb1d7d5..554f531c1a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -325,7 +325,7 @@ void nbd_export_close_all(void);
 
 void nbd_client_new(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 0910d09a6d..8ddfd3e319 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -111,7 +111,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) */
 
@@ -686,7 +686,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;
@@ -1348,7 +1348,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);
@@ -2425,7 +2425,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
  */
 void nbd_client_new(QIOChannelSocket *sioc,
                     QCryptoTLSCreds *tlscreds,
-                    const char *tlsaclname,
+                    const char *tlsauthz,
                     void (*close_fn)(NBDClient *, bool))
 {
     NBDClient *client;
@@ -2437,7 +2437,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
     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 00c07fd27e..99f0a4db20 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -58,6 +58,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
 
@@ -71,6 +72,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)
 {
@@ -103,6 +105,7 @@ static void usage(const char *name)
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
 "                            passwords and/or encryption keys\n"
 "  --tls-creds=ID            use id of an earlier --object to provide TLS\n"
+"  --tls-authz=ID            use id of an earlier --object to provide authorization\n"
 "  -T, --trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
 "                            specify tracing options\n"
 "  --fork                    fork off the server process and exit the parent\n"
@@ -452,7 +455,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
 
     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(cioc, tlscreds, NULL, nbd_client_closed);
+    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
 }
 
 static void nbd_update_server_watch(void)
@@ -643,6 +646,7 @@ int main(int argc, char **argv)
         { "export-name", required_argument, NULL, 'x' },
         { "description", required_argument, NULL, 'D' },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
+        { "tls-authz", required_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { "trace", required_argument, NULL, 'T' },
         { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
@@ -862,6 +866,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;
@@ -934,12 +941,21 @@ int main(int argc, char **argv)
             error_report("TLS is not supported with a host device");
             exit(EXIT_FAILURE);
         }
+        if (tlsauthz && list) {
+            error_report("TLS authorization is incompatible with export list");
+            exit(EXIT_FAILURE);
+        }
         tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
         if (local_err) {
             error_report("Failed to get TLS creds %s",
                          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 (list) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index d0c5182814..de342c76b8 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -117,6 +117,10 @@ option; or provide the credentials needed for connecting as a client
 in list mode.
 @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
@@ -142,13 +146,16 @@ qemu-nbd -f qcow2 file.qcow2
 @end example
 
 Start a long-running server listening with encryption on port 10810,
-and require clients to have a correct X.509 certificate to connect to
+and whitelist clients with a specific X.509 certificate to connect to
 a 1 megabyte subset of a raw file, using the export name 'subset':
 
 @example
 qemu-nbd \
   --object tls-creds-x509,id=tls0,endpoint=server,dir=/path/to/qemutls \
-  --tls-creds tls0 -t -x subset -p 10810 \
+  --object 'authz-simple,id=auth0,identity=CN=laptop.example.com,,\
+            O=Example Org,,L=London,,ST=London,,C=GB' \
+  --tls-creds tls0 --tls-authz auth0 \
+  -t -x subset -p 10810 \
   --image-opts driver=raw,offset=1M,size=1M,file.driver=file,file.filename=file.raw
 @end example
 
diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index fc345a1a46..4762c380cb 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -59,6 +59,7 @@ tls_x509_create_root_ca "ca2"
 tls_x509_create_server "ca1" "server1"
 tls_x509_create_client "ca1" "client1"
 tls_x509_create_client "ca2" "client2"
+tls_x509_create_client "ca1" "client3"
 
 echo
 echo "== preparing image =="
@@ -91,11 +92,15 @@ $QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port
 
 echo
 echo "== check TLS works =="
-obj=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
-$QEMU_IMG info --image-opts --object $obj \
+obj1=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
+obj2=tls-creds-x509,dir=${tls_dir}/client3,endpoint=client,id=tls0
+$QEMU_IMG info --image-opts --object $obj1 \
     driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
     2>&1 | sed "s/$nbd_tcp_port/PORT/g"
-$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj \
+$QEMU_IMG info --image-opts --object $obj2 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
+    2>&1 | sed "s/$nbd_tcp_port/PORT/g"
+$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj1 \
     --tls-creds=tls0
 
 echo
@@ -117,6 +122,26 @@ $QEMU_IO -c 'r -P 0x11 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
 
 $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "== check TLS with authorization =="
+
+nbd_server_stop
+
+nbd_server_start_tcp_socket \
+    --object tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes \
+    --object "authz-simple,identity=CN=localhost,,O=Cthulu Dark Lord Enterprises client1,,L=R'lyeh,,C=South Pacific,id=authz0" \
+    --tls-authz authz0 \
+    --tls-creds tls0 \
+    -f $IMGFMT "$TEST_IMG" 2>> "$TEST_DIR/server.log"
+
+$QEMU_IMG info --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
+
+$QEMU_IMG info --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client3,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
+
 echo
 echo "== final server log =="
 cat "$TEST_DIR/server.log"
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 6d45f3b230..5acbc13b54 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -6,6 +6,7 @@ Generating a self signed certificate...
 Generating a signed certificate...
 Generating a signed certificate...
 Generating a signed certificate...
+Generating a signed certificate...
 
 == preparing image ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -29,6 +30,10 @@ image: nbd://127.0.0.1:PORT
 file format: nbd
 virtual size: 64M (67108864 bytes)
 disk size: unavailable
+image: nbd://127.0.0.1:PORT
+file format: nbd
+virtual size: 64M (67108864 bytes)
+disk size: unavailable
 exports available: 1
  export: ''
   size:  67108864
@@ -51,7 +56,13 @@ wrote 1048576/1048576 bytes at offset 1048576
 read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+== check TLS with authorization ==
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
+
 == final server log ==
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
+qemu-nbd: option negotiation failed: TLS x509 authz check for CN=localhost,O=Cthulhu Dark Lord Enterprises client1,L=R'lyeh,C=South Pacific is denied
+qemu-nbd: option negotiation failed: TLS x509 authz check for CN=localhost,O=Cthulhu Dark Lord Enterprises client3,L=R'lyeh,C=South Pacific is denied
 *** done
-- 
2.20.1

[Qemu-devel] [PATCH v6 2/3] 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'
     }
   }

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 blockdev-nbd.c      | 11 ++++++++---
 hmp.c               |  2 +-
 include/block/nbd.h |  2 +-
 qapi/block.json     |  8 +++++++-
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index d73ac1b026..66eebab318 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;
@@ -36,7 +37,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
                        gpointer opaque)
 {
     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-    nbd_client_new(cioc, nbd_server->tlscreds, NULL,
+    nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
                    nbd_blockdev_client_closed);
 }
 
@@ -52,6 +53,7 @@ static void nbd_server_free(NBDServerData *server)
     if (server->tlscreds) {
         object_unref(OBJECT(server->tlscreds));
     }
+    g_free(server->tlsauthz);
 
     g_free(server);
 }
@@ -87,7 +89,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");
@@ -117,6 +119,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,
@@ -131,11 +135,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 1e006eeb49..c234634f8a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2307,7 +2307,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 554f531c1a..b852080d6a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -331,7 +331,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
  * Reads @size bytes from @ioc. Returns 0 on success.
diff --git a/qapi/block.json b/qapi/block.json
index 5a79d639e8..79623088e7 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -225,6 +225,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 4.0).
 #
 # Returns: error if the server is already running.
 #
@@ -232,7 +237,8 @@
 ##
 { 'command': 'nbd-server-start',
   'data': { 'addr': 'SocketAddressLegacy',
-            '*tls-creds': 'str'} }
+            '*tls-creds': 'str',
+            '*tls-authz': 'str'} }
 
 ##
 # @nbd-server-add:
-- 
2.20.1

[Qemu-devel] [PATCH v6 3/3] nbd: fix outdated qapi docs syntax for tls-creds

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/block.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/block.json b/qapi/block.json
index 79623088e7..145c268bb6 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -224,7 +224,7 @@
 # QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
 #
 # @addr: Address on which to listen.
-# @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
+# @tls-creds: 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
-- 
2.20.1

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

[Eric Blake]

On 2/27/19 10:20 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <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.
> 

> +++ b/qemu-nbd.c

> @@ -103,6 +105,7 @@ static void usage(const char *name)
>  "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
>  "                            passwords and/or encryption keys\n"
>  "  --tls-creds=ID            use id of an earlier --object to provide TLS\n"
> +"  --tls-authz=ID            use id of an earlier --object to provide authorization\n"

Usage line exceeds 80 columns; I don't mind splitting the line.

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

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


>  
> +== check TLS with authorization ==
> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort

A rather uninformative message for the client to figure out why it
failed, but (as with all things security-related), giving too many
details may in itself be an improper information leak. At any rate, I
don't know that you could make it work any better, so it is not a
problem with this patch.  It may be the sign of a server bug for closing
the socket too soon (before the client has a chance to read an actual
error message), where we could tweak things to provoke a nicer error
than 'Software caused connection abort', but that would be a separate patch.

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

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

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

Re: [Qemu-devel] [PATCH v6 3/3] nbd: fix outdated qapi docs syntax for tls-creds

[Eric Blake]

On 2/27/19 10:20 AM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/block.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 79623088e7..145c268bb6 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -224,7 +224,7 @@
>  # QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
>  #
>  # @addr: Address on which to listen.
> -# @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
> +# @tls-creds: ID of the TLS credentials object (since 2.6).

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

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

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

[Daniel P. Berrangé]

On Wed, Feb 27, 2019 at 10:43:40AM -0600, Eric Blake wrote:
> On 2/27/19 10:20 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.
> > 
> 
> > +++ b/qemu-nbd.c
> 
> > @@ -103,6 +105,7 @@ static void usage(const char *name)
> >  "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
> >  "                            passwords and/or encryption keys\n"
> >  "  --tls-creds=ID            use id of an earlier --object to provide TLS\n"
> > +"  --tls-authz=ID            use id of an earlier --object to provide authorization\n"
> 
> Usage line exceeds 80 columns; I don't mind splitting the line.

Odd that checkpatch.pl didn't complain about this.

> > +== check TLS with authorization ==
> > +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> > +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> 
> A rather uninformative message for the client to figure out why it
> failed, but (as with all things security-related), giving too many
> details may in itself be an improper information leak. At any rate, I
> don't know that you could make it work any better, so it is not a
> problem with this patch.  It may be the sign of a server bug for closing
> the socket too soon (before the client has a chance to read an actual
> error message), where we could tweak things to provoke a nicer error
> than 'Software caused connection abort', but that would be a separate patch.

I'm not sure how we'd do anything better here. From NBD's pov the TLS
handshake failed to complete. At that point NBD can't assume that it is
able to successfully able to send anything over the connection, so has
little choice but to close it.

To some extent this is a result of the QIOChannelTLS API design, because
in acutal fact we've completed the TLS protocol handshake, so we do
actually have an established TLS connection, but we don't consider the
client authorized.

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

[Eric Blake]

On 2/27/19 10:20 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <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.

It doesn't hold up this patch, but I note that with the qemu QMP command
changes you make in 2/3, you document that the object can be
created/removed on the fly, and the server will adjust which clients can
then subsequently connect. Is there any need for the same sort of
runtime configurability in qemu-nbd, and if so, how would we accomplish
it?  Perhaps by having a command-line option to parse --tls-authz from a
file, where you can send SIGHUP to qemu-nbd to force it to re-read the
file?  Or am I worrying about something unlikely to be needed in practice?

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

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

[Daniel P. Berrangé]

On Thu, Feb 28, 2019 at 12:11:00PM -0600, Eric Blake wrote:
> On 2/27/19 10:20 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.
> 
> It doesn't hold up this patch, but I note that with the qemu QMP command
> changes you make in 2/3, you document that the object can be
> created/removed on the fly, and the server will adjust which clients can
> then subsequently connect. Is there any need for the same sort of
> runtime configurability in qemu-nbd, and if so, how would we accomplish
> it?  Perhaps by having a command-line option to parse --tls-authz from a
> file, where you can send SIGHUP to qemu-nbd to force it to re-read the
> file?  Or am I worrying about something unlikely to be needed in practice?

Well the QAuthZListFile object type can store its contents in an external
file that gets auto-reloaded upon inotify triggers from the main loop.
The QAuthZPAM type can also be fairly dynamic depending on its backend.
I think any single process is unlikely  to need to switch between
different object types, so this is good enough dynamic support.

I can't help thinking we should add QMP to qemu-nbd one day though....

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

[Eric Blake]

On 2/27/19 10:43 AM, Eric Blake wrote:

>>  @example
>>  qemu-nbd \
>>    --object tls-creds-x509,id=tls0,endpoint=server,dir=/path/to/qemutls \
>> -  --tls-creds tls0 -t -x subset -p 10810 \
>> +  --object 'authz-simple,id=auth0,identity=CN=laptop.example.com,,\
>> +            O=Example Org,,L=London,,ST=London,,C=GB' \
> 
> A long line may be necessary here, unless the whitespace in the
> identity= parameter inserted by the line continuation is harmless.  Long
> lines in man pages are annoying, but even worse is an example that
> copies-and-pastes incorrectly.  I may just s/^ *O/O/.

I've just confirmed that whitespace in the identity= parameter is
harmless, via this change:

diff --git i/tests/qemu-iotests/233 w/tests/qemu-iotests/233
index 6adade45353..5e5fe1e8cdb 100755
--- i/tests/qemu-iotests/233
+++ w/tests/qemu-iotests/233
@@ -131,7 +131,8 @@ nbd_server_stop

 nbd_server_start_tcp_socket \
     --object
tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes
\
-    --object "authz-simple,identity=CN=localhost,,O=Cthulu Dark Lord
Enterprises client1,,L=R'lyeh,,C=South Pacific,id=authz0" \
+    --object "authz-simple,id=authz0,identity=CN=localhost,, \
+      O=Cthulu Dark Lord Enterprises client1,,L=R'lyeh,,C=South Pacific" \
     --tls-authz authz0 \
     --tls-creds tls0 \
     -f $IMGFMT "$TEST_IMG" 2>> "$TEST_DIR/server.log"


So I'll go ahead and tweak the patch along those lines.

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

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

[Eric Blake]

On 2/28/19 12:18 PM, Daniel P. Berrangé wrote:

>> It doesn't hold up this patch, but I note that with the qemu QMP command
>> changes you make in 2/3, you document that the object can be
>> created/removed on the fly, and the server will adjust which clients can
>> then subsequently connect. Is there any need for the same sort of
>> runtime configurability in qemu-nbd, and if so, how would we accomplish
>> it?  Perhaps by having a command-line option to parse --tls-authz from a
>> file, where you can send SIGHUP to qemu-nbd to force it to re-read the
>> file?  Or am I worrying about something unlikely to be needed in practice?
> 
> Well the QAuthZListFile object type can store its contents in an external
> file that gets auto-reloaded upon inotify triggers from the main loop.
> The QAuthZPAM type can also be fairly dynamic depending on its backend.
> I think any single process is unlikely  to need to switch between
> different object types, so this is good enough dynamic support.

That, and QMP nbd-server-start can serve up multiple exports, while
qemu-nbd serves exactly one export.

I also note that my work on incremental backups has raised the topic
that we someday want to support multiple parallel NBD servers.  Right
now, you are limited to exactly one (even though it can server multiple
exports), which makes serving different content to different clients
harder than if you could have different servers on separate ports with
per-server settings on which clients to allow.  Or, if we have tls-authz
items per-export instead of per-server, so that different clients see a
different subset of available exports when doing NBD_OPT_LIST.

> 
> I can't help thinking we should add QMP to qemu-nbd one day though....

Yeah, as QMP nbd server gets more powerful, we may eventually need a
similar monitor interface into controlling a long-running qemu-nbd
process...

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

Re: [Qemu-devel] [PATCH v6 0/3] nbd: support for authorization control on TLS connections

[Eric Blake]

On 2/27/19 10:20 AM, Daniel P. Berrangé wrote:
> This series provides the NBD parts of the authorization control series
> previously posted as:
> 
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04482.html
>   v2: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05727.html
>   v3: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01639.html
>   v4: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg04319.html
> 
> Then separated for merge:
> 
>   v5: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07347.html
> 
> The core authz framework is now merged & these patches have all had
> positive review. Thus these NBD parts are ready to go into the NBD
> maintainer's tree, should the maintainer consider them acceptable.

Thanks; series is queued for my next pull request.

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