use bcrypt instead of md5 for password hashing

.gitignore

@@ -2,7 +2,7 @@
 /testdb
 /*.dll
 /*.exe
-userman
+/userman
 userman.exe
 userman-test-*
 libuserman.a

dub.sdl

@@ -5,6 +5,9 @@ copyright "Copyright (c) 2012-2020 rejectedsoftware e.K."
 license "GPL-3.0"
 
 dependency "vibe-d" version=">=0.9.7 <0.11.0"
+dependency "botan" version="*" # needed for bcrypt, available since initial version
+
+subConfiguration "botan" "full"
 
 configuration "application" {
 	targetType "executable"

dub.selections.json

@@ -1,13 +1,14 @@
 {
 	"fileVersion": 1,
 	"versions": {
-		"botan": "1.12.18",
-		"botan-math": "1.0.3",
-		"diet-ng": "1.8.2",
-		"eventcore": "0.9.35",
+		"botan": "1.13.8",
+		"botan-math": "1.0.4",
+		"diet-ng": "1.8.4",
+		"during": "0.3.0",
+		"eventcore": "0.9.38",
 		"libasync": "0.8.6",
 		"libevent": "2.0.2+2.0.16",
-		"memutils": "1.0.10",
+		"memutils": "1.0.11",
 		"mir-linux-kernel": "1.2.1",
 		"openssl": "3.3.4",
 		"openssl-static": "1.0.5+3.0.8",

source/userman/api.d

@@ -7,12 +7,12 @@
 */
 module userman.api;
 
-import userman.db.controller : UserManController, UserManCommonSettings;
+import userman.db.controller : UserManCommonSettings, UserManController;
 static import userman.db.controller;
 
 import vibe.data.json : Json;
-import vibe.http.router : URLRouter;
 import vibe.http.common : enforceHTTP;
+import vibe.http.router : URLRouter;
 import vibe.http.status : HTTPStatus;
 import vibe.inet.url : URL;
 import vibe.web.rest;
@@ -72,6 +72,9 @@ interface UserManUserAPI {
 	@property Collection!UserManUserPropertyAPI properties(User.ID _user);
 
 	/// Tests a username/e-mail and password combination for validity.
+	UserLoginInfo testLoginInfo(string email_or_name, string password);
+
+	/// Deprecated: you should use testLoginInfo instead!
 	User.ID testLogin(string name, string password);
 
 	/// Registers a new user.
@@ -165,6 +168,15 @@ struct User {
 	}
 }
 
