Changing the default value for ssl_check_hostname to True, to ensure security validations are not skipped by default (redis#3626)

* Changing the default value for ssl_check_hostname to True, to ensure security validations  are not skipped by default

* Applying review comments

* Removing unused operation in tests.

* Removing unneeded comment from tests.

Author: petyaslavova

docs/examples/ssl_connection_examples.ipynb

@@ -34,9 +34,10 @@
     "import redis\n",
     "\n",
     "r = redis.Redis(\n",
-    "    host='localhost', \n",
-    "    port=6666, \n",
-    "    ssl=True, \n",
+    "    host='localhost',\n",
+    "    port=6666,\n",
+    "    ssl=True,\n",
+    "    ssl_check_hostname=False,\n",
     "    ssl_cert_reqs=\"none\",\n",
     ")\n",
     "r.ping()"
@@ -68,7 +69,7 @@
    "source": [
     "import redis\n",
     "\n",
-    "r = redis.from_url(\"rediss://localhost:6666?ssl_cert_reqs=none&decode_responses=True&health_check_interval=2\")\n",
+    "r = redis.from_url(\"rediss://localhost:6666?ssl_cert_reqs=none&ssl_check_hostname=False&decode_responses=True&health_check_interval=2\")\n",
     "r.ping()"
    ]
   },
@@ -99,13 +100,14 @@
     "import redis\n",
     "\n",
     "redis_pool = redis.ConnectionPool(\n",
-    "    host=\"localhost\", \n",
-    "    port=6666, \n",
-    "    connection_class=redis.SSLConnection, \n",
+    "    host=\"localhost\",\n",
+    "    port=6666,\n",
+    "    connection_class=redis.SSLConnection,\n",
+    "    ssl_check_hostname=False,\n",
     "    ssl_cert_reqs=\"none\",\n",
     ")\n",
     "\n",
-    "r = redis.StrictRedis(connection_pool=redis_pool) \n",
+    "r = redis.StrictRedis(connection_pool=redis_pool)\n",
     "r.ping()"
    ]
   },
@@ -141,6 +143,7 @@
     "    port=6666,\n",
     "    ssl=True,\n",
     "    ssl_min_version=ssl.TLSVersion.TLSv1_3,\n",
+    "    ssl_check_hostname=False,\n",
     "    ssl_cert_reqs=\"none\",\n",
     ")\n",
     "r.ping()"

redis/asyncio/client.py

