Copilot commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3975143198
##########
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()` returns a chain that includes the leaf
cert, but `X509_STORE_CTX_init(store, leaf, chain)` expects `chain` to be the
*untrusted intermediates* (typically excluding the leaf). Passing the full
chain (leaf included) can lead to redundant/incorrect path building. Consider
building a separate `STACK_OF(X509)` containing only entries `[1..N-1]` as
intermediates (or otherwise ensuring the stack passed to `X509_STORE_CTX_init`
excludes the leaf) before calling `X509_verify_cert()`.
##########
include/iocore/net/SSLSNIConfig.h:
##########
@@ -49,8 +50,15 @@
// Properties for the next hop server
struct NextHopProperty {
- std::string client_cert_file;
// full path to client cert file for lookup
- std::string client_key_file;
// full path to client key file for lookup
+ std::string client_cert_file; // full path to client cert file for
lookup
+ std::string client_key_file; // full path to client key file for
lookup
+ bool client_rpk_enabled = false; // offer a RFC 7250 raw public key
(derived from the configured client
+ // cert/key) alongside X.509 when
connecting to this next hop
+ std::string server_rpk_ca_file; // full path to the PEM of trusted
next-hop raw public keys to pin against
+ // Parsed contents of server_rpk_ca_file (SSLRPKUtils::TrustedKeySet, i.e. a
set of DER
+ // SubjectPublicKeyInfo blobs), loaded once at config load rather than
re-parsed from disk on
+ // every outbound handshake to this next hop.
+ std::shared_ptr<const std::vector<std::vector<unsigned char>>> server_rpk_ca;
Review Comment:
`server_rpk_ca` uses a raw `std::vector<std::vector<unsigned char>>` type
even though the comment describes it as `SSLRPKUtils::TrustedKeySet`. This
makes the API harder to read and easier to drift if `TrustedKeySet` changes.
Prefer using `std::shared_ptr<const SSLRPKUtils::TrustedKeySet>` (either by
including `SSLRPKUtils.h` here, or by introducing a shared type alias exposed
in a header that both can include).
##########
src/iocore/net/SSLRPKUtils.cc:
##########
@@ -0,0 +1,158 @@
+/** @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);
+ }
+ }
+
+ OPENSSL_free(name);
+ OPENSSL_free(header);
+
+ if (pkey == nullptr) {
+ OPENSSL_free(data);
+ return false;
+ }
+ OPENSSL_free(data);
+
+ int canonical_len = i2d_PUBKEY(pkey, nullptr);
+ if (canonical_len <= 0) {
+ SSLError("SSLRPKUtils: failed to re-encode a raw public key from %s",
path);
+ EVP_PKEY_free(pkey);
+ return false;
+ }
+ TrustedKey canonical(canonical_len);
+ unsigned char *cp = canonical.data();
+ int const written = i2d_PUBKEY(pkey, &cp);
+ EVP_PKEY_free(pkey);
+ if (written != canonical_len) {
+ // A short write would leave the pin zero padded, so it could never
match a peer's key --
+ // presenting as a mismatch against the peer rather than as a bad pin
file.
+ SSLError("SSLRPKUtils: inconsistent raw public key encoding from %s",
path);
+ return false;
+ }
+
+ out.push_back(std::move(canonical));
+ }
+
+ return true;
Review Comment:
`loadTrustedKeys()` appends into `out` as it parses, but returns `false` on
mid-file parse/read errors without rolling back. That can leave callers with a
partially-updated trust set (depending on how they handle the `false` return).
A safer pattern is to load into a local `TrustedKeySet tmp` and only `out =
std::move(tmp)` / `out.swap(tmp)` upon success (or explicitly clear `out` on
any failure).
##########
src/iocore/net/unit_tests/test_YamlSNIConfig.cc:
##########
@@ -112,6 +112,22 @@ TEST_CASE("YamlSNIConfig sets port ranges appropriately")
REQUIRE(item.ssl_ticket_number.has_value());
CHECK(item.ssl_ticket_number.value() == 3);
}
+
+ SECTION("Raw public key settings are parsed.")
+ {
+ // server_rpk_ca is deliberately not exercised here: SNIConfigParams
resolves it against the
+ // configured CA directory and probe-loads it, which a unit test can't
stage. The loader
+ // itself is covered directly in test_SSLRPKUtils.cc.
+ auto const &item{conf.items[13]};
+ CHECK(item.client_rpk_enabled);
+ }
+
+ SECTION("Raw public key settings default to off.")
+ {
+ auto const &item{conf.items[12]};
+ CHECK_FALSE(item.client_rpk_enabled);
+ CHECK(item.server_rpk_ca.empty());
Review Comment:
These sections hard-code `conf.items[13]` / `[12]`, which couples the test
to the exact ordering and count of entries in `sni_conf_test.yaml` (making
unrelated additions/removals reorder-sensitive). Consider locating the target
item by a stable attribute (e.g., matching `fqdn == "rpk.com"` for the enabled
case, and another known fqdn for the defaults case) rather than by index.
--
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]
