Comments on WPS patches for hostapd/wpa_supplicant

[Ted Merrill]

On Saturday 24 November 2007 17:33:47 Jouni Malinen wrote:
> Here are some semi-random comments on the latest version of patches you
> sent for hostapd/wpa_supplicant.
>
>
> Which madwifi driver version includes definitions for struct
> ieee80211req_wscie and IEEE80211_IOCTL_GETWSCIE?

Ouch, you are right i don't see that in the latest madwifi... it is something 
added here at Atheros that was never merged into madwifi. 
Diffing the driver versions, i can see there is a lot of divergence.
I'll talk to people here at Atheros and see where we can go with this.
I have some fear that this may broaden the scope beyond what i can take care 
of... 
on the other hand, this is something that isn't in the Intel code, so probably 
it is not essential.
I'll research this.

>
>
> Was wpa_supplicant/wps_enrollee.c forgotten from the tarball?
> wps_enrollee build fails since there is no rule to create
> wps_enrollee.o which would likely mean that the matching .c file was
> missing.

Sorry about that.  We should probably just proceed to my next release... 
hopefully soon. 

>
>
> Adding "-W -Werror" to the gcc compiler is recommended. I'm using this
> and require the code to compile without warnings before committing
> into into my git tree.

OK.  Perhaps you should add this to the Makefile?

>
>
> OpenSSL build did not seem to work, i.e., had to use internal crypto
> for both hostapd and wps_enrollee builds.

OK, i'll look into it.

>
>
> Passing struct sta_info into hostapd WPA state machines is not
> acceptable. The sta_info structure is internal to hostapd's 802.11
> management code and it must not be used in WPA code. This is new
> cleanup in hostapd 0.6.x and I do not want to move back to the old
> version. In other words, the following change won't go in and will
> need to be done differently (i.e., just pass in the needed
> information):
>
>  struct wpa_state_machine *
> -wpa_auth_sta_init(struct wpa_authenticator *wpa_auth, const u8 *addr)
> +wpa_auth_sta_init(struct wpa_authenticator *wpa_auth, struct sta_info
> *sta)
>
> It looks like this was used for wpa.c to be able to look for
> sta->flags and four different WPS flags. Does WPA code really need to
> know this level of details? Wouldn't simple 1-bit flag be enough? If
> this changes dynamically, that should be done using a function call to
> update the WPA authenticator state for the STA.
>
>
> src/eap_server/eap_identity.c:
>
> +#ifdef EAP_WPS
> +#include <eap_wps.h>
> +#endif
>
> +#ifdef EAP_WPS
> +        /* Examine to see if it relates to WPS.
> +         * If so, there is a side effect to the station state.
> +         */
> +        eap_server_wps_identity_process(pos, sm->sta);
> +#endif  /* EAP_WPS */
>
> Does this need to be in eap_identity.c? I would prefer to see a
> callback from eap.c. In addition, struct sta_info should not be
> touched from EAP methods, so cleaner solution would be needed to
> handle this side effect anyway. I'm not completely sure what this is
> used for, but the part that changes struct sta_info needs to be moved
> away from the EAP method and handled, e.g., by adding a new struct
> eapol_callbacks function for this.
>

WPS is pretty intrusive, but i'll try to figure out a better way to do this.
For that matter, i'm struggling to really understand this code myself... i'm 
guilty of blindly passing it on.
Basically all those flags are used at just two places in the code... 
and after looking at it for the last several hours i'm pretty convinced that 
at least one of those is unnecessary.

More later,

-Ted