CSHARP-4944: Enable use of native crypto in libmongocrypt bindings #796
Conversation
This file contains 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
BorisDog
requested changes
Apr 30, 2024
| } | ||
| catch (LibraryLoader.FunctionNotFoundException) | ||
| { | ||
| // mongocrypt_is_crypto_available is only available in libmongocrypt version >= 1.9 |
There was a problem hiding this comment.
Is this better than checking the Library.Version ?
In the case when the version is right, and mongocrypt_is_crypto_available throws, that might point to other problems, and then it's better to fail?
There was a problem hiding this comment.
Good point! Done
| @@ -186,10 +168,18 @@ private class LinuxLibrary : ISharedLibraryLoader | |||
| // #define RTLD_GLOBAL 0x100 | |||
| public const int RTLD_GLOBAL = 0x100; | |||
| public const int RTLD_NOW = 0x2; | |||
|
|
|||
| private static readonly string[] _suffixPaths = | |||
BorisDog
requested changes
May 2, 2024
| @@ -42,14 +42,15 @@ public static CryptClient Create(CryptOptions options) | |||
| MongoCryptSafeHandle handle = null; | |||
| Status status = null; | |||
|
|
|||
| // mongocrypt_is_crypto_available is only available in libmongocrypt version >= 1.9 | |||
| var cryptoAvailable = Version.Parse(Library.Version) >= Version.Parse("1.9") && Library.mongocrypt_is_crypto_available(); | |||
There was a problem hiding this comment.
minor: let's safe Version.Parse("1.9") as static readonly var.
bindings/cs/README.md
Outdated
| @@ -54,6 +54,9 @@ If you see `Windows Error: 126` during tests, like the example below, it means t | |||
| 2. cd <build>/bindings/cs | |||
| 3. dotnet build cs.build | |||
| ``` | |||
| *Note*: You can use the ```LIBMONGOCRYPT_PATH``` environment variable to load a locally installed | |||
| libmongocrypt build. You should specify the absolute path to the libmongocrypt build itself not just the containing folder. For example on Linux: | |||
There was a problem hiding this comment.
Suggested change
| libmongocrypt build. You should specify the absolute path to the libmongocrypt build itself not just the containing folder. For example on Linux: | |
| libmongocrypt build. You should specify the absolute path to the libmongocrypt library itself, not just the containing folder. For example on Linux: |
| @@ -150,9 +124,17 @@ private class DarwinLibraryLoader : ISharedLibraryLoader | |||
| public const int RTLD_GLOBAL = 0x8; | |||
| public const int RTLD_NOW = 0x2; | |||
|
|
|||
| private static readonly string[] _s_suffixPaths = | |||
There was a problem hiding this comment.
I see that we are missing conventions for static vars, in libmongocrypt. Let's go with the usual ___s_suffixPaths convention here.
BorisDog
requested changes
May 7, 2024
| @@ -34,6 +34,9 @@ public class CryptClientFactory | |||
| private static Library.Delegates.RandomCallback __randomCallback = new Library.Delegates.RandomCallback(SecureRandomCallback.GenerateRandom); | |||
| private static Library.Delegates.CryptoHmacCallback __signRsaesPkcs1HmacCallback = new Library.Delegates.CryptoHmacCallback(SigningRSAESPKCSCallback.RsaSign); | |||
|
|
|||
| // mongocrypt_is_crypto_available is only available in libmongocrypt version >= 1.9 | |||
| private static readonly Version __s_libVersionWithMongocryptIsCryptoAvailable = Version.Parse("1.9"); | |||
There was a problem hiding this comment.
Consider:
private static readonly Version __mongocryptIsCryptoAvailableMinVersion = Version.Parse("1.9");
bindings/cs/README.md
Outdated
| @@ -54,6 +54,9 @@ If you see `Windows Error: 126` during tests, like the example below, it means t | |||
| 2. cd <build>/bindings/cs | |||
| 3. dotnet build cs.build | |||
| ``` | |||
| *Note*: You can use the ```LIBMONGOCRYPT_PATH``` environment variable to load a locally installed | |||
| libmongocrypt build. You should specify the absolute path to the libmongocrypt library itself, not just the containing folder. For example on Linux: | |||
There was a problem hiding this comment.
I think this comment got lost:
#796 (comment)
| @@ -150,9 +124,17 @@ private class DarwinLibraryLoader : ISharedLibraryLoader | |||
| public const int RTLD_GLOBAL = 0x8; | |||
| public const int RTLD_NOW = 0x2; | |||
|
|
|||
| private static readonly string[] __s_suffixPaths = | |||
There was a problem hiding this comment.
I am sorry for confusion, I wanted to write __suffixPaths instead ___s_suffixPaths
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.
Description
Use libmongocrypt native crypto when available.
What's changing
LIBMONGOCRYPT_PATHto make it available to the driver. IfLIBMONGOCRYPT_PATHis not set then we default to using the packaged crypto-disabled libmongocrypt.Performance Results
Using crypto-disabled libmongocrypt:
Using crypto-enabled libmongocrypt: