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.
f8c3ca4 to
9bcedb1
Compare
9bcedb1 to
d77f7f0
Compare
|
Addressed review feedback in
Validation:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:180
Connection.__init__always creates an instancessl_optionsdict, using{}even when the option was omitted. Preferring any non-Noneconnection value here therefore makes every real plaintext control connection reportenabled: true(andcertValidation: false). The omitted-options test misses this because its mock leaves the attribute unset. Use_ssl_options_explicit(while retaining non-empty options for compatibility) before selecting the connection-level dict.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_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)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:180
Connectionalways normalizes omitted options toself.ssl_options = {}(connection.py:1057). Preferring every non-Noneconnection value here therefore makes a real plaintext control connection reportsslConfigured.enabled: true, despite_ssl_options_explicitbeing false; the omitted-options test misses this because its mock has nossl_optionsattribute. Only prefer the connection dictionary when it was explicitly supplied, otherwise fall back to the cluster value.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_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)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:176
- A real
Connectionalways normalizes omittedssl_options=Noneto{}(connection.py:1057). Selecting any non-Noneconnection options here therefore marks every plaintext control connection as SSL-enabled and reportscertValidation: false. Use_ssl_options_explicitto distinguish that normalized empty mapping from supplied options before falling back to the cluster configuration.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_options)
ssl_options_explicit = bool(_safe_getattr(connection, '_ssl_options_explicit', False))
941469c to
5eb98b0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:176
Connection.__init__always normalizes omitted options toconnection.ssl_options = {}while leaving_ssl_options_explicitfalse. Because this selects any non-Noneconnection value before checking explicitness, every real plaintext control connection is now treated as SSL-enabled (and reportscertValidation: false). The omitted-options test misses this because its mock has nossl_optionsattribute. Only prefer the connection options when they were explicit; otherwise fall back to the cluster value.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_options)
ssl_options_explicit = bool(_safe_getattr(connection, '_ssl_options_explicit', False))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:176
Connection.__init__always normalizes omitted options toself.ssl_options = {}(cassandra/connection.py:1071-1072). A real plaintext control connection therefore selects that empty dict here, and the followingssl_options is not Nonecheck reports SSL as enabled (and certificate validation as false). Gate connection-level options on_ssl_options_explicitbefore using them; the omitted-options test currently misses this because its mock leavesssl_optionsabsent.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_options)
ssl_options_explicit = bool(_safe_getattr(connection, '_ssl_options_explicit', False))
5eb98b0 to
d05df6c
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 2 comments.
Comments suppressed due to low confidence (1)
cassandra/datastax/insights/reporter.py:175
Connectionnormalizes omitted options toself.ssl_options = {}, so a real plaintext control connection always makesconnection_ssl_options is not None. This selection consequently reports TLS as enabled andcertValidationasFalseeven when both cluster SSL arguments were omitted; the mock-based omitted-options test misses that normalization. Only prefer the connection options when_ssl_options_explicitis true, otherwise fall back to the cluster value.
ssl_options = (connection_ssl_options
if connection_ssl_options is not None
else self._session.cluster.ssl_options)
d05df6c to
3c6f515
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)
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.