-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
fix(devtools-proxy-support): remove certificates without issuer from system CA list COMPASS-8252 #467
fix(devtools-proxy-support): remove certificates without issuer from system CA list COMPASS-8252 #467
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import { expect } from 'chai'; | ||
import { removeCertificatesWithoutIssuer } from './system-ca'; | ||
|
||
describe('removeCertificatesWithoutIssuer', function () { | ||
it('removes certificates that do not have an issuer', function () { | ||
const certs = [ | ||
`-----BEGIN CERTIFICATE----- | ||
MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAwTzELMAkG | ||
A1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2VhcmNoIEdyb3VwMRUw | ||
EwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4WhcNMzUwNjA0MTEwNDM4WjBP | ||
MQswCQYDVQQGEwJVUzEpMCcGA1UEChMgSW50ZXJuZXQgU2VjdXJpdHkgUmVzZWFyY2ggR3Jv | ||
dXAxFTATBgNVBAMTDElTUkcgUm9vdCBYMTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoC | ||
ggIBAK3oJHP0FDfzm54rVygch77ct984kIxuPOZXoHj3dcKi/vVqbvYATyjb3miGbESTtrFj | ||
/RQSa78f0uoxmyF+0TM8ukj13Xnfs7j/EvEhmkvBioZxaUpmZmyPfjxwv60pIgbz5MDmgK7i | ||
S4+3mX6UA5/TR5d8mUgjU+g4rk8Kb4Mu0UlXjIB0ttov0DiNewNwIRt18jA8+o+u3dpjq+sW | ||
T8KOEUt+zwvo/7V3LvSye0rgTBIlDHCNAymg4VMk7BPZ7hm/ELNKjD+Jo2FR3qyHB5T0Y3Hs | ||
LuJvW5iB4YlcNHlsdu87kGJ55tukmi8mxdAQ4Q7e2RCOFvu396j3x+UCB5iPNgiV5+I3lg02 | ||
dZ77DnKxHZu8A/lJBdiB3QW0KtZB6awBdpUKD9jf1b0SHzUvKBds0pjBqAlkd25HN7rOrFle | ||
aJ1/ctaJxQZBKT5ZPt0m9STJEadao0xAH0ahmbWnOlFuhjuefXKnEgV4We0+UXgVCwOPjdAv | ||
BbI+e0ocS3MFEvzG6uBQE3xDk3SzynTnjh8BCNAw1FtxNrQHusEwMFxIt4I7mKZ9YIqioymC | ||
zLq9gwQbooMDQaHWBfEbwrbwqHyGO0aoSCqI3Haadr8faqU9GY/rOPNk3sgrDQoo//fb4hVC | ||
1CLQJ13hef4Y53CIrU7m2Ys6xt0nUW7/vGT1M0NPAgMBAAGjQjBAMA4GA1UdDwEB/wQEAwIB | ||
BjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBR5tFnme7bl5AFzgAiIyBpY9umbbjANBgkq | ||
hkiG9w0BAQsFAAOCAgEAVR9YqbyyqFDQDLHYGmkgJykIrGF1XIpu+ILlaS/V9lZLubhzEFnT | ||
IZd+50xx+7LSYK05qAvqFyFWhfFQDlnrzuBZ6brJFe+GnY+EgPbk6ZGQ3BebYhtF8GaV0nxv | ||
wuo77x/Py9auJ/GpsMiu/X1+mvoiBOv/2X/qkSsisRcOj/KKNFtY2PwByVS5uCbMiogziUwt | ||
hDyC3+6WVwW6LLv3xLfHTjuCvjHIInNzktHCgKQ5ORAzI4JMPJ+GslWYHb4phowim57iaztX | ||
OoJwTdwJx4nLCgdNbOhdjsnvzqvHu7UrTkXWStAmzOVyyghqpZXjFaH3pO3JLF+l+/+sKAIu | ||
vtd7u+Nxe5AW0wdeRlN8NwdCjNPElpzVmbUq4JUagEiuTDkHzsxHpFKVK7q4+63SM1N95R1N | ||
bdWhscdCb+ZAJzVcoyi3B43njTOQ5yOf+1CceWxG1bQVs5ZufpsMljq4Ui0/1lvh+wjChP4k | ||
qKOJ2qxq4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA | ||
mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57demyPxgcY | ||
xn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc= | ||
-----END CERTIFICATE-----`, | ||
`-----BEGIN CERTIFICATE----- | ||
MIIFYDCCBEigAwIBAgIQQAF3ITfU6UK47naqPGQKtzANBgkqhkiG9w0BAQsFADA/ | ||
MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT | ||
DkRTVCBSb290IENBIFgzMB4XDTIxMDEyMDE5MTQwM1oXDTI0MDkzMDE4MTQwM1ow | ||
TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh | ||
cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwggIiMA0GCSqGSIb3DQEB | ||
AQUAA4ICDwAwggIKAoICAQCt6CRz9BQ385ueK1coHIe+3LffOJCMbjzmV6B493XC | ||
ov71am72AE8o295ohmxEk7axY/0UEmu/H9LqMZshftEzPLpI9d1537O4/xLxIZpL | ||
wYqGcWlKZmZsj348cL+tKSIG8+TA5oCu4kuPt5l+lAOf00eXfJlII1PoOK5PCm+D | ||
LtFJV4yAdLbaL9A4jXsDcCEbdfIwPPqPrt3aY6vrFk/CjhFLfs8L6P+1dy70sntK | ||
4EwSJQxwjQMpoOFTJOwT2e4ZvxCzSow/iaNhUd6shweU9GNx7C7ib1uYgeGJXDR5 | ||
bHbvO5BieebbpJovJsXQEOEO3tkQjhb7t/eo98flAgeYjzYIlefiN5YNNnWe+w5y | ||
sR2bvAP5SQXYgd0FtCrWQemsAXaVCg/Y39W9Eh81LygXbNKYwagJZHduRze6zqxZ | ||
Xmidf3LWicUGQSk+WT7dJvUkyRGnWqNMQB9GoZm1pzpRboY7nn1ypxIFeFntPlF4 | ||
FQsDj43QLwWyPntKHEtzBRL8xurgUBN8Q5N0s8p0544fAQjQMNRbcTa0B7rBMDBc | ||
SLeCO5imfWCKoqMpgsy6vYMEG6KDA0Gh1gXxG8K28Kh8hjtGqEgqiNx2mna/H2ql | ||
PRmP6zjzZN7IKw0KKP/32+IVQtQi0Cdd4Xn+GOdwiK1O5tmLOsbdJ1Fu/7xk9TND | ||
TwIDAQABo4IBRjCCAUIwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYw | ||
SwYIKwYBBQUHAQEEPzA9MDsGCCsGAQUFBzAChi9odHRwOi8vYXBwcy5pZGVudHJ1 | ||
c3QuY29tL3Jvb3RzL2RzdHJvb3RjYXgzLnA3YzAfBgNVHSMEGDAWgBTEp7Gkeyxx | ||
+tvhS5B1/8QVYIWJEDBUBgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEEAYLfEwEB | ||
ATAwMC4GCCsGAQUFBwIBFiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2VuY3J5cHQu | ||
b3JnMDwGA1UdHwQ1MDMwMaAvoC2GK2h0dHA6Ly9jcmwuaWRlbnRydXN0LmNvbS9E | ||
U1RST09UQ0FYM0NSTC5jcmwwHQYDVR0OBBYEFHm0WeZ7tuXkAXOACIjIGlj26Ztu | ||
MA0GCSqGSIb3DQEBCwUAA4IBAQAKcwBslm7/DlLQrt2M51oGrS+o44+/yQoDFVDC | ||
5WxCu2+b9LRPwkSICHXM6webFGJueN7sJ7o5XPWioW5WlHAQU7G75K/QosMrAdSW | ||
9MUgNTP52GE24HGNtLi1qoJFlcDyqSMo59ahy2cI2qBDLKobkx/J3vWraV0T9VuG | ||
WCLKTVXkcGdtwlfFRjlBz4pYg1htmf5X6DYO8A4jqv2Il9DjXA6USbW1FzXSLr9O | ||
he8Y4IWS6wY7bCkjCWDcRQJMEhg76fsO3txE+FiYruq9RUWhiF1myv4Q6W+CyBFC | ||
Dfvp7OOGAN6dEOM4+qR9sdjoSYKEBpsr6GtPAQw4dy753ec5 | ||
-----END CERTIFICATE-----`, | ||
]; | ||
expect(removeCertificatesWithoutIssuer(certs)).to.deep.equal({ | ||
ca: [certs[0]], | ||
messages: [ | ||
`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')`, | ||
], | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import { systemCertsAsync } from 'system-ca'; | |
import type { Options as SystemCAOptions } from 'system-ca'; | ||
import { promises as fs } from 'fs'; | ||
import { rootCertificates } from 'tls'; | ||
import { X509Certificate } from 'crypto'; | ||
|
||
// A bit more generic than SecureContextOptions['ca'] because of Uint8Array -> Buffer + readonly | ||
type NodeJSCAOption = string | Uint8Array | readonly (string | Uint8Array)[]; | ||
|
@@ -50,6 +51,54 @@ export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string { | |
return [...ca].join('\n'); | ||
} | ||
|
||
const pemWithParsedCache = new WeakMap< | ||
string[], | ||
{ pem: string; parsed: X509Certificate | null }[] | ||
>(); | ||
// 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[]; | ||
} { | ||
const messages: string[] = []; | ||
let caWithParsedCerts = | ||
pemWithParsedCache.get(ca) ?? | ||
ca.map((pem) => { | ||
let parsed: X509Certificate | null = null; | ||
try { | ||
parsed = new X509Certificate(pem); | ||
} catch (err: unknown) { | ||
messages.push( | ||
`Unable to parse certificate: ${ | ||
err && typeof err === 'object' && 'message' in err | ||
? String(err.message) | ||
: String(err) | ||
}` | ||
); | ||
} | ||
return { pem, parsed }; | ||
}); | ||
pemWithParsedCache.set(ca, caWithParsedCerts); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! Done in 4b60c71 |
||
caWithParsedCerts = caWithParsedCerts.filter(({ parsed }) => { | ||
const keep = | ||
!parsed || | ||
Comment on lines
+93
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
parsed.checkIssued(parsed) || | ||
caWithParsedCerts.find( | ||
({ parsed: issuer }) => issuer && parsed.checkIssued(issuer) | ||
); | ||
if (!keep) { | ||
messages.push( | ||
`Removing certificate for '${parsed.subject}' because issuer '${parsed.issuer}' could not be found (serial no '${parsed.serialNumber}')` | ||
); | ||
} | ||
return keep; | ||
}); | ||
return { ca: caWithParsedCerts.map(({ pem }) => pem), messages }; | ||
} | ||
|
||
// Thin wrapper around system-ca, which merges: | ||
// - Explicit CA options passed as options | ||
// - The Node.js TLS root store | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😄 |
||
): Promise<{ | ||
ca: string; | ||
systemCACount: number; | ||
asyncFallbackError?: Error; | ||
systemCertsError?: Error; | ||
messages: string[]; | ||
}> { | ||
let readTLSCAFilePromise: Promise<string> | undefined; | ||
if (existingOptions.tlsCAFile) { | ||
|
@@ -76,13 +127,25 @@ export async function systemCA( | |
let systemCertsError: Error | undefined; | ||
let asyncFallbackError: Error | undefined; | ||
let systemCerts: string[] = []; | ||
let messages: string[] = []; | ||
|
||
try { | ||
({ certs: systemCerts, asyncFallbackError } = await systemCertsCached()); | ||
} catch (err: any) { | ||
systemCertsError = err; | ||
} | ||
|
||
if ( | ||
!( | ||
allowCertificatesWithoutIssuer ?? | ||
!!process.env.DEVTOOLS_ALLOW_CERTIFICATES_WITHOUT_ISSUER | ||
) | ||
) { | ||
const reducedList = removeCertificatesWithoutIssuer(systemCerts); | ||
systemCerts = reducedList.ca; | ||
messages = messages.concat(reducedList.messages); | ||
} | ||
|
||
return { | ||
ca: mergeCA( | ||
systemCerts, | ||
|
@@ -93,5 +156,6 @@ export async function systemCA( | |
asyncFallbackError: asyncFallbackError, | ||
systemCertsError, | ||
systemCACount: systemCerts.length + rootCertificates.length, | ||
messages, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forerr
withany
, but other than that are there downsides?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, you pretty much got it – it's not more than a general tendency to avoid
any
whenever we can 🙂