open-{gnutls,network}-stream backwards compatibility

open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Hi,

Iʼm working on a fix for bug 33780 [1].

I have two questions:

1.  As part of that fix, Iʼd like to change the signature of
    open-gnutls-stream from

    (open-gnutls-stream name buffer host service &optional nowait)

    to

    (open-gnutls-stream name buffer host service &optional nowait parameters)

    Normally this would be fine, I think, except that the caller in
    Emacs core derives the value of 'nowait' from the plist that would be
    passed in via 'parameters' anyway, so Iʼm tempted to just change it
    to:

    (open-gnutls-stream name buffer host service &optional parameters)

    then adjust the single in-core caller and have open-gnutls-stream
    call plist-get.

    Does anyone see any issue with doing this? I have no idea if this
    function is used by external code, but the Emacs GnuTLS manual says
    "You should not have to use the ‘gnutls.el’ functions directly." so
    that should mean the calling convention is fair game. [2]

2.  The whole reason for doing this is so that people can use client
    certificates rather than passwords for authentication of network
    connections. Of course, for this to work at all, all the callers
    of open-network-stream [3] need to be updated to pass in
    ':client-certificate t' to cause 'auth-source' lookups to be
    performed for those certificates.

    Again Iʼm tempted to change open-network-stream such that not
    specifying :client-certificate is the same as specifying t, so
    that all Emacs core and external packages can take advantage of
    the feature just by adjusting their .authinfo entries, similarly
    to how password lookup automatically works today. However, this
    would be a change in default behaviour, plus I know some people
    are very sensitive to changes in this particular area, so I
    thought Iʼd ask here before doing anything.
    
Cheers

Robert

Footnotes:
[1]  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33780#17

[2]  I could keep the single optional parameter, and have it be the
     union of nil/t/plist, with nil/t referring to :nowait, but that
     seems needlessly complicated

[3]  Of which there are at least 12 in Emacs itself

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 02 Jan 2019 17:49:13 +0100
> 
> 1.  As part of that fix, Iʼd like to change the signature of
>     open-gnutls-stream from
> 
>     (open-gnutls-stream name buffer host service &optional nowait)
> 
>     to
> 
>     (open-gnutls-stream name buffer host service &optional nowait parameters)
> 
>     Normally this would be fine, I think, except that the caller in
>     Emacs core derives the value of 'nowait' from the plist that would be
>     passed in via 'parameters' anyway, so Iʼm tempted to just change it
>     to:
> 
>     (open-gnutls-stream name buffer host service &optional parameters)
> 
>     then adjust the single in-core caller and have open-gnutls-stream
>     call plist-get.
> 
>     Does anyone see any issue with doing this?

How about changing the code so that PARAMETERS could also be a symbol,
in which case it would be interpreted in backward-compatible way?

>     Again Iʼm tempted to change open-network-stream such that not
>     specifying :client-certificate is the same as specifying t, so
>     that all Emacs core and external packages can take advantage of
>     the feature just by adjusting their .authinfo entries, similarly
>     to how password lookup automatically works today. However, this
>     would be a change in default behaviour, plus I know some people
>     are very sensitive to changes in this particular area, so I
>     thought Iʼd ask here before doing anything.

IMO, we should resist the temptation of making backward-incompatible
changes.  From bitter experience, even obscure internal functions are
sometimes used, and their users don't expect us to break the APIs.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Wed, 02 Jan 2019 17:49:13 +0100
>> 
>> 1.  As part of that fix, Iʼd like to change the signature of
>>     open-gnutls-stream from
>> 
>>     (open-gnutls-stream name buffer host service &optional nowait)
>> 
>>     to
>> 
>>     (open-gnutls-stream name buffer host service &optional nowait parameters)
>> 
>>     Normally this would be fine, I think, except that the caller in
>>     Emacs core derives the value of 'nowait' from the plist that would be
>>     passed in via 'parameters' anyway, so Iʼm tempted to just change it
>>     to:
>> 
>>     (open-gnutls-stream name buffer host service &optional parameters)
>> 
>>     then adjust the single in-core caller and have open-gnutls-stream
>>     call plist-get.
>> 
>>     Does anyone see any issue with doing this?
>
> How about changing the code so that PARAMETERS could also be a symbol,
> in which case it would be interpreted in backward-compatible way?

You mean like in my footnote 2? :-)

So nil/t would mean :nowait nil/t, and anything else would be a
plist. Itʼs not perfect, but I guess backwards compatibility is
important.

