From 5f5641d304f1ac00066e8a716c99ae7f3688d330 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Apr 2022 17:41:33 +0200 Subject: [PATCH 1/4] ifcfg-rh/trivial: add fixme comments about lossy write/read of properties --- .../settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 48c4cae906..69344d9991 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -397,7 +397,9 @@ write_8021x_setting(NMConnection *connection, if (wired) svSetValueStr(ifcfg, "KEY_MGMT", "IEEE8021X"); - /* EAP method */ + /* EAP method + * + * FIXME(ifcfg-full-cycle): persist all values of eap-method. */ if (nm_setting_802_1x_get_num_eap_methods(s_8021x)) { value = nm_setting_802_1x_get_eap_method(s_8021x, 0); if (value) @@ -455,8 +457,10 @@ write_8021x_setting(NMConnection *connection, value = "allow-auth"; else if (strcmp(value, "3") == 0) value = "allow-unauth allow-auth"; - else + else { + /* FIXME(ifcfg-full-cycle): does not handle the value "0". */ value = NULL; + } } svSetValueStr(ifcfg, "IEEE_8021X_FAST_PROVISIONING", value); @@ -503,6 +507,8 @@ write_8021x_setting(NMConnection *connection, str = g_string_new(NULL); num = nm_setting_802_1x_get_num_altsubject_matches(s_8021x); for (i = 0; i < num; i++) { + /* FIXME(ifcfg-full-cycle): this cannot handle values with spaces, which + * are not rejected by nm_connection_verify(). */ if (i > 0) g_string_append_c(str, ' '); match = nm_setting_802_1x_get_altsubject_match(s_8021x, i); @@ -515,6 +521,8 @@ write_8021x_setting(NMConnection *connection, str = g_string_new(NULL); num = nm_setting_802_1x_get_num_phase2_altsubject_matches(s_8021x); for (i = 0; i < num; i++) { + /* FIXME(ifcfg-full-cycle): this cannot handle values with spaces, which + * are not rejected by nm_connection_verify(). */ if (i > 0) g_string_append_c(str, ' '); match = nm_setting_802_1x_get_phase2_altsubject_match(s_8021x, i); -- GitLab From 91cbbd99b999c2972be0798c996fa9b86b09eae6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Apr 2022 17:54:25 +0200 Subject: [PATCH 2/4] ifcfg-rh: move code around in write_8021x_setting() Makes more sense, to not interrupt the construction of the phase2_auth string. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 69344d9991..551f2b912e 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -464,6 +464,14 @@ write_8021x_setting(NMConnection *connection, } svSetValueStr(ifcfg, "IEEE_8021X_FAST_PROVISIONING", value); + auth_flags = nm_setting_802_1x_get_phase1_auth_flags(s_8021x); + if (auth_flags != NM_SETTING_802_1X_AUTH_FLAGS_NONE) { + svSetValueEnum(ifcfg, + "IEEE_8021X_PHASE1_AUTH_FLAGS", + nm_setting_802_1x_auth_flags_get_type(), + auth_flags); + } + /* Phase2 auth methods */ phase2_auth = g_string_new(NULL); @@ -484,14 +492,6 @@ write_8021x_setting(NMConnection *connection, g_free(tmp); } - auth_flags = nm_setting_802_1x_get_phase1_auth_flags(s_8021x); - if (auth_flags != NM_SETTING_802_1X_AUTH_FLAGS_NONE) { - svSetValueEnum(ifcfg, - "IEEE_8021X_PHASE1_AUTH_FLAGS", - nm_setting_802_1x_auth_flags_get_type(), - auth_flags); - } - svSetValueStr(ifcfg, "IEEE_8021X_INNER_AUTH_METHODS", phase2_auth->len ? phase2_auth->str : NULL); -- GitLab From 445e78377196d5b5321397594afed0b541fce052 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Apr 2022 17:46:38 +0200 Subject: [PATCH 3/4] libnm: fix printing NULL value in NMSetting8021x.verify() --- src/libnm-core-impl/nm-setting-8021x.c | 85 ++++++++++++++++++-------- 1 file changed, 60 insertions(+), 25 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index dce35cf016..8ea3acb87a 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2825,11 +2825,18 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } if (!NM_IN_STRSET(priv->phase1_peapver, NULL, "0", "1")) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->phase1_peapver); + if (priv->phase1_peapver) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->phase1_peapver); + } else { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, @@ -2838,11 +2845,18 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } if (!NM_IN_STRSET(priv->phase1_peaplabel, NULL, "0", "1")) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->phase1_peaplabel); + if (priv->phase1_peaplabel) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->phase1_peaplabel); + } else { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, @@ -2851,11 +2865,18 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } if (!NM_IN_STRSET(priv->phase1_fast_provisioning, NULL, "0", "1", "2", "3")) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->phase1_fast_provisioning); + if (priv->phase1_fast_provisioning) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->phase1_fast_provisioning); + } else { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, @@ -2885,11 +2906,18 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) "otp", "md5", "tls")) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->phase2_auth); + if (priv->phase2_auth) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->phase2_auth); + } else { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, @@ -2898,11 +2926,18 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } if (!NM_IN_STRSET(priv->phase2_autheap, NULL, "md5", "mschapv2", "otp", "gtc", "tls")) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->phase2_autheap); + if (priv->phase2_autheap) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->phase2_autheap); + } else { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, -- GitLab From 915e92392816a2fdcc574b63a4e8de1c7ae84b1b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Apr 2022 17:00:26 +0200 Subject: [PATCH 4/4] libnm: normalize empty strings in 802-1x setting Supplicant does not allow setting certain properties to empty values. It also does not make sense. Also, ifcfg-rh writer uses svSetValueStr() for these properties, so the ifcfg plugin would always loose having hte values set to "". Also, you couldn't enter these strings in nmcli. It's fair to assume that it makes no sense to have these values set to an empty value. Since we cannot just tighten up verification to reject them, normalize them. It also seems that some GUI now starts setting domain_suffix_match to an empty string. Or maybe it was always doing it, and ifcfg plugin just hid the problem? Anyway, we have users out there who set these properties to "". https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/973 --- src/libnm-core-impl/nm-connection.c | 61 ++++++++++++++++++++++++++ src/libnm-core-impl/nm-setting-8021x.c | 44 +++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index a9b130a792..27cea2cb2b 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -1641,6 +1641,66 @@ _normalize_gsm_auto_config(NMConnection *self) return TRUE; } +static gboolean +_normalize_802_1x_empty_strings(NMConnection *self) +{ + NMSetting8021x *s_8021x; + gboolean changed = FALSE; + + s_8021x = _connection_get_setting_by_meta_type(NM_CONNECTION_GET_PRIVATE(self), + NM_META_SETTING_TYPE_802_1X); + if (!s_8021x) + return FALSE; + +#define _norm_8021x(s_8021x, getter, prop_name, p_changed) \ + G_STMT_START \ + { \ + NMSetting8021x *_s_8021x = (s_8021x); \ + gboolean *_p_changed = (p_changed); \ + const char *_v; \ + \ + _v = getter(_s_8021x); \ + if (_v && _v[0] == '\0') { \ + g_object_set(_s_8021x, "" prop_name "", NULL, NULL); \ + *(_p_changed) = TRUE; \ + } \ + } \ + G_STMT_END + + _norm_8021x(s_8021x, nm_setting_802_1x_get_identity, NM_SETTING_802_1X_IDENTITY, &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_anonymous_identity, + NM_SETTING_802_1X_ANONYMOUS_IDENTITY, + &changed); + _norm_8021x(s_8021x, nm_setting_802_1x_get_pac_file, NM_SETTING_802_1X_PAC_FILE, &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_subject_match, + NM_SETTING_802_1X_SUBJECT_MATCH, + &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_phase2_subject_match, + NM_SETTING_802_1X_PHASE2_SUBJECT_MATCH, + &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_domain_suffix_match, + NM_SETTING_802_1X_DOMAIN_SUFFIX_MATCH, + &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_phase2_domain_suffix_match, + NM_SETTING_802_1X_PHASE2_DOMAIN_SUFFIX_MATCH, + &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_domain_match, + NM_SETTING_802_1X_DOMAIN_MATCH, + &changed); + _norm_8021x(s_8021x, + nm_setting_802_1x_get_phase2_domain_match, + NM_SETTING_802_1X_PHASE2_DOMAIN_MATCH, + &changed); + + return changed; +} + static gboolean _normalize_required_settings(NMConnection *self) { @@ -1952,6 +2012,7 @@ _connection_normalize(NMConnection *connection, was_modified |= _normalize_bridge_vlan_order(connection); was_modified |= _normalize_bridge_port_vlan_order(connection); was_modified |= _normalize_gsm_auto_config(connection); + was_modified |= _normalize_802_1x_empty_strings(connection); was_modified = !!was_modified; diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index 8ea3acb87a..41feae57a9 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2980,6 +2980,50 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) error)) return FALSE; + /* normalizable warnings from here on. */ + +#define _check_strempty_and_return(priv, prop_name, field, error) \ + G_STMT_START \ + { \ + NMSetting8021xPrivate *_priv = (priv); \ + GError **_error = (error); \ + \ + if (_priv->field && _priv->field[0] == '\0') { \ + g_set_error(_error, \ + NM_CONNECTION_ERROR, \ + NM_CONNECTION_ERROR_INVALID_PROPERTY, \ + _("property is empty")); \ + g_prefix_error(_error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, "" prop_name ""); \ + return NM_SETTING_VERIFY_NORMALIZABLE; \ + } \ + } \ + G_STMT_END + + _check_strempty_and_return(priv, NM_SETTING_802_1X_IDENTITY, identity, error); + _check_strempty_and_return(priv, + NM_SETTING_802_1X_ANONYMOUS_IDENTITY, + anonymous_identity, + error); + _check_strempty_and_return(priv, NM_SETTING_802_1X_PAC_FILE, pac_file, error); + _check_strempty_and_return(priv, NM_SETTING_802_1X_SUBJECT_MATCH, subject_match, error); + _check_strempty_and_return(priv, + NM_SETTING_802_1X_PHASE2_SUBJECT_MATCH, + phase2_subject_match, + error); + _check_strempty_and_return(priv, + NM_SETTING_802_1X_DOMAIN_SUFFIX_MATCH, + domain_suffix_match, + error); + _check_strempty_and_return(priv, + NM_SETTING_802_1X_PHASE2_DOMAIN_SUFFIX_MATCH, + phase2_domain_suffix_match, + error); + _check_strempty_and_return(priv, NM_SETTING_802_1X_DOMAIN_MATCH, domain_match, error); + _check_strempty_and_return(priv, + NM_SETTING_802_1X_PHASE2_DOMAIN_MATCH, + phase2_domain_match, + error); + return TRUE; } -- GitLab