Re: [Nbd] [PATCH/RFC 0/3] Introduce TLS on nbdserver
Hi Alex,
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).
- 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.
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
Reply to: