connection: preserve explicit empty ssl_options - #938
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change distinguishes omitted SSL options from explicitly supplied options, including Sequence Diagram(s)sequenceDiagram
participant Cluster
participant Connection
participant Reactor
participant pyOpenSSL
Cluster->>Connection: provide ssl_options or ssl_context
Connection->>Reactor: expose normalized SSL state
Reactor->>pyOpenSSL: build context and perform handshake
pyOpenSSL-->>Reactor: provide peer certificate
Reactor->>Connection: validate certificate hostname
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
704dde9 to
805b678
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cassandra/io/eventletreactor.py (1)
121-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead code:
uses_legacy_ssl_optionsis now alwaysFalse.Since
__init__hardcodesself.uses_legacy_ssl_options = Falseand nothing else ever sets itTrue, theif self.uses_legacy_ssl_options: super()...branches in_initiate_connectionand_validate_hostnameare now unreachable.TwistedConnectiondoesn't carry this flag at all and branches on_ssl_enableddirectly — consider removing the vestigial flag/branches here for consistency and to avoid implying conditional behavior that no longer exists.♻️ Suggested cleanup
def __init__(self, *args, **kwargs): Connection.__init__(self, *args, **kwargs) - self.uses_legacy_ssl_options = False self._write_queue = Queue() ... def _initiate_connection(self, sockaddr): - if self.uses_legacy_ssl_options: - super(EventletConnection, self)._initiate_connection(sockaddr) - else: - self._socket.connect(sockaddr) - if self._ssl_enabled: - self._socket.do_handshake() + self._socket.connect(sockaddr) + if self._ssl_enabled: + self._socket.do_handshake() def _validate_hostname(self): - if self.uses_legacy_ssl_options: - super(EventletConnection, self)._validate_hostname() - else: - expected_name = (self.ssl_options or {}).get('server_hostname') or self.endpoint.address - _validate_pyopenssl_hostname(self._socket.get_peer_certificate(), expected_name) + expected_name = (self.ssl_options or {}).get('server_hostname') or self.endpoint.address + _validate_pyopenssl_hostname(self._socket.get_peer_certificate(), expected_name)🤖 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/eventletreactor.py` around lines 121 - 158, Remove the hardcoded uses_legacy_ssl_options assignment from EventletConnection.__init__ and delete the unreachable legacy branches in _initiate_connection and _validate_hostname. Keep the current non-legacy socket connection, handshake, and hostname validation behavior as the unconditional implementation.
🤖 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.
Nitpick comments:
In `@cassandra/io/eventletreactor.py`:
- Around line 121-158: Remove the hardcoded uses_legacy_ssl_options assignment
from EventletConnection.__init__ and delete the unreachable legacy branches in
_initiate_connection and _validate_hostname. Keep the current non-legacy socket
connection, handshake, and hostname validation behavior as the unconditional
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f288eee-0f6d-48f6-b321-96aa8ca19b32
📒 Files selected for processing (13)
cassandra/cluster.pycassandra/connection.pycassandra/datastax/insights/reporter.pycassandra/io/asyncioreactor.pycassandra/io/eventletreactor.pycassandra/io/twistedreactor.pycassandra/pool.pytests/unit/advanced/test_insights.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_twistedreactor.pytests/unit/test_client_routes.pytests/unit/test_connection.pytests/unit/test_shard_aware.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cassandra/datastax/cloud/__init__.py (1)
188-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
raise ... fromfor cleaner exception chaining.Within an
exceptclause, usingraise ... from eis the idiomatic Python 3 approach. It automatically preserves the original traceback and sets the__cause__attribute, making it cleaner than manually chaining with.with_traceback().
As per static analysis hints, within anexceptclause, exceptions should be raised withraise ... from errto distinguish them from errors in exception handling.♻️ Proposed refactor
try: from OpenSSL import SSL except ImportError as e: raise ImportError( - "PyOpenSSL must be installed to connect to Astra with the Eventlet or Twisted event loops")\ - .with_traceback(e.__traceback__) + "PyOpenSSL must be installed to connect to Astra with the Eventlet or Twisted event loops" + ) from e🤖 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 188 - 193, Update the OpenSSL import error handling in the cloud initialization code to raise the custom ImportError using Python’s explicit exception chaining syntax with the caught exception as its cause. Remove the manual .with_traceback() chaining while preserving the existing error message and behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 188-193: Update the OpenSSL import error handling in the cloud
initialization code to raise the custom ImportError using Python’s explicit
exception chaining syntax with the caught exception as its cause. Remove the
manual .with_traceback() chaining while preserving the existing error message
and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b73f8af-9ad3-478c-ae6a-58f2804a5f23
📒 Files selected for processing (7)
cassandra/datastax/cloud/__init__.pycassandra/io/eventletreactor.pycassandra/io/twistedreactor.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_twistedreactor.pytests/unit/test_cloud.pytests/unit/test_cluster.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/io/test_twistedreactor.py
- cassandra/io/eventletreactor.py
- cassandra/io/twistedreactor.py
- tests/unit/io/test_eventletreactor.py
8e17dd5 to
f119103
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cassandra/io/twistedreactor.py (1)
43-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate pyOpenSSL helpers across reactors and cloud module.
_default_ssl_method(43-48) is an exact duplicate ofcassandra/datastax/cloud/__init__.py::_default_pyopenssl_ssl_method, and per the codebase graph,cassandra/io/eventletreactor.pydefines an identical_default_ssl_method/_build_pyopenssl_context_from_optionspair as well. Three independent copies of TLS negotiation/context-building logic increase the risk of divergence (e.g. one path missing a future security fix).Extracting these into a single shared module (e.g. near
_validate_pyopenssl_hostnameincassandra/connection.py) would remove the duplication.🤖 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 43 - 80, Extract the shared pyOpenSSL TLS negotiation and context-building logic from `_default_ssl_method` and `_build_pyopenssl_context_from_options` into a common helper module, alongside the existing shared SSL utilities such as `_validate_pyopenssl_hostname`. Update `cassandra/io/twistedreactor.py`, `cassandra/io/eventletreactor.py`, and `cassandra/datastax/cloud/__init__.py` to import and reuse the shared helpers, preserving their current certificate, verification, and fallback behavior.cassandra/datastax/cloud/__init__.py (1)
179-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate TLS-method-selection helper across three modules.
This exact loop-and-fallback logic is duplicated verbatim in
cassandra/io/twistedreactor.py(_default_ssl_method) and, per the codebase graph,cassandra/io/eventletreactor.py(_default_ssl_method). Three independent copies of security-relevant TLS negotiation logic risk silently diverging if one is patched without the others.Consider hoisting this into a single shared helper (e.g. alongside
_validate_pyopenssl_hostnameincassandra/connection.py) and having all three call sites import it.🤖 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 179 - 184, Consolidate the duplicated TLS method-selection loop from _default_pyopenssl_ssl_method, twistedreactor._default_ssl_method, and eventletreactor._default_ssl_method into one shared helper alongside _validate_pyopenssl_hostname in connection.py. Update all three modules to import and call the shared helper, removing their local implementations while preserving the existing method order and ImportError fallback.
🤖 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 `@tests/unit/io/test_eventletreactor.py`:
- Around line 122-131: Update test_validate_hostname_rejects_mismatch and
test_validate_hostname_prefers_san_over_common_name to assert
ssl.CertificateError specifically when _validate_hostname rejects the
certificate, replacing the broad Exception assertion while preserving the
existing test setup and invocation.
---
Nitpick comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 179-184: Consolidate the duplicated TLS method-selection loop from
_default_pyopenssl_ssl_method, twistedreactor._default_ssl_method, and
eventletreactor._default_ssl_method into one shared helper alongside
_validate_pyopenssl_hostname in connection.py. Update all three modules to
import and call the shared helper, removing their local implementations while
preserving the existing method order and ImportError fallback.
In `@cassandra/io/twistedreactor.py`:
- Around line 43-80: Extract the shared pyOpenSSL TLS negotiation and
context-building logic from `_default_ssl_method` and
`_build_pyopenssl_context_from_options` into a common helper module, alongside
the existing shared SSL utilities such as `_validate_pyopenssl_hostname`. Update
`cassandra/io/twistedreactor.py`, `cassandra/io/eventletreactor.py`, and
`cassandra/datastax/cloud/__init__.py` to import and reuse the shared helpers,
preserving their current certificate, verification, and fallback 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: e9c06e84-df9d-4864-bb9c-2a2a66a988d5
📒 Files selected for processing (16)
cassandra/cluster.pycassandra/connection.pycassandra/datastax/cloud/__init__.pycassandra/datastax/insights/reporter.pycassandra/io/asyncioreactor.pycassandra/io/eventletreactor.pycassandra/io/twistedreactor.pycassandra/pool.pytests/unit/advanced/test_insights.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_twistedreactor.pytests/unit/test_client_routes.pytests/unit/test_cloud.pytests/unit/test_cluster.pytests/unit/test_connection.pytests/unit/test_shard_aware.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/unit/test_cloud.py
- tests/unit/test_cluster.py
- tests/unit/test_client_routes.py
- cassandra/cluster.py
- tests/unit/io/test_twistedreactor.py
- cassandra/pool.py
- tests/unit/advanced/test_insights.py
- cassandra/io/eventletreactor.py
- cassandra/connection.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Normalizes explicit empty ssl_options handling so {} enables TLS while None remains disabled.
Changes:
- Preserves explicit SSL configuration across connections, reactors, routing, and Insights.
- Builds default SSL contexts for standard, Eventlet, and Twisted paths.
- Adds focused SSL behavior and compatibility tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/connection.py |
Preserves explicit SSL state and builds contexts. |
cassandra/cluster.py |
Corrects cloud validation and warnings. |
cassandra/pool.py |
Updates shard-aware TLS port selection. |
cassandra/io/asyncioreactor.py |
Uses normalized SSL state. |
cassandra/io/eventletreactor.py |
Builds pyOpenSSL contexts for Eventlet. |
cassandra/io/twistedreactor.py |
Builds pyOpenSSL contexts for Twisted. |
cassandra/datastax/cloud/__init__.py |
Selects modern pyOpenSSL methods. |
cassandra/datastax/insights/reporter.py |
Corrects SSL startup reporting. |
tests/unit/test_connection.py |
Tests connection SSL semantics. |
tests/unit/test_cluster.py |
Tests cloud conflicts and warnings. |
tests/unit/test_cloud.py |
Tests pyOpenSSL method fallback. |
tests/unit/test_client_routes.py |
Tests empty options with TLS routes. |
tests/unit/test_shard_aware.py |
Tests shard-aware SSL ports. |
tests/unit/io/test_eventletreactor.py |
Tests Eventlet SSL contexts. |
tests/unit/io/test_twistedreactor.py |
Tests Twisted SSL contexts. |
tests/unit/advanced/test_insights.py |
Tests Insights SSL reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f8c3ca4 to
9bcedb1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:180
Connection.__init__always normalizes omitted options toself.ssl_options = {}. On a real plaintext control connection,connection_ssl_optionsis therefore non-None, so this predicate reportsCluster()as SSL-enabled (and reports certificate validation as false) even though SSL was omitted. The test mock hides this by leaving the attribute absent. Select connection options only when_ssl_options_explicitis true; otherwise fall back to the cluster options.
ssl_options_explicit = bool(_safe_getattr(connection, '_ssl_options_explicit', False))
ssl_enabled = (ssl_context is not None or
ssl_options is not None or
endpoint_ssl_options is not None or
ssl_options_explicit)
3c6f515 to
ae74fbe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cassandra/io/eventletreactor.py:122
- When
check_hostname=Trueis used without an explicitserver_hostname, this path validates againstendpoint.addressbut never sends that name as SNI. The stdlib path supplies the endpoint name in this case (cassandra/connection.py:1238-1243), so an SNI-hosted server can return its default certificate and make Eventlet fail while the default reactor succeeds. Derive the SNI name from the endpoint when hostname checking is enabled.
if self.ssl_options and 'server_hostname' in self.ssl_options:
# This is necessary for SNI
self._socket.set_tlsext_host_name(self.ssl_options['server_hostname'].encode('ascii'))
f3387e6 to
c963ddb
Compare
c963ddb to
535986a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cassandra/io/twistedreactor.py:207
- This replaces Twisted's
tlsProtocolapplication data with a driver-private wrapper. The previous behavior follows theIOpenSSLClientConnectionCreatorconvention and allows callbacks on a caller-suppliedSSL.Contextto retrieve the protocol (for example, to callfailVerification); those callbacks will now receive_TLSAppDataand can fail during the handshake. PreservetlsProtocolas the connection's app data and keep the hostname metadata separately or on the protocol.
connection.set_app_data(_TLSAppData(
tlsProtocol, self.endpoint, server_hostname, self.check_hostname))
535986a to
8af317e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/unit/io/test_twistedreactor.py (1)
149-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ConnectionWithoutInfoCallbackis defined twice verbatim.Hoist it to module scope (or a shared helper) and reuse it in both tests.
♻️ Sketch
class _ConnectionWithoutInfoCallback(object): def __init__(self, context, socket): self.context = context self.socket = socket self.app_data = None self.peer_cert = Mock() def set_app_data(self, app_data): self.app_data = app_data def get_app_data(self): return self.app_data def get_peer_certificate(self): return self.peer_cert def set_tlsext_host_name(self, server_hostname): self.server_hostname = server_hostname🤖 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 `@tests/unit/io/test_twistedreactor.py` around lines 149 - 210, Remove the duplicated local ConnectionWithoutInfoCallback definitions from both tests and define one shared helper at module scope, such as _ConnectionWithoutInfoCallback, preserving all existing methods and attributes. Update both test_ssl_creator_uses_context_callback_when_connection_callback_is_unavailable and test_context_callback_uses_connection_app_data to reuse the shared class.tests/unit/io/test_eventletreactor.py (2)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnit tests depend on an integration-suite certificate file. Both reactor test modules define
CA_CERTSastests/integration/long/ssl/rootCa.crtand pass it to a realload_verify_locations(), so the unit suite fails if that integration fixture is missing, moved, or regenerated.
tests/unit/io/test_eventletreactor.py#L36-L37: replace the hard-coded integration path with a fixture generated in the unit test (or drop theca_certscase in favor of thecert_reqsassertion at Line 135).tests/unit/io/test_twistedreactor.py#L76-L79: apply the same change sotest_ca_certs_default_to_required_validationno longer reads from the integration tree.🤖 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 `@tests/unit/io/test_eventletreactor.py` around lines 36 - 37, Remove the unit tests’ dependency on the integration certificate fixture: in tests/unit/io/test_eventletreactor.py lines 36-37, replace CA_CERTS with a certificate generated within the test or drop the ca_certs case in favor of the existing cert_reqs assertion; apply the same change to tests/unit/io/test_twistedreactor.py lines 76-79 so test_ca_certs_default_to_required_validation no longer reads from the integration tree.
40-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated fake X509 scaffolding.
_FakeX509Name/_FakeX509Extension/_FakeX509Certificatehere largely duplicateFakeX509Name/FakeX509Extension/FakeX509Certificateintests/unit/test_connection.py(Lines 39-72), and_make_certificateadds nothing over the constructor. Consider moving one implementation into a shared test helper (e.g.tests/unit/io/utils.py) and importing it in both places.🤖 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 `@tests/unit/io/test_eventletreactor.py` around lines 40 - 81, Remove the duplicated _FakeX509Name, _FakeX509Extension, _FakeX509Certificate, and _make_certificate scaffolding from test_eventletreactor.py, move the shared implementation into a test utility module, and update both test_eventletreactor.py and test_connection.py to import and reuse the shared fake X509 classes directly.tests/unit/test_shard_aware.py (1)
169-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFold the endpoint/
ShardingInfosetup intomake_host.Both tests re-build a
ShardingInfothat already matches whatmake_hostproduced and then overwritehost.endpoint. Anendpoint=parameter (and returning the sharding info) would remove the duplicated construction and keep host/session state in sync by construction.🤖 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 `@tests/unit/test_shard_aware.py` around lines 169 - 201, The test setup duplicates endpoint and ShardingInfo construction outside make_host. Extend make_host to accept an endpoint parameter and return the generated sharding information, then update both endpoint SSL tests to use these values directly instead of overwriting host.endpoint or manually constructing ShardingInfo; preserve each test’s SSL and plaintext port configuration.tests/unit/test_connection.py (1)
98-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the pyOpenSSL method constants used by the builder.
_default_pyopenssl_ssl_method()and several protocol branches also readTLS_METHODandTLSv1_2_METHOD; adding them toFakePyOpenSSLModuleremoves the implicit fallback dependency and keeps the fake aligned with the pyOpenSSL surface the tests cover.🤖 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 `@tests/unit/test_connection.py` around lines 98 - 103, Update FakePyOpenSSLModule to define the TLS_METHOD and TLSv1_2_METHOD constants used by _default_pyopenssl_ssl_method() and the protocol branches, preserving the existing constant values and fake module interface.cassandra/connection.py (2)
1108-1114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCopy the endpoint options for consistency.
The explicit-options branch copies, but the endpoint-only branch aliases the endpoint's dict into
self.ssl_options, so any future in-place mutation (the block below still documentspop()semantics) would leak into the sharedEndPoint.♻️ Proposed change
elif endpoint_ssl_options is not None: self._ssl_options_explicit = True - self.ssl_options = endpoint_ssl_options + self.ssl_options = dict(endpoint_ssl_options)🤖 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 1108 - 1114, Update the endpoint-only branch in the SSL options initialization to copy endpoint_ssl_options before assigning it to self.ssl_options, matching the existing explicit-options branch. Preserve the current _ssl_options_explicit behavior and downstream mutation semantics without retaining an alias to the endpoint’s dictionary.
270-325: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse cryptography-typed SAN parsing and guard missing peer certificates.
X509.get_extension_count()/X509.get_extension()rely on pyOpenSSL’s obsolete extension API, so parse SANs viacert.to_cryptography().extensions.get_extension_for_extension_class(x509.SubjectAlternativeName)instead of human-readablestr(extension)text. Also handle_validate_pyopenssl_hostname(None, ...)andtwistedreactor’sssl.CertificateErrorhandler so a missing peer certificate maps to verification failure rather thanAttributeError.🤖 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 270 - 325, Replace the obsolete extension iteration in _pyopenssl_cert_subject_alt_names with cryptography-based SubjectAlternativeName parsing via cert.to_cryptography(), extracting DNS names and IP addresses from typed SAN values and handling an absent SAN extension. Update _validate_pyopenssl_hostname to handle cert=None as a hostname verification failure, and adjust the twistedreactor ssl.CertificateError handling so missing peer certificates produce verification failure rather than AttributeError.cassandra/io/eventletreactor.py (1)
120-126: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
encode('ascii')breaks for non-ASCII (IDN) hostnames.A configured
server_hostnamelikenodé.example.comraisesUnicodeEncodeErrorhere instead of a TLS error.hostname.encode('idna')(oridna.encode) is the conventional choice for SNI.♻️ Proposed change
- self._socket.set_tlsext_host_name(server_hostname.encode('ascii')) + self._socket.set_tlsext_host_name(server_hostname.encode('idna'))🤖 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/eventletreactor.py` around lines 120 - 126, Update the SNI hostname encoding in the socket setup around _socket and server_hostname to use IDNA encoding instead of ASCII. Preserve the existing hostname selection and set_tlsext_host_name flow while allowing configured non-ASCII hostnames to be converted to their ASCII-compatible IDN form.cassandra/datastax/insights/reporter.py (1)
36-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
object.__getattribute__is used instead ofgetattr.Using
object.__getattribute__intentionally bypasses__getattr__/__getattribute__overrides (relevant forMockconnections and proxies) but reads like an accident; a one-line comment — or plaingetattr(obj, name, default)if the bypass isn't required — would prevent someone "simplifying" it later.🤖 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/insights/reporter.py` around lines 36 - 57, Document the intentional use of object.__getattribute__ in _safe_getattr: add a concise comment explaining that it bypasses custom __getattr__/__getattribute__ behavior on Mock connections and proxies. Keep the existing AttributeError fallback and behavior unchanged.
🤖 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 1210-1218: Update the SSL context setup around SSLContext in the
connection initialization so check_hostname=True with cert_reqs=ssl.CERT_NONE is
promoted to ssl.CERT_REQUIRED before assigning verify_mode, matching the
pyOpenSSL builder’s behavior. Preserve explicit certificate requirements for all
other combinations and ensure verify_mode is set before enabling hostname
checking.
---
Nitpick comments:
In `@cassandra/connection.py`:
- Around line 1108-1114: Update the endpoint-only branch in the SSL options
initialization to copy endpoint_ssl_options before assigning it to
self.ssl_options, matching the existing explicit-options branch. Preserve the
current _ssl_options_explicit behavior and downstream mutation semantics without
retaining an alias to the endpoint’s dictionary.
- Around line 270-325: Replace the obsolete extension iteration in
_pyopenssl_cert_subject_alt_names with cryptography-based SubjectAlternativeName
parsing via cert.to_cryptography(), extracting DNS names and IP addresses from
typed SAN values and handling an absent SAN extension. Update
_validate_pyopenssl_hostname to handle cert=None as a hostname verification
failure, and adjust the twistedreactor ssl.CertificateError handling so missing
peer certificates produce verification failure rather than AttributeError.
In `@cassandra/datastax/insights/reporter.py`:
- Around line 36-57: Document the intentional use of object.__getattribute__ in
_safe_getattr: add a concise comment explaining that it bypasses custom
__getattr__/__getattribute__ behavior on Mock connections and proxies. Keep the
existing AttributeError fallback and behavior unchanged.
In `@cassandra/io/eventletreactor.py`:
- Around line 120-126: Update the SNI hostname encoding in the socket setup
around _socket and server_hostname to use IDNA encoding instead of ASCII.
Preserve the existing hostname selection and set_tlsext_host_name flow while
allowing configured non-ASCII hostnames to be converted to their
ASCII-compatible IDN form.
In `@tests/unit/io/test_eventletreactor.py`:
- Around line 36-37: Remove the unit tests’ dependency on the integration
certificate fixture: in tests/unit/io/test_eventletreactor.py lines 36-37,
replace CA_CERTS with a certificate generated within the test or drop the
ca_certs case in favor of the existing cert_reqs assertion; apply the same
change to tests/unit/io/test_twistedreactor.py lines 76-79 so
test_ca_certs_default_to_required_validation no longer reads from the
integration tree.
- Around line 40-81: Remove the duplicated _FakeX509Name, _FakeX509Extension,
_FakeX509Certificate, and _make_certificate scaffolding from
test_eventletreactor.py, move the shared implementation into a test utility
module, and update both test_eventletreactor.py and test_connection.py to import
and reuse the shared fake X509 classes directly.
In `@tests/unit/io/test_twistedreactor.py`:
- Around line 149-210: Remove the duplicated local ConnectionWithoutInfoCallback
definitions from both tests and define one shared helper at module scope, such
as _ConnectionWithoutInfoCallback, preserving all existing methods and
attributes. Update both
test_ssl_creator_uses_context_callback_when_connection_callback_is_unavailable
and test_context_callback_uses_connection_app_data to reuse the shared class.
In `@tests/unit/test_connection.py`:
- Around line 98-103: Update FakePyOpenSSLModule to define the TLS_METHOD and
TLSv1_2_METHOD constants used by _default_pyopenssl_ssl_method() and the
protocol branches, preserving the existing constant values and fake module
interface.
In `@tests/unit/test_shard_aware.py`:
- Around line 169-201: The test setup duplicates endpoint and ShardingInfo
construction outside make_host. Extend make_host to accept an endpoint parameter
and return the generated sharding information, then update both endpoint SSL
tests to use these values directly instead of overwriting host.endpoint or
manually constructing ShardingInfo; preserve each test’s SSL and plaintext port
configuration.
🪄 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: 87f5d84e-d165-4a6d-a5a3-b50d07536617
📒 Files selected for processing (17)
cassandra/cluster.pycassandra/connection.pycassandra/datastax/cloud/__init__.pycassandra/datastax/insights/reporter.pycassandra/io/asyncioreactor.pycassandra/io/eventletreactor.pycassandra/io/twistedreactor.pycassandra/pool.pytests/unit/advanced/test_insights.pytests/unit/io/test_asyncioreactor.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_twistedreactor.pytests/unit/test_client_routes.pytests/unit/test_cloud.pytests/unit/test_cluster.pytests/unit/test_connection.pytests/unit/test_shard_aware.py
🚧 Files skipped from review as they are similar to previous changes (7)
- cassandra/pool.py
- tests/unit/test_cloud.py
- cassandra/cluster.py
- tests/unit/test_client_routes.py
- tests/unit/test_cluster.py
- cassandra/datastax/cloud/init.py
- cassandra/io/twistedreactor.py
| cert_reqs = opts.get('cert_reqs', None) | ||
| if cert_reqs is None: | ||
| cert_reqs = (ssl.CERT_REQUIRED | ||
| if self.ssl_options | ||
| else ssl.CERT_NONE) | ||
| rv = ssl.SSLContext(protocol=int(ssl_version)) | ||
| rv.check_hostname = False | ||
| rv.verify_mode = cert_reqs | ||
| rv.check_hostname = bool(opts.get('check_hostname', False)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
cert_reqs=CERT_NONE with check_hostname=True now raises ValueError here.
Line 1218 assigns check_hostname after verify_mode, and stdlib rejects check_hostname=True while verify_mode == CERT_NONE, so Connection(..., ssl_options={'cert_reqs': ssl.CERT_NONE, 'check_hostname': True}) fails at construction. The pyOpenSSL builder above (Lines 208-209) handles the same combination by promoting to VERIFY_PEER; mirror that here.
🐛 Proposed fix
cert_reqs = opts.get('cert_reqs', None)
if cert_reqs is None:
cert_reqs = (ssl.CERT_REQUIRED
if self.ssl_options
else ssl.CERT_NONE)
+ elif opts.get('check_hostname', False) and cert_reqs == ssl.CERT_NONE:
+ cert_reqs = ssl.CERT_REQUIRED
rv = ssl.SSLContext(protocol=int(ssl_version))
rv.check_hostname = False
rv.verify_mode = cert_reqs📝 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.
| cert_reqs = opts.get('cert_reqs', None) | |
| if cert_reqs is None: | |
| cert_reqs = (ssl.CERT_REQUIRED | |
| if self.ssl_options | |
| else ssl.CERT_NONE) | |
| rv = ssl.SSLContext(protocol=int(ssl_version)) | |
| rv.check_hostname = False | |
| rv.verify_mode = cert_reqs | |
| rv.check_hostname = bool(opts.get('check_hostname', False)) | |
| cert_reqs = opts.get('cert_reqs', None) | |
| if cert_reqs is None: | |
| cert_reqs = (ssl.CERT_REQUIRED | |
| if self.ssl_options | |
| else ssl.CERT_NONE) | |
| elif opts.get('check_hostname', False) and cert_reqs == ssl.CERT_NONE: | |
| cert_reqs = ssl.CERT_REQUIRED | |
| rv = ssl.SSLContext(protocol=int(ssl_version)) | |
| rv.check_hostname = False | |
| rv.verify_mode = cert_reqs | |
| rv.check_hostname = bool(opts.get('check_hostname', False)) |
🤖 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 1210 - 1218, Update the SSL context
setup around SSLContext in the connection initialization so check_hostname=True
with cert_reqs=ssl.CERT_NONE is promoted to ssl.CERT_REQUIRED before assigning
verify_mode, matching the pyOpenSSL builder’s behavior. Preserve explicit
certificate requirements for all other combinations and ensure verify_mode is
set before enabling hostname checking.
8af317e to
9d63584
Compare
Summary
Fixes #937: explicit
ssl_options={}was treated like omittedssl_options=Nonebecause several paths checkedssl_optionstruthiness.This PR preserves whether
ssl_optionswas supplied and uses that state for SSL-enabled checks:ssl_options={}enables TLS with default options.ssl_options=Noneremains plaintext unless endpoint SSL options are supplied.In Scope
ssl_options.ssl_versionandcert_reqsmapping, cipher handling, and hostname/SAN/SNI validation.Public
ClusterAPI and protocol format are unchanged.Follow-up PR
Follow-up #941 remains for additional pyOpenSSL parity and cleanup after this branch, including cert/key loading parity, client-routes compatibility with pyOpenSSL-style contexts, and any remaining review-driven simplification after rebasing on this PR.
Compatibility and Protocol Risk
No protocol changes. Behavior changes only for callers that explicitly pass
ssl_options={}or use endpoint SSL options; those are now treated as SSL-enabled instead of plaintext.Existing non-empty legacy
ssl_optionskeep their prior default peer-verification behavior unlesscert_reqsis supplied explicitly.Tests
uv run pytest -rf tests/unit/test_connection.py tests/unit/test_cluster.py tests/unit/test_client_routes.py tests/unit/test_cloud.py tests/unit/test_shard_aware.py tests/unit/advanced/test_insights.py tests/unit/io/test_asyncioreactor.py tests/unit/io/test_eventletreactor.py tests/unit/io/test_twistedreactor.pyIntegration scenario to consider: connect to a TLS cluster with
Cluster(..., ssl_options={})under the default, Twisted, and Eventlet reactors.