Libguestfs - Sep 2019 - [PATCH libnbd 0/5] interop: Check that LIBNBD_TLS_ALLOW works against nbdkit.

I was a little surprised to find that LIBNBD_TLS_ALLOW worked out of
the box, so I had to examine the logs whereupon I saw the magic
message ...

  libnbd: debug: nbd1: nbd_connect_command: server refused TLS (policy),
continuing with unencrypted connection

I don't believe this path has ever been tested before.

It's possible the tests could be improved if they actually checked for
this message, but the patches as posted try to use regular APIs.

Rich.

Simple refactoring with no effect.
---
 interop/Makefile.am | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/interop/Makefile.am b/interop/Makefile.am
index 1d2d187..7bb44d9 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -47,8 +47,6 @@ if HAVE_QEMU_NBD
 
 check_PROGRAMS += \
 	interop-qemu-nbd \
-	interop-qemu-nbd-tls-certs \
-	interop-qemu-nbd-tls-psk \
 	dirty-bitmap \
 	structured-read \
 	$(NULL)
@@ -64,11 +62,17 @@ TESTS += \
 if HAVE_NBDKIT
 if HAVE_GNUTLS
 if HAVE_CERTTOOL
+check_PROGRAMS += \
+	interop-qemu-nbd-tls-certs \
+	$(NULL)
 TESTS += \
 	interop-qemu-nbd-tls-certs \
 	$(NULL)
 endif
 if HAVE_PSKTOOL
+check_PROGRAMS += \
+	interop-qemu-nbd-tls-psk \
+	$(NULL)
 TESTS += \
 	interop-qemu-nbd-tls-psk \
 	$(NULL)
-- 
2.23.0

Neutral refactoring.
---
 interop/interop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/interop/interop.c b/interop/interop.c
index a3ab39b..0b7b1a5 100644
--- a/interop/interop.c
+++ b/interop/interop.c
@@ -66,10 +66,12 @@ main (int argc, char *argv[])
     goto out;
   }
 
+#ifdef EXPORT_NAME
   if (nbd_set_export_name (nbd, EXPORT_NAME) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     goto out;
   }
