openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email

to review the following change.


Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................

Fix logic when pushed cipher triggers tun reopen and ignore more options

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-By: co...@jo...
Found-By: Zeropath
Change-Id: I76eb584024610a6054a069340adbac988abf686c
---
M src/openvpn/push.c
1 file changed, 14 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/1321/1

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..d7063e6 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1025,15 +1025,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

Patchset:

PS1: 
I don't think the logic is right now (neither before nor afterwards).  The commit message actually speaks about `link-mtu` but the code tests `tun_mtu_defined`.


File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/1321/comment/d77bec25_18f7eaaa?usp=email :
PS1, Line 1046:         if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
I don't think this is actually correct - `--tun-mtu` is the *inner* MTU, and that never changes on `--cipher`.  So the original variant means "if we have a configured tun-mtu, a cipher change will not affect this, so we don't care".

A more clear expression would be "if a `--link-mtu` was defined and `--cipher changes` (because in that case, `--tun-mtu` would be recalculated - or would it?)

Maybe that whole thing needs to go after the big mtu refactoring?



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 17:32:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: cron2, flichtenheld.

plaisthos has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1:

(2 comments)

Patchset:

PS1: 
> I don't think the logic is right now (neither before nor afterwards). […]
welcome to openvpn logic. link-mtu and tun-mtu are mutally exclusive. And you need to have link-mtu in the config to actually have a pushed cipher to change the MTU of the tun device.


File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/1321/comment/036771c8_d363c718?usp=email :
PS1, Line 1046:         if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
> I don't think this is actually correct - `--tun-mtu` is the *inner* MTU, and that never changes on ` […]
The question is more if we want to deprecate link-mtu and its weird behaviour.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 22:47:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <ge...@gr...>

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

Patchset:

PS1: 
> welcome to openvpn logic. link-mtu and tun-mtu are mutally exclusive. […]
Right, this is why the code should check for `link_mtu_defined` and not `tun_mtu_defined`.  Right?  `if (cipher && !link_mtu_defined) { skip this }`.

But you're right, I misread this - `tun_mtu_defined` implies `!link_mtu_defined`, so while I find the other one easier to follow ("if link_mtu defined, tun_mtu is derived from cipher etc.") the code is correct.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Wed, 29 Oct 2025 06:52:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: plaisthos <arn...@rf...>
Comment-In-Reply-To: cron2 <ge...@gr...>

[Openvpn-devel] [PATCH v1] Fix logic when pushed cipher triggers tun reopen and ignore more options

[Gert D. <ge...@gr...>]

From: Arne Schwabe <ar...@rf...>

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-By: co...@jo...
Found-By: Zeropath
Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
This mail reflects revision 1 of this Change.

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>

        
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..d7063e6 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1025,15 +1025,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

[Openvpn-devel] [PATCH applied] Re: Fix logic when pushed cipher triggers tun reopen and ignore more options

[Gert D. <ge...@gr...>]

So there was a bit of discussion about this in gerrit, and the logic is
confusing and twisted.  The tun device needs to reopened on a cipher
change *if* the tun-mtu ("ifconfig tun0 mtu 1400") could change - and
if the config has tun_mtu_defined, the tun MTU is fixed, so the cipher
could be ignored.  I think there are still funny edge cases (like
"no tun-mtu or link-mtu defined", what then?) and the check maybe should
be double-checked for "if (!opt->ce.link_mtu_defined)" - because
"link-mtu <set>" is the trigger for "tun mtu <calculated>", so in that
case we can't skip this.

OTOH, this is a small edge of an edge case, namely "cipher *change* upon
reconnect", which is rare enough - so for this to make a real difference
you need --user nobody or windows-dco (so "reopen tun" would fail) *and*
a cipher change.

I have changed the Reported-by:/Found-by: tags to look "like on the last
few commits", so it's consistent (basically adding URL and full name).

Your patch has been applied to the master branch.

commit 911a69dc1af20bc54557a208b6fd4e76261b2bb2
Author: Arne Schwabe
Date:   Wed Oct 29 07:53:10 2025 +0100

     Fix logic when pushed cipher triggers tun reopen and ignore more options

     Signed-off-by: Arne Schwabe <arn...@rf...>
     Acked-by: Gert Doering <ge...@gr...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg33989.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................

Fix logic when pushed cipher triggers tun reopen and ignore more options

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33989.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/push.c
1 file changed, 14 insertions(+), 4 deletions(-)




diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6f146fc..7852d36 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1029,15 +1029,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#2) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................

Fix logic when pushed cipher triggers tun reopen and ignore more options

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33989.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/push.c
1 file changed, 14 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/1321/2

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6f146fc..7852d36 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1029,15 +1029,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>