diff --git a/doc/developer-guide/api/functions/TSSslSecret.en.rst b/doc/developer-guide/api/functions/TSSslSecret.en.rst index 87478e22542..9f81d4f5665 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 @@ -27,7 +28,7 @@ Synopsis #include -.. function:: TSReturnCode TSSslSecretSet(const char * secret_name, int secret_name_length, const char * secret_data, int secret_data_len) +.. function:: TSReturnCode TSSslSecretSet(const char * secret_name, int secret_name_length, const char * secret_data, int secret_data_length) Description =========== @@ -48,12 +49,16 @@ Synopsis #include -.. function:: TSReturnCode TSSslSecretGet(const char * secret_name, int secret_name_length, const char ** secret_data_return, int * secret_data_len) +.. function:: char * TSSslSecretGet(const char * secret_name, int secret_name_length, int * secret_data_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 returned pointer will be null, and the :arg:`secret_data_length` output paramter will be set to zero. If +the returned pointer is not null, it points to a buffer containing the secret data. The :arg:`secret_data_length` output +parameter will be set to the length of the secret data. The buffer containing the data must be freed by +calling :func:`TSfree`. TSSslSecretUpdate ***************** diff --git a/include/ts/ts.h b/include/ts/ts.h index 4d35972e7e0..5286bf75c22 100644 --- a/include/ts/ts.h +++ b/include/ts/ts.h @@ -1303,9 +1303,11 @@ tsapi TSReturnCode TSSslClientCertUpdate(const char *cert_path, const char *key_ tsapi TSReturnCode TSSslServerCertUpdate(const char *cert_path, const char *key_path); /* 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); +tsapi TSReturnCode TSSslSecretSet(const char *secret_name, int secret_name_length, const char *secret_data, int secret_data_length); + +/* Returns secret with given name (not null terminted). If there is no secret with the given name, return value will +** be null and secret_data_lenght will be zero. Calling code must free data buffer by calling TSfree(). */ +tsapi char *TSSslSecretGet(const char *secret_name, int secret_name_length, int *secret_data_length); tsapi TSReturnCode TSSslSecretUpdate(const char *secret_name, int secret_name_length); 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 c31494bc7b1..3338b37da7b 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" @@ -165,6 +167,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 006a4e81c20..5825b0c66be 100644 --- a/iocore/net/SSLConfig.cc +++ b/iocore/net/SSLConfig.cc @@ -739,6 +739,24 @@ 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_load", "Update cert %s", cert_secret_name.c_str()); this->clearCTX(cert_secret_name); @@ -746,6 +764,8 @@ SSLConfigParams::updateCTX(const std::string &cert_secret_name) const // Update the server cert SSLMultiCertConfigLoader loader(this); loader.update_ssl_ctx(cert_secret_name); + + secret_for_updateCTX = nullptr; } void @@ -800,8 +820,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 b8b01895be1..c3c293a0ea3 100644 --- a/iocore/net/SSLSecret.cc +++ b/iocore/net/SSLSecret.cc @@ -18,14 +18,20 @@ 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) +#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) { // Call the load secret hooks // @@ -36,113 +42,93 @@ 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); + setSecret(name1, data1); + if (!name2.empty()) { + data2 = loadFile(name2); + setSecret(name2, data2); + } } - 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) { - Debug("ssl_secret", "File: %s received error: %s", name.c_str(), strerror(errno)); - 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 Debug("ssl_secret", "Loading file: %s failed ", name.c_str()); - 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{}; } - return data_item != nullptr; + if (iter->second.empty()) { + Debug("ssl_secret", "Get secret for %s: empty", name.c_str()); + return std::string{}; + } + // 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.empty() ? "[empty]" : 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] = ""; - } - 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; + 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 true; - } - } else { - return true; + return &(secret_map[name2]); + }(); + data1 = *data1ptr; + data2 = *data2ptr; + } + // If we can't find either secret, load them both again + 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); } - return false; } diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index d257f620406..551a1413deb 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -1020,8 +1020,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) { - Debug("ssl_load", "failed to load server private key from %s", - (!keyPath || keyPath[0] == '\0') ? "[empty key path]" : keyPath); + Debug("ssl_load", "failed to load server private key (%.*s) from %s", secret_data_len < 50 ? secret_data_len : 50, + secret_data, (!keyPath || keyPath[0] == '\0') ? "[empty key path]" : keyPath); return false; } if (!SSL_CTX_use_PrivateKey(ctx, pkey)) { @@ -2242,8 +2242,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()) { @@ -2402,8 +2402,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 with key path %s", cert_names_list[i].c_str(), @@ -2439,6 +2439,7 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector #include #include -#include -#include -#include +#include #include "tscore/ink_platform.h" #include "tscore/ink_base64.h" @@ -1362,7 +1359,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"); } @@ -1375,6 +1372,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 { @@ -9636,23 +9647,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; @@ -9670,32 +9678,34 @@ 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 char * +TSSslSecretGet(const char *secret_name, int secret_name_length, int *secret_data_length) { + sdk_assert(secret_name != nullptr); + sdk_assert(secret_data_length != nullptr); + 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)); + char *data{nullptr}; + if (secret_data.empty()) { + *secret_data_length = 0; + + } else { + data = static_cast(ats_malloc(secret_data.size())); + memcpy(data, secret_data.data(), secret_data.size()); + *secret_data_length = secret_data.size(); } if (loading) { SSLConfig::load_release(params); } else { SSLConfig::release(params); } - return retval; + return data; } /**