PYTHON-5805 CSFLE/QE Support for HTTP Proxies - #3002
Conversation
Implements all six cases of spec section 28 "KMS Connect Callback": plain and TLS proxy tunneling via kms_connect_callback, auto encryption through a proxy, callback error propagation, timeout visibility on KMSConnectContext, and retry after a callback network error.
… 5 gap The docstring promised the driver could pass timeout=None, but the only producer (max(_csot.clamp_remaining(...), 0.001)) is always a positive float. Reworded to describe actual behavior without narrowing the Optional[float] type. Also added a comment on the case 5 timeout assertion in TestKmsConnectCallbackProse recording that explicit ClientEncryption operations set no CSOT deadline, so timeoutMS on the key-vault client does not currently tighten the value asserted.
Case 5 asserts the KMS connect callback receives a non-zero timeout. That cannot fail in PyMongo: ClientEncryption does not support timeoutMS and explicit encryption operations establish no CSOT deadline, so the callback always receives the default KMS connect timeout. Skip the case rather than leave it passing vacuously, and record the deviation on KMSConnectContext, which the CSOT specification requires for any blocking section timeoutMS does not cover. Tracked in PYTHON-6037.
Factor the duplicated HTTP CONNECT handshake out of the two KMSConnectContext examples so the TLS-proxy example shows only what is different about it, the socketpair relay. Trim the CSOT deviation note, the changelog entry, and the case 5 comment, which restated the reason already carried by the skip decorator. No behavior change.
…pwire Raise ConfigurationError for kms_connect_callback contract violations instead of a private sentinel exception. It is the right public type for a misconfigured callback, and the no-retry clause in kms_request now keys off it. Remove the flavor-specific 'Must be a coroutine function.' sentence from the ClientEncryption parameter docs. KMSConnectContext already documents both flavors, so the synchro replacement entry and the tripwire that guarded it are no longer needed. Rewording the awaitable-guard message also removes the split string literals that dodged synchro's rewriting.
ssl.SSLContext.wrap_socket refuses a non-blocking socket, and a callback has no reason to care which mode it leaves the socket in. Set the timeout in _connect_kms rather than pushing the requirement onto the caller. Do it there and not in _async_wrap_socket_tls, which is shared with every MongoDB connection and currently handshakes under the connect-derived timeout; forcing socket_timeout for all callers would change behavior on that path. Narrow the docstring accordingly. The real constraint is a real socket the event loop is not managing, not the blocking mode: asyncio streams and transports are not sockets, and the socket under one stays registered with the loop.
transport.get_extra_info('socket') returns an asyncio TransportSocket,
which is not a socket.socket, so the existing contract check already
rejects it. Say so in the message and point at loop.sock_connect, rather
than leaving the caller to work out why their socket was refused.
Also correct the docstring: loop.sock_connect leaves an ordinary socket
behind once it completes, so it is usable here. Only streams, transports,
and the transport socket underneath them are not.
Merge the two KMSConnectContext paragraphs that both described what the callback returns, and shorten the asyncio guidance to the three facts a caller needs. Shorten the contract error: an exception should point at the mistake, not restate the docstring. Drop four test comments that the assertion on the next line already states, and condense the ones that explain something non-obvious.
Writing a callback by hand meant implementing the CONNECT handshake and, for a TLS proxy, a socketpair bridge with two relay threads, because Python cannot layer TLS over an ssl.SSLSocket. That is the most delicate code in the feature and every user would have copied it from a docstring. Ship it instead. HTTPProxyKMSConnect and its async variant handle plain and TLS proxies and are usable directly as kms_connect_callback. The callback option is unchanged and remains the escape hatch for cases the helper does not cover, such as proxy authentication. The prose tests now drive the shipped class rather than a private copy, so the spec tests cover the public API. KMSConnectContext's docstring drops from 117 lines to 44, since it no longer carries two worked examples.
asyncio.to_thread is run_in_executor(None, ...) plus a contextvars copy. The propagation buys nothing here, since the helper takes its timeout as an argument and never reads the CSOT contextvar in the thread, so the only effect was introducing a second idiom for a job auth_oidc.py already does one way when it runs a user-supplied callback off the loop.
The worked CONNECT example moved to HTTPProxyKMSConnect when the helper landed, so the cross-reference to KMSConnectContext was stale.
# Conflicts: # doc/changelog.rst
# Conflicts: # doc/changelog.rst
This reverts commit a60ed67.
There was a problem hiding this comment.
🟡 Changes recommended
HTTP CONNECT status parsing must be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/asynchronous/test_encryption.py:19
- The KMS callback tests were moved to
test_kms_connect.py, but their newly added imports remain here unused (asyncio,threading,time,TransportSocket,mock, the KMS helpers/private encryption symbols,PoolOptions, andget_ssl_context). Remove them from this asynchronous source test and regenerate the synchronous mirror to avoid stale dependencies and import-time overhead.
import asyncio
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Critical callback-validation and request-injection issues must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
pymongo/asynchronous/encryption.py:192
- This restores the original
opts.socket_timeoutafter the callback, so time spent establishing the proxy tunnel is not deducted before the KMS TLS handshake. Under automatic encryption withtimeoutMS, a callback can consume nearly the entire remaining budget and the handshake then receives that budget again, allowing the operation to overrun its deadline. Track a deadline across callback execution and TLS wrapping, or recompute the remaining CSOT budget before the handshake.
pymongo/asynchronous/encryption.py:175
- The async error directs users to
HTTPProxyKMSConnect, but that synchronous helper is incompatible with this API and can block the event loop before being rejected. NameAsyncHTTPProxyKMSConnectin the async source; the synchro mapping will produce the synchronous helper name in the generated module.
raise ConfigurationError(
"kms_connect_callback must return a connected, unwrapped "
f"socket.socket, not {type(sock)}; consider HTTPProxyKMSConnect."
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Five moderate timeout/deadline and HTTP status-validation issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
pymongo/encryption_options.py:176
- HTTP status codes are exactly three digits, but this accepts malformed codes such as
00200as success and can callint()on thousands of digits, raisingValueErrorinstead of the intended proxyOSError. Require a three-digit status code before converting it.
pymongo/encryption_options.py:235
socket.create_connectionapplies this timeout separately to every address returned by DNS. For a dual-stack or multi-address proxy, each unreachable address can consume the entire remaining budget, so this call can exceed the KMS/CSOT deadline by a multiple. Resolve and try addresses with_remaining(deadline)recomputed for each attempt instead of delegating the whole loop tocreate_connection.
sock = socket.create_connection((self.host, self.port), timeout=_remaining(deadline))
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Async TLS-handshake cancellation can leak sockets and proxy threads; async tests also contain blocking socket operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
test/asynchronous/test_kms_connect.py:315
sendallandrecvare blocking socket calls executed directly by this coroutine; if the delayed proxy response never arrives, the event loop is blocked for up to 10 seconds. Offload them withrun_in_executor(while retaining the sync branch for synchro generation).
sock.settimeout(10)
sock.sendall(b"ping")
self.assertEqual(sock.recv(64), b"echo:ping")
test/asynchronous/test_kms_connect.py:269
- These synchronous socket operations run inside an async test and can block the event-loop thread for up to the configured 10-second timeout when the relay stalls. Run both operations in an executor (as
proxy_requestdoes below) so failures do not freeze the async test loop.
sock.settimeout(10)
sock.sendall(b"ping")
self.assertEqual(sock.recv(64), b"echo:ping")
test/asynchronous/test_kms_connect.py:379
- This blocking
recvis invoked on the event-loop thread and can stall the entire async test for up to 10 seconds if the expected coalesced bytes are delayed. Await an executor-backed receive instead, following the pattern used byproxy_request.
sock.settimeout(10)
self.assertEqual(sock.recv(64), b"early-bytes")
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The HTTPS prose tests disable required certificate verification, and some new API documentation is incomplete.
Review details
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
test/asynchronous/test_kms_connect.py:626
- This disables both certificate and hostname verification for the HTTPS proxy, so prose Case 2 can pass with an untrusted or wrong-host proxy certificate. The specification requires the callback's proxy TLS connection to be verified with
ca.pem; keep the verification defaults established bycreate_default_context.
test/asynchronous/test_kms_connect.py:642 - The HTTPS control requests also disable the
ca.pemcertificate and hostname checks, contrary to the prose setup for Case 2. Leaving the default context verification enabled ensures reset/metrics calls fail when the proxy presents an invalid identity.
test/asynchronous/test_kms_connect.py:728 - This only proves that some KMS connection occurred. Prose Case 3 also expects exactly one request after the reset, verifying that the subsequent decrypt reuses the cached key; asserting equality would cover that required behavior.
pymongo/asynchronous/encryption.py:696 - This async API rejects ordinary callables before invoking them, but the new parameter documentation says any callable is accepted. Document the coroutine requirement so users do not pass the synchronous helper or a blocking function; phrasing it with
async defalso lets synchro generate the corresponding sync wording.
pymongo/encryption_options.py:124 - The example imports only
HTTPProxyKMSConnectbut immediately constructsAutoEncryptionOpts, so it cannot run as shown. Import both public classes in the snippet.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
PYTHON-5805
Changes in this PR
Lets CSFLE and Queryable Encryption route KMS traffic through an HTTP proxy via a user-supplied connect callback, while still performing the KMS TLS handshake end to end against the KMS host.
kms_connect_callbacktoAutoEncryptionOpts,ClientEncryption, andAsyncClientEncryption, plus aKMSConnectContext.HTTPProxyKMSConnect/AsyncHTTPProxyKMSConnecthelpers.pymongo/pool_shared.py, leaving existing connection paths unchanged.Test Plan
Ran the encryption prose suite against the drivers-evergreen-tools proxies: 10 passed, 2 skipped.
just lint,just typing, andjust docsclean.Checklist
Checklist for Author
Checklist for Reviewer