connection: fix ssl_options TLS handling - #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)
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.
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
8af317e to
9d63584
Compare
sylwiaszunejko
left a comment
sylwiaszunejko
left a comment
There was a problem hiding this comment.
I believe this PR's 1 commit should be split into more smaller ones, so it is easier to review, or at least have more descriptive commit message. E.g. new pyOpenSSL hostname validation seems to be independent change that could have its own commit explaining why it is necessary
|
Follow-up on the commit-shape review: I pushed the review fixes as a separate commit in 8d6fdc5. I agree the implementation commit is broad; I can split the original implementation commit before merge if we want to rewrite the PR history. |
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/connection.py:183
- When
check_hostname=Trueand the supplied pyOpenSSL context has noget_verify_mode, this silently leaves the context's defaultVERIFY_NONEin place. The new hostname check then authenticates the name on an untrusted certificate, so older pyOpenSSL contexts can accept an attacker-controlled certificate. Please either forceVERIFY_PEERwhen the mode cannot be queried or reject this combination rather than skipping verification.
get_verify_mode = getattr(context, 'get_verify_mode', None)
if (check_hostname and get_verify_mode is not None and
get_verify_mode() == ssl_module.VERIFY_NONE):
nikagra
left a comment
nikagra
left a comment
There was a problem hiding this comment.
Reviewed at 8d6fdc54. Solid PR that fixes considerably more than the title advertises — but two paths can still silently skip certificate verification, so I'd like those closed before merge.
What this actually fixes
I checked against upstream/master, and the pyOpenSSL reactors were completely broken for legacy ssl_options, not merely wrong about {}:
Twisted ssl_context built by master = <class 'ssl.SSLContext'>
Eventlet ssl_context built by master = <class 'ssl.SSLContext'>
uses_legacy_ssl_options would be: False
Connection.__init__built a stdlibSSLContext, so_SSLCreatorthen calledstdlib_context.set_info_callback(...)→AttributeError, and Eventlet calledSSL.Connection(stdlib_context, sock)→TypeError.- Because that context was always set, Eventlet's
uses_legacy_ssl_optionswas permanentlyFalse. - Eventlet's
_match_hostnamewas dead code — the base class has called_validate_hostnamesince PYTHON-1331. - master never assigned
self._check_hostname, so the class attribute stayedFalseand_validate_hostname()was unreachable on every reactor.
Separately, rv.options = int(cert_reqs) was assigning a verify_mode value into the options bitmask. Measured:
default options: 0x82520050
after master's `options = 2`: 0x2
NO_SSLv3 still set? False
NO_TLSv1 still set? False
NO_COMPRESSION still set? False
Good catch — that was silently clearing every hardening default. The ssl_module-parameterized helpers are a nice design (one implementation serving both OpenSSL.SSL and eventlet.green.OpenSSL.SSL, testable with a fake module), _dnsname_match is correctly stricter than the removed ssl.match_hostname, and moving info_callback to a staticmethod that reads per-connection app data properly fixes the shared-context aliasing bug.
Tests
Ran the suite from the description in a clean worktree: 135 passed / 55 skipped, plus 25 passed for Twisted and 14 passed for Eventlet. All green.
One gotcha worth knowing: the Twisted tests need service_identity installed, and without it twisted.test.proto_helpers fails to import and all 25 skip as "Twisted libraries are not available" — including the new TwistedSSLContextTest. Please make sure CI actually has it, otherwise the largest block of new logic here is covered by tests that never run.
Blocking
Two inline comments below: connection.py:171 (a plain-int cert_reqs bypasses verification on both pyOpenSSL reactors) and twistedreactor.py:190 (hostname verification fails open through the cffi callback). Both mean a caller who explicitly asked for verification can silently not get it.
Also needed before merge
1. Document the {} semantics — Cluster.ssl_options docstring, around line 877; it's outside the diff so I can't anchor inline.
ssl_options={} now enables TLS with no certificate verification at all (stdlib CERT_NONE, pyOpenSSL VERIFY_NONE), while the visually equivalent ssl_context=ssl.SSLContext(PROTOCOL_TLS_CLIENT) verifies. The pragmatism is understandable — CERT_REQUIRED with no CA store loaded would just fail — but someone writing {} to "turn on TLS" will not guess they got a MITM-able channel. Suggested addition:
.. versionchanged:: 3.29.12
An explicit empty dict (``ssl_options={}``) enables TLS with default options,
whereas ``ssl_options=None`` (the default) leaves the connection in plaintext.
Because an empty dict supplies no ``ca_certs`` and no ``cert_reqs``, it defaults
to ``ssl.CERT_NONE`` — the connection is encrypted but the server certificate is
**not** verified. Supply ``ca_certs`` (and optionally ``'check_hostname': True``)
to authenticate the server.2. CHANGELOG.rst entry. There's an active Unreleased section and the immediately preceding commit (e605de28) added one for a comparable change. Four things here are user-visible:
Bug Fixes
---------
* ``ssl_options={}`` is no longer treated as if ``ssl_options`` had been omitted.
An explicit empty dict now enables TLS with default options (see the
``Cluster.ssl_options`` docs for the certificate-verification implications);
``ssl_options=None`` remains plaintext. Endpoint-supplied SSL options also mark
the connection SSL-enabled, including for shard-aware port selection.
* Legacy ``ssl_options`` now work with the Twisted and Eventlet reactors. Both
previously received a stdlib ``ssl.SSLContext`` where a ``pyOpenSSL`` context
was required, raising ``AttributeError``/``TypeError`` on connect.
* Hostname verification is now actually performed. ``Connection._check_hostname``
was never assigned, so ``_validate_hostname()`` was unreachable, and the
Eventlet reactor's override was dead code under the pre-PYTHON-1331 name
``_match_hostname``. Verification now matches subjectAltName (DNS, IP) with
wildcard support and falls back to commonName, replacing a bare commonName
equality check.
* ``_build_ssl_context_from_options`` assigned ``cert_reqs`` to
``SSLContext.options`` instead of ``SSLContext.verify_mode``, clearing the
default option bitmask and with it ``OP_NO_SSLv3``, ``OP_NO_TLSv1`` and
``OP_NO_COMPRESSION``. These defaults are restored, so connections to servers
that only offer TLS 1.1 or older will now fail where they previously succeeded.3. Widen the PR description. "preserve explicit empty ssl_options" undersells this — the Twisted and Eventlet SSL paths were fully broken before it, and reviewers will size the risk from the title.
Cleanups — fine to defer to #941
datastax/insights/reporter.py:165— theif connection_ssl_enabled is Nonefallback is unreachable in production, since_ssl_enabledis a property that always returnsbool. It exists only becausetest_implicit_empty_connection_ssl_options_reported_as_disabledsetsconnection._ssl_enabled = Noneon aMock, so the test is exercising the defensive branch rather than the behaviour. Dropping both lets the surroundinggetattrchain collapse a long way.connection.py:1148—_ssl_enabledis exactly equivalent toself.ssl_context is not Noneonce__init__returns, because every_build_ssl_context_from_optionsoverride either returns a context or raises. Worth keeping the property for intent, but it doesn't need_ssl_options_explicitin the expression.- Dead code:
_default_ssl_method()ineventletreactor.py:42andtwistedreactor.py:57is never called outside tests;_SSLCreator.verify_callback(twistedreactor.py:187) is unused now that the shared builder installs its own lambda;twistedreactor.py:275readsself.ssl_context if self.ssl_context is not None else None; andTwistedConnection._check_pyopenssl()is unreachable on thessl_optionspath now that_build_ssl_context_from_optionsraises first. eventletreactor.py:117vs:121mixesself._check_hostnameandgetattr(self, '_check_hostname', False)in the same method.asyncioreactor.py:210still hasif self.ssl_options:. Harmless — theserver_hostname is Nonefallback covers{}— but inconsistent with the point of the PR.- Tests: the
FakeX509*doubles are duplicated verbatim betweentest_connection.pyandtest_eventletreactor.pyand could move totests/unit/io/utils.py;test_cloud.pytestscloud._default_pyopenssl_ssl_method, a symbol merely re-exported fromconnection, duplicatingtest_connection.pycoverage while leaving the cloud module's own_pyopenssl_context_from_certchange untested; and both reactor test modules reach intotests/integration/long/ssl/rootCa.crtfrom unit tests, coupling the unit suite to integration fixtures.
dkropachev
changed the title
|
Addressed the blocking items in b14fe37: raw integer cert_reqs mapping, missing peer certificate handling, Twisted fail-closed callback behavior, and Eventlet hostname error reporting. Also added Cluster.ssl_options docs, CHANGELOG entries, and updated the PR title/description. Deferring typed SAN parsing and cleanup-only items to #941. Validation: 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.py -> 208 passed, 5 skipped. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CHANGELOG.rst:12
- This newly advertised behavior conflicts with the SSL guide:
docs/security.rst:50-53says anssl_contextis required to enable SSL, and lines 70-72 say Twisted/Eventlet users must pass a pyOpenSSL context. Update that guide to document options-only TLS and the new reactor handling; otherwise the primary user documentation tells users this supported path is invalid.
* Legacy ``ssl_options`` now work with the Twisted and Eventlet reactors by
using pyOpenSSL contexts with mapped protocol, verification, cipher, SNI, and
hostname-validation settings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/connection.py:185
- When
check_hostname=Trueis used with a supplied context from an older pyOpenSSL that lacksget_verify_mode(), this helper now silently does nothing. Such contexts default toVERIFY_NONE, so Eventlet/Twisted proceed to hostname-match a certificate whose chain was never authenticated; the new compatibility test codifies that insecure path. Please fail closed for this combination or otherwise forceVERIFY_PEERwhile preserving any user verification callback.
def _ensure_pyopenssl_context_requires_verification(ssl_module, context, check_hostname):
get_verify_mode = getattr(context, 'get_verify_mode', None)
if (check_hostname and get_verify_mode is not None and
get_verify_mode() == ssl_module.VERIFY_NONE):
context.set_verify(
ssl_module.VERIFY_PEER,
callback=lambda _connection, _x509, _errnum, _errdepth, ok: ok
)
I think there is nothing wrong with rewriting PR history, if we would gain cleaner history on master. I think that is the case with this PR - one commit adding unnecessary noise, next one deleting it. It would be nice to have it cleaned before merge. |
Preserve the distinction between omitted and explicitly empty ssl_options across connection setup, reactors, shard-aware routing, cloud contexts, and Insights reporting. Strengthen pyOpenSSL protocol, verification, SNI, and hostname handling while retaining legacy option behavior.
2ee525e to
ab39732
Compare
| def _ensure_pyopenssl_context_requires_verification(ssl_module, context, check_hostname): | ||
| get_verify_mode = getattr(context, 'get_verify_mode', None) | ||
| if (check_hostname and get_verify_mode is not None and | ||
| get_verify_mode() == ssl_module.VERIFY_NONE): | ||
| context.set_verify( | ||
| ssl_module.VERIFY_PEER, | ||
| callback=lambda _connection, _x509, _errnum, _errdepth, ok: ok | ||
| ) |
Summary
Fixes #937: explicit
ssl_options={}was treated like omittedssl_options=Nonebecause several paths checkedssl_optionstruthiness.This PR preserves whether
ssl_optionswas supplied and fixes the reactor paths needed to use that state consistently:ssl_options={}enables TLS with default options.ssl_options=Noneremains plaintext unless endpoint SSL options are supplied.ssl_optionsnow build pyOpenSSL contexts for Eventlet/Twisted with protocol,cert_reqs, cipher, SNI, and hostname-validation handling.Public
ClusterAPI and protocol format are unchanged.In Scope
ssl_options.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. An empty dict encrypts traffic but does not verify the server certificate unless verification options such asca_certs,cert_reqs, orcheck_hostnameare supplied.Existing non-empty legacy
ssl_optionskeep their prior default peer-verification behavior unlesscert_reqsis supplied explicitly.Follow-up PR
Follow-up #941 remains for additional pyOpenSSL parity and cleanup after this branch, including typed SAN parsing, cert/key loading parity, client-routes compatibility with pyOpenSSL-style contexts, and remaining review-driven simplification after rebasing on this PR.
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.