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( > > >