Support more than one TLS cert - #651
Draft
michalhosna wants to merge 5 commits into
Draft
Conversation
TempYamlFile and TempFile were separate copies of the same unique-path construction, one per header that needed a temp path. TempDir.h holds the one copy plus the TempDir the cert-directory tests need. CertTestUtils.h carries the self-signed cert builder that PKCS#12 bundles and PEM pairs both start from; the header notes why folly's TemporaryDirectory/TemporaryFile are not usable here.
MoqxRelayServer and MoqxQmuxRelayServer each carried their own copy of the proxygen sample's context setup, and the mvfst copy had grown a third variant for PKCS#12 material. The three drifted independently; a knob added to one silently did not apply to the other listener type. makeCertManager is also the seam an alternative cert source plugs into (HSM/KMS): implement fizz::server::CertManager and hand it to the CertManager overload. The rationale for keeping selection in-process is on the declaration.
BREAKING: a listener setting insecure: true together with cert_file, key_file, or pkcs12_file now fails config load. It previously warned and served the compiled-in development certificate, so a deployment that believed it had real TLS only found out from production traffic. The two scripts that render cert paths from env diverge, because what a set path means differs. scripts/moqx-run.sh synthesizes one from DOMAIN whether or not the operator asked for it, so --insecure clears it. docker/entrypoint.sh defaults it to empty, so a value there is one the operator set: it exits rather than making the silent swap the config now refuses. entrypoint.sh canonicalizes MOQX_INSECURE first: YAML accepts yes/on/1 as true, and the shell tests around it compare against the literal, which also left the picoquic listener enabled under a spelling the config then rejected. A hand-written config setting both needs the cert source removed.
A relay fronting several hostnames could only present one certificate, so every name had to share a SAN list and every renewal touched all of them. tls.fizz.cert_dir serves a directory of <base>.pem/<base>.key pairs and picks one per connection from the client's SNI; cert_file/key_file or a PKCS#12 bundle, if also set, stay on as the fallback. Certificates and keys are read on the scanning thread, never on a handshake. fizz's cert-selection hook is synchronous, so a deferred read would land on the connection's IO thread and one slow cert_dir mount would stall every handshake sharing it. Startup is strict and a rescan is not: the reasoning for each retention rule is in docs/config.md. insecure: true rejects fizz.cert_dir like any other cert source, and warns for the rest of the block: ignoring a cert source serves something weaker than the operator configured, ignoring a reload interval does not. ListenerTlsConfig carries the fizz-only options so admin TLS, which shares TlsConfig, cannot pick up SNI behavior by construction. picoquic does not use fizz and rejects the block. Listeners resolving to the same cert source share one SniCertManager: one scan, one rescan thread, one set of loaded keys.
Each process picked a random ticket seed at startup, so TLS resumption died on restart and never worked across relay instances behind one VIP. Pointing every relay at the same seeds file fixes both: the first seed encrypts, every listed seed still decrypts, so a rotation is a prepended line and a restart. The format is moqx's own, not nginx's or HAProxy's. Seeds are validated per line at config load rather than left to fizz, which rejects a set with any sub-32-byte member wholesale and would otherwise leave the ticket cipher silently secretless — no tickets issued, no resumption, no error. The decoded seeds are secret material: the /config dump carries only how many are loaded, and the file buffer is wiped after parsing.
michalhosna
changed the title
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
1 participant
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.
Draft, up for visibility, more my passes to come. Currently, very non-human comments and code everywhere.
Fixes #45
Missing picoquic support, see #562
This change is