CDRIVER-4272: Add options and code to load the csfle dynamic library #968

vector-of-bool · 2022-04-15T20:16:34Z

This is an initial basic implementation of the csfle options from the new CSE spec changes to support the dynamic library. This also adds mlib as a support library, which now requires some C99 support.

TODO: A a testing scenario that injects the csfle library. Ensure tests do not load a csfle from the system, only from the isolated test copy or relies on mongocrypd.

kevinAlbs

The new options and code to load csfle LGTM.

If mlib is only used for csfle_override_path, I suggest leaving it out for now. It may be better to add mlib as a separate PR to expedite unblocking other drivers testing with csfle.

src/libmongoc/src/mongoc/mongoc-topology-private.h

…processes if possible

…ions. This is discovered because the mongocryptd spawner will cause the atexit() code to run from the csfle library loaded in the process.

kevinAlbs

Looking good overall.

The mongodl.py script looks robust and generally useful.

Great catch with the exit and allocation unsafety within the child process.

build/mongodl.py

src/libmongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mongoc-client-side-encryption.c

src/libmongoc/CMakeLists.txt

…-encryption Refer: [DRIVERS-2287]

vector-of-bool · 2022-04-22T19:33:29Z

@kevinAlbs I've found that I need to add an option for a test to completely disable csfle as the test will rely on mongocryptd behaviors. Take a look at the latest few commits.

.gitignore

src/libmongoc/CMakeLists.txt

src/libmongoc/tests/test-mongoc-client-side-encryption.c

src/libmongoc/src/mongoc/mongoc-crypt.c

src/libmongoc/src/mongoc/mongoc-client-side-encryption.c

src/libmongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mongoc-client-side-encryption.c

eramongodb · 2022-04-22T20:11:39Z

src/libmongoc/tests/json/client_side_encryption/explain.json

@@ -123,6 +123,9 @@
        "autoEncryptOpts": {
          "kmsProviders": {
            "aws": {}
+          },
+          "extra": {
+            "__csfleDisabled": true


Confirming, has the spec test this test file corresponds to been updated in the spec repo? If not, this test might not belong under the json directory.

True. This is somewhat experimenting on how to "disable" csfle for testing. We don't want to publish an "official" way to disable csfle, but this spec test also changed behavior depending on whether csfle is present. It's a catch-22.

I think it is fine to include the field in spec tests so long as it is clearly documented as for testing purposes only. However, if the specification repo will not be updated to match proposed test file contents, I would like the __csfleDisabled test to be moved out of the json directory and tested as a prose test instead.

This test failing may be a bug in csfle. csfle and mongocryptd should have the same output(slack thread for reference).

src/libmongoc/tests/test-mongoc-client-side-encryption.c

eramongodb

Other than the issue with __csfleDisabled in a modified unified spec test, LGTM.

eramongodb · 2022-04-22T21:28:27Z

src/libmongoc/tests/json/client_side_encryption/explain.json

@@ -123,6 +123,9 @@
        "autoEncryptOpts": {
          "kmsProviders": {
            "aws": {}
+          },
+          "extra": {
+            "__csfleDisabled": true


I think it is fine to include the field in spec tests so long as it is clearly documented as for testing purposes only. However, if the specification repo will not be updated to match proposed test file contents, I would like the __csfleDisabled test to be moved out of the json directory and tested as a prose test instead.

kevinAlbs

I left a suggestion to skip tests based on test environment rather than a __csfleDisabled option.

kevinAlbs · 2022-04-22T22:13:36Z

src/libmongoc/tests/json/client_side_encryption/explain.json

@@ -123,6 +123,9 @@
        "autoEncryptOpts": {
          "kmsProviders": {
            "aws": {}
+          },
+          "extra": {
+            "__csfleDisabled": true


This test failing may be a bug in csfle. csfle and mongocryptd should have the same output(slack thread for reference).

kevinAlbs · 2022-04-25T18:33:08Z

src/libmongoc/tests/test-mongoc-client-side-encryption.c

+                     "]",
+                     "mongocryptdURI",
+                     "mongodb://localhost:27021/?serverSelectionTimeoutMS=1000",
+                     "__csfleDisabled",


All drivers will need a solution to skip this prose test when running with csfle. I prefer not adding a hidden option to the API. I think it adds small risk that users rely on it.

I suggest only run this test if the test runner expects to use csfle. That could be passed through an environment variable. There is precedence: (e.g. MONGOC_TEST_IS_SERVERLESS, MONGOC_TEST_LOADBALANCED.

kevinAlbs

Latest changes and tests look good. Skipping Windows is not ideal, but seems low risk given that it has been validated locally, and libmongocrypt tests with the stub library in CI.

vector-of-bool added 8 commits April 5, 2022 00:42

Add mlib to the source tree

ae134d1

No -Winline

70cbd72

Tweak mlib usage in CMake

bd6501d

Incorporate changes for csfle in libmongocrypt

e6363ba

Enable C99 as the default language mode for the project

cddf5c7

Tweak macro name

33827b6

Test case to check linker symbol merging

44ff2e8

Missing dist listing

4099263

vector-of-bool requested a review from kevinAlbs April 15, 2022 20:16

vector-of-bool added 2 commits April 18, 2022 19:26

Merge branch 'master' into adopt-csfle

cbdc11a

test-libmongoc might not be created

21c5581

kevinAlbs reviewed Apr 18, 2022

View reviewed changes

src/libmongoc/src/mongoc/mongoc-topology-private.h Show resolved Hide resolved

vector-of-bool added 3 commits April 18, 2022 22:23

MinGW lacks WC_ERR_INVALID_CHARS

cbfdfca

Document new extraOptions for auto-encryption

834308f

Remove mlib for now

a334c4e

vector-of-bool force-pushed the adopt-csfle branch from 5f9d09a to a334c4e Compare April 19, 2022 00:47

vector-of-bool added 2 commits April 19, 2022 21:24

Add mongodl, download a csfle library for testing. Use csfle in test …

17687a7

…processes if possible

Fix: forked child processes must not used exit() nor allocating funct…

2566d33

…ions. This is discovered because the mongocryptd spawner will cause the atexit() code to run from the csfle library loaded in the process.

vector-of-bool requested review from kevinAlbs and eramongodb April 20, 2022 20:05

C99 is not required

8328de4

vector-of-bool marked this pull request as ready for review April 20, 2022 20:21

mlib is not part of the dist any more

c966020

kevinAlbs reviewed Apr 20, 2022

View reviewed changes

vector-of-bool added 6 commits April 20, 2022 23:13

Fix platform detection in mongodl

46fde37

Tweak filter output for --list

3a20ed6

Various spelling

6c6b01e

Only download csfle if we are building tests

3b7bb90

mongodl: Default edition is 'enterprise'

58fc76a

Do not append csfle search paths if we are explicit or bypassing auto…

864e513

…-encryption Refer: [DRIVERS-2287]

vector-of-bool added 7 commits April 22, 2022 01:20

Fix mongodl for rhel versions and ASCII stdout

c6a78b1

Prefer find_program() for finding Python3

addb083

Fix Ubuntu detection. Small formatting

aac7994

Missing dist for mongodl

40a2612

Using an $ORIGIN based path for the csfle library

ccc2547

Disable csfle for tests that rely on mongocryptd behaviors

c82bdb1

Speeling

57e53b7

vector-of-bool requested review from kevinAlbs and eramongodb April 22, 2022 19:33

eramongodb reviewed Apr 22, 2022

View reviewed changes

vector-of-bool added 4 commits April 22, 2022 20:37

Indentation fix

966d303

Clearer assertion error message

ec9ec9a

Use POSIX _exit instead of wrapping it

606b1a5

Formatting

37cb34d

eramongodb approved these changes Apr 22, 2022

View reviewed changes

csfle placement is difficult

aa5180b

kevinAlbs reviewed Apr 25, 2022

View reviewed changes

vector-of-bool added 8 commits April 26, 2022 11:16

Just put it in the top directory for now

3c8ccfa

Just use 'find'

45a9bf2

Better find, better output path

c497d22

Remove BSON_MEMCHECK. We're getting ODR trouble.

eda94f6

Tell me what you did.

7654848

No csflePath on Windows yet

5e29834

Remove __csfleDisabled option

281df33

Skip failing CSE tests for now

c7d54b4

kevinAlbs approved these changes Apr 27, 2022

View reviewed changes

vector-of-bool merged commit d9fa2a3 into mongodb:master Apr 27, 2022

vector-of-bool mentioned this pull request Apr 28, 2022

Fixes for csfle downloading on some Linux platforms #977

Merged

kevinAlbs mentioned this pull request Nov 12, 2024

Remove skip for loading crypt_shared on Windows #1787

Merged

CDRIVER-4272: Add options and code to load the csfle dynamic library #968

CDRIVER-4272: Add options and code to load the csfle dynamic library #968

Uh oh!

Conversation

vector-of-bool commented Apr 15, 2022

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vector-of-bool commented Apr 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

vector-of-bool Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

eramongodb Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

eramongodb Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eramongodb Apr 22, 2022 •

edited

Loading

eramongodb Apr 22, 2022 •

edited

Loading