TLS: add proxy.config.ssl.server.multicert.partial_reload#13373
TLS: add proxy.config.ssl.server.multicert.partial_reload#13373Jabrique wants to merge 1 commit into
Conversation
324571d to
a632611
Compare
a632611 to
71a042c
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds an operator-controlled “partial commit” mode for TLS server multicert reloads so that a single bad ssl_multicert.yaml entry doesn’t block unrelated certificate updates, while still surfacing the degraded state via ERROR logging and a new metric.
Changes:
- Add
proxy.config.ssl.server.multicert.partial_reload(dynamic, default0) and gate partial-commit behavior inSSLCertificateConfig::reconfigure(). - Introduce
proxy.process.ssl.ssl_multicert_load_failuresand increment it on per-item certificate load failures. - Add documentation and a new gold test covering strict vs partial reload behavior (including SNI-only and “all fail” cases).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/tls/ssl_multicert_partial_reload.test.py | New gold test exercising strict vs partial reload behavior and validating metric/log outcomes. |
| src/records/RecordsConfig.cc | Registers the new proxy.config.ssl.server.multicert.partial_reload runtime knob. |
| src/iocore/net/P_SSLConfig.h | Adds configPartialReload to SSL config parameters. |
| src/iocore/net/SSLConfig.cc | Implements the partial-commit gate in SSLCertificateConfig::reconfigure() with a startup-vs-reload distinction. |
| src/iocore/net/SSLStats.h | Declares the new ssl_multicert_load_failures counter pointer in the SSL stats block. |
| src/iocore/net/SSLStats.cc | Creates the new proxy.process.ssl.ssl_multicert_load_failures metric in SSLInitializeStatistics(). |
| src/iocore/net/SSLUtils.cc | Increments the new metric on per-cert load failure (with a startup guard). |
| doc/admin-guide/files/records.yaml.en.rst | Documents the new knob, reload semantics, and QUIC limitation note. |
| doc/admin-guide/monitoring/statistics/core/ssl.en.rst | Documents the new ssl_multicert_load_failures counter. |
71a042c to
b8ab221
Compare
| const bool hasAnyCert = lookup->count() > 0; | ||
| const bool partialCommit = !retStatus && params->configPartialReload && hasAnyCert; | ||
| const bool initialLoad = (configid == 0); |
There was a problem hiding this comment.
Agreeing with this, and it is my one blocker on the PR. count() defaults to the GENERIC context and returns ssl_storage->count() only. On BoringSSL builds insert() routes EC certificates into ec_storage, so an EC-only multicert config that loads cleanly still reads count() == 0, and the partial reload discards the new lookup and keeps the old config. The feature silently does nothing for exactly the build and cert type many of us run. Counting across both storages, with the EC term guarded for BoringSSL, plus an EC-cert reload scenario, would close it.
bryancall
left a comment
There was a problem hiding this comment.
Nice feature, and the gold test coverage is well above average for a config knob. One blocker before I can approve.
The hasAnyCert gate at SSLConfig.cc:719 uses lookup->count(), which defaults to the GENERIC context and returns only ssl_storage->count(). On BoringSSL builds insert() routes EC certificates into ec_storage, so an EC-only multicert config that loads cleanly still produces count() == 0. The result is that a partial reload with one bad cert plus otherwise-good ECDSA certs discards the whole new lookup and keeps the old config, silently defeating the knob for exactly the build and cert type a lot of us run in production. It fails safe (no crash, old config stays, errors still log) so it is not a security problem, but the feature quietly does nothing there. Please count across both storages, something like count(SSLCertContextType::RSA) + count(SSLCertContextType::EC) with the EC term guarded for BoringSSL, and add a scenario with a good EC (for example prime256v1) cert alongside a bad one that asserts the reload commits. Every current scenario is RSA only, so nothing exercises ec_storage.
Two smaller things, non-blocking:
ssl_multicert_load_failuresnever counts startup failures because the counter pointer is still null during the initial load, yet both doc pages advertise it as the alert signal for cert load failures even in strict mode. Either buffer and flush the startup count after stats init or document that it only covers live reloads.- Scenario D generates the distinct-CN
sni-d.example.comcert "to confirm it was committed" but never issues a handshake to check it is actually served, so the cert is dead setup. Add a curl that verifiesCN=sni-d.example.comis presented, and ideally one proving a previously-good SNI host that fails on reload stops serving its old cert.
b8ab221 to
f88f28d
Compare
|
@bryancall Thanks for the thorough review. I've pushed an update that addresses all your points: Blocker addressed:
Non-blocking points addressed:
|
f88f28d to
e144a67
Compare
|
[approve ci autest 2] |
5b45b35 to
bef5b78
Compare
The all-or-nothing reload behavior means a single misconfigured cert in ssl_multicert.yaml blocks every other cert update until the broken entry is fixed. When partial_reload is on, only the failing entry is skipped; cleanly-loaded certs go live immediately. The commit gate tracks user_cert_count: a counter incremented in _load_items only when a user-specified cert loads successfully. load() guarantees ssl_storage has a default context (inserting a bare bootstrap when no wildcard cert loaded), so a raw storage entry count is non-zero even when every user cert fails, making it an unreliable gate. user_cert_count == 0 correctly rejects a partial commit when there is nothing useful to serve. Skipped certs produce an ERROR in diags.log and increment proxy.process.ssl.ssl_multicert_load_failures, so degraded state is never silent. The counter increment is guarded against nullptr because startup() calls _load_items before SSLInitializeStatistics(); it therefore only covers live traffic_ctl reloads. Default is 0. QUICCertConfig is out of scope.
bef5b78 to
671e280
Compare
When a traffic_ctl config reload encounters a broken entry in ssl_multicert.yaml, the current behavior is all-or-nothing: the entire new SSLCertLookup is thrown away and the old configuration stays active. This is safe but operationally painful, as a single bad cert blocks all other cert updates from going live until the broken one is fixed.
This PR adds proxy.config.ssl.server.multicert.partial_reload (INT, default 0). When set to 1, a reload that finds one or more unloadable certs still commits the rest of the certs rather than discarding everything. Any skipped cert produces an ERROR in diags.log and increments the new proxy.process.ssl.ssl_multicert_load_failures counter, so the degraded state is never silent.
The default (0) keeps the existing strict/atomic behaviour, no existing deployments are affected.
Implementation notes
The commit gate is in SSLCertificateConfig::reconfigure(). The new partialCommit path only activates when configPartialReload == 1 AND lookup->user_cert_count > 0. user_cert_count is a new field on SSLCertLookup, incremented in _load_items only when _store_ssl_ctx succeeds for a user-specified entry. This correctly excludes the bare bootstrap context that load() guarantees in ssl_storage (inserting one when no wildcard cert loaded) so TLS handshakes can progress far enough for the SNI lookup. A raw storage entry count is non-zero even when every user cert fails (either from a successfully-loaded wildcard or from the bootstrap), making it an unreliable gate. user_cert_count handles EC certs correctly too: _store_ssl_ctx is the same code path for both RSA and EC (BoringSSL's routing to ec_storage happens inside _store_single_ssl_ctx), so user_cert_count accurately reflects both types. This correctly handles SNI-only deployments and the all-certs-fail case (user_cert_count == 0 means there is nothing useful to commit).
retStatus is flipped to true after a partial commit only on live reloads (i.e. when configid != 0 at entry). At initial startup the lookup is always committed anyway (via the existing configid == 0 path), and retStatus must stay false so that startup() can still honour exit_on_load_fail. This distinction is captured via const bool initialLoad = (configid == 0) that is read before configProcessor.set() mutates configid.
The new metric increment in _load_items() is guarded by a null pointer check. Unlike other counters that are only incremented from paths that execute after SSLInitializeStatistics(), _load_items() can be called during early startup before the stats block is fully initialized. Because of this, proxy.process.ssl.ssl_multicert_load_failures only counts failures from live traffic_ctl reloads (startup failures are visible only in diags.log). Both documentation pages note this limitation explicitly.
This knob applies only to SSLCertificateConfig (TLS server certs). QUICCertConfig (HTTP/3) retains strict all-or-nothing semantics and is documented as a known limitation.
Files changed
Testing
Gold test ssl_multicert_partial_reload covers seven scenarios:
No regressions against existing SSL/TLS gold tests (ssl_multicert_loader, exit_on_cert_load_fail, tls_check_cert_selection_reload, tls_reload_under_load, tls_check_cert_selection, tls_check_dual_cert_selection).
Known limitations / future work