From c09335a0305f7f345bf745cbe90c216834689425 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 5 Nov 2019 22:26:11 +0100 Subject: [PATCH] Use the new PasswordHash classes. Integrate the `PasswordHash` class and subclass in the user and password editing and authentication. Replaces the old code and the previous `SecurePasswordHashingUtils` class. --- src/main/distrib/data/defaults.properties | 8 +- .../com/gitblit/client/EditUserDialog.java | 30 ++- .../manager/AuthenticationManager.java | 57 ++---- .../java/com/gitblit/utils/PasswordHash.java | 82 +++++--- .../utils/SecurePasswordHashUtils.java | 190 ------------------ .../java/com/gitblit/utils/StringUtils.java | 4 - .../wicket/pages/ChangePasswordPage.java | 24 +-- .../gitblit/wicket/pages/EditUserPage.java | 21 +- src/site/administration.mkd | 2 +- .../tests/AuthenticationManagerTest.java | 38 +++- .../com/gitblit/utils/PasswordHashTest.java | 40 ++++ .../utils/SecurePasswordHashUtilsTest.java | 63 ------ 12 files changed, 185 insertions(+), 374 deletions(-) delete mode 100644 src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java delete mode 100644 src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 352a6750..db0c2c69 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -854,14 +854,14 @@ realm.userService = ${baseFolder}/users.conf realm.authenticationProviders = # How to store passwords. -# Valid values are plain, md5, combined-md5 or PBKDF2WithHmacSHA256. +# Valid values are plain, md5, combined-md5 or pbkdf2. # md5 is the hash of password. # combined-md5 is the hash of username.toLowerCase()+password. -# PBKDF2WithHmacSHA256 is salt+hash(salt+password) -# Default is PBKDF2WithHmacSHA256. +# pbkdf2 implements the PBKDF2 algorithm, which is a secure, salted password hashing scheme. +# Default is pbkdf2. Using plain, md5 or combined-md5 is deprecated, as these are insecure schemes by now. # # SINCE 0.5.0 -realm.passwordStorage = PBKDF2WithHmacSHA256 +realm.passwordStorage = pbkdf2 # Minimum valid length for a plain text password. # Default value is 5. Absolute minimum is 4. diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index 141f233b..ecaed13d 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -58,7 +58,7 @@ import com.gitblit.models.RepositoryModel; import com.gitblit.models.ServerSettings; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; -import com.gitblit.utils.SecurePasswordHashUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.utils.StringUtils; @@ -319,9 +319,10 @@ public class EditUserDialog extends JDialog { minLength)); return false; } - if (!password.toUpperCase().startsWith(StringUtils.MD5_TYPE) - && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE) - && !password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)) { + // What we actually test for here, is if the password has been changed. But this also catches if the password + // was not changed, but is stored in plain-text. Which is good because then editing the user will hash the + // password if by now the storage has been changed to a hashed variant. + if (!PasswordHash.isHashedEntry(password)) { String cpw = new String(confirmPasswordField.getPassword()); if (cpw == null || cpw.length() != password.length()) { error("Please confirm the password!"); @@ -335,22 +336,17 @@ public class EditUserDialog extends JDialog { // change the cookie user.cookie = user.createCookie(); - String type = settings.get(Keys.realm.passwordStorage).getString(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - user.password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - user.password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username + password); - } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { - // store PBKDF2WithHmacSHA256 digest of password - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + String type = settings.get(Keys.realm.passwordStorage).getString(PasswordHash.getDefaultType().name()); + PasswordHash pwdHash = PasswordHash.instanceOf(type); + if (pwdHash != null) { + user.password = pwdHash.toHashedEntry(password, user.username); } else { - // plain-text password + // plain-text password. + // TODO: This is also used when the "realm.passwordStorage" configuration is not a valid type. + // This is a rather bad default, and should probably caught and changed to a secure default. user.password = password; } - } else if (rename && password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + } else if (rename && password.toUpperCase().startsWith(PasswordHash.Type.CMD5.name())) { error("Gitblit is configured for combined-md5 password hashing. You must enter a new password on account rename."); return false; } else { diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 2b54e4b8..83ca4b70 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -52,7 +52,7 @@ import com.gitblit.models.UserModel; import com.gitblit.transport.ssh.SshKey; import com.gitblit.utils.Base64; import com.gitblit.utils.HttpUtils; -import com.gitblit.utils.SecurePasswordHashUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.utils.StringUtils; import com.gitblit.utils.X509Utils.X509Metadata; import com.google.inject.Inject; @@ -519,29 +519,12 @@ public class AuthenticationManager implements IAuthenticationManager { */ protected UserModel authenticateLocal(UserModel user, char [] password) { UserModel returnedUser = null; - boolean strongHashUsed = false; - if (user.password.startsWith(StringUtils.MD5_TYPE)) { - // password digest - String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password)); - if (user.password.equalsIgnoreCase(md5)) { + PasswordHash pwdHash = PasswordHash.instanceFor(user.password); + if (pwdHash != null) { + if (pwdHash.matches(user.password, password, user.username)) { returnedUser = user; } - } else if (user.password.startsWith(StringUtils.COMBINED_MD5_TYPE)) { - // username+password digest - String md5 = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username.toLowerCase() + new String(password)); - if (user.password.equalsIgnoreCase(md5)) { - returnedUser = user; - } - } else if (user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)){ - // strong hash - SecurePasswordHashUtils hashUtils = SecurePasswordHashUtils.get(); - boolean isPasswordValid = hashUtils.isPasswordCorrect(password, user.password); - if(isPasswordValid){ - returnedUser = user; - strongHashUsed = true; - } } else if (user.password.equals(new String(password))) { // plain-text password returnedUser = user; @@ -550,33 +533,37 @@ public class AuthenticationManager implements IAuthenticationManager { // validate user returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); - // if no strong hash was used to store the password, try to update it based on the settings - if(!strongHashUsed){ - updateStoredPassword(returnedUser, password); - } - + // try to upgrade the stored password hash to a stronger hash, if necessary + upgradeStoredPassword(returnedUser, password, pwdHash); + return returnedUser; } /** - * Update stored password to a strong hash if configured. + * Upgrade stored password to a strong hash if configured. * * @param user the user to be updated * @param password the password + * @param pwdHash + * Instance of PasswordHash for the stored password entry. If null, no current hashing is assumed. */ - protected void updateStoredPassword(UserModel user, char[] password) { + private void upgradeStoredPassword(UserModel user, char[] password, PasswordHash pwdHash) { // check if user has successfully authenticated i.e. is not null - if(user != null){ - // check if strong hash algorithm is configured - String algorithm = settings.getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if(algorithm.equals(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)){ - // rehash the provided correct password and update the user model - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + if (user == null) return; + + // check if strong hash algorithm is configured + String algorithm = settings.getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + if (pwdHash == null || pwdHash.needsUpgradeTo(algorithm)) { + // rehash the provided correct password and update the user model + pwdHash = PasswordHash.instanceOf(algorithm); + if (pwdHash != null) { // necessary since the algorithm name could be something not supported. + user.password = pwdHash.toHashedEntry(password, user.username); userManager.updateUserModel(user); - } + } } } + /** * Returns the Gitlbit cookie in the request. * diff --git a/src/main/java/com/gitblit/utils/PasswordHash.java b/src/main/java/com/gitblit/utils/PasswordHash.java index 13747805..ee60dfb6 100644 --- a/src/main/java/com/gitblit/utils/PasswordHash.java +++ b/src/main/java/com/gitblit/utils/PasswordHash.java @@ -31,14 +31,27 @@ import java.util.Arrays; */ public abstract class PasswordHash { - /** * The types of implemented password hashing schemes. */ - enum Type { + public enum Type { MD5, CMD5, - PBKDF2 + PBKDF2; + + static Type fromName(String name) { + if (name == null) return null; + for (Type type : Type.values()) { + if (type.name().equalsIgnoreCase(name)) return type; + } + // Compatibility with type id "PBKDF2WITHHMACSHA256", which is also handled by PBKDF2 type. + if (name.equalsIgnoreCase("PBKDF2WITHHMACSHA256")) return Type.PBKDF2; + + // Recognise the name used for CMD5 in the settings file. + if (name.equalsIgnoreCase("combined-md5")) return Type.CMD5; + + return null; + } } /** @@ -57,6 +70,14 @@ public abstract class PasswordHash { } + /** + * When no hash type is specified, this determines the default that should be used. + */ + public static Type getDefaultType() { + return Type.PBKDF2; + } + + /** * Create an instance of a password hashing class for the given hash type. * @@ -66,21 +87,17 @@ public abstract class PasswordHash { * or null if the given hash type is not a valid one. */ public static PasswordHash instanceOf(String type) { - try { - Type hashType = Type.valueOf(type.toUpperCase()); - switch (hashType) { - case MD5: - return new PasswordHashMD5(); - case CMD5: - return new PasswordHashCombinedMD5(); - case PBKDF2: - return new PasswordHashPbkdf2(); - default: - return null; - } - } - catch (Exception e) { - return null; + Type hashType = Type.fromName(type); + if (hashType == null) return null; + switch (hashType) { + case MD5: + return new PasswordHashMD5(); + case CMD5: + return new PasswordHashCombinedMD5(); + case PBKDF2: + return new PasswordHashPbkdf2(); + default: + return null; } } @@ -115,6 +132,27 @@ public abstract class PasswordHash { } + /** + * Some hash methods are considered more secure than others. This method determines for a certain type + * of password hash if it is inferior than a given other type and stored passwords should be + * upgraded to the given hashing method. + * + * @param algorithm + * Password hashing type to be checked if it is superior than the one of this instance. + * @return True, if the given type in parameter {@code algorithm} is better and stored passwords should be upgraded to it, + * false, otehrwise. + */ + public boolean needsUpgradeTo(String algorithm) { + Type hashType = Type.fromName(algorithm); + if (hashType == null) return false; + if (this.type == hashType) return false; + + // Right now we keep it really simple. With the existing types, only PBKDF2 is considered secure, + // everything else is inferior. This will need to be updated once more secure hashing algorithms + // are implemented, or the workload/parameters of the PBKDF2 are changed. + return hashType == Type.PBKDF2; + } + /** * Convert the given password to a hashed password entry to be stored in the user table. @@ -181,13 +219,7 @@ public abstract class PasswordHash { int indexOfSeparator = hashedEntry.indexOf(':'); if (indexOfSeparator <= 0) return null; String typeId = hashedEntry.substring(0, indexOfSeparator); - - // Compatibility with type id "PBKDF2WITHHMACSHA256", which is also handled by PBKDF2 type. - if (typeId.equalsIgnoreCase("PBKDF2WITHHMACSHA256")) return Type.PBKDF2; - try { - return Type.valueOf(typeId.toUpperCase()); - } - catch (Exception e) { return null;} + return Type.fromName(typeId); } diff --git a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java deleted file mode 100644 index 289084ee..00000000 --- a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java +++ /dev/null @@ -1,190 +0,0 @@ -package com.gitblit.utils; - -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; - -import org.apache.commons.codec.DecoderException; -import org.apache.commons.codec.binary.Hex; - -import java.security.NoSuchAlgorithmException; -import java.security.spec.InvalidKeySpecException; -import java.util.Arrays; - -/** - * The Class SecurePasswordHashUtils provides methods to create and validate - * secure hashes from user passwords. - * - * It uses the concept proposed by OWASP - Hashing Java: - * https://www.owasp.org/index.php/Hashing_Java - */ -public class SecurePasswordHashUtils { - - public static final String PBKDF2WITHHMACSHA256 = "PBKDF2WithHmacSHA256"; - public static final String PBKDF2WITHHMACSHA256_TYPE = PBKDF2WITHHMACSHA256.toUpperCase() + ":"; - - private static final SecureRandom RANDOM = new SecureRandom(); - private static final int ITERATIONS = 10000; - private static final int KEY_LENGTH = 256; - private static final int SALT_LENGTH = 32; - - /** The instance. */ - private static SecurePasswordHashUtils instance; - - /** - * Instantiates a new secure password hash utils. - */ - private SecurePasswordHashUtils() { - } - - /** - * Gets an instance of {@link SecurePasswordHashUtils}. - * - * @return the secure password hash utils - */ - public static SecurePasswordHashUtils get() { - if (instance == null) { - instance = new SecurePasswordHashUtils(); - } - return instance; - } - - /** - * Gets the next salt. - * - * @return the next salt - */ - protected byte[] getNextSalt() { - byte[] salt = new byte[SALT_LENGTH]; - RANDOM.nextBytes(salt); - return salt; - } - - /** - * Hash. - * - * @param password - * the password - * @param salt - * the salt - * @return the byte[] - */ - protected byte[] hash(char[] password, byte[] salt) { - PBEKeySpec spec = new PBEKeySpec(password, salt, ITERATIONS, KEY_LENGTH); - Arrays.fill(password, Character.MIN_VALUE); - try { - SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256"); - return skf.generateSecret(spec).getEncoded(); - } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new IllegalStateException("Error while hashing password", e); - } finally { - spec.clearPassword(); - } - } - - /** - * Checks if is password correct. - * - * @param passwordToCheck - * the password to check - * @param salt - * the salt - * @param expectedHash - * the expected hash - * @return true, if is password correct - */ - protected boolean isPasswordCorrect(char[] passwordToCheck, byte[] salt, byte[] expectedHash) { - byte[] hashToCheck = hash(passwordToCheck, salt); - Arrays.fill(passwordToCheck, Character.MIN_VALUE); - if (hashToCheck.length != expectedHash.length) { - return false; - } - for (int i = 0; i < hashToCheck.length; i++) { - if (hashToCheck[i] != expectedHash[i]) { - return false; - } - } - return true; - } - - /** - * Creates the new secure hash from a password and formats it properly to be - * stored in a file. - * - * @param password - * the password to be hashed - * @return the sting to be stored in a file (users.conf) - */ - public String createStoredPasswordFromPassword(String password) { - return createStoredPasswordFromPassword(password.toCharArray()); - } - - /** - * Creates the new secure hash from a password and formats it properly to be - * stored in a file. - * - * @param password - * the password to be hashed - * @return the sting to be stored in a file (users.conf) - */ - public String createStoredPasswordFromPassword(char[] password) { - byte[] salt = getNextSalt(); - return String.format("%s%s%s", SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE, StringUtils.toHex(salt), - StringUtils.toHex(hash(password, salt))); - } - - /** - * Gets the salt from stored password. - * - * @param storedPassword - * the stored password - * @return the salt from stored password - */ - protected byte[] getSaltFromStoredPassword(String storedPassword) { - byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); - return Arrays.copyOfRange(pw, 0, SALT_LENGTH); - } - - /** - * Gets the hash from stored password. - * - * @param storedPassword - * the stored password - * @return the hash from stored password - */ - protected byte[] getHashFromStoredPassword(String storedPassword) { - byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); - return Arrays.copyOfRange(pw, SALT_LENGTH, pw.length); - } - - /** - * Strips the prefix ({@link #PBKDF2WITHHMACSHA256_TYPE}) from a stored - * password and returns the decoded hash - * - * @param storedPassword - * the stored password - * @return the stored hash with stripped prefix - */ - protected byte[] getStoredHashWithStrippedPrefix(String storedPassword) { - String saltAndHash = storedPassword.substring(PBKDF2WITHHMACSHA256_TYPE.length()); - try { - return Hex.decodeHex(saltAndHash.toCharArray()); - } catch (DecoderException e) { - throw new IllegalStateException("Error while reading stored credentials", e); - } - } - - /** - * Checks if is password correct. - * - * @param password - * the password - * @param storedPassword - * the stored password - * @return true, if is password correct - */ - public boolean isPasswordCorrect(char[] password, String storedPassword) { - byte[] storedSalt = getSaltFromStoredPassword(storedPassword); - byte[] storedHash = getHashFromStoredPassword(storedPassword); - return isPasswordCorrect(password, storedSalt, storedHash); - } -} diff --git a/src/main/java/com/gitblit/utils/StringUtils.java b/src/main/java/com/gitblit/utils/StringUtils.java index 643c52c3..b192c80b 100644 --- a/src/main/java/com/gitblit/utils/StringUtils.java +++ b/src/main/java/com/gitblit/utils/StringUtils.java @@ -46,10 +46,6 @@ import java.util.regex.PatternSyntaxException; */ public class StringUtils { - public static final String MD5_TYPE = "MD5:"; - - public static final String COMBINED_MD5_TYPE = "CMD5:"; - /** * Returns true if the string is null or empty. * diff --git a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java index 4c074abc..53c3e14d 100644 --- a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java +++ b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java @@ -28,15 +28,14 @@ import org.apache.wicket.protocol.http.WebResponse; import com.gitblit.GitBlitException; import com.gitblit.Keys; import com.gitblit.models.UserModel; -import com.gitblit.utils.SecurePasswordHashUtils; -import com.gitblit.utils.StringUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.wicket.GitBlitWebSession; import com.gitblit.wicket.NonTrimmedPasswordTextField; public class ChangePasswordPage extends RootSubPage { - IModel password = new Model(""); - IModel confirmPassword = new Model(""); + private IModel password = new Model(""); + private IModel confirmPassword = new Model(""); public ChangePasswordPage() { super(); @@ -86,18 +85,11 @@ public class ChangePasswordPage extends RootSubPage { UserModel user = GitBlitWebSession.get().getUser(); - // convert to MD5 digest, if appropriate - String type = app().settings().getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username.toLowerCase() + password); - } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { - // store PBKDF2WithHmacSHA256 digest of password - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + // convert to digest, if appropriate + String type = app().settings().getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + PasswordHash pwdHash = PasswordHash.instanceOf(type); + if (pwdHash != null) { + password = pwdHash.toHashedEntry(password, user.username); } user.password = password; diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java index 72dee6b6..94dd35ab 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java +++ b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import com.gitblit.utils.PasswordHash; import org.apache.wicket.PageParameters; import org.apache.wicket.behavior.SimpleAttributeModifier; import org.apache.wicket.extensions.markup.html.form.palette.Palette; @@ -141,15 +142,14 @@ public class EditUserPage extends RootSubPage { return; } String password = userModel.password; - if (!password.toUpperCase().startsWith(StringUtils.MD5_TYPE) - && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + if (!PasswordHash.isHashedEntry(password)) { // This is a plain text password. // Check length. int minLength = app().settings().getInteger(Keys.realm.minPasswordLength, 5); if (minLength < 4) { minLength = 4; } - if (password.trim().length() < minLength) { + if (password.trim().length() < minLength) { // TODO: Why do we trim here, but not in EditUserDialog and ChangePasswordPage? error(MessageFormat.format(getString("gb.passwordTooShort"), minLength)); return; @@ -159,18 +159,13 @@ public class EditUserPage extends RootSubPage { userModel.cookie = userModel.createCookie(); // Optionally store the password MD5 digest. - String type = app().settings().getString(Keys.realm.passwordStorage, "md5"); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - userModel.password = StringUtils.MD5_TYPE - + StringUtils.getMD5(userModel.password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - userModel.password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(username + userModel.password); + String type = app().settings().getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + PasswordHash pwdh = PasswordHash.instanceOf(type); + if (pwdh != null) { // Hash the password + userModel.password = pwdh.toHashedEntry(password, username); } } else if (rename - && password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + && password.toUpperCase().startsWith(PasswordHash.Type.CMD5.name())) { error(getString("gb.combinedMd5Rename")); return; } diff --git a/src/site/administration.mkd b/src/site/administration.mkd index 5feedee9..fed71e8a 100644 --- a/src/site/administration.mkd +++ b/src/site/administration.mkd @@ -169,7 +169,7 @@ Usernames must be unique and are case-insensitive. Whitespace is illegal. ### Passwords -User passwords are CASE-SENSITIVE and may be *plain*, *md5*, *combined-md5* or *PBKDF2WithHmacSHA256* formatted (see `gitblit.properties` -> *realm.passwordStorage*). +User passwords are CASE-SENSITIVE and may be *plain*, *md5*, *combined-md5* or *pbkdf2* formatted (see `gitblit.properties` -> *realm.passwordStorage*). ### User Roles There are four actual *roles* in Gitblit: diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java index 31b7512c..45009856 100644 --- a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -43,6 +43,7 @@ import javax.servlet.http.HttpSessionContext; import javax.servlet.http.HttpUpgradeHandler; import javax.servlet.http.Part; +import com.gitblit.utils.PasswordHash; import org.junit.Test; import com.gitblit.IUserService; @@ -55,7 +56,6 @@ import com.gitblit.manager.UserManager; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; import com.gitblit.tests.mock.MemorySettings; -import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.XssFilter; import com.gitblit.utils.XssFilter.AllowXssFilter; @@ -659,17 +659,43 @@ public class AuthenticationManagerTest extends GitblitUnitTest { users.updateUserModel(user); assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); - - // validate that plaintext password was automatically updated to hashed one - assertTrue(user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); - user.disabled = true; users.updateUserModel(user); assertNull(auth.authenticate(user.username, user.password.toCharArray(), null)); users.deleteUserModel(user); } - + + + @Test + public void testAuthenticateUpgradePlaintext() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + UserModel user = new UserModel("sunnyjim"); + user.password = "password"; + users.updateUserModel(user); + + assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); + + // validate that plaintext password was automatically updated to hashed one + assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); + } + + + @Test + public void testAuthenticateUpgradeMD5() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + UserModel user = new UserModel("sunnyjim"); + user.password = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; + users.updateUserModel(user); + + assertNotNull(auth.authenticate(user.username, "password".toCharArray(), null)); + + // validate that MD5 password was automatically updated to hashed one + assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); + } + @Test public void testContenairAuthenticate() throws Exception { diff --git a/src/test/java/com/gitblit/utils/PasswordHashTest.java b/src/test/java/com/gitblit/utils/PasswordHashTest.java index c5a485dc..40c472aa 100644 --- a/src/test/java/com/gitblit/utils/PasswordHashTest.java +++ b/src/test/java/com/gitblit/utils/PasswordHashTest.java @@ -99,6 +99,15 @@ public class PasswordHashTest { assertTrue("Failed to match " +CMD5_HASHED_ENTRY_0, pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + pwdh = PasswordHash.instanceOf("combined-md5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + + pwdh = PasswordHash.instanceOf("COMBINED-MD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + + pwdh = PasswordHash.instanceOf("MD5"); assertNotNull(pwdh); assertNotEquals(PasswordHash.Type.CMD5, pwdh.type); @@ -593,4 +602,35 @@ public class PasswordHashTest { assertFalse("Matched wrong hashed entry, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "")); assertFalse("Matched wrong hashed entry, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "someuser")); } + + @Test + public void getEntryType() { + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("MD5:blah")); + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("md5:blah")); + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("mD5:blah")); + + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("CMD5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("cmd5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("Cmd5:blah")); + + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("combined-md5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("COMBINED-MD5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("combined-MD5:blah")); + + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("pbkdf2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("Pbkdf2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("pbKDF2:blah")); + + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2WithHmacSHA256:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2WITHHMACSHA256:blah")); + } + + @Test + public void getEntryValue() { + assertEquals("value", PasswordHash.getEntryValue("MD5:value")); + assertEquals("plain text", PasswordHash.getEntryValue("plain text")); + assertEquals("what this", PasswordHash.getEntryValue(":what this")); + assertEquals("", PasswordHash.getEntryValue(":")); + } } diff --git a/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java b/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java deleted file mode 100644 index f687bda6..00000000 --- a/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java +++ /dev/null @@ -1,63 +0,0 @@ -package com.gitblit.utils; - -import static org.junit.Assert.*; - -import org.junit.Before; -import org.junit.Test; - -public class SecurePasswordHashUtilsTest { - - private static final String STORED_PASSWORD = "PBKDF2WITHHMACSHA256:2d7d3ccaa277787f288e9f929247361bfc83607c6a8447bf496267512e360ba0a97b3114937213b23230072517d65a2e00695a1cbc47a732510840817f22c1bc"; - private static final byte[] STORED_SALT_BYTES = new byte[]{45, 125, 60, -54, -94, 119, 120, 127, 40, -114, -97, -110, -110, 71, 54, 27, -4, -125, 96, 124, 106, -124, 71, -65, 73, 98, 103, 81, 46, 54, 11, -96}; - private static final byte[] STORED_HASH_BYTES = new byte[]{-87, 123, 49, 20, -109, 114, 19, -78, 50, 48, 7, 37, 23, -42, 90, 46, 0, 105, 90, 28, -68, 71, -89, 50, 81, 8, 64, -127, 127, 34, -63, -68}; - - private SecurePasswordHashUtils utils; - - @Before - public void init(){ - utils = SecurePasswordHashUtils.get(); - } - - @Test - public void testGetNextSalt() { - assertEquals(32, utils.getNextSalt().length); - } - - @Test - public void testHash() { - byte[] hash = utils.hash("foo".toCharArray(), STORED_SALT_BYTES); - assertArrayEquals(STORED_HASH_BYTES, hash); - } - - @Test - public void testIsPasswordCorrectCharArrayByteArrayByteArray() { - assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); - assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); - } - - @Test - public void testCreateNewStorableHashFromPassword() { - String newPwHash = utils.createStoredPasswordFromPassword("foo"); - assertTrue(newPwHash.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); - } - - @Test - public void testGetSaltFromStoredPassword() { - byte[] saltFromStoredPassword = utils.getSaltFromStoredPassword(STORED_PASSWORD); - assertArrayEquals(STORED_SALT_BYTES, saltFromStoredPassword); - - } - - @Test - public void testGetHashFromStoredPassword() { - byte[] hashFromStoredPassword = utils.getHashFromStoredPassword(STORED_PASSWORD); - assertArrayEquals(STORED_HASH_BYTES, hashFromStoredPassword); - } - - @Test - public void testIsPasswordCorrectCharArrayString() { - assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_PASSWORD)); - assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_PASSWORD)); - } - -} -- 2.39.5