Skip to content

Add TLS session resumption via SSLSessionCache#789

Open
sylwiaszunejko wants to merge 5 commits into
scylladb:masterfrom
sylwiaszunejko:tls-ticket
Open

Add TLS session resumption via SSLSessionCache#789
sylwiaszunejko wants to merge 5 commits into
scylladb:masterfrom
sylwiaszunejko:tls-ticket

Conversation

@sylwiaszunejko

@sylwiaszunejko sylwiaszunejko commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR implements TLS session resumption for the Python driver. After the first
successful TLS handshake with a node, the negotiated session is stored in a
thread-safe cache and reused on subsequent connections, skipping the full
handshake.

Both TLS 1.2 (session IDs) and TLS 1.3 (session tickets / PSK) are supported.

Changes

cassandra/connection.pySSLSessionCache class & endpoint keys

  • _SessionCacheEntry namedtuple stores (session, timestamp) for TTL tracking.
  • SSLSessionCache: a thread-safe OrderedDict-based cache with LRU eviction,
    TTL expiration, and periodic cleanup (every 100 set() calls), keyed by
    endpoint tls_session_cache_key.
  • Configurable max_size (default 100) and ttl (default 3600 s).
  • Base EndPoint class provides a default tls_session_cache_key property
    returning (address, port). Subclasses override for context-specific keys:
    • DefaultEndPoint: (address, port) — inherits default
    • SniEndPoint: (address, port, server_name) — prevents proxy collisions
    • UnixSocketEndPoint: (unix_socket_path,)
    • ClientRoutesEndPoint: (host_id, address, port)

cassandra/connection.pyConnection wiring

  • Connection gains _ssl_session_cache attribute, set via ssl_session_cache
    kwarg in __init__.
  • _wrap_socket_from_context() restores a cached session via
    ssl_sock.session = ... after wrap_socket(); gracefully handles
    ssl.SSLError / AttributeError if the server rejects the session.
  • _ssl_session_cache_key() helper delegates to
    endpoint.tls_session_cache_key.
  • _cache_tls_session_if_needed() stores socket.session in the cache when
    ssl_context is set and the session is non-None.
  • Sessions are cached at three points to cover both TLS 1.2 and 1.3:
    1. After _initiate_connection() in _connect_socket() — TLS 1.2 sessions
      are available immediately after connect.
    2. After ReadyMessage in _handle_startup_response() — TLS 1.3 tickets
      arrive asynchronously after the first application-data exchange.
    3. After AuthSuccessMessage in _handle_auth_response() — same TLS 1.3
      coverage for authenticated connections.

cassandra/cluster.pyCluster integration

  • Imports SSLSessionCache.
  • Adds ssl_session_cache class attribute with docstring.
  • __init__ accepts ssl_session_cache=_NOT_SET parameter.
  • Auto-creates an SSLSessionCache() when ssl_context or ssl_options are
    set; no configuration required for the common case.
  • Pass ssl_session_cache=None explicitly to opt out.
  • A custom SSLSessionCache(max_size=…, ttl=…) can be supplied.
  • _make_connection_kwargs() passes the cache to every Connection via
    kwargs_dict.setdefault('ssl_session_cache', self.ssl_session_cache).

cassandra/io/eventletreactor.py — Eventlet (PyOpenSSL) support

  • _wrap_socket_from_context() restores cached PyOpenSSL sessions via
    set_session() before the handshake.
  • _initiate_connection() calls _cache_pyopenssl_session() after
    do_handshake().
  • New _cache_pyopenssl_session() helper stores the session via
    get_session(), logs whether the session was reused
    (session_reused()), and catches all exceptions silently.

cassandra/io/twistedreactor.py — Twisted (PyOpenSSL) support

  • _SSLCreator.__init__ accepts an optional ssl_session_cache parameter.
  • clientConnectionForTLS() restores cached sessions via set_session().
  • info_callback() stores sessions after SSL_CB_HANDSHAKE_DONE via
    get_session(), logs reuse status.
  • TwistedConnection.add_connection() passes ssl_session_cache=self._ssl_session_cache
    to _SSLCreator.

Tests

tests/unit/test_connection.py

  • TestSSLSessionCache — empty lookup, set/get, key isolation by
    address/port/SNI, overwrite, thread safety, TTL expiration, LRU eviction,
    max_size enforcement, clear(), clear_expired(), automatic periodic
    cleanup, None session handling.
  • TestEndPointTLSSessionCacheKey — cache key correctness for
    DefaultEndPoint, SniEndPoint, UnixSocketEndPoint,
    ClientRoutesEndPoint, plus isolation between different paths/addresses.
  • TestConnectionSSLSessionRestore — session restore from cache,
    tolerance when cache is None, ssl.SSLError on session setter,
    SNI-specific cached session lookup.
  • TestConnectionCacheTLSSession — session stored after connect,
    no-op when session=None, no-op when cache=None, no-op when
    ssl_context=None, SNI-specific key used for storage.

tests/unit/test_cluster.py

  • TestSSLSessionCacheAutoCreation — auto-create with ssl_context,
    auto-create with ssl_options, no cache without TLS, explicit None
    opt-out, custom cache injection, cache passed to connection_factory.

Fixes: https://scylladb.atlassian.net/browse/DRIVER-165

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Lorak-mmk

Copy link
Copy Markdown

This reduces reconnection latency and CPU overhead, especially in
deployments with short-lived connections or frequent reconnects.

Such claims would ideally be supported by benchmarks. Could you try to create some?
I very vaguely remember this feature being postponed because the performance gains were underwhelming (but perhaps memory is failing me).

@sylwiaszunejko

Copy link
Copy Markdown
Collaborator Author

This reduces reconnection latency and CPU overhead, especially in
deployments with short-lived connections or frequent reconnects.

Such claims would ideally be supported by benchmarks. Could you try to create some? I very vaguely remember this feature being postponed because the performance gains were underwhelming (but perhaps memory is failing me).

That's the goal, but you're right, I don't have any tests to prove that, removed this claim from the PR description. If I manage to create proper benchmarks I will update on that

@mykaul

mykaul commented Apr 3, 2026

Copy link
Copy Markdown

We could, if it helps, only support this for TLS 1.3.

@sylwiaszunejko

Copy link
Copy Markdown
Collaborator Author

@dkropachev @Lorak-mmk I pushed changes with improvement from older Dmitry's PR, will update PR description soon

@dkropachev dkropachev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked the TLS session-resumption path against the current branch. The ssl_options configuration still builds a fresh SSLContext per Connection, and a cached stdlib session from the previous connection is incompatible with that new context. I reproduced the failure locally on Python 3.10.12; the session restore path raises ValueError: Session refers to a different SSLContext. Since the new code only catches AttributeError and ssl.SSLError, reconnects fail instead of falling back to a full handshake, and the regression is enabled by default because Cluster auto-creates SSLSessionCache for ssl_options.

Comment thread cassandra/connection.py Outdated

@dkropachev dkropachev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two blocking issues from local validation:

  1. Twisted caches a TLS session even after hostname verification has already failed, which lets an untrusted peer populate the resumption cache.
  2. SSLSessionCache accepts max_size <= 0 and then crashes on the first insert (KeyError from popitem() on an empty OrderedDict).

transport = connection.get_app_data()
transport.failVerification(Failure(ConnectionException("Hostname verification failed", self.endpoint)))
# Store TLS session after successful handshake (PyOpenSSL)
if self.ssl_session_cache is not None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failVerification() should short-circuit this callback. As written, a hostname mismatch still falls through and caches the just-negotiated session, so an untrusted peer can seed the resumption cache. I reproduced this locally with a mocked _SSLCreator: failVerification was called and the session still landed in SSLSessionCache.

