chore(devtools-proxy-support): push expired certs to the bottom of the system CA list COMPASS-8322 #474
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,50 +31,39 @@ function systemCertsCached(systemCAOpts: SystemCAOptions = {}): Promise<{ | |
| return systemCertsCachePromise; | ||
| } | ||
|
|
||
| function certToString(cert: string | Uint8Array) { | ||
| return typeof cert === 'string' | ||
| ? cert | ||
| : Buffer.from(cert.buffer, cert.byteOffset, cert.byteLength).toString( | ||
| 'utf8' | ||
| ); | ||
| } | ||
|
|
||
| export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string { | ||
| const ca = new Set<string>(); | ||
| for (const item of args) { | ||
| if (!item) continue; | ||
| const caList = Array.isArray(item) ? item : [item]; | ||
| for (const cert of caList) { | ||
| ca.add(certToString(cert)); | ||
| } | ||
| } | ||
| return [...ca].join('\n'); | ||
| } | ||
|
|
||
| export type ParsedX509Cert = { pem: string; parsed: X509Certificate | null }; | ||
|
|
||
| /** | ||
| * Safely parse provided certs, push any encountered errors to the provided | ||
| * messages array | ||
| */ | ||
| export function parseCACerts( | ||
|
There was a problem hiding this comment. Split parsing out of removing function so that I can parse once, then optionally remove the ones with missing issuer, then use the same parsed certs when sorting without loosing the parsed certs inside the "removing" function |
||
| ca: NodeJSCAOption, | ||
| messages: string[] | ||
| ): ParsedX509Cert[] { | ||
| ca = Array.isArray(ca) ? ca : [ca]; | ||
| return ca.map((cert) => { | ||
| const pem = certToString(cert); | ||
| let parsed: X509Certificate | null = null; | ||
| try { | ||
| parsed = new X509Certificate(pem); | ||
|
|
@@ -89,23 +78,84 @@ export function removeCertificatesWithoutIssuer(ca: string[]): { | |
| } | ||
| return { pem, parsed }; | ||
| }); | ||
| } | ||
|
|
||
| function doesCertificateHasMatchingIssuer( | ||
|
There was a problem hiding this comment. grammar nit: have, not has :) There was a problem hiding this comment. so either |
||
| { parsed }: ParsedX509Cert, | ||
| certs: ParsedX509Cert[] | ||
| ) { | ||
| return ( | ||
| !parsed || | ||
|
There was a problem hiding this comment. So if parsed is falsey we return that it does have a matching issuer? There was a problem hiding this comment. I suppose that's old code. There was a problem hiding this comment. Yes, I'm just keeping it as-is, but FWIW Annas reasoning in the PR that introduced it seems fair to me. I'll add it as a comment here so that we don't need to track down the PR every time There was a problem hiding this comment. Ended up moving the |
||
| parsed.checkIssued(parsed) || | ||
| certs.some(({ parsed: issuer }) => { | ||
| return issuer && parsed.checkIssued(issuer); | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const withRemovedMissingIssuerCache = new WeakMap< | ||
| ParsedX509Cert[], | ||
| { | ||
| ca: ParsedX509Cert[]; | ||
| 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: ParsedX509Cert[], | ||
| messages: string[] | ||
| ): ParsedX509Cert[] { | ||
| const result: | ||
| | { | ||
| ca: ParsedX509Cert[]; | ||
| messages: string[]; | ||
| } | ||
| | undefined = withRemovedMissingIssuerCache.get(ca); | ||
|
|
||
| if (result) { | ||
| messages.push(...result.messages); | ||
| return result.ca; | ||
| } | ||
|
|
||
| const _messages: string[] = []; | ||
| const filteredCAlist = ca.filter((cert) => { | ||
| const keep = doesCertificateHasMatchingIssuer(cert, ca); | ||
| if (!keep && cert.parsed) { | ||
| const { parsed } = cert; | ||
| _messages.push( | ||
| `Removing certificate for '${parsed.subject}' because issuer '${parsed.issuer}' could not be found (serial no '${parsed.serialNumber}')` | ||
| ); | ||
| } | ||
| return keep; | ||
| }); | ||
| withRemovedMissingIssuerCache.set(ca, { | ||
| ca: filteredCAlist, | ||
| messages: _messages, | ||
| }); | ||
| messages.push(..._messages); | ||
| return filteredCAlist; | ||
| } | ||
|
|
||
| /** | ||
| * Sorts cerificates by the Not After value. Items that are higher in the list | ||
| * get picked up first by the CA issuer finding logic | ||
| * | ||
| * @see {@link https://jira.mongodb.org/browse/COMPASS-8322} | ||
| */ | ||
| export function sortByExpirationDate(ca: ParsedX509Cert[]) { | ||
| return ca.slice().sort((a, b) => { | ||
| if (!a.parsed || !b.parsed) { | ||
| return 0; | ||
| } | ||
| return ( | ||
| new Date(b.parsed.validTo).getTime() - | ||
| new Date(a.parsed.validTo).getTime() | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| // Thin wrapper around system-ca, which merges: | ||
|
|
@@ -135,11 +185,14 @@ export async function systemCA( | |
|
|
||
| let systemCertsError: Error | undefined; | ||
| let asyncFallbackError: Error | undefined; | ||
| let systemCerts: ParsedX509Cert[] = []; | ||
|
|
||
| const messages: string[] = []; | ||
|
|
||
| try { | ||
| const systemCertsResult = await systemCertsCached(); | ||
| asyncFallbackError = systemCertsResult.asyncFallbackError; | ||
| systemCerts = parseCACerts(systemCertsResult.certs, messages); | ||
| } catch (err: any) { | ||
| systemCertsError = err; | ||
| } | ||
|
|
@@ -150,14 +203,14 @@ export async function systemCA( | |
| !!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER | ||
| ) | ||
| ) { | ||
| systemCerts = removeCertificatesWithoutIssuer(systemCerts, messages); | ||
| } | ||
|
|
||
| return { | ||
| ca: mergeCA( | ||
|
There was a problem hiding this comment. Not something I'm planning to pursue right now, but assuming openssl doesn't fail when using the same CA list that we used to repro the issue because it is doing the sorting somewhere internally and Node.js doesn't, it seems like we might want to sort all the options combined, and not only the ones coming from system CA, but I'm not feeling confident enough to do this change right now, would like to confirm exactly why this is not happening when trying the same connection with openssl directly and not with Node.js tls module |
||
| sortByExpirationDate(systemCerts).map((cert) => { | ||
| return cert.pem; | ||
| }), | ||
| rootCertificates, | ||
| existingOptions.ca, | ||
| await readTLSCAFilePromise | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cache is still here, just got moved down with the function and renamed to clarify the purpose