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 1 commit
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
70 changes: 53 additions & 17 deletions buckets/ssl_buckets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,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_userdata;

apr_status_t pending_err;

/* Status of a fatal error, returned on subsequent encrypt or decrypt
Expand Down Expand Up @@ -334,10 +338,20 @@ 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())) {

serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
"SSL Error: %s\n", ERR_error_string(err, NULL));

if (err && ctx->error_callback) {
char ebuf[256];
ERR_error_string_n(err, ebuf, sizeof(ebuf));
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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_userdata, ebuf);
}

}
}

static void bio_set_data(BIO *bio, void *data)
Expand Down Expand Up @@ -1056,15 +1070,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 Down Expand Up @@ -1536,6 +1541,7 @@ static apr_status_t init_ssl_libraries(void)
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;
apr_status_t status;

serf__log(LOGLVL_DEBUG, LOGCOMP_SSL, __FILE__, ctx->config,
Expand Down Expand Up @@ -1606,7 +1612,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
Copy Markdown
Collaborator

@d-sahlberg d-sahlberg 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
Copy Markdown
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 @@ -1652,22 +1658,40 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, EVP_PKEY **pkey)
}
return 1;
}
else {
err = ERR_get_error();
ERR_clear_error();
Copy link
Copy Markdown
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.

You don't pass err to log_ssl_error() and clear it here; doesn't that mean that nothing will happen? Why even call ERR_get_error() here, given that it has no effect?

Copy link
Copy Markdown
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.

This was from before we used log_ssl_error(), I've simplified this.


goto error;
}
}
}
PKCS12_free(p12);
bio_meth_free(biom);
return 0;

goto error;
}
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);

goto error;
}
}
}

error:

serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
"OpenSSL cert error: %d %d\n", ERR_GET_LIB(err),
ERR_GET_REASON(err));

if (err && ctx->error_callback) {
char ebuf[256];
ERR_error_string_n(err, ebuf, sizeof(ebuf));
ctx->error_callback(ctx->error_userdata, ebuf);
}

return 0;
}

Expand Down Expand Up @@ -1724,6 +1748,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 *data)
{
context->error_callback = callback;
context->error_userdata = data;
}

static int ssl_new_session(SSL *ssl, SSL_SESSION *session)
{
serf_ssl_context_t *ctx = SSL_get_app_data(ssl);
Expand Down Expand Up @@ -1788,6 +1821,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_userdata = NULL;
Copy link
Copy Markdown
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.

We call those batons, so: error_baton. Hmph, should do the same for protocol_userdata ⟶ protocol_baton.

Copy link
Copy Markdown
Contributor Author

@minfrin minfrin Jul 1, 2025

Choose a reason for hiding this comment

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

It has been made so.


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
16 changes: 16 additions & 0 deletions serf_bucket_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,22 @@ 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.
*/
typedef apr_status_t (*serf_ssl_error_cb_t)(
void *data,
Copy link
Copy Markdown
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.

Again: void *baton.

Copy link
Copy Markdown
Contributor Author

@minfrin minfrin Jul 1, 2025

Choose a reason for hiding this comment

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

Done.

const char *message);

/**
* Set a callback to return any detailed certificate error from the underlying
* cryptographic library..
*/
void serf_ssl_error_cb_set(
serf_ssl_context_t *context,
serf_ssl_error_cb_t callback,
void *data);
Copy link
Copy Markdown
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.

And here, too.

Copy link
Copy Markdown
Contributor Author

@minfrin minfrin Jul 1, 2025

Choose a reason for hiding this comment

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

Done.


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