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

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())) {