Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9881228
chore(NODE-3924): update package-lock.json
W-A-James Jul 19, 2023
b7763ae
feat(NODE-3924)!: read tls files async
W-A-James Jul 19, 2023
0fbb2f9
test(NODE-3924): add unit test
W-A-James Jul 19, 2023
28a2ba5
test(NODE-3924): move tests to manual tests
W-A-James Jul 19, 2023
8bc7296
style(NODE-3924): eslint
W-A-James Jul 19, 2023
ddb1e7f
test(NODE-3924): update assertions
W-A-James Jul 19, 2023
32de88b
style(NODE-3924): remove unneeded import
W-A-James Jul 19, 2023
d69dd69
fix(NODE-3924): remove unused option
W-A-James Jul 20, 2023
b418f99
test(NODE-3924): update test name
W-A-James Jul 20, 2023
e571b80
test(NODE-3924): fix test setup and timeouts
W-A-James Jul 21, 2023
84a3a89
test(NODE-3924): update existing unit test
W-A-James Jul 21, 2023
8acfbb6
test(NODE-3924): update table
W-A-James Jul 21, 2023
52bf930
docs(NODE-3924): update tls options API docs
W-A-James Jul 21, 2023
694b4a7
style(NODE-3924): eslint
W-A-James Jul 21, 2023
0dfac88
test(NODE-3924): fix spec test
W-A-James Jul 21, 2023
135aadf
fix(NODE-3924): change option names to shadow spec tls option names
W-A-James Jul 21, 2023
428aa6f
Merge branch 'main' into NODE-3924
W-A-James Jul 21, 2023
1053452
ci(NODE-3924): revert unneeded ci change
W-A-James Jul 24, 2023
1e6d240
fix(NODE-3924): throw error on parse
W-A-James Jul 24, 2023
20d09ea
test(NODE-3924): update tests
W-A-James Jul 24, 2023
702f6f8
docs(NODE-3924): revert table change and modify heading
W-A-James Jul 24, 2023
7438df7
style(NODE-3924): eslint
W-A-James Jul 24, 2023
b1d4e51
fix(NODE-3924): address review comments
W-A-James Jul 26, 2023
253aa6d
fix(NODE-3924): revert unintended change
W-A-James Jul 26, 2023
c454e37
Update test/unit/mongo_client.test.js
W-A-James Jul 26, 2023
5352581
fix(NODE-3924): Change to throw at connect time
W-A-James Jul 28, 2023
a67d3a1
fix(NODE-3924): throw on non-string
W-A-James Jul 31, 2023
af14ffe
test(NODE-3924): add test to throw on non-string values
W-A-James Jul 31, 2023
1550a94
revert validation
W-A-James Jul 31, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .evergreen/config.in.yml
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ functions:
- command: shell.exec
type: test
params:
timeout_secs: 10
working_dir: "src"
script: |
export PROJECT_DIRECTORY="$(pwd)"
Expand Down
1 change: 1 addition & 0 deletions .evergreen/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ functions:
- command: shell.exec
type: test
params:
timeout_secs: 10
working_dir: src
script: |
export PROJECT_DIRECTORY="$(pwd)"
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
"check:oidc-azure": "mocha --config test/mocha_mongodb.json test/integration/auth/mongodb_oidc_azure.prose.test.ts",
"check:ocsp": "mocha --config test/manual/mocharc.json test/manual/ocsp_support.test.js",
"check:kerberos": "nyc mocha --config test/manual/mocharc.json test/manual/kerberos.test.ts",
"check:tls": "mocha --config test/manual/mocharc.json test/manual/tls_support.test.js",
"check:tls": "mocha --config test/manual/mocharc.json test/manual/tls_support.test.ts",
"check:ldap": "nyc mocha --config test/manual/mocharc.json test/manual/ldap.test.js",
"check:socks5": "mocha --config test/manual/mocharc.json test/manual/socks5.test.ts",
"check:csfle": "mocha --config test/mocha_mongodb.json test/integration/client-side-encryption",
Expand Down
11 changes: 2 additions & 9 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import * as dns from 'dns';
import * as fs from 'fs';
import ConnectionString from 'mongodb-connection-string-url';
import { URLSearchParams } from 'url';

