From 7d8b87c37f3a5fb993fd83eda6888ac217cd108e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastien=20G=C3=A9rard?= Date: Fri, 17 Mar 2017 22:59:47 +0100 Subject: [PATCH 1/3] #3926 raise IOError when providing an invalid path to a CA bundle or certificate files --- requests/adapters.py | 23 +++++++++++++++-------- tests/test_requests.py | 16 ++++++++++++++++ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/requests/adapters.py b/requests/adapters.py index 2475879c..03aa3b03 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -202,7 +202,8 @@ class HTTPAdapter(BaseAdapter): :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 verify: Whether we should actually verify the certificate + or it could be a path to a ca bundle :param cert: The SSL certificate to verify. """ if url.lower().startswith('https') and verify: @@ -216,15 +217,16 @@ class HTTPAdapter(BaseAdapter): if not cert_loc: cert_loc = DEFAULT_CA_BUNDLE_PATH - if not cert_loc: - raise Exception("Could not find a suitable SSL CA certificate bundle.") + if not cert_loc or not os.path.exists(cert_loc): + raise IOError("Could not find a suitable SSL CA certificate bundle, " + "invalid path: {0}".format(cert_loc)) conn.cert_reqs = 'CERT_REQUIRED' - if not os.path.isdir(cert_loc): - conn.ca_certs = cert_loc - else: + if os.path.isdir(cert_loc): conn.ca_cert_dir = cert_loc + else: + conn.ca_certs = cert_loc else: conn.cert_reqs = 'CERT_NONE' conn.ca_certs = None @@ -232,10 +234,15 @@ class HTTPAdapter(BaseAdapter): if cert: if not isinstance(cert, basestring): - conn.cert_file = cert[0] - conn.key_file = cert[1] + conn.cert_file, conn.key_file = cert else: conn.cert_file = cert + if conn.cert_file and not os.path.exists(conn.cert_file): + raise IOError("Could not find the SSL certificate file, " + "invalid path: {0}".format(conn.cert_file)) + if conn.key_file and not os.path.exists(conn.key_file): + raise IOError("Could not find the SSL key file, " + "invalid path: {0}".format(conn.key_file)) def build_response(self, req, resp): """Builds a :class:`Response ` object from a urllib3 diff --git a/tests/test_requests.py b/tests/test_requests.py index 829ab3eb..65f85e03 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -769,6 +769,22 @@ class TestRequests: def test_pyopenssl_redirect(self, httpbin_secure, httpbin_ca_bundle): requests.get(httpbin_secure('status', '301'), verify=httpbin_ca_bundle) + def test_invalid_ca_certificate_path(self): + INVALID_PATH = '/garbage' + with pytest.raises(IOError) as e: + requests.get('http://github.com', verify=INVALID_PATH) + assert str(e.value) == 'Could not find a suitable SSL CA certificate bundle, invalid path: {0}'.format(INVALID_PATH) + + def test_invalid_ssl_certificate_files(self): + INVALID_PATH = '/garbage' + with pytest.raises(IOError) as e: + requests.get('http://github.com', cert=INVALID_PATH) + assert str(e.value) == 'Could not find the SSL certificate file, invalid path: {0}'.format(INVALID_PATH) + + with pytest.raises(IOError) as e: + requests.get('http://github.com', cert=('.', INVALID_PATH)) + assert str(e.value) == 'Could not find the SSL key file, invalid path: {0}'.format(INVALID_PATH) + def test_https_warnings(self, httpbin_secure, httpbin_ca_bundle): """warnings are emitted with requests.get""" if HAS_MODERN_SSL or HAS_PYOPENSSL: From 42ec8b08f2446d0d69cb0c71816f6105dddc814d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastien=20G=C3=A9rard?= Date: Sun, 19 Mar 2017 23:04:41 +0100 Subject: [PATCH 2/3] #3927 fixes based on review --- requests/adapters.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/requests/adapters.py b/requests/adapters.py index 03aa3b03..9568d3d0 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -223,10 +223,10 @@ class HTTPAdapter(BaseAdapter): conn.cert_reqs = 'CERT_REQUIRED' - if os.path.isdir(cert_loc): - conn.ca_cert_dir = cert_loc - else: + if not os.path.isdir(cert_loc): conn.ca_certs = cert_loc + else: + conn.ca_cert_dir = cert_loc else: conn.cert_reqs = 'CERT_NONE' conn.ca_certs = None @@ -234,7 +234,8 @@ class HTTPAdapter(BaseAdapter): if cert: if not isinstance(cert, basestring): - conn.cert_file, conn.key_file = cert + conn.cert_file = cert[0] + conn.key_file = cert[1] else: conn.cert_file = cert if conn.cert_file and not os.path.exists(conn.cert_file): From 4207867aaf917ed8d223064cc5c163dfe027dc00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastien=20G=C3=A9rard?= Date: Wed, 22 Mar 2017 21:26:47 +0100 Subject: [PATCH 3/3] PR review fixes: - used httpbin_secure for tests - updated docstring related to `verify` param" - used TLS acronym instead of SSL --- requests/adapters.py | 19 ++++++++++++------- requests/api.py | 4 +++- requests/sessions.py | 5 +++-- tests/test_requests.py | 16 ++++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/requests/adapters.py b/requests/adapters.py index 9568d3d0..373fe5d0 100644 --- a/requests/adapters.py +++ b/requests/adapters.py @@ -64,7 +64,9 @@ class BaseAdapter(object): data before giving up, as a float, or a :ref:`(connect timeout, read timeout) ` tuple. :type timeout: float or tuple - :param verify: (optional) Whether to verify SSL certificates. + :param verify: (optional) Either a boolean, in which case it controls whether we verify + the server's TLS certificate, or a string, in which case it must be a path + to a CA bundle to use :param cert: (optional) Any user-provided SSL certificate to be trusted. :param proxies: (optional) The proxies dictionary to apply to the request. """ @@ -202,8 +204,9 @@ class HTTPAdapter(BaseAdapter): :param conn: The urllib3 connection object associated with the cert. :param url: The requested URL. - :param verify: Whether we should actually verify the certificate - or it could be a path to a ca bundle + :param verify: Either a boolean, in which case it controls whether we verify + the server's TLS certificate, or a string, in which case it must be a path + to a CA bundle to use :param cert: The SSL certificate to verify. """ if url.lower().startswith('https') and verify: @@ -218,7 +221,7 @@ class HTTPAdapter(BaseAdapter): cert_loc = DEFAULT_CA_BUNDLE_PATH if not cert_loc or not os.path.exists(cert_loc): - raise IOError("Could not find a suitable SSL CA certificate bundle, " + raise IOError("Could not find a suitable TLS CA certificate bundle, " "invalid path: {0}".format(cert_loc)) conn.cert_reqs = 'CERT_REQUIRED' @@ -239,10 +242,10 @@ class HTTPAdapter(BaseAdapter): else: conn.cert_file = cert if conn.cert_file and not os.path.exists(conn.cert_file): - raise IOError("Could not find the SSL certificate file, " + raise IOError("Could not find the TLS certificate file, " "invalid path: {0}".format(conn.cert_file)) if conn.key_file and not os.path.exists(conn.key_file): - raise IOError("Could not find the SSL key file, " + raise IOError("Could not find the TLS key file, " "invalid path: {0}".format(conn.key_file)) def build_response(self, req, resp): @@ -389,7 +392,9 @@ class HTTPAdapter(BaseAdapter): data before giving up, as a float, or a :ref:`(connect timeout, read timeout) ` tuple. :type timeout: float or tuple - :param verify: (optional) Whether to verify SSL certificates. + :param verify: (optional) Either a boolean, in which case it controls whether + we verify the server's TLS certificate, or a string, in which case it + must be a path to a CA bundle to use :param cert: (optional) Any user-provided SSL certificate to be trusted. :param proxies: (optional) The proxies dictionary to apply to the request. :rtype: requests.Response diff --git a/requests/api.py b/requests/api.py index 856f0b3a..687ed447 100644 --- a/requests/api.py +++ b/requests/api.py @@ -36,7 +36,9 @@ def request(method, url, **kwargs): :param allow_redirects: (optional) Boolean. Enable/disable GET/OPTIONS/POST/PUT/PATCH/DELETE/HEAD redirection. Defaults to ``True``. :type allow_redirects: bool :param proxies: (optional) Dictionary mapping protocol to the URL of the proxy. - :param verify: (optional) whether the SSL cert will be verified. A CA_BUNDLE path can also be provided. Defaults to ``True``. + :param verify: (optional) Either a boolean, in which case it controls whether we verify + the server's TLS certificate, or a string, in which case it must be a path + to a CA bundle to use. Defaults to ``True``. :param stream: (optional) if ``False``, the response content will be immediately downloaded. :param cert: (optional) if String, path to ssl client cert file (.pem). If Tuple, ('cert', 'key') pair. :return: :class:`Response ` object diff --git a/requests/sessions.py b/requests/sessions.py index bc67334a..06e65570 100644 --- a/requests/sessions.py +++ b/requests/sessions.py @@ -460,8 +460,9 @@ class Session(SessionRedirectMixin): hostname to the URL of the proxy. :param stream: (optional) whether to immediately download the response content. Defaults to ``False``. - :param verify: (optional) whether the SSL cert will be verified. - A CA_BUNDLE path can also be provided. Defaults to ``True``. + :param verify: (optional) Either a boolean, in which case it controls whether we verify + the server's TLS certificate, or a string, in which case it must be a path + to a CA bundle to use. Defaults to ``True``. :param cert: (optional) if String, path to ssl client cert file (.pem). If Tuple, ('cert', 'key') pair. :rtype: requests.Response diff --git a/tests/test_requests.py b/tests/test_requests.py index 65f85e03..ab1e6a9c 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -769,21 +769,21 @@ class TestRequests: def test_pyopenssl_redirect(self, httpbin_secure, httpbin_ca_bundle): requests.get(httpbin_secure('status', '301'), verify=httpbin_ca_bundle) - def test_invalid_ca_certificate_path(self): + def test_invalid_ca_certificate_path(self, httpbin_secure): INVALID_PATH = '/garbage' with pytest.raises(IOError) as e: - requests.get('http://github.com', verify=INVALID_PATH) - assert str(e.value) == 'Could not find a suitable SSL CA certificate bundle, invalid path: {0}'.format(INVALID_PATH) + requests.get(httpbin_secure(), verify=INVALID_PATH) + assert str(e.value) == 'Could not find a suitable TLS CA certificate bundle, invalid path: {0}'.format(INVALID_PATH) - def test_invalid_ssl_certificate_files(self): + def test_invalid_ssl_certificate_files(self, httpbin_secure): INVALID_PATH = '/garbage' with pytest.raises(IOError) as e: - requests.get('http://github.com', cert=INVALID_PATH) - assert str(e.value) == 'Could not find the SSL certificate file, invalid path: {0}'.format(INVALID_PATH) + requests.get(httpbin_secure(), cert=INVALID_PATH) + assert str(e.value) == 'Could not find the TLS certificate file, invalid path: {0}'.format(INVALID_PATH) with pytest.raises(IOError) as e: - requests.get('http://github.com', cert=('.', INVALID_PATH)) - assert str(e.value) == 'Could not find the SSL key file, invalid path: {0}'.format(INVALID_PATH) + requests.get(httpbin_secure(), cert=('.', INVALID_PATH)) + assert str(e.value) == 'Could not find the TLS key file, invalid path: {0}'.format(INVALID_PATH) def test_https_warnings(self, httpbin_secure, httpbin_ca_bundle): """warnings are emitted with requests.get"""