9.2.x: Fix an error on SSL config reload (plus some cleanup). (#9334) (#9647)

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

* 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>
(cherry picked from commit 0c2488c)

Conflicts:
src/traffic_server/InkAPI.cc

* Remove TSSslSecretXxx TS API functions.

* Restore version of Au test tls_keepalive not needing ssl_secret_load_test.so.

Author: ywkaras

doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst

@@ -40,8 +40,8 @@ specified by :arg:`id`. Lifecycle hooks are based on the Traffic Server
 process, not on any specific transaction or session. These will typically be
 called only once during the execution of the Traffic Server process and
 therefore should be added in :func:`TSPluginInit` (which could itself be
-considered a lifecycle hook). Unlike other hooks, lifecycle hooks may not have a
-well defined ordering and use of them should not assume that one of the hooks
+considered a lifecycle hook). Unlike other hooks, lifecycle hooks may not have
+a well defined ordering and use of them should not assume that one of the hooks
 is always called before another unless specifically mentioned.
 
 Types
@@ -106,14 +106,6 @@ Types
       Invoked with the event :c:data:`TS_EVENT_LIFECYCLE_TASK_THREADS_READY` and ``NULL``
       data.
 
-   .. cpp:enumerator:: TS_LIFECYCLE_SSL_SECRET_HOOK
-
-      Called before the data for the certificate or key is loaded.  The data argument to the callback is a pointer to a :type:`TSSecretID` which
-      contains a pointer to the name of the certificate or key and the relevant version if applicable.
-
-      This hook gives the plugin a chance to load the certificate or key from an alternative source and set via the :c:func:`TSSslSecretSet` API.
-      If there is no plugin override, the certificate or key will be loaded from disk and the secret name will be interpreted as a file path.
-
    .. cpp:enumerator:: TS_LIFECYCLE_SHUTDOWN_HOOK
 
       Called after |TS| receiving a shutdown signal, such as SIGTERM.

doc/developer-guide/api/functions/TSSslSecret.en.rst

@@ -547,9 +547,6 @@ make the older ``stats_over_http`` obsolete.
 Plugin APIs
 -----------
 
-A new hook for loading certificates was added, :cpp:enumerator:`TS_LIFECYCLE_SSL_SECRET_HOOK`. When using
-this hook, the plugin recieved a structure with a type :c:type:`TSSecretID`.
-
 The transction control APIs where refactored and promoted to that ``ts.h`` public APIs. This adds
 :c:func:`TSHttpTxnCntlGet` and :c:func:`TSHttpTxnCntlSet`, and the c:enum::`TSHttpCntlType` enum.
 

doc/release-notes/whats-new.en.rst

@@ -438,8 +438,8 @@ typedef enum {
   TS_LIFECYCLE_MSG_HOOK,
   TS_LIFECYCLE_TASK_THREADS_READY_HOOK,
   TS_LIFECYCLE_SHUTDOWN_HOOK,
-  TS_LIFECYCLE_SSL_SECRET_HOOK,
-  TS_LIFECYCLE_LAST_HOOK
+  // TS_LIFECYCLE_SSL_SECRET_HOOK, future release
+  TS_LIFECYCLE_LAST_HOOK = TS_LIFECYCLE_SHUTDOWN_HOOK + 2
 } TSLifecycleHookID;
 
 /**

include/ts/apidefs.h.in

@@ -1302,13 +1302,6 @@ tsapi TSSslContext TSSslClientContextFindByName(const char *ca_paths, const char
 tsapi TSReturnCode TSSslClientCertUpdate(const char *cert_path, const char *key_path);
 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 TSSslSecretUpdate(const char *secret_name, int secret_name_length);
-
 /* Create a new SSL context based on the settings in records.config */
 tsapi TSSslContext TSSslServerContextCreate(TSSslX509 cert, const char *certname, const char *rsp_file);
 tsapi void TSSslContextDestroy(TSSslContext ctx);

include/ts/ts.h

@@ -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/cache/test/stub.cc

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

iocore/net/P_SSLConfig.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/P_SSLSecret.h

@@ -739,13 +739,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
@@ -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)};