Bug 9165 - Allow preventing passwords from being stored locally when using LDAP
Summary: Allow preventing passwords from being stored locally when using LDAP
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P4 enhancement (vote)
Target Milestone: ---
Assignee: Robin Sheat
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-28 23:49 UTC by Robin Sheat
Modified: 2016-06-21 21:36 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-25 00:00:00
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9165 - Prevent LDAP passwords being stored locally (5.91 KB, patch)
2012-11-29 02:35 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9165 - Prevent LDAP passwords being stored locally (5.98 KB, patch)
2012-11-29 02:37 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9165 - Prevent LDAP passwords being stored locally (5.98 KB, patch)
2012-11-29 02:39 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9165 - Prevent LDAP passwords being stored locally (6.15 KB, patch)
2013-05-31 03:55 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9165 - Prevent LDAP passwords being stored locally (4.51 KB, patch)
2014-04-21 22:42 UTC, Robin Sheat
Details | Diff | Splinter Review
[Signed Off] Bug 9165 - Prevent LDAP passwords being stored locally (4.59 KB, patch)
2014-11-24 16:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12831 [Followup]: Clear existing sync (1.04 KB, patch)
2014-11-24 16:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12831 [Followup]: Clear existing sync (1.08 KB, patch)
2014-11-24 22:28 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12831 [QA Followup]: Tidied code slightly (1.27 KB, patch)
2014-11-27 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 9165 - Prevent LDAP passwords being stored locally (4.63 KB, patch)
2014-11-27 16:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12831 [Followup]: Clear existing sync (1.14 KB, patch)
2014-11-27 16:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12831 [QA Followup]: Tidied code slightly (1.32 KB, patch)
2014-11-27 16:09 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2012-11-28 23:49:03 UTC
Typically when a user logs in with LDAP, the password is hashed and stored in the local database. For security reasons, this isn't always desirable. So we need an option that prevents this from happening.
Comment 1 Robin Sheat 2012-11-29 02:35:04 UTC Comment hidden (obsolete)
Comment 2 Robin Sheat 2012-11-29 02:37:40 UTC Comment hidden (obsolete)
Comment 3 Robin Sheat 2012-11-29 02:39:11 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-05-31 03:35:56 UTC
Patch does not apply.  Please rebase and resubmit.
Comment 5 Robin Sheat 2013-05-31 03:55:14 UTC Comment hidden (obsolete)
Comment 6 Robin Sheat 2013-05-31 03:55:43 UTC
(In reply to comment #4)
> Patch does not apply.  Please rebase and resubmit.

Done.
Comment 7 I'm just a bot 2013-09-29 05:28:16 UTC
Patch applied cleanly, go forth and signoff
Comment 8 I'm just a bot 2013-10-24 19:35:19 UTC
Applying: Bug 9165 - Prevent LDAP passwords being stored locally
Using index info to reconstruct a base tree...
M	C4/Auth_with_ldap.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth_with_ldap.pm
CONFLICT (content): Merge conflict in C4/Auth_with_ldap.pm
Patch failed at 0001 Bug 9165 - Prevent LDAP passwords being stored locally
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 9 Robin Sheat 2014-04-21 22:42:01 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2014-08-18 03:35:48 UTC
It would be really good to get a sign off on this please.
Comment 11 Martin Renvoize 2014-11-24 16:48:17 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2014-11-24 16:48:20 UTC Comment hidden (obsolete)
Comment 13 Robin Sheat 2014-11-24 22:28:53 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2014-11-27 13:03:21 UTC
Just thought I aught to clarify the relation between the followup here and bugs 12831 and 8148.

Bug 8148 was added for security reasons and it's sole goal was to prevent users logging in with outdated LDAP passwords (Koha used to fallback on a local copy of the password should LDAP auth fail for ANY reason, and as such would allow for outdated passwords to till be used for login)

Bug 12831 was added as bug 8148 had the unintended side effect of disabling all local only accounts. Unfortunately it's proven difficult to distinguish between LDAP failures due to incorrect passwords and any other ldap failure for the ldap configuration where no anonymous search user is specified.

The followup here is designed to help alleviate the above issue by removing old synced passwrods upon an ldap users first login after the config preference is changed.  It is by no means an instant fix, but it the best I could come up with.

Therefore:

Warning, switching this config option to prevent syncing of ldap passwords to local cache will not instantly take affect. If you wish to imediately benefit form this added security, my advise would be to manually clear all existing ldap users passwords from the database.
Comment 15 Julian Maurice 2014-11-27 15:15:47 UTC
Theses patches work as intended and, as the current behaviour is unchanged by default, I see no reason to not apply it on master.

But there is some minor issues that should be addressed.
First, koha-qa is reporting an error:

testing 2 commit(s) (applied to b141e6d 'Bug 13332: Fix conflict between 5304 ')

 FAIL   C4/Auth_with_ldap.pm
   OK     pod
   OK     forbidden patterns
   FAIL   valid
                "my" variable $sth masks earlier declaration in same scope 
   OK     critic

Also I think the two prepared statements can be replaced by $dbh->selectrow_array() and $dbh->do()

Finally the die message (Unable to access borrowernumber...) starts on first column and breaks the indentation style. Maybe you could split the string, or just leave the string overflow the 80 characters limit, I don't know if there is any rule for that.

So, failing QA for the first point (koha-qa)
Comment 16 Martin Renvoize 2014-11-27 15:52:32 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2014-11-27 15:55:48 UTC
I've done some minor code tidying to squelch the qa script error.

I'm afraid my dbi is a bit lacking (i tend to use dbic instead these days) so I'm not so sure about the advantages gained by a switch from prepare to selectrow_array.

Also split the die across two lines.. I really really want to tidy that file one day!
Comment 18 Julian Maurice 2014-11-27 16:09:33 UTC
Created attachment 33998 [details] [review]
Bug 9165 - Prevent LDAP passwords being stored locally

This adds a configuration option to LDAP that prevents it from storing
user's passwords in the local database. This is useful when users of
hosted Koha wish to prevent any form of offsite password storage for
security reasons.

Notes:
 * if the option is not included in the koha-conf.xml file, then the
   current default behaviour of saving the password locally is retained.
 * this has no impact on passwords that are already in the database.
   They will not be erased.

To use:
 * edit the koha-conf.xml for a system that uses LDAP for
   authentication.
 * in the <ldapserver> configuration, add:
   <update_password>0</update_password>
 * feel a greater sense of security.

To test:
 1) have a Koha system that authenticates using LDAP.
 2) note that when a user logs in, their password is saved (hashed) in
    the database.
 2.5) it is important to note that, for whatever reason, a user's
      password is not stored on a login where their account is created,
      only when they log in after being created. Thus perhaps log in and
      log out a couple of times to be sure.
 3) add the <update_password>0</update_password> option to the
    <ldapserver> section of koha-conf.xml.
 4) login with a new user (or erase the password from the database for
    an existing user) and note that the password field is not populated.
 5) log out and log back in just to be sure, check the password field
    again.

Sponsored-By: National Institute of Water and Atmospheric Research (NIWA)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Julian Maurice 2014-11-27 16:09:41 UTC
Created attachment 33999 [details] [review]
Bug 12831 [Followup]: Clear existing sync

A small enhancement to clear existing synced passowrd should this
config option be enbled. This followup is related to bug 12831

http://bugs.koha-community.org/show_bug.cgi?id=9165
Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 20 Julian Maurice 2014-11-27 16:09:47 UTC
Created attachment 34000 [details] [review]
Bug 12831 [QA Followup]: Tidied code slightly

Minor code tidy to clean up qa script warning.

http://bugs.koha-community.org/show_bug.cgi?id=9165
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 21 Tomás Cohen Arazi 2014-11-27 18:05:55 UTC
Patches pushed to master.

Thanks Robin and Martin!

Note: I ammended the subjects for the followups so they have the correct bug number :-P