From 030a412dd6ac92118683e1e4bcd5c293cc5ac8c0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 10 Dec 2024 09:26:27 -0500 Subject: [PATCH] fix(devtools-connect): re-try connection in case of TLS errors without system CA MONGOSH-1935 (#495) Recent user reports have made it clear that our attempts to solve TLS errors caused by adding system certificates haven't fully been resolved yet. To address this, we add a generic fallback that attempts to connect a second time without the system certificate store added. This commit also adds TLS errors to the list of fail-fast errors, i.e. errors which should result in a quick connection end because they are unlikely to be resolved by the Node.js driver attempting to re-connect repeatedly on the level of individual connections. This should avoid situations in which timeouts make the connection attempt take twice as long for TLS errors. --- packages/devtools-connect/src/connect.spec.ts | 48 +++++++++ packages/devtools-connect/src/connect.ts | 101 +++++++++++++----- .../src/fast-failure-connect.ts | 81 +++++++++++++- packages/devtools-connect/src/index.ts | 1 + packages/devtools-connect/src/log-hook.ts | 16 +++ packages/devtools-connect/src/types.ts | 8 ++ 6 files changed, 226 insertions(+), 29 deletions(-) diff --git a/packages/devtools-connect/src/connect.spec.ts b/packages/devtools-connect/src/connect.spec.ts index 8e3c88fc..567fe42a 100644 --- a/packages/devtools-connect/src/connect.spec.ts +++ b/packages/devtools-connect/src/connect.spec.ts @@ -356,6 +356,54 @@ describe('devtools connect', function () { expect(mClient.connect.getCalls()).to.have.lengthOf(1); expect(result.client).to.equal(mClient); }); + + describe('retryable TLS errors', function () { + it('retries TLS errors without system CA integration enabled', async function () { + const uri = 'localhost:27017'; + const mClient = new FakeMongoClient(); + const mClientType = sinon.stub().returns(mClient); + const failure = new (Error as any)('blahblah', { + cause: Object.assign(new Error('self-signed cert'), { + code: 'SELF_SIGNED_CERT_IN_CHAIN', + }), + }); + let earlyFailures = 0; + bus.on('devtools-connect:connect-fail-early', () => earlyFailures++); + + mClient.connect = sinon.stub().callsFake(async () => { + await new Promise(setImmediate); + mClient.emit('serverHeartbeatFailed', { + failure, + connectionId: uri, + }); + await new Promise(setImmediate); + throw failure; + }); + mClient.topology = { + description: { + servers: new Map([['localhost:27017', {}]]), + }, + }; + + try { + await connectMongoClient(uri, defaultOpts, bus, mClientType as any); + expect.fail('missed exception'); + } catch (err) { + expect(err).to.equal(failure); + } + + expect(mClientType.getCalls()).to.have.lengthOf(2); + expect( + Object.keys(mClientType.getCalls()[0].args[1]).sort() + ).to.deep.equal(['allowPartialTrustChain', 'ca', 'lookup']); + expect( + Object.keys(mClientType.getCalls()[1].args[1]).sort() + ).to.deep.equal(['allowPartialTrustChain', 'lookup']); + expect((mClient as any).connect.getCalls()).to.have.lengthOf(2); + + expect(earlyFailures).to.equal(2); + }); + }); }); describe('integration', function () { diff --git a/packages/devtools-connect/src/connect.ts b/packages/devtools-connect/src/connect.ts index de3fb9b4..1c0c0b61 100644 --- a/packages/devtools-connect/src/connect.ts +++ b/packages/devtools-connect/src/connect.ts @@ -1,6 +1,9 @@ import type { ConnectLogEmitter } from './index'; import dns from 'dns'; -import { isFastFailureConnectionError } from './fast-failure-connect'; +import { + isFastFailureConnectionError, + isPotentialTLSCertificateError, +} from './fast-failure-connect'; import type { MongoClient, MongoClientOptions, @@ -318,7 +321,7 @@ export class DevtoolsConnectionState { 'productDocsLink' | 'productName' | 'oidc' | 'parentHandle' >, logger: ConnectLogEmitter, - ca: string + ca: string | undefined ) { this.productName = options.productName; if (options.parentHandle) { @@ -339,7 +342,7 @@ export class DevtoolsConnectionState { null, options ), - ...addToOIDCPluginHttpOptions(options.oidc, { ca }), + ...addToOIDCPluginHttpOptions(options.oidc, ca ? { ca } : {}), }); } } @@ -397,6 +400,11 @@ export interface DevtoolsConnectOptions extends MongoClientOptions { applyProxyToOIDC?: boolean; } +export type ConnectMongoClientResult = { + client: MongoClient; + state: DevtoolsConnectionState; +}; + /** * Connect a MongoClient. If AutoEncryption is requested, first connect without the encryption options and verify that * the connection is to an enterprise cluster. If not, then error, otherwise close the connection and reconnect with the @@ -407,35 +415,73 @@ export async function connectMongoClient( clientOptions: DevtoolsConnectOptions, logger: ConnectLogEmitter, MongoClientClass: typeof MongoClient -): Promise<{ - client: MongoClient; - state: DevtoolsConnectionState; -}> { +): Promise { + detectAndLogMissingOptionalDependencies(logger); + + const options = { uri, clientOptions, logger, MongoClientClass }; + // Connect once with the system certificate store added, and if that fails with + // a TLS error, try again. In theory adding certificates into the certificate store + // should not cause failures, but in practice we have observed some, hence this + // double connection establishment logic. + // We treat TLS errors as fail-fast errors, so in typical situations (even typical + // failure situations) we do not spend an unreasonable amount of time in the first + // connection attempt. + try { + return await connectMongoClientImpl({ ...options, useSystemCA: true }); + } catch (error: unknown) { + if (isPotentialTLSCertificateError(error)) { + logger.emit('devtools-connect:retry-after-tls-error', { + error: String(error), + }); + try { + return await connectMongoClientImpl({ ...options, useSystemCA: false }); + } catch {} + } + throw error; + } +} + +async function connectMongoClientImpl({ + uri, + clientOptions, + logger, + MongoClientClass, + useSystemCA, +}: { + uri: string; + clientOptions: DevtoolsConnectOptions; + logger: ConnectLogEmitter; + MongoClientClass: typeof MongoClient; + useSystemCA: boolean; +}): Promise { const cleanupOnClientClose: (() => void | Promise)[] = []; const runClose = async () => { let item: (() => void | Promise) | undefined; while ((item = cleanupOnClientClose.shift())) await item(); }; - detectAndLogMissingOptionalDependencies(logger); + let ca: string | undefined; try { - 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, - }); + if (useSystemCA) { + const { + ca: caWithSystemCerts, + 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, + }); + ca = caWithSystemCerts; + } // Create a proxy agent, if requested. `useOrCreateAgent()` takes a target argument // that can be used to select a proxy for a specific procotol or host; @@ -448,7 +494,7 @@ export async function connectMongoClient( ? clientOptions.proxy : { ...(clientOptions.proxy as DevtoolsProxyOptions), - ca, + ...(ca ? { ca } : {}), }, clientOptions.applyProxyToOIDC ? undefined : 'mongodb://' ); @@ -498,7 +544,8 @@ export async function connectMongoClient( {}, clientOptions, shouldAddOidcCallbacks ? state.oidcPlugin.mongoClientOptions : {}, - { ca, allowPartialTrustChain: true } + { allowPartialTrustChain: true }, + ca ? { ca } : {} ); // Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458. diff --git a/packages/devtools-connect/src/fast-failure-connect.ts b/packages/devtools-connect/src/fast-failure-connect.ts index 767507a9..39c6c130 100644 --- a/packages/devtools-connect/src/fast-failure-connect.ts +++ b/packages/devtools-connect/src/fast-failure-connect.ts @@ -1,7 +1,9 @@ // It probably makes sense to put this into its own package/repository once // other tools start using it. -export function isFastFailureConnectionError(error: Error): boolean { +function isFastFailureConnectionSingleError( + error: Error & { code?: string } +): boolean { switch (error.name) { case 'MongoNetworkError': return /\b(ECONNREFUSED|ENOTFOUND|ENETUNREACH|EINVAL)\b/.test( @@ -10,6 +12,81 @@ export function isFastFailureConnectionError(error: Error): boolean { case 'MongoError': return /The apiVersion parameter is required/.test(error.message); default: - return false; + return ( + ['ECONNREFUSED', 'ENOTFOUND', 'ENETUNREACH', 'EINVAL'].includes( + error.code ?? '' + ) || isPotentialTLSCertificateError(error) + ); } } + +export const isFastFailureConnectionError = handleNestedErrors( + isFastFailureConnectionSingleError +); + +function isPotentialTLSCertificateSingleError( + error: Error & { code?: string } +): boolean { + // https://nodejs.org/api/tls.html#x509-certificate-error-codes + return [ + 'UNABLE_TO_GET_ISSUER_CERT', + 'UNABLE_TO_GET_CRL', + 'UNABLE_TO_DECRYPT_CERT_SIGNATURE', + 'UNABLE_TO_DECRYPT_CRL_SIGNATURE', + 'UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY', + 'CERT_SIGNATURE_FAILURE', + 'CRL_SIGNATURE_FAILURE', + 'CERT_NOT_YET_VALID', + 'CERT_HAS_EXPIRED', + 'CRL_NOT_YET_VALID', + 'CRL_HAS_EXPIRED', + 'ERROR_IN_CERT_NOT_BEFORE_FIELD', + 'ERROR_IN_CERT_NOT_AFTER_FIELD', + 'ERROR_IN_CRL_LAST_UPDATE_FIELD', + 'ERROR_IN_CRL_NEXT_UPDATE_FIELD', + 'OUT_OF_MEM', + 'DEPTH_ZERO_SELF_SIGNED_CERT', + 'SELF_SIGNED_CERT_IN_CHAIN', + 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY', + 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', + 'CERT_CHAIN_TOO_LONG', + 'CERT_REVOKED', + 'INVALID_CA', + 'PATH_LENGTH_EXCEEDED', + 'INVALID_PURPOSE', + 'CERT_UNTRUSTED', + 'CERT_REJECTED', + 'HOSTNAME_MISMATCH', + ].includes(error.code ?? ''); +} + +export const isPotentialTLSCertificateError = handleNestedErrors( + isPotentialTLSCertificateSingleError +); + +// Convenience wrapper that ensures that the given error is an `Error` instance +// and that handles nested errors (.cause/AggregateError) as well +function handleNestedErrors( + fn: (err: Error & { code?: string }) => boolean +): (err: unknown) => boolean { + const checker = (err: unknown): boolean => { + if ( + Object.prototype.toString.call(err).toLowerCase() !== '[object error]' + ) { + return checker(new Error(String(err))); + } + if (!err || typeof err !== 'object') return false; // Make TS happy + if ('cause' in err && checker(err.cause)) { + return true; // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause + } + if ( + 'errors' in err && + Array.isArray(err.errors) && + err.errors.some((err) => checker(err)) + ) { + return true; // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError + } + return fn(err as Error); + }; + return checker; +} diff --git a/packages/devtools-connect/src/index.ts b/packages/devtools-connect/src/index.ts index 489987e5..99a6f365 100644 --- a/packages/devtools-connect/src/index.ts +++ b/packages/devtools-connect/src/index.ts @@ -5,6 +5,7 @@ export type { DevtoolsConnectionState, DevtoolsProxyOptions, AgentWithInitialize, + ConnectMongoClientResult, } from './connect'; export { hookLogger } from './log-hook'; export { oidcServerRequestHandler } from './oidc/handler'; diff --git a/packages/devtools-connect/src/log-hook.ts b/packages/devtools-connect/src/log-hook.ts index 392227f4..2b1d67bb 100644 --- a/packages/devtools-connect/src/log-hook.ts +++ b/packages/devtools-connect/src/log-hook.ts @@ -8,6 +8,7 @@ import type { ConnectMissingOptionalDependencyEvent, ConnectUsedSystemCAEvent, ConnectLogEmitter, + ConnectRetryAfterTLSErrorEvent, } from './types'; import { hookLoggerToMongoLogWriter as oidcHookLogger } from '@mongodb-js/oidc-plugin'; @@ -174,4 +175,19 @@ export function hookLogger( ); } ); + + emitter.on( + 'devtools-connect:retry-after-tls-error', + function (ev: ConnectRetryAfterTLSErrorEvent) { + log.info( + 'DEVTOOLS-CONNECT', + mongoLogId(1_000_000_054), + `${contextPrefix}-connect`, + 'Restarting connection attempt after TLS error', + { + error: ev.error, + } + ); + } + ); } diff --git a/packages/devtools-connect/src/types.ts b/packages/devtools-connect/src/types.ts index 16f4c7a9..acb04cda 100644 --- a/packages/devtools-connect/src/types.ts +++ b/packages/devtools-connect/src/types.ts @@ -58,6 +58,10 @@ export interface ConnectUsedSystemCAEvent { messages: string[]; } +export interface ConnectRetryAfterTLSErrorEvent { + error: string; +} + export interface ConnectEventMap extends MongoDBOIDCLogEventsMap, ProxyEventMap { @@ -93,6 +97,10 @@ export interface ConnectEventMap ) => void; /** Signals that the list of system certificates has been loaded and used for connecting. */ 'devtools-connect:used-system-ca': (ev: ConnectUsedSystemCAEvent) => void; + /** Signals that a connection attempt was restarted after a TLS error */ + 'devtools-connect:retry-after-tls-error': ( + ev: ConnectRetryAfterTLSErrorEvent + ) => void; } export type ConnectEventArgs =