diff --git a/src/rdkafka_cert.c b/src/rdkafka_cert.c index e4393c3288..cebfed3737 100644 --- a/src/rdkafka_cert.c +++ b/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 */ diff --git a/src/rdkafka_ssl.c b/src/rdkafka_ssl.c index 35c1fac7f8..72b928debf 100644 --- a/src/rdkafka_ssl.c +++ b/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,14 +824,19 @@ 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; @@ -833,6 +844,7 @@ static X509 *rd_kafka_ssl_X509_from_string(rd_kafka_t *rk, 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; } diff --git a/tests/0097-ssl_verify.cpp b/tests/0097-ssl_verify.cpp index 64691dec6c..f225dbf2dc 100644 --- a/tests/0097-ssl_verify.cpp +++ b/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:::..." 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; } }