ssl: improve pyOpenSSL hostname and context handling#941
Draft
dkropachev wants to merge 6 commits into
Draft
Conversation
Problem: several SSL paths used ssl_options truthiness, so explicit ssl_options={} was treated like omitted ssl_options=None. That disabled SSL setup in connection/reactor paths, skipped the ssl_options deprecation warning, and made Insights report SSL as disabled.
Fix: preserve whether ssl_options was supplied and expose a shared SSL-enabled predicate. Build default SSL contexts for explicit ssl_options on the standard path, use pyOpenSSL contexts for Twisted/Eventlet, and update Cluster and Insights checks to use explicit None comparisons. Add focused unit coverage for connection setup, client routes, Twisted/Eventlet, and Insights reporting.
This reverts commit f8c3ca4.
📝 WalkthroughWalkthroughIntroduces shared pyOpenSSL helpers for certificate parsing, hostname validation, TLS method selection, verification mapping, and SSL context construction. Cloud configuration and Eventlet/Twisted reactors now use these helpers. Hostname checks support SANs, wildcard DNS names, and exact IP matching. Tests cover TLS option handling, cloud context propagation, reactor certificate validation, and SSL contexts without Sequence Diagram(s)sequenceDiagram
participant Connection
participant Reactor
participant pyOpenSSL
Connection->>Reactor: provide shared TLS helpers
Reactor->>pyOpenSSL: build context and perform handshake
Reactor->>Connection: validate peer certificate hostname
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cassandra/datastax/cloud/__init__.py (1)
79-94: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConsider declaring
pyopenssl_contextas an explicit class attribute.
CloudConfigonly declaresssl_context = None(line 54), notpyopenssl_context, which is whygetattr(config, 'pyopenssl_context', config.ssl_context)is needed here to avoid anAttributeError. If the pyOpenSSL context is ever not built whilecreate_pyopenssl_context=True, this silently falls back to the stdlibssl.SSLContext, which the Eventlet/Twisted reactors don't expect. Declaringpyopenssl_context = Noneexplicitly would make the contract clearer than relying ongetattrmasking.♻️ Suggested clarification
host = None port = None keyspace = None local_dc = None ssl_context = None + pyopenssl_context = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/datastax/cloud/__init__.py` around lines 79 - 94, Declare pyopenssl_context = None explicitly on the CloudConfig class alongside ssl_context, then update get_cloud_config to access config.pyopenssl_context directly when create_pyopenssl_context is enabled. Preserve the existing context assignment behavior without using getattr as a fallback to the stdlib SSL context.
🧹 Nitpick comments (1)
cassandra/io/twistedreactor.py (1)
163-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead
verify_callbackmethod.This method is unused — context creation now always goes through
_build_pyopenssl_context_from_options, which installs its own internal verify callback. Leaving this in place is misleading, since it looks like an active hostname/cert verification hook but has no effect.♻️ Proposed removal
- def verify_callback(self, connection, x509, errnum, errdepth, ok): - return ok - def _hostname_to_verify(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/io/twistedreactor.py` around lines 163 - 164, Remove the unused verify_callback method from the relevant class, leaving _build_pyopenssl_context_from_options as the sole context verification setup. Do not alter the internal callback or other context behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cassandra/connection.py`:
- Around line 60-104: Update _validate_pyopenssl_hostname and
_pyopenssl_cert_to_ssl_cert to handle a None peer_cert from
get_peer_certificate(), raising the established ssl.CertificateError instead of
calling to_cryptography() and producing AttributeError. Ensure both
EventletConnection._validate_hostname and the Twisted handshake info_callback
retain this clear certificate-validation behavior without requiring separate
None checks.
---
Outside diff comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 79-94: Declare pyopenssl_context = None explicitly on the
CloudConfig class alongside ssl_context, then update get_cloud_config to access
config.pyopenssl_context directly when create_pyopenssl_context is enabled.
Preserve the existing context assignment behavior without using getattr as a
fallback to the stdlib SSL context.
---
Nitpick comments:
In `@cassandra/io/twistedreactor.py`:
- Around line 163-164: Remove the unused verify_callback method from the
relevant class, leaving _build_pyopenssl_context_from_options as the sole
context verification setup. Do not alter the internal callback or other context
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d1d8ea0-6096-420a-bdc4-c9e3a142e1c7
📒 Files selected for processing (10)
cassandra/cluster.pycassandra/connection.pycassandra/datastax/cloud/__init__.pycassandra/io/eventletreactor.pycassandra/io/twistedreactor.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_twistedreactor.pytests/unit/test_client_routes.pytests/unit/test_cloud.pytests/unit/test_connection.py
Comment on lines
+60
to
+104
| def _pyopenssl_cert_to_ssl_cert(peer_cert): | ||
| from cryptography import x509 | ||
| from cryptography.x509.oid import NameOID | ||
|
|
||
| cert = peer_cert.to_cryptography() | ||
| ssl_cert = {} | ||
|
|
||
| subject = tuple((('commonName', attr.value),) | ||
| for attr in cert.subject.get_attributes_for_oid(NameOID.COMMON_NAME)) | ||
| if subject: | ||
| ssl_cert['subject'] = subject | ||
|
|
||
| subject_alt_names = [] | ||
| try: | ||
| san = cert.extensions.get_extension_for_class(x509.SubjectAlternativeName).value | ||
| except x509.ExtensionNotFound: | ||
| pass | ||
| else: | ||
| subject_alt_names.extend(('DNS', name) for name in san.get_values_for_type(x509.DNSName)) | ||
| subject_alt_names.extend(('IP Address', str(address)) | ||
| for address in san.get_values_for_type(x509.IPAddress)) | ||
|
|
||
| if subject_alt_names: | ||
| ssl_cert['subjectAltName'] = tuple(subject_alt_names) | ||
|
|
||
| return ssl_cert | ||
|
|
||
|
|
||
| def _validate_pyopenssl_hostname(peer_cert, hostname): | ||
| cert = _pyopenssl_cert_to_ssl_cert(peer_cert) | ||
| try: | ||
| host_ip = ipaddress.ip_address(hostname) | ||
| except ValueError: | ||
| host_ip = None | ||
|
|
||
| names = [] | ||
| for key, value in cert.get('subjectAltName', ()): | ||
| if key == 'DNS': | ||
| if host_ip is None and _dnsname_match(value, hostname): | ||
| return | ||
| names.append(value) | ||
| elif key == 'IP Address': | ||
| if host_ip is not None and _ipaddress_match(value, host_ip): | ||
| return | ||
| names.append(value) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against peer_cert being None.
_pyopenssl_cert_to_ssl_cert calls peer_cert.to_cryptography() unconditionally. pyOpenSSL's get_peer_certificate() can return None when no certificate is presented, which would raise an AttributeError here instead of a clear ssl.CertificateError. Both EventletConnection._validate_hostname and the Twisted handshake info_callback invoke _validate_pyopenssl_hostname without a None-check on the peer cert.
🛡️ Proposed fix
def _validate_pyopenssl_hostname(peer_cert, hostname):
+ if peer_cert is None:
+ raise ssl.CertificateError("no certificate was presented by the peer")
cert = _pyopenssl_cert_to_ssl_cert(peer_cert)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _pyopenssl_cert_to_ssl_cert(peer_cert): | |
| from cryptography import x509 | |
| from cryptography.x509.oid import NameOID | |
| cert = peer_cert.to_cryptography() | |
| ssl_cert = {} | |
| subject = tuple((('commonName', attr.value),) | |
| for attr in cert.subject.get_attributes_for_oid(NameOID.COMMON_NAME)) | |
| if subject: | |
| ssl_cert['subject'] = subject | |
| subject_alt_names = [] | |
| try: | |
| san = cert.extensions.get_extension_for_class(x509.SubjectAlternativeName).value | |
| except x509.ExtensionNotFound: | |
| pass | |
| else: | |
| subject_alt_names.extend(('DNS', name) for name in san.get_values_for_type(x509.DNSName)) | |
| subject_alt_names.extend(('IP Address', str(address)) | |
| for address in san.get_values_for_type(x509.IPAddress)) | |
| if subject_alt_names: | |
| ssl_cert['subjectAltName'] = tuple(subject_alt_names) | |
| return ssl_cert | |
| def _validate_pyopenssl_hostname(peer_cert, hostname): | |
| cert = _pyopenssl_cert_to_ssl_cert(peer_cert) | |
| try: | |
| host_ip = ipaddress.ip_address(hostname) | |
| except ValueError: | |
| host_ip = None | |
| names = [] | |
| for key, value in cert.get('subjectAltName', ()): | |
| if key == 'DNS': | |
| if host_ip is None and _dnsname_match(value, hostname): | |
| return | |
| names.append(value) | |
| elif key == 'IP Address': | |
| if host_ip is not None and _ipaddress_match(value, host_ip): | |
| return | |
| names.append(value) | |
| def _pyopenssl_cert_to_ssl_cert(peer_cert): | |
| from cryptography import x509 | |
| from cryptography.x509.oid import NameOID | |
| cert = peer_cert.to_cryptography() | |
| ssl_cert = {} | |
| subject = tuple((('commonName', attr.value),) | |
| for attr in cert.subject.get_attributes_for_oid(NameOID.COMMON_NAME)) | |
| if subject: | |
| ssl_cert['subject'] = subject | |
| subject_alt_names = [] | |
| try: | |
| san = cert.extensions.get_extension_for_class(x509.SubjectAlternativeName).value | |
| except x509.ExtensionNotFound: | |
| pass | |
| else: | |
| subject_alt_names.extend(('DNS', name) for name in san.get_values_for_type(x509.DNSName)) | |
| subject_alt_names.extend(('IP Address', str(address)) | |
| for address in san.get_values_for_type(x509.IPAddress)) | |
| if subject_alt_names: | |
| ssl_cert['subjectAltName'] = tuple(subject_alt_names) | |
| return ssl_cert | |
| def _validate_pyopenssl_hostname(peer_cert, hostname): | |
| if peer_cert is None: | |
| raise ssl.CertificateError("no certificate was presented by the peer") | |
| cert = _pyopenssl_cert_to_ssl_cert(peer_cert) | |
| try: | |
| host_ip = ipaddress.ip_address(hostname) | |
| except ValueError: | |
| host_ip = None | |
| names = [] | |
| for key, value in cert.get('subjectAltName', ()): | |
| if key == 'DNS': | |
| if host_ip is None and _dnsname_match(value, hostname): | |
| return | |
| names.append(value) | |
| elif key == 'IP Address': | |
| if host_ip is not None and _ipaddress_match(value, host_ip): | |
| return | |
| names.append(value) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cassandra/connection.py` around lines 60 - 104, Update
_validate_pyopenssl_hostname and _pyopenssl_cert_to_ssl_cert to handle a None
peer_cert from get_peer_certificate(), raising the established
ssl.CertificateError instead of calling to_cryptography() and producing
AttributeError. Ensure both EventletConnection._validate_hostname and the
Twisted handshake info_callback retain this clear certificate-validation
behavior without requiring separate None checks.
dkropachev
force-pushed
the
fix-empty-ssl-options-reactors
branch
2 times, most recently
from
July 22, 2026 00:51
9bcedb1 to
d77f7f0
Compare
dkropachev
marked this pull request as draft
July 22, 2026 03:27
dkropachev
force-pushed
the
fix-empty-ssl-options-reactors
branch
3 times, most recently
from
July 22, 2026 04:19
d05df6c to
3c6f515
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #938.
Why
#938 should stay focused on preserving explicit empty
ssl_options={}as "SSL enabled". This PR contains the broader pyOpenSSL behavior changes so they can be reviewed separately.What Changed
server_hostnameas the expected certificate hostname when SNI or cloud/private-link endpoints provide it.SSL.Connectioninstead of the shared context.ssl_optionsincassandra.connection.ssl_versionandcert_reqsvalues to pyOpenSSL equivalents while preserving pyOpenSSL-native values.SSLContext, including cert chains and combined cert/key PEM files.ssl_context.check_hostnameattribute.Validation
uv run python -m py_compile cassandra/connection.py cassandra/io/eventletreactor.py cassandra/io/twistedreactor.py cassandra/datastax/cloud/__init__.py cassandra/cluster.pyuv run pytest -q tests/unit/test_connection.py tests/unit/test_cloud.py tests/unit/io/test_twistedreactor.py tests/unit/io/test_eventletreactor.py tests/unit/test_client_routes.pyResult:
116 passed, 2 skipped.