bryancall commented on PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5587742790
Thanks for the detailed follow-up. The OpenSSL half of the blocking item is
properly fixed and the new scenario 7 is exactly the multi-entry test I asked
for: two entries, the raw public key configuration on the provably non-default
one, and an assertion that only passes if the matched entry's client
certificate type reached the connection. I checked the ordering premise against
OpenSSL master rather than taking the comment's word for it, and it holds:
`client_hello_cb` runs at `statem_srvr.c:2076`, extension parsing at `:2314`,
`cert_cb` at `:2693`, and `tls_parse_ctos_server_cert_type` reads the
connection level list. Items 2 through 6 all check out, and the
`SSL_get_peer_full_cert_chain` rewrite is correct: BoringSSL calls
`session_cache_objects` unconditionally at `handshake_server.cc:1189` and the
noop x509 method is only installed by `TLS_with_buffers_method()`, which ATS
does not use.
Two things on the BoringSSL side, and they share one root cause.
**The new block at `SSLUtils.cc:522` discards a per-SNI `verify_client`
override.** `mode` is read from `SSL_CTX_get_verify_mode(matched_ctx)` and then
pushed onto the connection at `:529` and `:534`. Every multicert context takes
its verify mode from the single global
`proxy.config.ssl.client.certification_level` in
`_setup_client_cert_verification`, so the only value this can ever overwrite is
one a per-SNI action set. `VerifyClient::SNIAction` runs inside
`perform_sni_action`, which the `select_certificate_cb` lambda at `:1294` calls
before `ssl_cert_callback`, so the overwrite always wins. With
`certification_level: 1` and an `sni.yaml` entry set to `verify_client:
STRICT`, `setClientCertLevel` installs `SSL_VERIFY_PEER |
SSL_VERIFY_FAIL_IF_NO_PEER_CERT` and this block resets it to the context's
`SSL_VERIFY_PEER`, so a client sending no certificate completes the handshake
against a hostname marked STRICT. The reverse configuration, global level 2
with `verify_client: NONE` for
one hostname, re-imposes the requirement. The `else` branch at `:531` has no
raw public key condition, so this runs on every inbound connection on a
BoringSSL build with a non-zero certification level.
**The same read leaves item 1 open when the raw public key entry is not the
default entry.** `setClientCertLevel` at `:1354` inspects
`SSL_get_SSL_CTX(ssl)`, which on BoringSSL is still the default `"*"` context,
since `ssl_cert_callback` has not switched it yet. So with a global
certification level of 0, a per-SNI `verify_client: STRICT`, and
`ssl_client_rpk_ca_name` on a non-default entry, `setClientCertLevel` installs
the classic callback, and then this block skips `SSL_set_custom_verify` because
`SSL_CTX_get_verify_mode(matched_ctx)` is `SSL_VERIFY_NONE` at level 0. The
server advertises raw public key acceptance and rejects the raw public key the
client sends.
Reading `SSL_get_verify_mode(ssl)` instead of
`SSL_CTX_get_verify_mode(matched_ctx)` fixes both: it already carries the
`SSL_new` inheritance and any per-SNI override.
Non-blocking, in rough order:
- `SSLUtils.cc:531-535` does not do what its comment says. `SSL_set_verify`
writes `verify_mode` and `verify_callback` only; it never clears
`custom_verify_callback`, which `SSL_new` copied from the default context
(`ssl_lib.cc:512`), and `ssl_verify_peer_cert` prefers the custom callback
whenever it is non-null. A `SSL_set_custom_verify(ssl, mode, nullptr)` first
would. The same branch also never resets the inherited
`accepted_peer_cert_types`.
- The cert type re-application at `:401` and `:407` is one directional.
Guarded on `cert_type != nullptr`, so when the matched entry has no list the
connection keeps the default entry's. With `ssl_rpk_enabled` on `"*"`, a
connection matching a plain entry still negotiates a raw public key server
identity. Setting `{TLSEXT_cert_type_x509}` explicitly in that case would make
it symmetric.
- The `selectCertificate()` call at `:386` is inside
`HAVE_SSL_CTX_SET_CLIENT_HELLO_CB`, not `HAVE_SSL_CTX_SET1_SERVER_CERT_TYPE`,
so every OpenSSL build moves certificate selection to the ClientHello callback
whether or not it has raw public key support. Worth tightening the guard so
builds that gain nothing do not take the change.
- `SSLRPKUtils.cc:106-111`: the second `i2d_PUBKEY()` return is unchecked. A
short write leaves the vector zero padded, and since the comparison is an exact
`memcmp`, that pin can never match, presenting as a mismatch against the peer
rather than a bad pin file.
- Scenario 8's remaining OpenSSL assertions do not constrain much.
`Callback: verify client cert` is the unconditional first statement of
`ssl_verify_client_callback`, and `client certificate chain verification
failed` only exists inside the BoringSSL block, so the exclusion is
unfalsifiable on OpenSSL. `ExcludesExpression('Client authenticated with a raw
public key')` would at least prove the connection took the X.509 path.
- The comment at `:513` says `select_certificate_cb` runs before any
ClientHello extension is evaluated. `extract_sni()` runs first
(`handshake_server.cc:499` against `:518`), which is why `SSL_get_servername()`
works on that path. The comment at `:380` names `tls_process_client_hello()`;
the negotiation is actually in `tls_early_post_process_client_hello()`.
- On the CI point: agreed it is infrastructure, not this pull request. I
would still put one line in the description saying the BoringSSL X.509 fallback
ships without automated coverage, since that is where the next person looks,
and the "verified on both builds" checkbox predates the rewrite in 3a6e95cb18.
--
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]
