From e6f4ccd8b1fa75f1f0a22288f2bb3ed0d6bfdcfd Mon Sep 17 00:00:00 2001 From: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> Date: Thu, 22 Feb 2024 18:04:15 +0100 Subject: [PATCH 1/5] use password4j to generate and check hashes Signed-off-by: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> --- backend/pom.xml | 5 ++ .../apiv1/auth/PasswordCredential.java | 8 +- .../apiv1/auth/PasswordHashCredential.java | 17 ++++ .../conquery/apiv1/auth/ProtoUser.java | 4 +- .../apiv1/auth/UsernamePasswordToken.java | 2 +- .../models/auth/AuthorizationController.java | 2 +- .../models/auth/AuthorizationHelper.java | 5 +- .../conquery/models/auth/UserManageable.java | 4 +- .../models/auth/basic/AccessTokenCreator.java | 2 +- .../models/auth/basic/CredentialChecker.java | 54 ------------ .../auth/basic/LocalAuthenticationRealm.java | 85 +++++++++++++------ .../models/auth/basic/PasswordHasher.java | 67 +-------------- .../models/auth/basic/PasswordHelper.java | 36 ++++++++ ...UserAuthenticationManagementProcessor.java | 4 +- .../IdpDelegatingAccessTokenCreator.java | 6 +- .../auth/LocalAuthenticationConfig.java | 19 ++++- .../conquery/models/SerializationTests.java | 2 +- .../IdpDelegatingAccessTokenCreatorTest.java | 4 +- .../models/auth/LocalAuthRealmTest.java | 28 +++--- 19 files changed, 169 insertions(+), 185 deletions(-) create mode 100644 backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java delete mode 100644 backend/src/main/java/com/bakdata/conquery/models/auth/basic/CredentialChecker.java create mode 100644 backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java diff --git a/backend/pom.xml b/backend/pom.xml index c7a3e616d9..22cacffd34 100644 --- a/backend/pom.xml +++ b/backend/pom.xml @@ -244,6 +244,11 @@ + + com.password4j + password4j + 1.7.3 + io.dropwizard dropwizard-views-freemarker diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java index 31f161579e..51be370c29 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java +++ b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java @@ -3,23 +3,21 @@ 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.bakdata.conquery.models.config.auth.AuthorizationConfig; import com.fasterxml.jackson.annotation.JsonCreator; -import lombok.AllArgsConstructor; import lombok.Data; import lombok.RequiredArgsConstructor; /** - * Container for holding a password. This credential type is used by the + * 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; + private final CharSequence password; } diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java new file mode 100644 index 0000000000..80ce4bdd03 --- /dev/null +++ b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java @@ -0,0 +1,17 @@ +package com.bakdata.conquery.apiv1.auth; + +import javax.validation.constraints.NotEmpty; + +import com.bakdata.conquery.io.cps.CPSType; +import com.fasterxml.jackson.annotation.JsonCreator; +import lombok.Data; +import lombok.RequiredArgsConstructor; + +@CPSType(base = CredentialType.class, id = "PASSWORD_HASH") +@Data +@RequiredArgsConstructor(onConstructor = @__({@JsonCreator})) +public class PasswordHashCredential { + + @NotEmpty + private final String hash; +} diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/ProtoUser.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/ProtoUser.java index 6d57ee44de..5803a98b58 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/ProtoUser.java +++ b/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 credentials = Collections.emptyList(); + private CredentialType credential = null; public User createOrOverwriteUser(@NonNull MetaStorage storage) { if (label == null) { diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/UsernamePasswordToken.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/UsernamePasswordToken.java index 2c759685b1..ba43c430f8 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/UsernamePasswordToken.java +++ b/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; } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationController.java b/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationController.java index 266d565ce5..e95307e39a 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationController.java +++ b/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); } } } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationHelper.java b/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationHelper.java index 766b3abf54..788e856cc7 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/AuthorizationHelper.java +++ b/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> buildDatasetAbilityMap(Subject subjec } - public static boolean registerForAuthentication(UserManageable userManager, User user, List credentials, boolean override) { + public static boolean registerForAuthentication(UserManageable userManager, User user, CredentialType credentials, boolean override) { if(override) { return userManager.updateUser(user, credentials); } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/UserManageable.java b/backend/src/main/java/com/bakdata/conquery/models/auth/UserManageable.java index 8430603720..8af25184e7 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/UserManageable.java +++ b/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 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 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}). diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/AccessTokenCreator.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/AccessTokenCreator.java index 833dd88a84..b5dceb3194 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/AccessTokenCreator.java +++ b/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); } \ No newline at end of file diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/CredentialChecker.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/CredentialChecker.java deleted file mode 100644 index cd31063ccc..0000000000 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/CredentialChecker.java +++ /dev/null @@ -1,54 +0,0 @@ -package com.bakdata.conquery.models.auth.basic; - -import java.util.Arrays; - -import com.bakdata.conquery.io.storage.Store; -import com.bakdata.conquery.io.storage.xodus.stores.XodusStore; -import com.bakdata.conquery.models.auth.basic.PasswordHasher.HashedEntry; -import com.bakdata.conquery.models.identifiable.ids.specific.UserId; -import jetbrains.exodus.ByteIterable; -import jetbrains.exodus.bindings.StringBinding; -import lombok.experimental.UtilityClass; -import org.apache.shiro.authc.IncorrectCredentialsException; - -@UtilityClass -public class CredentialChecker { - - - /** - * Checks if the provided username password combination is valid. - * This is done by checking the password's hash with the stored hash. - * NOTE: After this operation the provided password is cleared. - * @param username The submitted username, here the email. - * @param providedPassword The submitted password - * @param passwordStore The store that holds the hashed passwords. - * @return True if the username-password combination is valid. - */ - public static boolean validUsernamePassword(String username, char[] providedPassword, Store passwordStore) { - try { - if(username.isEmpty()) { - throw new IncorrectCredentialsException("Username was empty"); - } - if(providedPassword.length < 1) { - throw new IncorrectCredentialsException("Password was empty"); - } - HashedEntry hashedEntry = passwordStore.get(new UserId(username)); - if(hashedEntry == null) { - return false; - } - return isCredentialValid(providedPassword, hashedEntry); - } - finally { - // Erase the provided password - Arrays.fill(providedPassword, '\0'); - } - } - - /** - * Hashes the provided credentials with the salt of the stored hash and compares both. - */ - public static boolean isCredentialValid(char[] providedCredentials, HashedEntry hashedEntry) { - byte[] hashFromProvided = PasswordHasher.generateHash(providedCredentials, hashedEntry.getSalt()); - return Arrays.equals(hashFromProvided, hashedEntry.getHash()); - } -} diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java index e2686f2125..c01ba3290d 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java @@ -11,6 +11,7 @@ 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 +20,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; @@ -29,15 +30,20 @@ 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.IncorrectCredentialsException; +import org.apache.shiro.authc.UnknownAccountException; import org.apache.shiro.realm.AuthenticatingRealm; import org.apache.shiro.util.Destroyable; @@ -67,7 +73,7 @@ public class LocalAuthenticationRealm extends AuthenticatingRealm implements Con @JsonIgnore private Environment passwordEnvironment; @JsonIgnore - private Store passwordStore; + private Store passwordStore; @JsonIgnore private final ConqueryTokenRealm centralTokenRealm; @@ -75,11 +81,14 @@ public class LocalAuthenticationRealm extends AuthenticatingRealm implements Con 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 +115,7 @@ protected void onInit() { validator, mapper, UserId.class, - PasswordHasher.HashedEntry.class, + HashEntry.class, false, true, null, Executors.newSingleThreadExecutor() @@ -126,20 +135,41 @@ public ConqueryAuthenticationInfo doGetAuthenticationInfo(AuthenticationToken to //////////////////// FOR USERNAME/PASSWORD - public String createAccessToken(String username, char[] password) { + public String createAccessToken(String username, String password) { // Check the password which is afterwards cleared - if (!CredentialChecker.validUsernamePassword(username, password, passwordStore)) { + if (username.isEmpty()) { + throw new IncorrectCredentialsException("Username was empty"); + } + if (password.isEmpty()) { + throw new IncorrectCredentialsException("Password was empty"); + } + HashEntry hashedEntry = passwordStore.get(new UserId(username)); + if (hashedEntry == null) { + throw new UnknownAccountException(username); + } + + final String hash = hashedEntry.getHash(); + if (!Password.check(password.getBytes(), hash.getBytes()).with(PasswordHelper.getHashingFunction(hash))) { throw new AuthenticationException("Provided username or password was not valid."); } - // The username is in this case the email + return centralTokenRealm.createTokenForUser(new UserId(username), 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) { + + + if (credential instanceof PasswordCredential passwordCredential) { + return new HashEntry(Password.hash(passwordCredential.getPassword()).addRandomSalt().with(defaultHashingFunction).getResult()); + } + else if (credential instanceof PasswordHashCredential passwordHashCredential) { + return new HashEntry(passwordHashCredential.getHash()); + } + + throw new IllegalArgumentException("CredentialType not supported yet: " + credential.getClass()); } /** @@ -164,28 +194,31 @@ private static Optional getTypePassword(List //////////////////// USER MANAGEMENT //////////////////// @Override - public boolean addUser(User user, List credentials) { - Optional 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 credentials) { - Optional optPassword = getTypePassword(credentials); - if (optPassword.isEmpty()) { - log.trace("No password credential provided. Not adding {} to {}", user.getName(), getName()); - return false; - } - HashedEntry passwordByteIt = optPassword.map(LocalAuthenticationRealm::passwordToHashedEntry).get(); + public boolean updateUser(User user, CredentialType credential) { - 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; } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHasher.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHasher.java index 8a12d1cd9f..68b1109acd 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHasher.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHasher.java @@ -1,17 +1,5 @@ package com.bakdata.conquery.models.auth.basic; -import java.io.IOException; -import java.security.NoSuchAlgorithmException; -import java.security.SecureRandom; -import java.security.spec.InvalidKeySpecException; - -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; - -import com.bakdata.conquery.io.jackson.Jackson; -import com.fasterxml.jackson.core.JsonProcessingException; -import jetbrains.exodus.ArrayByteIterable; -import jetbrains.exodus.ByteIterable; import lombok.Data; import lombok.experimental.UtilityClass; import lombok.extern.slf4j.Slf4j; @@ -25,63 +13,12 @@ @Slf4j public class PasswordHasher { - private static final SecureRandom RANDOM = new SecureRandom(); - private static final String ALGORITHM = "PBKDF2WithHmacSHA1"; - private static final int ITERATIONS = 10000; - private static final int KEY_LENGTH = 256; - - static { - log.info( - "Using the following settings to generate password hashes:\n\tAlgorithm: {}\n\tIterations: {}\n\tKey length: {}", - ALGORITHM, - ITERATIONS, - KEY_LENGTH); - } - - /** - * Returns a random salt to be used to hash a password. - * - * @return a 16 bytes random salt - */ - private static byte[] getNextSalt() { - byte[] salt = new byte[16]; - RANDOM.nextBytes(salt); - return salt; - } - - public static HashedEntry generateHashedEntry(char[] password) { - HashedEntry entry = new HashedEntry(); - entry.setSalt(getNextSalt()); - - entry.setHash(generateHash(password, entry.getSalt())); - return entry; - } - - public static byte[] generateHash(char[] password, byte[] salt) { - PBEKeySpec spec = new PBEKeySpec(password, salt, ITERATIONS, KEY_LENGTH); - SecretKeyFactory f = null; - try { - f = SecretKeyFactory.getInstance(ALGORITHM); - return f.generateSecret(spec).getEncoded(); - } - catch (NoSuchAlgorithmException e) { - throw new IllegalStateException("The indicated algorithm was not found", e); - } - catch (InvalidKeySpecException e) { - throw new IllegalStateException("The key specification was invalid", e); - } - finally { - spec.clearPassword(); - } - } - @Data /** * Container class for the entries in the store consisting of the salted password hash and the corresponding salt. */ - public static class HashedEntry { - byte[] hash; - byte[] salt; + public static class HashEntry { + final String hash; } } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java new file mode 100644 index 0000000000..2e312f635c --- /dev/null +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java @@ -0,0 +1,36 @@ +package com.bakdata.conquery.models.auth.basic; + +import java.util.Map; +import java.util.function.Function; + +import com.password4j.Argon2Function; +import com.password4j.BcryptFunction; +import com.password4j.CompressedPBKDF2Function; +import com.password4j.HashingFunction; +import com.password4j.ScryptFunction; +import lombok.experimental.UtilityClass; +import lombok.extern.slf4j.Slf4j; + +@UtilityClass +@Slf4j +public class PasswordHelper { + + private final static Map, Function> HASH_FUNCTION_GENERATORS = Map.of( + Argon2Function.class, Argon2Function::getInstanceFromHash, + ScryptFunction.class, ScryptFunction::getInstanceFromHash, + BcryptFunction.class, BcryptFunction::getInstanceFromHash, + CompressedPBKDF2Function.class, CompressedPBKDF2Function::getInstanceFromHash + ); + + HashingFunction getHashingFunction(String hash) { + for (Map.Entry, Function> hashFunctionGenerator : HASH_FUNCTION_GENERATORS.entrySet()) { + try { + return hashFunctionGenerator.getValue().apply(hash); + } + catch (Exception e) { + log.trace("Could not create hash function instance from hash using '{}'", hashFunctionGenerator.getKey()); + } + } + throw new IllegalArgumentException("No supported hash function recognized hash. Supported functions: " + HASH_FUNCTION_GENERATORS.keySet()); + } +} diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/UserAuthenticationManagementProcessor.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/UserAuthenticationManagementProcessor.java index 9723706fdf..a4724a36f9 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/UserAuthenticationManagementProcessor.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/UserAuthenticationManagementProcessor.java @@ -27,7 +27,7 @@ public boolean tryRegister(ProtoUser pUser) { return false; } log.trace("Added the user {} to the authorization storage", id); - if(AuthorizationHelper.registerForAuthentication(realm, user, pUser.getCredentials(), false)) { + if (AuthorizationHelper.registerForAuthentication(realm, user, pUser.getCredential(), false)) { log.trace("Added the user {} to the realm {}", id, realm.getName()); return true; } @@ -37,7 +37,7 @@ public boolean tryRegister(ProtoUser pUser) { public boolean updateUser(ProtoUser pUser) { final User user = pUser.createOrOverwriteUser(storage); - AuthorizationHelper.registerForAuthentication(realm, user,pUser.getCredentials(),false); + AuthorizationHelper.registerForAuthentication(realm, user, pUser.getCredential(), false); return true; } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/oidc/passwordflow/IdpDelegatingAccessTokenCreator.java b/backend/src/main/java/com/bakdata/conquery/models/auth/oidc/passwordflow/IdpDelegatingAccessTokenCreator.java index ede4952fde..d7242755b6 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/oidc/passwordflow/IdpDelegatingAccessTokenCreator.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/oidc/passwordflow/IdpDelegatingAccessTokenCreator.java @@ -32,9 +32,9 @@ public class IdpDelegatingAccessTokenCreator implements AccessTokenCreator { @Override @SneakyThrows - public String createAccessToken(String username, char[] password) { - - Secret passwordSecret = new Secret(new String(password)); + public String createAccessToken(String username, String password) { + + Secret passwordSecret = new Secret(password); AuthorizationGrant grant = new ResourceOwnerPasswordCredentialsGrant(username, passwordSecret); diff --git a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java index c39febe33f..c93c3caa1c 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java +++ b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java @@ -24,21 +24,27 @@ import com.bakdata.conquery.resources.admin.rest.UserAuthenticationManagementResource; import com.bakdata.conquery.resources.unprotected.LoginResource; import com.bakdata.conquery.resources.unprotected.TokenResource; +import com.password4j.BcryptFunction; +import com.password4j.BenchmarkResult; +import com.password4j.SystemChecker; import io.dropwizard.jersey.DropwizardResourceConfig; import io.dropwizard.util.Duration; import io.dropwizard.validation.MinDuration; import io.dropwizard.validation.ValidationMethod; import lombok.Getter; import lombok.Setter; +import lombok.extern.slf4j.Slf4j; @CPSType(base = AuthenticationRealmFactory.class, id = "LOCAL_AUTHENTICATION") @Getter @Setter +@Slf4j public class LocalAuthenticationConfig implements AuthenticationRealmFactory { public static final String REDIRECT_URI = "redirect_uri"; + public static final int BCRYPT_MAX_MILLISECONDS = 300; /** - * Configuration for the password store. An encryption for the store it self might be set here. + * Configuration for the password store. An encryption for the store itself might be set here. */ @NotNull private XodusConfig passwordStoreConfig = new XodusConfig(); @@ -74,6 +80,13 @@ public ConqueryAuthenticationRealm createRealm(ManagerNode manager) { // Token extractor is not needed because this realm depends on the ConqueryTokenRealm manager.getAuthController().getAuthenticationFilter().registerTokenExtractor(JWTokenHandler::extractToken); + log.info("Performing benchmark for default hash function (bcrypt) with max_milliseconds={}", BCRYPT_MAX_MILLISECONDS); + final BenchmarkResult result = SystemChecker.benchmarkBcrypt(BCRYPT_MAX_MILLISECONDS); + + int rounds = result.getPrototype().getLogarithmicRounds(); // 12 + long realElapsed = result.getElapsed(); + + log.info("Using bcrypt with {} logarithmic rounds. Elapsed time={}", rounds, realElapsed); LocalAuthenticationRealm realm = new LocalAuthenticationRealm( manager.getValidator(), @@ -82,7 +95,9 @@ public ConqueryAuthenticationRealm createRealm(ManagerNode manager) { storeName, directory, passwordStoreConfig, - jwtDuration); + jwtDuration, + result.getPrototype() + ); UserAuthenticationManagementProcessor processor = new UserAuthenticationManagementProcessor(realm, manager.getStorage()); // Register resources for users to exchange username and password for an access token diff --git a/backend/src/test/java/com/bakdata/conquery/models/SerializationTests.java b/backend/src/test/java/com/bakdata/conquery/models/SerializationTests.java index 3c8ed27c4f..18e949e1b3 100644 --- a/backend/src/test/java/com/bakdata/conquery/models/SerializationTests.java +++ b/backend/src/test/java/com/bakdata/conquery/models/SerializationTests.java @@ -117,7 +117,7 @@ public void dataset() throws IOException, JSONException { @Test public void passwordCredential() throws IOException, JSONException { - PasswordCredential credential = new PasswordCredential("testPassword".toCharArray()); + PasswordCredential credential = new PasswordCredential("testPassword"); SerializationTestUtil .forType(PasswordCredential.class) diff --git a/backend/src/test/java/com/bakdata/conquery/models/auth/IdpDelegatingAccessTokenCreatorTest.java b/backend/src/test/java/com/bakdata/conquery/models/auth/IdpDelegatingAccessTokenCreatorTest.java index d6a3dfb1fc..0d30bd9e8a 100644 --- a/backend/src/test/java/com/bakdata/conquery/models/auth/IdpDelegatingAccessTokenCreatorTest.java +++ b/backend/src/test/java/com/bakdata/conquery/models/auth/IdpDelegatingAccessTokenCreatorTest.java @@ -98,7 +98,7 @@ private static void initOIDCServer() { @Test public void vaildUsernamePassword() { - String jwt = idpDelegatingAccessTokenCreator.createAccessToken(USER_1_NAME, USER_1_PASSWORD.toCharArray()); + String jwt = idpDelegatingAccessTokenCreator.createAccessToken(USER_1_NAME, USER_1_PASSWORD); assertThat(jwt).isEqualTo(USER_1_TOKEN); } @@ -107,7 +107,7 @@ public void vaildUsernamePassword() { public void invaildUsernamePassword() { log.info("This test will print an Error below."); assertThatThrownBy( - () -> idpDelegatingAccessTokenCreator.createAccessToken(USER_1_NAME, "bad_password".toCharArray())) + () -> idpDelegatingAccessTokenCreator.createAccessToken(USER_1_NAME, "bad_password")) .isInstanceOf(IllegalStateException.class); } diff --git a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java index 3c2dd2d9fa..924b657ec1 100644 --- a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java +++ b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java @@ -4,7 +4,6 @@ import java.io.File; import java.nio.file.Files; -import java.util.List; import com.auth0.jwt.JWT; import com.bakdata.conquery.apiv1.auth.PasswordCredential; @@ -14,8 +13,8 @@ import com.bakdata.conquery.models.auth.conquerytoken.ConqueryTokenRealm; import com.bakdata.conquery.models.auth.entities.User; import com.bakdata.conquery.models.config.XodusConfig; -import com.bakdata.conquery.models.identifiable.ids.specific.UserId; import com.bakdata.conquery.util.NonPersistentStoreFactory; +import com.password4j.BcryptFunction; import io.dropwizard.jersey.validation.Validators; import io.dropwizard.util.Duration; import org.apache.commons.io.FileUtils; @@ -53,7 +52,8 @@ public void setupAll() throws Exception { conqueryTokenRealm = new ConqueryTokenRealm(storage); - realm = new LocalAuthenticationRealm(Validators.newValidator(), Jackson.BINARY_MAPPER, conqueryTokenRealm, "localtestRealm", tmpDir, new XodusConfig(), Duration.hours(1)); + realm = + new LocalAuthenticationRealm(Validators.newValidator(), Jackson.BINARY_MAPPER, conqueryTokenRealm, "localtestRealm", tmpDir, new XodusConfig(), Duration.hours(1), BcryptFunction.getInstance(1)); LifecycleUtils.init(realm); } @@ -61,9 +61,9 @@ public void setupAll() throws Exception { public void setupEach() { // Create User in Realm user1 = new User("TestUser", "Test User", storage); - PasswordCredential user1Password = new PasswordCredential("testPassword".toCharArray()); + PasswordCredential user1Password = new PasswordCredential("testPassword"); storage.addUser(user1); - realm.addUser(user1, List.of(user1Password)); + realm.addUser(user1, user1Password); } @AfterEach @@ -81,32 +81,32 @@ public void cleanUpAll() { @Test public void testEmptyUsername() { - assertThatThrownBy(() -> realm.createAccessToken("", "testPassword".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("", "testPassword")) .isInstanceOf(IncorrectCredentialsException.class).hasMessageContaining("Username was empty"); } @Test public void testEmptyPassword() { - assertThatThrownBy(() -> realm.createAccessToken("TestUser", "".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("TestUser", "")) .isInstanceOf(IncorrectCredentialsException.class).hasMessageContaining("Password was empty"); } @Test public void testWrongPassword() { - assertThatThrownBy(() -> realm.createAccessToken("TestUser", "wrongPassword".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("TestUser", "wrongPassword")) .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); } @Test public void testWrongUsername() { - assertThatThrownBy(() -> realm.createAccessToken("NoTestUser", "testPassword".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("NoTestUser", "testPassword")) .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); } @Test public void testValidUsernamePassword() { // Right username and password should yield a JWT - String jwt = realm.createAccessToken("TestUser", "testPassword".toCharArray()); + String jwt = realm.createAccessToken("TestUser", "testPassword"); assertThatCode(() -> JWT.decode(jwt)).doesNotThrowAnyException(); assertThat(conqueryTokenRealm.doGetAuthenticationInfo(new BearerToken(jwt)).getPrincipals().getPrimaryPrincipal()) @@ -116,13 +116,13 @@ public void testValidUsernamePassword() { @Test public void testUserUpdate() { - realm.updateUser(user1, List.of(new PasswordCredential("newTestPassword".toCharArray()))); + realm.updateUser(user1, new PasswordCredential("newTestPassword")); // Wrong (old) password - assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword")) .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); // Right (new) password - String jwt = realm.createAccessToken("TestUser", "newTestPassword".toCharArray()); + String jwt = realm.createAccessToken("TestUser", "newTestPassword"); assertThatCode(() -> JWT.decode(jwt)).doesNotThrowAnyException(); } @@ -130,7 +130,7 @@ public void testUserUpdate() { public void testRemoveUser() { realm.removeUser(user1); // Wrong password - assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword".toCharArray())) + assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword")) .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); } From 5cfc73a7755d6dbd6aab6c380a040f31287d0a00 Mon Sep 17 00:00:00 2001 From: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> Date: Tue, 27 Feb 2024 21:11:08 +0100 Subject: [PATCH 2/5] fix wip progress Signed-off-by: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> --- .../conquery/apiv1/auth/PasswordCredential.java | 12 ++---------- .../conquery/apiv1/auth/PasswordHashCredential.java | 10 +--------- .../models/auth/basic/LocalAuthenticationRealm.java | 10 ++++++---- .../config/auth/LocalAuthenticationConfig.java | 6 ++++-- .../conquery/models/auth/LocalAuthRealmTest.java | 12 ++++++++++-- 5 files changed, 23 insertions(+), 27 deletions(-) diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java index 51be370c29..1d44661943 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java +++ b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordCredential.java @@ -5,19 +5,11 @@ import com.bakdata.conquery.io.cps.CPSType; import com.bakdata.conquery.models.auth.basic.LocalAuthenticationRealm; import com.bakdata.conquery.models.config.auth.AuthorizationConfig; -import com.fasterxml.jackson.annotation.JsonCreator; -import lombok.Data; -import lombok.RequiredArgsConstructor; /** * Container for holding a plain-text password. This credential type is used by the - * {@link LocalAuthenticationRealm} and can be used in the {@link AuthorizationConfig}. + * {@link LocalAuthenticationRealm} and can be used in the {@link AuthorizationConfig}. */ @CPSType(base = CredentialType.class, id = "PASSWORD") -@Data -@RequiredArgsConstructor(onConstructor = @__({@JsonCreator})) -public class PasswordCredential implements CredentialType { - - @NotEmpty - private final CharSequence password; +public record PasswordCredential(@NotEmpty String password) implements CredentialType { } diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java index 80ce4bdd03..04d4bc009f 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java +++ b/backend/src/main/java/com/bakdata/conquery/apiv1/auth/PasswordHashCredential.java @@ -3,15 +3,7 @@ import javax.validation.constraints.NotEmpty; import com.bakdata.conquery.io.cps.CPSType; -import com.fasterxml.jackson.annotation.JsonCreator; -import lombok.Data; -import lombok.RequiredArgsConstructor; @CPSType(base = CredentialType.class, id = "PASSWORD_HASH") -@Data -@RequiredArgsConstructor(onConstructor = @__({@JsonCreator})) -public class PasswordHashCredential { - - @NotEmpty - private final String hash; +public record PasswordHashCredential(@NotEmpty String hash) implements CredentialType { } diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java index c01ba3290d..c3d44f1943 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java @@ -145,12 +145,12 @@ public String createAccessToken(String username, String password) { } HashEntry hashedEntry = passwordStore.get(new UserId(username)); if (hashedEntry == null) { - throw new UnknownAccountException(username); + throw new UnknownAccountException("Provided username or password was not valid."); } final String hash = hashedEntry.getHash(); if (!Password.check(password.getBytes(), hash.getBytes()).with(PasswordHelper.getHashingFunction(hash))) { - throw new AuthenticationException("Provided username or password was not valid."); + throw new UnknownAccountException("Provided username or password was not valid."); } return centralTokenRealm.createTokenForUser(new UserId(username), validDuration); @@ -163,10 +163,12 @@ private HashEntry toHashEntry(CredentialType credential) { if (credential instanceof PasswordCredential passwordCredential) { - return new HashEntry(Password.hash(passwordCredential.getPassword()).addRandomSalt().with(defaultHashingFunction).getResult()); + return new HashEntry(Password.hash(passwordCredential.password()) + .with(defaultHashingFunction) + .getResult()); //.with(defaultHashingFunction).getResult()); } else if (credential instanceof PasswordHashCredential passwordHashCredential) { - return new HashEntry(passwordHashCredential.getHash()); + return new HashEntry(passwordHashCredential.hash()); } throw new IllegalArgumentException("CredentialType not supported yet: " + credential.getClass()); diff --git a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java index c93c3caa1c..bc756a2f02 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java +++ b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java @@ -83,9 +83,11 @@ public ConqueryAuthenticationRealm createRealm(ManagerNode manager) { log.info("Performing benchmark for default hash function (bcrypt) with max_milliseconds={}", BCRYPT_MAX_MILLISECONDS); final BenchmarkResult result = SystemChecker.benchmarkBcrypt(BCRYPT_MAX_MILLISECONDS); - int rounds = result.getPrototype().getLogarithmicRounds(); // 12 + final BcryptFunction prototype = result.getPrototype(); + int rounds = prototype.getLogarithmicRounds(); // 12 long realElapsed = result.getElapsed(); + log.info("Using bcrypt with {} logarithmic rounds. Elapsed time={}", rounds, realElapsed); LocalAuthenticationRealm realm = new LocalAuthenticationRealm( @@ -96,7 +98,7 @@ public ConqueryAuthenticationRealm createRealm(ManagerNode manager) { directory, passwordStoreConfig, jwtDuration, - result.getPrototype() + prototype ); UserAuthenticationManagementProcessor processor = new UserAuthenticationManagementProcessor(realm, manager.getStorage()); diff --git a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java index 924b657ec1..1460d2be14 100644 --- a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java +++ b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java @@ -53,7 +53,15 @@ public void setupAll() throws Exception { conqueryTokenRealm = new ConqueryTokenRealm(storage); realm = - new LocalAuthenticationRealm(Validators.newValidator(), Jackson.BINARY_MAPPER, conqueryTokenRealm, "localtestRealm", tmpDir, new XodusConfig(), Duration.hours(1), BcryptFunction.getInstance(1)); + new LocalAuthenticationRealm( + Validators.newValidator(), + Jackson.BINARY_MAPPER, conqueryTokenRealm, + "localtestRealm", + tmpDir, + new XodusConfig(), + Duration.hours(4), + BcryptFunction.getInstance(4) + ); // 4 is minimum LifecycleUtils.init(realm); } @@ -100,7 +108,7 @@ public void testWrongPassword() { @Test public void testWrongUsername() { assertThatThrownBy(() -> realm.createAccessToken("NoTestUser", "testPassword")) - .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); + .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); } @Test From 7d5d22ca8f8e774f5ba32fb0b65532112c437287 Mon Sep 17 00:00:00 2001 From: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> Date: Wed, 28 Feb 2024 15:23:02 +0100 Subject: [PATCH 3/5] handle null credentials Signed-off-by: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> --- .../models/auth/basic/LocalAuthenticationRealm.java | 9 +++++++-- .../models/config/auth/LocalAuthenticationConfig.java | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java index c3d44f1943..016782e7e1 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java @@ -63,7 +63,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; @@ -165,7 +165,7 @@ private HashEntry toHashEntry(CredentialType credential) { if (credential instanceof PasswordCredential passwordCredential) { return new HashEntry(Password.hash(passwordCredential.password()) .with(defaultHashingFunction) - .getResult()); //.with(defaultHashingFunction).getResult()); + .getResult()); } else if (credential instanceof PasswordHashCredential passwordHashCredential) { return new HashEntry(passwordHashCredential.hash()); @@ -212,6 +212,11 @@ public boolean addUser(@NonNull User user, @NonNull CredentialType credential) { @Override public boolean updateUser(User user, CredentialType credential) { + if (credential == null) { + log.warn("Skipping user '{}' because no credential was provided", user.getId()); + return false; + } + try { final HashEntry hashEntry = toHashEntry(credential); passwordStore.update(user.getId(), hashEntry); diff --git a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java index bc756a2f02..fb3f005465 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java +++ b/backend/src/main/java/com/bakdata/conquery/models/config/auth/LocalAuthenticationConfig.java @@ -84,7 +84,7 @@ public ConqueryAuthenticationRealm createRealm(ManagerNode manager) { final BenchmarkResult result = SystemChecker.benchmarkBcrypt(BCRYPT_MAX_MILLISECONDS); final BcryptFunction prototype = result.getPrototype(); - int rounds = prototype.getLogarithmicRounds(); // 12 + int rounds = prototype.getLogarithmicRounds(); long realElapsed = result.getElapsed(); From 5b4b88c81d013bd5574ddc9466bfdf442381e1c3 Mon Sep 17 00:00:00 2001 From: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> Date: Thu, 29 Feb 2024 08:37:05 +0100 Subject: [PATCH 4/5] use explicit exceptions which are internally caught and generalized for the end user Signed-off-by: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> --- .../auth/basic/LocalAuthenticationRealm.java | 33 ++++--------------- .../resources/unprotected/TokenResource.java | 12 ++++++- .../models/auth/LocalAuthRealmTest.java | 10 +++--- 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java index 016782e7e1..26ad9445c8 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/LocalAuthenticationRealm.java @@ -3,7 +3,6 @@ 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; @@ -29,7 +28,6 @@ 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; @@ -42,8 +40,8 @@ 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.authc.UnknownAccountException; import org.apache.shiro.realm.AuthenticatingRealm; import org.apache.shiro.util.Destroyable; @@ -136,24 +134,24 @@ public ConqueryAuthenticationInfo doGetAuthenticationInfo(AuthenticationToken to //////////////////// FOR USERNAME/PASSWORD public String createAccessToken(String username, String password) { - // Check the password which is afterwards cleared if (username.isEmpty()) { throw new IncorrectCredentialsException("Username was empty"); } if (password.isEmpty()) { throw new IncorrectCredentialsException("Password was empty"); } - HashEntry hashedEntry = passwordStore.get(new UserId(username)); + final UserId userId = new UserId(username); + HashEntry hashedEntry = passwordStore.get(userId); if (hashedEntry == null) { - throw new UnknownAccountException("Provided username or password was not valid."); + 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 UnknownAccountException("Provided username or password was not valid."); + throw new IncorrectCredentialsException("Password was was invalid for user: " + userId); } - return centralTokenRealm.createTokenForUser(new UserId(username), validDuration); + return centralTokenRealm.createTokenForUser(userId, validDuration); } /** @@ -174,25 +172,6 @@ else if (credential instanceof PasswordHashCredential passwordHashCredential) { throw new IllegalArgumentException("CredentialType not supported yet: " + credential.getClass()); } - /** - * 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 getTypePassword(List credentials) { - if(credentials == null) { - return Optional.empty(); - } - return credentials.stream() - .filter(PasswordCredential.class::isInstance) - .map(PasswordCredential.class::cast) - .collect(MoreCollectors.toOptional()); - } - //////////////////// USER MANAGEMENT //////////////////// @Override diff --git a/backend/src/main/java/com/bakdata/conquery/resources/unprotected/TokenResource.java b/backend/src/main/java/com/bakdata/conquery/resources/unprotected/TokenResource.java index 23cd8c62d6..da4be49818 100644 --- a/backend/src/main/java/com/bakdata/conquery/resources/unprotected/TokenResource.java +++ b/backend/src/main/java/com/bakdata/conquery/resources/unprotected/TokenResource.java @@ -1,6 +1,7 @@ package com.bakdata.conquery.resources.unprotected; import javax.ws.rs.Consumes; +import javax.ws.rs.NotAuthorizedException; import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.Produces; @@ -10,9 +11,12 @@ import com.bakdata.conquery.apiv1.auth.UsernamePasswordToken; import com.bakdata.conquery.models.auth.basic.AccessTokenCreator; import lombok.AllArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.shiro.authc.AuthenticationException; @Path("/") @AllArgsConstructor +@Slf4j public class TokenResource { private final AccessTokenCreator realm; @@ -21,6 +25,12 @@ public class TokenResource { @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) public JwtWrapper getToken(UsernamePasswordToken token) { - return new JwtWrapper(realm.createAccessToken(token.getUser(), token.getPassword())); + try { + return new JwtWrapper(realm.createAccessToken(token.getUser(), token.getPassword())); + } + catch (AuthenticationException e) { + log.warn("Failed to authorize request", e); + throw new NotAuthorizedException("Failed to authenticate request. The cause has been logged."); + } } } diff --git a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java index 1460d2be14..fa560e3c01 100644 --- a/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java +++ b/backend/src/test/java/com/bakdata/conquery/models/auth/LocalAuthRealmTest.java @@ -18,8 +18,8 @@ import io.dropwizard.jersey.validation.Validators; import io.dropwizard.util.Duration; import org.apache.commons.io.FileUtils; -import org.apache.shiro.authc.AuthenticationException; import org.apache.shiro.authc.BearerToken; +import org.apache.shiro.authc.CredentialsException; import org.apache.shiro.authc.IncorrectCredentialsException; import org.apache.shiro.util.LifecycleUtils; import org.junit.jupiter.api.AfterAll; @@ -102,13 +102,13 @@ public void testEmptyPassword() { @Test public void testWrongPassword() { assertThatThrownBy(() -> realm.createAccessToken("TestUser", "wrongPassword")) - .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); + .isInstanceOf(IncorrectCredentialsException.class).hasMessageContaining("Password was was invalid for user"); } @Test public void testWrongUsername() { assertThatThrownBy(() -> realm.createAccessToken("NoTestUser", "testPassword")) - .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); + .isInstanceOf(CredentialsException.class).hasMessageContaining("No password hash was found for user"); } @Test @@ -127,7 +127,7 @@ public void testUserUpdate() { realm.updateUser(user1, new PasswordCredential("newTestPassword")); // Wrong (old) password assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword")) - .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); + .isInstanceOf(IncorrectCredentialsException.class).hasMessageContaining("Password was was invalid for user"); // Right (new) password String jwt = realm.createAccessToken("TestUser", "newTestPassword"); @@ -139,7 +139,7 @@ public void testRemoveUser() { realm.removeUser(user1); // Wrong password assertThatThrownBy(() -> realm.createAccessToken("TestUser", "testPassword")) - .isInstanceOf(AuthenticationException.class).hasMessageContaining("Provided username or password was not valid."); + .isInstanceOf(CredentialsException.class).hasMessageContaining("No password hash was found for user"); } } From 76b164a898cc33a4707dbf1b6e937501798d2c89 Mon Sep 17 00:00:00 2001 From: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> Date: Thu, 29 Feb 2024 14:14:58 +0100 Subject: [PATCH 5/5] adds test for password helper function Signed-off-by: Max Thonagel <12283268+thoniTUB@users.noreply.github.com> --- .../models/auth/basic/PasswordHelper.java | 6 ++- .../conquery/util/PasswordHelperTest.java | 54 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 backend/src/test/java/com/bakdata/conquery/util/PasswordHelperTest.java diff --git a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java index 2e312f635c..549c0ee70f 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java +++ b/backend/src/main/java/com/bakdata/conquery/models/auth/basic/PasswordHelper.java @@ -22,7 +22,11 @@ public class PasswordHelper { CompressedPBKDF2Function.class, CompressedPBKDF2Function::getInstanceFromHash ); - HashingFunction getHashingFunction(String hash) { + /** + * Determines the function used to create the provided hash. + * The function can be used to hash a plain credential in order to check the hashes for equality. + */ + public HashingFunction getHashingFunction(String hash) { for (Map.Entry, Function> hashFunctionGenerator : HASH_FUNCTION_GENERATORS.entrySet()) { try { return hashFunctionGenerator.getValue().apply(hash); diff --git a/backend/src/test/java/com/bakdata/conquery/util/PasswordHelperTest.java b/backend/src/test/java/com/bakdata/conquery/util/PasswordHelperTest.java new file mode 100644 index 0000000000..b6ae79e8c9 --- /dev/null +++ b/backend/src/test/java/com/bakdata/conquery/util/PasswordHelperTest.java @@ -0,0 +1,54 @@ +package com.bakdata.conquery.util; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.stream.Stream; + +import com.bakdata.conquery.models.auth.basic.PasswordHelper; +import com.password4j.Argon2Function; +import com.password4j.BcryptFunction; +import com.password4j.CompressedPBKDF2Function; +import com.password4j.HashingFunction; +import com.password4j.ScryptFunction; +import com.password4j.types.Argon2; +import com.password4j.types.Bcrypt; +import com.password4j.types.Hmac; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +public class PasswordHelperTest { + + + /** + * Arguments where generated with: + * + *
+	 * import com.password4j.Password;
+	 *
+	 * class Scratch {
+	 * 	public static void main(String[] args) {
+	 * 		System.out.println(Password.hash("test").withArgon2().getResult());
+	 * 		System.out.println(Password.hash("test").withScrypt().getResult());
+	 * 		System.out.println(Password.hash("test").withBcrypt().getResult());
+	 * 		System.out.println(Password.hash("test").withCompressedPBKDF2().getResult());
+	 *        }
+	 * }
+	 * 
+ */ + static Stream arguments() { + return Stream.of( + Arguments.arguments("$argon2id$v=19$m=15360,t=2,p=1$r35m/UGz8lq4ICjcNkb2GUcfYub07450QRTTapYwiJCQDOI9Maa0dlym/iL0AceTNNXgaxLUyGB5EfJoqr+Wng$WVnHZU8uwvufgWPlVh5T+MnTtX5Ry0hhCD0ej90L0Kk", Argon2Function.getInstance(15360, 2, 1, 32, Argon2.ID)), + Arguments.arguments("$100801$SBpPHCtLT+2FbJ2BS49J4sgRXfvduVm17U9yd0Ygky/3MgUgK1r4LMixKSQX4LQjSEuE6tV8ibABXXAr9tCZKA==$aPTssj2maVw34QgrhRIsUHu6irB1NrjiFpdpUXFHHA+XhjPG03PKrbj5CBXJx3cCUosU/IARQliSW2LWRLFtiw==", ScryptFunction.getInstance(65536, 8, 1, 64)), + Arguments.arguments("$2b$10$YMPj.MoAs81tO8HzrCYxnOujaPwbu5SGsSrdNyxdIJ9BlBIv9i0t.", BcryptFunction.getInstance(Bcrypt.B, 10)), + Arguments.arguments("$3$1331439861760256$+Rqke26gKhtP60UkVR2a3SfszrOkVrMiJ6LZUWvl2vI5OpW815zKiod8Sdz3aOcuajo6c1iKEXcWjk61emmgTw==$CiH0mwqibUZD5R5HqFNpaYCkWjiYcTQe0sjG+4ZYw/A=", CompressedPBKDF2Function.getInstance(Hmac.SHA256, 310000, 256)) + ); + } + + @ParameterizedTest + @MethodSource("arguments") + void test(String hash, HashingFunction hashProvider) { + assertThat(PasswordHelper.getHashingFunction(hash)).isEqualTo(hashProvider); + + } +}