openvpn-devel — OpenVPN developers list

Re: [Openvpn-devel] Quick question for the OpenVPN developers on the source code functionality [problem solving]

[Jon C. <ro...@fo...>]

Thanks Gert! :) I did find a nice built-in method in the OVPN source code
to extract both the source and destination ipv4 addresses
(mroute_extract_addr_ip) which I was able to use and build on top of to
help solve this multi-threaded "issue" of mine. It has helped greatly in
terms of performance now as I am able to simply associate an IPv4
connection state to an available thread which will result in ordered packet
processing of data from the TUN device (read/write). This has helped
greatly to prevent or reduce the amount of processing and reordering that
TCP and QUIC-UDP have to perform and things are loading "snappier" now it
seems. The other available threads can also work on their own connection
states at the same time allowing them to work in parallel but also
separately if you will. Thanks again for your work on this project and
inputs on this proof-of-concept experiment I'm running. I wouldn't have
been able to do any of this with WireGuard!

Blog post:
https://fossjon.com/2025/10/22/solving-a-final-remaining-performance-impact-with-mutli-threaded-operation-by-using-connection-state-mapping-in-the-highly-modified-openvpn-source-code-implementation/



On Tue, Oct 21, 2025 at 2:40 AM Gert Doering <ge...@gr...> wrote:

> Hi,
>
> On Mon, Oct 20, 2025 at 04:15:22PM -0400, Jon Chiappetta via Openvpn-devel
> wrote:
> > I was wondering if any part of the OpenVPN source code parses the IPv4
> > packet header for source/destination address already in place?
>
> With tun/tap and ipv4/ipv6, this is a bit of nastiness...
>
> You might want to have a look at drop_if_recursive_routing() in master,
> as that one has to find destination address & port and will log source
> address & port.
>
> Not sure we have something more convenient.  Grepping for openvpn_iphdr
> might turn up something, though :-) (like somewhere in the NAT code).
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
>  feed honest figures into a computer, honest figures come out. Never
> doubted
>  it myself till I met a computer with a sense of humor."
>                              Robert A. Heinlein, The Moon is a Harsh
> Mistress
>
> Gert Doering - Munich, Germany
> ge...@gr...
>

Re: [Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

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

Hi,

On Wed, Oct 22, 2025 at 06:06:21PM +0000, Joshua Rogers wrote:
> By the way, as mentioned, this was found with the ZeroPath tool. I was wondering if it would be of interest to send the raw results of this scanner to somebody that could allow them to review the findings without me manually triaging? I have done this with curl (https://daniel.haxx.se/blog/2025/10/10/a-new-breed-of-analyzers/) and it was quite succesful (~20% false positive rate).
> 
> If this is of interest, please let me know where to send them. The output is just markdown, and it includes potential security vulnerabilities. If not, I will (slowly) continue triaging myself.

This is of interest.

I'm not really sure where to send this - security bugs go to 
sec...@op..., but if it's not security, we should not spam
this list.  Non-security things could go to GH issues, but *if*
there is security relevant things in between, we might want to keep
the lid on it, for the moment...

So you could send everything my way for a start and I discuss with
my co-developers how to do this in the future.  I'll then try to
triage this in a timely fashion and forward to GH, security@, or
just drop :-)

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...

Re: [Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

[Joshua R. <co...@jo...>]

Hi all,

By the way, as mentioned, this was found with the ZeroPath tool. I was wondering if it would be of interest to send the raw results of this scanner to somebody that could allow them to review the findings without me manually triaging? I have done this with curl (https://daniel.haxx.se/blog/2025/10/10/a-new-breed-of-analyzers/) and it was quite succesful (~20% false positive rate).

If this is of interest, please let me know where to send them. The output is just markdown, and it includes potential security vulnerabilities. If not, I will (slowly) continue triaging myself.

Thank you.

On Wednesday, 22 October 2025 at 13:55, Gert Doering <ge...@gr...> wrote:

> 
> 
> Hi,
> 
> On Tue, Oct 21, 2025 at 10:34:21PM +0200, Arne Schwabe wrote:
> 
> > Before commiting we have to check that port-share does not rely on this
> > behaviour to pass the fd the forked instances. I didn't check right now.
> 
> 
> Good point. I have a port-share test instance, will test.
> 
> gert
> 
> --
> "If was one thing all people took for granted, was conviction that if you
> feed honest figures into a computer, honest figures come out. Never doubted
> it myself till I met a computer with a sense of humor."
> Robert A. Heinlein, The Moon is a Harsh Mistress
> 
> Gert Doering - Munich, Germany ge...@gr...

[Openvpn-devel] [XS] Change in openvpn[master]: Do not try to use the encrypt-then-mac ciphers from OpenSSL 3.6.0

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

Attention is currently required from: flichtenheld.

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

Change subject: Do not try to use the encrypt-then-mac ciphers from OpenSSL 3.6.0
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1: 
GHA run with OpenSSL 3.6.0 on macOS: https://github.com/schwabe/openvpn/actions/runs/18723419183



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1294?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: Iafe3c94b952cd3fbecf6f3d05816e5859f425e7d
Gerrit-Change-Number: 1294
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Wed, 22 Oct 2025 17:02:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

[Openvpn-devel] [XS] Change in openvpn[master]: Do not try to use the encrypt-then-mac ciphers from OpenSSL 3.6.0

[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/+/1294?usp=email

to review the following change.


Change subject: Do not try to use the encrypt-then-mac ciphers from OpenSSL 3.6.0
......................................................................

Do not try to use the encrypt-then-mac ciphers from OpenSSL 3.6.0

These ciphers claim to be CBC but since they are also include an HMAC
are more a mix of AEAD and CBC. Nevertheless, we do not support these
and also have no (good) reason to support them.

Change-Id: Iafe3c94b952cd3fbecf6f3d05816e5859f425e7d
Signed-off-by: Arne Schwabe <ar...@rf...>
---
M src/openvpn/crypto_openssl.c
1 file changed, 5 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/94/1294/1

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 7688add..04aefa2 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -789,7 +789,11 @@
 #ifdef EVP_CIPH_FLAG_CTS
                    && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_CTS)
 #endif
-                   && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_AEAD_CIPHER));
+                   && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_AEAD_CIPHER)
+#ifdef EVP_CIPH_FLAG_ENC_THEN_MAC
+                   && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_ENC_THEN_MAC)
+#endif
+               );
     EVP_CIPHER_free(cipher);
     return ret;
 }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1294?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: Iafe3c94b952cd3fbecf6f3d05816e5859f425e7d
