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

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

Wouter,

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

:-)

>> I think I understood about 70% of that.
> 
> Can you be a bit more specific than that? Which parts aren't entirely
> clear?

Let me look at that again in the cold light of day tomorrow. I suspect
(given your comment below) I may have missed bits.

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

OK

>>   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);
> }

Ah, so something like a layered set of backend drivers? This is
pretty much what qemu's block layer does, except without the
scary coroutines.

I was actually talking about bitbanging the network protocol
side.

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

OK, understand that better now.

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

I think the two things are orthogonal (as I'm talking about the
network side and hadn't really given any thought to the disk
backend side). In gonbdserver I do that through an interface
(aka abstract class), but I haven't yet needed to layer them

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

Yeah that's neat. Saw that.

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

Yeah :-(

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

The proxy thing (for now).

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

I mean "the dependency on PKG_CONFIG is 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.
> 
> 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?

No sure, but's it's a reasonable target to compile on. 16.04
is either not out yet or only just out, so it's the current
LTS version of Ubuntu. I'm guessing there are other long term
support distros which are the same. (EDIT: apparently not
RHEL, Centos or Debian's stable version, so that's weird).

But I wrote the proxy stuff deliberately so it wouldn't require new
features of GnuTLS, and yes I'm happy to maintain it that way,
at least for a while. And crypto-gnutls.c is now a byte-for-byte
copy of tlsproxy's file of the same name, so this should be
easy.

-- 
Alex Bligh