Wrong wpa_supplicant configuration is generated in case of MSCHAPv2

[kbabioch]

As diagnosed in bnc#1026807, a change to wpa_supplicant in commit f24e48861d50b6b6fc5681f75d4aa75144862853 has been made, which changed the behavior in regards to auth= and autheap=.

The commit message reads:

This patch fixes an issue with an invalid phase2 parameter value auth=MSCHAPv2 getting interpreted >as auth=MSCHAP (v1) which could degrade security (though, only within a protected TLS tunnel). >Now when invalid or unsupported auth= phase2 parameter combinations are specified, EAP-TTLS >initialization throws an error instead of silently doing something.

The configuration files generated by wicked are affected by this, since they set WIRELESS_EAP_AUTH='MSCHAPv2', which will no longer work, resulting in issues (see bug mentioned previously).

[kbabioch]

After having a look at the code, this is actually a little bit more complicated. WIRELESS_EAP_AUTH='MSCHAPv2' is only setting the internal representation, and is only loosely related to what is effectively passed to wpa_supplicant. In the original bug report the error is

TLS: Unsupported Phase2 EAP method 'mschapv2'

As you can see mschapv2 is passed to wpa_supplicant, not MSCHAPv2. However both are invalid, as MSCHAPv2 needs to be passed to wpa_supplicant.

Not sure what the best approach to fix this would be. We definitely need to fix the interface towards wpa_supplicant, but we could be more relaxed when parsing the configuration files. Or we should just pass this along as directly as possible as not modify these strings at all.

[kbabioch]

The problem is related to an array, which is used to transform strings to the internal representation of the type:

wicked/src/wireless.c

Lines 596 to 621 in e6e0039

	static ni_intmap_t __ni_wireless_eap_method_names[] = {
	{ "none", NI_WIRELESS_EAP_NONE },
	{ "md5", NI_WIRELESS_EAP_MD5 },
	{ "tls", NI_WIRELESS_EAP_TLS },
	{ "pap", NI_WIRELESS_EAP_PAP},
	{ "chap", NI_WIRELESS_EAP_CHAP},
	{ "mschap", NI_WIRELESS_EAP_MSCHAP},
	{ "mschapv2", NI_WIRELESS_EAP_MSCHAPV2},
	{ "peap", NI_WIRELESS_EAP_PEAP },
	{ "ttls", NI_WIRELESS_EAP_TTLS },
	{ "gtc", NI_WIRELESS_EAP_GTC },
	{ "otp", NI_WIRELESS_EAP_OTP },
	{ "leap", NI_WIRELESS_EAP_LEAP },
	{ "psk", NI_WIRELESS_EAP_PSK },
	{ "pax", NI_WIRELESS_EAP_PAX },
	{ "sake", NI_WIRELESS_EAP_SAKE },
	{ "gpsk", NI_WIRELESS_EAP_GPSK },
	{ "wsc", NI_WIRELESS_EAP_WSC },
	{ "ikev2", NI_WIRELESS_EAP_IKEV2 },
	{ "tnc", NI_WIRELESS_EAP_TNC },
	{ "fast", NI_WIRELESS_EAP_FAST },
	{ "aka", NI_WIRELESS_EAP_AKA },
	{ "aka_prime", NI_WIRELESS_EAP_AKA_PRIME },
	{ "sim", NI_WIRELESS_EAP_SIM },
	{ NULL }
	};

Not sure what the best approach to fix this, would be. I can come up with something, but I'm not sure about all of the consequences. Unfortunately there seem to be no continuous integration, so I would prefer if someone with more experience with this codebase, would change the code in the best possible way.

By the way: There are many more and similar definitions for key management, which are probably also not working correctly and/or rely on lenient parsing on wpa_supplicant side of things. At least it is written in uppercase letters in the man page and/or example configuration files.

These (among others) might be problematic:

wicked/src/wireless.c

Lines 581 to 588 in e6e0039

	static ni_intmap_t __ni_wireless_key_mgmt_names[] = {
	{ "none", NI_WIRELESS_KEY_MGMT_NONE },
	{ "proprietary", NI_WIRELESS_KEY_MGMT_PROPRIETARY },
	{ "wpa-eap", NI_WIRELESS_KEY_MGMT_EAP },
	{ "wpa-psk", NI_WIRELESS_KEY_MGMT_PSK },
	{ "ieee802-1x", NI_WIRELESS_KEY_MGMT_802_1X },
	{ NULL }
	};

wicked/src/wireless.c

Lines 556 to 565 in e6e0039

	static ni_intmap_t __ni_wireless_cipher_names[] = {
	{ "none", NI_WIRELESS_CIPHER_NONE },
	{ "proprietary", NI_WIRELESS_CIPHER_PROPRIETARY },
	{ "wep40", NI_WIRELESS_CIPHER_WEP40 },
	{ "tkip", NI_WIRELESS_CIPHER_TKIP },
	{ "wrap", NI_WIRELESS_CIPHER_WRAP },
	{ "ccmp", NI_WIRELESS_CIPHER_CCMP },
	{ "wep104", NI_WIRELESS_CIPHER_WEP104 },
	{ NULL }
	};

wicked/src/wireless.c

Lines 535 to 540 in e6e0039

	static ni_intmap_t __ni_wireless_auth_algo_names[] = {
	{ "open", NI_WIRELESS_AUTH_OPEN },
	{ "shared", NI_WIRELESS_AUTH_SHARED },
	{ "leap", NI_WIRELESS_AUTH_LEAP },
	{ NULL }
	};

wicked/src/wireless.c

Lines 513 to 519 in e6e0039

	static ni_intmap_t __ni_wireless_auth_mode_names[] = {
	{ "default", NI_WIRELESS_AUTH_MODE_NONE },
	{ "wpa1", NI_WIRELESS_AUTH_WPA1 },
	{ "wpa2", NI_WIRELESS_AUTH_WPA2 },
	{ "rsn", NI_WIRELESS_AUTH_WPA2 },
	{ NULL }
	};

[mtomaschewski]

Fix in #780 to use the correct wpa table not the xml schema table. Further, additional change to assume mschapv2 for PEAP by default (PEAPv0) -- yast2 can set it,
but it is required on a detail page.

[kbabioch]

Since this has been addressed with #780, let's close this issue.