Expand Down Expand Up @@ -1097,16 +1096,10 @@ export const OPTIONS = {
}
},
tlsCAFile: {
target: 'ca',
transform({ values: [value] }) {
return fs.readFileSync(String(value), { encoding: 'ascii' });
}
type: 'string'
},
tlsCertificateKeyFile: {
target: 'key',
transform({ values: [value] }) {
return fs.readFileSync(String(value), { encoding: 'ascii' });
}
type: 'string'
},
tlsCertificateKeyFilePassword: {
target: 'passphrase',
Expand Down
24 changes: 24 additions & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { promises as fs } from 'fs';
import type { TcpNetConnectOpts } from 'net';
import type { ConnectionOptions as TLSConnectionOptions, TLSSocketOptions } from 'tls';
import { promisify } from 'util';
Expand Down Expand Up @@ -433,6 +434,18 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {

const options = this[kOptions];

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (
!options.key &&
typeof options.tlsCertificateKeyFile === 'string' &&
options.tlsCertificateKeyFile.length > 0
) {
options.key = await fs.readFile(options.tlsCertificateKeyFile, { encoding: 'utf8' });
}
}
if (typeof options.srvHost === 'string') {
const hosts = await resolveSRVRecord(options);

Expand Down Expand Up @@ -783,9 +796,20 @@ export interface MongoOptions
* If `tlsInsecure` is set to `false`, then it will set the node native options `checkServerIdentity`
* to a no-op and `rejectUnauthorized` to the inverse value of `tlsAllowInvalidCertificates`. If
* `tlsAllowInvalidCertificates` is not set, then `rejectUnauthorized` will be set to `true`.
*
* ### Note on `tlsCAFile` and `tlsCertificateKeyFile`
*
* The files specified by the paths passed in to the `tlsCAFile` and `tlsCertificateKeyFile` fields
* are read lazily on the first call to `MongoClient.connect`. Once these files have been read and
* the `ca` and `key` fields are populated, they will not be read again on subsequent calls to
* `MongoClient.connect`. As a result, until the first call to `MongoClient.connect`, the `ca`
* and `key` fields will be undefined.
*/
tls: boolean;

tlsCAFile?: string;
tlsCertificateKeyFile?: string;

/** @internal */
[featureFlag: symbol]: any;

Expand Down
3 changes: 2 additions & 1 deletion test/manual/mocharc.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"require": "ts-node/register",
"reporter": "test/tools/reporter/mongodb_reporter.js",
"failZero": true,
"color": true
"color": true,
"timeout": 10000
}
44 changes: 0 additions & 44 deletions test/manual/tls_support.test.js

This file was deleted.

103 changes: 103 additions & 0 deletions test/manual/tls_support.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import { expect } from 'chai';
import { promises as fs } from 'fs';

import { LEGACY_HELLO_COMMAND, MongoClient, type MongoClientOptions } from '../mongodb';

const REQUIRED_ENV = ['MONGODB_URI', 'SSL_KEY_FILE', 'SSL_CA_FILE'];

describe('TLS Support', function () {
for (const key of REQUIRED_ENV) {
if (process.env[key] == null) {
throw new Error(`skipping SSL tests, ${key} environment variable is not defined`);
}
}

const CONNECTION_STRING = process.env.MONGODB_URI as string;
const TLS_CERT_KEY_FILE = process.env.SSL_KEY_FILE as string;
const TLS_CA_FILE = process.env.SSL_CA_FILE as string;
const tlsSettings = {
tls: true,
tlsCertificateKeyFile: TLS_CERT_KEY_FILE,
tlsCAFile: TLS_CA_FILE
};

it(
'should connect with tls via client options',
makeConnectionTest(CONNECTION_STRING, tlsSettings)
);

it(
'should connect with tls via url options',
makeConnectionTest(
`${CONNECTION_STRING}?${Object.keys(tlsSettings)
.map(key => `${key}=${tlsSettings[key]}`)
.join('&')}`
)
);

context('when tls filepaths are provided', () => {
let client: MongoClient;
context('when tls filepaths have length > 0', () => {
beforeEach(async () => {
client = new MongoClient(CONNECTION_STRING, tlsSettings);
});

afterEach(async () => {
if (client) await client.close();
});

it('should read in files async at connect time', async () => {
expect(client.options).property('tlsCAFile', TLS_CA_FILE);
expect(client.options).property('tlsCertificateKeyFile', TLS_CERT_KEY_FILE);
expect(client.options).not.have.property('ca');
expect(client.options).not.have.property('key');

await client.connect();

expect(client.options).property('ca').to.exist;
expect(client.options).property('key').to.exist;
});

context('when client has been opened and closed more than once', function () {
it('should only read files once', async () => {
await client.connect();
await client.close();

const caFileAccessTime = (await fs.stat(TLS_CA_FILE)).atime;
const certKeyFileAccessTime = (await fs.stat(TLS_CERT_KEY_FILE)).atime;

await client.connect();

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);
});
});
});
context('when tls filepaths have length == 0', () => {
beforeEach(async () => {
client = new MongoClient(CONNECTION_STRING, {
serverSelectionTimeoutMS: 2000,
tls: true,
tlsCAFile: '',
tlsCertificateKeyFile: ''
});
});

it('ignores file paths and fails to connect', async () => {
const err = await client.connect().catch(e => e);
expect(err).to.be.instanceOf(Error);
});
});
});
});

