[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mercurial-devel
Subject:    Re: [PATCH 10 of 15] sslutil: assert that the Python we run on supports TLS 1.1 and TLS 1.2
From:       Manuel Jacob <me () manueljacob ! de>
Date:       2020-05-30 17:24:11
Message-ID: 25831bc9bf0b326bac5e4c3cc7244a29 () manueljacob ! de
[Download RAW message or body]

On 2020-05-30 19:10, Gregory Szorc wrote:
> On Sat, May 30, 2020 at 9:51 AM Manuel Jacob <me@manueljacob.de> wrote:
> 
>> On 2020-05-30 17:54, Gregory Szorc wrote:
>> > On Sat, May 30, 2020 at 6:36 AM Yuya Nishihara <yuya@tcha.org> wrote:
>> >
>> >> On Sat, 30 May 2020 07:52:22 +0200, Manuel Jacob wrote:
>> >> > # HG changeset patch
>> >> > # User Manuel Jacob <me@manueljacob.de>
>> >> > # Date 1590783568 -7200
>> >> > #      Fri May 29 22:19:28 2020 +0200
>> >> > # Node ID 38f91fbf3f53237e4f5b7fd382f72cfab5e2c8fd
>> >> > # Parent  13922e383d20ca51752a2c3bd16429a5b0e30397
>> >> > # EXP-Topic require_modern_ssl
>> >> > sslutil: assert that the Python we run on supports TLS 1.1 and TLS 1.2
>> >> >
>> >> > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
>> >> > --- a/mercurial/sslutil.py
>> >> > +++ b/mercurial/sslutil.py
>> >> > @@ -44,13 +44,18 @@ configprotocols = {
>> >> >
>> >> >  hassni = getattr(ssl, 'HAS_SNI', False)
>> >> >
>> >> > -# TLS 1.1 and 1.2 may not be supported if the OpenSSL Python is
>> compiled
>> >> > -# against doesn't support them.
>> >> > -supportedprotocols = {b'tls1.0'}
>> >> > -if util.safehasattr(ssl, b'PROTOCOL_TLSv1_1'):
>> >> > -    supportedprotocols.add(b'tls1.1')
>> >> > -if util.safehasattr(ssl, b'PROTOCOL_TLSv1_2'):
>> >> > -    supportedprotocols.add(b'tls1.2')
>> >> > +# TLS 1.1 and 1.2 are supported since OpenSSL 1.0.1, released on
>> >> 2012-03-14.
>> >> > +# OpenSSL 1.0.0 is EOL since 2015-12-31. It is reasonable to expect
>> that
>> >> > +# distributions having Python 2.7.9+ or having backported modern
>> >> features to
>> >> > +# the ssl module (which we require) have OpenSSL 1.0.1+. To be sure,
>> we
>> >> assert
>> >> > +# that support is actually present.
>> >> > +assert util.safehasattr(ssl, b'PROTOCOL_TLSv1_1')
>> >> > +assert util.safehasattr(ssl, b'PROTOCOL_TLSv1_2')
>> >>
>> >> Can we expect that old RHEL/CentOS migrated to OpenSSL 1.0.1+?
>> >> I hope they did, but I'm not sure.
>> 
>> RHEL6 originally had OpenSSL 1.0.0 but "rebased" to OpenSSL 1.0.1 with
>> RHEL6.5 (according to https://access.redhat.com/articles/1462223). I 
>> do
>> not know whether they also maintain OpenSSL 1.0.0 with backports, but 
>> I
>> found no evidence for this.
>> 
>> >> Also, raising AssertionError at import time might break client code,
>> >> which
>> >> would expect ImportError/AttributeError on import error.
>> >>
>> >
>> > Agreed that we want to avoid the AssertionError at import time. I would
>> > refactor all the code for validating the sanity of the `ssl` module
>> > into a
>> > single function (perhaps the one that constructs an SSLContext) and
>> > have it
>> > abort if we fail to meet security requirements. That way we won't get
>> > an
>> > error until we actually attempt an operation that requires ssl. This
>> > feels
>> > better than running code at module import time, which can slow down
>> > code
>> > paths that don't need it.
>> 
>> Isn't that part already handled by demandimport? In any case, I'm fine
>> with changing the patches to check the TLS 1.2 support in setup.py, or
>> when creating the context, or a combination of both.
>> 
> 
> demandimport helps with deferring the module import. But there are many
> pieces of code that look at module attributes, which trigger an import. 
> So
> module-scoped code that we can avoid can potentially help with startup
> times.

That makes sense, I'll update the patch later.

>> > Regarding the minimum versions, given that TLS 1.2 is the minimum TLS
>> > version to be reasonably secure in 2020, I would strongly prefer
>> > requiring
>> > it by default. I'm not opposed to a config option to allow TLS 1.0 and
>> > 1.1
>> > for the legacy environments that can't do better. Just as long as we
>> > document that it weakens security.
>> 
>> Do you refer to requiring the underlying Python version to support TLS
>> 1.2 or to requiring the wire protocol to be at least TLS 1.2?
>> 
> 
> I think we should require TLS 1.2+ on the wire protocol by default. If 
> the
> Python environment or the server doesn't support TLS 1.2+, we should 
> allow
> people to downgrade via a config option and/or command argument. And we
> should consider a future where TLS 1.2 becomes insecure and we need to
> change defaults again. IMO the code should be structured to make these
> future transitions as easy as possible.

I think that topic is independent of this patch series. The 
infrastructure for what you describe is mostly there. It would seemingly 
be a matter of changing `defaultprotocol` from 'tls1.1' to 'tls1.2'.

> While I'm here, Python 3.7+ supports TLS 1.3 if the underlying SSL 
> library
> supports it. I'd love to see support for TLS 1.3 in `sslutil.py`. This 
> is
> out of scope for your series and you don't have to take on the work if 
> you
> don't want to.

I think that OpenSSL automatically uses a newer version if available. If 
not, we should fix our code so that this happens. What is not yet 
implemented is a config to make TLS 1.3 mandatory. It would be a matter 
of adding a few lines, and I can do it once this patch series got 
through.
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[prev in list] [next in list] [prev in thread] [next in thread]