maskit commented on PR #13548: URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5612011128
All items closed. Nine commits on top of `f9580f6e88`. ## Blocking **1 & 2 — verify mode read from the ctx.** `c1c6ae7b4f` Reads `SSL_get_verify_mode(ssl)` now. `setClientCertLevel()`'s RPK branch is gone rather than repaired: it inspected `SSL_get_SSL_CTX(ssl)`, still the default `"*"` context at that point, so it could only read the wrong entry's ex_data. `ssl_cert_callback` runs afterwards and is now the single decision point. The `else` branch now clears the inherited `custom_verify_callback` explicitly and resets `accepted_peer_cert_types` — `SSL_set_verify()` writes `verify_mode` and `verify_callback` only (`ssl_x509.cc:518-529`), as you said. **3 — null dereference on QUIC.** `ae96669b45` Server name comes from `TLSSNISupport`, which QUIC binds; the connection is optional since only the CA override needs it. One correction: `SSLClientUtils.cc` already guards `netvc` at the top of its callback, so both unguarded uses were inbound. **4 — lost purpose enforcement.** `cd9f5a9e6c`, `efa18816b5` `X509_STORE_CTX_set_default()` and `set_ex_data()` in both directions, checked. Both also now carry the connection's `X509_VERIFY_PARAM` over the store's, as the library path does — `SSL_set_verify_depth()` writes into that param, so taking depth from `SSL_CTX_get_verify_depth()` had the same context-versus-connection problem as item 1. Not replicated: the ECH name override (no ECH on this path) and installing `verify_callback` as the store's per-certificate callback, since these paths invoke the `TS_EVENT_SSL_VERIFY_*` hook themselves and wiring it would double-invoke. **5 — the raw public key skip defeats the pool-reuse guard.** `053753f0c3` Reuse is permitted only for the name the connection was established for. Uses `SSL_get0_peer_rpk()`, and the comment describes pool reuse rather than resumption. ## Also worth addressing | item | commit | | --- | --- | | `SSL_set_verify()` not clearing `custom_verify_callback`; mutual-exclusivity comment | `c1c6ae7b4f`, `c9b67135b5` | | `ssl_client_rpk_ca_index < 0` unchecked | `c9b67135b5` | | `X509_STORE` rebuilt inside the handshake | `9b9b95fdd1` | | `X509_STORE_CTX_set_ex_data()` missing in both fallbacks | `cd9f5a9e6c` | | second `i2d_PUBKEY()` unchecked | `c9b67135b5` | | PEM EOF check missing `ERR_LIB_PEM` | `c9b67135b5` | | cert-type re-application one directional | `c9b67135b5` | | `selectCertificate()` guard too broad | `c9b67135b5` | | `certification_level: 0` inertness | deviation, below | Taking your second option on the store: `setClientCertCACerts()` already builds it, so it stashes a non-owning pointer in ex_data and the callback consumes it. The open and parse are gone from the handshake path. The stash is written only in the branch that calls `SSL_set0_verify_cert_store()`, which matters because that runs twice on a connection with a `verify_client` action and `set0` frees the store it replaces. Configured-but-absent is still distinguished from not-configured, so a failed load fails the handshake. Narrowing the guard to `HAVE_SSL_CTX_SET1_SERVER_CERT_TYPE` also satisfies your "checked and not reporting" point: BoringSSL does not define it, so the OpenSSL-only symbols stay excluded. Verified by building all three backends. ## Tests Four scenarios added, three negative, each mutation-checked: | mutation | untrusted client chain | serverAuth-only client cert | unverifiable origin chain | | --- | --- | --- | --- | | none | pass | pass | pass | | inbound `verified = true` | **fail** | not reached | — | | inbound `set_default()` removed | pass | **fail** | — | | outbound `signature_ok = true` | — | — | **fail** | `verified = true` no longer passes the suite, and each test fails for its own reason rather than by side effect. The serverAuth-only case uses `server.ocsp.pem`, which chains cleanly to `ca.ocsp.pem` and fails on purpose alone. The fourth scenario is the ENFORCED positive — a chain that verifies, previously only exercised under PERMISSIVE. Scenario 2's chain check has been failing all along with the test passing regardless; its comment now says so and points at the new scenarios for the verdict. Scenario 8 has your suggested exclusion on the raw public key log line, so it proves the X.509 path was taken. `test_SSLRPKUtils.cc` has an `#else` case reporting that the tests were not built. Compiled with the guard forced off, since no build here disables the feature. ## Comment accuracy All three corrected in `c9b67135b5`. The unreachable empty-chain branch is annotated in `9b9b95fdd1`. ## One deviation Not failing the config load for `certification_level: 0`. With items 1 and 2 fixed, level 0 plus a per-SNI `verify_client: STRICT` is a working configuration — `setClientCertLevel()` sets the mode per connection and `ssl_cert_callback` installs custom_verify — so failing the load would reject it. The still-inert case is level 0 with no `verify_client` action anywhere, which `sni.yaml` being a separate config makes undetectable at multicert load time, so `338dd0af15` documents the dependency. A load-time warning is possible if you prefer, but it would fire on correct per-SNI deployments. ## The description The fallback now has four scenarios covering it, two mutation-checked negatives, so "ships without automated coverage" no longer holds. What does: no CI job compiles it, so none of that coverage runs automatically. I'll state that unless you'd word it differently. Newly untested rather than newly broken: OpenSSL builds with RFC 7250 support now select the certificate in the ClientHello callback, which includes QUIC listeners since `QUICMultiCertConfigLoader` inherits `_set_handshake_callbacks()`. `TLSCertSwitchSupport` and `TLSSNISupport` are both bound on QUIC so there is no null path, but nothing exercises H3. -- 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]
