From 09412869c10547a09d69012b54864a652f1f41d6 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Sun, 22 Feb 2009 17:16:09 -0500 Subject: [PATCH] system-settings: implement common GetSettings and GetSecrets methods (rh #486696) Fix a few problems... No plugin should return secrets in the GetSettings method, which some plugins did. When that was committed in the commit "system-settings: don't return secrets in the settings", it broke those plugins that didn't implement GetSecrets. Each plugin can actually use the same code for GetSettings and GetSecrets, so implement those generically in the NMExportedConnection class and remove plugin-specific implementations that all did the same thing. --- libnm-glib/nm-settings.c | 142 ++++++++++++++-- .../plugins/ifcfg-rh/nm-ifcfg-connection.c | 10 -- .../plugins/ifcfg-suse/nm-suse-connection.c | 10 -- .../plugins/ifupdown/nm-ifupdown-connection.c | 59 +------ system-settings/plugins/keyfile/io/reader.c | 34 ++-- system-settings/plugins/keyfile/io/reader.h | 2 +- .../plugins/keyfile/nm-keyfile-connection.c | 151 +----------------- .../plugins/keyfile/tests/test-keyfile.c | 8 +- 8 files changed, 150 insertions(+), 266 deletions(-) diff --git a/libnm-glib/nm-settings.c b/libnm-glib/nm-settings.c index d4f1b86ec6..33e13e8b02 100644 --- a/libnm-glib/nm-settings.c +++ b/libnm-glib/nm-settings.c @@ -255,10 +255,32 @@ nm_exported_connection_new (NMConnection *wrapped) NULL); } +static GHashTable * +real_get_settings (NMExportedConnection *exported) +{ + NMExportedConnectionPrivate *priv; + NMConnection *no_secrets; + GHashTable *hash; + + g_return_val_if_fail (NM_IS_EXPORTED_CONNECTION (exported), NULL); + + priv = NM_EXPORTED_CONNECTION_GET_PRIVATE (exported); + + /* Secrets should *never* be returned by the GetSettings method, they + * get returned by the GetSecrets method which can be better + * protected against leakage of secrets to unprivileged callers. + */ + no_secrets = nm_connection_duplicate (priv->wrapped); + nm_connection_clear_secrets (no_secrets); + hash = nm_connection_to_hash (no_secrets); + g_object_unref (G_OBJECT (no_secrets)); + return hash; +} + static gboolean impl_exported_connection_get_settings (NMExportedConnection *connection, - GHashTable **settings, - GError **error) + GHashTable **settings, + GError **error) { NMExportedConnectionPrivate *priv; @@ -267,7 +289,7 @@ impl_exported_connection_get_settings (NMExportedConnection *connection, priv = NM_EXPORTED_CONNECTION_GET_PRIVATE (connection); if (!EXPORTED_CONNECTION_CLASS (connection)->get_settings) - *settings = nm_connection_to_hash (priv->wrapped); + *settings = real_get_settings (connection); else *settings = EXPORTED_CONNECTION_CLASS (connection)->get_settings (connection); @@ -351,6 +373,103 @@ 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, @@ -369,16 +488,10 @@ impl_exported_connection_get_secrets (NMExportedConnection *connection, return; } - if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets) { - g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_SECRETS_UNAVAILABLE, - "%s.%d - Missing implementation for ConnectionSettings::GetSecrets.", - __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); + if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets) + real_get_secrets (connection, setting_name, hints, request_new, context); + else + EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets (connection, setting_name, hints, request_new, context); } static void @@ -451,6 +564,9 @@ nm_exported_connection_class_init (NMExportedConnectionClass *exported_connectio object_class->get_property = get_property; 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 (object_class, PROP_CONNECTION, diff --git a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c index 96829375bb..6fda6facf4 100644 --- a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c +++ b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c @@ -316,15 +316,6 @@ nm_ifcfg_connection_get_unmanaged (NMIfcfgConnection *self) return NM_IFCFG_CONNECTION_GET_PRIVATE (self)->unmanaged; } -static GHashTable * -get_settings (NMExportedConnection *exported) -{ - NMConnection *connection = nm_exported_connection_get_connection (exported); - - nm_connection_clear_secrets (connection); - return nm_connection_to_hash (connection); -} - static gboolean update (NMExportedConnection *exported, GHashTable *new_settings, GError **error) { @@ -448,7 +439,6 @@ nm_ifcfg_connection_class_init (NMIfcfgConnectionClass *ifcfg_connection_class) object_class->get_property = get_property; object_class->finalize = finalize; - connection_class->get_settings = get_settings; connection_class->update = update; connection_class->do_delete = do_delete; diff --git a/system-settings/plugins/ifcfg-suse/nm-suse-connection.c b/system-settings/plugins/ifcfg-suse/nm-suse-connection.c index 4cc92d18e7..d2949df12f 100644 --- a/system-settings/plugins/ifcfg-suse/nm-suse-connection.c +++ b/system-settings/plugins/ifcfg-suse/nm-suse-connection.c @@ -123,15 +123,6 @@ nm_suse_connection_new (const char *iface, NMDeviceType dev_type) return exported; } -static GHashTable * -get_settings (NMExportedConnection *exported) -{ - NMConnection *connection = nm_exported_connection_get_connection (exported); - - nm_connection_clear_secrets (connection); - return nm_connection_to_hash (connection); -} - static gboolean update (NMExportedConnection *exported, GHashTable *new_settings, @@ -190,7 +181,6 @@ nm_suse_connection_class_init (NMSuseConnectionClass *suse_connection_class) /* Virtual methods */ object_class->finalize = finalize; - connection_class->get_settings = get_settings; connection_class->update = update; connection_class->do_delete = do_delete; } diff --git a/system-settings/plugins/ifupdown/nm-ifupdown-connection.c b/system-settings/plugins/ifupdown/nm-ifupdown-connection.c index d90c8f4143..0b37e738a5 100644 --- a/system-settings/plugins/ifupdown/nm-ifupdown-connection.c +++ b/system-settings/plugins/ifupdown/nm-ifupdown-connection.c @@ -67,15 +67,6 @@ nm_ifupdown_connection_new (if_block *block) NULL); } -static GHashTable * -get_settings (NMExportedConnection *exported) -{ - NMConnection *connection = nm_exported_connection_get_connection (exported); - - nm_connection_clear_secrets (connection); - return nm_connection_to_hash (connection); -} - static gboolean update (NMExportedConnection *exported, GHashTable *new_settings, @@ -188,7 +179,6 @@ nm_ifupdown_connection_class_init (NMIfupdownConnectionClass *ifupdown_connectio object_class->get_property = get_property; object_class->finalize = finalize; - connection_class->get_settings = get_settings; connection_class->update = update; connection_class->do_delete = do_delete; connection_class->service_get_secrets = service_get_secrets; @@ -209,54 +199,14 @@ service_get_secrets (NMExportedConnection *exported, gboolean request_new, DBusGMethodInvocation *context) { - NMConnection *connection; GError *error = NULL; - GHashTable *settings = NULL; - GHashTable *secrets = NULL; - NMSetting *setting; PLUGIN_PRINT ("SCPlugin-Ifupdown", "get_secrets for setting_name:'%s')", setting_name); - 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); - PLUGIN_PRINT ("SCPlugin-Ifupdown", "%s", error->message); - dbus_g_method_return_error (context, error); - g_error_free (error); - return; - } - - settings = g_hash_table_new_full (g_str_hash, g_str_equal, - g_free, (GDestroyNotify) g_hash_table_destroy); - - if (!settings) { - g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INTERNAL_ERROR, - "%s.%d - failed to hash setting (OOM?)", - __FILE__, __LINE__); - dbus_g_method_return_error (context, error); - g_error_free (error); - return; - } - - if (!strcmp (setting_name, NM_SETTING_WIRELESS_SECURITY_SETTING_NAME)) { - secrets = nm_setting_to_hash (setting); - if (secrets) { - g_hash_table_insert(settings, g_strdup(setting_name), secrets); - dbus_g_method_return (context, settings); - } else { - g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INTERNAL_ERROR, - "%s.%d - nm_setting_to_hash failed (OOM?)", - __FILE__, __LINE__); - dbus_g_method_return_error (context, error); - g_error_free (error); - g_hash_table_destroy (settings); - } - } else { + /* FIXME: Only wifi secrets are supported for now */ + if (!strcmp (setting_name, NM_SETTING_WIRELESS_SECURITY_SETTING_NAME)) + NM_EXPORTED_CONNECTION_CLASS (nm_ifupdown_connection_parent_class)->service_get_secrets (exported, setting_name, hints, request_new, context); + else { g_set_error (&error, NM_SETTING_WIRELESS_SECURITY_ERROR, 1, "%s.%d - security setting name not supported '%s'.", __FILE__, __LINE__, setting_name); @@ -265,3 +215,4 @@ service_get_secrets (NMExportedConnection *exported, g_error_free (error); } } + diff --git a/system-settings/plugins/keyfile/io/reader.c b/system-settings/plugins/keyfile/io/reader.c index afa5418489..e7f360dc5f 100644 --- a/system-settings/plugins/keyfile/io/reader.c +++ b/system-settings/plugins/keyfile/io/reader.c @@ -398,11 +398,6 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key) } -typedef struct { - GKeyFile *keyfile; - gboolean secrets; -} ReadSettingInfo; - typedef struct { const char *setting_name; const char *key; @@ -450,8 +445,7 @@ read_one_setting_value (NMSetting *setting, GParamFlags flags, gpointer user_data) { - ReadSettingInfo *info = (ReadSettingInfo *) user_data; - GKeyFile *file = info->keyfile; + GKeyFile *file = user_data; const char *setting_name; GType type; GError *err = NULL; @@ -466,10 +460,6 @@ read_one_setting_value (NMSetting *setting, if (!strcmp (key, NM_SETTING_NAME)) return; - /* Don't read in secrets unless we want to */ - if ((flags & NM_SETTING_PARAM_SECRET) && !info->secrets) - return; - /* Don't read the NMSettingConnection object's 'read-only' property */ if ( NM_IS_SETTING_CONNECTION (setting) && !strcmp (key, NM_SETTING_CONNECTION_READ_ONLY)) @@ -606,17 +596,13 @@ read_one_setting_value (NMSetting *setting, } static NMSetting * -read_setting (GKeyFile *file, const char *name, gboolean secrets) +read_setting (GKeyFile *file, const char *name) { NMSetting *setting; - ReadSettingInfo info; - - info.keyfile = file; - info.secrets = secrets; setting = nm_connection_create_setting (name); if (setting) - nm_setting_enumerate_values (setting, read_one_setting_value, &info); + nm_setting_enumerate_values (setting, read_one_setting_value, (gpointer) file); else g_warning ("Invalid setting name '%s'", name); @@ -642,7 +628,7 @@ read_vpn_secrets (GKeyFile *file, NMSettingVPN *s_vpn) } NMConnection * -connection_from_file (const char *filename, gboolean secrets) +connection_from_file (const char *filename) { GKeyFile *key_file; struct stat statbuf; @@ -656,10 +642,10 @@ connection_from_file (const char *filename, gboolean secrets) bad_owner = getuid () != statbuf.st_uid; bad_permissions = statbuf.st_mode & 0077; - if (bad_owner || bad_permissions) { - g_warning ("Ignorning insecure configuration file '%s'", filename); - return NULL; - } + if (bad_owner || bad_permissions) { + g_warning ("Ignorning insecure configuration file '%s'", filename); + return NULL; + } key_file = g_key_file_new (); if (g_key_file_load_from_file (key_file, filename, G_KEY_FILE_NONE, &err)) { @@ -680,13 +666,13 @@ connection_from_file (const char *filename, gboolean secrets) continue; } - setting = read_setting (key_file, groups[i], secrets); + setting = read_setting (key_file, groups[i]); if (setting) nm_connection_add_setting (connection, setting); } /* Handle vpn secrets after the 'vpn' setting was read */ - if (secrets && vpn_secrets) { + if (vpn_secrets) { NMSettingVPN *s_vpn; s_vpn = (NMSettingVPN *) nm_connection_get_setting (connection, NM_TYPE_SETTING_VPN); diff --git a/system-settings/plugins/keyfile/io/reader.h b/system-settings/plugins/keyfile/io/reader.h index 5c40462335..beac866a25 100644 --- a/system-settings/plugins/keyfile/io/reader.h +++ b/system-settings/plugins/keyfile/io/reader.h @@ -27,6 +27,6 @@ #include #include -NMConnection *connection_from_file (const char *filename, gboolean secrets); +NMConnection *connection_from_file (const char *filename); #endif /* _KEYFILE_PLUGIN_READER_H */ diff --git a/system-settings/plugins/keyfile/nm-keyfile-connection.c b/system-settings/plugins/keyfile/nm-keyfile-connection.c index dc355069ce..9d9427b340 100644 --- a/system-settings/plugins/keyfile/nm-keyfile-connection.c +++ b/system-settings/plugins/keyfile/nm-keyfile-connection.c @@ -64,153 +64,6 @@ nm_keyfile_connection_get_filename (NMKeyfileConnection *self) return NM_KEYFILE_CONNECTION_GET_PRIVATE (self)->filename; } -static GHashTable * -get_settings (NMExportedConnection *exported) -{ - NMConnection *connection = nm_exported_connection_get_connection (exported); - - nm_connection_clear_secrets (connection); - return nm_connection_to_hash (connection); -} - -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); - } 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, NM_SETTINGS_ERROR_SECRETS_UNAVAILABLE, - "%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, NM_SETTINGS_ERROR_SECRETS_UNAVAILABLE, - "%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 -service_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); - 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, @@ -283,7 +136,7 @@ constructor (GType type, goto err; } - wrapped = connection_from_file (priv->filename, FALSE); + wrapped = connection_from_file (priv->filename); if (!wrapped) goto err; @@ -373,8 +226,6 @@ nm_keyfile_connection_class_init (NMKeyfileConnectionClass *keyfile_connection_c object_class->get_property = get_property; object_class->finalize = finalize; - connection_class->get_settings = get_settings; - connection_class->service_get_secrets = service_get_secrets; connection_class->update = update; connection_class->do_delete = do_delete; diff --git a/system-settings/plugins/keyfile/tests/test-keyfile.c b/system-settings/plugins/keyfile/tests/test-keyfile.c index bd1ee02af3..6f482b3965 100644 --- a/system-settings/plugins/keyfile/tests/test-keyfile.c +++ b/system-settings/plugins/keyfile/tests/test-keyfile.c @@ -67,7 +67,7 @@ test_read_valid_wired_connection (void) const char *expected_address2_gw = "1.2.1.1"; NMIP4Address *ip4_addr; - connection = connection_from_file (TEST_WIRED_FILE, TRUE); + connection = connection_from_file (TEST_WIRED_FILE); ASSERT (connection != NULL, "connection-read", "failed to read %s", TEST_WIRED_FILE); @@ -432,7 +432,7 @@ test_write_wired_connection (void) "connection-write", "didn't get keyfile name back after writing connection"); /* Read the connection back in and compare it to the one we just wrote out */ - reread = connection_from_file (testfile, TRUE); + reread = connection_from_file (testfile); ASSERT (reread != NULL, "connection-write", "failed to re-read test connection"); ASSERT (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT) == TRUE, @@ -462,7 +462,7 @@ test_read_valid_wireless_connection (void) const guint64 expected_timestamp = 1226604314; guint64 timestamp; - connection = connection_from_file (TEST_WIRELESS_FILE, TRUE); + connection = connection_from_file (TEST_WIRELESS_FILE); ASSERT (connection != NULL, "connection-read", "failed to read %s", TEST_WIRELESS_FILE); @@ -652,7 +652,7 @@ test_write_wireless_connection (void) "connection-write", "didn't get keyfile name back after writing connection"); /* Read the connection back in and compare it to the one we just wrote out */ - reread = connection_from_file (testfile, TRUE); + reread = connection_from_file (testfile); ASSERT (reread != NULL, "connection-write", "failed to re-read test connection"); ASSERT (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT) == TRUE,