Gerrit-Change-Number: 1294
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] IRC community meeting summary

[Johan D. <jo...@op...>]

Meeting summary for 22 October 2025:

  *

    *Updated: Release 2.7*
    OpenVPN 2.7 beta3 was released 13 October.
    It looks like next up will be beta4, after which a rc1 or stable
    release is expected, after the hackathon end of October.
    There are some bug reports for beta3 so we want to address those
    before doing a beta4.
    Current tentative release date for beta4 is 29 October.

  *

    *Updated: OpenVPN community meetup 2025*
    https://community.openvpn.net/openvpn/wiki/CommunityMeetup2025
    T-shirts arrived at the hotel.

  *

    *Updated: forums situation*
    minx from OpenVPN Inc. originally set up flarum and started
    migration process, but he left the company before completion.
    Now we have eduardo at OpenVPN Inc. who is taking a look and he
    managed to get migration working.
    He suggests some further migration testing, and to then plan a hard
    cutover date.

  *

    *Updated: wolfSSL license changed from gplv2 to gplv3*
    The release notes of wolfSSL indicate the license changed to GPLv3.
    This basically makes wolfSSL incompatible in regards to licensing
    with OpenVPN.
    Currently trying to arrange a meeting between relevant parties.

As always you're welcome to join at #openvpn-meeting on Libera IRC 
network every Wednesday at 14:00 Central European Time.

Kind regards,
Johan Draaisma

