fix(devtools-connect): fix TLS error fallback mechanism MONGOSH-2488 (#566)

Restructure our proxy integration logic to:

- Avoid modifying the OIDC options – previously we were assigning to
  `clientOptions.oidc`, which meant that the `fetch` function created
  to read the system certificates was also used for the non-system-cert
  `connect()` call, breaking our fallback mechanism
- Add a test that verifies that the fallback mechanism works both for
  TLS driver connections and HTTPS OIDC calls.
- Deduplicate the logic for adding CA options to the `DevtoolsProxyOptions`
  instances.
- Share log info from the OIDC agent instance, if a new one has been
  created.
- Make sure that TLS errors passed through node-fetch are actually
  picked up by our "nested error" detection logic (they have a different
  `toStringTag` defined than regular `Error` instances).

Author: addaleax

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

@@ -6,6 +6,7 @@ import { MongoClient } from 'mongodb';
 import sinon, { stubConstructor } from 'ts-sinon';
 import chai, { expect } from 'chai';
 import sinonChai from 'sinon-chai';
+import { Agent as HTTPSAgent } from 'https';
 
 chai.use(sinonChai);
 
@@ -375,7 +376,7 @@ describe('devtools connect', function () {
     });
 
     describe('retryable TLS errors', function () {
-      it('retries TLS errors without system CA integration enabled', async function () {
+      it('retries TLS errors without system CA integration enabled -- MongoClient error', async function () {
         const uri = 'localhost:27017';
         const mClient = new FakeMongoClient();
         const mClientType = sinon.stub().returns(mClient);
@@ -429,6 +430,67 @@ describe('devtools connect', function () {
 
         expect(earlyFailures).to.equal(2);
       });
+
+      it('retries TLS errors without system CA integration enabled -- OIDC error', async function () {
+        const uri = 'localhost:27017';
+        const mClient = new FakeMongoClient();
+        let opts: DevtoolsConnectOptions;
+
+        const proxyConnectEvents: any[] = [];
+        bus.on('proxy:connect', (ev) => {
+          proxyConnectEvents.push({ ...ev, opts: { ...ev.opts } });
+          ev.opts.ca = 'abcdef'; // will always fail
+          ev.agent.httpsAgent = new HTTPSAgent({ ca: 'abcdef' });
+        });
+        const mClientType = sinon.stub().callsFake((url, clientOpts) => {
+          opts = clientOpts;
+          return mClient;
+        });
+
+        mClient.connect = sinon.stub().callsFake(async () => {
+          return await opts.authMechanismProperties?.OIDC_HUMAN_CALLBACK?.({
+            version: 1,
+            timeoutContext: new AbortController().signal,
+            idpInfo: {
+              issuer: 'https://mongodb.com/',
+              clientId: 'meow',
+            },
+          });
+        });
+        mClient.topology = {
+          description: {
+            servers: new Map([['localhost:27017', {}]]),
+          },
+        };
+
+        try {
+          await connectMongoClient(
+            uri,
+            {
+              ...defaultOpts,
+              authMechanism: 'MONGODB-OIDC',
+            },
+            bus,
+            mClientType as any,
+          );
+          expect.fail('missed exception');
+        } catch (err: any) {
+          expect(err.message).to.include('Unable to fetch issuer metadata');
+        }
+
+        expect(mClientType.getCalls()).to.have.lengthOf(2);
+        expect((mClient as any).connect.getCalls()).to.have.lengthOf(2);
+        expect(proxyConnectEvents).to.have.lengthOf(2);
+        expect(proxyConnectEvents[0].agent).to.not.equal(
+          proxyConnectEvents[1].agent,
+        );
+        expect(proxyConnectEvents[0].opts.ca).to.not.equal(
+          proxyConnectEvents[1].opts.ca,
+        );
+        expect(proxyConnectEvents[0].agent.proxyOptions.ca).to.not.equal(
+          proxyConnectEvents[1].agent.proxyOptions.ca,
+        );
+      });
     });
   });
 

packages/devtools-connect/src/connect.ts

@@ -32,6 +32,7 @@ import {
   Tunnel,
   systemCA,
   createFetch,
+  isExistingAgentInstance,
 } from '@mongodb-js/devtools-proxy-support';
 export type { DevtoolsProxyOptions, AgentWithInitialize };
 
@@ -490,19 +491,25 @@ async function connectMongoClientImpl({
       ca = caWithSystemCerts;
     }
 
+    const addCAToProxyOptions = (
+      opts: AgentWithInitialize | DevtoolsProxyOptions | undefined,
+    ): AgentWithInitialize | DevtoolsProxyOptions => {
+      if (opts && isExistingAgentInstance(opts)) return opts;
+      return {
+        caExcludeSystemCerts: !useSystemCA,
+        ...opts,
+        ...(ca ? { ca } : {}),
+      };
+    };
+
     // 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;
     // here we specify 'mongodb://' if we only intend to use the proxy for database
     // connectivity.
     const proxyAgent =
       clientOptions.proxy &&
       useOrCreateAgent(
-        'createConnection' in clientOptions.proxy
-          ? clientOptions.proxy
-          : {
-              ...(clientOptions.proxy as DevtoolsProxyOptions),
-              ...(ca ? { ca } : {}),
-            },
+        addCAToProxyOptions(clientOptions.proxy),
         clientOptions.applyProxyToOIDC === true ? undefined : 'mongodb://',
       );
     cleanupOnClientClose.push(() => proxyAgent?.destroy());
@@ -516,17 +523,16 @@ async function connectMongoClientImpl({
     } else if (clientOptions.applyProxyToOIDC) {
       oidcProxyOptions = clientOptions.applyProxyToOIDC;
     }
-    if (!oidcProxyOptions && ca) {
-      oidcProxyOptions = { ca };
-    }
-    if (oidcProxyOptions) {
-      clientOptions.oidc = {
-        customFetch: createFetch(
-          oidcProxyOptions,
-        ) as unknown as MongoDBOIDCPluginOptions['customFetch'],
+
+    const oidcFetch = createFetch(addCAToProxyOptions(oidcProxyOptions));
+    clientOptions = {
+      ...clientOptions,
+      oidc: {
+        customFetch:
+          oidcFetch as unknown as MongoDBOIDCPluginOptions['customFetch'],
         ...clientOptions.oidc,
-      };
-    }
+      },
+    };
 
     let tunnel: Tunnel | undefined;
     if (proxyAgent && !hasProxyHostOption(uri, clientOptions)) {
@@ -537,7 +543,11 @@ async function connectMongoClientImpl({
       );
       cleanupOnClientClose.push(() => tunnel?.close());
     }
-    for (const proxyLogger of new Set([tunnel?.logger, proxyAgent?.logger])) {
+    for (const proxyLogger of new Set([
+      tunnel?.logger,
+      proxyAgent?.logger,
+      oidcFetch?.agent?.logger,
+    ])) {
       if (proxyLogger) {
         copyEventEmitterEvents(proxyLogger, logger);
       }

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

@@ -71,7 +71,8 @@ function handleNestedErrors(
 ): (err: unknown) => boolean {
   const checker = (err: unknown): boolean => {
     if (
-      Object.prototype.toString.call(err).toLowerCase() !== '[object error]'
+      // e.g. node-fetch defines its FetchError class to be a `[object FetchError]`
+      !Object.prototype.toString.call(err).match(/\[object .*error\]/i)
     ) {
       return checker(new Error(String(err)));
     }

packages/devtools-proxy-support/src/agent.ts

@@ -101,6 +101,7 @@ class DevtoolsProxyAgent extends ProxyAgent implements AgentWithInitialize {
     }
     this._req = req;
     this._reqLock = new Promise((resolve) => (this._reqLockResolve = resolve));
+    this.logger.emit('proxy:connect', { agent: this, req, opts });
     const agent = await super.connect(req, opts);
     // Work around https://github.com/TooTallNate/proxy-agents/pull/330
     if ('addRequest' in agent && typeof agent.addRequest === 'function') {
@@ -131,7 +132,10 @@ class DevtoolsProxyAgentWithSystemCA extends AgentBase {
     super();
     this.proxyOptions = proxyOptions;
     this.agent = (async () => {
-      const { ca } = await systemCA({ ca: proxyOptions.ca });
+      const { ca } = await systemCA({
+        ca: proxyOptions.ca,
+        excludeSystemCerts: proxyOptions.caExcludeSystemCerts,
+      });
       return new DevtoolsProxyAgent(
         { ...proxyOptions, ca, allowPartialTrustChain: true },
         this.logger,
@@ -167,8 +171,8 @@ export function useOrCreateAgent(
   target?: string,
   useTargetRegardlessOfExistingAgent = false,
 ): AgentWithInitialize | undefined {
-  if ('createConnection' in proxyOptions) {
-    const agent = proxyOptions as AgentWithInitialize;
+  if (isExistingAgentInstance(proxyOptions)) {
+    const agent = proxyOptions;
     if (
       useTargetRegardlessOfExistingAgent &&
       target !== undefined &&
@@ -179,12 +183,15 @@ export function useOrCreateAgent(
     }
     return agent;
   } else {
-    if (
-      target !== undefined &&
-      !proxyForUrl(proxyOptions as DevtoolsProxyOptions, target)
-    ) {
+    if (target !== undefined && !proxyForUrl(proxyOptions, target)) {
       return undefined;
     }
-    return createAgent(proxyOptions as DevtoolsProxyOptions);
+    return createAgent(proxyOptions);
   }
 }
+
+export function isExistingAgentInstance(
+  options: DevtoolsProxyOptions | AgentWithInitialize,
+): options is AgentWithInitialize {
+  return 'createConnection' in options;
+}

packages/devtools-proxy-support/src/index.ts

@@ -1,6 +1,11 @@
 export * from './proxy-options-public';
 export { Tunnel, TunnelOptions, createSocks5Tunnel } from './socks5';
-export { createAgent, useOrCreateAgent, AgentWithInitialize } from './agent';
+export {
+  createAgent,
+  useOrCreateAgent,
+  isExistingAgentInstance,
+  AgentWithInitialize,
+} from './agent';
 export {
   createFetch,
   Request,

packages/devtools-proxy-support/src/logging.ts

@@ -3,6 +3,11 @@
 // values used here; in particular,
 // https://github.com/mongodb-js/compass/tree/55a5a608713d7316d158dc66febeb6b114d8b40d/packages/ssh-tunnel/src
 
+import type { AgentConnectOpts } from 'agent-base';
+import type { ClientRequest } from 'http';
+import type { SecureContextOptions } from 'tls';
+import type { AgentWithInitialize } from './agent';
+
 interface BaseSocks5RequestMetadata {
   srcAddr: string;
   srcPort: number;
@@ -45,6 +50,11 @@ export interface ProxyEventMap {
     retryableError: boolean;
     retriesLeft: number;
   }) => void;
+  'proxy:connect': (ev: {
+    agent: AgentWithInitialize;
+    req: ClientRequest;
+    opts: AgentConnectOpts & Partial<SecureContextOptions>;
+  }) => void;
 }
 
 export type ProxyEventArgs<K extends keyof ProxyEventMap> =

packages/devtools-proxy-support/src/proxy-options.ts

@@ -21,8 +21,11 @@ export interface DevtoolsProxyOptions {
   };
 
   // Not being honored by the translate-to-electron functionality
-  // This will be merged with the system CA list and the Node.js built-in CA list
+  // This will be merged with the system CA list and the Node.js built-in CA list,
+  // unless `caExcludeSystemCerts` is set
   ca?: ConnectionOptions['ca'];
+  // Exclude system certificates from the full certificate list.
+  caExcludeSystemCerts?: boolean;
 
   // Mostly intended for testing, defaults to `process.env`.
   // This should usually not be stored, and secrets will not be

packages/devtools-proxy-support/src/system-ca.ts

@@ -58,6 +58,7 @@ export async function systemCA(
   existingOptions: {
     ca?: NodeJSCAOption;
     tlsCAFile?: string | null | undefined;
+    excludeSystemCerts?: boolean;
   } = {},
 ): Promise<{
   ca: string;
@@ -81,9 +82,11 @@ export async function systemCA(
   const messages: string[] = [];
 
   try {
-    const systemCertsResult = await systemCertsCached();
-    asyncFallbackError = systemCertsResult.asyncFallbackError;
-    systemCerts = systemCertsResult.certs;
+    if (!existingOptions.excludeSystemCerts) {
+      const systemCertsResult = await systemCertsCached();
+      asyncFallbackError = systemCertsResult.asyncFallbackError;
+      systemCerts = systemCertsResult.certs;
+    }
   } catch (err: any) {
     systemCertsError = err;
   }