system-settings: protect system connection secrets with PolicyKit

So that normal users who have PolicyKit authorization to edit system connections
can read secrets, move system connection secrets logic into the system connection
service from libnm-glib, and protect it with PolicyKit checks.  Convert the
ifcfg-rh plugin over to using NMSysconfigConnection so that it can take advantage
of the new PolicyKit protection.

libnm-glib/nm-settings.c

@@ -375,103 +375,6 @@ impl_exported_connection_delete (NMExportedConnection *connection,
 	return success;
 }
 
-static GValue *
-string_to_gvalue (const char *str)
-{
-	GValue *val;
-
-	val = g_slice_new0 (GValue);
-	g_value_init (val, G_TYPE_STRING);
-	g_value_set_string (val, str);
-
-	return val;
-}
-
-static void
-copy_one_secret (gpointer key, gpointer value, gpointer user_data)
-{
-	const char *value_str = (const char *) value;
-
-	if (value_str) {
-		g_hash_table_insert ((GHashTable *) user_data,
-		                     g_strdup ((char *) key),
-		                     string_to_gvalue (value_str));
-	}
-}
-
-static void
-add_secrets (NMSetting *setting,
-             const char *key,
-             const GValue *value,
-             GParamFlags flags,
-             gpointer user_data)
-{
-	GHashTable *secrets = user_data;
-
-	if (!(flags & NM_SETTING_PARAM_SECRET))
-		return;
-
-	if (G_VALUE_HOLDS_STRING (value)) {
-		const char *tmp;
-
-		tmp = g_value_get_string (value);
-		if (tmp)
-			g_hash_table_insert (secrets, g_strdup (key), string_to_gvalue (tmp));
-	} else if (G_VALUE_HOLDS (value, DBUS_TYPE_G_MAP_OF_STRING)) {
-		/* Flatten the string hash by pulling its keys/values out */
-		g_hash_table_foreach (g_value_get_boxed (value), copy_one_secret, secrets);
-	}
-}
-
-static void
-destroy_gvalue (gpointer data)
-{
-	GValue *value = (GValue *) data;
-
-	g_value_unset (value);
-	g_slice_free (GValue, value);
-}
-
-static void
-real_get_secrets (NMExportedConnection *exported,
-                  const gchar *setting_name,
-                  const gchar **hints,
-                  gboolean request_new,
-                  DBusGMethodInvocation *context)
-{
-	NMConnection *connection;
-	GError *error = NULL;
-	GHashTable *settings = NULL;
-	GHashTable *secrets = NULL;
-	NMSetting *setting;
-
-	connection = nm_exported_connection_get_connection (exported);
-	setting = nm_connection_get_setting_by_name (connection, setting_name);
-	if (!setting) {
-		g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION,
-		             "%s.%d - Connection didn't have requested setting '%s'.",
-		             __FILE__, __LINE__, setting_name);
-		dbus_g_method_return_error (context, error);
-		g_error_free (error);
-		return;
-	}
-
-	/* Returned secrets are a{sa{sv}}; this is the outer a{s...} hash that
-	 * will contain all the individual settings hashes.
-	 */
-	settings = g_hash_table_new_full (g_str_hash, g_str_equal,
-	                                  g_free, (GDestroyNotify) g_hash_table_destroy);
-
-	/* Add the secrets from this setting to the inner secrets hash for this setting */
-	secrets = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, destroy_gvalue);
-	nm_setting_enumerate_values (setting, add_secrets, secrets);
-
-	g_hash_table_insert (settings, g_strdup (setting_name), secrets);
-
-	dbus_g_method_return (context, settings);
-	g_hash_table_destroy (settings);
-}
-
 static void
 impl_exported_connection_get_secrets (NMExportedConnection *connection,
                                       const gchar *setting_name,
@@ -490,10 +393,16 @@ impl_exported_connection_get_secrets (NMExportedConnection *connection,
 		return;
 	}
 
-	if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets)
+	if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets) {
-		real_get_secrets (connection, setting_name, hints, request_new, context);
+		g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_SECRETS_UNAVAILABLE,
-	else
+		             "%s.%d - Missing implementation for ConnectionSettings::GetSecrets.",
-		EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets (connection, setting_name, hints, request_new, context);
+		             __FILE__, __LINE__);
+		dbus_g_method_return_error (context, error);
+		g_error_free (error);
+		return;
+	}
+
+	EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets (connection, setting_name, hints, request_new, context);
 }
 
 static void