Re: [Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

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

Hi,

On Tue, Oct 21, 2025 at 10:34:21PM +0200, Arne Schwabe wrote:
> Before commiting we have to check that port-share does not rely on this
> behaviour to pass the fd the forked instances. I didn't check right now.

Good point.  I have a port-share test instance, will test.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...

Re: [Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

[Arne S. <ar...@rf...>]

Am 21.10.2025 um 21:40 schrieb Gert Doering:
> Hi,
>
> On Tue, Oct 21, 2025 at 06:11:06PM +0000, Joshua Rogers via Openvpn-devel wrote:
>> The accept path calls set_cloexec(sd) after accept(). That re-flags the
>> listening socket, which is already CLOEXEC from create_socket_tcp(), and
>> leaves new_sd inheritable. As a result, client-connect and auth scripts
>> spawned after accept can inherit the connected socket and read or write
>> the raw TCP stream. This defeats the stated intent to prevent scripts from
>> accessing the client socket.
> Impressive find.  I had to actually look at the code to see what
> you are talking about :-)
>
> So we do
>
>          new_sd = accept(sd, &act->dest.addr.sa, &remote_len);
>
> and then
>
>          /* set socket file descriptor to not pass across execs, so that
>           * scripts don't have access to it */
>          set_cloexec(sd);
>
Before commiting we have to check that port-share does not rely on this 
behaviour to pass the fd the forked instances. I didn't check right now.

Arne

[Openvpn-devel] [PATCH applied] Re: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Bad API hack, correct fix...  verified by going to mroute.c and 
reading up on what mroute_extract_openvpn_sockaddr() does with
"addr.proto" which should be an *output* structure, but this
field is used as input as well...  can someone fix this for good,
please, after 2.7 release?

(Quite an impressive find by GCC)

Your patch has been applied to the master branch.

commit 0abf6e716b5a50b2b7f4287b1d50f4889eed36aa
Author: Frank Lichtenheld
Date:   Tue Oct 21 21:31:40 2025 +0200

     multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

     Signed-off-by: Frank Lichtenheld <fr...@li...>
     Acked-by: Gert Doering <ge...@gr...>
     Acked-by: Gianmarco De Gregori <gia...@ma...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1292
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg33830.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

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

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


Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................

multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

maddr.proto needs to be set before the call since that
will change the behavior.

Found by GCC "'maddr.proto' is used uninitialized"

Change-Id: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Acked-by: Gianmarco De Gregori <gia...@ma...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1292
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33830.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/multi.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1292/3

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..fa9c654 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3965,9 +3965,9 @@
     saddr.addr.in4.sin_family = AF_INET;
     saddr.addr.in4.sin_addr.s_addr = htonl(addr);
     saddr.addr.in4.sin_port = htons(port);
+    maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 3
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: its_Giaan <gia...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

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

Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................

multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

maddr.proto needs to be set before the call since that
will change the behavior.

Found by GCC "'maddr.proto' is used uninitialized"

Change-Id: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Acked-by: Gianmarco De Gregori <gia...@ma...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1292
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33830.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/multi.c
1 file changed, 1 insertion(+), 1 deletion(-)




diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..fa9c654 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3965,9 +3965,9 @@
     saddr.addr.in4.sin_family = AF_INET;
     saddr.addr.in4.sin_addr.s_addr = htonl(addr);
     saddr.addr.in4.sin_port = htons(port);
+    maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 3
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: its_Giaan <gia...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

Re: [Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

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

Hi,

On Tue, Oct 21, 2025 at 06:11:06PM +0000, Joshua Rogers via Openvpn-devel wrote:
> The accept path calls set_cloexec(sd) after accept(). That re-flags the
> listening socket, which is already CLOEXEC from create_socket_tcp(), and
> leaves new_sd inheritable. As a result, client-connect and auth scripts
> spawned after accept can inherit the connected socket and read or write
> the raw TCP stream. This defeats the stated intent to prevent scripts from
> accessing the client socket.

Impressive find.  I had to actually look at the code to see what
you are talking about :-)

So we do 

        new_sd = accept(sd, &act->dest.addr.sa, &remote_len);

and then

        /* set socket file descriptor to not pass across execs, so that
         * scripts don't have access to it */
        set_cloexec(sd);

    return new_sd;

which very clearly is not intended behaviour.

So,

Acked-by: ge...@gr...

will deal with it "as soon as possible" which will take a few days.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...

[Openvpn-devel] [PATCH v2] multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

From: Frank Lichtenheld <fr...@li...>

maddr.proto needs to be set before the call since that
will change the behavior.

Found by GCC "'maddr.proto' is used uninitialized"

Change-Id: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Acked-by: Gianmarco De Gregori <gia...@ma...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1292
---

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/+/1292
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>
Gianmarco De Gregori <gia...@ma...>

        
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..fa9c654 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3965,9 +3965,9 @@
     saddr.addr.in4.sin_family = AF_INET;
     saddr.addr.in4.sin_addr.s_addr = htonl(addr);
     saddr.addr.in4.sin_port = htons(port);
+    maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Attention is currently required from: flichtenheld, plaisthos.

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

Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

Patchset:

PS2: 
Oh, sorry.  That was a gerrit comparison artefact.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: its_Giaan <gia...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 21 Oct 2025 19:31:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Attention is currently required from: flichtenheld, plaisthos.

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

Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................


Patch Set 2: Code-Review-2

(1 comment)

Patchset:

PS2: 
v2 has some stuff in there that doesn't belong here (push.c, ssl_verify.c integers)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: its_Giaan <gia...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 21 Oct 2025 19:30:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [PATCH] ssl_ncp: make tls_item_in_cipher_list zero-alloc and linear

[Joshua R. <co...@jo...>]

This avoids avoids string_alloc + strtok on every membership check, which
for very long IV_CIPHERS with many colon-separated tokens, forced O(N*M)
comparisons with I(M) allocations in a single negotiatin.

[Openvpn-devel] [S] Change in openvpn[master]: push: Fix conversion issues related to timeout in send_auth_pending_m...

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

Attention is currently required from: plaisthos.

Hello plaisthos,

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

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

to review the following change.


Change subject: push: Fix conversion issues related to timeout in send_auth_pending_messages
......................................................................

push: Fix conversion issues related to timeout in send_auth_pending_messages

Add additional checking to make sure that the required
casts are safe.

Change-Id: Icc31b7fa0da86220df45552aecc15dc6c769cd54
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/push.c
M src/openvpn/ssl_verify.c
2 files changed, 15 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/93/1293/1

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6f146fc..a6f979d 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -429,11 +429,6 @@
     gc_free(&gc);
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
 bool
 send_auth_pending_messages(struct tls_multi *tls_multi, struct tls_session *session,
                            const char *extra, unsigned int timeout)
@@ -449,7 +444,12 @@
     /* Calculate the maximum timeout and subtract the time we already waited */
     unsigned int max_timeout =
         max_uint(tls_multi->opt.renegotiate_seconds / 2, tls_multi->opt.handshake_window);
-    max_timeout = max_timeout - (now - ks->initial);
+    time_t time_elapsed = now - ks->initial;
+    if (time_elapsed < 0 || time_elapsed >= (time_t)max_timeout)
+    {
+        return false;
+    }
+    max_timeout -= (unsigned int)time_elapsed;
     timeout = min_uint(max_timeout, timeout);
 
     struct gc_arena gc = gc_new();
@@ -734,6 +734,11 @@
     return true;
 }
 
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wconversion"
+#endif
+
 static bool
 send_push_options(struct context *c, struct buffer *buf, struct push_list *push_list, int safe_cap,
                   bool *push_sent, bool *multi_push)
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 04ef27e..fe95621 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -874,11 +874,6 @@
     return supported;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
 /**
  *  Checks if the deferred state should also send auth pending
  *  request to the client. Also removes the auth_pending control file
@@ -915,8 +910,9 @@
             buf_chomp(iv_buf);
             buf_chomp(extra_buf);
 
+            errno = 0;
             long timeout = strtol(BSTR(timeout_buf), NULL, 10);
-            if (timeout == 0)
+            if (timeout <= 0 || timeout > UINT_MAX || errno)
             {
                 msg(M_WARN, "could not parse auth pending file timeout");
                 buffer_list_free(lines);
@@ -933,14 +929,13 @@
                          pending_method);
                 auth_set_client_reason(multi, buf);
                 msg(M_INFO,
-                    "Client does not supported auth pending method "
-                    "'%s'",
+                    "Client does not supported auth pending method '%s'",
                     pending_method);
                 ret = false;
             }
             else
             {
-                send_auth_pending_messages(multi, session, BSTR(extra_buf), timeout);
+                send_auth_pending_messages(multi, session, BSTR(extra_buf), (unsigned int)timeout);
             }
         }
 
@@ -950,10 +945,6 @@
     return ret;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 /**
  *  Removes auth_pending and auth_control files from file system
  *  and key_state structure

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1293?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: Icc31b7fa0da86220df45552aecc15dc6c769cd54
Gerrit-Change-Number: 1293
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>

[Openvpn-devel] [S] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Attention is currently required from: plaisthos.

Hello plaisthos,

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

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

to review the following change.


Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................

multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

maddr.proto needs to be set before the call since that
will change the behavior.

Found by GCC "'maddr.proto' is used uninitialized"

Change-Id: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/multi.c
M src/openvpn/push.c
M src/openvpn/ssl_verify.c
3 files changed, 16 insertions(+), 20 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1292/1

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..fa9c654 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3965,9 +3965,9 @@
     saddr.addr.in4.sin_family = AF_INET;
     saddr.addr.in4.sin_addr.s_addr = htonl(addr);
     saddr.addr.in4.sin_port = htons(port);
+    maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..1871816 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -429,11 +429,6 @@
     gc_free(&gc);
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
 bool
 send_auth_pending_messages(struct tls_multi *tls_multi, struct tls_session *session,
                            const char *extra, unsigned int timeout)
@@ -449,7 +444,12 @@
     /* Calculate the maximum timeout and subtract the time we already waited */
     unsigned int max_timeout =
         max_uint(tls_multi->opt.renegotiate_seconds / 2, tls_multi->opt.handshake_window);
-    max_timeout = max_timeout - (now - ks->initial);
+    time_t time_elapsed = now - ks->initial;
+    if (time_elapsed < 0 || time_elapsed >= (time_t)max_timeout)
+    {
+        return false;
+    }
+    max_timeout -= (unsigned int)time_elapsed;
     timeout = min_uint(max_timeout, timeout);
 
     struct gc_arena gc = gc_new();
