x509.UnsupportedExtension is gone in cryptography >= 2.1

[jlaine]

This fixes the issue reported in #1341

[sigmavirus24]

Thanks @jlaine. I'm not sure we can just remove this without increasing the minimum version we require in setup.{py,cfg}. That said, I wonder if we could alias the exception on 2.2 or newer such that we can handle older versions and newer versions

[njsmith]

Requiring the latest version of pyopenssl/cryptography seems pretty reasonable. (In fact the latest version of pyopenssl does require the latest version of cryptography.)

If you want to alias it instead, I guess something like this would work:

try:
    from cryptography.x509 import UnsupportedExtension
except ImportError:
    # Putting this in an except: will never catch anything, because no-one ever throws it
    class UnsupportedExtension(Exception):
        pass

[haikuginger]

The try/except import is one option; does cryptography have a base exception class that we could catch instead of each of these three?

[jlaine]

The current release of pyopenssl only supports cryptography >= 2.1.4, and the next one will support only >= 2.2.1, do we really want to support older versions? This would means explicitly testing against these older versions.

[sigmavirus24]

do we really want to support older versions?

I'm pretty sure we already do our best to test older versions. The short of it is that downstream packagers cherry-pick (in my experience) what things they actually update. Things like cryptography and pyopenssl make it hard for us to raise our minimum versions if their already packaged software works fine with an older version and would break by us updating our minimum requirement.

We can try to push the entire world forward, but that will be very unpleasant for the maintainers of this project.

[jlaine]

I was looking through the travis config files and I don't see any entries for testing different pyopenssl / crypto versions, can someone point me to the right place maybe?

[njsmith]

It would also be really nice if this PR had a test that exercised that except: block, so we don't hit this again.

I asked about this on #cryptography, and @reaperhulk generously made this cert with a duplicate extension, so it should trigger that "A problem was encountered with the certificate..." block:

https://gist.github.com/reaperhulk/70f2c4daba422a7f83965111cc2134e3

[codecov-io]

Codecov Report

Merging #1342 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1342   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          21      21           
  Lines        2014    2014           
======================================
  Hits         2014    2014

Impacted Files	Coverage Δ
urllib3/util/selectors.py	100% <0%> (ø)	⬆️
urllib3/response.py	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c8c68...76e6589. Read the comment docs.

[jlaine]

The osx tests seem pretty flaky, the failures don't seem to be related to these changes.

[sigmavirus24]

@jlaine the OSX tests are passing but codecov shows this as a decrease in overall project coverage. Please address that.

[jlaine]

The codecov seems to be out of date, and based on the failing test run, the changes it is reporting are in unrelated files.

The exception I changed was in a branch which was previously untested, and I added a test for it.

The only part of the change which is likely to negatively affect coverage is the try/except I was asked to add around the import: as pointed out earlier, the build matrix always uses the latest version of cryptography, which does not exercise the "except".

I'll push an empty commit to see if we can get a full run to get a clearer picture of what is going on.

[jlaine]

Ok I give up, the test suite on osx seems really random.

[sigmavirus24]

@jlaine your empty commit seems to have passed all the CI. What is random at the moment?

[jlaine]

Hm, I guess I was wrong but I could have sworn both Python 3.6 and 3.7 had failed on osx.

[haikuginger]

It did. I re-ran it.

[sethmlarson]

LGTM

[sethmlarson]

If there aren't any additional comments I'd say this can get merged today? @sigmavirus24 @haikuginger?

[sethmlarson]

Thanks @jlaine! 🎉