Re: [Nbd] [PATCH/RFC 0/3] Introduce TLS on nbdserver

[Wouter Verhelst <w@...112...>]

On Mon, Apr 11, 2016 at 07:53:16AM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 11 Apr 2016, at 07:07, Wouter Verhelst <w@...112...> wrote:
> > I'm going to reply to this series in more detail later (have to go to
> > work soon), but some quick notes for now:
> > 
> > - I'm not sure I like the idea of having a proxy to do TLS *at the
> >  server side*, although I do agree that there's an upside of "more
> >  shared code with client". To be discussed (and I have some more
> >  thoughts on this that I don't currently have the time to write down).
> 
> I don't particularly like it either.

Good, at least we agree on that then :-)

> But putting it in the main code means wrapping (and thus changing)
> every read and write, which would have been far more invasive. I
> thought better in the first instance to get it in and working, then
> maybe refactor.

Not an unreasonable approach. OTOH, such "we'll do that later"
aproaches tend to end up not getting done in the end.

> OTOH wrapping every read and write may not be a bad idea. I stumbled
> across some problems in negotiate() yesterday where I was getting
> a short read. This is partly because read() is not currently
> retrying when it receives EINTR (I was getting a SIGCHLD).

Right.

Here's those thoughts I was referring to:

Currently, nbd-server consists of a series of functions, where function
X implements feature Y by checking the CLIENT struct to see if feature Y
is enabled, if so doing whatever necessary mangling, and then calling
the next function in the list, *hardcoded*.

I've been wanting to refactor the whole of that so that the CLIENT
struct would contain the heads of a number of linked lists of function
pointers, whereby every function just does whatever mangling or decoding
that is necessary and then calls the next function in the list --
unconditionally.

The config file parser would then need to build that linked list, rather
than set the necessary flags. If done right, I could eventually add a
config file option "backend_method" which would be a colon-separated
string of methods to call (with all those methods being in a GHashTable
or some such) -- resulting in a config line like

    backend_method = cow_sparse:file

for "sparse copy-on-write support over a regular file"; this would be
semantically equivalent to having a config file snippet saying:

    copyonwrite = true
    sparse_cow = true

etc

With such an infrastructure, adding some dlopen() infrastructure for
extra modules to add more such methods would also be possible.

The rest of nbd-server would then just call the first function in the
linked list, and expect that function to call the next one in the list
(if necessary/relevant).

The reason I haven't done this yet is that I'm not sure what the syntax
of that backend_method thing should look like, and that I haven't worked
out the whole backend infrastructure yet. I want to make it flexible
enough, eventually, that things like "export three files in multifile
mode, doing copy-on-write on the center file but not on the other two"
or some such is possible (although a first implementation would
certainly not *need* to do that, and could equally well restrict itself
to being feature-equivalent to older nbd-server implementations).

Once such a backend_method exists, there's no reason why a
linked list of function pointers for the socket side of the equation
could not also exist. These could then deal with the layering of TLS if
necessary, and do the necessary handling of close operations, etc,
abstracting away all the nitty gritty details of handling both encrypted
and not-encrypted connections in the same code.

I had been planning to use the TLS stuff to start work on this
refactoring (at least for the socket side of things), and have some
tentative work in that area done; but it's not nearly as far down as
what you've just sent, so I'm happy to junk that and continue on from
this patch series.

> > - The check for GnuTLS in configure.ac should probably be done after the
> >  AC_PROG_* checks (you shouldn't check for a library before you've
> >  checked for the compiler etc), and should probably use
> >  PKG_CHECK_MODULES rather than AC_CHECK_LIB.
> > - I prefer using an AM_CONDITIONAL to compile certain features
> >  conditionally over using an #ifdef for an entire file.
> 
> My configure.ac skills are somewhere between rusty and rubbish.

Heh :-)

diff --git a/Makefile.am b/Makefile.am
index 304db6d..d43fa90 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -7,6 +7,10 @@ libcliserv_la_SOURCES = cliserv.h cliserv.c
 libcliserv_la_CFLAGS = @CFLAGS@
 nbd_client_SOURCES = nbd-client.c cliserv.h
 nbd_server_SOURCES = nbd-server.c cliserv.h lfs.h nbd.h nbdsrv.h backend.h
+if STARTTLS
+nbd_client_SOURCES += starttls-client.c
+nbd_server_SOURCES += starttls-server.c
+endif
 nbd_trdump_SOURCES = nbd-trdump.c cliserv.h nbd.h
 nbd_server_CFLAGS = @CFLAGS@ @GLIB_CFLAGS@
 nbd_trdump_CFLAGS = @CFLAGS@ @GLIB_CFLAGS@
diff --git a/configure.ac b/configure.ac
index 7806f65..82c9578 100644
--- a/configure.ac
+++ b/configure.ac
@@ -113,6 +113,7 @@ AC_CHECK_SIZEOF(unsigned long int)
 AC_CHECK_SIZEOF(unsigned long long int)
 AC_STRUCT_DIRENT_D_TYPE
 AC_CHECK_FUNCS([llseek alarm gethostbyname inet_ntoa memset socket strerror strstr mkstemp fdatasync])
+PKG_PROG_PKG_CONFIG
 HAVE_FL_PH=no
 AC_CHECK_FUNC(fallocate,
   [
@@ -173,6 +174,14 @@ AC_SUBST(NBD_CLIENT_NAME)
 AC_SEARCH_LIBS(bind, socket,, AC_MSG_ERROR([Could not find an implementation of the bind() system call]))
 AC_SEARCH_LIBS(inet_ntoa, nsl,, AC_MSG_ERROR([Could not find an implementation of the inet_ntoa() system call]))
 AC_SEARCH_LIBS(daemon, resolv,, AC_MSG_ERROR([Could not find an implementation of the daemon() system call]))
+AC_ARG_ENABLE([gnutls], AS_HELP_STRING([--disable-starttls], [do not compile STARTTLS support (requires GnuTLS; default on)]))
+if test "x$enable_gnutls" != xno; then
+       PKG_CHECK_MODULES([GnuTLS], [gnutls >= 3.3.0])
+       AC_DEFINE(HAVE_GNUTLS, 1, [Define to 1 if GnuTLS is available])
+else
+       AC_DEFINE(HAVE_GNUTLS, 0) 
+fi
+AM_CONDITIONAL([STARTTLS], [test "x$enable_gnutls" != xno])
 AC_CHECK_HEADERS([sys/mount.h],,,
 [[#include <sys/param.h>
 ]])

I now notice that PKG_PROG_PKG_CONFIG is in the wrong place (it needs
to be before all those AC_CHECK_* calls), but other than that...

The requirement to have gnutls >= 3.3.0 (released in 2014, so old enough
that most distributions should have it by now) allows to skip
gnutls_global_init() at startup -- always nice to not to have to track
that.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12