maskit commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3867889155
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -205,6 +246,113 @@ ssl_verify_client_callback(int preverify_ok,
X509_STORE_CTX *ctx)
return preverify_ok;
}
+#if HAVE_SSL_CREDENTIAL_NEW_RAW_PUBLIC_KEY
+// BoringSSL's SSL_CTX_set_custom_verify(), required to accept RPK client
certs, replaces its
+// automatic X.509 chain verification entirely -- unlike OpenSSL's classic
SSL_CTX_set_verify(),
+// which only lets ssl_verify_client_callback() observe/override a chain
BoringSSL already
+// validated. For the X.509 fallback case (the client didn't offer an RPK this
time), this
+// callback must therefore redo that validation manually via the legacy
X509_STORE_CTX API,
+// against the same certificate store _setup_client_cert_verification()
configured on this ctx.
+static enum ssl_verify_result_t
+ssl_custom_verify_client_callback(SSL *ssl, uint8_t *out_alert)
+{
+ Dbg(dbg_ctl_ssl_verify, "Callback: custom verify client cert (RPK-enabled
ctx)");
+ SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
+ TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
+ if (tbs == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "ssl_custom_verify_client_callback call back on
stale netvc");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+
+ SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
+
+ if (SSL_get_peer_cert_type(ssl) == TLSEXT_cert_type_rpk) {
+ EVP_PKEY *peer_rpk = SSL_get0_peer_rpk(ssl);
+ auto *trusted = static_cast<SSLRPKUtils::TrustedKeySet
*>(SSL_CTX_get_ex_data(ctx, ssl_client_rpk_ca_index));
+ bool pin_ok = trusted != nullptr &&
SSLRPKUtils::pinnedKeyMatches(peer_rpk, *trusted);
+ if (!pin_ok) {
+ Warning("client raw public key did not match any trusted key for %s",
netvc->options.sni_servername.get());
+ }
+ // As above: the hook always runs, and can add rejection but not override
a failed pin match.
+ if (tbs->verify_certificate(nullptr) == 1 || !pin_ok) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ // X.509 fallback: BoringSSL parses the peer's certificate chain into X509
objects as soon as it
+ // receives the Certificate message, regardless of whether a custom verify
callback is
+ // installed -- SSL_get_peer_full_cert_chain() exposes that already-parsed
chain (leaf included),
+ // so there's no need to redo the CRYPTO_BUFFER-to-X509 decoding
SSL_get0_peer_certificates()
+ // would otherwise require here.
+ STACK_OF(X509) *chain = SSL_get_peer_full_cert_chain(ssl);
+ if (chain == nullptr || sk_X509_num(chain) == 0) {
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
Review Comment:
False positive, confirmed by tracing BoringSSL's own gating logic.
`do_verify_client_certificate()`/`do_read_client_certificate_verify()` only
invoke *any* verify callback (custom or classic) when
`ssl_session_has_peer_cred()` is true:
```cpp
bool ssl_session_has_peer_cred(const SSL_SESSION *session) {
return sk_CRYPTO_BUFFER_num(SSL_SESSION_get0_peer_certificates(session)) >
0 ||
SSL_SESSION_get0_peer_rpk(session) != nullptr ||
session->peer_sha256_valid;
}
```
For `certification_level=1` with no client cert sent, none of the three
hold, and BoringSSL skips the verify step entirely
(`handshake_server.cc:1279-1296`), setting `verify_result = X509_V_OK` itself
before our callback would ever run.
The third disjunct (`peer_sha256_valid`) only gets set when
`SSL_CTX_set_retain_only_sha256_of_client_certs()` is enabled -- ATS never
calls that API, so it's permanently false here. That leaves only "certs > 0" or
"RPK present" as ways into this callback at all, so whenever we're actually
running, either the X.509 chain is non-empty or `SSL_get_peer_cert_type(ssl) ==
TLSEXT_cert_type_rpk` and we take the RPK branch instead -- there's no path
that reaches the X.509 fallback's empty-chain check with an actually-empty
chain.
Also worth noting this specific check predates the recent fallback
simplification -- it's unchanged from the original hand-rolled
`SSL_get0_peer_certificates()` version, just carried forward with the same
shape.
--
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]
