connection: preserve explicit empty ssl_options#938
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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.
| endpoint = copy.copy(self.host.endpoint) | ||
| endpoint._port = self.host.sharding_info.shard_aware_port_ssl | ||
| elif not ssl_enabled and self.host.sharding_info.shard_aware_port: | ||
| if ssl_enabled: |
| ssl_version = (ssl_options['ssl_version'] | ||
| if 'ssl_version' in ssl_options else _default_ssl_method()) | ||
| context = SSL.Context(ssl_version) |
| def _build_pyopenssl_context_from_options(ssl_options): | ||
| ssl_version = (ssl_options['ssl_version'] | ||
| if 'ssl_version' in ssl_options else _default_ssl_method()) | ||
| context = SSL.Context(ssl_version) |
| context.use_privatekey_file(ssl_options['keyfile']) | ||
| if 'ca_certs' in ssl_options: | ||
| context.load_verify_locations(ssl_options['ca_certs']) | ||
| cert_reqs = ssl_options.get('cert_reqs', None) |
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.
f8c3ca4 to
9bcedb1
Compare
| def _validate_hostname(self): | ||
| if self.uses_legacy_ssl_options: | ||
| super(EventletConnection, self)._match_hostname() | ||
| super(EventletConnection, self)._validate_hostname() |
| 'enabled': (self._session.cluster.ssl_context is not None or | ||
| self._session.cluster.ssl_options is not None), |
| with patch.object(twistedreactor.SSL, 'Context') as context_mock: | ||
| context = twistedreactor._build_pyopenssl_context_from_options({}) | ||
|
|
||
| context_mock.assert_called_once_with(twistedreactor.SSL.TLS_CLIENT_METHOD) |
| with patch.object(eventletreactor.SSL, 'Context') as context_mock: | ||
| context = eventletreactor._build_pyopenssl_context_from_options({}) | ||
|
|
||
| context_mock.assert_called_once_with(eventletreactor.SSL.TLS_CLIENT_METHOD) |
Summary
Problem: several SSL paths used
ssl_optionstruthiness, so explicitssl_options={}was treated like omittedssl_options=None. That could disable SSL setup in connection/reactor paths, skip the legacy deprecation warning, and make Insights report SSL as disabled.Fix: preserve whether
ssl_optionswas supplied and use that state for SSL-enabled checks. Explicitssl_options={}now enables SSL with default options; omittedssl_options=Noneremains disabled. Twisted/Eventlet build pyOpenSSL contexts for legacy options, while the standard path builds anSSLContext.Closes #937.
Driver Surface
Touches connection initialization, asyncio/Eventlet/Twisted SSL setup, client-routes SSL mode, and Insights startup reporting. Public
ClusterAPI and protocol format are unchanged.Compatibility and Protocol Risk
No protocol changes. The behavior change is limited to callers that explicitly pass
ssl_options={}or endpoint SSL options: those are now treated as SSL-enabled instead of plaintext.Tests
uv run pytest -rf tests/unit/test_connection.py tests/unit/test_client_routes.py tests/unit/advanced/test_insights.py tests/unit/io/test_twistedreactor.py tests/unit/io/test_eventletreactor.py tests/unit/io/test_asyncioreactor.pyuv run pytest -rf tests/unit; it fails intests/unit/test_types.py::DateRangeDeserializationTests::test_deserialize_date_range_year, reproduced in isolation and unrelated to this change.Integration scenario to consider: connect to a TLS cluster with
Cluster(..., ssl_options={})under the default, Twisted, and Eventlet reactors.