Merge pull request #3313 from ingef/fix/update-password-hashing

use password4j to generate and check hashes

backend/pom.xml

@@ -244,6 +244,11 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>com.password4j</groupId>
+            <artifactId>password4j</artifactId>
+            <version>1.7.3</version>
+        </dependency>
         <dependency>
             <groupId>io.dropwizard</groupId>
             <artifactId>dropwizard-views-freemarker</artifactId>

backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java

@@ -3,23 +3,13 @@
 import javax.validation.constraints.NotEmpty;
 
 import com.bakdata.conquery.io.cps.CPSType;
-import com.bakdata.conquery.models.config.auth.AuthorizationConfig;
 import com.bakdata.conquery.models.auth.basic.LocalAuthenticationRealm;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import lombok.AllArgsConstructor;
-import lombok.Data;
-import lombok.RequiredArgsConstructor;
+import com.bakdata.conquery.models.config.auth.AuthorizationConfig;
 
 /**
- * Container for holding a password. This credential type is used by the
- * {@link LocalAuthenticationRealm} and can be used in the {@link AuthorizationConfig}. 
+ * Container for holding a plain-text password. This credential type is used by the
+ * {@link LocalAuthenticationRealm} and can be used in the {@link AuthorizationConfig}.
  */
 @CPSType(base = CredentialType.class, id = "PASSWORD")
