Re: NM 0.9 asks for PK auth without need

[Ludwig Nussel <ludwig nussel suse de>]

Ludwig Nussel wrote:
> 802.11x connections that are configured to always prompt for the
> password also always require polkit authentication (bgo#646187).

Here's a potentially embarrassing patch to fix or rather work around
the issue. Improvements welcome, I don't really know the first thing
about NM :-)

After struggling with the code for some hours I think there are
several problems that make this situation complicated to fix though:
- NM does not know whether a secret is (intentionally) not in the
  config file. It just looks at gobject properties that don't know
  why they are at their default value.
- there doesn't seem to be a flag that tells NM whether a connection
  is just being set up for the first time. Ie there is no need to
  ask for auth again until the setup phase is complete.
- I wonder whether the general concept of suddenly prompting the
  user for passwords/passphrases (with potential PK popup) after the
  initial setup phase is a good idea. It might be better and less
  annoying to simply fail the connection attempt and require the
  user to manually use the edit dialog.

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\
 V_/_  http://www.suse.de/
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 

>From 7e3c0930ff594c3e6b08c221dedadc9235006d9f Mon Sep 17 00:00:00 2001
From: Ludwig Nussel <ludwig nussel suse de>
Date: Fri, 7 Oct 2011 13:58:48 +0200
Subject: [PATCH] don't consider not needed secrets for has_system_secrets()

---
 src/settings/nm-agent-manager.c |   56 +++++++++++++++++++++++++++++++++++---
 1 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c
index 5ccbdc6..f3519c3 100644
--- a/src/settings/nm-agent-manager.c
+++ b/src/settings/nm-agent-manager.c
@@ -889,6 +889,11 @@ get_agent_modify_auth_cb (NMAuthChain *chain,
 	nm_auth_chain_unref (chain);
 }
 
+struct system_secrets_cb_data {
+	GHashTable *hash;
+	gboolean *has_system;
+};
+
 static void
 check_system_secrets_cb (NMSetting *setting,
                          const char *key,
@@ -897,11 +902,17 @@ check_system_secrets_cb (NMSetting *setting,
                          gpointer user_data)
 {
 	NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE;
-	gboolean *has_system = user_data;
+	struct system_secrets_cb_data *data = user_data;
+	gboolean has_system = FALSE;
 
 	if (!(flags & NM_SETTING_PARAM_SECRET))
 		return;
 
+	if (!g_hash_table_lookup(data->hash, key)) {
+		nm_log_dbg (LOGD_AGENTS, "%s: %s not needed", __FUNCTION__, key);
+		return;
+	}
+
 	/* Clear out system-owned or always-ask secrets */
 	if (NM_IS_SETTING_VPN (setting) && !strcmp (key, NM_SETTING_VPN_SECRETS)) {
 		GHashTableIter iter;
@@ -913,21 +924,44 @@ check_system_secrets_cb (NMSetting *setting,
 			secret_flags = NM_SETTING_SECRET_FLAG_NONE;
 			nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL);
 			if (secret_flags == NM_SETTING_SECRET_FLAG_NONE)
-				*has_system = TRUE;
+				has_system = TRUE;
 		}
 	} else {
 		nm_setting_get_secret_flags (setting, key, &secret_flags, NULL);
 		if (secret_flags == NM_SETTING_SECRET_FLAG_NONE)
-			*has_system = TRUE;
+			has_system = TRUE;
 	}
+	nm_log_dbg (LOGD_AGENTS, "%s: %s has_system=%d", __FUNCTION__, key, has_system);
+	*data->has_system = has_system;
 }
 
 static gboolean
 has_system_secrets (NMConnection *connection)
 {
 	gboolean has_system = FALSE;
+	GPtrArray *hints = NULL;
+	const char *setting_name;
+	unsigned i;
+	struct system_secrets_cb_data data = {
+		NULL,
+		&has_system,
+	};
+
+	setting_name = nm_connection_need_secrets (connection, &hints);
+	/* some secrets should be needed at this point */
+	g_return_val_if_fail(setting_name != NULL, has_system);
+	g_return_val_if_fail(hints != NULL, has_system);
+
+	data.hash = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+	for (i = 0; i < hints->len; i++) {
+		g_hash_table_insert (data.hash, g_strdup ((const char*)g_ptr_array_index(hints, i)), (void*)(long)1);
+	}
+
+	nm_connection_for_each_setting_value (connection, check_system_secrets_cb, &data);
+
+	g_hash_table_destroy (data.hash);
+	g_ptr_array_free(hints, TRUE);
 
-	nm_connection_for_each_setting_value (connection, check_system_secrets_cb, &has_system);
 	return has_system;
 }
 
@@ -936,20 +970,32 @@ get_next_cb (Request *req)
 {
 	NMSettingConnection *s_con;
 	const char *agent_dbus_owner, *perm;
+	gboolean has_system = FALSE;
 
 	if (!next_generic (req, "getting"))
 		return;
 
 	agent_dbus_owner = nm_secret_agent_get_dbus_owner (NM_SECRET_AGENT (req->current));
 
+	has_system = has_system_secrets (req->connection);
+	nm_log_dbg (LOGD_AGENTS, "flags %d, existing %p, has_system %d",
+			req->flags, req->existing_secrets, has_system);
+
 	/* If the request flags allow user interaction, and there are existing
 	 * system secrets (or blank secrets that are supposed to be system-owned),
 	 * check whether the agent has the 'modify' permission before sending those
 	 * secrets to the agent.  We shouldn't leak system-owned secrets to
 	 * unprivileged users.
 	 */
+	/* XXX: there needs to be a way to determine whether there
+	 * are missing system secrets (ie user clicked on a network
+	 * and wants to connect for the first time). Later we should
+	 * not ask for modifying system secrets. The connection
+	 * should simply fail then. Setting new secrets is a job for
+	 * the connection edit dialog.
+	 */
 	if (   (req->flags != NM_SETTINGS_GET_SECRETS_FLAG_NONE)
-	    && (req->existing_secrets || has_system_secrets (req->connection))) {
+	    && (req->existing_secrets || has_system)) {
 		nm_log_dbg (LOGD_AGENTS, "(%p/%s) request has system secrets; checking agent %s for MODIFY",
 		            req, req->setting_name, agent_dbus_owner);
 
-- 
1.7.3.4