Deprecated ldap_int_tls_config in rlm_ldap
I was going through some of our fixes for 2.x and noticed that the new rlm_ldap for 3.0 has the same problem. Apparently Arron wondered about the use of ldap_int_tls_config as well and wrote up this issue: https://github.com/FreeRADIUS/freeradius-server/issues/236 The short story is that ldap_int_tls_config was never meant to be a public LDAP API, that's why it's not in the public header! But more to the point OpenLDAP has addressed the TLS configuration issues a while ago. Note OpenLDAP provides the *client* library most everyone uses, this is not an OpenLDAP server issue, we're talking client code here independent of your LDAP server of choice. The code in question centers around setting the REQUIRE_CERT option. To be able to use the correct API for setting REQUIRE_CERT one has to enable the NEW_TLS_CTX option (this is how OpenLDAP fixed the use of bogus use ldap_int_tls_config, by moving the TLS options into the standard config area). The old deprecated LDAP API took string arguments, the new API uses enumerated constants. To preserve backwards compatibility with rlm_ldap and because we really don't want enumerated constants in the rlm_ldap module config we also need to translate the string option to an enumerated constant. I could not figure out how to attach a patch to the github issue, so a git formatted patch is attached which can be applied with git-am or git-apply. This patch is against master (the rewritten rlm_ldap). Caveat: I have not actually tested the proposed patch against master, I simply ported our 2.x patch which had been tested. Note, the 2.x rlm_ldap has the same issue and the only reason we didn't push a 2.x patch yet is because while testing we discovered a bug in the OpenLDAP TLS driver code in which operations related to TLS were performed in the wrong order, that has since been fixed and was restricted to the case where NSS was the TLS provider. Once we finish testing the 2.x I'll submit that as well. But as far as I'm concerned in the new FreeRADIUS 3.x version there should be no reference whatsoever to a non-public deprecated LDAP API, instead 3.x should only use the currently defined public LDAP API's, trying to be backward compatible to something that was broken is not something that should be carried forward into a new version. -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
On 6 May 2013, at 13:45, John Dennis <jdennis@redhat.com> wrote:
I was going through some of our fixes for 2.x and noticed that the new rlm_ldap for 3.0 has the same problem. Apparently Arron
So close, still better than Aaron :)
wondered about the use of ldap_int_tls_config as well and wrote up this issue:
https://github.com/FreeRADIUS/freeradius-server/issues/236
The short story is that ldap_int_tls_config was never meant to be a public LDAP API, that's why it's not in the public header!
Yep.
But more to the point OpenLDAP has addressed the TLS configuration issues a while ago.
Do you know the version? Want to make sure that it's checked in the autoconf script.
The code in question centers around setting the REQUIRE_CERT option. To be able to use the correct API for setting REQUIRE_CERT one has to enable the NEW_TLS_CTX option (this is how OpenLDAP fixed the use of bogus use ldap_int_tls_config, by moving the TLS options into the standard config area). The old deprecated LDAP API took string arguments, the new API uses enumerated constants. To preserve backwards compatibility with rlm_ldap and because we really don't want enumerated constants in the rlm_ldap module config we also need to translate the string option to an enumerated constant.
I could not figure out how to attach a patch to the github issue, so a git formatted patch is attached which can be applied with git-am or git-apply. This patch is against master (the rewritten rlm_ldap).
You'd create a pull request that references the original issue. I think if you use something like '#fixes <issue number>' in the commit message it'll automatically reference the original issue.
Caveat: I have not actually tested the proposed patch against master, I simply ported our 2.x patch which had been tested.
The code is pretty similar for configuring the handles IIRC.
Note, the 2.x rlm_ldap has the same issue and the only reason we didn't push a 2.x patch yet is because while testing we discovered a bug in the OpenLDAP TLS driver code in which operations related to TLS were performed in the wrong order, that has since been fixed and was restricted to the case where NSS was the TLS provider. Once we finish testing the 2.x I'll submit that as well. But as far as I'm concerned in the new FreeRADIUS 3.x version there should be no reference whatsoever to a non-public deprecated LDAP API, instead 3.x should only use the currently defined public LDAP API's, trying to be backward compatible to something that was broken is not something that should be carried forward into a new version.
OK. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 05/07/2013 02:45 PM, Arran Cudbard-Bell wrote:
But more to the point OpenLDAP has addressed the TLS configuration issues a while ago.
Do you know the version? Want to make sure that it's checked in the autoconf script.
I don't think you need to check this in autoconf, you're not looking for the presence of a function, the correct methodology is to use the existing interfaces that have always been there, but pass new constants. In the patch you'll see the code is ifdef'ed so it only compiles if the constant is in the header file. If the constant is in the header but the library still doesn't understand it (not likely) you should get an error back which will be logged. P.S. I'm not sure if you were thinking about falling back to the old methodology if you don't find the new mechanism. Don't do that. If the new mechanism isn't there for some reason the best approach IMHO would be to issue a warning, but don't try to use an internal non-public API instead. BTW the "_int_" in the symbol name ldap_int_tls_config stands for internal. -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
On 7 May 2013, at 15:01, John Dennis <jdennis@redhat.com> wrote:
On 05/07/2013 02:45 PM, Arran Cudbard-Bell wrote:
But more to the point OpenLDAP has addressed the TLS configuration issues a while ago.
Do you know the version? Want to make sure that it's checked in the autoconf script.
I don't think you need to check this in autoconf, you're not looking for the presence of a function, the correct methodology is to use the existing interfaces that have always been there, but pass new constants. In the patch you'll see the code is ifdef'ed so it only compiles if the constant is in the header file. If the constant is in the header but the library still doesn't understand it (not likely) you should get an error back which will be logged.
Fair enough.
P.S. I'm not sure if you were thinking about falling back to the old methodology if you don't find the new mechanism. Don't do that. If the new mechanism isn't there for some reason the best approach IMHO would be to issue a warning, but don't try to use an internal non-public API instead. BTW the "_int_" in the symbol name ldap_int_tls_config stands for internal.
Yes I know, it was wrong. Ok i've pushed back a modified version of your patch. I disliked the fact that it did the string to enum conversion when it created a connection as it means -C wouldn't of caught an invalid value for tls.require_cert. The conversion is now done when the module is instantiated. I've also removed the default value so that it'll use whatever libldap uses as a default. This is so we can warn appropriately if setting cert requirements is not supported. and apparently it's 'Fixes #<issue>' which makes more sense. Stupid google. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 05/07/2013 04:07 PM, Arran Cudbard-Bell wrote:
On 7 May 2013, at 15:01, John Dennis <jdennis@redhat.com> wrote:
On 05/07/2013 02:45 PM, Arran Cudbard-Bell wrote:
But more to the point OpenLDAP has addressed the TLS configuration issues a while ago.
Do you know the version? Want to make sure that it's checked in the autoconf script.
I don't think you need to check this in autoconf, you're not looking for the presence of a function, the correct methodology is to use the existing interfaces that have always been there, but pass new constants. In the patch you'll see the code is ifdef'ed so it only compiles if the constant is in the header file. If the constant is in the header but the library still doesn't understand it (not likely) you should get an error back which will be logged.
Fair enough.
P.S. I'm not sure if you were thinking about falling back to the old methodology if you don't find the new mechanism. Don't do that. If the new mechanism isn't there for some reason the best approach IMHO would be to issue a warning, but don't try to use an internal non-public API instead. BTW the "_int_" in the symbol name ldap_int_tls_config stands for internal.
Yes I know, it was wrong.
Ok i've pushed back a modified version of your patch. I disliked the fact that it did the string to enum conversion when it created a connection as it means -C wouldn't of caught an invalid value for tls.require_cert. The conversion is now done when the module is instantiated.
I've also removed the default value so that it'll use whatever libldap uses as a default. This is so we can warn appropriately if setting cert requirements is not supported.
and apparently it's 'Fixes #<issue>' which makes more sense. Stupid google.
Sounds good, all good improvements. Thanks! -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
participants (2)
-
Arran Cudbard-Bell -
John Dennis