Re: [Nbd] TLS implementation in reference nbd-server

[Wouter Verhelst]

On Fri, Oct 14, 2016 at 02:23:55PM -0500, Eric Blake wrote:
> On 10/14/2016 01:30 PM, Wouter Verhelst wrote:
> >>>
> >>> It's not been tested yet, however, because the client side hasn't been
> >>> done yet.
> >>
> >> It has at least one bug, from what I've quickly seen.  You HAVE to parse
> >> off length and any data the client sends before you can try to read the
> >> next option; you have this bug in multiple places.
> > 
> > Whoops.
> 
> > 
> >  static void socket_read(CLIENT* client, void *buf, size_t len) {
> > +   void *tmp = NULL;
> > +   if (!buf) {
> > +   /* FIXME: Enforce maximum bound on client-provided len? */
> > +   tmp = buf = malloc(len);
> > +   }
> > g_assert(client->socket_read != NULL);
> 
> Any thoughts about this one? Should we enforce a maximum client option
> length (such as: any client that wants to send us an NBD_OPT_* with more
> than 1M payload is trying to DoS us)?  If so, what is an appropriate
> limit?  We've sort of had the conversation in a past thread about
> maximum limits that a server must tolerate before declaring the client
> to be a waste of time (such as how many error messages can a server send
> before deciding the client isn't going to ask a successful question; how
> much data in NBD_OPT is too much, while still permitting our spec of 64k
> strings as acceptable and assuming we may add a future NBD_OPT that has
> multiple strings, etc).  At one point, I had even proposed a patch to
> force length to be 16 bits (freeing up 16 other bits that could be used
> for future expansion for some other purpose), but as I recall you didn't
> like the idea.

Mmm, right, and I still don't :-)

> Since we are malloc'ing a scratch buffer to hold a client-specified
> length, I do NOT want us to be casually allowing the client to tell us
> to make a 2G allocation.  Maybe when reading off dead length, it's
> better to write a loop that does a loop into a max-size buffer for as
> many loop iterations as needed, rather than allocating a single buffer
> that will just be thrown away; but such complexity doesn't belong on the
> hot-path of normal reads.  Still, even if I cap maximum allocation by
> reading in a loop, there's a question of how much time we allow to
> processing dead reads, vs. cutting our losses and disconnecting the
> client as ill-behaved.

As you've said downthread, there already is a function for that. Still,
I think it is a good idea to answer this question.

How about 4K? That's the maximum length for a string too, right now, and
it conveniently is just the size of a single memory page on most
architectures (i.e., the minimum allocation unit).

> >>send_reply(client, opt, NBD_REP_ERR_INVALID, 
> >> -1, "Invalid STARTTLS
> >> request: TLS has already been negotiated!");
> >> +  socket_read(client, , sizeof(len));
> >> +  len = ntohl(len);
> >> +  if(len) {
> >> +  socket_read(client, NULL, len);
> > 
> > This should probably send NBD_REP_ERR_INVALID, though (since STARTTLS does 
> > not
> > allow data).
> > 
> 
> But we've already sent NBD_REP_ERR_INVALID in the line just before we
> detect the invalid length

Ah, yes, I'm an idiot. Didn't see that.

> (note: the guest can't tell whether we replied with that error because
> of invalid length or because of requesting at an invalid point in the
> handshake protocol).

Sure.

> Since I repeated a common construct across multiple lines, I may create
> a helper function to parse off length.  Especially if I want to add
> complexity for handling over-long user-supplied lengths without a huge
> malloc.

Would be helpful. Also, we should probably read the whole header before
starting to process it.

(a packed struct, for instance, which we already damn well do for other
protocol messages...)

> >> +  }
> >>continue;
> >>}
> >>if(tlsdir == NULL) {
> >>send_reply(client, opt, NBD_REP_ERR_POLICY, -1, 
> >> "TLS not allowed on
> >> this server");
> >> +  socket_read(client, , sizeof(len));
> >> +  len = ntohl(len);
> >> +  if(len) {
> >> +  socket_read(client, NULL, len);
> > 
> > Same here.
> 
> Okay, here we have a case where two errors are both possible at the same
> time: NBD_REP_ERR_POLICY (that the code just sent in the line earlier)
> and NBD_REP_ERR_INVALID (for bad length).  The protocol doesn't state
> any priority scheme when multiple errors are simultaneously possible,
> does it?

Not that I know of...

> I don't see that it makes any particular difference, although it may
> mean that we want to add a sentence to the protocol clarifying that if
> more than one error makes sense, a client MUST NOT rely on the server
> to send a specific error, 

Re: [Nbd] TLS implementation in reference nbd-server

[Wouter Verhelst]

On Fri, Oct 14, 2016 at 04:56:02PM -0500, Eric Blake wrote:
> On 10/14/2016 01:18 PM, Wouter Verhelst wrote:
> 
> >>> If you want to check it out, just run nbd-server from git master.
> >>> Feedback (and/or review) welcome :-)
> >>
> >> I'm happy to have a detailed look at this later (and indeed
> >> do some interoperability testing - I'll see if I can dig out
> >> the qemu-img command line I used to test gonbdserver),
> 
> I've also got some experience creating qemu-nbd command lines for
> interoperability; here's one with new enough qemu:
> 
> $  qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
> dir=/path/to/qemutls --tls-creds tls0 --exportname default

Cool.

> where /path/to/qemutls contains the necessary certificate files.

Any particular names that should be used?

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

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] TLS implementation in reference nbd-server

[Eric Blake]

On 10/14/2016 01:18 PM, Wouter Verhelst wrote:

>>> If you want to check it out, just run nbd-server from git master.
>>> Feedback (and/or review) welcome :-)
>>
>> I'm happy to have a detailed look at this later (and indeed
>> do some interoperability testing - I'll see if I can dig out
>> the qemu-img command line I used to test gonbdserver),

