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

[Yuya Nishihara]

On Wed, 19 Oct 2016 18:07:01 +, 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Augie Fackler]

On Wed, Oct 19, 2016 at 06:07:01PM +, Gábor STEFANIK 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+mercur...@ringworld.org]
> > Sent: Wednesday, October 19, 2016 7:46 PM
> > To: Gábor STEFANIK <gabor.stefa...@nng.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi 
> > installations
> > (issue5406)
> >
> > > On Oct 19, 2016, at 12:07, Gábor STEFANIK <gabor.stefa...@nng.com>
> > wrote:
> > >
> > > -Original Message-
> > >> From: Kevin Bullock [mailto:kbullock+mercur...@ringworld.org]
> > >> Sent: Wednesday, October 19, 2016 6:18 PM
> > >> To: Gábor STEFANIK <gabor.stefa...@nng.com>
> > >> Cc: mercurial-devel@mercurial-scm.org
> > >> Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi
> > >> installations
> > >> (issue5406)
> > >>
> > >> 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.

That seems reasonable to me, provided we have some mechanism that logs
(perhaps a warning?) that certifi is installed but appears broken.

>
> >
> > pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
> > Kevin R. Bullock
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Gábor STEFANIK]

>


--
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+mercur...@ringworld.org]
> Sent: Wednesday, October 19, 2016 7:46 PM
> To: Gábor STEFANIK <gabor.stefa...@nng.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi 
> installations
> (issue5406)
>
> > On Oct 19, 2016, at 12:07, Gábor STEFANIK <gabor.stefa...@nng.com>
> wrote:
> >
> > -Original Message-
> >> From: Kevin Bullock [mailto:kbullock+mercur...@ringworld.org]
> >> Sent: Wednesday, October 19, 2016 6:18 PM
> >> To: Gábor STEFANIK <gabor.stefa...@nng.com>
> >> Cc: mercurial-devel@mercurial-scm.org
> >> Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi
> >> installations
> >> (issue5406)
> >>
> >> 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.

>
> pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
> Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Kevin Bullock]

> On Oct 19, 2016, at 12:07, Gábor STEFANIK <gabor.stefa...@nng.com> wrote:
> 
> -Original Message-
>> From: Kevin Bullock [mailto:kbullock+mercur...@ringworld.org]
>> Sent: Wednesday, October 19, 2016 6:18 PM
>> To: Gábor STEFANIK <gabor.stefa...@nng.com>
>> Cc: mercurial-devel@mercurial-scm.org
>> Subject: Re: [PATCH STABLE] sslutil: guard against broken certifi 
>> installations
>> (issue5406)
>> 
>> 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.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Gábor STEFANIK]

>


--
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+mercur...@ringworld.org]
> Sent: Wednesday, October 19, 2016 6:18 PM
> To: Gábor STEFANIK <gabor.stefa...@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.stefa...@nng.com>
> wrote:
> >
> > # HG changeset patch
> > # User Gábor Stefanik <gabor.stefa...@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.

>
> pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
> Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Kevin Bullock]

> On Oct 19, 2016, at 11:07, Gábor Stefanik  wrote:
> 
> # HG changeset patch
> # User Gábor Stefanik 
> # 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.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Gábor Stefanik]

# HG changeset patch
# User Gábor Stefanik 
# 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:
 pass
 
 # On Windows, only the modern ssl module is capable of loading the system
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel