bneradt commented on code in PR #13576:
URL: https://github.com/apache/trafficserver/pull/13576#discussion_r3875209872
##########
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:
Good catch, and yes it does. `SSLCreateClientContext` reads the PEM straight
from
disk, so the contexts this function replaces are correct, but
`params->secrets`
still holds the pre-update PEM keyed by the resolved certificate path.
Anything
built by `getCTX` afterwards goes through `getOrLoadSecret`, so it would come
back with the old certificate. The reachable case is a CA bucket that does
not
exist yet: sni.yaml and each distinct `ssl_client_ca_cert_name`
conf-override get
their own top-level bucket, and a bucket that has never been used is created
lazily on the first outbound connection through it, which can be long after
the
rotation.
Fixed by dropping the cached entries for the certificate and key before
building
the replacements, so the next `getOrLoadSecret` reloads them — through a
`TS_LIFECYCLE_SSL_SECRET_HOOK` provider if one is registered, and otherwise
from
the file. I invalidated rather than calling `setSecret` with the file
contents so
that a plugin acting as the secret source still wins, and so the load path
is not
duplicated here.
The AuTest now covers it. There is a third mapping, `/late-ca`, with its own
CA
configuration that is deliberately not requested until after the rotation,
so its
client context is constructed from scratch post-update. It asserts
`bob.com`, and
it reports `alice.com` if the cached data is left in place.
##########
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:
Done. The install pass now locates every target under the map lock and writes
nothing unless all of them were found, so the live map is either updated
completely or left untouched.
##########
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:
Those three files are already removed by this PR rather than emptied — the
commit
carries `delete mode 100644` for `gold/update.gold`,
`gold/client-cert-pre.gold`,
and `gold/client-cert-after.gold`. Only `server-cert-pre.gold` and
`server-cert-after.gold` remain, and both are still referenced by the
`Server-Cert-Pre` and `Server-Cert-After` runs.
--
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]
