Patchwork [STABLE] sslutil: guard against broken certifi installations (issue5406)

login
register
mail settings
Submitter Gábor Stefanik
Date Oct. 19, 2016, 4:07 p.m.
Message ID <77e20e2892a869717db6.1476893234@waste.org>
Download mbox | patch
Permalink /patch/17177/
State Changes Requested
Headers show

Comments

Gábor Stefanik - Oct. 19, 2016, 4:07 p.m.
# HG changeset patch
# User Gábor Stefanik <gabor.stefanik@nng.com>
# Date 1476893174 -7200
#      Wed Oct 19 18:06:14 2016 +0200
# Branch stable
# Node ID 77e20e2892a869717db636f56ab1b9664fc8b285
# Parent  e478f11e418288b8308457303d3ddf6a23f874f8
sslutil: guard against broken certifi installations (issue5406)

Certifi is currently incompatible with py2exe; the Python code for certifi gets
included in library.zip, but not the cacert.pem file - and even if it were
included, SSLContext can't load a cacert.pem file from library.zip.
This currently makes it impossible to build a standalone Windows version of
Mercurial.

Guard against this, and possibly other situations where a module with the name
"certifi" exists, but is not usable.
Gregory Szorc - Oct. 19, 2016, 5:28 p.m.
On Wed, Oct 19, 2016 at 10:07 AM, Gábor STEFANIK <Gabor.STEFANIK@nng.com>
wrote:

> >
>
>
> --------------------------------------------------------------------------
> This message, including its attachments, is confidential. For more
> information please read NNG's email policy here:
> http://www.nng.com/emailpolicy/
> By responding to this email you accept the email policy.
>
>
> -----Original Message-----
> > From: Kevin Bullock [mailto:kbullock+mercurial@ringworld.org]
> > Sent: Wednesday, October 19, 2016 6:18 PM
> > To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi
> installations
> > (issue5406)
> >
> > > On Oct 19, 2016, at 11:07, Gábor Stefanik <gabor.stefanik@nng.com>
> > wrote:
> > >
> > > # HG changeset patch
> > > # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1476893174 -7200
> > > #      Wed Oct 19 18:06:14 2016 +0200
> > > # Branch stable
> > > # Node ID 77e20e2892a869717db636f56ab1b9664fc8b285
> > > # Parent  e478f11e418288b8308457303d3ddf6a23f874f8
> > > sslutil: guard against broken certifi installations (issue5406)
> > >
> > > Certifi is currently incompatible with py2exe; the Python code for
> > > certifi gets included in library.zip, but not the cacert.pem file -
> > > and even if it were included, SSLContext can't load a cacert.pem file
> from
> > library.zip.
> > > This currently makes it impossible to build a standalone Windows
> > > version of Mercurial.
> > >
> > > Guard against this, and possibly other situations where a module with
> > > the name "certifi" exists, but is not usable.
> > >
> > > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> > > --- a/mercurial/sslutil.py
> > > +++ b/mercurial/sslutil.py
> > > @@ -695,9 +695,10 @@
> > >     try:
> > >         import certifi
> > >         certs = certifi.where()
> > > -        ui.debug('using ca certificates from certifi\n')
> > > -        return certs
> > > -    except ImportError:
> > > +        if os.path.exists(certs):
> > > +            ui.debug('using ca certificates from certifi\n')
> > > +            return certs
> > > +    except:
> >
> > You've gone from catching an ImportError to swallowing all exceptions.
>
> Intentional. ImportError is not the only thing that can be thrown here;
> e.g. if "certifi" is actually some unrelated module with no "where()"
> method.
>
> No reason to let certifi crash Hg under any circumstances.
>
>
Bareword "except" is evil because it catches all exceptions, including
SystemExit and KeyboardInterrupt. That means that if the process is killed
or the user hits ^C because certifi import is being slow because I/O or
some other badness, Python ignores that signal.

"except:" should be banned by the code checker if it isn't already.
Yuya Nishihara - Oct. 20, 2016, 3:32 p.m.
On Wed, 19 Oct 2016 18:07:01 +0000, Gábor STEFANIK wrote:
> > >> You've gone from catching an ImportError to swallowing all exceptions.
> > >
> > > Intentional. ImportError is not the only thing that can be thrown
> > > here; e.g. if "certifi" is actually some unrelated module with no "where()"
> > method.
> > >
> > > No reason to let certifi crash Hg under any circumstances.
> >
> > I have a hard time imagining how another module named "certifi" without a
> > where() method would show up on any sane system.
> >
> > As Greg said, bare `except:` is banned in Mercurial. Catch the exceptions you
> > expect might happen, none others.
> 
> Would "except Exception:" be acceptable? that one doesn't catch KeyboardInterrupt and other problematic exceptions.

ui.debug() might raise IOError. I would catch AttributeError instead.

  try:
      import certifi
      certs = certifi.where()
  except (AttributeError, ImportError):
      pass
  else:
      ui.debug('using ca certificates from certifi\n')
      return certs

And you'll need to update the comment added at a62c00f6dd04 since we'll have
more fallback cases.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -695,9 +695,10 @@ 
     try:
         import certifi
         certs = certifi.where()
-        ui.debug('using ca certificates from certifi\n')
-        return certs
-    except ImportError:
+        if os.path.exists(certs):
+            ui.debug('using ca certificates from certifi\n')
+            return certs
+    except:
         pass
 
     # On Windows, only the modern ssl module is capable of loading the system