feat(NODE-5566): add ability to provide CRL file via tlsCRLFile #3834

durran · 2023-08-22T13:08:54Z

Description

Adds the ability to provide a CRL file as an option in the connection string or client options.

What is changing?

Adds a new option, tlsCRLFile, that takes a string as a path to the crl.pem.
Refactors a bit of TLS testing variable names.

Is there new documentation needed for these changes?

Yes, TLS options should be updated.

What is the motivation for this change?

NODE-5549/NODE-5566

Release Highlight

Users may now provide a CRL file name as a client option.

For users wanting to provide a TLS client revokation list as a filename and not a Buffer, a new option tlsCRLFile has been added and can be provided in either the connection string or as a MongoClient option.

new MongoClient('mongodb://localhost:27017/db?tls=true&tlsCRLFile=path/to/crl.pem'); // option in connection string.
new MongoClient('mongodb://localhost:27017/db', { tls: true, tlsCRLFile: 'path/to/crl.pem' }) // option in client options.

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
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

durran · 2023-08-22T13:44:12Z

.evergreen/config.in.yml

@@ -636,8 +636,6 @@ functions:
          export PROJECT_DIRECTORY="$(pwd)"
          export NODE_LTS_VERSION=${NODE_LTS_VERSION}
          export DRIVERS_TOOLS="${DRIVERS_TOOLS}"
-          export SSL_CA_FILE="${SSL_CA_FILE}"


These are actually not defined but set up in run-tls-tests.sh so I removed them from here.

durran · 2023-08-22T13:44:39Z

.evergreen/run-tls-tests.sh

@@ -4,7 +4,8 @@ set -o errexit  # Exit the script with error if any of the commands fail

 source "${PROJECT_DIRECTORY}/.evergreen/init-node-and-npm-env.sh"

-export SSL_KEY_FILE="$DRIVERS_TOOLS/.evergreen/x509gen/client.pem"
-export SSL_CA_FILE="$DRIVERS_TOOLS/.evergreen/x509gen/ca.pem"
+export TLS_KEY_FILE="$DRIVERS_TOOLS/.evergreen/x509gen/client.pem"


Just renamed while here to align with us preferring "tls" over "ssl".

durran · 2023-08-22T13:45:19Z

src/mongo_client.ts

@@ -790,7 +795,7 @@ export interface MongoOptions
   * | nodejs native option  | driver spec equivalent option name            | driver option type |
   * |:----------------------|:----------------------------------------------|:-------------------|
   * | `ca`                  | `tlsCAFile`                                   | `string`           |
-   * | `crl`                 | N/A                                           | `string`           |
+   * | `crl`                 | `tlsCRLFile`                                  | `string`           |


It's not really a spec option but didn't want to create another column in the table just for the one option.

durran · 2023-08-22T13:46:20Z

test/manual/tls_support.test.ts

@@ -114,6 +115,29 @@ describe('TLS Support', function () {
    });
  });

+  context('when providing tlsCRLFile', () => {


We don't have a CRL in drivers-tools that wouldn't reject the CA we have, so just have an integration test for the failure. Looked at the Python driver and they do the same.

W-A-James · 2023-08-22T17:25:07Z

test/manual/tls_support.test.ts

Can we add an assertion to the

when client has been opened and closed more than once should only read files once

test that checks that the the crl file is read exactly once?

Relevant block should be at line 61

I've updated but needed to split them out for when the connection would succeed and fail.

Good on the test restructuring but I more so meant that we want to guarantee that no matter how many times we call connect, we only read in each of these files once. I added another comment directly on the assertions I'm making reference to.

Yes I just caught that and added the other test as well.

W-A-James · 2023-08-22T17:47:51Z

src/mongo_client.ts

Can you update the Note on tlsCAFile and tlsCertificateKeyFile to also mention that tlsCRLFile also has the same behaviour.

W-A-James · 2023-08-22T17:55:13Z

test/manual/tls_support.test.ts

+            expect((await fs.stat(TLS_CA_FILE)).atime).to.deep.equal(caFileAccessTime);
+            expect((await fs.stat(TLS_CERT_KEY_FILE)).atime).to.deep.equal(certKeyFileAccessTime);


Can we add assertions similar to these for the CRL file?

W-A-James · 2023-08-22T17:56:35Z

test/manual/tls_support.test.ts

Good on the test restructuring but I more so meant that we want to guarantee that no matter how many times we call connect, we only read in each of these files once. I added another comment directly on the assertions I'm making reference to.

W-A-James

Parse error in test/manual/tls_support.test.ts but otherwise LGTM

durran · 2023-08-22T18:40:26Z

Parse error in test/manual/tls_support.test.ts but otherwise LGTM

Fixed.

baileympearson · 2023-08-22T19:38:46Z

test/manual/tls_support.test.ts

-        it('should only read files once', async () => {
-          await client.connect();
-          await client.close();
+      context('when the connection will fail', () => {


What do these tests mean? I interpret them as "when the TLS settings are misconfigured" (resulting in the call to connect to fail. And the first test does demonstrate this.

But what about the second? I'd expect the call to connect on line 109 to throw, if the client were misconfigured, and that the test would fail. How is it passing?

The connection will fail here because the CRL will cause a revokation. The second was failing - I just fixed that test.

test/manual/tls_support.test.ts

durran force-pushed the NODE-5549 branch from c1a3924 to 46cfca3 Compare August 22, 2023 13:11

feat(NODE-5549): add ability to provide crl file

ca1efd8

durran force-pushed the NODE-5549 branch from 46cfca3 to ca1efd8 Compare August 22, 2023 13:17

durran commented Aug 22, 2023

View reviewed changes

durran marked this pull request as ready for review August 22, 2023 13:47

durran changed the title ~~feat(NODE-5549): add ability to provide crl file~~ feat(NODE-5566): add ability to provide crl file Aug 22, 2023

nbbeeken self-requested a review August 22, 2023 17:06

nbbeeken self-assigned this Aug 22, 2023

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 22, 2023

nbbeeken previously approved these changes Aug 22, 2023

View reviewed changes

nbbeeken changed the title ~~feat(NODE-5566): add ability to provide crl file~~ feat(NODE-5566): add ability to provide CRL file via tlsCRLFile Aug 22, 2023

nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 22, 2023

W-A-James self-requested a review August 22, 2023 17:18

W-A-James requested changes Aug 22, 2023

View reviewed changes

test: check when crl loaded

b25822a

durran dismissed nbbeeken’s stale review via b25822a August 22, 2023 17:36

durran requested a review from W-A-James August 22, 2023 17:36

W-A-James requested changes Aug 22, 2023

View reviewed changes

durran added 3 commits August 22, 2023 20:03

test: check crl access time

b9d71ec

chore: update comments

1e4755e

Merge branch 'main' into NODE-5549

3634429

durran requested a review from W-A-James August 22, 2023 18:06

W-A-James reviewed Aug 22, 2023

View reviewed changes

fix: parse error

9699d91

durran requested a review from W-A-James August 22, 2023 18:40

nbbeeken previously approved these changes Aug 22, 2023

View reviewed changes

W-A-James previously approved these changes Aug 22, 2023

View reviewed changes

test: fix test

9448075

durran dismissed stale reviews from W-A-James and nbbeeken via 9448075 August 22, 2023 19:25

baileympearson requested changes Aug 22, 2023

View reviewed changes

durran requested a review from baileympearson August 22, 2023 19:56

nbbeeken requested changes Aug 22, 2023

View reviewed changes

test/manual/tls_support.test.ts Show resolved Hide resolved

durran added 2 commits August 22, 2023 22:03

test: fix test

29b1791

test: lower timeout

b5a9386

durran requested a review from nbbeeken August 22, 2023 20:39

nbbeeken approved these changes Aug 22, 2023

View reviewed changes

baileympearson approved these changes Aug 22, 2023

View reviewed changes

nbbeeken merged commit 33c86c9 into main Aug 22, 2023
22 of 26 checks passed

nbbeeken deleted the NODE-5549 branch August 22, 2023 22:09

github-actions bot mentioned this pull request Aug 22, 2023

chore(main): release 6.0.0 [skip-ci] #3762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-5566): add ability to provide CRL file via tlsCRLFile #3834

feat(NODE-5566): add ability to provide CRL file via tlsCRLFile #3834

durran commented Aug 22, 2023 •

edited

durran Aug 22, 2023

durran Aug 22, 2023

durran Aug 22, 2023

durran Aug 22, 2023

W-A-James Aug 22, 2023

durran Aug 22, 2023

W-A-James Aug 22, 2023

durran Aug 22, 2023

W-A-James Aug 22, 2023 •

edited

durran Aug 22, 2023

W-A-James Aug 22, 2023

durran Aug 22, 2023

W-A-James Aug 22, 2023

W-A-James left a comment

durran commented Aug 22, 2023

baileympearson Aug 22, 2023

durran Aug 22, 2023

		expect((await fs.stat(TLS_CA_FILE)).atime).to.deep.equal(caFileAccessTime);
		expect((await fs.stat(TLS_CERT_KEY_FILE)).atime).to.deep.equal(certKeyFileAccessTime);

feat(NODE-5566): add ability to provide CRL file via tlsCRLFile #3834

feat(NODE-5566): add ability to provide CRL file via tlsCRLFile #3834

Conversation

durran commented Aug 22, 2023 • edited

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Users may now provide a CRL file name as a client option.

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W-A-James Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W-A-James left a comment

Choose a reason for hiding this comment

durran commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durran commented Aug 22, 2023 •

edited

W-A-James Aug 22, 2023 •

edited