Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [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.
Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [PATCH libnbd 1/5] interop: Don't build various check_PROGRAMS unless we run those tests.
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
Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [PATCH libnbd 2/5] interop: Allow -DEXPORT_NAME to be defined optionally.
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
Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [PATCH libnbd 3/5] interop: Add interop tests for nbdkit.
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
Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [PATCH libnbd 4/5] interop: Add -DTLS_MODE to the test.
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
Richard W.M. Jones
2019-Sep-17 22:35 UTC
[Libguestfs] [PATCH libnbd 5/5] interop: Add tests of nbdkit + LIBNBD_TLS_ALLOW.
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
Eric Blake
2019-Sep-18 12:41 UTC
Re: [Libguestfs] [PATCH libnbd 5/5] interop: Add tests of nbdkit + LIBNBD_TLS_ALLOW.
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
Eric Blake
2019-Sep-18 12:41 UTC
Re: [Libguestfs] [PATCH libnbd 4/5] interop: Add -DTLS_MODE to the test.
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
Reasonably Related Threads
- [PATCH libnbd 5/5] interop: Add tests of nbdkit + LIBNBD_TLS_ALLOW.
- [PATCH libnbd 0/2] Change qemu-nbd interop tests to use socket activation.
- [PATCH libnbd 4/5] interop: Add -DTLS_MODE to the test.
- Re: [PATCH libnbd 5/5] interop: Add tests of nbdkit + LIBNBD_TLS_ALLOW.
- [PATCH libnbd] interop: Add test of qemu-storage-daemon.