Add tests for OpenSSL cert loading

[JosiahWI]

This patch contains tests and cleanup that are preparing for adding support for OpenSSL's PROVIDER API.

[Copilot]

Pull request overview

This PR continues the OpenSSL 3.x migration work for ATS SSL context setup by removing ENGINE-based private key loading and extending unit test coverage around DH parameter loading and private key handling (via SSLMultiCertConfigLoader public entry points).

Changes:

• Initialize global lifecycle hooks in the unit test harness to support secret-loading paths that consult lifecycle hooks.
• Add Catch2 unit tests covering DH params loading and multiple private-key loading scenarios (file key, bundled key, encrypted key via password callback, mismatch cases).
• Refactor server private key attachment to use new helpers in SSLKeyUtils.*, moving key-load logic out of SSLUtils.cc.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/iocore/net/unit_tests/unit_test_main.cc	Initializes global lifecycle hooks needed by SSL secret loading during unit tests.
src/iocore/net/unit_tests/test_SSLDHParams.cc	Adds DH params tests plus new private-key loading tests (including encrypted key callback).
src/iocore/net/SSLUtils.cc	Removes ENGINE path and routes server key attachment through new key utility helpers.
src/iocore/net/SSLKeyUtils.h	Exposes new key-loading helper APIs for SSL context setup.
src/iocore/net/SSLKeyUtils.cc	Implements key-loading helpers and consolidates OpenSSL includes for DH/key handling.

Comments suppressed due to low confidence (2)

src/iocore/net/unit_tests/test_SSLDHParams.cc:82

• BIO_new() can return nullptr; PEM_write_bio_Parameters(bio, ...) will crash rather than producing a Catch2 failure. Add a REQUIRE(bio != nullptr) after allocation to make the test fail cleanly on allocation errors.

  BIO *bio = BIO_new(BIO_s_mem());
  REQUIRE(PEM_write_bio_Parameters(bio, pkey) == 1);
  std::string const out{bio_to_string(bio)};

src/iocore/net/unit_tests/test_SSLDHParams.cc:97

• BIO_new() can return nullptr; PEM_write_bio_PrivateKey(bio, ...) would crash rather than reporting a test failure. Add a REQUIRE(bio != nullptr) before using it.

  BIO *bio = BIO_new(BIO_s_mem());
  int  passlen{pass ? static_cast<int>(std::strlen(pass)) : 0};
  REQUIRE(PEM_write_bio_PrivateKey(bio, pkey, cipher, reinterpret_cast<unsigned char *>(pass), passlen, nullptr, nullptr) == 1);
  std::string out{bio_to_string(bio)};

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/iocore/net/unit_tests/test_SSLDHParams.cc:235

• settings.cert is allocated with ats_strdup but is never freed in load_key_via_load_certs. This can show up as a leak under LSAN/ASAN (and makes repeated calls accumulate leaks). Consider freeing settings.cert before returning (or using an RAII wrapper / owning string type in the test harness).

  SSLConfigParams          params;
  SSLMultiCertConfigParams settings;
  settings.cert = ats_strdup(cert_path);

src/iocore/net/unit_tests/test_SSLDHParams.cc:217

• The OpenSSL password callback commonly expects the returned buffer to be NUL-terminated when treated as a C-string by downstream code. Clamping to size can produce a non-terminated buffer when len == size. Consider clamping to size - 1, copying that many bytes, and writing buf[len] = '\\0' (while still returning len).

int
fixed_passphrase_cb(char *buf, int size, int /* rwflag */, void * /* u */)
{
  int len{static_cast<int>(std::strlen(test_passphrase))};
  if (len > size) {
    len = size;
  }
  std::memcpy(buf, test_passphrase, len);
  return len;
}

src/iocore/net/unit_tests/test_SSLDHParams.cc:137

• The return value of X509_NAME_add_entry_by_txt is not checked. If it fails, the certificate subject may be incomplete/invalid and could make failures harder to diagnose. Add a REQUIRE(... == 1) for the X509_NAME_add_entry_by_txt call.

  X509_NAME *name = X509_get_subject_name(x509);
  X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, reinterpret_cast<unsigned char const *>("ats-test"), -1, -1, 0);
  REQUIRE(X509_set_issuer_name(x509, name) == 1);
  REQUIRE(X509_sign(x509, pkey, EVP_sha256()) > 0);

src/iocore/net/SSLKeyUtils.cc:201

• OpenSSL maintains a per-thread error queue. Calling ERR_get_error() here only consumes one entry and may leave additional buffered errors that can confuse later error handling/logging. Consider clearing the queue before the OpenSSL call (e.g., ERR_clear_error()), and on failure draining/logging the full queue (looping ERR_get_error() until 0, or using ERR_print_errors_cb) so diagnostics are complete and the queue is left clean.

  int const result{SSL_CTX_use_PrivateKey_file(ctx, keyPath, SSL_FILETYPE_PEM)};
  if (1 != result) {
    char err_buf[256]{};
    ERR_error_string_n(ERR_get_error(), err_buf, sizeof(err_buf));
    Error("failed to load private key %s: %s", keyPath, err_buf);
  }

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/iocore/net/SSLKeyUtils.cc:207

• use_pkey_from_secret_data() doesn’t check whether BIO_new_mem_buf() succeeded. If it returns nullptr (e.g. low-memory), PEM_read_bio_PrivateKey(bio.get(), ...) will dereference a null BIO pointer. This should mirror the existing pattern elsewhere in SSLUtils.cc where the BIO allocation is checked and a clean failure is returned/logged.

  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);
  EVP_PKEY        *pkey        = PEM_read_bio_PrivateKey(bio.get(), nullptr, password_cb, u);

src/iocore/net/SSLUtils.cc:862

• This change removes ENGINE-based private key loading from SSLPrivateKeyHandler(), but SSLUtils.cc still calls ENGINE_load_dynamic() in SSLPostConfigInitialize() when proxy.config.ssl.engine.conf_file is set. If the goal of this PR (and #13347) is to eliminate remaining OpenSSL 3 ENGINE API usage, this PR likely isn’t complete yet (or needs to clarify that engine-conf support remains on some builds).

  bool result{false};
  if (keyPath && keyPath[0] != '\0') {
    result = use_pkey_from_file(ctx, keyPath);
  }

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/iocore/net/unit_tests/test_SSLDHParams.cc:250

• settings.cert is allocated with ats_strdup but is never freed in this helper, which will leak memory across the test process. Free settings.cert (e.g., via ats_free) before returning, or wrap it in a small RAII helper so the allocation is always released even when assertions fail.

  SSLConfigParams          params;
  SSLMultiCertConfigParams settings;
  settings.cert = ats_strdup(cert_path);

  SSLMultiCertConfigLoader::CertLoadData data;
  data.cert_names_list.emplace_back(cert_path);
  data.key_list.emplace_back(key_path);

  SSL_CTX *ctx = SSL_CTX_new(TLS_server_method());
  REQUIRE(ctx != nullptr);
  if (passwd_cb != nullptr) {
    SSL_CTX_set_default_passwd_cb(ctx, passwd_cb);
  }

  bool ok = SSLMultiCertConfigLoader::load_certs(ctx, data.cert_names_list, data.key_list, data, &params, &settings);

  SSL_CTX_free(ctx);
  return ok;

src/iocore/net/SSLUtils.cc:866

• If use_pkey_from_file fails, it likely leaves entries on OpenSSL’s per-thread error queue. If the subsequent secret-data load succeeds, those stale errors remain and can confuse later diagnostics (or get logged by unrelated code). Consider clearing the error queue before attempting the fallback (or clearing it after a successful fallback) so failures from earlier attempts don’t leak into later operations.

  if (keyPath && keyPath[0] != '\0') {
    result = use_pkey_from_file(ctx, keyPath);
  }

  if (!result) {
    result = use_pkey_from_secret_data(ctx, secret_data, secret_data_len);
  }

src/iocore/net/SSLKeyUtils.cc:237

• BIO_new_mem_buf can return nullptr (e.g., allocation failure). In that case, bio.get() will be null and passing it into PEM_read_bio_PrivateKey is unsafe. Add an explicit check for bio.get() != nullptr and return false early when allocation fails.

use_pkey_from_secret_data(SSL_CTX *ctx, const char *secret_data, int secret_data_len)
{
  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);
  EVP_PKEY        *pkey        = PEM_read_bio_PrivateKey(bio.get(), nullptr, password_cb, u);
  if (nullptr == pkey) {
    return false;
  }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/iocore/net/unit_tests/test_SSLDHParams.cc:236

• settings.cert is an ats_scoped_str, which already has an operator=(std::string_view) that duplicates and owns the string. Using ats_strdup() here is redundant extra allocation and makes ownership less clear.

  SSLConfigParams          params;
  SSLMultiCertConfigParams settings;
  settings.cert = ats_strdup(cert_path);

src/iocore/net/unit_tests/test_SSLDHParams.cc:108

• EVP_RSA_gen() can return nullptr on failure; without checking, key_to_pem() will be invoked with a null EVP_PKEY which can lead to a crash or misleading failures. Add a REQUIRE() right after key generation so the test fails with a clear assertion.

  EVP_PKEY         *pkey = EVP_RSA_gen(2048);
  std::string const out{key_to_pem(pkey, nullptr, nullptr)};

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/iocore/net/unit_tests/test_SSLDHParams.cc:137

• The return values from X509 subject-name helpers aren’t checked. If these fail, the test can end up generating/signing a certificate missing the intended subject, which makes failures harder to diagnose.

  X509_NAME *name = X509_get_subject_name(x509);
  X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, reinterpret_cast<unsigned char const *>("ats-test"), -1, -1, 0);
  REQUIRE(X509_set_issuer_name(x509, name) == 1);

src/iocore/net/unit_tests/test_SSLDHParams.cc:133

• Several OpenSSL setters here return status / pointers that can indicate failure. Because these helpers use REQUIRE (which aborts the test on failure), it’s better to assert the return values so a failure is reported at the right call site and you don’t continue building an invalid X509 object.

This issue also appears on line 135 of the same file.

  ASN1_INTEGER_set(X509_get_serialNumber(x509), 1);
  X509_gmtime_adj(X509_getm_notBefore(x509), 0);
  X509_gmtime_adj(X509_getm_notAfter(x509), 60L * 60L * 24L * 365L);
  REQUIRE(X509_set_pubkey(x509, pkey) == 1);

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 99e8cee for the 10.2.0 release.