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

fix(NODE-5985): throw Nodejs' certificate expired error when TLS fails to connect instead of CERT_HAS_EXPIRED #4014

Merged

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Mar 4, 2024

Description

What is changing?

When enabled, rejectUnauthorized configures Node's TLS API to reject / error when its unable to verify the certificates of the server. This is enabled by default. When the option is enabled and Node is unabled to verify the certificates of the server, an error event is emitted. In our connect logic, this means the connect promise rejects.

Prior to #3973, the code removed in this PR was in the connect handler (called on successful TLS connection). This was dead code because we will never receive an error that is triggered by rejectUnauthorized and successfully connect. When #3973 refactored connect() to be async, we moved this logic into the catch block instead of the try block, so that it did run when an error occurred during TLS connection.

The code throws socket.authorizationError instead of the caught error, which is a string value (https://nodejs.org/api/tls.html#tlssocketauthorizationerror). As a result, instead of throwing the error Nodejs gave us, we throw the authorization error property from the socket.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Release Highlight

Driver 6.3.0 included an internal refactor to the driver's TLS connection logic that introduced logic that intercepted TLS connection errors. In certain situations, the driver would erroneously throw the TLS socket's authorizationError property instead of the error thrown from Nodejs' TLS API.

This was observable in two ways:

  • The error message for TLS connection issues changed. For example, instead of certificate has expired, the driver threw an error with the message CERT_HAS_EXPIRED
  • The error thrown from the driver had a cause property set to a string instead of an error.

The driver now correctly propagates TLS errors.

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

@baileympearson baileympearson marked this pull request as ready for review March 5, 2024 17:31
@nbbeeken nbbeeken self-assigned this Mar 5, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 5, 2024
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Change LGTM, can you make the title and release notes reflect the bug being fixed, which I think is mainly the error message, but also that there is a cause being set to a string (while not broken, highly unexpected)

@baileympearson baileympearson changed the title fix(NODE-5985): remove rejectUnauthorized special handling in connect() fix(NODE-5985): throw Nodejs' certificate expired error when TLS fails to connect instead of CERT_HAS_EXPIRED Mar 5, 2024
@nbbeeken nbbeeken merged commit 057c223 into mongodb:main Mar 5, 2024
18 of 26 checks passed
aditi-khare-mongoDB added a commit that referenced this pull request Mar 8, 2024
synced new test files

added support for error response

added api docs

made MongoServerError.errorResponse required + casted resulting type errors

test(NODE-5992): fix env var restoration in tests (#4017)

refactor(NODE-5903): add newline to stdio logging (#4018)

fix(NODE-5985): throw Nodejs' certificate expired error when TLS fails to connect instead of `CERT_HAS_EXPIRED` (#4014)

test(NODE-5962): gossip cluster time in utr (#4019)

chore(NODE-5997): update saslprep to ^1.1.5 (#4023)

feat(NODE-5968): container and Kubernetes awareness in client metadata (#4005)

fix(NODE-5993): memory leak in the `Connection` class (#4022)

added TODO(NODE-XXXX)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants