Skip to contentSkip to conversation

Implement TLS support to QEMU NBD server & client

PATCH Superseded Daniel P. Berrange 2 participants 23 messages Nov 27, 2015 – Dec 3, 2015

Subsystems (2)

v1 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

cover Implement TLS support to QEMU NBD server & client

This series of patches implements support for TLS in the QEMU NBD server and client code.

It is implementing the NBD_OPT_STARTTLS option that was previously discussed here:

https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html

And is also described in the NBD spec here:

https://github.com/yoe/nbd/blob/master/doc/proto.md

Based on my impl I think the NBD_OPT_STARTTLS specification is fine. In my impl, when TLS is requested in the server, I chose to *refuse* all NBD options until NBD_OPT_STARTTLS has completed successfully. There is an annoying thing about the fixed new style protocol in that it made an exception for NBD_OPT_EXPORT_NAME, so that it is still not possible to report errors for that option.

[qupte]
- The server will reply to any option apart from NBD_OPT_EXPORT_NAME with reply packets in the following format:

S: 64 bits, 0x3e889045565a9 (magic number for replies)
S: 32 bits, the option as sent by the client to which this is a reply
S: 32 bits, reply type (e.g., NBD_REP_ACK for successful completion,
or NBD_REP_ERR_UNSUP to mark use of an option not known by this server
S: 32 bits, length of the reply. This may be zero for some replies, in
which case the next field is not sent
S: any data as required by the reply (e.g., an export name in the case
of NBD_REP_SERVER)
[/quote]

I wish those words "apart from NBD_OPT_EXPORT_NAME" were not there for fixed new style protocol - maybe it needs a very-fixed new style protocol...

As is, if the client connects to a TLS enabled NBD server and then immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us to send back NBD_REP_ERR_TLS_REQD as the spec requires that the server close the connection :-( For this reason I have made the qemu NBD client always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response.

I chose to *NOT* implement the NBD_OPT_PEEK_EXPORT option as it is inherently insecure to have a client to ask the server which exports need TLS, while the protocol is still running in plain text mode, as it allows a trivial MITM downgrade attack. I can see value in the NBD_OPT_PEEK_EXPORT option for probing other features, but only after TLS has been negotiated. As such I believe NBD_F_EXP_TLS_OK and NBD_F_EXP_TLS_REQ should be deleted from the spec as it is not possible to use them in a secure manner.

This series of patches has my v3 I/O channels patch series as a pre-requisite:

https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04208.html

Usage of TLS is described in the commit messages for each patch, but for sake of people who don't want to explore the series, here's the summary

Starting QEMU system emulator with a disk backed by an TLS encrypted NBD export

  $ qemu-system-x86_64 \
     -object tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls
     -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

Starting a standalone NBD server providing a TLS encrypted NBD export

  $ qemu-nbd \
     --object tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
     --tls-creds tls0 \
     --exportname default

NB, exportname is mandatory, since TLS requires the fixed-new style NBD protocol negotiation.

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
     -qmp unix:/tmp/qmp,server \
     -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
     -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server
  $ qmp-shell /tmp/qmp
  (qmp) nbd-server-start addr={"host":"localhost","port":"9000"} tls-creds=tls0
  (qmp) nbd-server-add device=ide0-hd0

The first 6 patches are the conversion to the I/O channels framework.

The next 5 patches are general tweaks to QEMU's impl of the NBD protocol for better compliance and/or future proofing.

The next patch provides the NBD protocol TLS implementation.

The final 3 patches allow TLS to be enabled in the QEMU NBD client and servers.

Daniel P. Berrange (15):
qom: add user_creatable_add & user_creatable_del methods
qemu-nbd: add support for --object command line arg
nbd: convert block client to use I/O channels for connection setup
nbd: convert qemu-nbd server to use I/O channels for connection setup
nbd: convert blockdev NBD server to use I/O channels for connection
setup
nbd: convert to using I/O channels for actual socket I/O
nbd: invert client logic for negotiating protocol version
nbd: make server compliant with fixed newstyle spec
nbd: make client request fixed new style if advertized
nbd: allow setting of an export name for qemu-nbd server
nbd: pick first exported volume if no export name is requested
nbd: implement TLS support in the protocol negotiation
nbd: enable use of TLS with NBD block driver
nbd: enable use of TLS with qemu-nbd server
nbd: enable use of TLS with nbd-server-start command

 Makefile                        |   6 +-
 block/nbd-client.c              |  74 ++--
 block/nbd-client.h              |  10 +-
 block/nbd.c                     | 105 ++++-
 blockdev-nbd.c                  | 132 +++++-
 hmp.c                           |  13 +-
 include/block/nbd.h             |  26 +-
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  31 ++
 nbd.c                           | 890 ++++++++++++++++++++++++++++++++--------
 qapi/block.json                 |   4 +-
 qemu-nbd.c                      | 242 +++++++++--
 qemu-nbd.texi                   |  14 +
 qmp-commands.hx                 |   2 +-
 qmp.c                           |  75 +---
 qom/object_interfaces.c         |  76 ++++
 tests/Makefile                  |   2 +-
 vl.c                            |   8 +-
 18 files changed, 1333 insertions(+), 380 deletions(-)
Signature · 1 line
2.5.0
#1 lore raw

PATCH 1/15 qom: add user_creatable_add & user_creatable_del methods

The QMP monitor code has two helper methods object_add and qmp_object_del that are called from several places in the code (QMP, HMP and main emulator startup).

We soon need to use this code from qemu-img, qemu-io and qemu-nbd too, but don't want those to depend on the monitor.

To avoid this, move object_add to user_creatable_add an qmp_object_del to user_creatable_del, in the object_interfaces.c file

---
 hmp.c                           | 11 ++++--
 include/monitor/monitor.h       |  3 --
 include/qom/object_interfaces.h | 31 +++++++++++++++++
 qmp.c                           | 75 ++++------------------------------------
 qom/object_interfaces.c         | 76 +++++++++++++++++++++++++++++++++++++++++
 vl.c                            |  8 +++--
 6 files changed, 127 insertions(+), 77 deletions(-)
hmp.c +8 -3
index 2140605..6044db3 100644
@@ -29,6 +29,7 @@
 #include "qapi/string-output-visitor.h"
 #include "qapi/util.h"
 #include "qapi-visit.h"
+#include "qom/object_interfaces.h"
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
@@ -1662,6 +1663,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     void *dummy = NULL;
     OptsVisitor *ov;
     QDict *pdict;
+    Object *obj = NULL;
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
@@ -1688,12 +1690,12 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
         goto out_end;
     }
 
-    object_add(type, id, pdict, opts_get_visitor(ov), &err);
+    obj = user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
 
 out_end:
     visit_end_struct(opts_get_visitor(ov), &err_end);
     if (!err && err_end) {
-        qmp_object_del(id, NULL);
+        user_creatable_del(id, NULL);
     }
     error_propagate(&err, err_end);
 out_clean:
@@ -1704,6 +1706,9 @@ out_clean:
     g_free(id);
     g_free(type);
     g_free(dummy);
+    if (obj) {
+        object_unref(obj);
+    }
 
 out:
     hmp_handle_error(mon, &err);
@@ -1936,7 +1941,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
     const char *id = qdict_get_str(qdict, "id");
     Error *err = NULL;
 
-    qmp_object_del(id, &err);
+    user_creatable_del(id, &err);
     hmp_handle_error(mon, &err);
 }
 
include/monitor/monitor.h +0 -3
index 91b95ae..aa0f373 100644
@@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
 
-void object_add(const char *type, const char *id, const QDict *qdict,
-                Visitor *v, Error **errp);
-
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp);
include/qom/object_interfaces.h +31 -0
index 283ae0d..3e2afeb 100644
@@ -2,6 +2,8 @@
 #define OBJECT_INTERFACES_H
 
 #include "qom/object.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/visitor.h"
 
 #define TYPE_USER_CREATABLE "user-creatable"
 
@@ -72,4 +74,33 @@ void user_creatable_complete(Object *obj, Error **errp);
  * from implements USER_CREATABLE interface.
  */
 bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
+
+/**
+ * user_creatable_add:
+ * @type: the object type name
+ * @id: the unique ID for the object
+ * @qdict: the object parameters
+ * @v: the visitor
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object @type, placing
+ * it in the object composition tree with name @id, initializing
+ * it with properties from @qdict
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add(const char *type, const char *id,
+                           const QDict *qdict,
+                           Visitor *v, Error **errp);
+
+/**
+ * user_creatable_del:
+ * @id: the unique ID for the object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Delete an instance of the user creatable object identified
+ * by @id.
+ */
+void user_creatable_del(const char *id, Error **errp);
+
 #endif
qmp.c +6 -69

98 lines not on this page · raw message

qom/object_interfaces.c +76 -0

82 lines not on this page · raw message

vl.c +6 -2
index 525929b..1f57aa3 100644
@@ -2834,6 +2834,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
     OptsVisitor *ov;
     QDict *pdict;
     bool (*type_predicate)(const char *) = opaque;
+    Object *obj = NULL;
 
     ov = opts_visitor_new(opts);
     pdict = qemu_opts_to_qdict(opts, NULL);
@@ -2858,13 +2859,13 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    object_add(type, id, pdict, opts_get_visitor(ov), &err);
+    obj = user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
     if (err) {
         goto out;
     }
     visit_end_struct(opts_get_visitor(ov), &err);
     if (err) {
-        qmp_object_del(id, NULL);
+        user_creatable_del(id, NULL);
     }
 
 out:
@@ -2874,6 +2875,9 @@ out:
     g_free(id);
     g_free(type);
     g_free(dummy);
