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..c8a6f457c0 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -1109,6 +1109,87 @@ 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, 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]])); + const preexistingUds = env_get('tls_unixDomainSockets'); + setProperty('tls_unixDomainSockets', true); + 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. + const deadline = Date.now() + 8000; + let yamlPath; + let content = ''; + while (Date.now() < deadline) { + const candidates = existsSync(socketsDir) + ? 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); + // 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}.`)) + : []) { + 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();