Replace BouncyCastle's OpenBSDBCrypt use with password4j for passwor…

…d hashing and verification (opensearch-project#4381)

Signed-off-by: Dan Cecoi <[email protected]>
Co-authored-by: Dan Cecoi <[email protected]>

build.gradle

@@ -582,7 +582,7 @@ dependencies {
     implementation 'org.ldaptive:ldaptive:1.2.3'
     implementation 'com.nimbusds:nimbus-jose-jwt:9.40'
     implementation 'com.rfksystems:blake2b:2.0.0'
-
+    implementation 'com.password4j:password4j:1.8.2'
     //JWT
     implementation "io.jsonwebtoken:jjwt-api:${jjwt_version}"
     implementation "io.jsonwebtoken:jjwt-impl:${jjwt_version}"

src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java

@@ -35,6 +35,8 @@
 import org.opensearch.core.xcontent.ToXContentObject;
 import org.opensearch.security.ConfigurationFiles;
 import org.opensearch.security.dlic.rest.api.Endpoint;
+import org.opensearch.security.hasher.BCryptPasswordHasher;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.test.framework.TestSecurityConfig;
 import org.opensearch.test.framework.certificate.CertificateData;
@@ -83,6 +85,8 @@ public abstract class AbstractApiIntegrationTest extends RandomizedTest {
 
     public static LocalCluster localCluster;
 
+    public static PasswordHasher passwordHasher = new BCryptPasswordHasher();
+
     @BeforeClass
     public static void startCluster() throws IOException {
         configurationFolder = ConfigurationFiles.createConfigurationDirectory();

src/integrationTest/java/org/opensearch/security/api/AccountRestApiIntegrationTest.java

@@ -20,7 +20,6 @@
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.not;
-import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
 public class AccountRestApiIntegrationTest extends AbstractApiIntegrationTest {
 
@@ -110,7 +109,10 @@ private void verifyPasswordCanBeChanged() throws Exception {
             TEST_USER,
             TEST_USER_PASSWORD,
             client -> ok(
-                () -> client.putJson(accountPath(), changePasswordWithHashPayload(TEST_USER_PASSWORD, hash(newPassword.toCharArray())))
+                () -> client.putJson(
+                    accountPath(),
+                    changePasswordWithHashPayload(TEST_USER_PASSWORD, passwordHasher.hash(newPassword.toCharArray()))
+                )
             )
         );
         withUser(

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java

@@ -31,7 +31,6 @@
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.nio.ByteBuffer;
-import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -46,7 +45,6 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
 
 import org.opensearch.action.admin.indices.create.CreateIndexRequest;
 import org.opensearch.action.index.IndexRequest;
@@ -57,6 +55,8 @@
 import org.opensearch.core.common.bytes.BytesReference;
 import org.opensearch.core.xcontent.ToXContentObject;
 import org.opensearch.core.xcontent.XContentBuilder;
+import org.opensearch.security.hasher.BCryptPasswordHasher;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.test.framework.cluster.OpenSearchClientProvider.UserCredentialsHolder;
 
@@ -76,9 +76,10 @@
 */
 public class TestSecurityConfig {
 
-    private static final Logger log = LogManager.getLogger(TestSecurityConfig.class);
+    public static final String REST_ADMIN_REST_API_ACCESS = "rest_admin__rest_api_access";
 
-    public final static String REST_ADMIN_REST_API_ACCESS = "rest_admin__rest_api_access";
+    private static final Logger log = LogManager.getLogger(TestSecurityConfig.class);
+    private static final PasswordHasher passwordHasher = new BCryptPasswordHasher();
 
     private Config config = new Config();
     private Map<String, User> internalUsers = new LinkedHashMap<>();
@@ -967,12 +968,7 @@ public void updateInternalUsersConfiguration(Client client, List<User> users) {
     }
 
     static String hash(final char[] clearTextPassword) {
-        final byte[] salt = new byte[16];
-        new SecureRandom().nextBytes(salt);
-        final String hash = OpenBSDBCrypt.generate((Objects.requireNonNull(clearTextPassword)), salt, 12);
-        Arrays.fill(salt, (byte) 0);
-        Arrays.fill(clearTextPassword, '\0');
-        return hash;
+        return passwordHasher.hash(clearTextPassword);
     }
 
     private void writeEmptyConfigToIndex(Client client, CType configType) {

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

@@ -160,6 +160,8 @@
 import org.opensearch.security.dlic.rest.validation.PasswordValidator;
 import org.opensearch.security.filter.SecurityFilter;
 import org.opensearch.security.filter.SecurityRestFilter;
+import org.opensearch.security.hasher.BCryptPasswordHasher;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.http.NonSslHttpServerTransport;
 import org.opensearch.security.http.XFFResolver;
 import org.opensearch.security.identity.SecurityTokenManager;
@@ -262,6 +264,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
     private volatile DlsFlsRequestValve dlsFlsValve = null;
     private volatile Salt salt;
     private volatile OpensearchDynamicSetting<Boolean> transportPassiveAuthSetting;
+    private volatile PasswordHasher passwordHasher;
 
     public static boolean isActionTraceEnabled() {
 
@@ -648,7 +651,8 @@ public List<RestHandler> getRestHandlers(
                         Objects.requireNonNull(auditLog),
                         sks,
                         Objects.requireNonNull(userService),
-                        sslCertReloadEnabled
+                        sslCertReloadEnabled,
+                        passwordHasher
                     )
                 );
                 log.debug("Added {} rest handler(s)", handlers.size());
@@ -1104,7 +1108,9 @@ public Collection<Object> createComponents(
 
         cr = ConfigurationRepository.create(settings, this.configPath, threadPool, localClient, clusterService, auditLog);
 
-        userService = new UserService(cs, cr, settings, localClient);
+        this.passwordHasher = new BCryptPasswordHasher();
+
+        userService = new UserService(cs, cr, passwordHasher, settings, localClient);
 
         final XFFResolver xffResolver = new XFFResolver(threadPool);
         backendRegistry = new BackendRegistry(settings, adminDns, xffResolver, auditLog, threadPool);
@@ -1150,7 +1156,7 @@ public Collection<Object> createComponents(
             configPath,
             compatConfig
         );
-        dcf = new DynamicConfigFactory(cr, settings, configPath, localClient, threadPool, cih);
+        dcf = new DynamicConfigFactory(cr, settings, configPath, localClient, threadPool, cih, passwordHasher);
         dcf.registerDCFListener(backendRegistry);
         dcf.registerDCFListener(compatConfig);
         dcf.registerDCFListener(irr);
@@ -1200,6 +1206,8 @@ public Collection<Object> createComponents(
         components.add(si);
         components.add(dcf);
         components.add(userService);
+        components.add(passwordHasher);
+
         if (!ExternalSecurityKeyStore.hasExternalSslContext(settings)) {
             components.add(sks);
         }
@@ -1209,7 +1217,6 @@ public Collection<Object> createComponents(
             clusterService.addListener(cr);
         }
         return components;
-
     }
 
     @Override

src/main/java/org/opensearch/security/auth/internal/InternalAuthenticationBackend.java

@@ -35,11 +35,10 @@
 import java.util.Map;
 import java.util.Map.Entry;
 
-import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
-
 import org.opensearch.OpenSearchSecurityException;
 import org.opensearch.security.auth.AuthenticationBackend;
 import org.opensearch.security.auth.AuthorizationBackend;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.securityconf.InternalUsersModel;
 import org.opensearch.security.user.AuthCredentials;
 import org.opensearch.security.user.User;
@@ -48,8 +47,13 @@
 
 public class InternalAuthenticationBackend implements AuthenticationBackend, AuthorizationBackend {
 
+    private final PasswordHasher passwordHasher;
     private InternalUsersModel internalUsersModel;
 
+    public InternalAuthenticationBackend(PasswordHasher passwordHasher) {
+        this.passwordHasher = passwordHasher;
+    }
+
     @Override
     public boolean exists(User user) {
 
@@ -91,7 +95,7 @@ public boolean exists(User user) {
      * @return Whether the hash matches the provided password
      */
     public boolean passwordMatchesHash(String hash, char[] array) {
-        return OpenBSDBCrypt.checkPassword(hash, array);
+        return passwordHasher.check(array, hash);
     }
 
     @Override

src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import org.apache.commons.lang3.tuple.Triple;
-import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
 
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.settings.Settings;
@@ -32,6 +31,7 @@
 import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
 import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
 import org.opensearch.security.dlic.rest.validation.ValidationResult;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.securityconf.Hashed;
 import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
@@ -44,24 +44,28 @@
 import static org.opensearch.security.dlic.rest.api.Responses.ok;
 import static org.opensearch.security.dlic.rest.api.Responses.response;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
-import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
 /**
  * Rest API action to fetch or update account details of the signed-in user.
  * Currently this action serves GET and PUT request for /_opendistro/_security/api/account endpoint
  */
 public class AccountApiAction extends AbstractApiAction {
+
+    private final PasswordHasher passwordHasher;
+
     private static final List<Route> routes = addRoutesPrefix(
         ImmutableList.of(new Route(Method.GET, "/account"), new Route(Method.PUT, "/account"))
     );
 
     public AccountApiAction(
         final ClusterService clusterService,
         final ThreadPool threadPool,
-        final SecurityApiDependencies securityApiDependencies
+        final SecurityApiDependencies securityApiDependencies,
+        final PasswordHasher passwordHasher
     ) {
         super(Endpoint.ACCOUNT, clusterService, threadPool, securityApiDependencies);
         this.requestHandlersBuilder.configureRequestHandlers(this::accountApiRequestHandlers);
+        this.passwordHasher = passwordHasher;
     }
 
     @Override
@@ -132,7 +136,7 @@ ValidationResult<SecurityConfiguration> validCurrentPassword(final SecurityConfi
         final var currentPassword = content.get("current_password").asText();
         final var internalUserEntry = (Hashed) securityConfiguration.configuration().getCEntry(username);
         final var currentHash = internalUserEntry.getHash();
-        if (currentHash == null || !OpenBSDBCrypt.checkPassword(currentHash, currentPassword.toCharArray())) {
+        if (currentHash == null || !passwordHasher.check(currentPassword.toCharArray(), currentHash)) {
             return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Could not validate your current password."));
         }
         return ValidationResult.success(securityConfiguration);
@@ -148,7 +152,7 @@ ValidationResult<SecurityConfiguration> updatePassword(final SecurityConfigurati
         if (Strings.isNullOrEmpty(password)) {
             hash = securityJsonNode.get("hash").asString();
         } else {
-            hash = hash(password.toCharArray());
+            hash = passwordHasher.hash(password.toCharArray());
         }
         if (Strings.isNullOrEmpty(hash)) {
             return ValidationResult.error(

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -31,6 +31,7 @@
 import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
 import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
 import org.opensearch.security.dlic.rest.validation.ValidationResult;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.securityconf.Hashed;
 import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
@@ -47,10 +48,11 @@
 import static org.opensearch.security.dlic.rest.api.Responses.payload;
 import static org.opensearch.security.dlic.rest.api.Responses.response;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
-import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
 public class InternalUsersApiAction extends AbstractApiAction {
 
+    private final PasswordHasher passwordHasher;
+
     @Override
     protected void consumeParameters(final RestRequest request) {
         request.param("name");
@@ -87,11 +89,13 @@ public InternalUsersApiAction(
         final ClusterService clusterService,
         final ThreadPool threadPool,
         final UserService userService,
-        final SecurityApiDependencies securityApiDependencies
+        final SecurityApiDependencies securityApiDependencies,
+        final PasswordHasher passwordHasher
     ) {
         super(Endpoint.INTERNALUSERS, clusterService, threadPool, securityApiDependencies);
         this.userService = userService;
         this.requestHandlersBuilder.configureRequestHandlers(this::internalUsersApiRequestHandlers);
+        this.passwordHasher = passwordHasher;
     }
 
     @Override
@@ -268,7 +272,7 @@ private ValidationResult<SecurityConfiguration> generateHashForPassword(final Se
                 if (content.has("password")) {
                     final var plainTextPassword = content.get("password").asText();
                     content.remove("password");
-                    content.put("hash", hash(plainTextPassword.toCharArray()));
+                    content.put("hash", passwordHasher.hash(plainTextPassword.toCharArray()));
                 }
                 return ValidationResult.success(securityConfiguration);
             }

src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java

@@ -23,6 +23,7 @@
 import org.opensearch.security.auditlog.AuditLog;
 import org.opensearch.security.configuration.AdminDNs;
 import org.opensearch.security.configuration.ConfigurationRepository;
+import org.opensearch.security.hasher.PasswordHasher;
 import org.opensearch.security.privileges.PrivilegesEvaluator;
 import org.opensearch.security.ssl.SecurityKeyStore;
 import org.opensearch.security.ssl.transport.PrincipalExtractor;
@@ -47,7 +48,8 @@ public static Collection<RestHandler> getHandler(
         final AuditLog auditLog,
         final SecurityKeyStore securityKeyStore,
         final UserService userService,
-        final boolean certificatesReloadEnabled
+        final boolean certificatesReloadEnabled,
+        final PasswordHasher passwordHasher
     ) {
         final var securityApiDependencies = new SecurityApiDependencies(
             adminDns,
@@ -64,7 +66,7 @@ public static Collection<RestHandler> getHandler(
             settings
         );
         return List.of(
-            new InternalUsersApiAction(clusterService, threadPool, userService, securityApiDependencies),
+            new InternalUsersApiAction(clusterService, threadPool, userService, securityApiDependencies, passwordHasher),
             new RolesMappingApiAction(clusterService, threadPool, securityApiDependencies),
             new RolesApiAction(clusterService, threadPool, securityApiDependencies),
             new ActionGroupsApiAction(clusterService, threadPool, securityApiDependencies),
@@ -88,7 +90,7 @@ public static Collection<RestHandler> getHandler(
             new TenantsApiAction(clusterService, threadPool, securityApiDependencies),
             new MigrateApiAction(clusterService, threadPool, securityApiDependencies),
             new ValidateApiAction(clusterService, threadPool, securityApiDependencies),
-            new AccountApiAction(clusterService, threadPool, securityApiDependencies),
+            new AccountApiAction(clusterService, threadPool, securityApiDependencies, passwordHasher),
             new NodesDnApiAction(clusterService, threadPool, securityApiDependencies),
             new WhitelistApiAction(clusterService, threadPool, securityApiDependencies),
             // FIXME change it as soon as WhitelistApiAction will be removed

src/main/java/org/opensearch/security/dlic/rest/support/Utils.java

@@ -16,12 +16,10 @@
 import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
-import java.security.SecureRandom;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 import com.google.common.collect.ImmutableList;
@@ -31,7 +29,6 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.commons.lang3.tuple.Pair;
-import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
 
 import org.opensearch.ExceptionsHelper;
 import org.opensearch.OpenSearchParseException;
@@ -195,21 +192,6 @@ public static Map<String, Object> byteArrayToMutableJsonMap(byte[] jsonBytes) th
         }
     }
 
-    /**
-     * This generates hash for a given password
-     * @param clearTextPassword plain text password for which hash should be generated.
-     *                          This will be cleared from memory.
-     * @return hash of the password
-     */
-    public static String hash(final char[] clearTextPassword) {
-        final byte[] salt = new byte[16];
-        new SecureRandom().nextBytes(salt);
-        final String hash = OpenBSDBCrypt.generate((Objects.requireNonNull(clearTextPassword)), salt, 12);
-        Arrays.fill(salt, (byte) 0);
-        Arrays.fill(clearTextPassword, '\0');
-        return hash;
-    }
-
     /**
      * Generate field resource paths
      * @param fields fields