-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
@@ -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}" |
There was a problem hiding this comment.
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.
@@ -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" |
There was a problem hiding this comment.
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".
@@ -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` | |
There was a problem hiding this comment.
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.
@@ -114,6 +115,29 @@ describe('TLS Support', function () { | |||
}); | |||
}); | |||
|
|||
context('when providing tlsCRLFile', () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parse error in test/manual/tls_support.test.ts
but otherwise LGTM
Fixed. |
it('should only read files once', async () => { | ||
await client.connect(); | ||
await client.close(); | ||
context('when the connection will fail', () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Description
Adds the ability to provide a CRL file as an option in the connection string or client options.
What is changing?
tlsCRLFile
, that takes a string as a path to the crl.pem.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 optiontlsCRLFile
has been added and can be provided in either the connection string or as aMongoClient
option.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript