bug#23759: 25.1.50; 25.1.50; open-tls-stream creates malformed gnutls-cli command if trusted cert files don't exist

[Konstantin Kliakhandler <kosta@slumpy.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3880 bytes --]

Hi,

On 5 July 2016 at 17:49, Noam Postavsky <npostavs@users.sourceforge.net>
wrote:
>
> I think gnutls is broken on master for OSX currently, see
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23503
>

When I do this, with my patch enabled, I get a buffer with:

Cache-Control: max-age=0
> Expires: Tue, 05 Jul 2016 14:58:42 GMT
> Content-Length: 3104
> Keep-Alive: timeout=3, max=100
> Connection: Keep-Alive
> Content-Type: text/html
> Content-Language: en
> ...


Of course, it would have worked even before the patch since currently
tls.el by default attempts two connections via gnutls-tls and then tries
via openssl s_client, which always worked for me (at least for ERC).

On 5 July 2016 at 17:36, Ted Zlatanov <tzz@lifelogs.com> wrote:
>
>
> As you said, one of the key points of your patch is this:
>
> -  '("gnutls-cli --x509cafile %t -p %p %h"
> +  '("gnutls-cli -p %p %h"
> +    "gnutls-cli --x509cafile %t -p %p %h"
>

I wouldn't characterize it as "one of the key points" of my patch, and the
patch would work just as well if instead the line without --x509cafile was
at the bottom of the list. Well, it would work worse for some users, but
the key word is that it would work - except that now now it would take
several more attempts to connect on my computer and on OPs (instead of just
not connecting at all for OP).

Which replaces the specific call with a generic call (no CA file
> specified). This is probably less secure because it will use the system
> CA trustfiles regardless of the user's preferred `gnutls-trustfiles', so
> I'd rather not make it the first thing attempted.


Personally, I also think that the default as defined in my current patch is
preferable, since anyone who messes around with the certificates would edit
this variable e.g. to set there --strict-tofu or the like (I did. It is a
bit more annoying to use, but since I rarely open a new domain in emacs,
it's not a big deal). For everyone else, they trust their system CAs all
the time when they go online. Especially considering that the previous
default for this variable had "--insecure" in the arguments, I thought that
the priorities for the new setting was 1>2>3 "1. It is secure by default.
2. It works by default. 3. It is secure in edge cases", rather than 1>3>2.

Anyway, I do concede that the second version is more secure. Attached is a
patch that I hope is more to your liking. I put the the call that do not
use an explicit certificate at the bottom of the list, even below the call
to openssl s_client. I'm not sure what are the implications, as I don't
know the relative merits of openssl s_client vs gnutls-cli. If you are
inclined to educate me, please do as a short googling did not reveal the
answers.


> Once the libraries are installed, you're all set, they'll be used
> automatically.
>

From what both of you said, I still am not sure what is meant by "native
support". However, for various reasons I don't like the version provided in
homebrew. I prefer the version from https://emacsformacosx.com. Noam, is
this *"one of the pre-built binary packages"* you were referring to, or did
you mean something else? How will I know that the libraries are being used?
Finally, is there a way to test them explicitly? Anyway, it seems that the
version I got from the site above does not have built in gnutls:

system-configuration-features is a variable defined in ‘C source code’.
Its value is "NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS"
system-configuration-options is a variable defined in ‘C source code’.
Its value is
"--with-ns '--enable-locallisppath=/Library/Application
Support/Emacs/${version}/site-lisp:/Library/Application
Support/Emacs/site-lisp'

I'll build one myself and see if the results I get are any different.

Thanks for your time,
Kosta

[-- Attachment #1.2: Type: text/html, Size: 5774 bytes --]

[-- Attachment #2: 0001-tls-Make-open-tls-stream-try-all-gnutls-trustfiles-a.patch --]
[-- Type: application/octet-stream, Size: 3387 bytes --]

From deccbb54d784cc20bf46072498071235b087795b Mon Sep 17 00:00:00 2001
From: Konstantin Kliakhandler <kosta@slumpy.org>
Date: Sat, 2 Jul 2016 02:42:33 +0300
Subject: [PATCH] tls: Make open-tls-stream try all gnutls-trustfiles and set
 better default tls-program

This fixes the bug reported in https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-06/msg00553.html

* lisp/net/tls.el (open-tls-stream): Iterate on all found
  gnutls-trustfiles instead of sticking with the first one found

(tls-program): Set the default to a list that includes both
gnutls-trustfiles substitution and without it.
---
 lisp/net/tls.el | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/lisp/net/tls.el b/lisp/net/tls.el
index f1219fd..bc20685 100644
--- a/lisp/net/tls.el
+++ b/lisp/net/tls.el
@@ -79,7 +79,10 @@ and `gnutls-cli' (version 2.0.1) output."
 (defcustom tls-program
   '("gnutls-cli --x509cafile %t -p %p %h"
     "gnutls-cli --x509cafile %t -p %p %h --protocols ssl3"
-    "openssl s_client -connect %h:%p -no_ssl2 -ign_eof")
+    "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof"
+    "openssl s_client -connect %h:%p -no_ssl2 -ign_eof"
+    "gnutls-cli -p %p %h")
+
   "List of strings containing commands to start TLS stream to a host.
 Each entry in the list is tried until a connection is successful.
 %h is replaced with the server hostname, %p with the port to
@@ -95,12 +98,16 @@ successful negotiation."
     (const :tag "Default list of commands"
 	   ("gnutls-cli --x509cafile %t -p %p %h"
 	    "gnutls-cli --x509cafile %t -p %p %h --protocols ssl3"
-	    "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof"))
+	    "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof"
+            "gnutls-cli -p %p %h"
+            "openssl s_client -connect %h:%p -no_ssl2 -ign_eof"))
     (list :tag "Choose commands"
 	  :value
 	  ("gnutls-cli --x509cafile %t -p %p %h"
 	   "gnutls-cli --x509cafile %t -p %p %h --protocols ssl3"
-	   "openssl s_client -connect %h:%p -no_ssl2 -ign_eof")
+	   "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof"
+           "gnutls-cli -p %p %h"
+           "openssl s_client -connect %h:%p -no_ssl2 -ign_eof")
 	  (set :inline t
 	       ;; FIXME: add brief `:tag "..."' descriptions.
 	       ;; (repeat :inline t :tag "Other" (string))
@@ -227,12 +234,15 @@ Fourth arg PORT is an integer specifying a port to connect to."
     (with-current-buffer buffer
       (message "Opening TLS connection to `%s'..." host)
       (while (and (not done) (setq cmd (pop cmds)))
-	(let ((process-connection-type tls-process-connection-type)
+        (let ((trustfiles (gnutls-trustfiles))
+	      (trustfile nil))
+          (while (and (not done) (setq trustfile (pop trustfiles)))
+        (let ((process-connection-type tls-process-connection-type)
 	      (formatted-cmd
 	       (format-spec
 		cmd
 		(format-spec-make
-                 ?t (car (gnutls-trustfiles))
+                 ?t trustfile
 		 ?h host
 		 ?p (if (integerp port)
 			(int-to-string port)
@@ -269,7 +279,7 @@ Fourth arg PORT is an integer specifying a port to connect to."
 	      (if start-of-data
 		  ;; move point to start of client data
 		  (goto-char start-of-data)))
-	    (setq done process))))
+	    (setq done process))))))
       (when (and done
 		 (or
 		  (and tls-checktrust
-- 
2.9.0