Bug 12831 - local only logins should still work when ldap authentication is enabled
Summary: local only logins should still work when ldap authentication is enabled
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P2 critical (vote)
Assignee: Galen Charlton
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 8148
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-27 14:16 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
13 users (show)

See Also:
Change sponsored?: Seeking developer
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12831 - local only logins should work with LDAP (1.35 KB, patch)
2014-08-29 11:41 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 12831 - local only logins should work with LDAP (1.50 KB, patch)
2014-09-16 08:58 UTC, Chris Cormack
Details | Diff | Splinter Review
BUG 12831: Local Only logins with LDAP (2.05 KB, patch)
2014-11-24 16:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUG 12831: Local Only logins with LDAP (2.10 KB, patch)
2014-11-26 09:32 UTC, Chris Cormack
Details | Diff | Splinter Review
BUG 12831: Local Only logins with LDAP (2.16 KB, patch)
2014-11-27 10:53 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12831: (RM followup) regression test (2.28 KB, patch)
2014-11-27 17:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12831: Local Only logins with LDAP (2.22 KB, patch)
2014-11-27 17:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2014-08-27 14:16:01 UTC
In fixing bug 8148 we introduced a regression.

If ldap is enabled, and using auth_by_bind, one will not be able to login using any local accounts which do not appear in the ldap directory.

