chore(devtools-proxy-support): push expired certs to the bottom of the system CA list COMPASS-8322 #474
Conversation
| } | ||
| } | ||
| return [...ca].join('\n'); | ||
| } | ||
|
|
||
| const pemWithParsedCache = new WeakMap< |
There was a problem hiding this comment.
This cache is still here, just got moved down with the function and renamed to clarify the purpose
| * 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
gribnoysup
force-pushed
the
compass-8322-sort-by-expired
branch
from
c58fbfd to
bfffacd
Compare
| const reducedList = removeCertificatesWithoutIssuer(systemCerts); | ||
| systemCerts = reducedList.ca; | ||
| messages = messages.concat(reducedList.messages); | ||
| 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
| messages.push( | ||
| } | ||
|
|
||
| function doesCertificateHasMatchingIssuer( |
There was a problem hiding this comment.
grammar nit: have, not has :)
There was a problem hiding this comment.
so either doesCertificateHaveMatchingIssuer or certificateHasMatchingIssuer.
| 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 check outside of the method just so this function name clearly describes and matches what it does. Moved the not parsed check to the filter itself and added a comment, seems a bit cleaner that way?
| // 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); |
We ran into more weird issues with CA cert validation failing in unexpected cases (see COMPASS-8322 for more details) affecting users because system CAs are now added to the CA list by default. To work around this particular one we're sorting a list of CAs returned from system store by expiration date to make sure that the ones that will expire sooner or expired already have less of a chance to get picked up from the CA list