Patchwork [4,of,9,V2] sslutil: restore behavior of web.cacerts=! to disable CA loading

login
register
mail settings
Submitter Gregory Szorc
Date June 1, 2016, 2:22 a.m.
Message ID <317bb3df2da17ffd258c.1464747720@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15310/
State Superseded
Headers show

Comments

Gregory Szorc - June 1, 2016, 2:22 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1464639083 25200
#      Mon May 30 13:11:23 2016 -0700
# Node ID 317bb3df2da17ffd258ca80072a6fbb0e36b84a1
# Parent  1b9bbb794da9681fd6639f321a0156c75ece764f
sslutil: restore behavior of web.cacerts=! to disable CA loading

This commit logically reverts ef316c653b7f.

There are various tests for behavior when CA certs aren't loaded.
Previously, we would pass --insecure to disable loading of CA
certs. This has worked up to this point because the error message
for --insecure and no CAs loaded is the same. Upcoming commits will
change the error message for --insecure and will change behavior
when CAs aren't loaded.

This commit restores the ability to disable loading of CA certs
by setting web.cacerts to "!". Unlike the previous implementation
which treated web.cacerts=! and --insecure as equivalent, this version
explicitly disables loading of CA certs when web.cacerts=!, even if
system/default CA certs are available.

Tests have been updated to use the new method. The variable used
to disable CA certs has been renamed because the method is not
OS X specific.
Yuya Nishihara - June 1, 2016, 1:20 p.m.
On Tue, 31 May 2016 19:22:00 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1464639083 25200
> #      Mon May 30 13:11:23 2016 -0700
> # Node ID 317bb3df2da17ffd258ca80072a6fbb0e36b84a1
> # Parent  1b9bbb794da9681fd6639f321a0156c75ece764f
> sslutil: restore behavior of web.cacerts=! to disable CA loading
> 
> This commit logically reverts ef316c653b7f.
> 
> There are various tests for behavior when CA certs aren't loaded.
n> Previously, we would pass --insecure to disable loading of CA
> certs. This has worked up to this point because the error message
> for --insecure and no CAs loaded is the same. Upcoming commits will
> change the error message for --insecure and will change behavior
> when CAs aren't loaded.
> 
> This commit restores the ability to disable loading of CA certs
> by setting web.cacerts to "!". Unlike the previous implementation
> which treated web.cacerts=! and --insecure as equivalent, this version
> explicitly disables loading of CA certs when web.cacerts=!, even if
> system/default CA certs are available.

If this is only for testing, I would add new option under debug. or devel.
section.
Pierre-Yves David - June 2, 2016, 12:44 a.m.
On 06/01/2016 03:20 PM, Yuya Nishihara wrote:
> On Tue, 31 May 2016 19:22:00 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1464639083 25200
>> #      Mon May 30 13:11:23 2016 -0700
>> # Node ID 317bb3df2da17ffd258ca80072a6fbb0e36b84a1
>> # Parent  1b9bbb794da9681fd6639f321a0156c75ece764f
>> sslutil: restore behavior of web.cacerts=! to disable CA loading
>>
>> This commit logically reverts ef316c653b7f.
>>
>> There are various tests for behavior when CA certs aren't loaded.
> n> Previously, we would pass --insecure to disable loading of CA
>> certs. This has worked up to this point because the error message
>> for --insecure and no CAs loaded is the same. Upcoming commits will
>> change the error message for --insecure and will change behavior
>> when CAs aren't loaded.
>>
>> This commit restores the ability to disable loading of CA certs
>> by setting web.cacerts to "!". Unlike the previous implementation
>> which treated web.cacerts=! and --insecure as equivalent, this version
>> explicitly disables loading of CA certs when web.cacerts=!, even if
>> system/default CA certs are available.
> 
> If this is only for testing, I would add new option under debug. or devel.
> section.

+1, if this is intended for testing only, something in devel would fit.

If there is valid usecase in the wild, this seems fine (even if a bit
arcane, at least it is the same arcane than before), some explicite
option might still be better if we introduce a new behavior (for this
hypothetic valid usecase)

Cheers,
Gregory Szorc - June 2, 2016, 2:53 a.m.
On Wed, Jun 1, 2016 at 5:44 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 06/01/2016 03:20 PM, Yuya Nishihara wrote:
> > On Tue, 31 May 2016 19:22:00 -0700, Gregory Szorc wrote:
> >> # HG changeset patch
> >> # User Gregory Szorc <gregory.szorc@gmail.com>
> >> # Date 1464639083 25200
> >> #      Mon May 30 13:11:23 2016 -0700
> >> # Node ID 317bb3df2da17ffd258ca80072a6fbb0e36b84a1
> >> # Parent  1b9bbb794da9681fd6639f321a0156c75ece764f
> >> sslutil: restore behavior of web.cacerts=! to disable CA loading
> >>
> >> This commit logically reverts ef316c653b7f.
> >>
> >> There are various tests for behavior when CA certs aren't loaded.
> > n> Previously, we would pass --insecure to disable loading of CA
> >> certs. This has worked up to this point because the error message
> >> for --insecure and no CAs loaded is the same. Upcoming commits will
> >> change the error message for --insecure and will change behavior
> >> when CAs aren't loaded.
> >>
> >> This commit restores the ability to disable loading of CA certs
> >> by setting web.cacerts to "!". Unlike the previous implementation
> >> which treated web.cacerts=! and --insecure as equivalent, this version
> >> explicitly disables loading of CA certs when web.cacerts=!, even if
> >> system/default CA certs are available.
> >
> > If this is only for testing, I would add new option under debug. or
> devel.
> > section.
>
> +1, if this is intended for testing only, something in devel would fit.
>
> If there is valid usecase in the wild, this seems fine (even if a bit
> arcane, at least it is the same arcane than before), some explicite
> option might still be better if we introduce a new behavior (for this
> hypothetic valid usecase)
>