>>     Again Iʼm tempted to change open-network-stream such that not
>>     specifying :client-certificate is the same as specifying t, so
>>     that all Emacs core and external packages can take advantage of
>>     the feature just by adjusting their .authinfo entries, similarly
>>     to how password lookup automatically works today. However, this
>>     would be a change in default behaviour, plus I know some people
>>     are very sensitive to changes in this particular area, so I
>>     thought Iʼd ask here before doing anything.
>
> IMO, we should resist the temptation of making backward-incompatible
> changes.  From bitter experience, even obscure internal functions are
> sometimes used, and their users don't expect us to break the APIs.

That applies to open-gnutls-stream, but I was asking about
open-network-stream. For people who have no client certificate entries
in their auth-source, there would be zero difference. Judging by the
lack of documentation in this area, I donʼt think many people have
such entries, and they almost certainly already have workarounds in
place for the current situation.

If we donʼt change open-network-stream, then I was planning on
changing all callers in Emacs to use :client-certificate t in any
case, so only external users of open-network-stream would need to
update their code to enable automatic use of client certificates. Itʼs
those external updates I was hoping to avoid.

Robert

Robert

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 02 Jan 2019 18:47:55 +0100
> 
> So nil/t would mean :nowait nil/t, and anything else would be a
> plist.

Not exactly.  What I meant is if that arg is _any_ symbol, it is
interpreted as the old NOWAIT argument; otherwise, it has to be a
plist (if not, we signal an error).

> Itʼs not perfect, but I guess backwards compatibility is important.

It is IMO.

> >>     Again Iʼm tempted to change open-network-stream such that not
> >>     specifying :client-certificate is the same as specifying t, so
> >>     that all Emacs core and external packages can take advantage of
> >>     the feature just by adjusting their .authinfo entries, similarly
> >>     to how password lookup automatically works today. However, this
> >>     would be a change in default behaviour, plus I know some people
> >>     are very sensitive to changes in this particular area, so I
> >>     thought Iʼd ask here before doing anything.
> >
> > IMO, we should resist the temptation of making backward-incompatible
> > changes.  From bitter experience, even obscure internal functions are
> > sometimes used, and their users don't expect us to break the APIs.
> 
> That applies to open-gnutls-stream, but I was asking about
> open-network-stream. For people who have no client certificate entries
> in their auth-source, there would be zero difference.

