-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-5998 share SCHANNEL_CRED
#2052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CDRIVER-5998 share SCHANNEL_CRED
#2052
Conversation
31c13b6
to
bcfdcca
Compare
Share `SCHANNEL_CRED` on all client/pool connections. This is intended to simplify future fix for CDRIVER-5998. As an added benefit, this reduces the repeated reading of PEM files for each connection.
Capture logs earlier to account for secure channel loading the PEM file when SSL options are set. This revealed the OpenSSL implementation also logs.
bcfdcca
to
61f17ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with only minor comments
|
||
*error = (bson_error_t) {0}; | ||
|
||
if (!_mongoc_host_list_from_string_with_err (&host, "localhost:27017", error)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This line will break with #2047 because it changes the signature of this function. Whichever is merged first will require changing the other.
} | ||
|
||
// Expect exactly one attempt to load the client cert: | ||
ASSERT_CMPSIZE_T (1, ==, cf.failures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use the type-generic mlib_check
macros
ASSERT_CMPSIZE_T (1, ==, cf.failures); | |
mlib_check (1, eq, cf.failures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor feedback remaining; otherwise, LGTM.
mongoc_stream_t *stream; | ||
mongoc_stream_tls_secure_channel_t *schannel_stream_view; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mongoc_stream_t *stream; | |
mongoc_stream_tls_secure_channel_t *schannel_stream_view; |
Stray changes?
mongoc_stream_t *stream; | ||
mongoc_stream_tls_secure_channel_t *schannel_stream_view; | ||
bson_error_t error; | ||
mongoc_ssl_opt_t ssl_opt = {.ca_file = CERT_TEST_DIR "/ca.pem", .pem_file = CERT_TEST_DIR "/client.pem"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mongoc_ssl_opt_t ssl_opt = {.ca_file = CERT_TEST_DIR "/ca.pem", .pem_file = CERT_TEST_DIR "/client.pem"}; | |
const mongoc_ssl_opt_t ssl_opt = {.ca_file = CERT_TEST_DIR "/ca.pem", .pem_file = CERT_TEST_DIR "/client.pem"}; |
I believe ssl_opt
can be made const
. It is used by:
connect_with_secure_channel_cred
(OK: no modification)mongoc_stream_tls_secure_channel_new_with_creds
(OK: no modification)mongoc_secure_channel_cred_new
(OK: already const)
mongoc_secure_channel_cred_new
(OK: already const)
This reveals the opt
param of mongoc_stream_tls_secure_channel_new
can also be made const
, even though it is invoked by mongoc_stream_tls_new_with_hostname
with a non-const opt
(after modification).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. Updated to use more const
.
Summary
Share
SCHANNEL_CRED
across connections.Verified with this patch: https://spruce.mongodb.com/version/686298d51b4881000729652b/
Background & Motivation
Sharing the
SCHANNEL_CRED
is motivated by an upcoming fix for CDRIVER-5998. The private key for a client certificate is currently imported as an ephemeral key stored in-process (via call to CryptImportKey). Supporting SHA2 signatures appears to require importing a persisted private key stored on the file-system (with a unique name). This will be described further in a follow-up PR. But to avoid importing many duplicate keys, I want to change the current behavior from "import key per connection" to "import key per client/pool and share".Though improving performance was not the main goal of this PR, it reduces file-reading from per-connection to per-client/per-pool. Sharing the credentials showed an improvement in a one-off benchmark creating 100 connections in 10 threads:
Sharing
SCHANNEL_CRED
follows a similar pattern to the shared OpenSSL context implemented in #1673.Microsoft documentation does not appear to document thread-safety of sharing
SCHANNEL_CRED
. However, mongod shares the sameSCHANNEL_CRED
for client connections (via _clientCred). I expect the TLS handshake only needs to readSCHANNEL_CRED
.SCHANNEL_CRED
is documented as deprecated. This is not addressed in this PR, but I expect can be replaced when adding support for TLS v1.3 (CDRIVER-6045).Shared pointer
A
mongoc_shared_ptr
is used to manageSCHANNEL_CRED
. This is to support changing the SSL options after creating connections (may have a use case for rotating certificates?). This behavior is tested in the OpenSSL implementation:mongoc_client_pool_t
however documents:Therefore, the updates to the
mongoc_shared_ptr
are assumed to not require thread-safety.