Description
Robert Fox
2011-10-05 19:37:44 UTC
Created attachment 5723 [details] [review] Patch for Bug 6979 - Auth_with_ldap.pm in C4 directory This patch is being submitted in order to address a bug we encountered in the checkpw_ldap subroutine in the C4/Auth_with_ldap.pm module. I did not touch the part of the conditional that obtains if the auth_by_bind variable is set to true in the configuration. QA comment investigating this bug before the signoff : 2 comments : === COMMENT 1 === + # BUG 6979 + # 2011-09-29 Robert Fox (rfox2@nd.edu) => those comments should not be in the code. Git is here to keep track of those informations. I agree you've reindented + # BUG #5094 + # 2010-08-04 JeremyC but it should not have been here either (and now we have a strong QA, it would not have been accepted) So, please resubmit without those comments. === COMMENT 2 === Replacing compare by a bind is not a good solution. Some LDAPs are configured to let no-one (except some specific accounts) bind. Some are configured to require binding. It means you'll solve a problem (for you probably, but not only, I agree), and create another problem for some other libraries that have Auth_with_ldap working now. A better patch would be : * to test compare, and if it fails, test binding (acceptable, although dirty & less secure I feel) or * add an entry in the ldap config file to select between bind & compare method (better but more work) So I think we should not integrate this patch for now, and mark as failed QA. Is it still valid or can be closed? cc Martin Hi, (In reply to Jonathan Druart from comment #3) > Is it still valid or can be closed? cc Martin I have recently been contracting for making KOHA LDAP authentication against a Debian Edu (aka Skolelinux) main server work. The customer also finances upstream communication and asked me to do my best to get whatever solution I come up with into upstream KOHA. After auditing the Auth_with_LDAP.pm code, I come to these conclusions: 1. The customer runs a Koha 3.08.01.002 [1]. In the meantime, Koha 3.20.something is out. However, the Auth_with_LDAP.pm file in latest HEAD (master branch) is still at version 3.07.00.049 [2]. Also the Auth_with_LDAP.pm code looks far more advanced than the Auth_with_LDAP.pm code on latest HEAD. Is is possible that some branch merging did not happen for the Auth_with_LDAP.pm file? It feels like this requires some portion of investigation. Thanks. [1]http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Auth_with_ldap.pm;h=fab6e44fafd6bb4cde5c1cd3e66655be0989338e;hb=e7c7f7af023172aea3fb02e4c1fa356c99f69fec [2] http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Auth_with_ldap.pm;h=58484a2ba700b0d469cbaf14f1b56083e01ebbf1;hb=6f81f8a0e2309447acc6e5bb74f444102d8adf56 2. KOHA LDAP Auth seems to be working fine for AD authentication using userPrincipal attribute description and a valid password. The default AD setup always allows user binding to their own account's DN. So that should work out well. 3. Authentication against openLDAP with clear text passwords stored in LDAP should also work fine as long as an administrative DN object is used for binding (e.g. cn=admin of objectClass simpleSecurityObject or such). However, storing clear text passwords in an LDAP tree is really really old school and should neither happen nor be expected anymore. On most setups, using $db->compare() will be unusable as passwords in most recent openLDAP setups are stored in a hashed way (and have been salted before hashing). 4. In KOHA, it even seems to be an option to use anonymous bind and $db->compare() for LDAP authentication. This should not be allowed at all, as it requires (a) an anonymous bind LDAP configuration that reveals the userPassword field (to everyone!!!) and requires the value in the userPassword attribute description to be stored in clear text. Nothing people really want... The approach for my customer (and also my proposal for getting the above sorted out in KOHA, if devs here agree) is this: o drop anonymous bind + userPassword LDAP CompareRequest completely o keep admin-bind + userPassword LDAP CompareRequest o keep AD authentication as is o try an auth for a specific user against LDAP using the user's DN (as proposed by a patch similar to the patch provided by Robert Fox) o make the openLDAP user-login-via-test-authbind method configurable via koha-conf.xml Any feedback on this is highly welcome. I am also open to discuss a different approach (as long as it works against openLDAP deployed in Debian Edu / Skolelinux setups). Greets, Mike PS: I am also a Debian Developer, being interested in getting KOHA into Debian in the long run... Hi Mike, Great to have some new blood on board; I totally agree that the Auth_with_ldap code needs a major rethink and would support such a piece of work. It's great to have your insight regarding best/worst practice's in the LDAP space and i'd be OK with deprecating some feature and clarifying the code.. though we'de need a good strong DEPRECATION warning because not all koha users are as technically able as yourself and may not be following current bets practice.. That's why it's always hard to get rid of some of these ldap related features. It might also be worth you taking a little look at bug 8993 as it was a piece of work aimed at re-working the LDAP code. On the debian front, Robin is your man there.. it's always good to get some extra feedback on our packaging approach. Are you on the Koha IRC channel yet.. that's probably your best place to start getting involved? (In reply to Martin Renvoize from comment #5) > It might also be worth you taking a little look at bug 8993 as it was a > piece of work aimed at re-working the LDAP code. The patches in #8993 look indeed promising. I will need some time to review and get back to you via #8993 or here (depends where it fits best). Mike (In reply to Martin Renvoize from comment #6) > On the debian front, Robin is your man there.. it's always good to get some > extra feedback on our packaging approach. Are you on the Koha IRC channel > yet.. that's probably your best place to start getting involved? Showing up on IRC now (my nick is around 24/7, nick is: sunweaver)... Mike Hello Mike, Nice to read your comment 4. I totally agree with you but i have a question: What do you mean by "openLDAP user-login-via-test-authbind method" ? For me, there is no difference between AD and openLDAP binds. Net::LDAP should work with both, right ? I think bug 8983 is quite tricky. It has the advantage that we can make more complex/useful mapping when replicating users from LDAP but it needs librian create a package with perl code. The attached patch here is more simple and could solve (with a little change) the problem of multiple branche. Created attachment 46646 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Created attachment 46654 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Created attachment 46659 [details] [review] Bug 6979 - Update tests Created attachment 46698 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Created attachment 46863 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin We had the same problem authenticating LDAP with KOHA 3.22 in Debian Jessie, we ran some tests and found that the file /usr/share/koha/lib/C4/Auth_with_ldap.pm in the line: my $cmpmesg = $db->compare( $userldapentry, attr=>'userpassword', value => $password ); Wasn't encoding the password to md5_base64 (which use LDAP), so we changed the line to: my $cmpmesg = $db->compare( $userldapentry, attr=>'userPassword', value => "{MD5}".md5_base64($password)."==" ); Retrieve userPassword attribute, encode the clear text password to md5_base64 and add '{MD5}' at start and '==' end of the password. If LDAP is using different encryption, that change should be made in the code. Hope it helps someone. Greetings. In general, my feeling more and more is that we should be looking to deprecate password comparison forms of ldap in the long term and we should plan for this. I'd like to see a bug adding warnings and possibly reports to the community hea app for the various ldap configuration combinations. I fear those users who ate using password comparisons still may not be aware of the intrinsic security concerns with such an approach. We should encourage a move forward to more secure methods. Having said all this, we 'could' retain the password comparison and hash before compare at our end.. But this would entail either some complex configuration to add various hashing algorithms or some ldap queries to ascertain the configuration to use. Along with this, extracting the salt for more complex hashing methods would need work too. There are pretty good cpan modules for this.. So it's all possible.. My two pence Comment on attachment 46863 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Review of attachment 46863 [details] [review]: ----------------------------------------------------------------- ::: C4/Auth_with_ldap.pm @@ +96,4 @@ > warn sprintf("LDAP Auth rejected : %s gets %d hits\n", $filter->as_string, $count) . description($search); > return 0; > } > + if ($count == 0) { So what does $count>1 mean? Pretty sure the greater than one is caught in the codeblock above your comment. If means more than one user in ldap matched the koha matchpoint. I.e. We can't perform a compare because we're not confident we're matching the right user. Created attachment 57480 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Created attachment 57481 [details] [review] Bug 6979 - Update tests Created attachment 57482 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Created attachment 57483 [details] [review] Bug 6979 - Fix Already in a transaction error Patch rebased on master The 'Update tests' patch fails to apply, when you try to apply this patch to koha. This is the result: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug #6979 Applying: Bug 6979 - Update tests Using index info to reconstruct a base tree... M t/db_dependent/Auth_with_ldap.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Auth_with_ldap.t CONFLICT (content): Merge conflict in t/db_dependent/Auth_with_ldap.t Failed to merge in the changes. Patch failed at 0001 Bug 6979 - Update tests The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-6979---Update-tests-EmFl3l.patch Created attachment 57963 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Created attachment 57964 [details] [review] Bug 6979 - Update tests Created attachment 57965 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Created attachment 57966 [details] [review] Bug 6979 - Fix Already in a transaction error Hello Alex, Patches are rebased on master. You can test again. Re: Comment 29 Hello Alex, Patches are rebased on master. You can test again. Thanks Alex. As I am new to Koha would it be possible to have a clearer test plan for this patch for me to follow in my testing? Cheers Alex I have tested the patch and it works fine! I am not able to generate the signed patch, sorry (In reply to Hugo Agud from comment #31) > I have tested the patch and it works fine! > > I am not able to generate the signed patch, sorry Is it a technical problem or you know how to signoff ? I am still learning to create sign-off patch with kohadevbox, I have pending a trainning ;) Any chance of a quick rebase Alex? I'm attempting to QA and I have a SHA1 missing error here ;) Created attachment 61295 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Created attachment 61296 [details] [review] Bug 6979 - Update tests Created attachment 61297 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Created attachment 61298 [details] [review] Bug 6979 - Fix Already in a transaction error Created attachment 61310 [details] [review] Bug #6979 I removed several lines of code in the checkpw_ldap subroutine where LDAP authentication takes place, in the "else" part of the conditional that checks for the auth_by_bind config parameter. I added several lines to check whether the user can log in to LDAP using their DN and the password supplied in the login form. If they are able to bind, login contiues as normal and the LDAP attributes can be harvested as normal if the update options are turned on. The routine that was in place was failing because it was trying to check against a non-existent LDAP entry attribute called 'userpassword'. Instead of checking against a 'userpassword' attribute, the routine really should be checking to make sure the user can actually bind to LDAP. That's what I set up, and it is a safer way to test authentication against LDAP. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 61311 [details] [review] Bug 6979 - Update tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 61312 [details] [review] Bug 6979 - Handle multiple branches in non-auth_by_bin Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 61313 [details] [review] Bug 6979 - Fix Already in a transaction error Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK.. Passing QA. I'm happy with the code here, and it moves us towards a more secure by default model. However, we'll need to highlight in the release notes that this patch effectively removes the auth by password comparison option from koha.. one always binds with this model. I don't think this is a bad move and I've yet to ever come across somewhere that actually requires a password comparison regime. We should push and fix the consequences in this case in my opinion. Pushed to master for 17.05, thanks Alex! (In reply to Martin Renvoize from comment #43) > OK.. Passing QA. > > I'm happy with the code here, and it moves us towards a more secure by > default model. > > However, we'll need to highlight in the release notes that this patch > effectively removes the auth by password comparison option from koha.. one > always binds with this model. I don't think this is a bad move and I've yet > to ever come across somewhere that actually requires a password comparison > regime. > > We should push and fix the consequences in this case in my opinion. If this is removing a 'feature' I is not suitable for pushing to a stable release. Martin, can you please take a look and confirm? commit 8c3fc47338fed6c35ea21a6524d6c1a109861ebf Author: = <=> Date: Wed Oct 5 16:47:21 2011 -0400 From IRC: rangi: can we fix the author lines on commits like that please? I agree that we should not push patches like that. |