This is an automated email from the ASF dual-hosted git repository.
cmcfarlen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 30d8fcad44 ocsp: add single-cert stapling fast path and certinfo RAII
(#13229)
30d8fcad44 is described below
commit 30d8fcad44a7cb9945cf55db722e5737c357e406
Author: craigt <[email protected]>
AuthorDate: Tue Jun 9 17:41:55 2026 -0600
ocsp: add single-cert stapling fast path and certinfo RAII (#13229)
* ocsp: add single-cert stapling fast path and certinfo RAII
Skip the SSL_get_certificate() lookup and X509_cmp() DER re-parse in the
stapling callback when an SSL_CTX has a single certificate. The shortcut
is gated to non-dual-cert builds; under HAVE_NATIVE_DUAL_CERT_SUPPORT a
CTX can hold multiple certs where only one has OCSP info, so map size
alone cannot identify the negotiated cert.
Give certinfo a constructor/destructor so its resources are managed by
RAII, and allocate it with make_unique. This consolidates the cleanup
that was duplicated across certinfo_map_free and the init error path,
and fixes two pre-existing leaks (cid and the BoringSSL cert ref) plus
an error path that could delete a certinfo_map still owned by the
SSL_CTX.
* ocsp: make certinfo map own values via unique_ptr
Switch the per-SSL_CTX certinfo map to store std::unique_ptr<certinfo>
so ownership is explicit and the value cleanup is handled by RAII. This
removes the manual delete in certinfo_map_free and closes a leak window
at the insert site: the old release()-then-insert dropped the pointer if
the key already existed or the insert threw, whereas emplace with a move
only transfers ownership once insertion succeeds.
---
src/iocore/net/OCSPStapling.cc | 149 +++++++++++++++++++++++------------------
1 file changed, 82 insertions(+), 67 deletions(-)
diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc
index fa3527dc40..0c500f9006 100644
--- a/src/iocore/net/OCSPStapling.cc
+++ b/src/iocore/net/OCSPStapling.cc
@@ -21,6 +21,8 @@
#include "P_OCSPStapling.h"
+#include <memory>
+
#include <openssl/ssl.h>
#include <openssl/x509v3.h>
#include <openssl/asn1.h>
@@ -279,17 +281,36 @@ namespace
// Cached info stored in SSL_CTX ex_info
struct certinfo {
- unsigned char idx[20]; // Index in session cache SHA1 hash of certificate
- TS_OCSP_CERTID *cid; // Certificate ID for OCSP requests or nullptr if
ID cannot be determined
- char *uri; // Responder details
- char *certname;
- char *user_agent;
+ unsigned char idx[20] = {}; // Index in session cache SHA1 hash of
certificate
+ TS_OCSP_CERTID *cid = nullptr; // Certificate ID for OCSP requests
+ char *uri = nullptr; // Responder details
+ char *certname = nullptr;
+ char *user_agent = nullptr;
ink_mutex stapling_mutex;
- unsigned char resp_der[MAX_STAPLING_DER];
- unsigned int resp_derlen;
- bool is_prefetched;
- bool is_expire;
- time_t expire_time;
+ unsigned char resp_der[MAX_STAPLING_DER] = {};
+ unsigned int resp_derlen = 0;
+ bool is_prefetched = false;
+ bool is_expire = true;
+ time_t expire_time = 0;
+
+ certinfo() { ink_mutex_init(&stapling_mutex); }
+ ~certinfo()
+ {
+ if (cid) {
+ TS_OCSP_CERTID_free(cid);
+ }
+ if (uri) {
+ OPENSSL_free(uri);
+ }
+ ats_free(certname);
+ ats_free(user_agent);
+ ink_mutex_destroy(&stapling_mutex);
+ }
+
+ certinfo(const certinfo &) = delete;
+ certinfo &operator=(const certinfo &) = delete;
+ certinfo(certinfo &&) = delete;
+ certinfo &operator=(certinfo &&) = delete;
};
class HTTPRequest : public Continuation
@@ -724,7 +745,7 @@ TS_OCSP_resp_find_status(TS_OCSP_BASICRESP *bs,
TS_OCSP_CERTID *id, int *status,
* In the case of multiple certificates associated with a SSL_CTX, we must
store a map
* of cached responses
*/
-using certinfo_map = std::map<X509 *, certinfo *>;
+using certinfo_map = std::map<X509 *, std::unique_ptr<certinfo>>;
void
certinfo_map_free(void * /*parent*/, void *ptr, CRYPTO_EX_DATA * /*ad*/, int
/*idx*/, long /*argl*/, void * /*argp*/)
@@ -735,18 +756,13 @@ certinfo_map_free(void * /*parent*/, void *ptr,
CRYPTO_EX_DATA * /*ad*/, int /*i
return;
}
+#ifdef OPENSSL_IS_BORINGSSL
+ // The map owns a reference on each X509 key under BoringSSL; the certinfo
+ // values are freed by their unique_ptr destructors when the map is deleted.
for (auto &iter : *map) {
- certinfo *cinf = iter.second;
- if (cinf->uri) {
- OPENSSL_free(cinf->uri);
- }
-
- ats_free(cinf->certname);
- ats_free(cinf->user_agent);
-
- ink_mutex_destroy(&cinf->stapling_mutex);
- OPENSSL_free(cinf);
+ X509_free(iter.first);
}
+#endif
delete map;
}
@@ -860,28 +876,20 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const
char *certname, const cha
return false;
}
+ bool map_is_new = false;
if (!map) {
- map = new certinfo_map;
- }
- certinfo *cinf = static_cast<certinfo *>(OPENSSL_malloc(sizeof(certinfo)));
- if (!cinf) {
- Error("error allocating memory for %s", certname);
- delete map;
- return false;
+ map = new certinfo_map;
+ map_is_new = true;
}
+ auto cinf_ptr = std::make_unique<certinfo>();
+ certinfo *cinf = cinf_ptr.get();
// Initialize certinfo
- cinf->cid = nullptr;
- cinf->uri = nullptr;
cinf->certname = ats_strdup(certname);
if (SSLConfigParams::ssl_ocsp_user_agent != nullptr) {
cinf->user_agent = ats_strdup(SSLConfigParams::ssl_ocsp_user_agent);
}
- cinf->resp_derlen = 0;
- ink_mutex_init(&cinf->stapling_mutex);
cinf->is_prefetched = rsp_file ? true : false;
- cinf->is_expire = true;
- cinf->expire_time = 0;
if (cinf->is_prefetched) {
Dbg(dbg_ctl_ssl_ocsp, "using OCSP prefetched response file %s", rsp_file);
@@ -949,24 +957,17 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const
char *certname, const cha
X509_up_ref(cert);
#endif
- map->insert(std::make_pair(cert, cinf));
+ map->emplace(cert, std::move(cinf_ptr));
SSL_CTX_set_ex_data(ctx, ssl_stapling_index, map);
Note("successfully initialized stapling for %s into SSL_CTX: %p uri=%s",
certname, ctx, cinf->uri);
return true;
err:
- if (cinf->cid) {
- TS_OCSP_CERTID_free(cinf->cid);
- }
-
- ats_free(cinf->certname);
- ats_free(cinf->user_agent);
-
- if (cinf) {
- OPENSSL_free(cinf);
- }
- if (map) {
+ // cinf_ptr destructor handles certinfo cleanup.
+ // Only tear down the map if this call created it; an existing map is still
+ // owned by the SSL_CTX.
+ if (map_is_new) {
delete map;
}
@@ -1327,7 +1328,7 @@ ocsp_update()
if (map) {
// Walk over all certs associated with this CTX
for (auto &iter : *map) {
- cinf = iter.second;
+ cinf = iter.second.get();
ink_mutex_acquire(&cinf->stapling_mutex);
current_time = time(nullptr);
if (cinf->resp_derlen == 0 || cinf->is_expire ||
cinf->expire_time < current_time) {
@@ -1370,32 +1371,46 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *)
return SSL_TLSEXT_ERR_NOACK;
}
- // Fetch the specific certificate used in this negotiation
- X509 *cert = SSL_get_certificate(ssl);
- if (!cert) {
- Error("ssl_callback_ocsp_stapling: failed to get certificate");
- return SSL_TLSEXT_ERR_NOACK;
- }
-
certinfo *cinf = nullptr;
+
+#ifndef HAVE_NATIVE_DUAL_CERT_SUPPORT
+ // Fast path: without native dual-cert support each SSL_CTX holds exactly one
+ // certificate, so a single map entry must be the negotiated cert. This skips
+ // the SSL_get_certificate() lookup and the X509_cmp() DER re-parse below.
+ // Not safe under HAVE_NATIVE_DUAL_CERT_SUPPORT: a dual-cert CTX where only
one
+ // cert has OCSP info also yields map->size()==1, but the negotiated cert may
+ // be the other one.
+ if (map->size() == 1) {
+ cinf = map->begin()->second.get();
+ } else
+#endif
+ {
+ // Fetch the specific certificate used in this negotiation
+ X509 *cert = SSL_get_certificate(ssl);
+ if (!cert) {
+ Error("ssl_callback_ocsp_stapling: failed to get certificate");
+ return SSL_TLSEXT_ERR_NOACK;
+ }
+
#if HAVE_NATIVE_DUAL_CERT_SUPPORT
- certinfo_map::iterator iter = map->find(cert);
- if (iter != map->end()) {
- cinf = iter->second;
- }
-#else
- for (certinfo_map::iterator iter = map->begin(); iter != map->end(); ++iter)
{
- X509 *key = iter->first;
- if (key == nullptr) {
- continue;
+ certinfo_map::iterator iter = map->find(cert);
+ if (iter != map->end()) {
+ cinf = iter->second.get();
}
+#else
+ for (certinfo_map::iterator iter = map->begin(); iter != map->end();
++iter) {
+ X509 *key = iter->first;
+ if (key == nullptr) {
+ continue;
+ }
- if (X509_cmp(key, cert) == 0) {
- cinf = iter->second;
- break;
+ if (X509_cmp(key, cert) == 0) {
+ cinf = iter->second.get();
+ break;
+ }
}
- }
#endif
+ }
if (cinf == nullptr) {
Error("ssl_callback_ocsp_stapling: failed to get certificate information
for ssl=%p", ssl);
