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

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

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 <w@...112...> 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