Re: Merge of two starttls.el's

[Simon Josefsson <jas@extundo.com>]

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 <monnier@iro.umontreal.ca> 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.