+    if (obj) {
+        object_unref(obj);
+    }
     if (err) {
         error_report_err(err);
         return -1;
Signature · 1 line
2.5.0
#2 lore raw

PATCH 2/15 qemu-nbd: add support for --object command line arg

In reply to Daniel P. Berrange

Allow creation of user creatable object types with qemu-nbd via a --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # echo -n letmein > mypasswd.txt
 # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
      ...other nbd args...
---
 qemu-nbd.c    | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-nbd.texi |  7 +++++
 2 files changed, 92 insertions(+)
qemu-nbd.c +85 -0
index 3afec76..41f4285 100644
@@ -23,9 +23,12 @@
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/opts-visitor.h"
+#include "qom/object_interfaces.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -45,6 +48,7 @@
 #define QEMU_NBD_OPT_AIO           2
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
 static int verbose;
@@ -78,6 +82,9 @@ static void usage(const char *name)
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
+"General purpose options:\n"
+"  --object type,id=ID,...   define an object such as 'secret' for providing\n"
+"                            passwords and/or encryption keys\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -380,6 +387,67 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    char *type = NULL;
+    char *id = NULL;
+    void *dummy = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+
+    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+    if (err) {
+        goto out;
+    }
+
+    qdict_del(pdict, "qom-type");
+    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+    if (err) {
+        goto out;
+    }
+
+    qdict_del(pdict, "id");
+    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+    if (err) {
+        goto out;
+    }
+
+    user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
+    if (err) {
+        goto out;
+    }
+    visit_end_struct(opts_get_visitor(ov), &err);
+    if (err) {
+        user_creatable_del(id, NULL);
+    }
+
+out:
+    opts_visitor_cleanup(ov);
+
+    QDECREF(pdict);
+    g_free(id);
+    g_free(type);
+    g_free(dummy);
+    if (err) {
+        error_report_err(err);
+        return -1;
+    }
+    return 0;
+}
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -417,6 +485,7 @@ int main(int argc, char **argv)
         { "format", 1, NULL, 'f' },
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
+        { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -434,6 +503,7 @@ int main(int argc, char **argv)
     Error *local_err = NULL;
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
+    QemuOpts *opts;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -442,6 +512,8 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);
     qemu_init_exec_dir(argv[0]);
 
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
@@ -578,6 +650,13 @@ int main(int argc, char **argv)
             usage(argv[0]);
             exit(0);
             break;
+        case QEMU_NBD_OPT_OBJECT:
+            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                           optarg, true);
+            if (!opts) {
+                exit(1);
+            }
+            break;
         case '?':
             errx(EXIT_FAILURE, "Try `%s --help' for more information.",
                  argv[0]);
@@ -590,6 +669,12 @@ int main(int argc, char **argv)
              argv[0]);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("object"),
+                          object_create,
+                          NULL, NULL)) {
+        exit(1);
+    }
+
     if (disconnect) {
         fd = open(argv[optind], O_RDWR);
         if (fd < 0) {
qemu-nbd.texi +7 -0
index 46fd483..3b2004e 100644
@@ -14,6 +14,13 @@ Export QEMU disk image using NBD protocol.
 @table @option
 @item @var{filename}
  is a disk image filename
+@item --object type,id=@var{id},...props...
+  define a new instance of the @var{type} object class
+  identified by @var{id}. See the @code{qemu(1)} manual
+  page for full details of the properties supported.
+  The only object type that it makes sense to define
+  is the @code{secret} object, which is used to supply
+  passwords and/or encryption keys.
 @item -p, --port=@var{port}
   port to listen on (default @samp{10809})
 @item -o, --offset=@var{offset}
Signature · 1 line
2.5.0
#3 lore raw

PATCH 3/15 nbd: convert block client to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the NBD block driver client to use the QIOChannelSocket class for initial connection setup. The NbdClientSession struct has two pointers, one to the master QIOChannelSocket providing the raw data channel, and one to a QIOChannel which is the current channel used for I/O. Initially the two point to the same object, but when TLS support is added, they will point to different objects.

In this initial conversion though, all I/O is still actually done using the raw POSIX sockets APIs.

---
 Makefile           |  6 +++---
 block/nbd-client.c | 59 ++++++++++++++++++++++++++++--------------------------
 block/nbd-client.h |  8 ++++++--
 block/nbd.c        | 39 ++++++++++++++++++------------------
 tests/Makefile     |  2 +-
 5 files changed, 61 insertions(+), 53 deletions(-)
Makefile +3 -3
index b0049ea..840e99c 100644
@@ -234,9 +234,9 @@ util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
 
 qemu-img.o: qemu-img-cmds.h
 
-qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
 
block/nbd-client.c +31 -28

144 lines not on this page · raw message

block/nbd-client.h +6 -2
index e841340..e8b3283 100644
@@ -4,6 +4,7 @@
 #include "qemu-common.h"
 #include "block/nbd.h"
 #include "block/block_int.h"
+#include "io/channel-socket.h"
 
 /* #define DEBUG_NBD */
 
@@ -17,7 +18,8 @@
 #define MAX_NBD_REQUESTS    16
 
 typedef struct NbdClientSession {
-    int sock;
+    QIOChannelSocket *sioc; /* The master data channel */
+    QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
     uint32_t nbdflags;
     off_t size;
 
@@ -34,7 +36,9 @@ typedef struct NbdClientSession {
 
 NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
 
-int nbd_client_init(BlockDriverState *bs, int sock, const char *export_name,
+int nbd_client_init(BlockDriverState *bs,
+                    QIOChannelSocket *sock,
+                    const char *export_name,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
block/nbd.c +20 -19
index cd6a587..e51acfc 100644
@@ -30,7 +30,6 @@
 #include "qemu/uri.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
-#include "qemu/sockets.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qint.h"
@@ -239,25 +238,25 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
     return &s->client;
 }
 
-static int nbd_establish_connection(BlockDriverState *bs,
-                                    SocketAddress *saddr,
-                                    Error **errp)
+static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+                                                  Error **errp)
 {
-    BDRVNBDState *s = bs->opaque;
-    int sock;
+    QIOChannelSocket *sioc;
+    Error *local_err = NULL;
 
-    sock = socket_connect(saddr, errp, NULL, NULL);
+    sioc = qio_channel_socket_new();
 
-    if (sock < 0) {
-        logout("Failed to establish connection to NBD server\n");
-        return -EIO;
+    qio_channel_socket_connect_sync(sioc,
+                                    saddr,
+                                    &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
     }
 
-    if (!s->client.is_unix) {
-        socket_set_nodelay(sock);
-    }
+    qio_channel_set_delay(QIO_CHANNEL(sioc), false);
 
-    return sock;
+    return sioc;
 }
 
 static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
@@ -265,7 +264,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVNBDState *s = bs->opaque;
     char *export = NULL;
-    int result, sock;
+    int result;
+    QIOChannelSocket *sioc;
     SocketAddress *saddr;
 
     /* Pop the config into our state object. Exit if invalid. */
@@ -277,15 +277,16 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sock = nbd_establish_connection(bs, saddr, errp);
+    sioc = nbd_establish_connection(saddr, errp);
     qapi_free_SocketAddress(saddr);
-    if (sock < 0) {
+    if (!sioc) {
         g_free(export);
-        return sock;
+        return -ECONNREFUSED;
     }
 
     /* NBD handshake */
-    result = nbd_client_init(bs, sock, export, errp);
+    result = nbd_client_init(bs, sioc, export, errp);
+    object_unref(OBJECT(sioc));
     g_free(export);
     return result;
 }
tests/Makefile +1 -1
index c0c9de6..d46b21b 100644
@@ -386,8 +386,8 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 	tests/test-qapi-event.o tests/test-qmp-introspect.o \
 	$(test-qom-obj-y)
 test-crypto-obj-y = $(crypto-obj-y) $(test-qom-obj-y)
-test-block-obj-y = $(block-obj-y) $(test-crypto-obj-y)
 test-io-obj-y = $(io-obj-y) $(test-crypto-obj-y)
+test-block-obj-y = $(block-obj-y) $(test-io-obj-y)
 
 tests/check-qint$(EXESUF): tests/check-qint.o $(test-util-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
Signature · 1 line
2.5.0
#4 lore raw

PATCH 4/15 nbd: convert qemu-nbd server to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the qemu-nbd server to use the QIOChannelSocket class for initial listener socket setup and accepting of client connections. Actual I/O is still being performed against the socket file descriptor using the POSIX socket APIs.

In this initial conversion though, all I/O is still actually done using the raw POSIX sockets APIs.

---
 qemu-nbd.c | 91 ++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 41 deletions(-)
qemu-nbd.c +50 -41
index 41f4285..28ab54f 100644
@@ -21,7 +21,6 @@
 #include "block/block_int.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
-#include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "block/snapshot.h"
@@ -29,16 +28,13 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/opts-visitor.h"
 #include "qom/object_interfaces.h"
+#include "io/channel-socket.h"
 
 #include <stdarg.h>
 #include <stdio.h>
 #include <getopt.h>
 #include <err.h>
 #include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
-#include <arpa/inet.h>
 #include <signal.h>
 #include <libgen.h>
 #include <pthread.h>
@@ -58,7 +54,8 @@ static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
 static int shared = 1;
 static int nb_fds;
-static int server_fd;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
 static void usage(const char *name)
 {
@@ -241,19 +238,21 @@ static void *nbd_client_thread(void *arg)
     char *device = arg;
     off_t size;
     uint32_t nbdflags;
-    int fd, sock;
+    QIOChannelSocket *sioc;
+    int fd;
     int ret;
     pthread_t show_parts_thread;
     Error *local_error = NULL;
 
-
-    sock = socket_connect(saddr, &local_error, NULL, NULL);
-    if (sock < 0) {
+    sioc = qio_channel_socket_new();
+    if (qio_channel_socket_connect_sync(sioc,
+                                        saddr,
+                                        &local_error) < 0) {
         error_report_err(local_error);
         goto out;
     }
 
-    ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
+    ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -270,7 +269,7 @@ static void *nbd_client_thread(void *arg)
         goto out_socket;
     }
 
-    ret = nbd_init(fd, sock, nbdflags, size);
+    ret = nbd_init(fd, sioc->fd, nbdflags, size);
     if (ret < 0) {
         goto out_fd;
     }
@@ -291,13 +290,14 @@ static void *nbd_client_thread(void *arg)
         goto out_fd;
     }
     close(fd);
+    object_unref(OBJECT(sioc));
     kill(getpid(), SIGTERM);
     return (void *) EXIT_SUCCESS;
 
 out_fd:
     close(fd);
 out_socket:
-    closesocket(sock);
+    object_unref(OBJECT(sioc));
 out:
     kill(getpid(), SIGTERM);
     return (void *) EXIT_FAILURE;
@@ -314,7 +314,7 @@ static void nbd_export_closed(NBDExport *exp)
     state = TERMINATED;
 }
 
-static void nbd_update_server_fd_handler(int fd);
+static void nbd_update_server_watch(void);
 
 static void nbd_client_closed(NBDClient *client)
 {
@@ -322,41 +322,51 @@ static void nbd_client_closed(NBDClient *client)
     if (nb_fds == 0 && !persistent && state == RUNNING) {
         state = TERMINATE;
     }
-    nbd_update_server_fd_handler(server_fd);
+    nbd_update_server_watch();
     nbd_client_put(client);
 }
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
-    struct sockaddr_in addr;
-    socklen_t addr_len = sizeof(addr);
+    QIOChannelSocket *cioc;
+    int fd;
 
-    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
-    if (fd < 0) {
-        perror("accept");
-        return;
+    cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+                                     NULL);
+    if (!cioc) {
+        return TRUE;
     }
 
     if (state >= TERMINATE) {
-        close(fd);
-        return;
+        object_unref(OBJECT(cioc));
+        return TRUE;
     }
 
-    if (nbd_client_new(exp, fd, nbd_client_closed)) {
+    fd = dup(cioc->fd);
+    if (fd >= 0 &&
+        nbd_client_new(exp, fd, nbd_client_closed)) {
         nb_fds++;
-        nbd_update_server_fd_handler(server_fd);
-    } else {
-        shutdown(fd, 2);
-        close(fd);
+        nbd_update_server_watch();
     }
+    object_unref(OBJECT(cioc));
+
+    return TRUE;
 }
 
-static void nbd_update_server_fd_handler(int fd)
+static void nbd_update_server_watch(void)
 {
     if (nbd_can_accept()) {
-        qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
+        if (server_watch == -1) {
+            server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+                                                 G_IO_IN,
+                                                 nbd_accept,
+                                                 NULL, NULL);
+        }
     } else {
-        qemu_set_fd_handler(fd, NULL, NULL, NULL);
+        if (server_watch != -1) {
+            g_source_remove(server_watch);
+            server_watch = -1;
+        }
     }
 }
 
@@ -494,7 +504,6 @@ int main(int argc, char **argv)
     int flags = BDRV_O_RDWR;
     int partition = -1;
     int ret = 0;
-    int fd;
     bool seen_cache = false;
     bool seen_discard = false;
     bool seen_aio = false;
@@ -676,13 +685,13 @@ int main(int argc, char **argv)
     }
 
     if (disconnect) {
-        fd = open(argv[optind], O_RDWR);
-        if (fd < 0) {
+        int nbdfd = open(argv[optind], O_RDWR);
+        if (nbdfd < 0) {
             err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
         }
-        nbd_disconnect(fd);
+        nbd_disconnect(nbdfd);
 
-        close(fd);
+        close(nbdfd);
 
         printf("%s disconnected\n", argv[optind]);
 
@@ -807,8 +816,9 @@ int main(int argc, char **argv)
         errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
     }
 
-    fd = socket_listen(saddr, &local_err);
-    if (fd < 0) {
+    server_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
+        object_unref(OBJECT(server_ioc));
         error_report_err(local_err);
         return 1;
     }
@@ -826,8 +836,7 @@ int main(int argc, char **argv)
         memset(&client_thread, 0, sizeof(client_thread));
     }
 
-    server_fd = fd;
-    nbd_update_server_fd_handler(fd);
+    nbd_update_server_watch();
 
     /* now when the initialization is (almost) complete, chdir("/")
      * to free any busy filesystems */
Signature · 1 line
2.5.0
#5 lore raw

PATCH 5/15 nbd: convert blockdev NBD server to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the blockdev NBD server to use the QIOChannelSocket class for initial listener socket setup and accepting of client connections. Actual I/O is still being performed against the socket file descriptor using the POSIX socket APIs.

---
 blockdev-nbd.c | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)
blockdev-nbd.c +36 -17
index bcdd18b..71ee021 100644
@@ -17,33 +17,49 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "block/nbd.h"
-#include "qemu/sockets.h"
+#include "io/channel-socket.h"
 
-static int server_fd = -1;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
+                           gpointer opaque)
 {
-    struct sockaddr_in addr;
-    socklen_t addr_len = sizeof(addr);
+    QIOChannelSocket *cioc;
+    int fd;
 
-    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
-    if (fd >= 0 && !nbd_client_new(NULL, fd, nbd_client_put)) {
-        shutdown(fd, 2);
+    cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+                                     NULL);
+    if (!cioc) {
+        return TRUE;
+    }
+
+    fd = dup(cioc->fd);
+    if (fd >= 0 &&
+        !nbd_client_new(NULL, fd, nbd_client_put)) {
         close(fd);
     }
+    object_unref(OBJECT(cioc));
+    return TRUE;
 }
 
 void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
 {
-    if (server_fd != -1) {
+    if (server_ioc) {
         error_setg(errp, "NBD server already running");
         return;
     }
 
-    server_fd = socket_listen(addr, errp);
-    if (server_fd != -1) {
-        qemu_set_fd_handler(server_fd, nbd_accept, NULL, NULL);
+    server_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+        return;
     }
+
+    server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+                                         G_IO_IN,
+                                         nbd_accept,
+                                         NULL,
+                                         NULL);
 }
 
 /*
@@ -78,7 +94,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     NBDExport *exp;
     NBDCloseNotifier *n;
 
-    if (server_fd == -1) {
+    if (!server_ioc) {
         error_setg(errp, "NBD server not running");
         return;
     }
@@ -128,9 +144,12 @@ void qmp_nbd_server_stop(Error **errp)
         nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
     }
 
-    if (server_fd != -1) {
-        qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
-        close(server_fd);
-        server_fd = -1;
+    if (server_watch != -1) {
+        g_source_remove(server_watch);
+        server_watch = -1;
+    }
+    if (server_ioc) {
+        object_unref(OBJECT(server_ioc));
+        server_ioc = NULL;
     }
 }
Signature · 1 line
2.5.0
#6 lore raw

PATCH 6/15 nbd: convert to using I/O channels for actual socket I/O

In reply to Daniel P. Berrange

Now that all callers are converted to use I/O channels for initial connection setup, it is possible to switch the core NBD protocol handling core over to use QIOChannel APIs for acutal sockets I/O.

---
 block/nbd-client.c  |  19 ++--
 blockdev-nbd.c      |   7 +-
 include/block/nbd.h |  19 ++--
 nbd.c               | 289 ++++++++++++++++++++++++++++++++--------------------
 qemu-nbd.c          |   9 +-
 5 files changed, 205 insertions(+), 138 deletions(-)
block/nbd-client.c +10 -9
index bf5b0a3..ed69d4b 100644
@@ -71,7 +71,7 @@ static void nbd_reply_ready(void *opaque)
          * that another thread has done the same thing in parallel, so
          * the socket is not readable anymore.
          */
-        ret = nbd_receive_reply(s->sioc->fd, &s->reply);
+        ret = nbd_receive_reply(s->ioc, &s->reply);
         if (ret == -EAGAIN) {
             return;
         }
@@ -122,6 +122,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
         }
     }
 
+    g_assert(qemu_in_coroutine());
     assert(i < MAX_NBD_REQUESTS);
     request->handle = INDEX_TO_HANDLE(s, i);
     s->send_coroutine = qemu_coroutine_self();
@@ -131,17 +132,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
                        nbd_reply_ready, nbd_restart_write, bs);
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
-        rc = nbd_send_request(s->sioc->fd, request);
+        rc = nbd_send_request(s->ioc, request);
         if (rc >= 0) {
-            ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
-                                offset, request->len);
+            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+                               offset, request->len, 0);
             if (ret != request->len) {
                 rc = -EIO;
             }
         }
         qio_channel_set_cork(s->ioc, false);
     } else {
-        rc = nbd_send_request(s->sioc->fd, request);
+        rc = nbd_send_request(s->ioc, request);
     }
     aio_set_fd_handler(aio_context, s->sioc->fd, false,
                        nbd_reply_ready, NULL, bs);
@@ -164,8 +165,8 @@ static void nbd_co_receive_reply(NbdClientSession *s,
         reply->error = EIO;
     } else {
         if (qiov && reply->error == 0) {
-            ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
-                                offset, request->len);
+            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+                               offset, request->len, 1);
             if (ret != request->len) {
                 reply->error = EIO;
             }
@@ -372,7 +373,7 @@ void nbd_client_close(BlockDriverState *bs)
         return;
     }
 
-    nbd_send_request(client->sioc->fd, &request);
+    nbd_send_request(client->ioc, &request);
 
     nbd_teardown_connection(bs);
 }
@@ -387,7 +388,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     logout("session init %s\n", export);
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
-    ret = nbd_receive_negotiate(sioc->fd, export,
+    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
                                 &client->nbdflags, &client->size, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
blockdev-nbd.c +1 -6
index 71ee021..7d4c415 100644
@@ -26,7 +26,6 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
                            gpointer opaque)
 {
     QIOChannelSocket *cioc;
-    int fd;
 
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
@@ -34,11 +33,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
         return TRUE;
     }
 
