Repository navigation
fix(tls,h3,loop): TLS audit round 4 (#352-#358, #361, #377-#382) with the soak-harness and accept-drop findings (#387, #388) - #389
Merged
Conversation
A soak cell that failed for any reason outside `Fail` and `asyncio.TimeoutError` recorded no cause at all. The canary's unhandled exception left its traceback on stderr, the canary container ran without `2>&1`, and the harness is driven as `nimble stress | tee stress.log`, which tees only stdout, so the archived log held a bare `FAILED (exit 1)` and the cell was undiagnosable (the 2026-10-03 streamupload h1 cell). run.sh now merges the canary's stderr into the tee'd stream, and the client's `main` has a catch-all that prints `FAIL <workload>: unexpected <type>: <msg>` plus the full traceback to stdout before returning 1, with an `ExceptionGroup` arm because a library's `TaskGroup` can raise a group that neither existing handler matches (`asyncio.gather` itself re-raises only the first exception). `KeyboardInterrupt` and `SystemExit` still propagate. The fire-and-forget `reporter` and `loop_watchdog` tasks, which `main` cancels without awaiting, now report their own death on stdout: their exception was never retrieved, so a dead reporter and a healthy quiet run both looked like zero report lines. The chaos sidecar prints its traceback for the same reason and its catch-all covers `BaseException`, so a `CancelledError` escaping `run()` exits 2 (internal error) rather than 1, which run.sh reads as an fd leak.
…advertise (#380) ngReceive read datagrams into a 2048-byte buffer with flags 0, while acceptConn left tp.max_udp_payload_size at ngtcp2's 65527 default and so told every peer that datagrams up to 65527 bytes were fine. A conforming client on a large-MTU path (a 9000-byte VPC MTU, 65536 on loopback) took us at our word: the kernel truncated the datagram, header protection and AEAD failed, ngtcp2 dropped the packet, the client retransmitted the same oversize datagram, and the connection died on the idle timer with nothing logged at either end. kMaxUdpPayload (1452) is the send-side path-MTU guess and was never the receive bound. Both numbers now come from one shim constant, kMaxRecvUdpPayload, which acceptConn writes into the transport parameters and which ngSetup reads through vq_max_recv_udp_payload to size the per-loop receive buffer. The advertisement was deliberately not clamped down to 2048: that would cap every datagram the peer sends us and cost throughput on exactly the paths this is about. On Linux the receive passes MSG_TRUNC, so a datagram that still does not fit is counted and dropped rather than handed to ngtcp2 as apparent line corruption; macOS recvfrom has no MSG_TRUNC semantics, so there the larger buffer is the whole mitigation.
…e TCP (#381) ngReceive drained the UDP socket with `while true` until recvfrom returned <= 0, and every datagram is decrypted and parsed synchronously (vq_engine_recv -> ngtcp2_conn_read_pkt -> nghttp3) before the next recvfrom. A source whose datagrams cost the server more than they cost the sender therefore kept the loop thread inside the receive loop, and the HTTP/1.1 and HTTP/2 fds on that same thread were not serviced at all. Reproduced with a flood of Initial packets naming an unsupported QUIC version, each of which ngtcp2 answers with a Version Negotiation packet (a parse plus a sendto per datagram): a plain HTTP/1.1 request on the same single-loop server went unserved past curl's 2 s timeout. ngReceive now takes at most ngRecvBudget (256) datagrams and reports whether it stopped on the budget rather than on an empty socket. The loop records that in quicMorePending and, exactly as it does for its sslReady queue, does not wait on the selector while it is set: the backlog is still drained promptly, but the TCP fds get serviced between batches. The budget is a work quantum, not a rate limit, and is the shape of the accept-side suspendAccept backoff. Address validation is unchanged and still absent: a spoofed-source flood can make acceptConn commit per-connection state up to maxConnections. The comment there now says what does and does not bound that today. Retry tokens stay out of scope.
…#388) handleAccept had four ways to let go of a connection the kernel had already handed us, and none of them left a trace: the maxConnections cap closed the fd silently, startTls returning false (newTlsSession nil) closed it silently, registerHandle raising swallowed the reason in `except CatchableError`, and suspendAccept wrote one bare stderr line the stress harness never captured. At the client all four are a socket that opens and dies with nothing on it, which httpx reports as an empty ConnectError, so a server shedding load on purpose was indistinguishable from a broken network. A 1-hour soak lost a streamdownload h2 sync cell to exactly that 90 s in with nothing anywhere to say which path fired. Each drop now bumps a per-loop counter plus a process-wide atomic tally, readable as server.acceptDrops() / vortex.acceptDrops() (and a no-argument acceptDrops(), because a {.gcsafe.} handler cannot touch the Vortex ref), and writes one line saying why: which cap was hit, the OpenSSL reason behind the nil session, the selector's own message. The lines are rate-limited to one per cause per 5 s per loop thread so a flood cannot turn the diagnosis into its own denial of service, and the first occurrence of each cause is never suppressed. They go through a single new sink, opLog, which the other operator messages in the loop and in server.nim now use instead of bare stderr.writeLine. On the harness side the stress server exposes the tally on /drops (/stats keeps its exact three-field contract with the client) and prints it on SIGTERM, and run.sh dumps the last 200 lines of the server container's log before removing it when a cell fails, so the server's stderr survives in the tee'd run log instead of dying with the container. Re-running the failing cell for 120 s against this build passed clean, with every drop counter at zero; the host was near idle, where the original soak ran eight cells at load 20-50, so the observability is the deliverable here and the root cause stays open.
loadKeyMem called ERR_clear_error() on its failure path, so the OpenSSL error queue was empty by the time buildTlsCtx read it with lastErrorMsg(). Every private-key problem -- a wrong keyPassword, a truncated PEM block, a key in an encoding OpenSSL's decoder rejects -- was therefore reported as "cannot load TLS certificate/key: unknown TLS error", which looks like a library fault rather than the configuration mistake it is. The certificate path never cleared the queue, which made the asymmetry easy to miss. loadKeyMem now clears on entry (so what it reports is its own error, not a leftover from an earlier step) and leaves the reason queued on failure, and lastErrorMsg drains the queue after reading the oldest entry so no stale error is attributed to a later failure or mistaken for corruption by pemReadEndedCleanly.
cstrEq and wildMatch compared the requested server name to the configured host byte for byte, but host names are case-insensitive (RFC 6066, and DNS generally). A client that sent Example.com for a configured example.com matched no host, fell through to the default certificate and failed the handshake on a name mismatch. Both helpers now fold ASCII case on both sides, so the configured host does not have to be lower-cased either. The fold is ASCII-only rather than locale-aware on purpose: a locale tolower maps I to a dotless i under tr_TR, which would make certificate selection depend on the server's locale. The h3 shim already folds case this way (hostEq/lcAscii), so the two paths now agree.
…361) buildTlsCtx raising inside newTlsConfigWith's per-host loop let the exception escape a half-initialized config: the default ctx, every per-host ctx built before the failing one, the createShared(TlsConfig) block and its two locks were all abandoned. A startup failure usually precedes process exit, so the practical impact is small, but an embedder that catches the raise and retries with corrected configuration leaks on every attempt, and a leak-checked test run reports noise that hides real findings. The loop now unwinds through freeTlsConfig (forward-declared for this) and re-raises with the host name prefixed. buildTlsCtx only ever sees the material, so without that an operator with several SNI entries could not tell which host was broken.
newTlsConfigWith installed three things on the default SSL_CTX that buildTlsCtx leaves to its caller -- the ALPN callback, the OCSP staple, and the servername callback plus its arg -- but reloadTlsConfig re-registered only ALPN on the replacement ctx. The servername callback therefore went away with the old ctx: from the first reloadTls() onwards every configured SNI host was served the default certificate and failed the handshake on a name mismatch, until the process restarted. The fault appears only after a reload, so it surfaces in production long after deployment. Both paths now call one installDefaultCbs helper, so the initial build and the reload cannot drift apart again.
reloadTlsConfig rebuilt only the default ctx, so a per-host SNI context kept the certificate it loaded at startup for the lifetime of the process and eventually served an expired one. The per-host certificates renew on the same ~90-day cycle as the default, and there was no API to rotate them short of a restart. TlsConfig now stores each host's TlsMaterial next to its host name and ctx, and the reload rebuilds every per-host ctx from it, so a bare reloadTls() re-reads the per-host files too (the certbot pattern). reloadTlsConfig and server.reloadTls take an `sni` override that replaces the per-host material wholesale, host set included, persisted only on success like cfg.material. The reload is all-or-nothing with the default ctx: one per-host certificate that fails to build rejects it and leaves everything as it was, rather than half-rotating or dropping a host back to the default certificate, which the client would reject as a name mismatch. The new table is published and the old ctxs released under ctxLock, and servernameCb now holds that lock across its whole lookup rather than reading cfg.sniCtx bare: the reload swaps host names, material and ctxs in one critical section, so a handshake can neither index a ctx array that disagrees with the host list it scanned nor have its chosen ctx freed between the load and SSL_set_SSL_CTX, which up-refs what it is handed. The `sni` override reaches the TCP listener only; the h3 engine rebuilds its per-host contexts from its own configured material (a file re-read) on the same reload, which is how #374 already works. The asymmetry is documented on reloadTls and in HARDENING.md.
reloadTlsConfig caught the exception carrying the only diagnostic that existed and returned a bare false, and server.reloadTls passed that on without writing anything anywhere or exposing an accessor. A certbot deploy hook that got false had no way to learn whether the certificate was unreadable, the key mismatched, the OCSP file missing, the cipher string rejected, or the material fine. A large share of this audit's findings end in "the operator has no signal"; the reload path's own silence is the one that made the others hard to diagnose. Every false return now goes through reloadFailed, which stores the reason on the config under reloadLock and writes one stderr line "vortex: TLS reload failed: <reason>" (the convention applyQuicReload follows), so a hook that ignores the bool still leaves a trace. That covers contradictory OCSP arguments, an unreadable explicit ocspFile, a key-only reload against a PKCS#12 bundle, the default ctx build failure, and the per-host SNI failures, which name the host. A successful reload clears the reason. lastTlsReloadError reads it back on TlsConfig, Server and Vortex; under -d:plainHttp it names the reason reloadTls always has there.
… engine ngtcp2_crypto_ossl_init allocates an OpenSSL ex_data index and stores it in a library-level global; it is documented as a once-per-process initializer and is not thread safe. vq_engine_new called it directly, and vq_engine_new runs once per loop thread as newLoop brings the loops up, so on a numThreads > 1 server several threads ran it at the same time. Each extra call leaked an index, and while they raced a session configured under one index could be read back under another, which yields a null crypto context and a failed handshake: only at startup, only under the race, and indistinguishable from a flaky client. It now runs under a std::call_once that captures the return code, so every engine sees the same verdict and a failure fails them all instead of leaving some loops on a half-initialized backend (#357). The race cannot be provoked on demand, so the test pins the invariant instead: tests/vq_h3_tls_ctx.cpp hammers the shared initializer from eight threads and reports how many times it actually ran, and tests/test_http3.nim adds a four-loop h3 server that must start and serve.
…new one loadCertChain called SSL_CTX_use_certificate and then appended each following PEM block with SSL_CTX_add0_chain_cert. SSL_CTX_use_certificate does not touch the chain -- unlike SSL_CTX_use_certificate_chain_file, which clears it first -- so loading into a context that already held a certificate left the previous leaf's intermediates in place and stacked the new ones on top. loadPkcs12 has the same shape through SSL_CTX_add1_chain_cert. On the initial makeCtx the chain is empty, so this only bit an in-place reload: after a CA rotated its intermediate, h3 clients were handed the new leaf together with the old, no-longer-valid intermediates, and the chain grew on every subsequent reload. Both loaders now clear the chain first (#354). Per-host (buildSniCtxs) contexts were never affected: they are built fresh through makeCtx. Nothing observes the chain length from an h3 client, so the regression test drives the loaders directly through tests/vq_h3_tls_ctx.cpp: loading a leaf + intermediate five times must still leave one chain certificate, and likewise for a PKCS#12 bundle carrying its CA.
… place vq_engine_reload_cert wrote the new material into the live SSL_CTX, certificate first then key, with no validation and no rollback. OpenSSL's ssl_set_cert silently frees the existing private key when the new leaf does not match it, and ssl_set_pkey silently frees the existing certificate when the new key does not match, so an unreadable, missing or mismatched key left the loop's context holding one half of a pair: every later h3 handshake on that loop failed, for good, while applyQuicReload logged "keeping the current certificate on this loop", which is precisely what had not happened. A cert-only reload reported success. SSL_CTX_check_private_key was never called. The reload now builds a fresh context through makeCtx from the new material plus the engine's retained TLS policy (passphrase, verify mode, cipher suites, version pinning, client CA) and swaps it in only after everything loaded and the halves match. The per-host (SNI) rebuild joins the same transaction, so a failure anywhere leaves every context untouched. Releasing the engine's reference to the old context immediately is safe: SSL_new up-refs the SSL_CTX, so an in-flight connection keeps the certificate it handshook with. Error reporting goes with it (#383 follow-up 2): makeCtx and the loaders no longer swallow the reason with a bare ERR_clear_error, makeCtx records the step that failed plus OpenSSL's own text, and the new vq_engine_last_error carries it to Nim. applyQuicReload includes it in its stderr line, and so does the "HTTP/3 engine setup failed" line in newLoop, which had the same generic wording (#352). A reload that fails only on the QUIC side is one server.reloadTls rejects before it signals the loops, so the regression tests drive the engine ABI directly through tests/vq_h3_tls_ctx.cpp. With the fix reverted they show the old behaviour exactly: a mismatched key leaves the engine serving the new subject with no usable key, and a cert-only reload returns success.
A reloadTls() with no arguments means "re-read the configured material", and the TCP listener has always honoured that. Nothing resolved it on the QUIC side: applyQuicReload read the two paths out of the CertReload signal and called readFile(cf) on them, so an empty path raised and the reload failed. That is exactly the certbot pattern the project documents, so after a renewal HTTP/1.1 and HTTP/2 picked up the new certificate and HTTP/3 kept serving the one loaded at startup until it expired -- a protocol-specific outage 90 days after deployment, with h1 and h2 healthy throughout. The engine already stored the per-host Material (#374); it now stores the default certificate's material the same way (cert/key paths, in-memory PEM, PKCS#12 bundle and file, passphrase), and vq_engine_reload_cert takes PEM file PATHS rather than PEM blobs, with empty/null meaning "rebuild from the configured material, re-reading any files" through the same fresh-context path. Explicit paths replace whatever the material was sourced from under the TCP path's reloadTlsConfig rules -- a certificate path clears the in-memory PEM and the bundle, a key-only reload against a PKCS#12-sourced certificate is refused -- and what loads successfully becomes what the next bare reload re-reads. For material configured as PEM bytes or a bundle there is nothing to re-read, so the rebuild is a correct no-op for the default certificate and still refreshes the per-host files. applyQuicReload passes the paths straight through: no readFile, nothing to raise (#353). Tests: an h3 server started from cert/key files, its files overwritten with a second certificate and reloaded with no arguments, must present the new subject over both --http3-only curl and the TCP listener; a certPem-configured server's bare reload must return true and keep serving. Both fail with the fix reverted (h3 stays on the old subject while TCP rotates). The white-box suite covers the material-merge rules the public API cannot reach.
Nothing checked notBefore/notAfter on either transport. buildTlsCtx verified that the key matched the certificate and nothing else, and the shim's makeCtx did the same, so an expired certificate loaded cleanly and reloadTls() returned true. A renewal hook racing a certbot symlink swap, or one pointed at archive/example.com/ instead of live/, got a success and every new connection from that moment failed at the client with certificate_expired, masked until the in-flight ones turned over. Verified against the source and reproduced: a reload to a cert dated 2020 (and to one dated 2040) was accepted on both transports and immediately served. The leaf's validity window is now checked right after the cert/key consistency check, in buildTlsCtx and in makeCtx, so it covers startup and reload, default and per-host certificates, TCP and QUIC. Hard failure, no clock-skew allowance, no warning-only mode: startup raises "certificate expired at <notAfter>" or "certificate not valid until <notBefore>", and a reload is rejected with the running certificate left serving. This is a behaviour change -- a server that previously started while serving an expired certificate now refuses to start -- so it is called out in README.md, HARDENING.md and the CHANGELOG (#379). Audited tests/, conformance/, demos/ and bench/ for static certificate fixtures first: there are none. Every certificate is minted at run time with openssl (-days 2 in the suites, -days 3650 in the conformance images), so nothing needed regenerating.
… QUIC context Verified against the source: vq_engine_new runs per loop thread and each engine builds its own SSL_CTX through makeCtx, OpenSSL generates a fresh random TLS 1.3 ticket key per context, and SSL_CTX_set_tlsext_ticket_key_evp_cb appears nowhere in the tree. A ticket was therefore decryptable only by the loop that issued it, while which loop receives a returning client's first datagram is decided by the kernel's SO_REUSEPORT hash over its new 4-tuple. On an N-loop server roughly (N-1)/N of resumption attempts fell back to a full handshake, invisibly: the connection succeeded, one round trip slower, every time. The contexts stay per-loop, since the per-loop in-place reload is built on them. A process-wide ticket key is installed on every one instead -- default and per-host, initial and rebuilt -- generated once under the same std::call_once that initializes the ngtcp2 ossl backend. Rotation is part of this rather than deferred: the current key encrypts, the previous one still decrypts for one more lifetime (the callback returns 2, so OpenSSL reissues the ticket under the current key) and any older name is refused, costing that client one full handshake. The lifetime is one hour. The key state is behind a mutex, since the callback runs on every loop thread. An explicit session-id context goes on the QUIC contexts too, as on the TCP path, which is what lets an mTLS deployment resume at all. SSL_OP_NO_TICKET is never set and the ngtcp2 ossl backend does not change ticket issuance: it only registers the QUIC record-layer callbacks on the SSL, so the TLS stack builds and encrypts tickets as usual and they travel in CRYPTO frames. 0-RTT early data is not offered anywhere in the tree (nothing enables early data or sets an early-data context), so resumption is all that changes and the CHANGELOG says so (#382). No QUIC client in the tree can be made to resume on demand, so the regression test drives the ticket callback the way OpenSSL does through tests/vq_h3_tls_ctx.cpp: a name stamped by the encrypt side decrypts, an unknown name falls back to a full handshake rather than erroring, building a second engine does not mint another key, and after the lifetime elapses the encrypt side stamps a new name while the retired one still decrypts with a reissue and a two-lifetime-old one is refused. The rotation assertions fail with rotateTicketKeyIfDue stubbed out.
…e an unreadable key file (#377, #388) Integration pass over the round-4 branch. The HTTP/3 engine-setup and certificate-reload failure lines in eventloop.nim, and server.nim's no-TLS reload rejection, were the three operator messages still written with a bare stderr.writeLine after #388 introduced opLog as the one sink; they now go through it. loadCertKey raised nothing and returned a bare false when the keyFile or pkcs12File could not be read, so that case still surfaced as "unknown TLS error" after #377 fixed the decoder-rejection case: a readMaterialFile helper carries the path and the OS reason out, and buildTlsCtx frees the partial ctx before re-raising it. Two doc comments still described the h3 reload as an in-place mutation; #352 made it a rebuild-and-swap.
…tal (#388) `AcceptDrops.total` summed `acceptSuspend` in with `cap`, `tls` and `register` and was documented as "connections this process accepted and then dropped". The first three are that; `acceptSuspend` is not. It counts the times accept() itself failed with EMFILE/ENFILE/ENOBUFS/ ENOMEM and the listener was deregistered for ~1s: nothing was accepted, nothing was dropped, and whatever is in the kernel's backlog waits for the listener to come back or times out there. Summing it made `total` read high against an operator's own connect-error rate, and one backoff could stand for any number of waiting connections. `total` is now `cap + tls + register`. `acceptSuspend` keeps its counter and its rate-limited operator line, and is reported beside the total as the backoff-event count it is ("raise the fd rlimit"). The field docs, the `Server.acceptDrops` accessor doc, HARDENING's "Seeing the limits fire" table, the README snippet and the stress server's `/drops` line and shutdown print all say so; `/drops` moved `acceptSuspend` after `total` so the grouping is visible in the soak log. test_security_dos pins the identity (`total == cap + tls + register`), and its no-argument-form check is now `>=` rather than `==`: both sides are reads of a process-wide counter that any loop thread of any server in the process can bump between them.
`opLog` used `stderr.writeLine`, which is two `fwrite` calls: the payload, then the newline. Every loop thread writes through this sink, so a line from another thread could land between the two and split a message across the log, which is the one thing a sink that exists to make drops visible must not do. Build the line (`msg & "\n"`) and write it once; the try/except shape is unchanged. Also reword the docstring, which read as if an operator could redirect the sink. It is internal and not exported from `vortex.nim`, so say that instead of promising a hook that does not exist.
#388) `srv.requestShutdown()` is non-blocking and it was the last statement of `isMainModule`, so main fell off the end and the process ran its exit sequence while the loop threads were still serving. That is a teardown race in the one binary whose whole purpose is to surface crashes, and it would be charged to vortex rather than to the harness. Use `srv.close()` (requestShutdown + waitFor), which blocks until the loops have drained and been joined, bounded by `shutdownHardTimeout` as everywhere else. The drop-counter print stays BEFORE the join so it is never lost if the join takes the grace period, and a "stopped" line after it marks an orderly exit. The module comment now says this path is for hand runs: run.sh tears a cell's server down with `docker rm -f`, a SIGKILL, so no handler runs there (which is why run.sh dumps the container's last 200 log lines on a failed cell). Verified by hand on the sync and chronos builds: SIGTERM prints the tally, then "stress_server: stopped", and the process is gone within a second.
…oute (#380, #381, #388) HARDENING had nothing at all about HTTP/3 ingress, although it is the one listener with no accept queue and no backlog: every datagram is decrypted and parsed synchronously on a loop thread that also owns TCP connections, so it has to be bounded in work rather than memory. A new "QUIC ingress (HTTP/3)" subsection under the resource limits states what #380 and #381 actually guarantee -- a receive buffer sized to the advertised 65527-byte `max_udp_payload_size` from one shim constant, a 256-datagram budget per drain (`ngRecvBudget`), up to four drains per loop pass, and no selector wait while a budget was exhausted so the TCP fds on the thread keep being served under a flood -- and, just as important, the residual risk the #381 entry records: no Retry token yet, so a spoofed-source Initial flood can still make the shim commit per-connection state up to `maxConnections`, and Version Negotiation is one reply per unsupported-version Initial with no per-source limit (amplification below 1, so work rather than reflection). The HTTP/3 window rows link down to it. The stress README gained a section on `/drops`: its exact format, what each field means (including that `acceptSuspend` is not in `total`), that nothing polls it automatically, and that run.sh dumps the server container's last 200 log lines on a failed cell -- which is where the `vortex:` lines live, since `docker rm -f` is a SIGKILL and the server's own SIGTERM print never runs under the harness.
…381) The suite spun an unconditional 4 sender threads and gave curl `-m 2`, the same 2 s it then asserted the request completed inside. On a 2-vCPU CI runner that oversubscribes the host and measures its scheduler, and a curl timeout equal to the bound makes "served slowly" indistinguish- able from "never served": both come back as a nonzero exit at exactly the bound. Senders are now `max(1, countProcessors() - 1)`, so a core is always left for the loop thread and one sender is still enough to starve an unbounded drain (these datagrams cost the server more than the sender). curl gets `-m 10`, longer than the bound on purpose, so a slow answer shows up as a latency the test prints. The bound itself is 5 s: the pre-fix failure is total, not slow, so it does not need to be tight. Verified: five runs on a 14-core host (13 senders) took 418, 457, 663, 972, 1059, 1675 ms. With `ngRecvBudget` raised to 1e9 to undo the fix, the request was never served and curl returned at its full 10 s timeout, so the 5 s bound keeps the pre-fix signal.
) lastErrorMsg formatted with ERR_error_string(e, nil), which writes into OpenSSL's single process-wide `static char buf[256]` and is documented as not thread-safe. #388 exported it as tlsLastErrorMsg() and the accept path calls it from every loop thread, so two threads reporting a failure at once could each be handed the other's message: a two-thread probe read 128 to 2,760 wrong messages per 400,000 samples. It now formats with ERR_error_string_n into a buffer of its own, trimmed at the NUL. newTlsSession also never cleared the thread's error queue, and tlsHandshake/tlsRead/tlsWrite leave their reason on it when a connection fails, so the accept path's "TLS session setup failed" line could name the failure of an earlier connection accepted on the same loop thread. It clears the queue on entry, and loadPkcs12 and buildTlsCtx do too, as the PEM loaders already did, so a rejected bundle, cipher string or protocol version is not reported with a leftover from unrelated OpenSSL work. tests/test_tls_error_queue.nim pins both: it pushes errors with known codes onto the queue (ERR_new + ERR_set_error) and reads them back from two threads, and it checks that a queued reason does not survive newTlsSession. Both cases fail with the fix reverted (9,180 wrong reads of 400,000, and the stale error:0A000064 reported after the new session).
…378) A PKCS#12 bundle carries the certificate and the key together, so a reloadTls(certFile = ...) against p12-sourced material cleared the bundle fields and left keyFile and keyPem empty. loadCertKey then called readMaterialFile("", "private key"), so the reload failed, as it must, but its reason was "cannot read private key : cannot open:" with both the path and the OS reason blank, naming neither the key nor the bundle. The symmetric keyFile-only case has had an explicit rejection since #378; the certFile-only case now gets the same one, before anything is built. readMaterialFile also treats an empty path as the configuration hole it is and raises "no private key configured" rather than asking the OS to open "". tests/test_tls_advanced.nim asserts the new reason through lastTlsReloadError and that the old blank one is gone, plus a case that reaches the empty-path message through the config. Both fail with the fix reverted, reporting exactly "cannot read private key : cannot open:".
servernameCb held cfg.ctxLock across SSL_set_SSL_CTX, which in OpenSSL 3 is not a pointer store: it ssl_cert_dups the entire CERT of the context it is handed (an allocation plus an up-ref per chain entry and per key slot) and frees the connection's old one. acquireCtx takes the same lock on every accept, so every accepted connection on every loop thread queued behind one SNI handshake's certificate duplication. The lock now covers only what needs it: the host scan and an SSL_CTX_up_ref of the chosen context, which is what keeps a concurrent reload's release from destroying it (#356 swaps the whole per-host table under that lock). The switch runs outside the lock and takes its own reference, so the callback drops ours straight afterwards. Verified with tests/test_tls_reload_race.nim (3 SNI handshake threads against 40 reloads) and tests/test_tls_advanced.nim, and the race suite once more under NIM_SANITIZE=1, which reports no double free or use-after-free.
lastTlsReloadError took cfg.reloadLock, which reloadTlsConfig holds across its file reads, key parsing, one buildTlsCtx per SNI host and a stderr write. A health handler on a loop thread reading the reason therefore stalled that whole loop for the length of a rotation. The field is now published by reloadFailed (and cleared with the ctx swap) under ctxLock, the short lock acquireCtx takes for an up-ref, and read under the same one; the writer still holds reloadLock, so the lock order reloadLock -> ctxLock is the one the swap already uses. Documented as cheap and safe from a handler in the accessor, HARDENING.md and README.md. Also fixes two stale comments noted in review: the TlsConfig field doc (which now also records that lastReloadError is a GC string in createShared memory, the same pre-existing pattern as material/ocsp/sni*), and the claim that the SNI-failure path's SSL_CTX_free releases an OCSP blob, which is not attached to that context yet. tests/test_tls_reload_race.nim gets a deterministic case: the reload is pointed at a FIFO as its ocspFile, so it sits in readFile holding reloadLock while a writer thread unblocks it after 2 s. The accessor must answer anyway; with the fix reverted it waits 2,004 ms, with it under 1 ms.
X509_cmp_current_time answers 0 only on failure: it reports an exact equality as -1, so 0 means an ASN.1 time it could not read. checkCertValidity compared `< 0` and `> 0`, so that 0 fell through as "inside the window" and the certificate was installed on the strength of a comparison that never happened. This is reachable, not theoretical: OpenSSL loads a certificate whose notAfter is a well-formed UTCTime that is not a real time, and its own x509 command prints "notAfter=Bad time value" for it. Such material now fails with "certificate validity time could not be parsed", at startup and on reload like any other bad material. The same change is going into the h3 shim. tests/test_tls_cert_validity.nim crafts one by rewriting the notAfter straight into the DER (same length, so the key still matches) and checks both paths. With the fix reverted the server starts on it and reloadTls returns true. The suite's dated-fixture gate now only guards the two suites that need openssl -not_before/-not_after, so the new case runs everywhere.
HARDENING.md claimed "The TCP listener still uses OpenSSL's own per-context ticket key, generated once at startup and not rotated." The second half is wrong: reloadTlsConfig builds a replacement SSL_CTX on every reload, so the TCP ticket key and the TLS 1.2 session cache are regenerated there and every ticket or cached session issued before the reload stops resuming. Probed with openssl s_client -sess_out/-sess_in against a vortex server: the saved session comes back "Reused" before reloadTls() and "New" after it. Rewritten to say what is true, and to name the consequence an operator cares about: since #382 made the h3 key process-wide and rotating, HTTP/3 resumption survives a certificate rotation while TCP's does not. The #382 CHANGELOG entry gets the same clarification. Sharing the rotating key with the TCP listener is left as a follow-up.
) The per-host override added by #356 reached the TCP listener only. server.reloadTls signalled the loops with requestCertReload(certFile, keyFile) alone, and the shim rebuilt its per-host contexts from the material vq_engine_new was handed, which is filled once at startup and never updated. So on an http3-enabled server reloadTls(sni = ...) returned true, TCP served the new host, and every h3 client that followed Alt-Svc was handed the DEFAULT certificate for that host and failed on a name mismatch -- the #374 bug, reintroduced for any host configured after startup -- while a host removed from the set kept being served its retired certificate over h3 for the life of the process. CertReload now carries the replacement set to the loop threads beside the paths: a serialised, GC-free blob published under the existing lock with the generation, freed and replaced by the next request, decoded by the loop under the lock exactly as the paths are. applyQuicReload hands it to ngReloadCert, and vq_engine_reload_cert takes a VqSniCert[] that, when non-empty, REPLACES the engine's per-host material and contexts in the same all-or-nothing transaction as the default certificate. An empty set keeps the old meaning, rebuild the configured material and re-read its files, so there is still no spelling for "drop every host" on either transport. server.reloadTls passes the set through only when the TCP reload succeeded, as it already did for the paths. Also fixes the shim's hostEq comment, which still called the TCP path's byte-exact SNI match issue #358 (fixed on this branch).
…2 reload Three diagnostics on the QUIC material path, mirrors of what the TCP side already does (#353, #377, #379). A missing or unreadable certificate, key or PKCS#12 FILE reached the operator as "cannot load TLS certificate/key: error:80000002:system library::No such file or directory", naming neither the path nor which half of the pair failed, because OpenSSL leaves nothing better in its error queue. makeCtx now opens each file up front and reports "cannot read private key <path>: <reason>", the shape readMaterialFile gives the TCP path. A certFile-only reload against a PKCS#12-configured engine cleared the bundle and left key_file empty, so it came back as a bare "certificate/key mismatch" instead of the actionable rejection the key-only case already got. It is now refused up front with the TCP path's wording. certValidityError treated X509_cmp_current_time == 0, which is a parse failure and not a verdict, as "in window", so a notBefore/notAfter OpenSSL cannot read installed as if it were valid. It is a rejection now ("certificate validity time could not be parsed"); the TCP side takes the same change. Also: sslErrStr's comment claimed it reported the queue's newest entry while ERR_get_error pops the oldest (behaviour unchanged), the #354 chain-clearing comments now say that SSL_CTX_clear_chain_certs clears the selected key slot's chain only (fine, because every live reload builds a fresh context), and the #353 notes stop calling a configured pkcs12File a no-op for a bare reload: it is a path, and it is re-read.
…all (#382) Rotation was driven only from the encrypt branch of ticketKeyCb, so the hourly lifetime held only under steady ticket traffic: on a server that went quiet, a ticket minted at t=0 was still accepted at t=10h because no ticket had been issued in between, and HARDENING's "a disclosed key exposes at most two hours of resumed sessions" was not true of an idle deployment. rotateTicketKeyIfDue now also runs from vq_engine_handle_expiry, which every loop enters at least once a second, at the cost of one steady_clock read under the mutex per tick. The three existing #382 cases call ticketKeyCb themselves, so they pass with the SSL_CTX_set_tlsext_ticket_key_evp_cb line deleted from makeCtx -- while every context would then fall back to its own OpenSSL-generated key and the cross-loop resumption the issue is about would be gone. A new case runs real TLS 1.3 handshakes over a BIO pair against the engines' own contexts: one against engine A to mint a ticket, one against engine B to resume it, asserting the shim's callback ran on both the encrypt and the decrypt side (two small counters on the shim make that observable) and that SSL_session_reused came back set. It fails with the install line removed.
gTruncDrops was a {.threadvar.}, so test_h3_recv_payload's
`check ngTruncatedDrops() == 0` read the main thread's own copy, which is
always zero whatever the loops did: a vacuous assertion. gRecvBufSize was
the opposite problem, a plain shared global written by every loop thread,
which is a data race even though all of them store the same number. Both
are Atomics now, relaxed, since neither orders anything.
gRecvBuf (a 64 KiB seq) and gReady were never released when a loop thread
ended, so a process that starts and stops servers leaked 64 KiB per loop
thread per cycle (25 cycles over 4 loops measured at 6 MiB). ngEngineFree
clears them with the engine.
Docs: gRecvBuf is 65527 bytes, which IS the largest possible UDP payload,
so the MSG_TRUNC branch and gTruncDrops can never fire. The guard stays
(a future smaller buffer would need it) but the #380 CHANGELOG entry and
the ngTruncatedDrops doc comment stop describing it as a live path, and
the test says it is asserting the guard stayed dormant (#380).
ngRecvBudget's docstring and the #381 entry said "256 per pass of the
event loop", but h3Drive can run up to four times per pass (the outbox,
the main drive, the tick's idle sweep, the end-of-pass flush re-drive),
so the budget is per drain and the per-pass ceiling is 1024. Reworded;
the budget itself is unchanged (#381).
The #381 entry quoted the four-sender reproduction (tens of milliseconds); the regression test now sizes its senders to the host and measures about a second with every core but one blasting, so the entry gives both figures and the bound the test asserts.
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
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.
Fixes the TLS-audit issues #352 through #358 and #361, the five unverified completeness findings #377 through #382 (all five held, each reproduced against the source before the fix), and the two 2026-10-03 soak findings #387 and #388. One commit per issue (sixteen), one integration commit, then fifteen commits from an adversarial review round that found one high and eleven medium findings in the first pass; every one is fixed here with its own regression test where one is feasible.
Fixes #352
Fixes #353
Fixes #354
Fixes #355
Fixes #356
Fixes #357
Fixes #358
Fixes #361
Fixes #377
Fixes #378
Fixes #379
Fixes #380
Fixes #381
Fixes #382
Fixes #387
Fixes #388
TCP TLS path (src/vortex/transport/tls.nim, server.nim)
loadKeyMemcleared the OpenSSL error queue on failure, so every key failure (wrong passphrase, truncated key, unsupported encoding) was reported as "unknown TLS error". It now clears on entry and leaves the reason queued;lastErrorMsgdrains the queue after reading so a stale entry is never attributed to a later failure. An unreadablekeyFile/pkcs12File(which never reaches OpenSSL's queue) is reported with the path and the OS reason viareadMaterialFile.cstrEq/wildMatchfold ASCII case on both sides (no locale, no allocation; the h3 shim already did), soExample.comandFOO.Example.COMselect the configured SNI certificate.newTlsConfigWithfrees the default context, the per-host contexts built so far and the shared block before re-raising, and the error names the failing host (SNI host "broken.example": ...).installDefaultCbsholds every callbackbuildTlsCtxleaves to its caller (ALPN, OCSP staple, servername cb + arg) and is used by both the initial build and the reload, so SNI no longer dies after the firstreloadTls().TlsConfigstores each host'sTlsMaterial; every reload rebuilds the per-host contexts from it (a barereloadTls()re-reads per-host files), all-or-nothing with the default context, swapped underctxLockand released after. New API:reloadTls(sni = @[SniCertEntry(...)])replaces the per-host set wholesale (hosts may be added or removed), on both transports: the material travels to the h3 loops throughCertReloadas anallocSharedblob andvq_engine_reload_certreplaces the engine's per-host set in the same transaction as the default context.falsefromreloadTlsConfigrecords its reason (srv.lastTlsReloadError, readable cheaply from a handler under the short lock) and writes onevortex: TLS reload failed: <reason>line. Covers the contradictory OCSP arguments, an unreadable explicitocspFile, a key-only or certificate-only reload against a PKCS#12 bundle, the default build failure and any per-host failure (named).checkCertValidityrejects material whose leaf is expired, not yet valid, or whose validity time will not parse, at startup and on reload, default and per-host, TCP and QUIC. Hard failure, no skew allowance: a server that previously started on an expired certificate now refuses. There are no static certificate fixtures in the tree (all minted at run time), so nothing needed regenerating.HTTP/3 (src/vortex/http3/ngtcp2)
ngtcp2_crypto_ossl_init()runs exactly once per process understd::call_once, with the return code cached so a failure fails every engine with a reason instead of leaving some loops half-initialised.loadCertChainandloadPkcs12clear the chain before loading.vq_engine_reload_certbuilds a replacement context throughmakeCtx(same policy: ciphers, versions, mTLS, validity) and swaps it in only afterSSL_CTX_check_private_keypasses, with the per-host rebuild staged alongside so a failure leaves every context untouched. In-flight connections keep their own reference. The reason reaches Nim throughvq_engine_last_errorand appears in the loop'sHTTP/3 certificate reload failed (...)andHTTP/3 engine setup failed (...)lines;makeCtx/loadKey/loadPkcs12no longer swallow the queue (PR fix(tls,h3): TLS audit round 3 (#362-#373) with the retire-ring, QUIC-config and renegotiation clusters (#351, #359, #360, #374, #376) #383 follow-up 2).vq_engine_reload_certtakes file paths (ABI change from PEM blobs), and empty paths mean "rebuild from the configured material, re-reading any files", so a barereloadTls()rotates h3 exactly as it rotates TCP. Explicit paths merge under the TCP rules (a certFile clears PEM/p12; one-sided reloads against a bundle are refused with a reason).call_once, rotated hourly on the loop clock with the previous key accepted for one more lifetime (renewed via return 2); explicit session-id contextvortex-h3/1. Resumption now works whichever loop the kernel's SO_REUSEPORT hash picks. 0-RTT early data is not offered anywhere and is unaffected.max_udp_payload_size(65527, ngtcp2's default, so the wire is unchanged);MSG_TRUNCis passed on Linux as a guard.ngReceiveprocesses at mostngRecvBudget(256) datagrams per drain and the loop does not sleep in the selector while a budget was exhausted, so a UDP flood no longer starves the TCP connections on that thread (reproduced with unsupported-version Initials: a plain HTTP/1.1 request was never served pre-fix, now well under the bound).Event loop and stress harness
acceptDrops()/srv.acceptDrops()returnscap,tls,register(their sum istotal) andacceptSuspend(fd-exhaustion backoff events, not drops). OneopLogsink replaces the scatteredstderr.writeLines and rate-limits each cause to one line per 5 s per loop, never suppressing the first occurrence; the TLS line names the OpenSSL reason (formatted into a private buffer, see review findings). The stress server exposes/drops, andrun.shdumps the server container's last 200 log lines when a cell fails. Thestreamdownload h2 synccell from the issue was re-run with the counters in place for 120 s at load average 2: it passed clean with every counter at 0, so the original failure (at load average 20-50) is not reproduced here; the observability it asked for is in place.FAILED (exit 1)with no cause (stderr is never teed) #387 the canarydocker runmerges stderr into the tee'd stream;stress_client.pyreports any exception class (includingExceptionGroups) with its traceback on stdout before exiting 1, and a dying reporter/watchdog task is announced instead of vanishing;chaos.pyprints the traceback for its internal-error path and no longer lets aCancelledErrorexit 1 (the fd-leak code).Adversarial review round (15 commits)
Three reviewers probed the integrated branch; findings and fixes:
lastErrorMsgformatted withERR_error_string(e, nil), OpenSSL's process-wide static buffer, and handleAccept still drops accepted connections with no log or counter (empty ConnectError); reproduced on streamdownload h2 sync after #343 #388 made it reachable from every loop thread (probe: 2% of concurrent reads returned another thread's message). NowERR_error_string_ninto a private buffer, with a two-thread regression test (test_tls_error_queue).servernameCbheldctxLockacrossSSL_set_SSL_CTX(a full CERT dup in OpenSSL 3), serialising every loop's accept path behind one SNI handshake; it now up-refs under the lock and switches outside it.lastTlsReloadErrortookreloadLock, which the reload holds across file reads and every per-host build, so a handler reading it stalled its loop for the whole reload (probe: 2 s); it now publishes under the shortctxLock(<1 ms during a reload storm).newTlsSessionnever cleared the error queue, so the new TLS-drop line could report an earlier connection's handshake failure;loadPkcs12/buildTlsCtxdid not clear on entry either.cannot read private key : cannot open:; both transports now refuse it with the same actionable reason as the key-only case.snioverride reached TCP only, so on an h3 server a host added at runtime was served the default certificate over QUIC (the fix(h3): HTTP/3 has no SNI support; per-host certificates are never served over QUIC #374 shape); it now reaches the engine (above).X509_cmp_current_time == 0(unparsable time) was treated as valid on both transports; it is rejected, with a TCP regression test that rewrites a notAfter in the DER.acceptSuspend(an accept() backoff event, nothing accepted) was summed intoAcceptDrops.total; it is reported separately.gTruncDropswas a threadvar (the test's read was vacuous),gRecvBufSizea plain shared global, and the 64 KiB per-thread receive buffer leaked per server start/stop; all three fixed.mainwith loop threads running; it now joins them.Reusedbefore,Newafter), so h3 resumption now survives a certificate reload while TCP's does not. Added a QUIC-ingress section (budget, buffer, the residual no-Retry-token flood) and/dropsto the stress README; corrected the fix(tls): bare reloadTls() never rotates the HTTP/3 certificate #353/fix(h3): QUIC receive buffer is 2048 bytes while the server advertises max_udp_payload_size 65527 #380/fix(h3): QUIC ingress has no per-tick budget; a UDP flood starves the loop's TCP connections #381 CHANGELOG wording.Decisions to double-check
vq_engine_reload_certtakes paths plus an optional per-host set, not PEM blobs. The only callers arebackend.nimand the new white-box suite.snioverride replaces the per-host set wholesale on both transports; an emptysnikeeps the configured set, so there is deliberately no way to drop every host through the API.Server);opLogis internal (not exported fromvortex.nim); the rate limit is a hardcoded 5 s per cause per loop.max_udp_payload_sizeat 65527 (the wire is unchanged) and sizes the buffer to it, rather than clamping the peer down to 2048.FAILED (exit 1)with no cause (stderr is never teed) #387's2>&1also routes docker's own errors onto stdout; aFailarriving wrapped in anExceptionGroupis reported asunexpected ExceptionGroup[Fail: ...].Follow-ups found during the work (not changed here)
SSL_CTXand regenerated on everyreloadTls, invalidating every outstanding ticket; sharing the h3 path's rotating key would fix both that and the lack of scheduled rotation.TlsConfigstores GC strings/seqs (material,ocsp*,sniMaterial,lastReloadError) increateSharedmemory and frees them cross-thread under the locks; a pre-existing pattern, now documented on the field.CertReloadalready uses fixed buffers /allocShared.maxConnections, and Version Negotiation replies are unrate-limited (amplification below 1). Documented in HARDENING.run.sh's chaos-gate and drain messages still go to stderr (>&2); the canary and server streams are now captured, these two harness lines are not.tests/run.shhas no-fand~/.cache/nimis shared across worktrees, so a suite that#includes the shim can link a stale object built from another checkout; a per-suite nimcache or-ffor the shim-including suites would remove the hazard./dropsendpoint is not polled by the canary or the chaos sidecar; its tally reaches a soak log only through the rate-limited stderr lines on a failed cell.handleAcceptstill drains the accept queue unbounded per pass (the TCP analogue of fix(h3): QUIC ingress has no per-tick budget; a UDP flood starves the loop's TCP connections #381).loadCaMemin the shim still clears the error queue before returning false, so a damaged client-CA bundle reaches the operator as the generic "cannot configure client verification" (PR fix(tls,h3): TLS audit round 3 (#362-#373) with the retire-ring, QUIC-config and renegotiation clusters (#351, #359, #360, #374, #376) #383 follow-up shape).Validation
bash tests/run.shon the final tree: 100 suites, 803 checks, 0 failures, 4 environment skips (the compression builds), after clearing the shared nim cache so every shim-including suite linked this tree's object. Every implementation and fix agent also ran the full suite on its own commits.-d:plainHttpbuild ofsrc/vortex.nimand all 15conformance/tls/harness.nimconfigs pass.test_tls_reload,test_tls_reload_race,test_tls_advanced,test_h3_tls_ctx,test_tls_reload_h3,test_tls_cert_validity,test_http3also underNIM_SANITIZE=1(ASan+UBSan): clean. (Note:NIM_SANITIZEimplies-d:useMalloc, so these runs say nothing about cross-thread ORC frees, follow-up 2.)total == cap + tls + registeridentity pins a contract).Stress smoke
One complete pass of
VORTEX_PROTO=all VORTEX_SERVER=all VORTEX_SECONDS=10 VORTEX_REPORT_SECONDS=2 nimble stresson the final tree (5 workloads x h1/h2/h3 x sync/async/async-await/chronos/chronos-await, 75 cells, canary plus chaos sidecar, 64 MiB streams): 75/75 cells passed, 75/75 chaos sidecars passed, every checksum verified, no panic, assertion, signal, traceback or unhandled-exception signature in any log, exit 0. Load average during the run was 2.4 to 2.8. Thestreamdownload sync h2cell from #388 passed (42 transfers, 279 MB/s, RSS 249 MB flat, fds 54).Mean throughput over the five server runtimes, for a same-host comparison against the #383 numbers:
h1 and h2 are level with #383. The h3 stream cells read lower; at 10 s with 64 MiB transfers a cell completes only 6 to 11 transfers, so the figure quantises by 15-50% depending on where the cut falls, and the aioquic client is the bottleneck in those cells (see
BENCHMARKS.md). To rule out a regression, both h3 stream cells were re-run for 60 s on thesyncruntime with the default 1 GiB streams and no chaos sidecar, on this branch and on main (fb7bc4c), sequentially on an idle host:Identical within noise, so the smoke's lower h3 stream figures are the 10 s quantisation, not the receive budget or buffer change. (With 1 GiB streams no download completes inside 60 s, so the harness verdict for that cell is "no successful iterations" on both trees; the bytes-moved rate is the comparable number.)