Amend OpenVPN client template to enable keepalive by default

[JonTheNiceGuy <jon@…>]

In /etc/openvpn/client.ovpn, add to the end:

keepalive 10 120

[matthijs]

What would be the usecase for this keepalive, and why did you choose the two values as you did?

[JonTheNiceGuy <jon@…>]

This switch instructs the remote system to send a control-channel "ping" every 10 seconds, and to consider the remote system as down (and thus restart the connection) after 120 seconds.

I use this with systems which have non-reliable network connections, for example 3G sticks, or where the server end-point has a dynamic IP address which may change at very short notice, and it will keep the connection established through events like a router reboot.

The values are straight from the default configuration file.

[matthijs]

On the other hand, for people with a 3G stick that pay per MB, the keepalive option might cost them extra money. So I guess we should make this an option in luci...

[jon@…]

Proposed patch for this issue.

[jon@…]

There were a few issues with the previous patch. Many thanks to blathijs on #fonosfera for pointing them out. I fixed these issues and spotted a couple more. Teamwork's great :)

[matthijs]

While working on upgrading OpenVPN, I noticed that the server already sends keepalives by default. I guess that these should be controlled by the same setting: It makes no sense to disable keepalives on the clients to save bandwidth but keep sending them from the server.

Also note that you've reused the "keepalive" option now: It was already passed to the openvpn server directly. Your current patch will thus pass --keepalive 1 to the openvpn server, instead of --keepalive 10 120 (at least I expect so, I did not test this).

This can probably exploited easily: Instead of making the values of the keepalive option "0" and "1" as in your patch, make them "" and "10 120". Then modify the client.sh script to just replace the value of the keepalive option instead of interpreting it as a boolean.

Finally, it might be good to make this option a tristate. e.g., Keepalive: never / often / seldom, so you can save bandwidth without completely disabling keepalives. With the above changes, this should be rather trivial to implement.

[JonTheNiceGuy <jon@…>]

I guess you can tell my lack of knowledge about this stuff :)

I'll try and put something together tonight or tomorrow night in order to add a new diff. to this ticket.

[jon@…]

This file replaces the previous patch and provides Optional Server or Client end configurations, as well as providing some configuration around the frequency and delay around the keepalive values (as per previous comments). This has set the UCI values the way I was expecting it to :) For interest's sake, I am trying to get guidance on which value takes precidence - lowest value, server's value or something else.

[JonTheNiceGuy <jon@…>]

Oops, forgot to update on here after the last Diff was uploaded.

The switch "--keepalive x y" actually sets "--ping x" and "--ping-restart y". It also checks whether it's in "server" mode, in which case it also sets "--push 'ping x'" and "--push 'ping-restart y'"

Therefore, setting keepalive on the server will override the client and force keepalive pings. Is it worth amending the Diff to reflect this information, or should we leave it as-is?

[matthijs]

Hmm, it seems this patch somehow fell of my radar all that time ago, sorry for that...

Given that there's a number of OpenVPN improvements planned for 2.3.7.0 already, I think we should put this feature into that release.

If OpenVPN will actually push the keepalive settings to the client automatically, I think it would be best to not put the settings into the client configuration at all (and remove the keepalive_client setting altogether). It's best to minimize the number of options that affect the client configuration, since those require regenerating all client configurations, which might not always be obvious to users.

I'll go ahead and make these changes when committing the diff (hopefully somewhere next week).

One more question: We usually try to credit people that contribute patches in the svn log, using their full real names. If you are ok with this, would you tell me your real name?

[Jon Spriggs <jon@…>]

Sure, it's Jon Spriggs.

I would concur with stripping it from the client config - I've been experimenting with the new build, and applying it to my 2.0n and then running

uci set "openvpn.openvpn.keepalive=10 120"

This has done everything I was expecting it to, so I'd just probably just stick the top 6 lines of the patch for luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua and ditch the rest of it.

[matthijs]

I've made a few more changes, here's the updated patch:

diff --git a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
index 63812e8..f424605 100644
--- a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
+++ b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
@@ -20,4 +20,11 @@ x:value("0", translate("disable"))
 x:value("1", translate("enable"))
 x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "wan") or "0"
 
