[PATCH 00/11] Add association state for VPNs

[PATCH 00/11] Add association state for VPNs

[Jussi Laakkonen]

This patch set adds the association state also for the VPNs. This state is to
indicate that the VPN is waiting for VPN agent to provide input given by user.
In this state service.c must not do connect timeout checks as the timers for
both differ in length, default being 120s for connect timeout and 300s for VPN
agent dialog timeout.

In order to facilitate this change the association state had to be implemented
also for VPNs. It is common state for services and like with services the
association state for VPNs preceeds the configuration state (on VPN side
connect state). Both vpn.c plugins on connmand and vpnd side require changes
to accommodate this state. When the VPN agent succeeds in getting the input
from the user the state transitions from association to connect (configuration)
state and, thus, requires no specific changes to VPN plugins.

On connmand side the association state is the initial state when VPN is getting
connected and the state needs to be accounted as a connecting state in
plugins/vpn.c to not to lose transport ident for it and in provider.c as a
pre-configuration state to not to start the connect timeout for the VPN before
the VPN is in configuration state. The reason for the latter is that the
connect timeout should be exact and start from the point when
connect/configuration state is entered.

On vpnd side association state is, like on connmand side, the initial state for
the VPN getting connected. After the VPN agent succeeds getting the information
from the user (credentials) the state transitions to connect (configuratioin).
There may be a possibility for a VPN plugin to run without VPN agent and thus
in these cases it is ensured that the vpn/plugins/vpn.c:vpn_notify() does
the state transition in such cases. It is allowed go back to association state
from connect state but not from other states.

Jussi Laakkonen (11):
  agent: Cancel agent request on NoReply D-Bus error
  vpn-provider: Use association state for VPN agent input wait
  vpn: Add association state before connect state
  vpn-agent: Do connect state transition after input dialog check
  service: Explicit VPN connect timeout, ignore in VPN agent wait
  provider: Handle VPN configuration and association states
  vpn: Add support for association state, add state getter
  vpn: Check if connecting when setting state or disconnecting
  vpn: Add VPN agent use callback for plugins
  vpn-provider: Transition to CONNECT state with agentless VPNs
  doc: Update VPN documentation for association state

 doc/vpn-connection-api.txt |  4 +--
 doc/vpn-overview.txt       |  7 ++++-
 include/provider.h         |  9 +++---
 plugins/vpn.c              | 23 ++++++++++++---
 src/agent.c                |  4 ++-
 src/connman.h              |  2 ++
 src/provider.c             | 22 ++++++++++++++-
 src/service.c              | 52 ++++++++++++++++++++++++++++++----
 vpn/plugins/vpn.c          | 44 ++++++++++++++++++++++++++++-
 vpn/plugins/vpn.h          | 12 ++++----
 vpn/vpn-agent.c            |  6 +++-
 vpn/vpn-provider.c         | 58 ++++++++++++++++++++++++++++++++++----
 vpn/vpn-provider.h         |  7 +++++
 13 files changed, 220 insertions(+), 30 deletions(-)

-- 
2.39.5

[PATCH 01/11] agent: Cancel agent request on NoReply D-Bus error

[Jussi Laakkonen]

Handle also the NoReply D-Bus error as this is commonly sent back when
the timeout set for the request is exceeded. Canceling the request later
becomes impossible as agent->pending will be set to NULL in
agent_finalize_pending(). Thus, making later calls to
connman_agent_cancel() to not to close down agent dialogs but instead
they are piled up on top of each other.
---
 src/agent.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/agent.c b/src/agent.c
index 23517d9b..e2d1ef09 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -201,7 +201,9 @@ static void agent_receive_message(DBusPendingCall *call, void *user_data)
 	if (dbus_message_is_error(reply,
 			"org.freedesktop.DBus.Error.Timeout") ||
 			dbus_message_is_error(reply,
-			"org.freedesktop.DBus.Error.TimedOut")) {
+			"org.freedesktop.DBus.Error.TimedOut") ||
+			dbus_message_is_error(reply,
+			"org.freedesktop.DBus.Error.NoReply")) {
 		send_cancel_request(agent, agent->pending);
 	}
 
-- 
2.39.5

[PATCH 02/11] vpn-provider: Use association state for VPN agent input wait

[Jussi Laakkonen]

Use the association state with VPNs to define that VPN is waiting for
input via agent. The same state is used for every service in connmand so
this change synchronizes the states in both.

Set the state to be identical to connmand side states by injecting this
into the VPN state machine before the connect state ("configuration"
state). This is then changed when the state is set to connected either
by getting a non-error reply from VPN agent or via VPN driver gets
connect state notify.

In this is association state the VPN indicates to connmand that the VPN
is requesting user input via agent and shouldn't be subject to connect
timeout checks. Having this additional state allows to obey the D-Bus VPN
agent query timeout value, instead of getting the dialog shut down at
connection timeout.
---
 vpn/vpn-provider.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 vpn/vpn-provider.h |  6 ++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 4bcb8373..56040e65 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -1487,6 +1487,23 @@ int __vpn_provider_disconnect(struct vpn_provider *provider)
 	return err;
 }
 
