strongSwan

Thank you for your fast response and feedback :)

I've had a look at the code in the new branch you created and compared it with what I submitted. With that, plus your comments above, I have the following comments:

• My code / patch aimed to have minimal impact on existing code and to be functionally separated/backwards compatible and/or simply because I figured that you would prefer to have "atomic" additions. For instance, I avoided to interleave my stuff into the existing process_class(), instead creating a similar process_class_relay() function that kicked in only when my proposed option switch eap-radius.accounting_relay_class was enabled.

• I wasn't fully aware of the auth_cfg_t functionality (in particular the usage of get_auth_cfg() and add() methods) and what implications it would have if I started to add() my stuff with it. This was why I chose to create a variable and a set()/get() to the ike_sa_t class instead. As long as auth_cfg_t keeps its data/has the same lifespan as ike_sa_t then I have no opinion on where to store the Class AVPs.

• Regarding the lifespan/how to keep and update the Class AVPs: It is important that the Class AVPs sent out to the accounting server are the same as the ones received in the most recent Access-Accept message (and also in the same order!). Theoretically, an Access-Accept message could be a follow-up re-authentication with updated Class data or even without Class AVPs, in either case consequent Accounting-Request messages shouldn't continue relaying initial/old Class data.

• I would strongly advice to have my proposed separate option switch eap-radius.accounting_relay_class (default no) enabling the relay function and not to bundle it with the existing eap-radius.class_group. There might be accounting servers in service out there that have support for incoming Class relay, but presently doesn't get that stimuli from strongSwan clients and there's no telling how those would react if they suddenly do! Also, I am not aware of how the class_group functionality as it exists in strongSwan will react if the RADIUS authorisation server starts to pump seemingly random, binary data in Class AVPs, that are really meant only to be interpreted in an accounting server?

• Hence: for the intents and purposes of RFC 2865 section 5.25, the Class AVP data contains 1-253 arbitrary binary octets, so no C-code string handling implied or guaranteed. chunk_t/memcpy() handling a must!

• My code / patch aimed to have minimal impact on existing code and to be functionally separated/backwards compatible and/or simply because I figured that you would prefer to have "atomic" additions. For instance, I avoided to interleave my stuff into the existing process_class(), instead creating a similar process_class_relay() function that kicked in only when my proposed option switch eap-radius.accounting_relay_class was enabled.

It always depends :) But the existing code in process_class() already does exactly the same thing you wanted (store the received attribute(s) somewhere).

• I wasn't fully aware of the auth_cfg_t functionality (in particular the usage of get_auth_cfg() and add() methods) and what implications it would have if I started to add() my stuff with it. This was why I chose to create a variable and a set()/get() to the ike_sa_t class instead. As long as auth_cfg_t keeps its data/has the same lifespan as ike_sa_t then I have no opinion on where to store the Class AVPs.

I try to avoid adding new members to major classes, in particular for features only a minority of users will use. And, yes, the lifespan of the auth data is the same as that of the IKE_SAs (unless the option charon.flush_auth_cfg is enabled). Another option would be to store them in the eap-radius plugin (in a hashtable mapping IKE_SAs to Class attributes).

• Regarding the lifespan/how to keep and update the Class AVPs: It is important that the Class AVPs sent out to the accounting server are the same as the ones received in the most recent Access-Accept message (and also in the same order!). Theoretically, an Access-Accept message could be a follow-up re-authentication with updated Class data or even without Class AVPs, in either case consequent Accounting-Request messages shouldn't continue relaying initial/old Class data.

I don't think that's an issue here. There is no EAP/RADIUS reauthentication with IKEv2 (a new IKE_SA has to be created to reauthenticate). So there won't be any update of these attributes. The only thing that could happen is using multiple authentication rounds, but using EAP for more than one is very uncommon (if at all possible). The order of rules in auth_cfg_t is maintained when enumerating them (however, only the first attribute is currently used).

• I would strongly advice to have my proposed separate option switch eap-radius.accounting_relay_class (default no) enabling the relay function and not to bundle it with the existing eap-radius.class_group. There might be accounting servers in service out there that have support for incoming Class relay, but presently doesn't get that stimuli from strongSwan clients and there's no telling how those would react if they suddenly do! Also, I am not aware of how the class_group functionality as it exists in strongSwan will react if the RADIUS authorisation server starts to pump seemingly random, binary data in Class AVPs, that are really meant only to be interpreted in an accounting server?

It's not bundled with that other option at all. But sure, we can add a separate option to enable this.

• Hence: for the intents and purposes of RFC 2865 section 5.25, the Class AVP data contains 1-253 arbitrary binary octets, so no C-code string handling implied or guaranteed. chunk_t/memcpy() handling a must!

Well, that RFC is 17 years old. So do people currently use random binary data here, or are these generally hex/base64 strings?

I pushed a version that stores the attributes in the eap-radius plugin (and has an option to enable it) to the 2451-radius-accounting-class-ht branch.

Hi,

Built and tested (to a limited extent) the 2451-radius-accounting-class-ht branch - OK!
Did not test sending multiple Class to see if those were relayed (and also in order).

I'm happy with the patch, you may merge the branch to master/HEAD now.