From cb7cfc3f127cef9f0e19770c6a91bd5b700343ae Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 08:01:23 +0100 Subject: [PATCH 01/13] shared: don't allow NULL arguments with g_hash_table_steal_extended() compat implementation We cannot know the key/value free functions, hence, our compat implementation cannot free the values if they are not requested. The "solution" is to require the caller to fetch all values, always. --- shared/nm-glib-aux/nm-glib.h | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/shared/nm-glib-aux/nm-glib.h b/shared/nm-glib-aux/nm-glib.h index e7e69dc5fe..4043ec2240 100644 --- a/shared/nm-glib-aux/nm-glib.h +++ b/shared/nm-glib-aux/nm-glib.h @@ -608,21 +608,30 @@ g_hash_table_steal_extended (GHashTable *hash_table, gpointer *stolen_key, gpointer *stolen_value) { + g_assert (stolen_key); + g_assert (stolen_value); + if (g_hash_table_lookup_extended (hash_table, lookup_key, stolen_key, stolen_value)) { g_hash_table_steal (hash_table, lookup_key); return TRUE; } - if (stolen_key) - *stolen_key = NULL; - if (stolen_value) - *stolen_value = NULL; + *stolen_key = NULL; + *stolen_value = NULL; return FALSE; } #else #define g_hash_table_steal_extended(hash_table, lookup_key, stolen_key, stolen_value) \ ({ \ + gpointer *_stolen_key = (stolen_key); \ + gpointer *_stolen_value = (stolen_value); \ + \ + /* we cannot allow NULL arguments, because then we would leak the values in + * the compat implementation. */ \ + g_assert (_stolen_key); \ + g_assert (_stolen_value); \ + \ G_GNUC_BEGIN_IGNORE_DEPRECATIONS \ - g_hash_table_steal_extended (hash_table, lookup_key, stolen_key, stolen_value); \ + g_hash_table_steal_extended (hash_table, lookup_key, _stolen_key, _stolen_value); \ G_GNUC_END_IGNORE_DEPRECATIONS \ }) #endif From 89bfb64af5f391e58c7a619b24d3509635f162bc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 10:54:16 +0100 Subject: [PATCH 02/13] auth-chain: add nm_auth_chain_get_context() accessor Will be used later. Also rename "struct NMAuthChain" to "struct _NMAuthChain". It follows how we commonly name this kind of struct. --- src/nm-auth-utils.c | 10 +++++++++- src/nm-auth-utils.h | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 099fcfbfc3..ca2870db72 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -16,7 +16,7 @@ /*****************************************************************************/ -struct NMAuthChain { +struct _NMAuthChain { CList parent_lst; @@ -266,6 +266,14 @@ nm_auth_chain_get_subject (NMAuthChain *self) return self->subject; } +GDBusMethodInvocation * +nm_auth_chain_get_context (NMAuthChain *self) +{ + g_return_val_if_fail (self, NULL); + + return self->context; +} + /*****************************************************************************/ static void diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 808d20fdb6..978d607aed 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -12,7 +12,7 @@ /*****************************************************************************/ -typedef struct NMAuthChain NMAuthChain; +typedef struct _NMAuthChain NMAuthChain; typedef void (*NMAuthChainResultFunc) (NMAuthChain *chain, GDBusMethodInvocation *context, @@ -53,6 +53,8 @@ void nm_auth_chain_destroy (NMAuthChain *chain); NMAuthSubject *nm_auth_chain_get_subject (NMAuthChain *self); +GDBusMethodInvocation *nm_auth_chain_get_context (NMAuthChain *self); + /*****************************************************************************/ struct CList; From b32d656d2627063fd3cee749d4dbf45aec85e52d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 07:49:36 +0100 Subject: [PATCH 03/13] agent-manager: drop unused error handling in agent_manager_register_with_capabilities() nm_auth_chain_new_subject() cannot fail. --- src/settings/nm-agent-manager.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index af6358a360..95e331795d 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -410,18 +410,10 @@ agent_manager_register_with_capabilities (NMAgentManager *self, /* Kick off permissions requests for this agent */ chain = nm_auth_chain_new_subject (subject, context, agent_register_permissions_done, self); - if (chain) { - nm_auth_chain_set_data (chain, "agent", agent, g_object_unref); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, FALSE); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, FALSE); - - priv->chains = g_slist_append (priv->chains, chain); - } else { - g_object_unref (agent); - error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, - NM_AGENT_MANAGER_ERROR_FAILED, - "Unable to start agent authentication."); - } + nm_auth_chain_set_data (chain, "agent", agent, g_object_unref); + nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, FALSE); + nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, FALSE); + priv->chains = g_slist_append (priv->chains, chain); done: if (error) From 2dcd9fa836b1dfc4f76a2955c60b07284c462abb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 07:50:08 +0100 Subject: [PATCH 04/13] agent-manager: use cleanup macro for subject in agent_manager_register_with_capabilities() More cleanup macros. --- src/settings/nm-agent-manager.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 95e331795d..47cb5c13d1 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -368,7 +368,7 @@ agent_manager_register_with_capabilities (NMAgentManager *self, guint32 capabilities) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - NMAuthSubject *subject; + gs_unref_object NMAuthSubject *subject = NULL; gulong sender_uid = G_MAXULONG; GError *error = NULL; NMSecretAgent *agent; @@ -418,7 +418,6 @@ agent_manager_register_with_capabilities (NMAgentManager *self, done: if (error) g_dbus_method_invocation_take_error (context, error); - g_clear_object (&subject); } static void From eba629fb07e7129381c477239d5fd19de79bac31 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 09:21:23 +0100 Subject: [PATCH 05/13] agent-manager: don't handle failure of nm_secret_agent_new() in agent_manager_register_with_capabilities() This never fails. There is no need to handle an "error". --- src/settings/nm-agent-manager.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 47cb5c13d1..3f77e00c0a 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -397,12 +397,6 @@ agent_manager_register_with_capabilities (NMAgentManager *self, /* Success, add the new agent */ agent = nm_secret_agent_new (context, subject, identifier, capabilities); - if (!agent) { - error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, - NM_AGENT_MANAGER_ERROR_FAILED, - "Failed to initialize the agent"); - goto done; - } g_signal_connect (agent, NM_SECRET_AGENT_DISCONNECTED, G_CALLBACK (agent_disconnected_cb), self); From 0f32326257d48dcc733bab7bc6691c67e09f4415 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 09:29:54 +0100 Subject: [PATCH 06/13] agent-manager/trivial: rename CList fields to track Request instances --- src/settings/nm-agent-manager.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 3f77e00c0a..227f289b68 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -43,7 +43,7 @@ typedef struct { */ GHashTable *agents; - CList requests; + CList request_lst_head; guint64 agent_version_id; } NMAgentManagerPrivate; @@ -152,7 +152,7 @@ _request_type_to_string (RequestType request_type, gboolean verbose) /*****************************************************************************/ struct _NMAgentManagerCallId { - CList lst_request; + CList request_lst; NMAgentManager *self; @@ -218,8 +218,8 @@ remove_agent (NMAgentManager *self, const char *owner) _LOGD (agent, "agent unregistered or disappeared"); /* Remove this agent from any in-progress secrets requests */ - c_list_for_each_safe (iter, safe, &priv->requests) - request_remove_agent (c_list_entry (iter, Request, lst_request), agent); + c_list_for_each_safe (iter, safe, &priv->request_lst_head) + request_remove_agent (c_list_entry (iter, Request, request_lst), agent); /* And dispose of the agent */ g_hash_table_remove (priv->agents, owner); @@ -331,8 +331,8 @@ agent_register_permissions_done (NMAuthChain *chain, g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); /* Add this agent to any in-progress secrets requests */ - c_list_for_each (iter, &priv->requests) - request_add_agent (c_list_entry (iter, Request, lst_request), agent); + c_list_for_each (iter, &priv->request_lst_head) + request_add_agent (c_list_entry (iter, Request, request_lst), agent); } static NMSecretAgent * @@ -482,7 +482,7 @@ request_new (NMAgentManager *self, req->request_type = request_type; req->detail = g_strdup (detail); req->subject = g_object_ref (subject); - c_list_link_tail (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &req->lst_request); + c_list_link_tail (&NM_AGENT_MANAGER_GET_PRIVATE (self)->request_lst_head, &req->request_lst); return req; } @@ -568,7 +568,7 @@ req_complete_cancel (Request *req, gboolean is_disposing) gs_free_error GError *error = NULL; nm_assert (req && req->self); - nm_assert (!c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (req->self)->requests, &req->lst_request)); + nm_assert (!c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (req->self)->request_lst_head, &req->request_lst)); nm_utils_error_set_cancelled (&error, is_disposing, "NMAgentManager"); req_complete_release (req, NULL, NULL, NULL, error); @@ -583,9 +583,9 @@ req_complete (Request *req, { NMAgentManager *self = req->self; - nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &req->lst_request)); + nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->request_lst_head, &req->request_lst)); - c_list_unlink (&req->lst_request); + c_list_unlink (&req->request_lst); req_complete_release (req, secrets, agent_dbus_owner, agent_username, error); } @@ -1188,9 +1188,9 @@ nm_agent_manager_cancel_secrets (NMAgentManager *self, g_return_if_fail (request_id); g_return_if_fail (request_id->request_type == REQUEST_TYPE_CON_GET); - nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &request_id->lst_request)); + nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->request_lst_head, &request_id->request_lst)); - c_list_unlink (&request_id->lst_request); + c_list_unlink (&request_id->request_lst); req_complete_cancel (request_id, FALSE); } @@ -1475,7 +1475,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); + c_list_init (&priv->request_lst_head); priv->agents = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_object_unref); } @@ -1504,9 +1504,9 @@ dispose (GObject *object) CList *iter; cancel_more: - c_list_for_each (iter, &priv->requests) { + c_list_for_each (iter, &priv->request_lst_head) { c_list_unlink (iter); - req_complete_cancel (c_list_entry (iter, Request, lst_request), TRUE); + req_complete_cancel (c_list_entry (iter, Request, request_lst), TRUE); goto cancel_more; } From 86ba66ee9be6596a522dd497aab57e7c608b83e4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 09:23:19 +0100 Subject: [PATCH 07/13] agent-manager: expose NMSecretAgent struct in header for tight coupling with NMAgentManager NMAgentManager and NMSecretAgent work closely together. In particular, the NMAgentManager creates and tracks the NMSecretAgents and controls it. Move NMSecretAgent struct to the header, so that some fields may become accessible to NMAgentManager. In particular, we will track secret agents with a CList, and this CList element can be embedded in the NMSecretAgent structure. --- src/settings/nm-secret-agent.c | 23 ++++++++++++----------- src/settings/nm-secret-agent.h | 7 +++++++ 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 82a8c904a5..480e774241 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -34,33 +34,28 @@ enum { static guint signals[LAST_SIGNAL] = { 0 }; -typedef struct { +typedef struct _NMSecretAgentPrivate { CList permissions; + CList requests; + GDBusConnection *dbus_connection; char *description; NMAuthSubject *subject; char *identifier; char *owner_username; char *dbus_owner; - GDBusConnection *dbus_connection; GCancellable *name_owner_cancellable; - CList requests; - NMSecretAgentCapabilities capabilities; guint name_owner_changed_id; + NMSecretAgentCapabilities capabilities; bool shutdown_wait_obj_registered:1; } NMSecretAgentPrivate; -struct _NMSecretAgent { - GObject parent; - NMSecretAgentPrivate _priv; -}; - struct _NMSecretAgentClass { GObjectClass parent; }; G_DEFINE_TYPE (NMSecretAgent, nm_secret_agent, G_TYPE_OBJECT) -#define NM_SECRET_AGENT_GET_PRIVATE(self) _NM_GET_PRIVATE (self, NMSecretAgent, NM_IS_SECRET_AGENT) +#define NM_SECRET_AGENT_GET_PRIVATE(self) _NM_GET_PRIVATE_PTR (self, NMSecretAgent, NM_IS_SECRET_AGENT) /*****************************************************************************/ @@ -765,7 +760,11 @@ nm_secret_agent_new (GDBusMethodInvocation *context, static void nm_secret_agent_init (NMSecretAgent *self) { - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); + NMSecretAgentPrivate *priv; + + priv = G_TYPE_INSTANCE_GET_PRIVATE (self, NM_TYPE_SECRET_AGENT, NMSecretAgentPrivate); + + self->_priv = priv; c_list_init (&priv->permissions); c_list_init (&priv->requests); @@ -814,6 +813,8 @@ nm_secret_agent_class_init (NMSecretAgentClass *config_class) { GObjectClass *object_class = G_OBJECT_CLASS (config_class); + g_type_class_add_private (object_class, sizeof (NMSecretAgentPrivate)); + object_class->dispose = dispose; object_class->finalize = finalize; diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h index ea86e432ad..d6e2feba66 100644 --- a/src/settings/nm-secret-agent.h +++ b/src/settings/nm-secret-agent.h @@ -20,6 +20,13 @@ typedef struct _NMSecretAgentClass NMSecretAgentClass; typedef struct _NMSecretAgentCallId NMSecretAgentCallId; +struct _NMSecretAgentPrivate; + +struct _NMSecretAgent { + GObject parent; + struct _NMSecretAgentPrivate *_priv; +}; + GType nm_secret_agent_get_type (void); NMSecretAgent *nm_secret_agent_new (GDBusMethodInvocation *context, From 3e0094af77fe1918f260c95af64cb70ee21a5ce1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 08:52:55 +0100 Subject: [PATCH 08/13] agent-manager: track secret agents with CList instead of hash table There was literally only one place where we would make use of O(1) lookup of secret-agents: during removal. In all other cases (which are the common cases) we had to iterate the known agents. CList is more efficient and more convenient to use when the main mode of operation is iterating. Also note that handling secret agents inevitably scales linear with the number of agents. That is, because for every check we will have to sort the list of agents and send requests to them. It would be very complicated (and probably less efficient for reasonable numbers of secret agents) to avoid O(n). --- src/settings/nm-agent-manager.c | 171 ++++++++++++++++++-------------- src/settings/nm-secret-agent.c | 2 + src/settings/nm-secret-agent.h | 3 + 3 files changed, 100 insertions(+), 76 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 227f289b68..c5a0d9dea1 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -38,10 +38,7 @@ typedef struct { /* Auth chains for checking agent permissions */ GSList *chains; - /* Hashed by owner name, not identifier, since two agents in different - * sessions can use the same identifier. - */ - GHashTable *agents; + CList agent_lst_head; CList request_lst_head; @@ -201,28 +198,84 @@ struct _NMAgentManagerCallId { /*****************************************************************************/ -static gboolean -remove_agent (NMAgentManager *self, const char *owner) +static NMSecretAgent * +_agent_find_by_owner (NMAgentManagerPrivate *priv, + const char *owner) +{ + NMSecretAgent *agent; + + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (nm_streq (nm_secret_agent_get_dbus_owner (agent), owner)) + return agent; + } + return NULL; +} + +static NMSecretAgent * +_agent_find_by_identifier_and_uid (NMAgentManagerPrivate *priv, + const char *identifier, + gulong sender_uid) +{ + NMSecretAgent *agent; + + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if ( nm_streq0 (nm_secret_agent_get_identifier (agent), identifier) + && sender_uid == nm_secret_agent_get_owner_uid (agent)) + return agent; + } + return NULL; +} + +static NMSecretAgent * +_agent_find_by_username (NMAgentManagerPrivate *priv, + const char *username) +{ + NMSecretAgent *agent; + + nm_assert (username); + + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (nm_streq0 (nm_secret_agent_get_owner_username (agent), username)) + return agent; + } + return NULL; +} + +/*****************************************************************************/ + +static void +_agent_remove (NMAgentManager *self, NMSecretAgent *agent) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - NMSecretAgent *agent; CList *iter, *safe; - g_return_val_if_fail (owner != NULL, FALSE); - - /* Make sure this agent has already registered */ - agent = g_hash_table_lookup (priv->agents, owner); - if (!agent) - return FALSE; + nm_assert (NM_IS_SECRET_AGENT (agent)); + nm_assert (c_list_contains (&priv->agent_lst_head, &agent->agent_lst)); _LOGD (agent, "agent unregistered or disappeared"); + c_list_unlink (&agent->agent_lst); + /* Remove this agent from any in-progress secrets requests */ c_list_for_each_safe (iter, safe, &priv->request_lst_head) request_remove_agent (c_list_entry (iter, Request, request_lst), agent); - /* And dispose of the agent */ - g_hash_table_remove (priv->agents, owner); + g_object_unref (agent); +} + +static gboolean +_agent_remove_by_owner (NMAgentManager *self, const char *owner) +{ + NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + NMSecretAgent *agent; + + g_return_val_if_fail (owner, FALSE); + + agent = _agent_find_by_owner (priv, owner); + if (!agent) + return FALSE; + + _agent_remove (self, agent); return TRUE; } @@ -234,7 +287,7 @@ maybe_remove_agent_on_error (NMSecretAgent *agent, if ( g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED) || g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_DISCONNECTED) || g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER)) - remove_agent (nm_agent_manager_get (), nm_secret_agent_get_dbus_owner (agent)); + _agent_remove_by_owner (nm_agent_manager_get (), nm_secret_agent_get_dbus_owner (agent)); } /*****************************************************************************/ @@ -302,7 +355,6 @@ agent_register_permissions_done (NMAuthChain *chain, NMAgentManager *self = NM_AGENT_MANAGER (user_data); NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; - const char *sender; NMAuthCallResult result; CList *iter; @@ -322,8 +374,10 @@ agent_register_permissions_done (NMAuthChain *chain, 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); + + nm_assert (c_list_is_empty (&agent->agent_lst)); + c_list_link_tail (&priv->agent_lst_head, &agent->agent_lst); + _LOGI (agent, "agent registered"); g_dbus_method_invocation_return_value (context, NULL); @@ -335,30 +389,12 @@ agent_register_permissions_done (NMAuthChain *chain, request_add_agent (c_list_entry (iter, Request, request_lst), agent); } -static NMSecretAgent * -find_agent_by_identifier_and_uid (NMAgentManager *self, - const char *identifier, - gulong sender_uid) -{ - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; - NMSecretAgent *agent; - - g_hash_table_iter_init (&iter, priv->agents); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &agent)) { - if ( g_strcmp0 (nm_secret_agent_get_identifier (agent), identifier) == 0 - && nm_secret_agent_get_owner_uid (agent) == sender_uid) - return agent; - } - return NULL; -} - static void agent_disconnected_cb (NMSecretAgent *agent, gpointer user_data) { /* The agent quit, so remove it and let interested clients know */ - remove_agent (NM_AGENT_MANAGER (user_data), - nm_secret_agent_get_dbus_owner (agent)); + _agent_remove_by_owner (NM_AGENT_MANAGER (user_data), + nm_secret_agent_get_dbus_owner (agent)); } static void @@ -388,7 +424,7 @@ agent_manager_register_with_capabilities (NMAgentManager *self, goto done; /* Only one agent for each identifier is allowed per user */ - if (find_agent_by_identifier_and_uid (self, identifier, sender_uid)) { + if (_agent_find_by_identifier_and_uid (priv, identifier, sender_uid)) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, "An agent with this ID is already registered for this user."); @@ -456,7 +492,7 @@ impl_agent_manager_unregister (NMDBusObject *obj, { NMAgentManager *self = NM_AGENT_MANAGER (obj); - if (!remove_agent (self, sender)) { + if (!_agent_remove_by_owner (self, sender)) { g_dbus_method_invocation_return_error_literal (invocation, NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_NOT_REGISTERED, @@ -695,12 +731,10 @@ static void request_add_agents (NMAgentManager *self, Request *req) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; - gpointer data; + NMSecretAgent *agent; - g_hash_table_iter_init (&iter, priv->agents); - while (g_hash_table_iter_next (&iter, NULL, &data)) - request_add_agent (req, NM_SECRET_AGENT (data)); + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) + request_add_agent (req, agent); } static void @@ -1371,17 +1405,10 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, NMSecretAgent * nm_agent_manager_get_agent_by_user (NMAgentManager *self, const char *username) { - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; - NMSecretAgent *agent; + g_return_val_if_fail (NM_IS_AGENT_MANAGER (self), NULL); + g_return_val_if_fail (username, NULL); - g_hash_table_iter_init (&iter, priv->agents); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &agent)) { - if (g_strcmp0 (nm_secret_agent_get_owner_username (agent), username) == 0) - return agent; - } - - return NULL; + return _agent_find_by_username (NM_AGENT_MANAGER_GET_PRIVATE (self), username); } /*****************************************************************************/ @@ -1392,17 +1419,14 @@ nm_agent_manager_all_agents_have_capability (NMAgentManager *manager, NMSecretAgentCapabilities capability) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (manager); - GHashTableIter iter; NMSecretAgent *agent; gboolean subject_is_unix_process = (nm_auth_subject_get_subject_type (subject) == NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS); gulong subject_uid = subject_is_unix_process ? nm_auth_subject_get_unix_process_uid (subject) : 0u; - g_hash_table_iter_init (&iter, priv->agents); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &agent)) { + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { if ( subject_is_unix_process && nm_secret_agent_get_owner_uid (agent) != subject_uid) continue; - if (!(nm_secret_agent_get_capabilities (agent) & capability)) return FALSE; } @@ -1442,12 +1466,10 @@ static void authority_changed_cb (NMAuthManager *auth_manager, NMAgentManager *self) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; NMSecretAgent *agent; /* Recheck the permissions of all secret agents */ - g_hash_table_iter_init (&iter, priv->agents); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &agent)) { + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { NMAuthChain *chain; /* Kick off permissions requests for this agent */ @@ -1455,7 +1477,6 @@ authority_changed_cb (NMAuthManager *auth_manager, NMAgentManager *self) NULL, agent_permissions_changed_done, self); - g_assert (chain); priv->chains = g_slist_append (priv->chains, chain); /* Make sure if the agent quits while the permissions call is in progress @@ -1475,8 +1496,8 @@ nm_agent_manager_init (NMAgentManager *self) NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); priv->agent_version_id = 1; + c_list_init (&priv->agent_lst_head); c_list_init (&priv->request_lst_head); - priv->agents = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_object_unref); } static void @@ -1500,23 +1521,21 @@ constructed (GObject *object) static void dispose (GObject *object) { - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE ((NMAgentManager *) object); - CList *iter; + NMAgentManager *self = NM_AGENT_MANAGER (object); + NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + Request *request; + NMSecretAgent *agent; -cancel_more: - c_list_for_each (iter, &priv->request_lst_head) { - c_list_unlink (iter); - req_complete_cancel (c_list_entry (iter, Request, request_lst), TRUE); - goto cancel_more; + while ((request = c_list_first_entry (&priv->request_lst_head, Request, request_lst))) { + c_list_unlink (&request->request_lst); + req_complete_cancel (request, TRUE); } g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_destroy); priv->chains = NULL; - if (priv->agents) { - g_hash_table_destroy (priv->agents); - priv->agents = NULL; - } + while ((agent = c_list_first_entry (&priv->agent_lst_head, NMSecretAgent, agent_lst))) + _agent_remove (self, agent); if (priv->auth_mgr) { g_signal_handlers_disconnect_by_func (priv->auth_mgr, diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 480e774241..890f7d77a7 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -766,6 +766,7 @@ nm_secret_agent_init (NMSecretAgent *self) self->_priv = priv; + c_list_init (&self->agent_lst); c_list_init (&priv->permissions); c_list_init (&priv->requests); } @@ -776,6 +777,7 @@ dispose (GObject *object) NMSecretAgent *self = NM_SECRET_AGENT (object); NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); + nm_assert (c_list_is_empty (&self->agent_lst)); nm_assert (c_list_is_empty (&priv->requests)); nm_clear_g_dbus_connection_signal (priv->dbus_connection, diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h index d6e2feba66..32d7238104 100644 --- a/src/settings/nm-secret-agent.h +++ b/src/settings/nm-secret-agent.h @@ -8,6 +8,8 @@ #include "nm-connection.h" +#include "c-list/src/c-list.h" + #define NM_TYPE_SECRET_AGENT (nm_secret_agent_get_type ()) #define NM_SECRET_AGENT(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_SECRET_AGENT, NMSecretAgent)) #define NM_SECRET_AGENT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_SECRET_AGENT, NMSecretAgentClass)) @@ -24,6 +26,7 @@ struct _NMSecretAgentPrivate; struct _NMSecretAgent { GObject parent; + CList agent_lst; struct _NMSecretAgentPrivate *_priv; }; From d4a821d53ebd9b75dd992d13cc1a68da919d4ee8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 11:02:30 +0100 Subject: [PATCH 09/13] agent-manager: let nm_settings_connection_check_permission() check all secret-agents searching for permission nm_agent_manager_get_agent_by_user() would only return the first matching secret agent for the user. This way, we might miss an agent that has permissions. Instead, add nm_agent_manager_has_agent_with_permission() and search all agents. --- src/settings/nm-agent-manager.c | 40 +++++++++++++-------------- src/settings/nm-agent-manager.h | 5 ++-- src/settings/nm-settings-connection.c | 5 +--- 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index c5a0d9dea1..7c45bc8f79 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -226,21 +226,6 @@ _agent_find_by_identifier_and_uid (NMAgentManagerPrivate *priv, return NULL; } -static NMSecretAgent * -_agent_find_by_username (NMAgentManagerPrivate *priv, - const char *username) -{ - NMSecretAgent *agent; - - nm_assert (username); - - c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { - if (nm_streq0 (nm_secret_agent_get_owner_username (agent), username)) - return agent; - } - return NULL; -} - /*****************************************************************************/ static void @@ -1402,13 +1387,28 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, /*****************************************************************************/ -NMSecretAgent * -nm_agent_manager_get_agent_by_user (NMAgentManager *self, const char *username) +gboolean +nm_agent_manager_has_agent_with_permission (NMAgentManager *self, + const char *username, + const char *permission) { - g_return_val_if_fail (NM_IS_AGENT_MANAGER (self), NULL); - g_return_val_if_fail (username, NULL); + NMAgentManagerPrivate *priv; + NMSecretAgent *agent; - return _agent_find_by_username (NM_AGENT_MANAGER_GET_PRIVATE (self), username); + g_return_val_if_fail (NM_IS_AGENT_MANAGER (self), FALSE); + g_return_val_if_fail (username, FALSE); + g_return_val_if_fail (permission, FALSE); + + priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (!nm_streq0 (nm_secret_agent_get_owner_username (agent), username)) + continue; + if (nm_secret_agent_has_permission (agent, permission)) + return TRUE; + } + + return FALSE; } /*****************************************************************************/ diff --git a/src/settings/nm-agent-manager.h b/src/settings/nm-agent-manager.h index 5200d241ca..bf2dcb6ccf 100644 --- a/src/settings/nm-agent-manager.h +++ b/src/settings/nm-agent-manager.h @@ -65,8 +65,9 @@ void nm_agent_manager_delete_secrets (NMAgentManager *manager, const char *path, NMConnection *connection); -NMSecretAgent *nm_agent_manager_get_agent_by_user (NMAgentManager *manager, - const char *username); +gboolean nm_agent_manager_has_agent_with_permission (NMAgentManager *self, + const char *username, + const char *permission); gboolean nm_agent_manager_all_agents_have_capability (NMAgentManager *manager, NMAuthSubject *subject, diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 3368359980..fd701ecde4 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -409,10 +409,7 @@ nm_settings_connection_check_permission (NMSettingsConnection *self, * either. */ if (nm_setting_connection_get_permission (s_con, i, NULL, &puser, NULL)) { - NMSecretAgent *agent = nm_agent_manager_get_agent_by_user (priv->agent_mgr, puser); - - if ( agent - && nm_secret_agent_has_permission (agent, permission)) + if (nm_agent_manager_has_agent_with_permission (priv->agent_mgr, puser, permission)) return TRUE; } } From 821efd87d848e66b4f7f82eb2cc40ca5657ab9eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 11:25:51 +0100 Subject: [PATCH 10/13] agent-manager: pass agent-manager to maybe_remove_agent_on_error() and don't lookup by name Don't access the singleton getter here. Pass the agent-manager argument instead to maybe_remove_agent_on_error(). Also, don't lookup the agent by name. We already know, whether the agent is still tracked or not. Look at agent->agent_lst. --- src/settings/nm-agent-manager.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 7c45bc8f79..549e3c2450 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -266,13 +266,17 @@ _agent_remove_by_owner (NMAgentManager *self, const char *owner) /* Call this *after* calling request_next_agent() */ static void -maybe_remove_agent_on_error (NMSecretAgent *agent, +maybe_remove_agent_on_error (NMAgentManager *self, + NMSecretAgent *agent, GError *error) { - if ( g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED) - || g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_DISCONNECTED) - || g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER)) - _agent_remove_by_owner (nm_agent_manager_get (), nm_secret_agent_get_dbus_owner (agent)); + if ( !g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED) + && !g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_DISCONNECTED) + && !g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER)) + return; + + if (!c_list_is_empty (&agent->agent_lst)) + _agent_remove (self, agent); } /*****************************************************************************/ @@ -872,7 +876,7 @@ _con_get_request_done (NMSecretAgent *agent, /* Try the next agent */ request_next_agent (req); - maybe_remove_agent_on_error (agent, error); + maybe_remove_agent_on_error (self, agent, error); } return; } @@ -1246,7 +1250,7 @@ _con_save_request_done (NMSecretAgent *agent, LOG_REQ_ARG (req), error->message); /* Try the next agent */ request_next_agent (req); - maybe_remove_agent_on_error (agent, error); + maybe_remove_agent_on_error (self, agent, error); return; } @@ -1337,7 +1341,7 @@ _con_del_request_done (NMSecretAgent *agent, /* Tell the next agent to delete secrets */ request_next_agent (req); if (error) - maybe_remove_agent_on_error (agent, error); + maybe_remove_agent_on_error (self, agent, error); } static void From ed85842c36fe8e9717736d81e4f5b3f6add416b0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 11:30:53 +0100 Subject: [PATCH 11/13] agent-manager: disconnect agent_disconnected_cb handler from secret-agent Also, we don't need to use _agent_remove_by_owner(). We know now the agent to be removed. --- src/settings/nm-agent-manager.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 549e3c2450..30f3db101e 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -116,6 +116,8 @@ static void _con_del_request_start (Request *req); static gboolean _con_get_try_complete_early (Request *req); +static void agent_disconnected_cb (NMSecretAgent *agent, gpointer user_data); + /*****************************************************************************/ guint64 @@ -241,6 +243,8 @@ _agent_remove (NMAgentManager *self, NMSecretAgent *agent) c_list_unlink (&agent->agent_lst); + g_signal_handlers_disconnect_by_func (agent, G_CALLBACK (agent_disconnected_cb), self); + /* Remove this agent from any in-progress secrets requests */ c_list_for_each_safe (iter, safe, &priv->request_lst_head) request_remove_agent (c_list_entry (iter, Request, request_lst), agent); @@ -381,9 +385,7 @@ agent_register_permissions_done (NMAuthChain *chain, static void agent_disconnected_cb (NMSecretAgent *agent, gpointer user_data) { - /* The agent quit, so remove it and let interested clients know */ - _agent_remove_by_owner (NM_AGENT_MANAGER (user_data), - nm_secret_agent_get_dbus_owner (agent)); + _agent_remove (NM_AGENT_MANAGER (user_data), agent); } static void From 9bdf95458e6bcd0edb963e5b18a1f504640ba188 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 11:32:28 +0100 Subject: [PATCH 12/13] agent-manager: move and inline _agent_remove_by_owner() to impl_agent_manager_unregister() --- src/settings/nm-agent-manager.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 30f3db101e..35a439e863 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -252,22 +252,6 @@ _agent_remove (NMAgentManager *self, NMSecretAgent *agent) g_object_unref (agent); } -static gboolean -_agent_remove_by_owner (NMAgentManager *self, const char *owner) -{ - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - NMSecretAgent *agent; - - g_return_val_if_fail (owner, FALSE); - - agent = _agent_find_by_owner (priv, owner); - if (!agent) - return FALSE; - - _agent_remove (self, agent); - return TRUE; -} - /* Call this *after* calling request_next_agent() */ static void maybe_remove_agent_on_error (NMAgentManager *self, @@ -482,8 +466,11 @@ impl_agent_manager_unregister (NMDBusObject *obj, GVariant *parameters) { NMAgentManager *self = NM_AGENT_MANAGER (obj); + NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + NMSecretAgent *agent; - if (!_agent_remove_by_owner (self, sender)) { + agent = _agent_find_by_owner (priv, sender); + if (!agent) { g_dbus_method_invocation_return_error_literal (invocation, NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_NOT_REGISTERED, @@ -491,6 +478,8 @@ impl_agent_manager_unregister (NMDBusObject *obj, return; } + _agent_remove (self, agent); + g_dbus_method_invocation_return_value (invocation, NULL); } From bf25081dfe8f6f0f8680ccd5edf576000102b36e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Dec 2019 10:58:03 +0100 Subject: [PATCH 13/13] agent-manager: fix races registering secret agent and track auth-chain per agent We don't need a separate "GSList *chains" to track the NMAuthChain requests for the agents. Every agent should only have one auth-chain in fly at any time. We can attach that NMAuthChain to the secret-agent. Also, fix a race where: 1) A secret agent registers. We would start an auth-chain check, but not yet track the secret agent. 2) Then the secret agent unregisters. The unregistration request will fail, because the secret agent is not yet in the list of fully registered agents. The same happens if the secret agent disconnects at this point. agent_disconnect_cb() would not find the secret agent to remove. 3) afterwards, authentication completes and we register the secret-agent, although we should not. There is also another race: if we get authority_changed_cb() we would not restart the authentication for the secret-agent that is still registering. Hence, we don't know whether the result once it completes would already contain the latest state. --- src/settings/nm-agent-manager.c | 186 +++++++++++++++----------------- src/settings/nm-secret-agent.c | 1 + src/settings/nm-secret-agent.h | 3 + 3 files changed, 93 insertions(+), 97 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 35a439e863..1519361314 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -35,9 +35,6 @@ typedef struct { NMAuthManager *auth_mgr; NMSessionMonitor *session_monitor; - /* Auth chains for checking agent permissions */ - GSList *chains; - CList agent_lst_head; CList request_lst_head; @@ -241,6 +238,8 @@ _agent_remove (NMAgentManager *self, NMSecretAgent *agent) _LOGD (agent, "agent unregistered or disappeared"); + nm_clear_pointer (&agent->auth_chain, nm_auth_chain_destroy); + c_list_unlink (&agent->agent_lst); g_signal_handlers_disconnect_by_func (agent, G_CALLBACK (agent_disconnected_cb), self); @@ -325,45 +324,86 @@ validate_identifier (const char *identifier, GError **error) } static void -agent_register_permissions_done (NMAuthChain *chain, - GDBusMethodInvocation *context, - gpointer user_data) +_agent_permissions_check_done (NMAuthChain *chain, + GDBusMethodInvocation *context, + gpointer user_data) { NMAgentManager *self = NM_AGENT_MANAGER (user_data); NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; - NMAuthCallResult result; - CList *iter; + Request *request; - nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); - - priv->chains = g_slist_remove (priv->chains, chain); + nm_assert (!context || G_IS_DBUS_METHOD_INVOCATION (context)); agent = nm_auth_chain_steal_data (chain, "agent"); - nm_assert (agent); - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED); - if (result == NM_AUTH_CALL_RESULT_YES) - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, TRUE); + nm_assert (NM_IS_SECRET_AGENT (agent)); + nm_assert (agent->auth_chain == chain); + nm_assert (agent->fully_registered == (!context)); + nm_assert (c_list_contains (&priv->agent_lst_head, &agent->agent_lst)); - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN); - if (result == NM_AUTH_CALL_RESULT_YES) - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, TRUE); + agent->auth_chain = NULL; + + nm_secret_agent_add_permission (agent, + NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, + (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED) == NM_AUTH_CALL_RESULT_YES)); + nm_secret_agent_add_permission (agent, + NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, + (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN) == NM_AUTH_CALL_RESULT_YES)); + + if (agent->fully_registered) { + _LOGD (agent, "updated agent permissions"); + return; + } + + _LOGI (agent, "agent registered"); + + agent->fully_registered = TRUE; priv->agent_version_id += 1; - nm_assert (c_list_is_empty (&agent->agent_lst)); - c_list_link_tail (&priv->agent_lst_head, &agent->agent_lst); - - _LOGI (agent, "agent registered"); g_dbus_method_invocation_return_value (context, NULL); - /* Signal an agent was registered */ - g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); + c_list_for_each_entry (request, &priv->request_lst_head, request_lst) + request_add_agent (request, agent); - /* Add this agent to any in-progress secrets requests */ - c_list_for_each (iter, &priv->request_lst_head) - request_add_agent (c_list_entry (iter, Request, request_lst), agent); + g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); +} + +static NMAuthChain * +_agent_create_auth_chain (NMAgentManager *self, + NMSecretAgent *agent, + GDBusMethodInvocation *context) +{ + NMAuthChain *chain; + + _LOGD (agent, "requesting permissions"); + + nm_assert ( !agent->auth_chain + || (agent->fully_registered == (!nm_auth_chain_get_context (agent->auth_chain)))); + + if ( agent->auth_chain + && !context + && !agent->fully_registered) { + /* we restart the authorization check (without a @context), but the currently + * pending auth-chain carries a context. We need to pass it on as we replace + * the auth-chain. */ + context = nm_auth_chain_get_context (agent->auth_chain); + nm_assert (context); + } + + chain = nm_auth_chain_new_subject (nm_secret_agent_get_subject (agent), + context, + _agent_permissions_check_done, + self); + + nm_auth_chain_set_data (chain, "agent", agent, NULL); + nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, FALSE); + nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, FALSE); + + nm_clear_pointer (&agent->auth_chain, nm_auth_chain_destroy); + agent->auth_chain = chain; + return chain; } static void @@ -383,46 +423,40 @@ agent_manager_register_with_capabilities (NMAgentManager *self, gulong sender_uid = G_MAXULONG; GError *error = NULL; NMSecretAgent *agent; - NMAuthChain *chain; subject = nm_dbus_manager_new_auth_subject_from_context (context); if (!subject) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); - goto done; + g_dbus_method_invocation_take_error (context, error); + return; } sender_uid = nm_auth_subject_get_unix_process_uid (subject); /* Validate the identifier */ - if (!validate_identifier (identifier, &error)) - goto done; + if (!validate_identifier (identifier, &error)) { + g_dbus_method_invocation_take_error (context, error); + return; + } /* Only one agent for each identifier is allowed per user */ if (_agent_find_by_identifier_and_uid (priv, identifier, sender_uid)) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, "An agent with this ID is already registered for this user."); - goto done; + g_dbus_method_invocation_take_error (context, error); + return; } - /* Success, add the new agent */ agent = nm_secret_agent_new (context, subject, identifier, capabilities); + g_signal_connect (agent, NM_SECRET_AGENT_DISCONNECTED, G_CALLBACK (agent_disconnected_cb), self); - _LOGD (agent, "requesting permissions"); + c_list_link_tail (&priv->agent_lst_head, &agent->agent_lst); - /* Kick off permissions requests for this agent */ - chain = nm_auth_chain_new_subject (subject, context, agent_register_permissions_done, self); - nm_auth_chain_set_data (chain, "agent", agent, g_object_unref); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, FALSE); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, FALSE); - priv->chains = g_slist_append (priv->chains, chain); - -done: - if (error) - g_dbus_method_invocation_take_error (context, error); + _agent_create_auth_chain (self, agent, context); } static void @@ -713,8 +747,10 @@ request_add_agents (NMAgentManager *self, Request *req) NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; - c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) - request_add_agent (req, agent); + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (agent->fully_registered) + request_add_agent (req, agent); + } } static void @@ -1036,7 +1072,7 @@ _con_get_request_start (Request *req) NULL, _con_get_request_start_validated, req); - g_assert (req->con.chain); + nm_assert (req->con.chain); /* If the caller is the only user in the connection's permissions, then * we use the 'modify.own' permission instead of 'modify.system'. If the @@ -1187,7 +1223,6 @@ nm_agent_manager_get_secrets (NMAgentManager *self, req->con.get.callback = callback; req->con.get.callback_data = callback_data; - /* Kick off the request */ if (!(req->con.get.flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM)) request_add_agents (self, req); req->idle_id = g_idle_add (request_start, req); @@ -1290,7 +1325,6 @@ nm_agent_manager_save_secrets (NMAgentManager *self, req->con.path = g_strdup (path); req->con.connection = g_object_ref (connection); - /* Kick off the request */ request_add_agents (self, req); req->idle_id = g_idle_add (request_start, req); } @@ -1375,7 +1409,6 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, req->con.connection = g_object_ref (connection); g_object_unref (subject); - /* Kick off the request */ request_add_agents (self, req); req->idle_id = g_idle_add (request_start, req); } @@ -1397,6 +1430,8 @@ nm_agent_manager_has_agent_with_permission (NMAgentManager *self, priv = NM_AGENT_MANAGER_GET_PRIVATE (self); c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (!agent->fully_registered) + continue; if (!nm_streq0 (nm_secret_agent_get_owner_username (agent), username)) continue; if (nm_secret_agent_has_permission (agent, permission)) @@ -1419,6 +1454,8 @@ nm_agent_manager_all_agents_have_capability (NMAgentManager *manager, gulong subject_uid = subject_is_unix_process ? nm_auth_subject_get_unix_process_uid (subject) : 0u; c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { + if (!agent->fully_registered) + continue; if ( subject_is_unix_process && nm_secret_agent_get_owner_uid (agent) != subject_uid) continue; @@ -1431,56 +1468,14 @@ nm_agent_manager_all_agents_have_capability (NMAgentManager *manager, /*****************************************************************************/ -static void -agent_permissions_changed_done (NMAuthChain *chain, - GDBusMethodInvocation *context, - gpointer user_data) -{ - NMAgentManager *self = NM_AGENT_MANAGER (user_data); - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - NMSecretAgent *agent; - gboolean share_protected = FALSE, share_open = FALSE; - - priv->chains = g_slist_remove (priv->chains, chain); - - agent = nm_auth_chain_get_data (chain, "agent"); - g_assert (agent); - - _LOGD (agent, "updated agent permissions"); - - if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED) == NM_AUTH_CALL_RESULT_YES) - share_protected = TRUE; - if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN) == NM_AUTH_CALL_RESULT_YES) - share_open = TRUE; - - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, share_protected); - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open); -} - static void authority_changed_cb (NMAuthManager *auth_manager, NMAgentManager *self) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; - /* Recheck the permissions of all secret agents */ - c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) { - NMAuthChain *chain; - - /* Kick off permissions requests for this agent */ - chain = nm_auth_chain_new_subject (nm_secret_agent_get_subject (agent), - NULL, - agent_permissions_changed_done, - self); - priv->chains = g_slist_append (priv->chains, chain); - - /* Make sure if the agent quits while the permissions call is in progress - * that the object sticks around until our callback. - */ - nm_auth_chain_set_data (chain, "agent", g_object_ref (agent), g_object_unref); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, FALSE); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, FALSE); - } + c_list_for_each_entry (agent, &priv->agent_lst_head, agent_lst) + _agent_create_auth_chain (self, agent, NULL); } /*****************************************************************************/ @@ -1526,9 +1521,6 @@ dispose (GObject *object) req_complete_cancel (request, TRUE); } - g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_destroy); - priv->chains = NULL; - while ((agent = c_list_first_entry (&priv->agent_lst_head, NMSecretAgent, agent_lst))) _agent_remove (self, agent); diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 890f7d77a7..e5c727b49f 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -778,6 +778,7 @@ dispose (GObject *object) NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); nm_assert (c_list_is_empty (&self->agent_lst)); + nm_assert (!self->auth_chain); nm_assert (c_list_is_empty (&priv->requests)); nm_clear_g_dbus_connection_signal (priv->dbus_connection, diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h index 32d7238104..37a93d1720 100644 --- a/src/settings/nm-secret-agent.h +++ b/src/settings/nm-secret-agent.h @@ -22,12 +22,15 @@ typedef struct _NMSecretAgentClass NMSecretAgentClass; typedef struct _NMSecretAgentCallId NMSecretAgentCallId; +struct _NMAuthChain; struct _NMSecretAgentPrivate; struct _NMSecretAgent { GObject parent; CList agent_lst; + struct _NMAuthChain *auth_chain; struct _NMSecretAgentPrivate *_priv; + bool fully_registered:1; }; GType nm_secret_agent_get_type (void);