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.

12 files changed, 185 insertions, 374 deletions

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;
+		}
 	}
 
 	/**
@@ -58,6 +71,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.
 	 *
 	 * @param 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<String> password = new Model<String>("");

-	IModel<String> confirmPassword = new Model<String>("");

+	private IModel<String> password = new Model<String>("");

+	private IModel<String> confirmPassword = new Model<String>("");

 

 	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));
-	}
-
-}