From 3d2b337906fb12ec28efae25b8b4995018ffcc5a Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Tue, 5 Apr 2016 16:27:06 -0400 Subject: [PATCH] HTTPAdapter now updates its PoolManager connection_pool_kw With the addition of https://github.com/shazow/urllib3/pull/830 requests should update the connection_pool_kw on the PoolManager so that new ConnectionPools get created when TLS/SSL settings change. This ensures that users can update the CA certificates used to verify servers as well as the client certificate and key it uses to authenticate with servers. This fixes issue #2863 --- AUTHORS.rst | 1 + requests/adapters.py | 78 +++++++++++++++++++++++++----------------- tests/test_requests.py | 29 ++++++++++++++++ 3 files changed, 76 insertions(+), 32 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index d011fccd..611d8063 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -171,3 +171,4 @@ Patches and Suggestions - Nate Prewitt (`@nateprewitt `_) - Maik Himstedt - Michael Hunsinger +- Jeremy Cline (`@jeremycline `_) diff --git a/requests/adapters.py b/requests/adapters.py index 885ffdc3..1032d2e4 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -10,6 +10,10 @@ 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 .models import Response from .packages.urllib3.poolmanager import PoolManager, proxy_from_url @@ -119,6 +123,7 @@ 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) @@ -131,6 +136,7 @@ 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) @@ -195,17 +201,21 @@ class HTTPAdapter(BaseAdapter): return manager - def cert_verify(self, conn, url, verify, cert): - """Verify a SSL certificate. This method should not be called from user - code, and is only exposed for use when subclassing the - :class:`HTTPAdapter `. + 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 conn: The urllib3 connection object associated with the cert. - :param url: The requested URL. - :param verify: Whether we should actually verify the certificate. - :param cert: The SSL certificate to verify. + :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 url.lower().startswith('https') and verify: + if verify: cert_loc = None @@ -219,23 +229,25 @@ class HTTPAdapter(BaseAdapter): if not cert_loc: raise Exception("Could not find a suitable SSL CA certificate bundle.") - conn.cert_reqs = 'CERT_REQUIRED' + self.poolmanager.connection_pool_kw['cert_reqs'] = 'CERT_REQUIRED' if not os.path.isdir(cert_loc): - conn.ca_certs = cert_loc + self.poolmanager.connection_pool_kw['ca_certs'] = cert_loc + self.poolmanager.connection_pool_kw['ca_cert_dir'] = None else: - conn.ca_cert_dir = cert_loc + self.poolmanager.connection_pool_kw['ca_cert_dir'] = cert_loc + self.poolmanager.connection_pool_kw['ca_certs'] = None else: - conn.cert_reqs = 'CERT_NONE' - conn.ca_certs = None - conn.ca_cert_dir = None + 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): - conn.cert_file = cert[0] - conn.key_file = cert[1] + self.poolmanager.connection_pool_kw['cert_file'] = cert[0] + self.poolmanager.connection_pool_kw['key_file'] = cert[1] else: - conn.cert_file = cert + self.poolmanager.connection_pool_kw['cert_file'] = cert def build_response(self, req, resp): """Builds a :class:`Response ` object from a urllib3 @@ -274,7 +286,7 @@ class HTTPAdapter(BaseAdapter): return response - def get_connection(self, url, proxies=None): + def get_connection(self, url, proxies=None, verify=None, cert=None): """Returns a urllib3 connection for the given URL. This should not be called from user code, and is only exposed for use when subclassing the :class:`HTTPAdapter `. @@ -283,17 +295,21 @@ class HTTPAdapter(BaseAdapter): :param proxies: (optional) A Requests-style dictionary of proxies used on this request. :rtype: requests.packages.urllib3.ConnectionPool """ - proxy = select_proxy(url, proxies) + with self._pool_kw_lock: + if url.lower().startswith('https'): + self._update_poolmanager_ssl_kw(verify, cert) - 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) + 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) return conn @@ -386,10 +402,8 @@ class HTTPAdapter(BaseAdapter): :param proxies: (optional) The proxies dictionary to apply to the request. :rtype: requests.Response """ + conn = self.get_connection(request.url, proxies, verify, cert) - conn = self.get_connection(request.url, proxies) - - self.cert_verify(conn, request.url, verify, cert) url = self.request_url(request, proxies) self.add_headers(request) diff --git a/tests/test_requests.py b/tests/test_requests.py index d8d7ea54..8fc89046 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -14,6 +14,7 @@ import warnings import io import requests import pytest +import pytest_httpbin from requests.adapters import HTTPAdapter from requests.auth import HTTPDigestAuth, _basic_auth_str from requests.compat import ( @@ -1487,6 +1488,34 @@ class TestRequests: resp.close() assert resp.raw.closed + def test_updating_ca_cert(self, httpbin_secure): + """Assert that requests use the latest configured CA certificates.""" + session = requests.session() + session.verify = pytest_httpbin.certs.where() + session.get(httpbin_secure('/')) + session.verify = True + with pytest.raises(requests.exceptions.SSLError) as e: + session.get(httpbin_secure('/')) + assert 'certificate verify failed' in str(e) + + def test_updating_client_cert(self, httpbin_secure): + """Assert that requests use the latest configured client certificates.""" + ca_file = pytest_httpbin.certs.where() + cert_dir = os.path.dirname(ca_file) + # All we need is a valid certificate and key to make a request. httpbin_secure + # won't check the signature or subject name, so it's okay that these happen to + # be the server's certificate and key. + cert = os.path.join(cert_dir, 'cert.pem') + key = os.path.join(cert_dir, 'key.pem') + session = requests.session() + session.verify = ca_file + resp = session.get(httpbin_secure('/')) + resp_with_cert = session.get(httpbin_secure('/'), cert=(cert, key)) + assert resp_with_cert.raw._pool.cert_file == cert + assert resp_with_cert.raw._pool.key_file == key + assert resp.raw._pool is not resp_with_cert.raw._pool + + class TestCaseInsensitiveDict: @pytest.mark.parametrize(