Skip to content

Commit

Permalink
chore(devtools-proxy-support): remove CA transformation hacks COMPASS…
Browse files Browse the repository at this point in the history 
…-8253 (#485)

We now have `allowPartialTrustChain` available in both Compass and mongosh
and consequently do not need to keep these hacks around anymore.
  • Loading branch information
@addaleax
addaleax authored Nov 1, 2024
1 parent 7aeaac8 commit 9f2ff40
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 290 deletions.
7 changes: 1 addition & 6 deletions packages/devtools-proxy-support/src/agent.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type { Server as TLSServer } from 'tls';
import { createServer as createTLSServer } from 'tls';
import { promises as fs } from 'fs';
import type { AddressInfo } from 'net';
import { tlsSupportsAllowPartialTrustChainFlag } from './system-ca';

describe('createAgent', function () {
let setup: HTTPServerProxyTestSetup;
Expand Down Expand Up @@ -393,11 +392,7 @@ q/I2+0j6dAkOGcK/68z7qQXByeGri3n28a1Kn6o=
});

it('can connect using partial trust chains in the system CA list', async function () {
if (
process.platform !== 'linux' ||
!tlsSupportsAllowPartialTrustChainFlag()
)
return this.skip(); // only really mock-able on Linux
if (process.platform !== 'linux') return this.skip(); // only really mock-able on Linux
resetSystemCACache({
env: {
SSL_CERT_FILE: path.join(fixtures, 'ca.pem'),
Expand Down
131 changes: 0 additions & 131 deletions packages/devtools-proxy-support/src/system-ca.spec.ts
View file

This file was deleted.

157 changes: 4 additions & 153 deletions packages/devtools-proxy-support/src/system-ca.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ 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)[];
Expand Down Expand Up @@ -51,131 +50,6 @@ export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string {
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(
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);
} catch (err: unknown) {
// Most definitely should happen never or extremely rarely, in case it
// does, if this cert will affect the TLS connection verification, the
// connection will most definitely fail and we'll see it in the logs. For
// that reason we're just logging, but not throwing an error here
messages.push(
`Unable to parse certificate: ${
err && typeof err === 'object' && 'message' in err
? String(err.message)
: String(err)
}`
);
}
return { pem, parsed };
});
}

function certificateHasMatchingIssuer(
cert: X509Certificate,
certs: ParsedX509Cert[]
) {
return (
cert.checkIssued(cert) ||
certs.some(({ parsed: issuer }) => {
return issuer && cert.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) => {
// If cert was not parsed, we want to keep it in the list. The case should
// be generally very rare, but in case it happens and this cert will affect
// the TLS handshake, it will show up in the logs as the connection error
// anyway, so it's safe to keep it
const keep = !cert.parsed || certificateHasMatchingIssuer(cert.parsed, 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()
);
});
}

const nodeVersion = process.versions.node.split('.').map(Number);

export function tlsSupportsAllowPartialTrustChainFlag(): boolean {
// TODO: Remove this flag and all X.509 parsing here once all our products
// are at least on these Node.js versions
return (
(nodeVersion[0] >= 22 && nodeVersion[1] >= 9) || // https://github.com/nodejs/node/commit/c2bf0134c
(nodeVersion[0] === 20 && nodeVersion[1] >= 18)
); // https://github.com/nodejs/node/commit/1b3420274
}

// Thin wrapper around system-ca, which merges:
// - Explicit CA options passed as options
// - The Node.js TLS root store
Expand All @@ -184,8 +58,7 @@ export async function systemCA(
existingOptions: {
ca?: NodeJSCAOption;
tlsCAFile?: string | null | undefined;
} = {},
allowCertificatesWithoutIssuer?: boolean // defaults to false
} = {}
): Promise<{
ca: string;
systemCACount: number;
Expand All @@ -203,43 +76,21 @@ export async function systemCA(

let systemCertsError: Error | undefined;
let asyncFallbackError: Error | undefined;
let systemCerts: ParsedX509Cert[] = [];
let systemCerts: string[] = [];

const messages: string[] = [];

const _tlsSupportsAllowPartialTrustChainFlag =
tlsSupportsAllowPartialTrustChainFlag();
try {
const systemCertsResult = await systemCertsCached();
asyncFallbackError = systemCertsResult.asyncFallbackError;
if (_tlsSupportsAllowPartialTrustChainFlag) {
systemCerts = systemCertsResult.certs.map((pem) => ({
pem,
parsed: null,
}));
} else {
systemCerts = parseCACerts(systemCertsResult.certs, messages);
}
systemCerts = systemCertsResult.certs;
} catch (err: any) {
systemCertsError = err;
}

if (
!(
allowCertificatesWithoutIssuer ??
!!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER
) &&
!_tlsSupportsAllowPartialTrustChainFlag
) {
systemCerts = removeCertificatesWithoutIssuer(systemCerts, messages);
}

return {
ca: mergeCA(
(_tlsSupportsAllowPartialTrustChainFlag
? systemCerts
: sortByExpirationDate(systemCerts)
).map(({ pem }) => pem),
systemCerts,
rootCertificates,
existingOptions.ca,
await readTLSCAFilePromise
Expand Down

0 comments on commit 9f2ff40

Please sign in to comment.