Skip to content

Commit 3039562

Browse files
gribnoysupgribnoysup
authored
chore(devtools-proxy-support): push expired certs to the bottom of the system CA list COMPASS-8322 (#474)
* chore(devtools-proxy-support): push expired certs to the bottom of the system CA list * chore(proxy-support): naming fixes and more comments * chore: fix typo * chore: update test description
1 parent 52bb21c commit 3039562

File tree

2 files changed

+180
-63
lines changed

2 files changed

+180
-63
lines changed

‎packages/devtools-proxy-support/src/system-ca.spec.ts

Lines changed: 68 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
import { expect } from 'chai';
2-
import { removeCertificatesWithoutIssuer } from './system-ca';
2+
import type { ParsedX509Cert } from './system-ca';
3+
import {
4+
parseCACerts,
5+
removeCertificatesWithoutIssuer,
6+
sortByExpirationDate,
7+
} from './system-ca';
38

4-
describe('removeCertificatesWithoutIssuer', function () {
5-
it('removes certificates that do not have an issuer', function () {
6-
const certs = [
7-
`-----BEGIN CERTIFICATE-----
9+
describe('system-ca helpers', function () {
10+
describe('removeCertificatesWithoutIssuer', function () {
11+
it('removes certificates that do not have an issuer', function () {
12+
const certs = [
13+
`-----BEGIN CERTIFICATE-----
814
MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAwTzELMAkG
915
A1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2VhcmNoIEdyb3VwMRUw
1016
EwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4WhcNMzUwNjA0MTEwNDM4WjBP
@@ -32,7 +38,7 @@ qKOJ2qxq4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA
3238
mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57demyPxgcY
3339
xn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=
3440
-----END CERTIFICATE-----`,
35-
`-----BEGIN CERTIFICATE-----
41+
`-----BEGIN CERTIFICATE-----
3642
MIIFYDCCBEigAwIBAgIQQAF3ITfU6UK47naqPGQKtzANBgkqhkiG9w0BAQsFADA/
3743
MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT
3844
DkRTVCBSb290IENBIFgzMB4XDTIxMDEyMDE5MTQwM1oXDTI0MDkzMDE4MTQwM1ow
@@ -63,12 +69,63 @@ WCLKTVXkcGdtwlfFRjlBz4pYg1htmf5X6DYO8A4jqv2Il9DjXA6USbW1FzXSLr9O
6369
he8Y4IWS6wY7bCkjCWDcRQJMEhg76fsO3txE+FiYruq9RUWhiF1myv4Q6W+CyBFC
6470
Dfvp7OOGAN6dEOM4+qR9sdjoSYKEBpsr6GtPAQw4dy753ec5
6571
-----END CERTIFICATE-----`,
66-
];
67-
expect(removeCertificatesWithoutIssuer(certs)).to.deep.equal({
68-
ca: [certs[0]],
69-
messages: [
72+
];
73+
const messages = [];
74+
const filtered = removeCertificatesWithoutIssuer(
75+
parseCACerts(certs, messages),
76+
messages
77+
);
78+
expect(
79+
filtered.map((cert) => {
80+
return cert.pem;
81+
})
82+
).to.deep.equal([certs[0]]);
83+
expect(messages).to.deep.eq([
7084
`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')`,
71-
],
85+
]);
86+
});
87+
});
88+
89+
describe('sortByExpirationDate', function () {
90+
it('sorts certs by expiration date in descending order (higher expiration date on top)', function () {
91+
const mockCerts = [
92+
{
93+
serialNumber: '01',
94+
validTo: new Date(Date.now() + 10_000).toUTCString(),
95+
},
96+
{
97+
serialNumber: '02',
98+
validTo: new Date(Date.now() - 60_000).toUTCString(),
99+
},
100+
{
101+
serialNumber: '03',
102+
validTo: new Date(Date.now() - 50_000).toUTCString(),
103+
},
104+
{
105+
serialNumber: '04',
106+
validTo: new Date(Date.now() + 30_000).toUTCString(),
107+
},
108+
{
109+
serialNumber: '05',
110+
validTo: new Date(Date.now() + 20_000).toUTCString(),
111+
},
112+
{
113+
serialNumber: '06',
114+
validTo: new Date(Date.now() + 20_000).toUTCString(),
115+
},
116+
];
117+
118+
const sorted = sortByExpirationDate(
119+
mockCerts.map((parsed) => {
120+
return { pem: '', parsed } as ParsedX509Cert;
121+
})
122+
);
123+
124+
expect(
125+
sorted.map((cert) => {
126+
return cert.parsed?.serialNumber;
127+
})
128+
).to.deep.eq(['04', '05', '06', '01', '03', '02']);
72129
});
73130
});
74131
});