@@ -734,6 +734,11 @@
     return true;
 }
 
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wconversion"
+#endif
+
 static bool
 send_push_options(struct context *c, struct buffer *buf, struct push_list *push_list, int safe_cap,
                   bool *push_sent, bool *multi_push)
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 04ef27e..fe95621 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -874,11 +874,6 @@
     return supported;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
 /**
  *  Checks if the deferred state should also send auth pending
  *  request to the client. Also removes the auth_pending control file
@@ -915,8 +910,9 @@
             buf_chomp(iv_buf);
             buf_chomp(extra_buf);
 
+            errno = 0;
             long timeout = strtol(BSTR(timeout_buf), NULL, 10);
-            if (timeout == 0)
+            if (timeout <= 0 || timeout > UINT_MAX || errno)
             {
                 msg(M_WARN, "could not parse auth pending file timeout");
                 buffer_list_free(lines);
@@ -933,14 +929,13 @@
                          pending_method);
                 auth_set_client_reason(multi, buf);
                 msg(M_INFO,
-                    "Client does not supported auth pending method "
-                    "'%s'",
+                    "Client does not supported auth pending method '%s'",
                     pending_method);
                 ret = false;
             }
             else
             {
-                send_auth_pending_messages(multi, session, BSTR(extra_buf), timeout);
+                send_auth_pending_messages(multi, session, BSTR(extra_buf), (unsigned int)timeout);
             }
         }
 
@@ -950,10 +945,6 @@
     return ret;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 /**
  *  Removes auth_pending and auth_control files from file system
  *  and key_state structure

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Attention is currently required from: plaisthos.

Hello plaisthos, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).


Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................

multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

maddr.proto needs to be set before the call since that
will change the behavior.

Found by GCC "'maddr.proto' is used uninitialized"

Change-Id: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/multi.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1292/2

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..fa9c654 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3965,9 +3965,9 @@
     saddr.addr.in4.sin_family = AF_INET;
     saddr.addr.in4.sin_addr.s_addr = htonl(addr);
     saddr.addr.in4.sin_port = htons(port);
+    maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>

[Openvpn-devel] [PATCH] tcp: apply CLOEXEC to accepted socket, not listener

[Joshua R. <co...@jo...>]

The accept path calls set_cloexec(sd) after accept(). That re-flags the
listening socket, which is already CLOEXEC from create_socket_tcp(), and
leaves new_sd inheritable. As a result, client-connect and auth scripts
spawned after accept can inherit the connected socket and read or write
the raw TCP stream. This defeats the stated intent to prevent scripts from
accessing the client socket.
This bug was found using ZeroPath.

[Openvpn-devel] [XS] Change in openvpn[master]: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr

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

Attention is currently required from: flichtenheld, plaisthos.

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

Change subject: multi: Fix wrong usage of mroute_extract_openvpn_sockaddr
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1292?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: I76babf08b041162ddedf7a9b7c2799847f15cbdc
Gerrit-Change-Number: 1292
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: its_Giaan <gia...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 21 Oct 2025 17:35:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes

[Openvpn-devel] [M] Change in openvpn[master]: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route

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

Attention is currently required from: cron2, flichtenheld, plaisthos.

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

Change subject: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route
......................................................................


Patch Set 12:

(2 comments)

Patchset:

PS11: 
> This changes the logic of adding routes on all platforms in a very drastic way. […]
Done


File src/openvpn/route.c:

http://gerrit.openvpn.net/c/openvpn/+/677/comment/2b9f3a14_b1ffc52f?usp=email :
PS11, Line 1606:             gateway_needed = true;
> Sure but why is the gateway needed in this case? […]
Done



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/677?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: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1
Gerrit-Change-Number: 677
Gerrit-PatchSet: 12
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 21 Oct 2025 14:10:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arn...@rf...>

[Openvpn-devel] [L] Change in openvpn[master]: route: extended logic to omit gateway when unnecessary

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

Attention is currently required from: flichtenheld, ordex, plaisthos.

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

Change subject: route: extended logic to omit gateway when unnecessary
......................................................................


Patch Set 12:

(6 comments)

File src/openvpn/route.h:

http://gerrit.openvpn.net/c/openvpn/+/721/comment/0a1b545d_f913bbc8?usp=email :
PS11, Line 274: bool add_route_ipv6(struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint);
> The naming here and the usage seem to disagree. […]
Done


File src/openvpn/route.c:

http://gerrit.openvpn.net/c/openvpn/+/721/comment/aaa6380b_66ab5947?usp=email :
PS11, Line 1811:     /* FIXME -- add on-link support for Dragonfly */
> Has this been really tested on DragonFly BSD?
i tested on the first patch but i tested again the #12 on 6.4.2 and found also a bug with the ifconfig, i fixed just moving from #if defined(TARGET_FREEBSD) || defined (TARGET_DRAGONFLY) code to #if defined(TARGET_NETBSD) || defined (TARGET_DRAGONFLY) code in do_ifconfig_ipv4(). Here the logs:

master:
2025-10-20 13:36:42 PUSH: Received control message: 'PUSH_REPLY,route-gateway 10.23.100.1,topology subnet,ping 5,ping-restart 30,ifconfig 10.23.100.2 255.255.255.0,peer-id 0,cipher AES-256-GCM,protocol-flags cc-exit tls-ekm dyn-tls-crypt,tun-mtu 1500'
2025-10-20 13:36:42 OPTIONS IMPORT: --ifconfig/up options modified
2025-10-20 13:36:42 OPTIONS IMPORT: route-related options modified
2025-10-20 13:36:42 OPTIONS IMPORT: tun-mtu set to 1500
2025-10-20 13:36:42 TUN/TAP device tun0 exists previously, keep at program end
2025-10-20 13:36:42 TUN/TAP device /dev/tun0 opened
2025-10-20 13:36:42 tun/tap device [tun0] opened

2025-10-20 13:36:42 /sbin/ifconfig tun0 10.23.100.2/24 mtu 1500 up
ifconfig: ioctl (SIOCAIFADDR): Destination address required
2025-10-20 13:36:42 FreeBSD ifconfig failed: external program exited with error status: 1
2025-10-20 13:36:42 Exiting due to fatal error


patchset #12:
2025-10-20 13:43:42 PUSH: Received control message: 'PUSH_REPLY,route-gateway 10.23.100.1,topology subnet,ping 5,ping-restart 30,ifconfig 10.23.100.2 255.255.255.0,peer-id 0,cipher AES-256-GCM,protocol-flags cc-exit tls-ekm dyn-tls-crypt,tun-mtu 1500'
2025-10-20 13:43:42 OPTIONS IMPORT: --ifconfig/up options modified
2025-10-20 13:43:42 OPTIONS IMPORT: route-related options modified
2025-10-20 13:43:42 OPTIONS IMPORT: tun-mtu set to 1500
2025-10-20 13:43:42 TUN/TAP device tun0 exists previously, keep at program end
2025-10-20 13:43:42 TUN/TAP device /dev/tun0 opened
2025-10-20 13:43:42 tun/tap device [tun0] opened

2025-10-20 13:43:42 /sbin/ifconfig tun0 10.23.100.2 10.23.100.1 mtu 1500 netmask 255.255.255.0 up
2025-10-20 13:43:42 /sbin/route add -net 10.23.100.0 -iface tun0
add net 10.23.100.0: gateway tun0

2025-10-20 13:43:42 Initialization Sequence Completed


If you have any thought about it or something to suggest (or some particular test) please feel free to share


http://gerrit.openvpn.net/c/openvpn/+/721/comment/b2bc803a_7e364e02?usp=email :
PS11, Line 1600:         && !( (r4->flags & RT_METRIC_DEFINED) && r4->metric == 0 ) )
> how does the metric play into needing a gateway or not?
Done


http://gerrit.openvpn.net/c/openvpn/+/721/comment/2a5d88e2_1d00ff0a?usp=email :
PS11, Line 1607:         return true;
> There should at least be some idea/comment in the source code as well. […]
Done


http://gerrit.openvpn.net/c/openvpn/+/721/comment/85295ff5_ccdfe664?usp=email :
PS11, Line 1898:     }
> Has this been tested on Open and NetBSD?
it was already tested from the first patch but i retested the latest patch on dradonfly 6.4.2, openbsd 7.7, freebsd 14.3, netbsd 10.0 and MacOS sonoma


http://gerrit.openvpn.net/c/openvpn/+/721/comment/0b6b4458_7d49c544?usp=email :
PS11, Line 2010: {
> I think this duplicates the code that was publicly accessible in https://gerrit.openvpn. […]
Done



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/721?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: I87777e74b1fd34781e1d72c9f994eb84f39d800c
Gerrit-Change-Number: 721
Gerrit-PatchSet: 12
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-CC: ordex <an...@ma...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: ordex <an...@ma...>
Gerrit-Comment-Date: Tue, 21 Oct 2025 14:10:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arn...@rf...>
Comment-In-Reply-To: ordex <an...@ma...>

[Openvpn-devel] [L] Change in openvpn[master]: route: extended logic to omit gateway when unnecessary

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

Attention is currently required from: flichtenheld, mrbff.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#12).


Change subject: route: extended logic to omit gateway when unnecessary
......................................................................

route: extended logic to omit gateway when unnecessary

Extracted and extended the logic behind gateway_needed both in
add_route() and add_route_ipv6(). Checking dev-type, special
routes, if the gateway is on-link and if the gateway is in the
vpn subnet. Additionally, extended support for these checks and
conditions to DARWIN and BSD-based operating systems.

These changes ensure that the gateway is only included when
necessary, optimizing route configuration and potentially
reducing redundant route entries.

Additionally, Dragonfly's do_ifconfig_ipv4() code is now shared
with NetBSD's, rather than FreeBSD's.

Change-Id: I87777e74b1fd34781e1d72c9f994eb84f39d800c
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/route.c
M src/openvpn/tun.c
2 files changed, 210 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/721/12

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index c249b38..4dc4005 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -42,6 +42,7 @@
 #include "options.h"
 #include "networking.h"
 #include "integer.h"
+#include "openvpn.h"
 
 #include "memdbg.h"
 
@@ -1396,6 +1397,7 @@
     }
     gc_free(&gc);
 }
+
 void
 setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6)
 {
@@ -1472,6 +1474,37 @@
 }
 #endif
 
+static bool
+is_gateway_needed_ipv4(const struct route_ipv4 *r4,
+                       const struct route_gateway_info *rgi,
+                       const struct tuntap *tt)
+{
+#ifndef _WIN32
+    /* server told us which interface to use; pass gateway if it gave one */
+    if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0)
+    {
+        if (rgi->flags & RGI_ADDR_DEFINED && r4->gateway != 0)
+        {
+            return true;
+        }
+    }
+#endif
+
+    /* TAP is an L2 device, so for destinations that are not explicitly
+     * on-link the kernel needs a gateway so it can ARP/ND for the
+     * MAC address. We make an exception only when the user explicitly sets
+     * metric 0, which is taken as a request for an on-link “route to
+     * interface” (no gateway). This keeps parity with the IPv6 code.
+     */
+    if (tt->type == DEV_TYPE_TAP
+        && !((r4->flags & RT_METRIC_DEFINED) && r4->metric == 0))
+    {
+        return true;
+    }
+
+    return false;
+}
+
 bool
 add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags,
           const struct route_gateway_info *rgi, /* may be NULL */
@@ -1503,28 +1536,7 @@
         goto done;
     }
 
-#ifndef _WIN32
-    /* server told us which interface to use; pass gateway if it gave one */
-    if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0)
-    {
-        if (rgi->flags & RGI_ADDR_DEFINED && r->gateway != 0)
-        {
-            gateway_needed = true;
-        }
-    }
-#endif
-
-    /* TAP is an L2 device, so for destinations that are not explicitly
-     * on-link the kernel needs a gateway so it can ARP/ND for the
-     * MAC address. We make an exception only when the user explicitly sets
-     * metric 0, which is taken as a request for an on-link “route to
-     * interface” (no gateway). This keeps parity with the IPv6 code.
-     */
-    if (tt->type == DEV_TYPE_TAP
-        && !((r->flags & RT_METRIC_DEFINED) && r->metric == 0))
-    {
-        gateway_needed = true;
-    }
+    gateway_needed = is_gateway_needed_ipv4(r, rgi, tt);
 
     if (gateway_needed && r->gateway == 0)
     {
@@ -1537,18 +1549,21 @@
         goto done;
     }
 
-#if defined(TARGET_LINUX)
-    const char *iface = tt->actual_name;
+#if !defined(_WIN32) && !defined(TARGET_SOLARIS) \
+    && !defined(TARGET_AIX)
+    const char *device = tt->actual_name;
     if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */
     {
-        iface = rgi->iface;
+        device = rgi->iface;
     }
+#endif
 
+#if defined(TARGET_LINUX)
     int metric = -1;
 
     if (is_on_link(is_local_route, flags, rgi))
     {
-        iface = rgi->iface;
+        device = rgi->iface;
     }
 
     if (r->flags & RT_METRIC_DEFINED)
