| 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 | expand |
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>
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
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
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; };
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(+)