Re: [Libguestfs] [PATCH nbdkit] tls: Implement Pre-Shared Keys (PSK) authentication.
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