Comment thread cassandra/connection.py
self._sessions.move_to_end(key)
return

if len(self._sessions) >= self._max_size:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSLSessionCache(max_size=0) currently crashes on the first insert: len(self._sessions) >= self._max_size is already true for an empty cache, so popitem(last=False) raises KeyError. Since this is now a public tuning knob, please validate max_size > 0 (and probably ttl > 0) or define zero as a disabled cache, and cover it with a unit test.

@dkropachev dkropachev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two correctness issues need attention before this lands: the PyOpenSSL TLS 1.3 cache point is too early to capture the resumable session, and the cache can evict a live entry while expired ones remain resident.

Comment thread cassandra/io/twistedreactor.py Outdated
Comment thread cassandra/connection.py Outdated
Comment thread cassandra/io/twistedreactor.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py
Comment thread cassandra/cluster.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a thread-safe LRU/TTL SSLSessionCache, endpoint-specific cache keys, and optional Cluster configuration with automatic cache creation. Connections and Eventlet/Twisted TLS reactors restore cached sessions before handshakes and store negotiated sessions afterward. Unit and integration tests cover cache behavior, endpoint identity, cluster wiring, reactor verification, and reconnection-based TLS ticket reuse.

Sequence Diagram(s)

sequenceDiagram
  participant Cluster
  participant Connection
  participant SSLSessionCache
  participant TLSReactor
  Cluster->>Connection: create with resolved ssl_session_cache
  Connection->>SSLSessionCache: retrieve endpoint session
  SSLSessionCache-->>Connection: cached session
  Connection->>TLSReactor: restore session before handshake
  TLSReactor-->>Connection: complete TLS handshake
  Connection->>SSLSessionCache: store negotiated session
Loading

Suggested reviewers: lorak-mmk, dkropachev, mykaul

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly names the main change: adding TLS session resumption via SSLSessionCache.
Description check ✅ Passed The PR description covers the required summary, changes, tests, and Fixes link; only the docstring/docs checklist items are left unchecked.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from dkropachev July 15, 2026 08:11
@coderabbitai
coderabbitai Bot requested a review from mykaul July 15, 2026 08:11

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/integration/standard/test_tls_resumption.py (1)

156-187: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Broad exception swallowing could mask real setup errors as "resumption unsupported".

Catching bare Exception and returning False means genuine connectivity/TLS-config errors (bad cert path, wrong port, etc.) silently produce a test skip instead of surfacing as a failure.

