Repository navigation
Conversation
The BoringSSL/OpenSSL upstream connector did a full TLS handshake for every new connection: its context had no client session cache and connect() never offered a previous session, so the session IDs and tickets upstreams issued were dropped. Whenever the keepalive pool could not satisfy a request (concurrent streaming requests over HTTP/1.1, mutual TLS, post-quantum key shares), the upstream paid a full handshake, where nginx as a client would have resumed. PeerOptions::tls_session_reuse (default off) now makes the connector keep the latest session the server issued and offer it on the next connection to a peer with the same reuse hash and ALPN. The sessions live in the connector context's ex data, keyed per peer, and are collected through the context's new-session callback, since TLS 1.3 tickets arrive after the handshake. They are stored serialized, because OpenSSL marks a session object not resumable when a connection holding it is freed without a TLS shutdown. The cache is a 1024-entry LRU. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6a6c7b47d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The session key was the peer's reuse hash and ALPN, and HttpPeer's hash leaves the CA bundle out, so a peer that only changed options.ca could resume a session established under the previous trust store, and a resumed handshake does not verify the server's certificate again. The DER of the peer's CAs is now part of the key, so a trust-store change is a full handshake. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
254de8e makes the session key include the peer's CA bundle (Codex's finding above); the PR description is updated accordingly. The |
Fixes #1114.
What
The BoringSSL/OpenSSL upstream connector did a full TLS handshake for every new upstream connection: the
SslConnectorwas built without a client session cache andconnect()never offered a previous session, so the session IDs and tickets the upstream issued were dropped. Whenever the keepalive pool could not satisfy a request (concurrent streaming requests over HTTP/1.1, mutual TLS, post-quantum key shares) the upstream paid a full handshake, where nginx as a client (proxy_ssl_session_reuse on, its default) resumes.PeerOptions::tls_session_reuse(default off, so nothing changes for existing users) makes the connector keep the latest session the server issued and offer it on the next connection to a peer with the samereuse_hashand ALPN:SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL_STOREand a new-session callback, the only way to see TLS 1.3 tickets, which arrive after the handshake (usually with the first read); the library's own session cache is for servers and stays off;hash(peer.reuse_hash(), alpn, peer CA bundle), so peers that may share a pooled connection, offer the same ALPN and trust the same CAs share sessions and nothing else does (the CA bundle is not inreuse_hash, and a resumed handshake does not verify the server's certificate again); the key of the connection being made travels in itsSsl's ex data to the callback;i2d/d2i), not asSslSessionobjects: OpenSSL marks a session object not resumable when a connection holding it is freed without a TLS shutdown (ssl_clear_bad_session()inSSL_free()), which is what happens to a pooled upstream connection when it is dropped. A copy per connection is immune to that; BoringSSL does not do this, and the serialized form costs the same on both;connect()callsSSL_set_sessionbefore the handshake when there is a session for the key. An expired or refused session falls back to a full handshake inside the library, and the new ticket replaces it.The docs of
upstream_tls_verify_callbackalready say it does not run for resumed sessions. Reporting resumption to callers needs no new field:upstream_tls_handshake_complete_hookreceives theSslRefand can callsession_reused().lrubecomes a dependency of theopenssl_derivedfeature (it already is one ofs2n), andpingora-boringssl/pingora-opensslre-exportex_datafor theIndextype.Tests
In
pingora-core/src/connectors/tls/boringssl_openssl/mod.rs, each against a localSslAcceptorkept across handshakes:tls_sessions_are_not_reused_by_default: two handshakes with one connector, neither resumed; this is also whatmaindoes whatever the peer says;tls13_sessions_are_resumed_when_the_peer_enables_it: the ticket of the first handshake (delivered by the read after it) resumes the second, whose ticket resumes the third;tls12_sessions_are_resumed_when_the_peer_enables_it;tls_sessions_are_kept_per_peer_alpn_and_ca: another SNI, another ALPN or another CA bundle is another entry, and the first entry survives them.They pass with
--features boringssland with--features openssl; the OpenSSL run is what found theSSL_free()behaviour above.Measured
A 32-core Linux x86_64 host running nginx 1.31.8 (OpenSSL 3.5.9) as a mutual-TLS upstream (TLS 1.3, ECDSA P-256 certificates,
ssl_verify_client on,$ssl_session_reusedlogged, andkeepalive_timeout 0so that the pool never has a connection to reuse and every request is a new upstream connection), apingora-proxyon this branch (boringsslfeature, 8 threads) in front of it, andoha -c 64 -z 20sover HTTP/1.1 as the client. nginx CPU is its cgroup'susage_usec, the proxy's is from/proc/<pid>/stat; both runs of a pair saturate the host, so the request rate follows the handshake cost.tls_session_reusePer upstream connection nginx spends 40 % (X25519) and 36 % (X25519MLKEM768) less CPU on the handshake, the proxy 33 % and 29 % less, and the same client load gets 40 % and 34 % more requests through. The 64 connections per run that are not resumed are the first upstream connection of each of the 64 concurrent client connections, made before the first ticket had arrived.
🤖 Generated with Claude Code