-
Notifications
You must be signed in to change notification settings - Fork 560
PYTHON-1331 ssl.match_hostname() is deprecated in 3.7 #1191
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
187b7cb
Updating comment to reflect changes in the previous commit
absurdfarce e54ff8c
Revert "Updating comment to reflect changes in the previous commit"
absurdfarce daac2c8
Heart of the fix, or at least my initial attempt at it
absurdfarce f94494d
Move pynose into test-dependencies rather than directly imported in J…
absurdfarce d66f4f9
A couple minor fixes
absurdfarce fca6eda
Fixes based on local testing
absurdfarce 3c73dd9
ssl_version passed to SSLContext must be an int
absurdfarce 2eab3f3
Fix to minor _build_ssl_context_from_options issue
absurdfarce 8de9b03
Hey, let's not use SSL when we don't mean to
absurdfarce 37823d1
Don't install deps for nose plugins in order to avoid overwriting pyn…
absurdfarce 96401f7
Be more judicious about how we construct args for the SSLContext cons…
absurdfarce cd94fe1
Install test-requirements in the Python 3.12 + DSE case so we can at …
absurdfarce 7b42e5f
Merge branch 'master' into python1331
absurdfarce File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -733,7 +733,6 @@ class Connection(object): | |
_socket = None | ||
|
||
_socket_impl = socket | ||
_ssl_impl = ssl | ||
|
||
_check_hostname = False | ||
_product_type = None | ||
|
@@ -757,7 +756,7 @@ def __init__(self, host='127.0.0.1', port=9042, authenticator=None, | |
self.endpoint = host if isinstance(host, EndPoint) else DefaultEndPoint(host, port) | ||
|
||
self.authenticator = authenticator | ||
self.ssl_options = ssl_options.copy() if ssl_options else None | ||
self.ssl_options = ssl_options.copy() if ssl_options else {} | ||
self.ssl_context = ssl_context | ||
self.sockopts = sockopts | ||
self.compression = compression | ||
|
@@ -777,15 +776,20 @@ def __init__(self, host='127.0.0.1', port=9042, authenticator=None, | |
self._on_orphaned_stream_released = on_orphaned_stream_released | ||
|
||
if ssl_options: | ||
self._check_hostname = bool(self.ssl_options.pop('check_hostname', False)) | ||
if self._check_hostname: | ||
if not getattr(ssl, 'match_hostname', None): | ||
raise RuntimeError("ssl_options specify 'check_hostname', but ssl.match_hostname is not provided. " | ||
"Patch or upgrade Python to use this option.") | ||
self.ssl_options.update(self.endpoint.ssl_options or {}) | ||
elif self.endpoint.ssl_options: | ||
self.ssl_options = self.endpoint.ssl_options | ||
|
||
# PYTHON-1331 | ||
# | ||
# We always use SSLContext.wrap_socket() now but legacy configs may have other params that were passed to ssl.wrap_socket()... | ||
# and either could have 'check_hostname'. Remove these params into a separate map and use them to build an SSLContext if | ||
# we need to do so. | ||
# | ||
# Note the use of pop() here; we are very deliberately removing these params from ssl_options if they're present. After this | ||
# operation ssl_options should contain only args needed for the ssl_context.wrap_socket() call. | ||
if not self.ssl_context and self.ssl_options: | ||
self.ssl_context = self._build_ssl_context_from_options() | ||
|
||
if protocol_version >= 3: | ||
self.max_request_id = min(self.max_in_flight - 1, (2 ** 15) - 1) | ||
|
@@ -852,21 +856,57 @@ def factory(cls, endpoint, timeout, *args, **kwargs): | |
else: | ||
return conn | ||
|
||
def _build_ssl_context_from_options(self): | ||
|
||
# Extract a subset of names from self.ssl_options which apply to SSLContext creation | ||
ssl_context_opt_names = ['ssl_version', 'cert_reqs', 'check_hostname', 'keyfile', 'certfile', 'ca_certs', 'ciphers'] | ||
opts = {k:self.ssl_options.get(k, None) for k in ssl_context_opt_names if k in self.ssl_options} | ||
|
||
# Python >= 3.10 requires either PROTOCOL_TLS_CLIENT or PROTOCOL_TLS_SERVER so we'll get ahead of things by always | ||
# being explicit | ||
ssl_version = opts.get('ssl_version', None) or ssl.PROTOCOL_TLS_CLIENT | ||
cert_reqs = opts.get('cert_reqs', None) or ssl.CERT_REQUIRED | ||
rv = ssl.SSLContext(protocol=int(ssl_version)) | ||
rv.check_hostname = bool(opts.get('check_hostname', False)) | ||
rv.options = int(cert_reqs) | ||
|
||
certfile = opts.get('certfile', None) | ||
keyfile = opts.get('keyfile', None) | ||
if certfile: | ||
rv.load_cert_chain(certfile, keyfile) | ||
ca_certs = opts.get('ca_certs', None) | ||
if ca_certs: | ||
rv.load_verify_locations(ca_certs) | ||
ciphers = opts.get('ciphers', None) | ||
if ciphers: | ||
rv.set_ciphers(ciphers) | ||
|
||
return rv | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follows the idea roughly sketched out in Python docs for ssl.wrap_socket() |
||
|
||
def _wrap_socket_from_context(self): | ||
ssl_options = self.ssl_options or {} | ||
|
||
# Extract a subset of names from self.ssl_options which apply to SSLContext.wrap_socket (or at least the parts | ||
# of it that don't involve building an SSLContext under the covers) | ||
wrap_socket_opt_names = ['server_side', 'do_handshake_on_connect', 'suppress_ragged_eofs', 'server_hostname'] | ||
opts = {k:self.ssl_options.get(k, None) for k in wrap_socket_opt_names if k in self.ssl_options} | ||
|
||
# PYTHON-1186: set the server_hostname only if the SSLContext has | ||
# check_hostname enabled and it is not already provided by the EndPoint ssl options | ||
if (self.ssl_context.check_hostname and | ||
'server_hostname' not in ssl_options): | ||
ssl_options = ssl_options.copy() | ||
ssl_options['server_hostname'] = self.endpoint.address | ||
self._socket = self.ssl_context.wrap_socket(self._socket, **ssl_options) | ||
#opts['server_hostname'] = self.endpoint.address | ||
if (self.ssl_context.check_hostname and 'server_hostname' not in opts): | ||
server_hostname = self.endpoint.address | ||
opts['server_hostname'] = server_hostname | ||
|
||
return self.ssl_context.wrap_socket(self._socket, **opts) | ||
|
||
def _initiate_connection(self, sockaddr): | ||
self._socket.connect(sockaddr) | ||
|
||
def _match_hostname(self): | ||
ssl.match_hostname(self._socket.getpeercert(), self.endpoint.address) | ||
# PYTHON-1331 | ||
# | ||
# Allow implementations specific to an event loop to add additional behaviours | ||
def _validate_hostname(self): | ||
pass | ||
|
||
def _get_socket_addresses(self): | ||
address, port = self.endpoint.resolve() | ||
|
@@ -887,16 +927,18 @@ def _connect_socket(self): | |
try: | ||
self._socket = self._socket_impl.socket(af, socktype, proto) | ||
if self.ssl_context: | ||
self._wrap_socket_from_context() | ||
elif self.ssl_options: | ||
if not self._ssl_impl: | ||
raise RuntimeError("This version of Python was not compiled with SSL support") | ||
self._socket = self._ssl_impl.wrap_socket(self._socket, **self.ssl_options) | ||
self._socket = self._wrap_socket_from_context() | ||
self._socket.settimeout(self.connect_timeout) | ||
self._initiate_connection(sockaddr) | ||
self._socket.settimeout(None) | ||
|
||
# PYTHON-1331 | ||
# | ||
# Most checking is done via the check_hostname param on the SSLContext. | ||
# Subclasses can add additional behaviours via _validate_hostname() so | ||
# run that here. | ||
if self._check_hostname: | ||
self._match_hostname() | ||
self._validate_hostname() | ||
sockerr = None | ||
break | ||
except socket.error as err: | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
-r requirements.txt | ||
scales | ||
nose | ||
pynose | ||
mock>1.1 | ||
ccm>=2.1.2 | ||
pytz | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
A fix for an issue discovered in testing. Goal of the changes to support asyncore as an optional dependency was to raise an error if we were unable to find a workable event loop at all. Unfortunately my original impl just looked for the presence of any exception in any event loop impl which is... not at all the same thing.