[v5.1] Fix MQTT secure-port UDS metadata publishing an empty certificate list - #2011
Conversation
…rver when a plain TCP port was also registered
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 <n>-8883.yaml empty, <n>-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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a bug (#1998) where registering both a secure port and a plain TCP port in a single socket() call caused the secure-port UDS metadata to publish an empty certificate list. This occurred because the shared socketServer binding was reassigned to the plain TCP server, which was then captured by the writeMetadata closure. The fix introduces a local secureSocketServer constant to ensure the correct server instance is captured by the closure. Additionally, a regression test has been added to verify that the secure-port UDS metadata correctly includes certificates under this scenario. There are no review comments, so no further feedback is provided.
- 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.
|
Reviewed; no blockers found. |
Devin-Holland
left a comment
Devin-Holland
left a comment
There was a problem hiding this comment.
Approving — this is the one IBM needs
Independently verified
I re-derived the diagnosis from the code rather than taking the write-up on trust, and it holds — this is the mechanism, and it explains every observation that didn't add up before.
The reassignment really does outlive the closure. On the shipped v5.1.25 tag, onSocket: let socketServer (:492) → secure branch assigns it (:497) → const writeMetadata = () => writeUdsMetadata(yamlPath, options.securePort, socketServer) (:542) → secureContextsListeners.push(writeMetadata) (:544) → if (options.port) { … socketServer = createSocketServer(…) } (:547, :549). The closure captures the binding, the binding is rebound to the plain-TCP server, and both write paths (the .ready.then() microtask and every later listener fan-out) run after onSocket returns. secureContexts is undefined on that object, writeUdsMetadata gates on contexts?.size > 0, so it emits a bare certificates:.
The precondition is the shipped default, not a config edge case. static/defaultConfig.yaml ships mqtt.network.port: 1883 and securePort: 8883, so every default install passes both, hits the reassignment, and publishes an empty 8883 list on every worker. That's the "deterministically, fleet-wide" claim, confirmed.
And it explains why the HTTP mirror was always fine — which is the part I most wanted an independent structural reason for, rather than an absence of evidence. In http.ts the same closure shape is safe on both counts: const server = (httpServers[port] = …) (:529) is a const, and the port it closes over (:600) is a parameter of getHTTPServer(port, secure, options), never rebound. Two files, same pattern, one has a mutable binding and one doesn't, and the symptom tracked exactly that.
Class sweep — this was the only instance. Since a finding like this is usually a sample of a class, I scripted a hunt across threadServer.js and http.ts for the same shape (a function-scope let, reassigned, captured by a closure that outlives the function). Four other candidates surfaced and all resolve to false positives with reasons: threadServer.js listen_on is declared inside the loop body (per-iteration) and passed to .listen() synchronously; port at :43 is a different scope from the for (let port in …) loop variables captured at :430/:451/:465 (also per-iteration); listening is a local const shadowing the module-level let; http.ts's socket/response/body are request-scoped with synchronous consumers. socketServer was the real one.
Fix completeness. writeMetadata was the only deferred closure in onSocket, so converting it plus every synchronous use to secureSocketServer is the whole surface. SERVERS[securePort] gets the TLS server and SERVERS[port] the plain one, unchanged; socketServer = secureSocketServer preserves the return value for the securePort-only case, so return semantics are identical either way — and the MQTT caller doesn't consume the return anyway.
The test is the right test. Driving global.server.socket() with both ports and asserting the written yaml carries BEGIN CERTIFICATE targets the artifact the proxy actually consumes, which is exactly the seam every prior round missed by driving createTLSSelector with a pseudo-server. It runs in CI and passes (✔ … 103ms), so it's genuinely gating and not just present.
CI is fully green here: 46 pass, 1 skip (review), zero failures, and the new regression test passes under both Node versions (✔ … 103ms, 927 passing in the unit job). Notably v5.1's unit suite doesn't carry the auditLog flake that's reddening #2010 on main, so this backport has a clean signal end to end.
I also confirmed the v5.1 variant is equivalent to #2010 modulo the effectiveCiphers line v5.1 doesn't have — same two files, same closure fix, same test.
Given IBM is on 5.1.x and v5.1.25 shipped with the symptom still live, this is the one that actually reaches them. Not merging it — that's yours.
On my own miss
Worth recording, since I reviewed #1999 twice and pushed hard on the root cause. I got as far as proving the yaml had been written and therefore updateTLS() must have completed with an empty map — that part was right, and it's why the original subscribe-throws diagnosis was wrong. But I then enumerated "exactly two ways to complete a pass with an empty map" and both were selector-side: every cert failing the per-cert build, or the table having no rows. The third way — the map is perfectly healthy and writeUdsMetadata is simply handed the wrong object — never occurred to me, even though I had read writeUdsMetadata and grep-traced all three of its call sites. I verified that socketServer was passed and never asked what it was bound to at call time. Reading a call site isn't reading the value.
Still open after this (not blockers)
- The empty-map-with-a-default-context case that #2008 targeted is now unaddressed with it closed. Narrow (needs a cert whose hostnames resolve to
[]— no usable SANs and no CN) and a different trigger from the customer's, but with this fix the write reaches the right object and still finds an empty map, so it would still publish an empty list. Worth an issue rather than scope here. - #2004 (swap while
secureContextsis non-empty leaves a dead subscription with no armed retry) is unrelated to this and still stands.
No code changes requested — the absence of inline threads here is deliberate, not an omission. Really nice piece of debugging; the mapSize=3, defaultSet=true / ctxSize=undefined instrumentation pair is what turns this from a plausible story into a proven one.
Reviewed by Claude (Opus 5). Verification commands reproducible against the v5.1.25 tag and both branches.
|
Reviewed Verified the fix against the real code path: Note: did not execute the test suite locally to avoid touching the shared Harper instance; relying on the on-branch pass documented in the PR. — |
…ion) 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.
|
Reviewed The only change since — |
3 participants
v5.1 backport of #2010 (see it for the full root-cause analysis and evidence). Hand-resolved cherry-pick:
tlsConfig.ciphersline (nogetEffectiveTlsCipherson this branch) and omitted main-only property assignments (appliedCiphers,verifiesClientCerts,mtlsRequired,dedicatedListener);createSocketServerarg-order fix that v5.1 doesn't have.Regression test verified passing on this branch (
unitTests/apiTests/mqtt-test.mjs --grep "secure-port UDS metadata"); build/prettier/oxlint clean.This is the branch that actually reaches the affected customer (5.1.x) — proposing for the next 5.1 patch release once #2010 lands on main.
Generated by Claude (Fable 5).