serrislew commented on code in PR #13576:
URL: https://github.com/apache/trafficserver/pull/13576#discussion_r3858188056
##########
src/api/InkAPI.cc:
##########
@@ -8238,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()) {
+ auto ctx_iter = ca_iter->second.find(key);
+
+ if (ctx_iter != ca_iter->second.end()) {
+ ctx_iter->second = std::move(client_ctx);
+ continue;
+ }
}
- ink_mutex_release(&map_lock);
+ updated_all = false;
}
+ ink_mutex_release(&map_lock);
Review Comment:
Doesn't `params->secrets` need updating too? `getCTX` builds contexts from
`SSLConfigParams::secrets`, which still holds the pre-update PEM after this
returns
--
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]
