I had previously indicated that I did not like this approach. This is a
one-off that is not suitable for the project.
If we're going to create callbacks to provide additional error information,
then I think we need a mechanism that can eventually be used across the
entire library. I think that we'd pass an error structure rather just a
simple string. If the only use of that callback is for SSL, that's fine and
we can grow into it. I think the structure would look something like this:
struct serf_error_t {
apr_status_t status;
int scope;
#define SERF_ERRSCOPE_CONTEXT 0
#define SERF_ERRSCOPE_CONNECTION 1
#define SERF_ERRSCOPE_REQUEST 2
...
/* ### not sure about this. needed? */
int sub_status;
/* NOTE: survives the duration of the callback; copy as needed */
const char *message;
/* Where was this error generated? FILE is static/forever. */
const char *file;
int lineno;
}
If we're going to use a callback approach, then let's do it right. I
disagree with this one-off.
Cheers,
-g
On Sat, Jul 5, 2025 at 4:47 AM <minf...@apache.org> wrote:
> Author: minfrin
> Date: Sat Jul 5 09:47:32 2025
> New Revision: 1926972
>
> URL: http://svn.apache.org/viewvc?rev=1926972&view=rev
> Log:
> Add support for SSL error handling
>
> - Allow the registration of an optional callback using
> serf_ssl_error_cb_set().
>
> - If the callback is registered, return a fixed string describing the
> error as created by the underlying crypto library.
>
> - Handle the error when a PKCS12 file cannot be opened, remove an infinite
> loop.
>
> - Client side SSL certificate errors now cause the client side to abort the
> connection. Previously no certificate was silently sent, and the error
> was
> access denied from the server.
>
> Example:
>
> [minfrin@rocky9 subversion]$ svn info
> https://svn.example.com/svn/example/core/
> svn: E170013: Unable to connect to a repository at URL '
> https://svn.example.com/svn/example/core'
> svn: E120170: TLS: error:0308010C:digital envelope routines::unsupported
> svn: E120170: TLS: could not parse PKCS12: /home/minfrin/.my-cert.p12
>
>
> Modified:
> serf/trunk/buckets/ssl_buckets.c
> serf/trunk/serf_bucket_types.h
>
> Modified: serf/trunk/buckets/ssl_buckets.c
> URL:
> http://svn.apache.org/viewvc/serf/trunk/buckets/ssl_buckets.c?rev=1926972&r1=1926971&r2=1926972&view=diff
>
> ==============================================================================
> --- serf/trunk/buckets/ssl_buckets.c (original)
> +++ serf/trunk/buckets/ssl_buckets.c Sat Jul 5 09:47:32 2025
> @@ -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
> @@ -353,10 +357,17 @@ detect_renegotiate(const SSL *s, int whe
>
> 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));
> + ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
> + }
>
> + }
> }
>
> static void bio_set_data(BIO *bio, void *data)
> @@ -1075,15 +1086,6 @@ static apr_status_t status_from_ssl_erro
> 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
> @@ -1093,6 +1095,15 @@ static apr_status_t status_from_ssl_erro
> 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);
> @@ -1592,6 +1603,7 @@ static int ssl_pass_cb(UI *ui, UI_STRING
> 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;
> @@ -1656,10 +1668,14 @@ static int ssl_need_client_cert(SSL *ssl
> 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;
> }
>
> @@ -1669,6 +1685,14 @@ static int ssl_need_client_cert(SSL *ssl
> 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;
> }
>
> @@ -1715,10 +1739,12 @@ static int ssl_need_client_cert(SSL *ssl
> 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))) {
> @@ -1785,6 +1811,12 @@ static int ssl_need_client_cert(SSL *ssl
> 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) {
> @@ -1843,9 +1875,15 @@ static int ssl_need_client_cert(SSL *ssl
> 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();
> @@ -1876,7 +1914,7 @@ static int ssl_need_client_cert(SSL *ssl
> return 1;
> }
> else {
> - int err = ERR_get_error();
> + err = ERR_get_error();
> ERR_clear_error();
> if (ERR_GET_LIB(err) == ERR_LIB_PKCS12 &&
> ERR_GET_REASON(err) == PKCS12_R_MAC_VERIFY_FAILURE) {
> @@ -1922,18 +1960,46 @@ static int ssl_need_client_cert(SSL *ssl
> }
> 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;
> }
> }
> }
> @@ -2010,6 +2076,15 @@ void serf_ssl_server_cert_chain_callback
> 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);
> @@ -2075,6 +2150,9 @@ static serf_ssl_context_t *ssl_init_cont
> 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);
> @@ -2386,8 +2464,9 @@ apr_status_t serf_ssl_add_crl_from_file(
>
> 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 */
>
> Modified: serf/trunk/serf_bucket_types.h
> URL:
> http://svn.apache.org/viewvc/serf/trunk/serf_bucket_types.h?rev=1926972&r1=1926971&r2=1926972&view=diff
>
> ==============================================================================
> --- serf/trunk/serf_bucket_types.h (original)
> +++ serf/trunk/serf_bucket_types.h Sat Jul 5 09:47:32 2025
> @@ -687,6 +687,33 @@ void serf_ssl_server_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.
> */
> apr_status_t serf_ssl_use_default_certificates(
>
>
>
