Patchwork [2,of,6] sslutil: move sslkwargs logic into internal function (API)

login
register
mail settings
Submitter Gregory Szorc
Date May 26, 2016, 3:03 a.m.
Message ID <f57cb70979b5399f7d55.1464231831@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15213/
State Accepted
Headers show

Comments

Gregory Szorc - May 26, 2016, 3:03 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1464231122 25200
#      Wed May 25 19:52:02 2016 -0700
# Node ID f57cb70979b5399f7d5543de268e6eb79fdcd51f
# Parent  62c63aafbff472307fcf4624d63d691f769b4fb2
sslutil: move sslkwargs logic into internal function (API)

As the previous commit documented, sslkwargs() doesn't add any
value since its return is treated as a black box and proxied
to wrapsocket().

We formalize its uselessness by moving its logic into a
new, internal function and make sslkwargs() return an empty
dict.

The certificate arguments that sslkwargs specified have been
removed from wrapsocket() because they should no longer be
set.
Yuya Nishihara - May 27, 2016, 2:52 p.m.
On Wed, 25 May 2016 20:03:51 -0700, Gregory Szorc wrote:
> +def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
>      """Add SSL/TLS to a socket.
>  
>      This is a glorified wrapper for ``ssl.wrap_socket()``. It makes sane
>      choices based on what security options are available.
>  
>      In addition to the arguments supported by ``ssl.wrap_socket``, we allow
>      the following additional arguments:
>  
>      * serverhostname - The expected hostname of the remote server. If the
>        server (and client) support SNI, this tells the server which certificate
>        to use.
>      """
>      if not serverhostname:
>          raise error.Abort('serverhostname argument is required')
>  
> +    cert_reqs, ca_certs = _determinecertoptions(ui, serverhostname)

I'm writing tests of mail.py, and found this patch seems to change the
behavior.

 hg 3.8.2, with web.cacerts=unknown-ca.pem:

   a) --insecure:             warning + patch sent
   b) smtp.verifycert=False:  no warning + patch sent
   c) smtp.verifycert=loose:  CERTIFICATE_VERIFY_FAILED
   d) smtp.verifycert=strict: CERTIFICATE_VERIFY_FAILED

 hg 9da137faaa9c, with web.cacerts=unknown-ca.pem:

   a) --insecure:             warning + patch sent
   b) smtp.verifycert=False:  CERTIFICATE_VERIFY_FAILED
   c) smtp.verifycert=loose:  CERTIFICATE_VERIFY_FAILED
   d) smtp.verifycert=strict: CERTIFICATE_VERIFY_FAILED

As of 3.8.2, verifycert=loose doesn't appear to agree with the doc if cacerts
is specified or modern Python used. verifycert=False is broken at the current
tip.

Do we still have to support smtp.verifycert in addition to --insecure option?
SSL without verification is utterly insecure.
Gregory Szorc - May 27, 2016, 3:06 p.m.
> On May 27, 2016, at 07:52, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Wed, 25 May 2016 20:03:51 -0700, Gregory Szorc wrote:
>> +def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
>>     """Add SSL/TLS to a socket.
>> 
>>     This is a glorified wrapper for ``ssl.wrap_socket()``. It makes sane
>>     choices based on what security options are available.
>> 
>>     In addition to the arguments supported by ``ssl.wrap_socket``, we allow
>>     the following additional arguments:
>> 
>>     * serverhostname - The expected hostname of the remote server. If the
>>       server (and client) support SNI, this tells the server which certificate
>>       to use.
>>     """
>>     if not serverhostname:
>>         raise error.Abort('serverhostname argument is required')
>> 
>> +    cert_reqs, ca_certs = _determinecertoptions(ui, serverhostname)
> 
> I'm writing tests of mail.py, and found this patch seems to change the
> behavior.
> 
> hg 3.8.2, with web.cacerts=unknown-ca.pem:
> 
>   a) --insecure:             warning + patch sent
>   b) smtp.verifycert=False:  no warning + patch sent
>   c) smtp.verifycert=loose:  CERTIFICATE_VERIFY_FAILED
>   d) smtp.verifycert=strict: CERTIFICATE_VERIFY_FAILED
> 
> hg 9da137faaa9c, with web.cacerts=unknown-ca.pem:
> 
>   a) --insecure:             warning + patch sent
>   b) smtp.verifycert=False:  CERTIFICATE_VERIFY_FAILED
>   c) smtp.verifycert=loose:  CERTIFICATE_VERIFY_FAILED
>   d) smtp.verifycert=strict: CERTIFICATE_VERIFY_FAILED
> 
> As of 3.8.2, verifycert=loose doesn't appear to agree with the doc if cacerts
> is specified or modern Python used. verifycert=False is broken at the current
> tip.
> 
> Do we still have to support smtp.verifycert in addition to --insecure option?
> SSL without verification is utterly insecure.

I was hoping to kill smtp.verifycert and replace with a newer, unified mechanism for host security. I was going to start those patches soon since the sslutil pre-work/refactor is mostly done.

I support a BC patch to drop smtp.verifycert. It will leave @ in a weird state until my stuff lands. But that should only last a week or two. You should be able to work around by defining cacerts or host fingerprints in the interim.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -101,33 +101,76 @@  except AttributeError:
                 'ca_certs': self._cacerts,
             }
 
             if self._supportsciphers:
                 args['ciphers'] = self._ciphers
 
             return ssl.wrap_socket(socket, **args)
 
