From f739f7a075dcc2e8c76dbdb1a331049ba6c5aa2b Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Thu, 30 Oct 2025 14:47:29 +0100 Subject: [PATCH 1/4] revocation: Pass "this" parameter along CRL/OCSP verification functions As future commits will require shared state during fetch operations, a common context is required. So pass along the "this" parameter in various revocation functions. --- .../plugins/revocation/revocation_validator.c | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/libstrongswan/plugins/revocation/revocation_validator.c b/src/libstrongswan/plugins/revocation/revocation_validator.c index 2ef6fdeac..f78c26621 100644 --- a/src/libstrongswan/plugins/revocation/revocation_validator.c +++ b/src/libstrongswan/plugins/revocation/revocation_validator.c @@ -327,7 +327,8 @@ static certificate_t *get_better_ocsp(certificate_t *cand, certificate_t *best, /** * validate a x509 certificate using OCSP */ -static cert_validation_t check_ocsp(x509_t *subject, x509_t *issuer, +static cert_validation_t check_ocsp(private_revocation_validator_t *this, + x509_t *subject, x509_t *issuer, auth_cfg_t *auth, u_int timeout, certificate_t **response) { @@ -615,7 +616,8 @@ static certificate_t *get_better_crl(certificate_t *cand, certificate_t *best, /** * Find or fetch a certificate for a given crlIssuer */ -static cert_validation_t find_crl(x509_t *subject, identification_t *issuer, +static cert_validation_t find_crl(private_revocation_validator_t *this, + x509_t *subject, identification_t *issuer, crl_t *base, certificate_t **best, bool *uri_found, u_int timeout) { @@ -698,7 +700,8 @@ static bool check_issuer(certificate_t *crl, x509_t *issuer, x509_cdp_t *cdp) /** * Look for a delta CRL for a given base CRL */ -static cert_validation_t check_delta_crl(x509_t *subject, x509_t *issuer, +static cert_validation_t check_delta_crl(private_revocation_validator_t *this, + x509_t *subject, x509_t *issuer, crl_t *base, cert_validation_t base_valid, u_int timeout) { @@ -716,7 +719,7 @@ static cert_validation_t check_delta_crl(x509_t *subject, x509_t *issuer, if (chunk.len) { id = identification_create_from_encoding(ID_KEY_ID, chunk); - valid = find_crl(subject, id, base, &best, &uri, timeout); + valid = find_crl(this, subject, id, base, &best, &uri, timeout); id->destroy(id); } @@ -727,7 +730,8 @@ static cert_validation_t check_delta_crl(x509_t *subject, x509_t *issuer, { if (cdp->issuer) { - valid = find_crl(subject, cdp->issuer, base, &best, &uri, timeout); + valid = find_crl(this, subject, cdp->issuer, base, &best, &uri, + timeout); } } enumerator->destroy(enumerator); @@ -769,7 +773,8 @@ static cert_validation_t check_delta_crl(x509_t *subject, x509_t *issuer, /** * validate a x509 certificate using CRL */ -static cert_validation_t check_crl(x509_t *subject, x509_t *issuer, +static cert_validation_t check_crl(private_revocation_validator_t *this, + x509_t *subject, x509_t *issuer, auth_cfg_t *auth, u_int timeout) { cert_validation_t valid = VALIDATION_SKIPPED; @@ -786,7 +791,7 @@ static cert_validation_t check_crl(x509_t *subject, x509_t *issuer, if (chunk.len) { id = identification_create_from_encoding(ID_KEY_ID, chunk); - valid = find_crl(subject, id, NULL, &best, &uri_found, timeout); + valid = find_crl(this, subject, id, NULL, &best, &uri_found, timeout); id->destroy(id); } @@ -797,8 +802,8 @@ static cert_validation_t check_crl(x509_t *subject, x509_t *issuer, { if (cdp->issuer) { - valid = find_crl(subject, cdp->issuer, NULL, &best, &uri_found, - timeout); + valid = find_crl(this, subject, cdp->issuer, NULL, &best, + &uri_found, timeout); } } enumerator->destroy(enumerator); @@ -836,7 +841,8 @@ static cert_validation_t check_crl(x509_t *subject, x509_t *issuer, /* look for delta CRLs */ if (best && (valid == VALIDATION_GOOD || valid == VALIDATION_STALE)) { - valid = check_delta_crl(subject, issuer, (crl_t*)best, valid, timeout); + valid = check_delta_crl(this, subject, issuer, (crl_t*)best, valid, + timeout); } /* an uri was found, but no result. switch validation state to failed */ @@ -880,8 +886,8 @@ METHOD(cert_validator_t, validate_online, bool, if (enable_ocsp) { - switch (check_ocsp((x509_t*)subject, (x509_t*)issuer, auth, timeout, - NULL)) + switch (check_ocsp(this, (x509_t*)subject, (x509_t*)issuer, + auth, timeout, NULL)) { case VALIDATION_GOOD: DBG1(DBG_CFG, "certificate status is good"); @@ -910,7 +916,8 @@ METHOD(cert_validator_t, validate_online, bool, if (enable_crl) { - switch (check_crl((x509_t*)subject, (x509_t*)issuer, auth, timeout)) + switch (check_crl(this, (x509_t*)subject, (x509_t*)issuer, auth, + timeout)) { case VALIDATION_GOOD: DBG1(DBG_CFG, "certificate status is good"); @@ -964,8 +971,8 @@ METHOD (cert_validator_t, ocsp, certificate_t *, subject->get_subject(subject)); auth = auth_cfg_create(); - switch (check_ocsp((x509_t*)subject, (x509_t*)issuer, auth, timeout, - &response)) + switch (check_ocsp(this, (x509_t*)subject, (x509_t*)issuer, + auth, timeout, &response)) { case VALIDATION_GOOD: case VALIDATION_ON_HOLD: From 119dfc2c38a07fb73953faec73b39e80e9e8807f Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 29 Oct 2025 07:49:49 +0100 Subject: [PATCH 2/4] revocation: Move CRL/OCSP fetch operations to a dedicated fetcher helper Before adding stateful CRL fetching extensions, refactor CRL fetching to a helper class for better separation. While there are currently no plans to extend OCSP fetching, move it as well for consistency. --- .../plugins/revocation/Makefile.am | 1 + .../plugins/revocation/revocation_fetcher.c | 159 ++++++++++++++++++ .../plugins/revocation/revocation_fetcher.h | 70 ++++++++ .../plugins/revocation/revocation_validator.c | 129 +++----------- 4 files changed, 249 insertions(+), 110 deletions(-) create mode 100644 src/libstrongswan/plugins/revocation/revocation_fetcher.c create mode 100644 src/libstrongswan/plugins/revocation/revocation_fetcher.h diff --git a/src/libstrongswan/plugins/revocation/Makefile.am b/src/libstrongswan/plugins/revocation/Makefile.am index 9532d5f03..84a879d75 100644 --- a/src/libstrongswan/plugins/revocation/Makefile.am +++ b/src/libstrongswan/plugins/revocation/Makefile.am @@ -12,6 +12,7 @@ endif libstrongswan_revocation_la_SOURCES = \ revocation_plugin.h revocation_plugin.c \ + revocation_fetcher.h revocation_fetcher.c \ revocation_validator.h revocation_validator.c libstrongswan_revocation_la_LDFLAGS = -module -avoid-version diff --git a/src/libstrongswan/plugins/revocation/revocation_fetcher.c b/src/libstrongswan/plugins/revocation/revocation_fetcher.c new file mode 100644 index 000000000..42620d35f --- /dev/null +++ b/src/libstrongswan/plugins/revocation/revocation_fetcher.c @@ -0,0 +1,159 @@ +/* + * Copyright (C) 2025 Martin Willi + * Copyright (C) 2015-2018 Tobias Brunner + * Copyright (C) 2009-2022 Andreas Steffen + * + * Copyright (C) secunet Security Networks AG + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. See . + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "revocation_fetcher.h" + +#include +#include +#include +#include + +typedef struct private_revocation_fetcher_t private_revocation_fetcher_t; + +/** + * Private data of an revocation_fetcher_t object. + */ +struct private_revocation_fetcher_t { + + /** + * Public revocation_fetcher_t interface. + */ + revocation_fetcher_t public; +}; + +METHOD(revocation_fetcher_t, fetch_crl, certificate_t *, + private_revocation_fetcher_t *this, char *url, u_int timeout) +{ + certificate_t *crl; + chunk_t chunk = chunk_empty; + + DBG1(DBG_CFG, " fetching crl from '%s' ...", url); + if (lib->fetcher->fetch(lib->fetcher, url, &chunk, + FETCH_TIMEOUT, timeout, + FETCH_END) != SUCCESS) + { + DBG1(DBG_CFG, "crl fetching failed"); + chunk_free(&chunk); + return NULL; + } + crl = lib->creds->create(lib->creds, CRED_CERTIFICATE, CERT_X509_CRL, + BUILD_BLOB_PEM, chunk, BUILD_END); + chunk_free(&chunk); + if (!crl) + { + DBG1(DBG_CFG, "crl fetched successfully but parsing failed"); + return NULL; + } + return crl; +} + +METHOD(revocation_fetcher_t, fetch_ocsp, certificate_t*, + private_revocation_fetcher_t *this, char *url, + certificate_t *subject, certificate_t *issuer, u_int timeout) +{ + certificate_t *request, *response; + ocsp_request_t *ocsp_request; + ocsp_response_t *ocsp_response; + chunk_t send, receive = chunk_empty; + + /* TODO: requestor name, signature */ + request = lib->creds->create(lib->creds, + CRED_CERTIFICATE, CERT_X509_OCSP_REQUEST, + BUILD_CA_CERT, issuer, + BUILD_CERT, subject, BUILD_END); + if (!request) + { + DBG1(DBG_CFG, "generating ocsp request failed"); + return NULL; + } + + if (!request->get_encoding(request, CERT_ASN1_DER, &send)) + { + DBG1(DBG_CFG, "encoding ocsp request failed"); + request->destroy(request); + return NULL; + } + + DBG1(DBG_CFG, " requesting ocsp status from '%s' ...", url); + if (lib->fetcher->fetch(lib->fetcher, url, &receive, + FETCH_REQUEST_DATA, send, + FETCH_REQUEST_TYPE, "application/ocsp-request", + FETCH_TIMEOUT, timeout, + FETCH_END) != SUCCESS) + { + DBG1(DBG_CFG, "ocsp request to %s failed", url); + request->destroy(request); + chunk_free(&receive); + chunk_free(&send); + return NULL; + } + chunk_free(&send); + + response = lib->creds->create(lib->creds, + CRED_CERTIFICATE, CERT_X509_OCSP_RESPONSE, + BUILD_BLOB_ASN1_DER, receive, BUILD_END); + chunk_free(&receive); + if (!response) + { + DBG1(DBG_CFG, "parsing ocsp response failed"); + request->destroy(request); + return NULL; + } + ocsp_response = (ocsp_response_t*)response; + if (ocsp_response->get_ocsp_status(ocsp_response) != OCSP_SUCCESSFUL) + { + response->destroy(response); + request->destroy(request); + return NULL; + } + ocsp_request = (ocsp_request_t*)request; + if (ocsp_response->get_nonce(ocsp_response).len && + !chunk_equals_const(ocsp_request->get_nonce(ocsp_request), + ocsp_response->get_nonce(ocsp_response))) + { + DBG1(DBG_CFG, "nonce in ocsp response doesn't match"); + request->destroy(request); + return NULL; + } + request->destroy(request); + return response; +} + +METHOD(revocation_fetcher_t, destroy, void, + private_revocation_fetcher_t *this) +{ + free(this); +} + +/** + * See header + */ +revocation_fetcher_t *revocation_fetcher_create() +{ + private_revocation_fetcher_t *this; + + INIT(this, + .public = { + .fetch_crl = _fetch_crl, + .fetch_ocsp = _fetch_ocsp, + .destroy = _destroy, + }, + ); + + return &this->public; +} diff --git a/src/libstrongswan/plugins/revocation/revocation_fetcher.h b/src/libstrongswan/plugins/revocation/revocation_fetcher.h new file mode 100644 index 000000000..432830677 --- /dev/null +++ b/src/libstrongswan/plugins/revocation/revocation_fetcher.h @@ -0,0 +1,70 @@ +/* + * Copyright (C) 2025 Martin Willi + * + * Copyright (C) secunet Security Networks AG + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. See . + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +/** + * @defgroup revocation_fetcher revocation_fetcher + * @{ @ingroup revocation + */ + +#ifndef REVOCATION_FETCHER_H_ +#define REVOCATION_FETCHER_H_ + +#include + +typedef struct revocation_fetcher_t revocation_fetcher_t; + +/** + * Certificate fetcher performing the CRL/OCSP transfer. + */ +struct revocation_fetcher_t { + + /** + * Fetch a CRL from given URL. + * + * @param this revocation fetcher + * @param url URL to retrieve the CRL from + * @param timeout timeout in seconds for the fetch operation + * @return fetched CRL or NULL on error + */ + certificate_t *(*fetch_crl)(revocation_fetcher_t *this, char *url, + u_int timeout); + + /** + * Fetch an OCSP response from given URL. + * + * @param this revocation fetcher + * @param url URL to retrieve the OCSP response from + * @param subject subject to request OSCP status for + * @param issuer issuer of the subject + * @param timeout timeout in seconds for the fetch operation + * @return fetched OCSP response or NULL on error + */ + certificate_t *(*fetch_ocsp)(revocation_fetcher_t *this, char *url, + certificate_t *subject, certificate_t *issuer, + u_int timeout); + + /** + * Destroy a revocation_fetcher_t. + */ + void (*destroy)(revocation_fetcher_t *this); +}; + +/** + * Create a revocation_fetcher instance. + */ +revocation_fetcher_t *revocation_fetcher_create(); + +#endif /** REVOCATION_FETCHER_H_ @}*/ diff --git a/src/libstrongswan/plugins/revocation/revocation_validator.c b/src/libstrongswan/plugins/revocation/revocation_validator.c index f78c26621..584e6071e 100644 --- a/src/libstrongswan/plugins/revocation/revocation_validator.c +++ b/src/libstrongswan/plugins/revocation/revocation_validator.c @@ -23,12 +23,13 @@ #include #include #include -#include #include #include #include #include +#include "revocation_fetcher.h" + /** * Default timeout in seconds when fetching OCSP/CRL. */ @@ -46,6 +47,11 @@ struct private_revocation_validator_t { */ revocation_validator_t public; + /** + * Fetch helper for CRL/OCSP. + */ + revocation_fetcher_t *fetcher; + /** * Enable OCSP validation */ @@ -67,80 +73,6 @@ struct private_revocation_validator_t { spinlock_t *lock; }; -/** - * Do an OCSP request - */ -static certificate_t *fetch_ocsp(char *url, certificate_t *subject, - certificate_t *issuer, u_int timeout) -{ - certificate_t *request, *response; - ocsp_request_t *ocsp_request; - ocsp_response_t *ocsp_response; - chunk_t send, receive = chunk_empty; - - /* TODO: requestor name, signature */ - request = lib->creds->create(lib->creds, - CRED_CERTIFICATE, CERT_X509_OCSP_REQUEST, - BUILD_CA_CERT, issuer, - BUILD_CERT, subject, BUILD_END); - if (!request) - { - DBG1(DBG_CFG, "generating ocsp request failed"); - return NULL; - } - - if (!request->get_encoding(request, CERT_ASN1_DER, &send)) - { - DBG1(DBG_CFG, "encoding ocsp request failed"); - request->destroy(request); - return NULL; - } - - DBG1(DBG_CFG, " requesting ocsp status from '%s' ...", url); - if (lib->fetcher->fetch(lib->fetcher, url, &receive, - FETCH_REQUEST_DATA, send, - FETCH_REQUEST_TYPE, "application/ocsp-request", - FETCH_TIMEOUT, timeout, - FETCH_END) != SUCCESS) - { - DBG1(DBG_CFG, "ocsp request to %s failed", url); - request->destroy(request); - chunk_free(&receive); - chunk_free(&send); - return NULL; - } - chunk_free(&send); - - response = lib->creds->create(lib->creds, - CRED_CERTIFICATE, CERT_X509_OCSP_RESPONSE, - BUILD_BLOB_ASN1_DER, receive, BUILD_END); - chunk_free(&receive); - if (!response) - { - DBG1(DBG_CFG, "parsing ocsp response failed"); - request->destroy(request); - return NULL; - } - ocsp_response = (ocsp_response_t*)response; - if (ocsp_response->get_ocsp_status(ocsp_response) != OCSP_SUCCESSFUL) - { - response->destroy(response); - request->destroy(request); - return NULL; - } - ocsp_request = (ocsp_request_t*)request; - if (ocsp_response->get_nonce(ocsp_response).len && - !chunk_equals_const(ocsp_request->get_nonce(ocsp_request), - ocsp_response->get_nonce(ocsp_response))) - { - DBG1(DBG_CFG, "nonce in ocsp response doesn't match"); - request->destroy(request); - return NULL; - } - request->destroy(request); - return response; -} - /** * Verify OCSP response signature */ @@ -369,8 +301,9 @@ static cert_validation_t check_ocsp(private_revocation_validator_t *this, CERT_X509_OCSP_RESPONSE, keyid); while (enumerator->enumerate(enumerator, &uri)) { - current = fetch_ocsp(uri, &subject->interface, &issuer->interface, - timeout); + current = this->fetcher->fetch_ocsp(this->fetcher, uri, + &subject->interface, + &issuer->interface, timeout); if (current) { best = get_better_ocsp(current, best, subject, issuer, @@ -392,8 +325,9 @@ static cert_validation_t check_ocsp(private_revocation_validator_t *this, enumerator = subject->create_ocsp_uri_enumerator(subject); while (enumerator->enumerate(enumerator, &uri)) { - current = fetch_ocsp(uri, &subject->interface, &issuer->interface, - timeout); + current = this->fetcher->fetch_ocsp(this->fetcher, uri, + &subject->interface, + &issuer->interface, timeout); if (current) { best = get_better_ocsp(current, best, subject, issuer, @@ -428,34 +362,6 @@ static cert_validation_t check_ocsp(private_revocation_validator_t *this, return valid; } -/** - * fetch a CRL from an URL - */ -static certificate_t* fetch_crl(char *url, u_int timeout) -{ - certificate_t *crl; - chunk_t chunk = chunk_empty; - - DBG1(DBG_CFG, " fetching crl from '%s' ...", url); - if (lib->fetcher->fetch(lib->fetcher, url, &chunk, - FETCH_TIMEOUT, timeout, - FETCH_END) != SUCCESS) - { - DBG1(DBG_CFG, "crl fetching failed"); - chunk_free(&chunk); - return NULL; - } - crl = lib->creds->create(lib->creds, CRED_CERTIFICATE, CERT_X509_CRL, - BUILD_BLOB_PEM, chunk, BUILD_END); - chunk_free(&chunk); - if (!crl) - { - DBG1(DBG_CFG, "crl fetched successfully but parsing failed"); - return NULL; - } - return crl; -} - /** * check the signature of an CRL */ @@ -649,7 +555,7 @@ static cert_validation_t find_crl(private_revocation_validator_t *this, while (enumerator->enumerate(enumerator, &uri)) { *uri_found = TRUE; - current = fetch_crl(uri, timeout); + current = this->fetcher->fetch_crl(this->fetcher, uri, timeout); if (current) { if (!current->has_issuer(current, issuer)) @@ -741,7 +647,7 @@ static cert_validation_t check_delta_crl(private_revocation_validator_t *this, while (valid != VALIDATION_GOOD && valid != VALIDATION_REVOKED && enumerator->enumerate(enumerator, &cdp)) { - current = fetch_crl(cdp->uri, timeout); + current = this->fetcher->fetch_crl(this->fetcher, cdp->uri, timeout); if (current) { if (!check_issuer(current, issuer, cdp)) @@ -815,7 +721,8 @@ static cert_validation_t check_crl(private_revocation_validator_t *this, while (enumerator->enumerate(enumerator, &cdp)) { uri_found = TRUE; - current = fetch_crl(cdp->uri, timeout); + current = this->fetcher->fetch_crl(this->fetcher, cdp->uri, + timeout); if (current) { if (!check_issuer(current, issuer, cdp)) @@ -1023,6 +930,7 @@ METHOD(revocation_validator_t, reload, void, METHOD(revocation_validator_t, destroy, void, private_revocation_validator_t *this) { + this->fetcher->destroy(this->fetcher); this->lock->destroy(this->lock); free(this); } @@ -1041,6 +949,7 @@ revocation_validator_t *revocation_validator_create() .reload = _reload, .destroy = _destroy, }, + .fetcher = revocation_fetcher_create(), .lock = spinlock_create(), ); From 330a7d1963032d03b8eae32e86346b99dc826b26 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 29 Oct 2025 09:48:31 +0100 Subject: [PATCH 3/4] revocation: Synchronize CRL fetches of multiple threads to the same URL When handling many connection attempts from peers using the same CA, a slow or non-responsive CRL distribution point can lead to concurrent fetches of the same CRL by multiple threads. This is not only inefficient, but results in all threads blocking for the full fetch timeout, potentially blocking all threads in the pool. As a first step, synchronize CRL fetches using a global mutex and a per-URL condvar, so threads can wait for the CRL if another is already fetching it. This reduces the number of useless concurrent CRL fetches, and allows threads joining the party late to get blocked only until the first fetch completes or times out. The URL entry is preserved in the hashtable after completing the fetch. This will allow subsequent optimizations to store the last fetch result and act accordingly. The CRL itself is not, as CRLs can be rather large and caching them can be done using existing mechanisms controlled via corresponding options. --- .../plugins/revocation/revocation_fetcher.c | 135 +++++++++++++++++- 1 file changed, 133 insertions(+), 2 deletions(-) diff --git a/src/libstrongswan/plugins/revocation/revocation_fetcher.c b/src/libstrongswan/plugins/revocation/revocation_fetcher.c index 42620d35f..8d0cd602b 100644 --- a/src/libstrongswan/plugins/revocation/revocation_fetcher.c +++ b/src/libstrongswan/plugins/revocation/revocation_fetcher.c @@ -19,6 +19,9 @@ #include "revocation_fetcher.h" #include +#include +#include +#include #include #include #include @@ -34,10 +37,51 @@ struct private_revocation_fetcher_t { * Public revocation_fetcher_t interface. */ revocation_fetcher_t public; + + /** + * Mutex to synchronize CRL fetches + */ + mutex_t *mutex; + + /** + * Active/completed/failed CRL fetches, crl_fetch_t. + */ + hashtable_t *crls; }; -METHOD(revocation_fetcher_t, fetch_crl, certificate_t *, - private_revocation_fetcher_t *this, char *url, u_int timeout) +typedef struct crl_fetch_t crl_fetch_t; + +/** + * Represents an active/completed/failed CRL fetch. + */ +struct crl_fetch_t { + + /** + * URL of the CRL. + */ + char *url; + + /** + * Condition variable to signal completion of the fetch. + */ + condvar_t *condvar; + + /** + * Number of threads fetching this CRL. + */ + u_int fetchers; + + /** + * CRL received in the currently active fetch. + */ + certificate_t *crl; +}; + +/** + * Perform the actual CRL fetch from the given URL. + */ +static certificate_t *do_crl_fetch(private_revocation_fetcher_t *this, + char *url, u_int timeout) { certificate_t *crl; chunk_t chunk = chunk_empty; @@ -62,6 +106,82 @@ METHOD(revocation_fetcher_t, fetch_crl, certificate_t *, return crl; } +/** + * Start a new CRL fetch and signal completion to waiting threads. + */ +static certificate_t *start_crl_fetch(private_revocation_fetcher_t *this, + crl_fetch_t *fetch, u_int timeout) +{ + certificate_t *crl; + + fetch->fetchers++; + this->mutex->unlock(this->mutex); + crl = do_crl_fetch(this, fetch->url, timeout); + this->mutex->lock(this->mutex); + fetch->crl = crl; + while (fetch->fetchers > 1) + { + fetch->condvar->signal(fetch->condvar); + fetch->condvar->wait(fetch->condvar, this->mutex); + } + fetch->fetchers--; + fetch->crl = NULL; + return crl; +} + +/** + * Wait for a CRL fetch performed by another thread to complete. + */ +static certificate_t *wait_for_crl(private_revocation_fetcher_t *this, + crl_fetch_t *fetch) +{ + certificate_t *crl = NULL; + + DBG1(DBG_CFG, " waiting for crl fetch from '%s' ...", fetch->url); + if (fetch->crl) + { + /* fetch is already complete, no need to wait */ + return fetch->crl->get_ref(fetch->crl); + } + fetch->fetchers++; + fetch->condvar->wait(fetch->condvar, this->mutex); + fetch->fetchers--; + if (fetch->crl) + { + crl = fetch->crl->get_ref(fetch->crl); + } + fetch->condvar->signal(fetch->condvar); + return crl; +} + +METHOD(revocation_fetcher_t, fetch_crl, certificate_t*, + private_revocation_fetcher_t *this, char *url, u_int timeout) +{ + certificate_t *crl; + crl_fetch_t *fetch; + + this->mutex->lock(this->mutex); + fetch = this->crls->get(this->crls, url); + if (!fetch) + { + INIT(fetch, + .url = strdup(url), + .condvar = condvar_create(CONDVAR_TYPE_DEFAULT), + ); + this->crls->put(this->crls, fetch->url, fetch); + } + if (fetch->fetchers) + { + crl = wait_for_crl(this, fetch); + } + else + { + crl = start_crl_fetch(this, fetch, timeout); + } + this->mutex->unlock(this->mutex); + return crl; +} + METHOD(revocation_fetcher_t, fetch_ocsp, certificate_t*, private_revocation_fetcher_t *this, char *url, certificate_t *subject, certificate_t *issuer, u_int timeout) @@ -134,9 +254,18 @@ METHOD(revocation_fetcher_t, fetch_ocsp, certificate_t*, return response; } +CALLBACK(crl_fetch_destroy, void, crl_fetch_t *fetch, const void *key) +{ + fetch->condvar->destroy(fetch->condvar); + free(fetch->url); + free(fetch); +} + METHOD(revocation_fetcher_t, destroy, void, private_revocation_fetcher_t *this) { + this->crls->destroy_function(this->crls, crl_fetch_destroy); + this->mutex->destroy(this->mutex); free(this); } @@ -153,6 +282,8 @@ revocation_fetcher_t *revocation_fetcher_create() .fetch_ocsp = _fetch_ocsp, .destroy = _destroy, }, + .mutex = mutex_create(MUTEX_TYPE_DEFAULT), + .crls = hashtable_create(hashtable_hash_str, hashtable_equals_str, 8), ); return &this->public; From df6977d4cf6e0953cc7b2a6936eb28ef3ba61f89 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 29 Oct 2025 10:35:10 +0100 Subject: [PATCH 4/4] revocation: Block only one thread per URL after a previous CRL fetch failed If a CRL server is unresponsive, all threads trying to fetch this CRL will block execution. If a recent previous attempt to fetch the CRL failed, it is likely that it will fail again. While it makes sense to retry fetching the CRL on demand with one thread, it hardly does to block additional threads while the first one is blocked during the fetch. So remember the timestamp of the last CRL fetch failure per URL, and do not block more than one thread in the CRL fetch for some time. This time is a multiple of the configured fetch timeout, so that it works well for any configured value. With the default configuration, a failing CRL fetch will impact concurrent CRL fetches for the same URL for 30s. --- .../plugins/revocation/revocation_fetcher.c | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/libstrongswan/plugins/revocation/revocation_fetcher.c b/src/libstrongswan/plugins/revocation/revocation_fetcher.c index 8d0cd602b..36c74921e 100644 --- a/src/libstrongswan/plugins/revocation/revocation_fetcher.c +++ b/src/libstrongswan/plugins/revocation/revocation_fetcher.c @@ -26,6 +26,9 @@ #include #include +/* number of fetch timeouts to degrade a CRL fetch after a failure */ +#define CRL_DEGRADATION_TIMES 3 + typedef struct private_revocation_fetcher_t private_revocation_fetcher_t; /** @@ -71,6 +74,11 @@ struct crl_fetch_t { */ u_int fetchers; + /** + * Has the previous fetch failed, until when is this URL degraded? + */ + time_t failing; + /** * CRL received in the currently active fetch. */ @@ -119,6 +127,14 @@ static certificate_t *start_crl_fetch(private_revocation_fetcher_t *this, crl = do_crl_fetch(this, fetch->url, timeout); this->mutex->lock(this->mutex); fetch->crl = crl; + if (crl) + { + fetch->failing = 0; + } + else + { + fetch->failing = time_monotonic(NULL) + timeout * CRL_DEGRADATION_TIMES; + } while (fetch->fetchers > 1) { fetch->condvar->signal(fetch->condvar); @@ -137,6 +153,12 @@ static certificate_t *wait_for_crl(private_revocation_fetcher_t *this, { certificate_t *crl = NULL; + if (fetch->failing && fetch->failing > time_monotonic(NULL)) + { + DBG1(DBG_CFG, " crl fetch from '%s' recently failed, skipping", + fetch->url); + return NULL; + } DBG1(DBG_CFG, " waiting for crl fetch from '%s' ...", fetch->url); if (fetch->crl) {