wps_enrollee cleaned up code

[Jouni Malinen]

Some comments here.. I didn't have time go through everything today, so
I skipped some parts of the message for now.


On Mon, Dec 03, 2007 at 08:56:19PM -0800, Ted Merrill wrote:

> 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...

Sure, getting the driver interfaces sorted out was next on my list since
that is needed regardless of how the rest of WPS is implemented.

> On Sunday 02 December 2007 10:42:22 Jouni Malinen wrote:
> > 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.

As far as I know, the full mechanism is called Wi-Fi Protected Setup
(WPS), but the EAP method itself is still EAP-WSC, not EAP-WPS. As such,
it is not obsolete in naming the EAP method. Sure, it may confuse
people, but I would expect this part to end up being just an internal
name and the option that will be enabled will just be plain CONFIG_WPS
(which will end up enabling EAP-WSC into the build).

> 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 think there is some confusion here.. EAP peer is the "client" side of
EAP (the part that is run in IEEE 802.1X Supplicant) while EAP server
is the "server" part (run in IEEE 802.1X Authenticator). WPS spec
defines different roles like Enrollee and Registrar which may be run in
either the Supplicant or Authenticator (i.e, Supplicant can function as
a Registrar when adding an external Registrar as part of initial AP
setup and this case the EAP peer actually is not operating as an
Enrollee).

> -- 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.

Sure and I was planning on passing this information to EAP layer from
the EAPOL statemachine or making it easy for the WPS Registration
Protocol part to access association information. This was just not
needed for the EAP method itself, so I left it waiting for more proper
design of the EAP method <-> WPS Registration Protocol <-> rest of the
client interfaces.

> -- 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.

This doesn't sound correct. EAP server for EAP-WSC can act both as the
Registrar (add a new Enrollee case) and Enrollee (and a new external
Registrar case). The identity from EAP-Response/Identity is used to
select which one is being used here. Similarly, the client side (EAP
peer) will need to be able to do both Enrollee and Registrar roles.

I think a good first step would be to just support the
Supplicant/Enrollee and Authenticator/Registrar case, but the EAP method
implementation itself should not be limited to this case.

> -- You could use a file for shared data definitions

Sure. This is still in the phase of figuring out what exactly is going
to be sharable between EAP server and peer.

> -- You have plain numbers in the code that should be #ifdefed

Many of the numbers will disappear when the dummy registrar protocol
test code (wps_* functions) is removed. The remaining couple of numbers
could be cleaned up at some point.

> -- Where did you get the size 2000 from?

I'm assuming you are referring to the size of the M7/M8. 2000 was just a
round number that is large enough for testing fragmentation. All the
wps_* functions are going to disappear from eap_wsc.c once a more
complete interface for EAP <-> registration protocol is ready.

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

I have that on my todo list and actually, I already implemented this
couple of weeks ago. However, this would have ended changing all EAP
method interfaces which is something that I did not want to do at that
time. I'm still unclear on whether this really should happen now in
0.6.x, but it may if I get to it again soon ;-). I do have a patch file
that does this for bit over half of the EAP methods in my Git tree.
However, this EAP method interface is one of the interfaces that I do
not like to be changing very frequently (along the driver_ops interface)
in a way that would be backwards incompatible.

> For client interaction, uPnP is not an option.

Depends on what one means with "client", I guess. The WPS Registration
Protocol is used to register external registrars and this can happen
over UPnP.

> 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...

WPS supports vendor-specific extensions and as such, there may be need
for passing more information. In such a case, the other end can be
assumed to support the spec and not be limited on what a reference
implementation supports. Anyway, EAP-WSC fragmentation is relatively
simple and hidden in the EAP method implementation (i.e., WPS
registration protocol does not need to know about this at all).

> > Like I mentioned above, I don't think Ethernet/EAPOL/EAP processing
> > belongs here.

> 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).

That is just one part of it. The other part is duplicating EAPOL and EAP
state machine and that is something I don't want to see.

> Please note that WPS >does< need to know our mac address being used... you 
> cannot hide this information.

WPS Registration Protocol does need it, the EAP method part does not.

> For security reasons, it is probably a good idea to check the address of the 
> AP as well.

Sure and this can also be made available.

> > - 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.

Sure, but the network management GUI on the device will likely need to
tell the user to do this.

> > - 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...

Depends on the case.. If there are multiple registrars, the
EAPOL/EAP-WSC processing is started and M2/M2D messages may be needed to
figure out what exactly is to be set and where. The cases of
Supplicant/client acting as a Registrar to configure an AP will be an
extra complexity here on top that. I did not go through all the
possibilities and what inputs are needed for them, but I'd guess that
should be done at some point ;-).

> > 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.

That sounds reasonable if the commands start getting more parameters and
it can be done for new commands.

> 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.

That sounds reasonable, but this will likely need to be something that
is on top of wpa_ctrl and separate for hostapd and wpa_supplicant (both
of which are using the same mechanism for sending messages over
ctrl_iface).

> 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!

That's a good point. I ended up using two connections to avoid the need
for msg_cb when working on a GUI, so this did not come up. Anyway, this
certainly needs to be fixed as long as the msg_cb case remains in
wpa_ctrl.

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

I think that all Linux distros that I've heard of use the "default
directory", so this has not been much of an issue so far. Providing a
standard mechanism for wpa_ctrl clients to specify an alternative
directory could be useful. However, this is somewhat more complex due to
the same wpa_ctrl implementation being used both with wpa_supplicant and
hostapd.

-- 
Jouni Malinen                                            PGP id EFC895FA