Skip to content

TLS: add proxy.config.ssl.server.multicert.partial_reload#13373

Open
Jabrique wants to merge 1 commit into
apache:masterfrom
Jabrique:feat/ssl-multicert-partial-reload
Open

TLS: add proxy.config.ssl.server.multicert.partial_reload#13373
Jabrique wants to merge 1 commit into
apache:masterfrom
Jabrique:feat/ssl-multicert-partial-reload

Conversation

@Jabrique

@Jabrique Jabrique commented Jul 9, 2026

Copy link
Copy Markdown

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 count(RSA|EC) > 0. The count check calls SSLCertLookup::count(SSLCertContextType::RSA) and SSLCertLookup::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 separate ec_storage, checking count(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 (no dest_ip: "*" entry, which always have a bare bootstrap context as ssl_default) and the all-certs-fail case (both counts == 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

File Change
src/records/RecordsConfig.cc Register new knob (RECU_DYNAMIC)
src/iocore/net/P_SSLConfig.h Add configPartialReload field
src/iocore/net/SSLConfig.cc Partial-commit gate in reconfigure() with startup guard
src/iocore/net/SSLStats.h/.cc New counter ssl_multicert_load_failures
src/iocore/net/SSLUtils.cc Increment counter on cert load failure (with null guard + comment)
doc/admin-guide/files/records.yaml.en.rst Document new knob (:reloadable:, BoringSSL note, startup caveat)
doc/admin-guide/monitoring/statistics/core/ssl.en.rst Document new metric with startup caveat
tests/gold_tests/tls/ssl_multicert_partial_reload.test.py New gold test (7 scenarios)

Testing

Gold test ssl_multicert_partial_reload covers seven scenarios:

  • A: strict mode (default). 1 bad cert causes whole reload to fail, old config stays active. Verifies ssl_multicert_load_failures counter increments even in strict mode.
  • B: partial mode with dest_ip: "*". bad cert skipped, new default cert (distinct CN reloaded.example.com) committed, proving new config was applied and not old one silently retained. Verifies ERROR in diags.log and counter >= 1.
  • C: SNI-only partial reload. config without any 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 old server.pem retained.
  • D: wildcard default fails, SNI cert succeeds. verifies partial commit still occurs (count(RSA|EC) > 0 despite ssl_default being a bare bootstrap context). Handshake to sni-d.example.com confirms the SNI cert is actually served.
  • E: all certs fail with partial_reload=1. verifies reload is reported as failed (both counts == 0 keeps retStatus false), old config stays active, and failure counter still increments.
  • F: EC cert (prime256v1) + 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 code path that all-RSA scenarios leave uncovered.
  • G: previously-good SNI host stops serving its old cert. alpha (CN=alpha.example.com) is served before reload. After alpha's cert fails, ExcludesExpression verifies 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_failures only counts live-reload failures, not startup failures (stats are not yet initialized at startup).

@Jabrique Jabrique marked this pull request as draft July 9, 2026 07:43
@Jabrique Jabrique force-pushed the feat/ssl-multicert-partial-reload branch from 324571d to a632611 Compare July 9, 2026 08:09
@Jabrique Jabrique marked this pull request as ready for review July 9, 2026 08:21
@Jabrique Jabrique force-pushed the feat/ssl-multicert-partial-reload branch from a632611 to 71a042c Compare July 9, 2026 09:55
@JosiahWI JosiahWI added this to the 11.0.0 milestone Jul 9, 2026
@bryancall bryancall requested a review from Copilot July 9, 2026 16:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, default 0) and gate partial-commit behavior in SSLCertificateConfig::reconfigure().
  • Introduce proxy.process.ssl.ssl_multicert_load_failures and 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.

Comment thread tests/gold_tests/tls/ssl_multicert_partial_reload.test.py Outdated
Comment thread src/iocore/net/SSLUtils.cc Outdated
@Jabrique Jabrique force-pushed the feat/ssl-multicert-partial-reload branch from 71a042c to b8ab221 Compare July 10, 2026 02:25
@bryancall bryancall requested a review from Copilot July 13, 2026 22:18
@bryancall bryancall self-requested a review July 13, 2026 22:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread src/iocore/net/SSLConfig.cc Outdated
Comment on lines +717 to +719
const bool hasAnyCert = lookup->count() > 0;
const bool partialCommit = !retStatus && params->configPartialReload && hasAnyCert;
const bool initialLoad = (configid == 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bryancall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_failures never 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.com cert "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 verifies CN=sni-d.example.com is presented, and ideally one proving a previously-good SNI host that fails on reload stops serving its old cert.

@Jabrique Jabrique force-pushed the feat/ssl-multicert-partial-reload branch from b8ab221 to f88f28d Compare July 14, 2026 04:01
@Jabrique

Copy link
Copy Markdown
Author

@bryancall Thanks for the thorough review. I've pushed an update that addresses all your points:

Blocker addressed:

  • Fixed the hasAnyCert gate to explicitly check both storages: count(SSLCertContextType::RSA) > 0 || count(SSLCertContextType::EC) > 0.
  • Added Scenario F to the gold test. It generates an EC cert (prime256v1) and an RSA cert alongside a bad entry.

Non-blocking points addressed:

  • Startup counter limitation: Both ssl.en.rst and records.yaml.en.rst now explicitly state that the counter only tracks failures from live traffic_ctl reloads.

  • Scenario D handshake: Added the missing curl step to verify that CN=sni-d.example.com is actually presented.

  • Previously-good SNI stops serving: I added a brand new Scenario G. It sets up an alpha SNI that is valid initially, but gets replaced with a bad entry on reload. The test asserts that alpha stops serving its old cert (falling back to the wildcard default), while beta continues serving correctly.

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.
@Jabrique Jabrique force-pushed the feat/ssl-multicert-partial-reload branch from f88f28d to e144a67 Compare July 14, 2026 05:51
@Jabrique Jabrique requested a review from bryancall July 15, 2026 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants