peacey / split-vpn

A split tunnel VPN script for Unifi OS routers (UDM, UXG, UDR) with policy based routing.
GNU General Public License v3.0
802 stars 56 forks source link

Blackholes not replaced #190

Open midzelis opened 1 year ago

midzelis commented 1 year ago

add_nexthop_routes() and or add_openvpn_routes() will not remove the blackhole added by add_blackhole_routes() because the blackhole is added via default (0.0.0.0/0) but replaced using 0.0.0.0/1, and 128.0.0.0/1.

I also noted a problem in run_rule_watcher() - I think.... its trying to remove the startup blackholes. But these routes are not the ones added by add_blackhole_routes - also, its not removing them from the ${ROUTE_TABLE} - so this basically doesn't do anything. Additionally, there is a another method for removing blackhole routes delete_blackhole_routes which seems like it ought to work, for anytype of blackhole route - using 0.0.0.0/0 or 0.0.0.0/1, 128.0.0.1.

This change fixes the problem with not removing blackholes, but please review the logic in the rule watcher - maybe I'm missing something?

peacey commented 1 year ago

Hi @midzelis,

There's nothing wrong with this logic. So there's two types of blackhole routes that we use. One is the blackholes routes added to the the VPN route table, and one is startup blackholes routes which are only added by the user manually to the main route table via the Unifi Network GUI (these startup blackholes are for cases when the user doesn't want any traffic when the UDM reboots and the VPN script hasn't started yet - they are optional and only for advanced configurations).

Regarding the VPN backhole routes, these are not supposed to be replaced. The default VPN route is added as 0.0.0.0/1, 128.0.0.1/1 so that it can override the default backhole route without removing the blackholes. This way if something happens with those routes for whatever reason, the blackhole routes will still be there to protect anything leaking out that's routed through the VPN table.

Regarding the startup blackholes, they are supposed to be removed immediately after the script sets up the route table and routing rules. Then the rule watcher watches out if they're ever added again by the OS so it removes them constantly.

Hopefully that clears up the logic...