From 48475d2ae57f3e5daeece2b04eb26e20a3c7c033 Mon Sep 17 00:00:00 2001 From: bneradt Date: Thu, 20 Aug 2026 11:08:41 -0500 Subject: [PATCH] Fix client certificate context updates TSSslClientCertUpdate has been unable to find normally configured outbound client contexts since 7dbb6cb188 changed the lookup key from certificate-and-key paths to the resolved certificate path. The existing AuTest hid the regression because its lowercase Streams.all assignments did not register assertions. This patch updates every matching CA bucket using the stored certificate path, preserves working contexts when a replacement cannot be built, and releases the SSL configuration after use. It also corrects the API documentation and strengthens the AuTest to verify both CA buckets and expected certificate subjects. Fixes: #13575 --- .../functions/TSSslClientCertUpdate.en.rst | 7 +- .../api/functions/TSSslClientContext.en.rst | 4 +- src/api/InkAPI.cc | 112 +++++++++++------- src/iocore/net/P_SSLConfig.h | 2 +- .../cert_update/cert_update.test.py | 38 +++++- .../cert_update/gold/client-cert-after.gold | 1 - .../cert_update/gold/client-cert-pre.gold | 1 - .../pluginTest/cert_update/gold/update.gold | 3 - 8 files changed, 109 insertions(+), 59 deletions(-) delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/update.gold diff --git a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst b/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst index f081acda953..39470be3d02 100644 --- a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst +++ b/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. diff --git a/doc/developer-guide/api/functions/TSSslClientContext.en.rst b/doc/developer-guide/api/functions/TSSslClientContext.en.rst index 9f685b2d486..e427734e0ca 100644 --- a/doc/developer-guide/api/functions/TSSslClientContext.en.rst +++ b/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). diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 581dae89982..4f1718763dd 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -28,6 +28,7 @@ #include #include #include +#include #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(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 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> 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 diff --git a/src/iocore/net/P_SSLConfig.h b/src/iocore/net/P_SSLConfig.h index a29755ed64b..cf092bbafe5 100644 --- a/src/iocore/net/P_SSLConfig.h +++ b/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; mutable std::unordered_map top_level_ctx_map; diff --git a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py index bbbaa31aa02..51acd12910c 100644 --- a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py +++ b/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 diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold deleted file mode 100644 index fef60f68d27..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold +++ /dev/null @@ -1 +0,0 @@ -``bob.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold deleted file mode 100644 index 6a94425920f..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold +++ /dev/null @@ -1 +0,0 @@ -``alice.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/update.gold b/tests/gold_tests/pluginTest/cert_update/gold/update.gold deleted file mode 100644 index 4160bb7dbf7..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/update.gold +++ /dev/null @@ -1,3 +0,0 @@ -`` -``Successfully updated`` -`` \ No newline at end of file