Skip to content

Add support for SSL error handling #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
minfrin wants to merge 11 commits into from
Closed

Add support for SSL error handling #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6c12e1f
Add support for SSL error handling
minfrin Jul 1, 2025
f7c70c9
The correct term is "baton".
minfrin Jul 1, 2025
6b6d66e
Add additional detail to the error callback description.
minfrin Jul 1, 2025
217d1dd
Add the APR status to the error callback.
minfrin Jul 1, 2025
2a9afdf
Don't log handled errors.
minfrin Jul 2, 2025
4863887
Remove unnecessary ERR_get_error() and simplify.
minfrin Jul 2, 2025
83cb75d
Do not hang indefintely when the file can't be found, handle the error.
minfrin Jul 2, 2025
329c475
Merge branch 'trunk' into sslcb
minfrin Jul 4, 2025
1c8c2fa
Add error handling to new openssl3+ support.
minfrin Jul 4, 2025
b881b44
Return a client side error when we cannot parse PKCS12 files. Don't
minfrin Jul 5, 2025
1407bdc
Put certificate failures into context, so that end users have more
minfrin Jul 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 105 additions & 26 deletions buckets/ssl_buckets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@ struct serf_ssl_context_t {
X509 *cached_cert;
EVP_PKEY *cached_cert_pw;

/* Error callback */
serf_ssl_error_cb_t error_callback;
void *error_baton;

apr_status_t pending_err;

/* Status of a fatal error, returned on subsequent encrypt or decrypt
Expand Down Expand Up @@ -353,10 +357,17 @@ detect_renegotiate(const SSL *s, int where, int ret)

static void log_ssl_error(serf_ssl_context_t *ctx)
{
unsigned long e = ERR_get_error();
serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
"SSL Error: %s\n", ERR_error_string(e, NULL));
unsigned long err;

while ((err = ERR_get_error())) {

if (err && ctx->error_callback) {
char ebuf[256];
ERR_error_string_n(err, ebuf, sizeof(ebuf));
Copy link
Member

@brainy brainy Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just above, you call ERR_error_string() to log the error, while here, you copy the string to the stack. Is this actually necessary? I'd just document to callback implementors that they have to copy the string inside the callback and call ERR_error_string() just once.

Copy link
Contributor Author

@minfrin minfrin Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging was left unchanged, but really should go.

The logging is the crutch that was supporting the lack of error handling. Libraries should definitely not be logging anything.

Copy link
Member

@brainy brainy Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with the "libraries should not be logging" sentiment. They shouldn't dump to stdout or stderr, but Serf doesn't do that -- it writes to whatever logging sink the application cares to provide, so basically integrates with the application's logging infrastructure. I've found this to be extremely useful for debugging; not everything can be done with error handling, no matter how sophisticated. Because logging by its nature leaves an audit trail for states that are not error conditions and can't even be captured when an error occurs.

In this case, I agree, we shouldn't be doing both.

ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
Copy link
Collaborator

@dsahlberg-apache-org dsahlberg-apache-org Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to use an internal char array and calling ERR_error_string_n to copy the error message to this buffer. The error_callback must copy the message to an application internal buffer anyway. Wouldn't it be enough to:

char *ebuf = ERR_error_string(err, NULL);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuff);

Copy link
Member

@gstein gstein Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. The documentation for the callback signature/compact should be that the life of the error string lasts only until the callback returns. Thus, the callback must use it immediately in some fashion, and not retain the pointer (eg. print it, or copy it).

As such, serf does not need to copy a portion of the error into a stack-based buffer.

Copy link
Contributor Author

@minfrin minfrin Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to use an internal char array and calling ERR_error_string_n to copy the error message to this buffer. The error_callback must copy the message to an application internal buffer anyway. Wouldn't it be enough to:

char *ebuf = ERR_error_string(err, NULL); ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuff);

Alas not.

ERR_error_string() uses an openssl-internal buffer that is overwritten on each call, and is not thread safe. ERR_error_string_n() fixed this.

Copy link
Collaborator

@dsahlberg-apache-org dsahlberg-apache-org Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good point. Missed that one, thanks!

Reverted to the original code form the PR in r1927348.

}

}
}

static void bio_set_data(BIO *bio, void *data)
Expand Down Expand Up @@ -1075,15 +1086,6 @@ static apr_status_t status_from_ssl_error(serf_ssl_context_t *ctx,
status = ctx->pending_err;
ctx->pending_err = APR_SUCCESS;
} else {
/*unsigned long l = ERR_peek_error();
int lib = ERR_GET_LIB(l);
int reason = ERR_GET_REASON(l);*/

/* ### Detect more specific errors?
When lib is ERR_LIB_SSL, then reason is one of the
many SSL_R_XXXX reasons in ssl.h
*/

if (SSL_in_init(ctx->ssl))
ctx->fatal_err = SERF_ERROR_SSL_SETUP_FAILED;
else
Expand All @@ -1093,6 +1095,15 @@ static apr_status_t status_from_ssl_error(serf_ssl_context_t *ctx,
log_ssl_error(ctx);
}
break;

case SSL_ERROR_WANT_X509_LOOKUP:
/* The ssl_need_client_cert() function returned -1 because an
* error occurred inside that function. The error has already
* been handled, just return the fatal error.
*/
status = ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
break;

default:
status = ctx->fatal_err = SERF_ERROR_SSL_COMM_FAILED;
log_ssl_error(ctx);
Expand Down Expand Up @@ -1593,6 +1604,7 @@ static int ssl_pass_cb(UI *ui, UI_STRING *uis)
static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
{
serf_ssl_context_t *ctx = SSL_get_app_data(ssl);
unsigned long err = 0;
#if defined(SERF_HAVE_OSSL_STORE_OPEN_EX)
STACK_OF(X509) *leaves;
STACK_OF(X509) *intermediates;
Expand Down Expand Up @@ -1657,10 +1669,14 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
store = OSSL_STORE_open_ex(cert_uri, NULL, NULL, ui_method, ctx, NULL,
NULL, NULL);
if (!store) {
int err = ERR_get_error();
serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
"OpenSSL store error (%s): %d %d\n", cert_uri,
ERR_GET_LIB(err), ERR_GET_REASON(err));

if (ctx->error_callback) {
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not open URI: %s", cert_uri);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}

break;
}

Expand All @@ -1670,6 +1686,14 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
info = OSSL_STORE_load(store);

if (!info) {

if (ctx->error_callback) {
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not read URI: %s", cert_uri);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}

break;
}

Expand Down Expand Up @@ -1716,10 +1740,12 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
OSSL_STORE_INFO_free(info);
}

/* FIXME: openssl error checking goes here */

OSSL_STORE_close(store);

if (ERR_peek_error()) {
break;
}

/* walk the leaf certificates, choose the best one */

while ((c = sk_X509_pop(leaves))) {
Expand Down Expand Up @@ -1786,6 +1812,12 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
X509_STORE_free(requests);
UI_destroy_method(ui_method);

if (ERR_peek_error()) {
log_ssl_error(ctx);

return -1;
}

/* we settled on a cert and key, cache it for later */

if (*cert && *pkey) {
Expand Down Expand Up @@ -1844,9 +1876,15 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
status = apr_file_open(&cert_file, cert_path, APR_READ, APR_OS_DEFAULT,
ctx->pool);

/* TODO: this will hang indefintely when the file can't be found. */
if (status) {
continue;
if (ctx->error_callback) {
char ebuf[1024];
apr_snprintf(ebuf, sizeof(ebuf), "could not open PKCS12: %s", cert_path);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
apr_strerror(status, ebuf, sizeof(ebuf));
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}
return -1;
}

biom = bio_meth_file_new();
Expand Down Expand Up @@ -1877,7 +1915,7 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
return 1;
}
else {
int err = ERR_get_error();
err = ERR_get_error();
ERR_clear_error();
Copy link
Collaborator

@dsahlberg-apache-org dsahlberg-apache-org Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why moving the declaration from here to the top of the function? Better keep scope limited whenever possible, just to catch accidental errors (pun intended).
(Yes it obviously should be unsigned long instead of int, so a change would be needed anyhow).

Copy link
Member

@gstein gstein Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also agree. All variables should be scoped as tightly as possible.

if (ERR_GET_LIB(err) == ERR_LIB_PKCS12 &&
ERR_GET_REASON(err) == PKCS12_R_MAC_VERIFY_FAILURE) {
Expand Down Expand Up @@ -1923,18 +1961,46 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
}
return 1;
}
else {

if (ctx->error_callback) {
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not parse PKCS12: %s", cert_path);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}

log_ssl_error(ctx);
return -1;
}
}
}
PKCS12_free(p12);
bio_meth_free(biom);
return 0;

if (ctx->error_callback) {
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "PKCS12 needs a password: %s", cert_path);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}

log_ssl_error(ctx);
return -1;
}
else {
serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
"OpenSSL cert error: %d %d\n", ERR_GET_LIB(err),
ERR_GET_REASON(err));
PKCS12_free(p12);
bio_meth_free(biom);