@@ -241,7 +241,7 @@ def __init__(
         ssl_cert_reqs: Union[str, VerifyMode] = "required",
         ssl_ca_certs: Optional[str] = None,
         ssl_ca_data: Optional[str] = None,
-        ssl_check_hostname: bool = False,
+        ssl_check_hostname: bool = True,
         ssl_min_version: Optional[TLSVersion] = None,
         ssl_ciphers: Optional[str] = None,
         max_connections: Optional[int] = None,

redis/asyncio/cluster.py

@@ -289,7 +289,7 @@ def __init__(
         ssl_ca_data: Optional[str] = None,
         ssl_cert_reqs: Union[str, VerifyMode] = "required",
         ssl_certfile: Optional[str] = None,
-        ssl_check_hostname: bool = False,
+        ssl_check_hostname: bool = True,
         ssl_keyfile: Optional[str] = None,
         ssl_min_version: Optional[TLSVersion] = None,
         ssl_ciphers: Optional[str] = None,

redis/asyncio/connection.py

@@ -794,7 +794,7 @@ def __init__(
         ssl_cert_reqs: Union[str, ssl.VerifyMode] = "required",
         ssl_ca_certs: Optional[str] = None,
         ssl_ca_data: Optional[str] = None,
-        ssl_check_hostname: bool = False,
+        ssl_check_hostname: bool = True,
         ssl_min_version: Optional[TLSVersion] = None,
         ssl_ciphers: Optional[str] = None,
         **kwargs,

redis/client.py

@@ -223,7 +223,7 @@ def __init__(
         ssl_ca_certs: Optional[str] = None,
         ssl_ca_path: Optional[str] = None,
         ssl_ca_data: Optional[str] = None,
-        ssl_check_hostname: bool = False,
+        ssl_check_hostname: bool = True,
         ssl_password: Optional[str] = None,
         ssl_validate_ocsp: bool = False,
         ssl_validate_ocsp_stapled: bool = False,

redis/connection.py

@@ -1028,7 +1028,7 @@ def __init__(
         ssl_cert_reqs="required",
         ssl_ca_certs=None,
         ssl_ca_data=None,
-        ssl_check_hostname=False,
+        ssl_check_hostname=True,
         ssl_ca_path=None,
         ssl_password=None,
         ssl_validate_ocsp=False,

tests/test_asyncio/test_cluster.py

@@ -3139,7 +3139,9 @@ async def test_ssl_with_invalid_cert(
     async def test_ssl_connection(
         self, create_client: Callable[..., Awaitable[RedisCluster]]
     ) -> None:
-        async with await create_client(ssl=True, ssl_cert_reqs="none") as rc:
+        async with await create_client(
+            ssl=True, ssl_check_hostname=False, ssl_cert_reqs="none"
+        ) as rc:
             assert await rc.ping()
 
     @pytest.mark.parametrize(
@@ -3155,6 +3157,7 @@ async def test_ssl_connection_tls12_custom_ciphers(
     ) -> None:
         async with await create_client(
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_2,
             ssl_ciphers=ssl_ciphers,
@@ -3166,6 +3169,7 @@ async def test_ssl_connection_tls12_custom_ciphers_invalid(
     ) -> None:
         async with await create_client(
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_2,
             ssl_ciphers="foo:bar",
@@ -3187,6 +3191,7 @@ async def test_ssl_connection_tls13_custom_ciphers(
         # TLSv1.3 does not support changing the ciphers
         async with await create_client(
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_2,
             ssl_ciphers=ssl_ciphers,
@@ -3198,12 +3203,20 @@ async def test_ssl_connection_tls13_custom_ciphers(
     async def test_validating_self_signed_certificate(
         self, create_client: Callable[..., Awaitable[RedisCluster]]
     ) -> None:
+        # ssl_check_hostname=False is used to avoid hostname verification
+        # in the test environment, where the server certificate is self-signed
+        # and does not match the hostname that is extracted for the cluster.
+        # Cert hostname is 'localhost' in the cluster initialization when using
+        # 'localhost' it gets transformed into 127.0.0.1
+        # In production code, ssl_check_hostname should be set to True
+        # to ensure proper hostname verification.
         async with await create_client(
             ssl=True,
             ssl_ca_certs=self.ca_cert,
             ssl_cert_reqs="required",
             ssl_certfile=self.client_cert,
             ssl_keyfile=self.client_key,
+            ssl_check_hostname=False,
         ) as rc:
             assert await rc.ping()
 
@@ -3213,10 +3226,18 @@ async def test_validating_self_signed_string_certificate(
         with open(self.ca_cert) as f:
             cert_data = f.read()
 
+        # ssl_check_hostname=False is used to avoid hostname verification
+        # in the test environment, where the server certificate is self-signed
+        # and does not match the hostname that is extracted for the cluster.
+        # Cert hostname is 'localhost' in the cluster initialization when using
+        # 'localhost' it gets transformed into 127.0.0.1
+        # In production code, ssl_check_hostname should be set to True
+        # to ensure proper hostname verification.
         async with await create_client(
             ssl=True,
             ssl_ca_data=cert_data,
             ssl_cert_reqs="required",
+            ssl_check_hostname=False,
             ssl_certfile=self.client_cert,
             ssl_keyfile=self.client_key,
         ) as rc:

tests/test_asyncio/test_connect.py

@@ -58,6 +58,10 @@ async def test_uds_connect(uds_address):
 async def test_tcp_ssl_tls12_custom_ciphers(tcp_address, ssl_ciphers):
     host, port = tcp_address
 
+    # in order to have working hostname verification, we need to use "localhost"
+    # as redis host as the server certificate is self-signed and only valid for "localhost"
+    host = "localhost"
+
     server_certs = get_tls_certificates(cert_type=CertificateType.server)
 
     conn = SSLConnection(
@@ -89,6 +93,10 @@ async def test_tcp_ssl_tls12_custom_ciphers(tcp_address, ssl_ciphers):
 async def test_tcp_ssl_connect(tcp_address, ssl_min_version):
     host, port = tcp_address
 
+    # in order to have working hostname verification, we need to use "localhost"
+    # as redis host as the server certificate is self-signed and only valid for "localhost"
+    host = "localhost"
+
     server_certs = get_tls_certificates(cert_type=CertificateType.server)
 
     conn = SSLConnection(
@@ -100,7 +108,10 @@ async def test_tcp_ssl_connect(tcp_address, ssl_min_version):
         ssl_min_version=ssl_min_version,
     )
     await _assert_connect(
-        conn, tcp_address, certfile=server_certs.certfile, keyfile=server_certs.keyfile
+        conn,
+        tcp_address,
+        certfile=server_certs.certfile,
+        keyfile=server_certs.keyfile,
     )
     await conn.disconnect()
 

tests/test_connect.py

@@ -54,10 +54,16 @@ def test_uds_connect(uds_address):
 )
 def test_tcp_ssl_connect(tcp_address, ssl_min_version):
     host, port = tcp_address
+
+    # in order to have working hostname verification, we need to use "localhost"
+    # as redis host as the server certificate is self-signed and only valid for "localhost"
+    host = "localhost"
     server_certs = get_tls_certificates(cert_type=CertificateType.server)
+
     conn = SSLConnection(
         host=host,
         port=port,
+        ssl_check_hostname=True,
         client_name=_CLIENT_NAME,
         ssl_ca_certs=server_certs.ca_certfile,
         socket_timeout=10,
@@ -80,6 +86,10 @@ def test_tcp_ssl_connect(tcp_address, ssl_min_version):
 def test_tcp_ssl_tls12_custom_ciphers(tcp_address, ssl_ciphers):
     host, port = tcp_address
 
+    # in order to have working hostname verification, we need to use "localhost"
+    # as redis host as the server certificate is self-signed and only valid for "localhost"
+    host = "localhost"
+
     server_certs = get_tls_certificates(cert_type=CertificateType.server)
 
     conn = SSLConnection(

tests/test_ssl.py

@@ -37,7 +37,14 @@ def test_ssl_with_invalid_cert(self, request):
     def test_ssl_connection(self, request):
         ssl_url = request.config.option.redis_ssl_url
         p = urlparse(ssl_url)[1].split(":")
-        r = redis.Redis(host=p[0], port=p[1], ssl=True, ssl_cert_reqs="none")
+
+        r = redis.Redis(
+            host=p[0],
+            port=p[1],
+            ssl=True,
+            ssl_check_hostname=False,
+            ssl_cert_reqs="none",
+        )
         assert r.ping()
         r.close()
 
@@ -98,6 +105,7 @@ def test_ssl_connection_tls12_custom_ciphers(self, request, ssl_ciphers):
             host=p[0],
             port=p[1],
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_3,
             ssl_ciphers=ssl_ciphers,
@@ -112,6 +120,7 @@ def test_ssl_connection_tls12_custom_ciphers_invalid(self, request):
             host=p[0],
             port=p[1],
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_2,
             ssl_ciphers="foo:bar",
@@ -136,6 +145,7 @@ def test_ssl_connection_tls13_custom_ciphers(self, request, ssl_ciphers):
             host=p[0],
             port=p[1],
             ssl=True,
+            ssl_check_hostname=False,
             ssl_cert_reqs="none",
             ssl_min_version=ssl.TLSVersion.TLSv1_2,
             ssl_ciphers=ssl_ciphers,