Fix RedisCluster ssl_check_hostname not set to connections. For SSL verification with ssl_cert_reqs="none", check_hostname is set to False (#3637)

* Fix SSL verification with ssl_cert_reqs=none and ssl_check_hostname=True

* Add ssl_check_hostname to REDIS_ALLOWED_KEYS and fix default value in RedisSSLContext

Author: omerfeyzioglu

CHANGES

@@ -71,6 +71,7 @@
     * Close Unix sockets if the connection attempt fails. This prevents `ResourceWarning`s. (#3314)
     * Close SSL sockets if the connection attempt fails, or if validations fail. (#3317)
     * Eliminate mutable default arguments in the `redis.commands.core.Script` class. (#3332)
+    * Fix SSL verification with `ssl_cert_reqs="none"` and `ssl_check_hostname=True` by automatically setting `check_hostname=False` when `verify_mode=ssl.CERT_NONE` (#3635) 
     * Allow newer versions of PyJWT as dependency. (#3630)
 
 * 4.1.3 (Feb 8, 2022)

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

@@ -868,7 +868,7 @@ def __init__(
         cert_reqs: Optional[Union[str, ssl.VerifyMode]] = None,
         ca_certs: Optional[str] = None,
         ca_data: Optional[str] = None,
-        check_hostname: bool = False,
+        check_hostname: bool = True,
         min_version: Optional[TLSVersion] = None,
         ciphers: Optional[str] = None,
     ):
@@ -893,7 +893,9 @@ 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

redis/cluster.py

@@ -185,6 +185,7 @@ def parse_cluster_myshardid(resp, **options):
     "ssl_cert_reqs",
     "ssl_keyfile",
     "ssl_password",
+    "ssl_check_hostname",
     "unix_socket_path",
     "username",
     "cache",

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

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

@@ -0,0 +1,56 @@
+from urllib.parse import urlparse
+import pytest
+import pytest_asyncio
+import redis.asyncio as redis
+
+# 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"
+    )
+
+
+@pytest.mark.ssl
+class TestSSL:
+    """Tests for SSL connections in asyncio."""
+
+    @pytest_asyncio.fixture()
+    async def _get_client(self, request):
+        ssl_url = request.config.option.redis_ssl_url
+        p = urlparse(ssl_url)[1].split(":")
+        client = redis.Redis(host=p[0], port=p[1], ssl=True)
+        yield client
+        await client.aclose()
+
+    async def test_ssl_with_invalid_cert(self, _get_client):
+        """Test SSL connection with invalid certificate."""
+        pass
+
+    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
+        parsed_url = urlparse(ssl_url)
+        r = redis.Redis(
+            host=parsed_url.hostname,
+            port=parsed_url.port,
+            ssl=True,
+            ssl_cert_reqs="none",
+            # Check that ssl_check_hostname is ignored, when ssl_cert_reqs=none
+            ssl_check_hostname=True,
+        )
+        try:
+            # Connection should be successful
+            assert await r.ping()
+            # check_hostname should have been automatically set to False
+            assert r.connection_pool.connection_class == redis.SSLConnection
+            conn = r.connection_pool.make_connection()
+            assert conn.check_hostname is False
+        finally:
+            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,
@@ -309,3 +305,26 @@ def test_mock_ocsp_staple(self, request):
             r.ping()
         assert "no ocsp response present" in str(e)
         r.close()
+
+    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
+        parsed_url = urlparse(ssl_url)
+        r = redis.Redis(
+            host=parsed_url.hostname,
+            port=parsed_url.port,
+            ssl=True,
+            ssl_cert_reqs="none",
+            # 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_class == redis.SSLConnection
+            conn = r.connection_pool.make_connection()
+            assert conn.check_hostname is False
+        finally:
+            r.close()