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(