+#endif
 
 #if CERTS || PSK
   /* Require TLS on the handle and fail if not available or if the
-- 
2.23.0

Although the main test suite already depends on nbdkit rather heavily
and therefore it's pretty well tested, add a separate set of interop
tests.  The purpose will become clear in a subsequent commit.
---
 .gitignore          |  3 +++
 README              |  4 +--
 interop/Makefile.am | 60 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 53aa206..ab47370 100644
--- a/.gitignore
+++ b/.gitignore
@@ -60,6 +60,9 @@ Makefile.in
 /html/*.?.html
 /include/libnbd.h
 /interop/dirty-bitmap
+/interop/interop-nbdkit
+/interop/interop-nbdkit-tls-certs
+/interop/interop-nbdkit-tls-psk
 /interop/interop-nbd-server
 /interop/interop-qemu-nbd
 /interop/interop-qemu-nbd-tls-certs
diff --git a/README b/README
index d6d1704..1c9d816 100644
--- a/README
+++ b/README
@@ -87,8 +87,8 @@ Optional, only needed to run the test suite:
  * nbdkit >= 1.12, the nbdkit basic plugins and the nbdkit basic
    filters are recommended as they are needed to run the test suite.
 
- * nbd-server and qemu-nbd if you want to do interoperability testing
-   against these servers.
+ * nbdkit, nbd-server and qemu-nbd if you want to do interoperability
+   testing against these servers.
 
  * A C++ compiler is needed if you want to test that the library works
    from C++.
diff --git a/interop/Makefile.am b/interop/Makefile.am
index 7bb44d9..9cb8071 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -129,5 +129,65 @@ structured_read_LDADD = $(top_builddir)/lib/libnbd.la
 
 endif HAVE_QEMU_NBD
 
+if HAVE_NBDKIT
+
+check_PROGRAMS += \
+	interop-nbdkit \
+	$(NULL)
+TESTS += \
+	interop-nbdkit \
+	$(NULL)
+
+# See above comment about files in ../tests/Makefile.am
+if HAVE_GNUTLS
+if HAVE_CERTTOOL
+check_PROGRAMS += \
+	interop-nbdkit-tls-certs \
+	$(NULL)
+TESTS += \
+	interop-nbdkit-tls-certs \
+	$(NULL)
+endif
+if HAVE_PSKTOOL
+check_PROGRAMS += \
+	interop-nbdkit-tls-psk \
+	$(NULL)
+TESTS += \
+	interop-nbdkit-tls-psk \
+	$(NULL)
+endif
+endif
+
+interop_nbdkit_SOURCES = interop.c
+interop_nbdkit_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"-s", "--exit-with-parent",
"file", tmpfile' \
+	$(NULL)
+interop_nbdkit_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_LDADD = $(top_builddir)/lib/libnbd.la
+
+interop_nbdkit_tls_certs_SOURCES = interop.c
+interop_nbdkit_tls_certs_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=require",
"--tls-certificates=../tests/pki", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DCERTS=1 \
+	$(NULL)
+interop_nbdkit_tls_certs_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_certs_LDADD = $(top_builddir)/lib/libnbd.la
+
+interop_nbdkit_tls_psk_SOURCES = interop.c
+interop_nbdkit_tls_psk_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=require",
"--tls-psk=../tests/keys.psk", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DPSK=1 \
+	$(NULL)
+interop_nbdkit_tls_psk_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_psk_LDADD = $(top_builddir)/lib/libnbd.la
+
+endif HAVE_NBDKIT
+
 check-valgrind:
 	LIBNBD_VALGRIND=1 $(MAKE) check
-- 
2.23.0

This neutral refactoring adds -DTLS_MODE.  We can in future change the
requested TLS mode, but not in this commit.

It also checks that nbd_get_tls_negotiated returns true after
connecting, when the requested mode was set to LIBNBD_TLS_REQUIRE.
---
 interop/Makefile.am |  4 ++++
 interop/interop.c   | 26 ++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/interop/Makefile.am b/interop/Makefile.am
index 9cb8071..8a5b787 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -100,6 +100,7 @@ interop_qemu_nbd_tls_certs_CPPFLAGS = \
 	-DSERVER_PARAMS='"--object",
"tls-creds-x509,id=tls0,endpoint=server,dir=$(abs_top_builddir)/tests/pki",
"--tls-creds", "tls0", "-f", "raw",
"-x", "/", "-p", port_str, tmpfile' \
 	-DEXPORT_NAME='"/"' \
 	-DCERTS=1 \
+	-DTLS_MODE=LIBNBD_TLS_REQUIRE \
 	$(NULL)
 interop_qemu_nbd_tls_certs_CFLAGS = $(WARNINGS_CFLAGS)
 interop_qemu_nbd_tls_certs_LDADD = $(top_builddir)/lib/libnbd.la
@@ -113,6 +114,7 @@ interop_qemu_nbd_tls_psk_CPPFLAGS = \
 	-DSERVER_PARAMS='"--object",
"tls-creds-psk,id=tls0,endpoint=server,dir=$(abs_top_builddir)/tests",
"--tls-creds", "tls0", "-f", "raw",
"-x", "/", "-p", port_str, tmpfile' \
 	-DEXPORT_NAME='"/"' \
 	-DPSK=1 \
+	-DTLS_MODE=LIBNBD_TLS_REQUIRE \
 	$(NULL)
 interop_qemu_nbd_tls_psk_CFLAGS = $(WARNINGS_CFLAGS)
 interop_qemu_nbd_tls_psk_LDADD = $(top_builddir)/lib/libnbd.la
@@ -173,6 +175,7 @@ interop_nbdkit_tls_certs_CPPFLAGS = \
 	-DSERVER=\"$(NBDKIT)\" \
 	-DSERVER_PARAMS='"--tls=require",
"--tls-certificates=../tests/pki", "-s",
"--exit-with-parent", "file", tmpfile' \
 	-DCERTS=1 \
+	-DTLS_MODE=LIBNBD_TLS_REQUIRE \
 	$(NULL)
 interop_nbdkit_tls_certs_CFLAGS = $(WARNINGS_CFLAGS)
 interop_nbdkit_tls_certs_LDADD = $(top_builddir)/lib/libnbd.la
@@ -183,6 +186,7 @@ interop_nbdkit_tls_psk_CPPFLAGS = \
 	-DSERVER=\"$(NBDKIT)\" \
 	-DSERVER_PARAMS='"--tls=require",
"--tls-psk=../tests/keys.psk", "-s",
"--exit-with-parent", "file", tmpfile' \
 	-DPSK=1 \
+	-DTLS_MODE=LIBNBD_TLS_REQUIRE \
 	$(NULL)
 interop_nbdkit_tls_psk_CFLAGS = $(WARNINGS_CFLAGS)
 interop_nbdkit_tls_psk_LDADD = $(top_builddir)/lib/libnbd.la
diff --git a/interop/interop.c b/interop/interop.c
index 0b7b1a5..2772721 100644
--- a/interop/interop.c
+++ b/interop/interop.c
@@ -35,6 +35,13 @@
 
 #define SIZE (1024*1024)
 
+#if CERTS || PSK
+#define TLS 1
+#ifndef TLS_MODE
+#error "TLS_MODE must be defined when using CERTS || PSK"
+#endif
+#endif
+
 int
 main (int argc, char *argv[])
 {
@@ -73,15 +80,12 @@ main (int argc, char *argv[])
   }
 #endif
 
-#if CERTS || PSK
-  /* Require TLS on the handle and fail if not available or if the
-   * handshake fails.
-   */
+#if TLS
   if (nbd_supports_tls (nbd) != 1) {
     fprintf (stderr, "skip: compiled without TLS support\n");
     exit (77);
   }
