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

[Richard W.M. Jones]

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