@@ -567,7 +476,6 @@ nm_exported_connection_class_init (NMExportedConnectionClass *exported_connectio
 	object_class->dispose = nm_exported_connection_dispose;
 
 	exported_connection_class->get_settings = real_get_settings;
-	exported_connection_class->service_get_secrets = real_get_secrets;
 
 	/* Properties */
 	g_object_class_install_property

system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c

@@ -40,7 +40,7 @@
 #include "reader.h"
 #include "nm-inotify-helper.h"
 
-G_DEFINE_TYPE (NMIfcfgConnection, nm_ifcfg_connection, NM_TYPE_EXPORTED_CONNECTION)
+G_DEFINE_TYPE (NMIfcfgConnection, nm_ifcfg_connection, NM_TYPE_SYSCONFIG_CONNECTION)
 
 #define NM_IFCFG_CONNECTION_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_IFCFG_CONNECTION, NMIfcfgConnectionPrivate))
 

system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.h

@@ -23,7 +23,8 @@
 
 G_BEGIN_DECLS
 
-#include <nm-settings.h>
+#include <NetworkManager.h>
+#include <nm-sysconfig-connection.h>
 #include "nm-system-config-hal-manager.h"
 
 #define NM_TYPE_IFCFG_CONNECTION            (nm_ifcfg_connection_get_type ())
@@ -38,11 +39,11 @@ G_BEGIN_DECLS
 #define NM_IFCFG_CONNECTION_UDI       "udi"
 
 typedef struct {
-	NMExportedConnection parent;
+	NMSysconfigConnection parent;
 } NMIfcfgConnection;
 
 typedef struct {
-	NMExportedConnectionClass parent;
+	NMSysconfigConnectionClass parent;
 } NMIfcfgConnectionClass;
 
 GType nm_ifcfg_connection_get_type (void);

system-settings/plugins/ifcfg-rh/reader.c

@@ -1337,8 +1337,8 @@ connection_from_file (const char *filename,
 	/* We don't write connections yet */
 	if (connection) {
 		s_con = (NMSettingConnection *) nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION);
-		if (s_con)
+//		if (s_con)
-			g_object_set (s_con, NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL);
+//			g_object_set (s_con, NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL);
 	}
 
 	/* Don't bother reading the connection fully if it's unmanaged */

system-settings/src/dbus-settings.c

@@ -355,11 +355,17 @@ nm_sysconfig_settings_init (NMSysconfigSettings *self)
 {
 	NMSysconfigSettingsPrivate *priv = NM_SYSCONFIG_SETTINGS_GET_PRIVATE (self);
 	char hostname[HOST_NAME_MAX + 2];
+	GError *error = NULL;
 
 	priv->connections = g_hash_table_new_full (g_direct_hash, g_direct_equal, g_object_unref, NULL);
 	priv->unmanaged_devices = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
 
-	priv->pol_ctx = create_polkit_context ();
+	priv->pol_ctx = create_polkit_context (&error);
+	if (!priv->pol_ctx) {
+		g_warning ("%s: failed to create PolicyKit context: %s",
+		           __func__,
+		           (error && error->message) ? error->message : "(unknown)");
+	}
 
 	/* Grab hostname on startup and use that if no plugins provide one */
 	memset (hostname, 0, sizeof (hostname));

system-settings/src/nm-polkit-helpers.c

@@ -62,22 +62,25 @@ pk_io_remove_watch (PolKitContext *pk_context, int watch_id)
 }
 
 PolKitContext *
-create_polkit_context (void)
+create_polkit_context (GError **error)
 {
 	static PolKitContext *global_context = NULL;
-	PolKitError *err;
+	PolKitError *pk_err = NULL;
 
 	if (G_LIKELY (global_context))
 		return polkit_context_ref (global_context);
 
 	global_context = polkit_context_new ();
 	polkit_context_set_io_watch_functions (global_context, pk_io_add_watch, pk_io_remove_watch);
-	err = NULL;
+	if (!polkit_context_init (global_context, &pk_err)) {
-	if (!polkit_context_init (global_context, &err)) {
+		g_set_error (error, NM_SYSCONFIG_SETTINGS_ERROR,
-		g_warning ("Cannot initialize libpolkit: %s",
+		             NM_SYSCONFIG_SETTINGS_ERROR_GENERAL,
-		           err ? polkit_error_get_error_message (err) : "unknown error");
+		             "%s (%d): %s",
-		if (err)
+		             pk_err ? polkit_error_get_error_name (pk_err) : "(unknown)",
-			polkit_error_free (err);
+		             pk_err ? polkit_error_get_error_code (pk_err) : -1,
+		             pk_err ? polkit_error_get_error_message (pk_err) : "(unknown)");
+		if (pk_err)
+			polkit_error_free (pk_err);
 
 		/* PK 0.6's polkit_context_init() unrefs the global_context on failure */
 #if (POLKIT_VERSION_MAJOR == 0) && (POLKIT_VERSION_MINOR >= 7)

system-settings/src/nm-polkit-helpers.h

@@ -28,7 +28,7 @@
 
 #define NM_SYSCONFIG_POLICY_ACTION "org.freedesktop.network-manager-settings.system.modify"
 
-PolKitContext *create_polkit_context   (void);
+PolKitContext *create_polkit_context   (GError **error);
 gboolean       check_polkit_privileges (DBusGConnection *dbus_connection,
 								PolKitContext *pol_ctx,
 								DBusGMethodInvocation *context,

system-settings/src/nm-sysconfig-connection.c

@@ -20,7 +20,9 @@
 
 #include <NetworkManager.h>
 #include "nm-sysconfig-connection.h"
+#include "nm-system-config-error.h"
 #include "nm-polkit-helpers.h"
+#include "nm-dbus-glib-types.h"
 
 G_DEFINE_ABSTRACT_TYPE (NMSysconfigConnection, nm_sysconfig_connection, NM_TYPE_EXPORTED_CONNECTION)
 
@@ -29,6 +31,8 @@ G_DEFINE_ABSTRACT_TYPE (NMSysconfigConnection, nm_sysconfig_connection, NM_TYPE_
 typedef struct {
 	DBusGConnection *dbus_connection;
 	PolKitContext *pol_ctx;
+
+	DBusGProxy *proxy;
 } NMSysconfigConnectionPrivate;
 
 static gboolean
@@ -57,6 +61,231 @@ do_delete (NMExportedConnection *exported, GError **err)
 	return check_polkit_privileges (priv->dbus_connection, priv->pol_ctx, context, err);
 }
 
+static GValue *
+string_to_gvalue (const char *str)
+{
+	GValue *val = g_slice_new0 (GValue);
+
+	g_value_init (val, G_TYPE_STRING);
+	g_value_set_string (val, str);
+	return val;
+}
+
+static void
+copy_one_secret (gpointer key, gpointer value, gpointer user_data)
+{
+	const char *value_str = (const char *) value;
+
+	if (value_str) {
+		g_hash_table_insert ((GHashTable *) user_data,
+		                     g_strdup ((char *) key),
+		                     string_to_gvalue (value_str));
+	}
+}
+
+static void
+add_secrets (NMSetting *setting,
+             const char *key,
+             const GValue *value,
+             GParamFlags flags,
+             gpointer user_data)
+{
+	GHashTable *secrets = user_data;
+
+	if (!(flags & NM_SETTING_PARAM_SECRET))
+		return;
+
+	if (G_VALUE_HOLDS_STRING (value)) {
+		const char *tmp;
+
+		tmp = g_value_get_string (value);
+		if (tmp)
+			g_hash_table_insert (secrets, g_strdup (key), string_to_gvalue (tmp));
+	} else if (G_VALUE_HOLDS (value, DBUS_TYPE_G_MAP_OF_STRING)) {
+		/* Flatten the string hash by pulling its keys/values out */
+		g_hash_table_foreach (g_value_get_boxed (value), copy_one_secret, secrets);
+	}
+}
+
+static void
+destroy_gvalue (gpointer data)
+{
+	GValue *value = (GValue *) data;
+
+	g_value_unset (value);
+	g_slice_free (GValue, value);
+}
+
+static GHashTable *
+real_get_secrets (NMSysconfigConnection *self,
+                  const gchar *setting_name,
+                  const gchar **hints,
+                  gboolean request_new,
+                  GError **error)
+{
+	NMConnection *connection;
+	GHashTable *settings = NULL;
+	GHashTable *secrets = NULL;
+	NMSetting *setting;
+
+	connection = nm_exported_connection_get_connection (NM_EXPORTED_CONNECTION (self));
+	setting = nm_connection_get_setting_by_name (connection, setting_name);
+	if (!setting) {
+		g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION,
+		             "%s.%d - Connection didn't have requested setting '%s'.",
+		             __FILE__, __LINE__, setting_name);
+		return NULL;
+	}
+
+	/* Returned secrets are a{sa{sv}}; this is the outer a{s...} hash that
+	 * will contain all the individual settings hashes.
+	 */
+	settings = g_hash_table_new_full (g_str_hash, g_str_equal,
+	                                  g_free, (GDestroyNotify) g_hash_table_destroy);
+
+	/* Add the secrets from this setting to the inner secrets hash for this setting */
+	secrets = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, destroy_gvalue);
+	nm_setting_enumerate_values (setting, add_secrets, secrets);
+
+	g_hash_table_insert (settings, g_strdup (setting_name), secrets);
+	return settings;
+}
+
+typedef struct {
+	NMSysconfigConnection *self;
+	char *setting_name;
+	DBusGMethodInvocation *context;
+} GetUnixUserInfo;
+
+static GetUnixUserInfo *
+get_unix_user_info_new (NMSysconfigConnection *self,
+                        const char *setting_name,
+                        DBusGMethodInvocation *context)
+{
+	GetUnixUserInfo *info;
+
+	g_return_val_if_fail (self != NULL, NULL);
+	g_return_val_if_fail (setting_name != NULL, NULL);
+	g_return_val_if_fail (context != NULL, NULL);
+
+	info = g_malloc0 (sizeof (GetUnixUserInfo));
+	info->self = self;
+	info->setting_name = g_strdup (setting_name);
+	info->context = context;
+	return info;
+}
+
+static void
+get_unix_user_info_free (gpointer user_data)
+{
+	GetUnixUserInfo *info = user_data;
+
+	g_free (info->setting_name);
+	g_free (info);
+}
+
+static void
+get_unix_user_cb (DBusGProxy *proxy, DBusGProxyCall *call, gpointer user_data)
+{
+	GetUnixUserInfo *info = user_data;
+	NMSysconfigConnection *self;
+	NMSysconfigConnectionPrivate *priv;
+	GError *error = NULL;
+	guint32 requestor_uid = G_MAXUINT32;
+	GHashTable *secrets;
+
+	g_return_if_fail (info != NULL);
+
+	self = info->self;
+	priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (self);
+
+	if (!dbus_g_proxy_end_call (proxy, call, &error, G_TYPE_UINT, &requestor_uid, G_TYPE_INVALID))
+		goto error;
+
+	/* Non-root users need PolicyKit authorization */
+	if (requestor_uid != 0) {
+		if (!check_polkit_privileges (priv->dbus_connection, priv->pol_ctx, info->context, &error))
+			goto error;
+	}
+
+	secrets = real_get_secrets (self, info->setting_name, NULL, FALSE, &error);
+	if (secrets) {
+		/* success; return secrets to caller */
+		dbus_g_method_return (info->context, secrets);
+		g_hash_table_destroy (secrets);
+		return;
+	}
+
+	if (!error) {
+		/* Shouldn't happen, but... */
+		g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR,
+		             NM_SYSCONFIG_SETTINGS_ERROR_GENERAL,
+		             "%s", "Could not get secrets from connection (unknown error ocurred)");
+	}
+
+error:
+	dbus_g_method_return_error (info->context, error);
+	g_clear_error (&error);
+}
+
+static void
+service_get_secrets (NMExportedConnection *exported,
+                     const gchar *setting_name,
+                     const gchar **hints,
+                     gboolean request_new,
+                     DBusGMethodInvocation *context)
+{
+	NMSysconfigConnection *self = NM_SYSCONFIG_CONNECTION (exported);
+	NMSysconfigConnectionPrivate *priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (self);
+	GetUnixUserInfo *info;
+	GError *error = NULL;
+	char *sender = NULL;
+
+	sender = dbus_g_method_get_sender (context);
+	if (!sender) {
+		g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR,
+		             NM_SYSCONFIG_SETTINGS_ERROR_GENERAL,
+		             "%s", "Could not determine D-Bus requestor to authorize GetSecrets request");
+		goto out;
+	}
+
+	if (priv->proxy)
+		g_object_unref (priv->proxy);
+
+	priv->proxy = dbus_g_proxy_new_for_name (priv->dbus_connection,
+	                                         DBUS_SERVICE_DBUS,
+	                                         DBUS_PATH_DBUS,
+	                                         DBUS_INTERFACE_DBUS);
+	if (!priv->proxy) {
+		g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR,
+		             NM_SYSCONFIG_SETTINGS_ERROR_GENERAL,
+		             "%s", "Could not connect to D-Bus to authorize GetSecrets request");
+		goto out;
+	}
+
+	info = get_unix_user_info_new (self, setting_name, context);
+	if (!info) {
+		g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR,
+		             NM_SYSCONFIG_SETTINGS_ERROR_GENERAL,
+		             "%s", "Not enough memory to authorize GetSecrets request");
+		goto out;
+	}
+
+	dbus_g_proxy_begin_call_with_timeout (priv->proxy, "GetConnectionUnixUser",
+	                                      get_unix_user_cb,
+	                                      info,
+	                                      get_unix_user_info_free,
+	                                      5000,
+	                                      G_TYPE_STRING, sender,
+	                                      G_TYPE_INVALID);
+
+out:
+	if (error) {
+		dbus_g_method_return_error (context, error);
+		g_error_free (error);
+	}
+}
+
 /* GObject */
 
 static void
@@ -67,25 +296,35 @@ nm_sysconfig_connection_init (NMSysconfigConnection *self)
 
 	priv->dbus_connection = dbus_g_bus_get (DBUS_BUS_SYSTEM, &err);
 	if (err) {
-		g_warning ("Could not get DBus connection: %s", err->message);
+		g_warning ("%s: error getting D-Bus connection: %s",
+		           __func__,
+		           (err && err->message) ? err->message : "(unknown)");
 		g_error_free (err);
 	}
 
-	priv->pol_ctx = create_polkit_context ();
+	priv->pol_ctx = create_polkit_context (&err);
+	if (!priv->pol_ctx) {
+		g_warning ("%s: error creating PolicyKit context: %s",
+		           __func__,
+		           (err && err->message) ? err->message : "(unknown)");
+	}
 }
 
 static void
-finalize (GObject *object)
+dispose (GObject *object)
 {
 	NMSysconfigConnectionPrivate *priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (object);
 
+	if (priv->proxy)
+		g_object_unref (priv->proxy);
+
 	if (priv->pol_ctx)
 		polkit_context_unref (priv->pol_ctx);
 
 	if (priv->dbus_connection)
 		dbus_g_connection_unref (priv->dbus_connection);
 
-	G_OBJECT_CLASS (nm_sysconfig_connection_parent_class)->finalize (object);
+	G_OBJECT_CLASS (nm_sysconfig_connection_parent_class)->dispose (object);
 }
 
 static void
@@ -97,8 +336,9 @@ nm_sysconfig_connection_class_init (NMSysconfigConnectionClass *sysconfig_connec
 	g_type_class_add_private (sysconfig_connection_class, sizeof (NMSysconfigConnectionPrivate));
 
 	/* Virtual methods */
-	object_class->finalize = finalize;
+	object_class->dispose = dispose;
 
 	connection_class->update = update;
 	connection_class->do_delete = do_delete;
+	connection_class->service_get_secrets = service_get_secrets;
 }

system-settings/src/nm-system-settings.conf

@@ -12,9 +12,9 @@
 
 		<allow send_destination="org.freedesktop.NetworkManagerSystemSettings"/>
 
-		<!-- Only root can get secrets -->
+		<!-- The org.freedesktop.NetworkManagerSettings.Connection.Secrets
-		<deny send_destination="org.freedesktop.NetworkManagerSystemSettings"
+		     interface is secured via PolicyKit.
-		      send_interface="org.freedesktop.NetworkManagerSettings.Connection.Secrets"/>
+		  -->
 	</policy>
 
         <limit name="max_replies_per_connection">512</limit>