From f3116b997d5274f881e09a1820ea4a1286c0aec8 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, drops the cached certificate data so that contexts created later do not resurrect the pre-update PEM, 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 every CA bucket and the expected certificate subjects. Fixes: #13575 --- .../functions/TSSslClientCertUpdate.en.rst | 11 +- .../api/functions/TSSslClientContext.en.rst | 4 +- src/api/InkAPI.cc | 132 ++++++++++++------ src/iocore/net/P_SSLConfig.h | 2 +- src/iocore/net/P_SSLSecret.h | 7 + src/iocore/net/SSLClientUtils.cc | 2 +- src/iocore/net/SSLSecret.cc | 10 ++ .../cert_update/cert_update.test.py | 75 +++++++++- .../cert_update/gold/client-cert-after.gold | 1 - .../cert_update/gold/client-cert-pre.gold | 1 - .../pluginTest/cert_update/gold/update.gold | 3 - 11 files changed, 188 insertions(+), 60 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..66a12078d8a 100644 --- a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst +++ b/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. 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..9b969fba247 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,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(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; + } + + // --- 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> 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 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 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/src/iocore/net/P_SSLSecret.h b/src/iocore/net/P_SSLSecret.h index 292f99ca7a7..d6b28c7e912 100644 --- a/src/iocore/net/P_SSLSecret.h +++ b/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); diff --git a/src/iocore/net/SSLClientUtils.cc b/src/iocore/net/SSLClientUtils.cc index 0f65c29f53c..584457178d7 100644 --- a/src/iocore/net/SSLClientUtils.cc +++ b/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 ctx(nullptr, &SSL_CTX_free); diff --git a/src/iocore/net/SSLSecret.cc b/src/iocore/net/SSLSecret.cc index fd8204a1c47..7249681aa28 100644 --- a/src/iocore/net/SSLSecret.cc +++ b/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 { 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..f07c5188929 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,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,7 +121,7 @@ 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( @@ -118,7 +129,23 @@ 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 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