Libguestfs - Jun 2018 - Re: [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