[Openvpn-devel,v2,3/3] Restore also ping related options on a reconnect

Message ID 20210317160038.25828-3-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v2,1/3] Move restoring pre pull options to initialising of c2 context
Related show

Commit Message

Arne Schwabe March 17, 2021, 4 p.m.
This fixes the issue that if a client reconnects the next connection
entries inherits the keepalive settings that were pushed or set by
the previous entry. Since UDP+PULL entries have an implicit 120s
timeout, this timeout also got applied to a TCP session after an
UDP entry.

Patch v2: rebase on master

Reported-By: Jan Just Keijser <janjust@nikhef.nl>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/options.c | 9 +++++++++
 src/openvpn/options.h | 4 ++++
 2 files changed, 13 insertions(+)

Comments

Antonio Quartulli March 17, 2021, 4:44 p.m. | #1
Hi all,

On 17/03/2021 17:00, Arne Schwabe wrote:
> This fixes the issue that if a client reconnects the next connection
> entries inherits the keepalive settings that were pushed or set by
> the previous entry. Since UDP+PULL entries have an implicit 120s
> timeout, this timeout also got applied to a TCP session after an
> UDP entry.
> 
> Patch v2: rebase on master
> 
> Reported-By: Jan Just Keijser <janjust@nikhef.nl>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

This patch is actually fixing a bug, as reported by JJK, however it is
such a coerner case that we may not want to backport this to 2.4
(backporting requires rewriting the patch).

This patch was reviewed with the Arne's guidance.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering March 23, 2021, 3:56 p.m. | #2
The patch is visually fine, and I was actually successfull in testing
pre/post behaviour.  As in: client with multiple <connection> blocks,
and using the management interface to make it connect to server A
("push ping, ping-restart") first, then server B ("do not push anything
ping-related").  Without the patch, the client would die with --ping-restart
timeout, with the patch it happily does "no ping restart".

It needs SIGUSR1 restarts to trigger the issue.  SIGHUP restarts do a
"full config reload" and never display "lingering pushed config" problems.

Testing has uncovered more save & restore bugs, related to compression
and "--route-gateway".  At least...

Your patch has been applied to the master and release/2.5 branch (bugfix),
but I'm not sure I want to really backport it to 2.4 - it's not a crucial
crashbug, not easy to trigger, and if you're annoyed by it, just upgrade.

I have not tested the 2.5 backport in full depth, just a basic t_client 
run (but the change is straightforward enough, and it applied without
much friction).

commit 5a2ed714d14acb2312d27fe40e300be96d970c27 (master)
commit a0e844c892d6f67977bf8e9162cbc43a3f64ba46 (release/2.5)
Author: Arne Schwabe
Date:   Wed Mar 17 17:00:38 2021 +0100

     Restore also ping related options on a reconnect

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210317160038.25828-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21675.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering March 23, 2021, 4:38 p.m. | #3
The patch is visually fine, and I was actually successfull in testing
pre/post behaviour.  As in: client with multiple <connection> blocks,
and using the management interface to make it connect to server A
("push ping, ping-restart") first, then server B ("do not push anything
ping-related").  Without the patch, the client would die with --ping-restart
timeout, with the patch it happily does "no ping restart".

It needs SIGUSR1 restarts to trigger the issue.  SIGHUP restarts do a
"full config reload" and never display "lingering pushed config" problems.

Testing has uncovered more save & restore bugs, related to compression
and "--route-gateway".  At least...

Your patch has been applied to the master and release/2.5 branch (bugfix),
but I'm not sure I want to really backport it to 2.4 - it's not a crucial
crashbug, not easy to trigger, and if you're annoyed by it, just upgrade.

I have not tested the 2.5 backport in full depth, just a basic t_client 
run (but the change is straightforward enough, and it applied without
much friction).

commit 5a2ed714d14acb2312d27fe40e300be96d970c27 (master)
commit a0e844c892d6f67977bf8e9162cbc43a3f64ba46 (release/2.5)
Author: Arne Schwabe
Date:   Wed Mar 17 17:00:38 2021 +0100

     Restore also ping related options on a reconnect

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210317160038.25828-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21675.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 645fc38b..42681b71 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3540,6 +3540,11 @@  pre_pull_save(struct options *o)
         o->pre_pull->ciphername = o->ciphername;
         o->pre_pull->authname = o->authname;
         o->pre_pull->keysize = o->keysize;
+
+        /* Ping related options should be reset to the config values on reconnect */
+        o->pre_pull->ping_rec_timeout = o->ping_rec_timeout;
+        o->pre_pull->ping_rec_timeout_action = o->ping_rec_timeout_action;
+        o->pre_pull->ping_send_timeout = o->ping_send_timeout;
     }
 
 }
@@ -3591,6 +3596,10 @@  pre_pull_restore(struct options *o, struct gc_arena *gc)
         o->ciphername = pp->ciphername;
         o->authname = pp->authname;
         o->keysize = pp->keysize;
+
+        o->ping_rec_timeout = pp->ping_rec_timeout;
+        o->ping_rec_timeout_action = pp->ping_rec_timeout_action;
+        o->ping_send_timeout = pp->ping_send_timeout;
     }
 
     o->push_continuation = 0;
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index fe67ec72..5317a60e 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -79,6 +79,10 @@  struct options_pre_pull
     const char* authname;
     int keysize;
 
+    int ping_send_timeout;
+    int ping_rec_timeout;
+    int ping_rec_timeout_action;
+
     int foreign_option_index;
 };