You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by wk...@apache.org on 2022/04/12 15:37:52 UTC

[trafficserver] 01/01: General cleanup of the SSLSecret code.

This is an automated email from the ASF dual-hosted git repository.

wkaras pushed a commit to branch os_pkey_cnf_reload
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit bf229205fe281d07282315039fda60c0c02126e4
Author: Walt Karas <wk...@yahooinc.com>
AuthorDate: Tue Mar 29 18:34:48 2022 -0500

    General cleanup of the SSLSecret code.
    
    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 the 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 handle, TSHeapBuf, in order
    to be able to cleanly return a copy of the secret data.
---
 doc/developer-guide/api/functions/TSHeapBuf.en.rst |  72 ++++++++++
 .../api/functions/TSSslSecret.en.rst               |   5 +-
 include/ts/apidefs.h.in                            |   3 +
 include/ts/ts.h                                    |  32 ++++-
 iocore/net/P_SSLSecret.h                           |  13 +-
 iocore/net/SSLConfig.cc                            |   4 +-
 iocore/net/SSLSecret.cc                            | 118 ++++++----------
 iocore/net/SSLUtils.cc                             |  12 +-
 plugins/xdebug/Cleanup.h                           |  11 ++
 src/traffic_server/InkAPI.cc                       | 157 +++++++++++++++++----
 .../gold_tests/tls/tls_client_cert2_plugin.test.py |   2 +-
 11 files changed, 313 insertions(+), 116 deletions(-)

diff --git a/doc/developer-guide/api/functions/TSHeapBuf.en.rst b/doc/developer-guide/api/functions/TSHeapBuf.en.rst
new file mode 100644
index 000000000..1687d234d
--- /dev/null
+++ b/doc/developer-guide/api/functions/TSHeapBuf.en.rst
@@ -0,0 +1,72 @@
+.. Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed
+   with this work for additional information regarding copyright
+   ownership.  The ASF licenses this file to you under the Apache
+   License, Version 2.0 (the "License"); you may not use this file
+   except in compliance with the License.  You may obtain a copy of
+   the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   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
+
+TSHeapBufData
+*************
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+    #include <ts/ts.h>
+
+.. function:: char *TSHeapBufData(TSHeapBuf hbp)
+
+Description
+===========
+
+Returns a pointer to the first byte of data storage within the
+heap buffer specified by :arg:`bufp`.
+
+TSHeapBufLength
+***************
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+    #include <ts/ts.h>
+
+.. function:: int TSHeapBufLength(TSHeapBuf hbp)
+
+Description
+===========
+
+Returns the (non-negative) length in bytes of data storage within the
+heap buffer specified by :arg:`bufp`.
+
+TSHeapBufFree
+*************
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+    #include <ts/ts.h>
+
+.. function:: void TSHeapBufFree(TSHeapBuf hbp)
+
+Description
+===========
+
+Frees the heap buffer specified by :arg:`bufp`.
diff --git a/doc/developer-guide/api/functions/TSSslSecret.en.rst b/doc/developer-guide/api/functions/TSSslSecret.en.rst
index 87478e225..a94ee6b44 100644
--- a/doc/developer-guide/api/functions/TSSslSecret.en.rst
+++ b/doc/developer-guide/api/functions/TSSslSecret.en.rst
@@ -48,12 +48,13 @@ 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:: TSHeapBuf 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.  Returns null if the secret is not found,
+or the secret data in a TS heap buffer.
 
 TSSslSecretUpdate
 *****************
diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in
index ac550a95f..14daeb0c8 100644
--- a/include/ts/apidefs.h.in
+++ b/include/ts/apidefs.h.in
@@ -1454,6 +1454,9 @@ namespace ts
 }
 #endif
 
+// Handle for dynamically allocated variable-length buffer, with data returned by a TS API call.  May be null.
+typedef struct tsapi_heapbuf *TSHeapBuf;
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/include/ts/ts.h b/include/ts/ts.h
index 4d35972e7..35111d430 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 in a heap buffer.  Returns a null TSHeapBuf handle if there is no secret with the
+** given name.  Calling code must free TSHeapBuf by calling TSHeapBufFree. */
+tsapi TSHeapBuf TSSslSecretGet(const char *secret_name, int secret_name_length);
 
 tsapi TSReturnCode TSSslSecretUpdate(const char *secret_name, int secret_name_length);
 
