Force 'def1' method when --redirect-gateway is done through service

The service deletes all added routes when the client process (openvpn) exits, causing the re-instated default route to disappear. Fix by rewriting "--redirect-gateway" to "--redirect-gateway def1" when routes are set using interactive service.

Only the behaviour on Windows with intereactive service is affected.

Resolves Trac 778

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

index 63dcc24..c540e05 100644
@@ -2526,6 +2526,22 @@ options_postprocess_mutate_ce (struct options *o, struct connection_entry *ce)
 
 }
 
+#ifdef _WIN32
+/* If iservice is in use, we need def1 method for redirect-gateway */
+static void
+remap_redirect_gateway_flags (struct options *opt)
+{
+  if (opt->routes
+      && opt->route_method == ROUTE_METHOD_SERVICE
+      && opt->routes->flags & RG_REROUTE_GW
+      && !(opt->routes->flags & RG_DEF1))
+    {
+      msg (M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)");
+      opt->routes->flags |= RG_DEF1;
+    }
+}
+#endif
+
 static void
 options_postprocess_mutate_invariant (struct options *options)
 {

@@ -2555,6 +2571,8 @@ options_postprocess_mutate_invariant (struct options *options)
       options->tuntap_options.ip_win32_type = IPW32_SET_MANUAL;
       options->ifconfig_noexec = false;
     }
+
+  remap_redirect_gateway_flags (options);
 #endif
 
 #if P2MP_SERVER

@@ -5705,6 +5723,10 @@ add_option (struct options *options,
 	      goto err;
 	    }
 	}
+#ifdef _WIN32
+      /* 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], "remote-random-hostname") && !p[1])

2.1.4

ACK, thanks.

The patch does what we agreed on in the trac ticket ("no user changes" for the normal case, and "in the service pipe change, imply def1 if no flags set"). This is very little extra code and easily checked, while fixing this in other ways would need much more code - for a sort-of historic approach to routing table handling (we really shouldn't be *removing* other people's routes, and we don't do that for IPv6 either).

Semi-ACK on the approach from Arne ("that's what we do on Android") :-)

I've added a note to Changes.rst.

Your patch has been applied to the master branch.

commit 788e5e4a08e0df7206d17e9cbc135764d6fc385f

Author: Selva Nair
Date:   Tue Nov 29 19:39:32 2016 -0500

     Force 'def1' method when --redirect-gateway is done through service

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13307.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Am 30.11.16 um 19:58 schrieb Gert Doering:

Slight correction. We actually set 0.0.0.0/0 on Android but Android *always* translates that into a 0.0.0.0/1 and a 128.0.0.0/1 rule.

We could do the same and do the translation in the interactive service instead of OpenVPN itself.

Arne

Yeah that's an option but we'll have to make the HandleRouteMsg() code in service
parse and understand the default route. Currently it just does what its told to do (touch wood!).
When the service is taught to change things like that, it could come and bite us one day..
The current approach is probably simpler.

Selva

Hi,

We could, but that wouldn't solve the issue of OpenVPN trying to remove the existing default route (which we would have to special-case-and- ignore) and reinstall it later on (which must not be translated to 2x /1, but ignored as well).

The service code doesn't know whether a route is "for the VPN API" or "for anything else" (which openvpn on "non VPN API" platforms always could do) so this would be a much more intrusive change...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

I think it could work like this:

In DeleteRoute()
if dest == 0/0 delete routes to 0/1 and 128/1
(ignore any "route does not exist" error)

In AddRoute() <-- this function doesn't currently exist
if dest == 0/0 add routes to 0/1 and 128/1
(this will add two extra routes when default gateway is restored, but
cant hurt)

In this scenario, route to 0/0 is never touched by the service.

When we ask the service to:
delete default route to gw --> nothing happens unless there were routes
to 0/1 and 128/1
add default route through tun --> adds routes to 0/1 and 128/1 via tun delete default route through tun --> OK
add default route to gw --> adds routes to 0/1 and 128/1 via gw

When process exits/crashes, the service thread exits and the extra routes to 0/1 and 128/1 get removed. If service crashes those routes will remain, not ideal, but safe, I suppose.

Did I overlook something?

Not that I like it. Wonder how android does it.

Selva

Hi,

Sounds too complex to me :-) - "just use def1" is good.

Well, there's a VPN API - you tell the API "these networks is what I want to connect to" and then it will do routing table / routing policy magic to get *your* packets into the tun (and nobody else's) - and on close of the API, that stuff gets removed again.

Which is in a way similar to "def1" :-) - except that openvpn doesn't have to do any cleanup (as far as I'm aware) and doesn't need to bother about redirecting the existing gateway (install host route) and anything because the API takes care of that.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...