if (ctx->error_callback) {
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not parse PKCS12: %s", cert_path);
ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
}

log_ssl_error(ctx);
return -1;
}
}
}
Expand Down Expand Up @@ -2011,6 +2077,15 @@ void serf_ssl_server_cert_chain_callback_set(
context->server_cert_userdata = data;
}

void serf_ssl_error_cb_set(
serf_ssl_context_t *context,
serf_ssl_error_cb_t callback,
void *baton)
{
context->error_callback = callback;
context->error_baton = baton;
}

static int ssl_new_session(SSL *ssl, SSL_SESSION *session)
{
serf_ssl_context_t *ctx = SSL_get_app_data(ssl);
Expand Down Expand Up @@ -2076,6 +2151,9 @@ static serf_ssl_context_t *ssl_init_context(serf_bucket_alloc_t *allocator)
ssl_ctx->protocol_callback = NULL;
ssl_ctx->protocol_userdata = NULL;

ssl_ctx->error_callback = NULL;
ssl_ctx->error_baton = NULL;

SSL_CTX_set_verify(ssl_ctx->ctx, SSL_VERIFY_PEER,
validate_server_certificate);
SSL_CTX_set_options(ssl_ctx->ctx, SSL_OP_ALL);
Expand Down Expand Up @@ -2387,8 +2465,9 @@ apr_status_t serf_ssl_add_crl_from_file(serf_ssl_context_t *ssl_ctx,

result = X509_STORE_add_crl(store, crl);
if (!result) {
ssl_ctx->fatal_err = status = SERF_ERROR_SSL_CERT_FAILED;
log_ssl_error(ssl_ctx);
return SERF_ERROR_SSL_CERT_FAILED;
return status;
}

/* TODO: free crl when closing ssl session */
Expand Down
27 changes: 27 additions & 0 deletions serf_bucket_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,33 @@ void serf_ssl_server_cert_chain_callback_set(
serf_ssl_server_cert_chain_cb_t cert_chain_callback,
void *data);

/**
* Callback type for detailed TLS error strings. This callback will be fired
* every time the underlying crypto library encounters an error. The message
* lasts only as long as the callback, if the caller wants to set aside the
* message for later use, a copy must be made.
*
* It is possible that for a given error multiple strings will be returned
* in multiple callbacks. The caller may choose to handle all strings, or
* may choose to ignore all strings but the last most detailed one.
*/
typedef apr_status_t (*serf_ssl_error_cb_t)(
void *baton,
apr_status_t status,
const char *message);

/**
* Set a callback to return any detailed certificate error from the underlying
* cryptographic library.
*
* The callback is associated with the context, however the choice of baton
* will depend on the needs of the caller.
*/
void serf_ssl_error_cb_set(
serf_ssl_context_t *context,
serf_ssl_error_cb_t callback,
void *baton);

/**
* Use the default root CA certificates as included with the OpenSSL library.
*/
Expand Down