Fix an error on SSL config reload (plus some cleanup). (#9334)

* 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.

* 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.

---------

Co-authored-by: Brian Neradt <brian.neradt@verizonmedia.com>

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 <ts/ts.h>
 
-.. 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 <ts/ts.h>
 
-.. 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
 *****************

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);
 

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
 {

iocore/net/P_SSLConfig.h

@@ -30,6 +30,8 @@
  ****************************************************************************/
 #pragma once
 
+#include <atomic>
+
 #include <openssl/rand.h>
 
 #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<char const *> secret_for_updateCTX{nullptr};
 };
 
 /////////////////////////////////////////////////////////////

iocore/net/P_SSLSecret.h

@@ -19,6 +19,8 @@
   limitations under the License.
  */
 
+#pragma once
+
 #include <string>
 #include <string_view>
 #include <mutex>
@@ -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<std::string, std::string> secret_map;
   mutable std::recursive_mutex secret_map_mutex;

iocore/net/SSLConfig.cc

@@ -745,13 +745,33 @@ 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);
 
   // Update the server cert
   SSLMultiCertConfigLoader loader(this);
   loader.update_ssl_ctx(cert_secret_name);
+
+  secret_for_updateCTX = nullptr;
 }
 
 void
@@ -806,8 +826,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)};

iocore/net/SSLSecret.cc

@@ -18,14 +18,20 @@
   See the License for the specific language governing permissions and
   limitations under the License.
  */
-#include <string>
-#include <map>
+
 #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 <utility>
+
+// 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;
 }

iocore/net/SSLUtils.cc

@@ -1053,8 +1053,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)) {
@@ -2285,8 +2285,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()) {
@@ -2445,8 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
 
   for (size_t i = 0; i < cert_names_list.size(); i++) {
     std::string keyPath = (i < key_list.size()) ? key_list[i] : "";
-    std::string_view secret_data;
-    std::string_view secret_key_data;
+    std::string secret_data;
+    std::string secret_key_data;
     params->secrets.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(),
@@ -2482,6 +2482,7 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
     }
 
     if (secret_key_data.empty()) {
+      Note("Empty private key for public key %.*s", int(secret_data.size()), secret_data.data());
       secret_key_data = secret_data;
     }
     if (!SSLPrivateKeyHandler(ctx, params, keyPath.c_str(), secret_key_data.data(), secret_key_data.size())) {