Re[2]: NetworkManager VPN and routing in bridged mode

[Valentine Sinitsyn <valentine sinitsyn usu ru>]

Hello,

The attached is updated version of the patch I've sent yesterday. The 
main changes are as follows:
  * Ported to NM 0.6.6 (SVN rev 3381)
  * Handle user-specified routes correctly too

There are some "architectural" issues with the code - I've implemented 
them the way to be as much backward-compatible as possible, nevertheless 
 they should be probably refactored in future. They are:
  - handling of 11th arg (remote_gateway) in D-BUS message in 
nm-vpn-service.c: it's no different from 10th, 9th, ... arg, so it 
should be sent by all VPN daemons (set to zero if there is no remote 
gateway i.e. we are dealing with P-t-P device) and warning should be 
generated if it can't be found. It's not difficult to implement but I 
have no setup to test pptp/vpnc and modify them accordingly.

 - nm_system_device_set_ip4_route_with_iface(). Essentially, I've 
reworked the function  to accept interface name instead of NMDevice * 
and create routes without RTF_GATEWAY flag if necessary. For now, an 
older nm_system_device_set_ip4_route() acts as a wrapper to this.


P.S. In the very side note - what about 
http://bugzilla.gnome.org/show_bug.cgi?id=353265 ? It's fixed for 0.7 
but still not committed to 0.6 branch.

--
Regards,
Valentine Sinitsyn

Index: src/nm-ip4-config.c
===================================================================
--- src/nm-ip4-config.c	(revision 3447)
+++ src/nm-ip4-config.c	(working copy)
@@ -43,6 +43,8 @@
 
 	guint32	mtu;	/* Maximum Transmission Unit of the interface */
 	guint32	mss;	/* Maximum Segment Size of the route */
+	
+	guint32 remote_gateway; /* Used for VPN networks with tap-style virtual device */
 
 	GSList *	nameservers;
 	GSList *	domains;
@@ -56,6 +58,7 @@
 	 * an IP4Config before it can be used.
 	 */
 	gboolean	secondary;
+
 };
 
 
@@ -357,6 +360,20 @@
 	config->mss = mss;
 }
 
+guint32	nm_ip4_config_get_remote_gateway (NMIP4Config *config)
+{
+	g_return_val_if_fail (config != NULL, 0);
+
+	return config->remote_gateway;
+}
+
+void nm_ip4_config_set_remote_gateway (NMIP4Config *config, guint32 remote_gateway)
+{
+	g_return_if_fail (config != NULL);
+
+	config->remote_gateway = remote_gateway;
+}
+
 /* libnl convenience/conversion functions */
 
 static int ip4_addr_to_rtnl_local (guint32 ip4_address, struct rtnl_addr *addr)
Index: src/nm-ip4-config.h
===================================================================
--- src/nm-ip4-config.h	(revision 3447)
+++ src/nm-ip4-config.h	(working copy)
@@ -74,6 +74,9 @@
 guint32		nm_ip4_config_get_mss			(NMIP4Config *config);
 void			nm_ip4_config_set_mss			(NMIP4Config *config, guint32 mss);
 
+guint32		nm_ip4_config_get_remote_gateway	(NMIP4Config *config);
+void			nm_ip4_config_set_remote_gateway	(NMIP4Config *config, guint32 remote_gateway);
+
 /* Flags for nm_ip4_config_to_rtnl_addr() */
 #define NM_RTNL_ADDR_NONE		0x0000
 #define NM_RTNL_ADDR_ADDR		0x0001
Index: src/vpn-manager/nm-vpn-service.c
===================================================================
--- src/vpn-manager/nm-vpn-service.c	(revision 3447)
+++ src/vpn-manager/nm-vpn-service.c	(working copy)
@@ -841,6 +841,17 @@
 	if (!get_dbus_string_helper (&iter, &login_banner, "Login Banner"))
 		goto out;
 
