[Libguestfs] [PATCH nbdkit] tls: Implement Pre-Shared Keys (PSK) authentication.

[Eric Blake]

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