From 940c5fc153cd6023221220ad0f4f6c948f649d32 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Felix=20D=C3=B6rre?= Date: Fri, 5 Dec 2014 18:43:53 +0100 Subject: [PATCH] add: implement password migration to scrypt on login. --- src/org/cacert/gigi/dbObjects/User.java | 2 +- src/org/cacert/gigi/pages/LoginPage.java | 10 ++++- src/org/cacert/gigi/util/PasswordHash.java | 29 +++++++++++++-- .../cacert/gigi/testUtils/RegisteredUser.java | 32 ++++++++++++++++ .../cacert/gigi/util/TestPasswordHash.java | 11 +++--- .../gigi/util/TestPasswordMigration.java | 37 +++++++++++++++++++ 6 files changed, 110 insertions(+), 11 deletions(-) create mode 100644 tests/org/cacert/gigi/testUtils/RegisteredUser.java create mode 100644 tests/org/cacert/gigi/util/TestPasswordMigration.java diff --git a/src/org/cacert/gigi/dbObjects/User.java b/src/org/cacert/gigi/dbObjects/User.java index cf9c40b9..0b77b97d 100644 --- a/src/org/cacert/gigi/dbObjects/User.java +++ b/src/org/cacert/gigi/dbObjects/User.java @@ -132,7 +132,7 @@ public class User extends CertificateOwner { if ( !rs.next()) { throw new GigiApiException("User not found... very bad."); } - if ( !PasswordHash.verifyHash(oldPass, rs.getString(1))) { + if (PasswordHash.verifyHash(oldPass, rs.getString(1)) == null) { throw new GigiApiException("Old password does not match."); } rs.close(); diff --git a/src/org/cacert/gigi/pages/LoginPage.java b/src/org/cacert/gigi/pages/LoginPage.java index b19e8f5b..128855fc 100644 --- a/src/org/cacert/gigi/pages/LoginPage.java +++ b/src/org/cacert/gigi/pages/LoginPage.java @@ -97,7 +97,15 @@ public class LoginPage extends Page { ps.setString(1, un); GigiResultSet rs = ps.executeQuery(); if (rs.next()) { - if (PasswordHash.verifyHash(pw, rs.getString(1))) { + String dbHash = rs.getString(1); + String hash = PasswordHash.verifyHash(pw, dbHash); + if (hash != null) { + if ( !hash.equals(dbHash)) { + GigiPreparedStatement gps = DatabaseConnection.getInstance().prepare("UPDATE `users` SET `password`=? WHERE `email`=?"); + gps.setString(1, hash); + gps.setString(2, un); + gps.executeUpdate(); + } loginSession(req, User.getById(rs.getInt(2))); } } diff --git a/src/org/cacert/gigi/util/PasswordHash.java b/src/org/cacert/gigi/util/PasswordHash.java index d6b0b906..11e4d344 100644 --- a/src/org/cacert/gigi/util/PasswordHash.java +++ b/src/org/cacert/gigi/util/PasswordHash.java @@ -7,9 +7,28 @@ import com.lambdaworks.crypto.SCryptUtil; public class PasswordHash { - public static boolean verifyHash(String password, String hash) { + /** + * Verifies a password hash. + * + * @param password + * The password that should result in the given hash. + * @param hash + * The hash to verify the password against. + * @return + */ + public static String verifyHash(String password, String hash) { if (hash.contains("$")) { - return SCryptUtil.check(password, hash); + if (SCryptUtil.check(password, hash)) { + return hash; + } else { + return null; + } } String newhash = sha1(password); boolean match = true; @@ -19,7 +38,11 @@ public class PasswordHash { for (int i = 0; i < newhash.length(); i++) { match &= newhash.charAt(i) == hash.charAt(i); } - return match; + if (match) { + return hash(password); + } else { + return null; + } } private static String sha1(String password) { diff --git a/tests/org/cacert/gigi/testUtils/RegisteredUser.java b/tests/org/cacert/gigi/testUtils/RegisteredUser.java new file mode 100644 index 00000000..a5b51728 --- /dev/null +++ b/tests/org/cacert/gigi/testUtils/RegisteredUser.java @@ -0,0 +1,32 @@ +package org.cacert.gigi.testUtils; + +import org.cacert.gigi.dbObjects.User; +import org.junit.rules.TestRule; +import org.junit.runner.Description; +import org.junit.runners.model.Statement; + +public class RegisteredUser implements TestRule { + + User u; + + @Override + public Statement apply(final Statement base, Description description) { + return new Statement() { + + @Override + public void evaluate() throws Throwable { + u = User.getById(ManagedTest.createVerifiedUser("fn", "ln", ManagedTest.createUniqueName() + "@example.org", ManagedTest.TEST_PASSWORD)); + try { + base.evaluate(); + } finally { + + } + } + }; + } + + public User getUser() { + return u; + } + +} diff --git a/tests/org/cacert/gigi/util/TestPasswordHash.java b/tests/org/cacert/gigi/util/TestPasswordHash.java index 1c2d9d3a..6e38a2bd 100644 --- a/tests/org/cacert/gigi/util/TestPasswordHash.java +++ b/tests/org/cacert/gigi/util/TestPasswordHash.java @@ -8,15 +8,14 @@ public class TestPasswordHash { @Test public void testVerify() { - assertTrue(PasswordHash.verifyHash("a", PasswordHash.hash("a"))); - assertTrue(PasswordHash.verifyHash("", PasswordHash.hash(""))); - assertTrue(PasswordHash.verifyHash("a1234", PasswordHash.hash("a1234"))); - assertTrue(PasswordHash.verifyHash("auhlcb4 9x,IUQẞ&lvrvä", PasswordHash.hash("auhlcb4 9x,IUQẞ&lvrvä"))); + assertTrue(PasswordHash.verifyHash("a", PasswordHash.hash("a")) != null); + assertTrue(PasswordHash.verifyHash("a1234", PasswordHash.hash("a1234")) != null); + assertTrue(PasswordHash.verifyHash("auhlcb4 9x,IUQẞ&lvrvä", PasswordHash.hash("auhlcb4 9x,IUQẞ&lvrvä")) != null); } @Test public void testVerifyNegative() { - assertFalse(PasswordHash.verifyHash("b", PasswordHash.hash("a"))); - assertFalse(PasswordHash.verifyHash("ae", PasswordHash.hash("auhlcb4 9x,IUQẞ&lvrvä"))); + assertFalse(PasswordHash.verifyHash("b", PasswordHash.hash("a")) != null); + assertFalse(PasswordHash.verifyHash("ae", PasswordHash.hash("auhlcb4 9x,IUQẞ&lvrvä")) != null); } } diff --git a/tests/org/cacert/gigi/util/TestPasswordMigration.java b/tests/org/cacert/gigi/util/TestPasswordMigration.java new file mode 100644 index 00000000..030794cb --- /dev/null +++ b/tests/org/cacert/gigi/util/TestPasswordMigration.java @@ -0,0 +1,37 @@ +package org.cacert.gigi.util; + +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.*; + +import java.io.IOException; + +import org.cacert.gigi.database.DatabaseConnection; +import org.cacert.gigi.database.GigiPreparedStatement; +import org.cacert.gigi.database.GigiResultSet; +import org.cacert.gigi.testUtils.ManagedTest; +import org.cacert.gigi.testUtils.RegisteredUser; +import org.junit.Rule; +import org.junit.Test; + +public class TestPasswordMigration extends ManagedTest { + + @Rule + public RegisteredUser ru = new RegisteredUser(); + + @Test + public void testPasswordMigration() throws IOException { + GigiPreparedStatement stmt = DatabaseConnection.getInstance().prepare("UPDATE users SET `password`=SHA1(?) WHERE id=?"); + stmt.setString(1, "a"); + stmt.setInt(2, ru.getUser().getId()); + stmt.execute(); + String cookie = login(ru.getUser().getEmail(), "a"); + assertTrue(isLoggedin(cookie)); + + stmt = DatabaseConnection.getInstance().prepare("SELECT `password` FROM users WHERE id=?"); + stmt.setInt(1, ru.getUser().getId()); + GigiResultSet res = stmt.executeQuery(); + assertTrue(res.next()); + String newHash = res.getString(1); + assertThat(newHash, containsString("$")); + } +} -- 2.39.2