feat(tls): give MQTT's raw-socket listener its own TLS usage type (split from #1999) - #2003
Merged
Conversation
Contributor
|
Reviewed; no blockers found. |
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a granular usageType configuration for socket listeners, migrating the MQTT listener to its own explicit 'mqtt' usage type. To maintain backward compatibility, a fallback mechanism (LEGACY_SERVER_FALLBACK_TYPES) is introduced, ensuring that legacy certificates tagged with uses: ['server'] continue to apply to MQTT listeners without incorrectly affecting other listener types like operations-api or replication. Unit tests have been added to verify this behavior. There are no review comments to evaluate, and I have no additional feedback to provide.
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
July 30, 2026 17:43
Member
|
Reviewed Traced the full path ( On the flagged quality-scoring caveat: preferring a CA-signed (Could not execute the unit tests locally — isolated worktree had no — |
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
…geType Addresses review on #2003: the resolveEffectiveTlsCiphers allowlist test only proved cipher/@SECLEVEL relevance, not that createTLSSelector's quality scoring actually prioritizes uses:['mqtt'] certs or that MQTT's raw-socket listener forwards usageType at all. Adds coverage for both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
createTLSSelector's updateTLS() accepts a falsy `server` everywhere else
in the function (`if (server) {...}`, `server?.secureContextsListeners`),
but one trace-log line dereffed `server.ports` unguarded. Harmless while
trace logging is disabled (logger.trace?.() short-circuits the argument
evaluation), but with it enabled every candidate throws inside the
per-cert try/catch before `defaultContext` gets assigned for that
candidate — surfaced by the #2003 review test additions, which are the
first callers of createTLSSelector(...).initialize(null) to assert on
`defaultContext`. One-character fix: `server?.ports`.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
Reviewed — |
A raw-socket TLS listener (onSocket, used by MQTT's network.securePort) always resolved its TLS usage type as the generic 'server', so a certificate tagged for a specific listener type (uses: ['mqtt']) never got its intended priority over a generic node certificate. onSocket() now accepts a usageType option (falling back to 'server') and MQTT passes 'mqtt', for both certificate quality scoring and effective cipher/@SECLEVEL resolution. Backward compat: certs tagged uses: ['server'] targeted MQTT under the old default, so 'server' earns the legacy generic-use credit for an explicit allowlist (LEGACY_SERVER_FALLBACK_TYPES, currently just 'mqtt'). It is an allowlist rather than "everything except operations-api" because every other existing type (operations-api, replication, ...) has always had its own dedicated identity and must not newly start accepting a ['server']-tagged record's ciphers. Split from #1999 per review feedback: this half is a no-op for stock deployments (Harper only writes uses: [] / ['operations-api'] / ['replication']) and is what made the v5.1 patch cherry-pick of the customer-facing readiness fix conflict, since v5.1 predates getEffectiveTlsCiphers. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…geType Addresses review on #2003: the resolveEffectiveTlsCiphers allowlist test only proved cipher/@SECLEVEL relevance, not that createTLSSelector's quality scoring actually prioritizes uses:['mqtt'] certs or that MQTT's raw-socket listener forwards usageType at all. Adds coverage for both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- unitTests/server/mqtt.test.js: drop sinon/chai for a plain assert + local call recorder (AGENTS.md's no-new-sinon rule for unitTests/server/). - unitTests/security/keys.test.js: withCerts() now puts fixtures inside the try so a partial failure can't permanently pollute the shared cert table, and deletes are tolerant of an already-missing record; the transient selectors pass liveReload=false so they don't leak rebuild subscriptions into module-global state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second-pass review findings: the liveReload=false comment claimed it stops the hdb_certificate subscription leak, but it only gates liveTLSRebuilders (pre-existing createTLSSelector behavior) — reworded to say what it actually does. Also assert the winning certificate's numeric quality, not just its name, so a future scoring change that produces a tie (broken only by cert-table scan order) fails here instead of passing by coincidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d spot Third-pass review findings, both confirmed by mutation testing: - The selector tests resolved through defaultContext (the global-best cert) rather than the per-hostname SNI map, so they'd still pass with secureContexts.set(hostname, ...) removed entirely. Candidates now register under a synthetic hostname via an explicit `hostnames` override, plus a same-table decoy (real cert SANs, so it wins defaultContext) that must never win a lookup for that hostname — proving the per-hostname map is what's actually exercised. - server/threads/threadServer.js:592 (`const usageType = options.usageType ?? 'server'`, feeding both createTLSSelector and getEffectiveTlsCiphers) had no coverage at all: mqtt.test.js proves MQTT emits usageType, keys.test.js proves createTLSSelector honors it directly, but nothing drove the seam joining them. A new describe block calls the real server.socket() (onSocket) and asserts getEffectiveTlsCiphers' config- layer output differs by usageType (operations-api vs mqtt), which transitively proves createTLSSelector gets the same value — they're the same local variable, not independently computed. - Renamed the mqtt.test.js "plain-port" case to describe what it actually pins (the port||securePort guard), not a nonexistent usageType branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…env, clean up sockets Fourth-pass review findings, all confirmed by mutation testing or empirical probing: - The threadServer seam describe block only asserted the cipher half of the contract; its selector was silently empty because server/threads/ threadServer.js requires security/keys.ts via plain `require`, a DIFFERENT module instance than this file's `rewire()`'d `keys` (rewire duplicates the module rather than patching the cached one). Every candidate cert threw "Missing private key" against that instance's empty privateKeys map and was swallowed. Fixed by calling the plain- required instance's own loadCertificates() (same on-disk test key, same shared config_utils stub) and asserting directly against socketServer.secureContexts — confirmed to fail when createTLSSelector is mutated back to a hardcoded 'server' type. - before()/after() replaced the live 'tls'/'operationsApi_tls' config wholesale and after() wiped them to undefined instead of restoring the prior values — latent state leakage into the rest of the mocha process. Now snapshots and restores, and spreads onto the existing config rather than replacing it. - server.socket() registers real SERVERS/portServer entries and opens (unlistened) TLS Server objects with no teardown; after() now closes and deregisters everything this describe block created. - unitTests/server/mqtt.test.js: the recording fake returned an integer from .socket() (Array.push's length) instead of an object, silently relying on handleApplication() never touching the return value; and added the missing negative case (neither port nor securePort configured registers nothing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ltContext directly Fifth-pass review findings: - describe-body-level requires for server/threads/threadServer (and its dependents) executed at mocha's load phase, before this file's own before() sets up the test environment. threadServer.js calls env.initSync() at module load, which could read a real machine's harperdb-config.yaml instead of the test config. Moved all of it into the describe block's own before(). - The decoy's role as the "wins on defaultContext fall-through" sentinel relied on an assumed +0.1 hostname-match margin that depends on getHost()/hostnamesFromCert case-matching, which isn't guaranteed in this environment (traced: getHost() lowercases the CN, hostnamesFromCert doesn't, so the bonus most likely never applies here). Replaced the quality-margin inference with a direct assertion that the decoy is selector.defaultContext, which is unambiguous regardless of the exact margin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ptions gaps Sixth-pass review findings: - The decoy's win over the intended per-hostname winner still depended on the +0.1 getHost() hostname-match bonus, which review proved is environment-dependent (case mismatch between getHost()'s lowercased node name and hostnamesFromCert's as-is SANs means the bonus silently doesn't apply on at least one real environment, and would make the defaultContext assertion order-dependent). Fixed by giving the hostname-scoped candidates is_self_signed: true (base quality 1) and keeping the decoy at is_self_signed: false + exact match (quality 6) — a deterministic 4-point margin that doesn't rely on any hostname bonus at all. Verified against the same secureContexts.set() mutation used in the prior round. - The threadServer seam block now forces tls_unixDomainSockets off for its duration (restored after) — onSocket takes a second, unmirrored UDS-server code path when it's on, which the block's cleanup didn't cover. - unitTests/server/mqtt.test.js: assert the full options object (port/securePort/mtls/usageType), not just usageType, so a regression dropping mtls or the port fields doesn't stay green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
createTLSSelector's updateTLS() accepts a falsy `server` everywhere else
in the function (`if (server) {...}`, `server?.secureContextsListeners`),
but one trace-log line dereffed `server.ports` unguarded. Harmless while
trace logging is disabled (logger.trace?.() short-circuits the argument
evaluation), but with it enabled every candidate throws inside the
per-cert try/catch before `defaultContext` gets assigned for that
candidate — surfaced by the #2003 review test additions, which are the
first callers of createTLSSelector(...).initialize(null) to assert on
`defaultContext`. One-character fix: `server?.ports`.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/mqtt-usage-type
branch
from
July 30, 2026 22:48
6995909 to
acb2ef3
Compare
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
…geType Addresses review on #2003: the resolveEffectiveTlsCiphers allowlist test only proved cipher/@SECLEVEL relevance, not that createTLSSelector's quality scoring actually prioritizes uses:['mqtt'] certs or that MQTT's raw-socket listener forwards usageType at all. Adds coverage for both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split out of #1999 per review feedback there, to unblock the
v5.1patch cherry-pick of the customer-facing readiness fix: this half is a no-op for every stock deployment (Harper only ever writesuses: []/['operations-api']/['replication']) and is what made the cherry-pick conflict, sincev5.1predatesgetEffectiveTlsCiphers.A raw-socket TLS listener (
onSocket(), used by MQTT'snetwork.securePort) always resolved its TLS usage type as the generic'server', so a certificate tagged for a specific listener type (uses: ['mqtt']) never got its intended priority over a generic node certificate.onSocket()now accepts ausageTypeoption (falling back to'server') and MQTT passes'mqtt'— applied to both certificate quality scoring and effective cipher/@SECLEVELresolution.Backward compat: certs tagged
uses: ['server']targeted MQTT under the old default, so'server'earns the legacy generic-use credit via an explicit allowlist (LEGACY_SERVER_FALLBACK_TYPES, currently just'mqtt'). An allowlist rather than "everything except operations-api" because every other existing type (operations-api,replication, …) has always had its own dedicated identity and must not newly start accepting a['server']-tagged record's ciphers — this scoping went through three review rounds on #1999 (Codex flagged the compat break, a Harper-domain pass caught the operations-api leak, human review caught the replication leak and requested the allowlist + negative tests).Known caveat (carried over from #1999's writeup)
Quality-scoring crossover for
uses: ['server']: previously a self-signed cert tagged['server']for MQTT's hostname beat a CA-signed['https']cert for the same hostname (4 vs 3.5, since MQTT's type literally was'server'). Now the'server'tag only earns the legacy-fallback credit (+0.5), so that same cert loses (1.5 vs 3.5). Only matters when multiple certs cover the same hostname with differentusestags; flagged for a reviewer with an opinion.Where to look
server/threads/threadServer.js—onSocket()acceptsusageType, threads it intocreateTLSSelectorandgetEffectiveTlsCiphers.server/mqtt.ts— passesusageType: 'mqtt'.security/keys.ts—LEGACY_SERVER_FALLBACK_TYPES+ its use inciphersCandidateRelevant()and the selector's quality scoring.unitTests/security/keys.test.js— allowlist test with negativeoperations-api/replicationcases.Testing
unitTests/security/keys.test.js: 48/48 passing on this branch (47 on main + the new allowlist test).npm run build,prettier --check,oxlintclean on changed files.Refs #1999.
Generated by Claude (Opus 5) for dispatch task fix-harper-1999.
🤖 Generated with Claude Code