From ec8c093caf51292bef318068f5654aaedfa40c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Feyzio=C4=9Flu?= Date: Wed, 7 May 2025 01:09:59 +0300 Subject: [PATCH 1/5] Fix SSL verification with ssl_cert_reqs=none and ssl_check_hostname=True --- CHANGES | 1 + redis/asyncio/connection.py | 5 ++- redis/connection.py | 5 ++- tests/test_asyncio/test_ssl.py | 56 ++++++++++++++++++++++++++++++++++ tests/test_ssl.py | 22 +++++++++++++ 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 tests/test_asyncio/test_ssl.py diff --git a/CHANGES b/CHANGES index 24b52c54db..d0ab4f520d 100644 --- a/CHANGES +++ b/CHANGES @@ -70,6 +70,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` (#3636) * 4.1.3 (Feb 8, 2022) * Fix flushdb and flushall (#1926) diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 77131ab951..54a6b3e34d 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -901,7 +901,10 @@ def __init__( def get(self) -> SSLContext: if not self.context: context = ssl.create_default_context() - context.check_hostname = self.check_hostname + if self.cert_reqs == ssl.CERT_NONE: + context.check_hostname = False + else: + 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) diff --git a/redis/connection.py b/redis/connection.py index dab45906d2..db58504d07 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1115,7 +1115,10 @@ def _wrap_socket_with_ssl(self, sock): An SSL wrapped socket. """ context = ssl.create_default_context() - context.check_hostname = self.check_hostname + if self.cert_reqs == ssl.CERT_NONE: + context.check_hostname = False + else: + context.check_hostname = self.check_hostname context.verify_mode = self.cert_reqs if self.certfile or self.keyfile: context.load_cert_chain( diff --git a/tests/test_asyncio/test_ssl.py b/tests/test_asyncio/test_ssl.py new file mode 100644 index 0000000000..3502780b0c --- /dev/null +++ b/tests/test_asyncio/test_ssl.py @@ -0,0 +1,56 @@ +import asyncio +import ssl +import socket +from urllib.parse import urlparse +import pytest +import pytest_asyncio +import redis.asyncio as redis +from redis.exceptions import RedisError, ConnectionError +import ssl + +# 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 + p = urlparse(ssl_url)[1].split(":") + r = redis.Redis( + host=p[0], + port=p[1], + ssl=True, + ssl_cert_reqs="none", + ssl_check_hostname=True, # This should work now because we handle the incompatibility + ) + 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() \ No newline at end of file diff --git a/tests/test_ssl.py b/tests/test_ssl.py index 5aa33353a8..439b0beb8c 100644 --- a/tests/test_ssl.py +++ b/tests/test_ssl.py @@ -309,3 +309,25 @@ 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 + p = urlparse(ssl_url)[1].split(":") + r = redis.Redis( + host=p[0], + port=p[1], + ssl=True, + ssl_cert_reqs="none", + ssl_check_hostname=True, # This should work now because we handle the incompatibility + ) + 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 + conn = r.connection_pool.make_connection() + assert conn.check_hostname is False + finally: + r.close() From ee988fd9b3b46e4c546995c15c58e91c9bacbc8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Feyzio=C4=9Flu?= Date: Wed, 7 May 2025 01:20:03 +0300 Subject: [PATCH 2/5] issue number fixed --- CHANGES | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index d0ab4f520d..e79854b978 100644 --- a/CHANGES +++ b/CHANGES @@ -70,7 +70,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` (#3636) + * 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) * 4.1.3 (Feb 8, 2022) * Fix flushdb and flushall (#1926) From 042599c4bf14779289eb7704dbdc26f774648d4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Feyzio=C4=9Flu?= Date: Wed, 7 May 2025 18:40:53 +0300 Subject: [PATCH 3/5] Add ssl_check_hostname to REDIS_ALLOWED_KEYS and fix default value in RedisSSLContext --- redis/asyncio/connection.py | 2 +- redis/cluster.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 54a6b3e34d..9a6dd463fb 100644 --- a/redis/asyncio/connection.py +++ b/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, ): diff --git a/redis/cluster.py b/redis/cluster.py index 99e5c37f9d..17bb1f3c27 100644 --- a/redis/cluster.py +++ b/redis/cluster.py @@ -174,6 +174,7 @@ def parse_cluster_myshardid(resp, **options): "ssl_cert_reqs", "ssl_keyfile", "ssl_password", + "ssl_check_hostname", "unix_socket_path", "username", "cache", From 0fe87e6fa48a94e8edf7a32535f734536f528cb1 Mon Sep 17 00:00:00 2001 From: Petya Slavova Date: Mon, 12 May 2025 15:38:19 +0300 Subject: [PATCH 4/5] Applying review comments. --- docs/examples/ssl_connection_examples.ipynb | 5 +---- redis/asyncio/connection.py | 9 ++++----- redis/connection.py | 9 ++++----- tests/test_asyncio/test_cluster.py | 7 +------ tests/test_asyncio/test_ssl.py | 17 +++++++++++------ tests/test_ssl.py | 15 ++++++--------- 6 files changed, 27 insertions(+), 35 deletions(-) diff --git a/docs/examples/ssl_connection_examples.ipynb b/docs/examples/ssl_connection_examples.ipynb index a09b87ec1f..3fcc7bc3cc 100644 --- a/docs/examples/ssl_connection_examples.ipynb +++ b/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()" diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 9a6dd463fb..d1ae81d269 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -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 @@ -901,10 +903,7 @@ def __init__( 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) diff --git a/redis/connection.py b/redis/connection.py index db58504d07..cc805e442f 100644 --- a/redis/connection.py +++ b/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( diff --git a/tests/test_asyncio/test_cluster.py b/tests/test_asyncio/test_cluster.py index aee9ebc86e..91e8b58a82 100644 --- a/tests/test_asyncio/test_cluster.py +++ b/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, diff --git a/tests/test_asyncio/test_ssl.py b/tests/test_asyncio/test_ssl.py index 3502780b0c..0befa2388e 100644 --- a/tests/test_asyncio/test_ssl.py +++ b/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() \ No newline at end of file + await r.aclose() diff --git a/tests/test_ssl.py b/tests/test_ssl.py index 439b0beb8c..cb3f227629 100644 --- a/tests/test_ssl.py +++ b/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: From f6c68ce73e941c42071eb993810d87ed171ffef0 Mon Sep 17 00:00:00 2001 From: Petya Slavova Date: Mon, 12 May 2025 15:44:31 +0300 Subject: [PATCH 5/5] Fixing linters --- tests/test_asyncio/test_ssl.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/test_asyncio/test_ssl.py b/tests/test_asyncio/test_ssl.py index 0befa2388e..75800f22de 100644 --- a/tests/test_asyncio/test_ssl.py +++ b/tests/test_asyncio/test_ssl.py @@ -1,12 +1,7 @@ -import asyncio -import ssl -import socket from urllib.parse import urlparse import pytest import pytest_asyncio import redis.asyncio as redis -from redis.exceptions import RedisError, ConnectionError -import ssl # Skip test or not based on cryptography installation try: