Copilot commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r4064738214
##########
src/iocore/net/SSLClientUtils.cc:
##########
@@ -155,6 +252,199 @@ verify_callback(int signature_ok, X509_STORE_CTX *ctx)
return true;
}
+#if HAVE_SSL_CREDENTIAL_NEW_RAW_PUBLIC_KEY
+// BoringSSL rejects raw public keys outright unless a custom verify callback
is installed, and
+// SSL_set_custom_verify() displaces SSL_set_verify() (and with it BoringSSL's
automatic chain
+// verification) for the whole connection. So this callback owns both cases:
pin the peer's raw
+// public key, or -- when the next hop negotiated X.509 after all, the normal
state mid-rollout --
+// rebuild and verify the chain by hand before deferring to the usual
policy/name/hook logic.
+static enum ssl_verify_result_t
+ssl_client_custom_verify_callback(SSL *ssl, uint8_t *out_alert)
+{
+ SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
+ if (netvc == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "WARNING, NetVC is NULL in custom cert verify
callback");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ if (netvc->options.verifyServerPolicy == YamlSNIConfig::Policy::DISABLED) {
+ return ssl_verify_ok;
+ }
+
+ bool const enforce_mode = netvc->options.verifyServerPolicy ==
YamlSNIConfig::Policy::ENFORCED;
+
+ TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
+ if (tbs == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "custom verify callback on stale netvc");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+
+ if (SSL_get_peer_cert_type(ssl) == TLSEXT_cert_type_rpk) {
+ EVP_PKEY *peer_rpk = SSL_get0_peer_rpk(ssl);
+ const SSLRPKUtils::TrustedKeySet *trusted = ssl_get_trusted_rpk(ssl);
+ bool pin_ok = trusted != nullptr &&
SSLRPKUtils::pinnedKeyMatches(peer_rpk, *trusted);
+ Dbg(dbg_ctl_ssl_verify, "Origin authenticated with a raw public key (RFC
7250), pin match=%s", pin_ok ? "yes" : "no");
+ if (!pin_ok) {
+ char buff[INET6_ADDRSTRLEN];
+ ats_ip_ntop(netvc->get_effective_remote_addr(), buff, INET6_ADDRSTRLEN);
+ Warning("Origin raw public key did not match any trusted key. Action=%s
server=%s(%s)",
+ enforce_mode ? "Terminate" : "Continue",
netvc->options.ssl_servername.get(), buff);
+ }
+
+ // There is no X509_STORE_CTX to hand the hook for a raw public key, but
the hook still runs on
+ // every raw public key attempt.
+ if (tbs->verify_certificate(nullptr) == 1) {
+ Warning("TS_EVENT_SSL_VERIFY_SERVER plugin failed the origin raw public
key check for %s. Action=%s",
+ netvc->options.ssl_servername.get(), enforce_mode ? "Terminate"
: "Continue");
+ if (enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+ if (!pin_ok && enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ // X.509 fallback. Rebuild the chain BoringSSL hands back as CRYPTO_BUFFERs
so the shared
+ // verify_callback() logic (signature/name/policy/hook) can run against a
real X509_STORE_CTX.
+ const STACK_OF(CRYPTO_BUFFER) *chain = SSL_get0_peer_certificates(ssl);
+ if (chain == nullptr || sk_CRYPTO_BUFFER_num(chain) == 0) {
+ if (enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ X509 *leaf = nullptr;
+ STACK_OF(X509) *intermediates = sk_X509_new_null();
+ if (intermediates == nullptr) {
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(chain); i++) {
+ const CRYPTO_BUFFER *buf = sk_CRYPTO_BUFFER_value(chain, i);
+ const uint8_t *data = CRYPTO_BUFFER_data(buf);
+ X509 *cert = d2i_X509(nullptr, &data,
CRYPTO_BUFFER_len(buf));
+ if (cert == nullptr) {
+ SSLError("failed to parse an origin certificate on a RPK-enabled
connection");
+ X509_free(leaf);
+ sk_X509_pop_free(intermediates, X509_free);
+ *out_alert = SSL_AD_BAD_CERTIFICATE;
+ return ssl_verify_invalid;
+ }
+ if (i == 0) {
+ leaf = cert;
+ } else if (!sk_X509_push(intermediates, cert)) {
+ SSLError("failed to append an intermediate certificate on a RPK-enabled
connection");
+ X509_free(cert);
+ X509_free(leaf);
+ sk_X509_pop_free(intermediates, X509_free);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ }
+
+ X509_STORE_CTX *store_ctx = X509_STORE_CTX_new();
+ bool const initialized = store_ctx != nullptr &&
+ X509_STORE_CTX_init(store_ctx,
SSL_CTX_get_cert_store(SSL_get_SSL_CTX(ssl)), leaf, intermediates) &&
+ // Sets param->purpose and param->trust, which gate
X509_check_purpose().
+ // The library's own path does this; without it a
clientAuth-only leaf
+ // from a trusted CA authenticates the next hop.
+ X509_STORE_CTX_set_default(store_ctx, "ssl_server")
&&
+ // Carries the connection's verify params (depth
included) over the
+ // store's, as the library path does.
+
X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(store_ctx),
SSL_get0_param(ssl)) &&
Review Comment:
The same ordering issue occurs for outbound fallback verification:
`X509_VERIFY_PARAM_set1` runs after `set_default(\"ssl_server\")` and can
overwrite the server-purpose parameters. That removes the EKU/purpose check
this hand-rolled path is intended to reproduce, allowing a clientAuth-only
certificate to authenticate as the origin. Copy the connection parameters
first, then apply the `ssl_server` default while retaining the required
per-connection settings.
##########
src/iocore/net/SSLRPKUtils.cc:
##########
@@ -0,0 +1,210 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "SSLRPKUtils.h"
+
+#include "P_SSLUtils.h"
+#include "iocore/net/SSLDiags.h"
+
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/x509.h>
+
+#include <cstring>
+
+namespace SSLRPKUtils
+{
+bool
+loadTrustedKeys(const char *path, TrustedKeySet &out)
+{
+ scoped_BIO bio(BIO_new_file(path, "r"));
+ if (!bio) {
+ SSLError("SSLRPKUtils: failed to open trusted RPK key file %s", path);
+ return false;
+ }
+
+ // Discard anything already on this thread's error queue so the
PEM_R_NO_START_LINE check
+ // below on the first iteration can't be confused by an unrelated error left
over from earlier,
+ // unrelated OpenSSL/BoringSSL calls on this thread.
+ ERR_clear_error();
+
+ for (;;) {
+ // Read the PEM envelope first and decode it separately. Asking
PEM_read_bio_PUBKEY() to do
+ // both makes end-of-file indistinguishable from a decode failure: on
OpenSSL 3 it runs the
+ // decoder framework, so the error left on the queue at EOF is the
decoder's "unsupported"
+ // rather than PEM_R_NO_START_LINE, and a "no more keys" stop looks
exactly like a malformed
+ // key. PEM_read_bio() reports EOF unambiguously via PEM_R_NO_START_LINE.
+ char *name = nullptr;
+ char *header = nullptr;
+ unsigned char *data = nullptr;
+ long len = 0;
+
+ if (PEM_read_bio(bio.get(), &name, &header, &data, &len) != 1) {
+ unsigned long err = ERR_peek_last_error();
+ // Reason codes 100-255 are per-sub-library, so the library has to be
checked too.
+ bool const at_eof = ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE;
+ ERR_clear_error();
+ if (at_eof && !out.empty()) {
+ break;
+ }
+ SSLError("SSLRPKUtils: failed to read a PEM block from %s", path);
+ return false;
+ }
+
+ bool const is_pubkey = name != nullptr && strcmp(name, PEM_STRING_PUBLIC)
== 0;
+ if (!is_pubkey) {
+ SSLError("SSLRPKUtils: %s contains a '%s' block; only bare public keys
are supported", path,
+ name != nullptr ? name : "(unnamed)");
+ }
+
+ EVP_PKEY *pkey = nullptr;
+ if (is_pubkey) {
+ const unsigned char *p = data;
+ // Decode the PEM payload so a corrupt key is rejected at config load
rather than silently
+ // pinned as opaque bytes -- and so the pin is the canonical re-encoding
of the key rather
+ // than the payload as received: d2i_PUBKEY() doesn't reject trailing
bytes after a valid
+ // DER structure, so a non-canonically-encoded payload would otherwise
get pinned including
+ // whatever garbage follows the key, and could never match a peer's
cleanly-encoded SPKI.
+ pkey = d2i_PUBKEY(nullptr, &p, len);
+ if (pkey == nullptr) {
+ SSLError("SSLRPKUtils: failed to parse a raw public key from %s",
path);
Review Comment:
`d2i_PUBKEY` may successfully decode a valid key while leaving trailing
bytes unread, so this accepts malformed PEM payloads despite the comment saying
corrupt keys are rejected. Check that `p == data + len` after decoding and
reject the block otherwise; otherwise operators can unknowingly pin a file
containing extra data.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -188,23 +248,168 @@ static int
ssl_verify_client_callback(int preverify_ok, X509_STORE_CTX *ctx)
{
Dbg(dbg_ctl_ssl_verify, "Callback: verify client cert");
- auto *ssl = static_cast<SSL
*>(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
- SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
- TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
+ auto *ssl = static_cast<SSL *>(X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx()));
+ TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
if (tbs == nullptr) {
Dbg(dbg_ctl_ssl_verify, "call back on stale netvc");
return false;
}
+ // QUIC binds TLSBasicSupport without ever calling SSLNetVCAttach(), so the
netvc is null on an H3
+ // connection even though tbs is not. Take the name from the support class
instead.
+ TLSSNISupport *snis = TLSSNISupport::getInstance(ssl);
+ const char *servername = snis != nullptr ? snis->get_sni_server_name() :
"";
+
+#if HAVE_SSL_CTX_SET1_SERVER_CERT_TYPE
+ if (EVP_PKEY *peer_rpk = X509_STORE_CTX_get0_rpk(ctx); peer_rpk != nullptr) {
+ // The client presented a raw public key instead of a certificate: there's
no chain and no
+ // hostname to check, so pinning against the configured trusted keys
stands in for preverify_ok.
+ // The hook still always runs, mirroring the X.509 path below: plugins see
every attempt, and
+ // may add further rejection, but can't turn a failed pin match into
acceptance.
+ //
+ // `preverify_ok` is always 0 here: with no DANE configured, OpenSSL
presets
+ // X509_V_ERR_RPK_UNTRUSTED before invoking this callback (see
verify_rpk() in
+ // crypto/x509/x509_vfy.c). Clear it on a successful pin match so the
preset error doesn't
+ // survive into SSL_get_verify_result() for a connection we actually
accepted.
+ auto *trusted = static_cast<SSLRPKUtils::TrustedKeySet
*>(SSL_CTX_get_ex_data(SSL_get_SSL_CTX(ssl), ssl_client_rpk_ca_index));
+ bool pin_ok = trusted != nullptr &&
SSLRPKUtils::pinnedKeyMatches(peer_rpk, *trusted);
+ Dbg(dbg_ctl_ssl_verify, "Client authenticated with a raw public key (RFC
7250), pin match=%s", pin_ok ? "yes" : "no");
+ if (pin_ok) {
+ X509_STORE_CTX_set_error(ctx, X509_V_OK);
+ } else {
+ Warning("client raw public key did not match any trusted key for %s",
servername);
+ }
+ if (tbs->verify_certificate(ctx) == 1) {
+ Warning("TS_EVENT_SSL_VERIFY_CLIENT plugin failed the client certificate
check for %s.", servername);
+ return false;
+ }
+ return pin_ok;
+ }
+#endif
+
if (tbs->verify_certificate(ctx) == 1) { // hook moved the handshake state
to terminal
- Warning("TS_EVENT_SSL_VERIFY_CLIENT plugin failed the client certificate
check for %s.", netvc->options.sni_servername.get());
+ Warning("TS_EVENT_SSL_VERIFY_CLIENT plugin failed the client certificate
check for %s.", servername);
return false;
}
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)) &&
Review Comment:
`X509_VERIFY_PARAM_set1` copies the source verification parameters,
including purpose/trust, after `X509_STORE_CTX_set_default` has set the
`ssl_client` purpose. This can overwrite the purpose that the fallback relies
on, allowing a serverAuth-only certificate to pass as a client certificate on
BoringSSL. Copy the connection parameters before applying the `ssl_client`
default, then explicitly preserve any per-connection settings that must take
precedence.
--
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]
