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