Patchwork [STABLE] sslutil: work around SSLContext.get_ca_certs bug on Windows (issue5313)

login
register
mail settings
Submitter Gregory Szorc
Date July 25, 2016, 7:01 p.m.
Message ID <94abd7d9e7a1b8f689da.1469473268@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15986/
State Accepted
Headers show

Comments

Gregory Szorc - July 25, 2016, 7:01 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1469473255 25200
#      Mon Jul 25 12:00:55 2016 -0700
# Branch stable
# Node ID 94abd7d9e7a1b8f689da7758927700a2e28959a6
# Parent  9c2cc107547fd701a7604349632f2f590366f17c
sslutil: work around SSLContext.get_ca_certs bug on Windows (issue5313)

SSLContext.get_ca_certs() can raise
"ssl.SSLError: unknown error (_ssl.c:636)" on Windows. See
https://bugs.python.org/issue20916 for more info.

We add a try..except that swallows the exception to work around
this bug. If we encounter the bug, we won't print a warning
message about attempting to load CA certificates. This is
unfortunate. But there appears to be little we can do :/
Matt Harbison - July 26, 2016, 12:57 a.m.
On Mon, 25 Jul 2016 15:01:08 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1469473255 25200
> #      Mon Jul 25 12:00:55 2016 -0700
> # Branch stable
> # Node ID 94abd7d9e7a1b8f689da7758927700a2e28959a6
> # Parent  9c2cc107547fd701a7604349632f2f590366f17c
> sslutil: work around SSLContext.get_ca_certs bug on Windows (issue5313)
>
> SSLContext.get_ca_certs() can raise
> "ssl.SSLError: unknown error (_ssl.c:636)" on Windows. See
> https://bugs.python.org/issue20916 for more info.
>
> We add a try..except that swallows the exception to work around
> this bug. If we encounter the bug, we won't print a warning
> message about attempting to load CA certificates. This is
> unfortunate. But there appears to be little we can do :/

This fixes it for me, thanks.

I wonder if the exception handler should duplicate the warning message,  
since it looks like get_ca_certs() is the only thing that can throw  
there.  OTOH, the output I got was pretty clear (though presumably there  
are other reasons the certs could fail to load?)

$ ../hg in https://mercurial-scm.org/hg
(could not negotiate a common security protocol (tls1.1+) with selenic.com;
the likely cause is Mercurial is configured to be more secure than the
server can support)
(consider contacting the operator of this server and ask them to support
modern TLS protocol versions; or, set
  hostsecurity.selenic.com:minimumprotocol=tls1.0 to allow use of legacy,
less secure protocols when communicating with this server)
(see https://mercurial-scm.org/wiki/SecureConnections for more info)
abort: error: unknown error (_ssl.c:628)

In either case, the warning message that would be bypassed isn't on the  
wiki page, so maybe it's just meant as a hint to developers  
troubleshooting?

> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -404,22 +404,28 @@ def wrapsocket(sock, keyfile, certfile,
>      try:
>          sslsocket = sslcontext.wrap_socket(sock,  
> server_hostname=serverhostname)
>      except ssl.SSLError as e:
>          # If we're doing certificate verification and no CA certs are  
> loaded,
>          # that is almost certainly the reason why verification failed.  
> Provide
>          # a hint to the user.
>          # Only modern ssl module exposes SSLContext.get_ca_certs() so  
> we can
>          # only show this warning if modern ssl is available.
> -        if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
> -            modernssl and not sslcontext.get_ca_certs()):
> -            ui.warn(_('(an attempt was made to load CA certificates but  
> none '
> -                      'were loaded; see '
> -                      'https://mercurial-scm.org/wiki/SecureConnections  
> for '
> -                      'how to configure Mercurial to avoid this  
> error)\n'))
> +        # The exception handler is here because of
> +        # https://bugs.python.org/issue20916.
> +        try:
> +            if (caloaded and settings['verifymode'] ==  
> ssl.CERT_REQUIRED and
> +                modernssl and not sslcontext.get_ca_certs()):
> +                ui.warn(_('(an attempt was made to load CA certificates  
> but '
> +                          'none were loaded; see '
> +                           
> 'https://mercurial-scm.org/wiki/SecureConnections '
> +                          'for how to configure Mercurial to avoid this  
> '
> +                          'error)\n'))
> +        except ssl.SSLError:
> +            pass
>          # Try to print more helpful error messages for known failures.
>          if util.safehasattr(e, 'reason'):
>              # This error occurs when the client and server don't share a
>              # common/supported SSL/TLS protocol. We've disabled SSLv2  
> and SSLv3
>              # outright. Hopefully the reason for this error is that we  
> require
>              # TLS 1.1+ and the server only supports TLS 1.0. Whatever  
> the
>              # reason, try to emit an actionable warning.
>              if e.reason == 'UNSUPPORTED_PROTOCOL':
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - July 26, 2016, 12:55 p.m.
On Mon, 25 Jul 2016 20:57:55 -0400, Matt Harbison wrote:
> On Mon, 25 Jul 2016 15:01:08 -0400, Gregory Szorc  
> <gregory.szorc@gmail.com> wrote:
> 
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1469473255 25200
> > #      Mon Jul 25 12:00:55 2016 -0700
> > # Branch stable
> > # Node ID 94abd7d9e7a1b8f689da7758927700a2e28959a6
> > # Parent  9c2cc107547fd701a7604349632f2f590366f17c
> > sslutil: work around SSLContext.get_ca_certs bug on Windows (issue5313)

Queued, thanks.

> > SSLContext.get_ca_certs() can raise
> > "ssl.SSLError: unknown error (_ssl.c:636)" on Windows. See
> > https://bugs.python.org/issue20916 for more info.
> >
> > We add a try..except that swallows the exception to work around
> > this bug. If we encounter the bug, we won't print a warning
> > message about attempting to load CA certificates. This is
> > unfortunate. But there appears to be little we can do :/
> 
> This fixes it for me, thanks.
> 
> I wonder if the exception handler should duplicate the warning message,  
> since it looks like get_ca_certs() is the only thing that can throw  
> there.  OTOH, the output I got was pretty clear (though presumably there  
> are other reasons the certs could fail to load?)

I think it doesn't show the warning because the failure of get_ca_certs()
doesn't mean no certs were loaded.
Gregory Szorc - July 26, 2016, 3:25 p.m.
On Tue, Jul 26, 2016 at 5:55 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 25 Jul 2016 20:57:55 -0400, Matt Harbison wrote:
> > On Mon, 25 Jul 2016 15:01:08 -0400, Gregory Szorc
> > <gregory.szorc@gmail.com> wrote:
> >
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1469473255 25200
> > > #      Mon Jul 25 12:00:55 2016 -0700
> > > # Branch stable
> > > # Node ID 94abd7d9e7a1b8f689da7758927700a2e28959a6
> > > # Parent  9c2cc107547fd701a7604349632f2f590366f17c
> > > sslutil: work around SSLContext.get_ca_certs bug on Windows (issue5313)
>
> Queued, thanks.
>
> > > SSLContext.get_ca_certs() can raise
> > > "ssl.SSLError: unknown error (_ssl.c:636)" on Windows. See
> > > https://bugs.python.org/issue20916 for more info.
> > >
> > > We add a try..except that swallows the exception to work around
> > > this bug. If we encounter the bug, we won't print a warning
> > > message about attempting to load CA certificates. This is
> > > unfortunate. But there appears to be little we can do :/
> >
> > This fixes it for me, thanks.
> >
> > I wonder if the exception handler should duplicate the warning message,
> > since it looks like get_ca_certs() is the only thing that can throw
> > there.  OTOH, the output I got was pretty clear (though presumably there
> > are other reasons the certs could fail to load?)
>
> I think it doesn't show the warning because the failure of get_ca_certs()
> doesn't mean no certs were loaded.
>

Right. If get_ca_certs() fails due to the CPython bug, it likely means CAs
were loaded. And since the warning message relates to lack of CAs being
loaded, we don't need to display it.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -404,22 +404,28 @@  def wrapsocket(sock, keyfile, certfile, 
     try:
         sslsocket = sslcontext.wrap_socket(sock, server_hostname=serverhostname)
     except ssl.SSLError as e:
         # If we're doing certificate verification and no CA certs are loaded,
         # that is almost certainly the reason why verification failed. Provide
         # a hint to the user.
         # Only modern ssl module exposes SSLContext.get_ca_certs() so we can
         # only show this warning if modern ssl is available.
-        if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
-            modernssl and not sslcontext.get_ca_certs()):
-            ui.warn(_('(an attempt was made to load CA certificates but none '
-                      'were loaded; see '
-                      'https://mercurial-scm.org/wiki/SecureConnections for '
-                      'how to configure Mercurial to avoid this error)\n'))
+        # The exception handler is here because of
+        # https://bugs.python.org/issue20916.
+        try:
+            if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
+                modernssl and not sslcontext.get_ca_certs()):
+                ui.warn(_('(an attempt was made to load CA certificates but '
+                          'none were loaded; see '
+                          'https://mercurial-scm.org/wiki/SecureConnections '
+                          'for how to configure Mercurial to avoid this '
+                          'error)\n'))
+        except ssl.SSLError:
+            pass
         # Try to print more helpful error messages for known failures.
         if util.safehasattr(e, 'reason'):
             # This error occurs when the client and server don't share a
             # common/supported SSL/TLS protocol. We've disabled SSLv2 and SSLv3
             # outright. Hopefully the reason for this error is that we require
             # TLS 1.1+ and the server only supports TLS 1.0. Whatever the
             # reason, try to emit an actionable warning.
             if e.reason == 'UNSUPPORTED_PROTOCOL':