bneradt commented on code in PR #13458:
URL: https://github.com/apache/trafficserver/pull/13458#discussion_r3691611902
##########
src/iocore/net/unit_tests/test_SSLDHParams.cc:
##########
@@ -184,3 +299,65 @@ TEST_CASE("ssl_context_enable_dhe: truncated DH PEM
(missing END marker) is reje
TempFile truncated{pem.substr(0, end)};
CHECK_FALSE(init_with_dhparams(truncated.get_path()));
}
+
+TEST_CASE("SSLPrivateKeyHandler: a key file matching the certificate is
loaded")
+{
+ CertAndKey ck = make_cert_and_key();
+ TempFile cert{ck.cert_pem};
+ TempFile key{ck.key_pem};
+ CHECK(load_key_via_load_certs(cert.get_path(), key.get_path()));
+}
+
+TEST_CASE("SSLPrivateKeyHandler: an empty key path loads the key bundled in
the certificate file")
+{
+ CertAndKey ck = make_cert_and_key();
+ TempFile cert{ck.cert_pem + ck.key_pem};
+ CHECK(load_key_via_load_certs(cert.get_path(), ""));
+}
+
+TEST_CASE("SSLPrivateKeyHandler: a valid key file not matching the certificate
is rejected")
+{
+ TempFile cert{make_cert_and_key().cert_pem};
+ TempFile key{make_cert_and_key().key_pem};
+ CHECK_FALSE(load_key_via_load_certs(cert.get_path(), key.get_path()));
+}
+
+TEST_CASE("SSLPrivateKeyHandler: an unparseable key file is rejected")
+{
+ TempFile cert{make_cert_and_key().cert_pem};
+ TempFile key{"-----BEGIN PRIVATE KEY-----\nnot base64\n-----END PRIVATE
KEY-----\n"};
+ CHECK_FALSE(load_key_via_load_certs(cert.get_path(), key.get_path()));
+}
+
+TEST_CASE("SSLPrivateKeyHandler: an encrypted key file is decrypted via the
SSL_CTX password callback")
+{
+ CertAndKey ck = make_cert_and_key(EVP_aes_256_cbc(), test_passphrase);
+ TempFile cert{ck.cert_pem};
+ TempFile key{ck.key_pem};
+ CHECK(load_key_via_load_certs(cert.get_path(), key.get_path(),
fixed_passphrase_cb));
+}
+
+TEST_CASE("SSLPrivateKeyHandler: an encrypted key file with the wrong
passphrase is rejected")
+{
+ char wrong_pass[]{"the-wrong-passphrase"};
+ CertAndKey ck = make_cert_and_key(EVP_aes_256_cbc(), wrong_pass);
+ TempFile cert{ck.cert_pem};
+ TempFile key{ck.key_pem};
+ CHECK_FALSE(load_key_via_load_certs(cert.get_path(), key.get_path(),
fixed_passphrase_cb));
+}
+
+// A hardware-backed key is named by a provider URI rather than a path, and the
+// key material never appears in the certificate secret. Loading one therefore
+// has to go through the provider: there is no file to read, so the secret-data
+// path has nothing to fall back to.
+//
+// This is the case an ENGINE-based ATS handled via ENGINE_load_private_key.
+TEST_CASE("SSLPrivateKeyHandler: a key named by an OpenSSL provider URI is
loaded from the provider")
+{
+ CertAndKey ck = make_cert_and_key();
+ MockHardwareProvider::ScopedProvider provider{ck.key_pem};
+ REQUIRE(provider.is_loaded());
+
+ TempFile cert{ck.cert_pem};
+ CHECK(load_key_via_load_certs(cert.get_path(), MockHardwareProvider::URI));
Review Comment:
**[P1] Preserve provider URIs before filesystem path resolution**
This test injects the raw URI directly into `CertLoadData`, bypassing the
production configuration path. `load_certs_and_cross_reference_names()` passes
every `ssl_key_name` through `Layout::relative_to(params->serverKeyPathOnly,
keyname)`, so the default key directory turns `pkcs11:...` into
`/.../pkcs11:...`. `OSSL_STORE_open()` consequently receives a filesystem path
instead of the provider scheme, making the new provider path unreachable from
normal `ssl_multicert.yaml` configuration. Please preserve recognized store
URIs before filesystem resolution and exercise the full normalization path in
this test.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -856,43 +856,24 @@ SSLMultiCertConfigLoader::default_server_ssl_ctx()
static bool
SSLPrivateKeyHandler(SSL_CTX *ctx, const char *keyPath, const char
*secret_data, int secret_data_len)
{
- EVP_PKEY *pkey = nullptr;
-#if HAVE_ENGINE_GET_DEFAULT_RSA && HAVE_ENGINE_LOAD_PRIVATE_KEY
- ENGINE *e = ENGINE_get_default_RSA();
- if (e != nullptr) {
- pkey = ENGINE_load_private_key(e, keyPath, nullptr, nullptr);
- if (pkey) {
- if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
- Dbg(dbg_ctl_ssl_load, "failed to load server private key from engine");
- EVP_PKEY_free(pkey);
- return false;
- }
- }
+ bool result{false};
+ if (keyPath && keyPath[0] != '\0') {
Review Comment:
**[P1] Keep SSL-secret plugin data authoritative**
Trying `keyPath` before the `secret_data` returned by
`SSLSecret::getOrLoadSecret()` reverses the documented
`TS_LIFECYCLE_SSL_SECRET_HOOK` contract, where plugin data replaces disk
loading. If the configured path contains a stale but parseable key, this helper
succeeds and skips the plugin key; the later certificate check can then fail
without ever trying the matching secret. This also defeats consistent updates
made through `TSSslSecretSet()`. Please retain secret-data precedence for
ordinary file names and use the path-first store lookup only for actual
provider URIs.
##########
src/iocore/net/SSLKeyUtils.cc:
##########
@@ -188,3 +192,113 @@ set_ctx_dh(SSL_CTX *ctx, dh_key_t *pkey)
}
#endif // OPENSSL_IS_OPENSSL3
+
+// A hardware-backed key is named rather than stored: the name resolves through
+// the device and there is no file holding the key. Both versions below
therefore
+// ask the hardware before falling back to reading keyPath as a file, which is
+// also the order ATS used when this was ENGINE-only, so that a configured
device
+// keeps precedence over any same-named file on disk.
+
+#ifdef OPENSSL_IS_OPENSSL3
+
+bool
+use_pkey_from_file(SSL_CTX *ctx, const char *keyPath)
+{
+ ink_assert(keyPath && keyPath[0] != '\0');
+
+ // A store prompts for a PIN or passphrase through a UI_METHOD rather than
the
+ // pem_password_cb an SSL_CTX carries, so wrap the configured callback. The
+ // wrapper invokes the callback unconditionally once prompted, so leave the
+ // UI_METHOD null when there is nothing to wrap rather than hand it a null
+ // callback to call.
+ pem_password_cb *password_cb = SSL_CTX_get_default_passwd_cb(ctx);
+ scoped_UI_Method ui{password_cb ?
UI_UTIL_wrap_read_pem_callback(password_cb, 0) : nullptr};
+
+ // OpenSSL 3 reaches a hardware key store through a provider, which exposes
+ // its keys as an OSSL_STORE under its own URI scheme -- "pkcs11:" for a
+ // PKCS#11 provider fronting an HSM, for instance.
+ scoped_Store_CTX store{OSSL_STORE_open(keyPath, ui.get(),
SSL_CTX_get_default_passwd_cb_userdata(ctx), nullptr, nullptr)};
+
+ if (store) {
+ // Announcing the one type wanted lets a loader retrieve it directly rather
+ // than enumerate the whole store. It does not make a single load
sufficient:
+ // a loader that ignores the hint still yields other objects first, which
the
+ // generic layer drops by returning nullptr without reaching EOF.
+ OSSL_STORE_expect(store.get(), OSSL_STORE_INFO_PKEY);
+ }
+
+ // A store URI may name a whole collection -- a token's worth of objects, of
+ // which only some are keys -- so scan until a private key turns up.
+ scoped_PKEY pkey;
+ while (store && !pkey && !OSSL_STORE_eof(store.get())) {
+ scoped_Store_Info info{OSSL_STORE_load(store.get())};
+ if (info && OSSL_STORE_INFO_get_type(info.get()) == OSSL_STORE_INFO_PKEY) {
+ pkey.reset(OSSL_STORE_INFO_get1_PKEY(info.get()));
+ }
+ // A single object failing to load is not fatal; OSSL_STORE_eof reports
true
+ // once the store itself gives up, so the loop terminates either way.
+ }
+
+ if (pkey && SSL_CTX_use_PrivateKey(ctx, pkey.get())) {
+ return true;
+ }
+
+ // OSSL_STORE also handles plain paths, but only for keys it can decode
+ // itself, so getting here says nothing about whether keyPath names a
loadable
+ // file. Not finding the key in hardware is the ordinary case for a
file-based
+ // configuration, so leave no errors behind for the caller to misread.
+ ERR_clear_error();
+
+ return 1 == SSL_CTX_use_PrivateKey_file(ctx, keyPath, SSL_FILETYPE_PEM);
+}
+
+#else
+
+bool
+use_pkey_from_file(SSL_CTX *ctx, const char *keyPath)
+{
+ ink_assert(keyPath && keyPath[0] != '\0');
+
+#if HAVE_ENGINE_GET_DEFAULT_RSA && HAVE_ENGINE_LOAD_PRIVATE_KEY
+ // Before providers, a hardware key store was reached through an ENGINE.
+ // Absent a configured engine there is nothing to ask.
+ if (ENGINE *e = ENGINE_get_default_RSA(); e != nullptr) {
Review Comment:
**[P2] Release the functional ENGINE reference**
`ENGINE_get_default_RSA()` returns an incremented functional reference that
must be released with `ENGINE_finish()` before it is discarded. Neither the
successful early return nor the failure/fallback path releases `e`, so repeated
certificate loads and configuration reloads leak ENGINE references. Please
manage this with an RAII deleter or explicitly finish the ENGINE on every path.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -856,43 +856,24 @@ SSLMultiCertConfigLoader::default_server_ssl_ctx()
static bool
SSLPrivateKeyHandler(SSL_CTX *ctx, const char *keyPath, const char
*secret_data, int secret_data_len)
{
- EVP_PKEY *pkey = nullptr;
-#if HAVE_ENGINE_GET_DEFAULT_RSA && HAVE_ENGINE_LOAD_PRIVATE_KEY
- ENGINE *e = ENGINE_get_default_RSA();
- if (e != nullptr) {
- pkey = ENGINE_load_private_key(e, keyPath, nullptr, nullptr);
- if (pkey) {
- if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
- Dbg(dbg_ctl_ssl_load, "failed to load server private key from engine");
- EVP_PKEY_free(pkey);
- return false;
- }
- }
+ bool result{false};
+ if (keyPath && keyPath[0] != '\0') {
+ result = use_pkey_from_file(ctx, keyPath);
}
-#else
- void *e = nullptr;
-#endif
- if (pkey == nullptr) {
- scoped_BIO bio(BIO_new_mem_buf(secret_data, secret_data_len));
-
- pem_password_cb *password_cb = SSL_CTX_get_default_passwd_cb(ctx);
- void *u = SSL_CTX_get_default_passwd_cb_userdata(ctx);
- pkey = PEM_read_bio_PrivateKey(bio.get(), nullptr,
password_cb, u);
- if (nullptr == pkey) {
- Dbg(dbg_ctl_ssl_load, "failed to load server private key (%.*s) from
%s", secret_data_len < 50 ? secret_data_len : 50,
- secret_data, (!keyPath || keyPath[0] == '\0') ? "[empty key path]" :
keyPath);
- return false;
- }
- if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
- Dbg(dbg_ctl_ssl_load, "failed to attach server private key loaded from
%s",
- (!keyPath || keyPath[0] == '\0') ? "[empty key path]" : keyPath);
- EVP_PKEY_free(pkey);
- return false;
- }
- if (e == nullptr && !SSL_CTX_check_private_key(ctx)) {
- Dbg(dbg_ctl_ssl_load, "server private key does not match the certificate
public key");
- return false;
- }
+
+ if (!result) {
+ result = use_pkey_from_secret_data(ctx, secret_data, secret_data_len);
+ }
+
+ if (!result) {
+ Dbg(dbg_ctl_ssl_load, "failed to load server private key (%.*s) from %s",
secret_data_len < 50 ? secret_data_len : 50,
+ secret_data, (!keyPath || keyPath[0] == '\0') ? "[empty key path]" :
keyPath);
+ return false;
+ }
+
+ if (!SSL_CTX_check_private_key(ctx)) {
Review Comment:
**[P1] Do not require this comparison for opaque hardware keys**
The previous `e == nullptr` guard was deliberate: hardware-backed ENGINE
keys, including HSM/TPM implementations, may support signing without exposing
enough key material for `SSL_CTX_check_private_key()` to compare them. Making
the check unconditional rejects those keys in the retained pre-OpenSSL-3 ENGINE
path and can likewise reject opaque provider keys. The mock provider does not
cover this because it returns an ordinary exportable PEM key. Please preserve
the hardware-key distinction or use a validation path that
provider/ENGINE-backed opaque keys can support.
--
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]
