From 6371802808966543bf903ddeaf58d14865870aa2 Mon Sep 17 00:00:00 2001 From: Walt Karas Date: Wed, 25 May 2022 15:18:01 -0500 Subject: [PATCH 1/3] Fix an error on SSL config reload (plus some cleanup). This seems to have eliminated some ERROR diags we were seeing in Yahoo Prod when doing config reloads. The SSLSecret public functions no longer return pointers into the unorded_map of secrets, they return a copy of the secret data. This seemed thread unsafe. A periodic poll running in the background can update the secret data for an entry for a secret name in the map. To avoid exporting pointers, I had to change the prototype of TSSslSecretGet(). Hopefully there are no existing plugins that are already using this TS API function, so breaking this rule will be moot. I added a new TS API type, TSAllocatedVarLenData, in order to be able to cleanly return a copy of the secret data. --- .../api/functions/TSSslSecret.en.rst | 6 +- .../api/functions/TSTypes.en.rst | 12 ++ include/ts/apidefs.h.in | 6 + include/ts/ts.h | 6 +- include/tscpp/api/Cleanup.h | 73 ++++++++++ iocore/cache/test/stub.cc | 7 + iocore/net/P_SSLConfig.h | 7 + iocore/net/P_SSLSecret.h | 13 +- iocore/net/SSLConfig.cc | 25 +++- iocore/net/SSLSecret.cc | 126 +++++++----------- iocore/net/SSLUtils.cc | 12 +- iocore/net/libinknet_stub.cc | 7 + proxy/InkAPIInternal.h | 6 + src/traffic_quic/traffic_quic.cc | 7 + src/traffic_server/InkAPI.cc | 59 ++++---- .../tls/tls_client_cert2_plugin.test.py | 2 +- 16 files changed, 254 insertions(+), 120 deletions(-) diff --git a/doc/developer-guide/api/functions/TSSslSecret.en.rst b/doc/developer-guide/api/functions/TSSslSecret.en.rst index 87478e22542..1bff1367b3c 100644 --- a/doc/developer-guide/api/functions/TSSslSecret.en.rst +++ b/doc/developer-guide/api/functions/TSSslSecret.en.rst @@ -11,6 +11,7 @@ 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:: /common.defs .. default-domain:: c @@ -48,12 +49,13 @@ Synopsis #include -.. function:: TSReturnCode TSSslSecretGet(const char * secret_name, int secret_name_length, const char ** secret_data_return, int * secret_data_len) +.. function:: TSAllocatedVarLenData TSSslSecretGet(const char * secret_name, int secret_name_length) Description =========== -:func:`TSSslSecretGet` fetches the named secret from the current secret map. TS_ERROR is returned if there is no entry for the secret. +:func:`TSSslSecretGet` fetches the named secret from the current secret map. If there is no secret with the +given name, the data pointer will be null, and the size will be zero. TSSslSecretUpdate ***************** diff --git a/doc/developer-guide/api/functions/TSTypes.en.rst b/doc/developer-guide/api/functions/TSTypes.en.rst index c2f9fe148a1..b4585a83519 100644 --- a/doc/developer-guide/api/functions/TSTypes.en.rst +++ b/doc/developer-guide/api/functions/TSTypes.en.rst @@ -56,6 +56,18 @@ more widely. Those are described on this page. .. type:: TSAction +.. type:: TSAllocatedVarLenData + + Valiable lenght data stored in memory allocated with :func:`TSmalloc`. + + .. member:: char * data + + Pointer to first byte of sequence of bytes containing data. Must be freed by plugin with :func:`TSfree`. + + .. member:: size_t size + + Number of bytes in sequence pointed to by data. + .. type:: TSCacheKey .. type:: TSConfig diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in index 9baae2bfda0..ef78897c417 100644 --- a/include/ts/apidefs.h.in +++ b/include/ts/apidefs.h.in @@ -1460,6 +1460,12 @@ namespace ts } #endif +/* Variable-length buffer, with data returned by a TS API call. Data must be freed by plugin with TSfree(). */ +typedef struct tsapi_allocated_var_len_data { + char *data; + size_t size; +} TSAllocatedVarLenData; + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/include/ts/ts.h b/include/ts/ts.h index 66af0983da1..af0ad4ad81d 100644 --- a/include/ts/ts.h +++ b/include/ts/ts.h @@ -1304,8 +1304,10 @@ tsapi TSReturnCode TSSslServerCertUpdate(const char *cert_path, const char *key_ /* Update the transient secret table for SSL_CTX loading */ tsapi TSReturnCode TSSslSecretSet(const char *secret_name, int secret_name_length, const char *secret_data, int secret_data_len); -tsapi TSReturnCode TSSslSecretGet(const char *secret_name, int secret_name_length, const char **secret_data_return, - int *secret_data_len); + +/* Returns secret with given name as TSAllocatedVerLenData. If there is no secret with the given name, data will +** be null and size will be zero. Calling code must free by calling TSfree(). */ +tsapi TSAllocatedVarLenData TSSslSecretGet(const char *secret_name, int secret_name_length); tsapi TSReturnCode TSSslSecretUpdate(const char *secret_name, int secret_name_length); diff --git a/include/tscpp/api/Cleanup.h b/include/tscpp/api/Cleanup.h index ab383a5ec36..2eea509fbea 100644 --- a/include/tscpp/api/Cleanup.h +++ b/include/tscpp/api/Cleanup.h @@ -26,6 +26,8 @@ #include #include +#include +#include #include @@ -100,6 +102,77 @@ struct TSIOBufferReaderDeleter { }; using TSIOBufferReaderUniqPtr = std::unique_ptr, TSIOBufferReaderDeleter>; +// Guard for TSAllocatedVarLenData. +// +class TSAllocatedVarLenDataGuard +{ +public: + TSAllocatedVarLenDataGuard() + { + _m.data = nullptr; + _m.size = 0; + } + + TSAllocatedVarLenDataGuard(TSAllocatedVarLenData const &d) : _m(d) { TSAssert((nullptr == d.data) == (0 == d.size)); } + + operator TSAllocatedVarLenData const &() const { return _m; } + + TSAllocatedVarLenData const & + operator()() const + { + return _m; + } + + TSAllocatedVarLenDataGuard(TSAllocatedVarLenDataGuard const &src) + { + TSAssert((nullptr == src._m.data) == (0 == src._m.size)); + + if (src._m.data && src._m.size) { + _m.data = static_cast(TSmalloc(src._m.size)); + std::memcpy(_m.data, src._m.data, src._m.size); + _m.size = src._m.size; + + } else { + _m.data = nullptr; + _m.size = 0; + } + } + + TSAllocatedVarLenDataGuard & + operator=(TSAllocatedVarLenDataGuard const &src) + { + TSAssert((nullptr == src._m.data) == (0 == src._m.size)); + + this->~TSAllocatedVarLenDataGuard(); + new (this) TSAllocatedVarLenDataGuard(src); + return *this; + } + + TSAllocatedVarLenDataGuard(TSAllocatedVarLenDataGuard &&src) + { + TSAssert((nullptr == src._m.data) == (0 == src._m.size)); + + _m = src; + src._m.data = nullptr; + src._m.size = 0; + } + + TSAllocatedVarLenDataGuard & + operator=(TSAllocatedVarLenDataGuard &&src) + { + TSAssert((nullptr == src._m.data) == (0 == src._m.size)); + + this->~TSAllocatedVarLenDataGuard(); + new (this) TSAllocatedVarLenDataGuard(std::move(src)); + return *this; + } + + ~TSAllocatedVarLenDataGuard() { TSfree(_m.data); } + +private: + TSAllocatedVarLenData _m; +}; + class TxnAuxDataMgrBase { protected: diff --git a/iocore/cache/test/stub.cc b/iocore/cache/test/stub.cc index b247da1bb39..8c28cf028ef 100644 --- a/iocore/cache/test/stub.cc +++ b/iocore/cache/test/stub.cc @@ -51,6 +51,13 @@ APIHook::invoke(int, void *) const return 0; } +int +APIHook::blocking_invoke(int, void *) const +{ + ink_assert(false); + return 0; +} + APIHook * APIHook::next() const { diff --git a/iocore/net/P_SSLConfig.h b/iocore/net/P_SSLConfig.h index d383e0ec30d..2eb1e10f73e 100644 --- a/iocore/net/P_SSLConfig.h +++ b/iocore/net/P_SSLConfig.h @@ -30,6 +30,8 @@ ****************************************************************************/ #pragma once +#include + #include #include "tscore/ink_inet.h" @@ -167,6 +169,11 @@ struct SSLConfigParams : public ConfigInfo { void cleanup(); void reset(); void SSLConfigInit(IpMap *global); + +private: + // c_str() of string passed to in-progess call to updateCTX(). + // + mutable std::atomic secret_for_updateCTX{nullptr}; }; ///////////////////////////////////////////////////////////// diff --git a/iocore/net/P_SSLSecret.h b/iocore/net/P_SSLSecret.h index f6b2a279421..212ab156c6e 100644 --- a/iocore/net/P_SSLSecret.h +++ b/iocore/net/P_SSLSecret.h @@ -19,6 +19,8 @@ limitations under the License. */ +#pragma once + #include #include #include @@ -28,14 +30,13 @@ class SSLSecret { public: SSLSecret() {} - bool getSecret(const std::string &name, std::string_view &data) const; - bool setSecret(const std::string &name, const char *data, int data_len); - bool getOrLoadSecret(const std::string &name, const std::string &name2, std::string_view &data, std::string_view &data2); + std::string getSecret(const std::string &name) const; + 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); private: - const std::string *getSecretItem(const std::string &name) const; - bool loadSecret(const std::string &name, const std::string &name2, std::string &data_item, std::string &data_item2); - bool loadFile(const std::string &name, std::string &data_item); + 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); std::unordered_map secret_map; mutable std::recursive_mutex secret_map_mutex; diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc index 336c084ccb2..d7ffae92b77 100644 --- a/iocore/net/SSLConfig.cc +++ b/iocore/net/SSLConfig.cc @@ -745,13 +745,32 @@ cleanup_bio(BIO *&biop) void SSLConfigParams::updateCTX(const std::string &cert_secret_name) const { + Debug("ssl_config_updateCTX", "Update cert %s, %p", cert_secret_name.c_str(), this); + + // Instances of SSLConfigParams should access by one thread at a time only. secret_for_updateCTX is accessed + // atomically as a fail-safe. + // + char const *expected = nullptr; + if (!secret_for_updateCTX.compare_exchange_strong(expected, cert_secret_name.c_str())) { + if (is_debug_tag_set("ssl_config_updateCTX")) { + // As a fail-safe, handle it if secret_for_updateCTX doesn't or no longer points to a null-terminated string. + // + char const *s{expected}; + for (; *s && (std::size_t(s - expected) < cert_secret_name.size()); ++s) { + } + Debug("ssl_config_updateCTX", "Update cert, indirect recusive call caused by call for %.*s", int(s - expected), expected); + } + return; + } + // Clear the corresponding client CTXs. They will be lazy loaded later - Debug("ssl", "Update cert %s", cert_secret_name.c_str()); this->clearCTX(cert_secret_name); // Update the server cert SSLMultiCertConfigLoader loader(this); loader.update_ssl_ctx(cert_secret_name); + + secret_for_updateCTX = nullptr; } void @@ -806,8 +825,8 @@ SSLConfigParams::getCTX(const std::string &client_cert, const std::string &key_f // Set public and private keys if (!client_cert.empty()) { - std::string_view secret_data; - std::string_view secret_key_data; + std::string secret_data; + std::string secret_key_data; // Fetch the client_cert data std::string completeSecretPath{Layout::get()->relative_to(this->clientCertPathOnly, client_cert)}; diff --git a/iocore/net/SSLSecret.cc b/iocore/net/SSLSecret.cc index 6cc10150b7e..7f780ccb7d3 100644 --- a/iocore/net/SSLSecret.cc +++ b/iocore/net/SSLSecret.cc @@ -18,14 +18,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -#include -#include + #include "InkAPIInternal.h" // Added to include the ssl_hook and lifestyle_hook definitions #include "tscore/ts_file.h" #include "P_SSLConfig.h" -bool -SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::string &data_item1, std::string &data_item2) +void +SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2) { // Call the load secret hooks // @@ -36,111 +35,84 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s secret_name.key_name = name2.data(); secret_name.key_name_len = name2.size(); while (curHook) { - curHook->invoke(TS_EVENT_SSL_SECRET, &secret_name); + curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name); curHook = curHook->next(); } - const std::string *data1 = this->getSecretItem(name1); - const std::string *data2 = this->getSecretItem(name2); - if ((nullptr == data1 || data1->length() == 0) || (!name2.empty() && (nullptr == data2 || data2->length() == 0))) { + data1 = this->getSecret(name1); + data2 = name2.empty() ? std::string{} : this->getSecret(name2); + if (data1.empty() || (!name2.empty() && data2.empty())) { // If none of them loaded it, assume it is a file - return loadFile(name1, data_item1) && (name2.empty() || loadFile(name2, data_item2)); + data1 = loadFile(name1); + if (!name2.empty()) { + data2 = loadFile(name2); + } } - return true; } -bool -SSLSecret::loadFile(const std::string &name, std::string &data_item) +std::string +SSLSecret::loadFile(const std::string &name) { - struct stat statdata; - // Load the secret and add it to the map - if (stat(name.c_str(), &statdata) < 0) { - return false; - } + Debug("ssl_secret", "SSLSecret::loadFile(%s)", name.c_str()); std::error_code error; - data_item = ts::file::load(ts::file::path(name), error); + std::string const data = ts::file::load(ts::file::path(name), error); if (error) { + Debug("ssl_secret_err", "SSLSecret::loadFile(%s) failed error code=%d message=%s", name.c_str(), error.value(), + error.message().c_str()); // Loading file failed - return false; + return std::string{}; } + Debug("ssl_secret", "Secret data: %.50s", data.c_str()); if (SSLConfigParams::load_ssl_file_cb) { SSLConfigParams::load_ssl_file_cb(name.c_str()); } - return true; + return data; } -bool -SSLSecret::setSecret(const std::string &name, const char *data, int data_len) +void +SSLSecret::setSecret(const std::string &name, std::string_view data) { std::scoped_lock lock(secret_map_mutex); - auto iter = secret_map.find(name); - if (iter == secret_map.end()) { - secret_map[name] = ""; - iter = secret_map.find(name); - } - if (iter == secret_map.end()) { - return false; - } - iter->second.assign(data, data_len); + secret_map[name] = std::string{data}; // The full secret data can be sensitive. Print only the first 50 bytes. - Debug("ssl_secret", "Set secret for %s to %.50s", name.c_str(), iter->second.c_str()); - return true; + Debug("ssl_secret", "Set secret for %s to %.*s", name.c_str(), int(data.size() > 50 ? 50 : data.size()), data.data()); } -const std::string * -SSLSecret::getSecretItem(const std::string &name) const +std::string +SSLSecret::getSecret(const std::string &name) const { std::scoped_lock lock(secret_map_mutex); auto iter = secret_map.find(name); - if (iter == secret_map.end()) { - return nullptr; - } - return &iter->second; -} - -bool -SSLSecret::getSecret(const std::string &name, std::string_view &data) const -{ - const std::string *data_item = this->getSecretItem(name); - if (data_item) { - // The full secret data can be sensitive. Print only the first 50 bytes. - Debug("ssl_secret", "Get secret for %s: %.50s", name.c_str(), data_item->c_str()); - data = *data_item; - } else { + if (secret_map.end() == iter) { Debug("ssl_secret", "Get secret for %s: not found", name.c_str()); - data = std::string_view{}; + return std::string{}; + } + if (iter->second.empty()) { + Debug("ssl_secret", "Get secret for %s: empty", name.c_str()); + return std::string{}; } - return data_item != nullptr; + // The full secret data can be sensitive. Print only the first 50 bytes. + Debug("ssl_secret", "Get secret for %s: %.50s", name.c_str(), iter->second.c_str()); + return iter->second; } -bool -SSLSecret::getOrLoadSecret(const std::string &name1, const std::string &name2, std::string_view &data1, std::string_view &data2) +void +SSLSecret::getOrLoadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2) { Debug("ssl_secret", "lookup up secrets for %s and %s", name1.c_str(), name2.c_str()); std::scoped_lock lock(secret_map_mutex); - bool found_secret1 = this->getSecret(name1, data1); - bool found_secret2 = name2.empty() || this->getSecret(name2, data2); - - // If we can't find either secret, load them both again - if (!found_secret1 || !found_secret2) { - // Make sure each name has an entry - if (!found_secret1) { - secret_map[name1] = ""; - } - if (!found_secret2) { - secret_map[name2] = ""; + std::string *const data1ptr = &(secret_map[name1]); + std::string *const data2ptr = [&]() -> std::string *const { + if (name2.empty()) { + data2.clear(); + return &data2; } - auto iter1 = secret_map.find(name1); - auto iter2 = name2.empty() ? iter1 : secret_map.find(name2); - if (this->loadSecret(name1, name2, iter1->second, iter2->second)) { - data1 = iter1->second; - if (!name2.empty()) { - data2 = iter2->second; - } - return true; - } - } else { - return true; + return &(secret_map[name2]); + }(); + // If we can't find either secret, load them both again + if (data1ptr->empty() || (!name2.empty() && data2ptr->empty())) { + this->loadSecret(name1, name2, *data1ptr, *data2ptr); } - return false; + data1 = *data1ptr; + data2 = *data2ptr; } diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 22f170ce8ca..9f11d17f6ff 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -1052,7 +1052,8 @@ SSLPrivateKeyHandler(SSL_CTX *ctx, const SSLConfigParams *params, const char *ke scoped_BIO bio(BIO_new_mem_buf(secret_data, secret_data_len)); pkey = PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr); if (nullptr == pkey) { - SSLError("failed to load server private key from %s", keyPath); + SSLError("failed to load server private key (%.*s) from %s", secret_data_len < 50 ? secret_data_len : 50, secret_data, + keyPath); return false; } if (!SSL_CTX_use_PrivateKey(ctx, pkey)) { @@ -2266,8 +2267,8 @@ SSLMultiCertConfigLoader::load_certs_and_cross_reference_names( } for (size_t i = 0; i < data.cert_names_list.size(); i++) { - std::string_view secret_data; - std::string_view secret_key_data; + std::string secret_data; + std::string secret_key_data; params->secrets.getOrLoadSecret(data.cert_names_list[i], data.key_list.size() > i ? data.key_list[i] : "", secret_data, secret_key_data); if (secret_data.empty()) { @@ -2417,8 +2418,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vectorsecrets.getOrLoadSecret(cert_names_list[i], keyPath, secret_data, secret_key_data); if (secret_data.empty()) { SSLError("failed to load certificate secret for %s", cert_names_list[i].c_str()); @@ -2448,6 +2449,7 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector #include #include +#include #include "tscore/ink_platform.h" #include "tscore/ink_base64.h" @@ -1361,7 +1361,7 @@ APIHook::prev() const int APIHook::invoke(int event, void *edata) const { - if ((event == EVENT_IMMEDIATE) || (event == EVENT_INTERVAL) || event == TS_EVENT_HTTP_TXN_CLOSE) { + if (event == EVENT_IMMEDIATE || event == EVENT_INTERVAL || event == TS_EVENT_HTTP_TXN_CLOSE) { if (ink_atomic_increment((int *)&m_cont->m_event_count, 1) < 0) { ink_assert(!"not reached"); } @@ -1374,6 +1374,20 @@ APIHook::invoke(int event, void *edata) const return m_cont->handleEvent(event, edata); } +int +APIHook::blocking_invoke(int event, void *edata) const +{ + if (event == EVENT_IMMEDIATE || event == EVENT_INTERVAL || event == TS_EVENT_HTTP_TXN_CLOSE) { + if (ink_atomic_increment((int *)&m_cont->m_event_count, 1) < 0) { + ink_assert(!"not reached"); + } + } + + WEAK_SCOPED_MUTEX_LOCK(lock, m_cont->mutex, this_ethread()); + + return m_cont->handleEvent(event, edata); +} + APIHook * APIHooks::head() const { @@ -9663,23 +9677,20 @@ TSSslContextFindByAddr(struct sockaddr const *addr) tsapi TSReturnCode TSSslSecretSet(const char *secret_name, int secret_name_length, const char *secret_data, int secret_data_len) { - TSReturnCode retval = TS_SUCCESS; + TSReturnCode retval = TS_SUCCESS; + std::string const secret_name_str{secret_name, unsigned(secret_name_length)}; SSLConfigParams *load_params = SSLConfig::load_acquire(); SSLConfigParams *params = SSLConfig::acquire(); if (load_params != nullptr) { // Update the current data structure Debug("ssl.cert_update", "Setting secrets in SSLConfig load for: %.*s", secret_name_length, secret_name); - if (!load_params->secrets.setSecret(std::string(secret_name, secret_name_length), secret_data, secret_data_len)) { - retval = TS_ERROR; - } - load_params->updateCTX(std::string(secret_name, secret_name_length)); + load_params->secrets.setSecret(secret_name_str, std::string_view(secret_data, secret_data_len)); + load_params->updateCTX(secret_name_str); SSLConfig::load_release(load_params); } if (params != nullptr) { Debug("ssl.cert_update", "Setting secrets in SSLConfig for: %.*s", secret_name_length, secret_name); - if (!params->secrets.setSecret(std::string(secret_name, secret_name_length), secret_data, secret_data_len)) { - retval = TS_ERROR; - } - params->updateCTX(std::string(secret_name, secret_name_length)); + params->secrets.setSecret(secret_name_str, std::string_view(secret_data, secret_data_len)); + params->updateCTX(secret_name_str); SSLConfig::release(params); } return retval; @@ -9697,32 +9708,32 @@ TSSslSecretUpdate(const char *secret_name, int secret_name_length) return retval; } -tsapi TSReturnCode -TSSslSecretGet(const char *secret_name, int secret_name_length, const char **secret_data_return, int *secret_data_len) +tsapi TSAllocatedVarLenData +TSSslSecretGet(const char *secret_name, int secret_name_length) { bool loading = true; - TSReturnCode retval = TS_SUCCESS; SSLConfigParams *params = SSLConfig::load_acquire(); if (params == nullptr) { params = SSLConfig::acquire(); loading = false; } - std::string_view secret_data; - if (!params->secrets.getSecret(std::string(secret_name, secret_name_length), secret_data)) { - retval = TS_ERROR; - } - if (secret_data_return) { - *secret_data_return = secret_data.data(); - } - if (secret_data_len) { - *secret_data_len = secret_data.size(); + std::string const secret_data = params->secrets.getSecret(std::string(secret_name, secret_name_length)); + TSAllocatedVarLenData vld; + if (secret_data.empty()) { + vld.data = nullptr; + vld.size = 0; + + } else { + vld.data = static_cast(ats_malloc(secret_data.size())); + memcpy(vld.data, secret_data.data(), secret_data.size()); + vld.size = secret_data.size(); } if (loading) { SSLConfig::load_release(params); } else { SSLConfig::release(params); } - return retval; + return vld; } /** diff --git a/tests/gold_tests/tls/tls_client_cert2_plugin.test.py b/tests/gold_tests/tls/tls_client_cert2_plugin.test.py index a3725445b08..0f7b7ec9ad0 100644 --- a/tests/gold_tests/tls/tls_client_cert2_plugin.test.py +++ b/tests/gold_tests/tls/tls_client_cert2_plugin.test.py @@ -105,7 +105,7 @@ ' client_cert: {0}/../signed-bar.pem'.format(ts.Variables.SSLDir), ' client_key: {0}/../signed-bar.key'.format(ts.Variables.SSLDir), '- fqdn: bob.*.com', - ' client_cert: {0}/../combo-signed-foo.pem'.format(ts.Variables.SSLDir), + ' client_cert: {0}/combo-signed-foo.pem'.format(ts.Variables.SSLDir), '- fqdn: "*bar.com"', ' client_cert: {0}/../signed2-bar.pem'.format(ts.Variables.SSLDir), ' client_key: {0}/../signed-bar.key'.format(ts.Variables.SSLDir), From 5d25fa2132205d8966f7740ef573782a409590b1 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Mon, 3 Oct 2022 12:27:32 -0500 Subject: [PATCH 2/3] YTSATS-4067: Fix deadlock with secret_map_mutex (#740) 1. getOrLoadSecret grabbed the secret_map_mutex and called loadSecret. 2. loadSecret dispatched to Continations that registered for the TS_EVENT_SSL_SECRET event. This would try to grab the Continuation's lock. 3. In the meantime, those Continuations could call setSecret which would try to grab the secret_map_mutex. If this Continuation did this while holding the lock that step 2 is waiting upon, then there will be a deadlock between the Continuation lock and the secret_map_mutex between the two threads. This patch avoids the deadlock by releasing the secret_map_mutex lock before calling loadSecret. It also updates the secret_map when loading secrets from a file in loadSecret. --- iocore/net/SSLSecret.cc | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/iocore/net/SSLSecret.cc b/iocore/net/SSLSecret.cc index 7f780ccb7d3..dc1c97e6b12 100644 --- a/iocore/net/SSLSecret.cc +++ b/iocore/net/SSLSecret.cc @@ -23,6 +23,13 @@ #include "tscore/ts_file.h" #include "P_SSLConfig.h" +#include + +// NOTE: The secret_map_mutex should not be held by the caller of this +// 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. void SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2) { @@ -44,8 +51,10 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s if (data1.empty() || (!name2.empty() && data2.empty())) { // If none of them loaded it, assume it is a file data1 = loadFile(name1); + setSecret(name1, data1); if (!name2.empty()) { data2 = loadFile(name2); + setSecret(name2, data2); } } } @@ -100,19 +109,25 @@ void SSLSecret::getOrLoadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2) { Debug("ssl_secret", "lookup up secrets for %s and %s", name1.c_str(), name2.c_str()); - std::scoped_lock lock(secret_map_mutex); - std::string *const data1ptr = &(secret_map[name1]); - std::string *const data2ptr = [&]() -> std::string *const { - if (name2.empty()) { - data2.clear(); - return &data2; - } - return &(secret_map[name2]); - }(); + { + std::scoped_lock lock(secret_map_mutex); + std::string *const data1ptr = &(secret_map[name1]); + std::string *const data2ptr = [&]() -> std::string *const { + if (name2.empty()) { + data2.clear(); + return &data2; + } + return &(secret_map[name2]); + }(); + data1 = *data1ptr; + data2 = *data2ptr; + } // If we can't find either secret, load them both again - if (data1ptr->empty() || (!name2.empty() && data2ptr->empty())) { - this->loadSecret(name1, name2, *data1ptr, *data2ptr); + if (data1.empty() || (!name2.empty() && data2.empty())) { + std::string data1tmp; + std::string data2tmp; + this->loadSecret(name1, name2, data1tmp, data2tmp); + data1 = std::move(data1tmp); + data2 = std::move(data2tmp); } - data1 = *data1ptr; - data2 = *data2ptr; } From 6a936774f8274e387050ff421c682e80f5d41566 Mon Sep 17 00:00:00 2001 From: Walt Karas Date: Tue, 1 Nov 2022 15:11:23 +0000 Subject: [PATCH 3/3] Fix clang-format CI job defect. --- doc/developer-guide/api/functions/TSTypes.en.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer-guide/api/functions/TSTypes.en.rst b/doc/developer-guide/api/functions/TSTypes.en.rst index b4585a83519..bff6ba33e4b 100644 --- a/doc/developer-guide/api/functions/TSTypes.en.rst +++ b/doc/developer-guide/api/functions/TSTypes.en.rst @@ -60,7 +60,7 @@ more widely. Those are described on this page. Valiable lenght data stored in memory allocated with :func:`TSmalloc`. - .. member:: char * data + .. member:: char * data Pointer to first byte of sequence of bytes containing data. Must be freed by plugin with :func:`TSfree`.