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

[Qemu-devel] [PATCH v5 0/2] 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

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.

Daniel P. Berrangé (2):
  qemu-nbd: add support for authorization of TLS clients
  nbd: allow authorization with nbd-server-start QMP command

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

-- 
2.20.1

[Qemu-devel] [PATCH v5 1/2] 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                 | 14 +++++++++++++-
 qemu-nbd.texi              |  4 ++++
 tests/qemu-iotests/233     | 31 ++++++++++++++++++++++++++++---
 tests/qemu-iotests/233.out | 11 +++++++++++
 6 files changed, 62 insertions(+), 10 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..0738073d95 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;
@@ -940,6 +947,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 (list) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index d0c5182814..d45ed9e0a0 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
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

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

[Eric Blake]

On 2/27/19 5:51 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Currently any client which can complete the TLS handshake is able to use
> the NBD server. The server admin can turn on the 'verify-peer' option
> for the x509 creds to require the client to provide a x509 certificate.
> This means the client will have to acquire a certificate from the CA
> before they are permitted to use the NBD server. This is still a fairly
> low bar to cross.
> 
> This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> takes the ID of a previously added 'QAuthZ' object instance. This will
> be used to validate the client's x509 distinguished name. Clients
> failing the authorization check will not be permitted to use the NBD
> server.
> 
> For example to setup authorization that only allows connection from a client
> whose x509 certificate distinguished name is
> 
>    CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> 
> 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.

