fix(tls): keep rebuilding the SNI selector when a pass produces only a default context (empty per-hostname map) - #2008
fix(tls): keep rebuilding the SNI selector when a pass produces only a default context (empty per-hostname map)#2008Devin-Holland wants to merge 1 commit into
Conversation
…a default context A raw-socket TLS listener's certificate selector (MQTT's `network.securePort`) could complete a pass that set a default context but built an empty per-hostname `secureContexts` map, then resolve and stop. The `!defaultContextSetThisPass` guard treated "a default was set" as success, so the retry never re-armed. When the real per-domain certs later landed via a bulk/initial-sync path that does not fire the selector's `hdb_certificate` subscription (the same event bypass #1999 documented for table swaps), nothing re-triggered a rebuild: the listener kept exporting an empty `certificates:` list to its UDS metadata until restart, and a fronting proxy that routes by that list (Symphony on 8883) served the node certificate for every custom-domain SNI. The selector now re-checks on the debounce whenever the per-hostname map is empty, regardless of whether a default was produced. If a default exists it still resolves `.ready` (the listener serves via that default immediately, no plaintext hang), but the scheduled rebuild stays armed so the per-domain certs are picked up and the exported metadata is rewritten once they load. The zero-cert warn latch is only cleared on a populated pass so the empty-map-with-default fall-through does not re-warn on every re-check. Follow-up to #1999 / #2005 (which fixed the not-loaded and table-swap cases but left this default-set variant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Reviewed; no blockers found. |
![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 updates the TLS selector in security/keys.ts to continuously rebuild and retry when the per-hostname certificate map is empty, even if a default certificate context is set. This ensures SNI-routing proxies receive the per-hostname certificates once they load. A unit test was added to verify this behavior. The reviewer noted that in a stable 'default-only' setup, this change triggers an infinite loop of scheduled rebuilds, causing unnecessary CPU overhead, and provided a code suggestion to optimize this by checking for the existence of hostname-eligible certificates.
| if (server && !server.tlsSelectorWarnedZeroCerts) { | ||
| server.tlsSelectorWarnedZeroCerts = true; | ||
| logger.warn?.( | ||
| `TLS selector for the '${type}' listener resolved zero certificates; retrying every ${TLS_REBUILD_DEBOUNCE_MS}ms` | ||
| `TLS selector for the '${type}' listener resolved an empty per-hostname certificate map; retrying every ${TLS_REBUILD_DEBOUNCE_MS}ms` | ||
| ); | ||
| } | ||
| scheduleRebuild(); |
There was a problem hiding this comment.
For a stable, healthy "default-only" configuration (where only a default certificate with no hostnames is configured), secureContexts.size will always be 0 and defaultContextSetThisPass will be true. Under the current implementation, this triggers an infinite loop of scheduleRebuild() every 1.5 seconds, causing unnecessary background CPU overhead and periodic database searches on an idle system.
We can optimize this by checking if there are any other certificates with hostnames in the database. If none exist, we are in a stable default-only state and can safely skip scheduling a rebuild.
let hasHostnameCerts = false;
for (const cert of databases.system.hdb_certificate.search([])) {
if (cert.is_authority) continue;
try {
const certParsed = new X509Certificate(cert.certificate);
let hostnames = cert.hostnames ?? hostnamesFromCert(certParsed);
if (!Array.isArray(hostnames)) hostnames = [hostnames];
if (hostnames.some(h => h)) {
hasHostnameCerts = true;
break;
}
} catch {}
}
if (!defaultContextSetThisPass || hasHostnameCerts) {
if (server && !server.tlsSelectorWarnedZeroCerts) {
server.tlsSelectorWarnedZeroCerts = true;
logger.warn?.(
"TLS selector for the '" + type + "' listener resolved an empty per-hostname certificate map; retrying every " + TLS_REBUILD_DEBOUNCE_MS + "ms"
);
}
scheduleRebuild();
}
|
Root cause found — and it's upstream of this PR's mechanism. Reproduced locally on v5.1.25 (fresh default-config install, deterministic on first boot) and fix-verified both directions. The actual bug is in const writeMetadata = () => httpComponent.writeUdsMetadata(yamlPath, options.securePort, socketServer);Then the plain-TCP branch below reassigns This explains the full evidence set: it's deterministic fleet-wide (any node with both MQTT ports enabled — the default — has always exported empty 8883 metadata, hence the host-manager code comment treating that as known), prod's in-memory maps are populated while the disk yaml stays empty, and none of #1999's breadcrumbs ever fired (the selector was never unhealthy). Implication for this PR: the armed-retry here re-runs the same wrong-object write, so it won't change the published metadata on prod; the regression test passes because it drives — Claude (Fable 5), for Kris — repro artifacts: local v5.1.25 boot with instrumented dist, details in the FDE Slack thread. |
…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
…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
…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
…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
2 participants
Summary
Follow-up to #1999 / #2005. On an affected production cluster, port 8883 (MQTT-over-TLS, terminated by the Symphony proxy) still serves the node certificate for custom-domain SNIs on
harper-pro5.1.25 (which contains #1999). Verified live, read-only:sockets/<n>-8883.yamlis 42 bytes (emptycertificates:) while the<n>-9926mirror is the full ~82 KB;resolved zero certificates,waiting for system.hdb_certificate) ever fired (checkedhdb.loganddocker logs).The gap #1999 left
#1999 fixed the not-loaded and table-swap cases, but not a pass that ends with a default context set but an empty per-hostname
secureContextsmap. The guardsecureContexts.size === 0 && !defaultContextSetThisPasstreats "a default was set" as success and resolves-and-stops, so on the boot pass that produced the empty map, no retry was armed.What this changes (
security/keys.ts,createTLSSelector)Re-check on the debounce whenever the per-hostname map is empty, regardless of whether a default was produced:
.ready(serve via the default, no hang) but keep the scheduled rebuild armed so a later pass republishes the per-hostname list.Warn-latch resets are split so the empty-map-with-default fall-through doesn't re-warn on each re-check.
Live CDP inspection of a running node shows the in-memory 8883 selector maps ARE populated (all custom domains, write-listener registered), while the on-disk yaml is still the 42-byte boot artifact — and calling
writeUdsMetadataagainst the live map produces a correct yaml. So a rebuild did run and populate the map after the empty publish, yet the listener fan-out that should have rewritten the yaml didn't take effect. There may be a second defect between map-population and republish that this PR does not address — this PR arms and relies on that same fan-out.Discriminating test before treating this as the fix: on a plain-5.1.25 node, one benign
hdb_certificatewrite, then check thesockets/<n>-8883.yamlmtime + content:A lead worth checking there: in the raw-socket path (
threads/threadServer.js onSocket) the write-listener ispushed ontosecureContextsListenersafterSNICallback.initialize(...), so the first populating pass's fan-out can fire against a listener set that doesn't yet includewriteMetadata.Notes
secureContextsmembership).unitTests/security/keys.test.js55/55 locally; the new test fails without the fix (verified by reverting). Full suites via CI.Generated by Claude (Opus 4.8), investigation + fix driven with Devin.