Then perhaps I misunderstand your suggestion.  Please tell more.  (And
I'm talking about those for whom this change _will_ mean some
difference, not those who don't use :client-certificate.)

I guess the part which is confusing me is this:

  change open-network-stream such that not specifying
  :client-certificate is the same as specifying t

Doesn't this mean an incompatible change in behavior?

> If we donʼt change open-network-stream, then I was planning on
> changing all callers in Emacs to use :client-certificate t in any
> case, so only external users of open-network-stream would need to
> update their code to enable automatic use of client certificates. Itʼs
> those external updates I was hoping to avoid.

Now I'm even more confused.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Wed, 02 Jan 2019 18:47:55 +0100
>> 
>> So nil/t would mean :nowait nil/t, and anything else would be a
>> plist.
>
> Not exactly.  What I meant is if that arg is _any_ symbol, it is
> interpreted as the old NOWAIT argument; otherwise, it has to be a
> plist (if not, we signal an error).
>

OK, Iʼll make those changes.

>> That applies to open-gnutls-stream, but I was asking about
>> open-network-stream. For people who have no client certificate entries
>> in their auth-source, there would be zero difference.
>
> Then perhaps I misunderstand your suggestion.  Please tell more.  (And
> I'm talking about those for whom this change _will_ mean some
> difference, not those who don't use :client-certificate.)
>
> I guess the part which is confusing me is this:
>
>   change open-network-stream such that not specifying
>   :client-certificate is the same as specifying t
>
> Doesn't this mean an incompatible change in behavior?
>

For what I think is a tiny (perhaps non-existent) minority of Emacs
users. I certainly didnʼt know about this corner of .authinfo until
the bug report came in.

>> If we donʼt change open-network-stream, then I was planning on
>> changing all callers in Emacs to use :client-certificate t in any
>> case, so only external users of open-network-stream would need to
>> update their code to enable automatic use of client certificates. Itʼs
>> those external updates I was hoping to avoid.
>
> Now I'm even more confused.

Iʼm explaining badly.

Currently, specifying ':client-certificate t' to open-network-stream
causes client certificates to be looked up via auth-source up when
using external gnutls-cli.

My fix intends to extend that behaviour to do the same when using
built-in GnuTLS. So far I think thatʼs uncontroversial.

My follow-on to that fix would be to assume that not specifying
:client-certificate at all when calling open-network-stream is the
same as specifying ':client certificate t'. Only people with existing
client certificate entries in their auth-source provider (such as
.authinfo) would see a change in behaviour.

If that last change is unacceptable, Iʼd want to change all the
callers of open-network-stream inside Emacs to specify
':client-certificate t'. People using built-in Emacs packages would
then be able to use client certificates by changing configuration data
only. People using packages developed outside Emacs would need to
update those external packages to versions which specify
':client-certificate t', which is what Iʼd like to avoid.

Short version: we assume a username/password entry in auth-source for
a specific server means 'use this username/password', so we should do
the same for a client-certificate specification.

Robert

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 02 Jan 2019 19:56:25 +0100
> 
> >   change open-network-stream such that not specifying
> >   :client-certificate is the same as specifying t
> >
> > Doesn't this mean an incompatible change in behavior?
> 
> For what I think is a tiny (perhaps non-existent) minority of Emacs
> users.

We have no reliable way of estimating the number of affected users.
Besides, even if there's only a single user affected, it requires us
to consider this an incompatible change, which should be avoided if
possible.

> Currently, specifying ':client-certificate t' to open-network-stream
> causes client certificates to be looked up via auth-source up when
> using external gnutls-cli.
> 
> My fix intends to extend that behaviour to do the same when using
> built-in GnuTLS. So far I think thatʼs uncontroversial.

Yes.

> My follow-on to that fix would be to assume that not specifying
> :client-certificate at all when calling open-network-stream is the
> same as specifying ':client certificate t'. Only people with existing
> client certificate entries in their auth-source provider (such as
> .authinfo) would see a change in behaviour.

It's still a change.  At the very least, we should provide a way to
have the old behavior back.

> If that last change is unacceptable, Iʼd want to change all the
> callers of open-network-stream inside Emacs to specify
> ':client-certificate t'. People using built-in Emacs packages would
> then be able to use client certificates by changing configuration data
> only. People using packages developed outside Emacs would need to
> update those external packages to versions which specify
> ':client-certificate t', which is what Iʼd like to avoid.
> 
> Short version: we assume a username/password entry in auth-source for
> a specific server means 'use this username/password', so we should do
> the same for a client-certificate specification.

I don't necessarily disagree, provided that we give users who
want/need that a way to get back the old behavior.  Maybe I'm
misunderstanding, but it sounds like both of your alternatives are
backward-incompatible, so they both need such a "fire escape".

Thanks.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Wed, 02 Jan 2019 19:56:25 +0100
>> 
>> >   change open-network-stream such that not specifying
>> >   :client-certificate is the same as specifying t
>> >
>> > Doesn't this mean an incompatible change in behavior?
>> 
>> For what I think is a tiny (perhaps non-existent) minority of Emacs
>> users.
>
> We have no reliable way of estimating the number of affected users.
> Besides, even if there's only a single user affected, it requires us
> to consider this an incompatible change, which should be avoided if
> possible.
>

OK. We can check back in a year to see how many people have changed
the configuration variable below from the default.

>
>> My follow-on to that fix would be to assume that not specifying
>> :client-certificate at all when calling open-network-stream is the
>> same as specifying ':client certificate t'. Only people with existing
>> client certificate entries in their auth-source provider (such as
>> .authinfo) would see a change in behaviour.
>
> It's still a change.  At the very least, we should provide a way to
> have the old behavior back.
>

See below

>> If that last change is unacceptable, Iʼd want to change all the
>> callers of open-network-stream inside Emacs to specify
>> ':client-certificate t'. People using built-in Emacs packages would
>> then be able to use client certificates by changing configuration data
>> only. People using packages developed outside Emacs would need to
>> update those external packages to versions which specify
>> ':client-certificate t', which is what Iʼd like to avoid.
>> 
>> Short version: we assume a username/password entry in auth-source for
>> a specific server means 'use this username/password', so we should do
>> the same for a client-certificate specification.
>
> I don't necessarily disagree, provided that we give users who
> want/need that a way to get back the old behavior.  Maybe I'm
> misunderstanding, but it sounds like both of your alternatives are
> backward-incompatible, so they both need such a "fire escape".

How about:

(defcustom network-stream-use-client-certificates
"Whether to use client certificates for network connections.

If set to the default value t, `open-network-stream' will
automatically look for matching client certificates (via
'auth-source') for a destination server, but only if it is called
without a :client-certificate keyword.

Set to nil to disable this lookup globally.  To disable on a
per-connection basis specify ':client-certificate nil' when
calling `open-network-stream'.

        :type '(choice (const t :tag "Always")
        (const nil :tag "Never")))

(Now to find which custom group this goes in...)

Robert

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 04 Jan 2019 23:29:17 +0100
> 
> (defcustom network-stream-use-client-certificates
> "Whether to use client certificates for network connections.
> 
> If set to the default value t, `open-network-stream' will
> automatically look for matching client certificates (via
> 'auth-source') for a destination server, but only if it is called
> without a :client-certificate keyword.

That "only if" is inaccurate, right?  In particular, using non-nil
:client-certificate attribute will still use client certificates as
directed, right?

> Set to nil to disable this lookup globally.  To disable on a
> per-connection basis specify ':client-certificate nil' when
> calling `open-network-stream'.

Sounds OK to me, provided we announce this in NEWS.

> (Now to find which custom group this goes in...)

You could make a new group, say, 'network'.

Thanks.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Fri, 04 Jan 2019 23:29:17 +0100
>> 
>> (defcustom network-stream-use-client-certificates
>> "Whether to use client certificates for network connections.
>> 
>> If set to the default value t, `open-network-stream' will
>> automatically look for matching client certificates (via
>> 'auth-source') for a destination server, but only if it is called
>> without a :client-certificate keyword.
>
> That "only if" is inaccurate, right?  In particular, using non-nil
> :client-certificate attribute will still use client certificates as
> directed, right?
>

True. How about:

   .  It will not override any caller-supplied :client-certificate
   specification.


>> Set to nil to disable this lookup globally.  To disable on a
>> per-connection basis specify ':client-certificate nil' when
>> calling `open-network-stream'.
>
> Sounds OK to me, provided we announce this in NEWS.

Nah, we'll spring it on the users unannounced and see what happens :-)

>
>> (Now to find which custom group this goes in...)
>
> You could make a new group, say, 'network'.

'network' already exists, but itʼs only got one member. Iʼll put it
there.

Robert

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 05 Jan 2019 08:36:38 +0100
> 
> >> If set to the default value t, `open-network-stream' will
> >> automatically look for matching client certificates (via
> >> 'auth-source') for a destination server, but only if it is called
> >> without a :client-certificate keyword.
> >
> > That "only if" is inaccurate, right?  In particular, using non-nil
> > :client-certificate attribute will still use client certificates as
> > directed, right?
> >
> 
> True. How about:
> 
>    .  It will not override any caller-supplied :client-certificate
>    specification.

I'd just drop the "but only" part.

> > You could make a new group, say, 'network'.
> 
> 'network' already exists, but itʼs only got one member. Iʼll put it
> there.

Thanks.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

>> Sounds OK to me, provided we announce this in NEWS.
>
> Nah, we'll spring it on the users unannounced and see what happens :-)

Proposed changes, on top of the proposed patch for Bug#33780.

diff --git i/doc/lispref/processes.texi w/doc/lispref/processes.texi
index 72b164c5d4..487dff76d1 100644
--- i/doc/lispref/processes.texi
+++ w/doc/lispref/processes.texi
@@ -2457,8 +2457,11 @@ Network
 Either a list of the form @code{(@var{key-file} @var{cert-file})},
 naming the certificate key file and certificate file itself, or
 @code{t}, meaning to query @code{auth-source} for this information
-(@pxref{Top,,Overview, auth, The Auth-Source Manual}).
-Only used for @acronym{TLS} or @acronym{STARTTLS}.
+(@pxref{Help for users,,auth-source, auth, Emacs auth-source Library}).
+Only used for @acronym{TLS} or @acronym{STARTTLS}.  If
+@code{:client-certificate} is not specified, behave as if it were t,
+customize @code{network-stream-use-client-certificates} to change
+this.
 
 @item :return-list @var{cons-or-nil}
 The return value of this function.  If omitted or @code{nil}, return a
diff --git i/etc/NEWS w/etc/NEWS
index 43997f8418..be62d3803e 100644
--- i/etc/NEWS
+++ w/etc/NEWS
@@ -206,6 +206,12 @@ gnutls-cli command.  Call 'open-network-stream' with
 ':client-certificate t' to trigger looking up of per-server
 certificates via 'auth-source'.
 
++++
+** New user option 'network-stream-use-client-certificates'.
+When non-nil, 'open-network-stream' performs lookups of client
+certificates using 'auth-source' as if ':client-certificate t' were
+specified.  Defaults to t.
+
 +++
 ** New function 'fill-polish-nobreak-p', to be used in 'fill-nobreak-predicate'.
 It blocks line breaking after a one-letter word, also in the case when
diff --git i/lisp/net/network-stream.el w/lisp/net/network-stream.el
index 1723931c67..53827bcefb 100644
--- i/lisp/net/network-stream.el
+++ w/lisp/net/network-stream.el
@@ -57,6 +57,21 @@ starttls-use-gnutls
 (defvar starttls-gnutls-program)
 (defvar starttls-program)
 
+(defcustom network-stream-use-client-certificates t
+  "Whether to use client certificates for network connections.
+
+When non-nil, `open-network-stream' will automatically look for
+matching client certificates (via 'auth-source') for a
+destination server, if it is called without a :client-certificate
+keyword.  Default is t.
+
+Set to nil to disable this lookup globally.  To disable on a
+per-connection basis, specify ':client-certificate nil' when
+calling `open-network-stream'."
+  :group 'network
+  :type '(choice (const t)
+                 (const nil)))
+
 ;;;###autoload
 (defun open-network-stream (name buffer host service &rest parameters)
   "Open a TCP connection to HOST, optionally with encryption.
@@ -128,10 +143,12 @@ open-network-stream
 
 :client-certificate should either be a list where the first
   element is the certificate key file name, and the second
-  element is the certificate file name itself, or t, which
-  means that `auth-source' will be queried for the key and the
+  element is the certificate file name itself, or t, which means
+  that `auth-source' will be queried for the key and the
   certificate.  This parameter will only be used when doing TLS
-  or STARTTLS connections.
+  or STARTTLS connections.  If :client-certificate is not
+  specified, behave as if it were t, customize
+  `network-stream-use-client-certificates' to change this.
 
 :use-starttls-if-possible is a boolean that says to do opportunistic
 STARTTLS upgrades even if Emacs doesn't have built-in TLS functionality.
@@ -180,6 +197,11 @@ open-network-stream
 		       ((memq type '(tls ssl)) 'network-stream-open-tls)
 		       ((eq type 'shell) 'network-stream-open-shell)
 		       (t (error "Invalid connection type %s" type))))
+            (parameters
+               (if (and network-stream-use-client-certificates
+                        (not (plist-member parameters :client-certificate)))
+                   (plist-put parameters :client-certificate t)
+                 parameters))
 	    result)
 	(unwind-protect
 	    (setq result (funcall fun name work-buffer host service parameters))
diff --git i/test/lisp/net/gnutls-tests.el w/test/lisp/net/gnutls-tests.el
index ea8dd7eb66..26f662aa69 100644
--- i/test/lisp/net/gnutls-tests.el
+++ w/test/lisp/net/gnutls-tests.el
@@ -29,6 +29,7 @@
 (require 'cl-lib)
 (require 'gnutls)
 (require 'hex-util)
+(require 'network-stream)
 
 (defvar gnutls-tests-message-prefix "")
 
@@ -291,5 +292,99 @@ gnutls-tests-pad-to-multiple
                   (should-not (gnutls-tests-hexstring-equal data reverse))
                   (should (gnutls-tests-hexstring-equal input reverse)))))))))))
 
