Re: [PATCH] GnuTLS support on Woe32

[Claudio Bley]

At Sun, 06 Mar 2011 18:58:46 +0200,
Eli Zaretskii wrote:
> 
> > From: address@hidden (Claudio Bley)
> > Date: Sun, 06 Mar 2011 16:16:34 +0100
> > 
> > Please find attached a patch which makes building Emacs with GnuTLS
> > support on Woe32 possible.
> 
> Thanks.
> 
> I have a few initial comments, based on reading through the patch.

I'll try to answer as best as I can...
 
> > --- lisp/gnus/starttls.el   2011-01-25 04:08:28 +0000
> > +++ lisp/gnus/starttls.el   2011-03-06 14:57:51 +0000
> > @@ -195,37 +195,46 @@
> >    :type 'regexp
> >    :group 'starttls)
> >  
> > +(eval-and-compile
> > +  (when (fboundp 'gnutls-boot) (require 'gnutls)))
> 
> Can you explain why are these fboundp calls needed?

I want to detect whether the current Emacs instance has been built
with GnuTLS support. If so, load gnutls, otherwise use the old
starttls / gnutls-cli approach.

> > --- lisp/net/gnutls.el      2011-01-25 04:08:28 +0000
> > +++ lisp/net/gnutls.el      2011-03-06 14:57:51 +0000
> > @@ -78,7 +78,8 @@
> >  KEYFILES is a list of client keys."
> >    (let* ((type (or type 'gnutls-x509pki))
> >           (trustfiles (or trustfiles
> > -                        '("/etc/ssl/certs/ca-certificates.crt")))
> > +                         (when (file-exists-p 
> > "/etc/ssl/certs/ca-certificates.crt")
> > +                              '("/etc/ssl/certs/ca-certificates.crt"))))
> 
> Can a file name that starts with a slash work reliably on Windows?

[A] No. It will refer to the drive of CWD. This was just a fix I dropped
in because on Windows there usually is no /etc directory at all and
hence the function failed when passing nil or '() as argument.

The parameter probably should be tri-state: use a default value, use
the given trustfiles or use no trustfiles at all.

> > +2011-03-06  Claudio Bley  <address@hidden>
> > +
> > +   * configure.bat: New options --without-gnutls and --lib, new build
> > +   variable USER_LIBS, automatically detect GnuTLS.
> > +   * INSTALL: Add instructions for GnuTLS support.
> > +   * gmake.defs: Prefix USER_LIB's with -l.
> 
> Why do we need the --lib switch?  We don't require it for any other
> optional libraries.  Can we arrange for GnuTLS support configury to
> work like the other optional libraries?

The difference is that the other libraries are not linked implicitly
but explicitly / dynamically at runtime.

Of course, one could also do this for GnuTLS, but that would need some
refactoring.

Furthermore, using --lib you may easily choose whether to link GnuTLS
statically or as a DLL.

> > +* Optional GnuTLS support
> > +
> > +  To build Emacs with GnuTLS support, make sure that the
> > +  gnutls/gnutls.h header file can be found in the include path and
> > +  link to the appropriate libraries (e.g. gnutls.dll and gcrypt.dll)
> > +  using the --lib option.
> 
> Is it possible to mention here good sites to look for the GnuTLS
> header and libraries?

Sure.

> > +#ifdef WINDOWSNT
> > +#  include "sys/socket.h"
> > +#  include "systime.h"
> > +
> > +/* we need to translate Winsock errors because GnuTLS only checks
> > + * for EAGAIN or EINTR */
> > +static int
> > +wsaerror_to_errno(int err)
> > +{
> > +  switch (err)
> > +    {
> > +    case WSAEWOULDBLOCK:
> > +      return EAGAIN;
> > +    case WSAEINTR:
> > +      return EINTR;
> > +    default:
> > +      return err;
> > +    }
> > +}
> 
> Why is this function needed?  Can you extend w32.c:set_errno instead
> (if it doesn't already support all the values of WSA* errors that you
> need)?

Yes, I could extend w32.c:set_errno, if I move the Windows-specific
function to w32.c proper...

> > +static ssize_t
> > +emacs_gnutls_pull(gnutls_transport_ptr_t p, void* buf, size_t sz)
> 
> Can we move the Windows-specific functions to w32.c, and only call
> them from gnutls.c?  I think we want to keep the Windows-related code
> outside w32*.c to the bare minimum.

OK.

> > +      /* On Windows we cannot transfer socket handles between
> > +       * different runtime libraries.
> > +       *
> > +       * We must handle reading / writing ourselves.
> > +       */
> 
> This is not the Emacs style of comments.

OK.

> > -  ret = gnutls_handshake (state);
> > +  do
> > +    {
> > +      ret = gnutls_handshake (state);
> > +      emacs_gnutls_handle_error (state, ret);
> > +    }
> > +  while (ret < 0 && gnutls_error_is_fatal (ret) == 0);
> 
> This change is not Windows-specific.  What problem(s) does it solve,
> and are those problems relevant to platforms other than Windows?

According to GnuTLS documentation, one should retry calling
gnutls_handshake until it returns 0 (and no fatal error occurred of
course).

For non-blocking sockets handshaking could fail with various non-fatal
errors (e.g. EAGAIN).

> > +  else
> > +    {
> > +        gnutls_alert_send_appropriate (state, ret);
> > +    }
> > +  return ret;
> 
> Likewise here.

This is for alarm handling. If the TLS Server sends an alarm, the
client should react appropriately.

> > -            return (bytes_written ? bytes_written : -1);
> > +            {
> > +              emacs_gnutls_handle_error (state, rtnval);
> > +
> > +              return (bytes_written ? bytes_written : -1);
> > +            }
> 
> And here.  Why do you introduce emacs_gnutls_handle_error?

That function is just a convenience to report errors using
message(). I copied this function shamelessly from gnutls/src/cli.c,
BTW.

> > --- src/process.c   2011-02-18 17:37:30 +0000
> > +++ src/process.c   2011-03-06 14:57:51 +0000
> > @@ -4785,6 +4785,21 @@
> >               &Available,
> >               (check_write ? &Writeok : (SELECT_TYPE *)0),
> >               (SELECT_TYPE *)0, &timeout);
> > +
> > +#ifdef HAVE_GNUTLS
> > +          /*
> > +           * GnuTLS buffers data internally. In lowat mode it leaves some 
> > data
> > +           * in the TCP buffers so that select works, but with custom 
> > pull/push
> > +           * functions we need to check if some data is available in the 
> > buffers
> > +           * manually.
> > +           */
> > +          if (nfds == 0 && wait_proc && wait_proc->gnutls_p
> > +              && gnutls_record_check_pending(wait_proc->gnutls_state) > 0)
> > +          {
> > +              FD_SET (wait_proc->infd, &Available);
> > +              nfds = 1;
> > +          }
> > +#endif
> 
> Is this for Windows only?  If so, please mention that in a comment.
> If not, what problems does it solve on other platforms?

I'm not quite sure. Quoting the GnuTLS manual
(http://www.gnu.org/software/gnutls/manual/html_node/The-transport-layer.html):

For non blocking sockets or other custom made pull/push functions the
gnutls_transport_set_lowat must be called, with a zero low water mark
value.

AFAIU, this means that if using non-blocking sockets (which Emacs does
on other platforms I guess?) you would need to check for pending data
in the GnuTLS buffers or else Emacs might hang, waiting for data on
the socket which will never arrive because it was already transferred
to the internal buffers.

> Last, but not least: I don't see your name on file with the FSF
> copyright assignments.  A contribution of this size will require you
> to sign legal papers.

OK, I'll see to get this done.

At Mon, 07 Mar 2011 08:44:47 +0100,
Robert Pluim wrote:
> 
> >> --- lisp/gnus/starttls.el  2011-01-25 04:08:28 +0000
> >> +++ lisp/gnus/starttls.el  2011-03-06 14:57:51 +0000
> >> @@ -195,37 +195,46 @@
> >>    :type 'regexp
> >>    :group 'starttls)
> >>  
> >> +(eval-and-compile
> >> +  (when (fboundp 'gnutls-boot) (require 'gnutls)))
> >
> > Can you explain why are these fboundp calls needed?
> >
> 
> Please don't install this bit of the patch.  Builtin TLS support builds
> on my platform, but doesn't actually work, so forcing it to be used
> would not be good for me.

Why do you build it if you know it doesn't work?

Anyway, that is a bug which should be fixed.

At Mon, 07 Mar 2011 11:44:56 +0100,
Robert Pluim wrote:
> I modify that comment: builtin TLS support works for me if I set
> 'trustfiles' to nil in gnutls-negotiate, instead of
> "/etc/ssl/certs/ca-certificates.crt", which I don't have.  What is that
> file, and why do I need it all of a sudden? (builtin TLS worked fine for
> me several months ago).

That issue would be fixed by part [A] of the patch (see above).

- Claudio