diff --git a/requests/adapters.py b/requests/adapters.py index 9b2015ae..779111d5 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -10,10 +10,6 @@ and maintain connections. import os.path import socket -try: - from threading import RLock -except ImportError: # threading is an optional module and may not be present. - from dummy_threading import RLock from urllib3.poolmanager import PoolManager, proxy_from_url from urllib3.response import HTTPResponse @@ -53,6 +49,67 @@ DEFAULT_RETRIES = 0 DEFAULT_POOL_TIMEOUT = None +def _pool_kwargs(verify, cert): + """Create a dictionary of keyword arguments to pass to a + :class:`PoolManager ` with the + necessary SSL configuration. + + :param verify: Whether we should actually verify the certificate; + optionally a path to a CA certificate bundle or + directory of CA certificates. + :param cert: The path to the client certificate and key, if any. + This can either be the path to the certificate and + key concatenated in a single file, or as a tuple of + (cert_file, key_file). + """ + pool_kwargs = {} + if verify: + + cert_loc = None + + # Allow self-specified cert location. + if verify is not True: + cert_loc = verify + + if not cert_loc: + cert_loc = DEFAULT_CA_BUNDLE_PATH + + if not cert_loc or not os.path.exists(cert_loc): + raise IOError("Could not find a suitable TLS CA certificate bundle, " + "invalid path: {0}".format(cert_loc)) + + pool_kwargs['cert_reqs'] = 'CERT_REQUIRED' + + if not os.path.isdir(cert_loc): + pool_kwargs['ca_certs'] = cert_loc + pool_kwargs['ca_cert_dir'] = None + else: + pool_kwargs['ca_cert_dir'] = cert_loc + pool_kwargs['ca_certs'] = None + else: + pool_kwargs['cert_reqs'] = 'CERT_NONE' + pool_kwargs['ca_certs'] = None + pool_kwargs['ca_cert_dir'] = None + + if cert: + if not isinstance(cert, basestring): + pool_kwargs['cert_file'] = cert[0] + pool_kwargs['key_file'] = cert[1] + else: + pool_kwargs['cert_file'] = cert + pool_kwargs['key_file'] = None + + cert_file = pool_kwargs['cert_file'] + key_file = pool_kwargs['key_file'] + if cert_file and not os.path.exists(cert_file): + raise IOError("Could not find the TLS certificate file, " + "invalid path: {0}".format(cert_file)) + if key_file and not os.path.exists(key_file): + raise IOError("Could not find the TLS key file, " + "invalid path: {0}".format(key_file)) + return pool_kwargs + + class BaseAdapter(object): """The Base Transport Adapter""" @@ -126,7 +183,6 @@ class HTTPAdapter(BaseAdapter): self._pool_connections = pool_connections self._pool_maxsize = pool_maxsize self._pool_block = pool_block - self._pool_kw_lock = RLock() self.init_poolmanager(pool_connections, pool_maxsize, block=pool_block) @@ -139,7 +195,6 @@ class HTTPAdapter(BaseAdapter): # self.poolmanager uses a lambda function, which isn't pickleable. self.proxy_manager = {} self.config = {} - self._pool_kw_lock = RLock() for attr, value in state.items(): setattr(self, attr, value) @@ -204,65 +259,6 @@ class HTTPAdapter(BaseAdapter): return manager - def _update_poolmanager_ssl_kw(self, verify, cert): - """Update the :class:`PoolManager ` - connection_pool_kw with the necessary SSL configuration. This method - should not be called from user code, and is only exposed for use when - subclassing the :class:`HTTPAdapter `. - - :param verify: Whether we should actually verify the certificate; - optionally a path to a CA certificate bundle or - directory of CA certificates. - :param cert: The path to the client certificate and key, if any. - This can either be the path to the certificate and - key concatenated in a single file, or as a tuple of - (cert_file, key_file). - """ - if verify: - - cert_loc = None - - # Allow self-specified cert location. - if verify is not True: - cert_loc = verify - - if not cert_loc: - cert_loc = DEFAULT_CA_BUNDLE_PATH - - if not cert_loc or not os.path.exists(cert_loc): - raise IOError("Could not find a suitable TLS CA certificate bundle, " - "invalid path: {0}".format(cert_loc)) - - self.poolmanager.connection_pool_kw['cert_reqs'] = 'CERT_REQUIRED' - - if not os.path.isdir(cert_loc): - self.poolmanager.connection_pool_kw['ca_certs'] = cert_loc - self.poolmanager.connection_pool_kw['ca_cert_dir'] = None - else: - self.poolmanager.connection_pool_kw['ca_cert_dir'] = cert_loc - self.poolmanager.connection_pool_kw['ca_certs'] = None - else: - self.poolmanager.connection_pool_kw['cert_reqs'] = 'CERT_NONE' - self.poolmanager.connection_pool_kw['ca_certs'] = None - self.poolmanager.connection_pool_kw['ca_cert_dir'] = None - - if cert: - if not isinstance(cert, basestring): - self.poolmanager.connection_pool_kw['cert_file'] = cert[0] - self.poolmanager.connection_pool_kw['key_file'] = cert[1] - else: - self.poolmanager.connection_pool_kw['cert_file'] = cert - self.poolmanager.connection_pool_kw['key_file'] = None - - cert_file = self.poolmanager.connection_pool_kw['cert_file'] - key_file = self.poolmanager.connection_pool_kw['key_file'] - if cert_file and not os.path.exists(cert_file): - raise IOError("Could not find the TLS certificate file, " - "invalid path: {0}".format(cert_file)) - if key_file and not os.path.exists(key_file): - raise IOError("Could not find the TLS key file, " - "invalid path: {0}".format(key_file)) - def build_response(self, req, resp): """Builds a :class:`Response ` object from a urllib3 response. This should not be called from user code, and is only exposed @@ -309,21 +305,18 @@ class HTTPAdapter(BaseAdapter): :param proxies: (optional) A Requests-style dictionary of proxies used on this request. :rtype: urllib3.ConnectionPool """ - with self._pool_kw_lock: - if url.lower().startswith('https'): - self._update_poolmanager_ssl_kw(verify, cert) + pool_kwargs = _pool_kwargs(verify, cert) + proxy = select_proxy(url, proxies) - proxy = select_proxy(url, proxies) - - if proxy: - proxy = prepend_scheme_if_needed(proxy, 'http') - proxy_manager = self.proxy_manager_for(proxy) - conn = proxy_manager.connection_from_url(url) - else: - # Only scheme should be lower case - parsed = urlparse(url) - url = parsed.geturl() - conn = self.poolmanager.connection_from_url(url) + if proxy: + proxy = prepend_scheme_if_needed(proxy, 'http') + proxy_manager = self.proxy_manager_for(proxy) + conn = proxy_manager.connection_from_url(url, pool_kwargs=pool_kwargs) + else: + # Only scheme should be lower case + parsed = urlparse(url) + url = parsed.geturl() + conn = self.poolmanager.connection_from_url(url, pool_kwargs=pool_kwargs) return conn diff --git a/tests/test_requests.py b/tests/test_requests.py index 7e35786b..08abdeb6 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -31,6 +31,7 @@ from requests.structures import CaseInsensitiveDict from requests.sessions import SessionRedirectMixin from requests.models import urlencode from requests.hooks import default_hooks +from requests.utils import DEFAULT_CA_BUNDLE_PATH from .compat import StringIO, u from .utils import override_environ @@ -2765,3 +2766,200 @@ class TestPreparingURLs(object): r = requests.Request('GET', url=input, params=params) p = r.prepare() assert p.url == expected + + +class TestGetConnection(object): + """ + Tests for the :meth:`requests.adapters.HTTPAdapter.get_connection` that assert + the connections are correctly configured. + """ + @pytest.mark.parametrize( + 'proxies, verify, cert, expected', + ( + ( + {}, + True, + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {}, + False, + None, + { + 'cert_reqs': 'CERT_NONE', + 'ca_certs': None, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {}, + __file__, + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': __file__, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {}, + os.path.dirname(__file__), + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': None, + 'ca_cert_dir': os.path.dirname(__file__), + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {}, + True, + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {}, + True, + __file__, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': __file__, + 'key_file': None, + }, + ), + ( + {}, + True, + (__file__, __file__), + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': __file__, + 'key_file': __file__, + }, + ), + ( + {}, + True, + (__file__, __file__), + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': __file__, + 'key_file': __file__, + }, + ), + ( + {'http': 'http://proxy.example.com', 'https': 'http://proxy.example.com'}, + True, + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {'http': 'http://proxy.example.com', 'https': 'http://proxy.example.com'}, + os.path.dirname(__file__), + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': None, + 'ca_cert_dir': os.path.dirname(__file__), + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {'http': 'http://proxy.example.com', 'https': 'http://proxy.example.com'}, + __file__, + None, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': __file__, + 'ca_cert_dir': None, + 'cert_file': None, + 'key_file': None, + }, + ), + ( + {'http': 'http://proxy.example.com', 'https': 'http://proxy.example.com'}, + True, + __file__, + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': __file__, + 'key_file': None, + }, + ), + ( + {'http': 'http://proxy.example.com', 'https': 'http://proxy.example.com'}, + True, + (__file__, __file__), + { + 'cert_reqs': 'CERT_REQUIRED', + 'ca_certs': DEFAULT_CA_BUNDLE_PATH, + 'ca_cert_dir': None, + 'cert_file': __file__, + 'key_file': __file__, + }, + ), + ) + ) + def test_get_https_connection(self, proxies, verify, cert, expected): + """Assert connections are configured correctly.""" + adapter = requests.adapters.HTTPAdapter() + connection = adapter.get_connection( + 'https://example.com', proxies=proxies, verify=verify, cert=cert) + actual_config = {} + for key, value in connection.__dict__.items(): + if key in expected: + actual_config[key] = value + assert actual_config == expected + + @pytest.mark.parametrize( + 'verify, cert', + ( + ('a/path/that/does/not/exist', None), + (True, 'a/path/that/does/not/exist'), + (True, (__file__, 'a/path/that/does/not/exist')), + (True, ('a/path/that/does/not/exist', __file__)), + ) + ) + def test_cert_files_missing(self, verify, cert): + """ + Assert an IOError is raised when one of the certificate files or + directories can't be found. + """ + adapter = requests.adapters.HTTPAdapter() + with pytest.raises(IOError) as excinfo: + adapter.get_connection('https://example.com', verify=verify, cert=cert) + excinfo.match('invalid path: a/path/that/does/not/exist')