Skip to content

CDRIVER-5998 import persisted private keys for SChannel #2059

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

Merged
merged 24 commits into from
Jul 23, 2025

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented Jul 14, 2025

Summary

Import private keys as persisted (not ephemeral) for Secure Channel.

Background & Motivation

This PR is intended to address an error sending a client certificate to servers that do not support the SHA1 client certificate signature:

The client and server cannot communicate, because they do not possess a common algorithm.

See comment for details. In particular, this prevents MONGODB-X509 auth (which authenticates with a client certificate).

Importing a persisted key appears required to support the SHA2 signature (and TLS 1.3, but that is planned in CDRIVER-6045). Experiments and references suggest persisting keys is relevant:

Quoting .NET runtime:

on Windows we do not support ephemeral keys.

Quoting Private Key Lifetime on Windows and the January 2021 .NET Framework Security and Quality Rollup:

The default behavior is that the private key gets loaded into a persisted (named) key via one of the system cryptography libraries

Quoting https://stackoverflow.com/a/72101855:

The TLS layer on Windows requires that the private key be written to disk

Quoting PFXImportCertStore:

This typically applies during a TLS handshake: if the handshake is performed externally to the calling process in LSASS.exe, it is not possible to use PKCS12_NO_PERSIST_KEY when moving from the calling process to LSASS

The MongoDB server currently persists keys in CAPI and does not attempt to delete.

Use of CAPI and CNG

Some deprecated calls in CryptoAPI (CAPI) are migrated to Cryptography API: Next Generation (CNG). Quoting About CNG:

Cryptography API: Next Generation (CNG) is the long-term replacement for the CryptoAPI

PKCS#8 keys now use CNG (NCryptImportKey). PKCS#1 keys still use CAPI (CryptImportKey) since CNG APIs do not appear to support PKCS#1. Atlas gives PKCS#8 certificates.

Key names

Keys are imported with a deterministic name. The imported key name has the form libmongoc-<SHA256 thumbprint>-<pkcs1|pkcs8>. The <SHA256 thumbprint> was chosen to be easily computable by a user (via openssl x509 command). The <pkcs1|pkcs8> suffix is to distinguish the same key imported via PKCS#1 and PKCS#8 files (will end up in different providers). Since the key name is chosen by the C driver, I want to document the key name so users can identify and delete if desired. If merged, I plan to request this documentation be added.

Rejected Alternative: Import with GUID

Importing the key with GUID key name was considered. Each client/pool would import with a unique GUID and delete in the destructor. This appears to match the .NET runtime behavior, but I am concerned that a C application is more likely to abort early:

const char* uri = "mongodb://localhost:27017/?tlsCertificateKeyFile=client.pem";
mongoc_client_t *client = mongoc_client_new(uri); // imports key with GUID name.
assert (returns_false()); // exits and does not delete imported key!
mongoc_client_destroy (client);

This may quickly lead to many orphaned imported keys if a process leaks a client/pool repeatedly. Imported keys appear to take 1-3kb of storage in the User private directories.

To fix error sending client cert to servers requiring newer signatures. Sending a client cert is required for MONGODB-X509.

Fixes observed error sending client cert to servers not accepting SHA1 for client cert signature:
> 0x80090331: The client and server cannot communicate, because they do not possess a common algorithm.
@kevinAlbs kevinAlbs marked this pull request as ready for review July 14, 2025 17:59
@kevinAlbs kevinAlbs requested a review from a team as a code owner July 14, 2025 17:59
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Recommend also updating connect_with_secure_channel_cred to improve its handling of pre-existing credentials (e.g. due to prior test failure):

FAIL:src\libmongoc\tests\test-mongoc-secure-channel.c:58  test_secure_channel_shared_creds_stream()
  stream
  Failed to initialize security context: (0x80092010) The certificate is revoked.

test-libmongoc : test_secure_channel_shared_creds_stream src\libmongoc\tests\test-mongoc-secure-channel.c:58

Copy link
Collaborator Author

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Recommend also updating connect_with_secure_channel_cred

I expect the error was due to a leftover imported CRL file. That can be deleted using the hash of the crl.pem:

certutil -delstore Root c2eb43936568e4dc59e26780d81fb84afac8f0c7
Root "Trusted Root Certification Authorities"
Deleting CRL 0: C=US, S=New York, L=New York City, O=MongoDB, OU=Drivers, CN=Drivers Testing CA:c2eb43936568e4dc59e26780d81fb84afac8f0c7
CertUtil: -delstore command completed successfully.

Related: re-running the /X509/crl test failed due to an log message change caused by #2052:

C:\code\mongo-c-driver\src\libmongoc\tests\test-mongoc-x509.c:439 test_crl(): testing tls didn't log "Mutual Authentication failed"

The test has been updated.

@kevinAlbs kevinAlbs requested a review from eramongodb July 17, 2025 14:05
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Minor suggestion; otherwise, LGTM.

Comment on lines 140 to 147
// Test whitespace is an error:
ASSERT (!hex_to_bin (" 66", &len));

// Test non-even number of digits is an error:
ASSERT (!hex_to_bin ("666", &len));

uint32_t len;
uint8_t *got = hex_to_bin (hexstr, &len);
ASSERT_CMPSTR ((const char *) got, expect);
ASSERT_CMPUINT32 (len, ==, 7);
bson_free (got);
// Test non-hex digits is an error:
ASSERT (!hex_to_bin ("ZZ", &len));
Copy link
Contributor

Choose a reason for hiding this comment

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

   ASSERT_WITH_MSG (!hex_to_bin ("  66", &len), "whitespace is an error");
   ASSERT_WITH_MSG (!hex_to_bin ("666", &len), "non-even number of digits is an error");
   ASSERT_WITH_MSG (!hex_to_bin ("ZZ", &len), "non-hex digits is an error");

Move reason into assertion message.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

size_t key_name_wide_chars = strlen (key_name) + 1;
key_name_wide = bson_malloc (sizeof (WCHAR) * (key_name_wide_chars));
BSON_ASSERT (mlib_in_range (int, key_name_wide_chars));
if (0 == MultiByteToWideChar (
Copy link
Contributor

Choose a reason for hiding this comment

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

MultiByteToWideChar (and other Win32 string conversion APIs) is a foot bazooka, and I get nervous every time I see it. In this case, it's probably safe, but will possibly allocate more WCHAR than necessary.

We're only using it in a few places, but it may be good to refactor it into an internal API that "does the right thing" every time rather than needing to do the math and audit it every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored to an internal utf8_to_wide function which gets the required size first to avoid over-allocating.

I left the calls to MultiByteToWideChar in _mongoc_cluster_sspi_new unchanged. I was concerned about changing untested code. SSPI is only used for GSSAPI auth, which appears only tested in run-auth-tests.sh and currently skipped (pending CDRIVER-5995). Though the new utf8_to_wide function could be shared and reused in the future.

Comment on lines 254 to 255
char *
bin_to_hex (const uint8_t *bin, uint32_t len);
bin_to_hex_uppercase (const uint8_t *bin, size_t bin_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to having separate uppercase/lowercase functions rather than using the lowercase_inplace (or uppercase_inplace) at the call-sites?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought: removed hex_to_bin_lowercase in favor of consistently producing uppercase. The hex_to_bin_lowercase was only used for test output.

Co-authored-by: vector-of-bool <[email protected]>
The lowercase variant was only used for test output. Use uppercase consistently.
@kevinAlbs kevinAlbs merged commit 912209d into mongodb:master Jul 23, 2025
38 of 40 checks passed
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.

3 participants