Bug 9611 - Changing the password hashing algorithm from MD5 to more secure Bcrypt
Summary: Changing the password hashing algorithm from MD5 to more secure Bcrypt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact:
URL:
Keywords:
: 11013 (view as bug list)
Depends on:
Blocks: 11013 12367
  Show dependency treegraph
 
Reported: 2013-02-14 00:29 UTC by Srikanth Dhondi
Modified: 2015-06-04 23:23 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt (8.35 KB, patch)
2013-02-14 02:14 UTC, Srikanth Dhondi
Details | Diff | Splinter Review
Bug 9611 : Database update, changing password from varchar(30) to varchar(60) (1.92 KB, patch)
2013-02-16 19:23 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9611 : Updating dependencies (903 bytes, patch)
2013-02-16 19:33 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9611 : Updating dependencies (916 bytes, patch)
2013-02-16 19:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt (1.87 KB, patch)
2013-02-19 02:18 UTC, Srikanth Dhondi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt (8.68 KB, patch)
2013-02-19 03:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611 : Database update, changing password from varchar(30) to varchar(60) (2.07 KB, patch)
2013-02-19 03:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611 : Updating dependencies (1004 bytes, patch)
2013-02-19 03:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt (2.14 KB, patch)
2013-02-19 03:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Followup Bug 9611 - Changing the password hashing algorithm (2.60 KB, patch)
2013-02-19 03:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt (8.77 KB, patch)
2013-02-19 04:25 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 : Updating dependencies (1.10 KB, patch)
2013-02-19 04:27 UTC, Mason James
Details | Diff | Splinter Review
Followup Bug 9611 - Changing the password hashing algorithm (2.72 KB, patch)
2013-02-19 04:28 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt (8.77 KB, patch)
2013-02-19 04:36 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt (8.77 KB, patch)
2013-02-19 04:36 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 : Database update, changing password from varchar(30) to varchar(60) (2.17 KB, patch)
2013-02-19 04:36 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 : Updating dependencies (1.10 KB, patch)
2013-02-19 04:37 UTC, Mason James
Details | Diff | Splinter Review
Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt (2.24 KB, patch)
2013-02-19 04:37 UTC, Mason James
Details | Diff | Splinter Review
Followup Bug 9611 - Changing the password hashing algorithm (2.72 KB, patch)
2013-02-19 04:37 UTC, Mason James
Details | Diff | Splinter Review
Database update, changing password from varchar(30) to varchar(60) (2.11 KB, patch)
2013-04-05 03:48 UTC, Chris Hall
Details | Diff | Splinter Review
Removing external dependency for password salting (4.05 KB, patch)
2013-04-05 05:09 UTC, Chris Hall
Details | Diff | Splinter Review
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (10.91 KB, patch)
2013-08-09 22:55 UTC, Galen Charlton
Details | Diff | Splinter Review
bug 9611: removing external dependency for password salt generator (3.78 KB, patch)
2013-08-09 22:56 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9611: Database update, changing password from varchar(30) to varchar(60) (2.18 KB, patch)
2013-08-09 22:56 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9611: add Crypt::Eksblowfish::Bcrypt to list of Perl dependencies (959 bytes, patch)
2013-08-09 22:56 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (10.98 KB, patch)
2013-08-23 04:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug 9611: removing external dependency for password salt generator (3.70 KB, patch)
2013-08-23 04:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (12.81 KB, patch)
2013-08-23 04:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() (4.11 KB, patch)
2013-08-26 06:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_9611: removed md5_base64 from imports - not used (682 bytes, patch)
2013-08-26 06:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_9611: use checkpw_hash() instead of md5 hash (2.17 KB, patch)
2013-08-26 06:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_9611: use hash_password() and checkpw_* instead of md5 hash (3.96 KB, patch)
2013-08-26 06:50 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_9611: use checkpw_hash() instead of md5 hash (2.24 KB, patch)
2013-09-30 00:27 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 9611: Database update, changing password from varchar(30) to varchar(60) (2.17 KB, patch)
2013-09-30 00:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611: add Crypt::Eksblowfish::Bcrypt to list of Perl dependencies (971 bytes, patch)
2013-09-30 02:50 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (12.83 KB, patch)
2013-09-30 02:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() (4.18 KB, patch)
2013-09-30 02:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] bug_9611: removed md5_base64 from imports - not used (762 bytes, patch)
2013-09-30 02:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] bug_9611: use hash_password() and checkpw_* instead of md5 hash (4.29 KB, patch)
2013-09-30 02:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] bug_9611: use checkpw_hash() instead of md5 hash (2.45 KB, patch)
2013-09-30 02:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9611: Database update, changing password from varchar(30) to varchar(60) (2.19 KB, patch)
2013-09-30 02:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9611 - followup to fix POD (1.06 KB, patch)
2013-09-30 02:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() (4.17 KB, patch)
2013-10-03 14:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9611: add Crypt::Eksblowfish::Bcrypt to list of Perl dependencies (958 bytes, patch)
2013-10-03 14:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (12.81 KB, patch)
2013-10-03 14:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() (4.17 KB, patch)
2013-10-03 14:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug_9611: removed md5_base64 from imports - not used (748 bytes, patch)
2013-10-03 14:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug_9611: use hash_password() and checkpw_* instead of md5 hash (4.24 KB, patch)
2013-10-03 14:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug_9611: use checkpw_hash() instead of md5 hash (2.44 KB, patch)
2013-10-03 14:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9611: Database update, changing password from varchar(30) to varchar(60) (2.18 KB, patch)
2013-10-03 14:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9611 - followup to fix POD (1.06 KB, patch)
2013-10-03 14:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9611: add Crypt::Eksblowfish::Bcrypt to list of Perl dependencies (1.00 KB, patch)
2013-10-03 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt (12.87 KB, patch)
2013-10-03 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() (4.23 KB, patch)
2013-10-03 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_9611: removed md5_base64 from imports - not used (806 bytes, patch)
2013-10-03 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_9611: use hash_password() and checkpw_* instead of md5 hash (4.30 KB, patch)
2013-10-03 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_9611: use checkpw_hash() instead of md5 hash (2.49 KB, patch)
2013-10-03 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9611: Database update, changing password from varchar(30) to varchar(60) (2.23 KB, patch)
2013-10-03 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9611 - followup to fix POD (1.11 KB, patch)
2013-10-03 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srikanth Dhondi 2013-02-14 00:29:12 UTC
In the current version of Koha, the passwords are stored as md5 hashes. MD5 hashing has its limitations, changing the hashing method to Bcrypt will improve in more secure password storage.
Comment 1 Srikanth Dhondi 2013-02-14 02:14:11 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2013-02-14 15:14:50 UTC
This patch add new perl dependencies, 
Crypt::Eksblowfish::Bcrypt
Crypt::Random::Source

They are not installed by default, so I think must be included in koha_perl_deps.pl.

Also, Crypt::Eksblowfish::Bcrypt is present on Ubuntu 12.04, but not the second which must be installed via cpan.

With those solved, koha-qa complains about tabulations.

Test with patch applied:
1) Old user can login
2) New user has "new" type of password
3) Old user with password update has "new" type of password

4) Old (updated) or New user CAN'T LOGIN

Log:
opac-user.pl: Use of uninitialized value in string ne at .../kohaclone/C4/Auth.pm line 671., referer: ...
opac-user.pl: Use of uninitialized value $pki_field in string eq at .../kohaclone/C4/Auth.pm line 757., referer: ...
opac-user.pl: Use of uninitialized value $pki_field in string eq at .../kohaclone/C4/Auth.pm line 757., referer: ...
opac-user.pl: Use of uninitialized value $retuserid in string ne at .../kohaclone/C4/Auth.pm line 793., referer: ...
Comment 3 Robin Sheat 2013-02-14 22:28:25 UTC
Crypt::Random::Source is in the koha-common repo now.
Comment 4 Kyle M Hall 2013-02-15 12:17:40 UTC
It looks like the new algorithm is used when changing passwords on the staff side, but not on the OPAC side. Also, you need to add the new dependencies to C4/Installer/PerlDependencies.pm
Comment 5 Bernardo Gonzalez Kriegel 2013-02-15 12:27:53 UTC
Anyone else tried the patch?
I want to know if the error I found is a problem of my install, or a real error.
Comment 6 Chris Cormack 2013-02-16 19:08:25 UTC
It's a real problem Bernardo, the length of the password column needs to be 60char not the 30 it is. 

