fix(devtools-proxy-support): remove certificates without issuer from …

…system CA list COMPASS-8252 (#467) We have discovered that system certificate stores may contain certificates that a) OpenSSL may choose over other roots for the same certificate chain and b) are not accompanied by their own root certificate, i.e. only present an immediate certificate. While we will ideally solve this by using `X509_V_FLAG_PARTIAL_CHAIN`, Node.js does not expose that as a feature yet.
mongodb-js · Sep 5, 2024 · 98f6f31 · 98f6f31
1 parent 7d2615b
commit 98f6f31
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 7 deletions.
diff --git a/packages/devtools-connect/src/connect.ts b/packages/devtools-connect/src/connect.ts
@@ -419,16 +419,22 @@ export async function connectMongoClient(
   detectAndLogMissingOptionalDependencies(logger);
 
   try {
-    const { ca, asyncFallbackError, systemCertsError, systemCACount } =
-      await systemCA({
-        ca: clientOptions.ca,
-        tlsCAFile:
-          clientOptions.tlsCAFile || getConnectionStringParam(uri, 'tlsCAFile'),
-      });
+    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,
     });
 
     // Create a proxy agent, if requested. `useOrCreateAgent()` takes a target argument

diff --git a/packages/devtools-connect/src/log-hook.spec.ts b/packages/devtools-connect/src/log-hook.spec.ts
@@ -78,6 +78,7 @@ describe('Logging setup', function () {
       caCount: 1234,
       asyncFallbackError: new Error('had to fallback to sync'),
       systemCertsError: new Error('could not load system certs at all'),
+      messages: ['A diagnostic message'],
     });
 
     await log.flush();
@@ -212,6 +213,7 @@ describe('Logging setup', function () {
           caCount: 1234,
           asyncFallbackError: 'had to fallback to sync',
           systemCertsError: 'could not load system certs at all',
+          messages: ['A diagnostic message'],
         },
       },
     ]);

diff --git a/packages/devtools-connect/src/log-hook.ts b/packages/devtools-connect/src/log-hook.ts
@@ -169,6 +169,7 @@ export function hookLogger(
           caCount: ev.caCount,
           asyncFallbackError: ev.asyncFallbackError?.message,
           systemCertsError: ev.systemCertsError?.message,
+          messages: ev.messages,
         }
       );
     }

diff --git a/packages/devtools-connect/src/types.ts b/packages/devtools-connect/src/types.ts
@@ -55,6 +55,7 @@ export interface ConnectUsedSystemCAEvent {
   caCount: number;
   asyncFallbackError: Error | undefined;
   systemCertsError: Error | undefined;
+  messages: string[];
 }
 
 export interface ConnectEventMap