-    fd = dup(cioc->fd);
-    if (fd >= 0 &&
-        !nbd_client_new(NULL, fd, nbd_client_put)) {
-        close(fd);
-    }
+    nbd_client_new(NULL, cioc, nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
include/block/nbd.h +13 -6
index 65f409d..0230c7f 100644
@@ -23,6 +23,7 @@
 
 #include "qemu-common.h"
 #include "qemu/option.h"
+#include "io/channel-socket.h"
 
 struct nbd_request {
     uint32_t magic;
@@ -73,12 +74,17 @@ enum {
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
 
-ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+ssize_t nbd_wr_syncv(QIOChannel *ioc,
+                     struct iovec *iov,
+                     size_t niov,
+                     size_t offset,
+                     size_t length,
+                     bool do_read);
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
                           off_t *size, Error **errp);
-int nbd_init(int fd, int csock, uint32_t flags, off_t size);
-ssize_t nbd_send_request(int csock, struct nbd_request *request);
-ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
+ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
+ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
 
@@ -98,7 +104,8 @@ NBDExport *nbd_export_find(const char *name);
 void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_close_all(void);
 
-NBDClient *nbd_client_new(NBDExport *exp, int csock,
+NBDClient *nbd_client_new(NBDExport *exp,
+                          QIOChannelSocket *sioc,
                           void (*close)(NBDClient *));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
nbd.c +178 -111

705 lines not on this page · raw message

qemu-nbd.c +3 -6
index 28ab54f..42ccf05 100644
@@ -252,7 +252,7 @@ static void *nbd_client_thread(void *arg)
         goto out;
     }
 
-    ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
+    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -269,7 +269,7 @@ static void *nbd_client_thread(void *arg)
         goto out_socket;
     }
 
-    ret = nbd_init(fd, sioc->fd, nbdflags, size);
+    ret = nbd_init(fd, sioc, nbdflags, size);
     if (ret < 0) {
         goto out_fd;
     }
@@ -329,7 +329,6 @@ static void nbd_client_closed(NBDClient *client)
 static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
     QIOChannelSocket *cioc;
-    int fd;
 
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
@@ -342,9 +341,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
         return TRUE;
     }
 
-    fd = dup(cioc->fd);
-    if (fd >= 0 &&
-        nbd_client_new(exp, fd, nbd_client_closed)) {
+    if (nbd_client_new(exp, cioc, nbd_client_closed)) {
         nb_fds++;
         nbd_update_server_watch();
     }
Signature · 1 line
2.5.0
#7 lore raw

PATCH 7/15 nbd: invert client logic for negotiating protocol version

In reply to Daniel P. Berrange

The nbd_receive_negotiate() method takes different code paths based on whether 'name == NULL', and then checks the expected protocol version in each branch.

This patch inverts the logic, so that it takes different code paths based on what protocol version it receives and then checks if name is NULL or not as needed.

This facilitates later code which allows the client to be caapble of using the new style protocol regardless of whether an export name is listed or not.

---
 nbd.c | 60 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)
nbd.c +29 -31
index 701b951..bdfc45e 100644
@@ -682,20 +682,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
     magic = be64_to_cpu(magic);
     TRACE("Magic is 0x%" PRIx64, magic);
 
-    if (name) {
+    if (magic == NBD_OPTS_MAGIC) {
         uint32_t reserved = 0;
         uint32_t opt;
         uint32_t namesize;
 
-        TRACE("Checking magic (opts_magic)");
-        if (magic != NBD_OPTS_MAGIC) {
-            if (magic == NBD_CLIENT_MAGIC) {
-                error_setg(errp, "Server does not support export names");
-            } else {
-                error_setg(errp, "Bad magic received");
-            }
-            goto fail;
-        }
         if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
@@ -708,6 +699,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             goto fail;
         }
         /* write the export name */
+        if (!name) {
+            error_setg(errp, "Server requires an export name");
+            goto fail;
+        }
         magic = cpu_to_be64(magic);
         if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
             error_setg(errp, "Failed to send export name magic");
@@ -728,39 +723,42 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             error_setg(errp, "Failed to send export name");
             goto fail;
         }
-    } else {
-        TRACE("Checking magic (cli_magic)");
 
-        if (magic != NBD_CLIENT_MAGIC) {
-            if (magic == NBD_OPTS_MAGIC) {
-                error_setg(errp, "Server requires an export name");
-            } else {
-                error_setg(errp, "Bad magic received");
-            }
+        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+            error_setg(errp, "Failed to read export length");
             goto fail;
         }
-    }
+        *size = be64_to_cpu(s);
+        TRACE("Size is %" PRIu64, *size);
 
-    if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
-        error_setg(errp, "Failed to read export length");
-        goto fail;
-    }
-    *size = be64_to_cpu(s);
-    TRACE("Size is %" PRIu64, *size);
+        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+            error_setg(errp, "Failed to read export flags");
+            goto fail;
+        }
+        *flags |= be16_to_cpu(tmp);
+    } else if (magic == NBD_CLIENT_MAGIC) {
+        if (name) {
+            error_setg(errp, "Server does not support export names");
+            goto fail;
+        }
+
+        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+            error_setg(errp, "Failed to read export length");
+            goto fail;
+        }
+        *size = be64_to_cpu(s);
+        TRACE("Size is %" PRIu64, *size);
 
-    if (!name) {
         if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
         *flags = be32_to_cpup(flags);
     } else {
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
-            error_setg(errp, "Failed to read export flags");
-            goto fail;
-        }
-        *flags |= be16_to_cpu(tmp);
+        error_setg(errp, "Bad magic received");
+        goto fail;
     }
+
     if (read_sync(ioc, &buf, 124) != 124) {
         error_setg(errp, "Failed to read reserved block");
         goto fail;
Signature · 1 line
2.5.0
#8 lore raw

PATCH 8/15 nbd: make server compliant with fixed newstyle spec

In reply to Daniel P. Berrange

If the client does not request the fixed new style protocol, then we should only accept NBD_OPT_EXPORT_NAME. All other options are only valid when fixed new style has been activated.

The qemu-nbd client doesn't currently request fixed new style protocol, but this change won't break qemu-nbd, because it fortunately only ever uses NBD_OPT_EXPORT_NAME, so was never triggering the non-compliant server behaviour.

---
 nbd.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 23 deletions(-)
nbd.c +45 -23
index bdfc45e..09a32a9 100644
@@ -486,6 +486,7 @@ static int nbd_receive_options(NBDClient *client)
 {
     QIOChannel *ioc = client->ioc;
     uint32_t flags;
+    bool fixedNewstyle = false;
 
     /* Client sends:
         [ 0 ..   3]   client flags
@@ -507,14 +508,19 @@ static int nbd_receive_options(NBDClient *client)
     }
     TRACE("Checking client flags");
     be32_to_cpus(&flags);
-    if (flags != 0 && flags != NBD_FLAG_C_FIXED_NEWSTYLE) {
-        LOG("Bad client flags received");
+    if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
+        TRACE("Support supports fixed newstyle handshake");
+        fixedNewstyle = true;
+        flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+    }
+    if (flags != 0) {
+        TRACE("Unknown client flags 0x%x received", flags);
         return -EIO;
     }
 
     while (1) {
         int ret;
-        uint32_t tmp, length;
+        uint32_t clientflags, length;
         uint64_t magic;
 
         if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
@@ -527,10 +533,12 @@ static int nbd_receive_options(NBDClient *client)
             return -EINVAL;
         }
 
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &clientflags, sizeof(clientflags)) !=
+            sizeof(clientflags)) {
             LOG("read failed");
             return -EINVAL;
         }
+        clientflags = be32_to_cpu(clientflags);
 
         if (read_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
             LOG("read failed");
@@ -538,26 +546,40 @@ static int nbd_receive_options(NBDClient *client)
         }
         length = be32_to_cpu(length);
 
-        TRACE("Checking option");
-        switch (be32_to_cpu(tmp)) {
-        case NBD_OPT_LIST:
-            ret = nbd_handle_list(client, length);
-            if (ret < 0) {
-                return ret;
+        TRACE("Checking option 0x%x", clientflags);
+        if (fixedNewstyle) {
+            switch (clientflags) {
+            case NBD_OPT_LIST:
+                ret = nbd_handle_list(client, length);
+                if (ret < 0) {
+                    return ret;
+                }
+                break;
+
+            case NBD_OPT_ABORT:
+                return -EINVAL;
+
+            case NBD_OPT_EXPORT_NAME:
+                return nbd_handle_export_name(client, length);
+
+            default:
+                TRACE("Unsupported option 0x%x", clientflags);
+                nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, clientflags);
+                return -EINVAL;
+            }
+        } else {
+            /*
+             * If broken new-style we should drop the connection
+             * for anything except NBD_OPT_EXPORT_NAME
+             */
+            switch (clientflags) {
+            case NBD_OPT_EXPORT_NAME:
+                return nbd_handle_export_name(client, length);
+
+            default:
+                TRACE("Unsupported option 0x%x", clientflags);
+                return -EINVAL;
             }
-            break;
-
-        case NBD_OPT_ABORT:
-            return -EINVAL;
-
-        case NBD_OPT_EXPORT_NAME:
-            return nbd_handle_export_name(client, length);
-
-        default:
-            tmp = be32_to_cpu(tmp);
-            LOG("Unsupported option 0x%x", tmp);
-            nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, tmp);
-            return -EINVAL;
         }
     }
 }
Signature · 1 line
2.5.0
#9 lore raw

PATCH 9/15 nbd: make client request fixed new style if advertized

In reply to Daniel P. Berrange

If the server advertizes support for the fixed new style negotiation, the client should in turn enable new style. This will allow the client to negotiate further NBD options besides the export name.

---
 nbd.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
nbd.c +17 -10
index 09a32a9..6ec04a7 100644
@@ -664,7 +664,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
 {
     char buf[256];
     uint64_t magic, s;
-    uint16_t tmp;
     int rc;
 
     TRACE("Receiving negotiation.");
@@ -705,19 +704,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
     TRACE("Magic is 0x%" PRIx64, magic);
 
     if (magic == NBD_OPTS_MAGIC) {
-        uint32_t reserved = 0;
+        uint32_t clientflags = 0;
         uint32_t opt;
         uint32_t namesize;
+        uint16_t globalflags;
+        uint16_t exportflags;
 
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
+            sizeof(globalflags)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
-        *flags = be16_to_cpu(tmp) << 16;
-        /* reserved for future use */
-        if (write_sync(ioc, &reserved, sizeof(reserved)) !=
-            sizeof(reserved)) {
-            error_setg(errp, "Failed to read reserved field");
+        *flags = be16_to_cpu(globalflags) << 16;
+        if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+            TRACE("Server supports fixed new style");
+            clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
+        }
+        /* client requested flags */
+        if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
+            sizeof(clientflags)) {
+            error_setg(errp, "Failed to send clientflags field");
             goto fail;
         }
         /* write the export name */
@@ -753,11 +759,12 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         *size = be64_to_cpu(s);
         TRACE("Size is %" PRIu64, *size);
 
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &exportflags, sizeof(exportflags)) !=
+            sizeof(exportflags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags |= be16_to_cpu(tmp);
+        *flags |= be16_to_cpu(exportflags);
     } else if (magic == NBD_CLIENT_MAGIC) {
         if (name) {
             error_setg(errp, "Server does not support export names");
Signature · 1 line
2.5.0
#10 lore raw

PATCH 10/15 nbd: allow setting of an export name for qemu-nbd server

In reply to Daniel P. Berrange

The qemu-nbd server currently always uses the old style protocol since it never sets any export name. This is a problem because future TLS support will require use of the new style protocol negotiation.

This adds a "--exportname NAME" argument to qemu-nbd which allows the user to set an explicit export name. When --exportname is set the server will always use the new style protocol.

---
 qemu-nbd.c    | 14 ++++++++++++--
 qemu-nbd.texi |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)
qemu-nbd.c +12 -2
index 42ccf05..3f7f2222 100644
@@ -47,6 +47,7 @@
 #define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
+static bool newproto;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -341,7 +342,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
         return TRUE;
     }
 
-    if (nbd_client_new(exp, cioc, nbd_client_closed)) {
+    if (nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed)) {
         nb_fds++;
         nbd_update_server_watch();
     }
@@ -469,7 +470,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -493,6 +494,7 @@ int main(int argc, char **argv)
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
         { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+        { "exportname", 1, NULL, 'x' },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -510,6 +512,7 @@ int main(int argc, char **argv)
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     QemuOpts *opts;
+    const char *exportname = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -645,6 +648,9 @@ int main(int argc, char **argv)
         case 't':
             persistent = 1;
             break;
+        case 'x':
+            exportname = optarg;
+            break;
         case 'v':
             verbose = 1;
             break;
@@ -812,6 +818,10 @@ int main(int argc, char **argv)
     if (!exp) {
         errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
     }
+    if (exportname) {
+        nbd_export_set_name(exp, exportname);
+        newproto = true;
+    }
 
     server_ioc = qio_channel_socket_new();
     if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
qemu-nbd.texi +3 -0
index 3b2004e..6ff3920 100644
@@ -64,6 +64,9 @@ Export QEMU disk image using NBD protocol.
   force block driver for format @var{fmt} instead of auto-detecting
 @item -t, --persistent
   don't exit on the last connection
+@item -x NAME, --exportname=NAME
+  set the NDB volume export name. This switches the server to use
+  the new style NBD protocol negotiation
 @item -v, --verbose
   display extra debugging information
 @item -h, --help
Signature · 1 line
2.5.0
#11 lore raw

PATCH 11/15 nbd: pick first exported volume if no export name is requested

In reply to Daniel P. Berrange

The NBD client is currently only capable of using the new style protocol negotiation if an explicit export name is provided. This is a problem, because TLS support will require use of the new style protocol in all cases, and we wish to keep the export name as an optional request for backwards compatibility.

The trivial way to deal with this is to use the NBD protocol option for listing exports and then pick the first returned export as the one to use. This makes the client "do the right thing" in the normal case where the qemu-nbd server only exports a single volume.

Furthermore, in order to get proper error reporting of unknown options with fixed new style protocol, we must not send the NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export name is provided we still send NBD_OPT_LIST to enumerate server exports. This also gives us clearer error reporting in the case that the requested export name does not exist. If NBD_OPT_LIST is not supported, we still fallback to using the specified export name, so as not to break compatibility with old QEMU NBD server impls that predated NBD_OPT_LIST

---
 nbd.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 197 insertions(+), 5 deletions(-)
nbd.c +197 -5
index 6ec04a7..0f6c755 100644
@@ -468,6 +468,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
         goto fail;
     }
     name[length] = '\0';
+    TRACE("Client requested export '%s'", name);
 
     client->exp = nbd_export_find(name);
     if (!client->exp) {
@@ -659,12 +660,187 @@ fail:
     return rc;
 }
 
+
+static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
+{
+    if (!(type & (1 << 31))) {
+        return 0;
+    }
+
+    switch (type) {
+    case NBD_REP_ERR_UNSUP:
+        error_setg(errp, "Unsupported option type %x", opt);
+        break;
+
+    case NBD_REP_ERR_INVALID:
+        error_setg(errp, "Invalid data length for option %x", opt);
+        break;
+
+    default:
+        error_setg(errp, "Unknown error code when asking for option %x", opt);
+        break;
+    }
+
+    return -1;
+}
+
+static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+{
+    uint64_t magic;
+    uint32_t opt;
+    uint32_t type;
+    uint32_t len;
+    uint32_t namelen;
+
+    *name = NULL;
+    if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+        error_setg(errp, "failed to read list option magic");
+        return -1;
+    }
+    magic = be64_to_cpu(magic);
+    if (magic != NBD_REP_MAGIC) {
+        error_setg(errp, "Unexpected option list magic");
+        return -1;
+    }
+    if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+        error_setg(errp, "failed to read list option");
+        return -1;
+    }
+    opt = be32_to_cpu(opt);
+    if (opt != NBD_OPT_LIST) {
+        error_setg(errp, "Unexpected option type %x expected %x",
+                   opt, NBD_OPT_LIST);
+        return -1;
+    }
+
+    if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
+        error_setg(errp, "failed to read list option type");
+        return -1;
+    }
+    type = be32_to_cpu(type);
+    if (type == NBD_REP_ERR_UNSUP) {
+        return 0;
+    }
+    if (nbd_handle_reply_err(opt, type, errp) < 0) {
+        return -1;
+    }
+
+    if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) {
+        error_setg(errp, "failed to read option length");
+        return -1;
+    }
+    len = be32_to_cpu(len);
+
+    if (type == NBD_REP_ACK) {
+        if (len != 0) {
+            error_setg(errp, "length too long for option end");
+            return -1;
+        }
+    } else if (type == NBD_REP_SERVER) {
+        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+            error_setg(errp, "failed to read option name length");
+            return -1;
+        }
+        namelen = be32_to_cpu(namelen);
+        if (len != (namelen + sizeof(namelen))) {
+            error_setg(errp, "incorrect option mame length");
+            return -1;
+        }
+        if (namelen > 255) {
+            error_setg(errp, "export name length too long %d", namelen);
+            return -1;
+        }
+
+        *name = g_new0(char, namelen + 1);
+        if (read_sync(ioc, *name, namelen) != namelen) {
+            error_setg(errp, "failed to read export name");
+            g_free(*name);
+            *name = NULL;
+            return -1;
+        }
+        (*name)[namelen] = '\0';
+    } else {
+        error_setg(errp, "Unexpected reply type %x expected %x",
+                   type, NBD_REP_SERVER);
+        return -1;
+    }
+    return 1;
+}
+
+
+static char *nbd_receive_pick_export(QIOChannel *ioc,
+                                     const char *wantname,
+                                     Error **errp)
+{
+    uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
+    uint32_t opt = cpu_to_be32(NBD_OPT_LIST);
+    uint32_t length = 0;
+    char *chosenname = NULL;
+
+    TRACE("Picking export name");
+    if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+        error_setg(errp, "Failed to send list option magic");
+        goto fail;
+    }
+
+    if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+        error_setg(errp, "Failed to send list option number");
+        goto fail;
+    }
+
+    if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
+        error_setg(errp, "Failed to send list option length");
+        goto fail;
+    }
+
+    TRACE("Reading available export names");
+    while (1) {
+        char *name = NULL;
+        int ret = nbd_receive_list(ioc, &name, errp);
+
+        if (ret < 0) {
+            g_free(name);
+            name = NULL;
+            goto fail;
+        }
+        if (ret == 0) { /* Server doesn't support export listing */
+            if (wantname == NULL) {
+                error_setg(errp, "Export name is required");
+                goto fail;
+            }
+            chosenname = g_strdup(wantname);
+            break;
+        }
+        if (name == NULL) {
+            TRACE("End of export name list");
+            break;
+        }
+        if (!chosenname && (!wantname || g_str_equal(name, wantname))) {
+            chosenname = name;
+            TRACE("Picked export name %s", name);
+        } else {
+            TRACE("Ignored export name %s", name);
+            g_free(name);
+        }
+    }
+
+    if (wantname && !chosenname) {
+        error_setg(errp, "No export with name %s available", wantname);
+        goto fail;
+    }
+
+ fail:
+    TRACE("Chosen export name %s", chosenname ? chosenname : "<null>");
+    return chosenname;
+}
+
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
                           off_t *size, Error **errp)
 {
     char buf[256];
     uint64_t magic, s;
     int rc;
+    char *autoname = NULL;
 
     TRACE("Receiving negotiation.");
 
@@ -709,27 +885,40 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         uint32_t namesize;
         uint16_t globalflags;
         uint16_t exportflags;
+        bool fixedNewStyle = false;
 
         if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
             sizeof(globalflags)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
-        *flags = be16_to_cpu(globalflags) << 16;
+        globalflags = be16_to_cpu(globalflags);
+        *flags = globalflags << 16;
+        TRACE("Global flags are %x", globalflags);
         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+            fixedNewStyle = true;
             TRACE("Server supports fixed new style");
             clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
         }
         /* client requested flags */
