maskit commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3975976288
##########
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:
Every section in this file locates its item by index (`items[0]`, `[1]`,
`[2]`, `[3]`, `[10]`). Converting only the two RPK sections to fqdn lookup
would leave the file inconsistent, and converting all nine is unrelated to this
change, so keeping the existing convention.
##########
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:
Done in 8e08afa. Including `SSLRPKUtils.h` here would invert the layering --
it is private to `src/iocore/net/` and pulls in `<openssl/evp.h>` -- so the
aliases moved to `SSLTypes.h` instead, the public header that already holds
shared TLS types and is already included from this directory. The member now
names `SSLRPKUtils::TrustedKeySet` and the comment explaining the type is gone.
##########
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:
Documented the contract in 8e08afa rather than changing the shape: `out` is
appended to as the file is parsed, so on failure it holds an arbitrary prefix
and callers must discard it. All ten call sites already do -- each builds a set
it owns and destroys or drops it on `false`, and `SNIConfigParams` assigns
`nps.prop.server_rpk_ca` only after the load succeeds -- so no caller can
observe a partial set.
--
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]
