[RFC PATCH 0/9] Add association state for VPNs

[RFC PATCH 0/9] 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.


I'd like to raise the following issues for commenting:
a) Is this current approach sound in the way that the vpn-agent.c is
   responsible of changing the state? vpn-provider.c:connect_cb() would be
   ideal but that does get called in notify if there is no error. Processing
   the message with the vpn_agent_check_and_process_reply_error() with success
   indicates that VPN agent succeeded getting the input.

b) Can we figure out a case where the state transition from VPN plugin notify
   SHOULD change the state back to ASSOCIATION when normally in CONNECT state?
   This change is in vpn/plugins/vpn.c that accommodates transitions from
   CONNECT->ASSOCIATION.

c) Furthermore, might any of the VPN plugins available (elsewhere) work
   without accessing the VPN agent? The flow now requires to use vpn-agent.c
   for processing and state transition. However, these cases should then use
   the vpn_provider_set_state() directly, as most of the plugins now do in,
   e.g., error cases.

d) The VPN and PROVIDER states, should these be harmonized to follow the same
   structure as the common service states to avoid confusion or might this be
   risky as many of the headers are public?

e) Should the specific VPNs that do need VPN agent more than once then do the
   transition back to ASSOCIATION when the first VPN agent returns and sets the
   state to CONNECT? This is what I couldn't decide, there is another option to
   have the vpn_agent_check_and_process_reply_error() to have an additional
   parameter to state that do not transition state, but in the case of OpenVPN
   the private key, for instance, can be decrypted and the management interface
   queries the passphrase on need basis. Thus, there is no apriori information
   on this.

And of course, any other issue that may trouble your mind with these changes.

Jussi Laakkonen (9):
  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
  doc: Update VPN documentation for association state

 doc/vpn-connection-api.txt |  4 +--
 doc/vpn-overview.txt       |  4 ++-
 include/provider.h         |  1 +
 plugins/vpn.c              | 23 ++++++++++++++---
 src/agent.c                |  4 ++-
 src/connman.h              |  2 ++
 src/provider.c             | 22 +++++++++++++++-
 src/service.c              | 52 ++++++++++++++++++++++++++++++++++----
 vpn/plugins/vpn.c          | 29 ++++++++++++++++++++-
 vpn/plugins/vpn.h          |  1 +
 vpn/vpn-agent.c            |  6 ++++-
 vpn/vpn-provider.c         | 45 ++++++++++++++++++++++++++++++---
 vpn/vpn-provider.h         |  6 +++++
 13 files changed, 179 insertions(+), 20 deletions(-)

-- 
2.30.2

[RFC PATCH 1/9] 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.30.2

[RFC PATCH 2/9] 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 cc325967..30b9fa25 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.30.2

[RFC PATCH 3/9] 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.

Handle also association state in vpn_notify() where the state reported by
plugin can only be transitioned back to association from connect state.
The initial state for the plugins is association so in that case there
is no state change done. If this state is notified back in any other
state it is logged as a warning with the plugin that caused the state
transition attempt.
---
 vpn/plugins/vpn.c | 29 ++++++++++++++++++++++++++++-
 vpn/plugins/vpn.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/vpn/plugins/vpn.c b/vpn/plugins/vpn.c
index cb0d304b..51b202e4 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;
@@ -280,7 +283,30 @@ static DBusMessage *vpn_notify(struct connman_task *task,
 	DBG("provider %p driver %s state %d", provider, name, state);
 
 	switch (state) {
+	case VPN_STATE_ASSOCIATION:
+		/*
+		 * If plugin states it should be still waiting for VPN agent
+		 * we revert the state to association only if previous was
+		 * CONNECT state.
+		 */
+		if (data->state == VPN_STATE_CONNECT) {
+			data->state = VPN_STATE_ASSOCIATION;
+			vpn_provider_set_state(provider,
+						VPN_PROVIDER_STATE_ASSOCIATION);
+		} else if (data->state != VPN_STATE_ASSOCIATION) {
+			connman_warn("Invalid %s vpn_notify() state transition "
+					"from %d to %d (ASSOCIATION)",
+					vpn_driver_data->name, data->state,
+					state);
+		}
+		break;
 	case VPN_STATE_CONNECT:
+		if (data->state != VPN_STATE_CONNECT) {
+			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) {
 			/*
@@ -565,6 +591,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 +672,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..92609b92 100644
--- a/vpn/plugins/vpn.h
+++ b/vpn/plugins/vpn.h
@@ -39,6 +39,7 @@ enum vpn_state {
 	VPN_STATE_DISCONNECT    = 4,
 	VPN_STATE_FAILURE       = 5,
 	VPN_STATE_AUTH_FAILURE  = 6,
+	VPN_STATE_ASSOCIATION   = 7,
 };
 
 struct vpn_driver {
-- 
2.30.2

[RFC PATCH 4/9] 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.30.2

[RFC PATCH 5/9] 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 b955d98b..863924de 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -754,6 +754,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);
 bool __connman_service_is_provider_pending(struct connman_service *service);
 void __connman_service_set_provider_pending(struct connman_service *service,
diff --git a/src/service.c b/src/service.c
index 079c7a6c..05527954 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4527,8 +4527,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);
 
@@ -4556,7 +4575,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,
@@ -6790,9 +6829,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.30.2

[RFC PATCH 6/9] 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 |  1 +
 src/provider.c     | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/provider.h b/include/provider.h
index 1f120990..f2954dc3 100644
--- a/include/provider.h
+++ b/include/provider.h
@@ -48,6 +48,7 @@ enum connman_provider_state {
 	CONNMAN_PROVIDER_STATE_READY         = 3,
 	CONNMAN_PROVIDER_STATE_DISCONNECT    = 4,
 	CONNMAN_PROVIDER_STATE_FAILURE       = 5,
+	CONNMAN_PROVIDER_STATE_ASSOCIATION   = 6,
 };
 
 enum connman_provider_error {
diff --git a/src/provider.c b/src/provider.c
index e2091846..0d09c85b 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.30.2

[RFC PATCH 7/9] 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.30.2

[RFC PATCH 8/9] 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.30.2

[RFC PATCH 9/9] 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       | 4 +++-
 2 files changed, 5 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..7a07fdd1 100644
--- a/doc/vpn-overview.txt
+++ b/doc/vpn-overview.txt
@@ -66,7 +66,9 @@ 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".
 
 In addition to basic credentials, there are additional types of optional
 and control parameters. The user can dictate whether to store the
-- 
2.30.2

Re: [RFC PATCH 0/9] Add association state for VPNs

[Daniel Wagner]

Hi Jussi,

On Tue, Dec 13, 2022 at 04:06:58PM +0200, Jussi Laakkonen 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.

I can't remember why the VPN part is missing the association state. Propaply it
was assummed having the configuration state is enough.

> I'd like to raise the following issues for commenting:
> a) Is this current approach sound in the way that the vpn-agent.c is
>    responsible of changing the state? vpn-provider.c:connect_cb() would be
>    ideal but that does get called in notify if there is no error. Processing
>    the message with the vpn_agent_check_and_process_reply_error() with success
>    indicates that VPN agent succeeded getting the input.

Unfortunatly, I can't remember all these details, though I think the idea of
connect_cb was to get called as final function. So it's sounds like you want to
extend it to handle all state transitions. Sounds reasonable, but I suggest to
update the name so that it refelcts more what it is doing, e.g. match it with
the service.c::indicate_state, vpn_indicate_state (?) Does this make any sense?

> b) Can we figure out a case where the state transition from VPN plugin notify
>    SHOULD change the state back to ASSOCIATION when normally in CONNECT state?
>    This change is in vpn/plugins/vpn.c that accommodates transitions from
>    CONNECT->ASSOCIATION.

Hmm, the main state machine for ConnMan doesn't allow for this transitioning
from CONNECT to ASSOCIATION (or CONFIGURATION) state. Don't know if we should
introduce it. It has a high potential to introduce a lot of bugs. At least
I would like to have a proper use case why we need it before we start
touching the main statemachine.

> c) Furthermore, might any of the VPN plugins available (elsewhere) work
>    without accessing the VPN agent? The flow now requires to use vpn-agent.c
>    for processing and state transition. However, these cases should then use
>    the vpn_provider_set_state() directly, as most of the plugins now do in,
>    e.g., error cases.

That might be the case, but I don't think we should consider this as really an
argument to stop us from updating the code base.

> d) The VPN and PROVIDER states, should these be harmonized to follow the same
>    structure as the common service states to avoid confusion or might this be
>    risky as many of the headers are public?

Yes, I think we should have all these state machines updated in sync, to avoid
complexity and reduce our maintainance burden. Yes, that might break exising
plugins though I am not sure there are any. In this case I would update our code
base and if someone is complaining we can still think about mitigation, such as
introducing some combat function/macros.

> e) Should the specific VPNs that do need VPN agent more than once then do the
>    transition back to ASSOCIATION when the first VPN agent returns and sets the
>    state to CONNECT? This is what I couldn't decide, there is another option to
>    have the vpn_agent_check_and_process_reply_error() to have an additional
>    parameter to state that do not transition state, but in the case of OpenVPN
>    the private key, for instance, can be decrypted and the management interface
>    queries the passphrase on need basis. Thus, there is no apriori information
>    on this.

No idea, you seem to have a why better understanding of the situation. It's up
to your :)

> And of course, any other issue that may trouble your mind with these changes.

I don't have any, right now. Let me quickly look though the patches. Though I
don't expect a lot of comments from my side.

Cheers,
Daniel

Re: [RFC PATCH 0/9] Add association state for VPNs

[Jussi Laakkonen]

Hi Daniel

On 1/2/23 10:10, Daniel Wagner wrote:
> Hi Jussi,
> 
>>
>> 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.
> 
> I can't remember why the VPN part is missing the association state. Propaply it
> was assummed having the configuration state is enough.

Most likely, yes. I had a bit of a head scratching with this that how to 
achieve this and missing state on VPN side seemed the best approach.

> 
>> I'd like to raise the following issues for commenting:
>> a) Is this current approach sound in the way that the vpn-agent.c is
>>     responsible of changing the state? vpn-provider.c:connect_cb() would be
>>     ideal but that does get called in notify if there is no error. Processing
>>     the message with the vpn_agent_check_and_process_reply_error() with success
>>     indicates that VPN agent succeeded getting the input.
> 
> Unfortunatly, I can't remember all these details, though I think the idea of
> connect_cb was to get called as final function. So it's sounds like you want to
> extend it to handle all state transitions. Sounds reasonable, but I suggest to
> update the name so that it refelcts more what it is doing, e.g. match it with
> the service.c::indicate_state, vpn_indicate_state (?) Does this make any sense?
> 

It does make some sense, however, there is the issue whether we'd want 
each of the plugins explicitly having to call the new 
vpn_indicate_state() for state transition or to automate the state 
transition for the plugins that use VPN agent, which, as of now, seems 
to be the case with the current VPN plugins. I was thinking that might 
it become a bit too complex if the new vpn_indicate_state() 
(connect_cb()) could be called more than once from within the VPN plugin 
as the current approach is that the callback function should be called 
only once.

So options are:
  1) the current proposed way that is cleaner for the plugins using VPN 
agent but needs one extra call for plugins that do not use it.
  2) make the plugins more in control of state transition, each would 
need to call in ASSOCIATION state to transition to CONNECT state with an 
explicit call.

But now as I'm thinking this again, maybe the state transition shouldn't 
be within plugins but by introducing as an additional callback through 
the vpn_driver, e.g., use_vpn_agent() that would be needed to be 
implemented only by the VPN plugins that do not use VPN agent. I.e., 
only when the function exists and explicitly states 1/false as return 
value the vpn-provider.c transitions the state to CONNECT right after 
the __vpn_provider_connect() has done the connect attempt and first 
indicated ASSOCIATION state.

I'm just thinking that which is the cleanest one and requires least 
amount of maintenance.

>> b) Can we figure out a case where the state transition from VPN plugin notify
>>     SHOULD change the state back to ASSOCIATION when normally in CONNECT state?
>>     This change is in vpn/plugins/vpn.c that accommodates transitions from
>>     CONNECT->ASSOCIATION.
> 
> Hmm, the main state machine for ConnMan doesn't allow for this transitioning
> from CONNECT to ASSOCIATION (or CONFIGURATION) state. Don't know if we should
> introduce it. It has a high potential to introduce a lot of bugs. At least
> I would like to have a proper use case why we need it before we start
> touching the main statemachine.

Yep, I will remove this from the changes. With an afterthought it might 
not be used after all since notify usually tells either failure or 
success of the connection attempt back to the caller.

> 
>> c) Furthermore, might any of the VPN plugins available (elsewhere) work
>>     without accessing the VPN agent? The flow now requires to use vpn-agent.c
>>     for processing and state transition. However, these cases should then use
>>     the vpn_provider_set_state() directly, as most of the plugins now do in,
>>     e.g., error cases.
> 
> That might be the case, but I don't think we should consider this as really an
> argument to stop us from updating the code base.

Well, with the approach suggested above there would be a way for those 
plugins that do not use VPN agent to indicate that state needs to be 
transitioned to CONNECT at start. So when VPN agent is not needed, 
implement the callback to return 1/false into the VPN plugin.

> 
>> d) The VPN and PROVIDER states, should these be harmonized to follow the same
>>     structure as the common service states to avoid confusion or might this be
>>     risky as many of the headers are public?
> 
> Yes, I think we should have all these state machines updated in sync, to avoid
> complexity and reduce our maintainance burden. Yes, that might break exising
> plugins though I am not sure there are any. In this case I would update our code
> base and if someone is complaining we can still think about mitigation, such as
> introducing some combat function/macros.

Ok, I'll sync those state enums and lets see if someone disagrees :).

> 
>> e) Should the specific VPNs that do need VPN agent more than once then do the
>>     transition back to ASSOCIATION when the first VPN agent returns and sets the
>>     state to CONNECT? This is what I couldn't decide, there is another option to
>>     have the vpn_agent_check_and_process_reply_error() to have an additional
>>     parameter to state that do not transition state, but in the case of OpenVPN
>>     the private key, for instance, can be decrypted and the management interface
>>     queries the passphrase on need basis. Thus, there is no apriori information
>>     on this.
> 
> No idea, you seem to have a why better understanding of the situation. It's up
> to your :)

Well, maybe that could be listed as a TODO as I have no proposal for 
this. In the cases where agent is required after the initial one, which 
leads to ASSOCIATION->CONNECT transition there would be the same timeout 
that there is for the first VPN agent dialog before these changes.

> 
>> And of course, any other issue that may trouble your mind with these changes.
> 
> I don't have any, right now. Let me quickly look though the patches. Though I
> don't expect a lot of comments from my side.
> 

I'll edit the patch set and send it as without RFC tag after I hear your 
thoughts on the matters I just wrote. And thanks for taking time in 
looking through these.


Cheers,
  Jussi

Re: [RFC PATCH 0/9] Add association state for VPNs

[Daniel Wagner]

Hi Jussi,

> I'm just thinking that which is the cleanest one and requires least amount
> of maintenance.

Honestely, I haven't looked at the code base for a long time. So I can't really
say. Usually, if I can't decide which interface works best, I prototype both and
see how they feel. It's always very hard to tell just by looking at it how good
the result will be.

> > > c) Furthermore, might any of the VPN plugins available (elsewhere) work
> > >     without accessing the VPN agent? The flow now requires to use vpn-agent.c
> > >     for processing and state transition. However, these cases should then use
> > >     the vpn_provider_set_state() directly, as most of the plugins now do in,
> > >     e.g., error cases.
> > 
> > That might be the case, but I don't think we should consider this as really an
> > argument to stop us from updating the code base.
> 
> Well, with the approach suggested above there would be a way for those
> plugins that do not use VPN agent to indicate that state needs to be
> transitioned to CONNECT at start. So when VPN agent is not needed, implement
> the callback to return 1/false into the VPN plugin.

Ah see, I am knowledge is pretty outdated here :)

> > I don't have any, right now. Let me quickly look though the patches. Though I
> > don't expect a lot of comments from my side.
> > 
> 
> I'll edit the patch set and send it as without RFC tag after I hear your
> thoughts on the matters I just wrote. And thanks for taking time in looking
> through these.

Great!

Thanks a lot!
Daniel