+/// Information about a user coming from a login attempt.
+struct UserLoginInfo {
+	/// User ID of the login credentials
+	User.ID userId;
+	/// Set to true if the current password is deemed too insecure now.
+	/// In that case, a password reset should be mandatory.
+	bool needsPasswordChange;
+}
+
 /// Interface suitable for manipulating group information
 interface UserManGroupAPI {
 @safe:
@@ -277,11 +289,18 @@ private class UserManUserAPIImpl : UserManUserAPI {
 		return Collection!UserManUserPropertyAPI(m_properties, _id);
 	}
 
+	UserLoginInfo testLoginInfo(string email_or_name, string password)
+	{
+		auto ret = m_ctrl.validateLogin(email_or_name, password);
+		enforceHTTP(!ret.userId.isNull, HTTPStatus.unauthorized, "Wrong user name or password.");
+		return UserLoginInfo(ret.userId.get(), ret.needsPasswordChange);
+	}
+
 	User.ID testLogin(string name, string password)
 	{
-		auto ret = m_ctrl.testLogin(name, password);
-		enforceHTTP(!ret.isNull, HTTPStatus.unauthorized, "Wrong user name or password.");
-		return ret.get();
+		auto ret = m_ctrl.validateLogin(name, password);
+		enforceHTTP(!ret.userId.isNull, HTTPStatus.unauthorized, "Wrong user name or password.");
+		return ret.userId.get();
 	}
 
 	User.ID register(string email, string name, string full_name, string password)

source/userman/db/controller.d

@@ -10,13 +10,13 @@ module userman.db.controller;
 public import userman.userman;
 import userman.id;
 
+import diet.html;
 import vibe.data.serialization;
 import vibe.db.mongo.mongo;
 import vibe.http.router;
 import vibe.mail.smtp;
 import vibe.stream.memory;
 import vibe.utils.validation;
-import diet.html;
 
 import std.algorithm;
 import std.array;
@@ -77,8 +77,8 @@ class UserManController {
 		user.name = name;
 		user.fullName = full_name;
 		user.auth.method = "password";
-		user.auth.passwordHash = generatePasswordHash(password);
-		assert(validatePasswordHash(user.auth.passwordHash, password));
+		user.auth.passwordHash = generateBcryptHash(password);
+		assert(validateBcryptHash(user.auth.passwordHash, password));
 		user.email = email;
 		if( need_activation )
 			user.activationCode = generateActivationCode();
@@ -127,17 +127,43 @@ class UserManController {
 		}
 	}
 
-	Nullable!(User.ID) testLogin(string name, string password)
+	static struct LoginResult
+	{
+		/// If set, the username and password combination was found in the DB.
+		Nullable!(User.ID) userId;
+		/// Set to true if the current password is deemed too insecure now.
+		/// In that case, a password reset should be mandatory.
+		bool needsPasswordChange;
+	}
+
+	/// Checks if the login credentials are correct, returns one of:
+	/// - invalid username or password (result is LoginResult.init)
+	/// - valid user, but password reset is required (userId is set, needsPasswordChange is true)
+	/// - valid: valid user (userId is set, needsPasswordChange is false)
+	LoginResult validateLogin(string emailOrName, string password)
 	{
-		string password_ = password;
-		auto user = getUserByEmailOrName(name);
-		assert(password == password_); // this used to be false to to a Nullable related codegen issue
-		Nullable!(User.ID) ret;
-		if (validatePasswordHash(user.auth.passwordHash, password_))
-			ret = user.id;
+		auto user = getUserByEmailOrName(emailOrName);
+		LoginResult ret;
+		bool validPassword;
+		if (user.auth.passwordHash.length && user.auth.passwordHash[0] == '$') {
+			validPassword = validateBcryptHash(user.auth.passwordHash, password);
+		} else {
+			validPassword = validatePasswordHashMD5(user.auth.passwordHash, password);
+			ret.needsPasswordChange = true;
+		}
+
+		if (!validPassword)
+			return LoginResult.init;
+
+		ret.userId = user.id;
 		return ret;
 	}
 
+	deprecated("Use validateLogin instead") Nullable!(User.ID) testLogin(string name, string password)
+	{
+		return validateLogin(name, password).userId;
+	}
+
 	void activateUser(string email, string activation_code)
 	{
 		email = email.toLower();
@@ -208,7 +234,7 @@ class UserManController {
 		usr.resetCode = "";
 		updateUser(usr);
 		enforce(reset_code == code, "Invalid request code, please request a new one.");
-		usr.auth.passwordHash = generatePasswordHash(new_password);
+		usr.auth.passwordHash = generateBcryptHash(new_password);
 		updateUser(usr);
 	}
 
@@ -263,8 +289,8 @@ class UserManController {
 	*/
 	static bool isValidGroupID(string name)
 	{
-		import std.ascii : isAlpha, isDigit;
 		import std.algorithm : splitter;
+		import std.ascii : isAlpha, isDigit;
 
 		if (name.length < 1) return false;
 		foreach (p; name.splitter('.')) {
@@ -322,7 +348,29 @@ string generateActivationCode()
 	return ret.data();
 }
 
-string generatePasswordHash(string password)
+string generateBcryptHash(in string password, ushort work_factor = 12)
+@trusted {
+	import botan.passhash.bcrypt : generateBcrypt;
+	import botan.rng.auto_rng;
+
+	static AutoSeededRNG rng;
+	if (rng is null)
+		rng = new AutoSeededRNG;
+
+	return generateBcrypt(password, rng, work_factor);
+}
+
+bool validateBcryptHash(in string password_hash, in string password)
+@trusted {
+	import botan.passhash.bcrypt : checkBcrypt;
+
+	return checkBcrypt(password, password_hash);
+}
+
+deprecated("Use generatePasswordHashMD5 for old (insecure) behavior or generateBcryptHash") alias generatePasswordHash = generatePasswordHashMD5;
+deprecated("Use validatePasswordHashMD5 for old (insecure) behavior or validateBcryptHash") alias validatePasswordHash = validatePasswordHashMD5;
+
+deprecated("This method is insecure") string generatePasswordHashMD5(string password)
 @safe {
 	import std.base64 : Base64;
 
@@ -333,7 +381,7 @@ string generatePasswordHash(string password)
 	return Base64.encode(salt ~ hash).idup;
 }
 
-bool validatePasswordHash(string password_hash, string password)
+bool validatePasswordHashMD5(string password_hash, string password)
 @safe {
 	import std.base64 : Base64;
 
@@ -347,10 +395,16 @@ bool validatePasswordHash(string password_hash, string password)
 	return hash == hashcmp;
 }
 
+deprecated unittest {
+	auto h = generatePasswordHashMD5("foobar");
+	assert(!validatePasswordHashMD5(h, "foo"));
+	assert(validatePasswordHashMD5(h, "foobar"));
+}
+
 unittest {
-	auto h = generatePasswordHash("foobar");
-	assert(!validatePasswordHash(h, "foo"));
-	assert(validatePasswordHash(h, "foobar"));
+	auto h = generateBcryptHash("foobar");
+	assert(!validateBcryptHash(h, "foo"));
+	assert(validateBcryptHash(h, "foobar"));
 }
 
 private ubyte[16] md5hash(scope const(ubyte)[] salt, const string[] strs...)

source/userman/db/file.d

@@ -14,10 +14,10 @@ import vibe.data.json;
 import vibe.textfilter.urlencode;
 import vibe.utils.validation;
 
+import std.conv;
 import std.datetime;
 import std.exception;
 import std.string;
-import std.conv;
 import std.uuid;
 
 
@@ -181,7 +181,7 @@ class FileUserManController : UserManController {
 	{
 		auto usr = getUser(user);
 		usr.auth.method = "password";
-		usr.auth.passwordHash = generatePasswordHash(password);
+		usr.auth.passwordHash = generateBcryptHash(password);
 		updateUser(usr);
 	}
 

source/userman/db/mongo.d

@@ -159,7 +159,7 @@ class MongoUserManController : UserManController {
 	override void setPassword(User.ID user, string password)
 	{
 		m_users.updateOne(["_id": user.bsonObjectIDValue], ["$set":
-			["auth.method": "password", "auth.passwordHash": generatePasswordHash(password)]]);
+			["auth.method": "password", "auth.passwordHash": generateBcryptHash(password)]]);
 	}
 
 	override void setProperty(User.ID user, string name, Json value)

source/userman/db/redis.d

@@ -9,18 +9,18 @@ module userman.db.redis;
 
 import userman.db.controller;
 
-import vibe.db.redis.redis;
-import vibe.db.redis.idioms;
-import vibe.db.redis.types;
 import vibe.data.bson;
 import vibe.data.json;
+import vibe.db.redis.idioms;
+import vibe.db.redis.redis;
+import vibe.db.redis.types;
 import vibe.utils.validation;
 
+import std.conv;
 import std.datetime;
 import std.exception;
-import std.string;
-import std.conv;
 import std.range : front;
+import std.string;
 
 
 class RedisUserManController : UserManController {
@@ -287,7 +287,7 @@ class RedisUserManController : UserManController {
 
 		AuthInfo auth = m_userAuthInfo[user.longValue];
 		auth.method = "password";
-		auth.passwordHash = generatePasswordHash(password);
+		auth.passwordHash = generateBcryptHash(password);
 		m_userAuthInfo[user.longValue] = auth;
 	}
 

source/userman/web.d

@@ -298,10 +298,15 @@ class UserManWebInterface {
 	@noAuth @errorDisplay!getLogin
 	void postLogin(string name, string password, string redirect = "")
 	{
+		import std.string : chomp;
+
 		User user;
+		string redirectUrl = redirect.length ? redirect : m_prefix;
 		try {
-			auto uid = m_api.users.testLogin(name, password);
-			user = m_api.users[uid].get();
+			auto result = m_api.users.testLoginInfo(name, password);
+			if (result.needsPasswordChange)
+				redirectUrl = m_prefix.chomp("/") ~ "/profile?changepw=true";
+			user = m_api.users[result.userId].get();
 		} catch (Exception e) {
 			import std.encoding : sanitize;
 			logDebug("Error logging in: %s", e.toString().sanitize);
@@ -314,7 +319,7 @@ class UserManWebInterface {
 		m_sessUserName = user.name;
 		m_sessUserFullName = user.fullName;
 		m_sessUserID = user.id.toString();
-		.redirect(redirect.length ? redirect : m_prefix);
+		.redirect(redirectUrl);
 	}
 
 	@noAuth
@@ -430,14 +435,14 @@ class UserManWebInterface {
 	}
 
 	@anyAuth
-	void getProfile(HTTPServerRequest req, User _user, string _error = "")
+	void getProfile(HTTPServerRequest req, User _user, string _error = "", bool changepw = false)
 	{
 		req.form["full_name"] = _user.fullName;
 		req.form["email"] = _user.email;
 		bool useUserNames = m_settings.useUserNames;
 		auto user = _user;
 		string error = _error;
-		render!("userman.profile.dt", user, useUserNames, error);
+		render!("userman.profile.dt", user, useUserNames, error, changepw);
 	}
 
 	@anyAuth @errorDisplay!getProfile