Re: [Nbd] [PATCHv5 1/7] Add GnuTLS infrastructure

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

On Wed, May 04, 2016 at 10:22:34AM +0100, Alex Bligh wrote:
> 
> On 4 May 2016, at 05:43, Wouter Verhelst <w@...112...> wrote:
> 
> > On Tue, May 03, 2016 at 02:14:09PM +0100, Alex Bligh wrote:
> >> diff --git a/configure.ac b/configure.ac
> >> index 7806f65..204667f 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -114,6 +114,21 @@ AC_CHECK_SIZEOF(unsigned long long int)
> >> AC_STRUCT_DIRENT_D_TYPE
> >> AC_CHECK_FUNCS([llseek alarm gethostbyname inet_ntoa memset socket strerror strstr mkstemp fdatasync])
> >> HAVE_FL_PH=no
> >> +AC_ARG_ENABLE(
> >> +  [gnutls],
> >> +  [AS_HELP_STRING([--enable-gnutls],[Build support for GnuTLS])],
> >> +  [
> >> +    if test "x$enableval" = "xyes"]; then
> >> +       AC_CHECK_LIB([gnutls], [gnutls_init], , AC_MSG_ERROR(no GnuTLS library found))
> > 
> > What happened to using pkg-config?
> 
> I said that I didn't want to introduce a dependency on pkg-config when none
> currently existed, and I don't think I heard back.

Ah, that's possible :-)

I think that's not something you should be worried about. pkg-config is
pretty universal these days; also, it makes it (must) easier on the user
to point to an alternative implementation of particular things.

> But no great shakes.
> 
> There's a simple answer to most of the stuff below:
> 
> The code is not nbd code. It's code from:
>   https://github.com/abligh/tlsproxy
> which is an MIT licensed TLS proxy written as an example for
> GnuTLS (not specifically for nbd).

I know that, but you're suggesting it be merged into the nbd repository.
Once you do that, it becomes nbd code, and we'd rightfully be expected
to maintain it here as well.

> It's a byte-for-byte copy of the files, and I don't particularly fancy
> maintaining two code bases.

Then don't, but essentially you're saying I can now *also* not make
changes to these files once it's merged into the nbd repository. Sorry,
but that's not going to fly. If at some point I think nbd needs to do
something different and it requires changes to some of the files that
are part of tlsproxy and are in the nbd repository, I'm not going to
hold back just because it's "external" code. 

If you make it a library (on which nbd is then going to depend) then
okay, we could do that. I don't think this code is particularly useful
as a library, but it's your call.

At any rate, I do not want to limit the nbd TLS implementation to the
capabilities of some random unrelated code that just happens to
implement something close to what we want. Your tlsproxy is useful, and
yes, it makes sense to use it for the client, but I am absolutely not
convinced it's the right (long-term) approach to TLS in the server.

> > [...]
> >> +  if (socksetnonblock (cryptfd, 0) < 0)
> >> +    {
> >> +      errout (s, "Could not turn on blocking: %m");
> >> +      goto error;
> >> +    }
> > 
> > Why not reuse the function for the same purpose in nbd-server.c? We can
> > obviously move it elsewhere if needs be, but it seems wrong to implement
> > the same thing twice in one codebase.
> 
> Because I neither want to expose that from tlsproxy, nor (for obvious
> reasons) have tlsproxy call nbd.

Then make something common that both use? This is the sort of minor
useful stuff that would be well served by a file containing some utility
functions.

> > [...]
> >> +      /* Repeat select whilst EINTR happens */
> >> +      do
> >> +	{
> >> +	  timeout.tv_sec = wait ? 1 : 0;
> >> +	  timeout.tv_usec = 0;
> >> +	  result = select (maxfd, &readfds, &writefds, NULL, &timeout);
> >> +
> >> +	  selecterrno = errno;
> >> +	}
> >> +      while ((result == -1) && (selecterrno == EINTR) && !quit (s));
> > 
> > Why timeout after a second? Just don't specify a timeout at all, and let
> > select() block until there's data. Otherwise you wake up the CPU for no
> > good reason.
> 
> In an ideal world, you would be right. However, you are presuming the
> crypto library never has any bugs where something happens where it's
> need for FDs being set writeable or readable is wrong (or
> more specifically changes during the call). Painful experiences
> with OpenSSL suggest doing the above is a very good idea, as it
> swaps the possibility of a permanent lockup for a once per second
> extra run through the select loop. Cases I found that failed on
> some versions of OpenSSL revolved around some form of key renegotiation
> (which happens after an hour or so) under heavy load.

Mm, right, makes sense I suppose.

[...]
> >> +      if (quit (s))
> >> +	break;
> > 
> > What does this do? The name "quit" is an imperative, which would imply
> > that the process would end. Only it doesn't, apparently, because you
> > have an if() and a break; statement around it. This is confusing.
> 
> Quit is a function. Perhaps it should be called 'shouldquit' but that's
> what it's called in tlsproxy and I don't particularly want to change it.
>
> > (also, indentation is *totally* wrong, because someone swapped tabs and
> > spaces. Here's why GNU indentation rules suck :-)
> 
> Indentation I think you will find is totally right, because it
> was run through GNU's 'indent' program. Unless something really odd
> has happened!

Point being, the if has six spaces, the break has a tab. The result is
that (in this mail) the break indends less than the if. In your reply,
it probably won't anymore.

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