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

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 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

On Thu, Jun 28, 2018 at 6:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> 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.

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
>