You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "Jabrique (via GitHub)" <gi...@apache.org> on 2026/07/09 06:30:28 UTC
[PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Jabrique opened a new pull request, #13373:
URL: https://github.com/apache/trafficserver/pull/13373
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, 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 `SSL_CTX_get0_certificate(lookup->ssl_default.get()) != nullptr`.
The second check distinguishes a real user-loaded default from the
bare bootstrap context that `load()` creates when all certs fail,
committing a bare context would silently break TLS for every
connection.
- `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 a `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. 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.
**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 guard + comment) |
| `doc/admin-guide/files/records.yaml.en.rst` | Document new knob (`:reloadable:`, QUIC limitation note) |
| `doc/admin-guide/monitoring/statistics/core/ssl.en.rst` | Document new metric |
| `tests/gold_tests/tls/ssl_multicert_partial_reload.test.py` | New gold test |
**Testing:**
Gold test `ssl_multicert_partial_reload` covers:
- **Scenario A** strict mode (default): a single bad cert causes the
whole reload to fail and the old config stays intact (verified by
CN of served cert).
- **Scenario B** partial mode: a bad cert is skipped, the newly
generated default cert (distinct CN `reloaded.example.com`) is
committed, proving the new config was applied and not the old one
silently retained. Also verifies:
- `ERROR` still appears in `diags.log` (degraded state is not silent)
- `proxy.process.ssl.ssl_multicert_load_failures` counter is ≥ 1
after the reload (via `traffic_ctl metric get`)
No regressions against the existing `ssl_multicert_loader` gold test.
**Known limitations / future work:**
- `QUICCertConfig` (HTTP/3) does not support partial reload yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-5021604845
[approve ci autest 1]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-5021915287
[approve ci autest 2]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Jabrique (via GitHub)" <gi...@apache.org>.
Jabrique commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-5350256208
@bryancall Thanks for the review! I’ve fixed the issues according to your suggestions, let me know if everything looks good or if I missed anything. Sorry for the messy commits earlier. I added a new commit for the fixes and did a rebase because include/iocore/net/QUICMultiCertConfigLoader.h had a conflict. Let me know if I should have just done a merge instead of a rebase. Apologies for the trouble!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-5342226050
[approve ci autest 2]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3553202656
##########
tests/gold_tests/tls/ssl_multicert_partial_reload.test.py:
##########
@@ -0,0 +1,481 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Test ssl_multicert partial_reload: with the knob on, a single bad cert should not
+block the reload of all other valid certs.
+'''
+
+# ---------------------------------------------------------------------------
+# Scenario A - default behavior (partial_reload=0): one bad cert aborts the
+# entire reload; old config stays fully in place.
+# ---------------------------------------------------------------------------
+
+sni_valid = 'valid.example.com'
+
+ts_strict = Test.MakeATSProcess("ts_strict", enable_tls=True, disable_log_checks=True)
+server_strict = Test.MakeOriginServer("server_strict")
+request_header = {"headers": f"GET / HTTP/1.1\r\nHost: {sni_valid}\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+server_strict.addResponse("sessionlog.json", request_header, response_header)
+
+ts_strict.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts_strict.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts_strict.Variables.SSLDir}',
+ 'proxy.config.ssl.server.multicert.exit_on_load_fail': 0,
+ # partial_reload intentionally left at default (0)
+ })
+
+ts_strict.addDefaultSSLFiles()
+ts_strict.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server_strict.Variables.Port}')
+
+ts_strict.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+tr_strict_1 = Test.AddTestRun("Strict: initial request succeeds")
+tr_strict_1.Processes.Default.StartBefore(Test.Processes.ts_strict)
+tr_strict_1.Processes.Default.StartBefore(server_strict)
+tr_strict_1.StillRunningAfter = ts_strict
+tr_strict_1.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_strict.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_strict.Variables.ssl_port}",
+ ts=ts_strict)
+tr_strict_1.Processes.Default.ReturnCode = 0
+
+# Overwrite config with one bad entry (missing file) + keep the good default
+tr_strict_update = Test.AddTestRun("Strict: inject bad cert into config")
+strict_yaml_path = ts_strict.Disk.ssl_multicert_yaml.AbsPath
+tr_strict_update.Disk.File(strict_yaml_path, id="strict_yaml", typename="ats:config")
+tr_strict_update.Disk.strict_yaml.AddLines(
+ """
+ssl_multicert:
+ - ssl_cert_name: does_not_exist.pem
+ ssl_key_name: does_not_exist.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+tr_strict_update.StillRunningAfter = ts_strict
+tr_strict_update.Processes.Default.Command = 'echo Updated strict config'
+tr_strict_update.Processes.Default.Env = ts_strict.Env
+tr_strict_update.Processes.Default.ReturnCode = 0
+
+# Reload should fail - strict mode rejects the whole config
+tr_strict_reload = Test.AddConfigReload(
+ ts_strict, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Strict: reload expected to fail")
+tr_strict_reload.StillRunningAfter = server_strict
+
+# Old cert still served because reload was rolled back
+tr_strict_2 = Test.AddTestRun("Strict: old cert still served after failed reload")
+tr_strict_2.StillRunningAfter = ts_strict
+tr_strict_2.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_strict.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_strict.Variables.ssl_port}",
+ ts=ts_strict)
+tr_strict_2.Processes.Default.ReturnCode = 0
+tr_strict_2.Processes.Default.Streams.stderr = Testers.IncludesExpression(
+ "CN=example.com", "Old default cert should still be served")
+
+# The counter must be incremented even in strict mode so operators can alert on failures
+# regardless of which reload policy is active.
+tr_strict_metric = Test.AddTestRun("Strict: ssl_multicert_load_failures counter incremented")
+tr_strict_metric.StillRunningAfter = ts_strict
+tr_strict_metric.Processes.Default.Command = (
+ f"{ts_strict.Variables.BINDIR}/traffic_ctl metric get"
+ f" proxy.process.ssl.ssl_multicert_load_failures")
+tr_strict_metric.Processes.Default.Env = ts_strict.Env
+tr_strict_metric.Processes.Default.ReturnCode = 0
+tr_strict_metric.Processes.Default.Streams.stdout = Testers.IncludesExpression(
+ "proxy.process.ssl.ssl_multicert_load_failures [1-9]",
+ "Failure counter must be incremented even when strict mode rolls back the config")
+
+# ---------------------------------------------------------------------------
+# Scenario B - partial_reload=1: a bad cert is skipped, valid certs are
+# applied. The domain whose cert failed falls back to the default context.
+# ---------------------------------------------------------------------------
+
+ts_partial = Test.MakeATSProcess("ts_partial", enable_tls=True, disable_log_checks=True)
+server_partial = Test.MakeOriginServer("server_partial")
+server_partial.addResponse("sessionlog.json", request_header, response_header)
+
+ts_partial.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts_partial.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts_partial.Variables.SSLDir}',
+ 'proxy.config.ssl.server.multicert.exit_on_load_fail': 0,
+ 'proxy.config.ssl.server.multicert.partial_reload': 1,
+ })
+
+ts_partial.addDefaultSSLFiles()
+ts_partial.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server_partial.Variables.Port}')
+
+ts_partial.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+tr_partial_1 = Test.AddTestRun("Partial: initial request succeeds")
+tr_partial_1.Processes.Default.StartBefore(Test.Processes.ts_partial)
+tr_partial_1.Processes.Default.StartBefore(server_partial)
+tr_partial_1.StillRunningAfter = ts_partial
+tr_partial_1.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_partial.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_partial.Variables.ssl_port}",
+ ts=ts_partial)
+tr_partial_1.Processes.Default.ReturnCode = 0
+
+# Inject a mix: one bad entry, one NEWLY-GENERATED good default (different CN)
+# The new default cert has CN=reloaded.example.com so that after the partial reload
+# we can verify the new config was committed rather than the old one retained.
+tr_partial_update = Test.AddTestRun("Partial: inject bad cert alongside new valid default")
+partial_yaml_path = ts_partial.Disk.ssl_multicert_yaml.AbsPath
+ssl_dir = ts_partial.Variables.SSLDir
+
+# Generate a new self-signed cert with a distinct CN in the same SSL dir
+new_cert = f"{ssl_dir}/newdefault.pem"
+new_key = f"{ssl_dir}/newdefault.key"
+gen_cmd = (
+ f"openssl req -x509 -newkey rsa:2048 "
+ f"-keyout {new_key} -out {new_cert} "
+ f"-days 365 -nodes -subj '/CN=reloaded.example.com' "
+ f"2>/dev/null")
+
+tr_partial_update.Disk.File(partial_yaml_path, id="partial_yaml", typename="ats:config")
+tr_partial_update.Disk.partial_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - ssl_cert_name: does_not_exist.pem
+ ssl_key_name: does_not_exist.key
+ - dest_ip: "*"
+ ssl_cert_name: newdefault.pem
+ ssl_key_name: newdefault.key
+""".split("\n"))
+tr_partial_update.StillRunningAfter = ts_partial
+tr_partial_update.Processes.Default.Command = f"{gen_cmd} && echo Updated partial config"
+tr_partial_update.Processes.Default.Env = ts_partial.Env
+tr_partial_update.Processes.Default.ReturnCode = 0
+
+# Reload should succeed despite the bad entry (partial_reload=1)
+tr_partial_reload = Test.AddConfigReload(
+ ts_partial,
+ expect="success",
+ expect_tasks=["ssl_multicert.yaml"],
+ description="Partial: reload expected to succeed despite bad cert")
+tr_partial_reload.StillRunningAfter = server_partial
+
+# Default cert is still reachable and the CN proves the NEW config was committed,
+# not the old server.pem (CN=example.com) that was kept.
+tr_partial_2 = Test.AddTestRun("Partial: new cert committed and served after partial reload")
+tr_partial_2.StillRunningAfter = ts_partial
+tr_partial_2.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_partial.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_partial.Variables.ssl_port}",
+ ts=ts_partial)
+tr_partial_2.Processes.Default.ReturnCode = 0
+tr_partial_2.Processes.Default.Streams.stderr = Testers.IncludesExpression(
+ "CN=reloaded.example.com", "New default cert (not old server.pem) must be served")
+
+# 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')
+
Review Comment:
The test currently only asserts that diags.log contains the string 'ERROR'. This can produce false positives (e.g., an unrelated ERROR would satisfy the assertion even if the expected cert-load failure error wasn’t logged), weakening the coverage for the requirement that skipped certs emit an error. Prefer matching the specific multicert loader error and/or the failing filename.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1963,6 +1963,12 @@ SSLMultiCertConfigLoader::_load_items(SSLCertLookup *lookup, config::SSLMultiCer
std::lock_guard<std::mutex> lock(_loader_mutex);
errata.note(ERRATA_ERROR, "Failed to load certificate '{}' at item {}",
sslMultiCertSettings->cert ? sslMultiCertSettings->cert : "(unnamed)", item_num);
+ // 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.
Review Comment:
This comment says _load_items() runs before SSLInitializeStatistics() only in “some configurations (e.g. tests)”, but the normal startup path calls SSLCertificateConfig::startup() before SSLInitializeStatistics() (see SSLNetProcessor::start()). Updating the wording would avoid misleading future readers about when this guard is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-4963319372
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Jabrique (via GitHub)" <gi...@apache.org>.
Jabrique commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3556005488
##########
tests/gold_tests/tls/ssl_multicert_partial_reload.test.py:
##########
@@ -0,0 +1,481 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Test ssl_multicert partial_reload: with the knob on, a single bad cert should not
+block the reload of all other valid certs.
+'''
+
+# ---------------------------------------------------------------------------
+# Scenario A - default behavior (partial_reload=0): one bad cert aborts the
+# entire reload; old config stays fully in place.
+# ---------------------------------------------------------------------------
+
+sni_valid = 'valid.example.com'
+
+ts_strict = Test.MakeATSProcess("ts_strict", enable_tls=True, disable_log_checks=True)
+server_strict = Test.MakeOriginServer("server_strict")
+request_header = {"headers": f"GET / HTTP/1.1\r\nHost: {sni_valid}\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+server_strict.addResponse("sessionlog.json", request_header, response_header)
+
+ts_strict.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts_strict.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts_strict.Variables.SSLDir}',
+ 'proxy.config.ssl.server.multicert.exit_on_load_fail': 0,
+ # partial_reload intentionally left at default (0)
+ })
+
+ts_strict.addDefaultSSLFiles()
+ts_strict.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server_strict.Variables.Port}')
+
+ts_strict.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+tr_strict_1 = Test.AddTestRun("Strict: initial request succeeds")
+tr_strict_1.Processes.Default.StartBefore(Test.Processes.ts_strict)
+tr_strict_1.Processes.Default.StartBefore(server_strict)
+tr_strict_1.StillRunningAfter = ts_strict
+tr_strict_1.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_strict.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_strict.Variables.ssl_port}",
+ ts=ts_strict)
+tr_strict_1.Processes.Default.ReturnCode = 0
+
+# Overwrite config with one bad entry (missing file) + keep the good default
+tr_strict_update = Test.AddTestRun("Strict: inject bad cert into config")
+strict_yaml_path = ts_strict.Disk.ssl_multicert_yaml.AbsPath
+tr_strict_update.Disk.File(strict_yaml_path, id="strict_yaml", typename="ats:config")
+tr_strict_update.Disk.strict_yaml.AddLines(
+ """
+ssl_multicert:
+ - ssl_cert_name: does_not_exist.pem
+ ssl_key_name: does_not_exist.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+tr_strict_update.StillRunningAfter = ts_strict
+tr_strict_update.Processes.Default.Command = 'echo Updated strict config'
+tr_strict_update.Processes.Default.Env = ts_strict.Env
+tr_strict_update.Processes.Default.ReturnCode = 0
+
+# Reload should fail - strict mode rejects the whole config
+tr_strict_reload = Test.AddConfigReload(
+ ts_strict, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Strict: reload expected to fail")
+tr_strict_reload.StillRunningAfter = server_strict
+
+# Old cert still served because reload was rolled back
+tr_strict_2 = Test.AddTestRun("Strict: old cert still served after failed reload")
+tr_strict_2.StillRunningAfter = ts_strict
+tr_strict_2.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_strict.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_strict.Variables.ssl_port}",
+ ts=ts_strict)
+tr_strict_2.Processes.Default.ReturnCode = 0
+tr_strict_2.Processes.Default.Streams.stderr = Testers.IncludesExpression(
+ "CN=example.com", "Old default cert should still be served")
+
+# The counter must be incremented even in strict mode so operators can alert on failures
+# regardless of which reload policy is active.
+tr_strict_metric = Test.AddTestRun("Strict: ssl_multicert_load_failures counter incremented")
+tr_strict_metric.StillRunningAfter = ts_strict
+tr_strict_metric.Processes.Default.Command = (
+ f"{ts_strict.Variables.BINDIR}/traffic_ctl metric get"
+ f" proxy.process.ssl.ssl_multicert_load_failures")
+tr_strict_metric.Processes.Default.Env = ts_strict.Env
+tr_strict_metric.Processes.Default.ReturnCode = 0
+tr_strict_metric.Processes.Default.Streams.stdout = Testers.IncludesExpression(
+ "proxy.process.ssl.ssl_multicert_load_failures [1-9]",
+ "Failure counter must be incremented even when strict mode rolls back the config")
+
+# ---------------------------------------------------------------------------
+# Scenario B - partial_reload=1: a bad cert is skipped, valid certs are
+# applied. The domain whose cert failed falls back to the default context.
+# ---------------------------------------------------------------------------
+
+ts_partial = Test.MakeATSProcess("ts_partial", enable_tls=True, disable_log_checks=True)
+server_partial = Test.MakeOriginServer("server_partial")
+server_partial.addResponse("sessionlog.json", request_header, response_header)
+
+ts_partial.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts_partial.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts_partial.Variables.SSLDir}',
+ 'proxy.config.ssl.server.multicert.exit_on_load_fail': 0,
+ 'proxy.config.ssl.server.multicert.partial_reload': 1,
+ })
+
+ts_partial.addDefaultSSLFiles()
+ts_partial.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server_partial.Variables.Port}')
+
+ts_partial.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+tr_partial_1 = Test.AddTestRun("Partial: initial request succeeds")
+tr_partial_1.Processes.Default.StartBefore(Test.Processes.ts_partial)
+tr_partial_1.Processes.Default.StartBefore(server_partial)
+tr_partial_1.StillRunningAfter = ts_partial
+tr_partial_1.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_partial.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_partial.Variables.ssl_port}",
+ ts=ts_partial)
+tr_partial_1.Processes.Default.ReturnCode = 0
+
+# Inject a mix: one bad entry, one NEWLY-GENERATED good default (different CN)
+# The new default cert has CN=reloaded.example.com so that after the partial reload
+# we can verify the new config was committed rather than the old one retained.
+tr_partial_update = Test.AddTestRun("Partial: inject bad cert alongside new valid default")
+partial_yaml_path = ts_partial.Disk.ssl_multicert_yaml.AbsPath
+ssl_dir = ts_partial.Variables.SSLDir
+
+# Generate a new self-signed cert with a distinct CN in the same SSL dir
+new_cert = f"{ssl_dir}/newdefault.pem"
+new_key = f"{ssl_dir}/newdefault.key"
+gen_cmd = (
+ f"openssl req -x509 -newkey rsa:2048 "
+ f"-keyout {new_key} -out {new_cert} "
+ f"-days 365 -nodes -subj '/CN=reloaded.example.com' "
+ f"2>/dev/null")
+
+tr_partial_update.Disk.File(partial_yaml_path, id="partial_yaml", typename="ats:config")
+tr_partial_update.Disk.partial_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - ssl_cert_name: does_not_exist.pem
+ ssl_key_name: does_not_exist.key
+ - dest_ip: "*"
+ ssl_cert_name: newdefault.pem
+ ssl_key_name: newdefault.key
+""".split("\n"))
+tr_partial_update.StillRunningAfter = ts_partial
+tr_partial_update.Processes.Default.Command = f"{gen_cmd} && echo Updated partial config"
+tr_partial_update.Processes.Default.Env = ts_partial.Env
+tr_partial_update.Processes.Default.ReturnCode = 0
+
+# Reload should succeed despite the bad entry (partial_reload=1)
+tr_partial_reload = Test.AddConfigReload(
+ ts_partial,
+ expect="success",
+ expect_tasks=["ssl_multicert.yaml"],
+ description="Partial: reload expected to succeed despite bad cert")
+tr_partial_reload.StillRunningAfter = server_partial
+
+# Default cert is still reachable and the CN proves the NEW config was committed,
+# not the old server.pem (CN=example.com) that was kept.
+tr_partial_2 = Test.AddTestRun("Partial: new cert committed and served after partial reload")
+tr_partial_2.StillRunningAfter = ts_partial
+tr_partial_2.MakeCurlCommand(
+ f"-q -s -v -k --resolve '{sni_valid}:{ts_partial.Variables.ssl_port}:127.0.0.1' "
+ f"https://{sni_valid}:{ts_partial.Variables.ssl_port}",
+ ts=ts_partial)
+tr_partial_2.Processes.Default.ReturnCode = 0
+tr_partial_2.Processes.Default.Streams.stderr = Testers.IncludesExpression(
+ "CN=reloaded.example.com", "New default cert (not old server.pem) must be served")
+
+# 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')
+
Review 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 ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Jabrique (via GitHub)" <gi...@apache.org>.
Jabrique commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3556010416
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1963,6 +1963,12 @@ SSLMultiCertConfigLoader::_load_items(SSLCertLookup *lookup, config::SSLMultiCer
std::lock_guard<std::mutex> lock(_loader_mutex);
errata.note(ERRATA_ERROR, "Failed to load certificate '{}' at item {}",
sslMultiCertSettings->cert ? sslMultiCertSettings->cert : "(unnamed)", item_num);
+ // 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.
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Jabrique (via GitHub)" <gi...@apache.org>.
Jabrique commented on PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#issuecomment-4965414120
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3575055984
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -703,8 +705,26 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx)
// If the load succeeded, load it. If there is no current configuration, load even a broken
// config so that a bad initial load doesn't completely disable TLS.
- if (retStatus || configid == 0) {
+ // If partial_reload is enabled and at least one cert was successfully inserted, commit the
+ // partial lookup so that a single bad cert does not block all other certs from being updated.
+ // lookup->count() is used rather than checking ssl_default, because ssl_default is always
+ // populated with a bare bootstrap context (no X.509) for SNI-only deployments that have no
+ // dest_ip: "*" entry, checking ssl_default would silently disable partial reload for those.
+ // count() == 0 means all certs failed: hasAnyCert is false, partialCommit is false,
+ // the lookup is discarded, and retStatus stays false so traffic_ctl reports failure.
+ // When count() > 0 and partial_reload is on, retStatus is flipped to true after the
+ // partial commit so that traffic_ctl reports the reload as successful.
+ const bool hasAnyCert = lookup->count() > 0;
+ const bool partialCommit = !retStatus && params->configPartialReload && hasAnyCert;
+ const bool initialLoad = (configid == 0);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3865670882
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -703,8 +705,26 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx)
// If the load succeeded, load it. If there is no current configuration, load even a broken
// config so that a bad initial load doesn't completely disable TLS.
- if (retStatus || configid == 0) {
+ // If partial_reload is enabled and at least one cert was successfully inserted, commit the
+ // partial lookup so that a single bad cert does not block all other certs from being updated.
+ // lookup->count() is used rather than checking ssl_default, because ssl_default is always
+ // populated with a bare bootstrap context (no X.509) for SNI-only deployments that have no
+ // dest_ip: "*" entry, checking ssl_default would silently disable partial reload for those.
+ // count() == 0 means all certs failed: hasAnyCert is false, partialCommit is false,
+ // the lookup is discarded, and retStatus stays false so traffic_ctl reports failure.
+ // When count() > 0 and partial_reload is on, retStatus is flipped to true after the
+ // partial commit so that traffic_ctl reports the reload as successful.
+ const bool hasAnyCert = lookup->count() > 0;
+ const bool partialCommit = !retStatus && params->configPartialReload && hasAnyCert;
+ const bool initialLoad = (configid == 0);
Review Comment:
Resolved. `hasAnyCert` no longer calls `count()` at all. It reads `lookup->user_cert_count`, which is incremented in `_load_items()` above the RSA/EC/GENERIC routing decision, so the BoringSSL `ec_storage` split cannot affect it on any library. That also avoids the second bug summing the storages would have left in place, since `load()` inserts a bare bootstrap context when no wildcard certificate loaded. The follow-on defect where a tunnel entry satisfied the gate is fixed as well, so the count is certificate-bearing entries only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TLS: add proxy.config.ssl.server.multicert.partial_reload [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13373:
URL: https://github.com/apache/trafficserver/pull/13373#discussion_r3574578006
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -703,8 +705,26 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx)
// If the load succeeded, load it. If there is no current configuration, load even a broken
// config so that a bad initial load doesn't completely disable TLS.
- if (retStatus || configid == 0) {
+ // If partial_reload is enabled and at least one cert was successfully inserted, commit the
+ // partial lookup so that a single bad cert does not block all other certs from being updated.
+ // lookup->count() is used rather than checking ssl_default, because ssl_default is always
+ // populated with a bare bootstrap context (no X.509) for SNI-only deployments that have no
+ // dest_ip: "*" entry, checking ssl_default would silently disable partial reload for those.
+ // count() == 0 means all certs failed: hasAnyCert is false, partialCommit is false,
+ // the lookup is discarded, and retStatus stays false so traffic_ctl reports failure.
+ // When count() > 0 and partial_reload is on, retStatus is flipped to true after the
+ // partial commit so that traffic_ctl reports the reload as successful.
+ const bool hasAnyCert = lookup->count() > 0;
+ const bool partialCommit = !retStatus && params->configPartialReload && hasAnyCert;
+ const bool initialLoad = (configid == 0);
Review Comment:
`hasAnyCert` uses `lookup->count()` with the default context type, which only counts `ssl_storage` entries. In BoringSSL builds, EC certificates are stored in `ec_storage` and will not be counted, so an EC-only multicert config would incorrectly be treated as `count()==0` and prevent partial commits even though certs loaded successfully. Consider checking both RSA and EC counts (or otherwise counting across storages).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org