Skip to content

chore(devtools-proxy-support): push expired certs to the bottom of the system CA list COMPASS-8322 #474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 24, 2024
Merged

chore(devtools-proxy-support): push expired certs to the bottom of the system CA list COMPASS-8322 #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bfffacd
chore(devtools-proxy-support): push expired certs to the bottom of th…
gribnoysup Sep 20, 2024
30c3ab1
Merge branch 'main' into compass-8322-sort-by-expired
gribnoysup Sep 23, 2024
aa51a92
chore(proxy-support): naming fixes and more comments
gribnoysup Sep 24, 2024
8dbd70f
chore: fix typo
gribnoysup Sep 24, 2024
8dc11dd
chore: update test description
gribnoysup Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 68 additions & 11 deletions packages/devtools-proxy-support/src/system-ca.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import { expect } from 'chai';
import { removeCertificatesWithoutIssuer } from './system-ca';
import type { ParsedX509Cert } from './system-ca';
import {
parseCACerts,
removeCertificatesWithoutIssuer,
sortByExpirationDate,
} from './system-ca';

describe('removeCertificatesWithoutIssuer', function () {
it('removes certificates that do not have an issuer', function () {
const certs = [
`-----BEGIN CERTIFICATE-----
describe('system-ca helpers', function () {
describe('removeCertificatesWithoutIssuer', function () {
it('removes certificates that do not have an issuer', function () {
const certs = [
`-----BEGIN CERTIFICATE-----
MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAwTzELMAkG
A1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2VhcmNoIEdyb3VwMRUw
EwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4WhcNMzUwNjA0MTEwNDM4WjBP
Expand Down Expand Up @@ -32,7 +38,7 @@ qKOJ2qxq4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA
mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57demyPxgcY
xn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=
-----END CERTIFICATE-----`,
`-----BEGIN CERTIFICATE-----
`-----BEGIN CERTIFICATE-----
MIIFYDCCBEigAwIBAgIQQAF3ITfU6UK47naqPGQKtzANBgkqhkiG9w0BAQsFADA/
MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT
DkRTVCBSb290IENBIFgzMB4XDTIxMDEyMDE5MTQwM1oXDTI0MDkzMDE4MTQwM1ow
Expand Down Expand Up @@ -63,12 +69,63 @@ WCLKTVXkcGdtwlfFRjlBz4pYg1htmf5X6DYO8A4jqv2Il9DjXA6USbW1FzXSLr9O
he8Y4IWS6wY7bCkjCWDcRQJMEhg76fsO3txE+FiYruq9RUWhiF1myv4Q6W+CyBFC
Dfvp7OOGAN6dEOM4+qR9sdjoSYKEBpsr6GtPAQw4dy753ec5
-----END CERTIFICATE-----`,
];
expect(removeCertificatesWithoutIssuer(certs)).to.deep.equal({
ca: [certs[0]],
messages: [
];
const messages = [];
const filtered = removeCertificatesWithoutIssuer(
parseCACerts(certs, messages),
messages
);
expect(
filtered.map((cert) => {
return cert.pem;
})
).to.deep.equal([certs[0]]);
expect(messages).to.deep.eq([
`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')`,
],
]);
});
});

describe('sortByExpirationDate', function () {
it('sorts certs by expiration date in descending order (higher expiration date on top)', function () {
const mockCerts = [
{
serialNumber: '01',
validTo: new Date(Date.now() + 10_000).toUTCString(),
},
{
serialNumber: '02',
validTo: new Date(Date.now() - 60_000).toUTCString(),
},
{
serialNumber: '03',
validTo: new Date(Date.now() - 50_000).toUTCString(),
},
{
serialNumber: '04',
validTo: new Date(Date.now() + 30_000).toUTCString(),
},
{
serialNumber: '05',
validTo: new Date(Date.now() + 20_000).toUTCString(),
},
{
serialNumber: '06',
validTo: new Date(Date.now() + 20_000).toUTCString(),
},
];

const sorted = sortByExpirationDate(
mockCerts.map((parsed) => {
return { pem: '', parsed } as ParsedX509Cert;
})
);

expect(
sorted.map((cert) => {
return cert.parsed?.serialNumber;
})
).to.deep.eq(['04', '05', '06', '01', '03', '02']);
});
});
});
164 changes: 112 additions & 52 deletions packages/devtools-proxy-support/src/system-ca.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,54 +31,47 @@ 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: readonly (string | Uint8Array)[] = Array.isArray(item)
? item
: [item];
const caList = Array.isArray(item) ? item : [item];
for (const cert of caList) {
const asString =
typeof cert === 'string'
? cert
: Buffer.from(cert.buffer, cert.byteOffset, cert.byteLength).toString(
'utf8'
);
ca.add(asString);
ca.add(certToString(cert));
}
}
return [...ca].join('\n');
}

const pemWithParsedCache = new WeakMap<
Copy link
Collaborator Author

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

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);
export type ParsedX509Cert = { pem: string; parsed: X509Certificate | null };

const messages: string[] = [];
let caWithParsedCerts = ca.map((pem) => {
/**
* Safely parse provided certs, push any encountered errors to the provided
* messages array
*/
export function parseCACerts(
Copy link
Collaborator Author

@gribnoysup gribnoysup Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
} 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
Expand All @@ -89,23 +82,87 @@ export function removeCertificatesWithoutIssuer(ca: string[]): {
}
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(
}

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

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;
});
result = { ca: caWithParsedCerts.map(({ pem }) => pem), messages };
pemWithParsedCache.set(ca, result);
return result;
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:
Expand Down Expand Up @@ -135,11 +192,14 @@ export async function systemCA(

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

const messages: string[] = [];

try {
({ certs: systemCerts, asyncFallbackError } = await systemCertsCached());
const systemCertsResult = await systemCertsCached();
asyncFallbackError = systemCertsResult.asyncFallbackError;
systemCerts = parseCACerts(systemCertsResult.certs, messages);
} catch (err: any) {
systemCertsError = err;
}
Expand All @@ -150,14 +210,14 @@ export async function systemCA(
!!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER
)
) {
const reducedList = removeCertificatesWithoutIssuer(systemCerts);
systemCerts = reducedList.ca;
messages = messages.concat(reducedList.messages);
systemCerts = removeCertificatesWithoutIssuer(systemCerts, messages);
}

return {
ca: mergeCA(
Copy link
Collaborator Author

@gribnoysup gribnoysup Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

systemCerts,
sortByExpirationDate(systemCerts).map((cert) => {
return cert.pem;
}),
rootCertificates,
existingOptions.ca,
await readTLSCAFilePromise
Expand Down
Loading