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..bff6ba33e4b 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..dc1c97e6b12 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,111 +42,92 @@ 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) { - 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{}; } - 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.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; + { + 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 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),