Bug 6979 - LDAP authentication fails during password comparison
Summary: LDAP authentication fails during password comparison
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: Other Linux
: P1 - high major (vote)
Assignee: Alex Arnaud
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 19:37 UTC by Robert Fox
Modified: 2017-08-23 10:55 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
LDAP USER NOTICE: The option to integrate LDAP via "auth by password" has been removed. Please update your LDAP integration setting to use "auth by bind" instead.


Attachments
Patch for Bug 6979 - Auth_with_ldap.pm in C4 directory (3.23 KB, patch)
2011-10-05 21:00 UTC, Robert Fox
Details | Diff | Splinter Review
Bug #6979 (2.19 KB, patch)
2016-01-14 10:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug #6979 (2.19 KB, patch)
2016-01-14 14:25 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Update tests (8.69 KB, patch)
2016-01-14 16:49 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.57 KB, patch)
2016-01-15 15:09 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.73 KB, patch)
2016-01-19 13:25 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug #6979 (2.19 KB, patch)
2016-11-15 10:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Update tests (8.66 KB, patch)
2016-11-15 10:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.68 KB, patch)
2016-11-15 10:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Fix Already in a transaction error (1.15 KB, patch)
2016-11-15 10:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug #6979 (2.19 KB, patch)
2016-12-05 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Update tests (8.68 KB, patch)
2016-12-05 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.68 KB, patch)
2016-12-05 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Fix Already in a transaction error (1.12 KB, patch)
2016-12-05 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug #6979 (2.19 KB, patch)
2017-03-20 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Update tests (8.71 KB, patch)
2017-03-20 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.68 KB, patch)
2017-03-20 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6979 - Fix Already in a transaction error (1.10 KB, patch)
2017-03-20 13:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug #6979 (2.25 KB, patch)
2017-03-20 14:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 6979 - Update tests (8.77 KB, patch)
2017-03-20 14:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 6979 - Handle multiple branches in non-auth_by_bin (5.75 KB, patch)
2017-03-20 14:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 6979 - Fix Already in a transaction error (1.16 KB, patch)
2017-03-20 14:27 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Fox 2011-10-05 19:37:44 UTC
Password is failing during comparison in Auth_with_ldap.pm in code (between lines 140 and 165) in this call:

my $cmpmesg = $db->compare( $userldapentry, attr=>'userpassword', value => $password );

This was failing 100% of the time, even if a correct password was submitted with:

"LDAP Auth rejected : invalid password for user ..."

The attribute comparison is not always a valid way to check the password validity because not all LDAP databases support the userpassword attribute. Also, many LDAP databases require a valid DN string from the user as opposed to the uid for authentication purposes.

I have a fix for this that does a recursive lookup of the user's DN in the LDAP database, and then uses that DN to perform a bind in a similar manner to the auth_by_bind method. The auth_by_bind method should also be changed so that it uses the retrieved DN of the user to perform a bind against the LDAP server.

I plan on submitting a code revision for this and soliciting feedback.
Comment 1 Robert Fox 2011-10-05 21:00:03 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-10-19 13:44:58 UTC
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.
Comment 3 Jonathan Druart 2015-02-16 12:30:37 UTC
Is it still valid or can be closed? cc Martin
Comment 4 Mike Gabriel 2015-10-04 15:14:11 UTC
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...
Comment 5 Martin Renvoize 2015-10-05 08:17:28 UTC
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.
Comment 6 Martin Renvoize 2015-10-05 08:18:59 UTC
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?
Comment 7 Mike Gabriel 2015-10-05 08:50:01 UTC
(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
Comment 8 Mike Gabriel 2015-10-05 08:51:01 UTC
(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
Comment 9 Alex Arnaud 2016-01-13 15:44:06 UTC
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.
Comment 10 Alex Arnaud 2016-01-14 10:29:07 UTC Comment hidden (obsolete)
Comment 11 Alex Arnaud 2016-01-14 14:25:18 UTC
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.
Comment 12 Alex Arnaud 2016-01-14 16:49:39 UTC
Created attachment 46659 [details] [review]
Bug 6979 - Update tests
Comment 13 Alex Arnaud 2016-01-15 15:09:21 UTC Comment hidden (obsolete)
Comment 14 Alex Arnaud 2016-01-19 13:25:45 UTC
Created attachment 46863 [details] [review]
Bug 6979 - Handle multiple branches in non-auth_by_bin
Comment 15 Manuel Flores 2016-05-12 23:10:37 UTC
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.
Comment 16 Martin Renvoize 2016-05-26 06:06:27 UTC
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 17 M. Tompsett 2016-08-13 16:27:13 UTC
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?
Comment 18 Martin Renvoize 2016-08-14 07:21:11 UTC
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.
Comment 19 Alex Arnaud 2016-11-15 10:32:01 UTC
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.
Comment 20 Alex Arnaud 2016-11-15 10:32:11 UTC
Created attachment 57481 [details] [review]
Bug 6979 - Update tests
Comment 21 Alex Arnaud 2016-11-15 10:32:18 UTC
Created attachment 57482 [details] [review]
Bug 6979 - Handle multiple branches in non-auth_by_bin
Comment 22 Alex Arnaud 2016-11-15 10:32:27 UTC
Created attachment 57483 [details] [review]
Bug 6979 - Fix Already in a transaction error
Comment 23 Alex Arnaud 2016-11-15 10:32:50 UTC
Patch rebased on master
Comment 24 Alex Buckley 2016-12-02 02:31:53 UTC
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
Comment 25 Alex Arnaud 2016-12-05 13:29:29 UTC Comment hidden (obsolete)
Comment 26 Alex Arnaud 2016-12-05 13:29:37 UTC Comment hidden (obsolete)
Comment 27 Alex Arnaud 2016-12-05 13:29:46 UTC Comment hidden (obsolete)
Comment 28 Alex Arnaud 2016-12-05 13:29:53 UTC Comment hidden (obsolete)
Comment 29 Alex Arnaud 2016-12-05 13:31:06 UTC
Hello Alex,

Patches are rebased on master. You can test again.
Comment 30 Alex Buckley 2016-12-05 19:52:22 UTC
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
Comment 31 Hugo Agud 2017-01-25 18:00:33 UTC
I have tested the patch and it works fine!

I am not able to generate the signed patch, sorry
Comment 32 Alex Arnaud 2017-01-26 11:34:15 UTC
(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 ?
Comment 33 Hugo Agud 2017-01-26 11:39:06 UTC
I am still learning to create sign-off patch with kohadevbox, I have pending a trainning ;)
Comment 34 Martin Renvoize 2017-03-20 12:24:27 UTC
Any chance of a quick rebase Alex? I'm attempting to QA and I have a SHA1 missing error here ;)
Comment 35 Alex Arnaud 2017-03-20 13:29:24 UTC
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.
Comment 36 Alex Arnaud 2017-03-20 13:29:34 UTC
Created attachment 61296 [details] [review]
Bug 6979 - Update tests
Comment 37 Alex Arnaud 2017-03-20 13:29:42 UTC
Created attachment 61297 [details] [review]
Bug 6979 - Handle multiple branches in non-auth_by_bin
Comment 38 Alex Arnaud 2017-03-20 13:29:51 UTC
Created attachment 61298 [details] [review]
Bug 6979 - Fix Already in a transaction error
Comment 39 Martin Renvoize 2017-03-20 14:27:11 UTC
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>
Comment 40 Martin Renvoize 2017-03-20 14:27:17 UTC
Created attachment 61311 [details] [review]
Bug 6979 - Update tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2017-03-20 14:27:25 UTC
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>
Comment 42 Martin Renvoize 2017-03-20 14:27:58 UTC
Created attachment 61313 [details] [review]
Bug 6979 - Fix Already in a transaction error

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2017-03-20 14:31:19 UTC
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.
Comment 44 Kyle M Hall 2017-03-31 13:05:09 UTC
Pushed to master for 17.05, thanks Alex!
Comment 45 Katrin Fischer 2017-04-02 17:00:45 UTC
(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?
Comment 46 Marcel de Rooy 2017-05-01 09:43:33 UTC
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.