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.
Created attachment 15311 [details] [review] Bug 9611 - Changing the 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 will be automatically Bcrypt-hashed, this applies to old members 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 members 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) Change the password of an existing member 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
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: ...
Crypt::Random::Source is in the koha-common repo now.
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
Anyone else tried the patch? I want to know if the error I found is a problem of my install, or a real error.
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
Kyle, i'll do a follow up for those 2 as well.
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.
Created attachment 15427 [details] [review] Bug 9611 : Database update, changing password from varchar(30) to varchar(60)
Created attachment 15428 [details] [review] Bug 9611 : Updating dependencies
Created attachment 15429 [details] [review] Bug 9611 : Updating dependencies
Patch to change OPAC to follow, the others can be tested though
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.
Created attachment 15505 [details] [review] Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt
Created attachment 15506 [details] [review] [SIGNED-OFF] Bug 9611 - Changing the 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 will be automatically Bcrypt-hashed, this applies to old members 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 members 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) Change the password of an existing member 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 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. Small tabulation errors fixed in followup. Test with patches 1-3 applied, run updatedatabase 1) Old user can login 2) New user can login 3) User with updated password can login 4) Inspection of DB shows different passwords length
Created attachment 15507 [details] [review] [SIGNED-OFF] Bug 9611 : Database update, changing password from varchar(30) to varchar(60) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Fixed small mering conflict.
Created attachment 15508 [details] [review] [SIGNED-OFF] Bug 9611 : Updating dependencies Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No comment.
Created attachment 15509 [details] [review] [SIGNED-OFF] Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. No errors Test: 1) Old user with old pass can change password, new format 2) New user with new pass can change password 3) Old and new user with self-updated pass can login
Created attachment 15510 [details] [review] Followup Bug 9611 - Changing the password hashing algorithm Fixes tabulations
Created attachment 15514 [details] [review] Bug 9611 - Changing the 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 will be automatically Bcrypt-hashed, this applies to old members 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 members 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) Change the password of an existing member 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 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. Small tabulation errors fixed in followup. Test with patches 1-3 applied, run updatedatabase 1) Old user can login 2) New user can login 3) User with updated password can login 4) Inspection of DB shows different passwords length Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15515 [details] [review] Bug 9611 : Updating dependencies Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No comment. Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15516 [details] [review] Followup Bug 9611 - Changing the password hashing algorithm Fixes tabulations Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15517 [details] [review] Bug 9611 - Changing the 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 will be automatically Bcrypt-hashed, this applies to old members 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 members 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) Change the password of an existing member 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 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. Small tabulation errors fixed in followup. Test with patches 1-3 applied, run updatedatabase 1) Old user can login 2) New user can login 3) User with updated password can login 4) Inspection of DB shows different passwords length Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15518 [details] [review] Bug 9611 - Changing the 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 will be automatically Bcrypt-hashed, this applies to old members 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 members 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) Change the password of an existing member 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 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. Small tabulation errors fixed in followup. Test with patches 1-3 applied, run updatedatabase 1) Old user can login 2) New user can login 3) User with updated password can login 4) Inspection of DB shows different passwords length Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15519 [details] [review] Bug 9611 : Database update, changing password from varchar(30) to varchar(60) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Fixed small mering conflict. Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15520 [details] [review] Bug 9611 : Updating dependencies Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No comment. Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15521 [details] [review] Bug 9611 - Changing the OPAC password hashing algorithm from MD5 to Bcrypt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. No errors Test: 1) Old user with old pass can change password, new format 2) New user with new pass can change password 3) Old and new user with self-updated pass can login Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 15522 [details] [review] Followup Bug 9611 - Changing the password hashing algorithm Fixes tabulations Signed-off-by: Mason James <mtj@kohaaloha.com>
passing-QA on these 5 patches... i've tested these, and they work as stated. awesome patch Srikanth, well done everyone
(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
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?
Where did you see Any::Moose? In one of the cpan modules ?
(In reply to comment #32) > Where did you see Any::Moose? In one of the cpan modules ? It's required by Crypt::Random::Source.
Ahh in that case I suspect we want a better random generator.
(In reply to comment #34) > Ahh in that case I suspect we want a better random generator. I think so.
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.
(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
> 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.
(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
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.
Created attachment 17203 [details] [review] Database update, changing password from varchar(30) to varchar(60) Attached is the second patch rebased so that it will apply cleanly against current master.
Created attachment 17204 [details] [review] Removing external dependency for password salting I have written a wrapper around /dev/urandom and /dev/random that will give back a specified number of bytes salting purposes, however this can be used anytime a pseudo-random number is needed within Koha. /dev/urandom should be sufficient for password general salting, /dev/random is more suited when higher entropy is needed (if we ever use server salts). This patch removes the Crypt::Random::Source dependency that was mentioned in the 'Updating dependencies' patch, it may be useful to squash this patch set down but I did not do so as I didn't want to remove authorship details. Testplan: In current master (before applying this patch) create a new user. Login to the koha mysql database (sudo koha-mysql instance) and run the following query: select userid, password from borrowers where userid='username'; The output should be something like: patron | vdpWxEZTtVVPhZSAq1NIMw Apply the patch series on this bug Change the users password from within koha (for testing it is fine to change it to the same password) Run the above query again and observe the output: patron | $2a$08$U93rGVfvcV0YUNhJY.so3OkNL46bGBrIR3ugyskXLIJY5aMD8ENme Notice that the new password is longer, but also that all passwords generated by this patch series should begin with '$2a$08$'. If we change the password again in the interface to the same password and run out database query again, we should get a different value in the password field (although it will still have the '$2a$08$' prefix). Attempt to login as the user using the password you just set. This patch series fails if any of the following occur: you cannot change a password you cannot login the new passwords (viewing them from within the database) do not start with "$2a$08$" changing the password twice to the same value (say, "testing") results in the same password value being stored in the database Otherwise it is a pass.
Created attachment 20252 [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>
Created attachment 20253 [details] [review] bug 9611: removing external dependency for password salt generator To address packaging issues with Crypt::Random::Source, this patch replaces the salt generation with a wrapper for /dev/urandom and /dev/random. The test plan for this patch is the same as that for the base patch for bug 9611. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 20254 [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>
Created attachment 20255 [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>
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.
Also, just to raise the inevitable question: do we care about the portability issues of using /dev/random and /dev/urandom?
Created attachment 20576 [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>
Created attachment 20577 [details] [review] bug 9611: removing external dependency for password salt generator To address packaging issues with Crypt::Random::Source, this patch replaces the salt generation with a wrapper for /dev/urandom and /dev/random. The test plan for this patch is the same as that for the base patch for bug 9611. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 20578 [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>
Created attachment 20621 [details] [review] bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw()
Created attachment 20622 [details] [review] bug_9611: removed md5_base64 from imports - not used
Created attachment 20623 [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
Created attachment 20624 [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.
See also http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10781
(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?
(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.
(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.
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".
Created attachment 21614 [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
> 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.
Created attachment 21615 [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>
(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
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.
Created attachment 21621 [details] [review] [SIGNED-OFF] 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>
Created attachment 21622 [details] [review] [SIGNED-OFF] 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>
Created attachment 21623 [details] [review] [SIGNED-OFF] bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21624 [details] [review] [SIGNED-OFF] bug_9611: removed md5_base64 from imports - not used Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21625 [details] [review] [SIGNED-OFF] 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
Created attachment 21626 [details] [review] [SIGNED-OFF] 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
Created attachment 21627 [details] [review] [SIGNED-OFF] 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>
Created attachment 21628 [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>
Last comment: for QA sake, perhaps you can summarize in a comment all tests that need to be done.
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
Created attachment 21767 [details] [review] bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Hi Kyle, I think I was able to fix the conflict - It was removing C4::Utils that has gone into master very recently.
Hm, but i messed up attaching the rebased patch, trying again.
Created attachment 21768 [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>
Created attachment 21769 [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>
Created attachment 21770 [details] [review] bug_9611: Extracted checkpw_internal() and checkpw_hash() from checkpw() Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21771 [details] [review] bug_9611: removed md5_base64 from imports - not used Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21772 [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
Created attachment 21775 [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
Created attachment 21776 [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>
Created attachment 21777 [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>
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 :)
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>
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>
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>
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>
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>
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>
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>
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!
Pushed to master. Thanks, Srikanth!
*** Bug 11013 has been marked as a duplicate of this bug. ***