[OpenWrt-Devel,v2] wpa_supplicant: fix generating phase2 config line for WPA-EAP

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: missed the default definition two lines above, so no need to
use shell-expansion for that then.

 package/network/services/hostapd/files/netifd.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 2015-12-29 04:37, Daniel Golle wrote:
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v2: missed the default definition two lines above, so no need to
> use shell-expansion for that then.
> 
>  package/network/services/hostapd/files/netifd.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/network/services/hostapd/files/netifd.sh b/package/network/services/hostapd/files/netifd.sh
> index 5541e4d..75d340a 100644
> --- a/package/network/services/hostapd/files/netifd.sh
> +++ b/package/network/services/hostapd/files/netifd.sh
> @@ -630,7 +630,7 @@ wpa_supplicant_add_network() {
>  				peap|ttls)
>  					json_get_vars auth password
>  					set_default auth MSCHAPV2
> -					append network_data "phase2=\"$auth\"" "$N$T"
> +					append network_data "phase2=\"auth=$auth\"" "$N$T"
This might break existing configurations that already include the auth=
part, so when reworking this part you should detect and fix this.
Also, for EAP-TLS, phase2 needs to be autheap=TLS, which is not
supported with this change.

- Felix

Hi Felix,

On Sun, Jan 03, 2016 at 09:33:13PM +0100, Felix Fietkau wrote:
> > -					append network_data "phase2=\"$auth\"" "$N$T"
> > +					append network_data "phase2=\"auth=$auth\"" "$N$T"
> This might break existing configurations that already include the auth=
> part, so when reworking this part you should detect and fix this.
> Also, for EAP-TLS, phase2 needs to be autheap=TLS, which is not
> supported with this change.

Right... Probably this should be changed in LuCI then, because
currently $auth is set to values like 'PAP', 'MSCHAPV2', ... see
https://github.com/openwrt/luci/blob/master/modules/luci-mod-admin-full/luasrc/model/cbi/admin_network/wifi.lua#L897
which still matches the pre-netifd behaviour as defined in
https://dev.openwrt.org/browser/trunk/package/network/services/hostapd/files/wpa_supplicant.sh#L107
which is what I wanted to restore.
Having a complex value stored in UCI and leaving it to the user/view
to set it seems a bit odd to me (but might still be the best thing to
do), maybe we should rather store eap_type in UCI as well and then
generate the phase2 string in netifd.sh according to that...?

Let me know what you think and I'll suggest a follow-up patch.

Cheers

Daniel

On 2016-01-03 22:06, Daniel Golle wrote:
> Hi Felix,
> 
> On Sun, Jan 03, 2016 at 09:33:13PM +0100, Felix Fietkau wrote:
>> > -					append network_data "phase2=\"$auth\"" "$N$T"
>> > +					append network_data "phase2=\"auth=$auth\"" "$N$T"
>> This might break existing configurations that already include the auth=
>> part, so when reworking this part you should detect and fix this.
>> Also, for EAP-TLS, phase2 needs to be autheap=TLS, which is not
>> supported with this change.
> 
> Right... Probably this should be changed in LuCI then, because
> currently $auth is set to values like 'PAP', 'MSCHAPV2', ... see
> https://github.com/openwrt/luci/blob/master/modules/luci-mod-admin-full/luasrc/model/cbi/admin_network/wifi.lua#L897
> which still matches the pre-netifd behaviour as defined in
> https://dev.openwrt.org/browser/trunk/package/network/services/hostapd/files/wpa_supplicant.sh#L107
> which is what I wanted to restore.
> Having a complex value stored in UCI and leaving it to the user/view
> to set it seems a bit odd to me (but might still be the best thing to
> do), maybe we should rather store eap_type in UCI as well and then
> generate the phase2 string in netifd.sh according to that...?
> 
> Let me know what you think and I'll suggest a follow-up patch.
How about this:

If $auth is set, add it with auth=$auth (strip existing auth= from the
variable if present).

Then add an eap_auth config option (should be a list), add autheap=<val>
for every entry (I think there can be multiple ones).

That way you can configure everything and avoid complex UI-hostile types.

- Felix