SONAR-6949 Implements bcrypt hash for password

Extract hash mechanism into a single class LocalAuthentication Implements SHA1 (deprecated) and bcrypt hash Set bcrypt as default Update the hash of a user during authentication if hash method was SHA1
SonarSource · Apr 17, 2018 · 7f88e7c · 7f88e7c
1 parent f7adccd
commit 7f88e7c
Show file tree

Hide file tree

Showing 26 changed files with 476 additions and 108 deletions.
diff --git a/server/sonar-db-core/src/main/resources/org/sonar/db/version/rows-h2.sql b/server/sonar-db-core/src/main/resources/org/sonar/db/version/rows-h2.sql
@@ -1,4 +1,4 @@
-INSERT INTO USERS(ID, LOGIN, NAME, EMAIL, EXTERNAL_IDENTITY, EXTERNAL_IDENTITY_PROVIDER, USER_LOCAL, CRYPTED_PASSWORD, SALT, IS_ROOT, ONBOARDED, CREATED_AT, UPDATED_AT) VALUES (1, 'admin', 'Administrator', '', 'admin', 'sonarqube', true, 'a373a0e667abb2604c1fd571eb4ad47fe8cc0878', '48bc4b0d93179b5103fd3885ea9119498e9d161b', false, false, '1418215735482', '1418215735482');
+INSERT INTO USERS(ID, LOGIN, NAME, EMAIL, EXTERNAL_IDENTITY, EXTERNAL_IDENTITY_PROVIDER, USER_LOCAL, CRYPTED_PASSWORD, SALT, HASH_METHOD, IS_ROOT, ONBOARDED, CREATED_AT, UPDATED_AT) VALUES (1, 'admin', 'Administrator', '', 'admin', 'sonarqube', true, '$2a$12$uCkkXmhW5ThVK8mpBvnXOOJRLd64LJeHTeCkSuB3lfaR2N0AYBaSi', null, 'BCRYPT', false, false, '1418215735482', '1418215735482');
 ALTER TABLE USERS ALTER COLUMN ID RESTART WITH 2;
 
 INSERT INTO GROUPS(ID, ORGANIZATION_UUID, NAME, DESCRIPTION, CREATED_AT, UPDATED_AT) VALUES (1, 'AVdqnciQUUs7Zd3KPvFD', 'sonar-administrators', 'System administrators', '2011-09-26 22:27:51.0', '2011-09-26 22:27:51.0');

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
@@ -25,11 +25,8 @@
 import java.util.List;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
-import org.apache.commons.codec.digest.DigestUtils;
 import org.sonar.core.user.DefaultUser;
 
-import static java.util.Objects.requireNonNull;
-
 /**
  * @since 3.2
  */
