[PATCH 0/5] Add heurestic and customizable value for VPN auth errors

[PATCH 0/5] Add heurestic and customizable value for VPN auth errors

[Jussi Laakkonen]

Implement heuristic for auth error counter to avoid losing credentials
because a VPN server can report back AUTH_FAILED control message
unnecessarily. This seemed to be common with OpenVPN providers that
allow only one authentication at a time from one account only and until
a short leeway-time (usually 30s) after an improper disconnect the
credentials are allowed again.

This is achieved by adding a per provider property "AuthErrorLimit" that
can be set by the user via D-Bus or by a VPN plugin. By default one (1)
auth error is allowed until credentials are re-requested (or cleared by
the VPN agent) if there was a successful connection made in the past
hour. The feature can be disabled by setting value 0 for
"AuthErrorLimit".

This was noticed to happen in a scenario where an OpenVPN is connected
over UDP or TCP (with some providers this seems to happen quite
frequently on both):
1. connect VPN over cellular with autoconnect set
2. connection is switched to WiFi and let VPN to autoconnect,
3. set cellular off and back on again (WiFi should be preferred here)
4. disconnect WiFi and let VPN to autoconnect over cellular
5. re-enable WiFi after which VPN server reports AUTH_FAILED to client
6. next connection attempt re-requests VPN creds with error set in msg.
Usually this is enough to time the network disconnects in the sense that
OpenVPN binary cannot send the disconnect message back to server having
following in the system log:
    openvpn: event_wait : Interrupted system call (code=4)
    openvpn: SIGTERM received, sending exit notification to peer
    openvpn: write UDP: Network is unreachable (code=101)
    openvpn: Closing TUN/TAP interface

As a solution each provider sets a run-time only previous_connect_time
using the monotonic boot time clock whenever the connection has been
successful. This, in conjunction with a limit for allowed authentication
errors is used to determine whether the actual authentication error count
is reported back to the VPN agent or by returning 0 attempt again with
the current credentials. After a connection has been made all errors are
cleared. If the connection abruptly goes away, e.g., the server is lost
only the conn_error_counter is increased and previous_connect_time is
cleared but this does not trigger clearing of the authentication error
and, provided the credentials are still valid, the next attempt succeeds
when the server is again reachable. Same applies for a proper
disconnect. Thus, this solution does not interfere with normal cases but
mitigates unwanted credential loss in case if the VPN server determines
connection limit being full and reports back with an authentication error
message.

OpenVPN sets this value by default to 10 unless already defined and can
be disabled by setting value zero (0). Some VPNs may not require this
option and that is why lenience of one (1) auth error is allowed by
default.

Also change to report back EALREADY in case a VPN is attempted to be
disconneced when already disconnecting. It shouldn't be treated as an
error.

Jussi Laakkonen (5):
  vpn: Report EALREADY back to caller if VPN is already disconnecting
  vpn-provider: Ignore error adding when state is idle/unknown
  vpn-provider: Add auth error check heuristic to avoid losing creds
  openvpn: Default to 10 AuthErrorLimit unless set by user
  doc: Document AuthErrorLimit in VPN connection API

 doc/vpn-connection-api.txt |  13 ++++
 plugins/vpn.c              |   3 +-
 vpn/plugins/openvpn.c      |   9 +++
 vpn/vpn-provider.c         | 125 +++++++++++++++++++++++++++++++++++++
 vpn/vpn-provider.h         |   2 +
 5 files changed, 151 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/5] vpn: Report EALREADY back to caller if VPN is already disconnecting

[Jussi Laakkonen]

Instead of reporting EINVAL report EALREADY to return correct
information back to the caller. This way caller knows that the call was
not to a wrong identifier but the VPN has already been requested to
shut down. The error eventually translates to InProgress D-Bus message
back to the caller.
---
 plugins/vpn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/plugins/vpn.c b/plugins/vpn.c