‎packages/devtools-proxy-support/src/system-ca.ts

Lines changed: 112 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -31,54 +31,47 @@ function systemCertsCached(systemCAOpts: SystemCAOptions = {}): Promise<{
3131
return systemCertsCachePromise;
3232
}
3333

34+
function certToString(cert: string | Uint8Array) {
35+
return typeof cert === 'string'
36+
? cert
37+
: Buffer.from(cert.buffer, cert.byteOffset, cert.byteLength).toString(
38+
'utf8'
39+
);
40+
}
41+
3442
export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string {
3543
const ca = new Set<string>();
3644
for (const item of args) {
3745
if (!item) continue;
38-
const caList: readonly (string | Uint8Array)[] = Array.isArray(item)
39-
? item
40-
: [item];
46+
const caList = Array.isArray(item) ? item : [item];
4147
for (const cert of caList) {
42-
const asString =
43-
typeof cert === 'string'
44-
? cert
45-
: Buffer.from(cert.buffer, cert.byteOffset, cert.byteLength).toString(
46-
'utf8'
47-
);
48-
ca.add(asString);
48+
ca.add(certToString(cert));
4949
}
5050
}
5151
return [...ca].join('\n');
5252
}
5353

54-
const pemWithParsedCache = new WeakMap<
55-
string[],
56-
{
57-
ca: string[];
58-
messages: string[];
59-
}
60-
>();
61-
// TODO(COMPASS-8253): Remove this in favor of OpenSSL's X509_V_FLAG_PARTIAL_CHAIN
62-
// See linked tickets for details on why we need this (tl;dr: the system certificate
63-
// store may contain intermediate certficiates without the corresponding trusted root,
64-
// and OpenSSL does not seem to accept that)
65-
export function removeCertificatesWithoutIssuer(ca: string[]): {
66-
ca: string[];
67-
messages: string[];
68-
} {
69-
let result:
70-
| {
71-
ca: string[];
72-
messages: string[];
73-
}
74-
| undefined = pemWithParsedCache.get(ca);
54+
export type ParsedX509Cert = { pem: string; parsed: X509Certificate | null };
7555

76-
const messages: string[] = [];
77-
let caWithParsedCerts = ca.map((pem) => {
56+
/**
57+
* Safely parse provided certs, push any encountered errors to the provided
58+
* messages array
59+
*/
60+
export function parseCACerts(
61+
ca: NodeJSCAOption,
62+
messages: string[]
63+
): ParsedX509Cert[] {
64+
ca = Array.isArray(ca) ? ca : [ca];
65+
return ca.map((cert) => {
66+
const pem = certToString(cert);
7867
let parsed: X509Certificate | null = null;
7968
try {
8069
parsed = new X509Certificate(pem);
8170
} catch (err: unknown) {
71+
// Most definitely should happen never or extremely rarely, in case it
72+
// does, if this cert will affect the TLS connection verification, the
73+
// connection will most definitely fail and we'll see it in the logs. For
74+
// that reason we're just logging, but not throwing an error here
8275
messages.push(
8376
`Unable to parse certificate: ${
8477
err && typeof err === 'object' && 'message' in err
@@ -89,23 +82,87 @@ export function removeCertificatesWithoutIssuer(ca: string[]): {
8982
}
9083
return { pem, parsed };
9184
});
92-
caWithParsedCerts = caWithParsedCerts.filter(({ parsed }) => {
93-
const keep =
94-
!parsed ||
95-
parsed.checkIssued(parsed) ||
96-
caWithParsedCerts.find(
97-
({ parsed: issuer }) => issuer && parsed.checkIssued(issuer)
98-
);
99-
if (!keep) {
100-
messages.push(
85+
}
86+
87+
function certificateHasMatchingIssuer(
88+
cert: X509Certificate,
89+
certs: ParsedX509Cert[]
90+
) {
91+
return (
92+
cert.checkIssued(cert) ||
93+
certs.some(({ parsed: issuer }) => {
94+
return issuer && cert.checkIssued(issuer);
95+
})
96+
);
97+
}
98+
99+
const withRemovedMissingIssuerCache = new WeakMap<
100+
ParsedX509Cert[],
101+
{
102+
ca: ParsedX509Cert[];
103+
messages: string[];
104+
}
105+
>();
106+
107+
// TODO(COMPASS-8253): Remove this in favor of OpenSSL's X509_V_FLAG_PARTIAL_CHAIN
108+
// See linked tickets for details on why we need this (tl;dr: the system certificate
109+
// store may contain intermediate certficiates without the corresponding trusted root,
110+
// and OpenSSL does not seem to accept that)
111+
export function removeCertificatesWithoutIssuer(
112+
ca: ParsedX509Cert[],
113+
messages: string[]
114+
): ParsedX509Cert[] {
115+
const result:
116+
| {
117+
ca: ParsedX509Cert[];
118+
messages: string[];
119+
}
120+
| undefined = withRemovedMissingIssuerCache.get(ca);
121+
122+
if (result) {
123+
messages.push(...result.messages);
124+
return result.ca;
125+
}
126+
127+
const _messages: string[] = [];
128+
const filteredCAlist = ca.filter((cert) => {
129+
// If cert was not parsed, we want to keep it in the list. The case should
130+
// be generally very rare, but in case it happens and this cert will affect
131+
// the TLS handshake, it will show up in the logs as the connection error
132+
// anyway, so it's safe to keep it
133+
const keep = !cert.parsed || certificateHasMatchingIssuer(cert.parsed, ca);
134+
if (!keep && cert.parsed) {
135+
const { parsed } = cert;
136+
_messages.push(
101137
`Removing certificate for '${parsed.subject}' because issuer '${parsed.issuer}' could not be found (serial no '${parsed.serialNumber}')`
102138
);
103139
}
104140
return keep;
105141
});
106-
result = { ca: caWithParsedCerts.map(({ pem }) => pem), messages };
107-
pemWithParsedCache.set(ca, result);
108-
return result;
142+
withRemovedMissingIssuerCache.set(ca, {
143+
ca: filteredCAlist,
144+
messages: _messages,
145+
});
146+
messages.push(..._messages);
147+
return filteredCAlist;
148+
}
149+
150+
/**
151+
* Sorts cerificates by the Not After value. Items that are higher in the list
152+
* get picked up first by the CA issuer finding logic
153+
*
154+
* @see {@link https://jira.mongodb.org/browse/COMPASS-8322}
155+
*/
156+
export function sortByExpirationDate(ca: ParsedX509Cert[]) {
157+
return ca.slice().sort((a, b) => {
158+
if (!a.parsed || !b.parsed) {
159+
return 0;
160+
}
161+
return (
162+
new Date(b.parsed.validTo).getTime() -
163+
new Date(a.parsed.validTo).getTime()
164+
);
165+
});
109166
}
110167

111168
// Thin wrapper around system-ca, which merges:
@@ -135,11 +192,14 @@ export async function systemCA(
135192

136193
let systemCertsError: Error | undefined;
137194
let asyncFallbackError: Error | undefined;
138-
let systemCerts: string[] = [];
139-
let messages: string[] = [];
195+
let systemCerts: ParsedX509Cert[] = [];
196+
197+
const messages: string[] = [];
140198

141199
try {
142-
({ certs: systemCerts, asyncFallbackError } = await systemCertsCached());
200+
const systemCertsResult = await systemCertsCached();
201+
asyncFallbackError = systemCertsResult.asyncFallbackError;
202+
systemCerts = parseCACerts(systemCertsResult.certs, messages);
143203
} catch (err: any) {
144204
systemCertsError = err;
145205
}
@@ -150,14 +210,14 @@ export async function systemCA(
150210
!!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER
151211
)
152212
) {
153-
const reducedList = removeCertificatesWithoutIssuer(systemCerts);
154-
systemCerts = reducedList.ca;
155-
messages = messages.concat(reducedList.messages);
213+
systemCerts = removeCertificatesWithoutIssuer(systemCerts, messages);
156214
}
157215

158216
return {
159217
ca: mergeCA(
160-
systemCerts,
218+
sortByExpirationDate(systemCerts).map((cert) => {
219+
return cert.pem;
220+
}),
161221
rootCertificates,
162222
existingOptions.ca,
163223
await readTLSCAFilePromise

0 commit comments

Comments
 (0)