Added Gluon tunneldigger/L2TP related packages #978

lcb01a · 2016-12-12T10:48:36Z

Hi,

Recreated the PR as there were issues with empty commits.

To answer a previous question:
Tunneldigger is an alternative for Fastd without any encryption which uses Kernelspace L2TP and thus is way faster than Fastd.

gluon-config-mode-tunneldigger
gluon-mesh-vpn-tunneldigger
gluon-migrate-vpn
gluon-tunneldigger-watchdog

Cheers
Cyrus

neocturne

I found a few issues, but they should be easy to fix.

neocturne · 2016-12-13T00:39:59Z

package/gluon-migrate-vpn/files/lib/gluon/upgrade/499-migrate-vpn

+local fastd_enabled = uci:get('fastd', 'mesh_vpn', 'enabled')
+
+local tunneldigger_installed = util.exec('sh' , '-c', 'opkg list-installed | grep -e \'^tunneldigger\'')
+local fastd_installed = util.exec('sh' , '-c', 'opkg list-installed | grep -e \'^fastd\'')


This will break on systems without opkg (for example the ar71xx-tiny target of the Gluon lede branch). I think testing for the existence of /usr/bin/fastd and /usr/bin/tunneldigger would be more robust.

neocturne · 2016-12-13T00:43:05Z

package/gluon-mesh-vpn-tunneldigger/files/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

+uci:section('network', 'interface', 'mesh_vpn',
+	  {
+		  ifname = 'mesh-vpn',
+		  proto = 'batadv',


This is outdated, see https://github.com/freifunk-gluon/gluon/blob/master/package/gluon-mesh-vpn-fastd/luasrc/lib/gluon/upgrade/400-mesh-vpn-fastd#L124 for the new mesh protocol agnostic configuration using proto gluon_mesh.

Does this also support setting the MTU of the underlying Interface manually?
e.g.:

uci:section('network', 'interface', 'mesh_vpn',
{
ifname = 'mesh-vpn',
proto = 'gluon_mesh',
transitive = 1,
fixed_mtu = 1,
mtu = site.tunneldigger_mesh_vpn.mtu,
}
)

We set the MTU when creating the initial interface configuration and not by using the daemon.
Otherwise we would either need a patch for gluon to support this or patch tunneldigger to set the MTU based on opkg settings.

neocturne · 2016-12-13T00:45:04Z

package/gluon-mesh-vpn-tunneldigger/files/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

+		  proto = 'batadv',
+		  mesh = 'bat0',
+		  mesh_no_rebroadcast = 1,
+		  mtu = site.tunneldigger_mesh_vpn.mtu,


How is the MAC address assigned for the tunneldigger interface?

As it can't be enabled at the same time as the fastd mesh VPN (as both use the interface name mesh-vpn), it could use the same MAC address, I think.

L2TP uses random MAC adresses with each connection. The MAC can be changed manually but only with kernel version 3.18 and up as the support is missing in earlier kernel versions.

There is however a kernel patch which I didn't bother adding at the time:
https://www.mail-archive.com/openl2tp-users@lists.sourceforge.net/msg00060.html

neocturne · 2016-12-13T00:48:46Z

package/gluon-tunneldigger-watchdog/files/usr/bin/tunneldigger-watchdog

+#!/bin/sh
+PIDFILE=/var/run/tunneldigger.mesh-vpn.pid
+
+if [ "$(uci get tunneldigger.@broker[0].enabled)" == "1" ]; then


== is invalid in [] expressions (but accepted by many implementations, sometimes with a warning), this should be =.

neocturne · 2016-12-13T00:50:03Z

package/gluon-tunneldigger-watchdog/files/usr/bin/tunneldigger-watchdog

+    if [ "$(pgrep tunneldigger | head -n 1)" != "$(cat $PIDFILE)" ]; then
+        /etc/init.d/tunneldigger restart
+        logger -t tunneldiger-watchdog "Daemon not running, restarted tunneldigger."
+    elif [ "$(batctl o |grep mesh-vpn |wc -l)" == "0" ]; then


Quite ugly, I wonder if we can find a nicer solution...

neocturne · 2016-12-13T00:52:48Z

package/gluon-mesh-vpn-tunneldigger/files/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

+
+section = uci:add('tunneldigger', 'broker')
+
+uci:section('tunneldigger', 'broker', section,


It would be nicer to use a static section name like mesh_vpn for the broker instead of referring to it using @broker[0], so this can't collide with other tunneldigger configurations (that is, if the tunneldigger init script can handle multiple instances, which I didn't check)

The init script only supports one instance at the time, I guess we could add the support later on but at the time I didn't want to derive too far from the original package.

neocturne · 2016-12-13T00:56:21Z

package/gluon-tunneldigger-watchdog/files/usr/bin/tunneldigger-watchdog

+PIDFILE=/var/run/tunneldigger.mesh-vpn.pid
+
+if [ "$(uci get tunneldigger.@broker[0].enabled)" == "1" ]; then
+    if [ "$(pgrep tunneldigger | head -n 1)" != "$(cat $PIDFILE)" ]; then


I guess this could be pidof tunneldigger? Again, this should probably learn to cope with multiple instances (but this can be done later).

neocturne · 2016-12-13T00:58:41Z

package/gluon-mesh-vpn-tunneldigger/files/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

@@ -0,0 +1,61 @@
+#!/usr/bin/lua


All Lua scripts should be moved from files to luasrc, and GluonSrcDiet (and luadest CP) calls be added to the corresponding Makefiles, as seen in most Gluon packages.

lcb01a · 2016-12-13T11:00:20Z

I commented on a few items, I will fix the rest of the issues as soon as possible.

lcb01a · 2016-12-13T14:53:02Z

I've updated the commit to address some of the issues.
I guess the most important issue left is regarding the new way to configure the interface.

jplitza · 2016-12-14T20:17:46Z

I probably missed something, but why do we need a watchdog? Is Tunneldigger that unstable?

lcb01a · 2017-01-12T12:05:36Z

@jplitza
The watchdog is there just in case the daemon dies, its an optional package.

Regarding the remaing issues:

Ugly watchdog: I could remove the package for now and people who need this can still grab it from the ffrl / düsseldorf repositories
Use the mesh protocol agnostic "gluon_mesh" in uci network config: This is not possible at this moment as the gluon_mesh protocol does not support to set the MTU of the unterlying interface.
e.g. setting mtu = "1360" is completely ignored, this should be added to the gluon_mesh protocol as tunneldigger is not responsible for the initial MTU configuration and we don't use the path MTU feature of tunneldigger (It doesn't work well with batman).

corny · 2017-01-12T13:57:01Z

@lcb01a Just use procd, which is able to restart a died process.

lcb01a · 2017-01-13T10:00:57Z

@corny
Thanks for the suggestion, this looks promising :)

