From 41be0c8fdece9e392e3ad03e1c0c113ef0bd0ee0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Jul 2021 09:38:41 +0200 Subject: [PATCH] ifcfg: log messages about invalid an unrecognized lines in ifcfg files Problems of this patch: - the code does not differentiate between an ifcfg file and an alias file. Different shell variables are honored however depending on the context and the warning should reflect that. - there are no warnings about /etc/sysconfig/network. The main problem is that we read this file for every ifcfg file we parse, and we would need to ratelimit the number of warnings. Another problem is that the file likely contains keys that we intentionally don't support. We would need a new way to omit warnings about those lines. Example: TYPE=Ethernet PROXY_METHOD=none BROWSER_ONLY=no BOOTPROTO=dhcp DEFROUTE=yes STABLE_ID=$'xxx\xF4yy' IPV4_FAILURE_FATAL=no IPV6INIT=yes XX=foo XX1=foo' ' IPV6_AUTOCONF=yes xxxx IPV6_DEFROUTE=yesx IPV6_DEFROUTE=yes IPV6_FAILURE_FATAL=no IPV6_ADDR_GEN_MODE=stable-privacy NAME=xxx UUID=9d8ed7ff-3cdd-4336-9e26-3e978dc87102 ONBOOT=no [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:6: key STABLE_ID does not contain valid UTF-8 and is treated as "" [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:9: key XX is unknown and ignored [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:10: key XX1 is badly quoted and is treated as "" [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:11: invalid line ignored [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:12: key IPV6_AUTOCONF is badly quoted and is treated as "" [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:13: key IPV6_DEFROUTE is duplicated and the early occurrence ignored https://bugzilla.redhat.com/show_bug.cgi?id=1959656 --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 15 +++- src/core/settings/plugins/ifcfg-rh/shvar.c | 85 +++++++++++++++++++ src/core/settings/plugins/ifcfg-rh/shvar.h | 2 + .../ifcfg-test-wifi-wpa-eap-suite-b-192-tls | 1 - .../ifcfg-test-wifi-wpa-eap-ttls-tls | 1 - .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 11 +++ 6 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index fd5e54d197..a98fa0f9a8 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2154,6 +2154,8 @@ read_aliases(NMSettingIPConfig *s_ip4, gboolean read_defroute, const char *filen continue; } + svWarnInvalid(parsed, "alias", _NMLOG_DOMAIN); + device = svGetValueStr(parsed, "DEVICE", &device_value); if (!device) { PARSE_WARNING("alias file '%s' has no DEVICE", full_path); @@ -6302,6 +6304,7 @@ connection_from_file_full(const char *filename, NMSetting * s_ip4; NMSetting * s_ip6; const char * ifcfg_name = NULL; + gs_free char * s_tmp = NULL; gboolean has_ip4_defroute = FALSE; gboolean has_complex_routes_v4; gboolean has_complex_routes_v6; @@ -6329,8 +6332,6 @@ connection_from_file_full(const char *filename, if (!main_ifcfg) return NULL; - network_ifcfg = svOpenFile(network_file, NULL); - if (!svGetValueBoolean(main_ifcfg, "NM_CONTROLLED", TRUE)) { connection = create_unhandled_connection(filename, main_ifcfg, "unmanaged", out_unhandled); if (!connection) { @@ -6344,6 +6345,16 @@ connection_from_file_full(const char *filename, return g_steal_pointer(&connection); } + if (NM_IN_STRSET(svGetValueStr(main_ifcfg, "DEVICE", &s_tmp), "lo")) { + /* "lo" is not handled by NetworkManager and we ignore it. */ + } else + svWarnInvalid(main_ifcfg, "ifcfg", _NMLOG_DOMAIN); + nm_clear_g_free(&s_tmp); + + network_ifcfg = svOpenFile(network_file, NULL); + /* we don't call svWarnInvalid(network_ifcfg), because we will load this file for + * every profile. So we would get a large number of duplicate warnings. */ + /* iBFT is handled by nm-initrd-generator during boot. */ bootproto = svGetValueStr_cp(main_ifcfg, "BOOTPROTO"); if (bootproto && !g_ascii_strcasecmp(bootproto, "ibft")) { diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c index 7b83c094a3..5f9cd2f57b 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.c +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c @@ -1523,6 +1523,91 @@ svUnsetValue(shvarFile *s, const char *key) /*****************************************************************************/ +void +svWarnInvalid(shvarFile *s, const char *file_type, NMLogDomain log_domain) +{ + shvarLine *line; + gsize n; + + if (!nm_logging_enabled(LOGL_WARN, log_domain)) + return; + + n = 0; + c_list_for_each_entry (line, &s->lst_head, lst) { + gs_free char *s_tmp = NULL; + + n++; + + if (!line->key) { + const char *str; + + nm_assert(line->line); + str = nm_str_skip_leading_spaces(line->line); + if (!NM_IN_SET(str[0], '\0', '#')) { + nm_log_warn(log_domain, + "ifcfg-rh: %s,%s:%zu: invalid line ignored", + file_type, + s->fileName, + n); + } + continue; + } + + if (g_hash_table_lookup(s->lst_idx, line) != line) { + nm_log_warn( + log_domain, + "ifcfg-rh: %s,%s:%zu: key %s is duplicated and the early occurrence ignored", + file_type, + s->fileName, + n, + line->key); + continue; + } + + if (!line->line) { + /* the line is deleted via svUnsetValue(). Ignore. */ + continue; + } + + if (!svUnescape(line->line, &s_tmp)) { + if (!svUnescape_full(line->line, &s_tmp, FALSE)) { + nm_log_warn(log_domain, + "ifcfg-rh: %s,%s:%zu: key %s is badly quoted and is treated as \"\"", + file_type, + s->fileName, + n, + line->key); + } else { + nm_log_warn(log_domain, + "ifcfg-rh: %s,%s:%zu: key %s does not contain valid UTF-8 and is " + "treated as \"\"", + file_type, + s->fileName, + n, + line->key); + } + continue; + } + + /* TODO: we read different shell scripts, and whether a key is recognized + * depends on the type. For example, alias files only accept a subset of + * known keys. + * + * Basically, depending on the @file_type, different keys are valid. */ + if (!nms_ifcfg_rh_utils_is_well_known_key(line->key)) { + nm_log_dbg(log_domain, + "ifcfg-rh: %s,%s:%zu: key %s is unknown and ignored", + file_type, + s->fileName, + n, + line->key); + continue; + } + } +} + +/*****************************************************************************/ + /* Write the current contents iff modified. Returns FALSE on error * and TRUE on success. Do not write if no values have been modified. * The mode argument is only used if creating the file, not if diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.h b/src/core/settings/plugins/ifcfg-rh/shvar.h index 49101669ef..cf91642fbc 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.h +++ b/src/core/settings/plugins/ifcfg-rh/shvar.h @@ -121,4 +121,6 @@ _nm_auto_shvar_file_close(shvarFile **p_s) } #define nm_auto_shvar_file_close nm_auto(_nm_auto_shvar_file_close) +void svWarnInvalid(shvarFile *s, const char *file_type, NMLogDomain log_domain); + #endif /* _SHVAR_H */ diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls index 9a74bb4d9b..a21c301c62 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls +++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls @@ -3,7 +3,6 @@ DEVICE=eth2 HWADDR=00:16:41:11:22:33 BOOTPROTO=dhcp ONBOOT=yes -ONBOOT=yes USERCTL=yes IPV6INIT=no NM_CONTROLLED=yes diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls index 42ed1d68a5..bb63d5e842 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls +++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls @@ -4,7 +4,6 @@ DEVICE=eth2 HWADDR=00:16:41:11:22:33 BOOTPROTO=dhcp ONBOOT=yes -ONBOOT=yes USERCTL=yes IPV6INIT=no NM_CONTROLLED=yes diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index f0c6c05396..8d07b15608 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -738,10 +738,13 @@ test_read_variables_corner_cases(void) const char * mac; char expected_mac_address[ETH_ALEN] = {0x00, 0x16, 0x41, 0x11, 0x22, 0x33}; + NMTST_EXPECT_NM_WARN("*key NAME is badly quoted and is treated as \"\"*"); + NMTST_EXPECT_NM_WARN("*key ZONE is badly quoted and is treated as \"\"*"); connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-variables-corner-cases-1", NULL, TYPE_ETHERNET, NULL); + g_test_assert_expected_messages(); /* ===== CONNECTION SETTING ===== */ s_con = nm_connection_get_setting_connection(connection); @@ -830,10 +833,12 @@ test_read_unrecognized(void) gs_free char * unhandled_spec = NULL; guint64 expected_timestamp = 0; + NMTST_EXPECT_NM_WARN("*key NAME is badly quoted and is treated as \"\"*"); connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-unrecognized", NULL, NULL, &unhandled_spec); + g_test_assert_expected_messages(); g_assert_cmpstr(unhandled_spec, ==, "unrecognized:mac:00:11:22:33"); /* ===== CONNECTION SETTING ===== */ @@ -1004,10 +1009,12 @@ test_read_wired_dhcp(void) char expected_mac_address[ETH_ALEN] = {0x00, 0x11, 0x22, 0x33, 0x44, 0xee}; const char * mac; + NMTST_EXPECT_NM_WARN("*key IPV6INIT is duplicated and the early occurrence ignored*"); connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wired-dhcp", NULL, TYPE_ETHERNET, &unmanaged); + g_test_assert_expected_messages(); g_assert(unmanaged == NULL); /* ===== CONNECTION SETTING ===== */ @@ -3583,10 +3590,12 @@ test_read_wifi_wpa_eap_tls(void) char * unmanaged = NULL; const char * expected_privkey_password = "test1"; + NMTST_EXPECT_NM_WARN("*key ONBOOT is duplicated and the early occurrence ignored*"); connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wifi-wpa-eap-tls", NULL, TYPE_ETHERNET, &unmanaged); + g_test_assert_expected_messages(); g_assert(!unmanaged); /* ===== WIRELESS SETTING ===== */ @@ -3791,10 +3800,12 @@ test_read_wifi_wep_eap_ttls_chap(void) NMSetting8021x * s_8021x; char * unmanaged = NULL; + NMTST_EXPECT_NM_WARN("*key ONBOOT is duplicated and the early occurrence ignored*"); connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wifi-wep-eap-ttls-chap", NULL, TYPE_WIRELESS, &unmanaged); + g_test_assert_expected_messages(); g_assert(!unmanaged); /* ===== WIRELESS SETTING ===== */