Add TLS session resumption via SSLSessionCache - #789
Conversation
Such claims would ideally be supported by benchmarks. Could you try to create some? |
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 |
|
We could, if it helps, only support this for TLS 1.3. |
7281340 to
4500773
Compare
|
@dkropachev @Lorak-mmk I pushed changes with improvement from older Dmitry's PR, will update PR description soon |
dkropachev
left a comment
There was a problem hiding this comment.
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.
4500773 to
d12db4a
Compare
dkropachev
left a comment
There was a problem hiding this comment.
Two blocking issues from local validation:
- Twisted caches a TLS session even after hostname verification has already failed, which lets an untrusted peer populate the resumption cache.
SSLSessionCacheacceptsmax_size <= 0and then crashes on the first insert (KeyErrorfrompopitem()on an emptyOrderedDict).
d12db4a to
f8eb94d
Compare
dkropachev
left a comment
There was a problem hiding this comment.
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.
08cabfd to
5a713f1
Compare
5a713f1 to
61f7523
Compare
|
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:
📝 WalkthroughWalkthroughAdds Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation 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. Comment |
afd6ec5 to
2a67558
Compare
2a67558 to
9358a49
Compare
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
nikagra
left a comment
There was a problem hiding this comment.
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.
2e6d4ce to
03c1717
Compare
5a037a3 to
46f065e
Compare
nikagra
left a comment
There was a problem hiding this comment.
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.
| return | ||
|
|
||
| try: | ||
| self._ssl_session_cache.discard(self._tls_session_cache_key()) |
There was a problem hiding this comment.
🟠 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.
| 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: |
There was a problem hiding this comment.
🟠 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.
| 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. |
There was a problem hiding this comment.
🟠 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.
| if session.has_ticket: | ||
| lifetime = session.ticket_lifetime_hint | ||
| if not lifetime: | ||
| if self._socket.version() == 'TLSv1.3': |
There was a problem hiding this comment.
🟠 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.
| # 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 = \ |
There was a problem hiding this comment.
🟢 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?
5cd9f22 to
fff3f4d
Compare
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. The announced lifetime is taken whole rather than reduced by the session's age: this connection established the session moments ago, so that age is one CQL handshake, and SSLSession.time is a wall-clock stamp, so subtracting it would let a clock step landing in between decide the answer -- far enough forward and nothing is cached at all. The deadline the cache keeps is monotonic, so nothing after the store can skew it either. 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 the sessions belonging to one can be found and released: 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. And only the session this connection offered goes: connections to one node are opened together, so another may have stored a fresh one under the same key in the meantime, and that one failed nothing. 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 and then dropped. Asking for resumption and silently getting none is worse than not having it: an unusable cache left in place would be handed to every connection -- which a connection class that does not take the keyword cannot even accept -- and would sit reachable and empty for anyone reading it back, which is also what a server that issues no tickets looks like. So the attribute holds a cache only where one will actually be used, and that is what decides whether connections are given it. Releasing the registration is the last thing shutdown does, and the cache is the caller's object, so a failure there is logged rather than allowed to leave the cluster half torn down. A cluster registers its SSLContext with the cache as it connects and gives that registration back as it shuts down, at which point the sessions established with that context are dropped -- otherwise they would hold its certificate chain and trust store for as long as the cache lived. The registrations are counted rather than assumed to be one apiece: sessions can only be replayed onto the context they came from, so clusters sharing a cache in order to share sessions are also sharing one context, and the first of them to shut down must not take sessions the others are still using. Registering as connections are about to use the cache, rather than in __init__, is what makes the two directions match: both attributes it reads can still be set after construction -- which is how a cluster configured with TLS afterwards gets a cache at all -- and a cluster that is constructed and never connected would otherwise hold a share of the context that nothing gives back. The pair taken is remembered, so shutdown releases that rather than whatever the attributes say by then. 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
fff3f4d to
f636dd4
Compare
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
sslmodule nor pyOpenSSLexposes
SSL_CTX_sess_set_new_cb, so there is no way around doing it by hand.This adds that: one
SSLSessionCacheperCluster, offered to every connection before itshandshake and refreshed after. On by default whenever
ssl_contextis set.Design notes
get()leaves the entry in place, andeach 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 aper-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 handshake. A TLS 1.3 server sends its NewSessionTicket as a post-handshake message;
confirmed against Scylla that
has_ticketisFalseimmediately afterconnect()andTrueafter 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 overan optimisation.
SSLContextis part of the cache key. A session cannot be replayed onto a differentcontext — 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 thoseconnections builds its own context.
costs one full handshake, which is the fallback anyway.
_get_resumable_tls_session/_set_tls_session) so areactor not using the stdlib
sslmodule overrides only those.Not covered
loop.create_connection(..., ssl=...), which offersno point at which a session could be restored.
AsyncioConnectiondeclaressupports_tls_session_resumption = Falseand no cache is created for it.Fixes: https://scylladb.atlassian.net/browse/DRIVER-165
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.