Specifying a fingerprint makes CAs irrelevant. Specifying --insecure
disables all security, including CA loading and certificate verification.
And I have plans to facilitate specifying per-host CAs, which would
override web.cacerts. So I don't think web.cacerts=! has any value in the
wild.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -107,16 +107,19 @@  except AttributeError:
             return ssl.wrap_socket(socket, **args)
 
 def _hostsettings(ui, hostname):
     """Obtain security settings for a hostname.
 
     Returns a dict of settings relevant to that hostname.
     """
     s = {
+        # Whether we should attempt to load default/available CA certs
+        # if an explicit ``cafile`` is not defined.
+        'allowloaddefaultcerts': True,
         # List of 2-tuple of (hash algorithm, hash).
         'certfingerprints': [],
         # Path to file containing concatenated CA certs. Used by
         # SSLContext.load_verify_locations().
         'cafile': None,
         # Whether certificate verification should be disabled.
         'disablecertverification': False,
         # Whether the legacy [hostfingerprints] section has data for this host.
@@ -157,31 +160,37 @@  def _hostsettings(ui, hostname):
         s['verifymode'] = ssl.CERT_NONE
 
     # Try to hook up CA certificate validation unless something above
     # makes it not necessary.
     if s['verifymode'] is None:
         # Find global certificates file in config.
         cafile = ui.config('web', 'cacerts')
 
-        if cafile:
+        # "!" means to disable loading of CA certificates. This is
+        # intentionally undocumented, as users should not do this:
+        # they should prefer per-host security settings instead.
+        if cafile == '!':
+            cafile = None
+            s['allowloaddefaultcerts'] = False
+        elif cafile:
             cafile = util.expandpath(cafile)
             if not os.path.exists(cafile):
                 raise error.Abort(_('could not find web.cacerts: %s') % cafile)
         else:
             # No global CA certs. See if we can load defaults.
             cafile = _defaultcacerts()
             if cafile:
                 ui.debug('using %s to enable OS X system CA\n' % cafile)
 
         s['cafile'] = cafile
 
         # Require certificate validation if CA certs are being loaded and
         # verification hasn't been disabled above.
-        if cafile or _canloaddefaultcerts:
+        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.
             s['verifymode'] = ssl.CERT_NONE
 
@@ -238,20 +247,22 @@  def wrapsocket(sock, keyfile, certfile, 
         def password():
             f = keyfile or certfile
             return ui.getpass(_('passphrase for %s: ') % f, '')
         sslcontext.load_cert_chain(certfile, keyfile, password)
 
     if settings['cafile'] is not None:
         sslcontext.load_verify_locations(cafile=settings['cafile'])
         caloaded = True
-    else:
+    elif settings['allowloaddefaultcerts']:
         # This is a no-op on old Python.
         sslcontext.load_default_certs()
-        caloaded = _canloaddefaultcerts
+        caloaded = True
+    else:
+        caloaded = False
 
     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'))
 
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -157,31 +157,30 @@  Test server address cannot be reused
   [255]
 #else
   $ hg serve -p $HGPORT --certificate=$PRIV 2>&1
   abort: cannot start server at ':$HGPORT': Address already in use
   [255]
 #endif
   $ cd ..
 
-OS X has a dummy CA cert that enables use of the system CA store when using
-Apple's OpenSSL. This trick do not work with plain OpenSSL.
+Our test cert is not signed by a trusted CA. It should fail to verify if
+we are able to load CA certs.
 
-  $ DISABLEOSXDUMMYCERT=
 #if defaultcacerts
   $ hg clone https://localhost:$HGPORT/ copy-pull
   abort: error: *certificate verify failed* (glob)
   [255]
+#endif
 
-  $ DISABLEOSXDUMMYCERT="--insecure"
-#endif
+  $ DISABLECACERTS="--config web.cacerts=!"
 
 clone via pull
 
-  $ hg clone https://localhost:$HGPORT/ copy-pull $DISABLEOSXDUMMYCERT
+  $ hg clone https://localhost:$HGPORT/ copy-pull $DISABLECACERTS
   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)
   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
@@ -197,17 +196,17 @@  clone via pull
   adding bar
   $ cd ..
 
 pull without cacert
 
   $ cd copy-pull
   $ echo '[hooks]' >> .hg/hgrc
   $ echo "changegroup = printenv.py changegroup" >> .hg/hgrc
-  $ hg pull $DISABLEOSXDUMMYCERT
+  $ hg pull $DISABLECACERTS
   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)
   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)