From bacd043256a271c27d6f0d8d01a8b81c95631680 Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Sat, 24 Jun 2017 18:10:53 -0400 Subject: [PATCH 1/4] Tests to demonstrate issue #3633 Signed-off-by: Jeremy Cline --- tests/test_requests.py | 196 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 196 insertions(+) diff --git a/tests/test_requests.py b/tests/test_requests.py index 7e35786b..66ce6952 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,198 @@ 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 = {key: value for key, value in connection.__dict__.items() + if key in expected} + 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') From 14a0bf291200e29d4f57a4571a746073d8275940 Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Sat, 24 Jun 2017 18:56:19 -0400 Subject: [PATCH 2/4] Pass pool_kwargs rather than updating connection_pool_kw This addresses an issue where making HTTPS through proxies used the default urllib3 connection pool settings. fixes #3633 Signed-off-by: Jeremy Cline --- requests/adapters.py | 73 +++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/requests/adapters.py b/requests/adapters.py index 9b2015ae..d9eb9955 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 @@ -126,7 +122,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 +134,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,11 +198,13 @@ 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 `. + @staticmethod + def _pool_kwargs(verify, cert): + """Create a dictionary of keyword arguments to pass to a + :class:`PoolManager ` 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 @@ -218,6 +214,7 @@ class HTTPAdapter(BaseAdapter): key concatenated in a single file, or as a tuple of (cert_file, key_file). """ + pool_kwargs = {} if verify: cert_loc = None @@ -233,35 +230,36 @@ class HTTPAdapter(BaseAdapter): 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' + pool_kwargs['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 + pool_kwargs['ca_certs'] = cert_loc + pool_kwargs['ca_cert_dir'] = None else: - self.poolmanager.connection_pool_kw['ca_cert_dir'] = cert_loc - self.poolmanager.connection_pool_kw['ca_certs'] = None + pool_kwargs['ca_cert_dir'] = cert_loc + pool_kwargs['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 + pool_kwargs['cert_reqs'] = 'CERT_NONE' + pool_kwargs['ca_certs'] = None + pool_kwargs['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] + pool_kwargs['cert_file'] = cert[0] + pool_kwargs['key_file'] = cert[1] else: - self.poolmanager.connection_pool_kw['cert_file'] = cert - self.poolmanager.connection_pool_kw['key_file'] = None + pool_kwargs['cert_file'] = cert + pool_kwargs['key_file'] = None - cert_file = self.poolmanager.connection_pool_kw['cert_file'] - key_file = self.poolmanager.connection_pool_kw['key_file'] + 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 def build_response(self, req, resp): """Builds a :class:`Response ` object from a urllib3 @@ -309,21 +307,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 = self._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 From f58d473d4e5b2b5463b34b9c2254a7ebf6a49007 Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Mon, 26 Jun 2017 09:35:14 -0400 Subject: [PATCH 3/4] Move _pool_kwargs out to a function Signed-off-by: Jeremy Cline --- requests/adapters.py | 126 +++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 64 deletions(-) diff --git a/requests/adapters.py b/requests/adapters.py index d9eb9955..779111d5 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -49,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""" @@ -198,69 +259,6 @@ class HTTPAdapter(BaseAdapter): return manager - @staticmethod - def _pool_kwargs(verify, cert): - """Create a dictionary of keyword arguments to pass to a - :class:`PoolManager ` 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). - """ - 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 - 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 @@ -307,7 +305,7 @@ class HTTPAdapter(BaseAdapter): :param proxies: (optional) A Requests-style dictionary of proxies used on this request. :rtype: urllib3.ConnectionPool """ - pool_kwargs = self._pool_kwargs(verify, cert) + pool_kwargs = _pool_kwargs(verify, cert) proxy = select_proxy(url, proxies) if proxy: From 66f5aebd3521810cfa1fa96d8d894dafc065f247 Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Tue, 27 Jun 2017 09:29:01 -0400 Subject: [PATCH 4/4] Remove the dictionary comprehension from the tests Signed-off-by: Jeremy Cline --- tests/test_requests.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_requests.py b/tests/test_requests.py index 66ce6952..08abdeb6 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -2939,8 +2939,10 @@ class TestGetConnection(object): adapter = requests.adapters.HTTPAdapter() connection = adapter.get_connection( 'https://example.com', proxies=proxies, verify=verify, cert=cert) - actual_config = {key: value for key, value in connection.__dict__.items() - if key in expected} + actual_config = {} + for key, value in connection.__dict__.items(): + if key in expected: + actual_config[key] = value assert actual_config == expected @pytest.mark.parametrize(