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

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

On Mon, Apr 11, 2016 at 08:18:40PM +0100, Alex Bligh wrote:
> 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

Yeah.

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

Sure. Like I said, I'm not opposed to what you're suggesting. It's just
that it adds more work for me later (experience tells me that
"refactoring" is mostly my job), and I'd like to avoid that if we can.

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

Can you be a bit more specific than that? Which parts aren't entirely
clear?

(note that this isn't a prerequisite to get any new features in to me;
but getting some feedback on the above approach would be useful)

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

Yes, that's what my brain dump above is trying to fix, too.

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

While that would be an improvement, it will retain the status quo where
function X has a hard coded call to function Y at the end. I'd like to
get rid of that.

My brain dump of above, if implemented, would result in something along
the following lines (pseudo code):

int copyonwrite_write(CLIENT* client, METHODS* next, int active_fd, off_t offset, size_t len, char* databuf) {
	assert(next->next != NULL);
	next = next->next;
	if(offset is in copyonwrite map) {
		active_fd = copyonwrite_difffile;
	}
	return next->write(client, next, active_fd, offset, min(len, length of current extent in copyonwrite map), databuf);
}

with similar functions for "multiple file support" and "tree files" etc.

Eventually, all the way down, we would have a function doing something
similar to what the writeit() function does today; and then the generic
code just runs:

off_t offset = request->offset;
size_t len = request->len;
size_t total = 0;

while(total < len) {
	total += client->storage_write->write(client, client->storage_write, -1, offset, len);
}

etc.

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

The above has that too, but is more flexible.

>    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've gotten somewhat in that direction with the current GThreadPool
approach. The intent is to improve upon that, certainly.

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

Ick. Right, didn't think of that.

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

Hrm. The auth stuff would probably also just need to hide certain
exports rather than drop the connection upon "you're not allowed that",
too.

I feel yet another full rewrite of negotiation coming up. Sigh.

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

Yeah, sounds like a good idea.

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

by "this", do you mean "the proxy thing", or something else?

> >> My configure.ac skills are somewhere between rusty and rubbish.
> > 
> > Heh :-)
> > 
> 
> Thanks. In the meantime I got most of it working - see PATCHv2 series.

Yeah, I saw.

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

No, my (local, not pushed anywhere) starttls branch from which this
patch was pulled puts it in the wrong location.

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

14.04 isn't going to get a new nbd-server with TLS support. I don't
think supporting ages-old versions of GnuTLS because "people might want
to compile it on ubuntu 4.04" is a particularly bright idea.

Then again, if you're writing the support, who am I to complain?

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

Yeah, well, so is Ubuntu ;-P

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