Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Ensure PyOpenSSLContext.load_verify_locations raises ssl.SSLError #1517

Closed
wants to merge 3 commits into from
Closed

WIP: Ensure PyOpenSSLContext.load_verify_locations raises ssl.SSLError #1517

wants to merge 3 commits into from

Conversation

pilou-
Copy link
Contributor

@pilou- pilou- commented Jan 11, 2019

Without this patch, an OpenSSL.SSL.Error is raised when PyOpenSSLContext.load_verify_locations fails:

$ python -c "import os; import urllib3; import urllib3.contrib.pyopenssl; urllib3.contrib.pyopenssl.inject_into_urllib3(); urllib3.util.ssl_wrap_socket(None, ca_certs=os.devnull)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/tmp/urllib3/local/lib/python2.7/site-packages/urllib3/util/ssl_.py", line 321, in ssl_wrap_socket
    context.load_verify_locations(ca_certs, ca_cert_dir)
  File "/tmp/urllib3/local/lib/python2.7/site-packages/urllib3/contrib/pyopenssl.py", line 428, in load_verify_locations
    self._ctx.load_verify_locations(cafile, capath)
  File "/tmp/urllib3/local/lib/python2.7/site-packages/OpenSSL/SSL.py", line 781, in load_verify_locations
    _raise_current_error()
  File "/tmp/urllib3/local/lib/python2.7/site-packages/OpenSSL/_util.py", line 53, in exception_from_error_queue
    raise exception_type(errors)
OpenSSL.SSL.Error: []

With this patch, an ssl.SSLError is raised, in the same way do_handshake errors are handled:

$ python -c "import os; import urllib3; import urllib3.contrib.pyopenssl; urllib3.contrib.pyopenssl.inject_into_urllib3(); urllib3.util.ssl_wrap_socket(None, ca_certs=os.devnull)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/pilou/src/urllib3/src/urllib3/util/ssl_.py", line 313, in ssl_wrap_socket
    raise SSLError(e)
urllib3.exceptions.SSLError: ('unable to load trusted certificates: Error([('x509 certificate routines', 'X509_load_cert_crl_file', 'no certificate or crl found')],)',)

then:

  1. this ssl.SSLError is caught by ssl_wrap_socket
  2. an urllib3.exceptions.SSLError error is raised
    • which is the same exception that the one raised when pyOpenSSL isn't available
    • and which is what users of urllib3 library expect (this can be tested with python -c "import requests, os; requests.get('https://github.com', verify=os.devnull)")

It allows ssl_wrap_socket to catch this ssl.SSLError which is a subclass
of built-in IOError, then urllib3.exceptions.SSLError will be raised.
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this! I have a few review comments below to address before we can merge this.

@@ -84,3 +87,14 @@ def test_get_subj_alt_name(self, mock_warning):
self.assertEqual(mock_warning.call_count, 1)
self.assertIsInstance(mock_warning.call_args[0][1],
x509.DuplicateExtension)

class TestPyOpenSSLException(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this testcase to TestClientCerts to ensure it's run against all SSLContext implementations.

class TestPyOpenSSLException(unittest.TestCase):
def test_load_verify_locations_exception(self):
"""
Ensure PyOpenSSLContext.load_verify_locations raises ssl.SSLError, which is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring will have to be modified to be generic.

self._ctx.load_verify_locations(cafile, capath)
if cadata is not None:
self._ctx.load_verify_locations(BytesIO(cadata))
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have to add this change to the SecureTransport SSLContext implementation as well. We also need a CHANGES.rst entry saying that all SSLContext.load_verify_locations() implementations raise urllib3.exceptions.SSLError on a failure.

@codecov-io
Copy link

Codecov Report

Merging #1517 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
+ Coverage   64.68%   64.72%   +0.03%     
==========================================
  Files          22       22              
  Lines        2897     2897              
==========================================
+ Hits         1874     1875       +1     
+ Misses       1023     1022       -1
Impacted Files Coverage Δ
src/urllib3/util/ssl_.py 37.06% <0%> (+0.43%) ⬆️

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 adb358f...b065bed. Read the comment docs.

@pilou- pilou- changed the title Ensure PyOpenSSLContext.load_verify_locations raises ssl.SSLError WIP: Ensure PyOpenSSLContext.load_verify_locations raises ssl.SSLError Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants