Copilot commented on code in PR #13576:
URL: https://github.com/apache/trafficserver/pull/13576#discussion_r3848196703
##########
src/api/InkAPI.cc:
##########
@@ -8232,59 +8233,86 @@ TSSslClientCertUpdate(const char *cert_path, const char
*key_path)
return TS_ERROR;
}
- std::string key;
- shared_SSL_CTX client_ctx = nullptr;
- SSLConfigParams *params = SSLConfig::acquire();
+ // --- Pin the active SSL configuration ---
+ //
+ // Keep this configuration generation alive across every early return and
+ // release it automatically when the update finishes.
+ std::string key{cert_path};
+ SSLConfig::scoped_config params;
- // Generate second level key for client context lookup
- swoc::bwprint(key, "{}:{}", cert_path, key_path);
+ // The client context map is keyed by the resolved certificate path.
Dbg(dbg_ctl_ssl_cert_update, "TSSslClientCertUpdate(): Use %.*s as key for
lookup", static_cast<int>(key.size()), key.data());
- if (nullptr != params) {
- // Try to update client contexts maps
- auto &ca_paths_map = params->top_level_ctx_map;
- auto &map_lock = params->ctxMapLock;
- std::string ca_paths_key;
- // First try to locate the client context and its CA path (by top level)
- ink_mutex_acquire(&map_lock);
- for (auto &ca_paths_pair : ca_paths_map) {
- auto &ctx_map = ca_paths_pair.second;
- auto iter = ctx_map.find(key);
- if (iter != ctx_map.end() && iter->second != nullptr) {
- ca_paths_key = ca_paths_pair.first;
- break;
- }
+ if (!params) {
+ return TS_ERROR;
+ }
+
+ auto &ca_paths_map = params->top_level_ctx_map;
+ auto &map_lock = params->ctxMapLock;
+ std::vector<std::string> ca_paths_keys;
+
+ // --- Find every matching CA bucket ---
+ //
+ // A certificate can be used with more than one CA configuration. Snapshot
+ // all matching bucket keys while holding the map lock, then release it
+ // before performing the expensive context construction.
+ ink_mutex_acquire(&map_lock);
+ for (auto const &[ca_paths_key, ctx_map] : ca_paths_map) {
+ if (ctx_map.contains(key)) {
+ ca_paths_keys.push_back(ca_paths_key);
}
- ink_mutex_release(&map_lock);
+ }
+ ink_mutex_release(&map_lock);
+
+ if (ca_paths_keys.empty()) {
+ return TS_ERROR;
+ }
- // Only update on existing
- if (ca_paths_key.empty()) {
+ std::vector<std::pair<std::string, shared_SSL_CTX>> client_contexts;
+
+ // --- Build every replacement context ---
+ //
+ // Build all replacements before changing the live map. If any construction
+ // fails, the existing working contexts remain installed.
+ client_contexts.reserve(ca_paths_keys.size());
+ for (auto const &ca_paths_key : ca_paths_keys) {
+ size_t sep = ca_paths_key.find(':');
+ std::string ca_bundle_file = ca_paths_key.substr(0, sep);
+ std::string ca_bundle_path = ca_paths_key.substr(sep + 1);
+ shared_SSL_CTX client_ctx(SSLCreateClientContext(params,
ca_bundle_path.empty() ? nullptr : ca_bundle_path.c_str(),
+ ca_bundle_file.empty() ?
nullptr : ca_bundle_file.c_str(), cert_path,
+ key_path),
Review Comment:
SSLCreateClientContext() is declared with (ca_bundle_file, ca_bundle_path)
in P_SSLClientUtils.h but defined with (ca_bundle_path, ca_bundle_file) in
SSLClientUtils.cc. This call site passes (path, file), which matches the
definition but contradicts the declaration’s parameter naming, making it easy
to accidentally swap arguments in future changes. Consider aligning the
declaration/definition (or renaming parameters / adding a wrapper with an
unambiguous struct) so call sites are self-documenting.
##########
tests/gold_tests/pluginTest/cert_update/cert_update.test.py:
##########
@@ -116,9 +122,11 @@
"s_server", "openssl s_server -www -key {0}/server1.pem -cert
{0}/server1.pem -accept {1} -Verify 1 -msg".format(
ts.Variables.SSLDir, ts.Variables.s_server_port))
s_server.Ready = When.PortReady(ts.Variables.s_server_port)
-tr.MakeCurlCommand('--verbose --insecure --ipv4 --header "Host: foo.com"
https://localhost:{}'.format(ts.Variables.ssl_port), ts=ts)
+tr.MakeCurlCommand(
+ '--verbose --insecure --ipv4 --header "Host: foo.com"
https://localhost:{}/override-ca'.format(ts.Variables.ssl_port), ts=ts)
tr.Processes.Default.StartBefore(s_server)
-s_server.Streams.all = "gold/client-cert-pre.gold"
+s_server.Streams.All = Testers.ContainsExpression(
+ "alice.com", "The initial outbound connection should use the original
client certificate")
Review Comment:
This pre-update step only exercises the CA-override remap, so the
non-overridden (normal SNI/global) client context may not exist until after the
update. That can mask failures where TSSslClientCertUpdate updates only the
overridden bucket while the default bucket is lazily created later from the
updated file. Add a pre-update request through the normal https://foo.com
mapping (no /override-ca) and assert it uses alice.com, so the post-update
request verifies an in-place context replacement rather than first-time context
creation.
--
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]
