[SSL] Report OpenSSL error when certificate chain can't be read (#5556) - #5561
Open
Ankith L (Ankith-Confluent) wants to merge 1 commit into
Open
[SSL] Report OpenSSL error when certificate chain can't be read (#5556)#5561Ankith L (Ankith-Confluent) wants to merge 1 commit into
Ankith L (Ankith-Confluent) wants to merge 1 commit into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Copilot started reviewing on behalf of
Ankith L (Ankith-Confluent)
August 7, 2026 12:47
View session
There was a problem hiding this comment.
Pull request overview
This PR improves SSL configuration error reporting when parsing PEM-encoded client certificates/chains, ensuring OpenSSL’s underlying parse errors remain available to be surfaced to the user, and adds regression coverage to prevent the error-queue from being inadvertently cleared on failure paths.
Changes:
- Preserve OpenSSL error-queue contents on certificate-chain parse failures (only clear on success / intentional EOF condition).
- Differentiate leaf-certificate vs trailing-chain parse failures so the reported message identifies the failing part.
- Add a focused regression sub-test covering valid cert PEM, malformed trailing chain block via
ssl.certificate.pem, and the same input viaset_ssl_cert().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/0097-ssl_verify.cpp | Adds a new local sub-test to validate improved error reporting for malformed PEM certificate chains and setter API behavior. |
| src/rdkafka_ssl.c | Adjusts cert-chain parsing to preserve OpenSSL errors on failure and improves failure-context messaging (leaf vs chain). |
| src/rdkafka_cert.c | Clears the OpenSSL error queue on rd_kafka_cert_new() failure after copying the reason into errstr to avoid stale error reuse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
846
to
850
| if (!x509) { | ||
| *reasonp = "not in PEM format?"; | ||
| BIO_free(bio); | ||
| return NULL; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5556
A PEM in
ssl.certificate.pemwhose leaf certificate parses but whosetrailing block does not is rejected with an error that names neither the
reason nor the part that failed:
Both halves of that message are unhelpful. The leaf was in PEM format —
it parsed fine — and the OpenSSL reason is reported as unavailable even
though OpenSSL did produce one.
Root cause
rd_kafka_ssl_read_cert_chain_from_BIO()calledERR_clear_error()unconditionally, including on the failure path. By the time the caller
reached
rd_kafka_ssl_error()the queue was empty, so the three ASN.1errors describing the failure had already been discarded.
The
not in PEM format?text was used for both a leaf failure and a chainfailure, so it pointed at the wrong part of the input.
The same applied to a PEM public key passed to
rd_kafka_conf_set_ssl_cert(), which reads the chain through the samefunction and reported an empty reason.
Changes
survives for the caller to report.
part that actually failed.
rd_kafka_cert_new()'s failure label. That pathonly peeks at the queue, so preserving errors would otherwise leave them
behind to be reported again by a later, unrelated failure.
The error now reads:
Not changed
Rejecting the certificate is intentional and stays. Since 2.8.0 the client
certificate chain is sent to the broker, so a chain that cannot be parsed
cannot be silently truncated — the leaf alone would produce an incomplete
chain and move the failure to the TLS handshake.
This also keeps the in-memory path consistent with the file-based one:
SSL_CTX_use_certificate_chain_file(), used byssl.certificate.location,rejects the same input with the same ASN.1 errors.
Testing
New sub-test in
0097_ssl_verify_local(no broker required) covering avalid certificate, a malformed trailing block via
ssl.certificate.pem,and the same input via
set_ssl_cert(). It fails on master with the exactmessage from the issue and passes with this change.