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

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

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

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

> - The check for GnuTLS in configure.ac should probably be done after the
>  AC_PROG_* checks (you shouldn't check for a library before you've
>  checked for the compiler etc), and should probably use
>  PKG_CHECK_MODULES rather than AC_CHECK_LIB.
> - I prefer using an AM_CONDITIONAL to compile certain features
>  conditionally over using an #ifdef for an entire file.

My configure.ac skills are somewhere between rusty and rubbish.

Alex

> 
> On Sun, Apr 10, 2016 at 07:59:05PM +0100, Alex Bligh wrote:
>> This is an RFC patch to introduce TLS support on nbdserver.
>> 
>> 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 (believe it or not) could be taken for nbdclient (which
>> is my plan). 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.
>> 
>> 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.
>> 
>> Alex Bligh (3):
>>  Add GnuTLS infrastructure
>>  Add options for TLS support for server
>>  Add TLS support to server
>> 
>> Makefile.am              |   4 +-
>> buffer.c                 | 225 +++++++++++++++++
>> buffer.h                 |  45 ++++
>> cliserv.h                |   1 +
>> configure.ac             |  11 +
>> crypto-gnutls.c          | 615 +++++++++++++++++++++++++++++++++++++++++++++++
>> crypto-gnutls.h          |  43 ++++
>> man/nbd-server.5.in.sgml |  65 +++++
>> nbd-server.c             | 204 +++++++++++++---
>> nbdsrv.h                 |   1 +
>> 10 files changed, 1185 insertions(+), 29 deletions(-)
>> create mode 100644 buffer.c
>> create mode 100644 buffer.h
>> create mode 100644 crypto-gnutls.c
>> create mode 100644 crypto-gnutls.h
>> 
>> -- 
>> 1.9.1
>> 
>> 
>> ------------------------------------------------------------------------------
>> Find and fix application performance issues faster with Applications Manager
>> Applications Manager provides deep performance insights into multiple tiers of
>> your business applications. It resolves application problems quickly and
>> reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
>> gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532
>> _______________________________________________
>> Nbd-general mailing list
>> Nbd-general@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/nbd-general
>> 
> 
> -- 
> < 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
> 

-- 
Alex Bligh