Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 0 additions & 2 deletions .evergreen/config.in.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Copy link
Member Author

Choose a reason for hiding this comment

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

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

export SSL_KEY_FILE="${SSL_KEY_FILE}"
export MONGODB_URI="${MONGODB_URI}"

bash ${PROJECT_DIRECTORY}/.evergreen/run-tls-tests.sh
Expand Down
2 changes: 0 additions & 2 deletions .evergreen/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,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}"
export SSL_KEY_FILE="${SSL_KEY_FILE}"
export MONGODB_URI="${MONGODB_URI}"
bash ${PROJECT_DIRECTORY}/.evergreen/run-tls-tests.sh
Expand Down
5 changes: 3 additions & 2 deletions .evergreen/run-tls-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

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

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

export TLS_CA_FILE="$DRIVERS_TOOLS/.evergreen/x509gen/ca.pem"
export TLS_CRL_FILE="$DRIVERS_TOOLS/.evergreen/x509gen/crl.pem"

npm run check:tls
3 changes: 3 additions & 0 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,9 @@ export const OPTIONS = {
tlsCAFile: {
type: 'string'
},
tlsCRLFile: {
type: 'string'
},
tlsCertificateKeyFile: {
type: 'string'
},
Expand Down
9 changes: 7 additions & 2 deletions src/mongo_client.ts
Copy link
Contributor

@W-A-James W-A-James Aug 22, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC
tlsCertificateKeyFilePassword?: string;
/** Specifies the location of a local .pem file that contains the root certificate chain from the Certificate Authority. This file is used to validate the certificate presented by the mongod/mongos instance. */
tlsCAFile?: string;
/** Specifies the location of a local CRL .pem file that contains the client revokation list. */
tlsCRLFile?: string;
/** Bypasses validation of the certificates presented by the mongod/mongos instance */
tlsAllowInvalidCertificates?: boolean;
/** Disables hostname validation of the certificate presented by the mongod/mongos instance. */
Expand Down Expand Up @@ -437,6 +439,9 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
if (typeof options.tlsCAFile === 'string') {
options.ca ??= await fs.readFile(options.tlsCAFile);
}
if (typeof options.tlsCRLFile === 'string') {
options.crl ??= await fs.readFile(options.tlsCRLFile);
}
if (typeof options.tlsCertificateKeyFile === 'string') {
if (!options.key || !options.cert) {
const contents = await fs.readFile(options.tlsCertificateKeyFile);
Expand Down Expand Up @@ -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` |
Copy link
Member Author

Choose a reason for hiding this comment

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

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

* | `cert` | `tlsCertificateKeyFile` | `string` |
* | `key` | `tlsCertificateKeyFile` | `string` |
* | `passphrase` | `tlsCertificateKeyFilePassword` | `string` |
Expand All @@ -814,8 +819,8 @@ export interface MongoOptions
* `cert` and `key` fields will be undefined.
*/
tls: boolean;

tlsCAFile?: string;
tlsCRLFile?: string;
tlsCertificateKeyFile?: string;

/** @internal */
Expand Down
99 changes: 73 additions & 26 deletions test/manual/tls_support.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,19 @@ import {
MongoServerSelectionError
} from '../mongodb';

const REQUIRED_ENV = ['MONGODB_URI', 'SSL_KEY_FILE', 'SSL_CA_FILE'];
const REQUIRED_ENV = ['MONGODB_URI', 'TLS_KEY_FILE', 'TLS_CA_FILE', 'TLS_CRL_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`);
throw new Error(`skipping TLS 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 TLS_CERT_KEY_FILE = process.env.TLS_KEY_FILE as string;
const TLS_CA_FILE = process.env.TLS_CA_FILE as string;
const TLS_CRL_FILE = process.env.TLS_CRL_FILE as string;
const tlsSettings = {
tls: true,
tlsCertificateKeyFile: TLS_CERT_KEY_FILE,
Expand Down Expand Up @@ -47,36 +48,59 @@ describe('TLS Support', function () {
});

context('when tls filepaths have length > 0', () => {
beforeEach(async () => {
client = new MongoClient(CONNECTION_STRING, tlsSettings);
});
context('when connection will succeed', () => {
beforeEach(async () => {
client = new MongoClient(CONNECTION_STRING, tlsSettings);
});

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');
expect(client.options).not.have.property('cert');

await client.connect();

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

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');
expect(client.options).not.have.property('cert');
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();

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

expect(client.options).property('ca').to.exist;
expect(client.options).property('key').to.exist;
expect(client.options).property('cert').to.exist;
await client.connect();
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

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);
Comment on lines +81 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

});
});
});

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();
context('when the connection will fail', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

beforeEach(async () => {
client = new MongoClient(CONNECTION_STRING, {
tls: true,
tlsCRLFile: TLS_CRL_FILE,
serverSelectionTimeoutMS: 5000,
connectTimeoutMS: 5000
});
});

const caFileAccessTime = (await fs.stat(TLS_CA_FILE)).atime;
const certKeyFileAccessTime = (await fs.stat(TLS_CERT_KEY_FILE)).atime;
it('should read in files async at connect time', async () => {
expect(client.options).property('tlsCRLFile', TLS_CRL_FILE);
expect(client.options).not.have.property('crl');

await client.connect();
const err = await client.connect().catch(e => e);

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);
expect(err).to.be.instanceof(Error);
expect(client.options).property('crl').to.exist;
});
});
});
Expand Down Expand Up @@ -114,6 +138,29 @@ describe('TLS Support', function () {
});
});

context('when providing tlsCRLFile', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

context('when the file will revoke the certificate', () => {
let client: MongoClient;
beforeEach(() => {
client = new MongoClient(CONNECTION_STRING, {
tls: true,
tlsCAFile: TLS_CA_FILE,
tlsCRLFile: TLS_CRL_FILE,
serverSelectionTimeoutMS: 5000,
connectTimeoutMS: 5000
});
});
afterEach(async () => {
await client?.close();
});

it('throws a MongoServerSelectionError', async () => {
const err = await client.connect().catch(e => e);
expect(err).to.be.instanceOf(MongoServerSelectionError);
});
});
});

context('when tlsCertificateKeyFile is provided, but tlsCAFile is missing', () => {
let client: MongoClient;
beforeEach(() => {
Expand Down
7 changes: 7 additions & 0 deletions test/unit/connection_string.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,13 @@ describe('Connection String', function () {
});
});

context('when providing tlsCRLFile', function () {
it('sets the tlsCRLFile option', function () {
const options = parseOptions('mongodb://localhost/?tls=true&tlsCRLFile=path/to/crl.pem');
expect(options.tlsCRLFile).to.equal('path/to/crl.pem');
});
});

context('when both tls and ssl options are provided', function () {
context('when the options are provided in the URI', function () {
context('when the options are equal', function () {
Expand Down
2 changes: 2 additions & 0 deletions test/unit/mongo_client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ describe('MongoOptions', function () {
const options = parseOptions('mongodb://localhost:27017/?ssl=true', {
tlsCertificateKeyFile: filename,
tlsCAFile: filename,
tlsCRLFile: filename,
tlsCertificateKeyFilePassword: 'tlsCertificateKeyFilePassword'
});
fs.unlinkSync(filename);
Expand All @@ -61,6 +62,7 @@ describe('MongoOptions', function () {
expect(options).to.not.have.property('cert');
expect(options).to.have.property('tlsCertificateKeyFile', filename);
expect(options).to.have.property('tlsCAFile', filename);
expect(options).to.have.property('tlsCRLFile', filename);
expect(options).has.property('passphrase', 'tlsCertificateKeyFilePassword');
expect(options).has.property('tls', true);
});
Expand Down