@@ -1560,7 +1575,7 @@
     status = RTA_SUCCESS;
     int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask),
                                gateway_needed ? &r->gateway : NULL,
-                               iface, r->table_id, metric);
+                               device, r->table_id, metric);
     if (ret == -EEXIST)
     {
         msg(D_ROUTE, "NOTE: Linux route add command failed because route exists");
@@ -1577,7 +1592,7 @@
 
     if (rgi)
     {
-        snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, rgi->iface);
+        snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, device);
     }
     else
     {
@@ -1672,7 +1687,7 @@
     bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: Solaris route add command failed");
     status = ret ? RTA_SUCCESS : RTA_ERROR;
 
-#elif defined(TARGET_FREEBSD)
+#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
 
     argv_printf(&argv, "%s add", ROUTE_PATH);
 
@@ -1683,31 +1698,18 @@
     }
 #endif
 
-    argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
-
-    /* FIXME -- add on-link support for FreeBSD */
-
-    argv_msg(D_ROUTE, &argv);
-    bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: FreeBSD route add command failed");
-    status = ret ? RTA_SUCCESS : RTA_ERROR;
-
-#elif defined(TARGET_DRAGONFLY)
-
-    argv_printf(&argv, "%s add", ROUTE_PATH);
-
-#if 0
-    if (r->flags & RT_METRIC_DEFINED)
+    if (gateway_needed)
     {
-        argv_printf_cat(&argv, "-rtt %d", r->metric);
+        argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
     }
-#endif
-
-    argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
-
-    /* FIXME -- add on-link support for Dragonfly */
+    else
+    {
+        argv_printf_cat(&argv, "-net %s -iface %s", network, device);
+    }
 
     argv_msg(D_ROUTE, &argv);
-    bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: DragonFly route add command failed");
+    bool ret = openvpn_execve_check(&argv, es, 0,
+                                    "ERROR: BSD route add command failed");
     status = ret ? RTA_SUCCESS : RTA_ERROR;
 
 #elif defined(TARGET_DARWIN)
@@ -1730,7 +1732,14 @@
     }
     else
     {
-        argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
+        if (gateway_needed)
+        {
+            argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
+        }
+        else
+        {
+            argv_printf_cat(&argv, "-net %s -interface %s", network, device);
+        }
     }
 
     argv_msg(D_ROUTE, &argv);
@@ -1748,9 +1757,14 @@
     }
 #endif
 
-    argv_printf_cat(&argv, "-net %s %s -netmask %s", network, gateway, netmask);
-
-    /* FIXME -- add on-link support for OpenBSD/NetBSD */
+    if (gateway_needed)
+    {
+        argv_printf_cat(&argv, "-net %s %s -netmask %s", network, gateway, netmask);
+    }
+    else
+    {
+        argv_printf_cat(&argv, "-net %s -netmask %s -link -iface %s", network, netmask, device);
+    }
 
     argv_msg(D_ROUTE, &argv);
     bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route add command failed");
@@ -1827,6 +1841,47 @@
     }
 }
 
+static bool
+is_gateway_needed_ipv6(const struct route_ipv6 *r6,
+                       const struct tuntap *tt,
+                       const char *network)
+{
+#ifndef _WIN32
+    if (r6->iface != NULL) /* VPN server special route */
+    {
+        if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway))
+        {
+            return true;
+        }
+    }
+#endif
+
+    /*
+     * Filter out routes which are essentially no-ops
+     * (not currently done for IPv6)
+     */
+
+    /* On "tun" interface, we never set a gateway if the operating system
+     * can do "route to interface" - it does not add value, as the target
+     * dev already fully qualifies the route destination on point-to-point
+     * interfaces.   OTOH, on "tap" interface, we must always set the
+     * gateway unless the route is to be an on-link network
+     */
+    if (tt->type == DEV_TYPE_TAP
+        && !((r6->flags & RT_METRIC_DEFINED) && r6->metric == 0))
+    {
+        return true;
+    }
+
+    if (ipv6_net_contains_host(&tt->local_ipv6, tt->netbits_ipv6, &r6->gateway))
+    {
+        msg(D_ROUTE, "Ignoring gateway in VPN subnet for route %s/%d", network, r6->netbits);
+        return false;
+    }
+
+    return false;
+}
+
 bool
 add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flags,
                const struct env_set *es, openvpn_net_ctx_t *ctx)
@@ -1842,22 +1897,12 @@
     struct argv argv = argv_new();
     struct gc_arena gc = gc_new();
 
-#ifndef _WIN32
-    const char *device = tt->actual_name;
-    if (r6->iface != NULL) /* vpn server special route */
-    {
-        device = r6->iface;
-        if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway))
-        {
-            gateway_needed = true;
-        }
-    }
-#endif
-
     route_ipv6_clear_host_bits(r6);
     const char *network = print_in6_addr(r6->network, 0, &gc);
     const char *gateway = print_in6_addr(r6->gateway, 0, &gc);
 
+    gateway_needed = is_gateway_needed_ipv6(r6, tt, network);
+
 #if defined(TARGET_DARWIN) || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \
     || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
@@ -1875,6 +1920,15 @@
     }
 #endif
 
+#if !defined(_WIN32)
+
+    const char *device = tt->actual_name;
+    if (r6->iface != NULL)
+    {
+        device = r6->iface;
+    }
+#endif
+
 #ifndef _WIN32
     msg(D_ROUTE, "add_route_ipv6(%s/%d -> %s metric %d) dev %s", network, r6->netbits, gateway,
         r6->metric, device);
@@ -1883,22 +1937,6 @@
         r6->metric, r6->adapter_index ? r6->adapter_index : tt->adapter_index);
 #endif
 
