2008-09-04 Dan Williams <dcbw@redhat.com>

* libnm-util/nm-setting-vpn.c
	  libnm-util/nm-setting-vpn.h
		- Split VPN secrets from VPN data so that settings services can actually
			figure out that they are secrets and store them accordingly

	* system-settings/plugins/keyfile/nm-keyfile-connection.c
	  system-settings/plugins/keyfile/reader.c
	  system-settings/plugins/keyfile/reader.h
	  system-settings/plugins/keyfile/writer.c
		- Store VPN secrets separately from VPN data so that they can be fetched
			on demand
		- Implement the get_secrets() call so that (a) secrets don't leak out
			to unprivileged callers, and (b) secrets can be sent to privileged
			callers when needed

	* vpn-daemons/vpnc/src/nm-vpnc-service.c
		- Handle split VPN secrets



git-svn-id: http://svn-archive.gnome.org/svn/NetworkManager/trunk@4031 4912f4e0-d625-0410-9fb7-b9a5a253dbdc

ChangeLog

@@ -1,3 +1,23 @@
+2008-09-04  Dan Williams  <dcbw@redhat.com>
+
+	* libnm-util/nm-setting-vpn.c
+	  libnm-util/nm-setting-vpn.h
+		- Split VPN secrets from VPN data so that settings services can actually
+			figure out that they are secrets and store them accordingly
+
+	* system-settings/plugins/keyfile/nm-keyfile-connection.c
+	  system-settings/plugins/keyfile/reader.c
+	  system-settings/plugins/keyfile/reader.h
+	  system-settings/plugins/keyfile/writer.c
+		- Store VPN secrets separately from VPN data so that they can be fetched
+			on demand
+		- Implement the get_secrets() call so that (a) secrets don't leak out
+			to unprivileged callers, and (b) secrets can be sent to privileged
+			callers when needed
+
+	* vpn-daemons/vpnc/src/nm-vpnc-service.c
+		- Handle split VPN secrets
+
 2008-08-27  Dan Williams  <dcbw@redhat.com>
 
 	* system-settings/plugins/ifcfg-fedora/reader.c

libnm-util/nm-setting-vpn.c

@@ -71,6 +71,7 @@ enum {
 	PROP_SERVICE_TYPE,
 	PROP_USER_NAME,
 	PROP_DATA,
+	PROP_SECRETS,
 
 	LAST_PROP
 };
@@ -123,8 +124,17 @@ update_one_secret (NMSetting *setting, const char *key, GValue *value)
 	g_return_if_fail (value != NULL);
 	g_return_if_fail (G_VALUE_HOLDS_STRING (value));
 
-	/* Secrets are really only known to the VPNs themselves. */
+	g_hash_table_insert (self->secrets, g_strdup (key), g_value_dup_string (value));
-	g_hash_table_insert (self->data, g_strdup (key), g_value_dup_string (value));
+}
+
+static void
+destroy_one_secret (gpointer data)
+{
+	char *secret = (char *) data;
+
+	/* Don't leave the secret lying around in memory */
+	memset (secret, 0, strlen (secret));
+	g_free (secret);
 }
 
 static void
@@ -133,6 +143,7 @@ nm_setting_vpn_init (NMSettingVPN *setting)
 	NM_SETTING (setting)->name = g_strdup (NM_SETTING_VPN_SETTING_NAME);
 
 	setting->data = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
+	setting->secrets = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, destroy_one_secret);
 }
 
 static void
@@ -143,6 +154,7 @@ finalize (GObject *object)
 	g_free (self->service_type);
 	g_free (self->user_name);
 	g_hash_table_destroy (self->data);
+	g_hash_table_destroy (self->secrets);
 
 	G_OBJECT_CLASS (nm_setting_vpn_parent_class)->finalize (object);
 }
