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

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/release-notes/whats-new.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.
 

include/ts/apidefs.h.in

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

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

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"
@@ -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_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

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