diff mbox series

[Openvpn-devel,16/28] Move CRL reload to key_state_init from S_START transition

Message ID 20220422142953.3805364-7-arne@rfc2549.org
State Accepted
Headers show
Series Stateless three-way handshake and control channel improvements | expand

Commit Message

Arne Schwabe April 22, 2022, 4:29 a.m. UTC 
The current place that we reload is a bit more efficient since it only
triggers reload after a completed 3way handshake. On the other hand the
key_state_init is a much more logical place and with the upcoming
HMAC based UDP code and TCP code, the initialisation will only be done
after a 3way handshake.
---
 src/openvpn/ssl.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Antonio Quartulli April 27, 2022, 4:04 a.m. UTC | #1 
Hi,

On 22/04/2022 16:29, Arne Schwabe wrote:
> The current place that we reload is a bit more efficient since it only
> triggers reload after a completed 3way handshake. On the other hand the
> key_state_init is a much more logical place and with the upcoming
> HMAC based UDP code and TCP code, the initialisation will only be done
> after a 3way handshake.

There is something strange. Upon client reconnection the CRL is not 
always reloaded. It feels as if "some stuff" are already initialized 
(because we have a session for this client floating around) so we skip 
that initialization and we also skip reloading the CRL.


Regards,
Gert Doering April 27, 2022, 4:26 a.m. UTC | #2 
Hi,

On Wed, Apr 27, 2022 at 04:04:41PM +0200, Antonio Quartulli wrote:
> On 22/04/2022 16:29, Arne Schwabe wrote:
> > The current place that we reload is a bit more efficient since it only
> > triggers reload after a completed 3way handshake. On the other hand the
> > key_state_init is a much more logical place and with the upcoming
> > HMAC based UDP code and TCP code, the initialisation will only be done
> > after a 3way handshake.
> 
> There is something strange. Upon client reconnection the CRL is not 
> always reloaded. It feels as if "some stuff" are already initialized 
> (because we have a session for this client floating around) so we skip 
> that initialization and we also skip reloading the CRL.

Is that different from "without the patch"?

gert
Antonio Quartulli April 27, 2022, 4:51 a.m. UTC | #3 
On 27/04/2022 16:26, Gert Doering wrote:
> Hi,
> 
> On Wed, Apr 27, 2022 at 04:04:41PM +0200, Antonio Quartulli wrote:
>> On 22/04/2022 16:29, Arne Schwabe wrote:
>>> The current place that we reload is a bit more efficient since it only
>>> triggers reload after a completed 3way handshake. On the other hand the
>>> key_state_init is a much more logical place and with the upcoming
>>> HMAC based UDP code and TCP code, the initialisation will only be done
>>> after a 3way handshake.
>>
>> There is something strange. Upon client reconnection the CRL is not
>> always reloaded. It feels as if "some stuff" are already initialized
>> (because we have a session for this client floating around) so we skip
>> that initialization and we also skip reloading the CRL.
> 

I take this back.

I managed to fool myself (and OpenVPN) because instead of really 
updating the CRL file, I was rather switching between two CRLs (one with 
client revoked, one with client allowed) using a symlink.

However, as reported in stat(2), stat() will follow the symlink and 
report stats about the linked file (which had a constant mtime).

To properly test the CRL-reload behaviour, I therefore had to change the 
symlink and then touch the linked file. This made my test correct and I 
could check that also the OpenVPN behaviour, with this patch, is 
actually correct.

Acked-by: Antonio Quartulli <a@unstable.cc>

Regards,
Gert Doering April 27, 2022, 6:01 a.m. UTC | #4 
Thanks, Antonio, for the detailed testing.  I already checked that 
"it's only code being moved" but since this is all magic event
driven functions, I found "verifying functionality" important here.

missing S-O-B added according to our developer docs.

Your patch has been applied to the master branch.

commit fb6e6c2ae3e6ecdc6ac0b69a79741d52189c0c70
Author: Arne Schwabe
Date:   Fri Apr 22 16:29:41 2022 +0200

     Move CRL reload to key_state_init from S_START transition

     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20220422142953.3805364-7-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24156.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 097be8c02..d7fec0276 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -958,6 +958,17 @@  key_state_init(struct tls_session *session, struct key_state *ks)
 #ifdef ENABLE_MANAGEMENT
     ks->mda_key_id = session->opt->mda_context->mda_key_id_counter++;
 #endif
+
+    /*
+     * 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);
+    }
 }
 
 
@@ -2512,20 +2523,8 @@  tls_process_state(struct tls_multi *multi,
         ks->state = S_START;
         state_change = true;
 
-        /*
-         * 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);
-        }
-
         /* New connection, remove any old X509 env variables */
         tls_x509_clear_env(session->opt->es);
-
         dmsg(D_TLS_DEBUG_MED, "STATE S_START");
     }