feat(ssl): support custom SSLContext - #4211
Conversation
Allow sync and async Redis clients to use a pre-configured SSLContext for standalone and cluster connections. Preserve the existing SSL option path when no custom context is supplied. Refs redis#3599
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the underlying gap is real. The ssl_* keywords can only express file-based PEM trust and client certificates, so anything needing a pre-built context (OS trust store via truststore, in-memory or PKCS#11 keys, ALPN, SNI callbacks) currently has no supported route, and accepting an SSLContext will cover that gap.
I'd like to take it in a slightly different shape though, which also shrinks the diff considerably.
Rather than branching inside _wrap_socket_with_ssl, let the existing RedisSSLContext own the
decision, and share one holder between both stacks:
- Move
RedisSSLContexttoredis/connection.pyand import it intoredis/asyncio/connection.py.
Async-imports-from-sync is the direction we already use (seeredis/asyncio/cluster.py), and the import binding keepsfrom redis.asyncio.connection import RedisSSLContextworking. This gives one class object, soisinstanceand the annotation mean the same thing on both sides. - Give
RedisSSLContextacontextargument. Please keepcontextreadable and writable — it's pre-existing public state — so store it in a_contextslot behind acontextproperty (a slot and a property can't share a name, and__slots__has no__dict__, so_contextneeds adding there). The setter should validatessl.SSLContext | None, and the getter should keep today's behaviour of returningNoneuntil the context is built or supplied. - When
contextis supplied, ignore the other fields; otherwise build from them as today. With that in the holder,_wrap_socket_with_sslcollapses tocontext = self.ssl_context.get()and the duplicated builder disappears. This also means the context is built once and reused on the sync side rather than rebuilt on every reconnect, which is what we want. The sync-only OCSP block stays where it is — it runs afterwrap_socketand needs the live socket. - Public
ssl_contextshould acceptssl.SSLContext | RedisSSLContext | None, defaultNone.
A holder is used as-is, a raw context is wrapped, anything else raisesRedisErrorat
construction rather than at first connect. The holder's owncontextargument stays
ssl.SSLContext | None— it shouldn't accept another holder. Please also annotate the sync
SSLConnectionparameter; it's currently baressl_context=None. - Honour it only when
ssl=True, consistent with the other SSL options, and document that explicitly. - Add the three missing async properties —
ca_path,ciphers,password— so both stacks expose the same set of concepts. Please don't rename anything: sync keeps
ssl_include_verify_flags,certificate_password,ssl_min_version, and async keeps its existing property names.tests/test_ssl.pyandtests/test_asyncio/test_ssl.pyread several of these directly. - Docstrings should state that a supplied context is used as-is, that the other
ssl_*options are then ignored, and that the caller owns full configuration — mirroring how we word "Argument is ignored when connection_pool is provided". - Please keep the parameter in the same position across
Redis,redis.asyncio.Redis,
RedisClusterandredis.asyncio.RedisCluster. It's currently appended inredis/client.pybut inserted mid-signature inredis/asyncio/cluster.py, whose parameters are all positional-or-keyword, so insertion shifts six later ones(this can break existing apps providing the config args as positional).
One detail to resolve while merging the two builders: they guard the same options with inverted conditions — sync uses is not None for the CA fields and truthiness for ciphers, async the opposite. Since an empty value is never meaningful for any of them (ca_certs="" → FileNotFoundError, ca_path="" → INVALID_DIRECTORY, ca_data="" → Empty certificate data, ciphers="" → No cipher can be selected), please reject empty strings — and empty bytes, since ca_data accepts DER — up front with a RedisError naming the option. The builder conditions can
then simply use truthiness, since only None or non-empty values can reach them. Note ssl_password="" should stay accepted, and min_version keeps its is not None check.
On tests, the current ones assert object identity or a mocked wrap_socket, which doesn't show the context reaching the handshake. tests/test_connect.py::test_tcp_ssl_connect plus
tests/ssl_utils.get_tls_certificates already give you a local TLS server, so please add real handshake coverage on both stacks for both input forms, plus cases for: the holder ignoring other fields when context is set, ssl=False leaving the context unused, an invalid ssl_context type, empty-value rejection per option, cluster and from_url propagation, and the new async properties.
An example in docs/examples/ssl_connection_examples.ipynb would be a nice addition for discoverability, though we don't consider it blocking.
Finally, could you retarget the description to Refs #3599 rather than Fixes? The reporter's TLSV13_ALERT_CERTIFICATE_REQUIRED is a mutual-TLS failure that ssl_certfile/ssl_keyfile should already cover, and we already load the system trust store via ssl.create_default_context(), so we want to confirm their case separately before closing that issue.
Happy to review a WIP push if you'd like feedback before it's complete.
Summary
redis-py currently exposes SSL configuration as individual keyword arguments, which prevents applications from supplying a pre-configured
ssl.SSLContext. This is required for deployments that need TLS settings not represented by the current keyword arguments, such as custom certificate stores or private CA handling.This change:
SSLContextfor synchronous and asynchronous connectionsFixes #3599.
Validation
invoke linters: passedpython -m compileall -q redis tests: passedgit diff --check: passedThe full TLS/integration matrix was not completed because this checkout has no TLS fixture certificates and the local Redis instance does not enable the
DEBUGcommand used by several unrelated connection tests.