Fix TSSslClientCertUpdate updates

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

@@ -35,6 +35,11 @@ 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.
+
+Any certificate data cached for :arg:`cert_path` and :arg:`key_path` is discarded as well, so client
+contexts that Traffic Server creates after the update also use the new certificate rather than the
+previously cached one.

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,106 @@ 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;
+  }
+
+  // --- Drop the cached certificate data ---
+  //
+  // getCTX() builds contexts from the cached secret data rather than from the
+  // files. Drop the cached copies so that a context built later for a CA
+  // bucket that does not exist yet also picks up the updated certificate
+  // instead of the pre-update PEM.
+  params->secrets.invalidateSecret(key);
+  if (key_path != nullptr && key_path[0] != '\0') {
+    params->secrets.invalidateSecret(key_path);
+  }
 
-    // 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;
+  std::vector<shared_SSL_CTX *> targets;
+
+  // --- Install all replacement contexts ---
+  //
+  // Reacquire the map lock and locate every target before overwriting any of
+  // them, so that the live map is either updated completely or left untouched.
+  targets.reserve(client_contexts.size());
+  ink_mutex_acquire(&map_lock);
+  for (auto const &client_context : client_contexts) {
+    auto ca_iter = ca_paths_map.find(client_context.first);
+
+    if (ca_iter == ca_paths_map.end()) {
+      break;
+    }
+    auto ctx_iter = ca_iter->second.find(key);
+
+    if (ctx_iter == ca_iter->second.end()) {
+      break;
+    }
+    targets.push_back(&ctx_iter->second);
+  }
+
+  bool const updated_all = targets.size() == client_contexts.size();
+
+  if (updated_all) {
+    for (std::size_t i = 0; i < targets.size(); ++i) {
+      *targets[i] = std::move(client_contexts[i].second);
     }
-    ink_mutex_release(&map_lock);
   }
+  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;

src/iocore/net/P_SSLSecret.h

@@ -34,6 +34,13 @@ class SSLSecret
   void        setSecret(const std::string &name, std::string_view data);
   void        getOrLoadSecret(const std::string &name1, const std::string &name2, std::string &data, std::string &data2);
 
+  /** Drop any cached data for @a name.
+   *
+   * The next getOrLoadSecret() for @a name reloads the data, either from a
+   * TS_LIFECYCLE_SSL_SECRET_HOOK plugin or from the file itself.
+   */
+  void invalidateSecret(const std::string &name);
+
 private:
   void        loadSecret(const std::string &name1, const std::string &name2, std::string &data_item, std::string &data_item2);
   std::string loadFile(const std::string &name);

src/iocore/net/SSLClientUtils.cc

@@ -338,7 +338,7 @@ SSLInitClientContext(const SSLConfigParams *params)
 }
 
 SSL_CTX *
-SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_path, const char *ca_bundle_file,
+SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_file, const char *ca_bundle_path,
                        const char *cert_path, const char *key_path)
 {
   std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> ctx(nullptr, &SSL_CTX_free);

src/iocore/net/SSLSecret.cc

@@ -119,6 +119,16 @@ SSLSecret::setSecret(const std::string &name, std::string_view data)
   Dbg(dbg_ctl_ssl_secret, "Set secret for %s to %.*s", name.c_str(), int(data.size() > 50 ? 50 : data.size()), data.data());
 }
 
+void
+SSLSecret::invalidateSecret(const std::string &name)
+{
+  std::scoped_lock lock(secret_map_mutex);
+
+  if (secret_map.erase(name) > 0) {
+    Dbg(dbg_ctl_ssl_secret, "Invalidated cached secret for %s", name.c_str());
+  }
+}
+
 std::string
 SSLSecret::getSecret(const std::string &name) const
 {

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,14 @@
 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),
+        # This CA configuration is only used after the certificate is updated so
+        # that its client context is created from scratch post-update.
+        'map https://foo.com/late-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=server2.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 +106,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
@@ -110,15 +121,31 @@
 ts.StillRunningAfter = server
 
 # Client-Cert-Pre
-# s_server should see client (Traffic Server) as alice.com
+# s_server should see client (Traffic Server) as alice.com with the default CA configuration.
 tr = Test.AddTestRun("Client-Cert-Pre")
 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.MakeCurlCommand('--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{}'.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 default CA context should initially use the original client certificate")
+tr.Processes.Default.ReturnCode = 0
+ts.StillRunningAfter = server
+
+# Client-Cert-Pre-CA-Override
+# s_server should also see alice.com with the overridden CA configuration.
+tr = Test.AddTestRun("Client-Cert-Pre-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.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 = Testers.ContainsExpression(
+    "alice.com", "The CA override context should initially use the original client certificate")
 tr.Processes.Default.ReturnCode = 0
 ts.StillRunningAfter = server
 
@@ -128,7 +155,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 +173,39 @@
 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
+
+# Client-Cert-After-Late-CA
+# The /late-ca mapping has not been used yet, so its client context is built
+# after the update. It must be built from the new certificate file rather than
+# from the certificate data cached before the update.
+tr = Test.AddTestRun("Client-Cert-After-Late-CA")
+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}/late-ca'.format(ts.Variables.ssl_port), ts=ts)
+tr.Processes.Default.StartBefore(s_server)
+s_server.Streams.All = Testers.ContainsExpression(
+    "bob.com", "A context created after the update should not use the cached pre-update certificate")
 tr.Processes.Default.ReturnCode = 0
 ts.StillRunningAfter = server