From mboxrd@z Thu Jan 1 00:00:00 1970 Path: main.gmane.org!not-for-mail From: Simon Josefsson Newsgroups: gmane.emacs.devel Subject: Re: Merge of two starttls.el's Date: Thu, 27 May 2004 09:10:54 +0200 Sender: emacs-devel-bounces+emacs-devel=quimby.gnus.org@gnu.org Message-ID: References: NNTP-Posting-Host: deer.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: sea.gmane.org 1085642483 29898 80.91.224.253 (27 May 2004 07:21:23 GMT) X-Complaints-To: usenet@sea.gmane.org NNTP-Posting-Date: Thu, 27 May 2004 07:21:23 +0000 (UTC) Cc: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+emacs-devel=quimby.gnus.org@gnu.org Thu May 27 09:21:13 2004 Return-path: Original-Received: from quimby.gnus.org ([80.91.224.244]) by deer.gmane.org with esmtp (Exim 3.35 #1 (Debian)) id 1BTFCT-0004go-00 for ; Thu, 27 May 2004 09:21:13 +0200 Original-Received: from monty-python.gnu.org ([199.232.76.173]) by quimby.gnus.org with esmtp (Exim 3.35 #1 (Debian)) id 1BTFCT-0007nq-00 for ; Thu, 27 May 2004 09:21:13 +0200 Original-Received: from localhost ([127.0.0.1] helo=monty-python.gnu.org) by monty-python.gnu.org with esmtp (Exim 4.34) id 1BTF49-00073i-Er for emacs-devel@quimby.gnus.org; Thu, 27 May 2004 03:12:37 -0400 Original-Received: from list by monty-python.gnu.org with tmda-scanned (Exim 4.34) id 1BTF3V-0006zG-1N for emacs-devel@gnu.org; Thu, 27 May 2004 03:11:57 -0400 Original-Received: from mail by monty-python.gnu.org with spam-scanned (Exim 4.34) id 1BTF2x-0006pQ-8P for emacs-devel@gnu.org; Thu, 27 May 2004 03:11:54 -0400 Original-Received: from [217.13.230.178] (helo=yxa.extundo.com) by monty-python.gnu.org with esmtp (TLSv1:DES-CBC3-SHA:168) (Exim 4.34) id 1BTF2v-0006n7-1V for emacs-devel@gnu.org; Thu, 27 May 2004 03:11:22 -0400 Original-Received: from latte.josefsson.org (jas@yxa-iv.extundo.com [217.13.230.178]) (authenticated bits=0) by yxa.extundo.com (8.12.11/8.12.11/Debian-3) with ESMTP id i4R7Auxv021564 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NO); Thu, 27 May 2004 09:11:04 +0200 Original-To: Stefan Monnier X-Hashcash: 0:040527:monnier@iro.umontreal.ca:199b9040c6a72eab X-Hashcash: 0:040527:emacs-devel@gnu.org:0374d1a9a092d788 In-Reply-To: (Stefan Monnier's message of "26 May 2004 16:25:44 -0400") User-Agent: Gnus/5.110003 (No Gnus v0.3) Emacs/21.3.50 (gnu/linux) X-Virus-Scanned: clamd / ClamAV version 0.70, clamav-milter version 0.70j X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.4 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+emacs-devel=quimby.gnus.org@gnu.org Xref: main.gmane.org gmane.emacs.devel:24004 X-Report-Spam: http://spam.gmane.org/gmane.emacs.devel:24004 Thanks for quick review! I'm adding emacs-devel to Cc, I hope you don't mind. Several of the issues can probably be regarded as open for discussion. Stefan Monnier writes: >> +(defcustom starttls-gnutls-program "gnutls-cli" >> + "Name of GNUTLS command line tool. >> +This program is used when GNUTLS is used, i.e. when >> +`starttls-use-gnutls' is non-nil." >> + :type 'string >> + :group 'starttls) >> + >> (defcustom starttls-program "starttls" >> - "The program to run in a subprocess to open an TLSv1 connection." >> + "The program to run in a subprocess to open an TLSv1 connection. >> +This program is used when the `starttls' command is used, >> +i.e. when `starttls-use-gnutls' is nil." >> :type 'string >> :group 'starttls) > >> +(defcustom starttls-use-gnutls (not (executable-find starttls-program)) >> + "*Whether to use GNUTLS instead of the `starttls' command." >> + :type 'boolean >> + :group 'starttls) > > I haven't looked enough at the code to understand whether it would make > sense, but I would naturally be tempted to replace the above with > > (defcustom starttls-use-gnutls (not (executable-find starttls-program)) > ... > (defcustom starttls-program (if starttls-use-gnutls "gnutls-cli" "starttls") The reason for doing it the other way is that people can't re-define the name of both tools, and switch between them using only starttls-use-gnutls. I.e., using your proposal, if the user toggle starttls-use-gnutls, she must also toggle starttls-program. Perhaps I'm the only one, but I'd like to toggle between both implementations using starttls-use-gnutls (because GNUTLS and Openssl are very different, with different default ciphers etc), and I often have both tools installed at weird locations (not in PATH). >> (defcustom starttls-extra-args nil >> - "Extra arguments to `starttls-program'." >> + "Extra arguments to `starttls-program'. >> +This program is used when the `starttls' command is used, >> +i.e. when `starttls-use-gnutls' is nil." >> :type '(repeat string) >> :group 'starttls) > >> +(defcustom starttls-extra-arguments nil >> + "Extra arguments to `starttls-program'. >> +This program is used when GNUTLS is used, i.e. when >> +`starttls-use-gnutls' is non-nil. > > Similarly here I would just use a single variable. Here there is another reason: the command line arguments to the two tools are incompatible. Look at smtpmail.el: (let* ((cred-key (smtpmail-cred-key cred)) (cred-cert (smtpmail-cred-cert cred)) (starttls-extra-args (when (and (stringp cred-key) (stringp cred-cert) (file-regular-p (setq cred-key (expand-file-name cred-key))) (file-regular-p (setq cred-cert (expand-file-name cred-cert)))) (list "--key-file" cred-key "--cert-file" cred-cert)))) (starttls-open-stream "SMTP" process-buffer host port))))) These command line parameters do not work with gnutls-cli. I expect that smtpmail will have to add a new let variable `starttls-extra-arguments' with the GNUTLS command line parameters too. I should have mentioned this complexity. The variable names do not suggest the GNUTLS/starttls incompatibility was the motivation for the two variables. It would be nicer to not require callers of starttls.el to know the command line syntax of various implementations that starttls.el uses. Hm. Perhaps add temporary variables for the cert/key filenames? I.e.: (defvar starttls-key-file nil) (defvar starttls-cert-file nil) Then elisp code calling starttls-open-stream could bind those variables, and the code for gnutls-cli and starttls could make sure the proper parameter is used. The starttls-extra-args variable would need to be kept, though, for backwards compatibility. >> +(defcustom starttls-process-connection-type nil >> + "*Value for `process-connection-type' to use when starting STARTTLS process." >> + :type 'boolean >> + :group 'starttls) > > This seems unrelated to adding support for gnutls. Is that right? Yes. > What is its purpose? I haven't needed the variable now, but my experience with imap.el suggest it sometimes work better to toggle that variable. Note that the default value isn't changed, it just make it possible for users to toggle it, which can be useful to suggest when people report problems. (Looking at my own .emacs, I have been running with imap-process-connection-type set to t for ages. Perhaps the problems that made me use that setting has been fixed, though...) >> +(defcustom starttls-connect "- Simple Client Mode:\n\n" >> +(defcustom starttls-failure "\\*\\*\\* Handshake has failed" >> +(defcustom starttls-success "- Compression: " > > Those shouldn't be a defcustom. I'd make it a defconst or maybe defvar. > Are users ever expected to fiddle with this? I'm not sure. There are two concerns. 1) GNUTLS is still under heavy development, and I'm not sure those strings will be stable. So it would be nice for users to change them, as Emacs have a longer release cycle than GNUTLS. 2) It might allow advanced users to swap in a completely different implementation than "gnutls-cli", change those strings to something else, and have things work. Given the above discussion about hard coded command line parameters, this argument is not a strong one, since the parameters would likely be incompatible as well. > Other than that, it looks safe, so I have no objection to your installing > this patch, Great. I'll wait a few days to let other people comment.