maskit commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3975973866
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -205,6 +253,120 @@ 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)");
+ 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;
+ }
+ // QUIC binds TLSBasicSupport without ever calling SSLNetVCAttach(), so this
is null on an H3
+ // connection even though tbs is not. Only the per-connection CA override
below needs it.
+ SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
+ TLSSNISupport *snis = TLSSNISupport::getInstance(ssl);
+ const char *servername = snis != nullptr ?
snis->get_sni_server_name() : "";
+
+ 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",
servername);
+ }
+ // 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) {
+ // Defensive only: BoringSSL gates ssl_verify_peer_cert() on
ssl_session_has_peer_cred(), so a
+ // peer that sent no certificate never reaches this callback, and
SSL_VERIFY_FAIL_IF_NO_PEER_CERT
+ // is enforced before that point. Optional client certs are unaffected.
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
+ X509 *leaf = sk_X509_value(chain, 0);
+
+ // A per-SNI verify_client action may have pinned a CA file/dir onto this
connection via
+ // setClientCertCACerts(), which BoringSSL exposes no getter for. Use the
store it stashed rather
+ // than opening and parsing the same files again on the event thread, once
per connection.
+ const char *ca_cert_file = netvc != nullptr ? netvc->get_ca_cert_file() :
nullptr;
+ const char *ca_cert_dir = netvc != nullptr ? netvc->get_ca_cert_dir() :
nullptr;
+ bool const ca_configured =
+ (ca_cert_file != nullptr && ca_cert_file[0] != '\0') || (ca_cert_dir !=
nullptr && ca_cert_dir[0] != '\0');
+ X509_STORE *verify_store =
+ ssl_verify_store_index >= 0 ? static_cast<X509_STORE
*>(SSL_get_ex_data(ssl, ssl_verify_store_index)) : nullptr;
+ if (verify_store == nullptr) {
+ if (ca_configured) {
+ // Configured but never materialized, so falling back to the SSL_CTX
store here would widen
+ // trust past what the SNI action asked for.
+ SSLError("no per-connection client CA store for %s despite one being
configured", servername);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ verify_store = SSL_CTX_get_cert_store(ctx);
+ }
+
+ X509_STORE_CTX *store_ctx = X509_STORE_CTX_new();
+ bool initialized = store_ctx != nullptr &&
X509_STORE_CTX_init(store_ctx, verify_store, leaf, chain) &&
+ // Sets param->purpose and param->trust, which gate
X509_check_purpose(). The
+ // library's own path does this; without it a
serverAuth-only leaf from the
+ // trusted client CA authenticates as a client.
+ X509_STORE_CTX_set_default(store_ctx, "ssl_client") &&
+ // Carries the connection's verify params (depth
included) over the store's,
+ // as the library path does. Anything set per-connection
wins; purpose and
+ // trust from set_default() above survive because ATS
never sets them here.
+
X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(store_ctx),
SSL_get0_param(ssl)) &&
+ X509_STORE_CTX_set_ex_data(store_ctx,
SSL_get_ex_data_X509_STORE_CTX_idx(), ssl);
Review Comment:
`SSL_get_peer_full_cert_chain()` is the variant that includes the leaf --
that is what distinguishes it from `SSL_get_peer_cert_chain()`, which omits the
leaf on a server:
> This is the same as `SSL_get_peer_cert_chain` except that this function
always returns the full chain, i.e. the first element of the return value (if
any) will be the leaf certificate. In contrast, `SSL_get_peer_cert_chain`
returns only the intermediate certificates if the `ssl` is a server.
So element 0 is the leaf, and passing the same stack as the untrusted list
is what BoringSSL itself does in the code path this replaces
(`ssl/ssl_x509.cc`):
```c
X509 *leaf = sk_X509_value(cert_chain, 0);
...
!X509_STORE_CTX_init(ctx.get(), verify_store, leaf, cert_chain) ||
```
Splitting the stack would diverge from the library path this stands in for,
so leaving as is.
--
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]
