Copilot commented on code in PR #13729:
URL: https://github.com/apache/trafficserver/pull/13729#discussion_r4095451623
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1063,24 +1063,29 @@
SSLMultiCertConfigLoader::_set_handshake_callbacks(SSL_CTX *ctx)
#elif HAVE_SSL_CTX_SET_SELECT_CERTIFICATE_CB
SSL_CTX_set_select_certificate_cb(ctx, [](const SSL_CLIENT_HELLO
*client_hello) -> ssl_select_cert_result_t {
ssl_select_cert_result_t res;
- res = ssl_client_hello_callback(client_hello);
- if (res == ssl_select_cert_error) {
- return res;
- }
+ TLSEventSupport *es =
TLSEventSupport::getInstance(client_hello->ssl);
+
+ // BoringSSL runs this whole callback again once a paused hook reenables,
whereas OpenSSL calls
+ // only the callback that paused. After a pause in the cert hook, the
client hello and
+ // servername work is already done and must not run twice.
+ if (es == nullptr || !es->reached_cert_hooks()) {
+ res = ssl_client_hello_callback(client_hello);
+ if (res != ssl_select_cert_success) {
+ return res;
+ }
Review Comment:
When a cert hook pauses, `SSLNetVConnection::reenable()` advances the hook
state past the final cert hook to `HANDSHAKE_HOOKS_CLIENT_CERT` before
BoringSSL re-enters this callback (`TLSEventSupport.cc:519-522`).
`reached_cert_hooks()` then makes this branch skip ClientHello/SNI, but
`ssl_cert_callback()` sees `calledHooks(TS_EVENT_SSL_CERT)` as false in
`HANDSHAKE_HOOKS_CLIENT_CERT`, so it selects the certificate a second time and
`callHooks(TS_EVENT_SSL_CERT)` can start the verify-client hook list with the
wrong event. Preserve an explicit cert-processing/retry state (or otherwise
make `ssl_cert_callback()` recognize this resumed state) so the retry does not
re-run certificate selection or enter client-cert hooks as `TS_EVENT_SSL_CERT`.
This issue also appears on line 1071 of the same file.
##########
src/iocore/net/TLSEventSupport.cc:
##########
@@ -375,6 +375,20 @@ TLSEventSupport::calledHooks(TSEvent eventId) const
return retval;
}
+bool
+TLSEventSupport::reached_cert_hooks() const
+{
+ switch (this->sslHandshakeHookState) {
+ case SSLHandshakeHookState::HANDSHAKE_HOOKS_CERT:
+ case SSLHandshakeHookState::HANDSHAKE_HOOKS_CERT_INVOKE:
+ case SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_CERT:
+ case SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_CERT_INVOKE:
Review Comment:
`HANDSHAKE_HOOKS_CLIENT_CERT` is reached after the last `TS_EVENT_SSL_CERT`
hook has already completed. On the retry callback, this predicate skips
client-hello/servername but still calls `ssl_cert_callback`;
`ssl_cert_callback`'s `calledHooks(TS_EVENT_SSL_CERT)` check is false in
`CLIENT_CERT`, so it reselects the certificate and enters the client-cert hook
list with `TS_EVENT_SSL_CERT` instead of finishing the handshake normally. The
retry path needs to distinguish an unfinished `CERT`/`CERT_INVOKE` phase from
the already-completed `CLIENT_CERT` phase.
--
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]
