Closed cboylan closed 1 year ago
Base: 94.66% // Head: 94.35% // Decreases project coverage by -0.32%
:warning:
Coverage data is based on head (
e9ae4f6
) compared to base (d7c44cd
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Prior to this commit python would produce these warnings when using kazoo with ssl:
The reason for this is that
ssl.PROTOCOL_SSLv23
is an alias forssl.PROTOCOL_TLS
andssl.PROTOCOL_TLS
is deprecated since Python 3.10.ssl.PROTOCOL_TLS
was replaced withssl.PROTOCOL_TLS_CLIENT
andssl.PROTOCOL_TLS_SERVER
. In kazoo's case we switch tossl.PROTOCOL_TLS_CLIENT
as kazoo is acting as an ssl client to zookeeper servers.There are a few things to note.
PROTOCOL_TLS_CLIENT
enablescontext.check_hostname
. We explicitly set this to False as this is required to setssl.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
withssl.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.
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_CLIENT
instead ofssl.PROTOCOL_SSLv23
ssl.CERT_OPTIONAL
withssl.CERT_REQUIRED
as 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.