Patchwork [2,of,3] sslutil: abort when unable to verify peer connection (BC)

login
register
mail settings
Submitter Gregory Szorc
Date June 25, 2016, 4:37 p.m.
Message ID <13818ab440e16c575b09.1466872634@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15613/
State Accepted
Headers show

Comments

Gregory Szorc - June 25, 2016, 4:37 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1466864803 25200
#      Sat Jun 25 07:26:43 2016 -0700
# Node ID 13818ab440e16c575b09a6e4583f9a17550a6e52
# Parent  3e53df5526513432e4c8eba52fb846062749c2bc
sslutil: abort when unable to verify peer connection (BC)

Previously, when we connected to a server and were unable to verify
its certificate against a trusted certificate authority we would
issue a warning and continue to connect. This is obviously not
great behavior because the x509 certificate model is based upon
trust of specific CAs. Failure to enforce that trust erodes security.
This behavior was defined several years ago when Python did not
support loading the system trusted CA store (Python 2.7.9's
backports of Python 3's improvements to the "ssl" module enabled
this).

This commit changes behavior when connecting to abort if the peer
certificate can't be validated. With an empty/default Mercurial
configuration, the peer certificate can be validated if Python is
able to load the system trusted CA store. Environments able to load
the system trusted CA store include:

* Python 2.7.9+ on most platforms and installations
* Python 2.7 distributions with a modern ssl module (e.g. RHEL7's
  patched 2.7.5 package)
* Python shipped on OS X

Environments unable to load the system trusted CA store include:

