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

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

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.

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

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

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

> 
> [...]
>> +  while ((!plainEOF || !cryptEOF) && !quit (s))
>> +    {
>> +      struct timeval timeout;
>> +      int result;
>> +      int selecterrno;
>> +      int wait = TRUE;
>> +
>> +      FD_ZERO (&readfds);
>> +      FD_ZERO (&writefds);
>> +
>> +      size_t buffered = gnutls_record_check_pending (s->session);
>> +      if (buffered)
>> +	wait = FALSE;		/* do not wait for select to return if we have buffered data */
>> +
>> +      if (plainEOF)
>> +	{
>> +	  /* plain text end has closed, but me may still have
>> +	   * data yet to write to the crypt end */
>> +	  if (bufIsEmpty (plainToCrypt) && !tls_wr_interrupted)
>> +	    {
>> +	      cryptEOF = TRUE;
>> +	      break;
>> +	    }
>> +	}
>> +      else
>> +	{
>> +	  if (!bufIsEmpty (cryptToPlain))
>> +	    FD_SET (plainfd, &writefds);
>> +	  if (!bufIsOverHWM (plainToCrypt))
>> +	    FD_SET (plainfd, &readfds);
>> +	}
>> +
>> +      if (cryptEOF)
>> +	{
>> +	  /* crypt end has closed, but me way still have data to
>> +	   * write from the crypt buffer */
>> +	  if (bufIsEmpty (cryptToPlain) && !buffered)
>> +	    {
>> +	      plainEOF = TRUE;
>> +	      break;
>> +	    }
>> +	}
>> +      else
>> +	{
>> +	  if (!bufIsEmpty (plainToCrypt) || tls_wr_interrupted)
>> +	    FD_SET (cryptfd, &writefds);
>> +	  if (!bufIsOverHWM (cryptToPlain))
>> +	    FD_SET (cryptfd, &readfds);
>> +	}
> 
> This is, essentially, twice the same code. I've developed an allergy to
> that. Can use a function or a macro or something of the sorts instead of
> copy/pasting lines of code?
> 
> Also, I can't help but think this is not the most efficient way to do
> things, although I'm not awake enough yet to come up with a better way
> :-)

It isn't the same code twice. It's *nearly* the same code twice. As with
everything TLS, nothing is ever simple. Specifically you need to look for
the treatment of tls_wr_interrupted and buffered, which are asymmetric
in their implementation, and necessarily so.

Again, I don't want to change this, as the whole tlsproxy thing is
external code which is written to be simple and understandable, even
if it's not fantastically efficient (actually the GnuTLS maintainer
looked at it and said was fine, but I don't claim any great efficiency).

> [...]
>> +      /* 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.

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

Again, it's not nbd code.

-- 
Alex Bligh