Re: [Nbd] [PATCHv5 0/7] Introduce TLS support on nbdserver & nbdclient

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

Hi Alex,

On Tue, May 03, 2016 at 02:14:08PM +0100, Alex Bligh wrote:
> This is an RFC patch to introduce TLS support on nbdserver & nbdclient.
> 
> This is *NOT* production ready by any means, and is submitted for comment.
> 
> I have added crypto-gnutls.[ch] from:
>    github.com/abligh/tlsproxy
> which is my attempt at an MIT licenced GnuTLS proxy. The proxy element
> is standalone, and is incorporated here. Whilst it's not GPL licensed,
> MIT is compatible. Also it uses GNU format indentation (sorry about that).
> However, it (together with buffer.[ch]) is almost entirely self-contained.
> 
> The same approach is taken for nbdclient. As the proxy runs as an independent
> process, nbdclient can launch it, then call the DOIT ioctl() on one end of
> the created socketpair(). The proxy process then drops into the background
> and closes after either the kernel closes the socket or the other end closes
> the socket.
> 
> I have tested this to a minimal extent against qemu-img (i.e. qemu
> acting as a client). The problem (see nbdgeneral ad nauseam) we have
> with NBD_CMD_DISC means that we see false reports of 'magic number mismatch'.
> This appears to be because read() returns 0 in negotiate(), and nbdserver
> does not check for this. It then reverses (again) the *previous* magic
> number, using ntohll(), and this causes the 'magic number mismatch' issue.
> This isn't really a problem but causes confusing errors.
> 
> I have also tested nbd-client against nbd-server with TLS, and
> the test suite tests nbd-tester-client against nbd-server with TLS.
> 
> The first two patches are preparatory work, and the third patch actually
> adds NBD_OPT_STARTTLS. The comment in that patch shows what is left to
> figure out.
> 
> Changes from v4:
> 
> * Fix hash table up so we can count number of server children

I'd rather see that the server doesn't spawn a child to do TLS at all,
as I've indicated before. My long-term plan is to go to a situation
where the server, at most, spawns a thread for each client (if even
that). Currently we have a process per client, which take resources and
is a suboptimal situation.  It also means the maxthreads option is to be
multiplied per client, rather than be a single configuration for the
whole server.

I'm not saying that fixing that up is a prerequisite for implementing
TLS, but "not making the situation worse" is.

(obviously the above only applies to the server; on the client, spawning
a proxy is the only right thing to do)

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