Applying review comments.

Author: petyaslavova

docs/examples/ssl_connection_examples.ipynb

@@ -37,7 +37,6 @@
     "    host='localhost',\n",
     "    port=6666,\n",
     "    ssl=True,\n",
-    "    ssl_check_hostname=False,\n",
     "    ssl_cert_reqs=\"none\",\n",
     ")\n",
     "r.ping()"
@@ -69,7 +68,7 @@
    "source": [
     "import redis\n",
     "\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 = redis.from_url(\"rediss://localhost:6666?ssl_cert_reqs=none&decode_responses=True&health_check_interval=2\")\n",
     "r.ping()"
    ]
   },
@@ -103,7 +102,6 @@
     "    host=\"localhost\",\n",
     "    port=6666,\n",
     "    connection_class=redis.SSLConnection,\n",
-    "    ssl_check_hostname=False,\n",
     "    ssl_cert_reqs=\"none\",\n",
     ")\n",
     "\n",
@@ -143,7 +141,6 @@
     "    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/connection.py

@@ -893,18 +893,17 @@ def __init__(
         self.cert_reqs = cert_reqs
         self.ca_certs = ca_certs
         self.ca_data = ca_data
-        self.check_hostname = check_hostname
+        self.check_hostname = (
+            check_hostname if self.cert_reqs != ssl.CERT_NONE else False
+        )
         self.min_version = min_version
         self.ciphers = ciphers
         self.context: Optional[SSLContext] = None
 
     def get(self) -> SSLContext:
         if not self.context:
             context = ssl.create_default_context()
-            if self.cert_reqs == ssl.CERT_NONE:
-                context.check_hostname = False
-            else:
-                context.check_hostname = self.check_hostname
+            context.check_hostname = self.check_hostname
             context.verify_mode = self.cert_reqs
             if self.certfile and self.keyfile:
                 context.load_cert_chain(certfile=self.certfile, keyfile=self.keyfile)

redis/connection.py

@@ -1083,7 +1083,9 @@ def __init__(
         self.ca_certs = ssl_ca_certs
         self.ca_data = ssl_ca_data
         self.ca_path = ssl_ca_path
-        self.check_hostname = ssl_check_hostname
+        self.check_hostname = (
+            ssl_check_hostname if self.cert_reqs != ssl.CERT_NONE else False
+        )
         self.certificate_password = ssl_password
         self.ssl_validate_ocsp = ssl_validate_ocsp
         self.ssl_validate_ocsp_stapled = ssl_validate_ocsp_stapled
@@ -1115,10 +1117,7 @@ def _wrap_socket_with_ssl(self, sock):
             An SSL wrapped socket.
         """
         context = ssl.create_default_context()
-        if self.cert_reqs == ssl.CERT_NONE:
-            context.check_hostname = False
-        else:
-            context.check_hostname = self.check_hostname
+        context.check_hostname = self.check_hostname
         context.verify_mode = self.cert_reqs
         if self.certfile or self.keyfile:
             context.load_cert_chain(

tests/test_asyncio/test_cluster.py

@@ -3118,9 +3118,7 @@ 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_check_hostname=False, ssl_cert_reqs="none"
-        ) as rc:
+        async with await create_client(ssl=True, ssl_cert_reqs="none") as rc:
             assert await rc.ping()
 
     @pytest.mark.parametrize(
@@ -3136,7 +3134,6 @@ 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,
@@ -3148,7 +3145,6 @@ 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",
@@ -3170,7 +3166,6 @@ 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,

tests/test_asyncio/test_ssl.py

@@ -11,11 +11,15 @@
 # Skip test or not based on cryptography installation
 try:
     import cryptography  # noqa
+
     skip_if_cryptography = pytest.mark.skipif(False, reason="")
     skip_if_nocryptography = pytest.mark.skipif(False, reason="")
 except ImportError:
     skip_if_cryptography = pytest.mark.skipif(True, reason="cryptography not installed")
-    skip_if_nocryptography = pytest.mark.skipif(True, reason="cryptography not installed")
+    skip_if_nocryptography = pytest.mark.skipif(
+        True, reason="cryptography not installed"
+    )
+
 
 @pytest.mark.ssl
 class TestSSL:
@@ -37,13 +41,14 @@ async def test_cert_reqs_none_with_check_hostname(self, request):
         """Test that when ssl_cert_reqs=none is used with ssl_check_hostname=True,
         the connection is created successfully with check_hostname internally set to False"""
         ssl_url = request.config.option.redis_ssl_url
-        p = urlparse(ssl_url)[1].split(":")
+        parsed_url = urlparse(ssl_url)
         r = redis.Redis(
-            host=p[0],
-            port=p[1],
+            host=parsed_url.hostname,
+            port=parsed_url.port,
             ssl=True,
             ssl_cert_reqs="none",
-            ssl_check_hostname=True,  # This should work now because we handle the incompatibility
+            # Check that ssl_check_hostname is ignored, when ssl_cert_reqs=none
+            ssl_check_hostname=True,
         )
         try:
             # Connection should be successful
@@ -53,4 +58,4 @@ async def test_cert_reqs_none_with_check_hostname(self, request):
             conn = r.connection_pool.make_connection()
             assert conn.check_hostname is False
         finally:
-            await r.aclose() 
+            await r.aclose()

tests/test_ssl.py

@@ -42,7 +42,6 @@ def test_ssl_connection(self, request):
             host=p[0],
             port=p[1],
             ssl=True,
-            ssl_check_hostname=False,
             ssl_cert_reqs="none",
         )
         assert r.ping()
@@ -105,7 +104,6 @@ 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,
@@ -120,7 +118,6 @@ 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",
@@ -145,7 +142,6 @@ 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,
@@ -314,19 +310,20 @@ def test_cert_reqs_none_with_check_hostname(self, request):
         """Test that when ssl_cert_reqs=none is used with ssl_check_hostname=True,
         the connection is created successfully with check_hostname internally set to False"""
         ssl_url = request.config.option.redis_ssl_url
-        p = urlparse(ssl_url)[1].split(":")
+        parsed_url = urlparse(ssl_url)
         r = redis.Redis(
-            host=p[0],
-            port=p[1],
+            host=parsed_url.hostname,
+            port=parsed_url.port,
             ssl=True,
             ssl_cert_reqs="none",
-            ssl_check_hostname=True,  # This should work now because we handle the incompatibility
+            # Check that ssl_check_hostname is ignored, when ssl_cert_reqs=none
+            ssl_check_hostname=True,
         )
         try:
             # Connection should be successful
             assert r.ping()
             # check_hostname should have been automatically set to False
-            assert r.connection_pool.connection_kwargs["connection_class"] == redis.SSLConnection
+            assert r.connection_pool.connection_class == redis.SSLConnection
             conn = r.connection_pool.make_connection()
             assert conn.check_hostname is False
         finally: