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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Aug 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ca1efd8
feat(NODE-5549): add ability to provide crl file
durran Aug 22, 2023
b25822a
test: check when crl loaded
durran Aug 22, 2023
b9d71ec
test: check crl access time
durran Aug 22, 2023
1e4755e
chore: update comments
durran Aug 22, 2023
3634429
Merge branch 'main' into NODE-5549
durran Aug 22, 2023
9699d91
fix: parse error
durran Aug 22, 2023
9448075
test: fix test
durran Aug 22, 2023
29b1791
test: fix test
durran Aug 22, 2023
b5a9386
test: lower timeout
durran Aug 22, 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
2 changes: 0 additions & 2 deletions .evergreen/config.in.yml
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Copy link
Contributor

@W-A-James W-A-James Aug 22, 2023 •
edited
Loading

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
32 changes: 28 additions & 4 deletions test/manual/tls_support.test.ts
View file
Open in desktop
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 @@ -114,6 +115,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
View file
Open in desktop
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
View file
Open in desktop
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