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:
|
When this knob is set to 1, a live traffic_ctl config reload that
hits one or more certificate load failures now commits the partial
SSLCertLookup (all certs that loaded cleanly) rather than discarding
the entire new configuration and keeping the old one.
The commit gate in reconfigure() checks count(SSLCertContextType::RSA)
and count(SSLCertContextType::EC) to verify that at least one cert was
successfully inserted before committing the partial result. Both storages
are checked explicitly because BoringSSL routes EC certs into a separate
ec_storage, so checking count(RSA) alone would return 0 for an EC-only
configuration, silently defeating the knob for that build and cert type.
This correctly handles SNI-only deployments (no dest_ip: "*" entry, which
always have a bare bootstrap context as ssl_default) and the all-certs-fail
case (both counts == 0 means nothing useful to commit).
lookup is committed unconditionally via the configid==0 path anyway,
and retStatus must stay false so that startup() can still honour
exit_on_load_fail. The initial value of configid is captured before
configProcessor.set() mutates it.
Any skipped cert still produces an ERROR entry in diags.log and
increments the new proxy.process.ssl.ssl_multicert_load_failures
counter, so the degraded state is never silent. The counter is also
incremented in strict mode (0) for use as an alert signal.
The null-check guard on the counter increment in SSLUtils.cc is
required because SSLCertificateConfig::startup() (which calls
_load_items()) always runs before SSLInitializeStatistics() in the
normal startup path, so the counter pointer is nullptr during the initial
cert load. The counter therefore only covers live reloads via traffic_ctl,
not initial startup failures. The documentation reflects this limitation.
Default is 0, preserving the existing strict/atomic behavior.
This knob applies only to SSLCertificateConfig. QUICCertConfig
retains strict semantics and is documented as a known limitation.
Add a gold test covering seven scenarios:
- A: strict mode, bad cert, reload fails, old config retained,
counter increments even in strict mode.
- B: partial mode + wildcard default, bad cert skipped, new cert
committed (verified by distinct CN), specific SSLError()
message confirmed in diags.log, counter >= 1.
- C: SNI-only (no dest_ip: "*"), partial reload commits a newly-
generated cert (CN=sni-c-new.example.com). Distinct CN proves
the new config was committed, not the old server.pem retained.
- D: wildcard default fails, SNI cert (CN=sni-d.example.com) succeeds,
partial commit still occurs (count(RSA|EC)>0 despite ssl_default
being a bare context). Handshake to sni-d.example.com verifies
the cert is actually served.
- E: all certs fail with partial_reload=1, reload reported as failed
(both counts==0 keeps retStatus false), old config stays active,
counter increments. Pins the count==0 behavior against reversion.
- F: EC cert (prime256v1, CN=ec.example.com) and RSA cert alongside a
bad entry. On BoringSSL both ec_storage and ssl_storage are
populated (count(EC)>0 AND count(RSA)>0). Handshakes verify both
CN=ec.example.com and CN=rsa-f.example.com are served. Exercises
the ec_storage path and dual-cert partial reload.
- G: previously-good SNI host stops serving its old cert after it
fails on partial reload. alpha.example.com (CN=alpha) served
before reload. After alpha fails, alpha falls back to the wildcard
default context (ExcludesExpression: CN=alpha no longer presented).
beta (unchanged) and gamma (new) remain correctly served. Pins
the 'stale cert not retained' behavior across the atomic lookup swap.
f88f28d to
e144a67
Compare
When a
traffic_ctl config reloadencounters a broken entry inssl_multicert.yaml, the current behavior is all-or-nothing: the entire newSSLCertLookupis 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 indiags.logand increments the newproxy.process.ssl.ssl_multicert_load_failurescounter, 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 newpartialCommitpath only activates whenconfigPartialReload == 1ANDcount(RSA|EC) > 0. The count check callsSSLCertLookup::count(SSLCertContextType::RSA)andSSLCertLookup::count(SSLCertContextType::EC)to verify that at least one real cert was successfully inserted. Both storages are checked explicitly because BoringSSL routes EC certificates into a separateec_storage, checkingcount(RSA)alone returns 0 for an EC-only configuration, silently defeating the knob for that build and cert type. This correctly handles SNI-only deployments (nodest_ip: "*"entry, which always have a bare bootstrap context asssl_default) and the all-certs-fail case (both counts == 0 means there is nothing useful to commit).retStatusis flipped to true after a partial commit only on live reloads (i.e. whenconfigid != 0at entry). At initial startup the lookup is always committed anyway (via the existingconfigid == 0path), andretStatusmust stay false so thatstartup()can still honourexit_on_load_fail. This distinction is captured viaconst bool initialLoad = (configid == 0)that is read beforeconfigProcessor.set()mutatesconfigid.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 afterSSLInitializeStatistics(),_load_items()can be called during early startup before the stats block is fully initialized. Because of this,proxy.process.ssl.ssl_multicert_load_failuresonly counts failures from livetraffic_ctlreloads (startup failures are visible only indiags.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
src/records/RecordsConfig.ccRECU_DYNAMIC)src/iocore/net/P_SSLConfig.hconfigPartialReloadfieldsrc/iocore/net/SSLConfig.ccreconfigure()with startup guardsrc/iocore/net/SSLStats.h/.ccssl_multicert_load_failuressrc/iocore/net/SSLUtils.ccdoc/admin-guide/files/records.yaml.en.rst:reloadable:, BoringSSL note, startup caveat)doc/admin-guide/monitoring/statistics/core/ssl.en.rsttests/gold_tests/tls/ssl_multicert_partial_reload.test.pyTesting
Gold test
ssl_multicert_partial_reloadcovers seven scenarios:ssl_multicert_load_failurescounter increments even in strict mode.dest_ip: "*". bad cert skipped, new default cert (distinct CNreloaded.example.com) committed, proving new config was applied and not old one silently retained. Verifies ERROR indiags.logand counter >= 1.dest_ip: "*"entry (CDN-style). A newly-generated cert (CN=sni-c-new.example.com) is committed alongside a bad cert, the distinct CN proves the new config was committed and not the oldserver.pemretained.count(RSA|EC) > 0despitessl_defaultbeing a bare bootstrap context). Handshake tosni-d.example.comconfirms the SNI cert is actually served.partial_reload=1. verifies reload is reported as failed (both counts == 0 keepsretStatusfalse), old config stays active, and failure counter still increments.ec_storageandssl_storageare populated (count(EC) > 0ANDcount(RSA) > 0). Handshakes verify bothCN=ec.example.comandCN=rsa-f.example.comare served. Exercises theec_storagecode path that all-RSA scenarios leave uncovered.CN=alpha.example.com) is served before reload. After alpha's cert fails,ExcludesExpressionverifies alpha no longer presents its old cert (falls back to wildcard default). Beta (unchanged) and gamma (new) remain correctly served. Pins the 'stale cert not retained' behavior across the atomic lookup swap.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
QUICCertConfig(HTTP/3) does not support partial reload yet.proxy.process.ssl.ssl_multicert_load_failuresonly counts live-reload failures, not startup failures (stats are not yet initialized at startup).