diff mbox series

[Openvpn-devel] Persist-key: enable persist-key option by default.

Message ID 20230509154657.2756-1-gianmarco@mandelbit.com
State Superseded
Headers show
Series [Openvpn-devel] Persist-key: enable persist-key option by default. | expand

Commit Message

Gianmarco De Gregori May 9, 2023, 3:46 p.m. UTC 
This commit changes the default behavior of
the OpenVPN configuration to enable the
persist-key option by default.

This means that all the key file content will be kept
in memory throughout the lifetime of the VPN connection.

Fixes: Trac #1405
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
---
 doc/man-sections/generic-options.rst |  2 ++
 src/openvpn/init.c                   | 12 ++----------
 src/openvpn/options.c                | 23 +++++++++++------------
 src/openvpn/options.h                |  1 -
 4 files changed, 15 insertions(+), 23 deletions(-)

Comments

Frank Lichtenheld May 24, 2023, 4:06 p.m. UTC | #1 
On Tue, May 09, 2023 at 05:46:58PM +0200, Gianmarco De Gregori wrote:
> This commit changes the default behavior of
> the OpenVPN configuration to enable the
> persist-key option by default.
> 
> This means that all the key file content will be kept
> in memory throughout the lifetime of the VPN connection.
> 
> Fixes: Trac #1405
> Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
> ---
>  doc/man-sections/generic-options.rst |  2 ++
>  src/openvpn/init.c                   | 12 ++----------
>  src/openvpn/options.c                | 23 +++++++++++------------
>  src/openvpn/options.h                |  1 -
>  4 files changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
> index 97e1b5aa..5f74ab67 100644
> --- a/doc/man-sections/generic-options.rst
> +++ b/doc/man-sections/generic-options.rst
> @@ -303,6 +303,8 @@ which mode OpenVPN is configured as.
>    lower priority, ``n`` less than zero is higher priority).
>  
>  --persist-key
> +  DEPRECATED OPTION, corresponding behavior is now always enabled.

The usual style in the documentation seems to be "**DEPRECATED**". See
e.g. --disable-occ in the same file.

> +
>    Don't re-read key files across :code:`SIGUSR1` or ``--ping-restart``.
>  
>    This option can be combined with ``--user`` to allow restarts
[...]
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2680f268..9ef21bc9 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c


Please also add (DEPRECATED) marker in usage_message.

[...]

Regards,
Arne Schwabe May 25, 2023, 3:39 p.m. UTC | #2 
Am 09.05.2023 um 17:46 schrieb Gianmarco De Gregori:
> -    bool persist_key;           /* Don't re-read key files on SIGUSR1 or PING_RESTART */

The downside of always enabling this option is that you can no longer 
replace the certificate and key without restarting the server completley.

Arne
Gert Doering May 26, 2023, 6:48 p.m. UTC | #3 
Hi,

On Thu, May 25, 2023 at 05:39:10PM +0200, Arne Schwabe wrote:
> Am 09.05.2023 um 17:46 schrieb Gianmarco De Gregori:
> > -    bool persist_key;           /* Don't re-read key files on SIGUSR1 or PING_RESTART */
> 
> The downside of always enabling this option is that you can no longer 
> replace the certificate and key without restarting the server completley.

We discussed this, and wondered if anyone is actually doing this.

So, are you aware of anyone doing this?  Is there a reason I'm overlooking
why this is substantially better than "just do a full restart when changing
any part of the config (including keys, even if residing in separate files)"?

Hard restart should be about as fast as full SIGUSR1 restart - "wait for
EEN to be delivered, close everything, exit()" vs. "close everything,
re-read key files, reopen tun, ..."

gert
diff mbox series

Patch

diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
index 97e1b5aa..5f74ab67 100644
--- a/doc/man-sections/generic-options.rst
+++ b/doc/man-sections/generic-options.rst
@@ -303,6 +303,8 @@  which mode OpenVPN is configured as.
   lower priority, ``n`` less than zero is higher priority).
 
 --persist-key
+  DEPRECATED OPTION, corresponding behavior is now always enabled.
+
   Don't re-read key files across :code:`SIGUSR1` or ``--ping-restart``.
 
   This option can be combined with ``--user`` to allow restarts
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index d358ad00..654d8645 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3606,14 +3606,6 @@  do_option_warnings(struct context *c)
         {
             msg(M_WARN, "WARNING: you are using user/group/chroot/setcon without persist-tun -- this may cause restarts to fail");
         }
-        if (!o->persist_key
-#ifdef ENABLE_PKCS11
-            && !o->pkcs11_id
-#endif
-            )
-        {
-            msg(M_WARN, "WARNING: you are using user/group/chroot/setcon without persist-key -- this may cause restarts to fail");
-        }
     }
 
     if (o->chroot_dir && !(o->username && o->groupname))
@@ -3901,7 +3893,7 @@  static void
 do_close_free_key_schedule(struct context *c, bool free_ssl_ctx)
 {
     /*
-     * always free the tls_auth/crypt key. If persist_key is true, the key will
+     * always free the tls_auth/crypt key. The key will
      * be reloaded from memory (pre-cached)
      */
     free_key_ctx(&c->c1.ks.tls_crypt_v2_server_key);
@@ -3910,7 +3902,7 @@  do_close_free_key_schedule(struct context *c, bool free_ssl_ctx)
     buf_clear(&c->c1.ks.tls_crypt_v2_wkc);
     free_buf(&c->c1.ks.tls_crypt_v2_wkc);
 
-    if (!(c->sig->signal_received == SIGUSR1 && c->options.persist_key))
+    if (!(c->sig->signal_received == SIGUSR1))
     {
         key_schedule_free(&c->c1.ks, free_ssl_ctx);
     }
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2680f268..9ef21bc9 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1860,7 +1860,6 @@  show_settings(const struct options *o)
     SHOW_BOOL(persist_tun);
     SHOW_BOOL(persist_local_ip);
     SHOW_BOOL(persist_remote_ip);
-    SHOW_BOOL(persist_key);
 
 #if PASSTOS_CAPABILITY
     SHOW_BOOL(passtos);
@@ -3239,18 +3238,15 @@  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(-v2) key file if persist-key was specified and
+    /* Pre-cache tls-auth/crypt(-v2) key file if
      * keys were not already embedded in the config file.
      */
-    if (o->persist_key)
-    {
-        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);
-    }
+    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);
 
     if (!proto_is_udp(ce->proto) && ce->explicit_exit_notification)
     {
@@ -6938,7 +6934,10 @@  add_option(struct options *options,
     else if (streq(p[0], "persist-key") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_PERSIST);
-        options->persist_key = true;
+        msg(M_WARN, "DEPRECATED OPTION: --persist-key option ignored."
+            "The corresponding behavior is now always activated."
+            "This option will be removed in a future version, "
+            "please remove it from your configuration.");
     }
     else if (streq(p[0], "persist-local-ip") && !p[1])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index f5890b90..cf9613b2 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -344,7 +344,6 @@  struct options
     bool persist_tun;           /* Don't close/reopen TUN/TAP dev on SIGUSR1 or PING_RESTART */
     bool persist_local_ip;      /* Don't re-resolve local address on SIGUSR1 or PING_RESTART */
     bool persist_remote_ip;     /* Don't re-resolve remote address on SIGUSR1 or PING_RESTART */
-    bool persist_key;           /* Don't re-read key files on SIGUSR1 or PING_RESTART */
 
 #if PASSTOS_CAPABILITY
     bool passtos;