-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix RedisCluster ssl_check_hostname not set to connections. For SSL verification with ssl_cert_reqs="none", check_hostname is set to False. #3637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an SSL-related bug where using ssl_cert_reqs="none" alongside ssl_check_hostname=True would trigger an error during connection establishment. The changes ensure that when certificate verification is disabled (i.e., ssl_cert_reqs is set to "none"), the check_hostname flag is automatically set to False for both synchronous and asynchronous clients.
- Updated connection logic in both sync (redis/connection.py) and async (redis/asyncio/connection.py) modules to enforce check_hostname=False when ssl_cert_reqs is ssl.CERT_NONE.
- Added tests in tests/test_ssl.py and tests/test_asyncio/test_ssl.py to verify the fix in both sync and async contexts.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
tests/test_ssl.py | Added a synchronous test to verify automatic disabling of hostname checking for SSL connections. |
tests/test_asyncio/test_ssl.py | Added an asynchronous test to verify automatic disabling of hostname checking for SSL connections. |
redis/connection.py | Modified sync connection to disable hostname check when ssl_cert_reqs is ssl.CERT_NONE. |
redis/asyncio/connection.py | Modified async connection to disable hostname check when ssl_cert_reqs is ssl.CERT_NONE. |
Files not reviewed (1)
- CHANGES: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to set check_hostname=False
when cert_reqs
is None makes sense, but it doesn't fully address the underlying issue—it mostly masks it.
To resolve this properly, we should ensure that ssl_check_hostname
passed through kwargs
is propagated to the connection initialization. In cluster.py, there's a tuple called REDIS_ALLOWED_KEYS
- adding ssl_check_hostname
there would prevent it from being stripped from kwargs
.
Also, since this change affects behavior demonstrated in the examples, could you please take a look at the documentation updated in PR #3626 and remove the explicit ssl_check_hostname=False setting if it’s no longer necessary?
It will also be good if you could change the tests in which cert_reqs is set to none not to provide explicitly ssl_check_hostname=False.
@omerfeyzioglu, thank you so much for taking the time and putting in the effort to prepare this contribution — it's truly appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an SSL verification issue by ensuring that when ssl_cert_reqs is set to "none" (or ssl.CERT_NONE), the connection automatically disables hostname verification regardless of the explicit ssl_check_hostname setting. Key changes include:
- Removing explicit ssl_check_hostname parameters from test and example code.
- Updating both synchronous and asynchronous connection initializers to conditionally set check_hostname to False when certificate verification is disabled.
- Adding tests to verify that when ssl_cert_reqs is "none", hostname verification is automatically turned off.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/test_ssl.py | Removed explicit ssl_check_hostname setting and added a test to validate the automatic behavior. |
tests/test_asyncio/test_ssl.py | Added a new asynchronous test to verify that ssl_check_hostname is disabled when using ssl_cert_reqs="none". |
tests/test_asyncio/test_cluster.py | Removed the explicit ssl_check_hostname parameter for cluster tests. |
redis/connection.py | Modified check_hostname assignment to conditionally disable hostname verification based on cert_reqs. |
redis/asyncio/connection.py | Updated asynchronous connection constructor to adjust check_hostname similarly. |
docs/examples/ssl_connection_examples.ipynb | Removed ssl_check_hostname argument from SSL connection examples. |
CHANGES | Added changelog entry for the SSL verification fix. |
Comments suppressed due to low confidence (2)
redis/connection.py:1086
- [nitpick] Consider adding an inline comment explaining that check_hostname is automatically set to False when ssl_cert_reqs is set to ssl.CERT_NONE to clarify the rationale behind this behavior for future maintainers.
self.check_hostname = (ssl_check_hostname if self.cert_reqs != ssl.CERT_NONE else False)
redis/asyncio/connection.py:896
- [nitpick] Consider adding a brief comment that explains the automatic disabling of hostname verification when using ssl.CERT_NONE to help maintain clarity in asynchronous connection initialization.
self.check_hostname = (check_hostname if self.cert_reqs != ssl.CERT_NONE else False)
…erification with ssl_cert_reqs="none", check_hostname is set to False (redis#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
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Fix SSL verification with
ssl_cert_reqs="none"
andssl_check_hostname=True
Problem
When connecting to a Redis cluster with SSL enabled, the following error occurs when trying to set
ssl_check_hostname=True
along withssl_cert_reqs="none"
:Fail to establish redis connection: Redis Cluster cannot be connected. Please provide at least one reachable node: Cannot set verify_mode to CERT_NONE when check_hostname is enabled.
This is because of a limitation in Python's SSL library that prevents
check_hostname=True
from being used withverify_mode=ssl.CERT_NONE
.Solution
This PR fixes the issue by:
Automatically setting
check_hostname=False
whenverify_mode=ssl.CERT_NONE
in both:redis/asyncio/connection.py
for async clientsredis/connection.py
for sync clientsAdding tests to verify the fix works correctly in both sync and async modes.
Notes
This issue is particularly relevant after the 6.0.0 release which changed the default value of
ssl_check_hostname
toTrue
for improved security. This fix ensures a smoother transition for users who need to usessl_cert_reqs="none"
while maintaining compatibility with this new default setting.The fix ensures that when users set
ssl_cert_reqs="none"
(or usessl.CERT_NONE
), hostname verification will be automatically disabled to prevent this error, while still respecting explicitssl_check_hostname=False
settings when certificate verification is enabled.