maskit commented on PR #13548: URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5402927831
Follow-up on this review — addressed everything, in 5 commits since: **Blocking (inbound RPK settings inert on non-default multicert entries):** fixed on both libraries. `SSL_set_SSL_CTX()` never refreshed `server_cert_type`/`client_cert_type`, so a non-default entry's RPK settings negotiated using the default entry's config instead of its own. BoringSSL's cert-selection callback runs early enough that reapplying in place was sufficient (`f9b7bb2e75`); OpenSSL locks in this negotiation inside `tls_process_client_hello()` before its cert-selection callback ever runs, so that needed moving context selection earlier, into `SSL_client_hello_cb` (`8d18fd284e`). **Also worth addressing:** - `setClientCertLevel()` unconditionally installed the classic verify callback, silently overriding an RPK-configured entry's need for `custom_verify` whenever a per-SNI `verify_client` action fired. Fixed in `fb0a43a466`. - Outbound pin file was re-parsed from disk on every handshake. Now parsed once at config load and shared via the SNI config generation (`cb71b0f10d`). **Smaller items:** non-canonical pin encoding, the doc/code mismatch on pin-failure semantics, and the unchecked `SSL_CTX_set_ex_data()` return — all fixed in `fb0a43a466`. The RPK-disabled-build config-load failure is fixed in `cb71b0f10d`. `SSL_SESSION_get0_peer_rpk()` — confirmed it exists in BoringSSL too, so the shared macro guard is fine as-is. **BoringSSL X.509 fallback:** simplified `ssl_custom_verify_client_callback()`'s fallback to use `SSL_get_peer_full_cert_chain()` instead of hand-decoding `CRYPTO_BUFFER`s — BoringSSL already parses the chain into `X509` objects unconditionally when it processes the Certificate message, so there was no need to redo that (`3a6e95cb18`). Also added the autest scenario this path was missing (X.509 client cert against an RPK-enabled, mTLS-required entry). One thing still open, called out here rather than in the description since it's about CI infra, not this PR's code: no CI job in this repo builds against BoringSSL at all, so none of this — the new autest coverage included — runs automatically anywhere. Worth tracking separately. -- 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]