diff --git a/packages/devtools-proxy-support/src/system-ca.spec.ts b/packages/devtools-proxy-support/src/system-ca.spec.ts
@@ -0,0 +1,74 @@
+import { expect } from 'chai';
+import { removeCertificatesWithoutIssuer } from './system-ca';
+
+describe('removeCertificatesWithoutIssuer', function () {
+  it('removes certificates that do not have an issuer', function () {
+    const certs = [
+      `-----BEGIN CERTIFICATE-----
+MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAwTzELMAkG
+A1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2VhcmNoIEdyb3VwMRUw
+EwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4WhcNMzUwNjA0MTEwNDM4WjBP
+MQswCQYDVQQGEwJVUzEpMCcGA1UEChMgSW50ZXJuZXQgU2VjdXJpdHkgUmVzZWFyY2ggR3Jv
+dXAxFTATBgNVBAMTDElTUkcgUm9vdCBYMTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoC
+ggIBAK3oJHP0FDfzm54rVygch77ct984kIxuPOZXoHj3dcKi/vVqbvYATyjb3miGbESTtrFj
+/RQSa78f0uoxmyF+0TM8ukj13Xnfs7j/EvEhmkvBioZxaUpmZmyPfjxwv60pIgbz5MDmgK7i
+S4+3mX6UA5/TR5d8mUgjU+g4rk8Kb4Mu0UlXjIB0ttov0DiNewNwIRt18jA8+o+u3dpjq+sW
+T8KOEUt+zwvo/7V3LvSye0rgTBIlDHCNAymg4VMk7BPZ7hm/ELNKjD+Jo2FR3qyHB5T0Y3Hs
+LuJvW5iB4YlcNHlsdu87kGJ55tukmi8mxdAQ4Q7e2RCOFvu396j3x+UCB5iPNgiV5+I3lg02
+dZ77DnKxHZu8A/lJBdiB3QW0KtZB6awBdpUKD9jf1b0SHzUvKBds0pjBqAlkd25HN7rOrFle
+aJ1/ctaJxQZBKT5ZPt0m9STJEadao0xAH0ahmbWnOlFuhjuefXKnEgV4We0+UXgVCwOPjdAv
+BbI+e0ocS3MFEvzG6uBQE3xDk3SzynTnjh8BCNAw1FtxNrQHusEwMFxIt4I7mKZ9YIqioymC
+zLq9gwQbooMDQaHWBfEbwrbwqHyGO0aoSCqI3Haadr8faqU9GY/rOPNk3sgrDQoo//fb4hVC
+1CLQJ13hef4Y53CIrU7m2Ys6xt0nUW7/vGT1M0NPAgMBAAGjQjBAMA4GA1UdDwEB/wQEAwIB
+BjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBR5tFnme7bl5AFzgAiIyBpY9umbbjANBgkq
+hkiG9w0BAQsFAAOCAgEAVR9YqbyyqFDQDLHYGmkgJykIrGF1XIpu+ILlaS/V9lZLubhzEFnT
+IZd+50xx+7LSYK05qAvqFyFWhfFQDlnrzuBZ6brJFe+GnY+EgPbk6ZGQ3BebYhtF8GaV0nxv
+wuo77x/Py9auJ/GpsMiu/X1+mvoiBOv/2X/qkSsisRcOj/KKNFtY2PwByVS5uCbMiogziUwt
+hDyC3+6WVwW6LLv3xLfHTjuCvjHIInNzktHCgKQ5ORAzI4JMPJ+GslWYHb4phowim57iaztX
+OoJwTdwJx4nLCgdNbOhdjsnvzqvHu7UrTkXWStAmzOVyyghqpZXjFaH3pO3JLF+l+/+sKAIu
+vtd7u+Nxe5AW0wdeRlN8NwdCjNPElpzVmbUq4JUagEiuTDkHzsxHpFKVK7q4+63SM1N95R1N
+bdWhscdCb+ZAJzVcoyi3B43njTOQ5yOf+1CceWxG1bQVs5ZufpsMljq4Ui0/1lvh+wjChP4k
+qKOJ2qxq4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA
+mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57demyPxgcY
+xn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=
+-----END CERTIFICATE-----`,
+      `-----BEGIN CERTIFICATE-----
+MIIFYDCCBEigAwIBAgIQQAF3ITfU6UK47naqPGQKtzANBgkqhkiG9w0BAQsFADA/
+MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT
+DkRTVCBSb290IENBIFgzMB4XDTIxMDEyMDE5MTQwM1oXDTI0MDkzMDE4MTQwM1ow
+TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh
+cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwggIiMA0GCSqGSIb3DQEB
+AQUAA4ICDwAwggIKAoICAQCt6CRz9BQ385ueK1coHIe+3LffOJCMbjzmV6B493XC
+ov71am72AE8o295ohmxEk7axY/0UEmu/H9LqMZshftEzPLpI9d1537O4/xLxIZpL
+wYqGcWlKZmZsj348cL+tKSIG8+TA5oCu4kuPt5l+lAOf00eXfJlII1PoOK5PCm+D
+LtFJV4yAdLbaL9A4jXsDcCEbdfIwPPqPrt3aY6vrFk/CjhFLfs8L6P+1dy70sntK
+4EwSJQxwjQMpoOFTJOwT2e4ZvxCzSow/iaNhUd6shweU9GNx7C7ib1uYgeGJXDR5
+bHbvO5BieebbpJovJsXQEOEO3tkQjhb7t/eo98flAgeYjzYIlefiN5YNNnWe+w5y
+sR2bvAP5SQXYgd0FtCrWQemsAXaVCg/Y39W9Eh81LygXbNKYwagJZHduRze6zqxZ
+Xmidf3LWicUGQSk+WT7dJvUkyRGnWqNMQB9GoZm1pzpRboY7nn1ypxIFeFntPlF4
+FQsDj43QLwWyPntKHEtzBRL8xurgUBN8Q5N0s8p0544fAQjQMNRbcTa0B7rBMDBc
+SLeCO5imfWCKoqMpgsy6vYMEG6KDA0Gh1gXxG8K28Kh8hjtGqEgqiNx2mna/H2ql
+PRmP6zjzZN7IKw0KKP/32+IVQtQi0Cdd4Xn+GOdwiK1O5tmLOsbdJ1Fu/7xk9TND
+TwIDAQABo4IBRjCCAUIwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYw
+SwYIKwYBBQUHAQEEPzA9MDsGCCsGAQUFBzAChi9odHRwOi8vYXBwcy5pZGVudHJ1
+c3QuY29tL3Jvb3RzL2RzdHJvb3RjYXgzLnA3YzAfBgNVHSMEGDAWgBTEp7Gkeyxx
++tvhS5B1/8QVYIWJEDBUBgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEEAYLfEwEB
+ATAwMC4GCCsGAQUFBwIBFiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2VuY3J5cHQu
+b3JnMDwGA1UdHwQ1MDMwMaAvoC2GK2h0dHA6Ly9jcmwuaWRlbnRydXN0LmNvbS9E
+U1RST09UQ0FYM0NSTC5jcmwwHQYDVR0OBBYEFHm0WeZ7tuXkAXOACIjIGlj26Ztu
+MA0GCSqGSIb3DQEBCwUAA4IBAQAKcwBslm7/DlLQrt2M51oGrS+o44+/yQoDFVDC
+5WxCu2+b9LRPwkSICHXM6webFGJueN7sJ7o5XPWioW5WlHAQU7G75K/QosMrAdSW
+9MUgNTP52GE24HGNtLi1qoJFlcDyqSMo59ahy2cI2qBDLKobkx/J3vWraV0T9VuG
+WCLKTVXkcGdtwlfFRjlBz4pYg1htmf5X6DYO8A4jqv2Il9DjXA6USbW1FzXSLr9O
+he8Y4IWS6wY7bCkjCWDcRQJMEhg76fsO3txE+FiYruq9RUWhiF1myv4Q6W+CyBFC
+Dfvp7OOGAN6dEOM4+qR9sdjoSYKEBpsr6GtPAQw4dy753ec5
+-----END CERTIFICATE-----`,
+    ];
+    expect(removeCertificatesWithoutIssuer(certs)).to.deep.equal({
+      ca: [certs[0]],
+      messages: [
+        `Removing certificate for 'C=US\nO=Internet Security Research Group\nCN=ISRG Root X1' because issuer 'O=Digital Signature Trust Co.\nCN=DST Root CA X3' could not be found (serial no '4001772137D4E942B8EE76AA3C640AB7')`,
+      ],
+    });
+  });
+});
diff --git a/packages/devtools-proxy-support/src/system-ca.ts b/packages/devtools-proxy-support/src/system-ca.ts
@@ -2,6 +2,7 @@ import { systemCertsAsync } from 'system-ca';
 import type { Options as SystemCAOptions } from 'system-ca';
 import { promises as fs } from 'fs';
 import { rootCertificates } from 'tls';
