Skip to content

Add TLS session resumption via SSLSessionCache - #789

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

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

Conversation

@sylwiaszunejko

@sylwiaszunejko sylwiaszunejko commented Apr 3, 2026

Copy link
Copy Markdown

What and why

A shard-aware driver opens one TLS connection per shard to every node, and each one currently
pays for a full handshake — certificate exchange plus a signature, which is the expensive part,
especially with certificate authentication. TLS lets a client skip that by replaying a session
established earlier with the same peer (RFC 5077 tickets for TLS 1.2, RFC 8446 PSKs for
TLS 1.3), but OpenSSL never does this on its own: the client has to hold on to the session and
offer it explicitly on the next connection. Neither the stdlib ssl module nor pyOpenSSL
exposes SSL_CTX_sess_set_new_cb, so there is no way around doing it by hand.

This adds that: one SSLSessionCache per Cluster, offered to every connection before its
handshake and refreshed after. On by default whenever ssl_context is set.

cluster = Cluster(ssl_context=ssl_context)                                  # resumption on
cluster = Cluster(ssl_context=ssl_context, ssl_session_cache=None)          # off
cluster = Cluster(ssl_context=ssl_context,
                  ssl_session_cache=SSLSessionCache(max_size=64))           # sized, or shared

Design notes

  • A cached session is not consumed by being used. get() leaves the entry in place, and
    each successful handshake stores a fresh session over it. Measured: one session is accepted by
    four concurrent connections on TLS 1.2 and 1.3, stdlib and pyOpenSSL, and against real Scylla.
    Treating tickets as single-use (removing on get()) would mean only the first connection of a
    per-shard burst resumes — precisely the case this ticket is about. RFC 8446's "SHOULD NOT
    reuse" concerns 0-RTT replay and tracking; the driver sends no early data.
  • The session is stored from the ReadyMessage / AuthSuccessMessage handlers, not right after
    the handshake.
    A TLS 1.3 server sends its NewSessionTicket as a post-handshake message;
    confirmed against Scylla that has_ticket is False immediately after connect() and True
    after the first CQL exchange. Storing is idempotent, so nothing needs to track whether it
    already happened, and every failure in this path is logged and dropped — both call sites are
    wrapped in @defunct_on_error, where a raised exception would kill a healthy connection over
    an optimisation.
  • The SSLContext is part of the cache key. A session cannot be replayed onto a different
    context — the stdlib rejects it with ValueError: Session refers to a different SSLContext.
    That is also why the deprecated ssl_options-only path does not participate: each of those
    connections builds its own context.
  • No TTL. OpenSSL enforces session lifetime itself; a session the server no longer accepts
    costs one full handshake, which is the fallback anyway.
  • Policy is separated from accessors (_get_resumable_tls_session / _set_tls_session) so a
    reactor not using the stdlib ssl module overrides only those.

Not covered

  • asyncio — the handshake happens inside loop.create_connection(..., ssl=...), which offers
    no point at which a session could be restored. AsyncioConnection declares
    supports_tls_session_resumption = False and no cache is created for it.

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
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
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

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

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).

Comment thread cassandra/io/twistedreactor.py Outdated
Comment thread cassandra/connection.py Outdated

@dkropachev dkropachev 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.

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 Outdated
Comment thread cassandra/cluster.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py Outdated
@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 SSLSessionCache, a thread-safe bounded LRU cache for TLS sessions. Cluster creates, disables, or accepts a cache and passes it to connections. Connections derive endpoint-specific keys, restore sessions before handshakes, and store sessions after startup or authentication. Reactor implementations declare unsupported resumption. Tests cover cache behavior, TLS 1.2 and TLS 1.3, concurrency, cache isolation, and shard-aware connections.

Possibly related PRs

Suggested reviewers: mykaul, dkropachev, lorak-mmk

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 19.83% 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 identifies the primary change: adding TLS session resumption through SSLSessionCache.
Description check ✅ Passed The description explains the motivation, design, configuration, limitations, tests, and linked issue. It follows the repository template and is sufficiently complete despite unchecked documentation-re…
Linked Issues check ✅ Passed The description includes a valid Fixes annotation for DRIVER-165.
Out of Scope Changes check ✅ Passed The changes support the stated TLS session resumption objective and add related implementation, reactor declarations, documentation, and tests. No unrelated changes are evident.
Full details: Description check

