Copilot commented on code in PR #13576:
URL: https://github.com/apache/trafficserver/pull/13576#discussion_r3857082366
##########
tests/gold_tests/pluginTest/cert_update/cert_update.test.py:
##########
@@ -96,7 +101,8 @@
tr.Processes.Default.Env = ts.Env
tr.Processes.Default.Command = (
'{0}/traffic_ctl plugin msg cert_update.server
{1}/server2.pem'.format(ts.Variables.BINDIR, ts.Variables.SSLDir))
-ts.Disk.traffic_out.Content = "gold/update.gold"
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "Successfully updated server cert", "The server certificate context should
be updated")
ts.StillRunningAfter = server
Review Comment:
This test no longer references the gold files update.gold,
client-cert-pre.gold, or client-cert-after.gold, but the PR leaves them in
place as empty files. Keeping unused empty gold files around is confusing for
future test maintenance; it would be clearer to delete them (git rm) now that
the checks are expressed inline via Testers.Contains/ExcludesExpression.
##########
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_file.empty() ? nullptr : ca_bundle_file.c_str(),
+ ca_bundle_path.empty() ?
nullptr : ca_bundle_path.c_str(), cert_path,
+ key_path),
+ SSL_CTX_free);
+
+ if (!client_ctx) {
return TS_ERROR;
}
+ client_contexts.emplace_back(ca_paths_key, std::move(client_ctx));
+ }
- // Extract CA related paths
- 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);
-
- // Build new client context
- client_ctx =
- shared_SSL_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),
- SSL_CTX_free);
-
- // Successfully generates a client context, update in the map
- ink_mutex_acquire(&map_lock);
- auto iter = ca_paths_map.find(ca_paths_key);
- if (iter != ca_paths_map.end() && iter->second.count(key)) {
- iter->second[key] = client_ctx;
- } else {
- client_ctx = nullptr;
+ bool updated_all = true;
+
+ // --- Install all replacement contexts ---
+ //
+ // Reacquire the map lock and replace each context only after every
+ // replacement was built successfully.
+ ink_mutex_acquire(&map_lock);
+ for (auto &[ca_paths_key, client_ctx] : client_contexts) {
+ auto ca_iter = ca_paths_map.find(ca_paths_key);
+
+ if (ca_iter != ca_paths_map.end()) {
Review Comment:
The install phase can partially update the live context map (updating some
CA buckets but not others) if the map changes between the snapshot and
installation pass. Since the function returns TS_ERROR in that case, leaving a
partial update behind is surprising and can lead to inconsistent outbound TLS
behavior across CA configurations. Consider doing an all-or-nothing install:
under the map lock, first re-validate that every target bucket/key is still
present, and only then apply all replacements; otherwise return TS_ERROR
without modifying the map.
--
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]