+import { X509Certificate } from 'crypto';
 
 // A bit more generic than SecureContextOptions['ca'] because of Uint8Array -> Buffer + readonly
 type NodeJSCAOption = string | Uint8Array | readonly (string | Uint8Array)[];
@@ -50,6 +51,63 @@ export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string {
   return [...ca].join('\n');
 }
 
+const pemWithParsedCache = new WeakMap<
+  string[],
+  {
+    ca: string[];
+    messages: string[];
+  }
+>();
+// TODO(COMPASS-8253): Remove this in favor of OpenSSL's X509_V_FLAG_PARTIAL_CHAIN
+// See linked tickets for details on why we need this (tl;dr: the system certificate
+// store may contain intermediate certficiates without the corresponding trusted root,
+// and OpenSSL does not seem to accept that)
+export function removeCertificatesWithoutIssuer(ca: string[]): {
+  ca: string[];
+  messages: string[];
+} {
+  let result:
+    | {
+        ca: string[];
+        messages: string[];
+      }
+    | undefined = pemWithParsedCache.get(ca);
+
+  const messages: string[] = [];
+  let caWithParsedCerts = ca.map((pem) => {
+    let parsed: X509Certificate | null = null;
+    try {
+      parsed = new X509Certificate(pem);
+    } catch (err: unknown) {
+      messages.push(
+        `Unable to parse certificate: ${
+          err && typeof err === 'object' && 'message' in err
+            ? String(err.message)
+            : String(err)
+        }`
+      );
+    }
+    return { pem, parsed };
+  });
+  caWithParsedCerts = caWithParsedCerts.filter(({ parsed }) => {
+    const keep =
+      !parsed ||
+      parsed.checkIssued(parsed) ||
+      caWithParsedCerts.find(
+        ({ parsed: issuer }) => issuer && parsed.checkIssued(issuer)
+      );
+    if (!keep) {
+      messages.push(
+        `Removing certificate for '${parsed.subject}' because issuer '${parsed.issuer}' could not be found (serial no '${parsed.serialNumber}')`
+      );
+    }
+    return keep;
+  });
+  result = { ca: caWithParsedCerts.map(({ pem }) => pem), messages };
+  pemWithParsedCache.set(ca, result);
+  return result;
+}
+
 // Thin wrapper around system-ca, which merges:
 // - Explicit CA options passed as options
 // - The Node.js TLS root store
