Use cert_reqs=CERT_REQUIRED by default (#1507)

12 files changed, 48 insertions, 53 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 56e9c509..c8d6b9b3 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -16,6 +16,8 @@ dev (master)
 * Fixed issue where OpenSSL would block if an encrypted client private key was
   given and no password was given. Instead an ``SSLError`` is raised. (Pull #1489)
 
+* Require and validate certificates by default when using HTTPS (Pull #1507)
+
 * ... [Short description of non-trivial change.] (Issue #)
 
 
diff --git a/docs/user-guide.rst b/docs/user-guide.rst
index 11c94f3e..a63af086 100644
--- a/docs/user-guide.rst
+++ b/docs/user-guide.rst
@@ -202,11 +202,15 @@ recommended to set the ``Content-Type`` header::
 Certificate verification
 ------------------------
 
-It is highly recommended to always use SSL certificate verification.
-**By default, urllib3 does not verify HTTPS requests**.
+ .. note:: *New in version 1.25*
 
-In order to enable verification you will need a set of root certificates. The easiest
-and most reliable method is to use the `certifi <https://certifi.io/>`_ package which provides Mozilla's root certificate bundle::
+    HTTPS connections are now verified by default (``cert_reqs = 'CERT_REQUIRED'``).
+
+While you can disable certification verification, it is highly recommend to leave it on.
+
+Unless otherwise specified urllib3 will try to load the default system certificate stores.
+The most reliable cross-platform method is to use the `certifi <https://certifi.io/>`_
+package which provides Mozilla's root certificate bundle::
 
     pip install certifi
 
diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py
index 5afd3d40..3d243964 100644
--- a/src/urllib3/connection.py
+++ b/src/urllib3/connection.py
@@ -282,15 +282,13 @@ class VerifiedHTTPSConnection(HTTPSConnection):
         """
         This method should only be called once, before the connection is used.
         """
-        # If cert_reqs is not provided, we can try to guess. If the user gave
-        # us a cert database, we assume they want to use it: otherwise, if
-        # they gave us an SSL Context object we should use whatever is set for
-        # it.
+        # If cert_reqs is not provided we'll assume CERT_REQUIRED unless we also
+        # have an SSLContext object in which case we'll use its verify_mode.
         if cert_reqs is None:
-            if ca_certs or ca_cert_dir:
-                cert_reqs = 'CERT_REQUIRED'
-            elif self.ssl_context is not None:
+            if self.ssl_context is not None:
                 cert_reqs = self.ssl_context.verify_mode
+            else:
+                cert_reqs = resolve_cert_reqs(None)
 
         self.key_file = key_file
         self.cert_file = cert_file
diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py
index addf93c0..ecc081a1 100644
--- a/src/urllib3/connectionpool.py
+++ b/src/urllib3/connectionpool.py
@@ -767,9 +767,6 @@ class HTTPSConnectionPool(HTTPConnectionPool):
                                     block, headers, retries, _proxy, _proxy_headers,
                                     **conn_kw)
 
-        if ca_certs and cert_reqs is None:
-            cert_reqs = 'CERT_REQUIRED'
-
         self.key_file = key_file
         self.cert_file = cert_file
         self.cert_reqs = cert_reqs
diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py
index f21ec58e..6fb1d0e9 100644
--- a/src/urllib3/util/ssl_.py
+++ b/src/urllib3/util/ssl_.py
@@ -54,7 +54,7 @@ _IP_ADDRESS_REGEX = re.compile(
 
 try:  # Test for SSL features
     import ssl
-    from ssl import wrap_socket, CERT_NONE, PROTOCOL_SSLv23
+    from ssl import wrap_socket, CERT_REQUIRED, PROTOCOL_SSLv23
     from ssl import HAS_SNI  # Has SNI?
 except ImportError:
     pass
@@ -189,7 +189,7 @@ def resolve_cert_reqs(candidate):
     constant which can directly be passed to wrap_socket.
     """
     if candidate is None:
-        return CERT_NONE
+        return CERT_REQUIRED
 
     if isinstance(candidate, str):
         res = getattr(ssl, candidate, None)
diff --git a/test/contrib/test_socks.py b/test/contrib/test_socks.py
index ce016452..4ed64f5a 100644
--- a/test/contrib/test_socks.py
+++ b/test/contrib/test_socks.py
@@ -4,7 +4,7 @@ import socket
 from urllib3.contrib import socks
 from urllib3.exceptions import ConnectTimeoutError, NewConnectionError
 
-from dummyserver.server import DEFAULT_CERTS
+from dummyserver.server import DEFAULT_CERTS, DEFAULT_CA
 from dummyserver.testcase import IPV4SocketDummyServerTestCase
 
 import pytest
@@ -715,7 +715,7 @@ class TestSOCKSWithTLS(IPV4SocketDummyServerTestCase):
 
         self._start_server(request_handler)
         proxy_url = "socks5h://%s:%s" % (self.host, self.port)
-        pm = socks.SOCKSProxyManager(proxy_url)
+        pm = socks.SOCKSProxyManager(proxy_url, ca_certs=DEFAULT_CA)
         self.addCleanup(pm.clear)
         response = pm.request('GET', 'https://localhost')
 
diff --git a/test/test_connectionpool.py b/test/test_connectionpool.py
index 71fe24ac..8bc74489 100644
--- a/test/test_connectionpool.py
+++ b/test/test_connectionpool.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import
 
+import ssl
 import pytest
 
 from urllib3.connectionpool import (
@@ -350,7 +351,7 @@ class TestConnectionPool(object):
     def test_ca_certs_default_cert_required(self):
         with connection_from_url('https://google.com:80', ca_certs=DEFAULT_CA) as pool:
             conn = pool._get_conn()
-            assert conn.cert_reqs == 'CERT_REQUIRED'
+            assert conn.cert_reqs == ssl.CERT_REQUIRED
 
     def test_cleanup_on_extreme_connection_error(self):
         """
diff --git a/test/test_util.py b/test/test_util.py
index 850bf3fd..508cb10b 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -473,7 +473,7 @@ class TestUtil(object):
         assert timeout.get_connect_duration() == 37
 
     @pytest.mark.parametrize('candidate, requirements', [
-        (None, ssl.CERT_NONE),
+        (None, ssl.CERT_REQUIRED),
         (ssl.CERT_NONE, ssl.CERT_NONE),
         (ssl.CERT_REQUIRED, ssl.CERT_REQUIRED),
         ('REQUIRED', ssl.CERT_REQUIRED),
diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py
index f404d613..648b5561 100644
--- a/test/with_dummyserver/test_https.py
+++ b/test/with_dummyserver/test_https.py
@@ -31,7 +31,6 @@ from test import (
 from urllib3 import HTTPSConnectionPool
 from urllib3.connection import (
     VerifiedHTTPSConnection,
-    UnverifiedHTTPSConnection,
     RECENT_DATE,
 )
 from urllib3.exceptions import (
@@ -60,7 +59,7 @@ log.addHandler(logging.StreamHandler(sys.stdout))
 
 class TestHTTPS(HTTPSDummyServerTestCase):
     def setUp(self):
-        self._pool = HTTPSConnectionPool(self.host, self.port)
+        self._pool = HTTPSConnectionPool(self.host, self.port, ca_certs=DEFAULT_CA)
         self.addCleanup(self._pool.close)
 
     def test_simple(self):
@@ -69,7 +68,7 @@ class TestHTTPS(HTTPSDummyServerTestCase):
 
     @fails_on_travis_gce
     def test_dotted_fqdn(self):
-        pool = HTTPSConnectionPool(self.host + '.', self.port)
+        pool = HTTPSConnectionPool(self.host + '.', self.port, ca_certs=DEFAULT_CA)
         r = pool.request('GET', '/')
         self.assertEqual(r.status, 200, r.data)
 
@@ -85,7 +84,8 @@ class TestHTTPS(HTTPSDummyServerTestCase):
         )
         https_pool = HTTPSConnectionPool(self.host, self.port,
                                          key_file=client_key,
-                                         cert_file=client_cert)
+                                         cert_file=client_cert,
+                                         ca_certs=DEFAULT_CA)
         r = https_pool.request('GET', '/certificate')
         subject = json.loads(r.data.decode('utf-8'))
         assert subject['organizationalUnitName'].startswith(
@@ -98,7 +98,8 @@ class TestHTTPS(HTTPSDummyServerTestCase):
         )
         https_pool = HTTPSConnectionPool(self.host, self.port,
                                          cert_file=client_cert,
-                                         key_file=client_key)
+                                         key_file=client_key,
+                                         ca_certs=DEFAULT_CA)
         try:
             https_pool.request('GET', '/certificate', retries=False)
         except SSLError as e:
@@ -121,6 +122,7 @@ class TestHTTPS(HTTPSDummyServerTestCase):
             PASSWORD_CLIENT_KEYFILE,
         )
         https_pool = HTTPSConnectionPool(self.host, self.port,
+                                         ca_certs=DEFAULT_CA,
                                          key_file=client_key,
                                          cert_file=client_cert,
                                          key_password="letmein")
@@ -310,8 +312,7 @@ class TestHTTPS(HTTPSDummyServerTestCase):
 
     def test_unverified_ssl(self):
         """ Test that bare HTTPSConnection can connect, make requests """
-        pool = HTTPSConnectionPool(self.host, self.port)
-        pool.ConnectionCls = UnverifiedHTTPSConnection
+        pool = HTTPSConnectionPool(self.host, self.port, cert_reqs=ssl.CERT_NONE)
         self.addCleanup(pool.close)
 
         with mock.patch('warnings.warn') as warn:
@@ -551,15 +552,14 @@ class TestHTTPS(HTTPSDummyServerTestCase):
     def test_enhanced_ssl_connection(self):
         fingerprint = '92:81:FE:85:F7:0C:26:60:EC:D6:B3:BF:93:CF:F9:71:CC:07:7D:0A'
 
-        conn = VerifiedHTTPSConnection(self.host, self.port)
-        self.addCleanup(conn.close)
         https_pool = HTTPSConnectionPool(self.host, self.port,
                                          cert_reqs='CERT_REQUIRED',
                                          ca_certs=DEFAULT_CA,
                                          assert_fingerprint=fingerprint)
         self.addCleanup(https_pool.close)
 
-        https_pool._make_request(conn, 'GET', '/')
+        r = https_pool.request('GET', '/')
+        assert r.status == 200
 
     @onlyPy279OrNewer
     def test_ssl_correct_system_time(self):
@@ -610,8 +610,8 @@ class TestHTTPS_TLSv1(HTTPSDummyServerTestCase):
 
     def test_set_cert_default_cert_required(self):
         conn = VerifiedHTTPSConnection(self.host, self.port)
-        conn.set_cert(ca_certs=DEFAULT_CA)
-        self.assertEqual(conn.cert_reqs, 'CERT_REQUIRED')
+        conn.set_cert()
+        self.assertEqual(conn.cert_reqs, ssl.CERT_REQUIRED)
 
 
 class TestHTTPS_NoSAN(HTTPSDummyServerTestCase):
diff --git a/test/with_dummyserver/test_no_ssl.py b/test/with_dummyserver/test_no_ssl.py
index 282c9faa..f52999f9 100644
--- a/test/with_dummyserver/test_no_ssl.py
+++ b/test/with_dummyserver/test_no_ssl.py
@@ -21,7 +21,7 @@ class TestHTTPWithoutSSL(HTTPDummyServerTestCase, TestWithoutSSL):
 
 class TestHTTPSWithoutSSL(HTTPSDummyServerTestCase, TestWithoutSSL):
     def test_simple(self):
-        pool = urllib3.HTTPSConnectionPool(self.host, self.port)
+        pool = urllib3.HTTPSConnectionPool(self.host, self.port, cert_reqs="NONE")
         self.addCleanup(pool.close)
         try:
             pool.request('GET', '/')
diff --git a/test/with_dummyserver/test_proxy_poolmanager.py b/test/with_dummyserver/test_proxy_poolmanager.py
index 225f5c45..0a478514 100644
--- a/test/with_dummyserver/test_proxy_poolmanager.py
+++ b/test/with_dummyserver/test_proxy_poolmanager.py
@@ -28,7 +28,7 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
         self.proxy_url = 'http://%s:%d' % (self.proxy_host, self.proxy_port)
 
     def test_basic_proxy(self):
-        http = proxy_from_url(self.proxy_url)
+        http = proxy_from_url(self.proxy_url, ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         r = http.request('GET', '%s/' % self.http_url)
@@ -66,7 +66,7 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
             self.assertEqual(type(e.reason), ProxyError)
 
     def test_oldapi(self):
-        http = ProxyManager(connection_from_url(self.proxy_url))
+        http = ProxyManager(connection_from_url(self.proxy_url), ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         r = http.request('GET', '%s/' % self.http_url)
@@ -146,7 +146,7 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
         self.assertNotEqual(r._pool.host, self.http_host_alt)
 
     def test_cross_protocol_redirect(self):
-        http = proxy_from_url(self.proxy_url)
+        http = proxy_from_url(self.proxy_url, ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         cross_protocol_location = '%s/echo?a=b' % self.https_url
@@ -166,7 +166,8 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
 
     def test_headers(self):
         http = proxy_from_url(self.proxy_url, headers={'Foo': 'bar'},
-                              proxy_headers={'Hickory': 'dickory'})
+                              proxy_headers={'Hickory': 'dickory'},
+                              ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         r = http.request_encode_url('GET', '%s/headers' % self.http_url)
@@ -190,13 +191,6 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
         self.assertEqual(returned_headers.get('Host'),
                          '%s:%s' % (self.https_host, self.https_port))
 
-        r = http.request_encode_url('GET', '%s/headers' % self.https_url_alt)
-        returned_headers = json.loads(r.data.decode())
-        self.assertEqual(returned_headers.get('Foo'), 'bar')
-        self.assertIsNone(returned_headers.get('Hickory'))
-        self.assertEqual(returned_headers.get('Host'),
-                         '%s:%s' % (self.https_host_alt, self.https_port))
-
         r = http.request_encode_body('POST', '%s/headers' % self.http_url)
         returned_headers = json.loads(r.data.decode())
         self.assertEqual(returned_headers.get('Foo'), 'bar')
@@ -254,7 +248,7 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
         self.assertEqual(returned_headers.get('Baz'), 'quux')
 
     def test_proxy_pooling(self):
-        http = proxy_from_url(self.proxy_url)
+        http = proxy_from_url(self.proxy_url, cert_reqs='NONE')
         self.addCleanup(http.clear)
 
         for x in range(2):
@@ -320,7 +314,7 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
 
     def test_scheme_host_case_insensitive(self):
         """Assert that upper-case schemes and hosts are normalized."""
-        http = proxy_from_url(self.proxy_url.upper())
+        http = proxy_from_url(self.proxy_url.upper(), ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         r = http.request('GET', '%s/' % self.http_url.upper())
@@ -342,7 +336,7 @@ class TestIPv6HTTPProxyManager(IPv6HTTPDummyProxyTestCase):
         self.proxy_url = 'http://[%s]:%d' % (self.proxy_host, self.proxy_port)
 
     def test_basic_ipv6_proxy(self):
-        http = proxy_from_url(self.proxy_url)
+        http = proxy_from_url(self.proxy_url, ca_certs=DEFAULT_CA)
         self.addCleanup(http.clear)
 
         r = http.request('GET', '%s/' % self.http_url)
diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py
index e209607c..0274e6e0 100644
--- a/test/with_dummyserver/test_socketlevel.py
+++ b/test/with_dummyserver/test_socketlevel.py
@@ -972,7 +972,7 @@ class TestProxyManager(SocketDummyServerTestCase):
         self._start_server(echo_socket_handler)
         base_url = 'http://%s:%d' % (self.host, self.port)
 
-        proxy = proxy_from_url(base_url)
+        proxy = proxy_from_url(base_url, ca_certs=DEFAULT_CA)
         self.addCleanup(proxy.clear)
 
         url = 'https://{0}'.format(self.host)
@@ -1016,7 +1016,7 @@ class TestProxyManager(SocketDummyServerTestCase):
         self._start_server(echo_socket_handler)
         base_url = 'http://%s:%d' % (self.host, self.port)
 
-        proxy = proxy_from_url(base_url)
+        proxy = proxy_from_url(base_url, cert_reqs='NONE')
         self.addCleanup(proxy.clear)
 
         url = 'https://[{0}]'.format(ipv6_addr)
@@ -1070,8 +1070,7 @@ class TestSSL(SocketDummyServerTestCase):
             ssl_sock = ssl.wrap_socket(sock,
                                        server_side=True,
                                        keyfile=DEFAULT_CERTS['keyfile'],
-                                       certfile=DEFAULT_CERTS['certfile'],
-                                       ca_certs=DEFAULT_CA)
+                                       certfile=DEFAULT_CERTS['certfile'])
 
             buf = b''
             while not buf.endswith(b'\r\n\r\n'):
@@ -1090,7 +1089,7 @@ class TestSSL(SocketDummyServerTestCase):
             ssl_sock.close()
 
         self._start_server(socket_handler)
-        pool = HTTPSConnectionPool(self.host, self.port)
+        pool = HTTPSConnectionPool(self.host, self.port, ca_certs=DEFAULT_CA)
         self.addCleanup(pool.close)
 
         response = pool.urlopen('GET', '/', retries=0, preload_content=False,
@@ -1180,7 +1179,7 @@ class TestSSL(SocketDummyServerTestCase):
 
         self._start_server(socket_handler)
 
-        pool = HTTPSConnectionPool(self.host, self.port)
+        pool = HTTPSConnectionPool(self.host, self.port, ca_certs=DEFAULT_CA)
         self.addCleanup(pool.close)
         response = pool.urlopen('GET', '/', retries=1)
         self.assertEqual(response.data, b'Success')