I've also got some experience creating qemu-nbd command lines for
interoperability; here's one with new enough qemu:

$  qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
dir=/path/to/qemutls --tls-creds tls0 --exportname default

where /path/to/qemutls contains the necessary certificate files.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] TLS implementation in reference nbd-server

[Wouter Verhelst]

Hi Alex,

On Thu, Oct 13, 2016 at 09:47:53AM +0100, Alex Bligh wrote:
> Wouter,
> 
> > On 12 Oct 2016, at 19:40, Wouter Verhelst  wrote:
> > 
> > While stuck in an airport on a 9-hour layover two days ago, I (finally)
> > spent some time working on a STARTTLS implementation for the reference
> > nbd-server implementation. Configuration is fairly basic; just add a
> > "tlsdir = " configuration item to the nbd-server config file, create a
> > ca.pem, priv.pem, and cert.pem file in that location, and you're good.
> > The current implementation doesn't allow for authenticating clients
> > through certificates or other means; I will probably want to add that at
> > some point in the future, but not just yet.
> > 
> > It's not been tested yet, however, because the client side hasn't been
> > done yet. I will do that before I release this (probably by adding Alex'
> > implementation from a few months back). Also, I think I should test
> > against current implementations of the NBD STARTTLS option (e.g., qemu),
> > and see if things interoperate with that too, before going further.
> > Hasn't been done yet (mostly because the documentation on how to do
> > starttls in qemu nbd seems incomplete, at best; a pointer to an example
> > or some such would be welcome), but expect this in the next few weeks or
> > so.
> > 
> > If you want to check it out, just run nbd-server from git master.
> > Feedback (and/or review) welcome :-)
> 
> I'm happy to have a detailed look at this later (and indeed
> do some interoperability testing - I'll see if I can dig out
> the qemu-img command line I used to test gonbdserver),

Would be cool, yes. Once you did so, would be nice if you could also
post the details here, so I can replicate what you do more easily ;-)

> but a couple of questions from now:
> 
> 1. I take it you want patch comments by email here (not in
>github)?

Not sure. I think the github workflow works pretty well, but I also
don't want to force it upon people. We've been using a "mailinglist
patches" workflow for quite a while now, and it works too.

Thus, I'm open to suggestions to change that, but for now mailinglist
patches might indeed be the right way to go.

> 2. I really don't like the 'tlsdir' configuration option.

Well, granted, it was a quick "this seems easier to code up" thing
rather than looking at how to do it properly. I don't really like it
either, I just didn't want to complicate things by way too much.

I was stuck in the airport for 9 hours, but really, it was late when I
wrote that code. Or, to put it differently: There's a reason why the man
page currently says "future versions may allow..." on that bit ;-)

>It seems like a recipe for getting things wrong. Firstly Linux (at
>least) already seems to have standards for where SSL certs go - in
>/etc/ssl/certs and /etc/ssl/private,

Sortof -- that stuff is really meant for other things, and using
/etc/ssl/certs and/or /etc/ssl/private is actually wrong (although yes,
I know it happens often).

>i.e. two separate directories. These already have permissions set
>correctly, and facilitate sharing with other applications for the
>same server name. Secondly, it's not obvious why the certificate,
>private key file, and cert should have that name.  Thirdly this
>precludes a configuration where the private key and certificate are
>in the same file and only one path is given.

All good points, and we probably should indeed fix that.

>Fourthly, if you aren't checking client certificates, why is a CA
>file mandatory?

Different CA. This is for the CA that contains the server certificate,
not the CA used for validating client certificates. Last I checked you
want to pass that to the server too (but it was late and I might have
been an idiot).

[...]
> 3. You don't seem to permit validating client certificates. This
>is (a) pretty easy, and (b) pretty important.

Haven't gotten around to it yet. The purpose of this patch was "let's
make basic encryption work", not "let's make everything work the way it
should". Now that we have basic STARTTLS, adding the required bits to
handle_starttls() should be trivial.

Yes, I agree that client certificate auth should be provided. Will
probably be done before releasing this (but patches are, as usual,
welcome)

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

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general