I'll send a follow up, both patches will need to be applied together to test
Comment 7 Chris Cormack 2013-02-16 19:09:31 UTC
Kyle, i'll do a follow up for those 2 as well.
Comment 8 Chris Cormack 2013-02-16 19:17:32 UTC
Comment on attachment 15311 [details] [review]
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt

Review of attachment 15311 [details] [review]:
-----------------------------------------------------------------

::: C4/Members.pm
@@ -1406,5 @@
> -    my $number_rows = $sth->fetchrow;
> -    return $number_rows;
> -
> -}
> -

Srikanth, why did you remove this as part of this patch? Are you sure nothing else is using it, if so you should document why you removed it.
Comment 9 Chris Cormack 2013-02-16 19:23:52 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2013-02-16 19:33:16 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2013-02-16 19:36:09 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-02-16 19:42:12 UTC
Patch to change OPAC to follow, the others can be tested though
Comment 13 Bernardo Gonzalez Kriegel 2013-02-16 20:22:23 UTC
Good job!

1) All current (3) patches applied, run updatedatabase
2) Old user with old password can login
3) Old user with updated password can login
4) New user with new password can login

Login tested on OPAC and STAFF.
All works, no errors.

Pending OPAC password change.
Comment 14 Srikanth Dhondi 2013-02-19 02:18:22 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2013-02-19 03:12:14 UTC Comment hidden (obsolete)
Comment 16 Bernardo Gonzalez Kriegel 2013-02-19 03:12:24 UTC Comment hidden (obsolete)
Comment 17 Bernardo Gonzalez Kriegel 2013-02-19 03:12:33 UTC Comment hidden (obsolete)
Comment 18 Bernardo Gonzalez Kriegel 2013-02-19 03:12:43 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2013-02-19 03:14:22 UTC Comment hidden (obsolete)
Comment 20 Mason James 2013-02-19 04:25:15 UTC Comment hidden (obsolete)
Comment 21 Mason James 2013-02-19 04:27:25 UTC Comment hidden (obsolete)
Comment 22 Mason James 2013-02-19 04:28:47 UTC Comment hidden (obsolete)
Comment 23 Mason James 2013-02-19 04:36:12 UTC Comment hidden (obsolete)
Comment 24 Mason James 2013-02-19 04:36:48 UTC Comment hidden (obsolete)
Comment 25 Mason James 2013-02-19 04:36:54 UTC Comment hidden (obsolete)
Comment 26 Mason James 2013-02-19 04:37:01 UTC Comment hidden (obsolete)
Comment 27 Mason James 2013-02-19 04:37:07 UTC Comment hidden (obsolete)
Comment 28 Mason James 2013-02-19 04:37:15 UTC Comment hidden (obsolete)
Comment 29 Mason James 2013-02-19 04:46:36 UTC
passing-QA on these 5 patches...

i've tested these, and they work as stated. 