Explanation

The description explains the motivation, design, configuration, limitations, tests, and linked issue. It follows the repository template and is sufficiently complete despite unchecked documentation-related items.


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
@mykaul

mykaul commented Aug 17, 2026

Copy link
Copy Markdown

@sylwiaszunejko - it's not clear which reactor DOES support resumption, seems like most supports_tls_session_resumption are False ?

@sylwiaszunejko

sylwiaszunejko commented Aug 17, 2026

Copy link
Copy Markdown
Author

@sylwiaszunejko - it's not clear which reactor DOES support resumption, seems like most supports_tls_session_resumption are False ?

Supported (stdlib ssl, inherit True): libev, asyncore, gevent. DefaultConnection resolves to LibevConnection here, with the flag True. Not supported: asyncio (handshake buried in loop.create_connection()), Twisted and Eventlet (pyOpenSSL). CI's matrix is libev / asyncore / asyncio, so two of its three legs exercise the feature.

Btw aren't eventlet, gevent and twisted suppose to be remove in near future? #969 after that we will have 2/3 support

@mykaul

mykaul commented Aug 17, 2026

Copy link
Copy Markdown

@sylwiaszunejko - it's not clear which reactor DOES support resumption, seems like most supports_tls_session_resumption are False ?

Supported (stdlib ssl, inherit True): libev, asyncore, gevent. DefaultConnection resolves to LibevConnection here, with the flag True. Not supported: asyncio (handshake buried in loop.create_connection()), Twisted and Eventlet (pyOpenSSL). CI's matrix is libev / asyncore / asyncio, so two of its three legs exercise the feature.

Btw aren't eventlet, gevent and twisted suppose to be remove in near future? #969 after that we will have 2/3 support

Yes, that was my thought as well.

@nikagra nikagra 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.

Reviewed at a24137d9, comments inline. Tags: blocking = would not approve as is, issue = real but not merge-blocking, question = an assumption I could not verify, nit = cosmetic.

Comment thread cassandra/connection.py
Comment thread cassandra/connection.py
else self.endpoint.tls_session_cache_key)
return (self.ssl_context, endpoint_key)

def _restore_tls_session(self, sock):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

question: Is there a case where a cached session makes a handshake fail outright rather than fall back? ssl.SSLError is an OSError, so _connect_socket would catch it, retry the next address with the same session, and keep re-offering it on every reconnect. RFC 5077 and RFC 8446 both mandate full-handshake fallback, so conformant servers are fine — but there's no way to drop a bad entry if one turns up.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No case I could produce: a session offered to a different node (different STEK), to a server with rotated ticket keys, or across a TLS 1.2/1.3 version change all fall back cleanly with session_reused=False, as RFC 5077 §3.3 and RFC 8446 §4.2.11 require, and the one error I did hit — ValueError: Session refers to a different SSLContext from the setter — is caught in _restore_tls_session, so it degrades to a full handshake. Your reading of the path is right though: ssl.SSLError is an OSError, so a handshake failure would land in _connect_socket's except socket.error, the retry would re-offer the same session because get() peeks rather than pops, and the cache has no per-key invalidation — a bad entry only self-corrects when some connection to that peer succeeds and overwrites it, which is exactly what isn't happening in that scenario.

Comment thread cassandra/connection.py Outdated
Comment thread cassandra/connection.py Outdated
Comment thread cassandra/connection.py
# Whether this connection implementation can restore a cached TLS session
# before the handshake. Reactors that hand the whole TLS setup to a
# third party (asyncio) cannot, and set this to False.
supports_tls_session_resumption = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue: Defaulting to True means a third-party reactor opts in by omission, and it fails silently: on a pyOpenSSL socket sock.session = ... just sets an inert attribute, and _get_resumable_tls_session's self._socket.version() raises AttributeError straight into _store_tls_session's except Exception. Defaulting to False and declaring True on the three stdlib reactors makes that case fail loudly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The version() call this refers to is gone — the predicate is now session.has_ticket or session.id, so nothing raises into that except Exception; on a pyOpenSSL socket getattr(sock, 'session', None) returns None and the store just returns early. Also worth noting there are two stdlib reactors now, not three, since the eventlet/gevent/twisted removal: asyncore and libev inherit True, asyncio declares False.