+	/* Eleventh arg: Remote VPN Gateway (UINT32) 
+	 * For now, only OpenVPN service knows about it so we won't complain
+	 * if it is absent from the message.
+	 */
+	if (dbus_message_iter_next (&iter)
+	    && (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_UINT32))
+	{    
+		dbus_message_iter_get_basic (&iter, &num);
+		nm_ip4_config_set_remote_gateway (config, num);
+	}	
+
 #ifdef NM_DEBUG_VPN_CONFIG
 	print_vpn_config (ip4_vpn_gateway,
 	                  tundev,
Index: src/NetworkManagerSystem.c
===================================================================
--- src/NetworkManagerSystem.c	(revision 3447)
+++ src/NetworkManagerSystem.c	(working copy)
@@ -1,6 +1,7 @@
 /* NetworkManager -- Network link manager
  *
  * Dan Williams <dcbw redhat com>
+
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -51,23 +52,19 @@
 #include <netlink/utils.h>
 #include <netlink/route/link.h>
 
-
 /*
- * nm_system_device_set_ip4_route
+ * nm_system_device_set_ip4_route_with_iface
  *
  */
-static gboolean nm_system_device_set_ip4_route (NMDevice *dev, int ip4_gateway, int ip4_dest, int ip4_netmask, int mss)
+static gboolean nm_system_device_set_ip4_route_with_iface (const char *iface, int ip4_gateway, int ip4_dest, int ip4_netmask, int mss)
 {
 	NMSock *			sk;
 	gboolean			success = FALSE;
 	struct rtentry		rtent;
 	struct sockaddr_in *p;
-	const char *		iface;
 	int				err;
-	NMIP4Config * config = NULL;
 
-	iface = nm_device_get_iface (dev);
-
+#if 0
 	/*
 	 * Zero is not a legal gateway and the ioctl will fail.  But zero is a
 	 * way of saying "no route" so we just return here.  Hopefully the
@@ -75,33 +72,39 @@
 	 */
 	if (ip4_gateway == 0)
 		return TRUE;
-
+#endif
+		
 	/*
-	 * Do not add the route if the destination is on the same subnet.
+	 * -1 is passed by nm_system_device_set_ip4_route() to indicate
+	 * there is no route to host. Previously, zero was used for this
+	 * purpose (see above) but since nm_system_device_set_ip4_route_with_iface()
+	 * can create routes without RTF_GATEWAY, the convention was changed.
+	 * It's impossible to create route without gateway using 
+	 * nm_system_device_set_ip4_route() but noone expects this from it anyway.
 	 */
-	config = nm_device_get_ip4_config(dev);
-	if (config &&
-	    ((guint32)ip4_dest & nm_ip4_config_get_netmask(config)) ==
-	        (nm_ip4_config_get_address(config) & nm_ip4_config_get_netmask(config)))
+	if (ip4_gateway == -1)
 		return TRUE;
 
-	if ((sk = nm_dev_sock_open (dev, NETWORK_CONTROL, __FUNCTION__, NULL)) == NULL)
+	if ((sk = nm_dev_sock_open (NULL, NETWORK_CONTROL, __FUNCTION__, NULL)) == NULL)
 		return FALSE;
 
 	memset (&rtent, 0, sizeof (struct rtentry));
 	p				= (struct sockaddr_in *) &rtent.rt_dst;
 	p->sin_family		= AF_INET;
 	p->sin_addr.s_addr	= ip4_dest;
-	p				= (struct sockaddr_in *) &rtent.rt_gateway;
-	p->sin_family		= AF_INET;
-	p->sin_addr.s_addr	= ip4_gateway;
+	if (ip4_gateway)
+	{
+		p			= (struct sockaddr_in *) &rtent.rt_gateway;
+		p->sin_family		= AF_INET;
+		p->sin_addr.s_addr	= ip4_gateway;
+	}
 	p				= (struct sockaddr_in *) &rtent.rt_genmask;
 	p->sin_family		= AF_INET;
 	p->sin_addr.s_addr	= ip4_netmask;
 	rtent.rt_dev		= (char *)iface;
 	rtent.rt_metric	= 1;
 	rtent.rt_window	= 0;
-	rtent.rt_flags		= RTF_UP | RTF_GATEWAY | (rtent.rt_window ? RTF_WINDOW : 0);
+	rtent.rt_flags		= RTF_UP | (ip4_gateway ? RTF_GATEWAY : 0) | (rtent.rt_window ? RTF_WINDOW : 0);
 
 	if (mss)
 	{
@@ -176,6 +179,10 @@
 	return success;
 }
 
+static gboolean nm_system_device_set_ip4_route (NMDevice *dev, int ip4_gateway, int ip4_dest, int ip4_netmask, int mss)
+{
+	return nm_system_device_set_ip4_route_with_iface (nm_device_get_iface (dev), ip4_gateway ? ip4_gateway : -1, ip4_dest, ip4_netmask, mss);
+}
 
 static void iface_to_rtnl_index (const char *iface, struct nl_handle *nlh, struct rtnl_addr *addr)
 {
@@ -458,6 +465,40 @@
 
 
 /*
+ * parse_ip4_route
+ *
+ * Converts CIDR notation in two integers in network order.
+ *
+ */
+static gboolean parse_ip4_route(const char *addr, guint32 *network, guint32 *netmask)
+{
+	guint32 quads[4];
+	guint32 bits;
+	int i, count;
+	
+	g_return_val_if_fail(addr != NULL, FALSE);
+	
+	count = sscanf(addr, "%u.%u.%u.%u/%u", 
+	               &quads[0], &quads[1], &quads[2], &quads[3],
+		       &bits);
+	
+	if (count != 5)
+		return FALSE;
+	for (i = 0; i < 4; i++)
+		if (quads[i] > 255)
+			return FALSE;
+	if (bits > 32)
+		return FALSE;		
+			
+	*network = htonl((quads[0] << 24) + (quads[1] << 16) + (quads[2] << 8) + quads[3]);
+	*netmask = bits ? htonl(~0 << (32 - bits)) : 0;
+	*network &= *netmask;
+	
+	return TRUE;
+}
+
+
+/*
  * nm_system_vpn_device_set_from_ip4_config
  *
  * Set IPv4 configuration of a VPN device from an NMIP4Config object.
@@ -475,8 +516,13 @@
 	struct nl_handle *	nlh = NULL;
 	struct rtnl_addr *	addr = NULL;
 	struct rtnl_link *	request = NULL;
+	guint32			remote_gateway = 0;
+	guint32			network = 0;
+	guint32			netmask = 0;	
 
 	g_return_val_if_fail (config != NULL, FALSE);
+	
+	remote_gateway = nm_ip4_config_get_remote_gateway (config);	
 
 	/* Set up a route to the VPN gateway through the real network device */
 	if (active_device && (ad_config = nm_device_get_ip4_config (active_device)))
@@ -485,67 +531,92 @@
 	if (!iface || !strlen (iface))
 		goto done;
 
-	nlh = new_nl_handle (FALSE);
-	if (!nlh)
-		goto done;
-
 	nm_system_device_set_up_down_with_iface (iface, TRUE);
 
-	if ((addr = nm_ip4_config_to_rtnl_addr (config, NM_RTNL_ADDR_PTP_DEFAULT)))
+	if (!remote_gateway)
 	{
-		int err = 0;
-		iface_to_rtnl_index (iface, nlh, addr);
-		if ((err = rtnl_addr_add (nlh, addr, 0)) < 0)
-			nm_warning ("nm_system_device_set_from_ip4_config(): error %d returned from rtnl_addr_add():\n%s", err, nl_geterror());
-		rtnl_addr_put (addr);
-	}
-	else
-		nm_warning ("nm_system_vpn_device_set_from_ip4_config(): couldn't create rtnl address!\n");
+		nlh = new_nl_handle (FALSE);
+		if (!nlh)
+			goto done;
 
-	/* Set the MTU */
-	if ((request = rtnl_link_alloc ()))
-	{
-		struct rtnl_link * old;
+		if ((addr = nm_ip4_config_to_rtnl_addr (config, NM_RTNL_ADDR_PTP_DEFAULT)))
+		{
+			int err = 0;
+			iface_to_rtnl_index (iface, nlh, addr);
+			if ((err = rtnl_addr_add (nlh, addr, 0)) < 0)
+				nm_warning ("nm_system_device_set_from_ip4_config(): error %d returned from rtnl_addr_add():\n%s", err, nl_geterror());
+			rtnl_addr_put (addr);
+		}
+		else
+			nm_warning ("nm_system_vpn_device_set_from_ip4_config(): couldn't create rtnl address!\n");
 
-		old = iface_to_rtnl_link (iface, nlh);
-		rtnl_link_set_mtu (request, 1412);
-		rtnl_link_change (nlh, old, request, 0);
+		/* Set the MTU */
+		if ((request = rtnl_link_alloc ()))
+		{
+			struct rtnl_link * old;
 
-		rtnl_link_put (old);
-		rtnl_link_put (request);
+			old = iface_to_rtnl_link (iface, nlh);
+			rtnl_link_set_mtu (request, 1412);
+			rtnl_link_change (nlh, old, request, 0);
+
+			rtnl_link_put (old);
+			rtnl_link_put (request);
+		}
+
+		destroy_nl_handle (nlh);
 	}
 
-	destroy_nl_handle (nlh);
-
 	sleep (1);
 
 	nm_system_device_flush_routes_with_iface (iface);
+	if (remote_gateway)
+	{
+		/* Add route to remote network */
+		netmask = nm_ip4_config_get_netmask (config);
+		network = (nm_ip4_config_get_address (config) & netmask);
+		nm_system_device_set_ip4_route_with_iface (iface, 0, network, netmask, nm_ip4_config_get_mss (config));
+	}	
 	if (num_routes <= 0)
 	{
 		nm_system_delete_default_route ();
-		nm_system_device_add_default_route_via_device_with_iface (iface);
+		if (!remote_gateway)
+			nm_system_device_add_default_route_via_device_with_iface (iface);
+		else
+			nm_system_device_set_ip4_route_with_iface (iface, remote_gateway, 0, 0, nm_ip4_config_get_mss (config));
 	}
 	else
 	{
-		int i;
-		for (i = 0; i < num_routes; i++)
+		int i;	
+
+		if (!remote_gateway)
 		{
-			char *valid_ip4_route;
+			for (i = 0; i < num_routes; i++)
+			{
+				char *valid_ip4_route;
 
-			/* Make sure the route is valid, otherwise it's a security risk as the route
-			 * text is simply taken from the user, and passed directly to system().  If
-			 * we did not check the route, think of:
-			 *
-			 *     system("/sbin/ip route add `rm -rf /` dev eth0")
-			 *
-			 * where `rm -rf /` was the route text.  As UID 0 (root), we have to be careful.
-			 */
-			if ((valid_ip4_route = validate_ip4_route (routes[i])))
-			{
-				nm_system_device_add_route_via_device_with_iface (iface, valid_ip4_route);
-				g_free (valid_ip4_route);
+				/* Make sure the route is valid, otherwise it's a security risk as the route
+				 * text is simply taken from the user, and passed directly to system().  If
+				 * we did not check the route, think of:
+				 *
+				 *     system("/sbin/ip route add `rm -rf /` dev eth0")
+				 *
+				 * where `rm -rf /` was the route text.  As UID 0 (root), we have to be careful.
+				 */
+				if ((valid_ip4_route = validate_ip4_route (routes[i])))
+				{
+					nm_system_device_add_route_via_device_with_iface (iface, valid_ip4_route);
+					g_free (valid_ip4_route);
+				}
 			}
 		}
+		else
+		{
+
+
+			for (i = 0; i < num_routes; i++)
+			    if (parse_ip4_route(routes[i], &network, &netmask))
+				    nm_system_device_set_ip4_route_with_iface (iface, remote_gateway, network, netmask, nm_ip4_config_get_mss (config));
+		}
 	}
 
 done:
Index: vpn-daemons/openvpn/src/nm-openvpn-service-openvpn-helper.c
===================================================================
--- vpn-daemons/openvpn/src/nm-openvpn-service-openvpn-helper.c	(revision 3447)
+++ vpn-daemons/openvpn/src/nm-openvpn-service-openvpn-helper.c	(working copy)
@@ -197,6 +197,7 @@
 static gboolean
 send_config_info (DBusConnection *con,
 		  const char *str_vpn_gateway,
+		  const char *str_remote_gateway,
 		  const char *str_tundev,
 		  const char *str_ip4_address,
 		  const char *str_ip4_ptpaddr,
@@ -208,6 +209,7 @@
   DBusMessage *	message;
   struct in_addr	temp_addr;
   guint32		uint_vpn_gateway = 0;
+  guint32               uint_remote_gateway = 0;
   guint32		uint_ip4_address = 0;
   guint32		uint_ip4_ptpaddr = 0;
   guint32		uint_ip4_netmask = 0xFFFFFFFF; /* Default mask of 255.255.255.255 */
@@ -231,6 +233,12 @@
     goto out;
   }
 
+  if (strlen (str_remote_gateway) > 0 && ! ipstr_to_uint32 (str_remote_gateway, &uint_remote_gateway) ) {
+    nm_warning ("nm-openvpn-service-openvpn-helper didn't receive a valid Remote Gateway from openvpn.");
+    send_config_error (con, "Remote VPN Gateway");
+    goto out;
+  }
+
   if (! ipstr_to_uint32 (str_ip4_address, &uint_ip4_address) ) {
     nm_warning ("nm-openvpn-service-openvpn-helper didn't receive a valid Internal IP4 Address from openvpn.");
     send_config_error (con, "IP4 Address");
@@ -257,6 +265,7 @@
 			    DBUS_TYPE_UINT32, &uint_ip4_netmask,
 			    DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32, &uint_ip4_dns, uint_ip4_dns_len,
 			    DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32, &uint_ip4_nbns, uint_ip4_nbns_len,
+			    DBUS_TYPE_UINT32, &uint_remote_gateway,
 			    DBUS_TYPE_INVALID);
   if (dbus_connection_send (con, message, NULL))
     success = TRUE;
@@ -304,6 +313,7 @@
   DBusConnection  *con;
   DBusError        error;
   char            *vpn_gateway = NULL;
+  char            *remote_gateway = NULL;
   char            *tundev = NULL;
   char            *ip4_address = NULL;
   char            *ip4_ptp = NULL;
@@ -339,14 +349,15 @@
 
   // print_env();
 
-  vpn_gateway = getenv( "trusted_ip" );
-  tundev      = getenv ("dev");
-  ip4_ptp     = getenv("ifconfig_remote");
-  ip4_address = getenv("ifconfig_local");
-  ip4_netmask = getenv("route_netmask_1");
+  vpn_gateway    = getenv( "trusted_ip" );
+  remote_gateway = getenv( "route_vpn_gateway" );
+  tundev         = getenv ("dev");
+  ip4_ptp        = getenv("ifconfig_remote");
+  ip4_address    = getenv("ifconfig_local");
+  ip4_netmask    = getenv("ifconfig_netmask");
   
-  ip4_dns     = g_ptr_array_new();
-  ip4_nbns    = g_ptr_array_new();
+  ip4_dns        = g_ptr_array_new();
+  ip4_nbns       = g_ptr_array_new();
   
   while (1) {
     sprintf(envname, "foreign_option_%i", i++);
@@ -381,6 +392,7 @@
 	{
 		FILE *file = fopen ("/tmp/vpnstuff", "w");
 		fprintf (file, "VPNGATEWAY: '%s'\n", vpn_gateway);
+		fprintf (file, "REMOTEGATEWAY: '%s'\n", remote_gateway);
 		fprintf (file, "TUNDEV: '%s'\n", tundev);
 		fprintf (file, "IP4_ADDRESS: '%s'\n", ip4_address);
 		fprintf (file, "IP4_NETMASK: '%s'\n", ip4_netmask);
@@ -393,6 +405,17 @@
     send_config_error (con, "VPN Gateway");
     exit (1);
   }
+  if (!remote_gateway) {
+/*    if (ip4_netmask) {
+      nm_warning ("nm-openvpn-service-openvpn-helper received Remote VPN Gateway but no netmask from openvpn.");
+      send_config_error (con, "Remote VPN Gateway");
+      exit (1);
+    } else {
+      remote_gateway = g_strdup("");
+    }
+*/
+    remote_gateway = g_strdup ("");    
+  }
   if (!tundev || !g_utf8_validate (tundev, -1, NULL)) {
     nm_warning ("nm-openvpn-service-openvpn-helper didn't receive a Tunnel Device from openvpn, or the tunnel device was not valid UTF-8.");
     send_config_error (con, "Tunnel Device");
@@ -408,7 +431,7 @@
     ip4_netmask = g_strdup ("");
   }
 
-  if (!send_config_info (con, vpn_gateway, tundev,
+  if (!send_config_info (con, vpn_gateway, remote_gateway, tundev,
 			 ip4_address, ip4_ptp, ip4_netmask,
 			 ip4_dns, ip4_nbns)) {
     exit_code = 1;
Index: vpn-daemons/openvpn/src/nm-openvpn-service.c
===================================================================
--- vpn-daemons/openvpn/src/nm-openvpn-service.c	(revision 3447)
+++ vpn-daemons/openvpn/src/nm-openvpn-service.c	(working copy)
@@ -1266,6 +1266,7 @@
   guint32 *		ip4_nbns;
   guint32		ip4_nbns_len;
   guint32		mss;
+  guint32		remote_gateway;
   gboolean		success = FALSE;
   char *                empty = "";
 
@@ -1288,6 +1289,7 @@
 			    DBUS_TYPE_UINT32, &ip4_netmask,
 			    DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32, &ip4_dns, &ip4_dns_len,
 			    DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32, &ip4_nbns, &ip4_nbns_len,
+			    DBUS_TYPE_UINT32, &remote_gateway,
 			    DBUS_TYPE_INVALID))
     {
       DBusMessage	*signal;
@@ -1312,6 +1314,7 @@
 				DBUS_TYPE_UINT32, &mss,
 				DBUS_TYPE_STRING, &empty,
 				DBUS_TYPE_STRING, &empty,
+				DBUS_TYPE_UINT32, &remote_gateway,
 				DBUS_TYPE_INVALID);
 
       if (!dbus_connection_send (data->con, signal, NULL))