Skip to content

Commit c58fbfd

Browse files
gribnoysupgribnoysup
committed
chore(devtools-proxy-support): push expired certs to the bottom of the system CA list
1 parent ca8aea2 commit c58fbfd

File tree

2 files changed

+182
-63
lines changed

2 files changed

+182
-63
lines changed

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

Lines changed: 64 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,59 @@ 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('pushes all expired certs to the bottom', function () {
91+
const mockCerts = [
92+
{
93+
serialNumber: '01',
94+
validTo: new Date(Date.now() + 60_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() - 60_000).toUTCString(),
103+
},
104+
{
105+
serialNumber: '04',
106+
validTo: new Date(Date.now() + 60_000).toUTCString(),
107+
},
108+
{
109+
serialNumber: '05',
110+
validTo: new Date(Date.now() + 60_000).toUTCString(),
111+
},
112+
];
113+
114+
const sorted = sortByExpirationDate(
115+
mockCerts.map((parsed) => {
116+
return { pem: '', parsed } as ParsedX509Cert;
117+
})
118+
);
119+
120+
expect(
121+
sorted.map((cert) => {
122+
return cert.parsed?.serialNumber;
123+
})
124+
).to.deep.eq(['01', '04', '05', '02', '03']);
72125
});
73126
});
74127
});

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

Lines changed: 118 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -31,50 +31,39 @@ 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);
@@ -89,23 +78,97 @@ export function removeCertificatesWithoutIssuer(ca: string[]): {
8978
}
9079
return { pem, parsed };
9180
});
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(
81+
}
82+
83+
function doesCertificateHasMatchingIssuer(
84+
{ parsed }: ParsedX509Cert,
85+
certs: ParsedX509Cert[]
86+
) {
87+
return (
88+
!parsed ||
89+
parsed.checkIssued(parsed) ||
90+
certs.some(({ parsed: issuer }) => {
91+
return issuer && parsed.checkIssued(issuer);
92+
})
93+
);
94+
}
95+
96+
const withRemovedMissingIssuerCache = new WeakMap<
97+
ParsedX509Cert[],
98+
{
99+
ca: ParsedX509Cert[];
100+
messages: string[];
101+
}
102+
>();
103+
104+
// TODO(COMPASS-8253): Remove this in favor of OpenSSL's X509_V_FLAG_PARTIAL_CHAIN
105+
// See linked tickets for details on why we need this (tl;dr: the system certificate
106+
// store may contain intermediate certficiates without the corresponding trusted root,
107+
// and OpenSSL does not seem to accept that)
108+
export function removeCertificatesWithoutIssuer(
109+
ca: ParsedX509Cert[],
110+
messages: string[]
111+
): ParsedX509Cert[] {
112+
const result:
113+
| {
114+
ca: ParsedX509Cert[];
115+
messages: string[];
116+
}
117+
| undefined = withRemovedMissingIssuerCache.get(ca);
118+
119+
if (result) {
120+
messages.push(...result.messages);
121+
return result.ca;
122+
}
123+
124+
const _messages: string[] = [];
125+
const filteredCAlist = ca.filter((cert) => {
126+
const keep = doesCertificateHasMatchingIssuer(cert, ca);
127+
if (!keep && cert.parsed) {
128+
const { parsed } = cert;
129+
_messages.push(
101130
`Removing certificate for '${parsed.subject}' because issuer '${parsed.issuer}' could not be found (serial no '${parsed.serialNumber}')`
102131
);
103132
}
104133
return keep;
105134
});
106-
result = { ca: caWithParsedCerts.map(({ pem }) => pem), messages };
107-
pemWithParsedCache.set(ca, result);
108-
return result;
135+
withRemovedMissingIssuerCache.set(ca, {
136+
ca: filteredCAlist,
137+
messages: _messages,
138+
});
139+
messages.push(..._messages);
140+
return filteredCAlist;
141+
}
142+
143+
function hasExpired(cert: X509Certificate) {
144+
return new Date(cert.validTo).getTime() < Date.now();
145+
}
146+
147+
/**
148+
* Pushes all expired certs to the bottom of the list (wihout moving other
149+
* certs) to make sure that they are not accidentally picked up by (maybe)
150+
* openssl in some corner-cases
151+
*
152+
* @see {@link https://jira.mongodb.org/browse/COMPASS-8322}
153+
*/
154+
export function sortByExpirationDate(ca: ParsedX509Cert[]) {
155+
return ca.slice().sort((a, b) => {
156+
if (!a.parsed || !b.parsed) {
157+
return 0;
158+
}
159+
const aExpired = hasExpired(a.parsed);
160+
const bExpired = hasExpired(b.parsed);
161+
if (aExpired && bExpired) {
162+
return 0;
163+
}
164+
if (aExpired) {
165+
return 1;
166+
}
167+
if (bExpired) {
168+
return -1;
169+
}
170+
return 0;
171+
});
109172
}
110173

111174
// Thin wrapper around system-ca, which merges:
@@ -135,11 +198,14 @@ export async function systemCA(
135198

136199
let systemCertsError: Error | undefined;
137200
let asyncFallbackError: Error | undefined;
138-
let systemCerts: string[] = [];
139-
let messages: string[] = [];
201+
let systemCerts: ParsedX509Cert[] = [];
202+
203+
const messages: string[] = [];
140204

141205
try {
142-
({ certs: systemCerts, asyncFallbackError } = await systemCertsCached());
206+
const systemCertsResult = await systemCertsCached();
207+
asyncFallbackError = systemCertsResult.asyncFallbackError;
208+
systemCerts = parseCACerts(systemCertsResult.certs, messages);
143209
} catch (err: any) {
144210
systemCertsError = err;
145211
}
@@ -150,14 +216,14 @@ export async function systemCA(
150216
!!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER
151217
)
152218
) {
153-
const reducedList = removeCertificatesWithoutIssuer(systemCerts);
154-
systemCerts = reducedList.ca;
155-
messages = messages.concat(reducedList.messages);
219+
systemCerts = removeCertificatesWithoutIssuer(systemCerts, messages);
156220
}
157221

158222
return {
159223
ca: mergeCA(
160-
systemCerts,
224+
sortByExpirationDate(systemCerts).map((cert) => {
225+
return cert.pem;
226+
}),
161227
rootCertificates,
162228
existingOptions.ca,
163229
await readTLSCAFilePromise

0 commit comments

Comments
 (0)