[PATCH nbdkit] tls: Implement Pre-Shared Keys (PSK) authentication.
This is ready for review but needs a bit more real-world testing before I'd be happy about it going upstream. It also needs tests. It does interoperate with qemu, at least in my limited tests. Rich.
--- docs/nbdkit.pod.in | 45 +++++++++-- src/crypto.c | 234 +++++++++++++++++++++++++++++++++++++---------------- src/internal.h | 1 + src/main.c | 8 +- 4 files changed, 210 insertions(+), 78 deletions(-) diff --git a/docs/nbdkit.pod.in b/docs/nbdkit.pod.in index 42e6e6b..80d1ecd 100644 --- a/docs/nbdkit.pod.in +++ b/docs/nbdkit.pod.in @@ -11,7 +11,7 @@ nbdkit - A toolkit for creating NBD servers [--newstyle] [--oldstyle] [-P PIDFILE] [-p PORT] [-r] [--run CMD] [-s] [--selinux-label LABEL] [-t THREADS] [--tls=off|on|require] [--tls-certificates /path/to/certificates] - [--tls-verify-peer] + [--tls-psk /path/to/pskfile] [--tls-verify-peer] [-U SOCKET] [-u USER] [-v] [-V] PLUGIN [key=value [key=value [...]]] @@ -288,6 +288,12 @@ support). See L</TLS> below. Set the path to the TLS certificates directory. If not specified, some built-in paths are checked. See L</TLS> below for more details. +=item B<--tls-psk> /path/to/pskfile + +Set the path to the pre-shared keys (PSK) file. If used, this +overrides certificate authentication. There is no built-in path. See +L</TLS> below for more details. + =item B<--tls-verify-peer> Enables TLS client certificate verification. The default is I<not> to @@ -757,6 +763,29 @@ denied. Also denied are clients which present a valid certificate signed by another CA. Also denied are clients with certificates added to the certificate revocation list (F<ca-crl.pem>). +=head2 TLS with Pre-Shared Keys (PSK) + +As a simpler alternative to TLS certificates, you may used pre-shared +keys to authenticate clients. Currently PSK support in NBD clients is +not widespread. + +Create a PSK file containing one or more C<username:key> pairs. It is +easiest to use L<psktool(1)> for this: + + psktool -u rich -p /tmp/psk + +The PSK file contains the hex-encoded random keys in plaintext. Any +client which can read this file will be able to connect to the server. + +Use the nbdkit I<--tls-psk> option to start the server: + + nbdkit --tls-psk=/tmp/psk file file=disk.img + +This option overrides X.509 certificate authentication. + +Clients must supply one of the usernames in the PSK file and the +corresponding key in order to connect. + =head2 Default TLS behaviour If nbdkit was compiled without GnuTLS support, then TLS is disabled @@ -765,11 +794,14 @@ on the command line). Also it is impossible to turn on TLS in this scenario. You can tell if nbdkit was compiled without GnuTLS support because C<nbdkit --dump-config> will contain C<tls=no>. -If TLS certificates cannot be loaded either from the built-in path or -from the directory specified by I<--tls-certificates>, then TLS -defaults to disabled. Turning TLS on will give a warning -(I<--tls=on>) or error (I<--tls=require>) about the missing -certificates. +If the I<--tls-psk> option is used then TLS is enabled with pre-shared +keys. + +If the I<--tls-psk> option is I<not> used: If TLS certificates cannot +be loaded either from the built-in path or from the directory +specified by I<--tls-certificates>, then TLS defaults to disabled. +Turning TLS on will give a warning (I<--tls=on>) or error +(I<--tls=require>) about the missing certificates. If TLS certificates can be loaded from the built-in path or from the I<--tls-certificates> directory, then TLS will by default be enabled @@ -968,6 +1000,7 @@ L<https://en.wikipedia.org/wiki/Fibre_Channel_over_Ethernet>. L<gnutls_priority_init(3)>, L<qemu-img(1)>, +L<psktool(1)>, L<systemd.socket(5)>. =head1 AUTHORS diff --git a/src/crypto.c b/src/crypto.c index 23c5c8f..6af3977 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -37,10 +37,12 @@ #include <stdlib.h> #include <stdarg.h> #include <stdint.h> +#include <stdbool.h> #include <inttypes.h> #include <string.h> #include <unistd.h> #include <fcntl.h> +#include <limits.h> #include <errno.h> #include <sys/types.h> #include <assert.h> @@ -51,7 +53,12 @@ #include <gnutls/gnutls.h> +static int crypto_auth = 0; +#define CRYPTO_AUTH_CERTIFICATES 1 +#define CRYPTO_AUTH_PSK 2 + static gnutls_certificate_credentials_t x509_creds; +static gnutls_psk_server_credentials_t psk_creds; static void print_gnutls_error (int err, const char *fs, ...) __attribute__((format (printf, 2, 3))); @@ -147,23 +154,9 @@ load_certificates (const char *path) return 1; } -/* Initialize crypto. This also handles the command line parameters - * and loading the server certificate. - */ -void -crypto_init (int tls_set_on_cli) +static int +start_certificates (void) { - int err; - - err = gnutls_global_init (); - if (err < 0) { - print_gnutls_error (err, "initializing GnuTLS"); - exit (EXIT_FAILURE); - } - - if (tls == 0) /* --tls=off */ - return; - /* Try to locate the certificates directory and load them. */ if (tls_certificates_dir == NULL) { const char *home; @@ -196,49 +189,119 @@ crypto_init (int tls_set_on_cli) if (load_certificates (tls_certificates_dir)) goto found_certificates; } - - /* If we get here, we didn't manage to load the certificates. If - * --tls=require was given on the command line then that's a - * problem. - */ - if (tls == 2) { /* --tls=require */ - fprintf (stderr, - "%s: --tls=require but could not load TLS certificates.\n" - "Try setting ‘--tls-certificates=/path/to/certificates’ or read\n" - "the \"TLS\" section in nbdkit(1).\n", - program_name); - exit (EXIT_FAILURE); - } - - /* If --tls=on was given on the command line, warn before we turn - * TLS off. - */ - if (tls == 1 && tls_set_on_cli) { /* explicit --tls=on */ - fprintf (stderr, - "%s: warning: --tls=on but could not load TLS certificates.\n" - "TLS will be disabled and TLS connections will be rejected.\n" - "Try setting ‘--tls-certificates=/path/to/certificates’ or read\n" - "the \"TLS\" section in nbdkit(1).\n", - program_name); - } - - tls = 0; - debug ("TLS disabled: could not load TLS certificates"); - return; + return -1; found_certificates: #ifdef HAVE_GNUTLS_CERTIFICATE_SET_KNOWN_DH_PARAMS gnutls_certificate_set_known_dh_params (x509_creds, GNUTLS_SEC_PARAM_MEDIUM); #endif + return 0; +} - debug ("TLS enabled"); +static int +start_psk (void) +{ + int err; + CLEANUP_FREE char *abs_psk_file = NULL; + + /* Make sure the path to the PSK file is absolute. */ + abs_psk_file = realpath (tls_psk, NULL); + if (abs_psk_file == NULL) { + perror (tls_psk); + exit (EXIT_FAILURE); + } + + err = gnutls_psk_allocate_server_credentials (&psk_creds); + if (err < 0) { + print_gnutls_error (err, "allocating PSK credentials"); + exit (EXIT_FAILURE); + } + + /* Note that this function makes a copy of the string so it + * is safe to free it afterwards. + */ + gnutls_psk_set_server_credentials_file (psk_creds, abs_psk_file); + + return 0; +} + +/* Initialize crypto. This also handles the command line parameters + * and loading the server certificate. + */ +void +crypto_init (int tls_set_on_cli) +{ + int err, r; + const char *what; + + err = gnutls_global_init (); + if (err < 0) { + print_gnutls_error (err, "initializing GnuTLS"); + exit (EXIT_FAILURE); + } + + if (tls == 0) /* --tls=off */ + return; + + /* --tls-psk overrides certificates. */ + if (tls_psk != NULL) { + r = start_psk (); + what = "Pre-Shared Keys (PSK)"; + if (r == 0) crypto_auth = CRYPTO_AUTH_PSK; + } + else { + r = start_certificates (); + what = "X.509 certificates"; + if (r == 0) crypto_auth = CRYPTO_AUTH_CERTIFICATES; + } + + if (r == 0) { + debug ("TLS enabled using: %s", what); + } + else { + /* If we get here, we didn't manage to load the PSK file / + * certificates. If --tls=require was given on the command line + * then that's a problem. + */ + if (tls == 2) { /* --tls=require */ + fprintf (stderr, + "%s: --tls=require but could not load TLS certificates.\n" + "Try setting ‘--tls-certificates=/path/to/certificates’ or read\n" + "the \"TLS\" section in nbdkit(1).\n", + program_name); + exit (EXIT_FAILURE); + } + + /* If --tls=on was given on the command line, warn before we turn + * TLS off. + */ + if (tls == 1 && tls_set_on_cli) { /* explicit --tls=on */ + fprintf (stderr, + "%s: warning: --tls=on but could not load TLS certificates.\n" + "TLS will be disabled and TLS connections will be rejected.\n" + "Try setting ‘--tls-certificates=/path/to/certificates’ or read\n" + "the \"TLS\" section in nbdkit(1).\n", + program_name); + } + + tls = 0; + debug ("TLS disabled: could not load TLS certificates"); + } } void crypto_free (void) { - if (tls > 0) - gnutls_certificate_free_credentials (x509_creds); + if (tls > 0) { + switch (crypto_auth) { + case CRYPTO_AUTH_CERTIFICATES: + gnutls_certificate_free_credentials (x509_creds); + break; + case CRYPTO_AUTH_PSK: + gnutls_psk_free_server_credentials (psk_creds); + break; + } + } gnutls_global_deinit (); } @@ -335,6 +398,7 @@ int crypto_negotiate_tls (struct connection *conn, int sockin, int sockout) { gnutls_session_t *session; + CLEANUP_FREE char *priority = NULL; int err; /* Create the GnuTLS session. */ @@ -351,33 +415,61 @@ crypto_negotiate_tls (struct connection *conn, int sockin, int sockout) return -1; } - err = gnutls_priority_set_direct (*session, TLS_PRIORITY, NULL); - if (err < 0) { - nbdkit_error ("failed to set TLS session priority to %s: %s", - TLS_PRIORITY, gnutls_strerror (err)); - goto error; - } + switch (crypto_auth) { + case CRYPTO_AUTH_CERTIFICATES: + /* Associate the session with the server credentials (key, cert). */ + err = gnutls_credentials_set (*session, GNUTLS_CRD_CERTIFICATE, + x509_creds); + if (err < 0) { + nbdkit_error ("gnutls_credentials_set: %s", gnutls_strerror (err)); + goto error; + } - /* Associate the session with the server credentials (key, cert). */ - err = gnutls_credentials_set (*session, GNUTLS_CRD_CERTIFICATE, - x509_creds); - if (err < 0) { - nbdkit_error ("gnutls_credentials_set: %s", gnutls_strerror (err)); - goto error; - } - - /* If verify peer is enabled, tell GnuTLS to request the client - * certificates. (Note the default is to not request or verify - * certificates). - */ - if (tls_verify_peer) { + /* If verify peer is enabled, tell GnuTLS to request the client + * certificates. (Note the default is to not request or verify + * certificates). + */ + if (tls_verify_peer) { #ifdef HAVE_GNUTLS_SESSION_SET_VERIFY_CERT - gnutls_certificate_server_set_request (*session, GNUTLS_CERT_REQUEST); - gnutls_session_set_verify_cert (*session, NULL, 0); + gnutls_certificate_server_set_request (*session, GNUTLS_CERT_REQUEST); + gnutls_session_set_verify_cert (*session, NULL, 0); #else - nbdkit_error ("--tls-verify-peer: GnuTLS >= 3.4.6 is required for this feature"); - goto error; + nbdkit_error ("--tls-verify-peer: GnuTLS >= 3.4.6 is required for this feature"); + goto error; #endif + } + + priority = strdup (TLS_PRIORITY); + if (priority == NULL) { + nbdkit_error ("strdup: %m"); + goto error; + } + break; + + case CRYPTO_AUTH_PSK: + /* Associate the session with the server PSK credentials. */ + err = gnutls_credentials_set (*session, GNUTLS_CRD_PSK, psk_creds); + if (err < 0) { + nbdkit_error ("gnutls_credentials_set: %s", gnutls_strerror (err)); + goto error; + } + + if (asprintf (&priority, "%s:+PSK:+DHE-PSK", TLS_PRIORITY) == -1) { + nbdkit_error ("asprintf: %m"); + goto error; + } + break; + + default: + abort (); + } + + assert (priority != NULL); + err = gnutls_priority_set_direct (*session, priority, NULL); + if (err < 0) { + nbdkit_error ("failed to set TLS session priority to %s: %s", + priority, gnutls_strerror (err)); + goto error; } /* Set up GnuTLS so it reads and writes on the raw sockets, and set diff --git a/src/internal.h b/src/internal.h index ea3155c..ec19841 100644 --- a/src/internal.h +++ b/src/internal.h @@ -108,6 +108,7 @@ extern int readonly; extern const char *selinux_label; extern int tls; extern const char *tls_certificates_dir; +extern const char *tls_psk; extern int tls_verify_peer; extern char *unixsocket; extern int verbose; diff --git a/src/main.c b/src/main.c index 8d901cf..6524b85 100644 --- a/src/main.c +++ b/src/main.c @@ -89,6 +89,7 @@ const char *selinux_label; /* --selinux-label */ int threads; /* -t */ int tls; /* --tls : 0=off 1=on 2=require */ const char *tls_certificates_dir; /* --tls-certificates */ +const char *tls_psk; /* --tls-psk */ int tls_verify_peer; /* --tls-verify-peer */ char *unixsocket; /* -U */ const char *user, *group; /* -u & -g */ @@ -144,6 +145,7 @@ static const struct option long_options[] = { { "threads", 1, NULL, 't' }, { "tls", 1, NULL, 0 }, { "tls-certificates", 1, NULL, 0 }, + { "tls-psk", 1, NULL, 0 }, { "tls-verify-peer", 0, NULL, 0 }, { "unix", 1, NULL, 'U' }, { "user", 1, NULL, 'u' }, @@ -161,7 +163,7 @@ usage (void) " [--newstyle] [--oldstyle] [-P PIDFILE] [-p PORT] [-r]\n" " [--run CMD] [-s] [--selinux-label LABEL] [-t THREADS]\n" " [--tls=off|on|require] [--tls-certificates /path/to/certificates]\n" - " [--tls-verify-peer]\n" + " [--tls-psk /path/to/pskfile] [--tls-verify-peer]\n" " [-U SOCKET] [-u USER] [-v] [-V]\n" " PLUGIN [key=value [key=value [...]]]\n" "\n" @@ -314,6 +316,10 @@ main (int argc, char *argv[]) tls_certificates_dir = optarg; break; } + else if (strcmp (long_options[option_index].name, "tls-psk") == 0) { + tls_psk = optarg; + break; + } else if (strcmp (long_options[option_index].name, "tls-verify-peer") == 0) { tls_verify_peer = 1; break; -- 2.16.2
On 06/25/2018 12:01 PM, Richard W.M. Jones wrote:
--- docs/nbdkit.pod.in | 45 +++++++++-- src/crypto.c | 234 +++++++++++++++++++++++++++++++++++++---------------- src/internal.h | 1 + src/main.c | 8 +- 4 files changed, 210 insertions(+), 78 deletions(-)
+Create a PSK file containing one or more C<username:key> pairs. It is +easiest to use L<psktool(1)> for this: + + psktool -u rich -p /tmp/psk + +The PSK file contains the hex-encoded random keys in plaintext. Any +client which can read this file will be able to connect to the server.
If I'm understanding correctly, it's also possible for a server to create a file that supports multiple users: c1:1234 c2:2345 but then hand out a smaller file to client c1 that contains only the c2:1234 line, and another small file to client c2, and then a single server can not only accept multiple clients but also distinguish which client connected. Is there any reason for nbdkit as a server to additionally limit which resources are served depending on which client connected? But that's probably overkill for now; as it's just as easy to point nbdkit to the smaller file in the first place, and since right now nbdkit serves up at most one file, provided the client can connect in the first place (it would matter more for something like nbd-server, which has an init file that specifies multiple resources served by a single server, and therefore might want per-user restrictions on those resources).
+++ b/src/crypto.c @@ -37,10 +37,12 @@ #include <stdlib.h> #include <stdarg.h> #include <stdint.h> +#include <stdbool.h> #include <inttypes.h> #include <string.h> #include <unistd.h> #include <fcntl.h> +#include <limits.h> #include <errno.h> #include <sys/types.h> #include <assert.h> @@ -51,7 +53,12 @@
#include <gnutls/gnutls.h>
+static int crypto_auth = 0;
Explicit assignment to 0 is not necessary for static variables (and in general, the assignment moves the variable from .bss into .data for a larger binary, although gcc has options for changing this).
+static int +start_psk (void) +{ + int err; + CLEANUP_FREE char *abs_psk_file = NULL; + + /* Make sure the path to the PSK file is absolute. */ + abs_psk_file = realpath (tls_psk, NULL); + if (abs_psk_file == NULL) { + perror (tls_psk); + exit (EXIT_FAILURE); + } + + err = gnutls_psk_allocate_server_credentials (&psk_creds); + if (err < 0) { + print_gnutls_error (err, "allocating PSK credentials"); + exit (EXIT_FAILURE);
Do you care about lint-like efforts to free abs_psk_file before exit? (I don't)
+ } + + /* Note that this function makes a copy of the string so it + * is safe to free it afterwards. + */ + gnutls_psk_set_server_credentials_file (psk_creds, abs_psk_file); + + return 0;
Based on the comment, isn't this a leak of abs_psk_file?
+} + +/* Initialize crypto. This also handles the command line parameters + * and loading the server certificate. + */ +void +crypto_init (int tls_set_on_cli) +{ + int err, r; + const char *what; + + err = gnutls_global_init (); + if (err < 0) { + print_gnutls_error (err, "initializing GnuTLS"); + exit (EXIT_FAILURE); + } + + if (tls == 0) /* --tls=off */ + return; + + /* --tls-psk overrides certificates. */ + if (tls_psk != NULL) { + r = start_psk (); + what = "Pre-Shared Keys (PSK)"; + if (r == 0) crypto_auth = CRYPTO_AUTH_PSK;
Isn't it more typical to put the 'if' and statement on separate lines?
+ } + else { + r = start_certificates (); + what = "X.509 certificates"; + if (r == 0) crypto_auth = CRYPTO_AUTH_CERTIFICATES;
but at least you're doing the one-liner layout consistently.
+++ b/src/main.c @@ -144,6 +145,7 @@ static const struct option long_options[] = { { "threads", 1, NULL, 't' }, { "tls", 1, NULL, 0 }, { "tls-certificates", 1, NULL, 0 }, + { "tls-psk", 1, NULL, 0 },
Pre-existing (so separate cleanup if desired), but we should probably use the symbolic names 'no_argument', 'required_argument', 'optional_argument' instead of 0, 1, 2.
{ "tls-verify-peer", 0, NULL, 0 }, { "unix", 1, NULL, 'U' }, { "user", 1, NULL, 'u' }, @@ -161,7 +163,7 @@ usage (void) " [--newstyle] [--oldstyle] [-P PIDFILE] [-p PORT] [-r]\n" " [--run CMD] [-s] [--selinux-label LABEL] [-t THREADS]\n" " [--tls=off|on|require] [--tls-certificates /path/to/certificates]\n" - " [--tls-verify-peer]\n" + " [--tls-psk /path/to/pskfile] [--tls-verify-peer]\n" " [-U SOCKET] [-u USER] [-v] [-V]\n" " PLUGIN [key=value [key=value [...]]]\n" "\n" @@ -314,6 +316,10 @@ main (int argc, char *argv[]) tls_certificates_dir = optarg; break; } + else if (strcmp (long_options[option_index].name, "tls-psk") == 0) {
Pre-existing, but if you assign values larger than 256 to the 'val' member in long_options above, then you can directly switch() to these cases rather than having an ever-growing 'if' branching chain for long-only options. (Hmm, since I'm pointing out pre-existing getopt_long() usage idioms, I should probably post that cleanup patch). Overall it looks good. The best part is that you've done interoperability testing with your pending qemu patch (I have not yet tried to repeat the interoperability testing at this point, though). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Jun 28, 2018 at 10:18:25AM -0500, Eric Blake wrote:
On 06/25/2018 12:01 PM, Richard W.M. Jones wrote:
--- docs/nbdkit.pod.in | 45 +++++++++-- src/crypto.c | 234 +++++++++++++++++++++++++++++++++++++---------------- src/internal.h | 1 + src/main.c | 8 +- 4 files changed, 210 insertions(+), 78 deletions(-)
+Create a PSK file containing one or more C<username:key> pairs. It is +easiest to use L<psktool(1)> for this: + + psktool -u rich -p /tmp/psk + +The PSK file contains the hex-encoded random keys in plaintext. Any +client which can read this file will be able to connect to the server.
If I'm understanding correctly, it's also possible for a server to create a file that supports multiple users:
c1:1234 c2:2345
but then hand out a smaller file to client c1 that contains only the c2:1234 line, and another small file to client c2, and then a single server can not only accept multiple clients but also distinguish which client connected. Is there any reason for nbdkit as a server to additionally limit which resources are served depending on which client connected?
But that's probably overkill for now; as it's just as easy to point nbdkit to the smaller file in the first place, and since right now nbdkit serves up at most one file, provided the client can connect in the first place (it would matter more for something like nbd-server, which has an init file that specifies multiple resources served by a single server, and therefore might want per-user restrictions on those resources).
I could see a situation where a host has a single file of keys + users used for all NBD servers, and then whitelists specific user names per instance. Not a blocker, but conceptually relevant. When I add the authorization work to QEMU I'd intend to use that to authorize the client username. 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, Jun 28, 2018 at 6:56 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On 06/25/2018 12:01 PM, Richard W.M. Jones wrote:
--- docs/nbdkit.pod.in | 45 +++++++++-- src/crypto.c | 234 +++++++++++++++++++++++++++++++++++++---------------- src/internal.h | 1 + src/main.c | 8 +- 4 files changed, 210 insertions(+), 78 deletions(-)
+Create a PSK file containing one or more C<username:key> pairs. It is +easiest to use L<psktool(1)> for this: + + psktool -u rich -p /tmp/psk + +The PSK file contains the hex-encoded random keys in plaintext. Any +client which can read this file will be able to connect to the server.
If I'm understanding correctly, it's also possible for a server to create a file that supports multiple users:
c1:1234 c2:2345
but then hand out a smaller file to client c1 that contains only the c2:1234 line, and another small file to client c2, and then a single server can not only accept multiple clients but also distinguish which client connected. Is there any reason for nbdkit as a server to additionally limit which resources are served depending on which client connected?
But that's probably overkill for now; as it's just as easy to point nbdkit to the smaller file in the first place, and since right now nbdkit serves up at most one file, provided the client can connect in the first place (it would matter more for something like nbd-server, which has an init file
On Thu, Jun 28, 2018 at 10:18:25AM -0500, Eric Blake wrote: that
specifies multiple resources served by a single server, and therefore might want per-user restrictions on those resources).
I could see a situation where a host has a single file of keys + users used for all NBD servers, and then whitelists specific user names per instance. Not a blocker, but conceptually relevant. When I add the authorization work to QEMU I'd intend to use that to authorize the client username.
I don't think we should make it easy to have a static files with many keys and user names. Shared key should be used exactly once, for single operation. This means that you cannot loose the key and you don't need to manage it. It would be best if we could pass the key to without writing it to actual file so we don't have to clean it up later. Nir
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 :|
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Fri, Jun 29, 2018 at 12:55:16AM +0300, Nir Soffer wrote:
I don't think we should make it easy to have a static files with many keys and user names. Shared key should be used exactly once, for single operation. This means that you cannot loose the key and you don't need to manage it.
It would be best if we could pass the key to without writing it to actual file so we don't have to clean it up later.
This is true, but it's difficult to pass the key securely to the server except through a temporary file or a pipe. Note that --tls-psk as proposed allows both (using a bit of bash trickery): nbdkit --tls-psk=/tmp/keys.psk nbdkit --tls-psk=<( my-secure-key-generating-program ) 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, Jun 28, 2018 at 10:18:25AM -0500, Eric Blake wrote:
On 06/25/2018 12:01 PM, Richard W.M. Jones wrote:
--- docs/nbdkit.pod.in | 45 +++++++++-- src/crypto.c | 234 +++++++++++++++++++++++++++++++++++++---------------- src/internal.h | 1 + src/main.c | 8 +- 4 files changed, 210 insertions(+), 78 deletions(-)
+Create a PSK file containing one or more C<username:key> pairs. It is +easiest to use L<psktool(1)> for this: + + psktool -u rich -p /tmp/psk + +The PSK file contains the hex-encoded random keys in plaintext. Any +client which can read this file will be able to connect to the server.
If I'm understanding correctly, it's also possible for a server to create a file that supports multiple users:
c1:1234 c2:2345
but then hand out a smaller file to client c1 that contains only the c2:1234 line, and another small file to client c2, and then a single server can not only accept multiple clients but also distinguish which client connected.
Correct. To actually distinguish which client is connected you can call gnutls_psk_server_get_username(3) in the server to return the user who connected, although I believe this only works after a successful handshake has completed (so no good for debugging failed connections for example).
Is there any reason for nbdkit as a server to additionally limit which resources are served depending on which client connected?
It could be done (maybe passed to the plugin?) At the moment nbdkit doesn't distinguish based on client, eg. with client certificate CN or IP address.
+ err = gnutls_psk_allocate_server_credentials (&psk_creds); + if (err < 0) { + print_gnutls_error (err, "allocating PSK credentials"); + exit (EXIT_FAILURE);
Do you care about lint-like efforts to free abs_psk_file before exit? (I don't)
Not on abort/exit paths. If it was returning then definitely.
+ } + + /* Note that this function makes a copy of the string so it + * is safe to free it afterwards. + */ + gnutls_psk_set_server_credentials_file (psk_creds, abs_psk_file); + + return 0;
Based on the comment, isn't this a leak of abs_psk_file?
It's marked with CLEANUP_FREE :-) The comment is a bit confusing though, so I'll try to reword it.
Overall it looks good. The best part is that you've done interoperability testing with your pending qemu patch (I have not yet tried to repeat the interoperability testing at this point, though).
That's actually very simple. In the nbdkit top build directory, and assuming you've patched and compiled qemu in ~/d/qemu: $ make -C tests check PATH=~/d/qemu:$PATH TESTS=test-tls-psk.sh Thanks for the rest of the review, I'll update the patch based on your comments. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (4)
-
Daniel P. Berrangé -
Eric Blake -
Nir Soffer -
Richard W.M. Jones