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: 2467 bytes --]

Hello,

First, I apologize for the double posting - I realize that I sent the
previous message with a messed up subject and this caused it not to be
grouped with the rest of the messages in this bug, and to not appear in the
tracker. I hope this second one will work now.

I am using emacs on OSX 10.12 from https://emacsformacosx.com/builds:

(emacs-version)
"GNU Emacs 25.1.50.1 (x86_64-apple-darwin13.4.0, NS appkit-1265.21 Version
10.9.5 (Build 13F1603))
 of 2016-05-30"

And experience the same problem when running emacs -Q. Furthermore, I
diagnosed the cause to be incorrect building of formatted-cmd in
open-tls-stream from the given arguments. Attached is a patch that fixes
the problem on my end. To make the patch smaller, I did not reindent the
whole function - I hope this is alright.

Finally, I'd like to reply to:


> Fran <flitterio <at> gmail.com> writes:

> If Cygwin is installed, open-tls-stream works, because gnutls-cli and
> openssl
> > are available. It has worked for some time. I routinely use this feature
> to
> > enable ERC to connect to servers using TLS using function erc-tls.



> At some point, tls.el will be deprecated. Why aren't you just using
> Emacs with the built-in TLS support?


In my honest opinion, a feature is either deprecated or not, and while it
is not yet deprecated, bugs should not be ignored. I hope you would agree
at least to the point of testing the patch and incorporating it if it works
well :-)

Fuller description of the problem and the fix:

The problem: `open-tls-stream' replaces %t with exactly one element, which
is nil if none of gnutls-trustfiles is readable, and the first element of
gnutls-trustfiles is more than one is readable.
The Solution: In the patch I make the test iterate on all the trustfiles as
a user might have more than one relevant. In addition, I made the default
setting for tls-program have entries that do not explicitly specify the
trustfile.

One thing to note here perhaps, is that if (gnutls-trustfiles) returns an
empty list and one has the %t substitution in one of the tls-program
entries, then that entry will not be run at all. I feel that this is
reasonable since by setting --x509cafile nil one makes gnutls-cli fail
anyway.

Finally, I'm experiencing the above behavior, as far as I can tell, by
default in e.g. erc-tls. What is the proper way to move to the built in
TLS? Is it likely to be something in my config or in the implementation of
ERC?

Thanks,
Kosta

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

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

From 057fc9158e816e5220b9303a2b634eab1d7c735e 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 | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/lisp/net/tls.el b/lisp/net/tls.el
index f1219fd..7890ac0 100644
--- a/lisp/net/tls.el
+++ b/lisp/net/tls.el
@@ -77,9 +77,11 @@ and `gnutls-cli' (version 2.0.1) output."
   :group 'tls)
 
 (defcustom tls-program
-  '("gnutls-cli --x509cafile %t -p %p %h"
+  '("gnutls-cli -p %p %h"
+    "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 -connect %h:%p -no_ssl2 -ign_eof"
+    "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof")
   "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
@@ -93,14 +95,18 @@ successful negotiation."
   :type
   '(choice
     (const :tag "Default list of commands"
-	   ("gnutls-cli --x509cafile %t -p %p %h"
+	   ("gnutls-cli -p %p %h"
+            "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"))
     (list :tag "Choose commands"
 	  :value
-	  ("gnutls-cli --x509cafile %t -p %p %h"
+	  ("gnutls-cli -p %p %h"
+           "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 -connect %h:%p -no_ssl2 -ign_eof"
+	   "openssl s_client -CAfile %t -connect %h:%p -no_ssl2 -ign_eof")
 	  (set :inline t
 	       ;; FIXME: add brief `:tag "..."' descriptions.
 	       ;; (repeat :inline t :tag "Other" (string))
@@ -227,12 +233,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 +278,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.7.4 (Apple Git-66)