index 387447c3..f72d85ab 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -3,6 +3,7 @@
  *  Connection Manager
  *
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -1026,7 +1027,7 @@ static int disconnect_provider(struct connection_data *data)
 
 	if (data->disconnect_call) {
 		DBG("already disconnecting");
-		return -EINVAL;
+		return -EALREADY;
 	}
 
 	message = dbus_message_new_method_call(VPN_SERVICE, data->path,
-- 
2.20.1

[PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown

[Jussi Laakkonen]

Do not allow to add errors for provider that is already set into idle
state or is in unknown state. This case may happen when networks are
rapidly changed and VPN did not call the callback connect_cb() until the
VPN is died and vpn.c:vpn_died() initiates cleanup in the VPN which
eventually calls connect_cb() with an error.
---
 vpn/vpn-provider.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index f30ce04f..90e21e95 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -2026,6 +2026,21 @@ int vpn_provider_indicate_error(struct vpn_provider *provider,
 	DBG("provider %p id %s error %d", provider, provider->identifier,
 									error);
 
+	/*
+	 * Ignore adding of errors when the VPN is idle or not set. Calls may
+	 * happen in a case when networks are rapidly changed and the call to
+	 * vpn_died() is done before executing the connect_cb() from the
+	 * plugin. Then vpn.c:vpn_died() executes the plugin specific died()
+	 * function which may free the plugin private data, containing also
+	 * the callback which hasn't yet been called. As a result the provider
+	 * might already been reset to idle state when the callback is executed
+	 * resulting in unnecessary reset of the previous successful connect
+	 * timer and adding of an error for already disconnected VPN.
+	 */
+	if (provider->state == VPN_PROVIDER_STATE_IDLE ||
+				provider->state == VPN_PROVIDER_STATE_UNKNOWN)
+		return 0;
+
 	vpn_provider_set_state(provider, VPN_PROVIDER_STATE_FAILURE);
 
 	vpn_provider_add_error(provider, error);
-- 
2.20.1

[PATCH 3/5] vpn-provider: Add auth error check heuristic to avoid losing creds

[Jussi Laakkonen]

Implement a heuristic into authentication error getter to avoid losing
credentials in scenarios where the VPN server expects a clean shutdown
but the route/network has gone down and client cannot do this and as a
result the server responds to consequtive authentications with auth
failed message. As a result this would cause the VPN agent to clear the
credentials as the authentication errors are passed to it, even though
there was no real authentication error in place.

This was noticed to happen in a scenario where an OpenVPN service is
1) connected over UDP using cellular, 2) connection is switched to WiFi
letting VPN to connect, 3) triggering cellular off and back on again,
4) disconnecting WiFi and letting VPN to connect over cellular after
which 5) WiFi is re-enabled. Usually this is enough to time the network
disconnects in the sense that OpenVPN binary cannot send the disconnect
message back to server having following in the system log:
    openvpn: event_wait : Interrupted system call (code=4)
    openvpn: SIGTERM received, sending exit notification to peer
    openvpn: write UDP: Network is unreachable (code=101)
    openvpn: Closing TUN/TAP interface

As a solution each provider sets a run-time only "previous_connect_time"
using the monotonic boot time clock whenever the connection has been
successful. This, in conjunction with a limit for allowed authentication
errors is used to determine whether the actual authentication error
count reported to VPN agent or not. After a connection has been made all
errors are cleared. If the connection abruptly goes away, e.g., the
server is lost only the conn_error_counter is increased and
previous_connect_time is cleared but this does not trigger clearing of
the authentication error and, provided the credentials are still valid,
next attempt succeeds whet the server is again reachable. Same applies
for a proper disconnect. Thus, this solution does not interfere with
normal cases but mitigates unwanted credential loss in case if the VPN
server determines connection limit being full and reports back with
authentication error message.

Some VPNs may not require this option at all. Thus, limit is set as a
configurable per provider parameter "AuthErrorLimit" which defaults to
"1" and the heuristic can be completely disabled by setting value "0".
The value can be changed either by the user over D-Bus or made to be a
higher default for a provider by using provider_set_auth_error_limit(),
e.g., by a VPN plugin. The default per provider value is used only when
there is no user defined one. This way the plugin default value will not
get saved to the VPN settings and plugin default is changeable.
---
 vpn/vpn-provider.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
 vpn/vpn-provider.h |   2 +
 2 files changed, 112 insertions(+)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 90e21e95..4200c462 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -24,6 +24,14 @@
 #include <config.h>
 #endif
 
+/*
+ * One hour difference in seconds between connections for checking whether to
+ * treat the authentication error as a real error or as a result of rapid
+ * transport change.
+ */
+#define CONNECT_OK_DIFF		((time_t)60*60)
+#define AUTH_ERROR_LIMIT_DEFAULT	1
+
 #define STATE_INTERVAL_DEFAULT		0
 
 #define CONNMAN_STATE_ONLINE		"online"