-@Data
-@RequiredArgsConstructor(onConstructor = @__({@JsonCreator}))
-@AllArgsConstructor
-public class PasswordCredential implements CredentialType {
-
-	@NotEmpty
-	private char[] password;
+public record PasswordCredential(@NotEmpty String password) implements CredentialType {
 }

backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java

@@ -0,0 +1,9 @@
+package com.bakdata.conquery.apiv1.auth;
+
+import javax.validation.constraints.NotEmpty;
+
+import com.bakdata.conquery.io.cps.CPSType;
+
+@CPSType(base = CredentialType.class, id = "PASSWORD_HASH")
+public record PasswordHashCredential(@NotEmpty String hash) implements CredentialType {
+}

backend/src/main/java/com/bakdata/conquery/apiv1/auth/ProtoUser.java

@@ -1,7 +1,6 @@
 package com.bakdata.conquery.apiv1.auth;
 
 import java.util.Collections;
-import java.util.List;
 import java.util.Set;
 
 import javax.validation.Valid;
@@ -45,9 +44,8 @@ public class ProtoUser {
 	 * {@link UserManageable}, such as {@link LocalAuthenticationRealm}).
 	 */
 	@Builder.Default
-	@NotNull
 	@Valid
-	private List<CredentialType> credentials = Collections.emptyList();
+	private CredentialType credential = null;
 
 	public User createOrOverwriteUser(@NonNull MetaStorage storage) {
 		if (label == null) {

backend/src/main/java/com/bakdata/conquery/apiv1/auth/UsernamePasswordToken.java

@@ -17,5 +17,5 @@ public class UsernamePasswordToken {
 	@NotEmpty
 	private String user;
 	@NotEmpty
-	private char[] password;
+	private String password;
 }

backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationController.java

@@ -145,7 +145,7 @@ private static void initializeAuthConstellation(@NonNull AuthorizationConfig con
 			final User user = pUser.createOrOverwriteUser(storage);
 			for (Realm realm : realms) {
 				if (realm instanceof UserManageable) {
-					AuthorizationHelper.registerForAuthentication((UserManageable) realm, user, pUser.getCredentials(), true);
+					AuthorizationHelper.registerForAuthentication((UserManageable) realm, user, pUser.getCredential(), true);
 				}
 			}
 		}

backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationHelper.java

@@ -1,6 +1,5 @@
 package com.bakdata.conquery.models.auth;
 
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -14,8 +13,8 @@
 import com.bakdata.conquery.io.storage.MetaStorage;
 import com.bakdata.conquery.models.auth.entities.Group;
 import com.bakdata.conquery.models.auth.entities.Role;
-import com.bakdata.conquery.models.auth.entities.User;
 import com.bakdata.conquery.models.auth.entities.Subject;
+import com.bakdata.conquery.models.auth.entities.User;
 import com.bakdata.conquery.models.auth.permissions.Ability;
 import com.bakdata.conquery.models.auth.permissions.ConqueryPermission;
 import com.bakdata.conquery.models.datasets.Dataset;
@@ -129,7 +128,7 @@ public static Map<DatasetId, Set<Ability>> buildDatasetAbilityMap(Subject subjec
 	}
 
 
-	public static boolean registerForAuthentication(UserManageable userManager, User user, List<CredentialType> credentials, boolean override) {
+	public static boolean registerForAuthentication(UserManageable userManager, User user, CredentialType credentials, boolean override) {
 		if(override) {
 			return userManager.updateUser(user, credentials);
 		}

backend/src/main/java/com/bakdata/conquery/models/auth/UserManageable.java

@@ -20,13 +20,13 @@ public interface UserManageable {
 	 * @param credentials A List of credentials that are provided by the user.
 	 * @return True upon successful adding of the user. False if the user could not be added or was already present.
 	 */
-	boolean addUser(User user, List<CredentialType> credentials);
+	boolean addUser(User user, CredentialType credential);
 
 
 	/**
 	 * Similar to {@link UserManageable#addUser(User, List)} but if the user already existed it is overridden, when a fitting {@link CredentialType} was found.
 	 */
-	boolean updateUser(User user, List<CredentialType> credentials);
+	boolean updateUser(User user, CredentialType credential);
 
 	/**
 	 * Removes a user from the realm only but not from the local permission storage (i.e. {@link MetaStorage}).

backend/src/main/java/com/bakdata/conquery/models/auth/basic/AccessTokenCreator.java

@@ -7,6 +7,6 @@ public interface AccessTokenCreator {
 	 *
 	 * @return A valid access token that authenticates the user that provided the credentials
 	 */
-	String createAccessToken(String username, char[] password);
+	String createAccessToken(String username, String password);
 
 }

backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java

@@ -3,14 +3,14 @@
 import java.io.File;
 import java.io.IOException;
 import java.util.List;
-import java.util.Optional;
 import java.util.concurrent.Executors;
 
 import javax.validation.Validator;
 
 import com.bakdata.conquery.Conquery;
 import com.bakdata.conquery.apiv1.auth.CredentialType;
 import com.bakdata.conquery.apiv1.auth.PasswordCredential;
+import com.bakdata.conquery.apiv1.auth.PasswordHashCredential;
 import com.bakdata.conquery.io.storage.MetaStorage;
 import com.bakdata.conquery.io.storage.Store;
 import com.bakdata.conquery.io.storage.StoreMappings;
@@ -19,7 +19,7 @@
 import com.bakdata.conquery.models.auth.ConqueryAuthenticationInfo;
 import com.bakdata.conquery.models.auth.ConqueryAuthenticationRealm;
 import com.bakdata.conquery.models.auth.UserManageable;
-import com.bakdata.conquery.models.auth.basic.PasswordHasher.HashedEntry;
+import com.bakdata.conquery.models.auth.basic.PasswordHasher.HashEntry;
 import com.bakdata.conquery.models.auth.conquerytoken.ConqueryTokenRealm;
 import com.bakdata.conquery.models.auth.entities.User;
 import com.bakdata.conquery.models.auth.util.SkippingCredentialsMatcher;
@@ -28,16 +28,20 @@
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.MoreCollectors;
+import com.password4j.HashingFunction;
+import com.password4j.Password;
 import io.dropwizard.util.Duration;
 import jetbrains.exodus.ExodusException;
 import jetbrains.exodus.env.Environment;
 import jetbrains.exodus.env.EnvironmentClosedException;
 import jetbrains.exodus.env.Environments;
+import lombok.NonNull;
 import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.shiro.authc.AuthenticationException;
 import org.apache.shiro.authc.AuthenticationToken;
+import org.apache.shiro.authc.CredentialsException;
+import org.apache.shiro.authc.IncorrectCredentialsException;
 import org.apache.shiro.realm.AuthenticatingRealm;
 import org.apache.shiro.util.Destroyable;
 
@@ -57,7 +61,7 @@ public class LocalAuthenticationRealm extends AuthenticatingRealm implements Con
 
 	private static final int ENVIRONMNENT_CLOSING_RETRYS = 2;
 	private static final int ENVIRONMNENT_CLOSING_TIMEOUT = 2; // seconds
-	// Get the path for the storage here so it is set when as soon the first class is instantiated (in the ManagerNode)
+	// Get the path for the storage here, so it is set as soon the first class is instantiated (in the ManagerNode)
 	// In the DistributedStandaloneCommand this directory is overriden multiple times before LocalAuthenticationRealm::onInit for the ShardNodes, so this is a problem.
 	private final File storageDir;
 
@@ -67,19 +71,22 @@ public class LocalAuthenticationRealm extends AuthenticatingRealm implements Con
 	@JsonIgnore
 	private Environment passwordEnvironment;
 	@JsonIgnore
-	private Store<UserId, PasswordHasher.HashedEntry> passwordStore;
+	private Store<UserId, HashEntry> passwordStore;
 
 	@JsonIgnore
 	private final ConqueryTokenRealm centralTokenRealm;
 	private final Duration validDuration;
 	private final Validator validator;
 	private final ObjectMapper mapper;
 
+	private final HashingFunction defaultHashingFunction;
+
 	//////////////////// INITIALIZATION ////////////////////
 
-	public LocalAuthenticationRealm(Validator validator, ObjectMapper mapper, ConqueryTokenRealm centralTokenRealm, String storeName, File storageDir, XodusConfig passwordStoreConfig, Duration validDuration) {
+	public LocalAuthenticationRealm(Validator validator, ObjectMapper mapper, ConqueryTokenRealm centralTokenRealm, String storeName, File storageDir, XodusConfig passwordStoreConfig, Duration validDuration, HashingFunction defaultHashingFunction) {
 		this.validator = validator;
 		this.mapper = mapper;
+		this.defaultHashingFunction = defaultHashingFunction;
 		this.setCredentialsMatcher(SkippingCredentialsMatcher.INSTANCE);
 		this.storeName = storeName;
 		this.storageDir = storageDir;
@@ -106,7 +113,7 @@ protected void onInit() {
 						validator,
 						mapper,
 						UserId.class,
-						PasswordHasher.HashedEntry.class,
+						HashEntry.class,
 						false,
 						true,
 						null, Executors.newSingleThreadExecutor()
@@ -126,66 +133,78 @@ public ConqueryAuthenticationInfo doGetAuthenticationInfo(AuthenticationToken to
 
 	//////////////////// FOR USERNAME/PASSWORD
 
-	public String createAccessToken(String username, char[] password) {
-		// Check the password which is afterwards cleared
-		if (!CredentialChecker.validUsernamePassword(username, password, passwordStore)) {
-			throw new AuthenticationException("Provided username or password was not valid.");
+	public String createAccessToken(String username, String password) {
+		if (username.isEmpty()) {
+			throw new IncorrectCredentialsException("Username was empty");
+		}
+		if (password.isEmpty()) {
+			throw new IncorrectCredentialsException("Password was empty");
+		}
+		final UserId userId = new UserId(username);
+		HashEntry hashedEntry = passwordStore.get(userId);
+		if (hashedEntry == null) {
+			throw new CredentialsException("No password hash was found for user: " + username);
+		}
+
+		final String hash = hashedEntry.getHash();
+		if (!Password.check(password.getBytes(), hash.getBytes()).with(PasswordHelper.getHashingFunction(hash))) {
+			throw new IncorrectCredentialsException("Password was was invalid for user: " + userId);
 		}
-		// The username is in this case the email
-		return centralTokenRealm.createTokenForUser(new UserId(username), validDuration);
+
+		return centralTokenRealm.createTokenForUser(userId, validDuration);
 	}
 
 	/**
 	 * Converts the provided password to a Xodus compatible hash.
 	 */
-	private static HashedEntry passwordToHashedEntry(PasswordCredential credential) {
-		return PasswordHasher.generateHashedEntry(credential.getPassword());
-	}
+	private HashEntry toHashEntry(CredentialType credential) {
 
-	/**
-	 * Checks the provided credentials for the realm-compatible
-	 * {@link PasswordCredential}. However only one credential of this type is
-	 * allowed to be provided.
-	 *
-	 * @param credentials
-	 *            A list of possible credentials.
-	 * @return The password credential.
-	 */
-	private static Optional<PasswordCredential> getTypePassword(List<CredentialType> credentials) {
-		if(credentials == null) {
-			return Optional.empty();
+
+		if (credential instanceof PasswordCredential passwordCredential) {
+			return new HashEntry(Password.hash(passwordCredential.password())
+										 .with(defaultHashingFunction)
+										 .getResult());
 		}
-		return credentials.stream()
-			.filter(PasswordCredential.class::isInstance)
-			.map(PasswordCredential.class::cast)
-			.collect(MoreCollectors.toOptional());
+		else if (credential instanceof PasswordHashCredential passwordHashCredential) {
+			return new HashEntry(passwordHashCredential.hash());
+		}
+
+		throw new IllegalArgumentException("CredentialType not supported yet: " + credential.getClass());
 	}
 
 	//////////////////// USER MANAGEMENT ////////////////////
 
 	@Override
-	public boolean addUser(User user, List<CredentialType> credentials) {
-		Optional<PasswordCredential> optPassword = getTypePassword(credentials);
-		if (optPassword.isEmpty()) {
-			log.trace("No password credential provided. Not adding {} to {}", user.getName(), getName());
-			return false;
+	public boolean addUser(@NonNull User user, @NonNull CredentialType credential) {
+
+		try {
+			final HashEntry hashEntry = toHashEntry(credential);
+			passwordStore.add(user.getId(), hashEntry);
+			return true;
 		}
-		HashedEntry passwordByteIt = optPassword.map(LocalAuthenticationRealm::passwordToHashedEntry).get();
-		passwordStore.add(user.getId(), passwordByteIt);
-		return true;
+		catch (IllegalArgumentException e) {
+			log.warn("Unable to add user '{}'", user.getId(), e);
+		}
+		return false;
 	}
 
 	@Override
-	public boolean updateUser(User user, List<CredentialType> credentials) {
-		Optional<PasswordCredential> optPassword = getTypePassword(credentials);
-		if (optPassword.isEmpty()) {
-			log.trace("No password credential provided. Not adding {} to {}", user.getName(), getName());
+	public boolean updateUser(User user, CredentialType credential) {
+
+		if (credential == null) {
+			log.warn("Skipping user '{}' because no credential was provided", user.getId());
 			return false;
 		}
-		HashedEntry passwordByteIt = optPassword.map(LocalAuthenticationRealm::passwordToHashedEntry).get();
 
-		passwordStore.update(user.getId(), passwordByteIt);
-		return true;
+		try {
+			final HashEntry hashEntry = toHashEntry(credential);
+			passwordStore.update(user.getId(), hashEntry);
+			return true;
+		}
+		catch (IllegalArgumentException e) {
+			log.warn("Unable to update user '{}'", user.getId(), e);
+		}
+		return false;
 
 	}