Skip to content

PYTHON-5805 CSFLE/QE Support for HTTP Proxies - #3002

Draft
blink1073 wants to merge 53 commits into
mongodb:mainfrom
blink1073:PYTHON-5805
Draft

PYTHON-5805 CSFLE/QE Support for HTTP Proxies#3002
blink1073 wants to merge 53 commits into
mongodb:mainfrom
blink1073:PYTHON-5805

Conversation

@blink1073

@blink1073 blink1073 commented Aug 21, 2026

Copy link
Copy Markdown
Member

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.

  • Adds kms_connect_callback to AutoEncryptionOpts, ClientEncryption, and AsyncClientEncryption, plus a KMSConnectContext.
  • Adds HTTPProxyKMSConnect/AsyncHTTPProxyKMSConnect helpers.
  • Adds internal tests and the required prose tests.
  • Splits TLS wrapping out of the connect helpers in 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, and just docs clean.

Checklist

Checklist for Author

  • Did you update the changelog (if necessary)?
  • Is there test coverage?
  • Is any followup work tracked in a JIRA ticket? If so, add link(s).

Checklist for Reviewer

  • Does the title of the PR reference a JIRA Ticket?
  • Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
  • Is all relevant documentation (README or docstring) updated?

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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, and get_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

Comment thread pymongo/encryption_options.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_timeout after the callback, so time spent establishing the proxy tunnel is not deducted before the KMS TLS handshake. Under automatic encryption with timeoutMS, 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. Name AsyncHTTPProxyKMSConnect in 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

Comment thread pymongo/asynchronous/encryption.py
Comment thread pymongo/encryption_options.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 00200 as success and can call int() on thousands of digits, raising ValueError instead of the intended proxy OSError. Require a three-digit status code before converting it.

pymongo/encryption_options.py:235

  • socket.create_connection applies 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 to create_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

Comment thread pymongo/asynchronous/encryption.py Outdated
Comment thread pymongo/encryption_options.py Outdated
Comment thread pymongo/encryption_options.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

  • sendall and recv are 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 with run_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_request does 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 recv is 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 by proxy_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

Comment thread pymongo/asynchronous/encryption.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 by create_default_context.
    test/asynchronous/test_kms_connect.py:642
  • The HTTPS control requests also disable the ca.pem certificate 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 def also lets synchro generate the corresponding sync wording.
    pymongo/encryption_options.py:124
  • The example imports only HTTPProxyKMSConnect but immediately constructs AutoEncryptionOpts, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants