Patchwork [5,of,9,V3] sslutil: move and change warning when cert verification is disabled

login
register
mail settings
Submitter Gregory Szorc
Date June 2, 2016, 3:16 a.m.
Message ID <a2294c4b56839b7afc0a.1464837379@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15331/
State Superseded
Delegated to: Yuya Nishihara
Headers show

Comments

Gregory Szorc - June 2, 2016, 3:16 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1464639353 25200
#      Mon May 30 13:15:53 2016 -0700
# Node ID a2294c4b56839b7afc0a3501890700b3fbc3de3c
# Parent  a2878fbb2db7d6bc273721dba95cf8a2ac159e3e
sslutil: move and change warning when cert verification is disabled

A short time ago, validatesocket() didn't know the reasons why
cert verification was disabled. Multiple code paths could lead
to cert verification being disabled. e.g. --insecure and lack
of loaded CAs.

With the recent refactorings to sslutil.py, we now know the reasons
behind security settings. This means we can recognize when the user
requested security be disabled (as opposed to being unable to provide
certificate verification due to lack of CAs).

This patch moves the check for certificate verification being disabled
and changes the wording to distinguish it from other states. The
warning message is purposefully more dangerous sounding in order
to help discourage people from disabling security outright.

We may want to add a URL or hint to this message. I'm going to wait
until additional changes to security defaults before committing to
something.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -349,16 +349,28 @@  def validatesocket(sock):
         peercert2 = sock.getpeercert()
     except AttributeError:
         raise error.Abort(_('%s ssl connection error') % host)
 
     if not peercert:
         raise error.Abort(_('%s certificate error: '
                            'no certificate received') % host)
 
+    if settings['disablecertverification']:
+        # We don't print the certificate fingerprint because it shouldn't
+        # be necessary: if the user requested certificate verification be
+        # disabled, they presumably already saw a message about the inability
+        # to verify the certificate and this message would have printed the
+        # fingerprint. So printing the fingerprint here adds little to no
+        # value.
+        ui.warn(_('warning: connection security to %s is disabled per current '
+                  'settings; communication is susceptible to eavesdropping '
+                  'and tampering\n') % host)
+        return
+
     # If a certificate fingerprint is pinned, use it and only it to
     # validate the remote cert.
     peerfingerprints = {
         'sha1': util.sha1(peercert).hexdigest(),
         'sha256': util.sha256(peercert).hexdigest(),
         'sha512': util.sha512(peercert).hexdigest(),
     }
     nicefingerprint = ':'.join([peerfingerprints['sha1'][x:x + 2]
@@ -378,29 +390,16 @@  def validatesocket(sock):
         if not fingerprintmatch:
             raise error.Abort(_('certificate for %s has unexpected '
                                'fingerprint %s') % (host, nicefingerprint),
                              hint=_('check %s configuration') % section)
         ui.debug('%s certificate matched fingerprint %s\n' %
                  (host, nicefingerprint))
         return
 
-    # If insecure connections were explicitly requested, print a warning
-    # and do no verification.
-    #
-    # It may seem odd that this is checked *after* host fingerprint pinning.
-    # This is for backwards compatibility (for now). The message is also
-    # the same as below for BC.
-    if settings['disablecertverification']:
-        ui.warn(_('warning: %s certificate with fingerprint %s not '
-                  'verified (check %s or web.cacerts '
-                  'config setting)\n') %
-                (host, nicefingerprint, section))
-        return
-
     if not sock._hgstate['caloaded']:
         ui.warn(_('warning: %s certificate with fingerprint %s '
                   'not verified (check %s or web.cacerts config '
                   'setting)\n') %
                 (host, nicefingerprint, section))
         return
 
     msg = _verifycert(peercert2, host)
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -230,39 +230,39 @@  variables in the filename
   $ echo "[web]" >> $HGRCPATH
   $ echo 'cacerts=$P/pub.pem' >> $HGRCPATH
   $ P=`pwd` hg -R copy-pull pull
   pulling from https://localhost:$HGPORT/
   searching for changes
   no changes found
   $ P=`pwd` hg -R copy-pull pull --insecure
   pulling from https://localhost:$HGPORT/
-  warning: localhost certificate with fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca not verified (check hostsecurity or web.cacerts config setting)
+  warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   no changes found
 
 cacert mismatch
 
   $ hg -R copy-pull pull --config web.cacerts=pub.pem https://127.0.0.1:$HGPORT/
   pulling from https://127.0.0.1:$HGPORT/
   abort: 127.0.0.1 certificate error: certificate is for localhost
   (configure hostsecurity 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca or use --insecure to connect insecurely)
   [255]
   $ hg -R copy-pull pull --config web.cacerts=pub.pem https://127.0.0.1:$HGPORT/ --insecure
   pulling from https://127.0.0.1:$HGPORT/
-  warning: 127.0.0.1 certificate with fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca not verified (check hostsecurity or web.cacerts config setting)
+  warning: connection security to 127.0.0.1 is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   no changes found
   $ hg -R copy-pull pull --config web.cacerts=pub-other.pem
   pulling from https://localhost:$HGPORT/
   abort: error: *certificate verify failed* (glob)
   [255]
   $ hg -R copy-pull pull --config web.cacerts=pub-other.pem --insecure
   pulling from https://localhost:$HGPORT/
-  warning: localhost certificate with fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca not verified (check hostsecurity or web.cacerts config setting)
+  warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   no changes found
 
 Test server cert which isn't valid yet
 
   $ hg serve -R test -p $HGPORT1 -d --pid-file=hg1.pid --certificate=server-not-yet.pem
   $ cat hg1.pid >> $DAEMON_PIDS
   $ hg -R copy-pull pull --config web.cacerts=pub-not-yet.pem https://localhost:$HGPORT1/
@@ -342,17 +342,17 @@  Prepare for connecting through proxy
   $ echo "always=True" >> copy-pull/.hg/hgrc
   $ echo "[hostfingerprints]" >> copy-pull/.hg/hgrc
   $ echo "localhost =" >> copy-pull/.hg/hgrc
 
 Test unvalidated https through proxy
 
   $ http_proxy=http://localhost:$HGPORT1/ hg -R copy-pull pull --insecure --traceback
   pulling from https://localhost:$HGPORT/
-  warning: localhost certificate with fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca not verified (check hostsecurity or web.cacerts config setting)
+  warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   no changes found
 
 Test https with cacert and fingerprint through proxy
 
   $ http_proxy=http://localhost:$HGPORT1/ hg -R copy-pull pull --config web.cacerts=pub.pem
   pulling from https://localhost:$HGPORT/
   searching for changes