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

Reply via email to