feat: resume TLS sessions instead of a full handshake per connection - #36
feat: resume TLS sessions instead of a full handshake per connection#36shreemaan-abhishek wants to merge 2 commits into
Conversation
Every https connection started a full handshake, even against a peer this worker handshook with seconds earlier and that was offering to resume. On the connection-per-request TLS shape the handshake is most of the request cost. A per-worker cache holds one session per TLS peer, up to 256, evicting the least recently used and dropping expired ones on the way out. A fresh connection offers the session for its key before the handshake; OpenSSL reports new sessions through the session callback, which is what makes TLS 1.3 resume too, since its tickets arrive after the handshake rather than during it. The cache key is the verify mode, the peer address, the SNI and the trusted CA - every input the original handshake was judged on, since a resumed one skips the certificate exchange and answers with the verify result stored in the session. A session is published only once the handshake has passed the checks the caller asked for, so a rejected peer leaves nothing behind. Resumption is on by default and both entry points take ssl_session_reuse = false to turn it off, as nginx's proxy_ssl_session_reuse does.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds opt-out TLS session reuse for stateless and stateful HTTPS clients. A worker-level 256-entry LRU cache stores resumable sessions with verification, peer, SNI, and CA isolation. Tests cover TLS 1.2/1.3 reuse, expiry, eviction, and handshake acceptance. ChangesTLS session resumption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ngx_http_ffi_client_lua
participant ngx_http_ffi_client_request
participant ngx_http_ffi_client_ssl_session
participant OpenSSL
ngx_http_ffi_client_lua->>ngx_http_ffi_client_request: Start HTTPS request with ssl_session_reuse
ngx_http_ffi_client_request->>ngx_http_ffi_client_ssl_session: Offer cached session
ngx_http_ffi_client_ssl_session-->>ngx_http_ffi_client_request: Return session or cache miss
ngx_http_ffi_client_request->>OpenSSL: Perform TLS handshake
OpenSSL-->>ngx_http_ffi_client_request: Accept handshake and provide session
ngx_http_ffi_client_request->>ngx_http_ffi_client_ssl_session: Commit accepted session
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ngx_http_ffi_client_request.c (1)
1874-1880: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider treating session-reuse setup failure as non-fatal.
ngx_http_ffi_client_ssl_session_offer()returns NGX_ERROR for allocation failures and for cache creation failure. Those conditions do not prevent a normal handshake. As written, they fail the whole request.Session resumption is an optimization. The session module already applies that reasoning to a refused session: src/ngx_http_ffi_client_ssl_session.c line 172 ignores the result of
ngx_ssl_set_session()because a session the peer forgot only costs a full handshake. The same argument applies to setup failure.Log the failure and continue with a full handshake instead.
♻️ Proposed change to degrade instead of failing
- if (op->ssl_session_reuse - && ngx_http_ffi_client_ssl_session_offer(op, c) != NGX_OK) - { - ngx_http_ffi_client_finalize(op, NGX_ERROR, - "failed to set up TLS session reuse"); - return NGX_ERROR; - } + /* resumption is an optimization: a setup failure only costs a full + * handshake, so it must not fail the request */ + if (op->ssl_session_reuse + && ngx_http_ffi_client_ssl_session_offer(op, c) != NGX_OK) + { + ngx_log_error(NGX_LOG_WARN, c->log, 0, + "ngx_http_ffi_client: TLS session reuse unavailable"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ngx_http_ffi_client_request.c` around lines 1874 - 1880, Update the ssl_session_reuse handling around ngx_http_ffi_client_ssl_session_offer() so NGX_ERROR is logged but does not call ngx_http_ffi_client_finalize() or return NGX_ERROR. Continue the request into the normal TLS handshake, allowing it to proceed without session reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 108-109: Update the HTTPS connection explanation in the README to
distinguish TLS versions: qualify TLS 1.2 session resumption as reducing two
round trips to one only when the server accepts the session, and state that
normal TLS 1.3 handshakes and TLS 1.3 PSK resumption use one round trip without
early data. Describe TLS 1.3 resumption as omitting certificate transmission
rather than universally reducing handshake latency.
In `@t/016-tls-session-resumption.t`:
- Around line 395-407: Update the TLS session resumption test to populate
exactly 256 cache entries, access h1 again to promote it in the LRU order, then
add h257. Change the assertions around the request/response flow so h1 and h257
resume successfully while h2 misses, thereby validating both the 256-entry
capacity and LRU promotion behavior.
---
Nitpick comments:
In `@src/ngx_http_ffi_client_request.c`:
- Around line 1874-1880: Update the ssl_session_reuse handling around
ngx_http_ffi_client_ssl_session_offer() so NGX_ERROR is logged but does not call
ngx_http_ffi_client_finalize() or return NGX_ERROR. Continue the request into
the normal TLS handshake, allowing it to proceed without session reuse.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 023c5ce9-e576-4f21-ac1b-a2c46d2f0b08
📒 Files selected for processing (7)
README.mdconfiglib/resty/ngx_http_ffi_client.luasrc/ngx_http_ffi_client.hsrc/ngx_http_ffi_client_request.csrc/ngx_http_ffi_client_ssl_session.ct/016-tls-session-resumption.t
| An https connection offers the server the session from an earlier handshake with | ||
| the same peer, so it costs one round trip instead of two and no certificate work. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- README.md lines 90-125 ---'
sed -n '90,125p' README.md
printf '%s\n' '--- session and handshake references ---'
rg -n -i 'session|handshake|TLS|https connection|certificate' --glob '!node_modules' --glob '!dist' --glob '!build' .
printf '%s\n' '--- tracked files near likely client/session implementation ---'
git ls-files | rg -i 'session|cache|tls|https|client|connection|readme'Repository: api7/ngx_http_ffi_client
Length of output: 36104
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- README.md TLS section ---'
sed -n '106,132p' README.md
printf '%s\n' '--- session offer and handshake implementation ---'
sed -n '108,218p' src/ngx_http_ffi_client_ssl_session.c
sed -n '1840,1960p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- TLS resumption tests ---'
sed -n '1,50p' t/016-tls-session-resumption.t
sed -n '100,205p' t/016-tls-session-resumption.t
printf '%s\n' '--- static path verifier ---'
python3 - <<'PY'
from pathlib import Path
readme = Path("README.md").read_text()
source = Path("src/ngx_http_ffi_client_request.c").read_text()
session = Path("src/ngx_http_ffi_client_ssl_session.c").read_text()
tests = Path("t/016-tls-session-resumption.t").read_text()
checks = {
"README claims unconditional one-vs-two RTT": (
"so it costs one round trip instead of two" in readme
),
"client offers cached session before handshake": (
source.index("ngx_http_ffi_client_ssl_session_offer(op, c)")
< source.index("rc = ngx_ssl_handshake(c)")
),
"OpenSSL handshake result controls fallback/failure path": (
"rc = ngx_ssl_handshake(c)" in source
and "if (!c->ssl->handshaked)" in source
),
"TLS 1.3 and TLS 1.2 are both exercised": (
"ssl_protocols TLSv1.3;" in tests
and "ssl_protocols TLSv1.2;" in tests
),
"tests explicitly cover both protocol resumptions": (
"TLS 1.3 resumes the session" in tests
and "TLS 1.2 resumes the session" in tests
),
"README states resumed handshake skips certificate exchange": (
"A resumed handshake skips the certificate exchange" in readme
),
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
PYRepository: api7/ngx_http_ffi_client
Length of output: 13203
🌐 Web query:
RFC 8446 TLS 1.3 full handshake PSK resumption round trips certificate exchange TLS 1.2 abbreviated handshake
💡 Result:
In TLS 1.3 (RFC 8446), the handshake design significantly reduces latency compared to previous versions [1][2]. A standard TLS 1.3 full handshake requires one round trip (1-RTT) [1][3], whereas a TLS 1.3 PSK (Pre-Shared Key) resumption handshake can achieve zero round trips (0-RTT) for application data [1][2][3]. Full Handshake vs. PSK Resumption In a TLS 1.3 full handshake, the client sends a ClientHello with key shares, and the server responds with a ServerHello, EncryptedExtensions, Certificate, CertificateVerify, and Finished message [2]. Because the server is authenticating via a certificate, it must send the Certificate and CertificateVerify messages [4][5]. In contrast, a PSK resumption handshake (often called "resumption" or "PSK mode") uses a shared secret from a previous connection [4][3]. When a server authenticates via a PSK, it does not send Certificate or CertificateVerify messages [4][5]. If 0-RTT is enabled, the client can send encrypted application data in its first flight (the ClientHello), effectively achieving zero round-trip latency [1][2][3]. Comparison with TLS 1.2 TLS 1.2 typically required two round trips for a full handshake [1][6]. While TLS 1.2 supported "abbreviated handshakes" (session resumption via Session IDs or Session Tickets), these still required a round trip [6][3]. TLS 1.3 obsoletes the older TLS 1.2 session ID and ticket mechanisms [4][7], replacing them with the integrated PSK resumption framework [6][3]. Key Differences Summary - Full Handshake: TLS 1.3 requires 1-RTT, compared to 2-RTT in TLS 1.2 [1][6][2]. - Certificate Exchange: Standard TLS 1.3 full handshakes include Certificate and CertificateVerify messages [2][5]. These are omitted in TLS 1.3 PSK resumption because authentication is performed via the shared secret [4][5]. - Resumption: TLS 1.2 abbreviated handshakes (1-RTT) are superseded by TLS 1.3 PSK resumption, which supports optional 0-RTT for even lower latency [1][6][3]. 0-RTT data, however, carries a risk of replay attacks [1][2][3].
Citations:
- 1: https://systeminternals.dev/tls/handshake/
- 2: https://orankit.com/en/blog/tls-1-2-vs-1-3/
- 3: https://link.springer.com/article/10.1007/s00145-021-09384-1
- 4: https://datatracker.ietf.org/doc/html/rfc8446
- 5: https://www.rfc-editor.org/rfc/rfc9846.txt
- 6: https://blog.cloudflare.com/rfc-8446-aka-tls-1-3/
- 7: https://tlswg.org/tls13-spec/rfc9846.html
Qualify the TLS handshake latency statement.
If the server accepts the offered session, TLS 1.2 resumption reduces the handshake from two round trips to one. A normal TLS 1.3 handshake and TLS 1.3 PSK resumption both use one round trip without early data. Describe TLS 1.3 resumption as omitting certificate transmission, not as reducing every connection from two round trips to one. (datatracker.ietf.org)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 108 - 109, Update the HTTPS connection explanation in
the README to distinguish TLS versions: qualify TLS 1.2 session resumption as
reducing two round trips to one only when the server accepts the session, and
state that normal TLS 1.3 handshakes and TLS 1.3 PSK resumption use one round
trip without early data. Describe TLS 1.3 resumption as omitting certificate
transmission rather than universally reducing handshake latency.
| for i = 1, 260 do | ||
| if not get("h" .. i .. ".test") then return end | ||
| end | ||
|
|
||
| ngx.say("first ", get("h1.test")) | ||
| ngx.say("last ", get("h260.test")) | ||
| } | ||
| } | ||
| --- request | ||
| GET /t | ||
| --- response_body | ||
| first . | ||
| last r |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the capacity and LRU assertion discriminating.
The current assertions can pass with a two-entry FIFO cache. h1 becomes a miss after overflow and h260 remains a hit in that implementation.
Populate exactly 256 entries. Reuse h1 before adding h257. Then require h1 and h257 to resume, and require h2 to miss. This detects both an incorrect capacity and missing LRU promotion.
Proposed test change
- for i = 1, 260 do
+ for i = 1, 256 do
if not get("h" .. i .. ".test") then return end
end
- ngx.say("first ", get("h1.test"))
- ngx.say("last ", get("h260.test"))
+ ngx.say("promoted ", get("h1.test"))
+ ngx.say("new ", get("h257.test"))
+ ngx.say("first ", get("h1.test"))
+ ngx.say("second ", get("h2.test"))- first .
- last r
+ promoted r
+ new .
+ first r
+ second .Based on PR objectives: the cache must be bounded at 256 peers and evict by LRU.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i = 1, 260 do | |
| if not get("h" .. i .. ".test") then return end | |
| end | |
| ngx.say("first ", get("h1.test")) | |
| ngx.say("last ", get("h260.test")) | |
| } | |
| } | |
| --- request | |
| GET /t | |
| --- response_body | |
| first . | |
| last r | |
| for i = 1, 256 do | |
| if not get("h" .. i .. ".test") then return end | |
| end | |
| ngx.say("promoted ", get("h1.test")) | |
| ngx.say("new ", get("h257.test")) | |
| ngx.say("first ", get("h1.test")) | |
| ngx.say("second ", get("h2.test")) | |
| } | |
| } | |
| --- request | |
| GET /t | |
| --- response_body | |
| promoted r | |
| new . | |
| first r | |
| second . |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@t/016-tls-session-resumption.t` around lines 395 - 407, Update the TLS
session resumption test to populate exactly 256 cache entries, access h1 again
to promote it in the LRU order, then add h257. Change the assertions around the
request/response flow so h1 and h257 resume successfully while h2 misses,
thereby validating both the 256-entry capacity and LRU promotion behavior.
Nothing ngx_http_ffi_client_ssl_session_offer() can fail on stops a normal handshake, and the full handshake it falls back to is what the connection would have done anyway, so a setup failure is logged rather than failing the request. The cache test now fills to exactly the cap, uses its oldest peer, and adds one more: the peer that goes has to be the second oldest, which a cache that never promotes on a hit would keep while dropping the one just used.
1 participant
Closes #34.
Every https connection paid a full handshake, even against a peer this worker
handshook with seconds earlier and that was offering to resume. Nothing stored a
session and nothing offered one.
A per-worker cache now holds one session per TLS peer. A fresh connection offers
the session for its key before the handshake, and the entry is refilled when the
peer hands a new session back.
The design questions from the issue
1. Where does the cache live? Its own cache, on
main_conf, rather thanOpenSSL's client-side store.
SSL_SESS_CACHE_CLIENT|SSL_SESS_CACHE_NO_INTERNALplus a
new_session_cbgives the callback without a second copy of everysession, and keeps eviction and expiry in one place. A client never looks a
session up by itself anyway, so the internal store would only have grown.
2. What is the cache key, and is it the pool key? The key is the verify
mode, the peer address, the SNI and the trusted CA. That is the pool key's
tagging with one difference: the pool groups by the caller's pool name, and a
session is keyed by the address actually connected to, so a pool spanning peers
still resumes each one only against itself. Every input the handshake was judged
on is in the key, which is what matters, since a resumed handshake skips the
certificate exchange and answers
SSL_get_verify_result()from the session.3. Interaction with #33. The verify flag is part of the key, so a session
established under verify-off is never offered to a verify-on request. The CA is
folded away with verify off, matching what #33 did to the
SSL_CTXcache:nothing reads it in that state, and keying on it would split the cache per
caller for nothing.
TEST 4covers this, with the same peer and the same SNI onboth sides so the verify flag is the only thing telling the two apart.
4. TLS 1.3 tickets. Sessions are taken from the session callback, not at
handshake completion, so a ticket that arrives after the handshake is picked up.
TEST 1runs against aTLSv1.3-only server andTEST 2against aTLSv1.2-only one, so the two delivery paths are covered separately rather thanby whatever the peer happened to negotiate.
5. Opt-out. On by default,
ssl_session_reuse = falseturns it off on bothentry points, as
proxy_ssl_session_reusedoes. The observable change is that aresumed handshake does not re-send the peer certificate; nothing here reads it
outside the verify path, which reads it from the session.
6. Eviction and memory. 256 peers per worker, least recently used evicted.
An entry past its expiry is dropped on the way out rather than offered.
One thing the issue did not raise: a session is published only once the
handshake has passed the checks the caller asked for. On TLS 1.2 the callback
fires before the certificate is judged, so a session arriving early is held on
the connection and committed by the handshake handler. A peer rejected for a bad
chain or a name mismatch leaves nothing behind for the next request.
Results
Connection per request over TLS, 300 sequential requests each way, resumption
off then on, same build and same peer:
2.8xto3.0xacross three runs. Both ends run in one worker there, so thefigure includes the handshake work the server no longer does; the shape is the
tlsshortone.Tests
t/016-tls-session-resumption.t, seven blocks, reading$ssl_session_reusedoff the upstream:
ssl_session_reuse = falsekeeps every handshake full.request resumes its own.
prove -r tpasses on the defaultllhttpbackend, 401 tests. Thehand-written fallback backend builds and its TLS files pass; its pre-existing
failures in
001,007,010,011and014are the same before and afterthis change.
Summary by CodeRabbit
New Features
ssl_session_reuse = false.Documentation