+(defconst network-stream-tests--datadir
+  (expand-file-name "test/data/net" source-directory))
+
+(defun make-tls-server (port)
+  (start-process "gnutls" (generate-new-buffer "*tls*")
+                 "gnutls-serv" "--http"
+                 "--x509keyfile"
+                 (concat network-stream-tests--datadir "/key.pem")
+                 "--x509certfile"
+                 (concat network-stream-tests--datadir "/cert.pem")
+                 "--port" (format "%s" port)))
+
+(ert-deftest test-gnutls-006-open-network-stream-tls-wait ()
+  (skip-unless (executable-find "gnutls-serv"))
+  (skip-unless (gnutls-available-p))
+  (let ((server (make-tls-server 44333))
+        (times 0)
+        (network-security-level 'low)
+        proc status)
+    (unwind-protect
+        (progn
+          (sleep-for 1)
+          (with-current-buffer (process-buffer server)
+            (message "gnutls-serv: %s" (buffer-string)))
+
+          ;; It takes a while for gnutls-serv to start.
+          (while (and (null (ignore-errors
+                              (setq proc (open-network-stream
+                                          "bar"
+                                          (generate-new-buffer "*foo*")
+                                          "localhost"
+                                          44333
+                                          :type 'tls
+                                          :nowait nil))))
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (should proc))
+      (if (process-live-p server) (delete-process server)))
+    (setq status (gnutls-peer-status proc))
+    (should (consp status))
+    (delete-process proc)
+    ;; This sleep-for is needed for the native MS-Windows build.  If
+    ;; it is removed, the next test mysteriously fails because the
+    ;; initial part of the echo is not received.
+    (sleep-for 0.1)
+    (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
+      (should (stringp issuer))
+      (setq issuer (split-string issuer ","))
+      (should (equal (nth 3 issuer) "O=Emacs Test Servicess LLC")))))
+
+(ert-deftest test-gnutls-007-open-network-stream-tls-nowait ()
+  (skip-unless (executable-find "gnutls-serv"))
+  (skip-unless (gnutls-available-p))
+  (let ((server (make-tls-server 44334))
+        (times 0)
+        (network-security-level 'low)
+        proc status)
+    (unwind-protect
+        (progn
+          (sleep-for 1)
+          (with-current-buffer (process-buffer server)
+            (message "gnutls-serv: %s" (buffer-string)))
+
+          ;; It takes a while for gnutls-serv to start.
+          (while (and (null (ignore-errors
+                              (setq proc (open-network-stream
+                                          "bar"
+                                          (generate-new-buffer "*foo*")
+                                          "localhost"
+                                          44334
+                                          :type 'tls
+                                          :nowait t))))
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (should proc)
+          (setq times 0)
+          (while (and (eq (process-status proc) 'connect)
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (skip-unless (not (eq (process-status proc) 'connect))))
+      (if (process-live-p server) (delete-process server)))
+    (setq status (gnutls-peer-status proc))
+    (message "status %s" status)
+    (should (consp status))
+    (delete-process proc)
+    ;; This sleep-for is needed for the native MS-Windows build.  If
+    ;; it is removed, the next test mysteriously fails because the
+    ;; initial part of the echo is not received.
+    (sleep-for 0.1)
+    (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
+      (should (stringp issuer))
+      (setq issuer (split-string issuer ","))
+      (should (equal (nth 3 issuer) "O=Emacs Test Servicess LLC")))))
+
 (provide 'gnutls-tests)
 ;;; gnutls-tests.el ends here
diff --git i/test/lisp/net/network-stream-tests.el w/test/lisp/net/network-stream-tests.el
index 29b92da3de..b68a99869b 100644
--- i/test/lisp/net/network-stream-tests.el
+++ w/test/lisp/net/network-stream-tests.el
@@ -25,6 +25,11 @@
 ;;; Code:
 
 (require 'gnutls)
+(require 'network-stream)
+; The require above is needed for 'open-network-stream', but it pulls
+; in nsm, which then makes the :nowait tests fail unless we disable
+; the nsm.
+(setq network-security-level 'low)
 
 (ert-deftest make-local-unix-server ()
   (skip-unless (featurep 'make-network-process '(:family local)))
@@ -294,4 +299,83 @@ make-tls-server
       (setq issuer (split-string issuer ","))
       (should (equal (nth 3 issuer) "O=Emacs Test Servicess LLC")))))
 
+(ert-deftest open-network-stream-tls-wait ()
+  (skip-unless (executable-find "gnutls-serv"))
+  (skip-unless (gnutls-available-p))
+  (let ((server (make-tls-server 44334))
+        (times 0)
+        proc status)
+    (unwind-protect
+        (progn
+          (sleep-for 1)
+          (with-current-buffer (process-buffer server)
+            (message "gnutls-serv: %s" (buffer-string)))
+
+          ;; It takes a while for gnutls-serv to start.
+          (while (and (null (ignore-errors
+                              (setq proc (open-network-stream
+                                          "bar"
+                                          (generate-new-buffer "*foo*")
+                                          "localhost"
+                                          44334
+                                          :type 'tls
+                                          :nowait nil))))
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (should proc))
+      (if (process-live-p server) (delete-process server)))
+    (setq status (gnutls-peer-status proc))
+    (should (consp status))
+    (delete-process proc)
+    ;; This sleep-for is needed for the native MS-Windows build.  If
+    ;; it is removed, the next test mysteriously fails because the
+    ;; initial part of the echo is not received.
+    (sleep-for 0.1)
+    (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
+      (should (stringp issuer))
+      (setq issuer (split-string issuer ","))
+      (should (equal (nth 3 issuer) "O=Emacs Test Servicess LLC")))))
+
+(ert-deftest open-network-stream-tls-nowait ()
+  (skip-unless (executable-find "gnutls-serv"))
+  (skip-unless (gnutls-available-p))
+  (let ((server (make-tls-server 44335))
+        (times 0)
+        proc status)
+    (unwind-protect
+        (progn
+          (sleep-for 1)
+          (with-current-buffer (process-buffer server)
+            (message "gnutls-serv: %s" (buffer-string)))
+
+          ;; It takes a while for gnutls-serv to start.
+          (while (and (null (ignore-errors
+                              (setq proc (open-network-stream
+                                          "bar"
+                                          (generate-new-buffer "*foo*")
+                                          "localhost"
+                                          44335
+                                          :type 'tls
+                                          :nowait t))))
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (should proc)
+          (setq times 0)
+          (while (and (eq (process-status proc) 'connect)
+                      (< (setq times (1+ times)) 10))
+            (sit-for 0.1))
+          (skip-unless (not (eq (process-status proc) 'connect))))
+      (if (process-live-p server) (delete-process server)))
+    (setq status (gnutls-peer-status proc))
+    (should (consp status))
+    (delete-process proc)
+    ;; This sleep-for is needed for the native MS-Windows build.  If
+    ;; it is removed, the next test mysteriously fails because the
+    ;; initial part of the echo is not received.
+    (sleep-for 0.1)
+    (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
+      (should (stringp issuer))
+      (setq issuer (split-string issuer ","))
+      (should (equal (nth 3 issuer) "O=Emacs Test Servicess LLC")))))
+
 ;;; network-stream-tests.el ends here

Re: open-{gnutls,network}-stream backwards compatibility

[Ted Zlatanov]

On Wed, 09 Jan 2019 22:50:13 +0100 Robert Pluim <rpluim@gmail.com> wrote: 

RP> Proposed changes, on top of the proposed patch for Bug#33780.

OK with me, and thank you for working on this. Eli?

Ted

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Date: Thu, 10 Jan 2019 15:53:51 +0000
> 
> On Wed, 09 Jan 2019 22:50:13 +0100 Robert Pluim <rpluim@gmail.com> wrote: 
> 
> RP> Proposed changes, on top of the proposed patch for Bug#33780.
> 
> OK with me, and thank you for working on this. Eli?

I didn't yet have time to look at the changes.  Will do soon.

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 09 Jan 2019 22:50:13 +0100
> 
> Robert Pluim <rpluim@gmail.com> writes:
> 
> >> Sounds OK to me, provided we announce this in NEWS.
> >
> > Nah, we'll spring it on the users unannounced and see what happens :-)
> 
> Proposed changes, on top of the proposed patch for Bug#33780.

Both LGTM, with the minor comments fixed.

> +(defcustom network-stream-use-client-certificates t
> +  "Whether to use client certificates for network connections.

Please add a :version tag to this defcustom.

> +When non-nil, `open-network-stream' will automatically look for
> +matching client certificates (via 'auth-source') for a
> +destination server, if it is called without a :client-certificate
> +keyword.  Default is t.
             ^^^^^^^^^^^^
No need to state that, as Emacs knows that automatically.

Thanks.

Re: open-{gnutls,network}-stream backwards compatibility

[Ted Zlatanov]

On Wed, 02 Jan 2019 18:47:55 +0100 Robert Pluim <rpluim@gmail.com> wrote: 

RP> So nil/t would mean :nowait nil/t, and anything else would be a
RP> plist. Itʼs not perfect, but I guess backwards compatibility is
RP> important.

I like that approach as well, noting also that the `open-gnutls-stream' docs say

  This is a very simple wrapper around `gnutls-negotiate'

which in turn takes a plist. So maybe the callers should just call
`gnutls-negotiate', and `open-gnutls-stream' should be deprecated
because it has failed to be a simple wrapper :)

Ted

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Date: Wed, 02 Jan 2019 18:12:22 +0000
> 
> On Wed, 02 Jan 2019 18:47:55 +0100 Robert Pluim <rpluim@gmail.com> wrote: 
> 
> RP> So nil/t would mean :nowait nil/t, and anything else would be a
> RP> plist. Itʼs not perfect, but I guess backwards compatibility is
> RP> important.
> 
> I like that approach as well, noting also that the `open-gnutls-stream' docs say
> 
>   This is a very simple wrapper around `gnutls-negotiate'
> 
> which in turn takes a plist. So maybe the callers should just call
> `gnutls-negotiate', and `open-gnutls-stream' should be deprecated
> because it has failed to be a simple wrapper :)

If we can make the change backward-compatible, there's no reason to
deprecate it.

Re: open-{gnutls,network}-stream backwards compatibility

[Ted Zlatanov]

On Wed, 02 Jan 2019 20:30:22 +0200 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Ted Zlatanov <tzz@lifelogs.com>

>> I like that approach as well, noting also that the `open-gnutls-stream' docs say
>> 
>> This is a very simple wrapper around `gnutls-negotiate'
>> 
>> which in turn takes a plist. So maybe the callers should just call
>> `gnutls-negotiate', and `open-gnutls-stream' should be deprecated
>> because it has failed to be a simple wrapper :)

EZ> If we can make the change backward-compatible, there's no reason to
EZ> deprecate it.

As the author, I think there is: the function is used in one place and
doesn't fit the use cases well. All the existing usage goes through
`open-network-stream'. So it's a layer of indirection with one user,
that removes API features in a way that's hard to work around. I think
it's better to remove the source of the problem than to patch it.

Ted

Re: open-{gnutls,network}-stream backwards compatibility

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 02 Jan 2019 19:18:26 +0000
> 
> EZ> If we can make the change backward-compatible, there's no reason to
> EZ> deprecate it.
> 
> As the author

I appreciate your authorship, but in Emacs, once we release our code,
we no longer have much of control on it.  It's out in the open.

> I think there is: the function is used in one place and
> doesn't fit the use cases well. All the existing usage goes through
> `open-network-stream'. So it's a layer of indirection with one user,
> that removes API features in a way that's hard to work around. I think
> it's better to remove the source of the problem than to patch it.

I don't think I understand why this justifies deprecation of an API.
Deprecation is a nuisance: it chatters when you compile code, it nags.
We need a seriously good reason to impose that on unsuspecting users.

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ted Zlatanov <tzz@lifelogs.com>
>> Cc: emacs-devel@gnu.org
>> Date: Wed, 02 Jan 2019 19:18:26 +0000
>> 
>> EZ> If we can make the change backward-compatible, there's no reason to
>> EZ> deprecate it.
>> 
>> As the author
>
> I appreciate your authorship, but in Emacs, once we release our code,
> we no longer have much of control on it.  It's out in the open.
>
>> I think there is: the function is used in one place and
>> doesn't fit the use cases well. All the existing usage goes through
>> `open-network-stream'. So it's a layer of indirection with one user,
>> that removes API features in a way that's hard to work around. I think
>> it's better to remove the source of the problem than to patch it.
>
> I don't think I understand why this justifies deprecation of an API.
> Deprecation is a nuisance: it chatters when you compile code, it nags.
> We need a seriously good reason to impose that on unsuspecting users.

I see no need to deprecate it. It would be easy enough to leave it in
place, and move a (modified) version of it inline into
network-stream-open-tls. That would avoid any backward compatibility
issues as, and allows network-stream-open-tls free to add
functionality as needed.

Robert

Re: open-{gnutls,network}-stream backwards compatibility

[Ted Zlatanov]

On Wed, 02 Jan 2019 21:49:53 +0200 Eli Zaretskii <eliz@gnu.org> wrote: 

EZ> I appreciate your authorship, but in Emacs, once we release our code,
EZ> we no longer have much of control on it.  It's out in the open.

I wasn't claiming ownership, but rather pointing out what in retrospect
is not a good design because it hides useful options that need to be
passed through. This is, regardless of the owner, bad code.

The author IMO has a responsibility to recognize this situation.

EZ> Deprecation is a nuisance: it chatters when you compile code, it nags.
EZ> We need a seriously good reason to impose that on unsuspecting users.

There's only one user, is what I'm saying... `open-gnutls-stream' is
just not that popular, fortunately. So we can just kill it and the end
users won't notice.

I'm OK with Robert's suggestion to leave it in place but unused as well.
It's just that it seems like avoiding deprecation in name only.

Ted

Re: open-{gnutls,network}-stream backwards compatibility

[Robert Pluim]

Ted Zlatanov <tzz@lifelogs.com> writes:

> EZ> Deprecation is a nuisance: it chatters when you compile code, it nags.
> EZ> We need a seriously good reason to impose that on unsuspecting users.
>
> There's only one user, is what I'm saying... `open-gnutls-stream' is
> just not that popular, fortunately. So we can just kill it and the end
> users won't notice.

Youʼre sure thereʼs no code outside Emacs using it? If so, I can just
change its signature with reckless abandon ;-)

> I'm OK with Robert's suggestion to leave it in place but unused as well.
> It's just that it seems like avoiding deprecation in name only.

True. And Iʼd have to add an almost identical copy of it with a
different name (or copy its contents into network-stream.el, which
would then gain a dependency on the internals of gnutls.el which Iʼd
rather avoid)

Robert