@@ -39,6 +47,7 @@
 #include <connman/log.h>
 #include <gweb/gresolv.h>
 #include <netdb.h>
+#include <time.h>
 
 #include "../src/connman.h"
 #include "connman/agent.h"
@@ -100,6 +109,8 @@ struct vpn_provider {
 	unsigned int auth_error_counter;
 	unsigned int conn_error_counter;
 	unsigned int signal_watch;
+	unsigned int auth_error_limit;
+	time_t previous_connect_time;
 };
 
 struct vpn_provider_connect_data {
@@ -1238,12 +1249,15 @@ static void reset_error_counters(struct vpn_provider *provider)
 	if (!provider)
 		return;
 
+	DBG("provider %p", provider);
+
 	provider->auth_error_counter = provider->conn_error_counter = 0;
 }
 
 static int vpn_provider_save(struct vpn_provider *provider)
 {
 	GKeyFile *keyfile;
+	const char *value;
 
 	DBG("provider %p immutable %s", provider,
 					provider->immutable ? "yes" : "no");
@@ -1274,6 +1288,11 @@ static int vpn_provider_save(struct vpn_provider *provider)
 	g_key_file_set_string(keyfile, provider->identifier,
 			"VPN.Domain", provider->domain);
 
+	value = vpn_provider_get_string(provider, "AuthErrorLimit");
+	if (value)
+		g_key_file_set_string(keyfile, provider->identifier,
+						"AuthErrorLimit", value);
+
 	if (provider->user_networks) {
 		gchar **networks;
 		gsize network_count;
@@ -1782,6 +1801,18 @@ static void append_dns(DBusMessageIter *iter, void *user_data)
 		append_nameservers(iter, provider->nameservers);
 }
 
+static time_t get_uptime(void)
+{
+	struct timespec t = { 0 };
+
+	if (clock_gettime(CLOCK_BOOTTIME, &t) == -1) {
+		connman_warn("clock_gettime() error %d, uptime failed", errno);
+		return 0;
+	}
+
+	return t.tv_sec;
+}
+
 static int provider_indicate_state(struct vpn_provider *provider,
 				enum vpn_provider_state state)
 {
@@ -1797,6 +1828,8 @@ static int provider_indicate_state(struct vpn_provider *provider,
 	provider->state = state;
 
 	if (state == VPN_PROVIDER_STATE_READY) {
+		provider->previous_connect_time = get_uptime();
+
 		connman_dbus_property_changed_basic(provider->path,
 					VPN_CONNECTION_INTERFACE, "Index",
 					DBUS_TYPE_INT32, &provider->index);
@@ -2009,8 +2042,10 @@ void vpn_provider_add_error(struct vpn_provider *provider,
 {
 	switch (error) {
 	case VPN_PROVIDER_ERROR_UNKNOWN:
+		provider->previous_connect_time = 0;
 		break;
 	case VPN_PROVIDER_ERROR_CONNECT_FAILED:
+		provider->previous_connect_time = 0;
 		++provider->conn_error_counter;
 		break;
 	case VPN_PROVIDER_ERROR_LOGIN_FAILED:
@@ -2018,6 +2053,10 @@ void vpn_provider_add_error(struct vpn_provider *provider,
 		++provider->auth_error_counter;
 		break;
 	}
+
+	DBG("%p connect errors %d auth errors %d", provider,
+						provider->conn_error_counter,
+						provider->auth_error_counter);
 }
 
 int vpn_provider_indicate_error(struct vpn_provider *provider,
@@ -2192,6 +2231,7 @@ static void provider_initialize(struct vpn_provider *provider)
 					g_free, free_route);
 	provider->setting_strings = g_hash_table_new_full(g_str_hash,
 					g_str_equal, g_free, free_setting);
+	provider->auth_error_limit = AUTH_ERROR_LIMIT_DEFAULT;
 }
 
 static struct vpn_provider *vpn_provider_new(void)
@@ -2897,6 +2937,15 @@ bool vpn_provider_get_string_immutable(struct vpn_provider *provider,
 	return setting->immutable;
 }
 
+void vpn_provider_set_auth_error_limit(struct vpn_provider *provider,
+							unsigned int limit)
+{
+	if (!provider)
+		return;
+
+	provider->auth_error_limit = limit;
+}
+
 bool __vpn_provider_check_routes(struct vpn_provider *provider)
 {
 	if (!provider)
@@ -3205,9 +3254,70 @@ const char *vpn_provider_get_path(struct vpn_provider *provider)
 	return provider->path;
 }
 
+/*
+ * This crude heuristic is meant to mitigate an issue with certain VPN
+ * providers that allow only one authentication per account at a time. These
+ * providers require the VPN client shuts down cleanly by sending an exit
+ * notification. In many cases this is not possible as the transport may
+ * already be gone and there is no route to the VPN server. In such case server
+ * may return authentication error to indicate that an other client is active
+ * and reserves the slot.
+ *
+ * By allowing the VPN client to try again with the following conditons the
+ * unnecessary credential resets done by VPN agent can be avoided. VPN client
+ * is allowed to retry if 1) there was a successful connection to the server
+ * within the specified CONNECT_OK_DIFF time and 2) the provider specific
+ * limit for auth errors is not reached the unnecessary credential resets in
+ * this case are avoided.
+ *
+ * This feature is controlled by the provider specific value for
+ * "AuthErrorLimit". Setting the value to 0 feature is disabled. User defined
+ * value is preferred if set, otherwise plugin default set with
+ * vpn_provider_set_auth_error_limit() is used, which defaults to
+ * AUTH_ERROR_LIMIT_DEFAULT.
+ */
+static bool ignore_authentication_errors(struct vpn_provider *provider)
+{
+	const char *val;
+	unsigned int limit;
+	time_t uptime;
+	time_t diff;
+
+	val = vpn_provider_get_string(provider, "AuthErrorLimit");
+	if (val)
+		limit = g_ascii_strtoull(val, NULL, 10);
+	else
+		limit = provider->auth_error_limit;
+
+	if (!limit || !provider->previous_connect_time) {
+		DBG("%p errors %u %s", provider, provider->auth_error_counter,
+					!limit ?
+					"disabled by 0 limit" :
+					"no previous ok conn");
+		return false;
+	}
+
+	uptime = get_uptime();
+	diff = uptime - provider->previous_connect_time;
+
+	DBG("%p errors %u limit %u uptime %jd time diff %jd", provider,
+				provider->auth_error_counter, limit,
+				(intmax_t)uptime, (intmax_t)diff);
+
+	if (diff <= CONNECT_OK_DIFF && provider->auth_error_counter <= limit) {
+		DBG("ignore auth errors");
+		return true;
+	}
+
+	return false;
+}
+
 unsigned int vpn_provider_get_authentication_errors(
 						struct vpn_provider *provider)
 {
+	if (ignore_authentication_errors(provider))
+		return 0;
+
 	return provider->auth_error_counter;
 }
 
diff --git a/vpn/vpn-provider.h b/vpn/vpn-provider.h
index f7fa8591..ee09e8ef 100644
--- a/vpn/vpn-provider.h
+++ b/vpn/vpn-provider.h
@@ -88,6 +88,8 @@ int vpn_provider_set_boolean(struct vpn_provider *provider, const char *key,
 							bool force_change);
 bool vpn_provider_get_boolean(struct vpn_provider *provider, const char *key,
 							bool default_value);
+void vpn_provider_set_auth_error_limit(struct vpn_provider *provider,
+							unsigned int limit);
 
 int vpn_provider_set_state(struct vpn_provider *provider,
 					enum vpn_provider_state state);
-- 
2.20.1

[PATCH 4/5] openvpn: Default to 10 AuthErrorLimit unless set by user

[Jussi Laakkonen]

Unless the user has set a value for provider property "AuthErrorLimit"
default to 10 attempts after a successful connection has been made before
allowing to clear the credentials. This is imperative for the cases when
OpenVPN server requires client to do a clean shutdown but the network
goes down before it can be completed. In these cases server may respond
back with AUTH_FAILED control message until it determines that old
client is realy gone. By using this limit credentials are not
unnecessarily cleared because there was no real problem with them.
---
 vpn/plugins/openvpn.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/vpn/plugins/openvpn.c b/vpn/plugins/openvpn.c
index daf66cd5..7089d6ce 100644
--- a/vpn/plugins/openvpn.c
+++ b/vpn/plugins/openvpn.c
@@ -1099,6 +1099,15 @@ static int ov_connect(struct vpn_provider *provider,
 	const char *tmpdir;
 	struct ov_private_data *data;
 
+	/*
+	 * Explicitly set limit of 10 for authentication errors. This defines
+	 * the authentication error message limit from the server before VPN
+	 * agent is instructed to clear the credentials. This is effective only
+	 * after a successful connection has been made within CONNECT_OK_DIFF
+	 * time. User defined value for "AuthErrorLimit" overrides this.
+	 */
+	vpn_provider_set_auth_error_limit(provider, 10);
+
 	data = g_try_new0(struct ov_private_data, 1);
 	if (!data)
 		return -ENOMEM;
-- 
2.20.1

[PATCH 5/5] doc: Document AuthErrorLimit in VPN connection API

[Jussi Laakkonen]

Add documentation of AuthErrorLimit that can be set for each provider
via the D-Bus API.
---
 doc/vpn-connection-api.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/doc/vpn-connection-api.txt b/doc/vpn-connection-api.txt
index 6e6293e4..1f6403fe 100644
--- a/doc/vpn-connection-api.txt
+++ b/doc/vpn-connection-api.txt
@@ -235,6 +235,19 @@ Properties	string State [readonly]
 			The VPN server activated route. These routes
 			are pushed to connman by VPN server.
 
+		string AuthErrorLimit
+
+			This value defines the amount of authentication errors
+			that are allowed before informing VPN agent to clear
+			the credentials in case there was a previous successful
+			VPN connection made within one hour. This is to be used
+			with providers that allow only one login from one
+			account at a time to prevent clearing of credentials
+			when networks are rapidly changed. This value is used
+			as an integer and if unset this default to "1" for all
+			except OpenVPN that uses value "10". Setting value "0"
+			disables the feature for the provider.
+
 		There can be other properties also but as the VPN
 		technologies are so different, they have different
 		kind of options that they need, so not all options
-- 
2.20.1

Re: [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown

[Daniel Wagner]

On Thu, Sep 02, 2021 at 06:11:21PM +0300, Jussi Laakkonen wrote:
> Do not allow to add errors for provider that is already set into idle
> state or is in unknown state. This case may happen when networks are
> rapidly changed and VPN did not call the callback connect_cb() until the
> VPN is died and vpn.c:vpn_died() initiates cleanup in the VPN which
> eventually calls connect_cb() with an error.

I wonder if we should set the VPN state immediately when starting the
connection process to some value != unknown.

> ---
>  vpn/vpn-provider.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
> index f30ce04f..90e21e95 100644
> --- a/vpn/vpn-provider.c
> +++ b/vpn/vpn-provider.c
> @@ -2026,6 +2026,21 @@ int vpn_provider_indicate_error(struct vpn_provider *provider,
>  	DBG("provider %p id %s error %d", provider, provider->identifier,
>  									error);
>  
> +	/*
> +	 * Ignore adding of errors when the VPN is idle or not set. Calls may
> +	 * happen in a case when networks are rapidly changed and the call to
> +	 * vpn_died() is done before executing the connect_cb() from the
> +	 * plugin. Then vpn.c:vpn_died() executes the plugin specific died()
> +	 * function which may free the plugin private data, containing also
> +	 * the callback which hasn't yet been called. As a result the provider
> +	 * might already been reset to idle state when the callback is executed
> +	 * resulting in unnecessary reset of the previous successful connect
> +	 * timer and adding of an error for already disconnected VPN.
> +	 */
> +	if (provider->state == VPN_PROVIDER_STATE_IDLE ||
> +				provider->state == VPN_PROVIDER_STATE_UNKNOWN)
> +		return 0;

Checking the unknown state looks here a bit strange. The usual meaning
of unknown is it hasn't been initialized. But then this is exactly
what's happening here I assume.

Re: [PATCH 0/5] Add heurestic and customizable value for VPN auth errors

[Daniel Wagner]

Hi Jussi,

On Thu, Sep 02, 2021 at 06:11:19PM +0300, Jussi Laakkonen wrote:
> Implement heuristic for auth error counter to avoid losing credentials
> because a VPN server can report back AUTH_FAILED control message
> unnecessarily. This seemed to be common with OpenVPN providers that
> allow only one authentication at a time from one account only and until
> a short leeway-time (usually 30s) after an improper disconnect the
> credentials are allowed again.
> 
> This is achieved by adding a per provider property "AuthErrorLimit" that
> can be set by the user via D-Bus or by a VPN plugin. By default one (1)
> auth error is allowed until credentials are re-requested (or cleared by
> the VPN agent) if there was a successful connection made in the past
> hour. The feature can be disabled by setting value 0 for
> "AuthErrorLimit".
> 
> This was noticed to happen in a scenario where an OpenVPN is connected
> over UDP or TCP (with some providers this seems to happen quite
> frequently on both):
> 1. connect VPN over cellular with autoconnect set
> 2. connection is switched to WiFi and let VPN to autoconnect,
> 3. set cellular off and back on again (WiFi should be preferred here)
> 4. disconnect WiFi and let VPN to autoconnect over cellular
> 5. re-enable WiFi after which VPN server reports AUTH_FAILED to client
> 6. next connection attempt re-requests VPN creds with error set in msg.
> Usually this is enough to time the network disconnects in the sense that
> OpenVPN binary cannot send the disconnect message back to server having
> following in the system log:
>     openvpn: event_wait : Interrupted system call (code=4)
>     openvpn: SIGTERM received, sending exit notification to peer
>     openvpn: write UDP: Network is unreachable (code=101)
>     openvpn: Closing TUN/TAP interface
> 
> As a solution each provider sets a run-time only previous_connect_time
> using the monotonic boot time clock whenever the connection has been
> successful. This, in conjunction with a limit for allowed authentication
> errors is used to determine whether the actual authentication error count
> is reported back to the VPN agent or by returning 0 attempt again with
> the current credentials. After a connection has been made all errors are
> cleared. If the connection abruptly goes away, e.g., the server is lost
> only the conn_error_counter is increased and previous_connect_time is
> cleared but this does not trigger clearing of the authentication error
> and, provided the credentials are still valid, the next attempt succeeds
> when the server is again reachable. Same applies for a proper
> disconnect. Thus, this solution does not interfere with normal cases but
> mitigates unwanted credential loss in case if the VPN server determines
> connection limit being full and reports back with an authentication error
> message.
> 
> OpenVPN sets this value by default to 10 unless already defined and can
> be disabled by setting value zero (0). Some VPNs may not require this
> option and that is why lenience of one (1) auth error is allowed by
> default.
> 
> Also change to report back EALREADY in case a VPN is attempted to be
> disconneced when already disconnecting. It shouldn't be treated as an
> error.

Thanks again for the extensive documentation. I've applied the series. I
assume this has been in production for a while, so it supposed to work
:) And it doesn't look too complex to figure out what's going on. So
let's see if there are any regression reports (which I don't expect)

Thanks,
Daniel

Re: [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown

[Jussi Laakkonen]

Hi Daniel,

On 9/13/21 9:32 AM, Daniel Wagner wrote:
> On Thu, Sep 02, 2021 at 06:11:21PM +0300, Jussi Laakkonen wrote:
>> Do not allow to add errors for provider that is already set into idle
>> state or is in unknown state. This case may happen when networks are
>> rapidly changed and VPN did not call the callback connect_cb() until the
>> VPN is died and vpn.c:vpn_died() initiates cleanup in the VPN which
>> eventually calls connect_cb() with an error.
> 
> I wonder if we should set the VPN state immediately when starting the
> connection process to some value != unknown.
> 

Perhaps. But that would need a bit of more work to make sure the state 
machine is not getting any new issues introduced. Maybe a task for some 
later work to improve that. Unknown is not perhaps the best 
representation as a name for that but it does the trick and works, as of 
now :).

>> ---
>>   vpn/vpn-provider.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
>> index f30ce04f..90e21e95 100644
>> --- a/vpn/vpn-provider.c
>> +++ b/vpn/vpn-provider.c
>> @@ -2026,6 +2026,21 @@ int vpn_provider_indicate_error(struct vpn_provider *provider,
>>   	DBG("provider %p id %s error %d", provider, provider->identifier,
>>   									error);
>>   
>> +	/*
>> +	 * Ignore adding of errors when the VPN is idle or not set. Calls may
>> +	 * happen in a case when networks are rapidly changed and the call to
>> +	 * vpn_died() is done before executing the connect_cb() from the
>> +	 * plugin. Then vpn.c:vpn_died() executes the plugin specific died()
>> +	 * function which may free the plugin private data, containing also
>> +	 * the callback which hasn't yet been called. As a result the provider
>> +	 * might already been reset to idle state when the callback is executed
>> +	 * resulting in unnecessary reset of the previous successful connect
>> +	 * timer and adding of an error for already disconnected VPN.
>> +	 */
>> +	if (provider->state == VPN_PROVIDER_STATE_IDLE ||
>> +				provider->state == VPN_PROVIDER_STATE_UNKNOWN)
>> +		return 0;
> 
> Checking the unknown state looks here a bit strange. The usual meaning
> of unknown is it hasn't been initialized. But then this is exactly
> what's happening here I assume.
> 

Yes, to cover all the cases as we may get these checks for a new VPN.

Cheers,
  Jussi

Re: [PATCH 0/5] Add heurestic and customizable value for VPN auth errors

[Jussi Laakkonen]

Hi Daniel,

On 9/13/21 9:43 AM, Daniel Wagner wrote:
> Hi Jussi,
> 
> On Thu, Sep 02, 2021 at 06:11:19PM +0300, Jussi Laakkonen wrote:
>> Implement heuristic for auth error counter to avoid losing credentials
>> because a VPN server can report back AUTH_FAILED control message
>> unnecessarily. This seemed to be common with OpenVPN providers that
>> allow only one authentication at a time from one account only and until
>> a short leeway-time (usually 30s) after an improper disconnect the
>> credentials are allowed again.
>>
>> This is achieved by adding a per provider property "AuthErrorLimit" that
>> can be set by the user via D-Bus or by a VPN plugin. By default one (1)
>> auth error is allowed until credentials are re-requested (or cleared by
>> the VPN agent) if there was a successful connection made in the past
>> hour. The feature can be disabled by setting value 0 for
>> "AuthErrorLimit".
>>
>> This was noticed to happen in a scenario where an OpenVPN is connected
>> over UDP or TCP (with some providers this seems to happen quite
>> frequently on both):
>> 1. connect VPN over cellular with autoconnect set
>> 2. connection is switched to WiFi and let VPN to autoconnect,
>> 3. set cellular off and back on again (WiFi should be preferred here)
>> 4. disconnect WiFi and let VPN to autoconnect over cellular
>> 5. re-enable WiFi after which VPN server reports AUTH_FAILED to client
>> 6. next connection attempt re-requests VPN creds with error set in msg.
>> Usually this is enough to time the network disconnects in the sense that
>> OpenVPN binary cannot send the disconnect message back to server having
>> following in the system log:
>>      openvpn: event_wait : Interrupted system call (code=4)
>>      openvpn: SIGTERM received, sending exit notification to peer
>>      openvpn: write UDP: Network is unreachable (code=101)
>>      openvpn: Closing TUN/TAP interface
>>
>> As a solution each provider sets a run-time only previous_connect_time
>> using the monotonic boot time clock whenever the connection has been
>> successful. This, in conjunction with a limit for allowed authentication
>> errors is used to determine whether the actual authentication error count
>> is reported back to the VPN agent or by returning 0 attempt again with
>> the current credentials. After a connection has been made all errors are
>> cleared. If the connection abruptly goes away, e.g., the server is lost
>> only the conn_error_counter is increased and previous_connect_time is
>> cleared but this does not trigger clearing of the authentication error
>> and, provided the credentials are still valid, the next attempt succeeds
>> when the server is again reachable. Same applies for a proper
>> disconnect. Thus, this solution does not interfere with normal cases but
>> mitigates unwanted credential loss in case if the VPN server determines
>> connection limit being full and reports back with an authentication error
>> message.
>>
>> OpenVPN sets this value by default to 10 unless already defined and can
>> be disabled by setting value zero (0). Some VPNs may not require this
>> option and that is why lenience of one (1) auth error is allowed by
>> default.
>>
>> Also change to report back EALREADY in case a VPN is attempted to be
>> disconneced when already disconnecting. It shouldn't be treated as an
>> error.
> 
> Thanks again for the extensive documentation. I've applied the series. I
> assume this has been in production for a while, so it supposed to work
> :) And it doesn't look too complex to figure out what's going on. So
> let's see if there are any regression reports (which I don't expect)
> 

You're welcome. These kind of odd issues that do not happen that often 
do really need a long explanation. If the fix to odd issue then has some 
issues itself these kind of stuff in commit messages help a lot I bet.

Yes we've been running this for a while now. Wider test comes when the 
new release comes out. It may be that the 1 hour limit could be adjusted 
downwards in the future but wider results show (or, when there are no 
more complaints - the origin of this was a report from an user) what are 
the most optimal limits. That is why the attempt count is an user 
configurable value. I'll setup new patches if needed.

Cheers,
  Jussi