-  if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) {
+  if (nbd_set_tls (nbd, TLS_MODE) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -142,6 +146,16 @@ main (int argc, char *argv[])
 
 #endif
 
+#if TLS
+  if (TLS_MODE == LIBNBD_TLS_REQUIRE &&
+      nbd_get_tls_negotiated (nbd) != 1) {
+    fprintf (stderr,
+             "%s: TLS required, but not negotiated on the
connection\n",
+             argv[0]);
+    goto out;
+  }
+#endif
+
   actual_size = nbd_get_size (nbd);
   if (actual_size == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
@@ -160,7 +174,7 @@ main (int argc, char *argv[])
 
   /* XXX In future test more operations here. */
 
-#if !CERTS && !PSK
+#if !TLS
   /* XXX qemu doesn't shut down the connection nicely (using
    * gnutls_bye) and because of this the following call will fail
    * with:
-- 
2.23.0

Test both the TLS enabled and fallback paths.

nbd-server doesn't appear to support TLS at all, and qemu-nbd is known
not to allow fallback to unencrypted, and therefore it only makes
sense to test nbdkit at the moment.
---
 .gitignore          |  4 ++++
 TODO                |  3 ---
 interop/Makefile.am | 54 +++++++++++++++++++++++++++++++++++++++++++++
 interop/interop.c   | 30 ++++++++++++++++++++-----
 4 files changed, 82 insertions(+), 9 deletions(-)

diff --git a/.gitignore b/.gitignore
index ab47370..dd8a052 100644
--- a/.gitignore
+++ b/.gitignore
@@ -62,7 +62,11 @@ Makefile.in
 /interop/dirty-bitmap
 /interop/interop-nbdkit
 /interop/interop-nbdkit-tls-certs
+/interop/interop-nbdkit-tls-certs-allow-enabled
+/interop/interop-nbdkit-tls-certs-allow-fallback
 /interop/interop-nbdkit-tls-psk
+/interop/interop-nbdkit-tls-psk-allow-enabled
+/interop/interop-nbdkit-tls-psk-allow-fallback
 /interop/interop-nbd-server
 /interop/interop-qemu-nbd
 /interop/interop-qemu-nbd-tls-certs
diff --git a/TODO b/TODO
index 21feb2f..642d39f 100644
--- a/TODO
+++ b/TODO
@@ -17,9 +17,6 @@ NBD_INFO_BLOCK_SIZE.
 
 TLS should properly shut down the session (calling gnutls_bye).
 
-LIBNBD_TLS_ALLOW is not tested.  Related to this,
-nbd_get_tls_negotiated is not tested.
-
 Implement nbd_connect + systemd socket activation.
 
 Improve function trace output so that:
diff --git a/interop/Makefile.am b/interop/Makefile.am
index 8a5b787..43350a8 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -145,17 +145,25 @@ if HAVE_GNUTLS
 if HAVE_CERTTOOL
 check_PROGRAMS += \
 	interop-nbdkit-tls-certs \
+	interop-nbdkit-tls-certs-allow-enabled \
+	interop-nbdkit-tls-certs-allow-fallback \
 	$(NULL)
 TESTS += \
 	interop-nbdkit-tls-certs \
+	interop-nbdkit-tls-certs-allow-enabled \
+	interop-nbdkit-tls-certs-allow-fallback \
 	$(NULL)
 endif
 if HAVE_PSKTOOL
 check_PROGRAMS += \
 	interop-nbdkit-tls-psk \
+	interop-nbdkit-tls-psk-allow-enabled \
+	interop-nbdkit-tls-psk-allow-fallback \
 	$(NULL)
 TESTS += \
 	interop-nbdkit-tls-psk \
+	interop-nbdkit-tls-psk-allow-enabled \
+	interop-nbdkit-tls-psk-allow-fallback \
 	$(NULL)
 endif
 endif
@@ -180,6 +188,29 @@ interop_nbdkit_tls_certs_CPPFLAGS = \
 interop_nbdkit_tls_certs_CFLAGS = $(WARNINGS_CFLAGS)
 interop_nbdkit_tls_certs_LDADD = $(top_builddir)/lib/libnbd.la
 
+interop_nbdkit_tls_certs_allow_enabled_SOURCES = interop.c
+interop_nbdkit_tls_certs_allow_enabled_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=require",
"--tls-certificates=../tests/pki", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DCERTS=1 \
+	-DTLS_MODE=LIBNBD_TLS_ALLOW \
+	$(NULL)
+interop_nbdkit_tls_certs_allow_enabled_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_certs_allow_enabled_LDADD = $(top_builddir)/lib/libnbd.la
+
+interop_nbdkit_tls_certs_allow_fallback_SOURCES = interop.c
+interop_nbdkit_tls_certs_allow_fallback_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=off", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DCERTS=1 \
+	-DTLS_MODE=LIBNBD_TLS_ALLOW \
+	-DTLS_FALLBACK=1 \
+	$(NULL)
+interop_nbdkit_tls_certs_allow_fallback_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_certs_allow_fallback_LDADD = $(top_builddir)/lib/libnbd.la
+
 interop_nbdkit_tls_psk_SOURCES = interop.c
 interop_nbdkit_tls_psk_CPPFLAGS = \
 	-I$(top_srcdir)/include \
@@ -191,6 +222,29 @@ interop_nbdkit_tls_psk_CPPFLAGS = \
 interop_nbdkit_tls_psk_CFLAGS = $(WARNINGS_CFLAGS)
 interop_nbdkit_tls_psk_LDADD = $(top_builddir)/lib/libnbd.la
 
+interop_nbdkit_tls_psk_allow_enabled_SOURCES = interop.c
+interop_nbdkit_tls_psk_allow_enabled_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=require",
"--tls-psk=../tests/keys.psk", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DPSK=1 \
+	-DTLS_MODE=LIBNBD_TLS_ALLOW \
+	$(NULL)
+interop_nbdkit_tls_psk_allow_enabled_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_psk_allow_enabled_LDADD = $(top_builddir)/lib/libnbd.la
+
+interop_nbdkit_tls_psk_allow_fallback_SOURCES = interop.c
+interop_nbdkit_tls_psk_allow_fallback_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-DSERVER=\"$(NBDKIT)\" \
+	-DSERVER_PARAMS='"--tls=off", "-s",
"--exit-with-parent", "file", tmpfile' \
+	-DPSK=1 \
+	-DTLS_MODE=LIBNBD_TLS_ALLOW \
+	-DTLS_FALLBACK=1 \
+	$(NULL)
+interop_nbdkit_tls_psk_allow_fallback_CFLAGS = $(WARNINGS_CFLAGS)
+interop_nbdkit_tls_psk_allow_fallback_LDADD = $(top_builddir)/lib/libnbd.la
+
 endif HAVE_NBDKIT
 
 check-valgrind:
diff --git a/interop/interop.c b/interop/interop.c
index 2772721..3d916f2 100644
--- a/interop/interop.c
+++ b/interop/interop.c
@@ -147,12 +147,30 @@ main (int argc, char *argv[])
 #endif
 
 #if TLS
-  if (TLS_MODE == LIBNBD_TLS_REQUIRE &&
-      nbd_get_tls_negotiated (nbd) != 1) {
-    fprintf (stderr,
-             "%s: TLS required, but not negotiated on the
connection\n",
-             argv[0]);
-    goto out;
+  if (TLS_MODE == LIBNBD_TLS_REQUIRE) {
+    if (nbd_get_tls_negotiated (nbd) != 1) {
+      fprintf (stderr,
+               "%s: TLS required, but not negotiated on the
connection\n",
+               argv[0]);
+      goto out;
+    }
+  }
+  else if (TLS_MODE == LIBNBD_TLS_ALLOW) {
+#if TLS_FALLBACK
+    if (nbd_get_tls_negotiated (nbd) != 0) {
+      fprintf (stderr,
+               "%s: TLS disabled, but connection didn't fall back to
plaintext\n",
+               argv[0]);
+      goto out;
+    }
+#else // !TLS_FALLBACK
+    if (nbd_get_tls_negotiated (nbd) != 1) {
+      fprintf (stderr,
+               "%s: TLS required, but not negotiated on the
connection\n",
+               argv[0]);
+      goto out;
+    }
+#endif
   }
 #endif
 
-- 
2.23.0

On 9/17/19 5:35 PM, Richard W.M. Jones wrote:
> Test both the TLS enabled and fallback paths.
> 
> nbd-server doesn't appear to support TLS at all, and qemu-nbd is known
> not to allow fallback to unencrypted, and therefore it only makes
> sense to test nbdkit at the moment.
> ---
>  .gitignore          |  4 ++++
> +interop_nbdkit_tls_certs_allow_enabled_SOURCES = interop.c
> +interop_nbdkit_tls_certs_allow_enabled_CPPFLAGS = \
> +	-I$(top_srcdir)/include \
> +	-DSERVER=\"$(NBDKIT)\" \
> +	-DSERVER_PARAMS='"--tls=require",
"--tls-certificates=../tests/pki", "-s",
"--exit-with-parent", "file", tmpfile' \
Is it worth testing nbdkit's --tls=yes (the counterpart to libnbd
TLS_ALLOW), to show that a server that permits but does not require
encryption can accept a plaintext client?

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

On 9/17/19 5:35 PM, Richard W.M. Jones wrote:
> This neutral refactoring adds -DTLS_MODE.  We can in future change the
> requested TLS mode, but not in this commit.
> 
> It also checks that nbd_get_tls_negotiated returns true after
> connecting, when the requested mode was set to LIBNBD_TLS_REQUIRE.
> ---
>  interop/Makefile.am |  4 ++++
>  interop/interop.c   | 26 ++++++++++++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> +#if CERTS || PSK
> +#define TLS 1
> +#ifndef TLS_MODE
> +#error "TLS_MODE must be defined when using CERTS || PSK"
> +#endif
> +#endif
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -73,15 +80,12 @@ main (int argc, char *argv[])
>    }
>  #endif
>  
> -#if CERTS || PSK
> -  /* Require TLS on the handle and fail if not available or if the
> -   * handshake fails.
> -   */
> +#if TLS
>    if (nbd_supports_tls (nbd) != 1) {
>      fprintf (stderr, "skip: compiled without TLS support\n");
>      exit (77);
>    }
This skips the test if we are compiled without TLS support, even if
TLS_ALLOW was requested.  What behavior do we really want there?  Is
TLS_ALLOW unconditionally falling back to plaintext okay, or do we only
want to permit TLS_ALLOW if TLS support is at least plausible?

Otherwise, the series is fine.

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