diff mbox series

[Openvpn-devel,v3,2/2] Abort if CRL file can't be stat-ed in ssl_init

Message ID 20210415093454.18324-1-maximilian.fillinger@foxcrypto.com
State Accepted
Delegated to: Antonio Quartulli
Headers
Series None |

Commit Message

Maximilian Fillinger April 14, 2021, 11:34 p.m. UTC 
  Now that the path for the CRL file is handled correctly when using
chroot, there's no good reason for the file to be inaccessible during
ssl_init().

This commit ensures that the CRL file is accessed successfully at least
once, which fixes a bug where the mbedtls version of OpenVPN wouldn't
use a reloaded CRL if it initially failed to access the file.

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/ssl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli April 18, 2021, 9:13 a.m. UTC | #1 
Hi,

On 15/04/2021 11:34, Max Fillinger wrote:
> Now that the path for the CRL file is handled correctly when using
> chroot, there's no good reason for the file to be inaccessible during
> ssl_init().
> 
> This commit ensures that the CRL file is accessed successfully at least
> once, which fixes a bug where the mbedtls version of OpenVPN wouldn't
> use a reloaded CRL if it initially failed to access the file.
> 
> Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>

It simply does what it says: when calling init_ssl() upon instance
initialization (i.e. upon openvpn startup, and upon USR1 on clients) if
the CRL is not accessible, then openvpn will abort.

This behaviour is what we need because "starting an instance without
having the CRL where it is expected" is equivalent to a configuration error.

Being unable to re-load the CRL at runtime is instead acceptable,
because some subsystem might be just recreating the CRL, therefore
openvpn will skip the reload and continue using what is currently in
memory. The CRL will be reloaded at the next occasion.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 20, 2021, 1:21 a.m. UTC | #2 
The actual code change looks very magical to me, and shrunk dramatically
between v1 and v3 of that code :-) - but if Antonio has tested this
and ACKs it, who am I to complain.  I have compile-tested it, at least.

Your patch has been applied to the master and release/2.5 branch (bugfix).

commit 940619c88067d95a1c9865795624bc3822a89bd7 (master)
commit 8a06459d0c969b083377b63be16e5da1245ce0ee (release/2.5)
Author: Max Fillinger
Date:   Thu Apr 15 11:34:54 2021 +0200

     Abort if CRL file can't be stat-ed in ssl_init

     Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210415093454.18324-1-maximilian.fillinger@foxcrypto.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22118.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 1e0e6170..6ce1d079 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -559,7 +559,15 @@  tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
     }
     else if (platform_stat(crl_file, &crl_stat) < 0)
     {
-        msg(M_WARN, "WARNING: Failed to stat CRL file, not (re)loading CRL.");
+        /* If crl_last_mtime is zero, the CRL file has not been read before. */
+        if (ssl_ctx->crl_last_mtime == 0)
+        {
+            msg(M_FATAL, "ERROR: Failed to stat CRL file during initialization, exiting.");
+        }
+        else
+        {
+            msg(M_WARN, "WARNING: Failed to stat CRL file, not reloading CRL.");
+        }
         return;
     }