-    /*
-     * Filter out routes which are essentially no-ops
-     * (not currently done for IPv6)
-     */
-
-    /* On "tun" interface, we never set a gateway if the operating system
-     * can do "route to interface" - it does not add value, as the target
-     * dev already fully qualifies the route destination on point-to-point
-     * interfaces.   OTOH, on "tap" interface, we must always set the
-     * gateway unless the route is to be an on-link network
-     */
-    if (tt->type == DEV_TYPE_TAP && !((r6->flags & RT_METRIC_DEFINED) && r6->metric == 0))
-    {
-        gateway_needed = true;
-    }
-
     if (gateway_needed && IN6_IS_ADDR_UNSPECIFIED(&r6->gateway))
     {
         msg(M_WARN,
@@ -1913,6 +1951,7 @@
 
 #if defined(TARGET_LINUX)
     int metric = -1;
+
     if ((r6->flags & RT_METRIC_DEFINED) && (r6->metric > 0))
     {
         metric = r6->metric;
@@ -2008,21 +2047,21 @@
     bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: MacOS X route add -inet6 command failed");
     status = ret ? RTA_SUCCESS : RTA_ERROR;
 
-#elif defined(TARGET_OPENBSD)
+#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
-    argv_printf(&argv, "%s add -inet6 %s -prefixlen %d %s", ROUTE_PATH, network, r6->netbits,
-                gateway);
+    argv_printf(&argv, "%s add -inet6 %s -prefixlen %d ", ROUTE_PATH, network, r6->netbits);
+
+    if (gateway_needed)
+    {
+        argv_printf_cat(&argv, "%s", gateway);
+    }
+    else
+    {
+        argv_printf_cat(&argv, "-link -iface %s", device);
+    }
 
     argv_msg(D_ROUTE, &argv);
-    bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD route add -inet6 command failed");
-    status = ret ? RTA_SUCCESS : RTA_ERROR;
-
-#elif defined(TARGET_NETBSD)
-
-    argv_printf(&argv, "%s add -inet6 %s/%d %s", ROUTE_PATH, network, r6->netbits, gateway);
-
-    argv_msg(D_ROUTE, &argv);
-    bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: NetBSD route add -inet6 command failed");
+    bool ret = openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route add -inet6 command failed");
     status = ret ? RTA_SUCCESS : RTA_ERROR;
 
 #elif defined(TARGET_AIX)
@@ -2080,6 +2119,21 @@
 #endif
     int is_local_route;
 
+#if defined(TARGET_DARWIN) || defined(TARGET_LINUX)         \
+    || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \
+    || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
+
+    bool gateway_needed = is_gateway_needed_ipv4(r, rgi, tt);
+
+#if !defined(TARGET_OPENBSD) && !defined(TARGET_NETBSD)
+    const char *device = tt->actual_name;
+    if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */
+    {
+        device = rgi->iface;
+    }
+#endif
+#endif
+
     if ((r->flags & (RT_DEFINED | RT_ADDED)) != (RT_DEFINED | RT_ADDED))
     {
         return;
@@ -2106,12 +2160,19 @@
 
 #if defined(TARGET_LINUX)
     metric = -1;
+
+    if (is_on_link(is_local_route, flags, rgi))
+    {
+        device = rgi->iface;
+    }
+
     if (r->flags & RT_METRIC_DEFINED)
     {
         metric = r->metric;
     }
 
-    if (net_route_v4_del(ctx, &r->network, netmask_to_netbits2(r->netmask), &r->gateway, NULL,
+    if (net_route_v4_del(ctx, &r->network, netmask_to_netbits2(r->netmask),
+                         gateway_needed ? &r->gateway : NULL, device,
                          r->table_id, metric)
         < 0)
     {
@@ -2165,19 +2226,21 @@
     argv_msg(D_ROUTE, &argv);
     openvpn_execve_check(&argv, es, 0, "ERROR: Solaris route delete command failed");
 
-#elif defined(TARGET_FREEBSD)
+#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
 
-    argv_printf(&argv, "%s delete -net %s %s %s", ROUTE_PATH, network, gateway, netmask);
+    argv_printf(&argv, "%s delete", ROUTE_PATH);
+
+    if (gateway_needed)
+    {
+        argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
+    }
+    else
+    {
+        argv_printf_cat(&argv, "-net %s -iface %s", network, device);
+    }
 
     argv_msg(D_ROUTE, &argv);
-    openvpn_execve_check(&argv, es, 0, "ERROR: FreeBSD route delete command failed");
-
-#elif defined(TARGET_DRAGONFLY)
-
-    argv_printf(&argv, "%s delete -net %s %s %s", ROUTE_PATH, network, gateway, netmask);
-
-    argv_msg(D_ROUTE, &argv);
-    openvpn_execve_check(&argv, es, 0, "ERROR: DragonFly route delete command failed");
+    openvpn_execve_check(&argv, es, 0, "ERROR: BSD route delete command failed");
 
 #elif defined(TARGET_DARWIN)
 
@@ -2188,7 +2251,16 @@
     }
     else
     {
-        argv_printf(&argv, "%s delete -net %s %s %s", ROUTE_PATH, network, gateway, netmask);
+        argv_printf(&argv, "%s delete ", ROUTE_PATH);
+
+        if (gateway_needed)
+        {
+            argv_printf_cat(&argv, "-net %s %s %s", network, gateway, netmask);
+        }
+        else
+        {
+            argv_printf_cat(&argv, "-net %s -interface %s", network, device);
+        }
     }
 
     argv_msg(D_ROUTE, &argv);
@@ -2196,7 +2268,16 @@
 
 #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
-    argv_printf(&argv, "%s delete -net %s %s -netmask %s", ROUTE_PATH, network, gateway, netmask);
+    argv_printf(&argv, "%s delete ", ROUTE_PATH);
+
+    if (gateway_needed)
+    {
+        argv_printf_cat(&argv, "-net %s %s -netmask %s", network, gateway, netmask);
+    }
+    else
+    {
+        argv_printf_cat(&argv, "-net %s -netmask %s", network, netmask);
+    }
 
     argv_msg(D_ROUTE, &argv);
     openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete command failed");
@@ -2250,23 +2331,16 @@
 #if !defined(TARGET_LINUX)
     const char *gateway;
 #endif
-#if !defined(TARGET_SOLARIS)
-    bool gateway_needed = false;
+#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \
+    || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
+
     const char *device = tt->actual_name;
     if (r6->iface != NULL) /* vpn server special route */
     {
         device = r6->iface;
-        gateway_needed = true;
     }
     (void)device; /* unused on some platforms */
 
-    /* if we used a gateway on "add route", we also need to specify it on
-     * delete, otherwise some OSes will refuse to delete the route
-     */
-    if (tt->type == DEV_TYPE_TAP && !((r6->flags & RT_METRIC_DEFINED) && r6->metric == 0))
-    {
-        gateway_needed = true;
-    }
 #endif
 #endif
 
@@ -2278,6 +2352,15 @@
     gateway = print_in6_addr(r6->gateway, 0, &gc);
 #endif
 
+#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) || defined(TARGET_FREEBSD) \
+    || defined(TARGET_DRAGONFLY) || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
+    /* if we used a gateway on "add route", we also need to specify it on
+     * delete, otherwise some OSes will refuse to delete the route
+     */
+    bool gateway_needed = false;
+    gateway_needed = is_gateway_needed_ipv6(r6, tt, network);
+#endif
+
 #if defined(TARGET_DARWIN) || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \
     || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
@@ -2362,20 +2445,17 @@
     argv_msg(D_ROUTE, &argv);
     openvpn_execve_check(&argv, es, 0, "ERROR: MacOS X route delete -inet6 command failed");
 
-#elif defined(TARGET_OPENBSD)
+#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
-    argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d %s", ROUTE_PATH, network, r6->netbits,
-                gateway);
+    argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d", ROUTE_PATH, network, r6->netbits);
+
+    if (gateway_needed)
+    {
+        argv_printf_cat(&argv, "%s", gateway);
+    }
 
     argv_msg(D_ROUTE, &argv);
-    openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD route delete -inet6 command failed");
-
-#elif defined(TARGET_NETBSD)
-
-    argv_printf(&argv, "%s delete -inet6 %s/%d %s", ROUTE_PATH, network, r6->netbits, gateway);
-
-    argv_msg(D_ROUTE, &argv);
-    openvpn_execve_check(&argv, es, 0, "ERROR: NetBSD route delete -inet6 command failed");
+    openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete -inet6 command failed");
 
 #elif defined(TARGET_AIX)
 
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 06b7ae5..0df946b 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1383,7 +1383,7 @@
         add_route(&r, tt, 0, NULL, es, NULL);
     }
 
-#elif defined(TARGET_NETBSD)
+#elif defined(TARGET_NETBSD) || defined(TARGET_DRAGONFLY)
     in_addr_t remote_end = INADDR_ANY; /* for "virtual" subnet topology */
 
     if (tun_p2p)
@@ -1466,7 +1466,7 @@
         add_route(&r, tt, 0, NULL, es, NULL);
     }
 
-#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
+#elif defined(TARGET_FREEBSD)
 
     /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
     if (tun_p2p) /* point-to-point tun */

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/721?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: I87777e74b1fd34781e1d72c9f994eb84f39d800c
Gerrit-Change-Number: 721
Gerrit-PatchSet: 12
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-CC: ordex <an...@ma...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>

[Openvpn-devel] [M] Change in openvpn[master]: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route

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

Attention is currently required from: cron2, flichtenheld, mrbff, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#12).

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


Change subject: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route
......................................................................

route: copied 'gateway_needed' logic from add_route_ipv6 to add_route

Under certain circumstances it may not be necessary to pass the
gateway when adding a new route via net_route_v4_add() API function.
add_route_ipv6() already accounts for some of these cases and
therefore this patch copies the same logic to add_route().

Change-Id: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1
Signed-off-by: Marco Baffo <ma...@ma...>
---
M Changes.rst
M src/openvpn/route.c
2 files changed, 51 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/77/677/12

diff --git a/Changes.rst b/Changes.rst
index 4feacad2..a3e00c2 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -170,6 +170,13 @@
 COPYING: license details only relevant to our Windows installers have
    been updated and moved to the openvpn-build repo
 
+The route gateway is now only added for IPv4 when strictly necessary.
+  The gateway-needed logic previously applied to IPv6 only has been copied
+  and extended to IPv4 route handling.  Route additions now omit the
+  gateway unless required after checking device type, special routes,
+  on-link gateways, and whether the gateway lies inside the VPN subnet,
+  optimizing route configuration and potentially reducing redundant route
+  entries.
 
 Deprecated features
 -------------------
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 7d988da..c249b38 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -1479,6 +1479,7 @@
 {
     int status = 0;
     int is_local_route;
+    bool gateway_needed = false;
 
     if (!(r->flags & RT_DEFINED))
     {
@@ -1488,8 +1489,8 @@
     struct argv argv = argv_new();
     struct gc_arena gc = gc_new();
 
-#if !defined(TARGET_LINUX)
     const char *network = print_in_addr_t(r->network, 0, &gc);
+#if !defined(TARGET_LINUX)
 #if !defined(TARGET_AIX)
     const char *netmask = print_in_addr_t(r->netmask, 0, &gc);
 #endif
@@ -1502,8 +1503,47 @@
         goto done;
     }
 
+#ifndef _WIN32
+    /* server told us which interface to use; pass gateway if it gave one */
+    if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0)
+    {
+        if (rgi->flags & RGI_ADDR_DEFINED && r->gateway != 0)
+        {
+            gateway_needed = true;
+        }
+    }
+#endif
+
+    /* TAP is an L2 device, so for destinations that are not explicitly
+     * on-link the kernel needs a gateway so it can ARP/ND for the
+     * MAC address. We make an exception only when the user explicitly sets
+     * metric 0, which is taken as a request for an on-link “route to
+     * interface” (no gateway). This keeps parity with the IPv6 code.
+     */
+    if (tt->type == DEV_TYPE_TAP
+        && !((r->flags & RT_METRIC_DEFINED) && r->metric == 0))
+    {
+        gateway_needed = true;
+    }
+
+    if (gateway_needed && r->gateway == 0)
+    {
+        msg(M_WARN, "ROUTE WARNING: " PACKAGE_NAME " needs a gateway "
+                    "parameter for a --route option and no default was set via "
+                    "--route-gateway or --ifconfig option. Not installing "
+                    "IPv4 route to %s/%d.",
+            network, netmask_to_netbits2(r->netmask));
+        status = 0;
+        goto done;
+    }
+
 #if defined(TARGET_LINUX)
-    const char *iface = NULL;
+    const char *iface = tt->actual_name;
+    if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */
+    {
+        iface = rgi->iface;
+    }
+
     int metric = -1;
 
     if (is_on_link(is_local_route, flags, rgi))
@@ -1518,7 +1558,8 @@
 
 
     status = RTA_SUCCESS;
-    int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), &r->gateway,
+    int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask),
+                               gateway_needed ? &r->gateway : NULL,
                                iface, r->table_id, metric);
     if (ret == -EEXIST)
     {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/677?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: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1
Gerrit-Change-Number: 677
Gerrit-PatchSet: 12
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>