wps_enrollee cleaned up code

[Ted Merrill]

Hi Jouni,
Thanks for the very good feedback.

A little background.  It would be best for Atheros (and everyone else of 
course) if WPS was part of the hostap tree, to eliminate the pain of having 
to keep a home-grown solution compatible with upgrades to hostap.
That said, i am rapidly approach the limit to which Atheros is willing to pay 
me to work on this, so it is possible i may not be able to see this 
integration through to completion.
However, i can see that you have been motivated to write some code for WPS so 
perhaps i have had some influence.
If i motivate you to write the entire thing yourself and using none of my 
code, that is fine, i'll feel like i've accomplished something.
However you are obviously a very busy person so i have some fear that this 
won't happen in a timely fashion.
This may put me in a somewhat difficult situation of trying to maintain  a 
repository that is both based on your repository but that also replaces your 
unfinished WPS code with my finished, but incompatible code.
(I'll send another email re. my current effort... im having kernel driver 
issues).
I'm happy in the near term to help debug your WPS code, if you do indeed have 
time to work on this.
Feel free to cannibalize anything i've done...

I would hope at least that we fix the impediments in the way of any WPS 
solution first.... such as the fix for IEEE80211_IOCTL_FILTERFRAME 
handling...

more below

-Ted Merrill
Atheros Communications

On Sunday 02 December 2007 10:42:22 Jouni Malinen wrote:
> On Mon, Nov 12, 2007 at 01:47:56PM -0800, Ted Merrill wrote:
> > Logically, the new wps_enrollee sits above and commands wpa_supplicant,
> > which continues to run... see below.
> > It also however makes direct l2_packet calls.
> > It could be argued that wps_enrollee should make use of the EAP code used
> > elsewhere, but i think this would only serve to confuse.
> > Although WPS >uses< the EAP mechanism, WPS is not actually an
> > authentication protocol at all... using EAP is basically just a hack.
>
> I think I will be arguing that this could will need to use the same
> EAPOL and EAP implementation as all other EAP methods. I do not see much
> point in duplicating this functionality just for WPS. A design that
> separates WPS Registration Protocol from EAP and shares the already
> existing EAPOL/EAP implementation looks much cleaner. I've added EAP-WSC
> implementation (just the EAP specific functionality) into my Git tree
> for both the EAP server and peer.

Do you have to call it WSC? that name  is obsolete and just confuses people.

Also, what you call "peer" the WPS spec calls "enrollee" and "client"... or 
perhaps i'm missing something... although i see it is a general naming 
convention you use....

I realize that your code is just a first pass, but i hope you address the 
following:
-- I don't see our mac address in the data structures... it will be required 
to be known somehow.  Knowing the ap's address may be required also.
-- It obtains EAP identity from configuration from wpa_ssid but this is 
inappropriate since essentially WPS has hijacked this... and anyway, 
eap_server/eap_wsc.c should ALWAYS use WFA-SimpleConfig-Registrar-1-0 and is 
always the registrar or the proxy thereof, and similarly for the client side 
code which will NOT be a registrar.
-- You could use a file for shared data definitions
-- You have plain numbers in the code that should be #ifdefed
-- Where did you get the size 2000 from?

Some sort of packet buffer management scheme (perhaps not quite as fancy as 
skbuf :~) would help i suppose.

>
> Since the Registration Protocol can be used over multiple transport
> mechanisms, it would benefit of clean interface that passed M1..M8, ACK,
> NACK, Done to WPS processing from wherever they came from (EAP, UPnP,
> etc.). In other words, the main WPS code should not need to worry about
> EAP details like fragmentation and reassembly or EAP header contents.
> Nor should it have to even know anything about l2_packet. Please also
> note that EAPOL frames can be received through the driver interface to
> better support some platforms. Direct calls to l2_packet would mean this
> option is not available (i.e., reduced OS/network stack support).

For client interaction, uPnP is not an option.
It is not mentioned in the WPS spec and there are various wifi specific 
requirements.
The "M" messages incorporate mac adddresses....
fragmentation is a non-issue, and is anyway WPS specific.

That said, i'm fine with how you are doing it so long as you don't lose the 
mac addresses.

>
> > -- wps_crypto -- key generation, authentication generation and checking.
> > The state data consists only of key data.  Relies on the same underlying
> > crypto routines as wpa_supplicant does (including can use the compact
> > internal routines).  Some of this will be useful for wps server.
>
> Some of this should really be integrated into src/crypto and only the
> WPS specific parts should remain in wps_crypto. For example, the current
> version seems to implement HMAC-SHA256 which is already available in
> src/crypto/sha256.c.

Yes, i see that hmac_sha256_vector has been added? since i originally wrote 
this code and that it appears to be the same algorithm.
(My implementation has the advantage of handling any number of elements 
instead of being limited to a specific number, but that is not is not a big 
practical problem).
I've added that to my list.

However, i think the rest of wps_crypto.c is pretty much WPS specific...


>
> > -- wpse_sm - enrollee state machine. The state data for the state machine
> > consists of: configuration parameters; copies of packets sent and
> > received; key state.  wpse_sm does NOT send or receive any packets, nor
> > does it generate timeouts,  instead the next higher level queries wpse_sm
> > and does the jobs for it.  This will simplify using this code in e.g. new
> > embedded applications.
>
> Does this really need to keep all possible packets (struct
> wps_sm_packet_buffers) in memory all the time? That structure is close
> to 30 kB.. Where does the 1520 byte maximum come from? WPS spec does not
> seem to define a maximum length and at least EAP transport could handle
> up to 64 kB packets..

