From 98f6f31b7f97677b1c74292101a5f1f2a67660d9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 16:56:31 +0200 Subject: [PATCH] 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. --- packages/devtools-connect/src/connect.ts | 18 +++-- .../devtools-connect/src/log-hook.spec.ts | 2 + packages/devtools-connect/src/log-hook.ts | 1 + packages/devtools-connect/src/types.ts | 1 + .../src/system-ca.spec.ts | 74 ++++++++++++++++++ .../devtools-proxy-support/src/system-ca.ts | 75 ++++++++++++++++++- 6 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 packages/devtools-proxy-support/src/system-ca.spec.ts diff --git a/packages/devtools-connect/src/connect.ts b/packages/devtools-connect/src/connect.ts index 7538cdf..8fd28bc 100644 --- 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 index 43973a3..2561573 100644 --- 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 index 75e38d5..392227f 100644 --- 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 index cf52020..16f4c7a 100644 --- 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 new file mode 100644 index 0000000..e0e447d --- /dev/null +++ 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 index 780347a..729aa7c 100644 --- 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 | undefined; if (existingOptions.tlsCAFile) { @@ -76,6 +136,7 @@ export async function systemCA( let systemCertsError: Error | undefined; let asyncFallbackError: Error | undefined; let systemCerts: string[] = []; + let messages: string[] = []; try { ({ certs: systemCerts, asyncFallbackError } = await systemCertsCached()); @@ -83,6 +144,17 @@ export async function systemCA( 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, }; }