Patchwork [2,of,6,V2] sslutil: use create_default_context()

login
register
mail settings
Submitter Gregory Szorc
Date July 13, 2016, 7:18 a.m.
Message ID <8fc44e26c415d33b15ed.1468394288@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15815/
State Accepted
Headers show

Comments

Gregory Szorc - July 13, 2016, 7:18 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1468390821 25200
#      Tue Jul 12 23:20:21 2016 -0700
# Node ID 8fc44e26c415d33b15ed9ba9dd1e29522eafb251
# Parent  2f6559dcc8b8036aaafe6c679913efff8f25455a
sslutil: use create_default_context()

ssl.create_default_context() creates a SSLContext with reasonable
default options. In addition to what we were doing before, it
disables compression to prevent CRIME and sets a reasonable default
cipher list, which Python distributions should keep up to date to
something reasonably secure.
Yuya Nishihara - July 14, 2016, 1:36 p.m.
On Wed, 13 Jul 2016 00:18:08 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1468390821 25200
> #      Tue Jul 12 23:20:21 2016 -0700
> # Node ID 8fc44e26c415d33b15ed9ba9dd1e29522eafb251
> # Parent  2f6559dcc8b8036aaafe6c679913efff8f25455a
> sslutil: use create_default_context()
> 
> ssl.create_default_context() creates a SSLContext with reasonable
> default options. In addition to what we were doing before, it
> disables compression to prevent CRIME and sets a reasonable default
> cipher list, which Python distributions should keep up to date to
> something reasonably secure.
> 
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -259,18 +259,24 @@ def wrapsocket(sock, keyfile, certfile, 
>        server (and client) support SNI, this tells the server which certificate
>        to use.
>      """
>      if not serverhostname:
>          raise error.Abort(_('serverhostname argument is required'))
>  
>      settings = _hostsettings(ui, serverhostname)
>  
> -    # TODO use ssl.create_default_context() on modernssl.
> -    sslcontext = SSLContext(settings['protocol'])
> +    if modernssl:
> +        assert settings['protocol'] == ssl.PROTOCOL_SSLv23
> +        sslcontext = ssl.create_default_context()

create_default_context() loads CA certificates from the system store, which
means you can no longer replace the system CA certs by web.cacerts.

https://docs.python.org/2.7/library/ssl.html#ssl.create_default_context
Gregory Szorc - July 15, 2016, 2:44 a.m.
On Thu, Jul 14, 2016 at 6:36 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 13 Jul 2016 00:18:08 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1468390821 25200
> > #      Tue Jul 12 23:20:21 2016 -0700
> > # Node ID 8fc44e26c415d33b15ed9ba9dd1e29522eafb251
> > # Parent  2f6559dcc8b8036aaafe6c679913efff8f25455a
> > sslutil: use create_default_context()
> >
> > ssl.create_default_context() creates a SSLContext with reasonable
> > default options. In addition to what we were doing before, it
> > disables compression to prevent CRIME and sets a reasonable default
> > cipher list, which Python distributions should keep up to date to
> > something reasonably secure.
> >
> > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> > --- a/mercurial/sslutil.py
> > +++ b/mercurial/sslutil.py
> > @@ -259,18 +259,24 @@ def wrapsocket(sock, keyfile, certfile,
> >        server (and client) support SNI, this tells the server which
> certificate
> >        to use.
> >      """
> >      if not serverhostname:
> >          raise error.Abort(_('serverhostname argument is required'))
> >
> >      settings = _hostsettings(ui, serverhostname)
> >
> > -    # TODO use ssl.create_default_context() on modernssl.
> > -    sslcontext = SSLContext(settings['protocol'])
> > +    if modernssl:
> > +        assert settings['protocol'] == ssl.PROTOCOL_SSLv23
> > +        sslcontext = ssl.create_default_context()
>
> create_default_context() loads CA certificates from the system store, which
> means you can no longer replace the system CA certs by web.cacerts.
>
> https://docs.python.org/2.7/library/ssl.html#ssl.create_default_context


I'm not sure about that. If this were true, I would expect numerous tests
to fail because they are unable to verify the self-signed cert we use in
the tests.

The real question is whether calling this function multiple times replaces
or appends values. SSL_CTX_load_verify_locations eventually calls
X509_STORE_add_lookup, which appears to append values.

Regardless, I consider loading any CAs at this stage to be a bug because we
want to give users explicit control over which CAs are loaded. e.g. if you
specify web.cacerts as a custom bundle that has removed <insert government
controlled CA here>, you don't want that CA getting loaded via
create_default_context.

Good catch on finding the bug.

Please drop this patch from the committed repo (8357adbd8ce2).
wrapserversocket() also uses create_default_context(), so you may want to
drop 4e3663fa155d, 0c1508794183, and 9872a685c123 as well. I'll try to
patchbomb a new series in the next few hours - one that is based on current
published @ (9d02bed8477b).

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -259,18 +259,24 @@  def wrapsocket(sock, keyfile, certfile, 
       server (and client) support SNI, this tells the server which certificate
       to use.
     """
     if not serverhostname:
         raise error.Abort(_('serverhostname argument is required'))
 
     settings = _hostsettings(ui, serverhostname)
 
-    # TODO use ssl.create_default_context() on modernssl.
-    sslcontext = SSLContext(settings['protocol'])
+    if modernssl:
+        assert settings['protocol'] == ssl.PROTOCOL_SSLv23
+        sslcontext = ssl.create_default_context()
+        sslcontext.protocol = settings['protocol']
+        # We have our own hostname verification code.
+        sslcontext.check_hostname = False
+    else:
+        sslcontext = SSLContext(settings['protocol'])
 
     # This is a no-op unless using modern ssl.
     sslcontext.options |= settings['ctxoptions']
 
     # This still works on our fake SSLContext.
     sslcontext.verify_mode = settings['verifymode']
 
     if certfile is not None: