Cleanup SSL deprecation warnings under Python 3.10 and newer #706
Conversation
Prior to this commit python would produce these warnings when using kazoo with ssl: /path/to/venv/lib/python3.11/site-packages/kazoo/handlers/utils.py:225: DeprecationWarning: ssl.PROTOCOL_TLS is deprecated context = ssl.SSLContext(ssl.PROTOCOL_SSLv23) The reason for this is that ssl.PROTOCOL_SSLv23 is an alias for ssl.PROTOCOL_TLS and ssl.PROTOCOL_TLS is deprecated since Python 3.10. ssl.PROTOCOL_TLS was replaced with ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER. In kazoo's case we switch to ssl.PROTOCOL_TLS_CLIENT as kazoo is acting as an ssl client to zookeeper servers. There are a few things to note. PROTOCOL_TLS_CLIENT enables context.check_hostname. We explicitly set this to False as this is required to set ssl.CHECK_NONE which kazoo supports, and not everyone may be using SSL certs with proper hostnames configured. For example if making connections to an IP address rather than a name and the certs don't have IP addrs in their altnames. This ensures backward compatibility with these use cases. Changing this should be done in a separate change and should likely be made configurable like verify_certs. Finally, while we are at it we replace ssl.CERT_OPTIONAL with ssl.CERT_REQUIRED as they are equivalent in a client context. This allows us to delete some code. Python documents all of these behaviors as being present since Python 3.6. Kazoo requires Python 3.7 or newer which should make this safe.
There was a problem hiding this comment.
Wow, this is one of the best drive-by PR descriptions I've read in a long time.
This is a textbook example of how to persuade/educate a maintainer on why it's safe to merge your patch, despite changing code that is higher risk due to potential security impact.
Thank you! 😍
| context.verify_mode = ( | ||
| ssl.CERT_OPTIONAL if verify_certs else ssl.CERT_NONE | ||
| ssl.CERT_REQUIRED if verify_certs else ssl.CERT_NONE |
There was a problem hiding this comment.
From PR description:
Finally, while we are at it we replace
ssl.CERT_OPTIONALwithssl.CERT_REQUIREDas they are equivalent in a client context. This allows us to delete some code.
Codecov ReportBase: 94.66% // Head: 94.35% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #706 +/- ##
==========================================
- Coverage 94.66% 94.35% -0.32%
==========================================
Files 57 57
Lines 8339 8339
==========================================
- Hits 7894 7868 -26
- Misses 445 471 +26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Prior to this commit python would produce these warnings when using kazoo with ssl:
The reason for this is that
ssl.PROTOCOL_SSLv23is an alias forssl.PROTOCOL_TLSandssl.PROTOCOL_TLSis deprecated since Python 3.10.ssl.PROTOCOL_TLSwas replaced withssl.PROTOCOL_TLS_CLIENTandssl.PROTOCOL_TLS_SERVER. In kazoo's case we switch tossl.PROTOCOL_TLS_CLIENTas kazoo is acting as an ssl client to zookeeper servers.There are a few things to note.
PROTOCOL_TLS_CLIENTenablescontext.check_hostname. We explicitly set this to False as this is required to setssl.CHECK_NONEwhich kazoo supports, and not everyone may be using SSL certs with proper hostnames configured. For example if making connections to an IP address rather than a name and the certs don't have IP addrs in their altnames. This ensures backward compatibility with these use cases. Changing this should be done in a separate change and should likely be made configurable like verify_certs.Finally, while we are at it we replace
ssl.CERT_OPTIONALwithssl.CERT_REQUIREDas they are equivalent in a client context. This allows us to delete some code.Python documents all of these behaviors as being present since Python 3.6. Kazoo requires Python 3.7 or newer which should make this safe.
Why is this needed?
This change should avoid problems with future Python updates. It also cuts down on noise in things like test suites that use kazoo under python3.10 or newer which is nice for end users.
Proposed Changes
ssl.PROTOCOL_TLS_CLIENTinstead ofssl.PROTOCOL_SSLv23ssl.CERT_OPTIONALwithssl.CERT_REQUIREDas they are equivalent in this context. Doing so allows us to delete some codeDoes this PR introduce any breaking change?
I've intentionally tried to make this backward compatible by setting context.check_hostname to False preserving old behavior. I think any changes to this behavior should happen in a separate change that can more fully understand the impacts.