From 118b9f29785834bd3790b17ac0314c1cac23c2a5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 27 Nov 2017 09:33:32 +0100 Subject: [PATCH] core: fix race of blocking autoconnect for no-secrets when a new secret-agent registers When activation of the connection fails with no-secrets, we block autoconnect due to that. However, NMPolicy also unblocks such autoconnect, whenever a new secret-agent registers. The reason is obviously, that the new secret-agent might be able to provide the previously missing secrets. However, there is a race between - making the secret request, failing activation and blocking autoconnect - new secret-agent registers If the secret-agent registers after making the request, but before we block autoconnect, then autoconnect stays blocked. [1511468634.5759] device (wlp4s0): state change: config -> need-auth (reason 'none', sys-iface-state: 'managed') [1511468634.5772] device (wlp4s0): No agents were available for this request. [1511468638.4082] agent-manager: req[0x55ea7e58a5d0, :1.32/org.kde.plasma.networkmanagement/1000]: agent registered [1511468638.4082] policy: re-enabling autoconnect for all connections with failed secrets [1511468664.6280] device (wlp4s0): state change: need-auth -> failed (reason 'no-secrets', sys-iface-state: 'managed') [1511468664.6287] policy: connection 'tuxmobil' now blocked from autoconnect due to no secrets Note the long timing between making the secret request and the activation failure. This race already existed before, but now with WPS push-button method enabled by default, the duraction of the activation is much longer and the race is easy to hit. https://bugzilla.gnome.org/show_bug.cgi?id=790571 (cherry picked from commit e2c8ef45ac9fba8d4f5722ab10831bf42085a110) --- src/nm-policy.c | 23 +++++++++++++++++++---- src/settings/nm-agent-manager.c | 14 ++++++++++++++ src/settings/nm-agent-manager.h | 2 ++ src/settings/nm-secret-agent.c | 7 ++----- src/settings/nm-settings-connection.c | 26 ++++++++++++++++++++++++-- src/settings/nm-settings-connection.h | 2 ++ 6 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 1a8dedc2d6..f5fac088f0 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -1750,20 +1750,35 @@ device_state_changed (NMDevice *device, if ( connection && old_state >= NM_DEVICE_STATE_PREPARE && old_state <= NM_DEVICE_STATE_ACTIVATED) { + gboolean block_no_secrets = FALSE; int tries; + guint64 con_v; - tries = nm_settings_connection_autoconnect_retries_get (connection); if (nm_device_state_reason_check (reason) == NM_DEVICE_STATE_REASON_NO_SECRETS) { + /* we want to block the connection from auto-connect if it failed due to no-secrets. + * However, if a secret-agent registered, since the connection made the last + * secret-request, we do not block it. The new secret-agent might not yet + * been consulted, and it may be able to provide the secrets. + * + * We detect this by using a version-id of the agent-manager, which increments + * whenever new agents register. */ + con_v = nm_settings_connection_get_last_secret_agent_version_id (connection); + if ( con_v == 0 + || con_v != nm_agent_manager_get_agent_version_id (priv->agent_mgr)) + block_no_secrets = TRUE; + } + + if (block_no_secrets) { _LOGD (LOGD_DEVICE, "connection '%s' now blocked from autoconnect due to no secrets", nm_settings_connection_get_id (connection)); - nm_settings_connection_autoconnect_blocked_reason_set (connection, NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_NO_SECRETS, TRUE); - } else if (tries != 0) { + } else { + tries = nm_settings_connection_autoconnect_retries_get (connection); if (tries > 0) { _LOGD (LOGD_DEVICE, "connection '%s' failed to autoconnect; %d tries left", nm_settings_connection_get_id (connection), tries - 1); _connection_autoconnect_retries_set (self, connection, tries - 1); - } else { + } else if (tries != 0) { _LOGD (LOGD_DEVICE, "connection '%s' failed to autoconnect; infinite tries left", nm_settings_connection_get_id (connection)); } diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 86de21a99a..345d6e6611 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -62,6 +62,8 @@ typedef struct { GHashTable *agents; CList requests; + + guint64 agent_version_id; } NMAgentManagerPrivate; struct _NMAgentManager { @@ -137,6 +139,16 @@ static gboolean _con_get_try_complete_early (Request *req); /*****************************************************************************/ +guint64 +nm_agent_manager_get_agent_version_id (NMAgentManager *self) +{ + g_return_val_if_fail (NM_IS_AGENT_MANAGER (self), 0); + + return NM_AGENT_MANAGER_GET_PRIVATE (self)->agent_version_id; +} + +/*****************************************************************************/ + typedef enum { REQUEST_TYPE_INVALID, REQUEST_TYPE_CON_GET, @@ -336,6 +348,7 @@ agent_register_permissions_done (NMAuthChain *chain, if (result == NM_AUTH_CALL_RESULT_YES) nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, TRUE); + priv->agent_version_id += 1; sender = nm_secret_agent_get_dbus_owner (agent); g_hash_table_insert (priv->agents, g_strdup (sender), agent); _LOGD (agent, "agent registered"); @@ -1558,6 +1571,7 @@ nm_agent_manager_init (NMAgentManager *self) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + priv->agent_version_id = 1; c_list_init (&priv->requests); priv->agents = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_object_unref); } diff --git a/src/settings/nm-agent-manager.h b/src/settings/nm-agent-manager.h index ddbece7c56..f6845818dd 100644 --- a/src/settings/nm-agent-manager.h +++ b/src/settings/nm-agent-manager.h @@ -43,6 +43,8 @@ GType nm_agent_manager_get_type (void); NMAgentManager *nm_agent_manager_get (void); +guint64 nm_agent_manager_get_agent_version_id (NMAgentManager *self); + /* If no agent fulfilled the secrets request, agent_dbus_owner will be NULL */ typedef void (*NMAgentSecretsResultFunc) (NMAgentManager *manager, NMAgentManagerCallId call_id, diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 99b955ed4a..5cb0ce4538 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -50,16 +50,13 @@ typedef struct { char *owner_username; char *dbus_owner; NMSecretAgentCapabilities capabilities; - GSList *permissions; - NMDBusSecretAgent *proxy; NMBusManager *bus_mgr; GDBusConnection *connection; - gboolean connection_is_private; - gulong on_disconnected_id; - CList requests; + gulong on_disconnected_id; + bool connection_is_private:1; } NMSecretAgentPrivate; struct _NMSecretAgent { diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 4fae50ed0c..ec73174a4d 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -106,13 +106,17 @@ typedef struct _NMSettingsConnectionPrivate { */ NMConnection *agent_secrets; - guint64 timestamp; /* Up-to-date timestamp of connection use */ + char *filename; + GHashTable *seen_bssids; /* Up-to-date BSSIDs that's been seen for the connection */ + guint64 timestamp; /* Up-to-date timestamp of connection use */ + + guint64 last_secret_agent_version_id; + int autoconnect_retries; gint32 autoconnect_retries_blocked_until; - char *filename; } NMSettingsConnectionPrivate; G_DEFINE_TYPE_WITH_CODE (NMSettingsConnection, nm_settings_connection, NM_TYPE_EXPORTED_OBJECT, @@ -171,6 +175,16 @@ nm_settings_connection_has_unmodified_applied_connection (NMSettingsConnection * /*****************************************************************************/ +guint64 +nm_settings_connection_get_last_secret_agent_version_id (NMSettingsConnection *self) +{ + g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (self), 0); + + return NM_SETTINGS_CONNECTION_GET_PRIVATE (self)->last_secret_agent_version_id; +} + +/*****************************************************************************/ + /* Return TRUE to keep, FALSE to drop */ typedef gboolean (*ForEachSecretFunc) (NMSettingSecretFlags flags, gpointer user_data); @@ -1261,6 +1275,14 @@ nm_settings_connection_get_secrets (NMSettingsConnection *self, existing_secrets = nm_connection_to_dbus (priv->system_secrets, NM_CONNECTION_SERIALIZE_ONLY_SECRETS); if (existing_secrets) g_variant_ref_sink (existing_secrets); + + /* we remember the current version-id of the secret-agents. The version-id is strictly increasing, + * as new agents register the number. We know hence, that this request was made against a certain + * set of secret-agents. + * If after making this request a new secret-agent registeres, the version-id increases. + * Then we know that the this request probably did not yet include the latest secret-agent. */ + priv->last_secret_agent_version_id = nm_agent_manager_get_agent_version_id (priv->agent_mgr); + call_id_a = nm_agent_manager_get_secrets (priv->agent_mgr, nm_connection_get_path (NM_CONNECTION (self)), NM_CONNECTION (self), diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index 7caf42b717..3672bc08c1 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -125,6 +125,8 @@ struct _NMSettingsConnectionClass { GType nm_settings_connection_get_type (void); +guint64 nm_settings_connection_get_last_secret_agent_version_id (NMSettingsConnection *self); + gboolean nm_settings_connection_has_unmodified_applied_connection (NMSettingsConnection *self, NMConnection *applied_connection, NMSettingCompareFlags compare_flage);