Regarding 1520 byte max:
In practice, no WPS implementation is going to come close to ethernet packet 
size (1500+ bytes) and there is no reason to think this will change anytime 
soon... there is a lot of reason to keep everything fitting in one packet.
Although the WPS standard specifies a method for fragmentation, wsccmd (the 
original defining implementation) doesn't bother to implement it.
However, there is something to be said for generality...

It is not necessary to keep all the packets, and some applications might be 
bothered by a 30 kB allocation.
It IS necessary to keep a copy of the last packet sent.


>
> Like I mentioned above, I don't think Ethernet/EAPOL/EAP processing
> belongs here. While I really do not like the idea of UPnP, it sounds
> better to keep the interface designed in a way that would handle, if
> needed, both EAP and UPnP transports, assuming this can be done without
> much extra complexity.

In some sense it is just a formatting issue.
Using an ethernet dix format is one way of encapsulating all of the needed 
information into one data structure (instead of e.g. having to pass 
additional subroutine arguments).
Please note that WPS >does< need to know our mac address being used... you 
cannot hide this information.
For security reasons, it is probably a good idea to check the address of the 
AP as well.

It is not a big change however to the code that i wrote to skip the lower 
levels, to the extent consistent with knowing our mac address.

>
> > -- wpse_supplicant -- enrollee interface to the supplicant, it commands
> > the supplicant (via wpa_ctrl mechanism, using socket files on linux) to
> > associate with the desired ssid in open mode... note that wpa_supplicant
> > runs the whole time. This portion could be easily replaced to e.g.
> > perform supplicant actions directly.
>
> If the enrollee code will run as a separate process, ctrl_iface is a
> proper place was interfacing with wpa_supplicant. However, if this is in
> the same process, something else should be used.

Some sort of ctrl_iface connection is needed assuming that the user will want 
to direct the use of WPS.

>
> > Currently, wps_enrollee is a separate program, and no modifications to
> > wpa_supplicant were necessary (except for the Makefile and defconfig, to
> > make wps_enrollee)... this is certainly the safe road for early
> > development. I've gotten the feedback from you that for various reasons
> > it would be better if it were part of wpa_supplicant.  My thinking is
> > that it can be linked as part
> > of the wpa_supplicant program yet still logically sit above (the rest of)
> > wpa_supplicant... i hope you agree with that. My intention is to extend
> > the wpa_ctrl mechanism to include control from within the same process or
> > just bypass wpa_ctrl entirely.
>
> I would agree that it would be fine to have the WPS Registration
> Protocol implemented as a separate component above wpa_supplicant.
> However, I do not think I would go as far with the full WPS
> implementation that would only use wpa_supplicant to associate with the
> AP and then take care of all EAPOL/EAP processing.

No problem.

>
> The way I've been thinking of the WPS process to go would be something
> like this:
> - wpa_supplicant is running; maybe with empty configuration, but still,
>   running and able to provide scan results
> - OS specific network management, say NetworkManager (NM), monitors
>   network configuration and scan results and finds that no configured
>   networks are available, but there is a WPS-enabled AP that could
>   potentially be used

That is possible, or user may want to add a new network even though they have 
other networks.
More importantly, there may be MANY APs that are WPS capable, but they are not 
WPS-ready until the user pushes the button or enters PIN which is further 
down in the process.

> - NM requests wpa_supplicant to start WPS with an AP

Wait!
We have to get the user to push the button on the AP, and possibly tell us 
what PIN to use, or possibly have us provide a PIN and tell the AP what PIN 
to use.
Only then will the AP be WPS-ready.
The standard says you are supposed to do a scan indicating that you want to do 
WPS in probe requests, and check in probe responses for readiness... and (for 
push button method, which will be most commonly used) there must be exactly 
one that is ready.

> - wpa_supplicant configures the driver for WPS and requests association
>   with the BSS
> - wpa_supplicant takes care of EAPOL and EAP processing; EAP-WSC payload
>   may be handled either in-process (integrated Enrollee/Registrar) or by
>   sending it to an external process (Enrollee/Registrar) (the mechanism
>   for this could be separate from ctrl_iface)
> - wpa_supplicant notifies NM about progress and if needed, requests
>   information from the user (say, PIN)

That had to be done way before...

> - wpa_supplicant notifies NM of completion of WPS and availability of
>   network parameters
> - wpa_supplicant could internally update its configuration and/or wait
>   for NM to do that
>
> > The wpa_ctrl mechanism is in my opinion rather weak, and could use some
> > extensive rework (in contrast to most of your code which is quite good).
> > It uses a text based communication protocol which isn't very easy to
> > extend.
>
> Could you please elaborate? I don't see why text based protocol would
> make it harder to extend. ctrl_iface was certainly not designed for
> in-process communication, but if there are areas that would benefit from
> changes to support external processes better, I would be interested in
> hearing more details.

Nothing to do with being text-based.
Everything to do with being (with some exception) positional parameter based.
Much better would be to use a tag=value approach.

It would also be really helpful to have unified text parsing instead of having 
to do it adhoc everywhere.
It would also be very helpful if wpa_ctrl.c provided friendly data structures 
and access routines for all remote procedure calls, e.g. for each current 
command.

wpa_ctrl_request allows specifying a callback routine, which is good except 
that there is no provision to pass a cookie to the callback routine!
I had to use static data to work around this, which is not good!

Minor things:
The specification for an alternative to /var/run/wpa_supplicant is painful;
using an environmental variable might be a reasonable approach.