From 7771473f46d30127075ebd5166d504a6197d8cdd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 11:28:27 +0100 Subject: [PATCH] libnm,core: add _nm_connection_aggregate() to replace nm_connection_for_each_setting_value() We should no longer use nm_connection_for_each_setting_value() and nm_setting_for_each_value(). It's fundamentally broken as it does not work with properties that are not backed by a GObject property and it cannot be fixed because it is public API. Add an internal function _nm_connection_aggregate() to replace it. Compare the implementation of the aggregation functionality inside libnm with the previous two checks for secret-flags that it replaces: - previous approach broke abstraction and require detailed knowledge of secret flags. Meaning, they must special case NMSettingVpn and GObject-property based secrets. If we implement a new way for implementing secrets (like we will need for WireGuard), then this the new way should only affect libnm-core, not require changes elsewhere. - it's very inefficient to itereate over all settings. It involves cloning and sorting the list of settings, and retrieve and clone all GObject properties. Only to look at secret properties alone. _nm_connection_aggregate() is supposed to be more flexible then just the two new aggregate types that perform a "find-any" search. The @arg argument and boolean return value can suffice to implement different aggregation types in the future. Also fixes the check of NMAgentManager for secret flags for VPNs (NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs is a property that either has a secret or a secret-flag. The previous implementation would only look at present secrets and check their flags. It wouldn't check secret-flags that are NM_SETTING_SECRET_FLAG_NONE, but have no secret. --- libnm-core/nm-connection.c | 67 ++++++++++++++++++++++++++ libnm-core/nm-core-internal.h | 20 ++++++++ libnm-core/nm-setting-private.h | 7 +++ libnm-core/nm-setting-vpn.c | 60 +++++++++++++++++++++++ libnm-core/nm-setting.c | 69 +++++++++++++++++++++++++++ libnm-core/tests/test-general.c | 51 ++++++++++++++++++-- src/settings/nm-agent-manager.c | 46 +----------------- src/settings/nm-settings-connection.c | 34 +------------ 8 files changed, 274 insertions(+), 80 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index eb42bc07a4..9e5de7b267 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -2033,6 +2033,73 @@ nm_connection_for_each_setting_value (NMConnection *connection, nm_setting_enumerate_values (settings[i], func, user_data); } +/** + * _nm_connection_aggregate: + * @connecition: the #NMConnection for which values are to be aggregated. + * @type: one of the supported aggrate types. + * @arg: the input/output argument that depends on @type. + * + * For example, with %NM_CONNECTION_AGGREGATE_ANY_SECRETS and + * %NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS @arg is a boolean + * output argument. It is either %NULL or a pointer to an gboolean + * out-argument. The function will always set @arg if given. + * Also, the return value of the function is likewise the result + * that is set to @arg. + * + * Returns: a boolean result with the meaning depending on the aggregation + * type @type. + */ +gboolean +_nm_connection_aggregate (NMConnection *connection, + NMConnectionAggregateType type, + gpointer arg) +{ + NMConnectionPrivate *priv; + GHashTableIter iter; + NMSetting *setting; + gboolean arg_boolean; + gboolean completed_early; + gpointer my_arg; + + g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); + + switch (type) { + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + arg_boolean = FALSE; + my_arg = &arg_boolean; + goto good; + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + arg_boolean = FALSE; + my_arg = &arg_boolean; + goto good; + } + g_return_val_if_reached (FALSE); + +good: + priv = NM_CONNECTION_GET_PRIVATE (connection); + + completed_early = FALSE; + g_hash_table_iter_init (&iter, priv->settings); + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &setting)) { + if (_nm_setting_aggregate (setting, type, my_arg)) { + completed_early = TRUE; + break; + } + nm_assert ( my_arg != &arg_boolean + || !arg_boolean); + } + + if (my_arg == &arg_boolean) { + nm_assert (completed_early == arg_boolean); + if (arg) + *((gboolean *) arg) = arg_boolean; + return arg_boolean; + } + + nm_assert_not_reached (); + return FALSE; +} + /** * nm_connection_dump: * @connection: the #NMConnection diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index ec9b108318..d661e49010 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -146,6 +146,26 @@ gpointer _nm_connection_check_main_setting (NMConnection *connection, const char *setting_name, GError **error); +typedef enum { + /* whether the connection has any secrets. + * + * @arg may be %NULL or a pointer to a gboolean for the result. The return + * value of _nm_connection_aggregate() is likewise the boolean result. */ + NM_CONNECTION_AGGREGATE_ANY_SECRETS, + + /* whether the connection has any secret with flags NM_SETTING_SECRET_FLAG_NONE. + * Note that this only cares about the flags, not whether the secret is actually + * present. + * + * @arg may be %NULL or a pointer to a gboolean for the result. The return + * value of _nm_connection_aggregate() is likewise the boolean result. */ + NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, +} NMConnectionAggregateType; + +gboolean _nm_connection_aggregate (NMConnection *connection, + NMConnectionAggregateType type, + gpointer arg); + /** * NMSettingVerifyResult: * @NM_SETTING_VERIFY_SUCCESS: the setting verifies successfully diff --git a/libnm-core/nm-setting-private.h b/libnm-core/nm-setting-private.h index c5a11b7a02..49d27336fa 100644 --- a/libnm-core/nm-setting-private.h +++ b/libnm-core/nm-setting-private.h @@ -97,6 +97,13 @@ gboolean _nm_setting_verify_secret_string (const char *str, const char *property, GError **error); +gboolean _nm_setting_aggregate (NMSetting *setting, + NMConnectionAggregateType type, + gpointer arg); +gboolean _nm_setting_vpn_aggregate (NMSettingVpn *setting, + NMConnectionAggregateType type, + gpointer arg); + gboolean _nm_setting_slave_type_is_valid (const char *slave_type, const char **out_port_type); GVariant *_nm_setting_to_dbus (NMSetting *setting, diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index 754c9555b8..5d488e7f07 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -461,6 +461,66 @@ nm_setting_vpn_foreach_secret (NMSettingVpn *setting, foreach_item_helper (setting, TRUE, func, user_data); } +gboolean +_nm_setting_vpn_aggregate (NMSettingVpn *setting, + NMConnectionAggregateType type, + gpointer arg) +{ + NMSettingVpnPrivate *priv; + NMSettingSecretFlags secret_flags; + const char *key_name; + GHashTableIter iter; + + g_return_val_if_fail (NM_IS_SETTING_VPN (setting), FALSE); + + priv = NM_SETTING_VPN_GET_PRIVATE (setting); + + switch (type) { + + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + if (g_hash_table_size (priv->secrets) > 0) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + return FALSE; + + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + + g_hash_table_iter_init (&iter, priv->secrets); + while (g_hash_table_iter_next (&iter, (gpointer *) &key_name, NULL)) { + if (!nm_setting_get_secret_flags (NM_SETTING (setting), key_name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + } + + /* Ok, we have no secrets with system-secret flags. + * But do we have any secret-flags (without secrets) that indicate system secrets? */ + g_hash_table_iter_init (&iter, priv->data); + while (g_hash_table_iter_next (&iter, (gpointer *) &key_name, NULL)) { + gs_free char *secret_name = NULL; + + if (!g_str_has_suffix (key_name, "-flags")) + continue; + secret_name = g_strndup (key_name, strlen (key_name) - NM_STRLEN ("-flags")); + if (secret_name[0] == '\0') + continue; + if (!nm_setting_get_secret_flags (NM_SETTING (setting), secret_name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + } + + return FALSE; + } + + g_return_val_if_reached (FALSE); +} + /** * nm_setting_vpn_get_timeout: * @setting: the #NMSettingVpn diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 1a3e80b499..8c4fea5045 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1650,6 +1650,75 @@ nm_setting_enumerate_values (NMSetting *setting, } } +/** + * _nm_setting_aggregate: + * @setting: the #NMSetting to aggregate. + * @type: the #NMConnectionAggregateType aggregate type. + * @arg: the in/out arguments for aggregation. They depend on @type. + * + * This is the implementation detail of _nm_connection_aggregate(). It + * makes no sense to call this function directly outside of _nm_connection_aggregate(). + * + * Returns: %TRUE if afterwards the aggregation is complete. That means, + * the only caller _nm_connection_aggregate() will not visit other settings + * after a setting returns %TRUE (indicating that there is nothing further + * to aggregate). Note that is very different from the boolean return + * argument of _nm_connection_aggregate(), which serves a different purpose. + */ +gboolean +_nm_setting_aggregate (NMSetting *setting, + NMConnectionAggregateType type, + gpointer arg) +{ + const NMSettInfoSetting *sett_info; + guint i; + + g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); + g_return_val_if_fail (arg, FALSE); + g_return_val_if_fail (NM_IN_SET (type, NM_CONNECTION_AGGREGATE_ANY_SECRETS, + NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS), + FALSE); + + if (NM_IS_SETTING_VPN (setting)) + return _nm_setting_vpn_aggregate (NM_SETTING_VPN (setting), type, arg); + + sett_info = _nm_sett_info_setting_get (NM_SETTING_GET_CLASS (setting)); + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + nm_auto_unset_gvalue GValue value = G_VALUE_INIT; + NMSettingSecretFlags secret_flags; + + if (!prop_spec) + continue; + if (!NM_FLAGS_HAS (prop_spec->flags, NM_SETTING_PARAM_SECRET)) + continue; + + switch (type) { + + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (prop_spec)); + g_object_get_property (G_OBJECT (setting), prop_spec->name, &value); + if (!g_param_value_defaults (prop_spec, &value)) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + break; + + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + if (!nm_setting_get_secret_flags (setting, prop_spec->name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + break; + + } + } + + return FALSE; +} + /** * _nm_setting_clear_secrets: * @setting: the #NMSetting diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 91f116de43..b6dd532616 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -755,10 +755,12 @@ vpn_check_empty_func (const char *key, const char *value, gpointer user_data) static void test_setting_vpn_items (void) { - gs_unref_object NMSettingVpn *s_vpn = NULL; + gs_unref_object NMConnection *connection = NULL; + NMSettingVpn *s_vpn; - s_vpn = (NMSettingVpn *) nm_setting_vpn_new (); - g_assert (s_vpn); + connection = nmtst_create_minimal_connection ("vpn-items", NULL, NM_SETTING_VPN_SETTING_NAME, NULL); + + s_vpn = nm_connection_get_setting_vpn (connection); nm_setting_vpn_add_data_item (s_vpn, "foobar1", "blahblah1"); nm_setting_vpn_add_data_item (s_vpn, "foobar2", "blahblah2"); @@ -772,7 +774,14 @@ test_setting_vpn_items (void) nm_setting_vpn_remove_data_item (s_vpn, "foobar3"); nm_setting_vpn_remove_data_item (s_vpn, "foobar4"); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_add_secret (s_vpn, "foobar1", "blahblah1"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_add_secret (s_vpn, "foobar2", "blahblah2"); nm_setting_vpn_add_secret (s_vpn, "foobar3", "blahblah3"); nm_setting_vpn_add_secret (s_vpn, "foobar4", "blahblah4"); @@ -782,8 +791,25 @@ test_setting_vpn_items (void) nm_setting_vpn_remove_secret (s_vpn, "foobar1"); nm_setting_vpn_remove_secret (s_vpn, "foobar2"); nm_setting_vpn_remove_secret (s_vpn, "foobar3"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_add_data_item (s_vpn, "foobar4-flags", "blahblah4"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_add_data_item (s_vpn, "foobar4-flags", "2"); + + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_remove_secret (s_vpn, "foobar4"); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_remove_data_item (s_vpn, "foobar4-flags"); + /* Try to add some blank values and make sure they are rejected */ NMTST_EXPECT_LIBNM_CRITICAL (NMTST_G_RETURN_MSG (key != NULL)); nm_setting_vpn_add_data_item (s_vpn, NULL, NULL); @@ -1632,6 +1658,25 @@ test_connection_to_dbus_setting_name (void) s_wsec = make_test_wsec_setting ("connection-to-dbus-setting-name"); nm_connection_add_setting (connection, NM_SETTING (s_wsec)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + g_object_set (s_wsec, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NULL); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + g_object_set (s_wsec, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, NM_SETTING_SECRET_FLAG_NONE, + NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NONE, + NULL); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); /* Make sure the keys of the first level dict are setting names, not diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 953d3ce459..edadee14bf 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -1058,49 +1058,6 @@ _con_get_request_start_validated (NMAuthChain *chain, nm_auth_chain_destroy (chain); } -static void -has_system_secrets_check (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) -{ - NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; - gboolean *has_system = user_data; - - if (!(flags & NM_SETTING_PARAM_SECRET)) - return; - - /* Clear out system-owned or always-ask secrets */ - if (NM_IS_SETTING_VPN (setting) && !strcmp (key, NM_SETTING_VPN_SECRETS)) { - GHashTableIter iter; - const char *secret_name = NULL; - - /* VPNs are special; need to handle each secret separately */ - g_hash_table_iter_init (&iter, (GHashTable *) g_value_get_boxed (value)); - while (g_hash_table_iter_next (&iter, (gpointer *) &secret_name, NULL)) { - 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; - } - } else { - if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL)) - g_return_if_reached (); - if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) - *has_system = TRUE; - } -} - -static gboolean -has_system_secrets (NMConnection *connection) -{ - gboolean has_system = FALSE; - - nm_connection_for_each_setting_value (connection, has_system_secrets_check, &has_system); - return has_system; -} - static void _con_get_request_start (Request *req) { @@ -1121,7 +1078,8 @@ _con_get_request_start (Request *req) * unprivileged users. */ if ( (req->con.get.flags != NM_SECRET_AGENT_GET_SECRETS_FLAG_NONE) - && (req->con.get.existing_secrets || has_system_secrets (req->con.connection))) { + && ( req->con.get.existing_secrets + || _nm_connection_aggregate (req->con.connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL))) { _LOGD (NULL, "("LOG_REQ_FMT") request has system secrets; checking agent %s for MODIFY", LOG_REQ_ARG (req), agent_dbus_owner); diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 392823f399..0beb5ea770 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1659,38 +1659,6 @@ typedef struct { bool is_update2:1; } UpdateInfo; -static void -has_some_secrets_cb (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) -{ - GParamSpec *pspec; - - if (NM_IS_SETTING_VPN (setting)) { - if (nm_setting_vpn_get_num_secrets (NM_SETTING_VPN(setting))) - *((gboolean *) user_data) = TRUE; - return; - } - - pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), key); - if (pspec) { - if ( (flags & NM_SETTING_PARAM_SECRET) - && !g_param_value_defaults (pspec, (GValue *)value)) - *((gboolean *) user_data) = TRUE; - } -} - -static gboolean -any_secrets_present (NMConnection *self) -{ - gboolean has_secrets = FALSE; - - nm_connection_for_each_setting_value (self, has_some_secrets_cb, &has_secrets); - return has_secrets; -} - static void cached_secrets_to_connection (NMSettingsConnection *self, NMConnection *connection) { @@ -1758,7 +1726,7 @@ update_auth_cb (NMSettingsConnection *self, } if (info->new_settings) { - if (!any_secrets_present (info->new_settings)) { + if (!_nm_connection_aggregate (info->new_settings, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)) { /* If the new connection has no secrets, we do not want to remove all * secrets, rather we keep all the existing ones. Do that by merging * them in to the new connection.