diff options
9 files changed, 304 insertions, 175 deletions
diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDto.java b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDto.java index 001db5f5eab..d58d4cda753 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDto.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDto.java @@ -45,7 +45,7 @@ public class UserDto implements UserId { private String externalIdentityProvider; // Hashed password that may be null in case of external authentication private String cryptedPassword; - // Salt used for SHA1, null when bcrypt is used or for external authentication + // Salt used for PBKDF2, null when bcrypt is used or for external authentication private String salt; // Hash method used to generate cryptedPassword, my be null in case of external authentication private String hashMethod; diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/DbVersion94.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/DbVersion94.java index 84a055a79c7..818ed58e6fe 100644 --- a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/DbVersion94.java +++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/DbVersion94.java @@ -30,6 +30,7 @@ public class DbVersion94 implements DbVersion { .add(6302, "Drop unused Issues Column ACTION_PLAN_KEY", DropActionPlanKeyIssueColumn.class) .add(6303, "Drop unused Issues Column ISSUE_ATTRIBUTES", DropIssuesAttributesIssueColumn.class) .add(6304, "Create table 'SCANNER_ANALYSIS_CACHE", CreateScannerAnalysisCacheTable.class) + .add(6305, "Issue warning for users using SHA1 hash method", SelectUsersWithSha1HashMethod.class) ; } } diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethod.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethod.java new file mode 100644 index 00000000000..4d9ee081ff7 --- /dev/null +++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethod.java @@ -0,0 +1,49 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.platform.db.migration.version.v94; + +import java.sql.SQLException; +import java.util.List; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; +import org.sonar.db.Database; +import org.sonar.server.platform.db.migration.step.DataChange; +import org.sonar.server.platform.db.migration.step.Select; + +public class SelectUsersWithSha1HashMethod extends DataChange { + private static final Logger LOG = Loggers.get(SelectUsersWithSha1HashMethod.class); + + private static final String UNSUPPORTED_HASH_METHOD = "SHA1"; + + public SelectUsersWithSha1HashMethod(Database db) { + super(db); + } + + @Override + protected void execute(Context context) throws SQLException { + Select select = context.prepareSelect("select login from users where hash_method = ?"); + select.setString(1, UNSUPPORTED_HASH_METHOD); + List<String> logins = select.list(row -> row.getString(1)); + if (!logins.isEmpty()) { + LOG.warn("The following local accounts have their password hashed with an algorithm which is not longer supported. " + + "They will not be able to login anymore. Please reset their password if the accounts need to be kept. {}", logins); + } + } +} diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest.java new file mode 100644 index 00000000000..71285657851 --- /dev/null +++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest.java @@ -0,0 +1,110 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.platform.db.migration.version.v94; + +import java.sql.SQLException; +import java.util.HashMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.junit.Rule; +import org.junit.Test; +import org.sonar.api.utils.log.LogAndArguments; +import org.sonar.api.utils.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; +import org.sonar.core.util.UuidFactory; +import org.sonar.core.util.UuidFactoryFast; +import org.sonar.db.CoreDbTester; +import org.sonar.server.platform.db.migration.step.DataChange; + +import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; +import static org.apache.commons.lang.RandomStringUtils.randomNumeric; +import static org.assertj.core.api.Assertions.assertThat; + +public class SelectUsersWithSha1HashMethodTest { + + private static final UuidFactory UUID_FACTORY = UuidFactoryFast.getInstance(); + + @Rule + public LogTester logTester = new LogTester(); + + @Rule + public CoreDbTester db = CoreDbTester.createForSchema(SelectUsersWithSha1HashMethodTest.class, "schema.sql"); + + private final DataChange underTest = new SelectUsersWithSha1HashMethod(db.database()); + + @Test + public void migration_ifSomeUsersUseSha1_shouldLogThem() throws SQLException { + String user1sha1 = insertUser("SHA1"); + String user2sha1 = insertUser("SHA1"); + insertUser(null); + insertUser("PBKDF2"); + insertUser("BCRYPT"); + insertUser(""); + + underTest.execute(); + + assertThat(logTester.getLogs(LoggerLevel.WARN)) + .hasSize(1) + .first() + .extracting(LogAndArguments::getFormattedMsg) + .asString() + .startsWith("The following local accounts have their password hashed with an algorithm which is not longer supported. They will not be able to login anymore. " + + "Please reset their password if the accounts need to be kept.") + .contains(user1sha1, user2sha1); + } + + @Test + public void migration_ifAllUsersAreNotUsingSha1_shouldNotLogAnything() throws SQLException { + insertUser(null); + insertUser("PBKDF2"); + insertUser("BCRYPT"); + insertUser(""); + + underTest.execute(); + + assertThat(logTester.getLogs()).isEmpty(); + } + + @Test + public void migration_should_be_reentrant() throws SQLException { + underTest.execute(); + // re-entrant + underTest.execute(); + } + + private String insertUser(@Nullable String hashMethod) { + String login = hashMethod + randomAlphabetic(20); + + Map<String, Object> map = new HashMap<>(); + String uuid = UUID_FACTORY.create(); + map.put("UUID", uuid); + map.put("LOGIN", login); + map.put("HASH_METHOD", hashMethod); + map.put("EXTERNAL_LOGIN", login); + map.put("EXTERNAL_IDENTITY_PROVIDER", "sonarqube"); + map.put("EXTERNAL_ID", randomNumeric(5)); + map.put("IS_ROOT", false); + map.put("ONBOARDED", false); + map.put("CREATED_AT", System.currentTimeMillis()); + map.put("RESET_PASSWORD", false); + db.executeInsert("users", map); + return login; + } +} diff --git a/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest/schema.sql b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest/schema.sql new file mode 100644 index 00000000000..e2eca57a0c0 --- /dev/null +++ b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v94/SelectUsersWithSha1HashMethodTest/schema.sql @@ -0,0 +1,30 @@ +CREATE TABLE "USERS"( + "UUID" CHARACTER VARYING(255) NOT NULL, + "LOGIN" CHARACTER VARYING(255) NOT NULL, + "NAME" CHARACTER VARYING(200), + "EMAIL" CHARACTER VARYING(100), + "CRYPTED_PASSWORD" CHARACTER VARYING(100), + "SALT" CHARACTER VARYING(40), + "HASH_METHOD" CHARACTER VARYING(10), + "ACTIVE" BOOLEAN DEFAULT TRUE, + "SCM_ACCOUNTS" CHARACTER VARYING(4000), + "EXTERNAL_LOGIN" CHARACTER VARYING(255) NOT NULL, + "EXTERNAL_IDENTITY_PROVIDER" CHARACTER VARYING(100) NOT NULL, + "EXTERNAL_ID" CHARACTER VARYING(255) NOT NULL, + "IS_ROOT" BOOLEAN NOT NULL, + "USER_LOCAL" BOOLEAN, + "ONBOARDED" BOOLEAN NOT NULL, + "HOMEPAGE_TYPE" CHARACTER VARYING(40), + "HOMEPAGE_PARAMETER" CHARACTER VARYING(40), + "LAST_CONNECTION_DATE" BIGINT, + "CREATED_AT" BIGINT, + "UPDATED_AT" BIGINT, + "RESET_PASSWORD" BOOLEAN NOT NULL, + "LAST_SONARLINT_CONNECTION" BIGINT, + "SONARLINT_AD_SEEN" BOOLEAN DEFAULT FALSE +); +ALTER TABLE "USERS" ADD CONSTRAINT "PK_USERS" PRIMARY KEY("UUID"); +CREATE UNIQUE INDEX "USERS_LOGIN" ON "USERS"("LOGIN" NULLS FIRST); +CREATE INDEX "USERS_UPDATED_AT" ON "USERS"("UPDATED_AT" NULLS FIRST); +CREATE UNIQUE INDEX "UNIQ_EXTERNAL_ID" ON "USERS"("EXTERNAL_IDENTITY_PROVIDER" NULLS FIRST, "EXTERNAL_ID" NULLS FIRST); +CREATE UNIQUE INDEX "UNIQ_EXTERNAL_LOGIN" ON "USERS"("EXTERNAL_IDENTITY_PROVIDER" NULLS FIRST, "EXTERNAL_LOGIN" NULLS FIRST); diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java index 5654f0a74a3..3933ba9d251 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java @@ -35,7 +35,7 @@ import static org.sonar.server.authentication.event.AuthenticationEvent.Source; * delegated to an external system, e.g. LDAP. */ public class CredentialsAuthentication { - + static final String ERROR_PASSWORD_CANNOT_BE_NULL = "Password cannot be null"; private final DbClient dbClient; private final AuthenticationEvent authenticationEvent; private final CredentialsExternalAuthentication externalAuthentication; @@ -58,7 +58,8 @@ public class CredentialsAuthentication { private UserDto authenticate(DbSession dbSession, Credentials credentials, HttpServletRequest request, Method method) { UserDto localUser = dbClient.userDao().selectActiveUserByLogin(dbSession, credentials.getLogin()); if (localUser != null && localUser.isLocal()) { - localAuthentication.authenticate(dbSession, localUser, credentials.getPassword().orElse(null), method); + String password = getNonNullPassword(credentials); + localAuthentication.authenticate(dbSession, localUser, password, method); dbSession.commit(); authenticationEvent.loginSuccess(request, localUser.getLogin(), Source.local(method)); return localUser; @@ -75,4 +76,8 @@ public class CredentialsAuthentication { .build(); } + private static String getNonNullPassword(Credentials credentials) { + return credentials.getPassword().orElseThrow(() -> new IllegalArgumentException(ERROR_PASSWORD_CANNOT_BE_NULL)); + } + } diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java index 757dd01075a..1ffa66aa015 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java @@ -27,7 +27,6 @@ import java.util.EnumMap; import javax.annotation.Nullable; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; -import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.lang.RandomStringUtils; import org.mindrot.jbcrypt.BCrypt; import org.sonar.api.config.Configuration; @@ -40,7 +39,6 @@ import org.sonar.server.authentication.event.AuthenticationException; import static com.google.common.base.Preconditions.checkArgument; import static java.lang.String.format; -import static java.util.Objects.requireNonNull; /** * Validates the password of a "local" user (password is stored in @@ -51,7 +49,6 @@ public class CredentialsLocalAuthentication { public static final String ERROR_NULL_PASSWORD_IN_DB = "null password in DB"; public static final String ERROR_NULL_SALT = "null salt"; public static final String ERROR_WRONG_PASSWORD = "wrong password"; - public static final String ERROR_PASSWORD_CANNOT_BE_NULL = "Password cannot be null"; public static final String ERROR_UNKNOWN_HASH_METHOD = "Unknown hash method [%s]"; private static final SecureRandom SECURE_RANDOM = new SecureRandom(); private static final String PBKDF2_ITERATIONS_PROP = "sonar.internal.pbkdf2.iterations"; @@ -62,13 +59,12 @@ public class CredentialsLocalAuthentication { private final EnumMap<HashMethod, HashFunction> hashFunctions = new EnumMap<>(HashMethod.class); public enum HashMethod { - SHA1, BCRYPT, PBKDF2 + BCRYPT, PBKDF2 } public CredentialsLocalAuthentication(DbClient dbClient, Configuration configuration) { this.dbClient = dbClient; hashFunctions.put(HashMethod.BCRYPT, new BcryptFunction()); - hashFunctions.put(HashMethod.SHA1, new Sha1Function()); hashFunctions.put(HashMethod.PBKDF2, new PBKDF2Function(configuration.getInt(PBKDF2_ITERATIONS_PROP).orElse(null))); } @@ -84,7 +80,19 @@ public class CredentialsLocalAuthentication { * If the password must be updated because an old algorithm is used, the UserDto is updated but the session * is not committed */ - public void authenticate(DbSession session, UserDto user, @Nullable String password, Method method) { + public void authenticate(DbSession session, UserDto user, String password, Method method) { + HashMethod hashMethod = getHashMethod(user, method); + HashFunction hashFunction = hashFunctions.get(hashMethod); + AuthenticationResult result = authenticate(user, password, method, hashFunction); + + // Upgrade the password if it's an old hashMethod + if (hashMethod != DEFAULT || result.needsUpdate) { + hashFunctions.get(DEFAULT).storeHashPassword(user, password); + dbClient.userDao().update(session, user); + } + } + + private HashMethod getHashMethod(UserDto user, Method method) { if (user.getHashMethod() == null) { throw AuthenticationException.newBuilder() .setSource(Source.local(method)) @@ -92,10 +100,8 @@ public class CredentialsLocalAuthentication { .setMessage(ERROR_NULL_HASH_METHOD) .build(); } - - HashMethod hashMethod; try { - hashMethod = HashMethod.valueOf(user.getHashMethod()); + return HashMethod.valueOf(user.getHashMethod()); } catch (IllegalArgumentException ex) { generateHashToAvoidEnumerationAttack(); throw AuthenticationException.newBuilder() @@ -104,9 +110,9 @@ public class CredentialsLocalAuthentication { .setMessage(format(ERROR_UNKNOWN_HASH_METHOD, user.getHashMethod())) .build(); } + } - HashFunction hashFunction = hashFunctions.get(hashMethod); - + private static AuthenticationResult authenticate(UserDto user, String password, Method method, HashFunction hashFunction) { AuthenticationResult result = hashFunction.checkCredentials(user, password); if (!result.isSuccessful()) { throw AuthenticationException.newBuilder() @@ -115,12 +121,7 @@ public class CredentialsLocalAuthentication { .setMessage(result.getFailureMessage()) .build(); } - - // Upgrade the password if it's an old hashMethod - if (hashMethod != DEFAULT || result.needsUpdate) { - hashFunctions.get(DEFAULT).storeHashPassword(user, password); - dbClient.userDao().update(session, user); - } + return result; } /** @@ -170,41 +171,6 @@ public class CredentialsLocalAuthentication { } } - /** - * Implementation of deprecated SHA1 hash function - */ - private static final class Sha1Function implements HashFunction { - @Override - public AuthenticationResult checkCredentials(UserDto user, String password) { - if (user.getCryptedPassword() == null) { - return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB); - } - if (user.getSalt() == null) { - return new AuthenticationResult(false, ERROR_NULL_SALT); - } - if (!user.getCryptedPassword().equals(hash(user.getSalt(), password))) { - return new AuthenticationResult(false, ERROR_WRONG_PASSWORD); - } - return new AuthenticationResult(true, ""); - } - - @Override - public void storeHashPassword(UserDto user, String password) { - requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL); - byte[] saltRandom = new byte[20]; - SECURE_RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - - user.setHashMethod(HashMethod.SHA1.name()) - .setCryptedPassword(hash(salt, password)) - .setSalt(salt); - } - - private static String hash(String salt, String password) { - return DigestUtils.sha1Hex("--" + salt + "--" + password + "--"); - } - } - static final class PBKDF2Function implements HashFunction { private static final char ITERATIONS_HASH_SEPARATOR = '$'; private static final int DEFAULT_ITERATIONS = 100_000; @@ -298,7 +264,6 @@ public class CredentialsLocalAuthentication { @Override public void storeHashPassword(UserDto user, String password) { - requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL); user.setHashMethod(HashMethod.BCRYPT.name()) .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12))) .setSalt(null); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java index 340a2fc3d3c..b02bb5fe859 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java @@ -23,7 +23,6 @@ import java.util.Optional; import javax.servlet.http.HttpServletRequest; import org.junit.Rule; import org.junit.Test; -import org.mockito.Mockito; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.utils.System2; import org.sonar.db.DbClient; @@ -33,13 +32,17 @@ import org.sonar.db.user.UserDto; import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationException; +import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.sonar.db.user.UserTesting.newUserDto; +import static org.sonar.server.authentication.CredentialsAuthentication.ERROR_PASSWORD_CANNOT_BE_NULL; +import static org.sonar.server.authentication.CredentialsLocalAuthentication.ERROR_UNKNOWN_HASH_METHOD; import static org.sonar.server.authentication.event.AuthenticationEvent.Method.BASIC; import static org.sonar.server.authentication.event.AuthenticationEvent.Method.BASIC_TOKEN; import static org.sonar.server.authentication.event.AuthenticationEvent.Source; @@ -49,25 +52,27 @@ public class CredentialsAuthenticationTest { private static final String LOGIN = "LOGIN"; private static final String PASSWORD = "PASSWORD"; private static final String SALT = "0242b0b4c0a93ddfe09dd886de50bc25ba000b51"; - private static final String ENCRYPTED_PASSWORD = "540e4fc4be4e047db995bc76d18374a5b5db08cc"; + private static final int NUMBER_OF_PBKDF2_ITERATIONS = 1; + private static final String ENCRYPTED_PASSWORD = format("%d$%s", NUMBER_OF_PBKDF2_ITERATIONS, "FVu1Wtpe0MM/Rs+CcLT7nbzMMQ0emHDXpcfjJoQrDtCe8cQqWP4rpCXZenBw9bC3/UWx5+kA9go9zKkhq2UmAQ=="); + private static final String DEPRECATED_HASH_METHOD = "SHA1"; @Rule public DbTester dbTester = DbTester.create(System2.INSTANCE); - private DbClient dbClient = dbTester.getDbClient(); - private DbSession dbSession = dbTester.getSession(); - private HttpServletRequest request = mock(HttpServletRequest.class); - private AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class); - private MapSettings settings = new MapSettings().setProperty("sonar.internal.pbkdf2.iterations", "1"); - private CredentialsExternalAuthentication externalAuthentication = mock(CredentialsExternalAuthentication.class); - private CredentialsLocalAuthentication localAuthentication = Mockito.spy(new CredentialsLocalAuthentication(dbClient, settings.asConfig())); - private CredentialsAuthentication underTest = new CredentialsAuthentication(dbClient, authenticationEvent, externalAuthentication, localAuthentication); + private final DbClient dbClient = dbTester.getDbClient(); + private final DbSession dbSession = dbTester.getSession(); + private final HttpServletRequest request = mock(HttpServletRequest.class); + private final AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class); + private final MapSettings settings = new MapSettings().setProperty("sonar.internal.pbkdf2.iterations", NUMBER_OF_PBKDF2_ITERATIONS); + private final CredentialsExternalAuthentication externalAuthentication = mock(CredentialsExternalAuthentication.class); + private final CredentialsLocalAuthentication localAuthentication = spy(new CredentialsLocalAuthentication(dbClient, settings.asConfig())); + private final CredentialsAuthentication underTest = new CredentialsAuthentication(dbClient, authenticationEvent, externalAuthentication, localAuthentication); @Test public void authenticate_local_user() { insertUser(newUserDto() .setLogin(LOGIN) .setCryptedPassword(ENCRYPTED_PASSWORD) - .setHashMethod(CredentialsLocalAuthentication.HashMethod.SHA1.name()) + .setHashMethod(CredentialsLocalAuthentication.HashMethod.PBKDF2.name()) .setSalt(SALT) .setLocal(true)); @@ -80,9 +85,9 @@ public class CredentialsAuthenticationTest { public void fail_to_authenticate_local_user_when_password_is_wrong() { insertUser(newUserDto() .setLogin(LOGIN) - .setCryptedPassword("Wrong password") - .setSalt("Wrong salt") - .setHashMethod(CredentialsLocalAuthentication.HashMethod.SHA1.name()) + .setCryptedPassword(format("%d$%s", NUMBER_OF_PBKDF2_ITERATIONS, "WrongPassword")) + .setSalt("salt") + .setHashMethod(CredentialsLocalAuthentication.HashMethod.PBKDF2.name()) .setLocal(true)); assertThatThrownBy(() -> executeAuthenticate(BASIC)) @@ -130,7 +135,7 @@ public class CredentialsAuthenticationTest { .setLogin(LOGIN) .setCryptedPassword(null) .setSalt(SALT) - .setHashMethod(CredentialsLocalAuthentication.HashMethod.SHA1.name()) + .setHashMethod(CredentialsLocalAuthentication.HashMethod.PBKDF2.name()) .setLocal(true)); assertThatThrownBy(() -> executeAuthenticate(BASIC)) @@ -148,7 +153,7 @@ public class CredentialsAuthenticationTest { .setLogin(LOGIN) .setCryptedPassword(ENCRYPTED_PASSWORD) .setSalt(null) - .setHashMethod(CredentialsLocalAuthentication.HashMethod.SHA1.name()) + .setHashMethod(CredentialsLocalAuthentication.HashMethod.PBKDF2.name()) .setLocal(true)); assertThatThrownBy(() -> executeAuthenticate(BASIC_TOKEN)) @@ -161,6 +166,42 @@ public class CredentialsAuthenticationTest { } @Test + public void fail_to_authenticate_unknown_hash_method_should_force_hash() { + insertUser(newUserDto() + .setLogin(LOGIN) + .setCryptedPassword(ENCRYPTED_PASSWORD) + .setSalt(SALT) + .setHashMethod(DEPRECATED_HASH_METHOD) + .setLocal(true)); + + assertThatThrownBy(() -> executeAuthenticate(BASIC_TOKEN)) + .hasMessage(format(ERROR_UNKNOWN_HASH_METHOD, DEPRECATED_HASH_METHOD)) + .isInstanceOf(AuthenticationException.class) + .hasFieldOrPropertyWithValue("source", Source.local(BASIC_TOKEN)) + .hasFieldOrPropertyWithValue("login", LOGIN); + + verify(localAuthentication).generateHashToAvoidEnumerationAttack(); + verifyNoInteractions(authenticationEvent); + } + + @Test + public void local_authentication_without_password_should_throw_IAE() { + insertUser(newUserDto() + .setLogin(LOGIN) + .setCryptedPassword(ENCRYPTED_PASSWORD) + .setSalt(SALT) + .setHashMethod(DEPRECATED_HASH_METHOD) + .setLocal(true)); + + Credentials credentials = new Credentials(LOGIN, null); + assertThatThrownBy(() -> underTest.authenticate(credentials, request, BASIC_TOKEN)) + .hasMessage(ERROR_PASSWORD_CANNOT_BE_NULL) + .isInstanceOf(IllegalArgumentException.class); + + verifyNoInteractions(authenticationEvent); + } + + @Test public void fail_to_authenticate_unknown_user_after_forcing_hash() { assertThatThrownBy(() -> executeAuthenticate(BASIC)) .hasMessage("No active user for login") diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java index 72631d2ee95..2ff39335325 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java @@ -19,6 +19,8 @@ */ package org.sonar.server.authentication; +import java.security.SecureRandom; +import java.util.Base64; import java.util.Optional; import java.util.Random; import org.apache.commons.codec.digest.DigestUtils; @@ -36,13 +38,17 @@ import org.sonar.server.authentication.event.AuthenticationException; import static java.lang.String.format; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.sonar.db.user.UserTesting.newUserDto; import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.BCRYPT; import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.PBKDF2; -import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.SHA1; public class CredentialsLocalAuthenticationTest { + + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + private static final String PBKDF2_SALT = generatePBKDF2Salt(); + @Rule public DbTester db = DbTester.create(); @@ -89,7 +95,7 @@ public class CredentialsLocalAuthenticationTest { } @Test - public void authentication_with_sha1_with_correct_password_should_work() { + public void authentication_with_sha1_should_throw_AuthenticationException() { String password = randomAlphanumeric(60); byte[] saltRandom = new byte[20]; @@ -97,62 +103,14 @@ public class CredentialsLocalAuthenticationTest { String salt = DigestUtils.sha1Hex(saltRandom); UserDto user = newUserDto() - .setHashMethod(SHA1.name()) + .setHashMethod("SHA1") .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) .setSalt(salt); - underTest.authenticate(db.getSession(), user, password, AuthenticationEvent.Method.BASIC); - } - - @Test - public void authentication_with_sha1_with_incorrect_password_should_throw_AuthenticationException() { - String password = randomAlphanumeric(60); - DbSession dbSession = db.getSession(); - - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - - UserDto user = newUserDto() - .setHashMethod(SHA1.name()) - .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) - .setSalt(salt); - - assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) - .isInstanceOf(AuthenticationException.class) - .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD); - } - - @Test - public void authentication_with_sha1_with_empty_password_should_throw_AuthenticationException() { - DbSession dbSession = db.getSession(); - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - - UserDto user = newUserDto() - .setCryptedPassword(null) - .setHashMethod(SHA1.name()) - .setSalt(salt); - - assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) - .isInstanceOf(AuthenticationException.class) - .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB); - } - - @Test - public void authentication_with_sha1_with_empty_salt_should_throw_AuthenticationException() { - DbSession dbSession = db.getSession(); - String password = randomAlphanumeric(60); - - UserDto user = newUserDto() - .setHashMethod(SHA1.name()) - .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--")) - .setSalt(null); - - assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) - .isInstanceOf(AuthenticationException.class) - .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT); + DbSession session = db.getSession(); + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> underTest.authenticate(session, user, password, AuthenticationEvent.Method.BASIC)) + .withMessage("Unknown hash method [SHA1]"); } @Test @@ -182,44 +140,14 @@ public class CredentialsLocalAuthenticationTest { } @Test - public void authentication_upgrade_hash_function_when_SHA1_was_used() { - String password = randomAlphanumeric(60); - - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - - UserDto user = newUserDto() - .setLogin("myself") - .setHashMethod(SHA1.name()) - .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) - .setSalt(salt); - db.users().insertUser(user); - - underTest.authenticate(db.getSession(), user, password, AuthenticationEvent.Method.BASIC); - - Optional<UserDto> myself = db.users().selectUserByLogin("myself"); - assertThat(myself).isPresent(); - assertThat(myself.get().getHashMethod()).isEqualTo(PBKDF2.name()); - assertThat(myself.get().getSalt()).isNotNull(); - - // authentication must work with upgraded hash method - underTest.authenticate(db.getSession(), user, password, AuthenticationEvent.Method.BASIC); - } - - @Test public void authentication_upgrade_hash_function_when_BCRYPT_was_used() { String password = randomAlphanumeric(60); - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - UserDto user = newUserDto() .setLogin("myself") .setHashMethod(BCRYPT.name()) .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12))) - .setSalt(salt); + .setSalt(null); db.users().insertUser(user); underTest.authenticate(db.getSession(), user, password, AuthenticationEvent.Method.BASIC); @@ -297,44 +225,37 @@ public class CredentialsLocalAuthenticationTest { } @Test - public void authentication_with_pbkdf2_with_invalid_password_should_throw_AuthenticationException() { + public void authentication_with_pbkdf2_with_invalid_hash_should_throw_AuthenticationException() { DbSession dbSession = db.getSession(); String password = randomAlphanumeric(60); - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); - UserDto userInvalidHash = newUserDto() .setHashMethod(PBKDF2.name()) - .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) - .setSalt(salt); + .setCryptedPassword(password) + .setSalt(PBKDF2_SALT); - assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidHash, "WHATEVER", AuthenticationEvent.Method.BASIC)) + assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidHash, password, AuthenticationEvent.Method.BASIC)) .isInstanceOf(AuthenticationException.class) .hasMessage("invalid hash stored"); UserDto userInvalidIterations = newUserDto() .setHashMethod(PBKDF2.name()) - .setCryptedPassword("a$") - .setSalt(salt); + .setCryptedPassword("a$" + password) + .setSalt(PBKDF2_SALT); - assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidIterations, "WHATEVER", AuthenticationEvent.Method.BASIC)) + assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidIterations, password, AuthenticationEvent.Method.BASIC)) .isInstanceOf(AuthenticationException.class) .hasMessage("invalid hash stored"); } @Test public void authentication_with_pbkdf2_with_empty_password_should_throw_AuthenticationException() { - byte[] saltRandom = new byte[20]; - RANDOM.nextBytes(saltRandom); - String salt = DigestUtils.sha1Hex(saltRandom); DbSession dbSession = db.getSession(); UserDto user = newUserDto() .setCryptedPassword(null) .setHashMethod(PBKDF2.name()) - .setSalt(salt); + .setSalt(PBKDF2_SALT); assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) .isInstanceOf(AuthenticationException.class) @@ -348,11 +269,18 @@ public class CredentialsLocalAuthenticationTest { UserDto user = newUserDto() .setHashMethod(PBKDF2.name()) - .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--")) + .setCryptedPassword("1$" + password) .setSalt(null); - assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, password, AuthenticationEvent.Method.BASIC)) .isInstanceOf(AuthenticationException.class) .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT); } + + private static String generatePBKDF2Salt() { + byte[] salt = new byte[20]; + SECURE_RANDOM.nextBytes(salt); + String saltStr = Base64.getEncoder().encodeToString(salt); + return saltStr; + } } |