TLS: add proxy.config.ssl.server.multicert.partial_reload#13373
Open
Jabrique wants to merge 1 commit into
Open
TLS: add proxy.config.ssl.server.multicert.partial_reload#13373Jabrique wants to merge 1 commit into
Jabrique wants to merge 1 commit into
Conversation
Jabrique
force-pushed
the
feat/ssl-multicert-partial-reload
branch
from
324571d to
a632611
Compare
Jabrique
force-pushed
the
feat/ssl-multicert-partial-reload
branch
from
a632611 to
71a042c
Compare
Contributor
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. |
Comment on lines
+200
to
+202
| # The bad cert must still produce an ERROR log entry (partial does not silence errors) | ||
| ts_partial.Disk.diags_log.Content = Testers.IncludesExpression('ERROR', 'bad cert should still produce an error log entry') | ||
|
|
Author
There was a problem hiding this comment.
Fixed. 'ERROR' is too broad. One note on the suggested fix though "Failed to load certificate 'does_not_exist.pem'" is from errata.note(), which goes to the JSONRPC output, not diags.log. What actually shows up in the log is from SSLError():
failed to load certificate secret for .../does_not_exist.pem with key path ...
Comment on lines
+1966
to
+1968
| // Guard required: _load_items() runs before SSLInitializeStatistics() during | ||
| // early startup in some configurations (e.g. tests). Other counters are | ||
| // incremented only from paths that cannot execute before stats init. |
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() uses lookup->count() to verify that
at least one cert was successfully inserted into ssl_storage before
committing the partial result. This correctly handles both SNI-only
deployments (no dest_ip: "*" entry, which always have a bare bootstrap
context as ssl_default) and the all-certs-fail case (count == 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.
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 five 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 succeeds and good
SNI cert remains accessible.
- D: wildcard default fails, SNI cert succeeds, partial commit still
occurs (count()>0 despite ssl_default being bare context).
- E: all certs fail with partial_reload=1, reload reported as failed
(count()==0 keeps retStatus false), old config stays active,
counter increments. Pins the count()==0 behavior against
accidental reversion.
Jabrique
force-pushed
the
feat/ssl-multicert-partial-reload
branch
from
71a042c to
b8ab221
Compare
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.
When a
traffic_ctl config reloadencounters a broken entry inssl_multicert.yaml, the current behavior is all-or-nothing: theentire new SSLCertLookup is thrown away and the old configuration stays
active. This is safe but operationally painful, 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_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 == 1AND
lookup->count() > 0. The count check usesSSLCertLookup::count()to verify that at least one real cert was successfully inserted into
ssl_storage. This correctly handles both SNI-only deployments (no
dest_ip: "*"entry) and the all-certs-fail case (count == 0 meansthere is nothing useful to commit). An earlier guard based on
SSL_CTX_get0_certificate(lookup->ssl_default)was replaced becauseit silently disabled partial reload for SNI-only configurations, which
always have a bare bootstrap context as ssl_default regardless of
whether other certs loaded successfully.
retStatusis flipped totrueafter a partial commit only onlive reloads (i.e. when
configid != 0at entry). At initialstartup the lookup is always committed anyway (via the existing
configid == 0path), andretStatusmust stayfalseso thatstartup()can still honourexit_on_load_fail. This distinctionis captured via a
const bool initialLoad = (configid == 0)that isread before
configProcessor.set()mutatesconfigid.The new metric increment in
_load_items()is guarded by a nullpointer 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. The guard is intentional defensive programming, not an
inconsistency.
This knob applies only to
SSLCertificateConfig(TLS server certs).QUICCertConfig(HTTP/3) retains strict all-or-nothing semanticsand is documented as a known limitation.
The comment block in
reconfigure()above thehasAnyCert/partialCommitdeclarations explicitly documents both branches:
count()==0keepsretStatusfalse (reload reported as failed, lookup discarded);count()>0with
partial_reload=1flipsretStatusto true after the partial commit.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:, QUIC limitation note)doc/admin-guide/monitoring/statistics/core/ssl.en.rsttests/gold_tests/tls/ssl_multicert_partial_reload.test.pyTesting:
Gold test
ssl_multicert_partial_reloadcovers five scenarios:to fail, old config stays active. Verifies
ssl_multicert_load_failurescounter increments even in strict mode.
dest_ip: "*": bad cert skipped,new default cert (distinct CN
reloaded.example.com) committed, provingnew config was applied, not old one silently retained. Verifies ERROR in
diags.logandssl_multicert_load_failurescounter >= 1.dest_ip: "*"entry (CDN-style). Verifies partial reload succeeds andgood SNI certs remain accessible.
partial commit still occurs (
lookup->count() > 0despitessl_defaultbeing a bare bootstrap context).
partial_reload=1: verifies reloadis reported as failed (
count()==0keepsretStatusfalse), old configstays active, and failure counter still increments. This pins the behavior
documented in the comment above
hasAnyCertagainst accidental reversion.No regressions against the existing
ssl_multicert_loadergold test.Known limitations / future work:
QUICCertConfig(HTTP/3) does not support partial reload yet.