Patchwork [09,of,11] sslutil: use CA loaded state to drive validation logic

login
register
mail settings
Submitter Gregory Szorc
Date May 5, 2016, 7:53 a.m.
Message ID <13bf97ebea3fb6d7c25b.1462434806@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/14902/
State Accepted
Headers show

Comments

Gregory Szorc - May 5, 2016, 7:53 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1462433898 25200
#      Thu May 05 00:38:18 2016 -0700
# Node ID 13bf97ebea3fb6d7c25b5b396ab2ed3e9948a02c
# Parent  bd7ebeaf995eaab11afe722a6fba61d9c7c0f3c4
sslutil: use CA loaded state to drive validation logic

Right now, sslkwargs may set web.cacerts=! to indicate
that system certs could not be found. This is really
obtuse because sslkwargs effectively sets state on a global
object which bypasses wrapsocket() and is later consulted
by validator.__call__. This is madness.

This patch introduces an attribute on the wrapped socket
instance indicating whether system CAs were loaded. We
can set this directly inside wrapsocket() because that
function knows everything that sslkwargs() does - and more.

With this attribute set on the socket, we refactor
validator.__call__ to use it.

Since we no longer have a need for setting web.cacerts=!
in sslkwargs, we remove that.

I think the new logic is much easier to understand and will
enable behavior to be changed more easily.
timeless - May 5, 2016, 8:31 a.m.
On Thu, May 5, 2016 at 3:53 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> Right now, sslkwargs may set web.cacerts=! to indicate

The style should be to document what the result of the commit is in
present tense.
So "Until now" would be better.

> Since we no longer have a need for setting web.cacerts=!
> in sslkwargs, we remove that.

Once you've fixed that, this second bit makes more sense.
Gregory Szorc - May 5, 2016, 9:26 p.m.
On Thu, May 5, 2016 at 1:31 AM, timeless <timeless@gmail.com> wrote:

> On Thu, May 5, 2016 at 3:53 AM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> > Right now, sslkwargs may set web.cacerts=! to indicate
>
> The style should be to document what the result of the commit is in
> present tense.
> So "Until now" would be better.
>
> > Since we no longer have a need for setting web.cacerts=!
> > in sslkwargs, we remove that.
>
> Once you've fixed that, this second bit makes more sense.
>

If this is the only thing wrong with the series, I'm going to hold off
sending a v2 so I don't spam inboxes. Perhaps the grammar police can
correct things in flight instead :)

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -150,26 +150,31 @@  def wrapsocket(sock, keyfile, certfile, 
     if certfile is not None:
         def password():
             f = keyfile or certfile
             return ui.getpass(_('passphrase for %s: ') % f, '')
         sslcontext.load_cert_chain(certfile, keyfile, password)
 
     if ca_certs is not None:
         sslcontext.load_verify_locations(cafile=ca_certs)
+        caloaded = True
     else:
         # This is a no-op on old Python.
         sslcontext.load_default_certs()
+        caloaded = _canloaddefaultcerts
 
     sslsocket = sslcontext.wrap_socket(sock, server_hostname=serverhostname)
     # check if wrap_socket failed silently because socket had been
     # closed
     # - see http://bugs.python.org/issue13721
     if not sslsocket.cipher():
         raise error.Abort(_('ssl connection failed'))
+
+    sslsocket._hgcaloaded = caloaded
+
     return sslsocket
 
 def _verifycert(cert, hostname):
     '''Verify that cert (in socket.getpeercert() format) matches hostname.
     CRLs is not handled.
 
     Returns error message if any problems are found and None on success.
     '''
@@ -275,22 +280,16 @@  def sslkwargs(ui, host):
                     '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
 
-    # This is effectively indicating that no CAs can be loaded because
-    # we can't get here if web.cacerts is set or if we can find
-    # CA certs elsewhere. Using a config option (which is later
-    # consulted by validator.__call__ is not very obvious).
-    # FUTURE fix this
-    ui.setconfig('web', 'cacerts', '!', 'defaultcacerts')
     return kws
 
 class validator(object):
     def __init__(self, ui, host):
         self.ui = ui
         self.host = host
 
     def __call__(self, sock, strict=False):
@@ -337,28 +336,28 @@  class validator(object):
         # the same as below for BC.
         if self.ui.insecureconnections:
             self.ui.warn(_('warning: %s certificate with fingerprint %s not '
                            'verified (check hostfingerprints or web.cacerts '
                            'config setting)\n') %
                          (host, nicefingerprint))
             return
 
-        # No pinned fingerprint. Establish trust by looking at the CAs.
-        cacerts = self.ui.config('web', 'cacerts')
-        if cacerts != '!':
-            msg = _verifycert(peercert2, host)
-            if msg:
-                raise error.Abort(_('%s certificate error: %s') % (host, msg),
-                                 hint=_('configure hostfingerprint %s or use '
-                                        '--insecure to connect insecurely') %
-                                      nicefingerprint)
-            self.ui.debug('%s certificate successfully verified\n' % host)
-        elif strict:
-            raise error.Abort(_('%s certificate with fingerprint %s not '
-                               'verified') % (host, nicefingerprint),
-                             hint=_('check hostfingerprints or web.cacerts '
-                                     'config setting'))
-        else:
-            self.ui.warn(_('warning: %s certificate with fingerprint %s not '
-                           'verified (check hostfingerprints or web.cacerts '
-                           'config setting)\n') %
-                         (host, nicefingerprint))
+        if not sock._hgcaloaded:
+            if strict:
+                raise error.Abort(_('%s certificate with fingerprint %s not '
+                                    'verified') % (host, nicefingerprint),
+                                  hint=_('check hostfingerprints or '
+                                         'web.cacerts config setting'))
+            else:
+                self.ui.warn(_('warning: %s certificate with fingerprint %s '
+                               'not verified (check hostfingerprints or '
+                               'web.cacerts config setting)\n') %
+                             (host, nicefingerprint))
+
+            return
+
+        msg = _verifycert(peercert2, host)
+        if msg:
+            raise error.Abort(_('%s certificate error: %s') % (host, msg),
+                             hint=_('configure hostfingerprint %s or use '
+                                    '--insecure to connect insecurely') %
+                                  nicefingerprint)