From 0c6792dcbed824be3ebbc01ed9a98538e6a5fb66 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Tue, 2 Jul 2024 10:31:43 +0100 Subject: [PATCH 01/13] Add support for PBKDF2 and BCrypt configuration for password hashing and verification This change introduces support for PBKDF2 as an alternative password hashing algorithm to BCrypt. Furthermore, it adds support for configuring the parameters of BCrypt (rounds, minor) & PBKDF2 (iterations, length, function). Signed-off-by: Dan Cecoi --- .../api/AbstractApiIntegrationTest.java | 8 +- .../hash/BCryptCustomConfigHashingTests.java | 93 +++++++++ .../hash/BCryptDefaultConfigHashingTests.java | 90 +++++++++ .../security/hash/HashingTests.java | 79 ++++++++ .../hash/PBKDF2DefaultConfigHashingTests.java | 86 ++++++++ .../test/framework/TestSecurityConfig.java | 27 ++- .../security/OpenSearchSecurityPlugin.java | 58 +++++- .../security/auditlog/impl/AuditMessage.java | 8 +- .../hasher/AbstractPasswordHasher.java | 33 ++++ .../security/hasher/BCryptPasswordHasher.java | 16 +- .../security/hasher/PBKDF2PasswordHasher.java | 81 ++++++++ .../hasher/PasswordHasherFactory.java | 89 +++++++++ .../security/support/ConfigConstants.java | 19 ++ .../org/opensearch/security/tools/Hasher.java | 161 +++++++++++++-- .../SecuritySettingsConfigurer.java | 10 +- .../security/UserServiceUnitTests.java | 9 +- .../org/opensearch/security/UtilTests.java | 6 +- .../auditlog/AbstractAuditlogiUnitTest.java | 4 +- .../RestApiComplianceAuditlogTest.java | 153 ++++++++++++-- .../auth/InternalAuthBackendTests.java | 12 +- .../api/AbstractApiActionValidationTest.java | 7 +- .../InternalUsersApiActionValidationTest.java | 14 +- .../hasher/AbstractPasswordHasherTests.java | 92 +++++++++ .../hasher/BCryptPasswordHasherTests.java | 90 +++------ .../hasher/PBKDF2PasswordHasherTests.java | 67 +++++++ .../hasher/PasswordHasherFactoryTests.java | 186 ++++++++++++++++++ .../security/tools/HasherTests.java | 145 ++++++++++++++ .../auditlog/internal_users_pbkdf2.yml | 46 +++++ 28 files changed, 1546 insertions(+), 143 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java create mode 100644 src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java create mode 100644 src/integrationTest/java/org/opensearch/security/hash/HashingTests.java create mode 100644 src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java create mode 100644 src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java create mode 100644 src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java create mode 100644 src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java create mode 100644 src/test/java/org/opensearch/security/hasher/AbstractPasswordHasherTests.java create mode 100644 src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java create mode 100644 src/test/java/org/opensearch/security/hasher/PasswordHasherFactoryTests.java create mode 100644 src/test/java/org/opensearch/security/tools/HasherTests.java create mode 100644 src/test/resources/auditlog/internal_users_pbkdf2.yml diff --git a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java index adf10cd098..67823e6681 100644 --- a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java @@ -30,14 +30,16 @@ import org.opensearch.common.CheckedConsumer; import org.opensearch.common.CheckedSupplier; +import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.xcontent.ToXContent; 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.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.certificate.CertificateData; import org.opensearch.test.framework.cluster.ClusterManager; @@ -85,7 +87,9 @@ public abstract class AbstractApiIntegrationTest extends RandomizedTest { public static LocalCluster localCluster; - public static PasswordHasher passwordHasher = new BCryptPasswordHasher(); + public static PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); @BeforeClass public static void startCluster() throws IOException { diff --git a/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java new file mode 100644 index 0000000000..855ba37b85 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java @@ -0,0 +1,93 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hash; + +import java.util.List; +import java.util.Map; + +import org.awaitility.Awaitility; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.apache.http.HttpStatus.*; +import static org.apache.http.HttpStatus.SC_UNAUTHORIZED; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.security.support.ConfigConstants.*; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +public class BCryptCustomConfigHashingTests extends HashingTests { + + private static LocalCluster cluster; + + private static String minor; + + private static int rounds; + + @BeforeClass + public static void startCluster() { + minor = randomFrom(List.of("A", "B", "Y")); + rounds = randomIntBetween(4, 10); + + TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS) + .hash(generateBCryptHash("secret", minor, rounds)); + cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .anonymousAuth(false) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_PASSWORD_HASHING_ALGORITHM, + BCRYPT, + SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, + minor, + SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, + rounds + ) + ) + .build(); + cluster.before(); + + try (TestRestClient client = cluster.getRestClient(ADMIN_USER.getName(), "secret")) { + Awaitility.await() + .alias("Load default configuration") + .until(() -> client.securityHealth().getTextFromJsonBody("/status"), equalTo("UP")); + } + } + + @Test + public void shouldAuthenticateWithCorrectPassword() { + String hash = generateBCryptHash(PASSWORD, minor, rounds); + createUserWithHashedPassword(cluster, "user_2", hash); + testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + + createUserWithPlainTextPassword(cluster, "user_3", PASSWORD); + testPasswordAuth(cluster, "user_3", PASSWORD, SC_OK); + } + + @Test + public void shouldNotAuthenticateWithIncorrectPassword() { + String hash = generateBCryptHash(PASSWORD, minor, rounds); + createUserWithHashedPassword(cluster, "user_4", hash); + testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + + createUserWithPlainTextPassword(cluster, "user_5", PASSWORD); + testPasswordAuth(cluster, "user_5", "wrong_password", SC_UNAUTHORIZED); + } +} diff --git a/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java new file mode 100644 index 0000000000..278de22149 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java @@ -0,0 +1,90 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hash; + +import java.security.SecureRandom; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import org.junit.ClassRule; +import org.junit.Test; +import org.bouncycastle.crypto.generators.OpenBSDBCrypt; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; + +import static org.apache.http.HttpStatus.*; +import static org.opensearch.security.support.ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT; +import static org.opensearch.security.support.ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +public class BCryptDefaultConfigHashingTests extends HashingTests { + + private static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + @ClassRule + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .anonymousAuth(false) + .nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()))) + .build(); + + @Test + public void shouldAuthenticateWhenUserCreatedWithLegacyHash() { + String hash = generateLegacyBCryptHash(PASSWORD.toCharArray()); + createUserWithHashedPassword(cluster, "user_1", hash); + testPasswordAuth(cluster, "user_1", PASSWORD, SC_OK); + } + + @Test + public void shouldAuthenticateWithCorrectPassword() { + String hash = generateBCryptHash( + PASSWORD, + SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ); + createUserWithHashedPassword(cluster, "user_2", hash); + testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + + createUserWithPlainTextPassword(cluster, "user_3", PASSWORD); + testPasswordAuth(cluster, "user_3", PASSWORD, SC_OK); + } + + @Test + public void shouldNotAuthenticateWithIncorrectPassword() { + String hash = generateBCryptHash( + PASSWORD, + SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ); + createUserWithHashedPassword(cluster, "user_4", hash); + testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + + createUserWithPlainTextPassword(cluster, "user_5", PASSWORD); + testPasswordAuth(cluster, "user_5", "wrong_password", SC_UNAUTHORIZED); + } + + private String generateLegacyBCryptHash(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; + } +} diff --git a/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java new file mode 100644 index 0000000000..9d2da3a9fd --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java @@ -0,0 +1,79 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hash; + +import java.nio.CharBuffer; + +import com.carrotsearch.randomizedtesting.RandomizedTest; +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import com.password4j.*; +import com.password4j.types.Bcrypt; + +import static org.apache.http.HttpStatus.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class HashingTests extends RandomizedTest { + + private static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + static final String PASSWORD = "top$ecret1234!"; + + public void createUserWithPlainTextPassword(LocalCluster cluster, String username, String password) { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + TestRestClient.HttpResponse httpResponse = client.putJson( + "_plugins/_security/api/internalusers/" + username, + String.format("{\"password\": \"%s\",\"opendistro_security_roles\": []}", password) + ); + assertThat(httpResponse.getStatusCode(), equalTo(SC_CREATED)); + } + } + + public void createUserWithHashedPassword(LocalCluster cluster, String username, String hashedPassword) { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + TestRestClient.HttpResponse httpResponse = client.putJson( + "_plugins/_security/api/internalusers/" + username, + String.format("{\"hash\": \"%s\",\"opendistro_security_roles\": []}", hashedPassword) + ); + assertThat(httpResponse.getStatusCode(), equalTo(SC_CREATED)); + } + } + + public void testPasswordAuth(LocalCluster cluster, String username, String password, int expectedStatusCode) { + try (TestRestClient client = cluster.getRestClient(username, password)) { + TestRestClient.HttpResponse response = client.getAuthInfo(); + response.assertStatusCode(expectedStatusCode); + } + } + + public static String generateBCryptHash(String password, String minor, int rounds) { + return Password.hash(CharBuffer.wrap(password.toCharArray())) + .with(BcryptFunction.getInstance(Bcrypt.valueOf(minor), rounds)) + .getResult(); + } + + public static String generatePBKDF2Hash(String password, String algorithm, int iterations, int length) { + return Password.hash(CharBuffer.wrap(password.toCharArray())) + .with(CompressedPBKDF2Function.getInstance(algorithm, iterations, length)) + .getResult(); + } + +} diff --git a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java new file mode 100644 index 0000000000..14a48c49b7 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java @@ -0,0 +1,86 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hash; + +import java.util.List; +import java.util.Map; + +import org.junit.ClassRule; +import org.junit.Test; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; + +import static org.apache.http.HttpStatus.SC_OK; +import static org.apache.http.HttpStatus.SC_UNAUTHORIZED; +import static org.opensearch.security.support.ConfigConstants.*; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +public class PBKDF2DefaultConfigHashingTests extends HashingTests { + + private static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS) + .hash( + generatePBKDF2Hash( + "secret", + SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ) + ); + + @ClassRule + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .anonymousAuth(false) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_PASSWORD_HASHING_ALGORITHM, + PBKDF2 + ) + ) + .build(); + + @Test + public void shouldAuthenticateWithCorrectPassword() { + String hash = generatePBKDF2Hash( + PASSWORD, + SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ); + createUserWithHashedPassword(cluster, "user_1", hash); + testPasswordAuth(cluster, "user_1", PASSWORD, SC_OK); + + createUserWithPlainTextPassword(cluster, "user_2", PASSWORD); + testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + } + + @Test + public void shouldNotAuthenticateWithIncorrectPassword() { + String hash = generatePBKDF2Hash( + PASSWORD, + SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ); + createUserWithHashedPassword(cluster, "user_3", hash); + testPasswordAuth(cluster, "user_3", "wrong_password", SC_UNAUTHORIZED); + + createUserWithPlainTextPassword(cluster, "user_4", PASSWORD); + testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + } +} diff --git a/src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java b/src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java index 0c332c54d2..eaabbbcbf6 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java +++ b/src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java @@ -50,14 +50,16 @@ import org.opensearch.action.index.IndexRequest; import org.opensearch.action.update.UpdateRequest; import org.opensearch.client.Client; +import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.Strings; 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.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.cluster.OpenSearchClientProvider.UserCredentialsHolder; import static org.apache.http.HttpHeaders.AUTHORIZATION; @@ -79,7 +81,10 @@ public class TestSecurityConfig { public static final 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 static final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); private Config config = new Config(); private Map internalUsers = new LinkedHashMap<>(); @@ -413,6 +418,8 @@ public static final class User implements UserCredentialsHolder, ToXContentObjec private String description; + private String hash; + public User(String name) { this(name, null); } @@ -457,6 +464,11 @@ public User attr(String key, String value) { return this; } + public User hash(String hash) { + this.hash = hash; + return this; + } + public String getName() { return name; } @@ -477,8 +489,11 @@ public Object getAttribute(String attributeName) { public XContentBuilder toXContent(XContentBuilder xContentBuilder, Params params) throws IOException { xContentBuilder.startObject(); - xContentBuilder.field("hash", hash(password.toCharArray())); - + if (this.hash == null) { + xContentBuilder.field("hash", hashPassword(password)); + } else { + xContentBuilder.field("hash", hash); + } Set roleNames = getRoleNames(); if (!roleNames.isEmpty()) { @@ -967,8 +982,8 @@ public void updateInternalUsersConfiguration(Client client, List users) { updateConfigInIndex(client, CType.INTERNALUSERS, userMap); } - static String hash(final char[] clearTextPassword) { - return passwordHasher.hash(clearTextPassword); + static String hashPassword(final String clearTextPassword) { + return passwordHasher.hash(clearTextPassword.toCharArray()); } private void writeEmptyConfigToIndex(Client client, CType configType) { diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index c3e00da109..53c4bfa560 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -160,8 +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.hasher.PasswordHasherFactory; import org.opensearch.security.http.NonSslHttpServerTransport; import org.opensearch.security.http.XFFResolver; import org.opensearch.security.identity.SecurityTokenManager; @@ -1108,7 +1108,7 @@ public Collection createComponents( cr = ConfigurationRepository.create(settings, this.configPath, threadPool, localClient, clusterService, auditLog); - this.passwordHasher = new BCryptPasswordHasher(); + this.passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); userService = new UserService(cs, cr, passwordHasher, settings, localClient); @@ -1308,6 +1308,60 @@ public List> getSettings() { ) ); + settings.add( + Setting.simpleString( + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + + settings.add( + Setting.intSetting( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + + settings.add( + Setting.simpleString( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + + settings.add( + Setting.intSetting( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + + settings.add( + Setting.intSetting( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + + settings.add( + Setting.simpleString( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + Property.NodeScope, + Property.Final + ) + ); + if (!SSLConfig.isSslOnlyMode()) { settings.add( Setting.listSetting( diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java index bf3395f193..81695b702b 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -73,8 +73,10 @@ public final class AuditMessage { ); @VisibleForTesting - public static final Pattern BCRYPT_HASH = Pattern.compile("\\$2[ayb]\\$.{56}"); - private static final String BCRYPT_HASH_REPLACEMENT_VALUE = "__HASH__"; + public static final String BCRYPT_REGEX = "\\$2[ayb]\\$.{56}"; + public static final String PBKDF2_REGEX = "\\$\\d+\\$\\d+\\$[A-Za-z0-9+/]+={0,2}\\$[A-Za-z0-9+/]+={0,2}"; + public static final Pattern HASH_REGEX_PATTERN = Pattern.compile(BCRYPT_REGEX + "|" + PBKDF2_REGEX); + private static final String HASH_REPLACEMENT_VALUE = "__HASH__"; private static final String INTERNALUSERS_DOC_ID = CType.INTERNALUSERS.toLCString(); public static final String FORMAT_VERSION = "audit_format_version"; @@ -237,7 +239,7 @@ public void addUnescapedJsonToRequestBody(String source) { private String redactSecurityConfigContent(String content, String id) { if (content != null && INTERNALUSERS_DOC_ID.equals(id)) { - content = BCRYPT_HASH.matcher(content).replaceAll(BCRYPT_HASH_REPLACEMENT_VALUE); + content = HASH_REGEX_PATTERN.matcher(content).replaceAll(HASH_REPLACEMENT_VALUE); } return content; } diff --git a/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java b/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java new file mode 100644 index 0000000000..48e5d01ff9 --- /dev/null +++ b/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java @@ -0,0 +1,33 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import java.nio.CharBuffer; +import java.util.Arrays; + +import com.password4j.HashingFunction; + +abstract class AbstractPasswordHasher implements PasswordHasher { + + HashingFunction hashingFunction; + + public abstract String hash(char[] password); + + public abstract boolean check(char[] password, String hash); + + protected void cleanup(CharBuffer password) { + password.clear(); + char[] passwordOverwrite = new char[password.capacity()]; + Arrays.fill(passwordOverwrite, '\0'); + password.put(passwordOverwrite); + } +} diff --git a/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java b/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java index 043c0392d9..eb419bdf65 100644 --- a/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java @@ -14,7 +14,6 @@ import java.nio.CharBuffer; import java.security.AccessController; import java.security.PrivilegedAction; -import java.util.Arrays; import org.opensearch.OpenSearchSecurityException; import org.opensearch.SpecialPermission; @@ -26,9 +25,11 @@ import static org.opensearch.core.common.Strings.isNullOrEmpty; -public class BCryptPasswordHasher implements PasswordHasher { +class BCryptPasswordHasher extends AbstractPasswordHasher { - private static final HashingFunction DEFAULT_BCRYPT_FUNCTION = BcryptFunction.getInstance(Bcrypt.Y, 12); + BCryptPasswordHasher(String minor, int logRounds) { + this.hashingFunction = BcryptFunction.getInstance(Bcrypt.valueOf(minor), logRounds); + } @Override public String hash(char[] password) { @@ -42,7 +43,7 @@ public String hash(char[] password) { securityManager.checkPermission(new SpecialPermission()); } return AccessController.doPrivileged( - (PrivilegedAction) () -> Password.hash(passwordBuffer).with(DEFAULT_BCRYPT_FUNCTION).getResult() + (PrivilegedAction) () -> Password.hash(passwordBuffer).with(hashingFunction).getResult() ); } finally { cleanup(passwordBuffer); @@ -71,13 +72,6 @@ public boolean check(char[] password, String hash) { } } - private void cleanup(CharBuffer password) { - password.clear(); - char[] passwordOverwrite = new char[password.capacity()]; - Arrays.fill(passwordOverwrite, '\0'); - password.put(passwordOverwrite); - } - private HashingFunction getBCryptFunctionFromHash(String hash) { return BcryptFunction.getInstanceFromHash(hash); } diff --git a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java new file mode 100644 index 0000000000..5c466c5e05 --- /dev/null +++ b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java @@ -0,0 +1,81 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import java.nio.CharBuffer; +import java.security.AccessController; +import java.security.PrivilegedAction; + +import org.opensearch.OpenSearchSecurityException; +import org.opensearch.SpecialPermission; + +import com.password4j.*; + +import static org.opensearch.core.common.Strings.isNullOrEmpty; + +class PBKDF2PasswordHasher extends AbstractPasswordHasher { + + PBKDF2PasswordHasher(String function, int iterations, int length) { + SecurityManager securityManager = System.getSecurityManager(); + if (securityManager != null) { + securityManager.checkPermission(new SpecialPermission()); + } + this.hashingFunction = AccessController.doPrivileged( + (PrivilegedAction) () -> CompressedPBKDF2Function.getInstance(function, iterations, length) + ); + } + + @Override + public String hash(char[] password) { + if (password == null || password.length == 0) { + throw new OpenSearchSecurityException("Password cannot be empty or null"); + } + CharBuffer passwordBuffer = CharBuffer.wrap(password); + try { + SecurityManager securityManager = System.getSecurityManager(); + if (securityManager != null) { + securityManager.checkPermission(new SpecialPermission()); + } + return AccessController.doPrivileged( + (PrivilegedAction) () -> Password.hash(passwordBuffer).addRandomSalt(64).with(hashingFunction).getResult() + ); + } finally { + cleanup(passwordBuffer); + } + } + + @Override + public boolean check(char[] password, String hash) { + if (password == null || password.length == 0) { + throw new OpenSearchSecurityException("Password cannot be empty or null"); + } + if (isNullOrEmpty(hash)) { + throw new OpenSearchSecurityException("Hash cannot be empty or null"); + } + CharBuffer passwordBuffer = CharBuffer.wrap(password); + try { + SecurityManager securityManager = System.getSecurityManager(); + if (securityManager != null) { + securityManager.checkPermission(new SpecialPermission()); + } + return AccessController.doPrivileged( + (PrivilegedAction) () -> Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash)) + ); + } finally { + cleanup(passwordBuffer); + } + } + + private HashingFunction getPBKDF2FunctionFromHash(String hash) { + return CompressedPBKDF2Function.getInstanceFromHash(hash); + } +} diff --git a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java new file mode 100644 index 0000000000..571d743f45 --- /dev/null +++ b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java @@ -0,0 +1,89 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import org.opensearch.common.settings.Settings; +import org.opensearch.security.support.ConfigConstants; + +import static org.opensearch.security.support.ConfigConstants.BCRYPT; +import static org.opensearch.security.support.ConfigConstants.PBKDF2; + +public class PasswordHasherFactory { + + public static PasswordHasher createPasswordHasher(Settings settings) { + + String algorithm = settings.get( + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT + ); + + PasswordHasher passwordHasher; + switch (algorithm.toLowerCase()) { + case BCRYPT: + passwordHasher = getBCryptHasher(settings); + break; + case PBKDF2: + passwordHasher = getPBKDF2Hasher(settings); + break; + default: + throw new IllegalArgumentException("Password hashing algorithm not supported"); + } + return passwordHasher; + } + + private static PasswordHasher getBCryptHasher(Settings settings) { + int rounds = settings.getAsInt( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ); + String minor = settings.get( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT + ).toUpperCase(); + + if (rounds < 4 || rounds > 31) { + throw new IllegalArgumentException("BCrypt rounds must be between 4 and 31."); + } + if (!minor.equals("A") && !minor.equals("B") && !minor.equals("Y")) { + throw new IllegalArgumentException("BCrypt minor must be 'a', 'b', or 'y'."); + } + return new BCryptPasswordHasher(minor, rounds); + } + + private static PasswordHasher getPBKDF2Hasher(Settings settings) { + String pbkdf2Function = settings.get( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT + ).toUpperCase(); + + int iterations = settings.getAsInt( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT + ); + int length = settings.getAsInt( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ); + + // todo: validate validation + if (!pbkdf2Function.matches("SHA(1|224|256|384|512)")) { + throw new IllegalArgumentException("PBKDF2 function must be one of SHA1, SHA224, SHA256, SHA384, or SHA512."); + } + if (iterations <= 0) { + throw new IllegalArgumentException("PBKDF2 iterations must be a positive integer."); + } + if (length <= 0) { + throw new IllegalArgumentException("PBKDF2 length must be a positive integer."); + } + return new PBKDF2PasswordHasher(pbkdf2Function, iterations, length); + } +} diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index a17678ea80..bd72b943ff 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -38,6 +38,8 @@ import org.opensearch.common.settings.Settings; import org.opensearch.security.auditlog.impl.AuditCategory; +import com.password4j.types.Hmac; + public class ConfigConstants { public static final String OPENDISTRO_SECURITY_CONFIG_PREFIX = "_opendistro_security_"; @@ -144,6 +146,23 @@ public class ConfigConstants { public static final String SECURITY_AUTHCZ_IMPERSONATION_DN = "plugins.security.authcz.impersonation_dn"; public static final String SECURITY_AUTHCZ_REST_IMPERSONATION_USERS = "plugins.security.authcz.rest_impersonation_user"; + public static final String BCRYPT = "bcrypt"; + public static final String PBKDF2 = "pbkdf2"; + + public static final String SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS = "plugins.security.password.hashing.bcrypt.rounds"; + public static final int SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT = 12; + public static final String SECURITY_PASSWORD_HASHING_BCRYPT_MINOR = "plugins.security.password.hashing.bcrypt.minor"; + public static final String SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT = "B"; + + public static final String SECURITY_PASSWORD_HASHING_ALGORITHM = "plugins.security.password.hashing.algorithm"; + public static final String SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT = BCRYPT; + public static final String SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS = "plugins.security.password.hashing.pbkdf2.iterations"; + public static final int SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT = 310000; + public static final String SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH = "plugins.security.password.hashing.pbkdf2.length"; + public static final int SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT = 512; + public static final String SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION = "plugins.security.password.hashing.pbkdf2.function"; + public static final String SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT = Hmac.SHA256.name(); + public static final String SECURITY_AUDIT_TYPE_DEFAULT = "plugins.security.audit.type"; public static final String SECURITY_AUDIT_CONFIG_DEFAULT = "plugins.security.audit.config"; public static final String SECURITY_AUDIT_CONFIG_ROUTES = "plugins.security.audit.routes"; diff --git a/src/main/java/org/opensearch/security/tools/Hasher.java b/src/main/java/org/opensearch/security/tools/Hasher.java index 76a4fec5c6..77fea7c28a 100644 --- a/src/main/java/org/opensearch/security/tools/Hasher.java +++ b/src/main/java/org/opensearch/security/tools/Hasher.java @@ -34,45 +34,65 @@ import org.apache.commons.cli.HelpFormatter; import org.apache.commons.cli.Option; import org.apache.commons.cli.Options; +import org.apache.commons.cli.ParseException; -import org.opensearch.security.hasher.BCryptPasswordHasher; +import org.opensearch.common.settings.Settings; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; +import org.opensearch.security.support.ConfigConstants; public class Hasher { - public static void main(final String[] args) { + private static final String PASSWORD_OPTION = "p"; + private static final String ENV_OPTION = "env"; + private static final String ALGORITHM_OPTION = "a"; + private static final String ROUNDS_OPTION = "r"; + private static final String FUNCTION_OPTION = "f"; + private static final String LENGTH_OPTION = "l"; + private static final String ITERATIONS_OPTION = "i"; + private static final String MINOR_OPTION = "min"; - final Options options = new Options(); + public static void main(final String[] args) { final HelpFormatter formatter = new HelpFormatter(); - options.addOption(Option.builder("p").argName("password").hasArg().desc("Cleartext password to hash").build()); - options.addOption( - Option.builder("env") - .argName("name environment variable") - .hasArg() - .desc("name environment variable to read password from") - .build() - ); - + Options options = buildOptions(); final CommandLineParser parser = new DefaultParser(); try { final CommandLine line = parser.parse(options, args); + final char[] password; - if (line.hasOption("p")) { - System.out.println(hash(line.getOptionValue("p").toCharArray())); - } else if (line.hasOption("env")) { - final String pwd = System.getenv(line.getOptionValue("env")); + if (line.hasOption(PASSWORD_OPTION)) { + password = line.getOptionValue(PASSWORD_OPTION).toCharArray(); + } else if (line.hasOption(ENV_OPTION)) { + final String pwd = System.getenv(line.getOptionValue(ENV_OPTION)); if (pwd == null || pwd.isEmpty()) { - throw new Exception("No environment variable '" + line.getOptionValue("env") + "' set"); + throw new Exception("No environment variable '" + line.getOptionValue(ENV_OPTION) + "' set"); } - System.out.println(hash(pwd.toCharArray())); + password = pwd.toCharArray(); } else { final Console console = System.console(); if (console == null) { throw new Exception("Cannot allocate a console"); } - final char[] passwd = console.readPassword("[%s]", "Password:"); - System.out.println(hash(passwd)); + password = console.readPassword("[%s]", "Password:"); } + if (line.hasOption(ALGORITHM_OPTION)) { + String algorithm = line.getOptionValue(ALGORITHM_OPTION); + Settings settings; + switch (algorithm.toLowerCase()) { + case ConfigConstants.BCRYPT: + settings = getBCryptSettings(line); + break; + case ConfigConstants.PBKDF2: + settings = getPBKDF2Settings(line); + break; + default: + throw new Exception("Unsupported hashing algorithm: " + algorithm); + } + System.out.println(hash(password, settings)); + } else { + System.out.println(hash(password)); + } + } catch (final Exception exp) { System.err.println("Parsing failed. Reason: " + exp.getMessage()); formatter.printHelp("hash.sh", options, true); @@ -81,7 +101,106 @@ public static void main(final String[] args) { } public static String hash(final char[] clearTextPassword) { - PasswordHasher passwordHasher = new BCryptPasswordHasher(); + return hash(clearTextPassword, Settings.EMPTY); + } + + public static String hash(final char[] clearTextPassword, Settings settings) { + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); return passwordHasher.hash(clearTextPassword); } + + private static Settings getBCryptSettings(CommandLine line) throws ParseException { + Settings.Builder settings = Settings.builder(); + settings.put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT); + if (line.hasOption(ROUNDS_OPTION)) { + settings.put( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, + ((Number) line.getParsedOptionValue(ROUNDS_OPTION)).intValue() + ); + } + if (line.hasOption(MINOR_OPTION)) { + settings.put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, line.getOptionValue(MINOR_OPTION).toUpperCase()); + } + return settings.build(); + } + + private static Settings getPBKDF2Settings(CommandLine line) throws ParseException { + Settings.Builder settings = Settings.builder(); + settings.put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2); + if (line.hasOption(FUNCTION_OPTION)) { + settings.put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, line.getOptionValue(FUNCTION_OPTION)); + } + if (line.hasOption(LENGTH_OPTION)) { + settings.put( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, + ((Number) line.getParsedOptionValue(LENGTH_OPTION)).intValue() + ); + } + if (line.hasOption(ITERATIONS_OPTION)) { + settings.put( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, + ((Number) line.getParsedOptionValue(ITERATIONS_OPTION)).intValue() + ); + } + return settings.build(); + } + + private static Options buildOptions() { + final Options options = new Options(); + options.addOption(Option.builder(PASSWORD_OPTION).argName("password").hasArg().desc("Cleartext password to hash").build()); + options.addOption( + Option.builder(ENV_OPTION) + .argName("name environment variable") + .hasArg() + .desc("name environment variable to read password from") + .build() + ); + options.addOption( + Option.builder(ALGORITHM_OPTION) + .longOpt("algorithm") + .argName("hashing algorithm") + .hasArg() + .desc("Hashing algorithm (BCrypt, PBKDF2, SCrypt, Argon2)") + .build() + ); + options.addOption( + Option.builder(ROUNDS_OPTION) + .longOpt("rounds") + .desc("Number of rounds (for BCrypt).") + .hasArg() + .argName("rounds") + .type(Number.class) + .build() + ); + options.addOption( + Option.builder(MINOR_OPTION).longOpt("minor").desc("Minor version (for BCrypt).").hasArg().argName("minor").build() + ); + options.addOption( + Option.builder(LENGTH_OPTION) + .longOpt("length") + .desc("Desired length of the final derived key (for Argon2, PBKDF2).") + .hasArg() + .argName("length") + .type(Number.class) + .build() + ); + options.addOption( + Option.builder(ITERATIONS_OPTION) + .longOpt("iterations") + .desc("Iterations to perform (for Argon2, PBKDF2).") + .hasArg() + .argName("iterations") + .type(Number.class) + .build() + ); + options.addOption( + Option.builder(FUNCTION_OPTION) + .longOpt("function") + .desc("Pseudo-random function applied to the password (for PBKDF2).") + .hasArg() + .argName("function") + .build() + ); + return options; + } } diff --git a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java index af509def1a..709d882a3e 100644 --- a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java +++ b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java @@ -29,16 +29,15 @@ import org.opensearch.core.common.Strings; import org.opensearch.security.dlic.rest.validation.PasswordValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; -import org.opensearch.security.hasher.BCryptPasswordHasher; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.support.ConfigConstants; import org.yaml.snakeyaml.DumperOptions; import org.yaml.snakeyaml.Yaml; import static org.opensearch.security.DefaultObjectMapper.YAML_MAPPER; -import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_MIN_LENGTH; -import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX; +import static org.opensearch.security.support.ConfigConstants.*; /** * This class updates the security related configuration, as needed. @@ -88,7 +87,9 @@ public class SecuritySettingsConfigurer { public SecuritySettingsConfigurer(Installer installer) { this.installer = installer; - this.passwordHasher = new BCryptPasswordHasher(); + this.passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, PBKDF2).build() + ); } /** @@ -201,7 +202,6 @@ void updateAdminPassword() throws IOException { */ private boolean isAdminPasswordSetToAdmin(String internalUsersFile) throws IOException { JsonNode internalUsers = YAML_MAPPER.readTree(new FileInputStream(internalUsersFile)); - PasswordHasher passwordHasher = new BCryptPasswordHasher(); return internalUsers.has("admin") && passwordHasher.check(DEFAULT_ADMIN_PASSWORD.toCharArray(), internalUsers.get("admin").get("hash").asText()); } diff --git a/src/test/java/org/opensearch/security/UserServiceUnitTests.java b/src/test/java/org/opensearch/security/UserServiceUnitTests.java index ccd3c9848e..ef13d4d934 100644 --- a/src/test/java/org/opensearch/security/UserServiceUnitTests.java +++ b/src/test/java/org/opensearch/security/UserServiceUnitTests.java @@ -26,10 +26,11 @@ import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.hasher.BCryptPasswordHasher; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.UserFilterType; import org.opensearch.security.user.UserService; @@ -53,9 +54,9 @@ public class UserServiceUnitTests { @Before public void setup() throws Exception { String usersYmlFile = "./internal_users.yml"; - Settings.Builder builder = Settings.builder(); - PasswordHasher passwordHasher = new BCryptPasswordHasher(); - userService = new UserService(clusterService, configurationRepository, passwordHasher, builder.build(), client); + Settings settings = Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + userService = new UserService(clusterService, configurationRepository, passwordHasher, settings, client); config = readConfigFromYml(usersYmlFile, CType.INTERNALUSERS); } diff --git a/src/test/java/org/opensearch/security/UtilTests.java b/src/test/java/org/opensearch/security/UtilTests.java index e545cde86f..1b71ba61ca 100644 --- a/src/test/java/org/opensearch/security/UtilTests.java +++ b/src/test/java/org/opensearch/security/UtilTests.java @@ -31,8 +31,8 @@ import org.junit.Test; import org.opensearch.common.settings.Settings; -import org.opensearch.security.hasher.BCryptPasswordHasher; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.SecurityUtils; import org.opensearch.security.support.WildcardMatcher; @@ -51,7 +51,9 @@ static private WildcardMatcher iwc(String pattern) { return WildcardMatcher.from(pattern, false); } - static private final PasswordHasher passwordHasher = new BCryptPasswordHasher(); + static private final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); @Test public void testWildcardMatcherClasses() { diff --git a/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java b/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java index b671378ad4..ce84e5f14b 100644 --- a/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java +++ b/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java @@ -38,7 +38,7 @@ protected String getResourceFolder() { return "auditlog"; } - protected final void setup(Settings settings) throws Exception { + protected final void setup(Settings settings, DynamicSecurityConfig securityConfig) throws Exception { final Settings.Builder auditConfigSettings = Settings.builder(); final Settings.Builder defaultNodeSettings = Settings.builder(); // Separate the cluster defaults from audit settings that will be applied after the cluster is up @@ -56,7 +56,7 @@ protected final void setup(Settings settings) throws Exception { }); final Settings nodeSettings = defaultNodeSettings(defaultNodeSettings.build()); - setup(Settings.EMPTY, new DynamicSecurityConfig(), nodeSettings, init); + setup(Settings.EMPTY, securityConfig, nodeSettings, init); rh = restHelper(); updateAuditConfig(auditConfigSettings.build()); } diff --git a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java index 784176e1dd..c3ab0799a7 100644 --- a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java @@ -1,13 +1,13 @@ /* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ +* SPDX-License-Identifier: Apache-2.0 +* +* The OpenSearch Contributors require contributions made to +* this file be licensed under the Apache-2.0 license or a +* compatible open source license. +* +* Modifications Copyright OpenSearch Contributors. See +* GitHub history for details. +*/ package org.opensearch.security.auditlog.compliance; @@ -24,6 +24,7 @@ import org.opensearch.security.auditlog.impl.AuditMessage; import org.opensearch.security.auditlog.integration.TestAuditlogImpl; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.test.DynamicSecurityConfig; import org.opensearch.security.test.helper.cluster.ClusterConfiguration; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; @@ -35,7 +36,6 @@ public class RestApiComplianceAuditlogTest extends AbstractAuditlogiUnitTest { @Test public void testRestApiRolesEnabled() throws Exception { - Settings additionalSettings = Settings.builder() .put("plugins.security.audit.type", TestAuditlogImpl.class.getName()) .put(ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, "opendistro_security_all_access") @@ -225,25 +225,147 @@ public void testBCryptHashRedaction() throws Exception { rh.executeGetRequest("/_opendistro/_security/api/internalusers"); }); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message1.toString()).matches()); + Assert.assertFalse(AuditMessage.HASH_REGEX_PATTERN.matcher(message1.toString()).matches()); // read internal user worf and verify no BCrypt hash is present in audit logs final AuditMessage message2 = TestAuditlogImpl.doThenWaitForMessage(() -> { rh.executeGetRequest("/_opendistro/_security/api/internalusers/worf"); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(TestAuditlogImpl.sb.toString()).matches()); + Assert.assertFalse(AuditMessage.HASH_REGEX_PATTERN.matcher(TestAuditlogImpl.sb.toString()).matches()); }); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message2.toString()).matches()); + Assert.assertFalse(AuditMessage.HASH_REGEX_PATTERN.matcher(message2.toString()).matches()); // create internal user and verify no BCrypt hash is present in audit logs final AuditMessage message3 = TestAuditlogImpl.doThenWaitForMessage(() -> { rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"password\":\"some new user password\"}"); }); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message3.toString()).matches()); + Assert.assertFalse(AuditMessage.HASH_REGEX_PATTERN.matcher(message3.toString()).matches()); + } + + @Test + public void testPBKDF2HashRedaction() { + final Settings settings = Settings.builder() + .put("plugins.security.audit.type", TestAuditlogImpl.class.getName()) + .put(ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, "opendistro_security_all_access") + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_ENABLE_REST, false) + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_ENABLE_TRANSPORT, false) + .put(ConfigConstants.SECURITY_COMPLIANCE_HISTORY_INTERNAL_CONFIG_ENABLED, true) + .put(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_LOG_DIFFS, true) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .build(); + final DynamicSecurityConfig securityConfig = new DynamicSecurityConfig().setSecurityInternalUsers("internal_users_pbkdf2.yml"); + setupAndReturnAuditMessages(settings, securityConfig); + rh.sendAdminCertificate = true; + rh.keystore = "kirk-keystore.jks"; + + // read internal users and verify no BCrypt hash is present in audit logs + final AuditMessage message1 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers"); + }); + + Assert.assertFalse( + message1.toString() + .contains( + "$3$1331439861760512$wBFrJJIAokWuJxlO6BQPLashXgznvR4tRmXk3aEy9SpHWrb9kFjPPLByZZzMLBNQFjhepgbngYh7RfMh8TrPLw==$vqGlzGsxqGf9TgfxhORjdoqRFB3npvBd9B0GAtBMg9mD2zBbSTohRYlOxUL7UQLma66zZdD67c4RNE9BKelabw==" + ) + ); + Assert.assertTrue(message1.toString().contains("__HASH__")); + + // read internal user worf and verify no BCrypt hash is present in audit logs + final AuditMessage message2 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers/user1"); + }); + + Assert.assertFalse( + message2.toString() + .contains( + "$3$1331439861760512$wBFrJJIAokWuJxlO6BQPLashXgznvR4tRmXk3aEy9SpHWrb9kFjPPLByZZzMLBNQFjhepgbngYh7RfMh8TrPLw==$vqGlzGsxqGf9TgfxhORjdoqRFB3npvBd9B0GAtBMg9mD2zBbSTohRYlOxUL7UQLma66zZdD67c4RNE9BKelabw==" + ) + ); + Assert.assertTrue(message2.toString().contains("__HASH__")); + + // create internal user and verify no PBKDF2 hash is present in audit logs + final AuditMessage message3 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"password\":\"some new user password\"}"); + }); + + Assert.assertFalse( + message3.toString() + .contains( + "$3$1331439861760512$wBFrJJIAokWuJxlO6BQPLashXgznvR4tRmXk3aEy9SpHWrb9kFjPPLByZZzMLBNQFjhepgbngYh7RfMh8TrPLw==$vqGlzGsxqGf9TgfxhORjdoqRFB3npvBd9B0GAtBMg9mD2zBbSTohRYlOxUL7UQLma66zZdD67c4RNE9BKelabw==" + ) + ); + Assert.assertTrue(message3.toString().contains("__HASH__")); + + // test with various users and different PBKDF2 hash formats to make sure they all get redacted + final AuditMessage message4 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers", encodeBasicHeader("user1", "user1")); + }); + + Assert.assertFalse( + message4.toString() + .contains( + "$1$4294967296128$VmnDMbQ4wLiFUq178RKvj+EYfdb3Q26qCiDcJDoCxpYnKpyuG0JhTC2wpUkMUveV5RmBFzldKQkdqZEfE0XAgg==$9u3aMWc6VP2oGkXei7UaXA==" + ) + ); + Assert.assertTrue(message4.toString().contains("__HASH__")); + + final AuditMessage message5 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers", encodeBasicHeader("user2", "user2")); + }); + + Assert.assertFalse( + message5.toString() + .contains( + "$2$214748364800224$eQgqv2RI6yo95yeVnM5sfwUCwxHo6re0w+wpx6ZqZtHQV+dzlyP6YFitjNG2mlaTkg0pR56xArQaAgapdVcBQQ==$tGHWhoc83cd5nZ7QIZKPORjW/N5jklhYhRgXpw==" + ) + ); + Assert.assertTrue(message5.toString().contains("__HASH__")); + + final AuditMessage message6 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers", encodeBasicHeader("user3", "user3")); + }); + + Assert.assertFalse( + message6.toString() + .contains( + "$3$322122547200256$5b3wEAsMc05EZFxfncCUfZRERgvbwlBhYXd5vVR14kNJtmhXSpYMzydRZxO9096IPTkc47doH4hIdKX6LTguLg==$oQQvAtUyOC6cwdAYi5WeIM7rGUN9l3IdJ9y2RNxZCWo=" + ) + ); + Assert.assertTrue(message6.toString().contains("__HASH__")); + + final AuditMessage message7 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers", encodeBasicHeader("user4", "user4")); + }); + + Assert.assertFalse( + message7.toString() + .contains( + "$4$429496729600384$+SNSgbZD67a1bd92iuEiHCq5pvvrCx3HrNIf5hbGIJdxgXegpWilpB6vUGvYigegAUzZqE9iIsL4pSJztUNJYw==$lTxZ7tax6dBQ0r4qPJpc8d7YuoTBiUujY9HJeAZvARXMjIgvnJwa6FeYugttOKc0" + ) + ); + Assert.assertTrue(message7.toString().contains("__HASH__")); + + final AuditMessage message8 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers", encodeBasicHeader("user5", "user5")); + }); + + Assert.assertFalse( + message8.toString() + .contains( + "$5$644245094400512$HQe/MOv/NAlgodNhqTmjqj5jGxBwG5xuRaxKwn7r4nlUba1kj/CYnpdFaXGvVeRxt2NLm8fbekS6NYonv358Ew==$1sDx+0tMbtGzU6jlQg/Dyt30Yxuy5RdNmP9B1EzMTxYWi8k1xg2gXLy7w1XbetEC8UD/lpyXJPlaoxXpsaADyA==" + ) + ); + Assert.assertTrue(message8.toString().contains("__HASH__")); + } private List setupAndReturnAuditMessages(Settings settings) { + return setupAndReturnAuditMessages(settings, new DynamicSecurityConfig()); + } + + private List setupAndReturnAuditMessages(Settings settings, DynamicSecurityConfig dynamicSecurityConfig) { // When OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED is set to true, there is: // - 1 message with COMPLIANCE_INTERNAL_CONFIG_WRITE as category. // - 1 message with COMPLIANCE_EXTERNAL_CONFIG as category for each node. @@ -255,7 +377,7 @@ private List setupAndReturnAuditMessages(Settings settings) { int expectedMessageCount = externalConfigEnabled ? (numNodes + 1) : 1; final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { try { - setup(settings); + setup(settings, dynamicSecurityConfig); } catch (final Exception ex) { throw new RuntimeException(ex); } @@ -275,4 +397,5 @@ private List setupAndReturnAuditMessages(Settings settings) { } return messages; } + } diff --git a/src/test/java/org/opensearch/security/auth/InternalAuthBackendTests.java b/src/test/java/org/opensearch/security/auth/InternalAuthBackendTests.java index 3832f866cb..3c4e55ca16 100644 --- a/src/test/java/org/opensearch/security/auth/InternalAuthBackendTests.java +++ b/src/test/java/org/opensearch/security/auth/InternalAuthBackendTests.java @@ -21,9 +21,11 @@ import org.junit.Test; import org.opensearch.OpenSearchSecurityException; +import org.opensearch.common.settings.Settings; import org.opensearch.security.auth.internal.InternalAuthenticationBackend; -import org.opensearch.security.hasher.BCryptPasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.InternalUsersModel; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.AuthCredentials; import org.mockito.Mockito; @@ -43,7 +45,13 @@ public class InternalAuthBackendTests { @Before public void internalAuthBackendTestsSetup() { - internalAuthenticationBackend = spy(new InternalAuthenticationBackend(new BCryptPasswordHasher())); + internalAuthenticationBackend = spy( + new InternalAuthenticationBackend( + PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ) + ) + ); internalUsersModel = mock(InternalUsersModel.class); internalAuthenticationBackend.onInternalUsersModelChanged(internalUsersModel); } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java index e6c4bb17d0..4e7dc1c6cc 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java @@ -28,10 +28,11 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.hasher.BCryptPasswordHasher; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.threadpool.ThreadPool; import org.mockito.Mock; @@ -78,7 +79,9 @@ public void setup() { Settings.EMPTY ); - passwordHasher = new BCryptPasswordHasher(); + passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); } void setupRolesConfiguration() throws IOException { diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java index 8e6b5a0bb2..f9c24efa8b 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java @@ -18,15 +18,17 @@ import org.junit.Before; import org.junit.Test; +import org.opensearch.common.settings.Settings; import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.dlic.rest.validation.ValidationResult; -import org.opensearch.security.hasher.BCryptPasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.securityconf.impl.v7.InternalUserV7; import org.opensearch.security.securityconf.impl.v7.RoleV7; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.UserService; import org.opensearch.security.util.FakeRestRequest; @@ -193,7 +195,15 @@ public void validateSecurityRolesWithImmutableRolesMappingConfig() throws Except } private InternalUsersApiAction createInternalUsersApiAction() { - return new InternalUsersApiAction(clusterService, threadPool, userService, securityApiDependencies, new BCryptPasswordHasher()); + return new InternalUsersApiAction( + clusterService, + threadPool, + userService, + securityApiDependencies, + PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ) + ); } } diff --git a/src/test/java/org/opensearch/security/hasher/AbstractPasswordHasherTests.java b/src/test/java/org/opensearch/security/hasher/AbstractPasswordHasherTests.java new file mode 100644 index 0000000000..065220a9d1 --- /dev/null +++ b/src/test/java/org/opensearch/security/hasher/AbstractPasswordHasherTests.java @@ -0,0 +1,92 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import java.nio.CharBuffer; + +import org.junit.Test; + +import org.opensearch.OpenSearchSecurityException; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThrows; + +public abstract class AbstractPasswordHasherTests { + + PasswordHasher passwordHasher; + + final String password = "testPassword"; + final String wrongPassword = "wrongTestPassword"; + + @Test + public void shouldMatchHashToCorrectPassword() { + String hashedPassword = passwordHasher.hash(password.toCharArray()); + assertThat(passwordHasher.check(password.toCharArray(), hashedPassword), is(true)); + } + + @Test + public void shouldNotMatchHashToWrongPassword() { + String hashedPassword = passwordHasher.hash(password.toCharArray()); + assertThat(passwordHasher.check(wrongPassword.toCharArray(), hashedPassword), is(false)); + + } + + @Test + public void shouldGenerateDifferentHashesForTheSamePassword() { + String hash1 = passwordHasher.hash(password.toCharArray()); + String hash2 = passwordHasher.hash(password.toCharArray()); + assertThat(hash1, is(not(hash2))); + } + + @Test + public void shouldHandleNullPasswordWhenHashing() { + char[] nullPassword = null; + assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.hash(nullPassword); }); + } + + @Test + public void shouldHandleNullPasswordWhenChecking() { + char[] nullPassword = null; + assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(nullPassword, "some hash"); }); + } + + @Test + public void shouldHandleEmptyHashWhenChecking() { + String emptyHash = ""; + assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(password.toCharArray(), emptyHash); }); + } + + @Test + public void shouldHandleNullHashWhenChecking() { + String nullHash = null; + assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(password.toCharArray(), nullHash); }); + } + + @Test + public void shouldCleanupPasswordCharArray() { + char[] password = new char[] { 'p', 'a', 's', 's', 'w', 'o', 'r', 'd' }; + passwordHasher.hash(password); + assertThat("\0\0\0\0\0\0\0\0", is(new String(password))); + } + + @Test + public void shouldCleanupPasswordCharBuffer() { + char[] password = new char[] { 'p', 'a', 's', 's', 'w', 'o', 'r', 'd' }; + CharBuffer passwordBuffer = CharBuffer.wrap(password); + passwordHasher.hash(password); + assertThat("\0\0\0\0\0\0\0\0", is(new String(password))); + assertThat("\0\0\0\0\0\0\0\0", is(passwordBuffer.toString())); + } + +} diff --git a/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java b/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java index 9fad74488f..701c205843 100644 --- a/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java +++ b/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java @@ -11,35 +11,23 @@ package org.opensearch.security.hasher; -import java.nio.CharBuffer; - +import org.junit.Before; import org.junit.Test; -import org.opensearch.OpenSearchSecurityException; +import org.opensearch.security.support.ConfigConstants; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; -import static org.junit.Assert.assertThrows; - -public class BCryptPasswordHasherTests { - - private final PasswordHasher passwordHasher = new BCryptPasswordHasher(); - - private final String password = "testPassword"; - private final String wrongPassword = "wrongTestPassword"; - - @Test - public void shouldMatchHashToCorrectPassword() { - String hashedPassword = passwordHasher.hash(password.toCharArray()); - assertThat(passwordHasher.check(password.toCharArray(), hashedPassword), is(true)); - } +import static org.hamcrest.Matchers.startsWith; - @Test - public void shouldNotMatchHashToWrongPassword() { - String hashedPassword = passwordHasher.hash(password.toCharArray()); - assertThat(passwordHasher.check(wrongPassword.toCharArray(), hashedPassword), is(false)); +public class BCryptPasswordHasherTests extends AbstractPasswordHasherTests { + @Before + public void setup() { + passwordHasher = new BCryptPasswordHasher( + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ); } /** @@ -53,49 +41,23 @@ public void shouldBeBackwardsCompatible() { } @Test - public void shouldGenerateDifferentHashesForTheSamePassword() { - String hash1 = passwordHasher.hash(password.toCharArray()); - String hash2 = passwordHasher.hash(password.toCharArray()); - assertThat(hash1, is(not(hash2))); - } - - @Test - public void shouldHandleNullPasswordWhenHashing() { - char[] nullPassword = null; - assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.hash(nullPassword); }); - } - - @Test - public void shouldHandleNullPasswordWhenChecking() { - char[] nullPassword = null; - assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(nullPassword, "some hash"); }); - } - - @Test - public void shouldHandleEmptyHashWhenChecking() { - String emptyHash = ""; - assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(password.toCharArray(), emptyHash); }); - } - - @Test - public void shouldHandleNullHashWhenChecking() { - String nullHash = null; - assertThrows(OpenSearchSecurityException.class, () -> { passwordHasher.check(password.toCharArray(), nullHash); }); - } + public void shouldGenerateAValidHashForParameters() { + PasswordHasher hasher = new BCryptPasswordHasher("A", 8); + String hash = hasher.hash(password.toCharArray()); + assertThat(hash, startsWith("$2a$08")); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); - @Test - public void shouldCleanupPasswordCharArray() { - char[] password = new char[] { 'p', 'a', 's', 's', 'w', 'o', 'r', 'd' }; - passwordHasher.hash(password); - assertThat("\0\0\0\0\0\0\0\0", is(new String(password))); - } + hasher = new BCryptPasswordHasher("B", 10); + hash = hasher.hash(password.toCharArray()); + assertThat(hash, startsWith("$2b$10")); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); - @Test - public void shouldCleanupPasswordCharBuffer() { - char[] password = new char[] { 'p', 'a', 's', 's', 'w', 'o', 'r', 'd' }; - CharBuffer passwordBuffer = CharBuffer.wrap(password); - passwordHasher.hash(password); - assertThat("\0\0\0\0\0\0\0\0", is(new String(password))); - assertThat("\0\0\0\0\0\0\0\0", is(passwordBuffer.toString())); + hasher = new BCryptPasswordHasher("Y", 13); + hash = hasher.hash(password.toCharArray()); + assertThat(hash, startsWith("$2y$13")); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); } } diff --git a/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java b/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java new file mode 100644 index 0000000000..1933e99d09 --- /dev/null +++ b/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java @@ -0,0 +1,67 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import org.junit.Before; +import org.junit.Test; + +import org.opensearch.security.support.ConfigConstants; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +public class PBKDF2PasswordHasherTests extends AbstractPasswordHasherTests { + + @Before + public void setup() { + passwordHasher = new PBKDF2PasswordHasher( + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ); + } + + @Test + public void shouldGenerateValidHashesFromParameters() { + PasswordHasher hasher = new PBKDF2PasswordHasher("SHA1", 150000, 128); + String hash = hasher.hash(password.toCharArray()); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); + + hasher = new PBKDF2PasswordHasher("SHA224", 100000, 224); + hash = hasher.hash(password.toCharArray()); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); + + hasher = new PBKDF2PasswordHasher("SHA256", 75000, 256); + hash = hasher.hash(password.toCharArray()); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); + + hasher = new PBKDF2PasswordHasher("SHA384", 50000, 384); + hash = hasher.hash(password.toCharArray()); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); + + hasher = new PBKDF2PasswordHasher("SHA512", 10000, 512); + hash = hasher.hash(password.toCharArray()); + assertThat(hasher.check(password.toCharArray(), hash), is(true)); + assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); + } + + @Test + public void getHash() { + String output = passwordHasher.hash("user1".toCharArray()); + System.out.println(output); + } + +} diff --git a/src/test/java/org/opensearch/security/hasher/PasswordHasherFactoryTests.java b/src/test/java/org/opensearch/security/hasher/PasswordHasherFactoryTests.java new file mode 100644 index 0000000000..81e250438e --- /dev/null +++ b/src/test/java/org/opensearch/security/hasher/PasswordHasherFactoryTests.java @@ -0,0 +1,186 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hasher; + +import org.junit.Test; + +import org.opensearch.common.settings.Settings; +import org.opensearch.security.support.ConfigConstants; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThrows; + +public class PasswordHasherFactoryTests { + + @Test + public void shouldReturnBCryptByDefault() { + final Settings settings = Settings.EMPTY; + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof BCryptPasswordHasher, is(true)); + } + + @Test + public void shouldReturnBCryptWhenBCryptSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof BCryptPasswordHasher, is(true)); + } + + @Test + public void shouldReturnBCryptWhenBCryptWithValidMinorVersionSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, "B") + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof BCryptPasswordHasher, is(true)); + } + + @Test + public void shouldReturnBCryptWhenBCryptWithValidLogRoundsSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, 8) + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof BCryptPasswordHasher, is(true)); + } + + @Test + public void shouldReturnExceptionWhenInvalidBCryptMinorVersionSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, "X") + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + } + + @Test + public void shouldReturnExceptionWhenInvalidBCryptRoundsSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, "3") + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + + final Settings settings2 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, "32") + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings2); }); + + } + + @Test + public void shouldReturnPBKDF2WhenPBKDF2Specified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + } + + @Test + public void shouldReturnPBKDF2WhenPBKDF2WithValidFunctionSpecified() { + final Settings settingsSHA1 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA1") + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settingsSHA1); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + + final Settings settingsSHA224 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA224") + .build(); + passwordHasher = PasswordHasherFactory.createPasswordHasher(settingsSHA224); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + + final Settings settingsSHA256 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA256") + .build(); + passwordHasher = PasswordHasherFactory.createPasswordHasher(settingsSHA256); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + + final Settings settingsSHA384 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA384") + .build(); + passwordHasher = PasswordHasherFactory.createPasswordHasher(settingsSHA384); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + + final Settings settingsSHA512 = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA512") + .build(); + passwordHasher = PasswordHasherFactory.createPasswordHasher(settingsSHA512); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + } + + @Test + public void shouldReturnPBKDF2WhenPBKDF2WithValidIterationsSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, 32000) + .build(); + + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + } + + @Test + public void shouldReturnPBKDF2WhenPBKDF2WithValidLengthSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, 512) + .build(); + PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(settings); + assertThat(passwordHasher instanceof PBKDF2PasswordHasher, is(true)); + } + + @Test + public void shouldReturnExceptionWhenInvalidPBKDF2FunctionSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, "SHA1000") + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + } + + @Test + public void shouldReturnExceptionWhenInvalidPBKDF2IterationsSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, -100000) + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + } + + @Test + public void shouldReturnExceptionWhenInvalidPBKDF2LengthSpecified() { + final Settings settings = Settings.builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.PBKDF2) + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, -100) + .build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + } + + @Test + public void shouldReturnExceptionWhenInvalidHashingAlgorithmSpecified() { + final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, "Invalid").build(); + assertThrows(IllegalArgumentException.class, () -> { PasswordHasherFactory.createPasswordHasher(settings); }); + } +} diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java new file mode 100644 index 0000000000..678982c255 --- /dev/null +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -0,0 +1,145 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.tools; + +import java.io.ByteArrayOutputStream; +import java.io.InputStream; +import java.io.PrintStream; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import com.password4j.CompressedPBKDF2Function; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class HasherTests { + private final ByteArrayOutputStream out = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + private final InputStream originalIn = System.in; + + @Before + public void setOutputStreams() { + System.setOut(new PrintStream(out)); + } + + @After + public void restoreStreams() { + System.setOut(originalOut); + System.setIn(originalIn); + } + + @Test + public void testWithDefaultArguments() { + Hasher.main(new String[] { "-p", "password" }); + assertTrue("should return a valid BCrypt hash with the default BCrypt configuration", out.toString().startsWith("$2b$12")); + } + + @Test + public void testWithBCryptRoundsArgument() { + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-r", "5" }); + assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2b$05")); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-r", "5" }); + assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2b$05")); + } + + @Test + public void testWithBCryptMinorArgument() { + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-min", "A" }); + assertTrue("should return a valid BCrypt hash with the correct value for \"minor\"", out.toString().startsWith("$2a$12")); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-min", "Y" }); + assertTrue("should return a valid BCrypt hash with the correct value for \"minor\"", out.toString().startsWith("$2y$12")); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-min", "B" }); + assertTrue("should return a valid BCrypt hash with the correct value for \"minor\"", out.toString().startsWith("$2b$12")); + out.reset(); + } + + @Test + public void testWithBCryptAllArguments() { + Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-min", "A", "-r", "5" }); + assertTrue("should return a valid BCrypt hash with the correct configuration", out.toString().startsWith("$2a$05")); + } + + @Test + public void testWithPBKDF2DefaultArguments() { + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2" }); + CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + } + + @Test + public void testWithPBKDF2FunctionArgument() { + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-f", "SHA512" }); + CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA512"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-f", "SHA384" }); + pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA384"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + } + + @Test + public void testWithPBKDF2IterationsArgument() { + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-i", "100000" }); + CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 100000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-i", "200000" }); + pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 200000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + } + + @Test + public void testWithPBKDF2LengthArgument() { + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-l", "400" }); + CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 400); + out.reset(); + + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-l", "300" }); + pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 300); + } + + @Test + public void testWithPBKDF2AllArguments() { + Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-l", "250", "-i", "150000", "-f", "SHA384" }); + CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); + assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA384"); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 150000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 250); + } +} diff --git a/src/test/resources/auditlog/internal_users_pbkdf2.yml b/src/test/resources/auditlog/internal_users_pbkdf2.yml new file mode 100644 index 0000000000..6f95dd107d --- /dev/null +++ b/src/test/resources/auditlog/internal_users_pbkdf2.yml @@ -0,0 +1,46 @@ +--- +_meta: + type: "internalusers" + config_version: 2 +admin: + hash: "$3$1331439861760512$wBFrJJIAokWuJxlO6BQPLashXgznvR4tRmXk3aEy9SpHWrb9kFjPPLByZZzMLBNQFjhepgbngYh7RfMh8TrPLw==$vqGlzGsxqGf9TgfxhORjdoqRFB3npvBd9B0GAtBMg9mD2zBbSTohRYlOxUL7UQLma66zZdD67c4RNE9BKelabw==" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with standard configuration" +user1: + hash: "$1$4294967296128$VmnDMbQ4wLiFUq178RKvj+EYfdb3Q26qCiDcJDoCxpYnKpyuG0JhTC2wpUkMUveV5RmBFzldKQkdqZEfE0XAgg==$9u3aMWc6VP2oGkXei7UaXA==" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with: SHA1 function | 1000 iterations | 128 length" +user2: + hash: "$2$214748364800224$eQgqv2RI6yo95yeVnM5sfwUCwxHo6re0w+wpx6ZqZtHQV+dzlyP6YFitjNG2mlaTkg0pR56xArQaAgapdVcBQQ==$tGHWhoc83cd5nZ7QIZKPORjW/N5jklhYhRgXpw==" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with: SHA224 function | 50000 iterations | 224 length" +user3: + hash: "$3$322122547200256$5b3wEAsMc05EZFxfncCUfZRERgvbwlBhYXd5vVR14kNJtmhXSpYMzydRZxO9096IPTkc47doH4hIdKX6LTguLg==$oQQvAtUyOC6cwdAYi5WeIM7rGUN9l3IdJ9y2RNxZCWo=" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with: SHA256 function | 75000 iterations | 256 length" +user4: + hash: "$4$429496729600384$+SNSgbZD67a1bd92iuEiHCq5pvvrCx3HrNIf5hbGIJdxgXegpWilpB6vUGvYigegAUzZqE9iIsL4pSJztUNJYw==$lTxZ7tax6dBQ0r4qPJpc8d7YuoTBiUujY9HJeAZvARXMjIgvnJwa6FeYugttOKc0" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with: SHA384 function | 100000 iterations | 384 length" +user5: + hash: "$5$644245094400512$HQe/MOv/NAlgodNhqTmjqj5jGxBwG5xuRaxKwn7r4nlUba1kj/CYnpdFaXGvVeRxt2NLm8fbekS6NYonv358Ew==$1sDx+0tMbtGzU6jlQg/Dyt30Yxuy5RdNmP9B1EzMTxYWi8k1xg2gXLy7w1XbetEC8UD/lpyXJPlaoxXpsaADyA==" + reserved: false + hidden: false + backend_roles: [] + attributes: {} + description: "Hashed with PBKDF2 with: SHA512 function | 150000 iterations | 512 length" From 79e666d45c49a083058ff341f4d9d9c7425e663c Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Tue, 2 Jul 2024 11:51:59 +0100 Subject: [PATCH 02/13] Added missing tests, fixed checkstyle test failures & added potential fix for integration tests failures Signed-off-by: Dan Cecoi --- .../hash/BCryptCustomConfigHashingTests.java | 23 +++-- .../hash/BCryptDefaultConfigHashingTests.java | 20 ---- .../security/hash/HashingTests.java | 10 +- .../hash/PBKDF2CustomConfigHashingTests.java | 97 +++++++++++++++++++ .../hash/PBKDF2DefaultConfigHashingTests.java | 37 ++++--- .../security/hasher/PBKDF2PasswordHasher.java | 4 +- .../SecuritySettingsConfigurer.java | 2 +- .../hasher/PBKDF2PasswordHasherTests.java | 7 -- 8 files changed, 136 insertions(+), 64 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java diff --git a/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java index 855ba37b85..95f05a7c0d 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/BCryptCustomConfigHashingTests.java @@ -14,19 +14,18 @@ import java.util.List; import java.util.Map; +import org.apache.http.HttpStatus; import org.awaitility.Awaitility; import org.junit.BeforeClass; import org.junit.Test; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; -import static org.apache.http.HttpStatus.*; -import static org.apache.http.HttpStatus.SC_UNAUTHORIZED; import static org.hamcrest.Matchers.equalTo; -import static org.opensearch.security.support.ConfigConstants.*; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -51,13 +50,13 @@ public static void startCluster() { .anonymousAuth(false) .nodeSettings( Map.of( - SECURITY_RESTAPI_ROLES_ENABLED, + ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), - SECURITY_PASSWORD_HASHING_ALGORITHM, - BCRYPT, - SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, + ConfigConstants.BCRYPT, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR, minor, - SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS, rounds ) ) @@ -75,19 +74,19 @@ public static void startCluster() { public void shouldAuthenticateWithCorrectPassword() { String hash = generateBCryptHash(PASSWORD, minor, rounds); createUserWithHashedPassword(cluster, "user_2", hash); - testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_2", PASSWORD, HttpStatus.SC_OK); createUserWithPlainTextPassword(cluster, "user_3", PASSWORD); - testPasswordAuth(cluster, "user_3", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_3", PASSWORD, HttpStatus.SC_OK); } @Test public void shouldNotAuthenticateWithIncorrectPassword() { String hash = generateBCryptHash(PASSWORD, minor, rounds); createUserWithHashedPassword(cluster, "user_4", hash); - testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_4", "wrong_password", HttpStatus.SC_UNAUTHORIZED); createUserWithPlainTextPassword(cluster, "user_5", PASSWORD); - testPasswordAuth(cluster, "user_5", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_5", "wrong_password", HttpStatus.SC_UNAUTHORIZED); } } diff --git a/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java index 278de22149..b515d2e00f 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java @@ -11,15 +11,11 @@ package org.opensearch.security.hash; -import java.security.SecureRandom; -import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.Objects; import org.junit.ClassRule; import org.junit.Test; -import org.bouncycastle.crypto.generators.OpenBSDBCrypt; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; @@ -44,13 +40,6 @@ public class BCryptDefaultConfigHashingTests extends HashingTests { .nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()))) .build(); - @Test - public void shouldAuthenticateWhenUserCreatedWithLegacyHash() { - String hash = generateLegacyBCryptHash(PASSWORD.toCharArray()); - createUserWithHashedPassword(cluster, "user_1", hash); - testPasswordAuth(cluster, "user_1", PASSWORD, SC_OK); - } - @Test public void shouldAuthenticateWithCorrectPassword() { String hash = generateBCryptHash( @@ -78,13 +67,4 @@ public void shouldNotAuthenticateWithIncorrectPassword() { createUserWithPlainTextPassword(cluster, "user_5", PASSWORD); testPasswordAuth(cluster, "user_5", "wrong_password", SC_UNAUTHORIZED); } - - private String generateLegacyBCryptHash(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; - } } diff --git a/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java index 9d2da3a9fd..4330af4ad1 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/HashingTests.java @@ -15,16 +15,18 @@ import com.carrotsearch.randomizedtesting.RandomizedTest; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.http.HttpStatus; import org.junit.runner.RunWith; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; -import com.password4j.*; +import com.password4j.BcryptFunction; +import com.password4j.CompressedPBKDF2Function; +import com.password4j.Password; import com.password4j.types.Bcrypt; -import static org.apache.http.HttpStatus.*; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -43,7 +45,7 @@ public void createUserWithPlainTextPassword(LocalCluster cluster, String usernam "_plugins/_security/api/internalusers/" + username, String.format("{\"password\": \"%s\",\"opendistro_security_roles\": []}", password) ); - assertThat(httpResponse.getStatusCode(), equalTo(SC_CREATED)); + assertThat(httpResponse.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); } } @@ -53,7 +55,7 @@ public void createUserWithHashedPassword(LocalCluster cluster, String username, "_plugins/_security/api/internalusers/" + username, String.format("{\"hash\": \"%s\",\"opendistro_security_roles\": []}", hashedPassword) ); - assertThat(httpResponse.getStatusCode(), equalTo(SC_CREATED)); + assertThat(httpResponse.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); } } diff --git a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java new file mode 100644 index 0000000000..a39d3de4a9 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java @@ -0,0 +1,97 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.hash; + +import java.util.List; +import java.util.Map; + +import org.apache.http.HttpStatus; +import org.awaitility.Awaitility; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.security.support.ConfigConstants.*; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +public class PBKDF2CustomConfigHashingTests extends HashingTests { + + public static LocalCluster cluster; + + private static final String PASSWORD = "top$ecret1234!"; + + private static String function; + private static int iterations, length; + + @BeforeClass + public static void startCluster() { + + function = randomFrom(List.of("SHA224", "SHA256", "SHA384", "SHA512")); + iterations = randomFrom(List.of(32000, 64000, 128000, 256000)); + length = randomFrom(List.of(128, 256, 512)); + + TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS) + .hash(generatePBKDF2Hash("secret", function, iterations, length)); + cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .anonymousAuth(false) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_PASSWORD_HASHING_ALGORITHM, + PBKDF2, + SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, + function, + SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, + iterations, + SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, + length + ) + ) + .build(); + cluster.before(); + + try (TestRestClient client = cluster.getRestClient(ADMIN_USER.getName(), "secret")) { + Awaitility.await() + .alias("Load default configuration") + .until(() -> client.securityHealth().getTextFromJsonBody("/status"), equalTo("UP")); + } + } + + @Test + public void shouldAuthenticateWithCorrectPassword() { + String hash = generatePBKDF2Hash(PASSWORD, function, iterations, length); + createUserWithHashedPassword(cluster, "user_1", hash); + testPasswordAuth(cluster, "user_1", PASSWORD, HttpStatus.SC_OK); + + createUserWithPlainTextPassword(cluster, "user_2", PASSWORD); + testPasswordAuth(cluster, "user_2", PASSWORD, HttpStatus.SC_OK); + } + + @Test + public void shouldNotAuthenticateWithIncorrectPassword() { + String hash = generatePBKDF2Hash(PASSWORD, function, iterations, length); + createUserWithHashedPassword(cluster, "user_3", hash); + testPasswordAuth(cluster, "user_3", "wrong_password", HttpStatus.SC_UNAUTHORIZED); + + createUserWithPlainTextPassword(cluster, "user_4", PASSWORD); + testPasswordAuth(cluster, "user_4", "wrong_password", HttpStatus.SC_UNAUTHORIZED); + } +} diff --git a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java index 14a48c49b7..2becee1f36 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2DefaultConfigHashingTests.java @@ -14,16 +14,15 @@ import java.util.List; import java.util.Map; +import org.apache.http.HttpStatus; import org.junit.ClassRule; import org.junit.Test; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; -import static org.apache.http.HttpStatus.SC_OK; -import static org.apache.http.HttpStatus.SC_UNAUTHORIZED; -import static org.opensearch.security.support.ConfigConstants.*; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -33,9 +32,9 @@ public class PBKDF2DefaultConfigHashingTests extends HashingTests { .hash( generatePBKDF2Hash( "secret", - SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT ) ); @@ -46,10 +45,10 @@ public class PBKDF2DefaultConfigHashingTests extends HashingTests { .anonymousAuth(false) .nodeSettings( Map.of( - SECURITY_RESTAPI_ROLES_ENABLED, + ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), - SECURITY_PASSWORD_HASHING_ALGORITHM, - PBKDF2 + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, + ConfigConstants.PBKDF2 ) ) .build(); @@ -58,29 +57,29 @@ public class PBKDF2DefaultConfigHashingTests extends HashingTests { public void shouldAuthenticateWithCorrectPassword() { String hash = generatePBKDF2Hash( PASSWORD, - SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT ); createUserWithHashedPassword(cluster, "user_1", hash); - testPasswordAuth(cluster, "user_1", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_1", PASSWORD, HttpStatus.SC_OK); createUserWithPlainTextPassword(cluster, "user_2", PASSWORD); - testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_2", PASSWORD, HttpStatus.SC_OK); } @Test public void shouldNotAuthenticateWithIncorrectPassword() { String hash = generatePBKDF2Hash( PASSWORD, - SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, - SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT ); createUserWithHashedPassword(cluster, "user_3", hash); - testPasswordAuth(cluster, "user_3", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_3", "wrong_password", HttpStatus.SC_UNAUTHORIZED); createUserWithPlainTextPassword(cluster, "user_4", PASSWORD); - testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_4", "wrong_password", HttpStatus.SC_UNAUTHORIZED); } } diff --git a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java index 5c466c5e05..b131fe3e72 100644 --- a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java @@ -18,7 +18,9 @@ import org.opensearch.OpenSearchSecurityException; import org.opensearch.SpecialPermission; -import com.password4j.*; +import com.password4j.CompressedPBKDF2Function; +import com.password4j.HashingFunction; +import com.password4j.Password; import static org.opensearch.core.common.Strings.isNullOrEmpty; diff --git a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java index 709d882a3e..ee3c67ecfe 100644 --- a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java +++ b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java @@ -88,7 +88,7 @@ public class SecuritySettingsConfigurer { public SecuritySettingsConfigurer(Installer installer) { this.installer = installer; this.passwordHasher = PasswordHasherFactory.createPasswordHasher( - Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, PBKDF2).build() + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, BCRYPT).build() ); } diff --git a/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java b/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java index 1933e99d09..6d54173a10 100644 --- a/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java +++ b/src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java @@ -57,11 +57,4 @@ public void shouldGenerateValidHashesFromParameters() { assertThat(hasher.check(password.toCharArray(), hash), is(true)); assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); } - - @Test - public void getHash() { - String output = passwordHasher.hash("user1".toCharArray()); - System.out.println(output); - } - } From cf65688e7fea34a8c217145de8a46b92e7a38905 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Tue, 2 Jul 2024 11:59:22 +0100 Subject: [PATCH 03/13] fixed checkstyle test failures Signed-off-by: Dan Cecoi --- .../hash/BCryptDefaultConfigHashingTests.java | 26 +++++++++---------- .../hash/PBKDF2CustomConfigHashingTests.java | 14 +++++----- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java index b515d2e00f..86adc728ea 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/BCryptDefaultConfigHashingTests.java @@ -14,17 +14,15 @@ import java.util.List; import java.util.Map; +import org.apache.http.HttpStatus; import org.junit.ClassRule; import org.junit.Test; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; -import static org.apache.http.HttpStatus.*; -import static org.opensearch.security.support.ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT; -import static org.opensearch.security.support.ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT; -import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -37,34 +35,36 @@ public class BCryptDefaultConfigHashingTests extends HashingTests { .authc(AUTHC_HTTPBASIC_INTERNAL) .users(ADMIN_USER) .anonymousAuth(false) - .nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()))) + .nodeSettings( + Map.of(ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) + ) .build(); @Test public void shouldAuthenticateWithCorrectPassword() { String hash = generateBCryptHash( PASSWORD, - SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, - SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT ); createUserWithHashedPassword(cluster, "user_2", hash); - testPasswordAuth(cluster, "user_2", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_2", PASSWORD, HttpStatus.SC_OK); createUserWithPlainTextPassword(cluster, "user_3", PASSWORD); - testPasswordAuth(cluster, "user_3", PASSWORD, SC_OK); + testPasswordAuth(cluster, "user_3", PASSWORD, HttpStatus.SC_OK); } @Test public void shouldNotAuthenticateWithIncorrectPassword() { String hash = generateBCryptHash( PASSWORD, - SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, - SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT, + ConfigConstants.SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT ); createUserWithHashedPassword(cluster, "user_4", hash); - testPasswordAuth(cluster, "user_4", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_4", "wrong_password", HttpStatus.SC_UNAUTHORIZED); createUserWithPlainTextPassword(cluster, "user_5", PASSWORD); - testPasswordAuth(cluster, "user_5", "wrong_password", SC_UNAUTHORIZED); + testPasswordAuth(cluster, "user_5", "wrong_password", HttpStatus.SC_UNAUTHORIZED); } } diff --git a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java index a39d3de4a9..9f2ceb2e93 100644 --- a/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java +++ b/src/integrationTest/java/org/opensearch/security/hash/PBKDF2CustomConfigHashingTests.java @@ -19,13 +19,13 @@ import org.junit.BeforeClass; import org.junit.Test; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; import static org.hamcrest.Matchers.equalTo; -import static org.opensearch.security.support.ConfigConstants.*; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -53,15 +53,15 @@ public static void startCluster() { .anonymousAuth(false) .nodeSettings( Map.of( - SECURITY_RESTAPI_ROLES_ENABLED, + ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()), - SECURITY_PASSWORD_HASHING_ALGORITHM, - PBKDF2, - SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, + ConfigConstants.PBKDF2, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION, function, - SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS, iterations, - SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, + ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH, length ) ) From 7f2f013399ad080a6ff48da664106bd18a533add Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Wed, 3 Jul 2024 15:21:28 +0100 Subject: [PATCH 04/13] Made changes to the default PBKDF2 rounds value to comply with OWASP recommendations; refactored hash.sh help messages; modified an exception message Signed-off-by: Dan Cecoi --- .../security/hasher/PasswordHasherFactory.java | 3 +-- .../opensearch/security/support/ConfigConstants.java | 2 +- .../java/org/opensearch/security/tools/Hasher.java | 10 +++++----- .../org/opensearch/security/tools/HasherTests.java | 10 +++++----- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java index 571d743f45..3f88e84d00 100644 --- a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java +++ b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java @@ -54,7 +54,7 @@ private static PasswordHasher getBCryptHasher(Settings settings) { throw new IllegalArgumentException("BCrypt rounds must be between 4 and 31."); } if (!minor.equals("A") && !minor.equals("B") && !minor.equals("Y")) { - throw new IllegalArgumentException("BCrypt minor must be 'a', 'b', or 'y'."); + throw new IllegalArgumentException("BCrypt minor must be 'A', 'B', or 'Y'."); } return new BCryptPasswordHasher(minor, rounds); } @@ -74,7 +74,6 @@ private static PasswordHasher getPBKDF2Hasher(Settings settings) { ConfigConstants.SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT ); - // todo: validate validation if (!pbkdf2Function.matches("SHA(1|224|256|384|512)")) { throw new IllegalArgumentException("PBKDF2 function must be one of SHA1, SHA224, SHA256, SHA384, or SHA512."); } diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index bd72b943ff..5f253f61cf 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -157,7 +157,7 @@ public class ConfigConstants { public static final String SECURITY_PASSWORD_HASHING_ALGORITHM = "plugins.security.password.hashing.algorithm"; public static final String SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT = BCRYPT; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS = "plugins.security.password.hashing.pbkdf2.iterations"; - public static final int SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT = 310000; + public static final int SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT = 600000; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH = "plugins.security.password.hashing.pbkdf2.length"; public static final int SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT = 512; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION = "plugins.security.password.hashing.pbkdf2.function"; diff --git a/src/main/java/org/opensearch/security/tools/Hasher.java b/src/main/java/org/opensearch/security/tools/Hasher.java index 77fea7c28a..2965dbb5c6 100644 --- a/src/main/java/org/opensearch/security/tools/Hasher.java +++ b/src/main/java/org/opensearch/security/tools/Hasher.java @@ -150,9 +150,9 @@ private static Options buildOptions() { options.addOption(Option.builder(PASSWORD_OPTION).argName("password").hasArg().desc("Cleartext password to hash").build()); options.addOption( Option.builder(ENV_OPTION) - .argName("name environment variable") + .argName("Environment variable name") .hasArg() - .desc("name environment variable to read password from") + .desc("Environment variable name to read password from") .build() ); options.addOption( @@ -160,7 +160,7 @@ private static Options buildOptions() { .longOpt("algorithm") .argName("hashing algorithm") .hasArg() - .desc("Hashing algorithm (BCrypt, PBKDF2, SCrypt, Argon2)") + .desc("Hashing algorithm (BCrypt, PBKDF2)") .build() ); options.addOption( @@ -178,7 +178,7 @@ private static Options buildOptions() { options.addOption( Option.builder(LENGTH_OPTION) .longOpt("length") - .desc("Desired length of the final derived key (for Argon2, PBKDF2).") + .desc("Desired length of the final derived key (for PBKDF2).") .hasArg() .argName("length") .type(Number.class) @@ -187,7 +187,7 @@ private static Options buildOptions() { options.addOption( Option.builder(ITERATIONS_OPTION) .longOpt("iterations") - .desc("Iterations to perform (for Argon2, PBKDF2).") + .desc("Iterations to perform (for PBKDF2).") .hasArg() .argName("iterations") .type(Number.class) diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java index 678982c255..9a06133e0a 100644 --- a/src/test/java/org/opensearch/security/tools/HasherTests.java +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -82,7 +82,7 @@ public void testWithPBKDF2DefaultArguments() { Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2" }); CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); } @@ -91,14 +91,14 @@ public void testWithPBKDF2FunctionArgument() { Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-f", "SHA512" }); CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA512"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); out.reset(); Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-f", "SHA384" }); pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA384"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); } @@ -123,14 +123,14 @@ public void testWithPBKDF2LengthArgument() { Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-l", "400" }); CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 400); out.reset(); Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-l", "300" }); pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 310000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 300); } From c61edfff37838b542917aa51306f0eefc852543f Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Wed, 3 Jul 2024 15:22:39 +0100 Subject: [PATCH 05/13] Fixed the expected rounds value in Hasher tests Signed-off-by: Dan Cecoi --- src/test/java/org/opensearch/security/tools/HasherTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java index 9a06133e0a..1d6b83c10f 100644 --- a/src/test/java/org/opensearch/security/tools/HasherTests.java +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -82,7 +82,7 @@ public void testWithPBKDF2DefaultArguments() { Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2" }); CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); } From e58828a2e1b30b19ae36ca6a9d55598a703a5938 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Wed, 3 Jul 2024 15:27:53 +0100 Subject: [PATCH 06/13] fixed spotlessCheck failure Signed-off-by: Dan Cecoi --- src/test/java/org/opensearch/security/tools/HasherTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java index 1d6b83c10f..9a06133e0a 100644 --- a/src/test/java/org/opensearch/security/tools/HasherTests.java +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -82,7 +82,7 @@ public void testWithPBKDF2DefaultArguments() { Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2" }); CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); - assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); + assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); } From 4041271a9b329b4727a7110073ed2b954441e961 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Thu, 4 Jul 2024 11:00:24 +0100 Subject: [PATCH 07/13] Fixed some test comments & added a long arg option for password in the Hasher script Signed-off-by: Dan Cecoi --- src/main/java/org/opensearch/security/tools/Hasher.java | 4 +++- .../auditlog/compliance/RestApiComplianceAuditlogTest.java | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/tools/Hasher.java b/src/main/java/org/opensearch/security/tools/Hasher.java index 2965dbb5c6..8fd337b9bc 100644 --- a/src/main/java/org/opensearch/security/tools/Hasher.java +++ b/src/main/java/org/opensearch/security/tools/Hasher.java @@ -147,7 +147,9 @@ private static Settings getPBKDF2Settings(CommandLine line) throws ParseExceptio private static Options buildOptions() { final Options options = new Options(); - options.addOption(Option.builder(PASSWORD_OPTION).argName("password").hasArg().desc("Cleartext password to hash").build()); + options.addOption( + Option.builder(PASSWORD_OPTION).longOpt("password").argName("password").hasArg().desc("Cleartext password to hash").build() + ); options.addOption( Option.builder(ENV_OPTION) .argName("Environment variable name") diff --git a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java index c3ab0799a7..5a6d7ad135 100644 --- a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java @@ -259,7 +259,7 @@ public void testPBKDF2HashRedaction() { rh.sendAdminCertificate = true; rh.keystore = "kirk-keystore.jks"; - // read internal users and verify no BCrypt hash is present in audit logs + // read internal users and verify no PBKDF2 hash is present in audit logs final AuditMessage message1 = TestAuditlogImpl.doThenWaitForMessage(() -> { rh.executeGetRequest("/_opendistro/_security/api/internalusers"); }); @@ -272,7 +272,7 @@ public void testPBKDF2HashRedaction() { ); Assert.assertTrue(message1.toString().contains("__HASH__")); - // read internal user worf and verify no BCrypt hash is present in audit logs + // read internal user and verify no PBKDF2 hash is present in audit logs final AuditMessage message2 = TestAuditlogImpl.doThenWaitForMessage(() -> { rh.executeGetRequest("/_opendistro/_security/api/internalusers/user1"); }); From 5b658defdc11c2a2ceae48c17f654c93fa2d78e0 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Thu, 4 Jul 2024 11:52:28 +0100 Subject: [PATCH 08/13] added @SuppressWarnings("removal") annotations where SecurityManager is used to fix compile errors with JDK21 Signed-off-by: Dan Cecoi --- .../org/opensearch/security/hasher/PBKDF2PasswordHasher.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java index b131fe3e72..756258f892 100644 --- a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java @@ -26,6 +26,7 @@ class PBKDF2PasswordHasher extends AbstractPasswordHasher { + @SuppressWarnings("removal") PBKDF2PasswordHasher(String function, int iterations, int length) { SecurityManager securityManager = System.getSecurityManager(); if (securityManager != null) { @@ -37,6 +38,7 @@ class PBKDF2PasswordHasher extends AbstractPasswordHasher { } @Override + @SuppressWarnings("removal") public String hash(char[] password) { if (password == null || password.length == 0) { throw new OpenSearchSecurityException("Password cannot be empty or null"); @@ -55,6 +57,7 @@ public String hash(char[] password) { } } + @SuppressWarnings("removal") @Override public boolean check(char[] password, String hash) { if (password == null || password.length == 0) { From 951abe19d58eb027819e5b90c82d94cca1915b51 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Fri, 5 Jul 2024 17:32:30 +0100 Subject: [PATCH 09/13] fixed test failure & removed wildcard import Signed-off-by: Dan Cecoi --- .../tools/democonfig/SecuritySettingsConfigurer.java | 10 ++++++---- .../security/auditlog/AbstractAuditlogiUnitTest.java | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java index ee3c67ecfe..d97fe2d1bc 100644 --- a/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java +++ b/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java @@ -37,7 +37,6 @@ import org.yaml.snakeyaml.Yaml; import static org.opensearch.security.DefaultObjectMapper.YAML_MAPPER; -import static org.opensearch.security.support.ConfigConstants.*; /** * This class updates the security related configuration, as needed. @@ -88,7 +87,7 @@ public class SecuritySettingsConfigurer { public SecuritySettingsConfigurer(Installer installer) { this.installer = installer; this.passwordHasher = PasswordHasherFactory.createPasswordHasher( - Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, BCRYPT).build() + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() ); } @@ -145,8 +144,11 @@ void updateAdminPassword() throws IOException { try { final PasswordValidator passwordValidator = PasswordValidator.of( Settings.builder() - .put(SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, "(?=.*[A-Z])(?=.*[^a-zA-Z\\\\d])(?=.*[0-9])(?=.*[a-z]).{8,}") - .put(SECURITY_RESTAPI_PASSWORD_MIN_LENGTH, DEFAULT_PASSWORD_MIN_LENGTH) + .put( + ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, + "(?=.*[A-Z])(?=.*[^a-zA-Z\\\\d])(?=.*[0-9])(?=.*[a-z]).{8,}" + ) + .put(ConfigConstants.SECURITY_RESTAPI_PASSWORD_MIN_LENGTH, DEFAULT_PASSWORD_MIN_LENGTH) .build() ); diff --git a/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java b/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java index ce84e5f14b..b4965d60ad 100644 --- a/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java +++ b/src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java @@ -38,6 +38,10 @@ protected String getResourceFolder() { return "auditlog"; } + protected final void setup(Settings settings) throws Exception { + setup(settings, new DynamicSecurityConfig()); + } + protected final void setup(Settings settings, DynamicSecurityConfig securityConfig) throws Exception { final Settings.Builder auditConfigSettings = Settings.builder(); final Settings.Builder defaultNodeSettings = Settings.builder(); From 9260028bb9e9fe9cd3c75177c463dc5c60df5e72 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Mon, 8 Jul 2024 11:56:16 +0100 Subject: [PATCH 10/13] changed the default BCrypt algorithm value to "B" and fixed some integration tests failures Signed-off-by: Dan Cecoi --- .../api/InternalUsersRestApiIntegrationTest.java | 10 +++++++--- .../opensearch/security/support/ConfigConstants.java | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java index 884f2ce2a6..21425560b2 100644 --- a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java @@ -27,13 +27,15 @@ import org.junit.Assert; import org.junit.Test; +import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.Strings; import org.opensearch.core.xcontent.ToXContentObject; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.dlic.rest.api.Endpoint; -import org.opensearch.security.hasher.BCryptPasswordHasher; import org.opensearch.security.hasher.PasswordHasher; +import org.opensearch.security.hasher.PasswordHasherFactory; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.TestRestClient; import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; @@ -59,8 +61,10 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi private final static String SOME_ROLE = "some-role"; - private final PasswordHasher passwordHasher = new BCryptPasswordHasher(); - + private final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(Settings + .builder() + .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) + .build()); static { testSecurityConfig.withRestAdminUser(REST_API_ADMIN_INTERNAL_USERS_ONLY, restAdminPermission(Endpoint.INTERNALUSERS)) .user(new TestSecurityConfig.User(SERVICE_ACCOUNT_USER).attr("service", "true").attr("enabled", "true")) diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 5f253f61cf..5a13d54ffb 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -152,7 +152,7 @@ public class ConfigConstants { public static final String SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS = "plugins.security.password.hashing.bcrypt.rounds"; public static final int SECURITY_PASSWORD_HASHING_BCRYPT_ROUNDS_DEFAULT = 12; public static final String SECURITY_PASSWORD_HASHING_BCRYPT_MINOR = "plugins.security.password.hashing.bcrypt.minor"; - public static final String SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT = "B"; + public static final String SECURITY_PASSWORD_HASHING_BCRYPT_MINOR_DEFAULT = "Y"; public static final String SECURITY_PASSWORD_HASHING_ALGORITHM = "plugins.security.password.hashing.algorithm"; public static final String SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT = BCRYPT; From a715d6eb71116dda4b16ac80aa5acaf644eb1a3f Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Mon, 8 Jul 2024 12:30:19 +0100 Subject: [PATCH 11/13] fixed HasherTests post changing the default BCrypt algorithm value from B to Y Signed-off-by: Dan Cecoi --- .../java/org/opensearch/security/tools/HasherTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java index 9a06133e0a..295fd3df08 100644 --- a/src/test/java/org/opensearch/security/tools/HasherTests.java +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -43,17 +43,17 @@ public void restoreStreams() { @Test public void testWithDefaultArguments() { Hasher.main(new String[] { "-p", "password" }); - assertTrue("should return a valid BCrypt hash with the default BCrypt configuration", out.toString().startsWith("$2b$12")); + assertTrue("should return a valid BCrypt hash with the default BCrypt configuration", out.toString().startsWith("$2y$12")); } @Test public void testWithBCryptRoundsArgument() { Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-r", "5" }); - assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2b$05")); + assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2y$05")); out.reset(); Hasher.main(new String[] { "-p", "password", "-a", "BCrypt", "-r", "5" }); - assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2b$05")); + assertTrue("should return a valid BCrypt hash with the correct value for \"rounds\"", out.toString().startsWith("$2y$05")); } @Test From ffa68f292c89a476bb9b933445bdafe008893b18 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Tue, 9 Jul 2024 15:42:56 +0100 Subject: [PATCH 12/13] implemented required changes as per review: - Added help option and improved the hash.sh help message - Added java docs to PasswordHasher & AbstractPasswordHasher - Changed the default PBKDF2 length from 512 to 256 - Improved the validation exception messages - Moved null check logic to AbstractPasswordHasher Signed-off-by: Dan Cecoi --- .../InternalUsersRestApiIntegrationTest.java | 7 ++- .../hasher/AbstractPasswordHasher.java | 46 ++++++++++++++++ .../security/hasher/BCryptPasswordHasher.java | 15 ++---- .../security/hasher/PBKDF2PasswordHasher.java | 22 ++++---- .../security/hasher/PasswordHasher.java | 17 ++++++ .../hasher/PasswordHasherFactory.java | 19 ++++--- .../security/support/ConfigConstants.java | 4 +- .../org/opensearch/security/tools/Hasher.java | 53 +++++++++++-------- .../hasher/BCryptPasswordHasherTests.java | 1 + .../security/tools/HasherTests.java | 10 ++-- 10 files changed, 129 insertions(+), 65 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java index 21425560b2..0a35793173 100644 --- a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java @@ -61,10 +61,9 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi private final static String SOME_ROLE = "some-role"; - private final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher(Settings - .builder() - .put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT) - .build()); + private final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); static { testSecurityConfig.withRestAdminUser(REST_API_ADMIN_INTERNAL_USERS_ONLY, restAdminPermission(Endpoint.INTERNALUSERS)) .user(new TestSecurityConfig.User(SERVICE_ACCOUNT_USER).attr("service", "true").attr("enabled", "true")) diff --git a/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java b/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java index 48e5d01ff9..151f657de1 100644 --- a/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java @@ -14,20 +14,66 @@ import java.nio.CharBuffer; import java.util.Arrays; +import org.opensearch.OpenSearchSecurityException; + import com.password4j.HashingFunction; +import static org.opensearch.core.common.Strings.isNullOrEmpty; + +/** + * Abstract implementation of PasswordHasher interface + */ abstract class AbstractPasswordHasher implements PasswordHasher { + /** + * The hashing function used by the hasher. + */ HashingFunction hashingFunction; + /** + * {@inheritDoc} + */ public abstract String hash(char[] password); + /** + * {@inheritDoc} + */ public abstract boolean check(char[] password, String hash); + /** + * Clears the given password buffer to prevent sensitive data from being left in memory. + * + * @param password the CharBuffer containing the password to clear + */ protected void cleanup(CharBuffer password) { password.clear(); char[] passwordOverwrite = new char[password.capacity()]; Arrays.fill(passwordOverwrite, '\0'); password.put(passwordOverwrite); } + + /** + * Checks if the given password is null or empty and throws an exception if it is. + * + * @param password the password to check + * @throws OpenSearchSecurityException if the password is null or empty + */ + protected void checkPasswordNotNullOrEmpty(char[] password) { + if (password == null || password.length == 0) { + throw new OpenSearchSecurityException("Password cannot be empty or null"); + } + } + + /** + * Checks if the given hash is null or empty and throws an exception if it is. + * + * @param hash the hash to check + * @throws OpenSearchSecurityException if the hash is null or empty + */ + protected void checkHashNotNullOrEmpty(String hash) { + if (isNullOrEmpty(hash)) { + throw new OpenSearchSecurityException("Hash cannot be empty or null"); + } + } + } diff --git a/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java b/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java index 791d84cb36..5fada16926 100644 --- a/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java @@ -15,7 +15,6 @@ import java.security.AccessController; import java.security.PrivilegedAction; -import org.opensearch.OpenSearchSecurityException; import org.opensearch.SpecialPermission; import com.password4j.BcryptFunction; @@ -23,8 +22,6 @@ import com.password4j.Password; import com.password4j.types.Bcrypt; -import static org.opensearch.core.common.Strings.isNullOrEmpty; - class BCryptPasswordHasher extends AbstractPasswordHasher { BCryptPasswordHasher(String minor, int logRounds) { @@ -34,9 +31,7 @@ class BCryptPasswordHasher extends AbstractPasswordHasher { @SuppressWarnings("removal") @Override public String hash(char[] password) { - if (password == null || password.length == 0) { - throw new OpenSearchSecurityException("Password cannot be empty or null"); - } + checkPasswordNotNullOrEmpty(password); CharBuffer passwordBuffer = CharBuffer.wrap(password); try { SecurityManager securityManager = System.getSecurityManager(); @@ -54,12 +49,8 @@ public String hash(char[] password) { @SuppressWarnings("removal") @Override public boolean check(char[] password, String hash) { - if (password == null || password.length == 0) { - throw new OpenSearchSecurityException("Password cannot be empty or null"); - } - if (isNullOrEmpty(hash)) { - throw new OpenSearchSecurityException("Hash cannot be empty or null"); - } + checkPasswordNotNullOrEmpty(password); + checkHashNotNullOrEmpty(hash); CharBuffer passwordBuffer = CharBuffer.wrap(password); try { SecurityManager securityManager = System.getSecurityManager(); diff --git a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java index 756258f892..36828371ad 100644 --- a/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java @@ -15,17 +15,16 @@ import java.security.AccessController; import java.security.PrivilegedAction; -import org.opensearch.OpenSearchSecurityException; import org.opensearch.SpecialPermission; import com.password4j.CompressedPBKDF2Function; import com.password4j.HashingFunction; import com.password4j.Password; -import static org.opensearch.core.common.Strings.isNullOrEmpty; - class PBKDF2PasswordHasher extends AbstractPasswordHasher { + private static final int DEFAULT_SALT_LENGTH = 128; + @SuppressWarnings("removal") PBKDF2PasswordHasher(String function, int iterations, int length) { SecurityManager securityManager = System.getSecurityManager(); @@ -40,9 +39,7 @@ class PBKDF2PasswordHasher extends AbstractPasswordHasher { @Override @SuppressWarnings("removal") public String hash(char[] password) { - if (password == null || password.length == 0) { - throw new OpenSearchSecurityException("Password cannot be empty or null"); - } + checkPasswordNotNullOrEmpty(password); CharBuffer passwordBuffer = CharBuffer.wrap(password); try { SecurityManager securityManager = System.getSecurityManager(); @@ -50,7 +47,10 @@ public String hash(char[] password) { securityManager.checkPermission(new SpecialPermission()); } return AccessController.doPrivileged( - (PrivilegedAction) () -> Password.hash(passwordBuffer).addRandomSalt(64).with(hashingFunction).getResult() + (PrivilegedAction) () -> Password.hash(passwordBuffer) + .addRandomSalt(DEFAULT_SALT_LENGTH) + .with(hashingFunction) + .getResult() ); } finally { cleanup(passwordBuffer); @@ -60,12 +60,8 @@ public String hash(char[] password) { @SuppressWarnings("removal") @Override public boolean check(char[] password, String hash) { - if (password == null || password.length == 0) { - throw new OpenSearchSecurityException("Password cannot be empty or null"); - } - if (isNullOrEmpty(hash)) { - throw new OpenSearchSecurityException("Hash cannot be empty or null"); - } + checkPasswordNotNullOrEmpty(password); + checkHashNotNullOrEmpty(hash); CharBuffer passwordBuffer = CharBuffer.wrap(password); try { SecurityManager securityManager = System.getSecurityManager(); diff --git a/src/main/java/org/opensearch/security/hasher/PasswordHasher.java b/src/main/java/org/opensearch/security/hasher/PasswordHasher.java index deaab7e073..b115feac58 100644 --- a/src/main/java/org/opensearch/security/hasher/PasswordHasher.java +++ b/src/main/java/org/opensearch/security/hasher/PasswordHasher.java @@ -11,9 +11,26 @@ package org.opensearch.security.hasher; +/** + * Interface representing a password hasher which provides methods + * to hash a password and check a password against a hashed password. + */ public interface PasswordHasher { + /** + * Generates a hashed representation of the given password. + * + * @param password the password to hash + * @return a hashed representation of the password + */ String hash(char[] password); + /** + * Checks if the given password matches the provided hashed password. + * + * @param password the password to check + * @param hashedPassword the hashed password to check against + * @return true if the password matches the hashed password, false otherwise + */ boolean check(char[] password, String hashedPassword); } diff --git a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java index 3f88e84d00..dd98105ec6 100644 --- a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java +++ b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java @@ -11,6 +11,8 @@ package org.opensearch.security.hasher; +import java.util.Set; + import org.opensearch.common.settings.Settings; import org.opensearch.security.support.ConfigConstants; @@ -19,8 +21,9 @@ public class PasswordHasherFactory { - public static PasswordHasher createPasswordHasher(Settings settings) { + private static final Set ALLOWED_BCRYPT_MINORS = Set.of("A", "B", "Y"); + public static PasswordHasher createPasswordHasher(Settings settings) { String algorithm = settings.get( ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT @@ -51,10 +54,10 @@ private static PasswordHasher getBCryptHasher(Settings settings) { ).toUpperCase(); if (rounds < 4 || rounds > 31) { - throw new IllegalArgumentException("BCrypt rounds must be between 4 and 31."); + throw new IllegalArgumentException(String.format("BCrypt rounds must be between 4 and 31. Got: %d", rounds)); } - if (!minor.equals("A") && !minor.equals("B") && !minor.equals("Y")) { - throw new IllegalArgumentException("BCrypt minor must be 'A', 'B', or 'Y'."); + if (!ALLOWED_BCRYPT_MINORS.contains(minor)) { + throw new IllegalArgumentException(String.format("BCrypt minor must be 'A', 'B', or 'Y'. Got: %s", minor)); } return new BCryptPasswordHasher(minor, rounds); } @@ -75,13 +78,15 @@ private static PasswordHasher getPBKDF2Hasher(Settings settings) { ); if (!pbkdf2Function.matches("SHA(1|224|256|384|512)")) { - throw new IllegalArgumentException("PBKDF2 function must be one of SHA1, SHA224, SHA256, SHA384, or SHA512."); + throw new IllegalArgumentException( + String.format("PBKDF2 function must be one of SHA1, SHA224, SHA256, SHA384, or SHA512. Got: %s", pbkdf2Function) + ); } if (iterations <= 0) { - throw new IllegalArgumentException("PBKDF2 iterations must be a positive integer."); + throw new IllegalArgumentException(String.format("PBKDF2 iterations must be a positive integer. Got: %d", iterations)); } if (length <= 0) { - throw new IllegalArgumentException("PBKDF2 length must be a positive integer."); + throw new IllegalArgumentException(String.format("PBKDF2 length must be a positive integer. Got: %d", length)); } return new PBKDF2PasswordHasher(pbkdf2Function, iterations, length); } diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 5a13d54ffb..11b3ac48ac 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -157,9 +157,9 @@ public class ConfigConstants { public static final String SECURITY_PASSWORD_HASHING_ALGORITHM = "plugins.security.password.hashing.algorithm"; public static final String SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT = BCRYPT; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS = "plugins.security.password.hashing.pbkdf2.iterations"; - public static final int SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT = 600000; + public static final int SECURITY_PASSWORD_HASHING_PBKDF2_ITERATIONS_DEFAULT = 600_000; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH = "plugins.security.password.hashing.pbkdf2.length"; - public static final int SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT = 512; + public static final int SECURITY_PASSWORD_HASHING_PBKDF2_LENGTH_DEFAULT = 256; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION = "plugins.security.password.hashing.pbkdf2.function"; public static final String SECURITY_PASSWORD_HASHING_PBKDF2_FUNCTION_DEFAULT = Hmac.SHA256.name(); diff --git a/src/main/java/org/opensearch/security/tools/Hasher.java b/src/main/java/org/opensearch/security/tools/Hasher.java index 8fd337b9bc..21ad0a62df 100644 --- a/src/main/java/org/opensearch/security/tools/Hasher.java +++ b/src/main/java/org/opensearch/security/tools/Hasher.java @@ -28,13 +28,7 @@ import java.io.Console; -import org.apache.commons.cli.CommandLine; -import org.apache.commons.cli.CommandLineParser; -import org.apache.commons.cli.DefaultParser; -import org.apache.commons.cli.HelpFormatter; -import org.apache.commons.cli.Option; -import org.apache.commons.cli.Options; -import org.apache.commons.cli.ParseException; +import org.apache.commons.cli.*; import org.opensearch.common.settings.Settings; import org.opensearch.security.hasher.PasswordHasher; @@ -51,15 +45,22 @@ public class Hasher { private static final String LENGTH_OPTION = "l"; private static final String ITERATIONS_OPTION = "i"; private static final String MINOR_OPTION = "min"; + private static final String HELP_OPTION = "h"; public static void main(final String[] args) { final HelpFormatter formatter = new HelpFormatter(); + formatter.setOptionComparator(null); Options options = buildOptions(); final CommandLineParser parser = new DefaultParser(); try { final CommandLine line = parser.parse(options, args); final char[] password; + if (line.hasOption(HELP_OPTION)) { + formatter.printHelp("hash.sh", options, true); + System.exit(0); + } + if (line.hasOption(PASSWORD_OPTION)) { password = line.getOptionValue(PASSWORD_OPTION).toCharArray(); } else if (line.hasOption(ENV_OPTION)) { @@ -147,6 +148,7 @@ private static Settings getPBKDF2Settings(CommandLine line) throws ParseExceptio private static Options buildOptions() { final Options options = new Options(); + options.addOption(Option.builder(HELP_OPTION).longOpt("help").desc("Display the help information").argName("help").build()); options.addOption( Option.builder(PASSWORD_OPTION).longOpt("password").argName("password").hasArg().desc("Cleartext password to hash").build() ); @@ -162,45 +164,52 @@ private static Options buildOptions() { .longOpt("algorithm") .argName("hashing algorithm") .hasArg() - .desc("Hashing algorithm (BCrypt, PBKDF2)") + .desc("Algorithm to use for password hashing. Valid values are: BCrypt | PBKDF2. Default: BCrypt") .build() ); options.addOption( Option.builder(ROUNDS_OPTION) .longOpt("rounds") - .desc("Number of rounds (for BCrypt).") + .desc("Number of rounds to use in logarithmic form. Valid values are: 4 to 31. Default: 12") .hasArg() - .argName("rounds") + .argName("rounds (BCrypt)") .type(Number.class) .build() ); options.addOption( - Option.builder(MINOR_OPTION).longOpt("minor").desc("Minor version (for BCrypt).").hasArg().argName("minor").build() + Option.builder(MINOR_OPTION) + .longOpt("minor") + .desc("Version of BCrypt algorithm to use. Valid values are: A | B | Y. Default: Y") + .hasArg() + .argName("minor (BCrypt)") + .build() ); options.addOption( Option.builder(LENGTH_OPTION) .longOpt("length") - .desc("Desired length of the final derived key (for PBKDF2).") + .desc("Desired length of the final derived key. Default: 256") .hasArg() - .argName("length") + .argName("length (PBKDF2)") .type(Number.class) .build() ); options.addOption( - Option.builder(ITERATIONS_OPTION) - .longOpt("iterations") - .desc("Iterations to perform (for PBKDF2).") + Option.builder(FUNCTION_OPTION) + .longOpt("function") + .desc( + "Pseudo-random function applied to the password. Valid values are SHA1 | SHA224 | SHA256 | SHA384 | SHA512. Default: SHA256" + ) .hasArg() - .argName("iterations") - .type(Number.class) + .argName("function (PBDKF2)") .build() ); options.addOption( - Option.builder(FUNCTION_OPTION) - .longOpt("function") - .desc("Pseudo-random function applied to the password (for PBKDF2).") + Option.builder(ITERATIONS_OPTION) + .longOpt("iterations") + .desc("Number of times the pseudo-random function is applied to the password. Default: 600000") .hasArg() - .argName("function") + .argName("iterations (PBKDF2)") + .type(Number.class) .build() ); return options; diff --git a/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java b/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java index 701c205843..ee950f1058 100644 --- a/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java +++ b/src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java @@ -60,4 +60,5 @@ public void shouldGenerateAValidHashForParameters() { assertThat(hasher.check(password.toCharArray(), hash), is(true)); assertThat(hasher.check(wrongPassword.toCharArray(), hash), is(false)); } + } diff --git a/src/test/java/org/opensearch/security/tools/HasherTests.java b/src/test/java/org/opensearch/security/tools/HasherTests.java index 295fd3df08..9b6ab8028a 100644 --- a/src/test/java/org/opensearch/security/tools/HasherTests.java +++ b/src/test/java/org/opensearch/security/tools/HasherTests.java @@ -83,7 +83,7 @@ public void testWithPBKDF2DefaultArguments() { CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); - assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 256); } @Test @@ -92,14 +92,14 @@ public void testWithPBKDF2FunctionArgument() { CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA512"); assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); - assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 256); out.reset(); Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-f", "SHA384" }); pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA384"); assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 600000); - assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 256); } @Test @@ -108,14 +108,14 @@ public void testWithPBKDF2IterationsArgument() { CompressedPBKDF2Function pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 100000); - assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 256); out.reset(); Hasher.main(new String[] { "-p", "password", "-a", "PBKDF2", "-i", "200000" }); pbkdf2Function = CompressedPBKDF2Function.getInstanceFromHash(out.toString()); assertEquals("should return a valid PBKDF2 hash with the correct value for \"function\"", pbkdf2Function.getAlgorithm(), "SHA256"); assertEquals("should return a valid PBKDF2 hash with the default value for \"iterations\"", pbkdf2Function.getIterations(), 200000); - assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 512); + assertEquals("should return a valid PBKDF2 hash with the default value for \"length\"", pbkdf2Function.getLength(), 256); } @Test From 78236897455339df1b6155b92d7649db5c675fa9 Mon Sep 17 00:00:00 2001 From: Dan Cecoi Date: Thu, 11 Jul 2024 10:13:43 +0100 Subject: [PATCH 13/13] implemented required changes as per review: - removed unnecessary instantiation of PasswordHasher in tests - improved the exception message when password hashing alg is not supported Signed-off-by: Dan Cecoi --- .../security/api/AbstractApiIntegrationTest.java | 8 ++++---- .../security/api/InternalUsersRestApiIntegrationTest.java | 7 ------- .../opensearch/security/hasher/PasswordHasherFactory.java | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java index 1f474dbc5a..868038e9dc 100644 --- a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java @@ -79,6 +79,10 @@ public abstract class AbstractApiIntegrationTest extends RandomizedTest { public static final ToXContentObject EMPTY_BODY = (builder, params) -> builder.startObject().endObject(); + public static final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( + Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() + ); + public static Path configurationFolder; public static ImmutableMap.Builder clusterSettings = ImmutableMap.builder(); @@ -87,10 +91,6 @@ public abstract class AbstractApiIntegrationTest extends RandomizedTest { public static LocalCluster localCluster; - public static PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( - Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() - ); - @BeforeClass public static void startCluster() throws IOException { configurationFolder = ConfigurationFiles.createConfigurationDirectory(); diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java index 0a35793173..ab95877cfd 100644 --- a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java @@ -27,15 +27,11 @@ import org.junit.Assert; import org.junit.Test; -import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.Strings; import org.opensearch.core.xcontent.ToXContentObject; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.dlic.rest.api.Endpoint; -import org.opensearch.security.hasher.PasswordHasher; -import org.opensearch.security.hasher.PasswordHasherFactory; -import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.TestRestClient; import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; @@ -61,9 +57,6 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi private final static String SOME_ROLE = "some-role"; - private final PasswordHasher passwordHasher = PasswordHasherFactory.createPasswordHasher( - Settings.builder().put(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.BCRYPT).build() - ); static { testSecurityConfig.withRestAdminUser(REST_API_ADMIN_INTERNAL_USERS_ONLY, restAdminPermission(Endpoint.INTERNALUSERS)) .user(new TestSecurityConfig.User(SERVICE_ACCOUNT_USER).attr("service", "true").attr("enabled", "true")) diff --git a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java index dd98105ec6..ad2dc7eb57 100644 --- a/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java +++ b/src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java @@ -38,7 +38,7 @@ public static PasswordHasher createPasswordHasher(Settings settings) { passwordHasher = getPBKDF2Hasher(settings); break; default: - throw new IllegalArgumentException("Password hashing algorithm not supported"); + throw new IllegalArgumentException(String.format("Password hashing algorithm '%s' not supported.", algorithm)); } return passwordHasher; }