fix(devtools-proxy-support): remove certificates without issuer from system CA list COMPASS-8252 #467
Conversation
…system CA list COMPASS-8252 We have discovered that system certificate stores may contain certificates that a) OpenSSL may choose over other roots for the same certificate chain and b) are not accompanied by their own root certificate, i.e. only present an immediate certificate. While we will ideally solve this by using `X509_V_FLAG_PARTIAL_CHAIN`, Node.js does not expose that as a feature yet.
| @@ -58,12 +107,14 @@ export async function systemCA( | |||
| existingOptions: { | |||
| ca?: NodeJSCAOption; | |||
| tlsCAFile?: string | null | undefined; | |||
| } = {} | |||
| } = {}, | |||
| allowCertificatesWithoutIssuer?: boolean // defaults to false | |||
There was a problem hiding this comment.
Do we want to also expose this in connect method options or is the idea that we'd allow to activate it with the env var?
There was a problem hiding this comment.
If we wanted to expose it the idea is that we'd forward it all the way to the user in the UI, right?
I am pretty hopeful that this won't be necessary, and it's a temporary solution anyway. So yeah, the environment variable would be the way to get back to the current (i.e. "broken" 1.44) behavior if somebody does end up needing that for some reason
There was a problem hiding this comment.
Ohhhhh, wait, I totally misread the variable name and thought it's the other way around, okay yeah, not exposing it at all makes sense 😄
| `Unable to parse certificate: ${ | ||
| err && typeof err === 'object' && 'message' in err | ||
| ? String(err.message) | ||
| : String(err) | ||
| }` |
There was a problem hiding this comment.
No strong opinion here and mostly out of curiosity, can this be rewritten more concisely as: Unable to parse certificate: ${ err?.message || err }? I guess it will require replacing the type for err with any, but other than that are there downsides?
There was a problem hiding this comment.
I guess it will require replacing the type for
errwithany, but other than that are there downsides?
Nah, you pretty much got it – it's not more than a general tendency to avoid any whenever we can 🙂
| const keep = | ||
| !parsed || |
There was a problem hiding this comment.
This is somewhat counterintuitive to me - if we were unable to parse the certificate, do we want to keep it? I guess there's no harm in doing it, but do we feel it's useful to hold on to pems that are not valid x509 certificates?
There was a problem hiding this comment.
I don't think we should ever encounter a situation like this in the first place. But if we do, I think this is the "safer" path – if nothing else, it aligns more closely with the previous behavior (and the one we're aiming for in the long term, once this code is removed again). So I think just leaving a diagnostic message here is probably okay?
| } | ||
| return { pem, parsed }; | ||
| }); | ||
| pemWithParsedCache.set(ca, caWithParsedCerts); |
There was a problem hiding this comment.
Likely inconsequential, but for my understanding, why are we storing the unfiltered certificates in the cache rather than the filtered ones? My expectation is that the result of the filter will always be the same, so we might as well cache that rather than do the filtering every time.
There was a problem hiding this comment.
Good idea! Done in 4b60c71
We have discovered that system certificate stores may contain certificates that a) OpenSSL may choose over other roots for the same certificate chain and b) are not accompanied by their own root certificate, i.e. only present an immediate certificate.
While we will ideally solve this by using
X509_V_FLAG_PARTIAL_CHAIN, Node.js does not expose that as a feature yet.Description
Open Questions
Checklist