function makeConnectionTest(connectionString: string, clientOptions?: MongoClientOptions) {
return async function () {
const client = new MongoClient(connectionString, clientOptions);

await client.connect();
await client.db('admin').command({ [LEGACY_HELLO_COMMAND]: 1 });
await client.db('test').collection('test').findOne({});
return await client.close();
};
}
4 changes: 2 additions & 2 deletions test/tools/uri_spec_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,13 +314,13 @@ export function executeUriValidationTest(
.equal(optionValue);
break;
case 'tlsCertificateKeyFile':
expectedProp = 'key';
expectedProp = 'tlsCertificateKeyFile';
expect(options, `${errorMessage} ${optionKey} -> ${expectedProp}`)
.to.have.property(expectedProp)
.equal(optionValue);
break;
case 'tlsCAFile':
expectedProp = 'ca';
expectedProp = 'tlsCAFile';
expect(options, `${errorMessage} ${optionKey} -> ${expectedProp}`)
.to.have.property(expectedProp)
.equal(optionValue);
Expand Down
31 changes: 15 additions & 16 deletions test/unit/mongo_client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,22 @@ describe('MongoOptions', function () {
*
* ### Additional options:
*
* | nodejs native option | driver spec compliant option name | driver option type |
* |:----------------------|:----------------------------------------------|:-------------------|
* | `ca` | `tlsCAFile` | `string` |
* | `crl` | N/A | `string` |
* | `key` | `tlsCertificateKeyFile` | `string` |
* | `passphrase` | `tlsCertificateKeyFilePassword` | `string` |
* | `rejectUnauthorized` | `tlsAllowInvalidCertificates` | `boolean` |
* | `checkServerIdentity` | `tlsAllowInvalidHostnames` | `boolean` |
* | see note below | `tlsInsecure` | `boolean` |
* | nodejs native option | driver spec compliant option name | driver option type |
* |:------------------------|:----------------------------------------------|:-------------------|
* | `tlsCAFile` | `tlsCAFile` | `string` |
* | `crl` | N/A | `string` |
* | `tlsCertificateKeyFile` | `tlsCertificateKeyFile` | `string` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be changed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* | `passphrase` | `tlsCertificateKeyFilePassword` | `string` |
* | `rejectUnauthorized` | `tlsAllowInvalidCertificates` | `boolean` |
* | `checkServerIdentity` | `tlsAllowInvalidHostnames` | `boolean` |
* | see note below | `tlsInsecure` | `boolean` |
*
*/
expect(options).to.not.have.property('tlsCertificateKeyFile');
expect(options).to.not.have.property('tlsCAFile');
expect(options).to.not.have.property('tlsCertificateKeyFilePassword');
expect(options).has.property('ca', '');
expect(options).has.property('key');
expect(options.key).has.length(0);
expect(options).to.not.have.property('key');
expect(options).to.not.have.property('ca');
expect(options).to.have.property('tlsCertificateKeyFile', filename);
expect(options).to.have.property('tlsCAFile', filename);
expect(options).has.property('passphrase', 'tlsCertificateKeyFilePassword');
expect(options).has.property('tls', true);
});
Expand Down Expand Up @@ -394,10 +393,10 @@ describe('MongoOptions', function () {
const optsFromObject = parseOptions('mongodb://localhost/', {
tlsCertificateKeyFile: 'testCertKey.pem'
});
expect(optsFromObject).to.have.property('key', 'cert key');
expect(optsFromObject).to.have.property('tlsCertificateKeyFile', 'testCertKey.pem');

const optsFromUri = parseOptions('mongodb://localhost?tlsCertificateKeyFile=testCertKey.pem');
expect(optsFromUri).to.have.property('key', 'cert key');
expect(optsFromUri).to.have.property('tlsCertificateKeyFile', 'testCertKey.pem');
});
});

Expand Down