@@ -44,8 +41,11 @@ public class UserDto {
   private String scmAccounts;
   private String externalIdentity;
   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
   private String salt;
+  // Hash method used to generate cryptedPassword, my be null in case of external authentication
   private String hashMethod;
   private Long createdAt;
   private Long updatedAt;
@@ -192,7 +192,7 @@ public String getHashMethod() {
     return hashMethod;
   }
 
-  public UserDto setHashMethod(String hashMethod) {
+  public UserDto setHashMethod(@Nullable String hashMethod) {
     this.hashMethod = hashMethod;
     return this;
   }
@@ -260,12 +260,6 @@ public UserDto setOnboarded(boolean onboarded) {
     return this;
   }
 
-  public static String encryptPassword(String password, String salt) {
-    requireNonNull(password, "Password cannot be empty");
-    requireNonNull(salt, "Salt cannot be empty");
-    return DigestUtils.sha1Hex("--" + salt + "--" + password + "--");
-  }
-
   public DefaultUser toUser() {
     return new DefaultUser()
       .setLogin(login)

diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml
@@ -221,6 +221,7 @@
         onboarded = #{user.onboarded, jdbcType=BOOLEAN},
         salt = #{user.salt, jdbcType=VARCHAR},
         crypted_password = #{user.cryptedPassword, jdbcType=BIGINT},
+        hash_method = #{user.hashMethod, jdbcType=VARCHAR},
         updated_at = #{now, jdbcType=BIGINT},
         homepage_type = #{user.homepageType, jdbcType=VARCHAR},
         homepage_parameter = #{user.homepageParameter, jdbcType=VARCHAR}

diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java
@@ -319,6 +319,7 @@ public void insert_user() {
       .setOnboarded(true)
       .setSalt("1234")
       .setCryptedPassword("abcd")
+      .setHashMethod("SHA1")
       .setExternalIdentity("johngithub")
       .setExternalIdentityProvider("github")
       .setLocal(true)
@@ -340,6 +341,7 @@ public void insert_user() {
     assertThat(user.getScmAccounts()).isEqualTo(",jo.hn,john2,");
     assertThat(user.getSalt()).isEqualTo("1234");
     assertThat(user.getCryptedPassword()).isEqualTo("abcd");
+    assertThat(user.getHashMethod()).isEqualTo("SHA1");
     assertThat(user.getExternalIdentity()).isEqualTo("johngithub");
     assertThat(user.getExternalIdentityProvider()).isEqualTo("github");
     assertThat(user.isLocal()).isTrue();
@@ -368,6 +370,7 @@ public void update_user() {
       .setOnboarded(true)
       .setSalt("12345")
       .setCryptedPassword("abcde")
+      .setHashMethod("BCRYPT")
       .setExternalIdentity("johngithub")
       .setExternalIdentityProvider("github")
       .setLocal(false)
@@ -386,6 +389,7 @@ public void update_user() {
     assertThat(reloaded.getScmAccounts()).isEqualTo(",jo.hn,john2,johndoo,");
     assertThat(reloaded.getSalt()).isEqualTo("12345");
     assertThat(reloaded.getCryptedPassword()).isEqualTo("abcde");
+    assertThat(reloaded.getHashMethod()).isEqualTo("BCRYPT");
     assertThat(reloaded.getExternalIdentity()).isEqualTo("johngithub");
     assertThat(reloaded.getExternalIdentityProvider()).isEqualTo("github");
     assertThat(reloaded.isLocal()).isFalse();

diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDtoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDtoTest.java
@@ -46,21 +46,4 @@ public void decode_scm_accounts() {
     assertThat(UserDto.decodeScmAccounts("\nfoo\n")).containsOnly("foo");
     assertThat(UserDto.decodeScmAccounts("\nfoo\nbar\n")).containsOnly("foo", "bar");
   }
-
-  @Test
-  public void encrypt_password() {
-    assertThat(UserDto.encryptPassword("PASSWORD", "0242b0b4c0a93ddfe09dd886de50bc25ba000b51")).isEqualTo("540e4fc4be4e047db995bc76d18374a5b5db08cc");
-  }
-
-  @Test
-  public void fail_to_encrypt_password_when_password_is_null() {
-    expectedException.expect(NullPointerException.class);
-    UserDto.encryptPassword(null, "salt");
-  }
-
-  @Test
-  public void fail_to_encrypt_password_when_salt_is_null() {
-    expectedException.expect(NullPointerException.class);
-    UserDto.encryptPassword("password", null);
-  }
 }
diff --git a/server/sonar-db-migration/build.gradle b/server/sonar-db-migration/build.gradle
@@ -18,6 +18,7 @@ dependencies {
   testCompile 'org.assertj:assertj-core'
   testCompile 'org.dbunit:dbunit'
   testCompile 'org.mockito:mockito-core'
+  testCompile 'org.mindrot:jbcrypt'
   testCompile project(':sonar-testing-harness')
   testCompile project(':server:sonar-db-core').sourceSets.test.output
 

diff --git a/...gration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java b/...gration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java
@@ -27,7 +27,7 @@ public class DbVersion72 implements DbVersion {
   @Override
   public void addSteps(MigrationStepRegistry registry) {
     registry
-      .add(2100, "Increase size of CRYPTED_PASSWORD", IncreaseCryptedPasswordSize.class)
+      .add(2100, "Increase size of USERS.CRYPTED_PASSWORD", IncreaseCryptedPasswordSize.class)
       .add(2101, "Add HASH_METHOD to table users", AddHashMethodToUsersTable.class)
       .add(2102, "Populate HASH_METHOD on table users", PopulateHashMethodOnUsers.class)
     ;

diff --git a/...a/org/sonar/server/platform/db/migration/version/v72/IncreaseCryptedPasswordSizeTest.java b/...a/org/sonar/server/platform/db/migration/version/v72/IncreaseCryptedPasswordSizeTest.java
@@ -1,4 +1,4 @@
-package org.sonar.server.platform.db.migration.version.v72;/*
+/*
  * SonarQube
  * Copyright (C) 2009-2018 SonarSource SA
  * mailto:info AT sonarsource DOT com
@@ -17,12 +17,12 @@
  * 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.v72;
 
 import java.sql.SQLException;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
-import org.mindrot.jbcrypt.BCrypt;
 import org.sonar.db.CoreDbTester;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -53,9 +53,10 @@ public void can_insert_crypted_password_after_execute() throws SQLException {
   }
 
   private void insertRow() {
+    // bcrypt hash is 60 characters
     db.executeInsert(
       "USERS",
-      "CRYPTED_PASSWORD", BCrypt.hashpw("a", BCrypt.gensalt()),
+      "CRYPTED_PASSWORD", "$2a$10$8tscphgcElKF5vOBer4H.OVfLKpPIH74hK.rxyhOP5HVyZHyfgRGy",
       "IS_ROOT", false,
       "ONBOARDED", false);
   }

diff --git a/server/sonar-server/build.gradle b/server/sonar-server/build.gradle
@@ -45,6 +45,7 @@ dependencies {
   compile 'org.slf4j:jul-to-slf4j'
   compile 'org.slf4j:slf4j-api'
   compile 'org.sonarsource.update-center:sonar-update-center-common'
+  compile 'org.mindrot:jbcrypt'
 
   compile project(':server:sonar-db-dao')
   compile project(':server:sonar-db-migration')

diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java
@@ -47,6 +47,7 @@ protected void configureModule() {
       LoginAction.class,
       LogoutAction.class,
       CredentialsAuthenticator.class,
+      LocalAuthentication.class,
       RealmAuthenticator.class,
       BasicAuthenticator.class,
       ValidateAction.class,

diff --git a/.../sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthenticator.java b/.../sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthenticator.java
@@ -20,16 +20,13 @@
 package org.sonar.server.authentication;
 
 import java.util.Optional;
-import javax.annotation.CheckForNull;
-import javax.annotation.Nullable;
 import javax.servlet.http.HttpServletRequest;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.user.UserDto;
 import org.sonar.server.authentication.event.AuthenticationEvent;
 import org.sonar.server.authentication.event.AuthenticationException;
 
-import static org.sonar.db.user.UserDto.encryptPassword;
 import static org.sonar.server.authentication.event.AuthenticationEvent.Method;
 import static org.sonar.server.authentication.event.AuthenticationEvent.Source;
 
@@ -38,11 +35,14 @@ public class CredentialsAuthenticator {
   private final DbClient dbClient;
   private final RealmAuthenticator externalAuthenticator;
   private final AuthenticationEvent authenticationEvent;
+  private final LocalAuthentication localAuthentication;
 
-  public CredentialsAuthenticator(DbClient dbClient, RealmAuthenticator externalAuthenticator, AuthenticationEvent authenticationEvent) {
+  public CredentialsAuthenticator(DbClient dbClient, RealmAuthenticator externalAuthenticator, AuthenticationEvent authenticationEvent,
+    LocalAuthentication localAuthentication) {
     this.dbClient = dbClient;
     this.externalAuthenticator = externalAuthenticator;
     this.authenticationEvent = authenticationEvent;
+    this.localAuthentication = localAuthentication;
   }
 
   public UserDto authenticate(String userLogin, String userPassword, HttpServletRequest request, Method method) {
@@ -54,9 +54,9 @@ public UserDto authenticate(String userLogin, String userPassword, HttpServletRe
   private UserDto authenticate(DbSession dbSession, String userLogin, String userPassword, HttpServletRequest request, Method method) {
     UserDto localUser = dbClient.userDao().selectActiveUserByLogin(dbSession, userLogin);
     if (localUser != null && localUser.isLocal()) {
-      UserDto userDto = authenticateFromDb(localUser, userPassword, method);
+      localAuthentication.authenticate(dbSession, localUser, userPassword, method);
       authenticationEvent.loginSuccess(request, userLogin, Source.local(method));
-      return userDto;
+      return localUser;
     }
     Optional<UserDto> externalUser = externalAuthenticator.authenticate(userLogin, userPassword, request, method);
     if (externalUser.isPresent()) {
@@ -68,30 +68,4 @@ private UserDto authenticate(DbSession dbSession, String userLogin, String userP
       .setMessage(localUser != null && !localUser.isLocal() ? "User is not local" : "No active user for login")
       .build();
   }
-
-  private static UserDto authenticateFromDb(UserDto userDto, String userPassword, Method method) {
-    String cryptedPassword = userDto.getCryptedPassword();
-    String salt = userDto.getSalt();
-    String failureCause = checkPassword(cryptedPassword, salt, userPassword);
-    if (failureCause == null) {
-      return userDto;
-    }
-    throw AuthenticationException.newBuilder()
-      .setSource(Source.local(method))
-      .setLogin(userDto.getLogin())
-      .setMessage(failureCause)
-      .build();
-  }
-
-  @CheckForNull
-  private static String checkPassword(@Nullable String cryptedPassword, @Nullable String salt, String userPassword) {
-    if (cryptedPassword == null) {
-      return "null password in DB";
-    } else if (salt == null) {
-      return "null salt";
-    } else if (!cryptedPassword.equals(encryptPassword(userPassword, salt))) {
-      return "wrong password";
-    }
-    return null;
-  }
 }