This is problematic because a number of koha system accounts are unlikely to have a parallel in the active directory.. for instance SIP Service User accounts.
Comment 1 Dobrica Pavlinusic 2014-08-29 11:41:10 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2014-08-29 11:45:08 UTC
In our case, we are seeing invalid DN as error message when connecting to OpenLDAP with local accounts (this makes sense, but we also have a little bit of magic in front of our LDAP to transfer logins in form of joe.doe@ffzg.hr to cn=joe.doe,dn=ffzg,dn=hr, so I'm not sure that this is only error on which we want to fallback to local auth.

Unfortunately, I can't connect to OpenLDAP directly (since bind needs this cn=,dn=,dn= syntax) and don't have Active directory to test against that (but I suspect that AD users are not using auth_by_bind - again I might be wrong).
Comment 3 Chris Cormack 2014-09-16 08:58:03 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2014-09-18 15:05:51 UTC
Hmm.. I'm still not really sure if a return code of 34 (LDAP_INVALID_DN_SYNTAX (34)) is all we should be looking for here.

For example, what about: LDAP_NO_SUCH_OBJECT (32)

Or, should we be catching LDAP_INVALID_CREDENTIALS (49) for an expired password only?

Dobrica, do you have any comment?
Comment 5 Dobrica Pavlinusic 2014-09-20 10:07:34 UTC
Sure. it makes perfect sense to catch all three of them.
Comment 6 Martin Renvoize 2014-09-30 13:41:46 UTC
Do either of your fancy doing that followup, then I can proceed further with QA?
Comment 7 Dobrica Pavlinusic 2014-10-03 14:47:27 UTC
Sorry for delay, I wanted to test things before submitting more code.

I can't really get LDAP_NO_SUCH_OBJECT error on bind. My guess is that this error would appear if I was searching for object which wasn't defined in LDAP schema as opposed to bind call. Did you manage to get this error from LDAP server on bind?

I wouldn't like to insert code there just because it seems right, since LDAP code in Koha is already black magic to almost everyone and accumulating cruft doesn't seem like a good idea.

Regarding LDAP_INVALID_CREDENTIALS, it's already handled by else part of if statement and I did test with invalid password and it works as-is.
Comment 8 Dobrica Pavlinusic 2014-10-03 16:01:02 UTC
Upon further investigation, I decided to obsolete my patch because I suspect that if fixes problem only for our particular installation instead of providing general solution to problem. Let me explain...

Problem comes from the fact that in LDAP bind we can't distinguish between users which doesn't exist in LDAP and invalid passwords. Both of those return LDAP_INVALID_CREDENTIALS (in our installation we have a bit of LDAP software in between which creates invalid DN for users which are not in LDAP, but that is not generally applicable).

This means that we have to know (somehow) weather users should use LDAP passwords or not. Currently we don't have that information.

One possibility is to do LDAP search (anonymous or via some account) and if user does exist in LDAP force him to provide proper LDAP password. However, this might not be practical because it creates additional load on LDAP server on one hand, and on another anonymous searches might be disabled (that's probably why we are using auth_by_bind in the first place) or there might not be special user which can search whole LDAP (or library might not have permissions to add such user which is our case).

Another possibility is to track locally in Koha if users should use LDAP or local passwords. This might be implemented simply by invalidating local password (with word LDAP for example :-) which would prevent local fallback from working if user is logged in using LDAP.

I'm biased towards second solution but I would love to hear some feedback about it before producing code.

However, in any case we don't have clear migration path for existing installations because they do have local LDAP passwords in database (like we do). After first LDAP login users will have local password invalidated, but users which never login with correct LDAP password (or who are already disabled) we still be able to login using cached password.

To be quite honest, I did use that side-effect of old code which allowed our patrons to login while LDAP wasn't available, so it might even make sense to have that behavior again (as LDAP configuration option perhaps). But if that is the case, we again don't have ability to expire users just in LDAP and expect them to be disabled in Koha also.
Comment 9 Katrin Fischer 2014-10-09 21:21:38 UTC
I am not sure I understood completely what Dobrica wrote, but I feel like ideally we shouldn't store passwords for LDAP users, but still have the option to have 'local password only' users - think of external library users that are not in LDAP. And as this worked before, we really need to make sure it does again.
Comment 10 Martin Renvoize 2014-10-10 07:08:59 UTC
Unfortunately I don't think there's going to be a 'quick fix' here. As Dobrica says, I believe his fix doesn't really catch the general case and as such we could be chasing our tails for a while.  That said, it's probably better than nothing to start with.

Our LDAP implementation relies heavily on 'quirks' all over the place and isn't really a very thorough implementation at all.

The more I look at it, the more questions I ask about it. I'd love to get a some feedback from the community as to how people are configuring the module and expecting it to behave. Then we'de have a better idea of what features are 'broken', what features are in fact bugs, and where people are using bugs as features to get around our bad implementation.
Comment 11 Katrin Fischer 2014-11-03 22:47:43 UTC
I think we can't leave that it is currently - we need local logins or we will break people's LDAP setups on update :(
Comment 12 Larry Baerveldt 2014-11-18 17:39:47 UTC
Speaking for ourselves, we feel that it's absolutely essential that there by fallback logins. That is, that if the LDAP login fails for any reason, you can still login with a local Koha account. In fact, on a number of our partner sites, the staff accounts aren't even using the LDAP accounts; they are only local.

ByWater's $0.02.

- Larry
Comment 13 Martin Renvoize 2014-11-19 08:44:21 UTC
I think we can all agree with that sentiment Larry ;).

The problem is how to achieve it in a secure way; any chance you could share a few examples of how your ldap customers are set up, and what response you get from the ldap server given an incorrect username:password combination vs's the response for a user not in the ldap directory?

That would really help move things along on this bug.

I think what we really want to achieve is a failure for bad username:password combination only.. and allow local fallback for users that are plain missing from the ldap directory.

Does everyone agree that's what we need?
Comment 14 Martin Renvoize 2014-11-19 08:55:40 UTC
(In reply to Dobrica Pavlinusic from comment #8)

Having re-read Dobrica's suggestion here I'm also in favour of the last option he puts forward of invalidating the local password upon first successful LDAP login. (Though I would insert an randomised string as the 'bad' password rather than a set word for all official LDAP users)

I also feel this should be optional, with the option being something like 'Allow local fallback when LDAP server not found'.

Finally, getting around the issue of expiring ldap users, there's no clear single signal for such an action it seems, so I tihnk we're really going to struggle to get 'best of both' here.

Dobrica, do you have the time to develop such a patch as you suggested... I'm kinda tight for time at the minute, but feel this really needs to be fixed for release :(
Comment 15 Katrin Fischer 2014-11-19 08:58:56 UTC
I think it's possible to just delete it or set it to a special thing that indicates "no password" as you can generate borrowers with no password in the GUI. Then it would also be easy to see, who has logged in over LDAP.

A pref restoring the old (and maybe bad behaviour) if anything else fails would maybe be an option too. Generally having a pref could make sense.

We also don't have staff users in LDAP but only in Koha, as we keep the normal accounts and staff accounts separate or at least encourage to do that.
Comment 16 Katrin Fischer 2014-11-23 08:09:19 UTC
Could 9165 help with this?
Comment 17 Martin Renvoize 2014-11-24 16:12:59 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2014-11-24 16:19:32 UTC
My attached patch 'should' fix this for the case where a library is using auth_by_bind and not anonymous_bind. Reading the code, I believe this was the only case that needed fixing in this way.

To re-introduce the functionality of bug 8148 fully however, we should also take a careful look at bug 9165 as suggested by cait, as with some minor adjustments with would allow for the functionality we're looking for for all three major LDAP configurations.
Comment 19 Katrin Fischer 2014-11-24 21:44:07 UTC
Please, we need testing on this ASAP.
Comment 20 Chris Cormack 2014-11-26 09:32:59 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2014-11-27 10:52:53 UTC
Here's the test plan I followed:

1. Create a LDAP user (uid=julian, userpassword=foobar)
2. Create a Koha user (userid=julian, password=julian)
3. Create another Koha (local) user (userid=123, password=123)
3. Enable LDAP in $KOHA_CONF with auth_by_bind = 1, and anonymous_bind = 0
4. Update source code to current master

5. Login with julian/foobar => success => OK
6. Login with julian/julian => failure => OK
7. Login with 123/123 => failure => NOT OK
8. Set anonymous_bind = 1 in $KOHA_CONF
9. Login with julian/foobar => success => OK
10. Login with julian/julian => failure => OK
10. Login with 123/123 => success => OK
11. So, here I can confirm the problem for local login when auth_by_bind = 1 and anonymous_bind = 1, let's apply the patch...

12. Apply the patch
13. Reset anonymous_bind to 0
14. Login with julian/foobar => success => OK
15. Login with julian/julian => success => ??
16. Login with 123/123 => success => OK
17. Set anonymous_bind to 1
18. Login with julian/foobar => success => OK
19. Login with julian/julian => failure => OK
20. Login with 123/123 => success => OK

So the patch fixes the problem, but allows LDAP users to login with their LDAP password and their local password.
But this should be fixed by bug 9165, so I will QA this patch.
Comment 22 Julian Maurice 2014-11-27 10:53:44 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2014-11-27 12:54:51 UTC
Very thorough and well explained, cheers Julian.

You're spot on with point 15 being exactly where bug 9165 comes in.  

Unfortunately 9165, even with my follow-up, isn't an instant solution as it relies on one successful ldap login per user, after the config preference is updated, to remove the local password and therefore disallow local fallback. I think this is the best possibly solution however with our current workflows.

I'll clarify the above point in the other bug to make sure people are aware of this.
Comment 24 Tomás Cohen Arazi 2014-11-27 17:47:38 UTC
Created attachment 34002 [details] [review]
Bug 12831: (RM followup) regression test

checkpw_ldap should return 0 if it is not an anonymous bind, and authentication
fails. This is better explained on the bug comments. This is just a regression
test for the revised functionality.

To test:
- Run
  $ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: it fails because C4::Auth_with_ldap doesn't match the expected behaviour
- Apply the bugfix from Martin
- Run
  $ prove t/db_dependent/Auth_with_ldap.t
=> SUCCESS: tests now pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 25 Tomás Cohen Arazi 2014-11-27 17:47:48 UTC
Created attachment 34003 [details] [review]
Bug 12831: Local Only logins with LDAP

Local only logins should continue to function when LDAP is enabled.
This was not the case after bug 8148 [LDAP Auth should FAIL when ldap
contains a NEW password].  For this case, we need to diferentiate
between local accounts and ldap accounts.  This is somewhat challenging
and thus this patch is only part of the story.

The other half can be achieved with bug 9165

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 26 Tomás Cohen Arazi 2014-11-27 18:04:48 UTC
Patches pushed to master.

Thanks Martin!