🤖 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/integration/standard/test_tls_resumption.py` around lines 156 - 187,
Update _server_supports_tls_resumption to stop catching all Exception instances
and converting setup or connection failures into False. Restrict the handled
exceptions to the expected “TLS resumption unsupported” cases, while allowing
invalid certificate paths, connectivity failures, and other configuration errors
to propagate as test failures; preserve False for a valid handshake where no
session is produced or the second session is not reused.

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.

Inline comments:
In `@tests/integration/standard/test_tls_resumption.py`:
- Around line 134-153: Update _make_ssl_context so the USES_PYOPENSSL branch
also caps the created context at TLS 1.2 by setting its maximum protocol version
to SSL.TLS1_2_VERSION, matching the stdlib branch while leaving the certificate
setup unchanged.

---

Nitpick comments:
In `@tests/integration/standard/test_tls_resumption.py`:
- Around line 156-187: Update _server_supports_tls_resumption to stop catching
all Exception instances and converting setup or connection failures into False.
Restrict the handled exceptions to the expected “TLS resumption unsupported”
cases, while allowing invalid certificate paths, connectivity failures, and
other configuration errors to propagate as test failures; preserve False for a
valid handshake where no session is produced or the second session is not
reused.
🪄 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: 49e185b2-4dea-4614-baa7-96ec644f5534

📥 Commits

Reviewing files that changed from the base of the PR and between 6e12577 and 61f7523.

📒 Files selected for processing (8)
  • cassandra/cluster.py
  • cassandra/connection.py
  • cassandra/io/eventletreactor.py
  • cassandra/io/twistedreactor.py
  • tests/integration/standard/test_tls_resumption.py
  • tests/unit/io/test_twistedreactor.py
  • tests/unit/test_cluster.py
  • tests/unit/test_connection.py

Comment on lines +134 to +153
def _make_ssl_context(ca_cert_path):
"""Return a client ``ssl.SSLContext`` that trusts the test CA at *ca_cert_path*.

TLS 1.2 is explicitly required: Python's ``ssl.SSLSocket.session_reused``
maps to OpenSSL's ``SSL_session_reused()``, which always returns *False*
for TLS 1.3 (PSK resumption is not reflected by that API). Forcing
TLS 1.2 ensures ``session_reused`` is set correctly after a resumed
handshake, which is required by the test's assertion logic.
"""
if USES_PYOPENSSL:
ssl_context = SSL.Context(SSL.TLS_CLIENT_METHOD)
ssl_context.load_verify_locations(ca_cert_path)
else:
ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
ssl_context.load_verify_locations(ca_cert_path)
ssl_context.verify_mode = ssl.CERT_REQUIRED
ssl_context.check_hostname = False
# Restrict to TLS 1.2 so that session_reused is reliable.
ssl_context.maximum_version = ssl.TLSVersion.TLSv1_2
return ssl_context

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

rg -n "pyOpenSSL|pyopenssl|OpenSSL" -S .

Repository: scylladb/python-driver

Length of output: 5629


🏁 Script executed:

sed -n '1,340p' tests/integration/standard/test_tls_resumption.py

Repository: scylladb/python-driver

Length of output: 12859


🏁 Script executed:

sed -n '180,330p' cassandra/io/twistedreactor.py && printf '\n---\n' && sed -n '1,220p' cassandra/io/eventletreactor.py && printf '\n---\n' && rg -n "pyOpenSSL|pyopenssl|pyopenssl|OpenSSL|cryptography" -S requirements* setup* pyproject.toml tox.ini constraints* .

Repository: scylladb/python-driver

Length of output: 21028


🌐 Web query:

pyOpenSSL Context set_max_proto_version TLS1_2_VERSION API docs

💡 Result:

In pyOpenSSL, the method set_max_proto_version is available on Context objects to configure the maximum supported TLS protocol version [1][2][3]. To restrict the maximum protocol version to TLS 1.2, you should pass the constant OpenSSL.SSL.TLS1_2_VERSION to this method [1][2]. Usage example: context = OpenSSL.SSL.Context(OpenSSL.SSL.TLS_METHOD) context.set_max_proto_version(OpenSSL.SSL.TLS1_2_VERSION) Key points regarding this API: 1. Availability: This method was added to provide explicit control over supported TLS versions, replacing older, deprecated protocol-specific methods (such as TLSv1_2_METHOD) [4][5][3]. 2. Functionality: Setting the maximum version to 0 (the default behavior if not restricted) allows the library to negotiate up to the highest version supported by the underlying OpenSSL build [1][2]. 3. Error Handling: If the underlying OpenSSL library does not support the version requested, the method will raise an exception [1][2]. Official documentation can be found in the pyOpenSSL SSL module API reference under Context objects [6][1][2].

Citations:


Cap the PyOpenSSL context at TLS 1.2

_make_ssl_context only restricts the stdlib branch today. Add the same TLS 1.2 cap to the USES_PYOPENSSL branch (ssl_context.set_max_proto_version(SSL.TLS1_2_VERSION)) so session_reused() stays meaningful for this test under Twisted/Eventlet.

🤖 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/integration/standard/test_tls_resumption.py` around lines 134 - 153,
Update _make_ssl_context so the USES_PYOPENSSL branch also caps the created
context at TLS 1.2 by setting its maximum protocol version to
SSL.TLS1_2_VERSION, matching the stdlib branch while leaving the certificate
setup unchanged.

@classmethod
def tearDownClass(cls):
if cls._cert_dir:
shutil.rmtree(cls._cert_dir, ignore_errors=True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setUpClass enables client_encryption_options on the shared single_node CCM cluster, but teardown only deletes the generated certs. Later standard tests that reuse use_single_node() will inherit TLS config and connect without SSL, and any restart will point Scylla at deleted certificate files. Please restore the CCM TLS configuration/restart, or remove the cluster, before deleting the cert directory.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment thread cassandra/cluster.py Outdated
sessions are cached after the CQL handshake completes (Ready / AuthSuccess),
because session tickets are sent asynchronously by the server.

Works with all connection classes: stdlib ``ssl`` (asyncore, libev, gevent,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This advertises asyncio support, but AsyncioConnection does not use the shared TLS cache path. It overrides _connect_socket() so self._socket stays plain, then performs TLS through loop.create_connection(..., ssl=ssl_context). Nothing restores a cached session before the handshake or stores transport.get_extra_info("ssl_object").session afterward, so EVENT_LOOP_MANAGER=asyncio never populates or uses the cache. Please wire the asyncio reactor into session restore/store, or exclude it from the advertised support.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded it from the advertised support

@sylwiaszunejko

Copy link
Copy Markdown
Collaborator Author

CI failures unrelated to these PR, they are due to #931

Comment thread cassandra/io/twistedreactor.py Outdated
# For TLS 1.2 the session is already available at handshake completion; cache
# it immediately. For TLS 1.3 get_session() returns None here and the deferred
# path in TwistedConnection._cache_tls_session_if_needed() handles it instead.
if self.ssl_session_cache is not None and not connection.session_reused():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current pyOpenSSL OpenSSL.SSL.Connection does not expose session_reused(), only get_session()/set_session(). This raises from the handshake info callback when Twisted TLS is used with the auto-created cache, so the session never gets cached and this reactor cannot actually resume TLS sessions.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

Comment thread cassandra/io/eventletreactor.py Outdated
if self._ssl_session_cache is None or not (self.ssl_context or self.ssl_options):
return
try:
if self._socket.session_reused():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same PyOpenSSL API problem: the green SSL.Connection exposes get_session() but not session_reused(). The resulting AttributeError is swallowed by the broad except, so we never reach get_session() and the Eventlet session cache remains empty.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/unit/io/test_twistedreactor.py (1)

245-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused unpacked session variable.

Ruff flags session as unused in test_hostname_mismatch_does_not_cache (line 245) and test_no_check_hostname_caches_session (line 276).

Proposed fix
-        creator, connection, transport, session = self._make_creator(
+        creator, connection, transport, _session = self._make_creator(
             check_hostname=True,
             endpoint_address='good.example.com',
             cert_cn='evil.example.com',
             ssl_session_cache=cache,
         )

Also applies to: 276-276

🤖 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` at line 245, Remove the unused session
unpacking from _make_creator calls in test_hostname_mismatch_does_not_cache and
test_no_check_hostname_caches_session, while preserving the other returned
values and test 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.

Inline comments:
In `@tests/integration/standard/test_tls_resumption.py`:
- Around line 106-133: Update _TrackingSSLSessionCache.set so ticket_count
represents only unique fresh TLS sessions rather than every non-None write;
deduplicate using the cache key/session identity, or skip the equality assertion
when USES_PYOPENSSL applies. Preserve accurate fresh-handshake counting for
paths that expose session reuse.

---

Nitpick comments:
In `@tests/unit/io/test_twistedreactor.py`:
- Line 245: Remove the unused session unpacking from _make_creator calls in
test_hostname_mismatch_does_not_cache and test_no_check_hostname_caches_session,
while preserving the other returned values and test 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: baf7a667-420e-436e-a9fb-2cb4a161d94e

📥 Commits

Reviewing files that changed from the base of the PR and between 03af527 and f2faf64.

📒 Files selected for processing (4)
  • cassandra/io/eventletreactor.py
  • cassandra/io/twistedreactor.py
  • tests/integration/standard/test_tls_resumption.py
  • tests/unit/io/test_twistedreactor.py

Comment thread tests/integration/standard/test_tls_resumption.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds TLS session resumption support to the driver by introducing a shared session cache keyed per endpoint, and wiring that cache into the stdlib-ssl and PyOpenSSL connection paths so subsequent connections can reuse prior TLS sessions (TLS 1.2 and TLS 1.3).

Changes:

  • Introduces SSLSessionCache (LRU + TTL) and per-endpoint tls_session_cache_key for correct cache isolation (incl. SNI / unix socket / client-routes).
  • Plumbs the cache from Cluster into Connection and uses it to restore/store TLS sessions across connection lifecycles.
  • Adds unit + integration coverage for cache semantics and for session reuse across reconnections (where observable).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
cassandra/connection.py Adds endpoint cache keys, SSLSessionCache, and stdlib-ssl connection integration for restore/store.
cassandra/cluster.py Adds Cluster.ssl_session_cache and passes it into connection creation.
cassandra/io/eventletreactor.py Adds PyOpenSSL session restore and post-handshake session caching for Eventlet connections.
cassandra/io/twistedreactor.py Adds PyOpenSSL session restore + caching hooks via _SSLCreator and info_callback().
cassandra/io/asyncioreactor.py Documents that session resumption is not supported on the asyncio reactor.
tests/unit/test_connection.py Adds unit tests for SSLSessionCache, endpoint keying, and stdlib session restore/store wiring.
tests/unit/test_cluster.py Adds unit tests for cluster-level cache auto-creation and propagation to connections.
tests/unit/io/test_twistedreactor.py Adds Twisted-side tests around session caching behavior on handshake completion.
tests/integration/standard/test_tls_resumption.py Adds an integration test asserting resumption behavior across reconnections.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cassandra/connection.py
Comment thread cassandra/connection.py Outdated
Comment thread cassandra/cluster.py Outdated
Comment thread tests/unit/io/test_twistedreactor.py Outdated
Comment thread tests/unit/test_connection.py Outdated
Comment thread tests/unit/test_connection.py Outdated
Comment thread tests/unit/test_connection.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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/integration/standard/test_tls_resumption.py`:
- Around line 176-193: Update _server_supports_tls_resumption to replace the
blanket except Exception with an exception tuple containing only the expected
socket and SSL connection/handshake failure types, while preserving the existing
return False behavior for those failures and allowing unexpected programming
errors to propagate.
- Around line 259-286: Register class cleanup in setUpClass before stopping the
shared CCM cluster, ensuring failure during configuration or startup restores
the cluster state. Update the cleanup to restart or remove the CCM cluster as
appropriate, while preserving the existing stop, configuration, and start
sequence.
- Line 41: Guard the module-level OpenSSL import in the TLS resumption test so
missing pyOpenSSL does not fail test collection. Set an unavailable marker for
SSL and skip the Twisted/Eventlet execution path when SSL cannot be imported,
while preserving the existing behavior when the import succeeds.
🪄 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: 3267e3fb-7cc3-4650-a701-56702bea5c6a

📥 Commits

Reviewing files that changed from the base of the PR and between f2faf64 and 7f65023.

📒 Files selected for processing (1)
  • tests/integration/standard/test_tls_resumption.py

Comment thread tests/integration/standard/test_tls_resumption.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py
Comment thread tests/integration/standard/test_tls_resumption.py
Introduce SSLSessionCache in connection.py: a thread-safe OrderedDict-based
cache with LRU eviction (max_size, default 100) and TTL expiration (default
3600s), keyed by endpoint tls_session_cache_key.

Add tls_session_cache_key property to all EndPoint subclasses:
  - DefaultEndPoint: (address, port)
  - SniEndPoint: (address, port, server_name) — prevents proxy collisions
  - UnixSocketEndPoint: (unix_socket_path,)
  - ClientRoutesEndPoint: (host_id, address, port)

Includes unit tests for basic ops, key isolation, SNI keys, overwrite,
thread safety, TTL expiration, LRU eviction, clear/clear_expired,
automatic cleanup, custom parameters, and endpoint cache key tests.
- Add _ssl_session_cache attribute on Connection, set via ssl_session_cache param
- Restore cached TLS sessions in _wrap_socket_from_context with error tolerance
- Add _cache_tls_session_if_needed helper (delegates to endpoint.tls_session_cache_key)
- Cache sessions at 3 points: after connect, ReadyMessage, AuthSuccessMessage
  (handles TLS 1.3 async ticket delivery)
- Add TestConnectionSSLSessionRestore and TestConnectionCacheTLSSession tests
- Import SSLSessionCache in cluster.py
- Add ssl_session_cache attribute with comprehensive docstring
- Add ssl_session_cache parameter to Cluster.__init__ (default _NOT_SET)
- Auto-create SSLSessionCache when ssl_context or ssl_options are set
- Pass ssl_session_cache to connection factory via _make_connection_kwargs
- Add TestSSLSessionCacheAutoCreation tests (6 tests)
- EventletConnection: restore cached session before handshake via set_session()
- TwistedConnection: pass ssl_session_cache to _SSLCreator, restore cached
  session in clientConnectionForTLS()
- Both reactors: defer session storage to _cache_tls_session_if_needed() override
  called at ReadyMessage / AuthSuccessMessage time, ensuring TLS 1.3 session
  tickets (which arrive after the first application-data exchange) are captured
- Skip caching when session_reused() is True (abbreviated handshake)
- All operations wrapped in try/except for error tolerance
- Debug logging for session reuse and restore/store failures
Tests TLS ticket resumption end-to-end using a dynamically generated
CA + server certificate pair. The test spins up a single-node CCM
cluster configured for TLS, opens multiple connections, and verifies
that subsequent connections reuse the TLS session rather than
performing a full handshake.

Skips automatically when the Scylla CCM node does not support
server-side TLS session resumption (i.e. does not echo the session
ticket back on reconnect).
Copilot AI review requested due to automatic review settings July 22, 2026 13:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Comment thread cassandra/connection.py
Comment on lines +1277 to +1281
if session_reused:
return
if session is not None:
self._ssl_session_cache.set(self._ssl_session_cache_key(), session)
self._tls_session_cached = True
Comment on lines +166 to +178
if self._tls_session_cached:
return
if self._ssl_session_cache is None or not (self.ssl_context or self.ssl_options):
return
# Note: pyOpenSSL's SSL.Connection exposes no session_reused(), so we cannot
# skip resumed handshakes; the per-connection ``_tls_session_cached`` guard
# above still prevents storing twice for the same connection.
try:
session = self._socket.get_session()
if session is not None:
self._ssl_session_cache.set(
self._ssl_session_cache_key(), session)
self._tls_session_cached = True
Comment on lines +189 to +193
if self.ssl_session_cache is not None:
session = connection.get_session()
if session is not None:
self.ssl_session_cache.set(endpoint.tls_session_cache_key, session)
connection._cassandra_session_cached = True
Comment on lines +192 to +193
self.ssl_session_cache.set(endpoint.tls_session_cache_key, session)
connection._cassandra_session_cached = True
Comment on lines +158 to +159
# Restrict to TLS 1.2 so that session_reused is reliable.
ssl_context.maximum_version = ssl.TLSVersion.TLSv1_2
Comment on lines +350 to +355
# PyOpenSSL's SSL.Connection exposes no session_reused(), so the
# driver re-stores the session on every (including resumed)
# handshake and ticket_count cannot distinguish resumption from
# renegotiation. Only assert unchanged counts on the stdlib ssl
# paths, where resumed handshakes are filtered out upstream.
if not USES_PYOPENSSL:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants