[SSL] Report OpenSSL error when certificate chain can't be read (#5556)

src/rdkafka_cert.c

@@ -452,6 +452,10 @@ static rd_kafka_cert_t *rd_kafka_cert_new(const rd_kafka_conf_t *conf,
         if (p12)
                 PKCS12_free(p12);
 
+        /* The reason has been copied to errstr, don't leave it on the queue
+         * where a later unrelated failure would report it again. */
+        ERR_clear_error();
+
         return NULL;
 }
 #endif /* WITH_SSL */

src/rdkafka_ssl.c

@@ -769,11 +769,15 @@ static EVP_PKEY *rd_kafka_ssl_PKEY_from_string(rd_kafka_t *rk,
 /**
  * Read a PEM formatted cert chain from BIO \p in into \p chainp .
  *
- * @param rk rdkafka instance.
  * @param in BIO to read from.
  * @param chainp Stack to push the certificates to.
+ * @param password_cb Password callback for encrypted certificates.
+ * @param password_cb_opaque Opaque passed to \p password_cb .
  *
  * @return 0 on success, -1 on error.
+ *
+ * @remark On error the OpenSSL error queue is left intact for the caller to
+ *         report the underlying reason.
  */
 int rd_kafka_ssl_read_cert_chain_from_BIO(BIO *in,
                                           STACK_OF(X509) * chainp,
@@ -800,14 +804,16 @@ int rd_kafka_ssl_read_cert_chain_from_BIO(BIO *in,
                         break;
                 }
         }
-        /* When the while loop ends, it's usually just EOF. */
+        /* When the while loop ends, it's usually just EOF.
+         * Anything else is a real error and is left on the error queue. */
         err = ERR_peek_last_error();
         if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
-            ERR_GET_REASON(err) == PEM_R_NO_START_LINE)
+            ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
                 ret = 0;
-        else
+                ERR_clear_error();
+        } else {
                 ret = -1; /* some real error */
-        ERR_clear_error();
+        }
 end:
         return ret;
 }
@@ -818,21 +824,27 @@ int rd_kafka_ssl_read_cert_chain_from_BIO(BIO *in,
  *
  * @param str Input PEM string, nul-terminated.
  * @param chainp Stack to push the certificates to.
+ * @param reasonp Set to a static string identifying what could not be read,
+ *                when NULL is returned.
  *
  * @returns a new X509 on success or NULL on error.
  *
- * @remark When NULL is returned the chainp stack is not modified.
+ * @remark When NULL is returned the caller is still responsible for freeing
+ *         any certificate that was pushed to the chainp stack before the
+ *         failing one.
  */
 static X509 *rd_kafka_ssl_X509_from_string(rd_kafka_t *rk,
                                            const char *str,
-                                           STACK_OF(X509) * chainp) {
+                                           STACK_OF(X509) * chainp,
+                                           const char **reasonp) {
         BIO *bio = BIO_new_mem_buf((void *)str, -1);
         X509 *x509;
 
         x509 =
             PEM_read_bio_X509(bio, NULL, rd_kafka_transport_ssl_passwd_cb, rk);
 
         if (!x509) {
+                *reasonp = "not in PEM format?";
                 BIO_free(bio);
                 return NULL;
         }
@@ -846,6 +858,7 @@ static X509 *rd_kafka_ssl_X509_from_string(rd_kafka_t *rk,
                 rd_kafka_log(rk, LOG_WARNING, "SSL",
                              "Failed to read certificate chain from PEM. "
                              "Returning NULL certificate too.");
+                *reasonp = "error reading certificate chain";
                 X509_free(x509);
                 BIO_free(bio);
                 return NULL;
@@ -1455,6 +1468,7 @@ static int rd_kafka_ssl_set_certs(rd_kafka_t *rk,
 
         if (rk->rk_conf.ssl.cert_pem) {
                 X509 *x509;
+                const char *reason = NULL;
                 STACK_OF(X509) *ca = sk_X509_new_null();
                 if (!ca) {
                         rd_assert(!*"sk_X509_new_null() allocation failed");
@@ -1464,11 +1478,10 @@ static int rd_kafka_ssl_set_certs(rd_kafka_t *rk,
                              "Loading public key from string");
 
                 x509 = rd_kafka_ssl_X509_from_string(
-                    rk, rk->rk_conf.ssl.cert_pem, ca);
+                    rk, rk->rk_conf.ssl.cert_pem, ca, &reason);
                 if (!x509) {
                         rd_snprintf(errstr, errstr_size,
-                                    "ssl.certificate.pem failed: "
-                                    "not in PEM format?: ");
+                                    "ssl.certificate.pem failed: %s: ", reason);
                         sk_X509_pop_free(ca, X509_free);
                         return -1;
                 }

tests/0097-ssl_verify.cpp

@@ -544,6 +544,134 @@ static void do_test_bad_calls() {
   Test::Say("Producer creation failed expectedly: " + errstr + "\n");
 }
 
+
+/* Self-signed certificate valid until 2126. Only ever parsed, never used
+ * for a handshake. */
+static const std::string valid_certificate_pem =
+    "-----BEGIN CERTIFICATE-----\n"
+    "MIIDITCCAgmgAwIBAgIUbC4ibhU9bHAgjEcO7z3D34wrKlYwDQYJKoZIhvcNAQEL\n"
+    "BQAwHzEdMBsGA1UEAwwUbGlicmRrYWZrYS10ZXN0LTAwOTcwIBcNMjYwODA3MTIw\n"
+    "ODQ3WhgPMjEyNjA3MTQxMjA4NDdaMB8xHTAbBgNVBAMMFGxpYnJka2Fma2EtdGVz\n"
+    "dC0wMDk3MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnW50fTvi7yLy\n"
+    "lzvruQrD7qQsHfSRf7iD6Ihta9hkmXziLgbVMB9WSCOMOKQb9qLxxxolIl8cpC/j\n"
+    "QiOydK3UTimXkK4j4bVkNURa/EsQb+n3gqdxUHb9GlZOERTZNpZX3xvqXIcR4gF4\n"
+    "4SC9QcV88oMD3e0OlPH75JuiT41NbYNiBUIQ6hm/UD5l1yxuYotMTg5py16F5KxC\n"
+    "OA+QBvjXIy768R5nru0ZH+r0lbmlcvE0wojqFWcjyhHz/i0gT7lzjnoV30lsh5es\n"
+    "ksNorsZIAKYqGGSju36ckQm0ApD0DLWUwP1us2lCWcaF9fvOFlapnUwreAewrG8Z\n"
+    "5gh88Z5EgQIDAQABo1MwUTAdBgNVHQ4EFgQUrsO/5SSiOwxvquORFEBqhmVsRpcw\n"
+    "HwYDVR0jBBgwFoAUrsO/5SSiOwxvquORFEBqhmVsRpcwDwYDVR0TAQH/BAUwAwEB\n"
+    "/zANBgkqhkiG9w0BAQsFAAOCAQEAeNXkpmzf7ecN52c7ME8DTmpS6NUlMxSwRXJl\n"
+    "87KOAqOB/K/YfYlIdDMqpwqFb9axa498sIhGAtMjKFGr8YfajupUBjK3EUIGIgWm\n"
+    "aXErE7PQXwuGubtxgasFVYIsK8+w1zuUVgm76PJpfeHGmPDLJx2T0fNJbYgfmyPH\n"
+    "/L38xGpRLdP5mv4JJ/zM8R8MkJXbitwER0Xszzv0gaB/Xf/sg4oRLzQ85pku5ted\n"
+    "9hmclXmZyUOTJmrY7Sf9CQEgut89RDnpWlmRHzz6o4aHZt25j3Cm04aZ9oYEEgc9\n"
+    "bWnDPoD0X7geanPhHiJvbAEJpqQ2fUuZa8AwVcVQYIRX2tcZew==\n"
+    "-----END CERTIFICATE-----\n";
+
+/* A well-formed PEM block whose payload does not decode to an X.509
+ * certificate. */
+static const std::string malformed_certificate_pem =
+    "-----BEGIN CERTIFICATE-----\n"
+    "AAAA\n"
+    "-----END CERTIFICATE-----\n";
+
+
+/**
+ * @brief A trailing certificate block that can't be parsed makes the chain
+ *        unusable and is rejected. That rejection is intentional: the client
+ *        certificate chain is sent to the broker, so it can't be silently
+ *        truncated. The error must name the underlying OpenSSL reason
+ *        rather than "No further error information available".
+ */
+static void do_test_cert_pem_chain_error_reporting() {
+  SUB_TEST();
+
+  {
+    RdKafka::Conf *conf = RdKafka::Conf::create(RdKafka::Conf::CONF_GLOBAL);
+    std::string errstr;
+
+    if (conf->set("security.protocol", "SSL", errstr))
+      Test::Fail(errstr);
+    if (conf->set("ssl.certificate.pem", valid_certificate_pem, errstr))
+      Test::Fail(errstr);
+
+    RdKafka::Producer *producer = RdKafka::Producer::create(conf, errstr);
+    delete conf;
+    if (!producer)
+      Test::Fail(
+          "Expected producer creation to succeed with a valid "
+          "ssl.certificate.pem, got: " +
+          errstr);
+    Test::Say("Valid ssl.certificate.pem accepted (as expected)\n");
+    delete producer;
+  }
+
+  {
+    RdKafka::Conf *conf = RdKafka::Conf::create(RdKafka::Conf::CONF_GLOBAL);
+    std::string errstr;
+
+    if (conf->set("security.protocol", "SSL", errstr))
+      Test::Fail(errstr);
+    if (conf->set("ssl.certificate.pem",
+                  valid_certificate_pem + malformed_certificate_pem, errstr))
+      Test::Fail(errstr);
+
+    RdKafka::Producer *producer = RdKafka::Producer::create(conf, errstr);
+    delete conf;
+    if (producer) {
+      delete producer;
+      Test::Fail(
+          "Expected producer creation to fail with a malformed "
+          "certificate chain in ssl.certificate.pem");
+    }
+
+    Test::Say("Malformed certificate chain rejected (as expected): " + errstr +
+              "\n");
+
+    if (errstr.find("No further error information available") !=
+        std::string::npos)
+      Test::Fail("Expected the underlying OpenSSL error to be reported, not: " +
+                 errstr);
+
+    if (errstr.find("error reading certificate chain") == std::string::npos)
+      Test::Fail(
+          "Expected the error to identify the certificate chain as the "
+          "failing part, not: " +
+          errstr);
+
+    /* The error text differs between OpenSSL versions, only its
+     * "error:<code>:<lib>:..." prefix is checked. */
+    if (errstr.find("error:") == std::string::npos)
+      Test::Fail("Expected an OpenSSL error string in: " + errstr);
+  }
+
+  /* Same input through the setter API, which uses a different error path. */
+  {
+    RdKafka::Conf *conf = RdKafka::Conf::create(RdKafka::Conf::CONF_GLOBAL);
+    std::string errstr;
+    std::string pem = valid_certificate_pem + malformed_certificate_pem;
+
+    if (conf->set_ssl_cert(RdKafka::CERT_PUBLIC_KEY, RdKafka::CERT_ENC_PEM,
+                           (void *)pem.data(), pem.size(),
+                           errstr) == RdKafka::Conf::CONF_OK)
+      Test::Fail(
+          "Expected set_ssl_cert() to fail with a malformed "
+          "certificate chain");
+
+    Test::Say("set_ssl_cert() rejected malformed chain (as expected): " +
+              errstr + "\n");
+
+    /* This path appends the reason after ": ", so an empty reason leaves
+     * the message ending in ": ". */
+    if (errstr.size() < 2 || errstr.compare(errstr.size() - 2, 2, ": ") == 0)
+      Test::Fail("Expected an OpenSSL error to be appended to: " + errstr);
+
+    delete conf;
+  }
+
+  SUB_TEST_PASS();
+}
+
 }  // namespace TestSSLVerify
 
 using namespace TestSSLVerify;
@@ -669,6 +797,8 @@ int main_0097_ssl_verify_local(int argc, char **argv) {
                 " (as expected): " + errstr + "\n");
   }
 
+  do_test_cert_pem_chain_error_reporting();
+
   return 0;
 }
 }