diff mbox series

[Openvpn-devel,v5] options: enable IPv4 redirection logic only if really required

Message ID 20200608153239.2260-1-a@unstable.cc
State Accepted
Headers
Series [Openvpn-devel,v5] options: enable IPv4 redirection logic only if really required |

Commit Message

Antonio Quartulli June 8, 2020, 5:32 a.m. UTC 
  From: Antonio Quartulli <antonio@openvpn.net>

If no IPv4 redirection flag is set, do not enable the IPv4
redirection logic at all so that it won't bother adding any
useless IPv4 route.

Trac: #208
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

---
Changes from v4:
- add warning about undefined behaviour when specifying
  redirect-gateway/private at the same time
- fix behaviour of redirect-private

Changes from v3:
- move error message modification to previous patch

Changes from v2:
- patchset rebased on top of pre-ipv6-only patchset
---
 src/openvpn/options.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Gert Doering June 8, 2020, 8:23 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks :-)

Tested with "redirect-private", "redirect-gateway", "redirect-gateay !ipv4",
and it seems to do what we want - not fumble any hostroutes if !ipv4 is
set, but *do* fumble them if needed.

If you do "--redirect-private --redirect-gateway !ipv4 ipv6", it will
unset the effect of "--redirect-private" (which is expected).  If you
do it the other way round, it will actually do what you told it to -
redirect IPv6 default, and add a /32 host route for IPv4.  So even 
in this somewhat very special case, we're not losing functionality, 
but if it might need reordering of config options.

There is changed behaviour for anything that has "!ipv4" in it - if
you do "redirect-gateway !ipv4 block-local", before this patch, we'd
"route the local lan into the tunnel, but not the default gateway" -
now, we will just ignore the "block-local" bit.  Anything with IPv4,
actually, as RG_ENABLE gets dropped.  This was not intentional, but
it is actually making sense - you said "!ipv4", after all.


Your patch has been applied to the master branch.

commit 070319c13524125d8325a0df15fe795cc2a4bcf2
Author: Antonio Quartulli
Date:   Mon Jun 8 17:32:39 2020 +0200

     options: enable IPv4 redirection logic only if really required

     Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200608153239.2260-1-a@unstable.cc>
     URL: https://www.mail-archive.com/search?l=mid&q=20200608153239.2260-1-a@unstable.cc
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7556e7ee..018f6f18 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6542,6 +6542,18 @@  add_option(struct options *options,
         int j;
         VERIFY_PERMISSION(OPT_P_ROUTE);
         rol_check_alloc(options);
+
+        if (options->routes->flags & RG_ENABLE)
+        {
+            msg(M_WARN,
+                "WARNING: You have specified redirect-gateway and "
+                "redirect-private at the same time (or the same option "
+                "multiple times). This is not well supported and may lead to "
+                "unexpected results");
+        }
+
+        options->routes->flags |= RG_ENABLE;
+
         if (streq(p[0], "redirect-gateway"))
         {
             options->routes->flags |= RG_REROUTE_GW;
@@ -6579,7 +6591,7 @@  add_option(struct options *options,
             }
             else if (streq(p[j], "!ipv4"))
             {
-                options->routes->flags &= ~RG_REROUTE_GW;
+                options->routes->flags &= ~(RG_REROUTE_GW | RG_ENABLE);
             }
             else
             {
@@ -6591,7 +6603,6 @@  add_option(struct options *options,
         /* we need this here to handle pushed --redirect-gateway */
         remap_redirect_gateway_flags(options);
 #endif
-        options->routes->flags |= RG_ENABLE;
     }
     else if (streq(p[0], "block-ipv6") && !p[1])
     {