* Python 2.6
* Python 2.7 on many existing Linux installs (because they don't
  ship 2.7.9+ or haven't backported modern ssl module)
* Python 2.7.9+ on some installs where Python is unable to locate
  the system CA store (this is hopefully rare)

Users of these Pythongs will need to configure Mercurial to load the
system CA store using web.cacerts. This should ideally be performed
by packagers (by setting web.cacerts in the global/system hgrc file).
Where Mercurial packagers aren't setting this, the linked URL in the
new abort message can contain instructions for users.

In the future, we may want to add more code for finding the system
CA store. For example, many Linux distributions have the CA store
at well-known locations (such as /etc/ssl/certs/ca-certificates.crt
in the case of Ubuntu). This will enable CA loading to "just work"
on more Python configurations and will be best for our users since
they won't have to change anything after upgrading to a Mercurial
with this patch.

We may also want to consider distributing a trusted CA store with
Mercurial. Although we should think long and hard about that because
most systems have a global CA store and Mercurial should almost
certainly use the same store used by everything else on the system.
Pierre-Yves David - June 26, 2016, 2:02 a.m.
On 06/25/2016 06:37 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1466864803 25200
> #      Sat Jun 25 07:26:43 2016 -0700
> # Node ID 13818ab440e16c575b09a6e4583f9a17550a6e52
> # Parent  3e53df5526513432e4c8eba52fb846062749c2bc
> sslutil: abort when unable to verify peer connection (BC)

Patches 1 and 2 pushed. Thanks for cleaning up all this.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -202,18 +202,19 @@  def _hostsettings(ui, hostname):
 
         # Require certificate validation if CA certs are being loaded and
         # verification hasn't been disabled above.
         if cafile or (_canloaddefaultcerts and s['allowloaddefaultcerts']):
             s['verifymode'] = ssl.CERT_REQUIRED
         else:
             # At this point we don't have a fingerprint, aren't being
             # explicitly insecure, and can't load CA certs. Connecting
-            # at this point is insecure. But we do it for BC reasons.
-            # TODO abort here to make secure by default.
+            # is insecure. We allow the connection and abort during
+            # validation (once we have the fingerprint to print to the
+            # user).
             s['verifymode'] = ssl.CERT_NONE
 
     assert s['verifymode'] is not None
 
     return s
 
 def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
     """Add SSL/TLS to a socket.
@@ -408,21 +409,26 @@  def validatesocket(sock):
             nice = fmtfingerprint(peerfingerprints['sha1'])
         else:
             section = 'hostsecurity'
             nice = '%s:%s' % (hash, fmtfingerprint(peerfingerprints[hash]))
         raise error.Abort(_('certificate for %s has unexpected '
                             'fingerprint %s') % (host, nice),
                           hint=_('check %s configuration') % section)
 
+    # Security is enabled but no CAs are loaded. We can't establish trust
+    # for the cert so abort.
     if not sock._hgstate['caloaded']:
-        ui.warn(_('warning: certificate for %s not verified '
-                  '(set hostsecurity.%s:certfingerprints=%s or web.cacerts '
-                  'config settings)\n') % (host, host, nicefingerprint))
-        return
+        raise error.Abort(
+            _('unable to verify security of %s (no loaded CA certificates); '
+              'refusing to connect') % host,
+            hint=_('see https://mercurial-scm.org/wiki/SecureConnections for '
+                   'how to configure Mercurial to avoid this error or set '
+                   'hostsecurity.%s:fingerprints=%s to trust this server') %
+                   (host, nicefingerprint))
 
     msg = _verifycert(peercert2, host)
     if msg:
         raise error.Abort(_('%s certificate error: %s') % (host, msg),
                          hint=_('set hostsecurity.%s:certfingerprints=%s '
                                 'config setting or use --insecure to connect '
                                 'insecurely') %
                               (host, nicefingerprint))
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -98,20 +98,25 @@  Defining both per-host certificate and a
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 4 changes to 4 files
 
   $ DISABLECACERTS="--config devel.disableloaddefaultcerts=true"
 
-clone via pull
+Inability to verify peer certificate will result in abort
 
   $ hg clone https://localhost:$HGPORT/ copy-pull $DISABLECACERTS
-  warning: certificate for localhost not verified (set hostsecurity.localhost:certfingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 or web.cacerts config settings)
+  abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  [255]
+
+  $ hg clone --insecure https://localhost:$HGPORT/ copy-pull
+  warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 4 changes to 4 files
   updating to branch default
   4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg verify -R copy-pull
@@ -128,17 +133,23 @@  clone via pull
 
 pull without cacert
 
   $ cd copy-pull
   $ echo '[hooks]' >> .hg/hgrc
   $ echo "changegroup = printenv.py changegroup" >> .hg/hgrc
   $ hg pull $DISABLECACERTS
   pulling from https://localhost:$HGPORT/
-  warning: certificate for localhost not verified (set hostsecurity.localhost:certfingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 or web.cacerts config settings)
+  abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  [255]
+
+  $ hg pull --insecure
+  pulling from https://localhost:$HGPORT/
+  warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
   changegroup hook: HG_NODE=5fed3813f7f5e1824344fdc9cf8f63bb662c292d HG_NODE_LAST=5fed3813f7f5e1824344fdc9cf8f63bb662c292d HG_SOURCE=pull HG_TXNID=TXN:* HG_URL=https://localhost:$HGPORT/ (glob)
   (run 'hg update' to get a working copy)
   $ cd ..
diff --git a/tests/test-patchbomb-tls.t b/tests/test-patchbomb-tls.t
--- a/tests/test-patchbomb-tls.t
+++ b/tests/test-patchbomb-tls.t
@@ -58,18 +58,19 @@  Without certificates:
 
   $ try --debug
   this patch series consists of 1 patches.
   
   
   (using smtps)
   sending mail: smtp host localhost, port * (glob)
   (verifying remote certificate)
-  warning: certificate for localhost not verified (set hostsecurity.localhost:certfingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 or web.cacerts config settings)
-  sending [PATCH] a ...
+  abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  [255]
 
 With global certificates:
 
   $ try --debug --config web.cacerts="$CERTSDIR/pub.pem"
   this patch series consists of 1 patches.
   
   
   (using smtps)