Skip to content
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

Merged
merged 2 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions packages/devtools-connect/src/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,16 +419,22 @@ export async function connectMongoClient(
detectAndLogMissingOptionalDependencies(logger);

try {
const { ca, asyncFallbackError, systemCertsError, systemCACount } =
await systemCA({
ca: clientOptions.ca,
tlsCAFile:
clientOptions.tlsCAFile || getConnectionStringParam(uri, 'tlsCAFile'),
});
const {
ca,
asyncFallbackError,
systemCertsError,
systemCACount,
messages,
} = await systemCA({
ca: clientOptions.ca,
tlsCAFile:
clientOptions.tlsCAFile || getConnectionStringParam(uri, 'tlsCAFile'),
});
logger.emit('devtools-connect:used-system-ca', {
caCount: systemCACount,
asyncFallbackError,
systemCertsError,
messages,
});

// Create a proxy agent, if requested. `useOrCreateAgent()` takes a target argument
Expand Down
2 changes: 2 additions & 0 deletions packages/devtools-connect/src/log-hook.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ describe('Logging setup', function () {
caCount: 1234,
asyncFallbackError: new Error('had to fallback to sync'),
systemCertsError: new Error('could not load system certs at all'),
messages: ['A diagnostic message'],
});

await log.flush();
Expand Down Expand Up @@ -212,6 +213,7 @@ describe('Logging setup', function () {
caCount: 1234,
asyncFallbackError: 'had to fallback to sync',
systemCertsError: 'could not load system certs at all',
messages: ['A diagnostic message'],
},
},
]);
Expand Down
1 change: 1 addition & 0 deletions packages/devtools-connect/src/log-hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ export function hookLogger(
caCount: ev.caCount,
asyncFallbackError: ev.asyncFallbackError?.message,
systemCertsError: ev.systemCertsError?.message,
messages: ev.messages,
}
);
}
Expand Down
1 change: 1 addition & 0 deletions packages/devtools-connect/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export interface ConnectUsedSystemCAEvent {
caCount: number;
asyncFallbackError: Error | undefined;
systemCertsError: Error | undefined;
messages: string[];
}

export interface ConnectEventMap
Expand Down
74 changes: 74 additions & 0 deletions packages/devtools-proxy-support/src/system-ca.spec.ts
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')`,
],
});
});
});
75 changes: 74 additions & 1 deletion packages/devtools-proxy-support/src/system-ca.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)[];
Expand Down Expand Up @@ -50,6 +51,63 @@ export function mergeCA(...args: (NodeJSCAOption | undefined)[]): string {
return [...ca].join('\n');
}

const pemWithParsedCache = new WeakMap<
string[],
{
ca: string[];
messages: string[];
}
>();
// 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[];
} {
let result:
| {
ca: string[];
messages: string[];
}
| undefined = pemWithParsedCache.get(ca);

const messages: string[] = [];
let caWithParsedCerts = 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 };
});
caWithParsedCerts = caWithParsedCerts.filter(({ parsed }) => {
const keep =
!parsed ||
Comment on lines +93 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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;
});
result = { ca: caWithParsedCerts.map(({ pem }) => pem), messages };
pemWithParsedCache.set(ca, result);
return result;
}

// Thin wrapper around system-ca, which merges:
// - Explicit CA options passed as options
// - The Node.js TLS root store
Expand All @@ -58,12 +116,14 @@ export async function systemCA(
existingOptions: {
ca?: NodeJSCAOption;
tlsCAFile?: string | null | undefined;
} = {}
} = {},
allowCertificatesWithoutIssuer?: boolean // defaults to false
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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) {
Expand All @@ -76,13 +136,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,
Expand All @@ -93,5 +165,6 @@ export async function systemCA(
asyncFallbackError: asyncFallbackError,
systemCertsError,
systemCACount: systemCerts.length + rootCertificates.length,
messages,
};
}
Loading