I'm half-tempted to propose a QemuOpts patch that eats whitespace after
comma, to make examples like this actually valid command lines.
(Actually, your example breaks line after an escaped comma, which would
result in a literal 'identity=CN=...,   O=Example...' option even if
space is eaten, and I don't know if that forms a valid identity).  But
doesn't hold up this patch.

> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

> +++ b/qemu-nbd.c

> +    } else {
> +        if (tlsauthz) {
> +            error_report("--tls-authz is not permitted without --tls-creds");
> +            exit(EXIT_FAILURE);
> +        }
>      }
>  
>      if (list) {

--list is new compared to when you originally proposed this patch. Does
anything need to be changed on that front?  (That is, does --tls-authz
make sense when qemu-nbd is used as a client to probe a remote server's
advertisements, or is it a server-only setting where we need to enhance
the --list mode to reject use of the option?)

> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index d0c5182814..d45ed9e0a0 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

qemu-nbd.texi now has examples (which it did not have when you first
wrote the patch).  I'd love it if your command line example from the
commit message were also included in the examples section of the man page.


>  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

It looks like you have both a positive and negative test that a qemu-img
client can only connect with the right auth, but only a positive test
for a qemu-nbd --list client.  Should we also duplicate the
QEMU_NBD_PROG line to use $obj2 for a negative test?

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

Similarly, do we want --list testing here?

Overall the patch looks good, but I'm wondering if we want a v6 to
address the items pointed out above, or if a followup patch is sufficient.

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

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

[Daniel P. Berrangé]

On Wed, Feb 27, 2019 at 08:25:20AM -0600, Eric Blake wrote:
> On 2/27/19 5:51 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > Currently any client which can complete the TLS handshake is able to use
> > the NBD server. The server admin can turn on the 'verify-peer' option
> > for the x509 creds to require the client to provide a x509 certificate.
> > This means the client will have to acquire a certificate from the CA
> > before they are permitted to use the NBD server. This is still a fairly
> > low bar to cross.
> > 
> > This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> > takes the ID of a previously added 'QAuthZ' object instance. This will
> > be used to validate the client's x509 distinguished name. Clients
> > failing the authorization check will not be permitted to use the NBD
> > server.
> > 
> > For example to setup authorization that only allows connection from a client
> > whose x509 certificate distinguished name is
> > 
> >    CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> > 
> > 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.

> > +    } else {
> > +        if (tlsauthz) {
> > +            error_report("--tls-authz is not permitted without --tls-creds");
> > +            exit(EXIT_FAILURE);
> > +        }
> >      }
> >  
> >      if (list) {
> 
> --list is new compared to when you originally proposed this patch. Does
> anything need to be changed on that front?  (That is, does --tls-authz
> make sense when qemu-nbd is used as a client to probe a remote server's
> advertisements, or is it a server-only setting where we need to enhance
> the --list mode to reject use of the option?)

We should reject tls-creds when --list is set, since it only makes
sense on the server side, not client.

> > diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> > index d0c5182814..d45ed9e0a0 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
> 
> qemu-nbd.texi now has examples (which it did not have when you first
> wrote the patch).  I'd love it if your command line example from the
> commit message were also included in the examples section of the man page.

Ok.

> >  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
> 
> It looks like you have both a positive and negative test that a qemu-img
> client can only connect with the right auth, but only a positive test
> for a qemu-nbd --list client.  Should we also duplicate the
> QEMU_NBD_PROG line to use $obj2 for a negative test?

I can look at that

> 
> >  
> >  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
> 
> Similarly, do we want --list testing here?

I'll have a look.

> Overall the patch looks good, but I'm wondering if we want a v6 to
> address the items pointed out above, or if a followup patch is sufficient.

I'll do a v6 since there's still enough time before soft freeze.

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

[Daniel P. Berrangé]

On Wed, Feb 27, 2019 at 03:33:31PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 27, 2019 at 08:25:20AM -0600, Eric Blake wrote:
> > On 2/27/19 5:51 AM, Daniel P. Berrangé wrote:
> > > From: "Daniel P. Berrange" <berrange@redhat.com>
> > > 
> > > Currently any client which can complete the TLS handshake is able to use
> > > the NBD server. The server admin can turn on the 'verify-peer' option
> > > for the x509 creds to require the client to provide a x509 certificate.
> > > This means the client will have to acquire a certificate from the CA
> > > before they are permitted to use the NBD server. This is still a fairly
> > > low bar to cross.
> > > 
> > > This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> > > takes the ID of a previously added 'QAuthZ' object instance. This will
> > > be used to validate the client's x509 distinguished name. Clients
> > > failing the authorization check will not be permitted to use the NBD
> > > server.
> > > 
> > > For example to setup authorization that only allows connection from a client
> > > whose x509 certificate distinguished name is
> > > 
> > >    CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> > > 
> > > 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.


> > >  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
> > 
> > It looks like you have both a positive and negative test that a qemu-img
> > client can only connect with the right auth, but only a positive test
> > for a qemu-nbd --list client.  Should we also duplicate the
> > QEMU_NBD_PROG line to use $obj2 for a negative test?
> 
> I can look at that

On reflection I don't think that's needed. This is a server side feature we
are validating. As such it is sufficient to validate with any client app
supporting TLS. IOW, qemu-img is sufficient to prove the server side feature
is working correctly.

> 
> > 
> > >  
> > >  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
> > 
> > Similarly, do we want --list testing here?
> 
> I'll have a look.

For same reason as above, we don't need to repeat with qemu-nbd --list

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

[Eric Blake]

On 2/27/19 9:58 AM, Daniel P. Berrangé wrote:

>>>>  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
>>>
>>> It looks like you have both a positive and negative test that a qemu-img
>>> client can only connect with the right auth, but only a positive test
>>> for a qemu-nbd --list client.  Should we also duplicate the
>>> QEMU_NBD_PROG line to use $obj2 for a negative test?
>>
>> I can look at that
> 
> On reflection I don't think that's needed. This is a server side feature we
> are validating. As such it is sufficient to validate with any client app
> supporting TLS. IOW, qemu-img is sufficient to prove the server side feature
> is working correctly.

There's two things being validated in this test: does the server
properly honor authorization (accepting/denying clients as appropriate),
and does the client properly expose a way to pass correct credentials.
You're right that once we've tested that the server can reject at least
one client lacking credentials, we don't need any more negative tests
(the server will reject any other client without credentials); and as
long as both clients that we care about (qemu-img, qemu-nbd --list) have
at least one positive test, then we've covered that each important
client has a way to provide the required credentials.

Okay, I can live with your claim that we have sufficient coverage, and
don't need anything more with --list (for this test, where the server is
accepting regardless of client certificate).

>>>> +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
>>>
>>> Similarly, do we want --list testing here?
>>
>> I'll have a look.
> 
> For same reason as above, we don't need to repeat with qemu-nbd --list

Here, it's a bit fuzzier - now that the server DOES require the client
to use a correct certificate (you're showing that the server accepts
client 1 but rejects client 2, because they used a different
certificate).  But, since client 1 is the same here as it was before,
and the earlier test showed how to let qemu-nbd --list as client give
the same credentials as before, I can also live with the notion that we
don't need a --list test here (the server being stricter about
certificates doesn't change that the client can still pass the correct
certificate).

Okay.

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

[Qemu-devel] [PATCH v5 2/2] 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: 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..5fa0782dfd 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

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

[Eric Blake]

On 2/27/19 5:51 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.
> 

> 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'
>      }
>    }
> 

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

Pre-existing formatting nit - per-variable release notes tend to be
wrapped in (), as in '(since 4.0)'. tls-creds would need fixing, if we
also want that for tls-authz.  But we aren't 100% consistent, and it is
minor, so it does not stop me from:

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

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