diff mbox series

[Openvpn-devel,v4,6/7] options: enable IPv4 redirection logic only if really required

Message ID 20200530000600.1680-7-a@unstable.cc
State Superseded
Headers
Series Allow IPv6-only tunnels |

Commit Message

Antonio Quartulli May 29, 2020, 2:05 p.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:
- move error message modification to previous patch

Changes from v3:
- patchset rebased on top of pre-ipv6-only patchset


 src/openvpn/options.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Gert Doering June 7, 2020, 1:25 a.m. UTC | #1 
Hi,

On Sat, May 30, 2020 at 02:05:59AM +0200, Antonio Quartulli wrote:
> 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>

I can see why we want this - I tried to connect to a "v6-only-in-tunnel"
server over v4, specifying "redirect-gateway !ipv4 ipv6", and it tried
to install a v4 /32 redirect route...

Sun Jun  7 13:20:43 2020 net_route_v4_add: 199.102.77.82/32 via 193.149.48.190 dev [NULL] table 0 metric -1

... which is harmless, but "unnecesary fumbling" is not desirable.


The reason why I'm a bit unhappy about applying it is that it will
change behaviour for the "redirect-private" case, and that might break
people's setups.  For "redirect-gateway" or "redirect-gateway def1" (etc),
it will not change anything.

Can we make this conditional in a way that does not break "redirect-private"?

(I used to use "redirect-private" to handle overlapping IPv4 routes without
actually redirecting the whole gateway - think "VPN server is on 192.0.2.1
and you want to push 'route 192.0.2.0/24'".  IPv6 handles this automatically,
but v4 needs "redirect-private" for that to work)

thanks :)

gert
Gert Doering June 8, 2020, 4:16 a.m. UTC | #2 
Hi,

On Sun, Jun 07, 2020 at 01:25:01PM +0200, Gert Doering wrote:
> Can we make this conditional in a way that does not break "redirect-private"?

A very simple patch would be

        if (streq(p[0], "redirect-gateway"))
        {
            options->routes->flags |= RG_REROUTE_GW;
        }
+       if (streq(p[0], "redirect-private"))
+       {
+           options->routes->flags |= RG_ENABLE;
+       }

and then take your patch as-is ("redirect-gateway with no options" would
set RG_REROUTE_GW, which, if not cleared by !ipv4, would set RG_ENABLE
later on, while "redirect-private" sets the RG_ENABLE itself).

Alternatively, set RG_ENABLE at the top (always), and clear it for "!ipv4"

            else if (streq(p[j], "!ipv4"))
            {
                options->routes->flags &= ~(RG_REROUTE_GW|RG_ENABLE);
            }

... this should do the same thing, with less code convolutions.

Configs that have *both* "redirect-private" and "redirect-gateway !ipv4 ipv6"
would still be broken.  But I'm not sure such a config is well-defined
in the first place.

gert
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7556e7ee..3798731e 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6591,7 +6591,14 @@  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;
+        /* enable IPv4 redirection logic only if at least one IPv4 flag is set.
+         * For instance, when "redirect-gateway !ipv4 ipv6" is specified no
+         * IPv4 redirection should be activated.
+         */
+        if (options->routes->flags)
+        {
+            options->routes->flags |= RG_ENABLE;
+        }
     }
     else if (streq(p[0], "block-ipv6") && !p[1])
     {