+local x = s:option(ListValue, "keepalive", translate("openvpn_keepalive", "Send periodic keepalives"))
+x:value("0 0", translate("openvpn_keepalive_never", "Never"))
+x:value("10 120", translate("openvpn_keepalive_normal", "Normal (every 10 seconds)"))
+x:value("60 300", translate("openvpn_keepalive_infreq", "Reduced (every 60 seconds)"))
+x:value("600 1200", translate("openvpn_keepalive_rare", "Rare (every 10 minutes)"))
+x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "keepalive") or "10 120"
+
 return m
diff --git a/luci/i18n/english/luasrc/i18n/default.en.lua b/luci/i18n/english/luasrc/i18n/default.en.lua
index 6eb2b8f..ca31d9c 100644
--- a/luci/i18n/english/luasrc/i18n/default.en.lua
+++ b/luci/i18n/english/luasrc/i18n/default.en.lua
@@ -121,6 +121,11 @@ openvpn_client_title = "Clients"
 openvpn_ddns = " You currently have no DynDns configured. <br>If you are on a dial-up line and your ip changes, you need to active DynDns."
 openvpn_desc = "Here you can configure your vpn server. VPN allows you to remotely connect to your home network from anywhere in the world."
 openvpn_generating = " Generating Keys"
+openvpn_keepalive = "Send periodic keepalives"
+openvpn_keepalive_never = "Never"
+openvpn_keepalive_normal = "Normal (every 10 secons)"
+openvpn_keepalive_rare = "Seldom (every 10 minutes)"
+openvpn_keepalive_reduced = "Reduced (every 60 secons)"
 openvpn_key = "OpenVPN is generating keys. This can take up to 10 minutes."
 openvpn_lan = "Local Network"
 openvpn_new_desc = "Please specify the name of this connection."

As you can see, I put the default back in, since as you commented on IRC, it's also done like that for the other options in the file (and since it doesn't matter much, let's go for consistency).

I've also changed the values a bit, now there is a really seldom option sending keepalive every 10 minutes. Does these look sensible to you?

I haven't actually tested anything about this yet, since I'm busy preparing the beta3 release. Perhaps you could see if the above works for you still?

[matthijs]

w00ps, screwed up some of the translation string names. This one is better:

diff --git a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
index 63812e8..9104b47 100644
--- a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
+++ b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
@@ -20,4 +20,11 @@ x:value("0", translate("disable"))
 x:value("1", translate("enable"))
 x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "wan") or "0"
 
+local x = s:option(ListValue, "keepalive", translate("openvpn_keepalive", "Send periodic keepalives"))
+x:value("0 0", translate("openvpn_keepalive_never", "Never"))
+x:value("10 120", translate("openvpn_keepalive_normal", "Normal (every 10 seconds)"))
+x:value("60 300", translate("openvpn_keepalive_reduced", "Reduced (every 60 seconds)"))
+x:value("600 1200", translate("openvpn_keepalive_seldom", "Seldom (every 10 minutes)"))
+x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "keepalive") or "10 120"
+
 return m
diff --git a/luci/i18n/english/luasrc/i18n/default.en.lua b/luci/i18n/english/luasrc/i18n/default.en.lua
index 6eb2b8f..0810bcc 100644
--- a/luci/i18n/english/luasrc/i18n/default.en.lua
+++ b/luci/i18n/english/luasrc/i18n/default.en.lua
@@ -121,6 +121,11 @@ openvpn_client_title = "Clients"
 openvpn_ddns = " You currently have no DynDns configured. <br>If you are on a dial-up line and your ip changes, you need to active DynDns."
 openvpn_desc = "Here you can configure your vpn server. VPN allows you to remotely connect to your home network from anywhere in the world."
 openvpn_generating = " Generating Keys"
+openvpn_keepalive = "Send periodic keepalives"
+openvpn_keepalive_never = "Never"
+openvpn_keepalive_normal = "Normal (every 10 secons)"
+openvpn_keepalive_reduced = "Reduced (every 60 secons)"
+openvpn_keepalive_seldom = "Seldom (every 10 minutes)"
 openvpn_key = "OpenVPN is generating keys. This can take up to 10 minutes."
 openvpn_lan = "Local Network"
 openvpn_new_desc = "Please specify the name of this connection."

[Jon "The Nice Guy" Spriggs <jon@…>]

There's a typo in default.en.lua - "secons" should be "seconds" but aside from that, it's all good.

[matthijs]

I applied this patch locally, I'll push it out to svn later this week.

[matthijs]

(In [2174]) luci-openvpn: Allow changing the keepalive settings.

Thanks to Jon Spriggs for this patch.

Closes: #907