[Qemu-devel] [PATCH 0/3] qemu-nbd: Disallow listing exports

[Qemu-devel] [PATCH 0/3] qemu-nbd: Disallow listing exports

[Nir Soffer]

oVirt uses random URLs to expose images temporarily via HTTPS. We would
like to integrated qemu-nbd in the same system, proving a user an easy
and uniform way to access an image - either using HTTPS:

    https://server:54322/images/dc72d3cc-b933-45e8-89a2-e028e1c2ef3d

Or using NBD over TLS:

    nbd://server:10809/dc72d3cc-b933-45e8-89a2-e028e1c2ef3d

Unfortunatly, qemu-nbd allows listing exports by default. Allowing
anyone to find the secret export using easy to guess port number.

These patches:
- add --nolist option to qemu-nbd, disabling NBD_OPT_LIST command.
- add some infrastructure ot iotests.py
- and use the new infrastructure to add test the new option using
  nbd-client.

Adding dependency on nbd-client may be probelematic, but I think
qemu-nbd should have tests ensuring compatibility with other tools.

Nir Soffer (3):
  nbd: Add option to disallow listing exports
  iotests.py: Add helper for running commands
  qemu-iotests: Test new qemu-nbd --nolist option

 blockdev-nbd.c                |  2 +-
 include/block/nbd.h           |  1 +
 nbd/server.c                  |  7 +++++++
 qemu-nbd.c                    |  9 ++++++++-
 qemu-nbd.texi                 |  2 ++
 tests/qemu-iotests/214        | 46 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/214.out    |  2 ++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py | 18 +++++++++++++++++
 9 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/214
 create mode 100644 tests/qemu-iotests/214.out

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] nbd: Add option to disallow listing exports

[Nir Soffer]

When a management application expose images using qemu-nbd, it needs a
secure way to allow temporary access to the disk. Using a random export
name can solve this problem:

    nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433

Assuming that the url is passed to the user in a secure way, and the
user is using TLS to access the image.

However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find
the secret export:

    $ nbd-client -l server 10809
    Negotiation: ..
    22965f19-9ab5-4d18-94e1-cbeb321fa433

Add a new --nolist option, disabling listing, similar the "allowlist"
nbd-server configuration option.

When used, listing exports will fail like this:

    $ nbd-client -l localhost 10809
    Negotiation: ..

    E: listing not allowed by server.
    Server said: Listing exports is forbidden

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 blockdev-nbd.c      | 2 +-
 include/block/nbd.h | 1 +
 nbd/server.c        | 7 +++++++
 qemu-nbd.c          | 9 ++++++++-
 qemu-nbd.texi       | 2 ++
 5 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 65a84739ed..b9a885dc4b 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -37,7 +37,7 @@ 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, NULL, true,
                    nbd_blockdev_client_closed);
 }
 
diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..5c6b6272a0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -308,6 +308,7 @@ void nbd_client_new(NBDExport *exp,
                     QIOChannelSocket *sioc,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsaclname,
+                    bool allow_list,
                     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..7b91922d1d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -115,6 +115,7 @@ struct NBDClient {
 
     bool structured_reply;
     NBDExportMetaContexts export_meta;
+    bool allow_list;
 
     uint32_t opt; /* Current option being negotiated */
     uint32_t optlen; /* remaining length of data in ioc for the option being
@@ -1032,6 +1033,10 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
             case NBD_OPT_LIST:
                 if (length) {
                     ret = nbd_reject_length(client, false, errp);
+                } else if (!client->allow_list) {
+                    ret = nbd_negotiate_send_rep_err(client,
+                                                     NBD_REP_ERR_POLICY, errp,
+                                                     "Listing exports is forbidden");
                 } else {
                     ret = nbd_negotiate_handle_list(client, errp);
                 }
@@ -2141,6 +2146,7 @@ void nbd_client_new(NBDExport *exp,
                     QIOChannelSocket *sioc,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsaclname,
+                    bool allow_list,
                     void (*close_fn)(NBDClient *, bool))
 {
     NBDClient *client;
@@ -2158,6 +2164,7 @@ void nbd_client_new(NBDExport *exp,
     object_ref(OBJECT(client->sioc));
     client->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(client->ioc));
+    client->allow_list = allow_list;
     client->close_fn = close_fn;
 
     co = qemu_coroutine_create(nbd_co_client_start, client);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0af0560ad1..b63d4d9e8b 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_NOLIST        264
 
 #define MBR_SIZE 512
 
@@ -66,6 +67,7 @@ static int shared = 1;
 static int nb_fds;
 static QIONetListener *server;
 static QCryptoTLSCreds *tlscreds;
+static bool allow_list = true;
 
 static void usage(const char *name)
 {
@@ -86,6 +88,7 @@ static void usage(const char *name)
 "  -v, --verbose             display extra debugging information\n"
 "  -x, --export-name=NAME    expose export by name\n"
 "  -D, --description=TEXT    with -x, also export a human-readable description\n"
+"      --nolist              do not list export\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
@@ -355,7 +358,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, NULL, allow_list, nbd_client_closed);
 }
 
 static void nbd_update_server_watch(void)
@@ -523,6 +526,7 @@ int main(int argc, char **argv)
         { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", required_argument, NULL, 'x' },
         { "description", required_argument, NULL, 'D' },
+        { "nolist", no_argument, NULL, QEMU_NBD_OPT_NOLIST },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { "trace", required_argument, NULL, 'T' },
@@ -717,6 +721,9 @@ int main(int argc, char **argv)
         case 'D':
             export_description = optarg;
             break;
+        case QEMU_NBD_OPT_NOLIST:
+            allow_list = false;
+            break;
         case 'v':
             verbose = 1;
             break;
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed..010b29588f 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -85,6 +85,8 @@ the new style NBD protocol negotiation
 @item -D, --description=@var{description}
 Set the NBD volume export description, as a human-readable
 string. Requires the use of @option{-x}
+@item --nolist
+Do not allow the client to fetch a list of exports from this server.
 @item --tls-creds=ID
 Enable mandatory TLS encryption for the server by setting the ID
 of the TLS credentials object previously created with the --object
-- 
2.14.3

[Qemu-devel] [PATCH 2/3] iotests.py: Add helper for running commands

[Nir Soffer]

Add few helpers for running external commands:

- CommandFailed: exception, keeping all the info related to a failed
  command, and providing a useful error message. (Unfortunately
  subprocess.CalledProcessError does not).

- run(): run a command collecting output from the underlying process
  stdout and stderr, returning the command output or raising
  CommandFailed.

These helpers will be used by new qemu-nbd tests. And later can be used
to cleanup helpers for running qemu-* tools in iotests.py.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 tests/qemu-iotests/iotests.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b25d48a91b..0f8abf99cb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -64,6 +64,24 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
                              os.environ['IMGKEYSECRET']
 luks_default_key_secret_opt = 'key-secret=keysec0'
 
+class CommandFailed(Exception):
+
+    def __init__(self, cmd, rc, out, err):
+        self.cmd = cmd
+        self.rc = rc
+        self.out = out
+        self.err = err
+
+    def __str__(self):
+        return ("Command {self.cmd} failed: rc={self.rc}, out={self.out!r}, "
+                "err={self.err!r}").format(self=self)
+
+def run(*args):
+    p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    out, err = p.communicate()
+    if p.returncode != 0:
+        raise CommandFailed(args, p.returncode, out, err)
+    return out
 
 def qemu_img(*args):
     '''Run qemu-img and return the exit code'''
-- 
2.14.3

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test new qemu-nbd --nolist option

[Nir Soffer]

Add new test module for tesing the --nolist option.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 tests/qemu-iotests/214     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/214.out |  2 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 49 insertions(+)
 create mode 100755 tests/qemu-iotests/214
 create mode 100644 tests/qemu-iotests/214.out

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
new file mode 100755
index 0000000000..779e382070
--- /dev/null
+++ b/tests/qemu-iotests/214
@@ -0,0 +1,46 @@
+#!/usr/bin/env python
+#
+# Test qemu-nbd compatibility with other tools.
+#
+# Copyright (C) 2018 Nir Soffer <nirsof@gmail.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+
+iotests.verify_image_format(supported_fmts=['raw'])
+
+iotests.log('Check that listing exports is allowed by default')
+disk, nbd_sock = iotests.file_path('disk1', 'nbd-sock1')
+iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
+iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'export', disk)
+out = iotests.run('nbd-client', '-l', '--unix', nbd_sock)
+
+assert 'export' in out.splitlines(), 'Export not in %r' % out
+
+iotests.log('Check that listing exports is forbidden with --nolist')
+disk, nbd_sock = iotests.file_path('disk2', 'nbd-sock2')
+iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
+iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'secret',
+                 '--nolist', disk)
+
+# nbd-client fails when listing is not allowed, but lets not depend on 3rd
+# party tool behavior here.
+try:
+    out = iotests.run('nbd-client', '-l', '--unix', nbd_sock)
+    assert 'secret' not in out, 'Export in %r' % out
+except iotests.CommandFailed as e:
+    # This text comes from qemu-nbd.
+    assert 'Listing exports is forbidden' in e.err, 'Unexpected error: %s' % e
diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
new file mode 100644
index 0000000000..dae61b5a57
--- /dev/null
+++ b/tests/qemu-iotests/214.out
@@ -0,0 +1,2 @@
+Check that listing exports is allowed by default
+Check that listing exports is forbidden with --nolist
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 52a80f3f9e..a820dcb91f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -212,3 +212,4 @@
 211 rw auto quick
 212 rw auto quick
 213 rw auto quick
+214 rw auto quick
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/3] nbd: Add option to disallow listing exports

[Richard W.M. Jones]

On Fri, Apr 13, 2018 at 10:26:03PM +0300, Nir Soffer wrote:
> When a management application expose images using qemu-nbd, it needs a
> secure way to allow temporary access to the disk. Using a random export
> name can solve this problem:
> 
>     nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433
> 
> Assuming that the url is passed to the user in a secure way, and the
> user is using TLS to access the image.
> 
> However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find
> the secret export:
> 
>     $ nbd-client -l server 10809
>     Negotiation: ..
>     22965f19-9ab5-4d18-94e1-cbeb321fa433
> 
> Add a new --nolist option, disabling listing, similar the "allowlist"
> nbd-server configuration option.
> 
> When used, listing exports will fail like this:
> 
>     $ nbd-client -l localhost 10809
>     Negotiation: ..
> 
>     E: listing not allowed by server.
>     Server said: Listing exports is forbidden
> 
> Signed-off-by: Nir Soffer <nirsof@gmail.com>

Tested-by: Richard W.M. Jones <rjones@redhat.com>

The code looks good to me too, so ACK.

Rich.

>  blockdev-nbd.c      | 2 +-
>  include/block/nbd.h | 1 +
>  nbd/server.c        | 7 +++++++
>  qemu-nbd.c          | 9 ++++++++-
>  qemu-nbd.texi       | 2 ++
>  5 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 65a84739ed..b9a885dc4b 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -37,7 +37,7 @@ 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, NULL, true,
>                     nbd_blockdev_client_closed);
>  }
>  
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index fcdcd54502..5c6b6272a0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -308,6 +308,7 @@ void nbd_client_new(NBDExport *exp,
>                      QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsaclname,
> +                    bool allow_list,
>                      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..7b91922d1d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -115,6 +115,7 @@ struct NBDClient {
>  
>      bool structured_reply;
>      NBDExportMetaContexts export_meta;
> +    bool allow_list;
>  
>      uint32_t opt; /* Current option being negotiated */
>      uint32_t optlen; /* remaining length of data in ioc for the option being
> @@ -1032,6 +1033,10 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>              case NBD_OPT_LIST:
>                  if (length) {
>                      ret = nbd_reject_length(client, false, errp);
> +                } else if (!client->allow_list) {
> +                    ret = nbd_negotiate_send_rep_err(client,
> +                                                     NBD_REP_ERR_POLICY, errp,
> +                                                     "Listing exports is forbidden");
>                  } else {
>                      ret = nbd_negotiate_handle_list(client, errp);
>                  }
> @@ -2141,6 +2146,7 @@ void nbd_client_new(NBDExport *exp,
>                      QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsaclname,
> +                    bool allow_list,
>                      void (*close_fn)(NBDClient *, bool))
>  {
>      NBDClient *client;
> @@ -2158,6 +2164,7 @@ void nbd_client_new(NBDExport *exp,
>      object_ref(OBJECT(client->sioc));
>      client->ioc = QIO_CHANNEL(sioc);
>      object_ref(OBJECT(client->ioc));
> +    client->allow_list = allow_list;
>      client->close_fn = close_fn;
>  
>      co = qemu_coroutine_create(nbd_co_client_start, client);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0af0560ad1..b63d4d9e8b 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_NOLIST        264
>  
>  #define MBR_SIZE 512
>  
> @@ -66,6 +67,7 @@ static int shared = 1;
>  static int nb_fds;
>  static QIONetListener *server;
>  static QCryptoTLSCreds *tlscreds;
> +static bool allow_list = true;
>  
>  static void usage(const char *name)
>  {
> @@ -86,6 +88,7 @@ static void usage(const char *name)
>  "  -v, --verbose             display extra debugging information\n"
>  "  -x, --export-name=NAME    expose export by name\n"
>  "  -D, --description=TEXT    with -x, also export a human-readable description\n"
> +"      --nolist              do not list export\n"
>  "\n"
>  "Exposing part of the image:\n"
>  "  -o, --offset=OFFSET       offset into the image\n"
> @@ -355,7 +358,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, NULL, allow_list, nbd_client_closed);
>  }
>  
>  static void nbd_update_server_watch(void)
> @@ -523,6 +526,7 @@ int main(int argc, char **argv)
>          { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
>          { "export-name", required_argument, NULL, 'x' },
>          { "description", required_argument, NULL, 'D' },
> +        { "nolist", no_argument, NULL, QEMU_NBD_OPT_NOLIST },
>          { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
>          { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>          { "trace", required_argument, NULL, 'T' },
> @@ -717,6 +721,9 @@ int main(int argc, char **argv)
>          case 'D':
>              export_description = optarg;
>              break;
> +        case QEMU_NBD_OPT_NOLIST:
> +            allow_list = false;
> +            break;
>          case 'v':
>              verbose = 1;
>              break;
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 9a84e81eed..010b29588f 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -85,6 +85,8 @@ the new style NBD protocol negotiation
>  @item -D, --description=@var{description}
>  Set the NBD volume export description, as a human-readable
>  string. Requires the use of @option{-x}
> +@item --nolist
> +Do not allow the client to fetch a list of exports from this server.
>  @item --tls-creds=ID
>  Enable mandatory TLS encryption for the server by setting the ID
>  of the TLS credentials object previously created with the --object
> -- 
> 2.14.3

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 1/3] nbd: Add option to disallow listing exports

[Daniel P. Berrangé]

On Fri, Apr 13, 2018 at 10:26:03PM +0300, Nir Soffer wrote:
> When a management application expose images using qemu-nbd, it needs a
> secure way to allow temporary access to the disk. Using a random export
> name can solve this problem:
> 
>     nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433
> 
> Assuming that the url is passed to the user in a secure way, and the
> user is using TLS to access the image.
> 
> However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find
> the secret export:
> 
>     $ nbd-client -l server 10809
>     Negotiation: ..
>     22965f19-9ab5-4d18-94e1-cbeb321fa433
> 
> Add a new --nolist option, disabling listing, similar the "allowlist"
> nbd-server configuration option.
> 
> When used, listing exports will fail like this:
> 
>     $ nbd-client -l localhost 10809
>     Negotiation: ..
> 
>     E: listing not allowed by server.
>     Server said: Listing exports is forbidden

Essentially this is abusing the export name as a crude authentication
token. There are NBD servers that expect NBD_OPT_LIST to always succeeed
when they detect that the new style protocol is available. I really hate
the idea of making it possible to break the NBD_OPT_LIST functionality
via a command line arg like this.

Furthermore, applications are *not* considering the export names to be
security sensitive data, so will not be taking any precautions to ensure
they remain secret, as they would do with authentication credentials.
Again I really hate the idea of using NBD exports an an auth credential.

So I don't think we should be suggesting that security through obscurity of
the export name is a supported approach to securing NBD.

I understand the desire to be able to secure NBD exports though, so think
we need to come up with some kind of supportable solution for this. There
are two approaches we should take

 - Add support for TLS client certification whitelisting. eg every client
   has a unique identity based on the distinguished name (dname) in the
   x509 cert they were issued. The NBD server can be told which of these
   dnames should be a permitted to connect. This is supported in VNC for
   years, and I've had patches pending to support this in a QEMU for chardevs
   NBD and migration for a while. These were stalled on way to convert
   -object ... syntax into nested QOM objects.

 - Define a mapping of the SASL protocol ontop NBD. SASL is a generic pluggable
   authentication mechanism for network protocols. It is already used in
   libvirt, VNC and SPICE, and would easily fit in with NBD from a conceptual
   POV. When used in combination with TLS, this offers a wide range of auth
   mechanisms from simple username+password, to full integration with Kerberos.

If this need is urgent, I think we could partially unblock the TLS x509
whitelisting support without much difficulty. We haven't been pushing hard
to unblock it simply because no one was urgently blocked by its absence
so far. This provides a strong solution, but the difficulty is that the
server may not know the x509 dname of the permitted client, which makes
it hard to use in practice. SASL with a simple username+password scheme
is thus still very compelling to implement, but will obviously  take longer
due to the amount of code/spec work required.

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/3] nbd: Add option to disallow listing exports

[Richard W.M. Jones]

On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote:
> Essentially this is abusing the export name as a crude authentication
> token. There are NBD servers that expect NBD_OPT_LIST to always succeeed

I guess you mean "NBD clients" ...

> when they detect that the new style protocol is available. I really hate
> the idea of making it possible to break the NBD_OPT_LIST functionality
> via a command line arg like this.

The specific use case I have in mind is virt-v2v forked an instance of
‘qemu-img convert’ which connects to the NBD server.

Of course this does also reveal a flaw in the plan because ...

> Furthermore, applications are *not* considering the export names to be
> security sensitive data, so will not be taking any precautions to ensure
> they remain secret, as they would do with authentication credentials.
> Again I really hate the idea of using NBD exports an an auth credential.

‘ps ax’ on the conversion server will reveal the export name/ticket
from the qemu-img command line.

> So I don't think we should be suggesting that security through obscurity of
> the export name is a supported approach to securing NBD.
> 
> I understand the desire to be able to secure NBD exports though, so think
> we need to come up with some kind of supportable solution for this. There
> are two approaches we should take
> 
>  - Add support for TLS client certification whitelisting. eg every client
>    has a unique identity based on the distinguished name (dname) in the
>    x509 cert they were issued. The NBD server can be told which of these
>    dnames should be a permitted to connect. This is supported in VNC for
>    years, and I've had patches pending to support this in a QEMU for chardevs
>    NBD and migration for a while. These were stalled on way to convert
>    -object ... syntax into nested QOM objects.
>
>  - Define a mapping of the SASL protocol ontop NBD. SASL is a
>    generic pluggable authentication mechanism for network
>    protocols. It is already used in libvirt, VNC and SPICE, and
>    would easily fit in with NBD from a conceptual POV. When used in
>    combination with TLS, this offers a wide range of auth mechanisms
>    from simple username+password, to full integration with Kerberos.

The first one sounds heavyweight but at least implementable from the
virt-v2v point of view.  The second one sounds like it would be
impossible for mere humans to set it up.

> If this need is urgent, I think we could partially unblock the TLS x509
> whitelisting support without much difficulty. We haven't been pushing hard
> to unblock it simply because no one was urgently blocked by its absence
> so far. This provides a strong solution, but the difficulty is that the
> server may not know the x509 dname of the permitted client, which makes
> it hard to use in practice.

Can you clarify what you mean by the last sentence above?  Can't we
just create a client certificate in virt-v2v and pass that to
qemu-img, and at the same time pass the server a list of permitted
names? (likely only a single name in practice)

> SASL with a simple username+password scheme
> is thus still very compelling to implement, but will obviously  take longer
> due to the amount of code/spec work required.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 1/3] nbd: Add option to disallow listing exports

[Daniel P. Berrangé]

On Mon, Apr 16, 2018 at 11:53:41AM +0100, Richard W.M. Jones wrote:
> On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote:
> > Essentially this is abusing the export name as a crude authentication
> > token. There are NBD servers that expect NBD_OPT_LIST to always succeeed
> 
> I guess you mean "NBD clients" ...

Sigh, yes, of course.

> > when they detect that the new style protocol is available. I really hate
> > the idea of making it possible to break the NBD_OPT_LIST functionality
> > via a command line arg like this.
> 
> The specific use case I have in mind is virt-v2v forked an instance of
> ‘qemu-img convert’ which connects to the NBD server.
> 
> Of course this does also reveal a flaw in the plan because ...
> 
> > Furthermore, applications are *not* considering the export names to be
> > security sensitive data, so will not be taking any precautions to ensure
> > they remain secret, as they would do with authentication credentials.
> > Again I really hate the idea of using NBD exports an an auth credential.
> 
> ‘ps ax’ on the conversion server will reveal the export name/ticket
> from the qemu-img command line.

Yeah, exactly the kind of problem that hits when you mis-use a piece of
traditionally public info as a security credential.

> 
> > So I don't think we should be suggesting that security through obscurity of
> > the export name is a supported approach to securing NBD.
> > 
> > I understand the desire to be able to secure NBD exports though, so think
> > we need to come up with some kind of supportable solution for this. There
> > are two approaches we should take
> > 
> >  - Add support for TLS client certification whitelisting. eg every client
> >    has a unique identity based on the distinguished name (dname) in the
> >    x509 cert they were issued. The NBD server can be told which of these
> >    dnames should be a permitted to connect. This is supported in VNC for
> >    years, and I've had patches pending to support this in a QEMU for chardevs
> >    NBD and migration for a while. These were stalled on way to convert
> >    -object ... syntax into nested QOM objects.
> >
> >  - Define a mapping of the SASL protocol ontop NBD. SASL is a
> >    generic pluggable authentication mechanism for network
> >    protocols. It is already used in libvirt, VNC and SPICE, and
> >    would easily fit in with NBD from a conceptual POV. When used in
> >    combination with TLS, this offers a wide range of auth mechanisms
> >    from simple username+password, to full integration with Kerberos.
> 
> The first one sounds heavyweight but at least implementable from the
> virt-v2v point of view.  The second one sounds like it would be
> impossible for mere humans to set it up.

You'll want TLS no matter what really. All SASL mechanisms, with the
exception of Kerberos, require that you have a secure data channel
first - which means either UNIX sockets, or TCP with TLS.

If you're using SASL for auth you can, however, avoid the need to
require x509 client certs.

> > If this need is urgent, I think we could partially unblock the TLS x509
> > whitelisting support without much difficulty. We haven't been pushing hard
> > to unblock it simply because no one was urgently blocked by its absence
> > so far. This provides a strong solution, but the difficulty is that the
> > server may not know the x509 dname of the permitted client, which makes
> > it hard to use in practice.
> 
> Can you clarify what you mean by the last sentence above?  Can't we
> just create a client certificate in virt-v2v and pass that to
> qemu-img, and at the same time pass the server a list of permitted
> names? (likely only a single name in practice)

I just mean that at the time the mgmt app sets up the NBD export, it might
not know which client is going to use it, so it can't setup a x509 dname
whitelist at that time. 

With SASL and username/password, you don't need to know who will use the
export at setup time - you can simply give up username/password at time
of use.


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/3] nbd: Add option to disallow listing exports

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On 04/13/2018 02:26 PM, Nir Soffer wrote:
> When a management application expose images using qemu-nbd, it needs a
> secure way to allow temporary access to the disk. Using a random export
> name can solve this problem:
> 
>     nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433

I share Dan's concerns that you are trying to protect information
without requiring TLS.  If you would just use TLS, then only clients
that can authenticate can list the export names; the fact that the name
leaks at all means you aren't using TLS, so you are just as vulnerable
to a man-in-the-middle attack as you are to the information leak.

> 
> Assuming that the url is passed to the user in a secure way, and the
> user is using TLS to access the image.
> 
> However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find
> the secret export:
> 
>     $ nbd-client -l server 10809
>     Negotiation: ..
>     22965f19-9ab5-4d18-94e1-cbeb321fa433

If the server requires TLS, then 'nbd-client -l' already cannot list
names without first negotiating TLS (all commands other than
NBD_OPT_STARTTLS are rejected with NBD_REP_ERR_TLS_REQD if the server
required TLS).  Your example is thus invalidating your above assumption
that the user is using TLS.

> 
> Add a new --nolist option, disabling listing, similar the "allowlist"
> nbd-server configuration option.

This may still make sense to implement, but not necessarily for the
reasons you are giving.


> @@ -86,6 +88,7 @@ static void usage(const char *name)
>  "  -v, --verbose             display extra debugging information\n"
>  "  -x, --export-name=NAME    expose export by name\n"
>  "  -D, --description=TEXT    with -x, also export a human-readable description\n"
> +"      --nolist              do not list export\n"

s/export/exports/

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/3] nbd: Add option to disallow listing exports

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On 04/16/2018 06:00 AM, Daniel P. Berrangé wrote:
> On Mon, Apr 16, 2018 at 11:53:41AM +0100, Richard W.M. Jones wrote:
>> On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote:
>>> Essentially this is abusing the export name as a crude authentication
>>> token. There are NBD servers that expect NBD_OPT_LIST to always succeeed
>>
>> I guess you mean "NBD clients" ...
> 
> Sigh, yes, of course.

qemu 2.10 and older tries to use NBD_OPT_LIST, but gracefully still
tries to connect even if the LIST fails (that is, it's use of
NBD_OPT_LIST was for better error handling than what NBD_OPT_EXPORT_NAME
gives, and not because it actually needed the list).  The recent
introduction in qemu 2.11 for support of NBD_OPT_GO means that modern
qemu is no longer even attempting NBD_OPT_LIST when talking to a new
server.  But cross-implementation compatibility is still a concern, and
there may indeed be non-qemu clients that choke if LIST fails, even
though...

> 
>>> when they detect that the new style protocol is available. I really hate
>>> the idea of making it possible to break the NBD_OPT_LIST functionality
>>> via a command line arg like this.

...the NBD spec suggests that a client that requires LIST to work is not
fully compliant, since NBD_OPT_LIST is an optional feature.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test new qemu-nbd --nolist option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]

On 04/13/2018 02:26 PM, Nir Soffer wrote:
> Add new test module for tesing the --nolist option.
> 
> Signed-off-by: Nir Soffer <nirsof@gmail.com>
> ---

> +iotests.log('Check that listing exports is allowed by default')
> +disk, nbd_sock = iotests.file_path('disk1', 'nbd-sock1')
> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
> +iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'export', disk)
> +out = iotests.run('nbd-client', '-l', '--unix', nbd_sock)

Should we really be relying on the third-party nbd-client to be
installed?  Would it not be better to teach our own NBD client to learn
how to do interesting things over NBD?  Your use case of listing the
output of NBD_OPT_LIST is one, but another that readily comes to mind is
listing the possibilities of NBD_OPT_LIST_META_CONTEXT that just went
into 2.12.  Maybe making 'qemu-img info' give more details when
connecting to an NBD server, compared to what is normally needed just
for connecting to an export on that server?

Additionally, once we merge in Vladimir's work to expose persistent
dirty bitmaps via NBD_OPT_SET_META_CONTEXT/NBD_CMD_BLOCK_STATUS, it
would be nice to have an in-tree tool for reading out the context
information of an NBD export, perhaps extending what 'qemu-img map' can
already do (Vladimir already mentioned that he only implemented the
server side, and left the client side for an out-of-tree solution [1],
although I'm wondering if that is still the wisest course of action).

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05701.html

> +
> +assert 'export' in out.splitlines(), 'Export not in %r' % out
> +
> +iotests.log('Check that listing exports is forbidden with --nolist')
> +disk, nbd_sock = iotests.file_path('disk2', 'nbd-sock2')
> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
> +iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'secret',
> +                 '--nolist', disk)
> +
> +# nbd-client fails when listing is not allowed, but lets not depend on 3rd

s/lets/let's/

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test new qemu-nbd --nolist option

[Vladimir Sementsov-Ogievskiy]

17.04.2018 22:56, Eric Blake wrote:
> On 04/13/2018 02:26 PM, Nir Soffer wrote:
>> Add new test module for tesing the --nolist option.
>>
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>> ---
>> +iotests.log('Check that listing exports is allowed by default')
>> +disk, nbd_sock = iotests.file_path('disk1', 'nbd-sock1')
>> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
>> +iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'export', disk)
>> +out = iotests.run('nbd-client', '-l', '--unix', nbd_sock)
> Should we really be relying on the third-party nbd-client to be
> installed?  Would it not be better to teach our own NBD client to learn
> how to do interesting things over NBD?  Your use case of listing the
> output of NBD_OPT_LIST is one, but another that readily comes to mind is
> listing the possibilities of NBD_OPT_LIST_META_CONTEXT that just went
> into 2.12.  Maybe making 'qemu-img info' give more details when
> connecting to an NBD server, compared to what is normally needed just
> for connecting to an export on that server?
>
> Additionally, once we merge in Vladimir's work to expose persistent
> dirty bitmaps via NBD_OPT_SET_META_CONTEXT/NBD_CMD_BLOCK_STATUS, it
> would be nice to have an in-tree tool for reading out the context
> information of an NBD export, perhaps extending what 'qemu-img map' can
> already do (Vladimir already mentioned that he only implemented the
> server side, and left the client side for an out-of-tree solution [1],
> although I'm wondering if that is still the wisest course of action).
>
> [1]

Client part for base:allocation is done, and qemu-img map shows it, 
client part for dirty bitmaps export - isn't.


>
>> +
>> +assert 'export' in out.splitlines(), 'Export not in %r' % out
>> +
>> +iotests.log('Check that listing exports is forbidden with --nolist')
>> +disk, nbd_sock = iotests.file_path('disk2', 'nbd-sock2')
>> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, '1m')
>> +iotests.qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-x', 'secret',
>> +                 '--nolist', disk)
>> +
>> +# nbd-client fails when listing is not allowed, but lets not depend on 3rd
> s/lets/let's/
>


-- 
Best regards,
Vladimir