@@ -158,6 +170,7 @@ set_property (GObject *object, guint prop_id,
 		    const GValue *value, GParamSpec *pspec)
 {
 	NMSettingVPN *setting = NM_SETTING_VPN (object);
+	GHashTable *new_hash;
 
 	switch (prop_id) {
 	case PROP_SERVICE_TYPE:
@@ -171,7 +184,16 @@ set_property (GObject *object, guint prop_id,
 	case PROP_DATA:
 		/* Must make a deep copy of the hash table here... */
 		g_hash_table_remove_all (setting->data);
-		g_hash_table_foreach (g_value_get_boxed (value), copy_hash, setting->data);
+		new_hash = g_value_get_boxed (value);
+		if (new_hash)
+			g_hash_table_foreach (new_hash, copy_hash, setting->data);
+		break;
+	case PROP_SECRETS:
+		/* Must make a deep copy of the hash table here... */
+		g_hash_table_remove_all (setting->secrets);
+		new_hash = g_value_get_boxed (value);
+		if (new_hash)
+			g_hash_table_foreach (new_hash, copy_hash, setting->secrets);
 		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
@@ -195,6 +217,9 @@ get_property (GObject *object, guint prop_id,
 	case PROP_DATA:
 		g_value_set_boxed (value, setting->data);
 		break;
+	case PROP_SECRETS:
+		g_value_set_boxed (value, setting->secrets);
+		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
 		break;
@@ -238,4 +263,13 @@ nm_setting_vpn_class_init (NMSettingVPNClass *setting_class)
 							   "VPN Service specific data",
 							   DBUS_TYPE_G_MAP_OF_STRING,
 							   G_PARAM_READWRITE | NM_SETTING_PARAM_SERIALIZE));
+
+	g_object_class_install_property
+		(object_class, PROP_SECRETS,
+		 _nm_param_spec_specialized (NM_SETTING_VPN_SECRETS,
+							   "Secrets",
+							   "VPN Service specific secrets",
+							   DBUS_TYPE_G_MAP_OF_STRING,
+							   G_PARAM_READWRITE | NM_SETTING_PARAM_SERIALIZE | NM_SETTING_PARAM_SECRET));
 }
+

libnm-util/nm-setting-vpn.h

@@ -55,6 +55,7 @@ GQuark nm_setting_vpn_error_quark (void);
 #define NM_SETTING_VPN_SERVICE_TYPE "service-type"
 #define NM_SETTING_VPN_USER_NAME    "user-name"
 #define NM_SETTING_VPN_DATA         "data"
+#define NM_SETTING_VPN_SECRETS      "secrets"
 
 typedef struct {
 	NMSetting parent;
@@ -72,9 +73,18 @@ typedef struct {
 	 * a char * -> char * mapping, and both the key
 	 * and value are owned by the hash table, and should
 	 * be allocated with functions whose value can be
-	 * freed with g_free()
+	 * freed with g_free().  Should not contain secrets.
 	 */
 	GHashTable *data;
+
+	/* The hash table is created at setting object
+	 * init time and should not be replaced.  It is
+	 * a char * -> char * mapping, and both the key
+	 * and value are owned by the hash table, and should
+	 * be allocated with functions whose value can be
+	 * freed with g_free().  Should contain secrets only.
+	 */
+	GHashTable *secrets;
 } NMSettingVPN;
 
 typedef struct {

src/vpn-manager/nm-vpn-connection.c

@@ -658,13 +658,12 @@ static void
 update_vpn_properties_secrets (gpointer key, gpointer data, gpointer user_data)
 {
 	NMConnection *connection = NM_CONNECTION (user_data);
+	GHashTable *secrets = (GHashTable *) data;
 
 	if (strcmp (key, NM_SETTING_VPN_SETTING_NAME))
 		return;
 
-	nm_connection_update_secrets (connection,
+	nm_connection_update_secrets (connection, NM_SETTING_VPN_SETTING_NAME, secrets);
-	                              NM_SETTING_VPN_SETTING_NAME,
-	                              (GHashTable *) data);
 }
 
 static void

system-settings/plugins/keyfile/nm-keyfile-connection.c

@@ -6,6 +6,7 @@
 #include <nm-setting-connection.h>
 #include <nm-utils.h>
 
+#include "nm-dbus-glib-types.h"
 #include "nm-keyfile-connection.h"
 #include "reader.h"
 #include "writer.h"
@@ -49,6 +50,137 @@ get_settings (NMExportedConnection *exported)
 	return nm_connection_to_hash (nm_exported_connection_get_connection (exported));
 }
 
+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)
+{
+	g_hash_table_insert ((GHashTable *) user_data,
+	                     g_strdup ((char *) key),
+	                     string_to_gvalue (value));
+}
+
+static void
+add_secrets (NMSetting *setting,
+             const char *key,
+             const GValue *value,
+             gboolean secret,
+             gpointer user_data)
+{
+	GHashTable *secrets = user_data;
+
+	if (!secret)
+		return;
+
+	if (G_VALUE_HOLDS_STRING (value)) {
+		g_hash_table_insert (secrets, g_strdup (key), string_to_gvalue (g_value_get_string (value)));
+	} 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);
+	} else
+		g_message ("%s: unhandled secret %s type %s", __func__, key, G_VALUE_TYPE_NAME (value));
+}
+
+static void
+destroy_gvalue (gpointer data)
+{
+	GValue *value = (GValue *) data;
+
+	g_value_unset (value);
+	g_slice_free (GValue, value);
+}
+
+static GHashTable *
+extract_secrets (NMKeyfileConnection *exported,
+                 const char *setting_name,
+                 GError **error)
+{
+	NMKeyfileConnectionPrivate *priv = NM_KEYFILE_CONNECTION_GET_PRIVATE (exported);
+	NMConnection *tmp;
+	GHashTable *secrets;
+	NMSetting *setting;
+
+	tmp = connection_from_file (priv->filename, TRUE);
+	if (!tmp) {
+		g_set_error (error, NM_SETTINGS_ERROR, 1,
+		             "%s.%d - Could not read secrets from file %s.",
+		             __FILE__, __LINE__, priv->filename);
+		return NULL;
+	}
+
+	setting = nm_connection_get_setting_by_name (tmp, setting_name);
+	if (!setting) {
+		g_object_unref (tmp);
+		g_set_error (error, NM_SETTINGS_ERROR, 1,
+		             "%s.%d - Could not read secrets from file %s.",
+		             __FILE__, __LINE__, priv->filename);
+		return NULL;
+	}
+
+	/* Add the secrets from this setting to the secrets hash */
+	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_object_unref (tmp);
+
+	return secrets;
+}
+
+static void
+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, 1,
+		             "%s.%d - Connection didn't have requested setting '%s'.",
+		             __FILE__, __LINE__, setting_name);
+		goto error;
+	}
+
+	/* 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);
+
+	/* Read in a temporary connection and just extract the secrets */
+	secrets = extract_secrets (NM_KEYFILE_CONNECTION (exported), setting_name, &error);
+	if (!secrets)
+		goto error;
+
+	g_hash_table_insert (settings, g_strdup (setting_name), secrets);
+
+	dbus_g_method_return (context, settings);
+	g_hash_table_destroy (settings);
+	return;
+
+error:
+	nm_warning ("%s", error->message);
+	dbus_g_method_return_error (context, error);
+	g_error_free (error);
+}
+
 static gboolean
 update (NMExportedConnection *exported,
         GHashTable *new_settings,
@@ -118,7 +250,7 @@ constructor (GType type,
 		goto err;
 	}
 
-	wrapped = connection_from_file (priv->filename);
+	wrapped = connection_from_file (priv->filename, FALSE);
 	if (!wrapped)
 		goto err;
 
@@ -205,6 +337,7 @@ nm_keyfile_connection_class_init (NMKeyfileConnectionClass *keyfile_connection_c
 	object_class->finalize     = finalize;
 
 	connection_class->get_settings = get_settings;
+	connection_class->get_secrets  = get_secrets;
 	connection_class->update       = update;
 	connection_class->delete       = delete;
 

system-settings/plugins/keyfile/reader.c

@@ -315,6 +315,11 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key)
 	g_strfreev (keys);
 }
 
