You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "c-taylor (via GitHub)" <gi...@apache.org> on 2026/04/17 20:19:16 UTC
[PR] ocsp: replace stapling_mutex with bravo shared_mutex [trafficserver]
c-taylor opened a new pull request, #13097:
URL: https://github.com/apache/trafficserver/pull/13097
Replace the ink_mutex in OCSP stapling with ts::bravo::shared_mutex to allow true reader concurrency on the TLS handshake hot path. BRAVO provides a lock-free fast path for readers via per-thread atomic slots, avoiding the shared counter bounce of std::shared_mutex.
Skip the SSL_get_certificate() call in the OCSP stapling callback when only one certificate exists in the map, avoiding a DER re-parse on every handshake. On older Intel systems this gave +5% HS/sec
Give certinfo a proper destructor to consolidate resource cleanup and fix two pre-existing leaks (cid and BoringSSL cert ref). Fix a pre-existing bug where the error path in ssl_stapling_init_cert could delete a certinfo_map still owned by the SSL_CTX.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#discussion_r3108374635
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -831,12 +848,13 @@ stapling_cache_response(TS_OCSP_RESPONSE *rsp, certinfo *cinf)
return false;
}
- ink_mutex_acquire(&cinf->stapling_mutex);
- memcpy(cinf->resp_der, resp_der, resp_derlen);
- cinf->resp_derlen = resp_derlen;
- cinf->is_expire = false;
- cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout;
- ink_mutex_release(&cinf->stapling_mutex);
+ {
+ std::lock_guard lock(cinf->resp_mutex);
+ memcpy(cinf->resp_der, resp_der, resp_derlen);
Review Comment:
This file now uses `std::lock_guard`, but it doesn't include `<mutex>` directly. To avoid relying on transitive includes (which can vary by standard library implementation), add the explicit standard header include for the locking helpers being used here.
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -1370,56 +1372,69 @@ 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;
-#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;
+
+ // Fast path: if only one certificate in the map, skip SSL_get_certificate() lookup
+ if (map->size() == 1) {
+ cinf = map->begin()->second;
+ } else {
+ // 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 (X509_cmp(key, cert) == 0) {
+#if HAVE_NATIVE_DUAL_CERT_SUPPORT
+ certinfo_map::iterator iter = map->find(cert);
+ if (iter != map->end()) {
cinf = iter->second;
- break;
}
- }
+#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;
+ }
+ }
#endif
+ }
if (cinf == nullptr) {
Error("ssl_callback_ocsp_stapling: failed to get certificate information for ssl=%p", ssl);
return SSL_TLSEXT_ERR_NOACK;
}
- ink_mutex_acquire(&cinf->stapling_mutex);
- time_t current_time = time(nullptr);
- if ((cinf->resp_derlen == 0 || cinf->is_expire) || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
- ink_mutex_release(&cinf->stapling_mutex);
- Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
- return SSL_TLSEXT_ERR_NOACK;
- } else {
- unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(cinf->resp_derlen));
- if (p == nullptr) {
- ink_mutex_release(&cinf->stapling_mutex);
- Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+ unsigned char resp_copy[MAX_STAPLING_DER];
+ unsigned int resp_copylen;
+
+ {
+ ts::bravo::shared_lock lock(cinf->resp_mutex);
+
+ time_t current_time = time(nullptr);
+ if (cinf->resp_derlen == 0 || cinf->is_expire || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
+ Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
return SSL_TLSEXT_ERR_NOACK;
}
- memcpy(p, cinf->resp_der, cinf->resp_derlen);
- ink_mutex_release(&cinf->stapling_mutex);
- SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
- Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname);
- Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri);
- return SSL_TLSEXT_ERR_OK;
+
+ resp_copylen = cinf->resp_derlen;
+ memcpy(resp_copy, cinf->resp_der, resp_copylen);
+ }
+
+ unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(resp_copylen));
+ if (p == nullptr) {
+ Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+ return SSL_TLSEXT_ERR_NOACK;
}
+ memcpy(p, resp_copy, resp_copylen);
+ SSL_set_tlsext_status_ocsp_resp(ssl, p, resp_copylen);
Review Comment:
The new reader path copies the OCSP response twice (into the fixed-size stack buffer, then into the heap buffer passed to OpenSSL). Since this runs on the TLS handshake hot path, consider whether you can avoid the extra copy while still keeping lock hold times short (e.g., allocate once and copy once).
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -279,17 +281,38 @@ 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;
- 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 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;
+ bool is_prefetched = false;
+
+ // OCSP response data, protected by resp_mutex.
+ // Readers take a shared lock; the updater takes an exclusive lock.
+ unsigned char resp_der[MAX_STAPLING_DER] = {};
+ unsigned int resp_derlen = 0;
+ bool is_expire = true;
+ time_t expire_time = 0;
+ mutable ts::bravo::shared_mutex resp_mutex;
Review Comment:
`ts::bravo::shared_mutex` embeds an array of 256 cacheline-aligned reader slots (see tsutil/Bravo.h), so storing it inline in each `certinfo` significantly increases per-certificate memory footprint (~16KB just for the slot array, in addition to the 10KB `resp_der`). If deployments can have many SSL_CTX/SNI certs, consider whether a single mutex per `certinfo_map` (or another shared location) would provide the same concurrency benefits with less memory overhead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#issuecomment-4654371284
Abandoning now as the 2x "independent" PRs are making progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on code in PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#discussion_r3112227001
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -1370,56 +1372,69 @@ 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;
-#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;
+
+ // Fast path: if only one certificate in the map, skip SSL_get_certificate() lookup
+ if (map->size() == 1) {
+ cinf = map->begin()->second;
+ } else {
+ // 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 (X509_cmp(key, cert) == 0) {
+#if HAVE_NATIVE_DUAL_CERT_SUPPORT
+ certinfo_map::iterator iter = map->find(cert);
+ if (iter != map->end()) {
cinf = iter->second;
- break;
}
- }
+#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;
+ }
+ }
#endif
+ }
if (cinf == nullptr) {
Error("ssl_callback_ocsp_stapling: failed to get certificate information for ssl=%p", ssl);
return SSL_TLSEXT_ERR_NOACK;
}
- ink_mutex_acquire(&cinf->stapling_mutex);
- time_t current_time = time(nullptr);
- if ((cinf->resp_derlen == 0 || cinf->is_expire) || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
- ink_mutex_release(&cinf->stapling_mutex);
- Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
- return SSL_TLSEXT_ERR_NOACK;
- } else {
- unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(cinf->resp_derlen));
- if (p == nullptr) {
- ink_mutex_release(&cinf->stapling_mutex);
- Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+ unsigned char resp_copy[MAX_STAPLING_DER];
+ unsigned int resp_copylen;
+
+ {
+ ts::bravo::shared_lock lock(cinf->resp_mutex);
+
+ time_t current_time = time(nullptr);
+ if (cinf->resp_derlen == 0 || cinf->is_expire || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
+ Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
return SSL_TLSEXT_ERR_NOACK;
}
- memcpy(p, cinf->resp_der, cinf->resp_derlen);
- ink_mutex_release(&cinf->stapling_mutex);
- SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
- Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname);
- Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri);
- return SSL_TLSEXT_ERR_OK;
+
+ resp_copylen = cinf->resp_derlen;
+ memcpy(resp_copy, cinf->resp_der, resp_copylen);
+ }
+
+ unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(resp_copylen));
+ if (p == nullptr) {
+ Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+ return SSL_TLSEXT_ERR_NOACK;
}
+ memcpy(p, resp_copy, resp_copylen);
+ SSL_set_tlsext_status_ocsp_resp(ssl, p, resp_copylen);
Review Comment:
Agreed; Resolving.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor closed pull request #13097: ocsp: stapling fast path and shared mutex
URL: https://github.com/apache/trafficserver/pull/13097
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on code in PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#discussion_r3112235904
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -279,17 +281,38 @@ 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;
- 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 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;
+ bool is_prefetched = false;
+
+ // OCSP response data, protected by resp_mutex.
+ // Readers take a shared lock; the updater takes an exclusive lock.
+ unsigned char resp_der[MAX_STAPLING_DER] = {};
+ unsigned int resp_derlen = 0;
+ bool is_expire = true;
+ time_t expire_time = 0;
+ mutable ts::bravo::shared_mutex resp_mutex;
Review Comment:
bravo was selected based on advice: After reviewing the use case and work pattern, will be migrating this and https://github.com/apache/trafficserver/pull/13098 to std:shared_mutex
1. Simplicity
2. Memory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on code in PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#discussion_r3112912025
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -831,12 +848,13 @@ stapling_cache_response(TS_OCSP_RESPONSE *rsp, certinfo *cinf)
return false;
}
- ink_mutex_acquire(&cinf->stapling_mutex);
- memcpy(cinf->resp_der, resp_der, resp_derlen);
- cinf->resp_derlen = resp_derlen;
- cinf->is_expire = false;
- cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout;
- ink_mutex_release(&cinf->stapling_mutex);
+ {
+ std::lock_guard lock(cinf->resp_mutex);
+ memcpy(cinf->resp_der, resp_der, resp_derlen);
Review Comment:
Agreed; will resolve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ocsp: stapling fast path and shared mutex [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#issuecomment-4299226643
After speaking with @cmcfarlen - There's 2x changes in this PR
1. The SSL_CTX map that avoids duplicate DER decode
2. mutex changes
I'm going to switch this back to draft and resubmit as 2x PR. Will abandon in due course.
I'll re-run the tests on both parts, as I only have data for the combination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org