awesome patch Srikanth, well done everyone
Comment 30 Mason James 2013-02-19 04:49:01 UTC
(In reply to comment #29)
> passing-QA on these 5 patches...

ps: sorry about the multiple attempts to upload my signed-of patches, 

i just worked out how to git-bz-attach a rangle of commits
Comment 31 Jared Camins-Esakov 2013-02-20 15:29:40 UTC
I have a question about the use of Any::Moose. I know in some circumstances there can be a conflict between Any::Moose and direct Moose/Mouse loads. Is that a concern here between this and the Solr code?
Comment 32 Chris Cormack 2013-02-20 15:59:51 UTC
Where did you see Any::Moose? In one of the cpan modules ?
Comment 33 Jared Camins-Esakov 2013-02-20 16:50:14 UTC
(In reply to comment #32)
> Where did you see Any::Moose? In one of the cpan modules ?

It's required by Crypt::Random::Source.
Comment 34 Chris Cormack 2013-02-21 18:40:10 UTC
Ahh in that case I suspect we want a better random generator.
Comment 35 Jared Camins-Esakov 2013-02-24 15:02:00 UTC
(In reply to comment #34)
> Ahh in that case I suspect we want a better random generator.

I think so.
Comment 36 Robin Sheat 2013-02-25 01:22:22 UTC
A very perfunctory search didn't find anything suitable as a replacement. Are the problematic circumstances the sort of thing that we're likely to encounter? If so, then we'll have to find something else of course. However, there won't be too many that are very pluggable like this is. I suppose we could be OK using a pseudo-random source for salts if push came to shove.
Comment 37 Mason James 2013-04-03 00:37:38 UTC
(In reply to comment #35)
> (In reply to comment #34)
> > Ahh in that case I suspect we want a better random generator.
> 
> I think so.

hmm, how about Crypt::Random::TESHA2? 

it's a minimal and portable module 
http://search.cpan.org/~danaj/Crypt-Random-TESHA2-0.01/lib/Crypt/Random/TESHA2.pm
Comment 38 Robin Sheat 2013-04-03 00:40:53 UTC
> This module implements userspace voodoo entropy. You should use a proper O/S supplied entropy source such as /dev/random or the Win32 Crypt API.
Comment 39 Mason James 2013-04-03 00:45:16 UTC
(In reply to comment #37)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > Ahh in that case I suspect we want a better random generator.
> > 
> > I think so.
> 
> hmm, how about Crypt::Random::TESHA2? 
> it's a minimal and portable module 

hmmm, Bytes::Random::Secure looks even better...

"Prior to version 0.20, a heavy dependency chain was required for reliably and securely seeding the ISAAC generator. Earlier versions required Crypt::Random::Source, which in turn required Any::Moose. 

Thanks to Dana Jacobsen's new Crypt::Random::Seed module, this situation has been resolved. So if you're looking for a secure random bytes solution that "just works" portably, and on Perl versions as far back as 5.6.0, you've come to the right place."

http://search.cpan.org/~davido/Bytes-Random-Secure-0.25/lib/Bytes/Random/Secure.pm
Comment 40 Chris Hall 2013-04-05 03:47:14 UTC
Hey Mason

http://deps.cpantesters.org/?module=Bytes::Random::Secure

Correct me if I am wrong, but a quick glance suggests we would have to package bytes::random::secure, scalar::util, Crypt::random::seed, Crypt::random::tesha2 and maybe some of their dependencies.

The closest thing I could find that was also in squeeze was crypt::openssl::random, but as this requires a decent seed (so that we get unique values).
I was going to use /dev/urandom for the seed value, however this also suffices for use as a salt so I just cut out the middleman.
Comment 41 Chris Hall 2013-04-05 03:48:49 UTC Comment hidden (obsolete)
Comment 42 Chris Hall 2013-04-05 05:09:22 UTC Comment hidden (obsolete)
Comment 43 Galen Charlton 2013-08-09 22:55:50 UTC Comment hidden (obsolete)
Comment 44 Galen Charlton 2013-08-09 22:56:00 UTC Comment hidden (obsolete)
Comment 45 Galen Charlton 2013-08-09 22:56:09 UTC Comment hidden (obsolete)
Comment 46 Galen Charlton 2013-08-09 22:56:21 UTC Comment hidden (obsolete)
Comment 47 Galen Charlton 2013-08-09 23:03:42 UTC
I've squashed the patch series into a set of four that reduce the flapping while retaining author attribution, and have signed off on them.

I am not, however, setting this bug to signed off, but to assigned.  This is because patron records using the new hash would be unable to authenticate using SIP or ILS-DI.  Note that the following files are using md5_base64 exclusive when comparing hashes:

C4/SIP/ILS/Patron.pm
C4/ILSDI/Utility.pm

Also, although of less import, C4/Auth_with_ldap.pm isn't using the new hashing style either when caching the password.

Consequently, follow-ups are needed.  I may poke at this some more this weekend, but the field is free if anybody else wants to work on it.

I'm inclined to think that it may be time to get started on a Koha::Auth module, if only for the initial reason of creating a home for a single password verification routine.
Comment 48 Galen Charlton 2013-08-09 23:06:01 UTC
Also, just to raise the inevitable question: do we care about the portability issues of using /dev/random and /dev/urandom?
Comment 49 Srdjan Jankovic 2013-08-23 04:36:28 UTC Comment hidden (obsolete)
Comment 50 Srdjan Jankovic 2013-08-23 04:41:53 UTC Comment hidden (obsolete)
Comment 51 Srdjan Jankovic 2013-08-23 04:56:41 UTC Comment hidden (obsolete)
Comment 52 Srdjan Jankovic 2013-08-26 06:48:21 UTC Comment hidden (obsolete)
Comment 53 Srdjan Jankovic 2013-08-26 06:48:32 UTC Comment hidden (obsolete)
Comment 54 Srdjan Jankovic 2013-08-26 06:48:43 UTC Comment hidden (obsolete)
Comment 55 Srdjan Jankovic 2013-08-26 06:50:45 UTC Comment hidden (obsolete)
Comment 56 Srdjan Jankovic 2013-08-26 06:52:30 UTC
See also http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10781
Comment 57 Bernardo Gonzalez Kriegel 2013-09-29 00:57:52 UTC
(In reply to Srdjan Jankovic from comment #55)
> I do not have a LDAP facility, so I cheated. I ran
> perl -e 'use C4::Auth_with_ldap;
> C4::Auth_with_ldap::_do_changepassword("srdjan", 1000022259, "srdjan");'
> and was able to change the password.

This part of the cheat I can't replicate. 
Could you explain a little more?
Comment 58 Robin Sheat 2013-09-29 02:37:54 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #57)
> This part of the cheat I can't replicate. 
> Could you explain a little more?

What happens when you try to replicate it? It seems to me like it should work, though I haven't tested it myself. 

It's just loading the module and calling the password changing function directly, as a real LDAP auth process would.
Comment 59 Bernardo Gonzalez Kriegel 2013-09-29 03:14:37 UTC
(In reply to Robin Sheat from comment #58)
> (In reply to Bernardo Gonzalez Kriegel from comment #57)
> > This part of the cheat I can't replicate. 
> > Could you explain a little more?
> 
> What happens when you try to replicate it? It seems to me like it should
> work, though I haven't tested it myself. 
> 
> It's just loading the module and calling the password changing function
> directly, as a real LDAP auth process would.

If I feed 
perl -e 'use C4::Auth_with_ldap; C4::Auth_with_ldap::_do_changepassword("test", 12345, "test");'
with current password, it returns nothing, and pass is not updated

If I put a new password, last argument I suppose, then it returns 
Password mismatch after update to borrowernumber=12345 at /home/bgkriegel/kohaclone/C4/Auth_with_ldap.pm line 274, <DATA> line 522.
and pass is not updated

I enabled ldap changing to 1 <useldapserver>1</useldapserver>
and just copy/paste ladp server conf from perldoc. And did this
because the oneliner do not run if there is no configuration.

So, perhaps I'm just doing something wrong, I don't know.
Comment 60 I'm just a bot 2013-09-29 04:31:09 UTC
Applying: bug_9611: use checkpw_hash() instead of md5 hash
Using index info to reconstruct a base tree...
M	C4/SIP/ILS/Patron.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/SIP/ILS/Patron.pm
CONFLICT (content): Merge conflict in C4/SIP/ILS/Patron.pm
Patch failed at 0001 bug_9611: use checkpw_hash() instead of md5 hash
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 61 Srdjan Jankovic 2013-09-30 00:27:00 UTC Comment hidden (obsolete)
Comment 62 Srdjan Jankovic 2013-09-30 00:45:49 UTC
> If I feed 
> perl -e 'use C4::Auth_with_ldap;
> C4::Auth_with_ldap::_do_changepassword("test", 12345, "test");'
> with current password, it returns nothing, and pass is not updated
> 
> If I put a new password, last argument I suppose, then it returns 
> Password mismatch after update to borrowernumber=12345 at
> /home/bgkriegel/kohaclone/C4/Auth_with_ldap.pm line 274, <DATA> line 522.
> and pass is not updated
> 

12345 should be the borrowernumber for the user 'test'. Is that the case?

> I enabled ldap changing to 1 <useldapserver>1</useldapserver>
> and just copy/paste ladp server conf from perldoc. And did this
> because the oneliner do not run if there is no configuration.

No, this scripts goes directly to Auth_with_ldap.pm, the purpose was to avoid checking the config and doing the LDAP bit so we can test password change only without having to connect to an LDAP server.

> 
> So, perhaps I'm just doing something wrong, I don't know.

No, you are doing the right thing, just please check the borrowernumber.
Comment 63 Srdjan Jankovic 2013-09-30 00:56:05 UTC Comment hidden (obsolete)
Comment 64 Bernardo Gonzalez Kriegel 2013-09-30 02:25:21 UTC
(In reply to Srdjan Jankovic from comment #62)
> No, this scripts goes directly to Auth_with_ldap.pm, the purpose was to
> avoid checking the config and doing the LDAP bit so we can test password
> change only without having to connect to an LDAP server.
> 
> > 
> > So, perhaps I'm just doing something wrong, I don't know.
> 
> No, you are doing the right thing, just please check the borrowernumber.

Well, not so :)
I need to add <ldapserver> stanza, if not

perl -e 'use C4::Auth_with_ldap; C4::Auth_with_ldap::_do_changepassword("test", 12345, "xxxx");'
No "ldapserver" in server hash from KOHA_CONF: /home/bgkriegel/kohadev/etc/koha-conf.xml at /home/bgkriegel/kohaclone/C4/Auth_with_ldap.pm line 57, <DATA> line 522.
Compilation failed in require at -e line 1, <DATA> line 522.
BEGIN failed--compilation aborted at -e line 1, <DATA> line 522.

But my previous problem was not using correct borrowernumber, so it's ready
I'll re-upload all, with a followup to fix a small POD error
Comment 65 Srdjan Jankovic 2013-09-30 02:29:03 UTC
 the borrowernumber.
> 
> Well, not so :)
> I need to add <ldapserver> stanza, if not
> 
> perl -e 'use C4::Auth_with_ldap;
> C4::Auth_with_ldap::_do_changepassword("test", 12345, "xxxx");'
> No "ldapserver" in server hash from KOHA_CONF:
> /home/bgkriegel/kohadev/etc/koha-conf.xml at
> /home/bgkriegel/kohaclone/C4/Auth_with_ldap.pm line 57, <DATA> line 522.
> Compilation failed in require at -e line 1, <DATA> line 522.
> BEGIN failed--compilation aborted at -e line 1, <DATA> line 522.
> 

Oops, sorry about that, was some time ago, and my memory is perishable obviously.
Comment 66 Bernardo Gonzalez Kriegel 2013-09-30 02:50:56 UTC Comment hidden (obsolete)
Comment 67 Bernardo Gonzalez Kriegel 2013-09-30 02:51:15 UTC Comment hidden (obsolete)
Comment 68 Bernardo Gonzalez Kriegel 2013-09-30 02:51:31 UTC Comment hidden (obsolete)
Comment 69 Bernardo Gonzalez Kriegel 2013-09-30 02:51:51 UTC Comment hidden (obsolete)
Comment 70 Bernardo Gonzalez Kriegel 2013-09-30 02:52:07 UTC Comment hidden (obsolete)
Comment 71 Bernardo Gonzalez Kriegel 2013-09-30 02:52:24 UTC Comment hidden (obsolete)
Comment 72 Bernardo Gonzalez Kriegel 2013-09-30 02:52:51 UTC Comment hidden (obsolete)
Comment 73 Bernardo Gonzalez Kriegel 2013-09-30 02:56:32 UTC Comment hidden (obsolete)
Comment 74 Bernardo Gonzalez Kriegel 2013-09-30 03:09:32 UTC
Last comment: for QA sake, perhaps you can summarize in a comment all tests that need to be done.
Comment 75 Srdjan Jankovic 2013-09-30 04:12:29 UTC
That's why I prefer testing instructions in the comments rather than in the commits. Maybe the best way would be to have a separate attachment.
Anyway, by popular demand:

Test plan:
  1) Add new users and check whether their passwords are stored as Bcrypt 
     hashes or not (directly on the database).
  2) To test that authentication works for both old as well as new users:
       a) Login as an existing user whose password is stored as a MD5 hash
       b) Login as an existing user whose password is stored as a Bcrypt hash
  3) In the staff interface, change the password of an existing user
     whose password is stored as an MD5 hash
    a) Check the new password is stored as a Bcrypt-hash in the database
    b) Try to login with the new password
  4) In the OPAC, verify that
    a) Old user with old pass can change password, new format
    b) New user with new pass can change password
    c) Old and new user with self-updated pass can login
  5) SIP: Have an old user and create a new user
    a) use either tenet sip test or C4/SIP/interactive_patron_check_password.pl 
       to check old userid/password
    b) do the same for the new user
  6) LDAP:
     - Turn on LDAP auth in koha-config.xml.
     - Set "update" in your server config to 1
    a) Change user's password on LDAP
    b) Login to Koha using LDAP - Koha password should be updated. To check
       turn off LDAP auth in koha-config.xml. You should be ble to log in with
       the new password
Comment 76 Katrin Fischer 2013-10-03 14:17:51 UTC Comment hidden (obsolete)
Comment 77 Katrin Fischer 2013-10-03 14:18:54 UTC
Hi Kyle, I think I was able to fix the conflict - It was removing C4::Utils that has gone into master very recently.
Comment 78 Katrin Fischer 2013-10-03 14:19:52 UTC
Hm, but i messed up attaching the rebased patch, trying again.
Comment 79 Katrin Fischer 2013-10-03 14:21:19 UTC Comment hidden (obsolete)
Comment 80 Katrin Fischer 2013-10-03 14:21:34 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2013-10-03 14:21:44 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2013-10-03 14:21:53 UTC Comment hidden (obsolete)
Comment 83 Katrin Fischer 2013-10-03 14:22:05 UTC Comment hidden (obsolete)
Comment 84 Katrin Fischer 2013-10-03 14:22:23 UTC Comment hidden (obsolete)
Comment 85 Katrin Fischer 2013-10-03 14:22:33 UTC Comment hidden (obsolete)
Comment 86 Katrin Fischer 2013-10-03 14:22:47 UTC Comment hidden (obsolete)
Comment 87 Katrin Fischer 2013-10-03 14:24:58 UTC
The conflict was in  bug_9611: use hash_password() and checkpw_* instead of md5 hash at the very beginning. I think it was caused by this very recent change:
http://git.koha-community.org/gitweb/?p=koha.git;a=blobdiff;f=C4/Auth_with_ldap.pm;h=0efeb95dca806015fbcbc9f05e7227a9d93138a3;hp=d7a5e9a3cfe588359e2cee593b939fdd00c8900c;hb=561107bb5b348eaa14054e3470f39ff9cf080d22;hpb=7e3f8e0838584a89f3fbdce8e956880de8556d7a

