diff mbox series

[Openvpn-devel,2/2] tls-crypt-v2: also preload tls-crypt-v2 keys (if --persist-key)

Message ID 20201203154951.29382-2-steffan@karger.me
State Accepted
Headers show
Series [Openvpn-devel,1/2] tls-crypt-v2: fix server memory leak | expand

Commit Message

Steffan Karger Dec. 3, 2020, 4:49 a.m. UTC 
This allows tls-crypt-v2 servers to drop privileges after reading the
keys. Without it, the server would try to read the key file for each
connecting client. (And clients for each reconnect.)

As with the previous patch, the pre-loading was developed in parallel
with tls-crypt-v2, and the tls-crypt-v2 patches were never amended to
implement the pre-loading.

Also as with the previous patch, it would be nicer if servers would not
reload the tls-crypt-v2 server key for each connecting client. But let's
first fix the issue, and see if we can improve later.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
 src/openvpn/options.c | 52 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

Comments

Arne Schwabe Dec. 3, 2020, 5:06 a.m. UTC | #1 
Am 03.12.20 um 16:49 schrieb Steffan Karger:
> This allows tls-crypt-v2 servers to drop privileges after reading the
> keys. Without it, the server would try to read the key file for each
> connecting client. (And clients for each reconnect.)
> 
> As with the previous patch, the pre-loading was developed in parallel
> with tls-crypt-v2, and the tls-crypt-v2 patches were never amended to
> implement the pre-loading.
> 
> Also as with the previous patch, it would be nicer if servers would not
> reload the tls-crypt-v2 server key for each connecting client. But let's
> first fix the issue, and see if we can improve later.

Agreed. One thing at a time.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Dec. 3, 2020, 11:47 p.m. UTC | #2 
As for 1/2, I have not really reviewed or tested anything
here.  Trusting you and Arne here.

From a quick view this looks like a nice "code cleanup en passant",
though :-) (and it does pass the client side tests, of course).

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

commit 4d307ed431bf18d554f524ebaf111f5e136147fe (master)
commit bac760f84b39bf8f6b97778a15e5fcf863f6d366 (release/2.5)
Author: Steffan Karger
Date:   Thu Dec 3 16:49:51 2020 +0100

     tls-crypt-v2: also preload tls-crypt-v2 keys (if --persist-key)

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20201203154951.29382-2-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21307.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 21f8d494..599f534c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1980,6 +1980,23 @@  connection_entry_load_re(struct connection_entry *ce, const struct remote_entry
     }
 }
 
+static void
+connection_entry_preload_key(const char **key_file, bool *key_inline,
+                             struct gc_arena *gc)
+{
+    if (key_file && *key_file && !(*key_inline))
+    {
+        struct buffer in = buffer_read_from_file(*key_file, gc);
+        if (!buf_valid(&in))
+        {
+            msg(M_FATAL, "Cannot pre-load keyfile (%s)", *key_file);
+        }
+
+        *key_file = (const char *) in.data;
+        *key_inline = true;
+    }
+}
+
 static void
 options_postprocess_verify_ce(const struct options *options,
                               const struct connection_entry *ce)
@@ -2931,36 +2948,17 @@  options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         ce->tls_crypt_v2_file_inline = o->tls_crypt_v2_file_inline;
     }
 
-    /* pre-cache tls-auth/crypt key file if persist-key was specified and keys
-     * were not already embedded in the config file
+    /* Pre-cache tls-auth/crypt(-v2) key file if persist-key was specified and
+     * keys were not already embedded in the config file.
      */
     if (o->persist_key)
     {
-        if (ce->tls_auth_file && !ce->tls_auth_file_inline)
-        {
-            struct buffer in = buffer_read_from_file(ce->tls_auth_file, &o->gc);
-            if (!buf_valid(&in))
-            {
-                msg(M_FATAL, "Cannot pre-load tls-auth keyfile (%s)",
-                    ce->tls_auth_file);
-            }
-
-            ce->tls_auth_file = (char *)in.data;
-            ce->tls_auth_file_inline = true;
-        }
-
-        if (ce->tls_crypt_file && !ce->tls_crypt_file_inline)
-        {
-            struct buffer in = buffer_read_from_file(ce->tls_crypt_file, &o->gc);
-            if (!buf_valid(&in))
-            {
-                msg(M_FATAL, "Cannot pre-load tls-crypt keyfile (%s)",
-                    ce->tls_crypt_file);
-            }
-
-            ce->tls_crypt_file = (char *)in.data;
-            ce->tls_crypt_file_inline = true;
-        }
+        connection_entry_preload_key(&ce->tls_auth_file,
+                                     &ce->tls_auth_file_inline, &o->gc);
+        connection_entry_preload_key(&ce->tls_crypt_file,
+                                     &ce->tls_crypt_file_inline, &o->gc);
+        connection_entry_preload_key(&ce->tls_crypt_v2_file,
+                                     &ce->tls_crypt_v2_file_inline, &o->gc);
     }
 }