Repository navigation
Conversation
…result feed_kms passed timeout_ms straight to Timeout.timeout, which takes seconds, so a CSOT budget of 30000 bounded socket reads and writes to 30000 seconds. Convert the budget to seconds and use the CSOT timeout error class when a budget is in effect. Also reject a kms_connect_callback that returns nil or a non-IO value with a clear KmsError, instead of silently dialing the KMS host directly.
build_tcp_socket ran outside the rescue in SSL#initialize, so a failure in setsockopt or set_socket_options leaked the freshly created socket. It now closes the socket before re-raising. Likewise, when create_context raised, a caller-provided :connected_socket was never closed because @tcp_socket had not been assigned yet. Close it on that failure path.
connect_callback always opened a plain TCPSocket and wrote a cleartext CONNECT, so against a proxy started in TLS mode (--ca_file/--cert_file) the TLS handshake failed with ECONNRESET and spec case 2 could never pass. Add a tls: keyword to connect_callback. When true, the CONNECT handshake runs over a TLS connection to the proxy, and the callback returns one end of a socketpair bridged to the proxy by a background thread. The bridge is needed because the driver wraps the returned socket in TLS to the KMS host, and Ruby's OpenSSL::SSL::SSLSocket cannot wrap another SSLSocket. Also pass extra_options from the auto-encryption client (matching every other FLE integration spec) and record the deliberate timeout_ms deviation in case 5.
The option docs described the callback's contract but gave no usage example and did not state the return type precisely. The returned object must be fd-backed (a real IO or Socket) because the driver wraps it in TLS to the KMS host and Ruby's OpenSSL requires an fd-backed object; a TLS-to-proxy connection therefore needs a socketpair bridge. Add a usage example to ClientEncryption#initialize and reference it from the Client and AutoEncrypter option docs.
comandeo-mongo
requested changes
Oct 27, 2026
comandeo-mongo
left a comment
comandeo-mongo
left a comment
Contributor
There was a problem hiding this comment.
Many tests are red
The KMS HTTP proxy specs expect createDataKey to succeed against the real AWS KMS endpoint, but they passed kms_tls_options with ssl_ca_cert set to the test CA. The driver then verified the real KMS certificate against a CA that cannot have issued it, and every case failed with "certificate verify failed (unable to get local issuer certificate)". TLS to KMS is negotiated end-to-end with the real KMS host, so drop kms_tls_options and let verification use the system trust store, matching custom_endpoint_spec and on_demand_aws_credentials_spec. The test CA only applies to the local mock KMS servers, which cannot serve these cases.
…llback spec The new kms_connect_callback example built an AutoEncrypter without extra_options, so its Handle fell back to libmongocrypt's $SYSTEM crypt_shared search. Every other FLE Handle in the process loads crypt_shared via the explicit MONGO_RUBY_DRIVER_CRYPT_SHARED_LIB_PATH override, so the $SYSTEM search could not resolve to that same resident library and mongocrypt_init raised "An existing crypt_shared library is loaded by the application". Pass extra_options like the rest of the file so all Handles use the same load mechanism.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical timeout-handling gaps across TLS setup, I/O, and retries allow KMS requests to exceed operation deadlines or hang indefinitely.
7 open issues
What changed in this PR
Adds kms_connect_callback to the MongoDB Ruby driver so automatic and explicit encryption can route KMS connections through HTTP proxies while retaining TLS verification against the KMS endpoint.
Changes:
- Exposes and forwards the callback through both encryption APIs.
- Adds TLS support for pre-connected sockets and propagates operation timeouts to KMS I/O, correcting milliseconds-to-seconds conversion.
- Adds proxy helpers, integration coverage, socket cleanup tests, and usage documentation.
Suggested label: feature.
| File | Description |
|---|---|
spec/support/kms_http_proxy.rb |
Adds proxy lifecycle, CONNECT, and TLS bridge helpers. |
spec/spec_helper.rb |
Loads the proxy helper. |
spec/mongo/socket/ssl_spec.rb |
Tests pre-connected sockets and failure cleanup. |
spec/mongo/crypt/encryption_io_spec.rb |
Tests callbacks, validation, and KMS I/O timeouts. |
spec/mongo/crypt/auto_encrypter_spec.rb |
Tests automatic-encryption callback forwarding. |
spec/mongo/client_encryption_spec.rb |
Tests explicit-encryption callback forwarding. |
spec/integration/client_side_encryption/kms_http_proxy_spec.rb |
Adds AWS-backed proxy integration scenarios. |
lib/mongo/socket/ssl.rb |
Supports TLS over supplied sockets and improves cleanup. |
lib/mongo/crypt/explicit_encrypter.rb |
Passes the callback to encryption I/O. |
lib/mongo/crypt/encryption_io.rb |
Invokes callbacks and applies KMS timeout conversion. |
lib/mongo/crypt/context.rb |
Forwards operation timeouts to KMS requests. |
lib/mongo/crypt/auto_encrypter.rb |
Documents and forwards the callback option. |
lib/mongo/client.rb |
Documents the automatic-encryption callback option. |
lib/mongo/client_encryption.rb |
Exposes the callback and documents proxy usage. |
.evergreen/run-tests.sh |
Adds comments explaining spec-managed proxy processes. |
Review effort: Balanced
The non-callback path in with_ssl_socket goes through Address#socket, which always defaults connect_timeout to Server::CONNECT_TIMEOUT. The callback path only set connect_timeout when CSOT was in effect, so with no operation budget Socket::SSL#connect! fell through to a bare blocking connect with no deadline. Give the callback path the same default: the remaining budget under CSOT, otherwise Server::CONNECT_TIMEOUT.
The KMS loop snapshotted the remaining operation budget once per state-machine iteration and reused it for every KMS context, every in-place retry, and the libmongocrypt backoff. Later attempts could therefore run with a stale positive timeout well past the operation deadline. Thread the CsotTimeoutHolder into the loop and recompute the remaining budget before each request, raising Error::TimeoutError once the budget is spent. Clamp the backoff sleep to the remaining budget so it cannot overshoot the deadline.
feed_kms restarted the full operation budget for the TLS handshake, for the request write, and for every read chunk, so a slow, chunked KMS response could run for roughly (chunks + 2) times the budget. Compute a single monotonic deadline for the exchange and give each phase only the time left before it, raising Error::TimeoutError once it has passed. The SOCKET_TIMEOUT fallback is kept per phase when no operation budget is set.
CsotTimeoutHolder#remaining_timeout_ms truncates the remaining budget, so in the final sub-millisecond before the deadline it returned 0. check_timeout! only fires at or past the deadline, so that 0 was reachable, and 0 is the driver-wide sentinel for "unlimited": Timeout.timeout(0) enforces nothing and Socket::SSL#connect! treats a zero connect_timeout as unbounded, so a KMS request at the deadline could block indefinitely. Floor a finite remaining budget to 1 ms. This fixes the whole family of consumers at the single accessor they all read from, not just the KMS path.
The kms_connect_callback runs before any socket is constructed, so it could not inherit the connect timeout the socket gets, and nothing bounded it: timeout_ms was passed in but only the callback could honor it. A callback that blocked in TCPSocket.new or a CONNECT read hung the encryption operation regardless of the operation deadline. Wrap the call in Timeout.timeout, using the same budget the socket gets: the remaining CSOT budget when one is in effect, otherwise the driver's connect timeout. The raised timeout propagates through the existing rescue as a retryable KmsError, the same path the TLS write and read timeouts take. Extract that budget into a connect_budget helper so the callback and the TLS handshake share one value, and document that the callback should still honor timeout_ms itself so it can fail cleanly and close its socket.
create_data_key drives the proxy on the same port, so connect_count was already non-zero before the auto-encrypted client was built. The assertion therefore passed even if automatic encryption never invoked the callback. Reset the counter after creating the data key so it measures only auto-encryption traffic.
KmsHttpProxy.start spawned the proxy and then waited for it to become ready. If that wait raised, start never returned the pid, so the spec's after(:all) hook had nothing to stop and the child kept holding the fixed port (9004/9005) for later runs. Stop and reap the child before re-raising.
The kms_connect_callback example read only the status line with a single gets, leaving the remaining headers buffered. The TLS handshake then read those plaintext bytes and failed. Consume the full header block up to the blank line and check the status, matching the test helper. Also honor timeout_ms in the example, as the surrounding docs ask callbacks to do.
…dshake The connect callback and the TLS handshake that follows it each got the full connect budget, so together they could run for twice the operation deadline. Compute a single connect deadline and give the handshake only the time the callback left, raising the timeout error if the callback consumed the whole budget.
A CSOT deadline expiry during the KMS TLS write or read raised Error::TimeoutError, which with_ssl_socket wrapped as a retryable network KmsError, so feed_kms ran an extra backoff and retry cycle before the deadline surfaced. Wrap timeouts without the network_error flag so they surface immediately instead of being retried.
The callback result check accepted anything responding to to_io, but Ruby's OpenSSL requires an fd-backed IO and rejects everything else with an obscure TypeError deep inside SSLSocket.new. Require a real IO so an SSLSocket or other non-IO return fails with the intended KmsError.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
:kms_connect_callbackoption to auto-encryption andMongo::ClientEncryption, so CSFLE/QE users behind an HTTP proxy can route KMS requests through it. The callback gets the KMS host, port, and the remaining timeout, and hands back a socket already connected to that host. The driver wraps that in TLS to the KMS host itself, so SNI and certificate verification still target the real endpoint rather than the proxy.Socket::SSLcan now handshake over a socket it didn't create, and the CSOT timeout now reaches the KMS socket. That last part also exposed a milliseconds-vs-seconds bug that would have made CSOT KMS I/O effectively unbounded, so that's fixed here too.The six prose tests from the spec need AWS credentials, so they only really run in Evergreen under the
test-fletask; locally they skip cleanly. Note: KMS socket operations are now bounded by the remaining operation budget instead of a flat 10 seconds.