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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