+        clientflags = cpu_to_be32(clientflags);
         if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
             sizeof(clientflags)) {
             error_setg(errp, "Failed to send clientflags field");
             goto fail;
         }
         /* write the export name */
-        if (!name) {
-            error_setg(errp, "Server requires an export name");
-            goto fail;
+        if (fixedNewStyle) {
+            autoname = nbd_receive_pick_export(ioc, name, errp);
+            if (!autoname) {
+                goto fail;
+            }
+            name = autoname;
+        } else {
+            if (!name) {
+                error_setg(errp, "Server requires an export name");
+                goto fail;
+            }
         }
         magic = cpu_to_be64(magic);
         if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
@@ -764,7 +953,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags |= be16_to_cpu(exportflags);
+        exportflags = be16_to_cpu(exportflags);
+        *flags |= exportflags;
+        TRACE("Export flags are %x", exportflags);
     } else if (magic == NBD_CLIENT_MAGIC) {
         if (name) {
             error_setg(errp, "Server does not support export names");
@@ -795,6 +986,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
     rc = 0;
 
 fail:
+    free(autoname);
     return rc;
 }
 
Signature · 1 line
2.5.0
#12 lore raw

PATCH 12/15 nbd: implement TLS support in the protocol negotiation

In reply to Daniel P. Berrange

This extends the NBD protocol handling code so that it is capable of negotiating TLS support during the connection setup. This involves requesting the STARTTLS protocol option before any other NBD options.

---
 block/nbd-client.c  |  12 ++-
 blockdev-nbd.c      |   2 +-
 include/block/nbd.h |   7 ++
 nbd.c               | 284 +++++++++++++++++++++++++++++++++++++++++++++++++---
 qemu-nbd.c          |   4 +-
 5 files changed, 290 insertions(+), 19 deletions(-)
block/nbd-client.c +9 -3
index ed69d4b..f7bacfd 100644
@@ -389,7 +389,10 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
-                                &client->nbdflags, &client->size, errp);
+                                &client->nbdflags,
+                                NULL, NULL,
+                                &client->ioc,
+                                &client->size, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
         return ret;
@@ -399,8 +402,11 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     qemu_co_mutex_init(&client->free_sema);
     client->sioc = sioc;
     object_ref(OBJECT(client->sioc));
-    client->ioc = QIO_CHANNEL(sioc);
-    object_ref(OBJECT(client->ioc));
+
+    if (!client->ioc) {
+        client->ioc = QIO_CHANNEL(sioc);
+        object_ref(OBJECT(client->ioc));
+    }
 
     /* Now that we're connected, set the socket to be non-blocking and
      * kick the reply mechanism.  */
blockdev-nbd.c +1 -1
index 7d4c415..6af1684 100644
@@ -33,7 +33,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
         return TRUE;
     }
 
-    nbd_client_new(NULL, cioc, nbd_client_put);
+    nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
include/block/nbd.h +7 -0
index 0230c7f..502edfe 100644
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "io/channel-socket.h"
+#include "crypto/tlscreds.h"
 
 struct nbd_request {
     uint32_t magic;
@@ -57,6 +58,8 @@ struct nbd_reply {
 #define NBD_REP_SERVER          (2)             /* Export description. */
 #define NBD_REP_ERR_UNSUP       ((UINT32_C(1) << 31) | 1) /* Unknown option. */
 #define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */
+#define NBD_REP_ERR_TLS_REQD    ((UINT32_C(1) << 31) | 5) /* TLS required */
+
 
 #define NBD_CMD_MASK_COMMAND	0x0000ffff
 #define NBD_CMD_FLAG_FUA	(1 << 16)
@@ -81,6 +84,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
                      size_t length,
                      bool do_read);
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+                          QCryptoTLSCreds *tlscreds, const char *hostname,
+                          QIOChannel **outioc,
                           off_t *size, Error **errp);
 int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
 ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
@@ -106,6 +111,8 @@ void nbd_export_close_all(void);
 
 NBDClient *nbd_client_new(NBDExport *exp,
                           QIOChannelSocket *sioc,
+                          QCryptoTLSCreds *tlscreds,
+                          const char *tlsaclname,
                           void (*close)(NBDClient *));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
nbd.c +270 -14

436 lines not on this page · raw message

qemu-nbd.c +3 -1
index 3f7f2222..24cdb65 100644
@@ -254,6 +254,7 @@ static void *nbd_client_thread(void *arg)
     }
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
+                                NULL, NULL, NULL,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -342,7 +343,8 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
         return TRUE;
     }
 
-    if (nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed)) {
+    if (nbd_client_new(newproto ? NULL : exp, cioc,
+                       NULL, NULL, nbd_client_closed)) {
         nb_fds++;
         nbd_update_server_watch();
     }
Signature · 1 line
2.5.0
#13 lore raw

Re: [Qemu-devel] [PATCH 12/15] nbd: implement TLS support in the protocol negotiation

Minor nitpick:

On Fri, Nov 27, 2015 at 12:20:50PM +0000, Daniel P. Berrange wrote:
[...]

nbd.c static int nbd_receive_options(NBDClient *client) · line 668

Quoted 11 lines
nbd.c +7 -0
@@ -563,6 +659,14 @@ static int nbd_receive_options(NBDClient *client)
             case NBD_OPT_EXPORT_NAME:
                 return nbd_handle_export_name(client, length);
 
+            case NBD_OPT_STARTTLS:
+                if (client->tlscreds) {
+                    TRACE("TLS already enabled");
+                } else {
+                    TRACE("TLS not configured");
+                }
+                nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, clientflags);

NBD_REP_ERR_UNSUP is supposed to be reserved as the default reply for replies unknown to a server implementation (i.e., it's "this request is not supported by this server"). Trying to negotiate TLS in a TLS channel would be NBD_REP_ERR_INVALID ("invalid request"). Trying to negotiate TLS when no TLS configuration is available server-side would be NBD_REP_ERR_POLICY ("request not allowed by server-side policy").

Keeping to these error codes would allow a client to provide more useful information to a user beyond "haha it fail"; but I suppose there can be arguments for not doing so, too.

Beyond this and the default export that I talked about earlier, no comments.

Signature · 3 lines
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26
#14 lore raw

Re: [Qemu-devel] [PATCH 12/15] nbd: implement TLS support in the protocol negotiation

Quoted 8 lines · Wouter Verhelst

Minor nitpick:

On Fri, Nov 27, 2015 at 12:20:50PM +0000, Daniel P. Berrange wrote:
[...]

Quoted 4 lines
nbd.c +0 -0
@@ -563,6 +659,14 @@ static int nbd_receive_options(NBDClient *client)
             case NBD_OPT_EXPORT_NAME:
                 return nbd_handle_export_name(client, length);
 
Quoted 14 lines

nbd.c static int nbd_receive_options(NBDClient *client) · line 668

Quoted 7 lines
nbd.c +7 -0
+            case NBD_OPT_STARTTLS:
+                if (client->tlscreds) {
+                    TRACE("TLS already enabled");
+                } else {
+                    TRACE("TLS not configured");
+                }
+                nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, clientflags);

NBD_REP_ERR_UNSUP is supposed to be reserved as the default reply for replies unknown to a server implementation (i.e., it's "this request is not supported by this server"). Trying to negotiate TLS in a TLS channel would be NBD_REP_ERR_INVALID ("invalid request"). Trying to negotiate TLS when no TLS configuration is available server-side would be NBD_REP_ERR_POLICY ("request not allowed by server-side policy").

Yep that makes sense.

Quoted 2 lines

Beyond this and the default export that I talked about earlier, no comments.

Ok, thanks for taking the time to look at this.

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#15 lore raw

PATCH 13/15 nbd: enable use of TLS with NBD block driver

In reply to Daniel P. Berrange

This modifies the NBD driver so that it is possible to request use of TLS. This is done by providing the 'tls-creds' parameter with the ID of a previously created QCryptoTLSCreds object.

For example

  $QEMU -object tls-creds-x509,id=tls0,endpoint=client,\
                dir=/home/berrange/security/qemutls \
        -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

The client will drop the connection if the NBD server does not provide TLS.

---
 block/nbd-client.c | 10 ++++---
 block/nbd-client.h |  2 ++
 block/nbd.c        | 78 +++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 78 insertions(+), 12 deletions(-)
block/nbd-client.c +7 -3
index f7bacfd..024d5f3 100644
@@ -378,8 +378,12 @@ void nbd_client_close(BlockDriverState *bs)
     nbd_teardown_connection(bs);
 }
 
