fix(devtools-connect): re-try connection in case of TLS errors without system CA MONGOSH-1935

Recent user reports have made it clear that our attempts to solve
TLS errors caused by adding system certificates haven't fully been
resolved yet. To address this, we add a generic fallback that
attempts to connect a second time without the system certificate store
added.

This commit also adds TLS errors to the list of fail-fast errors,
i.e. errors which should result in a quick connection end because they
are unlikely to be resolved by the Node.js driver attempting to
re-connect repeatedly on the level of individual connections.
This should avoid situations in which timeouts make the connection
attempt take twice as long for TLS errors.

Author: addaleax

packages/devtools-connect/src/connect.spec.ts

@@ -356,6 +356,54 @@ describe('devtools connect', function () {
       expect(mClient.connect.getCalls()).to.have.lengthOf(1);
       expect(result.client).to.equal(mClient);
     });
+
+    describe('retryable TLS errors', function () {
+      it('retries TLS errors without system CA integration enabled', async function () {
+        const uri = 'localhost:27017';
+        const mClient = new FakeMongoClient();
+        const mClientType = sinon.stub().returns(mClient);
+        const failure = new (Error as any)('blahblah', {
+          cause: Object.assign(new Error('self-signed cert'), {
+            code: 'SELF_SIGNED_CERT_IN_CHAIN',
+          }),
+        });
+        let earlyFailures = 0;
+        bus.on('devtools-connect:connect-fail-early', () => earlyFailures++);
+
+        mClient.connect = sinon.stub().callsFake(async () => {
+          await new Promise(setImmediate);
+          mClient.emit('serverHeartbeatFailed', {
+            failure,
+            connectionId: uri,
+          });
+          await new Promise(setImmediate);
+          throw failure;
+        });
+        mClient.topology = {
+          description: {
+            servers: new Map([['localhost:27017', {}]]),
+          },
+        };
+
+        try {
+          await connectMongoClient(uri, defaultOpts, bus, mClientType as any);
+          expect.fail('missed exception');
+        } catch (err) {
+          expect(err).to.equal(failure);
+        }
+
+        expect(mClientType.getCalls()).to.have.lengthOf(2);
+        expect(
+          Object.keys(mClientType.getCalls()[0].args[1]).sort()
+        ).to.deep.equal(['allowPartialTrustChain', 'ca', 'lookup']);
+        expect(
+          Object.keys(mClientType.getCalls()[1].args[1]).sort()
+        ).to.deep.equal(['allowPartialTrustChain', 'lookup']);
+        expect((mClient as any).connect.getCalls()).to.have.lengthOf(2);
+
+        expect(earlyFailures).to.equal(2);
+      });
+    });
   });
 
   describe('integration', function () {

packages/devtools-connect/src/connect.ts

@@ -1,6 +1,9 @@
 import type { ConnectLogEmitter } from './index';
 import dns from 'dns';
-import { isFastFailureConnectionError } from './fast-failure-connect';
+import {
+  isFastFailureConnectionError,
+  isPotentialTLSCertificateError,
+} from './fast-failure-connect';
 import type {
   MongoClient,
   MongoClientOptions,
@@ -318,7 +321,7 @@ export class DevtoolsConnectionState {
       'productDocsLink' | 'productName' | 'oidc' | 'parentHandle'
     >,
     logger: ConnectLogEmitter,
-    ca: string
+    ca: string | undefined
   ) {
     this.productName = options.productName;
     if (options.parentHandle) {
@@ -339,7 +342,7 @@ export class DevtoolsConnectionState {
           null,
           options
         ),
-        ...addToOIDCPluginHttpOptions(options.oidc, { ca }),
+        ...addToOIDCPluginHttpOptions(options.oidc, ca ? { ca } : {}),
       });
     }
   }
@@ -397,6 +400,11 @@ export interface DevtoolsConnectOptions extends MongoClientOptions {
   applyProxyToOIDC?: boolean;
 }
 
+export type ConnectMongoClientResult = {
+  client: MongoClient;
+  state: DevtoolsConnectionState;
+};
+
 /**
  * Connect a MongoClient. If AutoEncryption is requested, first connect without the encryption options and verify that
  * the connection is to an enterprise cluster. If not, then error, otherwise close the connection and reconnect with the
@@ -407,35 +415,73 @@ export async function connectMongoClient(
   clientOptions: DevtoolsConnectOptions,
   logger: ConnectLogEmitter,
   MongoClientClass: typeof MongoClient
-): Promise<{
-  client: MongoClient;
-  state: DevtoolsConnectionState;
-}> {
+): Promise<ConnectMongoClientResult> {
+  detectAndLogMissingOptionalDependencies(logger);
+
+  const options = { uri, clientOptions, logger, MongoClientClass };
+  // Connect once with the system certificate store added, and if that fails with
+  // a TLS error, try again. In theory adding certificates into the certificate store
+  // should not cause failures, but in practice we have observed some, hence this
+  // double connection establishment logic.
+  // We treat TLS errors as fail-fast errors, so in typical situations (even typical
+  // failure situations) we do not spend an unreasonable amount of time in the first
+  // connection attempt.
+  try {
+    return await connectMongoClientImpl({ ...options, useSystemCA: true });
+  } catch (error: unknown) {
+    if (isPotentialTLSCertificateError(error)) {
+      logger.emit('devtools-connect:retry-after-tls-error', {
+        error: String(error),
+      });
+      try {
+        return await connectMongoClientImpl({ ...options, useSystemCA: false });
+      } catch {}
+    }
+    throw error;
+  }
+}
+
+async function connectMongoClientImpl({
+  uri,
+  clientOptions,
+  logger,
+  MongoClientClass,
+  useSystemCA,
+}: {
+  uri: string;
+  clientOptions: DevtoolsConnectOptions;
+  logger: ConnectLogEmitter;
+  MongoClientClass: typeof MongoClient;
+  useSystemCA: boolean;
+}): Promise<ConnectMongoClientResult> {
   const cleanupOnClientClose: (() => void | Promise<void>)[] = [];
   const runClose = async () => {
     let item: (() => void | Promise<void>) | undefined;
     while ((item = cleanupOnClientClose.shift())) await item();
   };
-  detectAndLogMissingOptionalDependencies(logger);
 
+  let ca: string | undefined;
   try {
-    const {
-      ca,
-      asyncFallbackError,
-      systemCertsError,
-      systemCACount,
-      messages,
-    } = await systemCA({
-      ca: clientOptions.ca,
-      tlsCAFile:
-        clientOptions.tlsCAFile || getConnectionStringParam(uri, 'tlsCAFile'),
-    });
-    logger.emit('devtools-connect:used-system-ca', {
-      caCount: systemCACount,
-      asyncFallbackError,
-      systemCertsError,
-      messages,
-    });
+    if (useSystemCA) {
+      const {
+        ca: caWithSystemCerts,
+        asyncFallbackError,
+        systemCertsError,
+        systemCACount,
+        messages,
+      } = await systemCA({
+        ca: clientOptions.ca,
+        tlsCAFile:
+          clientOptions.tlsCAFile || getConnectionStringParam(uri, 'tlsCAFile'),
+      });
+      logger.emit('devtools-connect:used-system-ca', {
+        caCount: systemCACount,
+        asyncFallbackError,
+        systemCertsError,
+        messages,
+      });
+      ca = caWithSystemCerts;
+    }
 
     // Create a proxy agent, if requested. `useOrCreateAgent()` takes a target argument
     // that can be used to select a proxy for a specific procotol or host;
@@ -448,7 +494,7 @@ export async function connectMongoClient(
           ? clientOptions.proxy
           : {
               ...(clientOptions.proxy as DevtoolsProxyOptions),
-              ca,
+              ...(ca ? { ca } : {}),
             },
         clientOptions.applyProxyToOIDC ? undefined : 'mongodb://'
       );
@@ -498,7 +544,8 @@ export async function connectMongoClient(
       {},
       clientOptions,
       shouldAddOidcCallbacks ? state.oidcPlugin.mongoClientOptions : {},
-      { ca, allowPartialTrustChain: true }
+      { allowPartialTrustChain: true },
+      ca ? { ca } : {}
     );
 
     // Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458.

packages/devtools-connect/src/fast-failure-connect.ts

@@ -1,7 +1,9 @@
 // It probably makes sense to put this into its own package/repository once
 // other tools start using it.
 
-export function isFastFailureConnectionError(error: Error): boolean {
+function isFastFailureConnectionSingleError(
+  error: Error & { code?: string }
+): boolean {
   switch (error.name) {
     case 'MongoNetworkError':
       return /\b(ECONNREFUSED|ENOTFOUND|ENETUNREACH|EINVAL)\b/.test(
@@ -10,6 +12,81 @@ export function isFastFailureConnectionError(error: Error): boolean {
     case 'MongoError':
       return /The apiVersion parameter is required/.test(error.message);
     default:
-      return false;
+      return (
+        ['ECONNREFUSED', 'ENOTFOUND', 'ENETUNREACH', 'EINVAL'].includes(
+          error.code ?? ''
+        ) || isPotentialTLSCertificateError(error)
+      );
   }
 }
+
+export const isFastFailureConnectionError = handleNestedErrors(
+  isFastFailureConnectionSingleError
+);
+
+function isPotentialTLSCertificateSingleError(
+  error: Error & { code?: string }
+): boolean {
+  // https://nodejs.org/api/tls.html#x509-certificate-error-codes
+  return [
+    'UNABLE_TO_GET_ISSUER_CERT',
+    'UNABLE_TO_GET_CRL',
+    'UNABLE_TO_DECRYPT_CERT_SIGNATURE',
+    'UNABLE_TO_DECRYPT_CRL_SIGNATURE',
+    'UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY',
+    'CERT_SIGNATURE_FAILURE',
+    'CRL_SIGNATURE_FAILURE',
+    'CERT_NOT_YET_VALID',
+    'CERT_HAS_EXPIRED',
+    'CRL_NOT_YET_VALID',
+    'CRL_HAS_EXPIRED',
+    'ERROR_IN_CERT_NOT_BEFORE_FIELD',
+    'ERROR_IN_CERT_NOT_AFTER_FIELD',
+    'ERROR_IN_CRL_LAST_UPDATE_FIELD',
+    'ERROR_IN_CRL_NEXT_UPDATE_FIELD',
+    'OUT_OF_MEM',
+    'DEPTH_ZERO_SELF_SIGNED_CERT',
+    'SELF_SIGNED_CERT_IN_CHAIN',
+    'UNABLE_TO_GET_ISSUER_CERT_LOCALLY',
+    'UNABLE_TO_VERIFY_LEAF_SIGNATURE',
+    'CERT_CHAIN_TOO_LONG',
+    'CERT_REVOKED',
+    'INVALID_CA',
+    'PATH_LENGTH_EXCEEDED',
+    'INVALID_PURPOSE',
+    'CERT_UNTRUSTED',
+    'CERT_REJECTED',
+    'HOSTNAME_MISMATCH',
+  ].includes(error.code ?? '');
+}
+
+export const isPotentialTLSCertificateError = handleNestedErrors(
+  isPotentialTLSCertificateSingleError
+);
+
+// Convenience wrapper that ensures that the given error is an `Error` instance
+// and that handles nested errors (.cause/AggregateError) as well
+function handleNestedErrors(
+  fn: (err: Error & { code?: string }) => boolean
+): (err: unknown) => boolean {
+  const checker = (err: unknown): boolean => {
+    if (
+      Object.prototype.toString.call(err).toLowerCase() !== '[object error]'
+    ) {
+      return checker(new Error(String(err)));
+    }
+    if (!err || typeof err !== 'object') return false; // Make TS happy
+    if ('cause' in err && checker(err.cause)) {
+      return true; // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
+    }
+    if (
+      'errors' in err &&
+      Array.isArray(err.errors) &&
+      err.errors.some((err) => checker(err))
+    ) {
+      return true; // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError
+    }
+    return fn(err as Error);
+  };
+  return checker;
+}

packages/devtools-connect/src/index.ts

@@ -5,6 +5,7 @@ export type {
   DevtoolsConnectionState,
   DevtoolsProxyOptions,
   AgentWithInitialize,
+  ConnectMongoClientResult,
 } from './connect';
 export { hookLogger } from './log-hook';
 export { oidcServerRequestHandler } from './oidc/handler';

packages/devtools-connect/src/log-hook.ts

@@ -8,6 +8,7 @@ import type {
   ConnectMissingOptionalDependencyEvent,
   ConnectUsedSystemCAEvent,
   ConnectLogEmitter,
+  ConnectRetryAfterTLSErrorEvent,
 } from './types';
 
 import { hookLoggerToMongoLogWriter as oidcHookLogger } from '@mongodb-js/oidc-plugin';
@@ -174,4 +175,19 @@ export function hookLogger(
       );
     }
   );
+
+  emitter.on(
+    'devtools-connect:retry-after-tls-error',
+    function (ev: ConnectRetryAfterTLSErrorEvent) {
+      log.info(
+        'DEVTOOLS-CONNECT',
+        mongoLogId(1_000_000_054),
+        `${contextPrefix}-connect`,
+        'Restarting connection attempt after TLS error',
+        {
+          error: ev.error,
+        }
+      );
+    }
+  );
 }

packages/devtools-connect/src/types.ts

@@ -58,6 +58,10 @@ export interface ConnectUsedSystemCAEvent {
   messages: string[];
 }
 
+export interface ConnectRetryAfterTLSErrorEvent {
+  error: string;
+}
+
 export interface ConnectEventMap
   extends MongoDBOIDCLogEventsMap,
     ProxyEventMap {
@@ -93,6 +97,10 @@ export interface ConnectEventMap
   ) => void;
   /** Signals that the list of system certificates has been loaded and used for connecting. */
   'devtools-connect:used-system-ca': (ev: ConnectUsedSystemCAEvent) => void;
+  /** Signals that a connection attempt was restarted after a TLS error */
+  'devtools-connect:retry-after-tls-error': (
+    ev: ConnectRetryAfterTLSErrorEvent
+  ) => void;
 }
 
 export type ConnectEventArgs<K extends keyof ConnectEventMap> =