reload CRL only if file was modified
PATCH v2 1/1 reload CRL only if file was modified
In order to prevent annoying delays upon client connection, reload the CRL file only if it was modified since the last reload operation.
If not, keep on using the already stored CRL.
This change will boost client connection time in instances where the CRL file is quite large (dropping from several seconds to few milliseconds).
---
Tested on linux by using my VM.
No test was performed on Windows* (compiled-only).
Note: the check "!(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))" may not always work as expected. The user may forge a wrong, but still compliant, configuration that would fool this test. This issue exists even before applying this patch.
Changes since v1:
- move tls_ctx_reload_crl() before any invocation
- add doxygen-doc for tls_ctx_reload_crl()
- add check against CRL size file
- simplify tls_ctx_reload_crl() by removing reload argument
src/openvpn/ssl.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- src/openvpn/ssl_backend.h | 2 +- src/openvpn/ssl_mbedtls.c | 2 +- src/openvpn/ssl_mbedtls.h | 2 ++ src/openvpn/ssl_openssl.c | 2 +- src/openvpn/ssl_openssl.h | 2 ++ 6 files changed, 54 insertions(+), 4 deletions(-)
src/openvpn/ssl.c +47 -1
@@ -512,6 +512,49 @@ tls_version_parse(const char *vstr, const char *extra)return TLS_VER_BAD ; } +/** +* Load (or possibly reload) the CRL file into the SSL context. +* No reload is performed under the following conditions: +* - the CRL file was passed inline +* - the CRL file was not modified since the last (re)load +* +* @param ssl_ctx The TLS context to use when reloading the CRL +* @param crl_file The file name to load the CRL from, or +* "[[INLINE]]" in the case of inline files. +* @param crl_inline A string containing the CRL +*/ +static void +tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +const char *crl_file_inline) +{ +/* if something goes wrong with stat(), we'll store 0 as mtime */ +platform_stat_t crl_stat = {0 }; + +/* +* an inline CRL can't change at runtime, therefore there is no need to +* reload it. It will be reloaded upon config change + SIGHUP. +* Use always '1' as dummy timestamp in this case: it will trigger the +* first load, but will prevent any future reload. +*/ +if (crl_file_inline) + crl_stat.st_mtime =1 ; +else +platform_stat (crl_file, &crl_stat); + +/* +* Store the CRL if this is the first time or if the file was changed since +* the last load. +* Note: Windows does not support tv_nsec. +*/ +if ((ssl_ctx->crl_last_size == crl_stat.st_size) && + (ssl_ctx->crl_last_mtime.tv_sec == crl_stat.st_mtime)) +return ; + + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; + ssl_ctx->crl_last_size = crl_stat.st_size; +backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); +} +/* * Initialize SSL context. * All files are in PEM format.
@@ -2581,7 +2624,10 @@ tls_process (struct tls_multi *multi, ks->state =S_START ; state_change = true; -/* Reload the CRL before TLS negotiation */ +/* +* Attempt CRL reload before TLS negotiation. Won't be performed if +* the file was not modified since the last reload +*/ if (session->opt->crl_file && !(session->opt->ssl_flags &SSLF_CRL_VERIFY_DIR )) {
src/openvpn/ssl_backend.h +1 -1
@@ -353,7 +353,7 @@ void key_state_ssl_free(struct key_state_ssl *ks_ssl);* "[[INLINE]]" in the case of inline files. * @param crl_inline A string containing the CRL */ -void tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx, +void backend_tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file,const char *crl_inline);/**
src/openvpn/ssl_mbedtls.c +1 -1
@@ -771,7 +771,7 @@ static void tls_version_to_major_minor(int tls_ver, int *major, int *minor) { }void -tls_ctx_reload_crl (struct tls_root_ctx *ctx,const char *crl_file, +backend_tls_ctx_reload_crl (struct tls_root_ctx *ctx,const char *crl_file,const char *crl_inline) {ASSERT (crl_file);
src/openvpn/ssl_mbedtls.h +2 -0
@@ -74,6 +74,8 @@ struct tls_root_ctx {mbedtls_x509_crt *ca_chain;/**< CA chain for remote verification */ mbedtls_pk_context *priv_key;/**< Local private key */ mbedtls_x509_crl *crl;/**< Certificate Revocation List */ +struct timespec crl_last_mtime;/**< CRL last modification time */ +off_t crl_last_size;/**< size of last loaded CRL */ #if defined(ENABLE_PKCS11 )mbedtls_pkcs11_context *priv_key_pkcs11;/**< PKCS11 private key */ #endif
src/openvpn/ssl_openssl.c +1 -1
@@ -772,7 +772,7 @@ end: }void -tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +backend_tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file,const char *crl_inline) {X509_CRL *crl =NULL ;
src/openvpn/ssl_openssl.h +2 -0
@@ -49,6 +49,8 @@*/ struct tls_root_ctx {SSL_CTX *ctx; +struct timespec crl_last_mtime; +off_t crl_last_size; };struct key_state_ssl {
Signature · 1 line
2.11.0
Hi,
Tested on linux and windows, works as expected, except for one thing:
src/openvpn/ssl.c tls_version_parse(const char *vstr, const char *extra) · line 542
Quoted 10 lines · Antonio Quartulli
src/openvpn/ssl.c +10
-0+/* +* an inline CRL can't change at runtime, therefore there is no need to +* reload it. It will be reloaded upon config change + SIGHUP. +* Use always '1' as dummy timestamp in this case: it will trigger the +* first load, but will prevent any future reload. +*/ +if (crl_file_inline) + crl_stat.st_mtime =1 ; +else +platform_stat (crl_file, &crl_stat);
I still think this should issue a warning when we do not reload the CRL because platform_stat() fails, e.g. due to the CRL file missing. So, if we replace this with:
if (crl_file_inline)
{
crl_stat.st_mtime = 1;
}
else if (0 != platform_stat(crl_file, &crl_stat))
{
msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
}I can ACK this.
Since the deadline for 2.4_rc1 is this afternoon (CET), and Antonio seems to be awake at quite different times than the Europeans doing the tagging, maybe one of the committers can make this change on the file if he agrees? Of course, if Antonio does read this in time, a v3 would be even better!
Thanks,
-Steffan
Quoted 29 lines · Steffan Karger
Hi,
Tested on linux and windows, works as expected, except for one thing:
src/openvpn/ssl.c tls_version_parse(const char *vstr, const char *extra) · line 542
Quoted 10 lines · Antonio Quartulli
src/openvpn/ssl.c +10
-0+/* +* an inline CRL can't change at runtime, therefore there is no need to +* reload it. It will be reloaded upon config change + SIGHUP. +* Use always '1' as dummy timestamp in this case: it will trigger the +* first load, but will prevent any future reload. +*/ +if (crl_file_inline) + crl_stat.st_mtime =1 ; +else +platform_stat (crl_file, &crl_stat);I still think this should issue a warning when we do not reload the CRL because
platform_stat()fails, e.g. due to the CRL file missing. So, if we replace this with:if (crl_file_inline) { crl_stat.st_mtime = 1; } else if (0 != platform_stat(crl_file, &crl_stat)) { msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL."); }
Quoted 7 lines
I can ACK this.
Since the deadline for 2.4_rc1 is this afternoon (CET), and Antonio seems to be awake at quite different times than the Europeans doing the tagging, maybe one of the committers can make this change on the file if he agrees? Of course, if Antonio does read this in time, a v3 would be even better!
v3 is coming ;)
Quoted 10 lines
Thanks,
-Steffan------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Signature · 1 line
Antonio Quartulli
Quoted 4 lines · Steffan Karger
else if (0 != platform_stat(crl_file, &crl_stat))
{
msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
}Ahum, as Gert noted on IRC, this missed a return statement to actually *not* load the CRL. So, better suggestion:
else if (platform_stat(crl_file, &crl_stat) < 0)
{
msg (M_WARN, "WARNING: Failed to stat CRL file, using (re)loading
CRL.");
return;
}-Steffan