reload CRL only if file was modified
PATCH 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.
Cheers,
src/openvpn/ssl.c | 40 +++++++++++++++++++++++++++++++++++++--- src/openvpn/ssl_backend.h | 2 +- src/openvpn/ssl_mbedtls.c | 2 +- src/openvpn/ssl_mbedtls.h | 1 + src/openvpn/ssl_openssl.c | 2 +- src/openvpn/ssl_openssl.h | 1 + 6 files changed, 42 insertions(+), 6 deletions(-)
src/openvpn/ssl.c +37 -3
@@ -612,7 +612,8 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)/* Read CRL */ if (options->crl_file && !(options->ssl_flags &SSLF_CRL_VERIFY_DIR )) { -tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline); +tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline, + false); }/* Once keys and cert are loaded, load ECDH parameters */
@@ -2450,6 +2451,36 @@ auth_deferred_expire_window (const struct tls_options *o)return ret; } +void +tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +const char *crl_file_inline,bool reload) +{ +/* 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. +*/ +if (crl_file_inline && reload) +return ; + +if (!crl_file_inline) +platform_stat (crl_file, &crl_stat); + +/* +* If this is not a reload, update the CRL right away. +* Otherwise, update only if the CRL file was modified after the last reload. +* Note: Windows does not support tv_nsec. +*/ +if (reload && + (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime)) +return ; + + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; +backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); +} +/* * This is the primary routine for processing TLS stuff inside the * the main event loop. When this routine exits
@@ -2581,12 +2612,15 @@ 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 )) {tls_ctx_reload_crl (&session->opt->ssl_ctx, - session->opt->crl_file, session->opt->crl_file_inline); + session->opt->crl_file, session->opt->crl_file_inline, true); }dmsg (D_TLS_DEBUG_MED ,"STATE S_START" );
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 +1 -0
@@ -74,6 +74,7 @@ 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 */ #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 +1 -0
@@ -49,6 +49,7 @@*/ struct tls_root_ctx {SSL_CTX *ctx; +struct timespec crl_last_mtime; };struct key_state_ssl {
Signature · 1 line
2.11.0.rc2
Hi,
Thanks for following up. I did some stare-at-code and trivial tests. Will test more thoroughly tonight (hopefully on Windows too), but have a lot of faith that those will succeed. I have some comments from staring at the code though, see below.
Quoted 46 lines · Antonio Quartulli
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).
Cc Steffan Karger <[email protected]...>Signed-off-by Antonio Quartulli <[email protected]...>---
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.Cheers,
src/openvpn/ssl.c | 40 +++++++++++++++++++++++++++++++++++++--- src/openvpn/ssl_backend.h | 2 +- src/openvpn/ssl_mbedtls.c | 2 +- src/openvpn/ssl_mbedtls.h | 1 + src/openvpn/ssl_openssl.c | 2 +- src/openvpn/ssl_openssl.h | 1 + 6 files changed, 42 insertions(+), 6 deletions(-)src/openvpn/ssl.c +2
-1@@ -612,7 +612,8 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)/* Read CRL */ if (options->crl_file && !(options->ssl_flags &SSLF_CRL_VERIFY_DIR )) { -tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline); +tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline, + false); }/* Once keys and cert are loaded, load ECDH parameters */
src/openvpn/ssl.c auth_deferred_expire_window (const struct tls_options *o) · line 2456
Quoted 6 lines
src/openvpn/ssl.c +3
-0@@ -2450,6 +2451,36 @@ auth_deferred_expire_window (const struct tls_options *o)return ret; } +void +tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +const char *crl_file_inline,bool reload)
This is only used within ssl.c, so should be static and have a function declaration, preferably doxygen-documented, at the beginning of ssl.c.
Quoted 16 lines
src/openvpn/ssl.c +16
-0+{ +/* 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. +*/ +if (crl_file_inline && reload) +return ; + +if (!crl_file_inline) +platform_stat (crl_file, &crl_stat); + +/* +* If this is not a reload, update the CRL right away.
src/openvpn/ssl.c auth_deferred_expire_window (const struct tls_options *o) · line 2482
Quoted 10 lines
src/openvpn/ssl.c +10
-0+* Otherwise, update only if the CRL file was modified after the last reload. +* Note: Windows does not support tv_nsec. +*/ +if (reload && + (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime)) +return ; + + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; +backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); +}
This can be done without the 'reload' argument too, something like:
/* if something goes wrong with stat(), we'll store 0 as mtime */ platform_stat_t crl_stat = {0};
if (crl_file_inline)
{
/*
* an inline CRL can't change at runtime, therefore there is no
need to
* reload it. It will be reloaded upon config change + SIGHUP.
*/
if (ssl_ctx->crl_last_mtime.tv_sec)
return;
else
crl_stat.st_mtime = now; /* Set dummy mtime */
}
else
{
platform_stat (crl_file, &crl_stat);
} /*
* If this is not a reload, update the CRL right away.
* Otherwise, update only if the CRL file was modified after the last
reload.
* Note: Windows does not support tv_nsec.
*/
if (ssl_ctx->crl_last_mtime.tv_sec <= crl_stat.st_mtime)
{
ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline);
}I slightly prefer this over adding the extra argument, but can live with your approach just fine too. Pick what you like best :)
-Steffan
Steffan Karger <[email protected]...> on Tue, 2016/11/29 17:43:
Quoted 6 lines
Hi,
Thanks for following up. I did some stare-at-code and trivial tests. Will test more thoroughly tonight (hopefully on Windows too), but have a lot of faith that those will succeed. I have some comments from staring at the code though, see below.
Oops, missed that in my logs (and did not find the code)... You are right, cache is cleared.
Either of both is just fine and it works as-is. So ignore my patch.
Signature · 3 lines
main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];)
putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}Christian Hesse <[email protected]...> on Tue, 2016/11/29 20:16:
Quoted 4 lines
Oops, missed that in my logs (and did not find the code)... You are right, cache is cleared.
Either of both is just fine and it works as-is. So ignore my patch.
Oops again... Looks like I answered a wrong mail. Please ignore... (The mail, not any patch. ;)
Signature · 3 lines
main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];)
putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}Hi,
One more thing.
src/openvpn/ssl.c auth_deferred_expire_window (const struct tls_options *o) · line 2469
Quoted 2 lines · Antonio Quartulli
src/openvpn/ssl.c +2
-0+if (!crl_file_inline) +platform_stat (crl_file, &crl_stat);
If platform_stat() fails, we now silently ignore the error and continue using the old CRL. I think using the old CRL is fine, but we should print a warning to tell the user so:
if (!crl_file_inline && 0 != platform_stat(crl_file, &crl_stat))
msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
-Steffan
Quoted 2 lines · Steffan Karger
Will test more thoroughly tonight (hopefully on Windows too), but have a lot of faith that those will succeed.
Just did this, on linux and windows, and code works as expected. So once you've taken care of the (minor) remarks in my previous mails, I think this patch is good to go!
-Steffan
On Tue, Nov 29, 2016 at 05:43:33PM +0100, Steffan Karger wrote:
[CUT..]
Quoted 7 lines
src/openvpn/ssl.c auth_deferred_expire_window (const struct tls_options *o) · line 2456
Quoted 4 lines
src/openvpn/ssl.c +3
-0+void +tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +const char *crl_file_inline,bool reload)This is only used within
ssl.c, so should be static and have a function declaration, preferably doxygen-documented, at the beginning ofssl.c.
You're absolutely right about this function supposed to be static. I missed that.
Do I really need to split declaration and definition? Or can I just doxy-document the definition in the same place where it is now?
Quoted 57 lines
Quoted 17 lines
src/openvpn/ssl.c +16
-0+{ +/* 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. +*/ +if (crl_file_inline && reload) +return ; + +if (!crl_file_inline) +platform_stat (crl_file, &crl_stat); + +/* +* If this is not a reload, update the CRL right away. src/openvpn/ssl.c auth_deferred_expire_window (const struct tls_options *o) · line 2482
Quoted 10 lines
src/openvpn/ssl.c +10
-0+* Otherwise, update only if the CRL file was modified after the last reload. +* Note: Windows does not support tv_nsec. +*/ +if (reload && + (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime)) +return ; + + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; +backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); +}This can be done without the 'reload' argument too, something like:
/* if something goes wrong with
stat(), we'll store 0 as mtime */platform_stat_tcrl_stat= {0};if (
crl_file_inline){ /* * an inline CRL can't change at runtime, therefore there is no need to * reload it. It will be reloaded upon config change + SIGHUP. */ if (ssl_ctx->crl_last_mtime.tv_sec) return; else crl_stat.st_mtime = now; /* Set dummy mtime */ } else { platform_stat (crl_file, &crl_stat); }/*
* If this is not a reload, update the CRL right away.
* Otherwise, update only if the CRL file was modified after the last
reload.
* Note: Windows does not supporttv_nsec.
*/
if (ssl_ctx->crl_last_mtime.tv_sec<= crl_stat.st_mtime)
Quoted 7 lines
{ ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); }I slightly prefer this over adding the extra argument, but can live with your approach just fine too. Pick what you like best :)
I guess the assumption here is that crl_last_mtime.tv_sec is initialized with 0. This would make your code work. And yeah, I also prefer to avoid the reload argument ;)
Will make these changes and resend the patch.
Thanks for the comments!
Cheers,
Signature · 1 line
Antonio Quartulli
Hi,
Quoted 2 lines · Antonio Quartulli
Do I really need to split declaration and definition? Or can I just doxy-document the definition in the same place where it is now?
Well, either add a declaration at the top of the file (before the first usage), or move the function up to before the first usage and document it without splitting it.
Quoted 34 lines
Quoted 34 lines
This can be done without the 'reload' argument too, something like:
/* if something goes wrong with
stat(), we'll store 0 as mtime */platform_stat_tcrl_stat= {0};if (
crl_file_inline){ /* * an inline CRL can't change at runtime, therefore there is no need to * reload it. It will be reloaded upon config change + SIGHUP. */ if (ssl_ctx->crl_last_mtime.tv_sec) return; else crl_stat.st_mtime = now; /* Set dummy mtime */ } else { platform_stat (crl_file, &crl_stat); }/*
* If this is not a reload, update the CRL right away.
* Otherwise, update only if the CRL file was modified after the last
reload.
* Note: Windows does not supporttv_nsec.
*/
if (ssl_ctx->crl_last_mtime.tv_sec<= crl_stat.st_mtime){ ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); }
Quoted 6 lines
Quoted 2 lines
I slightly prefer this over adding the extra argument, but can live with your approach just fine too. Pick what you like best :)
I guess the assumption here is that crl_last_mtime.tv_sec is initialized with 0. This would make your code work. And yeah, I also prefer to avoid the reload argument ;)
Correct. This might not be immediately clear in the code, but we have the good habit of zero-initilializing structs like this :)
Quoted 1 line
Will make these changes and resend the patch.
Thanks!
-Steffan
1) I would also check if the file size was changed, not only mtime.
2) I wasn't digging the code deeply, but the
Quoted 1 line
ssl_ctx->crl_last_mtime.tv_sec>= crl_stat.st_mtime
makes me think it would fail if the file goes reverted to a previous version. Perhaps the check shall be != instead of >=.
Quoted 181 lines
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).
Cc Steffan Karger <[email protected]...>Signed-off-by Antonio Quartulli <[email protected]...>---
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.Cheers,
src/openvpn/ssl.c | 40 +++++++++++++++++++++++++++++++++++++--- src/openvpn/ssl_backend.h | 2 +- src/openvpn/ssl_mbedtls.c | 2 +- src/openvpn/ssl_mbedtls.h | 1 + src/openvpn/ssl_openssl.c | 2 +- src/openvpn/ssl_openssl.h | 1 + 6 files changed, 42 insertions(+), 6 deletions(-)src/openvpn/ssl.c +2
-1@@ -612,7 +612,8 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)/* Read CRL */ if (options->crl_file && !(options->ssl_flags &SSLF_CRL_VERIFY_DIR )) { -tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline); +tls_ctx_reload_crl (new_ctx, options->crl_file, options->crl_file_inline, + false);}/* Once keys and cert are loaded, load ECDH parameters */src/openvpn/ssl.c +20
-0@@ -2450,6 +2451,36 @@ auth_deferred_expire_window (const struct tls_options *o)return ret; } +void +tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, +const char *crl_file_inline,bool reload) +{ +/* 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. +*/ +if (crl_file_inline && reload) +return ; + +if (!crl_file_inline) +platform_stat (crl_file, &crl_stat); + +/* +* If this is not a reload, update the CRL right away. +* Otherwise, update only if the CRL file was modified after the last reload.
src/openvpn/ssl.c +15
-2+* Note: Windows does not support tv_nsec. +*/ +if (reload && + (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime)) +return ; + + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime; +backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline); +} +/* * This is the primary routine for processing TLS stuff inside the * the main event loop. When this routine exits @@ -2581,12 +2612,15 @@ 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 )) {tls_ctx_reload_crl (&session->opt->ssl_ctx, - session->opt->crl_file, session->opt->crl_file_inline); + session->opt->crl_file, session->opt->crl_file_inline, true); }dmsg (D_TLS_DEBUG_MED ,"STATE S_START" );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 +1
-0@@ -74,6 +74,7 @@ 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 */ #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 +1
-0@@ -49,6 +49,7 @@*/ struct tls_root_ctx {SSL_CTX *ctx; +struct timespec crl_last_mtime; };struct key_state_ssl {Signature · 8 lines
2.11.0.rc2 ------------------------------------------------------------------------------ _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Signature · 1 line
Svjatoslav Mikhailov
Quoted 2 lines · SviMik
1) I would also check if the file size was changed, not only mtime.
this would work against 2 CRLs with the same mtime but different size: is this is a real case we have to worry about?
Anyway, adding this check is easy. I'd do it if it makes the whole check more robust.
Quoted 4 lines
2) I wasn't digging the code deeply, but the
Quoted 1 line
ssl_ctx->crl_last_mtime.tv_sec>= crl_stat.st_mtimemakes me think it would fail if the file goes reverted to a previous version. Perhaps the check shall be != instead of >=.
good point! I think we should definitely switch to !=.
Thanks!
Signature · 1 line
Antonio Quartulli