-def wrapsocket(sock, keyfile, certfile, ui, cert_reqs=ssl.CERT_NONE,
-               ca_certs=None, serverhostname=None):
+def _determinecertoptions(ui, host):
+    """Determine certificate options for a connections.
+
+    Returns a tuple of (cert_reqs, ca_certs).
+    """
+    # If a host key fingerprint is on file, it is the only thing that matters
+    # and CA certs don't come into play.
+    hostfingerprint = ui.config('hostfingerprints', host)
+    if hostfingerprint:
+        return ssl.CERT_NONE, None
+
+    # The code below sets up CA verification arguments. If --insecure is
+    # used, we don't take CAs into consideration, so return early.
+    if ui.insecureconnections:
+        return ssl.CERT_NONE, None
+
+    cacerts = ui.config('web', 'cacerts')
+
+    # If a value is set in the config, validate against a path and load
+    # and require those certs.
+    if cacerts:
+        cacerts = util.expandpath(cacerts)
+        if not os.path.exists(cacerts):
+            raise error.Abort(_('could not find web.cacerts: %s') % cacerts)
+
+        return ssl.CERT_REQUIRED, cacerts
+
+    # No CAs in config. See if we can load defaults.
+    cacerts = _defaultcacerts()
+
+    # We found an alternate CA bundle to use. Load it.
+    if cacerts:
+        ui.debug('using %s to enable OS X system CA\n' % cacerts)
+        ui.setconfig('web', 'cacerts', cacerts, 'defaultcacerts')
+        return ssl.CERT_REQUIRED, cacerts
+
+    # FUTURE this can disappear once wrapsocket() is secure by default.
+    if _canloaddefaultcerts:
+        return ssl.CERT_REQUIRED, None
+
+    return ssl.CERT_NONE, None
+
+def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
     """Add SSL/TLS to a socket.
 
     This is a glorified wrapper for ``ssl.wrap_socket()``. It makes sane
     choices based on what security options are available.
 
     In addition to the arguments supported by ``ssl.wrap_socket``, we allow
     the following additional arguments:
 
     * serverhostname - The expected hostname of the remote server. If the
       server (and client) support SNI, this tells the server which certificate
       to use.
     """
     if not serverhostname:
         raise error.Abort('serverhostname argument is required')
 
+    cert_reqs, ca_certs = _determinecertoptions(ui, serverhostname)
+
     # Despite its name, PROTOCOL_SSLv23 selects the highest protocol
     # that both ends support, including TLS protocols. On legacy stacks,
     # the highest it likely goes in TLS 1.0. On modern stacks, it can
     # support TLS 1.2.
     #
     # The PROTOCOL_TLSv* constants select a specific TLS version
     # only (as opposed to multiple versions). So the method for
     # supporting multiple TLS versions is to use PROTOCOL_SSLv23 and
@@ -238,63 +281,17 @@  def _defaultcacerts():
     if _plainapplepython():
         dummycert = os.path.join(os.path.dirname(__file__), 'dummycert.pem')
         if os.path.exists(dummycert):
             return dummycert
 
     return None
 
 def sslkwargs(ui, host):
-    """Determine arguments to pass to wrapsocket().
-
-    ``host`` is the hostname being connected to.
-    """
-    kws = {}
-
-    # If a host key fingerprint is on file, it is the only thing that matters
-    # and CA certs don't come into play.
-    hostfingerprint = ui.config('hostfingerprints', host)
-    if hostfingerprint:
-        return kws
-
-    # The code below sets up CA verification arguments. If --insecure is
-    # used, we don't take CAs into consideration, so return early.
-    if ui.insecureconnections:
-        return kws
-
-    cacerts = ui.config('web', 'cacerts')
-
-    # If a value is set in the config, validate against a path and load
-    # and require those certs.
-    if cacerts:
-        cacerts = util.expandpath(cacerts)
-        if not os.path.exists(cacerts):
-            raise error.Abort(_('could not find web.cacerts: %s') % cacerts)
-
-        kws.update({'ca_certs': cacerts,
-                    'cert_reqs': ssl.CERT_REQUIRED})
-        return kws
-
-    # No CAs in config. See if we can load defaults.
-    cacerts = _defaultcacerts()
-
-    # We found an alternate CA bundle to use. Load it.
-    if cacerts:
-        ui.debug('using %s to enable OS X system CA\n' % cacerts)
-        ui.setconfig('web', 'cacerts', cacerts, 'defaultcacerts')
-        kws.update({'ca_certs': cacerts,
-                    'cert_reqs': ssl.CERT_REQUIRED})
-        return kws
-
-    # FUTURE this can disappear once wrapsocket() is secure by default.
-    if _canloaddefaultcerts:
-        kws['cert_reqs'] = ssl.CERT_REQUIRED
-        return kws
-
-    return kws
+    return {}
 
 def validatesocket(sock, strict=False):
     """Validate a socket meets security requiremnets.
 
     The passed socket must have been created with ``wrapsocket()``.
     """
     host = sock._hgstate['hostname']
     ui = sock._hgstate['ui']