Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for customizing how KMS sockets are established for CSFLE/Queryable Encryption by introducing a kmsConnectCallback API (enabling HTTP CONNECT proxy tunneling) and wiring it through the CSFLE state machine, with accompanying unit + prose integration coverage.
Changes:
- Introduces
KMSConnectCallback(and related TLS/socket option types) in a newkms_optionsmodule and exports them from the public index. - Wires
kmsConnectCallbackthroughAutoEncrypter/ClientEncryptioninto theStateMachineKMS request path (including CSOT-aware behavior) and enforces mutual exclusivity withproxyOptions. - Adds unit tests and a new prose integration test (Prose test 28) validating callback invocation, proxy tunneling behavior, and timeout/error propagation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/client-side-encryption/state_machine.test.ts | Adds unit coverage for CSOT behavior and new kmsConnectCallback socket establishment + timeout semantics. |
| test/unit/client-side-encryption/client_encryption.test.ts | Adds unit coverage for proxyOptions vs kmsConnectCallback conflict validation in ClientEncryption. |
| test/unit/client-side-encryption/auto_encrypter.test.ts | Adds unit coverage for proxyOptions vs kmsConnectCallback conflict validation in AutoEncrypter. |
| test/mongodb_all.ts | Re-exports new kms_options types for tests. |
| test/integration/client-side-encryption/client_side_encryption.prose.28.kms_connect_callback.test.ts | Adds prose integration coverage for KMS connect callback through HTTP/HTTPS proxy. |
| src/index.ts | Publicly exports kms_options types and removes those type exports from state_machine. |
| src/client-side-encryption/state_machine.ts | Implements kmsConnectCallback support and revises CSOT timeout handling for KMS requests. |
| src/client-side-encryption/kms_options.ts | New module defining public KMS TLS/socket option types and KMSConnectCallback contract. |
| src/client-side-encryption/client_encryption.ts | Stores/validates kmsConnectCallback and passes it into StateMachine. |
| src/client-side-encryption/auto_encrypter.ts | Stores/validates kmsConnectCallback and passes it into StateMachine. |
…k is going to be used
nbbeeken
left a comment
There was a problem hiding this comment.
apologies for the piecemeal review, noticed this on the second read. LGTM either way
There was a problem hiding this comment.
Note that this PR must wait for mongodb/specifications#1956 to merge before it can be fully reviewed and merged. (Will resolve this comment once this PR is unblocked).
PavelSafronov
left a comment
There was a problem hiding this comment.
New changes look good, approving and moving to team review.
| const timeoutMS = Number.isFinite(remainingTimeMS) ? remainingTimeMS : undefined; | ||
| const controller = new AbortController(); | ||
| // Forward the operation's own abort signal so aborting the operation also aborts the callback. | ||
| const forwardAbort = addAbortListener(options?.signal, function () { |
There was a problem hiding this comment.
Should we have a guard here to protect against already-aborted signals? E.g: something like:
options?.signal?.throwIfAborted() before this.
There was a problem hiding this comment.
Yes, great catch. I updated all paths to respect already expired timeout (test coverage added as well).
| if (this.options.proxyOptions && this.options.proxyOptions.proxyHost) { | ||
| if (this.options.kmsConnectCallback) { | ||
| const remainingTimeMS = options?.timeoutContext?.csotEnabled() | ||
| ? options.timeoutContext.getRemainingTimeMSOrThrow('KMS request timed out') |
There was a problem hiding this comment.
Nit: Can we include the timeoutMS in the throw reason? The default message includes it, so technically this is less informative.
| const timeout = timeoutMS ? Timeout.expires(timeoutMS) : undefined; | ||
| try { | ||
| socketOptions.socket = timeout | ||
| ? await Promise.race([connectPromise, timeout]) |
There was a problem hiding this comment.
Idea: I'm noticing here we're exclusively relying on the user to unblock (either with a supplied CSOT timeout or by resolving from kmsConnectCallback()). The timeout here can be very long (or infinite, with a timeoutMS of 0). This makes me worry about open handles (http requests) dangling beyond the lifetime of parent objects (e.g: a client closure). Probably a separate PR, but I think it's worth considering.
There was a problem hiding this comment.
Great call, I will request spec owners to update spec, I guess the legacy timeout might fit here just fine.
There was a problem hiding this comment.
This is an existing gap, feels like we can address it here or separately, just need to know which.
There was a problem hiding this comment.
Spec owner confirms the gap. In our current implementation we are 100% current spec-compliant and as @johnmtll suggested I would postpone the work until the future update of spec.
Description
Summary of Changes
Adds
kmsConnectCallbacktoAutoEncryptionOptionsandClientEncryptionOptions. When provided, the driver invokes the callback to establish the TCP connection to a KMS host instead of opening the socket itself, then wraps the returned socket with TLS using the KMS provider's configured TLS options. The primary use case is routing KMS requests through an HTTP proxy via the HTTP CONNECT method, which the existing SOCKS5proxyOptionsdoes not cover.Spec: mongodb/specifications#1956
Release Highlight
HTTP proxy support for KMS requests in CSFLE and Queryable Encryption
In-use encryption can now route KMS requests through an HTTP proxy. Set
kmsConnectCallbackon yourClientEncryptionor auto-encryption options to control how the driver connects to a KMS host. The callback receives the targethostandportand returns a connected socket (for example, a tunnelopened with HTTP CONNECT); the driver then performs the KMS TLS handshake over that socket using the provider's configured TLS options. This unblocks CSFLE and Queryable Encryption in environments that require an HTTP forward proxy for outbound KMS traffic, which the existing SOCKS5
proxyOptionsdoes not cover.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript