feat(NODE-3924)!: read tls files async

[W-A-James]

Description

What is changing?

connection string options

• updated tlsCAFile and tlsCertificateKeyFile options to store the file names and defer reading to first invocation of MongoClient.connect
• We now throw at option-parse time when provided an empty string for tlsCAFile or tlsCertificateKeyFile

MongoClient

• Added logic to MongoClient.constructor and MongoClient.connect to store options.tlsCAFile and options.tlsCertificateKeyFile on construction and read them into options.ca and options.key respectively on connect.

TLS support test

• refactored to typescript
• Added test that ensures that tls files are read in exactly once
• Added test that ensures that tls files are read at connect time
• Added test that ensures that connection fails when connecting with empty tls filepaths

TLS support test evergreen task and mocha config

• Updated mocha config to timeout in 10s instead of 2s
• Updated run tls tests evergreen task to timeout in 10s

API docs

• Updated API documentation to mention how and when files specified by paths in tlsCertificateKeyFile and tlsCAFile are read

Is there new documentation needed for these changes?

Yes, docs ticket has been generated.

What is the motivation for this change?

NODE-3924

Release Highlight

TLS certificate authority and certificate-key files are now read asynchronously

In this release, we have updated the MongoClient to store the file names provided to the existing tlsCAFile and tlsCertificateKeyFile options and only read these files the first time it connects. Prior to this change, the files were read synchronously on MongoClient construction.

Note: This has no effect on driver functionality when TLS configuration files are properly specified. However, if there are any issues with the TLS configuration files (invalid file name), the error is now thrown when the MongoClient is connected instead of at construction time.

const client = new MongoClient(CONNECTION_STRING, {
		tls: true,
		tlsCAFile: 'caFileName',
		tlsCertificateKeyFile: 'certKeyFile'
}); // Files are not read here, but file names are stored on the MongoClient

await client.connect(); // Files are now read and their contents stored
await client.close();

await client.connect(); // Since the file contents have already been cached, the files will not be read again.

Take a look at our TLS documentation for more information on the tlsCAFile and tlsCertificateKeyFile 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

[W-A-James]

Can change this to not redirect to caFileName

[baileympearson]

This shouldn't be changed, right?

[W-A-James]

I may have misinterpreted what this table is for. I though the leftmost column corresponded to the option as it exists in node driver and the second column was the spec compliant option name. Is it actually the case that the leftmost column corresponds to node's internal tls options?

[baileympearson]

I have two related cleanups here (these apply to tlsCertificateKeyFile too)

first, we can use the nullish coelescing assignment operator here to avoid checking !options.ca:

      if (typeof options.tlsCAFile === 'string' && options.tlsCAFile.length > 0) {
        options.ca ??= await fs.readFile(options.tlsCAFile, { encoding: 'utf8' });
      }

---

second, I'm not sure we should be checking options.tlsCAFile.length (even though the AC says to). the user must manually provide the filepath, and if we receive an empty string, the user's configuration is likely messed up. We should probably alert the user that we received an empty file path instead of silently ignoring it. Attempting to read from a file named "" will throw an error, so that was the old behavior.

If we do decide to ignore empty strings, I suggest we move the logic into the options parser. The requirement for these options would actually be "a non-empty string", instead of just "a string", so that should be codified in the options parsing and tested there.

[W-A-James]

Your suggestion to handle empty file names in option parsing is more user friendly as far as catching bad config is concerned, so that's the solution I'd prefer as well.

[baileympearson]

Daria wrote the ticket and included "ignore empty filenames" as an AC. Can you ask her why that was included?

[nbbeeken]

IIRC we wanted to have a nicer experience for environment variables being the source of the file name which can be set to an empty string as a way of clearing the value. I do not feel strongly, if we do ignore the empty string wouldn't TLS be misconfigured and fail? Is there a scenario where a server with TLS configured allows a client without TLS to connect? Also happy with keeping the behavior as it was (validate string and not length)

[baileympearson]

Well, in both cases it fails to connect, but if we throw ourselves it's a much more actionable error than a generic connection error.

Did we have a specific environment / usecase / testing variant we wanted this for?

[addaleax]

Is there a scenario where a server with TLS configured allows a client without TLS to connect?

Not sure what impact that has here, but sure, the server’s TLS mode can be disabled, allowTLS, preferTLS, requireTLS, and two of these are modes where the server accepts connections with or without TLS.

[nbbeeken]

One comment about how we should error. And there's slack thread about aligning on "empty" vs "unset".