[PATCH nbdkit 0/3] Add tls-* parameters to nbdkit URIs & more
I pushed some preliminary patches for this series already since they were all neutral refactorings. These three patches are the guts of it. They pass tests, I would push them ... but is this secure? I can't imagine a way that it's not secure, but maybe there's something about copying file locations from the nbdkit command line into the --run script which is problematic. Anyway ... The first patch modifies $uri in the --run script to add &tls-certificates=... or &tls-psk-file=... so that TLS connections work (with libnbd anyway, I didn't check qemu). The second patch adds these as variables as well. The third patch updates a test to use this. A final point is that this doesn't work well with PSK. PSK has an implicit username (passed in the URI as nbds://user@...). The same username has to appear in the PSK file. We don't set a username at all right now, so libnbd uses the LOGNAME, which may or may not be what you want. The way to fix this would be to add a --tls-username parameter to nbdkit, but it would only be useful for URIs and might confuse things otherwise (eg. it could be used with --tls-psk but would have no apparent effect, thus confusing users). Rich.
If TLS is required (--tls=require), when either --run or --print-uri is used, include extra query parameters ?tls-certificates=... or ?tls-psk-key=... in the URI. These are simply copied from the corresponding --tls-certificates or --tls-psk parameter on the nbdkit command line without any other modification (we don't make it an absolute path as it's not needed). This will probably only work for libnbd-based clients. This commit as it stands doesn't actually work well for PSK, unless the username in the PSK file happens to coincide with the local login name. Example: $ nbdkit null \ --tls=require --tls-certificates=tests/pki --tls-verify-peer \ --print-uri \ --run 'echo uri="$uri" ; nbdinfo "$uri"' nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki Shell-quoted URI: "nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki" Command to query the NBD endpoint: nbdinfo "nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki" uri=nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki protocol: newstyle-fixed with TLS, using structured packets export="": export-size: 0 content: empty uri: nbds+unix:///?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki contexts: base:allocation is_rotational: false is_read_only: false can_block_status_payload: false can_cache: true can_df: true can_fast_zero: true can_flush: true can_fua: true can_multi_conn: true can_trim: true can_zero: true --- server/uri.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/server/uri.c b/server/uri.c index 0810ee4c06..441034261a 100644 --- a/server/uri.c +++ b/server/uri.c @@ -52,6 +52,7 @@ make_uri (void) char *r = NULL; const bool tls_required = tls == 2; const char *scheme; + bool query_appended; switch (service_mode) { case SERVICE_MODE_SOCKET_ACTIVATION: @@ -97,6 +98,7 @@ make_uri (void) } fprintf (fp, "?socket="); uri_quote (unixsocket, fp); + query_appended = true; break; case SERVICE_MODE_VSOCK: /* 1 = VMADDR_CID_LOCAL */ @@ -109,6 +111,7 @@ make_uri (void) putc ('/', fp); uri_quote (export_name, fp); } + query_appended = false; break; case SERVICE_MODE_TCPIP: fputs ("localhost", fp); @@ -120,6 +123,7 @@ make_uri (void) putc ('/', fp); uri_quote (export_name, fp); } + query_appended = false; break; case SERVICE_MODE_SOCKET_ACTIVATION: @@ -130,6 +134,24 @@ make_uri (void) abort (); } + /* For TLS, append tls-certificates or tls-psk-file. Note that + * tls-certificates requires libnbd >= 1.10 (Sep 2021) and it fails + * strangely with older versions (RHEL 8 is too old). Hopefully + * this will resolve itself over time as people upgrade libnbd. + * qemu probably ignores these parameters. + */ + if (tls_required && (tls_certificates_dir || tls_psk)) { + putc (query_appended ? '&' : '?', fp); + if (tls_certificates_dir) { + fputs ("tls-certificates=", fp); + uri_quote (tls_certificates_dir, fp); + } + else if (tls_psk) { + fputs ("tls-psk-file=", fp); + uri_quote (tls_psk, fp); + } + } + if (close_memstream (fp) == EOF) { perror ("uri: close_memstream"); exit (EXIT_FAILURE); -- 2.44.0
On Tue, May 14, 2024 at 04:28:28PM +0100, Richard W.M. Jones wrote:
If TLS is required (--tls=require), when either --run or --print-uri is used, include extra query parameters ?tls-certificates=... or ?tls-psk-key=... in the URI.
These are simply copied from the corresponding --tls-certificates or --tls-psk parameter on the nbdkit command line without any other modification (we don't make it an absolute path as it's not needed).
This will probably only work for libnbd-based clients.
This commit as it stands doesn't actually work well for PSK, unless the username in the PSK file happens to coincide with the local login name.
Example:
$ nbdkit null \ --tls=require --tls-certificates=tests/pki --tls-verify-peer \ --print-uri \ --run 'echo uri="$uri" ; nbdinfo "$uri"' nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki Shell-quoted URI: "nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki" Command to query the NBD endpoint: nbdinfo "nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki" uri=nbds+unix://?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki protocol: newstyle-fixed with TLS, using structured packets export="": export-size: 0 content: empty uri: nbds+unix:///?socket=/tmp/nbdkitIdKUcE/socket&tls-certificates=tests/pki contexts: base:allocation is_rotational: false is_read_only: false can_block_status_payload: false can_cache: true can_df: true can_fast_zero: true can_flush: true can_fua: true can_multi_conn: true can_trim: true can_zero: true --- server/uri.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/server/uri.c b/server/uri.c index 0810ee4c06..441034261a 100644 --- a/server/uri.c +++ b/server/uri.c @@ -52,6 +52,7 @@ make_uri (void) char *r = NULL; const bool tls_required = tls == 2; const char *scheme; + bool query_appended;
switch (service_mode) { case SERVICE_MODE_SOCKET_ACTIVATION: @@ -97,6 +98,7 @@ make_uri (void) } fprintf (fp, "?socket="); uri_quote (unixsocket, fp); + query_appended = true; break; case SERVICE_MODE_VSOCK: /* 1 = VMADDR_CID_LOCAL */ @@ -109,6 +111,7 @@ make_uri (void) putc ('/', fp); uri_quote (export_name, fp); } + query_appended = false; break; case SERVICE_MODE_TCPIP: fputs ("localhost", fp); @@ -120,6 +123,7 @@ make_uri (void) putc ('/', fp); uri_quote (export_name, fp); } + query_appended = false; break;
case SERVICE_MODE_SOCKET_ACTIVATION: @@ -130,6 +134,24 @@ make_uri (void) abort (); }
+ /* For TLS, append tls-certificates or tls-psk-file. Note that + * tls-certificates requires libnbd >= 1.10 (Sep 2021) and it fails + * strangely with older versions (RHEL 8 is too old). Hopefully + * this will resolve itself over time as people upgrade libnbd. + * qemu probably ignores these parameters. + */
Yes, you can't pass TLS certs via an NBD URI to QEMU. They need to be loaded with QEMU's '-object id=$ID,....' argument and then referenced from the nbd blockdev with 'tls-creds=$ID' I don't believe QEMU complains about unknown URI query parameters, though I might make the argument that it should complain about anything unknown as an aid to dianose user errors. I'm not planning any such change myself though.
+ if (tls_required && (tls_certificates_dir || tls_psk)) { + putc (query_appended ? '&' : '?', fp); + if (tls_certificates_dir) { + fputs ("tls-certificates=", fp); + uri_quote (tls_certificates_dir, fp); + } + else if (tls_psk) { + fputs ("tls-psk-file=", fp); + uri_quote (tls_psk, fp); + } + } + if (close_memstream (fp) == EOF) { perror ("uri: close_memstream"); exit (EXIT_FAILURE); -- 2.44.0 _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-leave@lists.libguestfs.org
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, May 16, 2024 at 11:12:55AM +0100, Daniel P. Berrangé wrote:
I don't believe QEMU complains about unknown URI query parameters, though I might make the argument that it should complain about anything unknown as an aid to dianose user errors. I'm not planning any such change myself though.
Let me actually look at what qemu does ... qemu has partial support for NBD URIs (https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md) It supports nbd://, nbd+unix://, and nbd+tcp:// (non-standard). It supports server, port, export name, and ?socket. It doesn't support TLS username or any tls-* parameters, but since it doesn't support nbds:// (ie. TLS) via URIs that doesn't matter at the moment. tls-certificates and tls-psk-file are strictly libnbd extensions, and libnbd doesn't support tls-type=anon|x509|psk. So there's a bit of work for both libnbd & qemu to get fully compliant TLS support. Using the separate -object argument is painful and obscure for users. Is there a reason why qemu could never support tls-certificates=<path> or tls-psk-file=<filename> in future? For example that it's architecturally difficult for qemu to work this way? Thanks for the review. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Thu, May 16, 2024 at 11:28:06AM +0100, Richard W.M. Jones wrote:
So there's a bit of work for both libnbd & qemu to get fully compliant TLS support.
I mean TLS URI support, not TLS in general. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Thu, May 16, 2024 at 11:28:06AM +0100, Richard W.M. Jones wrote:
On Thu, May 16, 2024 at 11:12:55AM +0100, Daniel P. Berrangé wrote:
I don't believe QEMU complains about unknown URI query parameters, though I might make the argument that it should complain about anything unknown as an aid to dianose user errors. I'm not planning any such change myself though.
Let me actually look at what qemu does ...
qemu has partial support for NBD URIs (https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md)
It supports nbd://, nbd+unix://, and nbd+tcp:// (non-standard).
It supports server, port, export name, and ?socket.
It doesn't support TLS username or any tls-* parameters, but since it doesn't support nbds:// (ie. TLS) via URIs that doesn't matter at the moment.
tls-certificates and tls-psk-file are strictly libnbd extensions, and libnbd doesn't support tls-type=anon|x509|psk.
So there's a bit of work for both libnbd & qemu to get fully compliant TLS support.
Using the separate -object argument is painful and obscure for users. Is there a reason why qemu could never support tls-certificates=<path> or tls-psk-file=<filename> in future? For example that it's architecturally difficult for qemu to work this way?
There are a bunch of TLS parameters beyond merely the path, and many places which use TLS in QEMU. By separating out the specification of TLS credentials, from the individual consumers, we avoid having to repeat the addition of many different TLS config parameters across many different locations in the code, and thus ensure a consistent way to configure TLS that "just works" everywhere in QEMU. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--- docs/nbdkit-captive.pod | 24 +++++++--- tests/Makefile.am | 4 ++ server/captive.c | 16 +++++-- tests/test-captive-tls-certificates.sh | 64 ++++++++++++++++++++++++++ tests/test-captive-tls-psk.sh | 63 +++++++++++++++++++++++++ 5 files changed, 162 insertions(+), 9 deletions(-) diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod index f38863bfd7..d5c40d91b9 100644 --- a/docs/nbdkit-captive.pod +++ b/docs/nbdkit-captive.pod @@ -77,12 +77,6 @@ both variables expand to the same URI. See also L<nbdkit(1)/NBD URIs and endpoints>. -=item C<$tls> - -Corresponds to the I<--tls> option passed to nbdkit. If I<--tls=off> -this is not set. If I<--tls=on> this is set to C<"1">. If -I<--tls=require> this is set to C<"2">. - =item C<$port> If E<ne> "", the port number that nbdkit is listening on. @@ -99,6 +93,24 @@ line option of nbdkit. This only matters to plugins that differentiate what they serve based on the export name requested by the client. +=item C<$tls> + +Corresponds to the I<--tls> option passed to nbdkit. If I<--tls=off> +this is not set. If I<--tls=on> this is set to C<"1">. If +I<--tls=require> this is set to C<"2">. + +=item C<$tls_certificates> + +If I<--tls-certificates> was passed to nbdkit, the value is copied +here. It is usually the directory containing PKI certificates. Note +that the path might not be an absolute path, or even valid. + +=item C<$tls_psk> + +If I<--tls-psk> was passed to nbdkit, the value is copied here. It is +usually the filename of a TLS Pre-Shared Keys (PSK) file. Note that +the filename might not be an absolute path, or even valid. + =back I<--run> implies I<--foreground>. It is not possible, and probably diff --git a/tests/Makefile.am b/tests/Makefile.am index 08473ee801..67732f8303 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -258,6 +258,8 @@ TESTS += \ test-single-sh.sh \ test-captive.sh \ test-captive-tls.sh \ + test-captive-tls-certificates.sh \ + test-captive-tls-psk.sh \ test-random-sock.sh \ test-tls.sh \ test-tls-psk.sh \ @@ -289,6 +291,8 @@ endif EXTRA_DIST += \ test-bad-filter-name.sh \ test-bad-plugin-name.sh \ + test-captive-tls-certificates.sh \ + test-captive-tls-psk.sh \ test-captive-tls.sh \ test-captive.sh \ test-client-death-tls.sh \ diff --git a/server/captive.c b/server/captive.c index 51dafca34a..51d92b3297 100644 --- a/server/captive.c +++ b/server/captive.c @@ -88,9 +88,7 @@ run_command (void) shell_quote (export_name, fp); putc ('\n', fp); - /* Construct $tls, $port and $unixsocket. */ - if (tls > 0) - fprintf (fp, "tls=%d\n", tls); + /* Construct $port and $unixsocket. */ fprintf (fp, "port="); if (port) shell_quote (port, fp); @@ -100,6 +98,18 @@ run_command (void) shell_quote (unixsocket, fp); fprintf (fp, "\n"); + /* Construct TLS-related variables. */ + if (tls > 0) + fprintf (fp, "tls=%d\n", tls); + fprintf (fp, "tls_certificates="); + if (tls_certificates_dir) + shell_quote (tls_certificates_dir, fp); + fprintf (fp, "\n"); + fprintf (fp, "tls_psk="); + if (tls_psk) + shell_quote (tls_psk, fp); + fprintf (fp, "\n"); + /* Add the --run command. Note we don't have to quote this. */ fprintf (fp, "%s", run); diff --git a/tests/test-captive-tls-certificates.sh b/tests/test-captive-tls-certificates.sh new file mode 100755 index 0000000000..4700c9f6ef --- /dev/null +++ b/tests/test-captive-tls-certificates.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Test that nbdkit --tls-certificates is passed to --run script. + +source ./functions.sh +set -e +set -x + +requires_run + +# Does the nbdkit binary support TLS? +if ! nbdkit --dump-config | grep -sq tls=yes; then + echo "$0: nbdkit built without TLS support" + exit 77 +fi + +# Did we create the PKI files? +# Probably 'certtool' is missing. +pkidir="pki" +if [ ! -f "$pkidir/ca-cert.pem" ]; then + echo "$0: PKI files were not created by the test harness" + exit 77 +fi + +out=test-captive-tls-certificates.out +cleanup_fn rm -f $out +rm -f $out + +LANG=C \ +nbdkit --tls=require --tls-certificates="$pkidir" \ + null \ + --run 'echo OUTPUT: "$tls_certificates"' > $out +cat $out +grep "OUTPUT: $pkidir" $out diff --git a/tests/test-captive-tls-psk.sh b/tests/test-captive-tls-psk.sh new file mode 100755 index 0000000000..d225dbc9ff --- /dev/null +++ b/tests/test-captive-tls-psk.sh @@ -0,0 +1,63 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Test that nbdkit --tls-psk is passed to --run script. + +source ./functions.sh +set -e +set -x + +requires_run + +# Does the nbdkit binary support TLS? +if ! nbdkit --dump-config | grep -sq tls=yes; then + echo "$0: nbdkit built without TLS support" + exit 77 +fi + +# Did we create the PSK keys file? +# Probably 'psktool' is missing. +if [ ! -s keys.psk ]; then + echo "$0: PSK keys file was not created by the test harness" + exit 77 +fi + +out=test-captive-tls-psk.out +cleanup_fn rm -f $out +rm -f $out + +LANG=C \ +nbdkit --tls=require --tls-psk=keys.psk \ + null \ + --run 'echo OUTPUT: "$tls_psk"' > $out +cat $out +grep "OUTPUT: keys.psk" $out -- 2.44.0
As a result of previous commits, we can now modify this test to use "$uri", since it will include the tls-certificates parameter. We cannot update tests/test-tls-psk.sh in the same way as we still don't set the username in the generated URI. --- tests/test-tls.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-tls.sh b/tests/test-tls.sh index 4abe1f36c9..d70fe0c97c 100755 --- a/tests/test-tls.sh +++ b/tests/test-tls.sh @@ -70,7 +70,7 @@ nbdkit --tls=require --tls-certificates="$pkidir" --tls-verify-peer \ example1 \ --run ' # Run nbdinfo against the server. - nbdinfo "nbds+unix://qemu@/?socket=$unixsocket&tls-certificates=$pkidir" + nbdinfo "$uri" ' > $out cat $out -- 2.44.0
participants (2)
-
Daniel P. Berrangé -
Richard W.M. Jones