You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bc...@apache.org on 2026/10/20 04:02:47 UTC
(trafficserver) branch master updated: Serialize SSL_SECRET hooks during parallel cert loading (#13844)
This is an automated email from the ASF dual-hosted git repository.
bryancall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 4296d668d3 Serialize SSL_SECRET hooks during parallel cert loading (#13844)
4296d668d3 is described below
commit 4296d668d3e1dd9767811122f6abd6b349c3ed57
Author: Bryan Call <bc...@apache.org>
AuthorDate: Mon Oct 19 21:02:41 2026 -0700
Serialize SSL_SECRET hooks during parallel cert loading (#13844)
Parallel ssl_multicert loading (#12998) runs TS_LIFECYCLE_SSL_SECRET_HOOK on
several std::threads at once, so plugins written for one call at a time race
on their own state; ssl_secret_load_test's unguarded map crashed
traffic_server at startup. The loader threads are not EThreads, so a
continuation mutex cannot guard it. Run the hooks under one process-wide
recursive lock in SSLSecret::loadSecret(), so a hook can still re-enter via
TSSslSecretSet(), while cert parsing stays parallel. Adds docs and unit tests.
---
.../api/functions/TSLifecycleHookAdd.en.rst | 8 ++
src/iocore/net/CMakeLists.txt | 1 +
src/iocore/net/SSLSecret.cc | 24 +++-
src/iocore/net/unit_tests/test_SSLSecret.cc | 157 +++++++++++++++++++++
4 files changed, 186 insertions(+), 4 deletions(-)
diff --git a/doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst b/doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst
index b9c3a70976..f5f9bf3dc4 100644
--- a/doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst
+++ b/doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst
@@ -114,6 +114,14 @@ Types
This hook gives the plugin a chance to load the certificate or key from an alternative source and set via the :func:`TSSslSecretSet` API.
If there is no plugin override, the certificate or key will be loaded from disk and the secret name will be interpreted as a file path.
+ |TS| may load certificates on several threads at once, and some of those are not event threads, so a mutex on the
+ continuation does not serialize the calls. |TS| instead runs this hook on one thread at a time, so a plugin does not
+ need its own lock to guard state that only this hook touches. The hook can still be called again on the same thread
+ before it returns, if it calls :func:`TSSslSecretSet` and that update needs another secret. Because the hook runs
+ under a process-wide lock, create the hook continuation without a mutex, and do not hold any lock the hook takes
+ while calling :func:`TSSslSecretSet` or :func:`TSSslSecretUpdate` from another callback. Otherwise a configuration
+ reload can deadlock.
+
.. cpp:enumerator:: TS_LIFECYCLE_SHUTDOWN_HOOK
Called after |TS| receiving a shutdown signal, such as SIGTERM.
diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt
index 6941b95827..dc185d310f 100644
--- a/src/iocore/net/CMakeLists.txt
+++ b/src/iocore/net/CMakeLists.txt
@@ -150,6 +150,7 @@ if(BUILD_TESTING)
unit_tests/test_ProxyProtocol.cc
unit_tests/test_SSLCertLookup.cc
unit_tests/test_SSLNetVConnectionAsyncEp.cc
+ unit_tests/test_SSLSecret.cc
unit_tests/test_SSLSNIConfig.cc
unit_tests/test_YamlSNIConfig.cc
unit_tests/test_OCSPStapling.cc
diff --git a/src/iocore/net/SSLSecret.cc b/src/iocore/net/SSLSecret.cc
index 5cf9aa6c99..52e3ed0dc2 100644
--- a/src/iocore/net/SSLSecret.cc
+++ b/src/iocore/net/SSLSecret.cc
@@ -25,6 +25,7 @@
#include "P_SSLConfig.h"
+#include <mutex>
#include <utility>
#include <openssl/evp.h>
@@ -34,6 +35,15 @@ namespace
DbgCtl dbg_ctl_ssl_secret{"ssl_secret"};
DbgCtl dbg_ctl_ssl_secret_err{"ssl_secret_err"};
+// Parallel ssl_multicert loading reaches loadSecret() from several std::threads
+// at once. Those are not EThreads, so a hook continuation's mutex does not
+// exclude them, and plugins written for one call at a time corrupt their own
+// state. This lock keeps the SSL_SECRET hooks to one thread at a time. It is
+// recursive because a hook that calls TSSslSecretSet() re-enters loadSecret()
+// on the same thread when the updated certificate needs a secret that is not
+// cached yet.
+std::recursive_mutex secret_hook_mutex;
+
} // end anonymous namespace
static void
@@ -54,7 +64,10 @@ get_hash_str(const std::string &input, char hash_str[EVP_MAX_MD_SIZE * 2], unsig
// function. The implementation of this function may call a plugin's
// TS_EVENT_SSL_SECRET handler which in turn may grab a lock for
// secret_map_mutex via a TSSslSecretSet call. These events will result in a
-// deadlock.
+// deadlock. The handlers run under secret_hook_mutex, which is recursive
+// because a TSSslSecretSet call from a handler can come back into this
+// function on the same thread. For the same reason, no thread may hold a lock
+// that a handler takes while it calls TSSslSecretSet or TSSslSecretUpdate.
void
SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2)
{
@@ -66,9 +79,12 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s
secret_name.cert_name_len = name1.size();
secret_name.key_name = name2.data();
secret_name.key_name_len = name2.size();
- while (curHook) {
- curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name);
- curHook = curHook->next();
+ {
+ std::scoped_lock lock(secret_hook_mutex);
+ while (curHook) {
+ curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name);
+ curHook = curHook->next();
+ }
}
data1 = this->getSecret(name1);
diff --git a/src/iocore/net/unit_tests/test_SSLSecret.cc b/src/iocore/net/unit_tests/test_SSLSecret.cc
new file mode 100644
index 0000000000..92d7169c2e
--- /dev/null
+++ b/src/iocore/net/unit_tests/test_SSLSecret.cc
@@ -0,0 +1,157 @@
+/** @file
+
+ Catch based unit tests for SSLSecret
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "../P_SSLSecret.h"
+
+#include "api/LifecycleAPIHooks.h"
+#include "ts/InkAPIPrivateIOCore.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <algorithm>
+#include <atomic>
+#include <chrono>
+#include <condition_variable>
+#include <cstdlib>
+#include <future>
+#include <iostream>
+#include <memory>
+#include <mutex>
+#include <string>
+#include <thread>
+#include <vector>
+
+namespace
+{
+// Only count hook calls made by this test. The hook stays registered for the
+// rest of the test binary, since lifecycle hooks cannot be removed.
+std::atomic<bool> hook_armed{false};
+
+std::mutex hook_mutex;
+std::condition_variable hook_cv;
+int callers_in_hook{0};
+int max_callers_in_hook{0};
+
+int
+count_concurrent_callers(TSCont /* cont ATS_UNUSED */, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+{
+ if (!hook_armed) {
+ return 0;
+ }
+ std::unique_lock lock(hook_mutex);
+ ++callers_in_hook;
+ max_callers_in_hook = std::max(max_callers_in_hook, callers_in_hook);
+ hook_cv.notify_all();
+ // Linger until a second caller arrives, or give up. If the hook is not
+ // serialized, the other threads are already waiting to enter it.
+ hook_cv.wait_for(lock, std::chrono::milliseconds(250), [] { return callers_in_hook > 1; });
+ --callers_in_hook;
+ return 0;
+}
+
+// The SSLSecret that reenter_load_secret() calls back into, and how many times
+// that hook has run.
+std::atomic<SSLSecret *> reentry_secrets{nullptr};
+std::atomic<int> reentry_calls{0};
+
+// Like a plugin whose hook calls TSSslSecretSet(): that can reach loadSecret()
+// again on the same thread before the hook returns. Re-enter from the
+// outermost call only.
+int
+reenter_load_secret(TSCont /* cont ATS_UNUSED */, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+{
+ SSLSecret *secrets = reentry_secrets;
+ if (secrets == nullptr || reentry_calls++ > 0) {
+ return 0;
+ }
+ std::string cert_data;
+ std::string key_data;
+ secrets->getOrLoadSecret("/nonexistent/reentry-inner-cert.pem", "/nonexistent/reentry-inner-key.pem", cert_data, key_data);
+ return 0;
+}
+} // end anonymous namespace
+
+TEST_CASE("SSL_SECRET hooks run one caller at a time")
+{
+ // Parallel ssl_multicert loading calls getOrLoadSecret() from several plain
+ // std::threads. A plugin's secret hook must still see one call at a time.
+ g_lifecycle_hooks->append(TS_LIFECYCLE_SSL_SECRET_HOOK, new INKContInternal(count_concurrent_callers, nullptr));
+ hook_armed = true;
+
+ constexpr int num_threads = 4;
+ SSLSecret secrets;
+ std::atomic<int> waiting{num_threads};
+ std::vector<std::thread> threads;
+ for (int i = 0; i < num_threads; ++i) {
+ threads.emplace_back([&secrets, &waiting, i]() {
+ --waiting;
+ while (waiting > 0) {
+ std::this_thread::yield();
+ }
+ // Each name is new to the secret map, so getOrLoadSecret() calls
+ // loadSecret(), which runs the hook before it reads anything from disk.
+ std::string cert_data;
+ std::string key_data;
+ secrets.getOrLoadSecret("/nonexistent/cert" + std::to_string(i) + ".pem", "/nonexistent/key" + std::to_string(i) + ".pem",
+ cert_data, key_data);
+ });
+ }
+ for (auto &t : threads) {
+ t.join();
+ }
+ hook_armed = false;
+
+ CHECK(max_callers_in_hook == 1);
+}
+
+TEST_CASE("An SSL_SECRET hook can call back into SSLSecret on its own thread")
+{
+ // Static so it outlives the loader thread if that thread deadlocks.
+ static SSLSecret secrets;
+ g_lifecycle_hooks->append(TS_LIFECYCLE_SSL_SECRET_HOOK, new INKContInternal(reenter_load_secret, nullptr));
+ reentry_calls = 0;
+ reentry_secrets = &secrets;
+
+ // Run the load on a detached thread so a deadlock fails the test instead of
+ // hanging the binary. A std::async future would block in its destructor.
+ auto done = std::make_shared<std::promise<void>>();
+ auto finished = done->get_future();
+ std::thread([done]() {
+ std::string cert_data;
+ std::string key_data;
+ secrets.getOrLoadSecret("/nonexistent/reentry-outer-cert.pem", "/nonexistent/reentry-outer-key.pem", cert_data, key_data);
+ done->set_value();
+ }).detach();
+ bool const completed = finished.wait_for(std::chrono::seconds(5)) == std::future_status::ready;
+ reentry_secrets = nullptr;
+
+ if (!completed) {
+ // The stuck thread still holds the hook lock, so any later test that loads
+ // a secret would hang. Report the failure and stop the binary here.
+ FAIL_CHECK("getOrLoadSecret() did not finish in 5 seconds: re-entering the SSL_SECRET hook deadlocked");
+ std::cout.flush();
+ std::cerr.flush();
+ std::_Exit(1);
+ }
+ CHECK(reentry_calls == 2);
+}