Fix TSSslClientCertUpdate updates

doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst

@@ -35,6 +35,7 @@ Description
 ===========
 
 :func:`TSSslClientCertUpdate` updates existing client certificates configured in :file:`sni.yaml` or
-`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` should be exact match as provided in
-configurations. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if :arg:`cert_path` exists
-in configuration and reloaded to update the context.
+`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` must match the resolved certificate path used by
+Traffic Server. Relative certificate names in the configuration are resolved against
+`proxy.config.ssl.client.cert.path`. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if
+:arg:`cert_path` exists in the configuration and is reloaded into every matching context.

doc/developer-guide/api/functions/TSSslClientContext.en.rst

@@ -37,8 +37,8 @@ Description
 These functions are used to explore the client contexts that |TS| uses to connect to upstreams.
 
 :func:`TSSslClientContextsNamesGet` can be used to retrieve the entire client context mappings. Note
-that in |TS|, client contexts are stored in a 2-level mapping with ca paths and cert/key
-paths as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context.
+that in |TS|, client contexts are stored in a 2-level mapping with CA paths and the resolved certificate
+path as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context.
 :arg:`result` points to an user allocated array that will hold pointers to lookup key strings and
 :arg:`n` is the size for :arg:`result` array. :arg:`actual`, if valid, will be filled with actual number
 of lookup keys (2 for each context).

src/api/InkAPI.cc

@@ -28,6 +28,7 @@
 #include <string_view>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "iocore/net/NetVConnection.h"
 #include "iocore/net/NetHandler.h"
@@ -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),
+                              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);
 
-  return client_ctx ? TS_SUCCESS : TS_ERROR;
+  return updated_all ? TS_SUCCESS : TS_ERROR;
 }
 
 TSReturnCode

src/iocore/net/P_SSLConfig.h

@@ -138,7 +138,7 @@ struct SSLConfigParams : public ConfigInfo {
 
   // Client contexts are held by 2-level map:
   // The first level maps from CA bundle file&path to next level map;
-  // The second level maps from cert&key to actual SSL_CTX;
+  // The second level maps from the resolved certificate path to the actual SSL_CTX;
   // The second level map owns the client SSL_CTX objects and is responsible for cleaning them up
   using CTX_MAP = std::unordered_map<std::string, shared_SSL_CTX>;
   mutable std::unordered_map<std::string, CTX_MAP> top_level_ctx_map;

tests/gold_tests/pluginTest/cert_update/cert_update.test.py

@@ -26,7 +26,7 @@
 Test.SkipIf(Condition.CurlUsingUnixDomainSocket())
 Test.SkipUnless(
     Condition.HasProgram("openssl", "Openssl need to be installed on system for this test to work"),
-    Condition.PluginExists('cert_update.so'))
+    Condition.PluginExists('cert_update.so'), Condition.PluginExists('conf_remap.so'))
 
 # Set up origin server
 server = Test.MakeOriginServer("server")
@@ -54,6 +54,8 @@
         'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir),
         'proxy.config.ssl.client.cert.path': '{0}'.format(ts.Variables.SSLDir),
         'proxy.config.ssl.client.private_key.path': '{0}'.format(ts.Variables.SSLDir),
+        'proxy.config.ssl.client.CA.cert.path': '{0}'.format(ts.Variables.SSLDir),
+        'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
         'proxy.config.url_remap.pristine_host_hdr': 1
     })
 
@@ -68,6 +70,9 @@
 ts.Disk.remap_config.AddLines(
     [
         'map https://bar.com http://127.0.0.1:{0}'.format(server.Variables.Port),
+        'map https://foo.com/override-ca https://127.0.0.1:{0} @plugin=conf_remap.so '
+        '@pparam=proxy.config.ssl.client.cert.filename=client1.pem '
+        '@pparam=proxy.config.ssl.client.CA.cert.filename=server1.pem'.format(ts.Variables.s_server_port),
         'map https://foo.com https://127.0.0.1:{0}'.format(ts.Variables.s_server_port),
     ])
 
@@ -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
 
 # Server-Cert-After
@@ -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")
 tr.Processes.Default.ReturnCode = 0
 ts.StillRunningAfter = server
 
@@ -128,7 +136,10 @@
 tr.Processes.Default.Command = (
     'mv {0}/client2.pem {0}/client1.pem && {1}/traffic_ctl plugin msg cert_update.client {0}/client1.pem'.format(
         ts.Variables.SSLDir, ts.Variables.BINDIR))
-ts.Disk.traffic_out.Content = "gold/update.gold"
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+    "Successfully updated client cert", "The client certificate context should be updated")
+ts.Disk.traffic_out.Content += Testers.ExcludesExpression(
+    "Failed to update client cert", "The client certificate context update should not fail")
 ts.StillRunningAfter = server
 
 # Client-Cert-After
@@ -143,6 +154,21 @@
 tr.MakeCurlCommand(
     '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}'.format(ts.Variables.ssl_port), ts=ts)
 tr.Processes.Default.StartBefore(s_server)
-s_server.Streams.all = "gold/client-cert-after.gold"
+s_server.Streams.All = Testers.ContainsExpression(
+    "bob.com", "The next outbound connection should use the replacement client certificate")
+tr.Processes.Default.ReturnCode = 0
+ts.StillRunningAfter = server
+
+# Verify that the context under the overridden CA configuration was also updated.
+tr = Test.AddTestRun("Client-Cert-After-CA-Override")
+s_server = tr.Processes.Process(
+    "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.Processes.Default.Env = ts.Env
+tr.MakeCurlCommand(
+    '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}/override-ca'.format(ts.Variables.ssl_port), ts=ts)
+tr.Processes.Default.StartBefore(s_server)
+s_server.Streams.All = Testers.ContainsExpression("bob.com", "The client certificate should be updated for every CA configuration")
 tr.Processes.Default.ReturnCode = 0
 ts.StillRunningAfter = server