-int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
-                    const char *export, Error **errp)
+int nbd_client_init(BlockDriverState *bs,
+                    QIOChannelSocket *sioc,
+                    const char *export,
+                    QCryptoTLSCreds *tlscreds,
+                    const char *hostname,
+                    Error **errp)
 {
     NbdClientSession *client = nbd_get_client_session(bs);
     int ret;
@@ -390,7 +394,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
                                 &client->nbdflags,
-                                NULL, NULL,
+                                tlscreds, hostname,
                                 &client->ioc,
                                 &client->size, errp);
     if (ret < 0) {
block/nbd-client.h +2 -0
index e8b3283..53f116d 100644
@@ -39,6 +39,8 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
 int nbd_client_init(BlockDriverState *bs,
                     QIOChannelSocket *sock,
                     const char *export_name,
+                    QCryptoTLSCreds *tlscreds,
+                    const char *hostname,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
block/nbd.c +69 -9

117 lines not on this page · raw message

Signature · 1 line
2.5.0
#16 lore raw

PATCH 14/15 nbd: enable use of TLS with qemu-nbd server

In reply to Daniel P. Berrange

This modifies the qemu-nbd program so that it is possible to request the use of TLS with the server. It simply adds a new command line option --tls-creds which is used to provide the ID of a QCryptoTLSCreds object previously created via the --object command line option.

For example

  qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
                    dir=/home/berrange/security/qemutls \
           --tls-creds tls0
	   --exportname default

Note that it is mandatory to supply the --exportname argument when requesting TLS, since it requires use of the new style NBD protocol where the client requests a volume name explicitly.

TLS is only supported when using an IPv4/IPv6 socket listener. It is not possible to use with UNIX sockets, which includes when connecting the NBD server to a host device.

---
 qemu-nbd.c    | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-nbd.texi |  4 ++++
 2 files changed, 60 insertions(+), 1 deletion(-)
qemu-nbd.c +56 -1
index 24cdb65..e579188 100644
@@ -45,6 +45,7 @@
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
 #define QEMU_NBD_OPT_OBJECT        5
+#define QEMU_NBD_OPT_TLSCREDS      6
 
 static NBDExport *exp;
 static bool newproto;
@@ -57,6 +58,7 @@ static int shared = 1;
 static int nb_fds;
 static QIOChannelSocket *server_ioc;
 static int server_watch = -1;
+static QCryptoTLSCreds *tlscreds;
 
 static void usage(const char *name)
 {
@@ -344,7 +346,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
     }
 
     if (nbd_client_new(newproto ? NULL : exp, cioc,
-                       NULL, NULL, nbd_client_closed)) {
+                       tlscreds, NULL, nbd_client_closed)) {
         nb_fds++;
         nbd_update_server_watch();
     }
@@ -458,6 +460,37 @@ out:
     return 0;
 }
 
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+    Object *obj;
+    QCryptoTLSCreds *creds;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), id);
+    if (!obj) {
+        error_setg(errp, "No TLS credentials with id '%s'",
+                   id);
+        return NULL;
+    }
+    creds = (QCryptoTLSCreds *)
+        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+    if (!creds) {
+        error_setg(errp, "Object with id '%s' is not TLS credentials",
+                   id);
+        return NULL;
+    }
+
+    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+        error_setg(errp,
+                   "Expecting TLS credentials with a server endpoint");
+        return NULL;
+    }
+    object_ref(obj);
+    return creds;
+}
+
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -497,6 +530,7 @@ int main(int argc, char **argv)
         { "verbose", 0, NULL, 'v' },
         { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { "exportname", 1, NULL, 'x' },
+        { "tls-creds", 1, NULL, QEMU_NBD_OPT_TLSCREDS },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -515,6 +549,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     QemuOpts *opts;
     const char *exportname = NULL;
+    const char *tlscredsid = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -671,6 +706,9 @@ int main(int argc, char **argv)
                 exit(1);
             }
             break;
+        case QEMU_NBD_OPT_TLSCREDS:
+            tlscredsid = optarg;
+            break;
         case '?':
             errx(EXIT_FAILURE, "Try `%s --help' for more information.",
                  argv[0]);
@@ -689,6 +727,23 @@ int main(int argc, char **argv)
         exit(1);
     }
 
