test(NODE-4227): add explicit encryption prose tests #3297

durran · 2022-06-23T18:47:40Z

Description

Adds explicit encryption prose tests

What is changing?

Adds 5 new prose tests for #12 in the client side encryption spec. https://github.com/mongodb/specifications/blob/7f37c9325b423223edf54d795afed444e6989b98/source/client-side-encryption/tests/README.rst#explicit-encryption

I've also included the spec prose test language in code comments in the test here for easier following.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

NODE-4227

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

durran · 2022-06-23T20:55:08Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

@@ -40,6 +40,14 @@ const metadata = {
  }
 };

+const eeMetadata = {


#12 prose test section for explicit encryption requires server 6.0 and higher and not standalone.

durran · 2022-06-23T20:56:06Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

+    beforeEach(async function () {
+      const mongodbClientEncryption = this.configuration.mongodbClientEncryption;
+      // Load the file encryptedFields.json as encryptedFields.
+      encryptedFields = EJSON.parse(


Data for the encryptedFields option and key document are now provided in 2 new files in the etc/ directory in the csfle spec directory.

durran · 2022-06-23T21:01:04Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

+          // Assert 10 documents are returned. Assert each returned document contains the
+          // field { "encryptedIndexed": "encrypted indexed value" }.
+          const collection = encryptedClient.db('db').collection('explicit_encryption');
+          const result = await collection.find({ encryptedIndexed: findPayload2 }).toArray();


Just pointing out I find this absolutely crazy. A value is directly encrypted on line 1603 with a contention factor equal to the amount of documents in the collection that would match the original value of the field. But each field's encrypted value would not match the encrypted value passed to the find in theory as all the bin data would be different for each document on insert. But it still matches all of them. Mind blown.

durran · 2022-06-23T21:02:23Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

+      });
+    });
+
+    context('Case 4: can roundtrip encrypted indexed', eeMetadata, function () {


These last 2 are just encrypt/decrypt roundtrips. So pretty straightforward.

durran · 2022-06-23T21:33:53Z

test/integration/shared.js

-function dropCollection(dbObj, collectionName) {
-  return dbObj.dropCollection(collectionName).catch(ignoreNsNotFound);
+function dropCollection(dbObj, collectionName, options = {}) {
+  return dbObj.dropCollection(collectionName, options).catch(ignoreNsNotFound);


Just adding the ability to pass options to the dropCollection helper. If we pass encryptedFields here an extra 3 collections will get dropped. (The esc/ecc/ecoc collections)

addaleax

Do you want to drop these tests from the libmongocrypt repo? https://github.com/mongodb/libmongocrypt/blob/93b82421dfe9ad472ef12b26a47aa94e631f9be1/bindings/node/test/clientEncryption.test.js#L584-L797

Fwiw, I really like that the Node.js driver deviates from other drivers and instead maintains a lot more functionality in the libmongocrypt repo (i.e. the full AutoEncrypter and ClientEncryption classes, rather than just raw bindings).

durran · 2022-06-24T16:31:31Z

Do you want to drop these tests from the libmongocrypt repo? https://github.com/mongodb/libmongocrypt/blob/93b82421dfe9ad472ef12b26a47aa94e631f9be1/bindings/node/test/clientEncryption.test.js#L584-L797

Fwiw, I really like that the Node.js driver deviates from other drivers and instead maintains a lot more functionality in the libmongocrypt repo (i.e. the full AutoEncrypter and ClientEncryption classes, rather than just raw bindings).

I think it's fine to keep them there - can't have too many tests. :)

dariakp · 2022-06-24T19:32:39Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

+  requires: {
+    clientSideEncryption: true,
+    mongodb: '>=6.0.0',
+    topology: ['replicaset', 'sharded']


what about load balanced? is that considered more like a standalone?

We don't run the other fle tests against load balanced clusters so I was keeping in line with that. The spec in general pre-dates the load balancer spec and does not mention that topology at all, and enabling it got some strange setup errors that I felt were outside the scope of the ticket. I can create a ticket to look into running all the fle tests with load balancers as well.

I think we should check with the spec owners regarding this, and maybe file a drivers ticket to add lb to the testing matrix if it makes sense to do so, then the node ticket will automatically come down to us

Just for context, the reason for requiring these topologies is that QE requires transactions, which in turn requires a replset topology. I guess technically one could run a load balancer in front of a single standalone node, but I assume that won’t happen in practice or in tests?

Just as a follow up, other drivers are not running prose tests against a load balancer. However, the LB spec says it should run all tests in unified format so we'll need to make sure the new FLE unified tests are running against a load balanced topology.

test/integration/client-side-encryption/client_side_encryption.prose.test.js

durran force-pushed the NODE-4227 branch 2 times, most recently from a9e1a3f to afd2724 Compare June 23, 2022 19:16

durran added 3 commits June 23, 2022 22:04

test(NODE-4227): add explicit encryption prose tests

52dec16

test(NODE-4227): bypass query analysis in fle opts

306eace

test(NODE-4227): reorder prose tests

d6e5e44

durran force-pushed the NODE-4227 branch 2 times, most recently from 00a9c16 to 29e3f35 Compare June 23, 2022 20:43

durran marked this pull request as ready for review June 23, 2022 20:43

test(NODE-4227): fix tests

c9d468c

durran force-pushed the NODE-4227 branch from 29e3f35 to c9d468c Compare June 23, 2022 20:45

durran commented Jun 23, 2022

View reviewed changes

test(NODE-4227): use csfle alpha 3

5789257

durran commented Jun 23, 2022

View reviewed changes

addaleax reviewed Jun 24, 2022

View reviewed changes

dariakp self-assigned this Jun 24, 2022

dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 24, 2022

dariakp requested changes Jun 24, 2022

View reviewed changes

test(NODE-4227): update per suggestions

c526461

durran requested a review from dariakp June 27, 2022 13:20

chore(NODE-4227): fix lint errors

3cf6751

dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 27, 2022

dariakp approved these changes Jun 27, 2022

View reviewed changes

nbbeeken approved these changes Jun 29, 2022

View reviewed changes

dariakp merged commit fb2b4a5 into main Jun 29, 2022

dariakp deleted the NODE-4227 branch June 29, 2022 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(NODE-4227): add explicit encryption prose tests #3297

test(NODE-4227): add explicit encryption prose tests #3297

Uh oh!

durran commented Jun 23, 2022 •

edited

Loading

Uh oh!

durran Jun 23, 2022

Uh oh!

durran Jun 23, 2022

Uh oh!

durran Jun 23, 2022 •

edited

Loading

Uh oh!

durran Jun 23, 2022

Uh oh!

durran Jun 23, 2022

Uh oh!

addaleax left a comment

Uh oh!

durran commented Jun 24, 2022

Uh oh!

dariakp Jun 24, 2022

Uh oh!

durran Jun 27, 2022

Uh oh!

dariakp Jun 27, 2022

Uh oh!

addaleax Jun 29, 2022

Uh oh!

durran Jun 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

test(NODE-4227): add explicit encryption prose tests #3297

test(NODE-4227): add explicit encryption prose tests #3297

Uh oh!

Conversation

durran commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

durran Jun 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

durran commented Jun 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

durran commented Jun 23, 2022 •

edited

Loading

durran Jun 23, 2022 •

edited

Loading