Comment thread cassandra/cluster.py
Comment thread cassandra/pool.py Outdated
# same node, with the same TLS credentials, so it
# offers and refreshes the session cached for the
# node rather than one of its own.
tls_session_cache_key=self.host.endpoint.tls_session_cache_key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

question: Do the two listeners actually share TLS ticket state? If 9042 can't decrypt a ticket issued by 19142, the single entry alternates between them and neither ever resumes — worse than keying per endpoint. The integration test only exercises this with native_shard_aware_transport_port_ssl explicitly set.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes — verified in both directions, re-run just now on the current branch against Scylla release:2026.1:

9042 ticket -> offered on 19142: reused = True
19142 ticket -> offered on 9042 : reused = True
Both listeners are the same process sharing one GnuTLS credentials object, so they decrypt each other's tickets and the alternation you describe doesn't arise. The end-to-end run agrees: 6 connections go to :19142 and the assertion is 7 of 7 connections resumed, which is only reachable if the single per-node entry works on both ports.
On the test: forcing native_shard_aware_transport_port_ssl is the only way to cover the path, since Scylla leaves it commented out by default — so that part is deliberate.

Comment thread tests/integration/standard/test_tls_resumption.py
Comment thread tests/integration/standard/test_tls_resumption.py
Comment thread tests/unit/test_connection.py Outdated
Comment thread cassandra/connection.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py
Comment thread cassandra/connection.py
Comment thread cassandra/pool.py Outdated
# same node, with the same TLS credentials, so it
# offers and refreshes the session cached for the
# node rather than one of its own.
tls_session_cache_key=self.host.endpoint.tls_session_cache_key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tls_session_cache_key is passed for every shard-aware connection, including plaintext clusters. A custom factory with the previously valid explicit signature then fails with TypeError, even though resumption is inactive. Please pass this override only when a TLS session cache is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The premise holds, but this line isn't what breaks such a class. _make_connection_kwargs already passes ssl_session_cache to every connection, and assigns session_id and driver_config_reporter unconditionally (not even via setdefault) — so a subclass with an explicit signature already gets a TypeError, one argument earlier, TLS or not. If we want to protect custom connection classes from signature churn, I'd rather do it for all of these at once than special-case this one. Maybe separate issue is needed?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is still an independent regression. session_id and driver_config_reporter were already part of the pre-PR contract. ssl_session_cache is another PR-added incompatibility covered separately; once it is gated, this shard-only keyword still breaks the same explicit factory during plaintext shard-aware pool expansion. Both new keywords can share one “resumption active” gate; no separate issue is needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed

@nikagra nikagra 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.

A pass over the TLS-resumption path. The two I'd treat as blocking are the TLS 1.2 ticket_lifetime_hint = 0 handling and the Twisted/Eventlet capability flag — the PR description says those two reactors declare it, but neither file is in the diff.

Comment thread cassandra/connection.py
Comment thread cassandra/connection.py Outdated
Comment thread tests/unit/test_connection.py Outdated
Comment thread cassandra/connection.py
Comment thread cassandra/connection.py
Comment thread tests/unit/test_connection.py Outdated
Comment thread cassandra/connection.py
Comment thread cassandra/pool.py Outdated
Comment thread tests/integration/standard/test_tls_resumption.py Outdated
TLS clients can skip the expensive part of a handshake by replaying a
session established earlier with the same peer (RFC 5077 tickets for
TLS 1.2, RFC 8446 PSKs for TLS 1.3), but OpenSSL never does this on its
own: the client has to hold on to the session and offer it explicitly on
the next connection.

