The way ldap authentication works now is that the patron password is both in ldap and in mysql, usually the primary location being ldap. If sys admins change the userpassword, it's changed directly in ldap, and koha still has the old password stored in mysql. If the user then tries to logon with the OLD password, he should get 'access denied'. But instead he/she gets in, using the old, no longer valid, password. Various possible solutions: The best one: - an option not to store the password in mysql AT ALL (passwords are very sensitive info, I would like to store them in as few places as possible) Two other solutions: - a 'flush authentication cache' button in the staff interface? - a syspref to select the order of precedence when authenticating a user?
Changing this to critical. It really is a security issue. What if the user's account is compromised, and the sysadmin believes changing it on the LDAP server fixes it immediately? At the very least some loud documentation of the real behavior is in order.
Well, we're in that situation now... And that's how I found out. What I did: UPDATE borrowers SET password = '';
I mean: What I did: UPDATE borrowers SET password = 'asdfj3409u53f96394jkashsdfghkj';
(In reply to comment #0) > The way ldap authentication works now please attach your ldap config to this bug
<useldapserver>1</useldapserver> <ldapserver id="ldapserver"> <hostname>x.x.x.x</hostname> <base>ou=users,dc=xx,dc=yy</base> <replicate>1</replicate> <update>1</update> <auth_by_bind>1</auth_by_bind> <principal_name>uid=%s,ou=users,dc=xx,dc=yy</principal_name> <mapping> <firstname is="givenName" ></firstname> <surname is="cn" ></surname> <address is="postaladdress" ></address> <city is="l" ></city> <zipcode is="postalcode" ></zipcode> <branchcode is="branch" ></branchcode> <userid is="uid" ></userid> <password is="userpassword" ></password> <email is="mail" ></email> <categorycode is="employeetype" ></categorycode> <phone is="telephoneNumber"></phone> </mapping> </ldapserver>
Created attachment 25522 [details] [review] Prevent local authentification fallback if an invalid LDAP password was entered. We encountered the same problem with on our instances and created a patch to fix it. With this patch, the authentication now fails if the LDAP authentication fails because of an invalid password. Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Created attachment 25523 [details] [review] Only update the password if the LDAP password field was mapped. Here is a second patch I think is related to this problem. With this patch, local password won't be updated if the "password" field is not mapped in the LDAP configuration. Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Created attachment 26024 [details] [review] [SIGNED OFF] Prevent local authentification fallback if an invalid LDAP password was entered. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 26025 [details] [review] [SIGNED OFF] Only update the password if the LDAP password field was mapped. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected for me, and code looks reasonable.
This patch still allows users to login with old database password if auth_by_bind method is used.
Hi Dobrica, is this a fail or ok?
Created attachment 26147 [details] [review] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth This patch covers LDAP auth_by_bind configuration so that wrong LDAP password will return -1 to C4::Auth so we can abort local auth and prevent users logging in with stale database passwords.
This patch is fail without my addition. It should probably be squashed into one commit (I don't mind loosing credits for it, just make Frédérick author).
I would also like to add comment in C4::Auth to note that -1 results from LDAP auth is wrong password, because it's not totally obvious from looking in the code why we are breaking out early.
Well spotted Dobrica, I missed the alternate configuration. I also agree that some documentation would be wise.
Created attachment 26543 [details] [review] [SIGNED OFF] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth This patch covers LDAP auth_by_bind configuration so that wrong LDAP password will return -1 to C4::Auth so we can abort local auth and prevent users logging in with stale database passwords. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm happy that with Dobrica's amendment this catches all cases now. Signing off.
I can't test LDAP currently, can someone else take on QA here?
This has been stuck in the QA queue for a while - Brendan or Galen, could you take a look?
Created attachment 29977 [details] [review] Prevent local authentification fallback if an invalid LDAP password was entered. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Created attachment 29978 [details] [review] Only update the password if the LDAP password field was mapped. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Created attachment 29979 [details] [review] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth This patch covers LDAP auth_by_bind configuration so that wrong LDAP password will return -1 to C4::Auth so we can abort local auth and prevent users logging in with stale database passwords. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Martin, now that Dobrica has signed off on the first 2 and the third is only a tiny change, would you be ok with passing this?
Created attachment 30035 [details] [review] [PASSED QA] Prevent local authentification fallback if an invalid LDAP password was entered. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Created attachment 30036 [details] [review] [PASSED QA] Only update the password if the LDAP password field was mapped. http://bugs.koha-community.org/show_bug.cgi?id=8148 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Created attachment 30037 [details] [review] [PASSED QA] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth This patch covers LDAP auth_by_bind configuration so that wrong LDAP password will return -1 to C4::Auth so we can abort local auth and prevent users logging in with stale database passwords. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Happy to pass this now.. thanks for the followup dobrica.
Please provide regression tests for this to be included in master. I'd accept it gets PASSED-QA straight on the presence of those tests.
Apologies Tomas, I should have noticed the lack of tests and pushed back before it hit your pile. The patches have been around for a while now, and I only re-checked the 'new stuff' when I did QA.. Woops
Created attachment 30473 [details] [review] Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests This followup provides some unit tests for C4::Auth_with_ldap. It implements regression tests for this bug's changes too. The aim was to cover all use cases for checkpw_ldap from the first line up to those touched by this bug's patches. It should serve as a start for having unit tests for the LDAP thing. Having said that: no excuses for not providing regression tests for LDAP from now on :-/ Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Guys, I leave the regression tests file for you to take a look, criticize, fix, etc. Please mark it PASSED-QA if you agree on the tests, so we push this ASAP. Thanks for your hard work on getting LDAP fixed!
Created attachment 30524 [details] [review] Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests Edited to remove debugging leftover (use Data::Printer).
Created attachment 30525 [details] [review] Bug 8148: add squeeze's Test::MockObject dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Created attachment 30586 [details] [review] [PASSED QA] Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests This followup provides some unit tests for C4::Auth_with_ldap. It implements regression tests for this bug's changes too. The aim was to cover all use cases for checkpw_ldap from the first line up to those touched by this bug's patches. It should serve as a start for having unit tests for the LDAP thing. Having said that: no excuses for not providing regression tests for LDAP from now on :-/ Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 30587 [details] [review] [PASSED QA] Bug 8148: add squeeze's Test::MockObject dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I happy with these follow up tests by Tomas, and the MockObject dependency makes decent sense to me as I believe it'll be a useful addition to the testing toolbox. Fantastic to see some tests going in for LDAP, no excuses for future LDAP tests now to be written! I've expressed my confusion over what constitutes a sensible rule for db_dependant vs normal tests but I believe that can be left for a later conversation during the next dev meeting. Lets get this fix in :)
Patches pushed to master. Everyone involved, please put your eyes in all LDAP usage use cases, so we fix any regression, as we didn't have any tests before this patches were pushed. Thanks!
Unfortunately, this patch does have one regression: with ldap enabled and using auth_by_bind you won't be able to login using local accounts which aren't in LDAP. This is a problem, especially for sip accounts which (in our case) don't have LDAP equivalent (and they shouldn't have it since they are not real users so they don't belong in our institutional LDAP). I'm writing this just as a note, I will open separate ticket for fix (if I was able to find one, that is).
Pants, how did I miss this! This certainly seems to be a bit of a conundrum.. we need to allow local fallback, but only in the case that the user is not found in the ldap directory.. rather than when the bind/password compare operation fails. Hmm..
New bug, with new regression tests :-D
Created bug 12831 to record this issue