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

[Alex Bligh <alex@...872...>]

Wouter,

On 11 Apr 2016, at 12:42, Wouter Verhelst <w@...112...> wrote:

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

My further thought was that doing it right is HARD and will
require significant refactoring (see other email). I've
just found ANOTHER reason for that - see below for the bad news.

I think I'd quite like to get this in as is and get the refactoring
done later (given SSL is in the standard now :-) )

>> 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 think I understood about 70% of that.

Whilst you are looking at refactoring approaches ( :-) ), here's
some comments:

1. The individual functions in nbdserver.c do their own bitbanging.
   (i.e. sending things to the wire). This is partly a result of
   history where there was not much commonality and structure between
   nbd options. This isn't entirely true (see e.g. send_reply()).
   Another approach would be to have the entire option consumed
   or not in one place, and the entire reply sent or not in one place.
   Then you have a single place dealing with the network, you can check
   you've read the option / command completely in one place, and
   generally it's easier. Rather than lots of switch statements on
   what commands / options / replies do, look them up in one function
   which will tell you about the command / option / reply (e.g.
   'does it have a payload', 'do I disconnect afterwards' etc.
   I tried (with about 70% success) to use this approach in
   gondbserver - you might see if you like anything at
   https://github.com/abligh/gonbdserver

2. There is a serious problem implementing NBD_OPT_INFO. In order
   to get the export size, you need to call setupexport(). This
   is currently done much later. The way it's currently written
   (and it's a twisty turny maze of passages all alike) it not
   only gets the export length, but opens, modifies and possibly
   creates various files. I tried moving this (and sendexportinfo)
   into the negotiation bit, but then realised this let
   unauthorized clients write to the disk (EEK!). Of course
   unauthorized clients shouldn't be able to get NBD_OPT_INFO
   anyway. So we need to move the auth stuff (and thus the setpeername
   stuff) into the negotiation bit. And for sanity (I have the
   insane version if you want it) setupexport() needs splitting
   into two functions, one that calculates export size, and one
   that does whatever else that does.

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

As you may remember from a few years ago, I'm not averse to
refactoring nbd-server.c, but I thin kthis is the right approach.

>> My configure.ac skills are somewhere between rusty and rubbish.
> 
> Heh :-)
> 

Thanks. In the meantime I got most of it working - see PATCHv2 series.

> @@ -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>
> ]])

See PATCHv2 and also me trying to avoid introducing a dependency on
pkg-config, but ...

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

... oh, it's already there?

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

Nope. I'm running Ubuntu 14.04 (which is a reasonable OS I think) and
you *definitely* need gnutls_global_init() there. There's no harm in
callign it.

It ships with 2.12.23-12ubuntu2.4 (library version - CLI tools is
all weird).

-- 
Alex Bligh