Skip to content

DRIVERS-2920 CSFLE/QE Support for HTTP Proxies#1956

Open
mdb-ad wants to merge 24 commits into
mongodb:masterfrom
mdb-ad:http-proxy
Open

DRIVERS-2920 CSFLE/QE Support for HTTP Proxies#1956
mdb-ad wants to merge 24 commits into
mongodb:masterfrom
mdb-ad:http-proxy

Conversation

@mdb-ad

@mdb-ad mdb-ad commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds kmsConnectCallback to AutoEncryptionOpts and ClientEncryptionOpts, enabling drivers to override how TCP connections to KMS hosts are established. The primary use case is routing KMS traffic through an HTTP proxy via HTTPS CONNECT.

Spec changes

Added kmsConnectCallback: Optional<Callback> to both opts types. The callback receives (host, port) and returns a socket- or stream-like object (exact type is up to implementers). Drivers that provide an alternative proxy mechanism MAY omit the callback.

Prose tests

Added prose test 28, KMS Connect Callback:
Case 1: createDataKey() via a plain HTTP proxy; asserts proxy received a CONNECT.
Case 2: Same via an HTTPS proxy (proxy connection itself is TLS).
Case 3: Full auto encryption pipeline — creates a data key, inserts an auto-encrypted document, finds and decrypts it, verifies the raw document is encrypted, and asserts the proxy was used throughout.

The tests use kms_http_proxy.py as the local proxy server and run on AWS.

C Driver implementation

mongodb/mongo-c-driver#2318

@mdb-ad
mdb-ad marked this pull request as ready for review June 25, 2026 20:19
@mdb-ad
mdb-ad requested a review from a team as a code owner June 25, 2026 20:19
Comment thread source/client-side-encryption/client-side-encryption.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md
Comment thread source/client-side-encryption/client-side-encryption.md
@mdb-ad
mdb-ad requested a review from kevinAlbs July 2, 2026 07:11
kevinAlbs
kevinAlbs previously approved these changes Jul 2, 2026

@kevinAlbs kevinAlbs 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.

LGTM with some additional formatting suggestions.

Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/client-side-encryption.md
Comment thread source/client-side-encryption/tests/README.md Outdated
Co-authored-by: Kevin Albertson <kevin.eric.albertson@gmail.com>
Comment thread source/client-side-encryption/client-side-encryption.md
Comment thread source/client-side-encryption/tests/README.md Outdated
@mdb-ad
mdb-ad requested a review from adelinowona July 10, 2026 17:36
Comment thread source/client-side-encryption/client-side-encryption.md Outdated

@adelinowona adelinowona 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.

Reviewed against a C# reference implementation (CSHARP-6005): modeled kmsConnectCallback as a two-method interface (Connect/ConnectAsync) due to C#'s sync/async I/O convention and ran prose test 28 cases 1–4 in Evergreen against the real kms_http_proxy.py + AWS KMS. The design holds end-to-end — the "driver TLS-wraps the returned socket" model dropped cleanly onto our existing TLS layer. Two things surfaced that I think are worth addressing in the spec, plus one note.

1. The spec doesn't say whether a callback failure is terminal or retryable, and that collides with the test assertions.

Drivers already have a KMS-request retry path (in C#, the send loop catches IOException/SocketException and calls mongocrypt_kms_ctx_fail() rather than propagating; libmongocrypt then bounds the retries). Once kmsConnectCallback is routed through that loop, a callback that reports a connection failure as a network-type error can be re-invoked (retried) instead of propagated. That interacts with the tests two ways:

  • Case 4 asserts the error propagates — which only holds deterministically if the callback raises a non-network/opaque error. A proxy callback that naturally throws a socket/IO error (the common case) could be swallowed into retry.
  • Cases 1–3 assert connect_count == 1 — a retried transient error re-invokes the callback and inflates the count.

Suggestion: state explicitly whether a callback failure is terminal (propagate immediately, no KMS retry) or participates in the existing retry, and have Case 4 use a non-network error so it tests pure propagation.

2. Be explicit that the driver's TLS — SNI and certificate/hostname verification — targets the KMS host, not the proxy.

Once a proxy is in the path, "the host the socket connected to" (the proxy) and "the host the TLS handshake authenticates" (the KMS host, reached through the CONNECT tunnel) split apart. The prose says the driver "wraps the returned socket with TLS (using the KMS provider's configured TLS options)" but never says whose identity is verified. An implementation that derives the TLS target host from the socket's actual remote endpoint would either fail the handshake or "fix" it by disabling validation — silently insecure. This is clearest in Case 2, where there are two independent TLS layers: client↔proxy (verified with the proxy CA, inside the callback) and client↔KMS (end-to-end through the tunnel, verified against the KMS host, done by the driver). Suggested one-liner: "TLS is negotiated end-to-end with the KMS host; SNI and certificate/hostname verification MUST target the KMS host, not the address the callback connected to."

@mdb-ad

mdb-ad commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@adelinowona thanks for the comments, we didn't realize that the C driver retry mechanism was propagating the error. Changed the test to just require that it fails without any error propagation requirement.

Clarified the TLS verification especially on Case 2 with the HTTPS proxy.

@mdb-ad
mdb-ad requested a review from adelinowona July 21, 2026 05:12
Comment thread source/client-side-encryption/client-side-encryption.md Outdated
Comment thread source/client-side-encryption/tests/README.md

@tadjik1 tadjik1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Node implementaion
mongodb/node-mongodb-native#5007

Left couple of small comments and suggestion in the code.

@mdb-ad
mdb-ad requested review from kevinAlbs and tadjik1 July 24, 2026 17:47

@kevinAlbs kevinAlbs 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.

LGTM with a test tweak.

Comment thread source/client-side-encryption/tests/README.md Outdated
Comment thread source/client-side-encryption/tests/README.md Outdated
@mdb-ad
mdb-ad requested a review from adelinowona July 24, 2026 19:32
Co-authored-by: Kevin Albertson <kevin.eric.albertson@gmail.com>

@tadjik1 tadjik1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! LGTM 👍

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.

4 participants