ocsp: use Bravo lock for cached responses

[bneradt]

Originally authored by Craig Taylor (@c-taylor) in #13226. This carries his work forward on current master and preserves his commit authorship.

Replace the per-certinfo ink_mutex with a ts::bravo::shared_mutex so readers on the TLS handshake hot path (ssl_callback_ocsp_stapling) and the refresh scan (ocsp_update) can run concurrently; only the cache update takes an exclusive lock. The OpenSSL handshake reader now allocates outside the shared lock, then revalidates the response size and copies the DER staple directly from the cache while locked, avoiding an extra full copy. BoringSSL copies the cached response directly while holding the shared lock.

The original work also fixed three pre-existing bugs. Those companion fixes have since landed on master and are preserved by this rebase:

• certinfo_map_free never freed cinf->cid (leak on every CTX teardown).
• On BoringSSL the X509 key ref taken via X509_up_ref was never released; free it in certinfo_map_free.
• ssl_stapling_init_cert error path could delete a certinfo_map still owned by the SSL_CTX. Only delete a map this call created.

This is a rework of #13097 to separate into 2x independent PRs.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Improve OCSP stapling cache concurrency by replacing per-certificate mutex serialization with a Bravo reader-writer lock, reducing TLS handshake hot-path contention while keeping cache updates exclusive.

Changes:

• Replace ink_mutex in certinfo with ts::bravo::shared_mutex and adopt shared/exclusive locking patterns.
• Copy cached OCSP DER under a shared lock, then perform OpenSSL/BoringSSL handoff outside the critical section.
• Update refresh scanner (ocsp_update) to use shared reads for staleness checks.

[c-taylor]

Thank you!

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/iocore/net/OCSPStapling.cc:1451

• In the OpenSSL (non-BoringSSL) path, resp_derlen, required_capacity, and is_response_available are declared without initialization and only conditionally assigned inside the loop. Even though the control flow ensures they’re set before use, this pattern can trigger -Wmaybe-uninitialized/static-analysis warnings and makes the loop harder to reason about. Initialize them defensively at declaration to keep the code warning-free and clearer.

  unsigned char *p             = nullptr;
  unsigned int   resp_capacity = 0;
  unsigned int   resp_derlen;

  while (true) {

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[cmcfarlen]

The retry loop is correct, though it took some staring to convince myself. is_response_available guarantees resp_derlen != 0, so on the first pass resp_derlen > 0 == resp_capacity and the memcpy(p, ...) with p == nullptr is unreachable. Responses are bounded at cache time (resp_derlen > MAX_STAPLING_DER is rejected at line 845), so the loop realistically runs at most twice. The !is_response_available exit frees p correctly. is_prefetched becoming const and read unlocked is right — it is construction-time only.

Main suggestion: use ts::bravo::shared_mutex instead of std::shared_mutex.

That is the codebase's own idiom for exactly this shape — read-mostly state with hot readers. src/iocore/net/ConnectionTracker.cc uses it throughout (std::lock_guard<ts::bravo::shared_mutex> for writers, ts::bravo::shared_lock<...> for readers), as does P_SSLCertLookup.h. BRAVO is biased toward readers specifically to avoid the reader cache-line contention a plain std::shared_mutex suffers, which is the whole motivation for this change on a TLS handshake path.

Two reasons beyond consistency:

1. ts::bravo::shared_mutex is annotated for -Wthread-safety — include/tsutil/Bravo.h carries TS_CAPABILITY("shared_mutex"), TS_SCOPED_CAPABILITY, TS_ACQUIRE_SHARED, TS_RELEASE_SHARED. std::shared_mutex is annotated by libc++ but not by libstdc++, so using it here produces clang thread-safety findings on macOS that Linux CI will not show.
2. include/tsutil/Bravo.h is present on both master and 10.2.x, so this costs nothing for the backport.

Possible crossed wire: your reply to Copilot above says the writer path "now explicitly uses std::lock_guard<ts::bravo::shared_mutex>", but the current diff has std::lock_guard<std::shared_mutex> and mutable std::shared_mutex resp_mutex. The substance of Copilot's point is addressed — it is an explicit type rather than CTAD — but the type named in the reply isn't what landed. Did a Bravo conversion get lost somewhere, or was that describing intent?

Minor: the description says certinfo is allocated with new/delete "instead of OPENSSL_malloc", but master is already on std::make_unique<certinfo>() (line 885). Stale wording only, no code impact.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/iocore/net/OCSPStapling.cc:1438

• In the BoringSSL path, SiteThrottledError(...) is invoked while holding cinf->resp_mutex (shared lock). This does log formatting + throttling and can take additional internal locks; doing that while holding the OCSP cache lock increases contention on the TLS handshake hot path and can amplify lock-ordering risk. Consider computing the availability predicate under the shared lock, then releasing the lock before logging/returning.

    ts::bravo::shared_lock<ts::bravo::shared_mutex> 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)) {
      SiteThrottledError("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
      return SSL_TLSEXT_ERR_NOACK;
    }

[cmcfarlen]

Lets make sure the squash commit message says we used bravo lock since it says std::shared_mutex atm. Thanks again for getting this across the finish line!

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 25a5858 for the 10.2.0 release.