Conversation
On BoringSSL builds, a plugin that parks the handshake in the client hello or cert hook is not waited for. The select certificate callback dropped the retry from the client hello step and mapped a cert hook pause to success, so the handshake carried on without the plugin. Return a retry for both. BoringSSL then calls the callback again from the top, so after a cert hook pause the client hello and servername steps are skipped rather than run twice. OpenSSL builds use a separate callback and are unaffected. The callback has behaved this way since BoringSSL support for it was added in apache#8014. The rate_limit_sni_expiry and tls_hooks_close_while_parked tests from apache#13406 rely on the pause and fail on BoringSSL builds without this change.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical certificate-hook resume issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Updates BoringSSL TLS certificate callbacks to correctly pause and resume handshake hooks.
Changes:
- Propagates paused ClientHello and certificate hooks as retries.
- Tracks certificate-hook progress to avoid repeating completed stages.
- Adds a certificate-phase state helper.
| File | Description |
|---|---|
src/iocore/net/TLSEventSupport.cc |
Adds certificate-phase state detection. |
src/iocore/net/SSLUtils.cc |
Updates BoringSSL callback retry handling. |
include/iocore/net/TLSEventSupport.h |
Declares the new state helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (es == nullptr || !es->reached_cert_hooks()) { | ||
| res = ssl_client_hello_callback(client_hello); | ||
| if (res != ssl_select_cert_success) { | ||
| return res; | ||
| } |
There was a problem hiding this comment.
The resumed call ending up in HANDSHAKE_HOOKS_CLIENT_CERT is not specific to this change. OpenSSL re-invokes cert_cb after a -1 pause as well, so OpenSSL builds already re-run selectCertificate() and start the verify-client hook list with TS_EVENT_SSL_CERT in the same situation. This PR brings BoringSSL onto that shared path; fixing ssl_cert_callback() for both backends is tracked in #13730.
| case SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_CERT: | ||
| case SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_CERT_INVOKE: |
There was a problem hiding this comment.
The resumed call ending up in HANDSHAKE_HOOKS_CLIENT_CERT is not specific to this change. OpenSSL re-invokes cert_cb after a -1 pause as well, so OpenSSL builds already re-run selectCertificate() and start the verify-client hook list with TS_EVENT_SSL_CERT in the same situation. This PR brings BoringSSL onto that shared path; fixing ssl_cert_callback() for both backends is tracked in #13730.
|
[approve ci rocky] |
On BoringSSL builds, a plugin that parks the handshake in the client hello or cert hook is not waited for. The select certificate callback dropped the retry from the client hello step and mapped a cert hook pause to success, so the handshake carried on without the plugin.
The callback has behaved this way since it was added in #8014. The
rate_limit_sni_expiryandtls_hooks_close_while_parkedtests from #13406 rely on the pause and fail on BoringSSL builds without this change.Changes
TLSEventSupport::reached_cert_hooks()tells the callback which case it is in.OpenSSL builds use a separate callback and are unaffected.