+static bool is_connected_state(enum vpn_provider_state state)
+{
+	switch (state) {
+	case VPN_PROVIDER_STATE_UNKNOWN:
+	case VPN_PROVIDER_STATE_IDLE:
+	case VPN_PROVIDER_STATE_DISCONNECT:
+	case VPN_PROVIDER_STATE_FAILURE:
+		break;
+	case VPN_PROVIDER_STATE_CONNECT:
+	case VPN_PROVIDER_STATE_READY:
+	case VPN_PROVIDER_STATE_ASSOCIATION:
+		return true;
+	}
+
+	return false;
+}
+
 static void connect_cb(struct vpn_provider *provider, void *user_data,
 								int error)
 {
@@ -1509,6 +1526,8 @@ static void connect_cb(struct vpn_provider *provider, void *user_data,
 			 * No reply, disconnect called by connmand because of
 			 * connection timeout.
 			 */
+			vpn_provider_indicate_error(provider,
+					VPN_PROVIDER_ERROR_CONNECT_FAILED);
 			break;
 		case ENOMSG:
 			/* fall through */
@@ -1533,9 +1552,7 @@ static void connect_cb(struct vpn_provider *provider, void *user_data,
 			 * process gets killed and vpn_died() is called to make
 			 * the provider back to idle state.
 			 */
-			if (provider->state == VPN_PROVIDER_STATE_CONNECT ||
-						provider->state ==
-						VPN_PROVIDER_STATE_READY) {
+			if (is_connected_state(provider->state)) {
 				if (provider->driver->set_state)
 					provider->driver->set_state(provider,
 						VPN_PROVIDER_STATE_DISCONNECT);
@@ -1597,6 +1614,17 @@ int __vpn_provider_connect(struct vpn_provider *provider, DBusMessage *msg)
 		if (reply)
 			g_dbus_send_message(connection, reply);
 
+		return -EINPROGRESS;
+	case VPN_PROVIDER_STATE_ASSOCIATION:
+		/*
+		 * Do not interrupt user when inputting credentials via agent.
+		 * The driver is in CONNECT state that would return EINPROGRESS
+		 * and change provider state to CONNECT.
+		 */
+		reply = __connman_error_in_progress(msg);
+		if (reply)
+			g_dbus_send_message(connection, reply);
+
 		return -EINPROGRESS;
 	case VPN_PROVIDER_STATE_UNKNOWN:
 	case VPN_PROVIDER_STATE_IDLE:
@@ -1626,7 +1654,7 @@ int __vpn_provider_connect(struct vpn_provider *provider, DBusMessage *msg)
 		return -EOPNOTSUPP;
 
 	if (err == -EINPROGRESS)
-		vpn_provider_set_state(provider, VPN_PROVIDER_STATE_CONNECT);
+		vpn_provider_set_state(provider, VPN_PROVIDER_STATE_ASSOCIATION);
 
 	return err;
 }
@@ -1767,6 +1795,8 @@ static const char *state2string(enum vpn_provider_state state)
 		break;
 	case VPN_PROVIDER_STATE_IDLE:
 		return "idle";
+	case VPN_PROVIDER_STATE_ASSOCIATION:
+		return "association";
 	case VPN_PROVIDER_STATE_CONNECT:
 		return "configuration";
 	case VPN_PROVIDER_STATE_READY:
@@ -1875,6 +1905,9 @@ static void append_state(DBusMessageIter *iter,
 	case VPN_PROVIDER_STATE_IDLE:
 		str = "idle";
 		break;
+	case VPN_PROVIDER_STATE_ASSOCIATION:
+		str = "association";
+		break;
 	case VPN_PROVIDER_STATE_CONNECT:
 		str = "configuration";
 		break;
@@ -2026,6 +2059,10 @@ int vpn_provider_set_state(struct vpn_provider *provider,
 	case VPN_PROVIDER_STATE_IDLE:
 		return set_connected(provider, false);
 	case VPN_PROVIDER_STATE_CONNECT:
+		if (provider->driver && provider->driver->set_state)
+			provider->driver->set_state(provider, state);
+		return provider_indicate_state(provider, state);
+	case VPN_PROVIDER_STATE_ASSOCIATION:
 		return provider_indicate_state(provider, state);
 	case VPN_PROVIDER_STATE_READY:
 		return set_connected(provider, true);
diff --git a/vpn/vpn-provider.h b/vpn/vpn-provider.h
index 5d1455da..c81476c6 100644
--- a/vpn/vpn-provider.h
+++ b/vpn/vpn-provider.h
@@ -44,6 +44,12 @@ enum vpn_provider_state {
 	VPN_PROVIDER_STATE_READY         = 3,
 	VPN_PROVIDER_STATE_DISCONNECT    = 4,
 	VPN_PROVIDER_STATE_FAILURE       = 5,
+	/*
+	 * Special state to indicate that user interaction is being waited for
+	 * and disconnect timeout in connmand should not terminate this VPN but
+	 * to let the agent timeout handle the case.
+	 */
+	VPN_PROVIDER_STATE_ASSOCIATION   = 6,
 };
 
 enum vpn_provider_error {
-- 
2.39.5

[PATCH 03/11] vpn: Add association state before connect state

[Jussi Laakkonen]

This changes the state machine by adding the VPN_STATE_ASSOCIATION to be
entered right after connect() callback is called. This is needed in
order to properly react with the user input dialog waiting on VPNs.
connect state is now set when the dialog is closed to indicate that user
input is given and now the VPN really connects.

When VPN notify() allback is called the connect state is enforced if the
return value indicates so and the internal state is different. This is
to accommodate the changes required and to operate as a fallback that
the states of provider and driver are kept in sync.

Warn about invalid transition to ASSOCIATION state in case vpn_notify()
gets it as a reply back from plugin notify.
---
 vpn/plugins/vpn.c | 22 +++++++++++++++++++++-
 vpn/plugins/vpn.h | 11 ++++++-----
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/vpn/plugins/vpn.c b/vpn/plugins/vpn.c
index cb0d304b..5cc4c757 100644
--- a/vpn/plugins/vpn.c
+++ b/vpn/plugins/vpn.c
@@ -219,6 +219,9 @@ static int vpn_set_state(struct vpn_provider *provider,
 	case VPN_PROVIDER_STATE_IDLE:
 		data->state = VPN_STATE_IDLE;
 		break;
+	case VPN_PROVIDER_STATE_ASSOCIATION:
+		data->state = VPN_STATE_ASSOCIATION;
+		break;
 	case VPN_PROVIDER_STATE_CONNECT:
 	case VPN_PROVIDER_STATE_READY:
 		data->state = VPN_STATE_CONNECT;
@@ -281,6 +284,12 @@ static DBusMessage *vpn_notify(struct connman_task *task,
 
 	switch (state) {
 	case VPN_STATE_CONNECT:
+		if (data->state == VPN_STATE_ASSOCIATION) {
+			data->state = VPN_STATE_CONNECT;
+			vpn_provider_set_state(provider,
+						VPN_PROVIDER_STATE_CONNECT);
+		}
+		/* fall through */
 	case VPN_STATE_READY:
 		if (data->state == VPN_STATE_READY) {
 			/*
@@ -333,6 +342,16 @@ static DBusMessage *vpn_notify(struct connman_task *task,
 		break;
 
 	case VPN_STATE_UNKNOWN:
+		break;
+
+	/* State transition to ASSOCIATION via notify is not allowed */
+	case VPN_STATE_ASSOCIATION:
+		connman_warn("Invalid %s vpn_notify() state transition "
+					"from %d to %d (ASSOCIATION)."
+					"VPN provider %p is disconnected",
+					vpn_driver_data->name, data->state,
+					state, provider);
+		/* fall through */
 	case VPN_STATE_IDLE:
 	case VPN_STATE_DISCONNECT:
 	case VPN_STATE_FAILURE:
@@ -565,6 +584,7 @@ static int vpn_connect(struct vpn_provider *provider,
 		data->state = VPN_STATE_IDLE;
 		break;
 
+	case VPN_STATE_ASSOCIATION:
 	case VPN_STATE_CONNECT:
 		return -EINPROGRESS;
 
@@ -645,7 +665,7 @@ static int vpn_connect(struct vpn_provider *provider,
 	DBG("%s started with dev %s",
 		vpn_driver_data->provider_driver.name, data->if_name);
 
-	data->state = VPN_STATE_CONNECT;
+	data->state = VPN_STATE_ASSOCIATION;
 
 	return -EINPROGRESS;
 
diff --git a/vpn/plugins/vpn.h b/vpn/plugins/vpn.h
index fd10addf..a8d24fc3 100644
--- a/vpn/plugins/vpn.h
+++ b/vpn/plugins/vpn.h
@@ -34,11 +34,12 @@ extern "C" {
 enum vpn_state {
 	VPN_STATE_UNKNOWN       = 0,
 	VPN_STATE_IDLE          = 1,
-	VPN_STATE_CONNECT       = 2,
-	VPN_STATE_READY         = 3,
-	VPN_STATE_DISCONNECT    = 4,
-	VPN_STATE_FAILURE       = 5,
-	VPN_STATE_AUTH_FAILURE  = 6,
+	VPN_STATE_ASSOCIATION   = 2,
+	VPN_STATE_CONNECT       = 3,
+	VPN_STATE_READY         = 4,
+	VPN_STATE_DISCONNECT    = 5,
+	VPN_STATE_FAILURE       = 6,
+	VPN_STATE_AUTH_FAILURE  = 7,
 };
 
 struct vpn_driver {
-- 
2.39.5

[PATCH 04/11] vpn-agent: Do connect state transition after input dialog check

[Jussi Laakkonen]

When the VPN requests input (credentials) via the VPN agent the
vpn_agent_check_and_process_reply_error() does transition the state of
the VPN provider to connect state when there is no error. This is done
to facilitate the transition from the association state to connect
state as each VPN should use this function to verify the D-Bus reply
and, thus will be called after each reply.
---
 vpn/vpn-agent.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/vpn/vpn-agent.c b/vpn/vpn-agent.c
index ab6fea55..f1cc7e36 100644
--- a/vpn/vpn-agent.c
+++ b/vpn/vpn-agent.c
@@ -257,8 +257,12 @@ int vpn_agent_check_and_process_reply_error(DBusMessage *reply,
 
 	dbus_error_init(&error);
 
-	if (!dbus_set_error_from_message(&error, reply))
+	if (!dbus_set_error_from_message(&error, reply)) {
+		DBG("Dialog without error, set provider %p to CONNECT",
+					provider);
+		vpn_provider_set_state(provider, VPN_PROVIDER_STATE_CONNECT);
 		return 0;
+	}
 
 	if (!g_strcmp0(error.name, VPN_AGENT_INTERFACE ".Error.Canceled"))
 		err = ECANCELED;
-- 
2.39.5

[PATCH 05/11] service: Explicit VPN connect timeout, ignore in VPN agent wait

[Jussi Laakkonen]

Ignore the connect timeout autostarting when connecting a VPN service
because initially the VPN is in association state in which the VPN is
waiting for the VPN agent. Separate the starting of connect timeout into
its own function __connman_service_start_connect_timeout() so provider.c
can call it when it enters configuration state.

When a VPN is waiting for user input it should not be affected by
connect timeout as the connection is not yet attempted. This may happen
if VPN resumes to association state when requiring the VPN agent for
other, e.g., encrypted private key input after credential input.
---
 src/connman.h |  2 ++
 src/service.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 32ba5591..a92e19d7 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -787,6 +787,8 @@ int __connman_service_connect(struct connman_service *service,
 int __connman_service_disconnect(struct connman_service *service);
 void __connman_service_set_active_session(bool enable, GSList *list);
 void __connman_service_auto_connect(enum connman_service_connect_reason reason);
+void __connman_service_start_connect_timeout(struct connman_service *service,
+				bool restart);
 bool __connman_service_remove(struct connman_service *service);
 void __connman_service_set_hidden_data(struct connman_service *service,
 				gpointer user_data);
diff --git a/src/service.c b/src/service.c
index 805cfca7..9dfb4ead 100644
--- a/src/service.c
+++ b/src/service.c
@@ -7098,8 +7098,27 @@ static gboolean connect_timeout(gpointer user_data)
 
 	if (service->network)
 		__connman_network_disconnect(service->network);
-	else if (service->provider)
+	else if (service->provider) {
+		/*
+		 * Remove timeout when the VPN is waiting for user input in
+		 * association state. By default the VPN agent timeout is
+		 * 300s whereas default connection timeout is 120s. Provider
+		 * will start connect timeout for the service when it enters
+		 * configuration state.
+		 */
+		const char *statestr = connman_provider_get_string(
+					service->provider, "State");
+		if (!g_strcmp0(statestr, "association")) {
+			DBG("VPN provider %p is waiting for VPN agent, "
+						"stop connect timeout",
+						service->provider);
+			return G_SOURCE_REMOVE;
+		}
+
 		connman_provider_disconnect(service->provider);
+	}
+
+
 
 	__connman_stats_service_unregister(service);
 
@@ -7127,7 +7146,27 @@ static gboolean connect_timeout(gpointer user_data)
 				CONNMAN_SERVICE_CONNECT_REASON_USER)
 		do_auto_connect(service, CONNMAN_SERVICE_CONNECT_REASON_AUTO);
 
-	return FALSE;
+	return G_SOURCE_REMOVE;
+}
+
+void __connman_service_start_connect_timeout(struct connman_service *service,
+								bool restart)
+{
+	DBG("");
+
+	if (!service)
+		return;
+
+	if (!restart && service->timeout)
+		return;
+
+	if (restart && service->timeout) {
+		DBG("cancel running connect timeout");
+		g_source_remove(service->timeout);
+	}
+
+	service->timeout = g_timeout_add_seconds(CONNECT_TIMEOUT,
+				connect_timeout, service);
 }
 
 static DBusMessage *connect_service(DBusConnection *conn,
@@ -9751,9 +9790,12 @@ int __connman_service_connect(struct connman_service *service,
 		return 0;
 
 	if (err == -EINPROGRESS) {
-		if (service->timeout == 0)
-			service->timeout = g_timeout_add_seconds(
-				CONNECT_TIMEOUT, connect_timeout, service);
+		/*
+		 * VPN will start connect timeout when it enters CONFIGURATION
+		 * state.
+		 */
+		if (service->type != CONNMAN_SERVICE_TYPE_VPN)
+			__connman_service_start_connect_timeout(service, false);
 
 		return -EINPROGRESS;
 	}
-- 
2.39.5

[PATCH 06/11] provider: Handle VPN configuration and association states

[Jussi Laakkonen]

Set the association state when VPN is waiting for user input as an
initial state after connecting the provider. Set the configuration
state (as it is declaced to be the string to connect state in VPN)
accordingly as well. Start VPN connect timeout in configuration
state with restart option to ensure that the timeout begins from the
last known configuration (connect) state.
---
 include/provider.h |  9 +++++----
 src/provider.c     | 22 +++++++++++++++++++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/provider.h b/include/provider.h
index 3f2e36ad..aac47527 100644
--- a/include/provider.h
+++ b/include/provider.h
@@ -44,10 +44,11 @@ enum connman_provider_type {
 enum connman_provider_state {
 	CONNMAN_PROVIDER_STATE_UNKNOWN       = 0,
 	CONNMAN_PROVIDER_STATE_IDLE          = 1,
-	CONNMAN_PROVIDER_STATE_CONNECT       = 2,
-	CONNMAN_PROVIDER_STATE_READY         = 3,
-	CONNMAN_PROVIDER_STATE_DISCONNECT    = 4,
-	CONNMAN_PROVIDER_STATE_FAILURE       = 5,
+	CONNMAN_PROVIDER_STATE_ASSOCIATION   = 2,
+	CONNMAN_PROVIDER_STATE_CONNECT       = 3,
+	CONNMAN_PROVIDER_STATE_READY         = 4,
+	CONNMAN_PROVIDER_STATE_DISCONNECT    = 5,
+	CONNMAN_PROVIDER_STATE_FAILURE       = 6,
 };
 
 enum connman_provider_error {
diff --git a/src/provider.c b/src/provider.c
index 1f0ce10d..ab4aeafb 100644
--- a/src/provider.c
+++ b/src/provider.c
@@ -126,6 +126,22 @@ static int provider_indicate_state(struct connman_provider *provider,
 {
 	DBG("state %d", state);
 
+	switch (state) {
+	case CONNMAN_SERVICE_STATE_UNKNOWN:
+	case CONNMAN_SERVICE_STATE_IDLE:
+	case CONNMAN_SERVICE_STATE_ASSOCIATION:
+		break;
+	case CONNMAN_SERVICE_STATE_CONFIGURATION:
+		__connman_service_start_connect_timeout(provider->vpn_service,
+								true);
+		break;
+	case CONNMAN_SERVICE_STATE_READY:
+	case CONNMAN_SERVICE_STATE_ONLINE:
+	case CONNMAN_SERVICE_STATE_DISCONNECT:
+	case CONNMAN_SERVICE_STATE_FAILURE:
+		break;
+	}
+
 	__connman_service_ipconfig_indicate_state(provider->vpn_service, state,
 					CONNMAN_IPCONFIG_TYPE_IPV4);
 
@@ -291,9 +307,13 @@ int connman_provider_set_state(struct connman_provider *provider,
 		return -EINVAL;
 	case CONNMAN_PROVIDER_STATE_IDLE:
 		return set_connected(provider, false);
-	case CONNMAN_PROVIDER_STATE_CONNECT:
+	case CONNMAN_PROVIDER_STATE_ASSOCIATION:
+		/* Connect timeout is not effective for VPNs in this state */
 		return provider_indicate_state(provider,
 					CONNMAN_SERVICE_STATE_ASSOCIATION);
+	case CONNMAN_PROVIDER_STATE_CONNECT:
+		return provider_indicate_state(provider,
+					CONNMAN_SERVICE_STATE_CONFIGURATION);
 	case CONNMAN_PROVIDER_STATE_READY:
 		return set_connected(provider, true);
 	case CONNMAN_PROVIDER_STATE_DISCONNECT:
-- 
2.39.5

[PATCH 07/11] vpn: Add support for association state, add state getter

[Jussi Laakkonen]

Support VPN wait user input state as the association state.

Add support for "State" string into the get_property() driver callback.
---
 plugins/vpn.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/plugins/vpn.c b/plugins/vpn.c
index 42396d2a..d9a56ae1 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -156,6 +156,8 @@ static const char *get_string(struct connman_provider *provider,
 		return data->domain;
 	else if (g_str_equal(key, "Transport"))
 		return data->service_ident;
+	else if (g_str_equal(key, "State"))
+		return data->state;
 
 	return g_hash_table_lookup(data->setting_strings, key);
 }
@@ -283,6 +285,8 @@ static void set_provider_state(struct connection_data *data)
 		goto set;
 	} else if (g_str_equal(data->state, "configuration")) {
 		state = CONNMAN_PROVIDER_STATE_CONNECT;
+	} else if (g_str_equal(data->state, "association")) {
+		state = CONNMAN_PROVIDER_STATE_ASSOCIATION;
 	} else if (g_str_equal(data->state, "idle")) {
 		state = CONNMAN_PROVIDER_STATE_IDLE;
 	} else if (g_str_equal(data->state, "disconnect")) {
-- 
2.39.5

[PATCH 08/11] vpn: Check if connecting when setting state or disconnecting

[Jussi Laakkonen]

Add checking of connected and connecting state in cases when the state
is being set and state transitions to disconnecting. This change avoids
clearing the transport ident when VPN is waiting for input from VPN
agent (association state).
---
 plugins/vpn.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/plugins/vpn.c b/plugins/vpn.c
index d9a56ae1..bec7f59f 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -270,6 +270,13 @@ static bool provider_is_connected(struct connection_data *data)
 			g_str_equal(data->state, "configuration"));
 }
 
+static bool provider_is_connected_or_connecting(struct connection_data *data)
+{
+	return data && (g_str_equal(data->state, "ready") ||
+			g_str_equal(data->state, "configuration") ||
+			g_str_equal(data->state, "association"));
+}
+
 static void set_provider_state(struct connection_data *data)
 {
 	enum connman_provider_state state = CONNMAN_PROVIDER_STATE_UNKNOWN;
@@ -278,7 +285,11 @@ static void set_provider_state(struct connection_data *data)
 
 	DBG("provider %p new state %s", data->provider, data->state);
 
-	connected = provider_is_connected(data);
+	/*
+	 * To avoid clearing transport ident when VPN is waiting for agent
+	 * take also connecting state into account.
+	 */
+	connected = provider_is_connected_or_connecting(data);
 
 	if (g_str_equal(data->state, "ready")) {
 		state = CONNMAN_PROVIDER_STATE_READY;
@@ -1076,7 +1087,7 @@ static int provider_disconnect(struct connman_provider *provider)
 	if (!data)
 		return -EINVAL;
 
-	if (provider_is_connected(data))
+	if (provider_is_connected_or_connecting(data))
 		err = disconnect_provider(data);
 
 	if (data->call) {
@@ -1730,7 +1741,7 @@ static void destroy_provider(struct connection_data *data)
 {
 	DBG("data %p", data);
 
-	if (provider_is_connected(data))
+	if (provider_is_connected_or_connecting(data))
 		connman_provider_disconnect(data->provider);
 
 	connman_provider_set_data(data->provider, NULL);
@@ -2183,7 +2194,7 @@ static bool vpn_is_valid_transport(struct connman_service *transport)
 
 static void vpn_disconnect_check_provider(struct connection_data *data)
 {
-	if (provider_is_connected(data)) {
+	if (provider_is_connected_or_connecting(data)) {
 		/* With NULL service ident NULL is returned immediately */
 		struct connman_service *service =
 			connman_service_lookup_from_identifier
-- 
2.39.5

[PATCH 09/11] vpn: Add VPN agent use callback for plugins

[Jussi Laakkonen]

[vpn] Add VPN agent use callback for plugins. JB#59447

Add callback that can be used by the VPN plugins to tell the vpn_driver
whether it uses VPN agent or not. Default to using VPN agent if the
function is not defined.

This is done to accommodate the state transition in vpn-provider when
the VPN does not utilize VPN agent.
---
 vpn/plugins/vpn.c  | 22 ++++++++++++++++++++++
 vpn/plugins/vpn.h  |  1 +
 vpn/vpn-provider.h |  1 +
 3 files changed, 24 insertions(+)

diff --git a/vpn/plugins/vpn.c b/vpn/plugins/vpn.c
index 5cc4c757..b55b1222 100644
--- a/vpn/plugins/vpn.c
+++ b/vpn/plugins/vpn.c
@@ -797,6 +797,27 @@ static int vpn_route_env_parse(struct vpn_provider *provider, const char *key,
 	return 0;
 }
 
+static bool vpn_uses_vpn_agent(struct vpn_provider *provider)
+{
+	struct vpn_driver_data *vpn_driver_data = NULL;
+	const char *name = NULL;
+
+	if (!provider)
+		return false;
+
+	name = vpn_provider_get_driver_name(provider);
+	vpn_driver_data = g_hash_table_lookup(driver_hash, name);
+
+	if (vpn_driver_data && vpn_driver_data->vpn_driver->uses_vpn_agent)
+		return vpn_driver_data->vpn_driver->uses_vpn_agent(provider);
+
+	/*
+	 * Default to using the VPN agent, in cases where the function is not
+	 * implemented. The use of VPN agent must be explicitly dropped.
+	 */
+	return true;
+}
+
 int vpn_register(const char *name, const struct vpn_driver *vpn_driver,
 			const char *program)
 {
@@ -822,6 +843,7 @@ int vpn_register(const char *name, const struct vpn_driver *vpn_driver,
 	data->provider_driver.save = vpn_save;
 	data->provider_driver.set_state = vpn_set_state;
 	data->provider_driver.route_env_parse = vpn_route_env_parse;
+	data->provider_driver.uses_vpn_agent = vpn_uses_vpn_agent;
 
 	if (!driver_hash)
 		driver_hash = g_hash_table_new_full(g_str_hash,
diff --git a/vpn/plugins/vpn.h b/vpn/plugins/vpn.h
index a8d24fc3..b24cbf9b 100644
--- a/vpn/plugins/vpn.h
+++ b/vpn/plugins/vpn.h
@@ -57,6 +57,7 @@ struct vpn_driver {
 	int (*route_env_parse) (struct vpn_provider *provider, const char *key,
 			int *family, unsigned long *idx,
 			enum vpn_provider_route_type *type);
+	bool (*uses_vpn_agent) (struct vpn_provider *provider);
 };
 
 int vpn_register(const char *name, const struct vpn_driver *driver,
diff --git a/vpn/vpn-provider.h b/vpn/vpn-provider.h
index c81476c6..8a8b6bfd 100644
--- a/vpn/vpn-provider.h
+++ b/vpn/vpn-provider.h
@@ -167,6 +167,7 @@ struct vpn_provider_driver {
 	int (*route_env_parse) (struct vpn_provider *provider, const char *key,
 			int *family, unsigned long *idx,
 			enum vpn_provider_route_type *type);
+	bool (*uses_vpn_agent) (struct vpn_provider *provider);
 };
 
 int vpn_provider_driver_register(struct vpn_provider_driver *driver);
-- 
2.39.5

[PATCH 10/11] vpn-provider: Transition to CONNECT state with agentless VPNs

[Jussi Laakkonen]

[vpn-provider] Transition to CONNECT state with agentless VPNs

Set to transition to CONNECT state immediately after ASSOCIATION state
after initializing connection procedure with a VPN if it does not use
VPN agent. This is done to accommodate the full state machine
transitions as the VPN agent, when success, will do the transition but
when VPN agent is not used the transition would be required to be done
by the plugin.
---
 vpn/vpn-provider.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 56040e65..b21e9e61 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -1653,8 +1653,19 @@ int __vpn_provider_connect(struct vpn_provider *provider, DBusMessage *msg)
 	} else
 		return -EOPNOTSUPP;
 
-	if (err == -EINPROGRESS)
-		vpn_provider_set_state(provider, VPN_PROVIDER_STATE_ASSOCIATION);
+	if (err == -EINPROGRESS) {
+		vpn_provider_set_state(provider,
+					VPN_PROVIDER_STATE_ASSOCIATION);
+
+		/*
+		 * If the VPN does not use VPN agent do direct transition to
+		 * connect in order to support the complete state machine.
+		 */
+		if (provider->driver && provider->driver->uses_vpn_agent &&
+				!provider->driver->uses_vpn_agent(provider))
+			vpn_provider_set_state(provider,
+						VPN_PROVIDER_STATE_CONNECT);
+	}
 
 	return err;
 }
-- 
2.39.5

[PATCH 11/11] doc: Update VPN documentation for association state

[Jussi Laakkonen]

Add brief descriptions of the association state. Add it to parameter
descriptions as well.
---
 doc/vpn-connection-api.txt | 4 ++--
 doc/vpn-overview.txt       | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/doc/vpn-connection-api.txt b/doc/vpn-connection-api.txt
index 2d3e0078..df070957 100644
--- a/doc/vpn-connection-api.txt
+++ b/doc/vpn-connection-api.txt
@@ -104,8 +104,8 @@ Properties	string State [readonly]
 
 			The connection state information.
 
-			Valid states are "idle", "failure", "configuration",
-			"ready", "disconnect".
+			Valid states are "idle", "failure", "association",
+			"configuration", "ready", "disconnect".
 
 		string Type [readonly]
 
diff --git a/doc/vpn-overview.txt b/doc/vpn-overview.txt
index d2d14a0c..74f5695e 100644
--- a/doc/vpn-overview.txt
+++ b/doc/vpn-overview.txt
@@ -66,7 +66,12 @@ VPN agent interface described in vpn-agent-api.txt is used for
 interaction between the connectivity UI and ConnMan. A VPN agent
 registered via Management interface gets requests from the VPN plugins
 to input credentials or other authentication information for the VPN
-connection and offers information about the VPN to be connected.
+connection and offers information about the VPN to be connected. When
+waiting for input via VPN agent the state of the VPN is "association"
+and after getting the input the state transitions to "connect". If the
+VPN does not wish to use VPN agent this can be explicitly defined by
+implementing "uses_vpn_agent()" returning "false" indicating that the
+state is transitioned to "connect" when connecting the VPN.
 
 In addition to basic credentials, there are additional types of optional
 and control parameters. The user can dictate whether to store the
-- 
2.39.5

Re: [PATCH 00/11] Add association state for VPNs

[Christian Hewitt]

> On 24 Jan 2025, at 10:58 pm, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote:
> 
> This patch set adds the association state also for the VPNs. This state is to
> indicate that the VPN is waiting for VPN agent to provide input given by user.
> In this state service.c must not do connect timeout checks as the timers for
> both differ in length, default being 120s for connect timeout and 300s for VPN
> agent dialog timeout.
> 
> In order to facilitate this change the association state had to be implemented
> also for VPNs. It is common state for services and like with services the
> association state for VPNs preceeds the configuration state (on VPN side
> connect state). Both vpn.c plugins on connmand and vpnd side require changes
> to accommodate this state. When the VPN agent succeeds in getting the input
> from the user the state transitions from association to connect (configuration)
> state and, thus, requires no specific changes to VPN plugins.
> 
> On connmand side the association state is the initial state when VPN is getting
> connected and the state needs to be accounted as a connecting state in
> plugins/vpn.c to not to lose transport ident for it and in provider.c as a
> pre-configuration state to not to start the connect timeout for the VPN before
> the VPN is in configuration state. The reason for the latter is that the
> connect timeout should be exact and start from the point when
> connect/configuration state is entered.
> 
> On vpnd side association state is, like on connmand side, the initial state for
> the VPN getting connected. After the VPN agent succeeds getting the information
> from the user (credentials) the state transitions to connect (configuratioin).
> There may be a possibility for a VPN plugin to run without VPN agent and thus
> in these cases it is ensured that the vpn/plugins/vpn.c:vpn_notify() does
> the state transition in such cases. It is allowed go back to association state
> from connect state but not from other states.
> 
> Jussi Laakkonen (11):
>  agent: Cancel agent request on NoReply D-Bus error
>  vpn-provider: Use association state for VPN agent input wait
>  vpn: Add association state before connect state
>  vpn-agent: Do connect state transition after input dialog check
>  service: Explicit VPN connect timeout, ignore in VPN agent wait
>  provider: Handle VPN configuration and association states
>  vpn: Add support for association state, add state getter
>  vpn: Check if connecting when setting state or disconnecting
>  vpn: Add VPN agent use callback for plugins
>  vpn-provider: Transition to CONNECT state with agentless VPNs
>  doc: Update VPN documentation for association state
> 
> doc/vpn-connection-api.txt |  4 +--
> doc/vpn-overview.txt       |  7 ++++-
> include/provider.h         |  9 +++---
> plugins/vpn.c              | 23 ++++++++++++---
> src/agent.c                |  4 ++-
> src/connman.h              |  2 ++
> src/provider.c             | 22 ++++++++++++++-
> src/service.c              | 52 ++++++++++++++++++++++++++++++----
> vpn/plugins/vpn.c          | 44 ++++++++++++++++++++++++++++-
> vpn/plugins/vpn.h          | 12 ++++----
> vpn/vpn-agent.c            |  6 +++-
> vpn/vpn-provider.c         | 58 ++++++++++++++++++++++++++++++++++----
> vpn/vpn-provider.h         |  7 +++++
> 13 files changed, 220 insertions(+), 30 deletions(-)

I’ve been using this series combined with the following series and patch for several weeks:

https://patchwork.kernel.org/project/connman/list/?series=928220
https://patchwork.kernel.org/project/connman/patch/DB6PR10MB1845EBB07DB785B580B3F6ECE1E02@DB6PR10MB1845.EURPRD10.PROD.OUTLOOK.COM/

Plus a revert of this commit which I’ve previously flagged as breaking the default “route all traffic down tunnel” with WireGuard (the main use-case for LibreELEC users):

https://git.kernel.org/pub/scm/network/connman/connman.git/commit/?id=9eb1772d31b6fcf78e8711976696491aec9ff5df

For easier testing all patches are in this branch: https://github.com/chewitt/connman/commits/wireguard

No issues observed, so:

Tested-by: Christian Hewitt <christianshewitt@gmail.com>

Re: [PATCH 00/11] Add association state for VPNs

[Jussi Laakkonen]

Hi all!

On 2/14/25 08:30, Christian Hewitt wrote:
>> On 24 Jan 2025, at 10:58 pm, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote:
>>
>> This patch set adds the association state also for the VPNs. This state is to
>> indicate that the VPN is waiting for VPN agent to provide input given by user.
>> In this state service.c must not do connect timeout checks as the timers for
>> both differ in length, default being 120s for connect timeout and 300s for VPN
>> agent dialog timeout.
>>
>> In order to facilitate this change the association state had to be implemented
>> also for VPNs. It is common state for services and like with services the
>> association state for VPNs preceeds the configuration state (on VPN side
>> connect state). Both vpn.c plugins on connmand and vpnd side require changes
>> to accommodate this state. When the VPN agent succeeds in getting the input
>> from the user the state transitions from association to connect (configuration)
>> state and, thus, requires no specific changes to VPN plugins.
>>
>> On connmand side the association state is the initial state when VPN is getting
>> connected and the state needs to be accounted as a connecting state in
>> plugins/vpn.c to not to lose transport ident for it and in provider.c as a
>> pre-configuration state to not to start the connect timeout for the VPN before
>> the VPN is in configuration state. The reason for the latter is that the
>> connect timeout should be exact and start from the point when
>> connect/configuration state is entered.
>>
>> On vpnd side association state is, like on connmand side, the initial state for
>> the VPN getting connected. After the VPN agent succeeds getting the information
>> from the user (credentials) the state transitions to connect (configuratioin).
>> There may be a possibility for a VPN plugin to run without VPN agent and thus
>> in these cases it is ensured that the vpn/plugins/vpn.c:vpn_notify() does
>> the state transition in such cases. It is allowed go back to association state
>> from connect state but not from other states.
>>
>> Jussi Laakkonen (11):
>>   agent: Cancel agent request on NoReply D-Bus error
>>   vpn-provider: Use association state for VPN agent input wait
>>   vpn: Add association state before connect state
>>   vpn-agent: Do connect state transition after input dialog check
>>   service: Explicit VPN connect timeout, ignore in VPN agent wait
>>   provider: Handle VPN configuration and association states
>>   vpn: Add support for association state, add state getter
>>   vpn: Check if connecting when setting state or disconnecting
>>   vpn: Add VPN agent use callback for plugins
>>   vpn-provider: Transition to CONNECT state with agentless VPNs
>>   doc: Update VPN documentation for association state
>>
>> doc/vpn-connection-api.txt |  4 +--
>> doc/vpn-overview.txt       |  7 ++++-
>> include/provider.h         |  9 +++---
>> plugins/vpn.c              | 23 ++++++++++++---
>> src/agent.c                |  4 ++-
>> src/connman.h              |  2 ++
>> src/provider.c             | 22 ++++++++++++++-
>> src/service.c              | 52 ++++++++++++++++++++++++++++++----
>> vpn/plugins/vpn.c          | 44 ++++++++++++++++++++++++++++-
>> vpn/plugins/vpn.h          | 12 ++++----
>> vpn/vpn-agent.c            |  6 +++-
>> vpn/vpn-provider.c         | 58 ++++++++++++++++++++++++++++++++++----
>> vpn/vpn-provider.h         |  7 +++++
>> 13 files changed, 220 insertions(+), 30 deletions(-)
> 
> I’ve been using this series combined with the following series and patch for several weeks:
> 
> https://patchwork.kernel.org/project/connman/list/?series=928220
> https://patchwork.kernel.org/project/connman/patch/DB6PR10MB1845EBB07DB785B580B3F6ECE1E02@DB6PR10MB1845.EURPRD10.PROD.OUTLOOK.COM/
> 
> Plus a revert of this commit which I’ve previously flagged as breaking the default “route all traffic down tunnel” with WireGuard (the main use-case for LibreELEC users):
> 
> https://git.kernel.org/pub/scm/network/connman/connman.git/commit/?id=9eb1772d31b6fcf78e8711976696491aec9ff5df
> 
> For easier testing all patches are in this branch: https://github.com/chewitt/connman/commits/wireguard
> 
> No issues observed, so:
> 
> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
> 

Denis: The testing of WireGuard changes was done with these changes as 
well, should I resend these patches? I'll try to find time 
today/tomorrow to check if they still apply, and if not, I'll send v2.

- Jussi

Re: [PATCH 00/11] Add association state for VPNs

[Denis Kenzior]

Hi Jussi,

 >
> Denis: The testing of WireGuard changes was done with these changes as well, 
> should I resend these patches? I'll try to find time today/tomorrow to check if 
> they still apply, and if not, I'll send v2.

If you've added/changed anything since the original submission, then go ahead 
and resend.

Regards,
-Denis

Re: [PATCH 00/11] Add association state for VPNs

[Jussi Laakkonen]

Hi Denis,

On 4/15/25 19:04, Denis Kenzior wrote:
> Hi Jussi,
> 
>  >
>> Denis: The testing of WireGuard changes was done with these changes as 
>> well, should I resend these patches? I'll try to find time today/ 
>> tomorrow to check if they still apply, and if not, I'll send v2.
> 
> If you've added/changed anything since the original submission, then go 
> ahead and resend.
> 

I just re-checked these, they did not have any changes from our side and 
they seemed to apply on top of latest master. So this set is good to go 
for review.


Cheers,
  Jussi

Re: [PATCH 00/11] Add association state for VPNs

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Fri, 24 Jan 2025 20:58:34 +0200 you wrote:
> This patch set adds the association state also for the VPNs. This state is to
> indicate that the VPN is waiting for VPN agent to provide input given by user.
> In this state service.c must not do connect timeout checks as the timers for
> both differ in length, default being 120s for connect timeout and 300s for VPN
> agent dialog timeout.
> 
> In order to facilitate this change the association state had to be implemented
> also for VPNs. It is common state for services and like with services the
> association state for VPNs preceeds the configuration state (on VPN side
> connect state). Both vpn.c plugins on connmand and vpnd side require changes
> to accommodate this state. When the VPN agent succeeds in getting the input
> from the user the state transitions from association to connect (configuration)
> state and, thus, requires no specific changes to VPN plugins.
> 
> [...]

Here is the summary with links:
  - [01/11] agent: Cancel agent request on NoReply D-Bus error
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=01c10f2ce960
  - [02/11] vpn-provider: Use association state for VPN agent input wait
    (no matching commit)
  - [03/11] vpn: Add association state before connect state
    (no matching commit)
  - [04/11] vpn-agent: Do connect state transition after input dialog check
    (no matching commit)
  - [05/11] service: Explicit VPN connect timeout, ignore in VPN agent wait
    (no matching commit)
  - [06/11] provider: Handle VPN configuration and association states
    (no matching commit)
  - [07/11] vpn: Add support for association state, add state getter
    (no matching commit)
  - [08/11] vpn: Check if connecting when setting state or disconnecting
    (no matching commit)
  - [09/11] vpn: Add VPN agent use callback for plugins
    (no matching commit)
  - [10/11] vpn-provider: Transition to CONNECT state with agentless VPNs
    (no matching commit)
  - [11/11] doc: Update VPN documentation for association state
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html