Add the storage half of that: a bounded, thread-safe LRU of TLS sessions
keyed by TLS peer identity, plus an EndPoint.tls_session_cache_key
property that produces the key.  A cached session is not consumed by
being used -- one session can be replayed by any number of concurrent
connections -- so get() leaves the entry in place and each successful
handshake stores a fresh session back over it.

Entries carry the lifetime the caller gives them and are dropped once it
runs out, so a session is never offered past the point the peer said it
would honour it; what that lifetime should be is for the caller to work
out, since it depends on how the session resumes.

SNI endpoints add the server name to their key, since they all share a
proxy address and port but are distinct TLS peers.  Client-routes
endpoints key on the node's host_id rather than the proxy address they
happen to resolve to at the moment.

Nothing uses the cache yet.

Refs DRIVER-165
Offer the cached session for the endpoint before the handshake, and
store the negotiated session once the connection is up, so that the next
connection to the same node -- in particular the burst of per-shard
connections a pool opens at once -- can skip the certificate exchange
and signature of a full handshake.

The session is stored from the ReadyMessage / AuthSuccessMessage
handlers rather than right after the handshake.  A TLS 1.3 server sends
its NewSessionTicket as a post-handshake message, so a session read
straight after connect() carries no ticket and would not resume; by the
time the CQL handshake has completed the ticket has been read off the
socket.  Storing is idempotent, so nothing needs to track whether it
already happened, and every failure in this path is logged and dropped:
resumption is an optimisation, and both call sites are wrapped in
@defunct_on_error, where a raised exception would kill a healthy
connection.

How long a session may be offered is worked out here, because it depends on
how the session resumes: a ticket's lifetime is the one the server
announced, while SSLSession.timeout is only the local context's default and
says nothing about what the peer will still accept, so it is used solely for
a session that resumes by id.  RFC 8446 section 4.6.1 also caps the
client at seven days however long the server asked for.  A zero lifetime is
read against the negotiated version, since the two RFCs disagree on it: TLS
1.3 says discard the ticket immediately, while RFC 5077 section 3.3 reserves
zero for "lifetime unspecified" and leaves retention to local policy, so a
TLS 1.2 ticket is kept and timed by the local timeout.
OpenSSL does not apply either limit on the client's behalf -- it will offer
an expired ticket and let the server refuse it.

A pool reaches a shard-aware node on a second port, which would otherwise
key those connections separately from the one the control connection
established, leaving the whole per-shard burst to handshake in full.  The
endpoint alias that _get_shard_aware_endpoint already builds for that port
therefore carries the node's cache key, so both listeners share one session
and nothing has to be threaded through the connection factory.  The port
stays part of the key by default, so two unrelated TLS servers on one
address still cannot share a session; only an endpoint that names another
node is exempt.

The context is named first in the key so that a cluster can release its own
entries as it shuts down: a cached session keeps a strong reference to the
SSLContext it was established with, so entries left in a cache shared between
clusters would hold a departed cluster's certificate chain and trust store.

The key also carries the name wrap_socket() is given, which is the name
the peer certificate is verified against.  A resumed handshake sends no
Certificate, so that name is never checked again; offering a session to a
connection expecting a different name would silently skip hostname
verification for it.  Both the key and wrap_socket() take the name from
one accessor so the two cannot drift apart.

A session offered on a connection whose handshake then failed is dropped from
the cache.  Both RFCs have a server fall back to a full handshake rather than
fail when it will not resume, so this should not happen; but nothing stores a
fresh session for a connection that never came up, so an entry that did
provoke a failure would otherwise be offered again by every later connection
until its lifetime ran out.  Only a TLS error counts: a refused or reset
connection says nothing about the session.

The session accessors are kept separate from the policy around them, so
that a reactor whose TLS does not go through the stdlib ssl module can
take part by overriding just those.  Connections whose SSLContext is
derived from ssl_options do not participate, because a session cannot be
replayed onto a different context and each of those connections builds
its own.  The asyncio reactor opts out entirely: its handshake happens
inside loop.create_connection(), with no point at which a session could
be restored.