@@ -2720,6 +2722,32 @@ tsapi void TSHostStatusSet(const char *hostname, const size_t hostname_len, TSHo
 tsapi bool TSHttpTxnCntlGet(TSHttpTxn txnp, TSHttpCntlType ctrl);
 tsapi TSReturnCode TSHttpTxnCntlSet(TSHttpTxn txnp, TSHttpCntlType ctrl, bool data);
 
+/**
+ * @brief Function that returns a pointer to data associated with a TSHeapBuf handle.
+ *
+ * @param A TSHeapBuf handle.
+ * @return A (non-null) pointer to data associated with a TSHeapBuf handle.
+ * @note The returned pointer has universal alignment (like a pointer returned by malloc() ), you can (reinterpret) cast it
+ * to a pointer to any type.
+ */
+tsapi char *TSHeapBufData(TSHeapBuf);
+
+/**
+ * @brief Function that returns the lenghth of the data associated with a TSHeapBuf handle.
+ *
+ * @param A TSHeapBuf handle.
+ * @return The length of the data associated with a TSHeapBuf handle.
+ */
+tsapi size_t TSHeapBufLength(TSHeapBuf);
+
+/**
+ * @brief Function that frees the buffer associated with a TSHeapBuf handle.
+ *
+ * @param The TSHeapBuf handle of the buffer to be freed.
+ * @note This function does nothing if the handle is null.
+ */
+tsapi void TSHeapBufFree(TSHeapBuf);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/iocore/net/P_SSLSecret.h b/iocore/net/P_SSLSecret.h
index f6b2a2794..212ab156c 100644
--- a/iocore/net/P_SSLSecret.h
+++ b/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;
diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
index f315c68fe..777246e77 100644
--- a/iocore/net/SSLConfig.cc
+++ b/iocore/net/SSLConfig.cc
@@ -805,8 +805,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 6cc10150b..32982ad04 100644
--- a/iocore/net/SSLSecret.cc
+++ b/iocore/net/SSLSecret.cc
@@ -18,14 +18,12 @@
   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)
+void
+SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::string &data1, std::string &data2)
 {
   // Call the load secret hooks
   //
@@ -40,107 +38,83 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s
     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() || 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);
+    if (!name2.empty()) {
+      data2 = loadFile(name2);
+    }
   }
-  return true;
 }
 
-bool
-SSLSecret::loadFile(const std::string &name, std::string &data_item)
+std::string
+SSLSecret::loadFile(const std::string &name)
 {
+  Debug("ssl_secret", "SSLSecret::loadFile(%s)", name.c_str());
   struct stat statdata;
   // Load the secret and add it to the map
   if (stat(name.c_str(), &statdata) < 0) {
-    return false;
+    return std::string{};
   }
   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) {
     // 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] = "";
+  std::string *const data1ptr = &(secret_map[name1]);
+  std::string *const data2ptr = [&]() -> std::string *const {
+    if (name2.empty()) {
+      data2.clear();
+      return &data2;
     }
-    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;
-      }
-      return true;
-    }
-  } else {
-    return true;
+    return &(secret_map[name2]);
+  }();
+  // If we can't find either secret, load them both again
+  if (data1ptr->empty() || (!name2.empty() && data2ptr->empty())) {
+    this->loadSecret(name1, name2, *data1ptr, *data2ptr);
   }
-  return false;
+  data1 = *data1ptr;
+  data2 = *data2ptr;
 }
diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
index 50492f5c7..28ec158fd 100644
--- a/iocore/net/SSLUtils.cc
+++ b/iocore/net/SSLUtils.cc
@@ -1044,7 +1044,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)) {
@@ -2258,8 +2259,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()) {
@@ -2409,8 +2410,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", cert_names_list[i].c_str());
@@ -2440,6 +2441,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())) {
diff --git a/plugins/xdebug/Cleanup.h b/plugins/xdebug/Cleanup.h
index ab383a5ec..e6391180b 100644
--- a/plugins/xdebug/Cleanup.h
+++ b/plugins/xdebug/Cleanup.h
@@ -100,6 +100,17 @@ struct TSIOBufferReaderDeleter {
 };
 using TSIOBufferReaderUniqPtr = std::unique_ptr<std::remove_pointer_t<TSIOBufferReader>, TSIOBufferReaderDeleter>;
 
+// Deleter and unique pointer for TSHeapBuf.
+//
+struct TSHeapBufDeleter {
+  void
+  operator()(TSHeapBuf ptr)
+  {
+    TSHeapBufFree(ptr);
+  }
+};
+using TSHeapBufUniqPtr = std::unique_ptr<std::remove_pointer_t<TSHeapBuf>, TSHeapBufDeleter>;
+
 class TxnAuxDataMgrBase
 {
 protected:
diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc
index f8d92e722..5146f4b83 100644
--- a/src/traffic_server/InkAPI.cc
+++ b/src/traffic_server/InkAPI.cc
@@ -21,12 +21,9 @@
   limitations under the License.
  */
 
-#include <cstdio>
 #include <atomic>
 #include <string_view>
-#include <tuple>
-#include <unordered_map>
-#include <string_view>
+#include <string>
 
 #include "tscore/ink_platform.h"
 #include "tscore/ink_base64.h"
@@ -1875,6 +1872,122 @@ _TSfree(void *ptr)
   ats_free(ptr);
 }
 
+// Support code for TSHeapBuf.
+namespace
+{
+class HeapBuf
+{
+public:
+  HeapBuf(TSHeapBuf b = nullptr) : _p{reinterpret_cast<_S *>(b)} {}
+
+  HeapBuf(HeapBuf const &src) = delete;
+
+  HeapBuf(HeapBuf &&src)
+  {
+    _p     = src._p;
+    src._p = nullptr;
+    ;
+  }
+
+  HeapBuf &operator=(HeapBuf const &src) = delete;
+
+  HeapBuf &
+  operator=(HeapBuf &&src)
+  {
+    if (&src != this) {
+      if (_p) {
+        std::free(_p);
+      }
+      _p     = src._p;
+      src._p = nullptr;
+      ;
+    }
+    return *this;
+  }
+
+  static HeapBuf
+  allocate(size_t size)
+  {
+    if (!size) {
+      size = 1;
+    }
+    HeapBuf b{static_cast<TSHeapBuf>(std::malloc(sizeof(_S) - 1 + size))};
+    b._p->data_size = size;
+    return b;
+  }
+
+  char *
+  data()
+  {
+    if (!_p) {
+      return nullptr;
+    }
+    return _p->data;
+  }
+
+  size_t
+  size() const
+  {
+    if (!_p) {
+      return 0;
+    }
+    return _p->data_size;
+  }
+
+  TSHeapBuf
+  release()
+  {
+    TSHeapBuf result = reinterpret_cast<TSHeapBuf>(_p);
+    _p               = nullptr;
+    return result;
+  }
+
+  ~HeapBuf()
+  {
+    if (_p) {
+      std::free(_p);
+    }
+  }
+
+private:
+  struct _S {
+    size_t data_size;
+    alignas(std::max_align_t) char data[1]; // Variable size.
+  };
+
+  _S *_p{nullptr};
+};
+
+} // end anonymous namespace
+
+char *
+TSHeapBufData(TSHeapBuf b)
+{
+  sdk_assert(sdk_sanity_check_null_ptr(b) == TS_SUCCESS);
+
+  HeapBuf bb{b};
+  char *data = bb.data();
+  bb.release();
+  return data;
+}
+
+size_t
+TSHeapBufLength(TSHeapBuf b)
+{
+  sdk_assert(sdk_sanity_check_null_ptr(b) == TS_SUCCESS);
+
+  HeapBuf bb{b};
+  size_t size = bb.size();
+  bb.release();
+  return size;
+}
+
+void
+TSHeapBufFree(TSHeapBuf b)
+{
+  HeapBuf bb(b);
+}
+
 ////////////////////////////////////////////////////////////////////
 //
 // Encoding utility
@@ -9617,23 +9730,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;
@@ -9651,32 +9761,27 @@ 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 TSHeapBuf
+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));
+  HeapBuf b;
+  if (!secret_data.empty()) {
+    b = HeapBuf::allocate(secret_data.size());
+    memcpy(b.data(), secret_data.data(), secret_data.size());
   }
   if (loading) {
     SSLConfig::load_release(params);
   } else {
     SSLConfig::release(params);
   }
-  return retval;
+  return b.release();
 }
 
 /**
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 4410e27c3..7a3ed7917 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 @@ ts.Disk.sni_yaml.AddLines([
     '  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),