After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721724 - [review] dcbw/vpn-no-iface: handle VPNs that don't have tunnel interfaces
[review] dcbw/vpn-no-iface: handle VPNs that don't have tunnel interfaces
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: IP and DNS config
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-01-07 20:24 UTC by Dan Williams
Modified: 2014-01-24 18:00 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Dan Williams 2014-01-07 20:24:11 UTC
IPSec-type VPNs that rely on the kernel's built-in IPSec functionality don't have tunnel interfaces, instead they direct the kernel to create security associations between the local machine and the VPN gateway and rely on routes to provide access to the remote network.

In these cases, the routes are typically added by the VPN service itself, and NetworkManager doesn't need to touch them.  The routes and the IP address that would normally be set on the tunnel interface are instead set on the parent device (eg, eth0 or wlan0).

----

There are two parts to this branch. (a) changes to allow VPNs without interface names, which should be pretty self-explanatory. (b) changes to stop touching routes that aren't owned by NM, which require a bit more explanation.

The first core problem for (b) is that NetworkManager always overwrote device route metrics with the device's priority on IP changes, even if the route was an external route.  This meant you could add a route with /sbin/ip and a metric of 50, and NetworkManager would eventually remove that route and replace it with the same route, except with a metric of 10.

We want NM to set the metric for routes *it* owns (that come from DHCP, or RA, or static configuration) but we do not want this to happen for routes that are added outside of NM.

The second problem is that NetworkManager always adds all routes and addresses when updating IP configuration, and relies on the kernel to return EEXIST when that item already exists.  Normally this would be fine, and it works for addresses (where we want to update the lifetime) because when it gets the NLM_F_REPLACE flag, the kernel only updates address lifetimes, it does not remove and replace the address.

In the specific case of IPSec, replacing the route, even with the exact same route, appears to break IPSec routing and results in unreachable networks on the other side of the tunnel.  The kernel interprets the NLM_F_REPLACE flag differently from addresses, and actually removes the entire route and re-adds the new one (see net/ipv4/fib_trie.c::fib_table_insert()).

Instead of trying to send different flags to the kernel based on which operation NM is doing, it should just not touch routes that are already in the routing table that it wouldn't be changing anyway.
Comment 1 Pavel Simerda 2014-01-08 09:54:15 UTC
+1 for the solution but I don't have a clear idea of the source of the problem (as I already stated on IRC).

I'm still insterested which routes need to be added at all, given that kernel ipsec implementation provides ipsec rules which can catch *any* packets based on their source and destination attributes and encapsulate them as ESP packets. At least that's what happens in tunnel mode. And tunnel mode is required if one of the sides of the channel is a network, not a single host.
Comment 2 Dan Winship 2014-01-14 01:03:08 UTC
looks good

> vpn: handle missing tunnel interface for IP-based VPNs (rh #1030068)

nm_vpn_connection_apply_config() confused me at first; maybe rename vpn4_config to vpn4_parent_config or vpn4_device_config? (and likewise with 6)


> core/platform: add address/route sources

Is there any particular logic to the ordering of NMPlatformSource other than that KERNEL has to be lowest?


> core/platform: preserve external and static route metrics

>+	/* Use the default metric only if the route was created by NM */
>+	if (route.source != NM_PLATFORM_SOURCE_KERNEL && !route.metric)

"... and the configuration didn't explicitly specify a metric" ?

>+#define ROUTE_METRIC_DEFAULT 1024

NM_PLATFORM_ROUTE_METRIC_DEFAULT ? (To make it obvious where to go to find the definition if nothing else.)


> platform: don't replace routes that already exist

>+	gboolean success = FALSE;
...
>+	for (i = 0, success = TRUE; i < known_routes->len && success; i++) {

weird that you initialize success to FALSE but then change it to TRUE before ever using it

>+		success = nm_platform_ip4_route_add (ifindex,
>+			                                 known_route->network,

indentation
Comment 3 Thomas Haller 2014-01-15 19:42:14 UTC
> vpn: handle missing tunnel interface for IP-based VPNs (rh #1030068)

Please add the full BZ URLs:
https://bugzilla.gnome.org/show_bug.cgi?id=721724
https://bugzilla.redhat.com/show_bug.cgi?id=1030068

Also, how does this branch relate to
https://bugzilla.redhat.com/show_bug.cgi?id=865883
https://bugzilla.redhat.com/show_bug.cgi?id=845599
... should they be mentioned in the commit message too?


Also, the patches from bug 845599#c5 and bug 845599#c6 were not applied(?). Was that intended? Maybe a comment about that on bug 845599?


> core/platform: add address/route sources

This commit fixes https://bugzilla.redhat.com/show_bug.cgi?id=1005416
Please mention it in the commit message (and close bug later)


Also, pushed two !fixups and two commits on top of your branch.
Comment 4 Dan Williams 2014-01-22 18:50:52 UTC
Reworked for comments and repushed.  No additional changes beyond what was requested in comments 2 and 3.
Comment 5 Thomas Haller 2014-01-22 19:19:33 UTC
ACK, looks good to me now.


>> core/platform: revise failure of activation a connection on error of setting route

This seems correct to me, but I'm not particularly sure about it. Please some proof-thinking.
Comment 6 Thomas Haller 2014-01-23 09:44:57 UTC
Pushed a !fixup and a new commit on top of dcbw/vpn-no-iface.

The new commit changes nm_platform_ipX_route_sync(), so, it would conflict with this branch, that's why I put it here.



Also, please fixup commit
>> platform: don't replace routes that already exist
so that nm_platform_ip6_route_sync() ends with "return success;"
Comment 7 Thomas Haller 2014-01-23 17:05:43 UTC
Rebased branch. I reworded commit messages (to reference BZ URLs) and included before-mentioned fixups.

For reference, before my rebase, HEAD was pointing to
fe232b44da0ae11d49a52610257405115a09f3fc.
Comment 8 Dan Williams 2014-01-24 18:00:16 UTC
All your fixes grabbed, rebased to master, and merged to master.