maskit commented on PR #13548: URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5765401478
Addressed at `180bdfe5`. **The pooling fix was broken and you found why.** `get_sni_server_name()` returns `""`, never `nullptr`, so the `session_sni == nullptr` test was dead and an absent name on either side refused reuse outright. I had told you that helper treats empty as matching empty; it did the opposite. Worse, I had verified that function returns `""` earlier in this cycle for an unrelated reason and never connected it to the claim I was making. Fixed by comparing the two names directly, which is what the dead branch was written to do. On your question about the shared helper: I changed `validate_sni()` rather than special-casing the raw public key caller. Its stated job is whether a new connection would use the same name, and two connections that send none do match, so the old behaviour was over-strict for the mask-gated caller too. `h2origin` sets `server_session_sharing.match: ip,sni,cert` and uses H2 origin sessions, so it covers both that caller and this one; it passes. I extracted the comparison as `ServerSessionPool::sni_matches()` to get it under test, since `validate_sni()` needs an `HttpSM` and nothing in the reuse path is reachable from a unit test otherwise. Restoring the old logic makes the both-empty case fail and only that case, which is the check I should have run the first time. **Session resumption is fixed**, in `4febe1ac`, pushed after you reviewed `0fcb674b13`. The pinned set now identifies the connection in the origin session cache key. Two details: the identity is digested from the key material rather than taken from the parsed object's address, because an address rotates on every reload and would make any `config reload` dump every cached origin session, whereas a content digest retires exactly the sessions whose pin set changed; and it is added inside `get_verify_str()` rather than at the two sites that build the key, so the insert and the lookup cannot drift apart. Taken once at config load, so no per-handshake hashing. **The two pool comments are documented rather than fixed,** as you asked. `sni.yaml.en.rst` now states that replacing `server_rpk_ca` does not reach already-pooled connections, that this matches how a replaced certificate authority behaves for X.509, and that cached sessions are no longer affected since they are keyed on the trusted set. I agree it wants a verification-scope generation on the pooled session covering CA and policy too, and that is its own change rather than a raw-public-key special case. **The non-blocking note is fixed too** -- the `SSL_set_ex_data()` failure path now clears the slot, releasing the previous store only if the slot really was cleared. One thing worth flagging for sequencing: the inbound pin set has the same resumption staleness, and I have deliberately not put a term for it in the per-`SSL_CTX` `set_session_id_context()`. ATSUnderground#629 sets the context per connection and unconditionally, so a term added there would pass a test now and silently stop having any effect once that lands. It belongs in `bind_session_id_context_to_verify_scope()`, and `tls_resume_ca_bundle_reload.test.py` from that PR is the pattern for its test, since a pin file replaced in place is the same scenario as a CA bundle replaced in place. Happy to do that as a follow-up here once #629 is upstream, or to hold this PR for it -- your call. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