lcb01a · 2017-02-21T16:22:37Z

Hi,

The PR has been updated:

Removed tunneldigger-watchdog, this can still be included via repo if needed
gluon-mesh-von-tunneldigger now uses the gluon_mesh proto correctly

I guess this should address all issues that have emerged.

lcb01a · 2017-02-27T11:48:59Z

Hi, any update on this? :)

neocturne

Found a few things that need to be updated, and I'd really like to see that unnamed broker section go away. The rest looks good!

neocturne · 2017-02-28T11:37:19Z

package/gluon-config-mode-tunneldigger/luasrc/lib/gluon/config-mode/wizard/0300-mesh-vpn.lua

+  end
+end
+
+return M


This file will need to be updated for gluon-web. Doing so should be straightforward, as it is very similar to the fastd-mesh-vpn one.

neocturne · 2017-02-28T11:38:41Z

package/gluon-config-mode-tunneldigger/luasrc/lib/gluon/config-mode/wizard/0300-mesh-vpn.lua

+
+function M.section(form)
+  local msg = i18n.translate('Your internet connection can be used to establish a ' ..
+                             'L2TP VPN connection with other nodes. ' ..


I'm not sure if we want to explicitly mention "L2TP" here, as we're trying to avoid technical terms in the wizard. Unfortunately, I have no idea how to distinguish this from the fastd mesh VPN otherwise...

I guess everyone in the freifunk community knows it by the term L2TP so sticking with it for now should be the best way. We could also use the daemon name instead but I don't know if that is any more meaningful

i'm not sure whether there will be two vpn options or if it's even possible to build with both (fastd/l2tp)
if not, we gain nothing from mentioning the technical term L2TP (or fastd)

thats true, but I rather not use the term "unencrypted" VPN since people falsely assume this means they are not safe from the receiving a law suit.
I can leave out the term completely and just call it VPN :)

neocturne · 2017-02-28T11:39:47Z

package/gluon-mesh-vpn-tunneldigger/Makefile

+  SECTION:=gluon
+  CATEGORY:=Gluon
+  TITLE:=Support for connecting batman-adv meshes via tunneltigger/l2tpv3 pseudowire
+  DEPENDS:=+gluon-core gluon-mesh-batman-adv +gluon-wan-dnsmasq +tunneldigger +iptables-mod-extra +simple-tc


Remove gluon-mesh-batman-adv from the depends, so the package is usable with Babel as well (as soon as we merge that).

neocturne · 2017-02-28T11:40:16Z

package/gluon-mesh-vpn-tunneldigger/check_site.lua

+if need_table('tunneldigger_mesh_vpn.bandwidth_limit', nil, false) then
+  need_boolean('tunneldigger_mesh_vpn.bandwidth_limit.enabled', false)
+  need_number('tunneldigger_mesh_vpn.bandwidth_limit.ingress', false)
+  need_number('tunneldigger_mesh_vpn.bandwidth_limit.egress', false)


Please always use tabs for indentation (in the other files as well).

neocturne · 2017-02-28T11:43:59Z

package/gluon-mesh-vpn-tunneldigger/luasrc/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

+
+section = uci:add('tunneldigger', 'broker')
+
+uci:section('tunneldigger', 'broker', section,


It would be nicer to just explicitly name the section instead of using an anonymous section. You could just

uci:delete('tunneldigger', 'mesh_vpn') uci:section('tunneldigger', 'broker', 'mesh_vpn', ...)

instead of deleting all broker sections. Instead of uci:get_first you could use uci:get_bool, like we to in other scripts.

For migration from an older configuration using an anonymous section, you could use:

if not uci:get('tunneldigger', 'mesh_vpn') then uci:delete_all('tunneldigger', 'broker') end

neocturne · 2017-02-28T11:51:31Z

package/gluon-mesh-vpn-tunneldigger/luasrc/lib/gluon/upgrade/400-mesh-vpn-tunneldigger

+)
+
+uci:save('firewall')
+uci:commit('firewall')


Never commit UCI config in upgrade scripts, the last upgrade script will commit everything.

neocturne · 2017-02-28T11:53:00Z

...age/gluon-mesh-vpn-tunneldigger/luasrc/lib/gluon/upgrade/420-mesh-vpn-tunneldigger-simple-tc

+if not uci:get('simple-tc', 'mesh_vpn') then
+  local config = {
+    ifname = 'mesh-vpn',
+    enabled = 0,


These 0 and 1 can be changed to false and true as well with simple-uci.

neocturne · 2017-02-28T11:53:47Z

package/gluon-migrate-vpn/luasrc/lib/gluon/upgrade/499-migrate-vpn

+local uci = require("simple-uci").cursor()
+
+
+function file_exists(name)


Please use nixio.fs.access as in the other files instead of defining your own function.

neocturne · 2017-02-28T11:54:54Z

package/gluon-migrate-vpn/luasrc/lib/gluon/upgrade/499-migrate-vpn

+  uci:set("fastd", "mesh_vpn", "enabled", enabled)
+  uci:save("fastd")
+  uci:commit("fastd")
+end


Again, you can change enabled to use booleans, and don't commit UCI here.

neocturne · 2017-02-28T11:55:14Z

package/gluon-migrate-vpn/luasrc/lib/gluon/upgrade/499-migrate-vpn

+  uci:save("fastd")
+  uci:commit("fastd")
+end
+


One empty line too much here.

lcb01a · 2017-03-01T15:49:36Z

Hi

The issues you mentioned have been adressed and verified by me. Only the terminology for the config mode isn't clear (Whether to use L2TP in the text or not).

Cheers

neocturne · 2017-03-02T14:46:47Z

As we don't plan to support fastd and L2TP VPN in the same image (yet?), I think it's preferable to keep the texts the same and not to mention L2TP explcitly.

lcb01a · 2017-03-04T21:00:34Z

I removed the mentioning of L2TP from the config mode package and updated the commit :)

*gluon-config-mode-tunneldigger *gluon-mesh-vpn-tunneldigger *gluon-migrate-vpn

…on#978

neocturne · 2017-03-10T19:14:17Z

Thanks, I've just merged your changes.

I ended up doing some refactoring to decrease duplication in the fastd and tunneldigger mesh VPN packages, and added the new site.conf options to the docs. I integerated the migrate-vpn script into a new gluon-mesh-vpn-core package and added support for tunneldigger to the existing gluon-config-mode-mesh-vpn package after adding your translation updates.

It would be great if you could check that I didn't break anything when refactoring.

neocturne requested changes Dec 13, 2016

View reviewed changes

lcb01a mentioned this pull request Dec 13, 2016

L2TP support #752

Closed

rotanid added the 0. type: enhancement The changeset is an enhancement label Dec 13, 2016

neocturne added this to the 2017.1 milestone Feb 25, 2017

neocturne requested changes Feb 28, 2017

View reviewed changes

Added Gluon tunneldigger related packages:

f3b7804

*gluon-config-mode-tunneldigger *gluon-mesh-vpn-tunneldigger *gluon-migrate-vpn

joerg-d pushed a commit to ffggrz/tunneldigger-packages that referenced this pull request Mar 7, 2017

switch all packages to gluon pull request version: freifunk-gluon/glu…

0ce3667

…on#978

joerg-d mentioned this pull request Mar 7, 2017

LEDE fixes ffggrz/tunneldigger-packages#1

Closed

neocturne approved these changes Mar 10, 2017

View reviewed changes

neocturne closed this Mar 10, 2017


		section = uci:add('tunneldigger', 'broker')

		uci:section('tunneldigger', 'broker', section,

		local uci = require("simple-uci").cursor()


		function file_exists(name)

Added Gluon tunneldigger/L2TP related packages #978

Added Gluon tunneldigger/L2TP related packages #978

Conversation

lcb01a commented Dec 12, 2016 • edited

neocturne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcb01a commented Dec 13, 2016

lcb01a commented Dec 13, 2016

jplitza commented Dec 14, 2016

lcb01a commented Jan 12, 2017

corny commented Jan 12, 2017

lcb01a commented Jan 13, 2017

lcb01a commented Feb 21, 2017

lcb01a commented Feb 27, 2017

neocturne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcb01a commented Mar 1, 2017

neocturne commented Mar 2, 2017

lcb01a commented Mar 4, 2017

neocturne commented Mar 10, 2017

lcb01a commented Dec 12, 2016 •

edited