@@ -58,12 +116,14 @@ export async function systemCA(
   existingOptions: {
     ca?: NodeJSCAOption;
     tlsCAFile?: string | null | undefined;
-  } = {}
+  } = {},
+  allowCertificatesWithoutIssuer?: boolean // defaults to false
 ): Promise<{
   ca: string;
   systemCACount: number;
   asyncFallbackError?: Error;
   systemCertsError?: Error;
+  messages: string[];
 }> {
   let readTLSCAFilePromise: Promise<string> | undefined;
   if (existingOptions.tlsCAFile) {
@@ -76,13 +136,25 @@ export async function systemCA(
   let systemCertsError: Error | undefined;
   let asyncFallbackError: Error | undefined;
   let systemCerts: string[] = [];
+  let messages: string[] = [];
 
   try {
     ({ certs: systemCerts, asyncFallbackError } = await systemCertsCached());
   } catch (err: any) {
     systemCertsError = err;
   }
 
+  if (
+    !(
+      allowCertificatesWithoutIssuer ??
+      !!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER
+    )
+  ) {
+    const reducedList = removeCertificatesWithoutIssuer(systemCerts);
+    systemCerts = reducedList.ca;
+    messages = messages.concat(reducedList.messages);
+  }
+
   return {
     ca: mergeCA(
       systemCerts,
@@ -93,5 +165,6 @@ export async function systemCA(
     asyncFallbackError: asyncFallbackError,
     systemCertsError,
     systemCACount: systemCerts.length + rootCertificates.length,
+    messages,
   };
 }