Patch for updated EAP-TNC support for freeradius from tnc@fhh
Hello Alan, I have created a forked version of latest freeradius v2 branch on github and merged the updated EAP-TNC support from tnch@fhh. The link is here: https://github.com/aveshagarwal/freeradius-server/commit/40104449aa7bce926a2... I have tested the patch on a testbed with 802.1x managed switch and wpa_supplicant and freeradius-server with EAP-TTLS as outer tunnel and EAP-MD5 and EAP-TNC as two inner EAP methods. I would appreciate any feedback on this patch to get it merged upstream. Thanks and Regards Avesh On Fri, Feb 1, 2013 at 4:32 PM, Alan DeKok <aland@deployingradius.com>wrote:
Avesh Agarwal wrote:
Thanks for your response. But if you could tell some specific reasons why it is not enabled by default that would really help me understand what is required to be done.
Because I don't think it works. I haven't had time to look into it.
It is here: https://github.com/trustatfhh/tnc-fhh
OK.
Yes, I agree. But some specifics would really help.
I don't have many. Make sure it works. Make sure the code is clean, and formatted correctly. Make sure the patches are readable.
I know pretty much nothing about EAP-TNC. So all I can give is general guidelines for getting code into the server.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On 8 Mar 2013, at 17:15, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
Hello Alan,
Although Alan is the project leader, many other people contribute to FreeRADIUS. Please address messages to the wider list if you're not emailing a developer directly.
I have created a forked version of latest freeradius v2 branch on github and merged the updated EAP-TNC support from tnch@fhh. The link is here:
In future please create patches against the master branch so we don't have to go through the pain of forward porting them. You went to the trouble of forking FreeRADIUS on GitHub, creating the commit, but inexplicably decided not to open a pull request and instead posted the commit to the developers list? Why? It makes it harder to merge, and means your code doesn't get submitted to Travis CI... But anyway thanks for doing this. I'll merge the patches manually, and make any required fixes. -Arran
I've refactored the code and shrunk it. The code you submitted leaked opaque data, which i've fixed. I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing? ------ Regarding the library. The build scripts for tnc-fhh are poor and do not work correctly on my system (OSX 10.8.2, Apple clang version 4.0, cmake version 2.8.10.1). I get errors from the linker/clang clang: error: invalid argument '-current_version 0.8.4' only allowed with '-dynamiclib' and when I add clang: error: invalid argument '-bundle' not allowed with '-dynamiclib' Adding -dynamiclib and removing -bundle from CMakeFiles/dummyimv.dir/link.txt fixes this, but i'm guessing there's a better way. ditto for: imcv/hostscanner/imv/CMakeFiles/hostscannerimv.dir/link.txt imcv/clamav/imv/CMakeFiles/clamavimv.dir/link.txt imcv/example/imv/CMakeFiles/exampleimv.dir/link.txt imcv/platid/imv/CMakeFiles/platidimv.dir/link.txt tncutil/CMakeLists.txt doesn't specify linker flags for libssl libcrypto or boost_system / boost_system-mt, so the linker fails with unresolved symbols. I added them manually: target_link_libraries(${PROJECT_NAME} ${Log4cxx_LIBRARY} ${XercesC_LIBRARY} ssl crypto boost_system-mt) I'm guessing there's a better way of doing this. The code itself has typos and won't compile: tnc-fhh/imcv/hostscanner/imv/src/HostScannerIMV.cpp:318:9: error: use of undeclared identifier 'u_int' u_int pos = line.find_first_of('='); Changing it to unsigned int at least allowed the build system to get past that. and there's an obvious typo here: nc-fhh/imcv/platid/imv/src/PlatidIMV.cpp:209:16: warning: equality comparison result unused [-Wunused-comparison] nothingWrong == false; That should be an assignment operator. Finally: CMake Error at imunit/cmake_install.cmake:31 (FILE): file INSTALL cannot set permissions on "/usr/local/include" Why is the install script attempting to change permissions on /usr/local/include in the first place? I'll take a look at the eap-ttls modifications later, and do them as a separate commit. -Arran
On Sat, Mar 9, 2013 at 8:50 AM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
I've refactored the code and shrunk it.
I really appreciate your help on this.
The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
------
Regarding the library.
I actually packaged it for fedora listed here http://koji.fedoraproject.org/koji/buildinfo?buildID=383324 .
So I use the that packaged version when building EAP-TNC module for freeradius, and somehow I did not have any of these issues while building the module. I will surely look for and try to eliminate the errors you noticed.
The build scripts for tnc-fhh are poor and do not work correctly on my system (OSX 10.8.2, Apple clang version 4.0, cmake version 2.8.10.1).
I get errors from the linker/clang
clang: error: invalid argument '-current_version 0.8.4' only allowed with '-dynamiclib'
and when I add
clang: error: invalid argument '-bundle' not allowed with '-dynamiclib'
Adding -dynamiclib and removing -bundle from CMakeFiles/dummyimv.dir/link.txt fixes this, but i'm guessing there's a better way.
ditto for: imcv/hostscanner/imv/CMakeFiles/hostscannerimv.dir/link.txt imcv/clamav/imv/CMakeFiles/clamavimv.dir/link.txt imcv/example/imv/CMakeFiles/exampleimv.dir/link.txt imcv/platid/imv/CMakeFiles/platidimv.dir/link.txt
tncutil/CMakeLists.txt doesn't specify linker flags for libssl libcrypto or boost_system / boost_system-mt, so the linker fails with unresolved symbols.
I added them manually: target_link_libraries(${PROJECT_NAME} ${Log4cxx_LIBRARY} ${XercesC_LIBRARY} ssl crypto boost_system-mt)
I'm guessing there's a better way of doing this.
The code itself has typos and won't compile:
tnc-fhh/imcv/hostscanner/imv/src/HostScannerIMV.cpp:318:9: error: use of undeclared identifier 'u_int' u_int pos = line.find_first_of('=');
Changing it to unsigned int at least allowed the build system to get past that.
and there's an obvious typo here:
nc-fhh/imcv/platid/imv/src/PlatidIMV.cpp:209:16: warning: equality comparison result unused [-Wunused-comparison] nothingWrong == false;
That should be an assignment operator.
Finally:
CMake Error at imunit/cmake_install.cmake:31 (FILE): file INSTALL cannot set permissions on "/usr/local/include"
Why is the install script attempting to change permissions on /usr/local/include in the first place?
I'll take a look at the eap-ttls modifications later, and do them as a separate commit.
Again, your effort is very much appreciated.
-Arran
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On 10 Mar 2013, at 00:13, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
On Sat, Mar 9, 2013 at 8:50 AM, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote: I've refactored the code and shrunk it.
I really appreciate your help on this. The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
------
Regarding the library.
I actually packaged it for fedora listed here http://koji.fedoraproject.org/koji/buildinfo?buildID=383324 .
So I use the that packaged version when building EAP-TNC module for freeradius, and somehow I did not have any of these issues while building the module. I will surely look for and try to eliminate the errors you noticed.
Ok i've sent a pull request. It adds the missing dependencies properly, fixes the build system to work on OSX, and fixes the typos. -Arran
On Sun, Mar 10, 2013 at 3:22 PM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
On 10 Mar 2013, at 00:13, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
On Sat, Mar 9, 2013 at 8:50 AM, Arran Cudbard-Bell <
a.cudbardb@freeradius.org> wrote:
I've refactored the code and shrunk it.
I really appreciate your help on this. The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
------
Regarding the library.
I actually packaged it for fedora listed here http://koji.fedoraproject.org/koji/buildinfo?buildID=383324 .
So I use the that packaged version when building EAP-TNC module for freeradius, and somehow I did not have any of these issues while building the module. I will surely look for and try to eliminate the errors you noticed.
Ok i've sent a pull request. It adds the missing dependencies properly, fixes the build system to work on OSX, and fixes the typos.
Appreciated, merged the patch.
-Arran - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
I really appreciate your help on this. The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
Still concerned about this . Can you explain how this memory gets freed? -- OK going through and cleaning up the eap_types.h file, your declared EAP-TNC to be type 38? IANA has type 38 to be HTTP-Digest. You need to request a number from IANA for EAP-TNC. -Arran
Hello Aaron, Thanks for your response. On Fri, Mar 15, 2013 at 4:19 PM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
I really appreciate your help on this. The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
Still concerned about this . Can you explain how this memory gets freed?
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
--
OK going through and cleaning up the eap_types.h file, your declared EAP-TNC to be type 38? IANA has type 38 to be HTTP-Digest. You need to request a number from IANA for EAP-TNC.
I looked at the IANA website http://www.iana.org/assignments/eap-numbers/eap-numbers.xhtml And it assigns 54 for EAP TNC. I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0. Thanks a lot for your help. Regards Avesh -Arran
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On Thu, Jul 18, 2013 at 10:51 AM, Avesh Agarwal <avesh.ncsu@gmail.com>wrote:
Hello Aaron,
Arran, I am sorry that I spelled it incorrectly.
Thanks for your response.
On Fri, Mar 15, 2013 at 4:19 PM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
I really appreciate your help on this. The code you submitted leaked opaque data, which i've fixed.
I'm concerned about the call to storeUserName, and getConnection, there's no where where the connection is explicitly destroyed, so I don't see how memory passed to those functions or allocated by those functions is ever freed. I guess there must be some magic GC/reference counting i'm missing?
Still concerned about this . Can you explain how this memory gets freed?
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
--
OK going through and cleaning up the eap_types.h file, your declared EAP-TNC to be type 38? IANA has type 38 to be HTTP-Digest. You need to request a number from IANA for EAP-TNC.
I looked at the IANA website
http://www.iana.org/assignments/eap-numbers/eap-numbers.xhtml
And it assigns 54 for EAP TNC.
I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0.
Thanks a lot for your help.
Regards Avesh
-Arran
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Avesh Agarwal wrote:
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
The memory gets freed when the module detaches? That's wrong. The connections should be freed when they're no longer needed.
I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0.
3.0 is frozen while we check some last minute issues. Maybe for the next release. Alan DeKok.
On Thu, Jul 18, 2013 at 11:35 AM, Alan DeKok <aland@deployingradius.com>wrote:
Avesh Agarwal wrote:
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
The memory gets freed when the module detaches? That's wrong. The connections should be freed when they're no longer needed.
EAP TNC authentication is done as a second authentication method inside EAP TTLS. As soon as EAP TNC authentication is done, the connection gets destroyed and all the resources are freed, and the connection does not linger forever.
I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0.
3.0 is frozen while we check some last minute issues.
EAP TTLS changes are not very significant as I understand, so if those changes could be considered for inclusion, that would be appreciated. Thanks for your help. Regards Avesh Maybe for the next release.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On 18 Jul 2013, at 21:09, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
On Thu, Jul 18, 2013 at 11:35 AM, Alan DeKok <aland@deployingradius.com> wrote: Avesh Agarwal wrote:
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
The memory gets freed when the module detaches? That's wrong. The connections should be freed when they're no longer needed.
EAP TNC authentication is done as a second authentication method inside EAP TTLS. As soon as EAP TNC authentication is done, the connection gets destroyed and all the resources are freed, and the connection does not linger forever.
I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0.
3.0 is frozen while we check some last minute issues.
EAP TTLS changes are not very significant as I understand, so if those changes could be considered for inclusion, that would be appreciated.
They weren't significant they were just poorly integrated and hacked in IIRC. Feel free to submit a new pull request against the current HEAD, and i'll see about integrating them in a sane and maintainable way. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On Thu, Jul 18, 2013 at 4:47 PM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
On 18 Jul 2013, at 21:09, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
On Thu, Jul 18, 2013 at 11:35 AM, Alan DeKok <aland@deployingradius.com>
wrote:
Avesh Agarwal wrote:
This memory gets freed in the call to "terminate" in the detach. Inside that it calls destructor to free objects associated with the connection id.
The memory gets freed when the module detaches? That's wrong. The connections should be freed when they're no longer needed.
EAP TNC authentication is done as a second authentication method inside EAP TTLS. As soon as EAP TNC authentication is done, the connection gets destroyed and all the resources are freed, and the connection does not linger forever.
I would also appreciate if you could look into the EAP TTLS modifications and if they can be available as part of FR3.0.
3.0 is frozen while we check some last minute issues.
EAP TTLS changes are not very significant as I understand, so if those changes could be considered for inclusion, that would be appreciated.
They weren't significant they were just poorly integrated and hacked in IIRC. Feel free to submit a new pull request against the current HEAD, and i'll see about integrating them in a sane and maintainable way.
Thanks for your response. I have been working on porting EAP-TTLS patches to FR3.0 and will send pull request once I am done with porting and testing.
Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On Fri, Mar 8, 2013 at 6:19 PM, Arran Cudbard-Bell < a.cudbardb@freeradius.org> wrote:
On 8 Mar 2013, at 17:15, Avesh Agarwal <avesh.ncsu@gmail.com> wrote:
Hello Alan,
Although Alan is the project leader, many other people contribute to FreeRADIUS. Please address messages to the wider list if you're not emailing a developer directly.
I agree with you. As the last conversation I had about it was with Alan,
so I just addressed to him.
I have created a forked version of latest freeradius v2 branch on github and merged the updated EAP-TNC support from tnch@fhh. The link is here:
In future please create patches against the master branch so we don't have to go through the pain of forward porting them.
You went to the trouble of forking FreeRADIUS on GitHub, creating the commit, but inexplicably decided not to open a pull request and instead posted the commit to the developers list? Why? It makes it harder to merge, and means your code doesn't get submitted to Travis CI...
I agree with you here too. As it was first time patch from me for freeradius, so I just thought to show the patch to the developers here before opening a pull request. Thanks for letting me know about and I will keep it in mind next time.
But anyway thanks for doing this. I'll merge the patches manually, and make any required fixes.
I appreciate your help and effort you put in for this patch.
-Arran - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Avesh Agarwal