[PATCHv3 3/3] radius: report taxonomy assoc/probe IEs

[Jouni Malinen]

On Wed, May 05, 2021 at 06:23:36PM +0200, michael-dev wrote:
> Am 04.05.2021 19:35, schrieb Jouni Malinen:
> > > VENDOR  HostAP  39068
> > > BEGIN-VENDOR    HostAP  parent=Extended-Vendor-Specific-5
> > > ATTRIBUTE       HostAP-Probe-IES        0       octets
> > > ATTRIBUTE       HostAP-Assoc-IES        1       octets
> > > END-VENDOR      HostAP
> > 
> > Hmm.. Could you please remind me how those claimed attribute value
> > definitions were assigned for this IANA assigned enterprise number?
> 
> I was also wondering about that, but then I found that hostap has already
> been registered there, as
> http://www.iana.org/assignments/enterprise-numbers/enterprise-numbers (that
> is the PEN reference in RFC 6929) reads:
> > > 39068

> I guess this because of this entry in src/eap_common/eap_defs.h:
> > > EAP_VENDOR_HOSTAP = 39068 /* hostapd/wpa_supplicant project */,
> 
> The sub-IDs for HostAP-Probe-IES / HostAP-Assoc-IES need to be managed
> within the hostapd/wpa_supplicant project.

That should be clearly mentioned in the commit message to make this
clear. The FreeRADIUS dictionary example is fine to include, but it is
really confusing without there being first a clear indication that this
patch defines those new vendor attributes and reserves the specified
values for this purpose and only after that, giving an example on how
this could be used on the RADIUS server side.

> > And if so, what does the FreeRADIUS reference in the commit message
> > refer to?
> 
> The commit messages provides an example FreeRADIUS dictionary file entry to
> enable the use of HostAP-Probe-IES / HostAP-Assoc-IES attribute names within
> FreeRADIUS config files. As this entry needs to be added manually to
> FreeRADIUS and was non-obvious to me, I found it convenient to provide it
> there and to describe what this change actually adds to the RADIUS requests.

Something like this should precede that example in the commit message to
describe the purpose of included it there.

As far as unconditional inclusion of these new attributes in RADIUS
messages is concerned, I'm a bit concerned about potential
interoperability issues with old deployed RADIUS servers. Maybe it would
be safer to do this only based on a new explicit hostapd configuration
parameter? Or is there clear data available to believe that the RFC 6929
design has no issues without deployed servers?

Would you happen to be interested in adding a hwsim test case for this
functionality? At least something minimal to send out the values and
have hostapd-as-RADIUS-server print them out in debug would be of use.

Regarding patch 1/3 and 2/3, it would be nice to get a bit more complex
commit messages explaining why these changes are needed. Patch 2/3 is
obviously needed for this new functionality used in 3/3, but it was not
immediately obvious what the connection of patch 1/3 is to the other two
patches. In general, it would be nice if all patches come with a commit
message that provide justification for the changes beyond the
single-line title.

-- 
Jouni Malinen                                            PGP id EFC895FA