From 2c19f7a27aa7f4df795c8c5f9659d20cc37792c2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 15:28:44 -0600 Subject: [PATCH 1/3] fix(tls): secure-port UDS metadata read certificates off the wrong server when a plain TCP port was also registered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit onSocket() built the secure (TLS) server into the function-scoped `socketServer` binding, and the UDS metadata-write closure captured that binding. A caller registering BOTH ports in one server.socket() call — which MQTT does by default ({ port: 1883, securePort: 8883 }) — then reached the plain-TCP branch, which reassigned `socketServer` to the 1883 server. Every secure-port metadata write (the boot-time .ready.then() and every later rebuild's listener fan-out) therefore read `secureContexts` off the plain TCP server (undefined) and published an empty `certificates:` list, deterministically, on every worker of every node with both MQTT ports enabled. A fronting SNI-routing proxy (Symphony) that selects certificates from that metadata then falls back to the node certificate for every custom-domain SNI on 8883 — the customer-visible symptom that survived #1999/#2005. Give the secure server its own const and capture that in the closure; `socketServer` remains the branch-shared return value. The selector, its certificate map, and the publish/retry logic were always healthy (verified live: in-memory maps fully populated while the disk yaml stayed empty), which is why the selector-focused fixes and tests in #1999/#2005/#2008 could not catch this: every existing test drove createTLSSelector directly with a pseudo-server. The new regression test goes through server.socket() with both ports — the real wiring — and asserts the written yaml carries certificates; it fails against the unfixed code at exactly that assertion. Root-caused via deterministic local reproduction on v5.1.25 (fresh default-config install: all -8883.yaml empty, -9926.yaml populated; instrumented dist showed the 8883 write firing with secureContexts=undefined; one-line dist patch produced fully populated metadata on reboot). Fixes #1998 --- server/threads/threadServer.js | 19 ++++++---- unitTests/apiTests/mqtt-test.mjs | 61 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/server/threads/threadServer.js b/server/threads/threadServer.js index 0e90d9d1c1..c6088ee75d 100644 --- a/server/threads/threadServer.js +++ b/server/threads/threadServer.js @@ -494,7 +494,13 @@ function onSocket(listener, options) { setPortServerMap(options.securePort, { protocol_name: 'TLS', name: getComponentName() }); const SNICallback = createTLSSelector('server', options.mtls); const tlsConfig = env.get('tls'); - socketServer = createSecureSocketServer( + // Own const, NOT the shared `socketServer` binding: a caller registering both ports (MQTT's + // port + securePort) reaches the plain-TCP branch below, which reassigns `socketServer` to + // the 1883 server. The writeMetadata closure below outlives this function, so capturing the + // mutable binding made every secure-port metadata write read `secureContexts` off the plain + // TCP server (undefined) and publish an empty `certificates:` list — the #1998 bug that let + // an SNI-routing proxy (Symphony) fall back to the node certificate on 8883. + const secureSocketServer = createSecureSocketServer( { rejectUnauthorized: Boolean(options.mtls?.required), requestCert: Boolean(options.mtls), @@ -508,14 +514,15 @@ function onSocket(listener, options) { }, listener ); - SNICallback.initialize(socketServer); + socketServer = secureSocketServer; + SNICallback.initialize(secureSocketServer); // Only opt out of reusePort on macOS, which doesn't reliably support SO_REUSEPORT on all // socket types (ENOTSUP). Everywhere else, sharing the port lets every worker accept // connections for this listener (e.g. MQTT), matching how HTTP servers are bound; without // it only the first worker to bind serves the port and every sibling's listen() fails with // a silently-swallowed EADDRINUSE. - if (process.platform === 'darwin') socketServer.noReusePort = true; - SERVERS[options.securePort] = socketServer; + if (process.platform === 'darwin') secureSocketServer.noReusePort = true; + SERVERS[options.securePort] = secureSocketServer; // Create a corresponding Unix Domain Socket mirror for the secure socket if (env.get(terms.CONFIG_PARAMS.TLS_UNIXDOMAINSOCKETS)) { @@ -539,9 +546,9 @@ function onSocket(listener, options) { SERVERS[udsPath] = udsServer; httpComponent.registerUdsCleanupPaths(udsPath, yamlPath); - const writeMetadata = () => httpComponent.writeUdsMetadata(yamlPath, options.securePort, socketServer); + const writeMetadata = () => httpComponent.writeUdsMetadata(yamlPath, options.securePort, secureSocketServer); SNICallback.ready.then(writeMetadata); - socketServer.secureContextsListeners.push(writeMetadata); + secureSocketServer.secureContextsListeners.push(writeMetadata); } } if (options.port) { diff --git a/unitTests/apiTests/mqtt-test.mjs b/unitTests/apiTests/mqtt-test.mjs index 09b281f6bb..4149f6bb70 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -1109,6 +1109,67 @@ describe('test MQTT connections and commands', function () { } }); + it('secure-port UDS metadata carries the certificates when the same socket() call also registers a plain TCP port', async function () { + // Regression for #1998's surviving symptom: MQTT registers `{ port, securePort }` in ONE + // server.socket() call. onSocket built the TLS server into the function-scoped `socketServer` + // binding, the metadata-write closure captured that binding, and the plain-TCP branch then + // reassigned it to the port-only server — so every secure-port metadata write read + // `secureContexts` off the TCP server (undefined) and published an EMPTY `certificates:` + // list. A fronting SNI proxy (Symphony on 8883) then served the node certificate for every + // custom-domain SNI, fleet-wide, deterministically. The selector itself was always healthy, + // which is why selector-level tests (which drive createTLSSelector with a pseudo-server and + // never go through onSocket with both ports) missed it four review rounds in a row — this + // test goes through the real wiring and asserts the artifact a proxy actually consumes. + this.timeout(10000); + const { existsSync, readFileSync: readFile, unlinkSync } = await import('node:fs'); + const { join } = await import('node:path'); + const preexistingServers = { ...SERVERS }; + const preexistingPortServer = new Map([...portServer.entries()].map(([key, servers]) => [key, [...servers]])); + const preexistingUds = env_get('tls_unixDomainSockets'); + setProperty('tls_unixDomainSockets', true); + const securePort = 28887; + const socketsDir = join(environmentManager.getHdbBasePath(), 'sockets'); + try { + global.server.socket(() => {}, { port: 21887, securePort }); + // The yaml is written when the TLS selector's `.ready` resolves (or on a later rebuild); + // poll for a yaml for our securePort that actually carries certificates. + const deadline = Date.now() + 8000; + let yamlPath; + let content = ''; + while (Date.now() < deadline) { + const candidates = existsSync(socketsDir) + ? (await import('node:fs')).readdirSync(socketsDir).filter((name) => name.endsWith(`-${securePort}.yaml`)) + : []; + if (candidates.length > 0) { + yamlPath = join(socketsDir, candidates[0]); + content = readFile(yamlPath, 'utf8'); + if (content.includes('BEGIN CERTIFICATE')) break; + } + await delay(100); + } + assert.ok(yamlPath, `a -${securePort}.yaml should be written to ${socketsDir}`); + assert.ok( + content.includes('BEGIN CERTIFICATE'), + 'the secure-port UDS metadata must carry the certificate list even when the same socket() call ' + + 'also registered a plain TCP port — an empty `certificates:` list here is exactly what makes an ' + + `SNI-routing proxy fall back to the node certificate (got:\n${content.slice(0, 200)})` + ); + } finally { + setProperty('tls_unixDomainSockets', preexistingUds); + for (const name of existsSync(socketsDir) + ? (await import('node:fs')).readdirSync(socketsDir).filter((n) => n.includes(`-${securePort}.`)) + : []) { + try { + unlinkSync(join(socketsDir, name)); + } catch {} + } + for (const key of Object.keys(SERVERS)) delete SERVERS[key]; + Object.assign(SERVERS, preexistingServers); + portServer.clear(); + for (const [key, servers] of preexistingPortServer) portServer.set(key, servers); + } + }); + after(() => { clientV4?.end(); clientV5?.end(); From 93bb6cecad9bf00c71748de3b4a756e480e1d6f7 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 15:34:04 -0600 Subject: [PATCH 2/3] test: address quick-review findings on the UDS-metadata regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Destructure readdirSync once instead of re-importing node:fs per use. - Unlink any yaml a crashed prior run left for this port before polling — a stale populated file would false-pass even if the current write regressed. - Note the (pre-existing, shared-with-sibling-tests) liveReload selector registration that onSocket exposes no teardown for. --- unitTests/apiTests/mqtt-test.mjs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/unitTests/apiTests/mqtt-test.mjs b/unitTests/apiTests/mqtt-test.mjs index 4149f6bb70..4e3f33c2c8 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -1121,7 +1121,7 @@ describe('test MQTT connections and commands', function () { // never go through onSocket with both ports) missed it four review rounds in a row — this // test goes through the real wiring and asserts the artifact a proxy actually consumes. this.timeout(10000); - const { existsSync, readFileSync: readFile, unlinkSync } = await import('node:fs'); + const { existsSync, readFileSync: readFile, readdirSync, unlinkSync } = await import('node:fs'); const { join } = await import('node:path'); const preexistingServers = { ...SERVERS }; const preexistingPortServer = new Map([...portServer.entries()].map(([key, servers]) => [key, [...servers]])); @@ -1130,6 +1130,17 @@ describe('test MQTT connections and commands', function () { const securePort = 28887; const socketsDir = join(environmentManager.getHdbBasePath(), 'sockets'); try { + // Clear any yaml a crashed prior run left behind — a stale populated file would false-pass + // the poll below even if the current write regressed. + if (existsSync(socketsDir)) { + for (const name of readdirSync(socketsDir).filter((n) => n.includes(`-${securePort}.`))) { + try { + unlinkSync(join(socketsDir, name)); + } catch {} + } + } + // Note: like the sibling socket tests above, this leaves the selector's liveReload + // registration in keys.ts's module-global rebuild set — onSocket exposes no teardown. global.server.socket(() => {}, { port: 21887, securePort }); // The yaml is written when the TLS selector's `.ready` resolves (or on a later rebuild); // poll for a yaml for our securePort that actually carries certificates. @@ -1138,7 +1149,7 @@ describe('test MQTT connections and commands', function () { let content = ''; while (Date.now() < deadline) { const candidates = existsSync(socketsDir) - ? (await import('node:fs')).readdirSync(socketsDir).filter((name) => name.endsWith(`-${securePort}.yaml`)) + ? readdirSync(socketsDir).filter((name) => name.endsWith(`-${securePort}.yaml`)) : []; if (candidates.length > 0) { yamlPath = join(socketsDir, candidates[0]); @@ -1157,7 +1168,7 @@ describe('test MQTT connections and commands', function () { } finally { setProperty('tls_unixDomainSockets', preexistingUds); for (const name of existsSync(socketsDir) - ? (await import('node:fs')).readdirSync(socketsDir).filter((n) => n.includes(`-${securePort}.`)) + ? readdirSync(socketsDir).filter((n) => n.includes(`-${securePort}.`)) : []) { try { unlinkSync(join(socketsDir, name)); From abed331a7736c5ccf86d1365f7d497ad4cd070ac Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 16:11:17 -0600 Subject: [PATCH 3/3] test: close the servers the regression test creates (reviewer suggestion) Rules out any timing pressure on later tests in the same mocha process from the unclosed 21887/28887/UDS-mirror servers, per Devin's note on the v22 audit-log flake analysis. --- unitTests/apiTests/mqtt-test.mjs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/unitTests/apiTests/mqtt-test.mjs b/unitTests/apiTests/mqtt-test.mjs index 4e3f33c2c8..c8a6f457c0 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -1167,6 +1167,15 @@ describe('test MQTT connections and commands', function () { ); } finally { setProperty('tls_unixDomainSockets', preexistingUds); + // Close the servers this call created (TLS 28887, TCP 21887, and the UDS mirror) before + // dropping them from the registry, so they can't exert timing pressure on later tests in + // this mocha process even if something bound them. + for (const key of Object.keys(SERVERS)) { + if (preexistingServers[key]) continue; + try { + SERVERS[key]?.close?.(() => {}); + } catch {} + } for (const name of existsSync(socketsDir) ? readdirSync(socketsDir).filter((n) => n.includes(`-${securePort}.`)) : []) {