Copilot commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3779936064
##########
src/iocore/net/SSLClientUtils.cc:
##########
@@ -236,6 +482,92 @@ ssl_new_session_callback(SSL *ssl, SSL_SESSION *sess)
return 0;
}
+#if TS_USE_RPK
+bool
+ssl_client_setup_rpk(SSL *ssl, bool offer_rpk, const std::string
&trusted_key_file)
+{
+ if (!offer_rpk && trusted_key_file.empty()) {
+ return true;
+ }
+
+ static std::once_flag rpk_index_once;
+ std::call_once(rpk_index_once, []() {
+ ssl_server_rpk_index = SSL_get_ex_new_index(0, (void *)"Trusted next-hop
RPK keys", nullptr, nullptr, ssl_server_rpk_ex_free);
+ });
+ if (ssl_server_rpk_index < 0) {
+ SSLError("failed to reserve an ex_data index for next-hop raw public
keys");
+ return false;
+ }
+
+ if (!trusted_key_file.empty()) {
+ auto *trusted = new SSLRPKUtils::TrustedKeySet();
+ if (!SSLRPKUtils::loadTrustedKeys(trusted_key_file.c_str(), *trusted)) {
+ delete trusted;
+ return false;
+ }
+ // ssl_server_rpk_ex_free() releases `trusted` when ssl is freed.
+ if (!SSL_set_ex_data(ssl, ssl_server_rpk_index, trusted)) {
+ delete trusted;
+ SSLError("failed to attach trusted next-hop raw public keys to the
connection");
+ return false;
+ }
+
+ // Accept a raw public key from the next hop, still preferring it over
X.509 only when the
+ // peer also supports it.
+ static const unsigned char server_types[] = {TLSEXT_cert_type_rpk,
TLSEXT_cert_type_x509};
+#if HAVE_SSL_CTX_SET1_SERVER_CERT_TYPE
+ if (!SSL_set1_server_cert_type(ssl, server_types, sizeof(server_types))) {
+#else
+ if (!SSL_set1_accepted_peer_cert_types(ssl, server_types,
sizeof(server_types))) {
+#endif
+ SSLError("failed to enable RPK server cert type negotiation for the
outbound connection");
+ return false;
+ }
+ }
+
+ if (offer_rpk) {
+ static const unsigned char client_types[] = {TLSEXT_cert_type_rpk,
TLSEXT_cert_type_x509};
+ // Both libraries derive/wrap the offered raw public key from the client
certificate/key
+ // already configured on the context -- there is nothing to offer if
that's unset.
+ if (SSL_CTX_get0_privatekey(SSL_get_SSL_CTX(ssl)) == nullptr) {
+ SSLError("client_rpk_enabled requires a client certificate/key
configured for this next hop");
+ return false;
+ }
+#if HAVE_SSL_CTX_SET1_SERVER_CERT_TYPE
+ // OpenSSL derives the offered raw public key from the certificate/key
already on the context.
+ if (!SSL_set1_client_cert_type(ssl, client_types, sizeof(client_types))) {
+ SSLError("failed to enable RPK client cert type negotiation for the
outbound connection");
+ return false;
+ }
+#else
+ // BoringSSL needs an explicit credential, wrapping that same
already-configured key.
+ EVP_PKEY *pkey = SSL_CTX_get0_privatekey(SSL_get_SSL_CTX(ssl));
+ SSL_CREDENTIAL *cred = SSL_CREDENTIAL_new_raw_public_key(pkey);
+ if (cred == nullptr || !SSL_add1_credential(ssl, cred)) {
+ SSLError("failed to add the outbound RPK credential");
+ SSL_CREDENTIAL_free(cred);
+ return false;
+ }
+ SSL_CREDENTIAL_free(cred);
+ if (!SSL_set1_available_client_cert_types(ssl, client_types,
sizeof(client_types))) {
+ SSLError("failed to advertise RPK client cert types for the outbound
connection");
+ return false;
+ }
+#endif
+ }
+
+#if HAVE_SSL_CREDENTIAL_NEW_RAW_PUBLIC_KEY
+ // BoringSSL rejects raw public keys unless a custom verify callback is
installed, and this
+ // displaces the SSL_set_verify()/verify_callback() pair the caller already
set for this
+ // connection. Only RPK-configured next hops take this path; every other
outbound connection
+ // keeps the classic callback untouched.
+ SSL_set_custom_verify(ssl, SSL_VERIFY_PEER,
ssl_client_custom_verify_callback);
+#endif
Review Comment:
On BoringSSL builds, SSL_set_custom_verify() replaces the existing
SSL_set_verify()/verify_callback path for the entire connection. Installing it
unconditionally here means a next hop with only client_rpk_enabled (offer-only)
will also switch to the manual X.509 verification path even though it will
never negotiate an RPK server identity, risking behavioral drift. Limit
custom-verify to cases where we actually accept/pin an RPK server key (i.e.
trusted_key_file is non-empty).
##########
src/iocore/net/SSLRPKUtils.cc:
##########
@@ -0,0 +1,134 @@
+/** @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;
+ }
+
+ 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();
+ bool const at_eof = ERR_GET_REASON(err) == PEM_R_NO_START_LINE;
+ ERR_clear_error();
Review Comment:
loadTrustedKeys() determines EOF by inspecting the OpenSSL error queue after
PEM_read_bio() fails, but it never clears any pre-existing errors before
calling PEM_read_bio(). If the thread already has an unrelated error queued,
ERR_peek_last_error() may not report PEM_R_NO_START_LINE and a valid file could
be treated as malformed.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -205,6 +246,107 @@ 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 hands back the raw chain as CRYPTO_BUFFERs, not
X509 objects, so
+ // rebuild it and run the same verification SSL_CTX_set_verify() would
otherwise do for us.
+ const STACK_OF(CRYPTO_BUFFER) *chain = SSL_get0_peer_certificates(ssl);
+ if (chain == nullptr || sk_CRYPTO_BUFFER_num(chain) == 0) {
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
+
+ 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 a client certificate offered to a RPK-enabled
context");
+ 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 {
+ sk_X509_push(intermediates, cert);
+ }
+ }
+
+ X509_STORE_CTX *store_ctx = X509_STORE_CTX_new();
+ bool initialized = store_ctx != nullptr && X509_STORE_CTX_init(store_ctx,
SSL_CTX_get_cert_store(ctx), leaf, intermediates);
Review Comment:
The BoringSSL custom-verify X.509 fallback path verifies against
SSL_CTX_get_cert_store(ctx), but ATS can override the verify store per
connection via SSL_set0_verify_cert_store (e.g.
SNIActionPerformer/setClientCertCACerts). Using the SSL_CTX store here can
silently ignore per-SNI CA configuration when falling back to X.509.
##########
tests/gold_tests/tls/tls_rpk_hop.test.py:
##########
@@ -0,0 +1,230 @@
+'''
+Test RFC 7250 raw public key (RPK) TLS between two ATS instances.
+'''
+# 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.
+
+Test.Summary = '''
+Test raw public keys (RFC 7250) on ATS-to-ATS (layered cache) TLS hops.
+'''
+
+# RPK is only compiled in when the linked TLS library supports it, so skip
rather
+# than fail where it is unavailable.
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_RPK'))
+
+server = Test.MakeOriginServer("server")
+request_header = {'headers': 'GET / HTTP/1.1\r\nHost:
www.example.com\r\n\r\n', 'timestamp': '1469733493.993', 'body': ''}
+response_header = {
+ 'headers': 'HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n',
+ 'timestamp': '1469733493.993',
+ 'body': 'origin response'
+}
+server.addResponse("sessionlog.json", request_header, response_header)
+
+
+def make_parent(name, rpk_enabled, client_rpk_ca_file=None,
client_cert_level=0):
+ """An upstream (parent) ATS: terminates TLS from the edge, forwards to the
origin.
+
+ `client_rpk_ca_file`, if set, configures ssl_client_rpk_ca_name to pin the
edge's raw public
+ key for mTLS; `client_cert_level` then requires/requests a client cert
accordingly.
+ """
+ ts = Test.MakeATSProcess(name, enable_tls=True)
+ ts.addSSLfile("ssl/server.pem")
+ ts.addSSLfile("ssl/server.key")
+ if client_rpk_ca_file is not None:
+ ts.addSSLfile("ssl/{0}".format(client_rpk_ca_file))
+ ts.Disk.remap_config.AddLine('map /
http://127.0.0.1:{0}'.format(server.Variables.Port))
+ multicert_lines = [
+ 'ssl_multicert:',
+ ' - dest_ip: "*"',
+ ' ssl_cert_name: server.pem',
+ ' ssl_key_name: server.key',
+ ]
+ if rpk_enabled:
+ multicert_lines.append(' ssl_rpk_enabled: 1')
+ if client_rpk_ca_file is not None:
+ # The file name is deliberately bare here (not
ts.Variables.SSLDir-prefixed) to exercise
+ # that ssl_client_rpk_ca_name resolves against
proxy.config.ssl.CA.cert.path, matching
+ # the equivalent resolution ssl_ca_name already gets.
+ multicert_lines.append(' ssl_client_rpk_ca_name:
{0}'.format(client_rpk_ca_file))
+ ts.Disk.ssl_multicert_yaml.AddLines(multicert_lines)
+ records = {
+ 'proxy.config.http.cache.http': 0,
+ 'proxy.config.ssl.server.cert.path': '{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.ssl.server.private_key.path':
'{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'ssl_verify|ssl_load',
+ }
+ if client_cert_level:
+ records['proxy.config.ssl.client.certification_level'] =
client_cert_level
+ records['proxy.config.ssl.CA.cert.path'] =
'{0}'.format(ts.Variables.SSLDir)
+ ts.Disk.records_config.update(records)
+ return ts
+
+
+def make_edge(name, parent, pin_file, policy='ENFORCED',
offer_client_rpk=False):
+ """A downstream (edge) ATS: connects to `parent` over TLS, pinning its raw
public key.
+
+ `offer_client_rpk`, if set, also offers a raw public key (derived from
ssl/server.pem/.key,
+ the same identity the edge uses inbound) as its own client cert toward
`parent`, for `parent`
+ to pin via ssl_client_rpk_ca_name.
+ """
+ ts = Test.MakeATSProcess(name, enable_tls=True)
+ ts.addSSLfile("ssl/server.pem")
+ ts.addSSLfile("ssl/server.key")
+ ts.addSSLfile("ssl/server.pubkey.pem")
+ ts.addSSLfile("ssl/server.wrongpubkey.pem")
+ ts.Disk.remap_config.AddLine('map /
https://127.0.0.1:{0}'.format(parent.Variables.ssl_port))
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ [
+ 'ssl_multicert:',
+ ' - dest_ip: "*"',
+ ' ssl_cert_name: server.pem',
+ ' ssl_key_name: server.key',
+ ])
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.http.cache.http': 0,
+ 'proxy.config.ssl.server.cert.path':
'{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.ssl.server.private_key.path':
'{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.ssl.client.cert.path':
'{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.ssl.client.private_key.path':
'{0}'.format(ts.Variables.SSLDir),
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'ssl_verify',
+ 'proxy.config.ssl.client.verify.server.policy': policy,
+ # Pin the exact key instead of matching a name: a raw public key
carries no SAN.
+ 'proxy.config.ssl.client.verify.server.properties': 'SIGNATURE',
+ })
+ if pin_file is not None or offer_client_rpk:
+ sni_lines = [
+ 'sni:',
+ '- fqdn: 127.0.0.1',
+ ]
+ if pin_file is not None:
+ sni_lines.append(' server_rpk_ca:
{0}/{1}'.format(ts.Variables.SSLDir, pin_file))
+ if offer_client_rpk:
+ sni_lines += [
+ ' client_cert: server.pem',
+ ' client_key: server.key',
+ ' client_rpk_enabled: true',
+ ]
+ ts.Disk.sni_yaml.AddLines(sni_lines)
+ return ts
+
+
+# 1. Both hops speak RPK and the pin matches -> RPK is negotiated and accepted.
+parent_rpk = make_parent("parent_rpk", rpk_enabled=True)
+edge_ok = make_edge("edge_ok", parent_rpk, "server.pubkey.pem")
+
+# 2. The parent has not been upgraded (no RPK), the edge is configured for it
->
+# negotiation must fall back to X.509 rather than failing. This is the
steady state
+# for the whole duration of a rolling upgrade.
+parent_x509 = make_parent("parent_x509", rpk_enabled=False)
+edge_fallback = make_edge("edge_fallback", parent_x509, "server.pubkey.pem",
policy='PERMISSIVE')
+
+# 3. The pin does not match the key the parent presents -> rejected under
ENFORCED.
+edge_badpin = make_edge("edge_badpin", parent_rpk, "server.wrongpubkey.pem")
+
+# 4. Same mismatch under PERMISSIVE -> warned about, but the request still
succeeds.
+edge_badpin_permissive = make_edge("edge_badpin_permissive", parent_rpk,
"server.wrongpubkey.pem", policy='PERMISSIVE')
+
+# 5. mTLS: the parent requires and pins the edge's raw public key, and the pin
matches.
+parent_mtls = make_parent("parent_mtls", rpk_enabled=True,
client_rpk_ca_file="server.pubkey.pem", client_cert_level=2)
+edge_mtls = make_edge("edge_mtls", parent_mtls, "server.pubkey.pem",
offer_client_rpk=True)
+
+# 6. mTLS: same setup, but the parent pins a different key than the edge
actually offers ->
+# a required client cert is always fatal, unlike verify_server_policy which
has a
+# PERMISSIVE mode -- there is no equivalent "warn only" mode for inbound
mTLS.
+parent_mtls_badpin = make_parent(
+ "parent_mtls_badpin", rpk_enabled=True,
client_rpk_ca_file="server.wrongpubkey.pem", client_cert_level=2)
+edge_mtls_badpin = make_edge("edge_mtls_badpin", parent_mtls_badpin,
"server.pubkey.pem", offer_client_rpk=True)
+
+tr = Test.AddTestRun("RPK negotiated and pin matches")
+tr.MakeCurlCommand('-k
https://127.0.0.1:{0}/'.format(edge_ok.Variables.ssl_port))
+tr.Processes.Default.ReturnCode = 0
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(parent_rpk)
+tr.Processes.Default.StartBefore(edge_ok)
+tr.Processes.Default.Streams.All = Testers.ContainsExpression('origin
response', 'the request should succeed end to end')
+edge_ok.Disk.traffic_out.Content = Testers.ContainsExpression(
+ 'Origin authenticated with a raw public key .*pin match=yes', 'the hop
should use RPK, not fall back to X.509')
+tr.StillRunningAfter = server
+tr.StillRunningAfter += parent_rpk
+tr.StillRunningAfter += edge_ok
+
+tr = Test.AddTestRun("falls back to X.509 against a parent without RPK
support")
+tr.MakeCurlCommand('-k
https://127.0.0.1:{0}/'.format(edge_fallback.Variables.ssl_port))
+tr.Processes.Default.ReturnCode = 0
+tr.Processes.Default.StartBefore(parent_x509)
+tr.Processes.Default.StartBefore(edge_fallback)
+tr.Processes.Default.Streams.All = Testers.ContainsExpression('origin
response', 'the request should still succeed')
+# No RPK was negotiated, so the RPK branch must never run for this hop.
+edge_fallback.Disk.traffic_out.Content = Testers.ExcludesExpression(
+ 'Origin authenticated with a raw public key', 'the hop should quietly
negotiate X.509 instead')
+tr.StillRunningAfter = server
+tr.StillRunningAfter += parent_x509
+tr.StillRunningAfter += edge_fallback
+
+tr = Test.AddTestRun("pin mismatch is fatal under ENFORCED")
+tr.MakeCurlCommand('-k
https://127.0.0.1:{0}/'.format(edge_badpin.Variables.ssl_port))
+# curl sees a 5xx from the edge (upstream connect failed) rather than a
transport error.
+tr.Processes.Default.ReturnCode = 0
+tr.Processes.Default.StartBefore(edge_badpin)
Review Comment:
This TestRun depends on parent_rpk being up to accept the upstream TLS
connection, but it is not started in this run. Since the previous run's
StillRunningAfter list doesn't include parent_rpk, it may have been stopped
before this run begins, leading to a failing/flaky test.
This issue also appears on line 199 of the same file.
--
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]
