From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: claudio.bley@gmail.com (Claudio Bley) Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] GnuTLS support on Woe32 Date: Mon, 07 Mar 2011 22:03:46 +0100 Message-ID: <87ipvuwslp.wl%claudio.bley@gmail.com> References: <87ipvwl1nx.wl%claudio.bley@gmail.com> <83oc5ogp89.fsf@gnu.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII X-Trace: dough.gmane.org 1299532008 3521 80.91.229.12 (7 Mar 2011 21:06:48 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 7 Mar 2011 21:06:48 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Mar 07 22:06:42 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Pwhd9-0003bh-6q for ged-emacs-devel@m.gmane.org; Mon, 07 Mar 2011 22:06:38 +0100 Original-Received: from localhost ([127.0.0.1]:36274 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pwhb8-0002rv-9I for ged-emacs-devel@m.gmane.org; Mon, 07 Mar 2011 16:04:10 -0500 Original-Received: from [140.186.70.92] (port=53661 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pwhb2-0002ra-La for emacs-devel@gnu.org; Mon, 07 Mar 2011 16:04:06 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pwhb0-00016g-Cc for emacs-devel@gnu.org; Mon, 07 Mar 2011 16:04:04 -0500 Original-Received: from lo.gmane.org ([80.91.229.12]:45473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pwhaz-00016K-TD for emacs-devel@gnu.org; Mon, 07 Mar 2011 16:04:02 -0500 Original-Received: from list by lo.gmane.org with local (Exim 4.69) (envelope-from ) id 1Pwhax-0002WR-AC for emacs-devel@gnu.org; Mon, 07 Mar 2011 22:03:59 +0100 Original-Received: from dslb-188-106-012-040.pools.arcor-ip.net ([188.106.12.40]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 07 Mar 2011 22:03:59 +0100 Original-Received: from claudio.bley by dslb-188-106-012-040.pools.arcor-ip.net with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 07 Mar 2011 22:03:59 +0100 X-Injected-Via-Gmane: http://gmane.org/ Mail-Followup-To: emacs-devel@gnu.org Original-Lines: 248 Original-X-Complaints-To: usenet@dough.gmane.org X-Gmane-NNTP-Posting-Host: dslb-188-106-012-040.pools.arcor-ip.net In-Reply-To: <83oc5ogp89.fsf@gnu.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/23.1 (i686-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) Mail-Copies-To: never X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 80.91.229.12 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:136846 Archived-At: At Sun, 06 Mar 2011 18:58:46 +0200, Eli Zaretskii wrote: > > > From: claudio.bley@gmail.com (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 > > + > > + * 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