+typedef struct {
+	GKeyFile *keyfile;
+	gboolean secrets;
+} ReadSettingInfo;
+
 static void
 read_one_setting_value (NMSetting *setting,
 				    const char *key,
@@ -322,7 +327,8 @@ read_one_setting_value (NMSetting *setting,
 				    gboolean secret,
 				    gpointer user_data)
 {
-	GKeyFile *file = (GKeyFile *) user_data;
+	ReadSettingInfo *info = (ReadSettingInfo *) user_data;
+	GKeyFile *file = info->keyfile;
 	GType type;
 	GError *err = NULL;
 	gboolean check_for_key = TRUE;
@@ -331,9 +337,15 @@ read_one_setting_value (NMSetting *setting,
 	if (!strcmp (key, NM_SETTING_NAME))
 		return;
 
-	/* IPv4 addresses don't have the exact key name */
+	/* Don't read in secrets unless we want to */
+	if (secret && !info->secrets)
+		return;
+
+	/* IPv4 addresses and VPN properties don't have the exact key name */
 	if (NM_IS_SETTING_IP4_CONFIG (setting) && !strcmp (key, NM_SETTING_IP4_CONFIG_ADDRESSES))
 		check_for_key = FALSE;
+	else if (NM_IS_SETTING_VPN (setting))
+		check_for_key = FALSE;
 
 	if (check_for_key && !g_key_file_has_key (file, setting->name, key, &err)) {
 		if (err) {
@@ -407,7 +419,7 @@ read_one_setting_value (NMSetting *setting,
 		g_object_set (setting, key, array, NULL);
 		g_byte_array_free (array, TRUE);
 		g_free (tmp);
- 	} else if (type == dbus_g_type_get_collection ("GSList", G_TYPE_STRING)) {
+ 	} else if (type == DBUS_TYPE_G_LIST_OF_STRING) {
 		gchar **sa;
 		gsize length;
 		int i;
@@ -422,7 +434,7 @@ read_one_setting_value (NMSetting *setting,
 
 		g_slist_free (list);
 		g_strfreev (sa);
-	} else if (type == dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_STRING)) {
+	} else if (type == DBUS_TYPE_G_MAP_OF_STRING) {
 		read_hash_of_string (file, setting, key);
 	} else if (type == DBUS_TYPE_G_UINT_ARRAY) {
 		if (!read_array_of_uint (file, setting, key)) {
@@ -441,21 +453,41 @@ read_one_setting_value (NMSetting *setting,
 }
 
 static NMSetting *
-read_setting (GKeyFile *file, const char *name)
+read_setting (GKeyFile *file, const char *name, gboolean secrets)
 {
 	NMSetting *setting;
+	ReadSettingInfo info;
+
+	info.keyfile = file;
+	info.secrets = secrets;
 
 	setting = nm_connection_create_setting (name);
-	if (setting) {
+	if (setting)
-		nm_setting_enumerate_values (setting, read_one_setting_value, file);
+		nm_setting_enumerate_values (setting, read_one_setting_value, &info);
-	} else
+	else
 		g_warning ("Invalid setting name '%s'", name);
 
 	return setting;
 }
 
+static void
+read_vpn_secrets (GKeyFile *file, NMSettingVPN *s_vpn)
+{
+	char **keys, **iter;
+
+	keys = g_key_file_get_keys (file, VPN_SECRETS_GROUP, NULL, NULL);
+	for (iter = keys; *iter; iter++) {
+		char *secret;
+
+		secret = g_key_file_get_string (file, VPN_SECRETS_GROUP, *iter, NULL);
+		if (secret)
+			g_hash_table_insert (s_vpn->secrets, g_strdup (*iter), secret);
+	}
+	g_strfreev (keys);
+}
+
 NMConnection *
-connection_from_file (const char *filename)
+connection_from_file (const char *filename, gboolean secrets)
 {
 	GKeyFile *key_file;
 	struct stat statbuf;
@@ -479,6 +511,7 @@ connection_from_file (const char *filename)
 		gchar **groups;
 		gsize length;
 		int i;
+		gboolean vpn_secrets = FALSE;
 
 		connection = nm_connection_new ();
 
@@ -486,11 +519,26 @@ connection_from_file (const char *filename)
 		for (i = 0; i < length; i++) {
 			NMSetting *setting;
 
-			setting = read_setting (key_file, groups[i]);
+			/* Only read out secrets when needed */
+			if (!strcmp (groups[i], VPN_SECRETS_GROUP)) {
+				vpn_secrets = TRUE;
+				continue;
+			}
+
+			setting = read_setting (key_file, groups[i], secrets);
 			if (setting)
 				nm_connection_add_setting (connection, setting);
 		}
 
+		/* Handle vpn secrets after the 'vpn' setting was read */
+		if (secrets && vpn_secrets) {
+			NMSettingVPN *s_vpn;
+
+			s_vpn = (NMSettingVPN *) nm_connection_get_setting (connection, NM_TYPE_SETTING_VPN);
+			if (s_vpn)
+				read_vpn_secrets (key_file, s_vpn);
+		}
+
 		g_strfreev (groups);
 	} else {
 		g_warning ("Error parsing file '%s': %s", filename, err->message);

system-settings/plugins/keyfile/reader.h

@@ -3,9 +3,11 @@
 #ifndef _KEYFILE_PLUGIN_READER_H
 #define _KEYFILE_PLUGIN_READER_H
 
+#define VPN_SECRETS_GROUP "vpn-secrets"
+
 #include <glib.h>
 #include <nm-connection.h>
 
-NMConnection *connection_from_file (const char *filename);
+NMConnection *connection_from_file (const char *filename, gboolean secrets);
 
 #endif /* _KEYFILE_PLUGIN_READER_H */

system-settings/plugins/keyfile/writer.c

@@ -14,6 +14,7 @@
 
 #include "nm-dbus-glib-types.h"
 #include "writer.h"
+#include "reader.h"
 
 static gboolean
 write_array_of_uint (GKeyFile *file,
@@ -154,10 +155,6 @@ write_hash_of_string_helper (gpointer key, gpointer data, gpointer user_data)
 	const char *property = (const char *) key;
 	const char *value = (const char *) data;
 
-	if (   !strcmp (info->setting_name, NM_SETTING_VPN_SETTING_NAME)
-	    && !strcmp (property, NM_SETTING_VPN_SERVICE_TYPE))
-		return;
-
 	g_key_file_set_string (info->file,
 	                       info->setting_name,
 	                       property,
@@ -174,7 +171,14 @@ write_hash_of_string (GKeyFile *file,
 	WriteStringHashInfo info;
 
 	info.file = file;
-	info.setting_name = setting->name;
+
+	/* Write VPN secrets out to a different group to keep them separate */
+	if (   (G_OBJECT_TYPE (setting) == NM_TYPE_SETTING_VPN)
+	    && !strcmp (key, NM_SETTING_VPN_SECRETS)) {
+		info.setting_name = VPN_SECRETS_GROUP;
+	} else
+		info.setting_name = setting->name;
+
 	g_hash_table_foreach (hash, write_hash_of_string_helper, &info);
 }
 

vpn-daemons/vpnc/src/nm-vpnc-service.c

@@ -321,6 +321,7 @@ static gboolean
 nm_vpnc_config_write (gint vpnc_fd,
                       const char *default_user_name,
                       GHashTable *properties,
+                      GHashTable *secrets,
                       GError **error)
 {
 	WriteConfigInfo *info;
@@ -354,6 +355,7 @@ nm_vpnc_config_write (gint vpnc_fd,
 	info = g_malloc0 (sizeof (WriteConfigInfo));
 	info->fd = vpnc_fd;
 	g_hash_table_foreach (properties, write_one_property, info);
+	g_hash_table_foreach (secrets, write_one_property, info);
 	*error = info->error;
 	g_free (info);
 
@@ -373,12 +375,14 @@ real_connect (NMVPNPlugin   *plugin,
 	g_assert (s_vpn);
 	if (!nm_vpnc_properties_validate (s_vpn->data, error))
 		goto out;
+	if (!nm_vpnc_properties_validate (s_vpn->secrets, error))
+		goto out;
 
 	vpnc_fd = nm_vpnc_start_vpnc_binary (NM_VPNC_PLUGIN (plugin), error);
 	if (vpnc_fd < 0)
 		goto out;
 
-	if (!nm_vpnc_config_write (vpnc_fd, s_vpn->user_name, s_vpn->data, error))
+	if (!nm_vpnc_config_write (vpnc_fd, s_vpn->user_name, s_vpn->data, s_vpn->secrets, error))
 		goto out;
 
 	success = TRUE;
@@ -412,11 +416,11 @@ real_need_secrets (NMVPNPlugin *plugin,
 
 	// FIXME: there are some configurations where both passwords are not
 	// required.  Make sure they work somehow.
-	if (!g_hash_table_lookup (s_vpn->data, NM_VPNC_KEY_SECRET)) {
+	if (!g_hash_table_lookup (s_vpn->secrets, NM_VPNC_KEY_SECRET)) {
 		*setting_name = NM_SETTING_VPN_SETTING_NAME;
 		return TRUE;
 	}
-	if (!g_hash_table_lookup (s_vpn->data, NM_VPNC_KEY_XAUTH_PASSWORD)) {
+	if (!g_hash_table_lookup (s_vpn->secrets, NM_VPNC_KEY_XAUTH_PASSWORD)) {
 		*setting_name = NM_SETTING_VPN_SETTING_NAME;
 		return TRUE;
 	}