| 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
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
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: