Patchwork [10,of,15] sslutil: assert that the Python we run on supports TLS 1.1 and TLS 1.2

login
register
mail settings
Submitter Manuel Jacob
Date May 30, 2020, 5:52 a.m.
Message ID <38f91fbf3f53237e4f5b.1590817942@tmp>
Download mbox | patch
Permalink /patch/46405/
State New
Headers show

Comments

Manuel Jacob - May 30, 2020, 5:52 a.m.
# 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
Yuya Nishihara - May 30, 2020, 1:18 p.m.
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.

Also, raising AssertionError at import time might break client code, which
would expect ImportError/AttributeError on import error.
Gregory Szorc - May 30, 2020, 3:54 p.m.
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.
>
> 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.

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.
Joerg Sonnenberger - May 30, 2020, 4:10 p.m.
On Sat, May 30, 2020 at 07:52:22AM +0200, Manuel Jacob wrote:
> sslutil: assert that the Python we run on supports TLS 1.1 and TLS 1.2

Given that many systems want to phase out TLS 1.1, it seems questionable
to enforce this.

Joerg
Manuel Jacob - May 30, 2020, 4:51 p.m.
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.

> 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?
Gregory Szorc - May 30, 2020, 5:10 p.m.
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.


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

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.
Manuel Jacob - May 30, 2020, 5:24 p.m.
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.

Patch

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')
+supportedprotocols = {
+    b'tls1.0',
+    b'tls1.1',
+    b'tls1.2',
+}
 
 
 def _hostsettings(ui, hostname):