Refs DRIVER-165
Create an SSLSessionCache per Cluster whenever TLS is configured through
ssl_context, and hand it to every connection the cluster opens, so that
resumption is on by default with no configuration.  Pass
ssl_session_cache=None to turn it off, or an instance of your own to size
it or share it between clusters.

No cache is created where resumption cannot work: the deprecated
ssl_options-only path, whose per-connection SSLContexts a session cannot
be replayed onto, and reactors that report they cannot restore a session
before the handshake, which today means asyncio.  connection_class is not
required to derive from Connection, so one that does not report the
capability at all is treated as lacking it rather than raising.

A cache supplied for one of those configurations is warned about.  Asking
for resumption and silently getting none is worse than not having it: the
cache stays reachable and empty, which is also what a server that issues no
tickets looks like, leaving nothing to say which of the two happened.

Refs DRIVER-165
Stand up a TLS server on loopback and connect to it with the driver's own
socket setup, so the restore-before-handshake and store-after-startup
paths run for real and the result is read back the way OpenSSL reports
it, through SSLSocket.session_reused.  Covers TLS 1.2 and TLS 1.3.

Two of these pin down behaviour that is easy to regress: that four
connections opened at once all resume from the single cached session --
the per-shard burst DRIVER-165 is about -- and that on TLS 1.3 nothing is
cached until the server's NewSessionTicket has actually been read off the
socket.

Refs DRIVER-165
Restart the cluster with client encryption on, warm a session cache with
one cluster, then hand it to a second one and require every connection it
opens to have resumed -- which is the question only a real server can
answer: whether it accepts one session offered concurrently by the whole
batch of per-shard connections.

The cluster is given a shard-aware TLS port, since that is the port those
per-shard connections use and therefore where resumption has to pay off;
Scylla leaves it unset by default.  The certificate names every node
rather than only the contact point, or the driver could not build pools to
the rest of the cluster and the test would quietly examine a single host.
Each Session is held for the duration of a test: Cluster.sessions is a
WeakSet, so a dropped Session takes its pools -- everything worth
inspecting -- with it and leaves only the control connection behind.  The
number of connections collected is asserted before their resumption
flags, so the test cannot pass by examining almost nothing.

Follows the reconfigure-and-remove pattern the other modules here use for
cluster-level options, and generates the server certificate with
cryptography so the test does not depend on an openssl binary.

Refs DRIVER-165
Scylla only issues session tickets when enable_session_tickets is set in
client_encryption_options, and that is off by default -- without it the
cache stays empty and every connection performs a full handshake, with no
indication of why.

Refs DRIVER-165
Comment thread cassandra/cluster.py
# certificate chain and trust store for as long as the cache lives.
# Only this cluster's own entries go: a cache may be shared, and the
# other clusters' sessions are still in use.
self.ssl_session_cache.discard_context(self.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.

Two live clusters may share both this cache and the same SSLContext, which is also required for cross-cluster session reuse. Shutting either cluster down removes every cached session for that context, including sessions still used by the other cluster. Please track context ownership and remove these entries only after the last owner shuts down.

Comment thread cassandra/connection.py
return

try:
self._ssl_session_cache.discard(self._tls_session_cache_key())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This unconditional discard can delete a newer session stored concurrently. If connection A offers an old session, connection B succeeds and refreshes the same key, then A fails, A removes B's fresh entry. Please retain the offered session and make this an atomic compare-and-discard.

@nikagra nikagra 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.

Reviewed at 46f065e2, with every point checked against a build of this branch. Tags are 🔴 blocking, 🟠 nit, 🟢 question.

One 🔴 blocking: shutdown() clears the whole SSLContext out of a shared cache instead of just this cluster's entries, so one cluster shutting down empties the cache of another that is still running — the sharing pattern the ssl_session_cache docs recommend.

The rest are six 🟠 nits and one 🟢 question. One thread runs through three of them: the cache key is (ssl_context, endpoint, hostname) and carries no cluster or connection identity, but shutdown() reads it as “my entries”, _discard_tls_session reads it as “the session I offered”, and the shard-aware override reads it as “the same peer”.

The caching itself held up to everything I tried: resumption, refresh on reuse, the TLS 1.3 ticket timing and the LRU bound all behave as documented.

Comment thread cassandra/cluster.py
# certificate chain and trust store for as long as the cache lives.
# Only this cluster's own entries go: a cache may be shared, and the
# other clusters' sessions are still in use.
self.ssl_session_cache.discard_context(self.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.

🔴 blocking The key is (ssl_context, endpoint_key, server_hostname) and holds no cluster identity, so this drops every entry for the context, not only this cluster's. With two clusters sharing a context and a cache — the pattern the ssl_session_cache docs recommend — one shutting down empties the cache of the other while it is still running; I get len(cache) going 2 → 0. The comment just above says the opposite of what the code does.

Comment thread cassandra/cluster.py
kwargs_dict.setdefault('sockopts', self.sockopts)
kwargs_dict.setdefault('ssl_options', self.ssl_options)
kwargs_dict.setdefault('ssl_context', self.ssl_context)
if self.ssl_session_cache is not None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 nit The guard is on the cache being set, but the comment says it should be on resumption being active. Pass a cache plus a connection_class that does not derive from Connection, and __init__ warns that resumption is unavailable and then this still sends ssl_session_cache= to it — TypeError: unexpected keyword argument, and no connection opens. Guard on the same resumable flag __init__ already computed.

Comment thread cassandra/connection.py
first.
"""
with self._lock:
for key in [k for k in self._sessions if k[0] is 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.

🟠 nit k[0] assumes every key is a tuple, and Cluster.shutdown calls this unguarded. A cache shared with non-driver code (test_concurrent_access_keeps_the_cache_bounded uses plain int keys) makes shutdown() raise TypeError: 'int' object is not subscriptable before _discard_cluster_shutdown, leaving is_shutdown set and the cluster half torn down. ssl_session_cache={} fails the same way with AttributeError. Suggest isinstance(k, tuple) and k and k[0] is ssl_context.

Comment thread cassandra/connection.py
return

try:
self._ssl_session_cache.discard(self._tls_session_cache_key())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 nit This drops whatever is under the key now, not the session that was offered. In a per-shard burst the sibling connections have already stored a fresh session by this point, so one TLS error evicts a good entry the others just wrote. Consider discarding only when the cached object is still the one that was offered.

Comment thread cassandra/cluster.py
if self.metrics_enabled and self.metrics:
self.metrics.shutdown()

if self.ssl_session_cache is not None and self.ssl_context is not None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 nit Not in a finally, so if any earlier shutdown step raises, the context is never released — and is_shutdown is already set by then, so a retry returns early and can never clean up. A metrics.shutdown() that throws is enough to pin the context in a shared cache for good.

Comment thread cassandra/cluster.py
Resumption is available when TLS is configured through
:attr:`~Cluster.ssl_context` and the reactor establishes TLS with the
standard library's ``ssl`` module: the ``libev`` and ``asyncore`` reactors,
which is to say the default one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 nit On Python 3.12 asyncore is gone from the standard library, so without the libev extension DefaultConnection resolves to AsyncioConnection and resumption is off. Worth not calling libev/asyncore “the default one” here.

Comment thread cassandra/connection.py
if session.has_ticket:
lifetime = session.ticket_lifetime_hint
if not lifetime:
if self._socket.version() == 'TLSv1.3':

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 nit The comment above says a reactor that does not use the stdlib ssl module only has to override _set_tls_session and _get_resumable_tls_session, but _tls_session_lifetime needs self._socket.version() too. Worth either saying so there or taking the version through the accessor layer.

Comment thread cassandra/pool.py
# Another listener of this same node, with the same TLS
# credentials, so it offers and refreshes the session cached for
# the node rather than one of its own.
endpoint._tls_session_cache_key_override = \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 question This gives the shard-aware port the node's cache key, so a session verified against one listener can be offered to the other — both keys become ('10.0.0.5', 9142) while the endpoint still dials 19142. Behind NAT or a proxy that port may be a different TLS terminator, and a resumed handshake sends no certificate to re-check. Is the same-peer assumption safe here?

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.

6 participants