Hoping it's ok :)
Comment 88 Kyle M Hall 2013-10-03 16:48:17 UTC
Created attachment 21789 [details] [review]
Bug 9611: add Crypt::Eksblowfish::Bcrypt to list of Perl dependencies

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 89 Kyle M Hall 2013-10-03 16:48:27 UTC
Created attachment 21790 [details] [review]
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt

What this patch aims to accomplish?

 * All new passwords are stored as Bcrypt-hashes
 * For password verification:
     - If the user was created before this patch was applied then use
        MD5 to hash the entered password <-- backwards compatibility
     - If the user was created after this patch was applied then use
       Bcrypt to hash the entered password
 * Any password change made via the staff interface or the OPAC will
   be automatically Bcrypt-hashed; this applies to old users whose
   passwords were stored as MD5 hashes previously

Test plan:
  1) Add new users and check whether their passwords are stored as
     Bcrypt hashes or not.
  2) To test that authentication works for both old as well as new
     users:
       a) Login as an existing user whose password is stored as a
          MD5 hash
       b) Login as an existing user whose password is stored as a
          Bcrypt hash
  3) In the staff interface, change the password of an existing user
     whose password is stored as an MD5 hash
	a) Check the new password is stored as a Bcrypt-hash in the database
	b) Try to login with the new password
  4) In the OPAC, verify that
    a) Old user with old pass can change password, new format
    b) New user with new pass can change password
    c) Old and new user with self-updated pass can login

Whitespace cleanup was contributed by  Bernardo Gonzalez Kriegel.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 90 Kyle M Hall 2013-10-03 16:48:52 UTC
Created attachment 21791 [details] [review]
bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw()

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 91 Kyle M Hall 2013-10-03 16:49:03 UTC
Created attachment 21792 [details] [review]
bug_9611: removed md5_base64 from imports - not used

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 92 Kyle M Hall 2013-10-03 16:49:13 UTC
Created attachment 21793 [details] [review]
bug_9611: use hash_password() and checkpw_* instead of md5 hash

Test:

* LDAP:
- Turn on LDAP auth in koha-config.xml. Sset "update" in your server config to 1
- Change user's password on LDAP
- Login to Koha using LDAP - Koha password should be updated, to check
- Turn off LDAP auth in koha-config.xml
- You should be ble to log in with the new password

I do not have a LDAP facility, so I cheated. I ran
perl -e 'use C4::Auth_with_ldap; C4::Auth_with_ldap::_do_changepassword("srdjan", 1000022259, "srdjan");'
and was able to change the password.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described.

Test
1) change <useldapserver> to 1
2) copy/paste sample <ldapserver> config from perldoc C4/Auth_with_ldap
3) using sample script was able to change password,
use (userid, borrowernumber, newpass) as arguments
4) checked with OPAC and in database

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 93 Kyle M Hall 2013-10-03 16:49:23 UTC
Created attachment 21794 [details] [review]
bug_9611: use checkpw_hash() instead of md5 hash

Test:

* SIP: Have an old user and create a new user
- use either tenet sip test or
  C4/SIP/interactive_patron_check_password.pl to check old
  userid/password
- do the same for the new user

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described

Test
1) using perl C4/SIP/interactive_patron_check_password.pl
can check current (short) and new (long) passwords

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 94 Kyle M Hall 2013-10-03 16:49:33 UTC
Created attachment 21795 [details] [review]
Bug 9611: Database update, changing password from varchar(30) to varchar(60)

This is necessary because Bcrypt hashes are longer than MD5 hashes.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 95 Kyle M Hall 2013-10-03 16:50:12 UTC
Created attachment 21796 [details] [review]
Bug 9611 - followup to fix POD

Small patch to make koha-qa happy.
Fixes small POD error

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
All patches pass koha-qa.pl, works as advertised!
Comment 96 Galen Charlton 2013-10-04 00:18:51 UTC
Pushed to master.  Thanks, Srikanth!
Comment 97 Robin Sheat 2013-10-09 07:38:24 UTC
*** Bug 11013 has been marked as a duplicate of this bug. ***