You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "moonchen (via GitHub)" <gi...@apache.org> on 2026/10/05 22:22:55 UTC
[PR] Fix TLS callback retries after paused hooks [trafficserver]
moonchen opened a new pull request, #13799:
URL: https://github.com/apache/trafficserver/pull/13799
When the TLS library retries a callback after a delayed hook completes, ATS has already advanced to the next hook phase. Repeating the callback's work can replace a plugin-selected certificate or dispatch later hooks with the wrong event.
Make the ClientHello and certificate callbacks recognize completed phases:
- Skip completed hooks and preserve the plugin-selected certificate on retry.
- Still finish CA and session-ticket setup when the certificate callback is retried.
- Preserve existing SNI processing and certificate selection during allowed renegotiation, and early rejection when renegotiation is disabled.
Regression tests cover delayed hooks, correct events, certificate preservation, and restoration of per-SNI client trust. The resume cases pass on OpenSSL and BoringSSL with TLS 1.2 and TLS 1.3. Allowed and disallowed renegotiation were also checked, including changes of SNI.
Fixes #13730.
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen merged PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799#discussion_r4252612082
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -219,6 +219,18 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
TLSSNISupport::ClientHello ch = {client_hello};
#endif
+ TLSEventSupport *es = TLSEventSupport::getInstance(s);
+ // The TLS library calls back again once a paused ClientHello hook reenables, by which time the
+ // ClientHello work and its hooks have run and the hook state has moved on. Dispatching from
+ // that state would hand the next stage's hooks TS_EVENT_SSL_CLIENT_HELLO.
+ if (es != nullptr && es->finished_client_hello_hooks()) {
+ // Renegotiation carries a new ClientHello, whose SNI actions must still run.
+ auto *ssl_vc = SSLNetVCAccess(s);
Review Comment:
Done in 0e1053289f. `TLSBasicSupport` now has `is_tls_handshake_complete()`, backed by a pure virtual that `SSLNetVConnection` implements with `getSSLHandShakeComplete()` and `QUICNetVConnection` with `_handshake_completed`. The ClientHello guard and the `is_renegotiation` check in `ssl_cert_callback` both use it, so neither depends on `SSLNetVConnection` anymore.
I didn't reuse `TLSCertSwitchSupport::_isTryingRenegotiation()`, because on TCP it is true only when renegotiation is disallowed, and this guard must let an allowed renegotiation reach the SNI actions.
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799#discussion_r4252922196
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -284,21 +305,26 @@ ssl_cert_callback(SSL *ssl, [[maybe_unused]] void *arg)
if (tcss) {
if (tes) {
- // Do the common certificate lookup only once. If we pause
- // and restart processing, do not execute the common logic again
- if (!tes->calledHooks(TS_EVENT_SSL_CERT)) {
+ bool const hooks_finished = tes->finished_cert_hooks();
+
+ // On retry, keep a context selected by a completed cert hook. A new handshake
+ // must retain the existing certificate lookup behavior.
+ if ((!hooks_finished || is_renegotiation) && !tes->calledHooks(TS_EVENT_SSL_CERT)) {
Review Comment:
The mechanics are right: in `HANDSHAKE_HOOKS_DONE`, `calledHooks(TS_EVENT_SSL_CERT)` is true, so an allowed renegotiation after a verify-client hook skips `selectCertificate()`. The base revision skips it in the same state, so this PR doesn't change it. OpenSSL 3 and BoringSSL also refuse a client-initiated renegotiation before these callbacks run, because ATS never sets `SSL_OP_ALLOW_CLIENT_RENEGOTIATION`, so only OpenSSL 1.1.1 builds can reach it. I'll leave it for a follow-up.
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799#discussion_r4200695603
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -219,6 +219,18 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
TLSSNISupport::ClientHello ch = {client_hello};
#endif
+ TLSEventSupport *es = TLSEventSupport::getInstance(s);
+ // The TLS library calls back again once a paused ClientHello hook reenables, by which time the
+ // ClientHello work and its hooks have run and the hook state has moved on. Dispatching from
+ // that state would hand the next stage's hooks TS_EVENT_SSL_CLIENT_HELLO.
+ if (es != nullptr && es->finished_client_hello_hooks()) {
+ // Renegotiation carries a new ClientHello, whose SNI actions must still run.
+ auto *ssl_vc = SSLNetVCAccess(s);
Review Comment:
Hmm, I don't want another call for `SSLNetVCAccess`. It does not work for QUIC connection. I'd add an equivalent function to TLSBasicSupport.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -219,6 +219,18 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
TLSSNISupport::ClientHello ch = {client_hello};
#endif
+ TLSEventSupport *es = TLSEventSupport::getInstance(s);
+ // The TLS library calls back again once a paused ClientHello hook reenables, by which time the
+ // ClientHello work and its hooks have run and the hook state has moved on. Dispatching from
+ // that state would hand the next stage's hooks TS_EVENT_SSL_CLIENT_HELLO.
+ if (es != nullptr && es->finished_client_hello_hooks()) {
+ // Renegotiation carries a new ClientHello, whose SNI actions must still run.
+ auto *ssl_vc = SSLNetVCAccess(s);
Review Comment:
Hmm, I don't want another call for `SSLNetVCAccess`. It does not work for QUIC connection. I'd add an equivalent function to `TLSBasicSupport`.
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799#discussion_r4189494515
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -284,21 +305,26 @@ ssl_cert_callback(SSL *ssl, [[maybe_unused]] void *arg)
if (tcss) {
if (tes) {
- // Do the common certificate lookup only once. If we pause
- // and restart processing, do not execute the common logic again
- if (!tes->calledHooks(TS_EVENT_SSL_CERT)) {
+ bool const hooks_finished = tes->finished_cert_hooks();
+
+ // On retry, keep a context selected by a completed cert hook. A new handshake
+ // must retain the existing certificate lookup behavior.
+ if ((!hooks_finished || is_renegotiation) && !tes->calledHooks(TS_EVENT_SSL_CERT)) {
Review Comment:
Allowed renegotiation still skips certificate selection once a previous verify-client hook has advanced the hook state to `HANDSHAKE_HOOKS_DONE`: `calledHooks()` returns true for every event in that state, so the `is_renegotiation` branch is neutralized by the trailing `&&`. A renegotiation with changed SNI can therefore retain the old certificate. Make renegotiation bypass the completed-hook check (and cover the DONE-state/SNI-change case).
--
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] Fix TLS callback retries after paused hooks [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13799:
URL: https://github.com/apache/trafficserver/pull/13799#discussion_r4200695603
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -219,6 +219,18 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
TLSSNISupport::ClientHello ch = {client_hello};
#endif
+ TLSEventSupport *es = TLSEventSupport::getInstance(s);
+ // The TLS library calls back again once a paused ClientHello hook reenables, by which time the
+ // ClientHello work and its hooks have run and the hook state has moved on. Dispatching from
+ // that state would hand the next stage's hooks TS_EVENT_SSL_CLIENT_HELLO.
+ if (es != nullptr && es->finished_client_hello_hooks()) {
+ // Renegotiation carries a new ClientHello, whose SNI actions must still run.
+ auto *ssl_vc = SSLNetVCAccess(s);
Review Comment:
Hmm, I don't want another call for `SSLNetVCAccess`. It does not work for QUIC connection. I'd add `getSSLHandShakeComplete` equivalent function to `TLSBasicSupport`.
--
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