fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-02-06 01:10:33 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

libnm,core: add _nm_connection_aggregate() to replace nm_connection_for_each_setting_value()

Browse source 
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.
This commit is contained in:
Thomas Haller 2019-01-04 11:28:27 +01:00
parent 4a5514dc0f
commit 7771473f46
8 changed files with 274 additions and 80 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

67
libnm-core/nm-connection.c
View file

@ -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

20
libnm-core/nm-core-internal.h
View file

@ -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

7
libnm-core/nm-setting-private.h
View file

@ -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,

60
libnm-core/nm-setting-vpn.c
View file

@ -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

69
libnm-core/nm-setting.c
View file

@ -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

51
libnm-core/tests/test-general.c
View file

@ -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

46
src/settings/nm-agent-manager.c
View file

@ -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);

34
src/settings/nm-settings-connection.c
View file

@ -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.