+    if (tlscredsid) {
+        if (!exportname) {
+            errx(EXIT_FAILURE, "Export name is required when using TLS");
+        }
+        if (sockpath) {
+            errx(EXIT_FAILURE, "TLS is only supported with IPv4/IPv6");
+        }
+        if (device) {
+            errx(EXIT_FAILURE, "TLS is not supported with a host device");
+        }
+        tlscreds = nbd_get_tls_creds(tlscredsid, &local_err);
+        if (local_err) {
+            errx(EXIT_FAILURE, "Failed to get TLS creds %s",
+                 error_get_pretty(local_err));
+        }
+    }
+
     if (disconnect) {
         int nbdfd = open(argv[optind], O_RDWR);
         if (nbdfd < 0) {
qemu-nbd.texi +4 -0
index 6ff3920..a12d2e1 100644
@@ -67,6 +67,10 @@ Export QEMU disk image using NBD protocol.
 @item -x NAME, --exportname=NAME
   set the NDB volume export name. This switches the server to use
   the new style NBD protocol negotiation
+@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
+  option.
 @item -v, --verbose
   display extra debugging information
 @item -h, --help
Signature · 1 line
2.5.0
#17 lore raw

PATCH 15/15 nbd: enable use of TLS with nbd-server-start command

In reply to Daniel P. Berrange

This modifies the nbd-server-start QMP command so that it is possible to request use of TLS. This is done by adding a new optional parameter "tls-creds" which provides the ID of a previously created QCryptoTLSCreds object instance.

TLS is only supported when using an IPv4/IPv6 socket listener.

---
 blockdev-nbd.c  | 122 ++++++++++++++++++++++++++++++++++++++++++++++----------
 hmp.c           |   2 +-
 qapi/block.json |   4 +-
 qmp-commands.hx |   2 +-
 4 files changed, 105 insertions(+), 25 deletions(-)
blockdev-nbd.c +100 -22
index 6af1684..e287f05 100644
@@ -19,42 +19,126 @@
 #include "block/nbd.h"
 #include "io/channel-socket.h"
 
-static QIOChannelSocket *server_ioc;
-static int server_watch = -1;
+typedef struct NBDServerData {
+    QIOChannelSocket *listen_ioc;
+    int watch;
+    QCryptoTLSCreds *tlscreds;
+} NBDServerData;
+
+static NBDServerData *nbd_server;
+
 
 static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
                            gpointer opaque)
 {
     QIOChannelSocket *cioc;
 
+    if (!nbd_server) {
+        return FALSE;
+    }
+
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
     if (!cioc) {
         return TRUE;
     }
 
-    nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
+    nbd_client_new(NULL, cioc,
+                   nbd_server->tlscreds, NULL,
+                   nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
 
-void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+
+static void nbd_server_free(NBDServerData *server)
 {
-    if (server_ioc) {
-        error_setg(errp, "NBD server already running");
+    if (!server) {
         return;
     }
 
-    server_ioc = qio_channel_socket_new();
-    if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+    if (server->watch != -1) {
+        g_source_remove(server->watch);
+    }
+    object_unref(OBJECT(server->listen_ioc));
+    if (server->tlscreds) {
+        object_unref(OBJECT(server->tlscreds));
+    }
+
+    g_free(server);
+}
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+    Object *obj;
+    QCryptoTLSCreds *creds;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), id);
+    if (!obj) {
+        error_setg(errp, "No TLS credentials with id '%s'",
+                   id);
+        return NULL;
+    }
+    creds = (QCryptoTLSCreds *)
+        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+    if (!creds) {
+        error_setg(errp, "Object with id '%s' is not TLS credentials",
+                   id);
+        return NULL;
+    }
+
+    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+        error_setg(errp,
+                   "Expecting TLS credentials with a server endpoint");
+        return NULL;
+    }
+    object_ref(obj);
+    return creds;
+}
+
+
+void qmp_nbd_server_start(SocketAddress *addr,
+                          bool has_tls_creds, const char *tls_creds,
+                          Error **errp)
+{
+    if (nbd_server) {
+        error_setg(errp, "NBD server already running");
         return;
     }
 
-    server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
-                                         G_IO_IN,
-                                         nbd_accept,
-                                         NULL,
-                                         NULL);
+    nbd_server = g_new0(NBDServerData, 1);
+    nbd_server->watch = -1;
+    nbd_server->listen_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(
+            nbd_server->listen_ioc, addr, errp) < 0) {
+        goto error;
+    }
+
+    if (has_tls_creds) {
+        nbd_server->tlscreds = nbd_get_tls_creds(tls_creds, errp);
+        if (!nbd_server->tlscreds) {
+            goto error;
+        }
+
+        if (addr->type != SOCKET_ADDRESS_KIND_INET) {
+            error_setg(errp, "TLS is only supported with IPv4/IPv6");
+            goto error;
+        }
+    }
+
+    nbd_server->watch = qio_channel_add_watch(
+        QIO_CHANNEL(nbd_server->listen_ioc),
+        G_IO_IN,
+        nbd_accept,
+        NULL,
+        NULL);
+
+    return;
+
+ error:
+    nbd_server_free(nbd_server);
+    nbd_server = NULL;
 }
 
 /*
@@ -89,7 +173,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     NBDExport *exp;
     NBDCloseNotifier *n;
 
-    if (!server_ioc) {
+    if (!nbd_server) {
         error_setg(errp, "NBD server not running");
         return;
     }
@@ -139,12 +223,6 @@ void qmp_nbd_server_stop(Error **errp)
         nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
     }
 
-    if (server_watch != -1) {
-        g_source_remove(server_watch);
-        server_watch = -1;
-    }
-    if (server_ioc) {
-        object_unref(OBJECT(server_ioc));
-        server_ioc = NULL;
-    }
+    nbd_server_free(nbd_server);
+    nbd_server = NULL;
 }
hmp.c +1 -1
index 6044db3..89a37cf 100644
@@ -1830,7 +1830,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }
 
-    qmp_nbd_server_start(addr, &local_err);
+    qmp_nbd_server_start(addr, false, NULL, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
qapi/block.json +3 -1
index 84022f1..876bac4 100644
@@ -130,13 +130,15 @@
 # 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
 #
 # Returns: error if the server is already running.
 #
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-start',
-  'data': { 'addr': 'SocketAddress' } }
+  'data': { 'addr': 'SocketAddress',
+            '*tls-creds': 'str'} }
 
 ##
 # @nbd-server-add:
qmp-commands.hx +1 -1
index 9d8b42f..fd353e6 100644
@@ -3752,7 +3752,7 @@ EQMP
 
     {
         .name       = "nbd-server-start",
-        .args_type  = "addr:q",
+        .args_type  = "addr:q,tls-creds:s?",
         .mhandler.cmd_new = qmp_marshal_nbd_server_start,
     },
     {
Signature · 1 line
2.5.0
#18 lore raw
In reply to Daniel P. Berrange

[nbd-general added to Cc]

Hi Daniel,

Quoted 34 lines · Daniel P. Berrange

This series of patches implements support for TLS in the QEMU NBD server and client code.

It is implementing the NBD_OPT_STARTTLS option that was previously discussed here:

https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html

And is also described in the NBD spec here:

https://github.com/yoe/nbd/blob/master/doc/proto.md

Based on my impl I think the NBD_OPT_STARTTLS specification is fine. In my impl, when TLS is requested in the server, I chose to *refuse* all NBD options until NBD_OPT_STARTTLS has completed successfully. There is an annoying thing about the fixed new style protocol in that it made an exception for NBD_OPT_EXPORT_NAME, so that it is still not possible to report errors for that option.

[qupte]
- The server will reply to any option apart from NBD_OPT_EXPORT_NAME with reply packets in the following format:

S: 64 bits, 0x3e889045565a9 (magic number for replies)
S: 32 bits, the option as sent by the client to which this is a reply
S: 32 bits, reply type (e.g., NBD_REP_ACK for successful completion,
or NBD_REP_ERR_UNSUP to mark use of an option not known by this server
S: 32 bits, length of the reply. This may be zero for some replies, in
which case the next field is not sent
S: any data as required by the reply (e.g., an export name in the case
of NBD_REP_SERVER)
[/quote]

Quoted 3 lines

I wish those words "apart from NBD_OPT_EXPORT_NAME" were not there for fixed new style protocol - maybe it needs a very-fixed new style protocol...

Yes. This was done in this way to more easily retain backwards compatibility with non-fixed newstyle. In hindsight, it was probably a mistake to do things that way, but it's not going to be easy to turn back the clock now...

I have been thinking of adding a message NBD_OPT_SELECT_EXPORT to replace NBD_OPT_EXPORT_NAME, which would select an export but not end negotiation. That would also require another message to end negotiation and move on to the transmission phase, obviously.

The negotation would then look something like:

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ACK
NBD_OPT_GO
<- NBD_REP_ACK
[transmission phase]

or, to add some error conditions:

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ERR_UNSUP # this server doesn't support SELECT_EXPORT
NBD_OPT_EXPORT_NAME # fall back to older message [transmission phase]

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ERR_TLS_REQD # this export requires TLS
NBD_OPT_STARTTLS
<- NBD_REP_ACK
[... tls ...]
NBD_OPT_SELECT_EXPORT
<- NBD_REP_ACK
NBD_OPT_GO
<- NBD_REP_ACK
[transmission phase]

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ACK
NBD_OPT_STARTTLS
<- NBD_REP_ACK
[... tls ...]
NBD_OPT_GO
<- NBD_REP_ERR_INVALID # export selected before starttls is not retained

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ACK
NBD_OPT_SELECT_EXPORT (we actually want this other one)
<- NBD_REP_ERR_UNKNOWN # unknown export
NBD_OPT_GO
<- NBD_REP_ERR_INVALID # second select cleared the result of the first one

etc

I'm not sure whether it's worth the trouble, though.

Quoted 6 lines

As is, if the client connects to a TLS enabled NBD server and then immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us to send back NBD_REP_ERR_TLS_REQD as the spec requires that the server close the connection :-( For this reason I have made the qemu NBD client always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response.

I suppose that is a valid workaround.

Quoted 8 lines

I chose to *NOT* implement the NBD_OPT_PEEK_EXPORT option as it is inherently insecure to have a client to ask the server which exports need TLS, while the protocol is still running in plain text mode, as it allows a trivial MITM downgrade attack. I can see value in the NBD_OPT_PEEK_EXPORT option for probing other features, but only after TLS has been negotiated. As such I believe NBD_F_EXP_TLS_OK and NBD_F_EXP_TLS_REQ should be deleted from the spec as it is not possible to use them in a secure manner.

Mm. Fair enough.

The reason for adding that flag was to support encrypted and non-encrypted exports from the same server, but I suppose we can signal those things with NBD_REP_ERR_TLS_REQD and NBD_REP_ERR_POLICY, too.

Quoted 18 lines

This series of patches has my v3 I/O channels patch series as a pre-requisite:

https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04208.html

Usage of TLS is described in the commit messages for each patch, but for sake of people who don't want to explore the series, here's the summary

Starting QEMU system emulator with a disk backed by an TLS encrypted NBD export

  $ qemu-system-x86_64 \
     -object tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls
     -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

Starting a standalone NBD server providing a TLS encrypted NBD export

Quoted 7 lines
  $ qemu-nbd \
     --object tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
     --tls-creds tls0 \
     --exportname default

NB, exportname is mandatory, since TLS requires the fixed-new style NBD protocol negotiation.

Note, I have modified the spec of the fixed newstyle negotiation a while back to include support for a default export:

'A special, "empty", name (i.e., the length field is zero and no name is specified), is reserved for a "default" export, to be used in cases where explicitly specifying an export name makes no sense.'

(in the "Option types" section for NBD_OPT_EXPORT_NAME)

This removes the requirement to have an export name specified externally. Having said that though, obviously adding support for specifying a name isn't a bad thing.

Signature · 3 lines
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26
#19 lore raw

Hi all,

Quoted 4 lines · Wouter Verhelst

I have been thinking of adding a message NBD_OPT_SELECT_EXPORT to replace NBD_OPT_EXPORT_NAME, which would select an export but not end negotiation. That would also require another message to end negotiation and move on to the transmission phase, obviously.

I've formalized this now, and removed the PEEK_EXPORT extension (which, as you say, can't be used safely).

Quoted 4 lines

The negotation would then look something like:

NBD_OPT_SELECT_EXPORT
<- NBD_REP_ACK

This has become an NBD_REP_SERVER instead, with (64-bit) export size and (16-bit) per-export flags appended (rather than yet another flags field).

(the name is also NBD_OPT_SELECT rather than NBD_OPT_SELECT_EXPORT; details)

NBD_OPT_SELECT is 6, NBD_OPT_GO is 7, NBD_REP_ERR_UNKNOWN is 2^31+6.

For more details, see the proto.md document.

Thoughts are welcome.

Signature · 3 lines
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26
#20 lore raw
In reply to Daniel P. Berrange

Hi Daniel,

Something occurred to me earlier today:

Quoted 6 lines · Daniel P. Berrange

As is, if the client connects to a TLS enabled NBD server and then immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us to send back NBD_REP_ERR_TLS_REQD as the spec requires that the server close the connection :-( For this reason I have made the qemu NBD client always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response.

Why not have it send NBD_OPT_STARTTLS as the first message if you want to do TLS? That way, either the server doesn't support it because too old (and you get NBD_REP_ERR_UNSUP) or configuration (and you get NBD_REP_ERR_POLICY), or it does and you're in TLS.

Did I miss something?

Signature · 3 lines
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26
#21 lore raw
Quoted 12 lines · Wouter Verhelst

Hi Daniel,

Something occurred to me earlier today:

Quoted 6 lines · Daniel P. Berrange

As is, if the client connects to a TLS enabled NBD server and then immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us to send back NBD_REP_ERR_TLS_REQD as the spec requires that the server close the connection :-( For this reason I have made the qemu NBD client always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response.

Quoted 6 lines

Why not have it send NBD_OPT_STARTTLS as the first message if you want to do TLS? That way, either the server doesn't support it because too old (and you get NBD_REP_ERR_UNSUP) or configuration (and you get NBD_REP_ERR_POLICY), or it does and you're in TLS.

Did I miss something?

Yes, if the client wants TLS, it will send NBD_OPT_STARTTLS as the first thing it does.

My comment above was refering to the case of a client not wanting TLS which is connecting to a server that does want TLS. In that case sending NBD_OPT_LIST is what we need to do in order to get a suitable error from the server about requirement for TLS.

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#22 lore raw
Quoted 20 lines · Daniel P. Berrange
Quoted 12 lines · Wouter Verhelst

Hi Daniel,

Something occurred to me earlier today:

Quoted 6 lines · Daniel P. Berrange

As is, if the client connects to a TLS enabled NBD server and then immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us to send back NBD_REP_ERR_TLS_REQD as the spec requires that the server close the connection :-( For this reason I have made the qemu NBD client always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response.

Quoted 6 lines

Why not have it send NBD_OPT_STARTTLS as the first message if you want to do TLS? That way, either the server doesn't support it because too old (and you get NBD_REP_ERR_UNSUP) or configuration (and you get NBD_REP_ERR_POLICY), or it does and you're in TLS.

Did I miss something?

Quoted 7 lines

Yes, if the client wants TLS, it will send NBD_OPT_STARTTLS as the first thing it does.

My comment above was refering to the case of a client not wanting TLS which is connecting to a server that does want TLS. In that case sending NBD_OPT_LIST is what we need to do in order to get a suitable error from the server about requirement for TLS.

Right, of course. Yes, that makes sense. Ignore what I said :-)

Signature · 3 lines
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26