Bug 8148

Summary: ldap authentication should FAIL if ldap contains NEW password, and user types the PREVIOUS password
Product: Koha Reporter: mourik jan <heupink>
Component: AuthenticationAssignee: Frédérick Capovilla <fcapovilla>
Status: CLOSED FIXED QA Contact:
Severity: critical    
Priority: P5 - low CC: brendan, cnighswonger, dpavlin, fcapovilla, gmc, katrin.fischer, m.de.rooy, martin.renvoize, mtj, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12831
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12831    
Attachments: Prevent local authentification fallback if an invalid LDAP password was entered.
Only update the password if the LDAP password field was mapped.
[SIGNED OFF] Prevent local authentification fallback if an invalid LDAP password was entered.
[SIGNED OFF] Only update the password if the LDAP password field was mapped.
Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth
[SIGNED OFF] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth
Prevent local authentification fallback if an invalid LDAP password was entered.
Only update the password if the LDAP password field was mapped.
Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth
[PASSED QA] Prevent local authentification fallback if an invalid LDAP password was entered.
[PASSED QA] Only update the password if the LDAP password field was mapped.
[PASSED QA] Bug 8148 - LDAP auth_by_bind doesn't fallback to local auth
Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests
Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests
Bug 8148: add squeeze's Test::MockObject dependency
[PASSED QA] Bug 8148: (regression tests) C4::Auth_with_ldap needs unit tests
[PASSED QA] Bug 8148: add squeeze's Test::MockObject dependency

Description mourik jan 2012-05-24 13:17:45 UTC
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?
Comment 1 Chris Nighswonger 2012-05-24 13:23:08 UTC
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.
Comment 2 mourik jan 2012-05-24 13:29:35 UTC
Well, we're in that situation now... And that's how I found out.

What I did: UPDATE borrowers SET password = '';
Comment 3 mourik jan 2012-05-24 13:31:58 UTC
I mean:
What I did: UPDATE borrowers SET password = 'asdfj3409u53f96394jkashsdfghkj';
Comment 4 Mason James 2012-05-24 13:33:43 UTC
(In reply to comment #0)
> The way ldap authentication works now 

please attach your ldap config to this bug
Comment 5 mourik jan 2012-05-24 13:38:57 UTC
  <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>
Comment 6 Frédérick Capovilla 2014-02-20 20:37:24 UTC Comment hidden (obsolete)
Comment 7 Frédérick Capovilla 2014-02-20 20:41:55 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2014-03-10 16:07:24 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2014-03-10 16:07:27 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2014-03-10 16:09:51 UTC
Works as expected for me, and code looks reasonable.
Comment 11 Dobrica Pavlinusic 2014-03-12 09:04:47 UTC
This patch still allows users to login with old database password if auth_by_bind method is used.
Comment 12 Katrin Fischer 2014-03-12 09:44:48 UTC
Hi Dobrica, is this a fail or ok?
Comment 13 Dobrica Pavlinusic 2014-03-12 09:48:24 UTC Comment hidden (obsolete)
Comment 14 Dobrica Pavlinusic 2014-03-12 09:50:50 UTC
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).
Comment 15 Dobrica Pavlinusic 2014-03-12 09:57:38 UTC
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.
Comment 16 Martin Renvoize 2014-03-25 08:19:47 UTC
Well spotted Dobrica, I missed the alternate configuration.

I also agree that some documentation would be wise.
Comment 17 Martin Renvoize 2014-03-25 08:40:25 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2014-03-25 08:41:32 UTC
I'm happy that with Dobrica's amendment this catches all cases now.  Signing off.
Comment 19 Katrin Fischer 2014-04-06 12:07:42 UTC
I can't test LDAP currently, can someone else take on QA here?
Comment 20 Katrin Fischer 2014-06-11 11:58:28 UTC
This has been stuck in the QA queue for a while - Brendan or Galen, could you take a look?
Comment 21 Dobrica Pavlinusic 2014-07-23 10:53:30 UTC Comment hidden (obsolete)
Comment 22 Dobrica Pavlinusic 2014-07-23 10:53:41 UTC Comment hidden (obsolete)
Comment 23 Dobrica Pavlinusic 2014-07-23 10:53:51 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-07-23 10:55:51 UTC
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?
Comment 25 Martin Renvoize 2014-07-24 15:20:27 UTC
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>
Comment 26 Martin Renvoize 2014-07-24 15:20:43 UTC
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>
Comment 27 Martin Renvoize 2014-07-24 15:20:49 UTC
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>
Comment 28 Martin Renvoize 2014-07-24 15:22:06 UTC
Happy to pass this now.. thanks for the followup dobrica.
Comment 29 Tomás Cohen Arazi 2014-07-25 17:36:58 UTC
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.
Comment 30 Martin Renvoize 2014-07-28 07:58:21 UTC
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
Comment 31 Tomás Cohen Arazi 2014-08-01 19:29:16 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2014-08-03 15:07:07 UTC
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!
Comment 33 Tomás Cohen Arazi 2014-08-03 23:45:22 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2014-08-03 23:50:50 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2014-08-07 14:49:38 UTC
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>
Comment 36 Martin Renvoize 2014-08-07 14:49:48 UTC
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>
Comment 37 Martin Renvoize 2014-08-07 14:52:36 UTC
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 :)
Comment 38 Tomás Cohen Arazi 2014-08-07 19:58:02 UTC
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!
Comment 39 Dobrica Pavlinusic 2014-08-27 11:31:18 UTC
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).
Comment 40 Martin Renvoize 2014-08-27 13:24:19 UTC
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..
Comment 41 Tomás Cohen Arazi 2014-08-27 13:25:27 UTC
New bug, with new regression tests :-D
Comment 42 Martin Renvoize 2014-08-27 14:17:49 UTC
Created bug 12831 to record this issue