From ac5ef8c3892207e65ca6ecb1be331c0cbf566c55 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Tue, 14 Aug 2018 13:34:22 -0600 Subject: [PATCH] Security: remove password hash bootstrap check (#32440) This change removes the PasswordHashingBootstrapCheck and replaces it with validation on the setting itself. This ensures we always get a valid value from the setting when it is used. --- .../xpack/core/XPackSettings.java | 13 +++++- .../xpack/core/XPackSettingsTests.java | 31 +++++++++++++ ...asswordHashingAlgorithmBootstrapCheck.java | 41 ----------------- .../xpack/security/Security.java | 1 - ...rdHashingAlgorithmBootstrapCheckTests.java | 44 ------------------- .../authc/esnative/ReservedRealmTests.java | 2 +- 6 files changed, 43 insertions(+), 89 deletions(-) delete mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheck.java delete mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheckTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java index 1a80c6d873b..fb4ce0b90f4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.core.ssl.SSLConfigurationSettings; import org.elasticsearch.xpack.core.ssl.VerificationMode; import javax.crypto.Cipher; +import javax.crypto.SecretKeyFactory; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; @@ -127,8 +128,16 @@ public class XPackSettings { public static final Setting PASSWORD_HASHING_ALGORITHM = new Setting<>( "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), (v, s) -> { if (Hasher.getAvailableAlgoStoredHash().contains(v.toLowerCase(Locale.ROOT)) == false) { - throw new IllegalArgumentException("Invalid algorithm: " + v + ". Only pbkdf2 or bcrypt family algorithms can be used for " + - "password hashing."); + throw new IllegalArgumentException("Invalid algorithm: " + v + ". Valid values for password hashing are " + + Hasher.getAvailableAlgoStoredHash().toString()); + } else if (v.regionMatches(true, 0, "pbkdf2", 0, "pbkdf2".length())) { + try { + SecretKeyFactory.getInstance("PBKDF2withHMACSHA512"); + } catch (NoSuchAlgorithmException e) { + throw new IllegalArgumentException( + "Support for PBKDF2WithHMACSHA512 must be available in order to use any of the " + + "PBKDF2 algorithms for the [xpack.security.authc.password_hashing.algorithm] setting.", e); + } } }, Setting.Property.NodeScope); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackSettingsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackSettingsTests.java index 17934efe0a5..7689ae4088f 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackSettingsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackSettingsTests.java @@ -5,9 +5,14 @@ */ package org.elasticsearch.xpack.core; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; import javax.crypto.Cipher; +import javax.crypto.SecretKeyFactory; +import java.security.NoSuchAlgorithmException; + +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.not; @@ -25,4 +30,30 @@ public class XPackSettingsTests extends ESTestCase { assertThat(XPackSettings.DEFAULT_CIPHERS, not(hasItem("TLS_RSA_WITH_AES_256_CBC_SHA"))); } } + + public void testPasswordHashingAlgorithmSettingValidation() { + final boolean isPBKDF2Available = isSecretkeyFactoryAlgoAvailable("PBKDF2WithHMACSHA512"); + final String pbkdf2Algo = randomFrom("PBKDF2_10000", "PBKDF2"); + final Settings settings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), pbkdf2Algo).build(); + if (isPBKDF2Available) { + assertEquals(pbkdf2Algo, XPackSettings.PASSWORD_HASHING_ALGORITHM.get(settings)); + } else { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> XPackSettings.PASSWORD_HASHING_ALGORITHM.get(settings)); + assertThat(e.getMessage(), containsString("Support for PBKDF2WithHMACSHA512 must be available")); + } + + final String bcryptAlgo = randomFrom("BCRYPT", "BCRYPT11"); + assertEquals(bcryptAlgo, XPackSettings.PASSWORD_HASHING_ALGORITHM.get( + Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), bcryptAlgo).build())); + } + + private boolean isSecretkeyFactoryAlgoAvailable(String algorithmId) { + try { + SecretKeyFactory.getInstance(algorithmId); + return true; + } catch (NoSuchAlgorithmException e) { + return false; + } + } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheck.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheck.java deleted file mode 100644 index c60c2ea18d0..00000000000 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheck.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.security; - -import org.elasticsearch.bootstrap.BootstrapCheck; -import org.elasticsearch.bootstrap.BootstrapContext; -import org.elasticsearch.xpack.core.XPackSettings; - -import javax.crypto.SecretKeyFactory; -import java.security.NoSuchAlgorithmException; -import java.util.Locale; - -/** - * Bootstrap check to ensure that one of the allowed password hashing algorithms is - * selected and that it is available. - */ -public class PasswordHashingAlgorithmBootstrapCheck implements BootstrapCheck { - @Override - public BootstrapCheckResult check(BootstrapContext context) { - final String selectedAlgorithm = XPackSettings.PASSWORD_HASHING_ALGORITHM.get(context.settings); - if (selectedAlgorithm.toLowerCase(Locale.ROOT).startsWith("pbkdf2")) { - try { - SecretKeyFactory.getInstance("PBKDF2withHMACSHA512"); - } catch (NoSuchAlgorithmException e) { - final String errorMessage = String.format(Locale.ROOT, - "Support for PBKDF2WithHMACSHA512 must be available in order to use any of the " + - "PBKDF2 algorithms for the [%s] setting.", XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey()); - return BootstrapCheckResult.failure(errorMessage); - } - } - return BootstrapCheckResult.success(); - } - - @Override - public boolean alwaysEnforce() { - return true; - } -} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 18bcfeb94a5..d31ffae13f2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -300,7 +300,6 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw new TokenSSLBootstrapCheck(), new PkiRealmBootstrapCheck(getSslService()), new TLSLicenseBootstrapCheck(), - new PasswordHashingAlgorithmBootstrapCheck(), new FIPS140SecureSettingsBootstrapCheck(settings, env), new FIPS140JKSKeystoreBootstrapCheck(settings), new FIPS140PasswordHashingAlgorithmBootstrapCheck(settings))); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheckTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheckTests.java deleted file mode 100644 index 8ca5c6c7216..00000000000 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/PasswordHashingAlgorithmBootstrapCheckTests.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.security; - -import org.elasticsearch.bootstrap.BootstrapContext; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.XPackSettings; - -import javax.crypto.SecretKeyFactory; -import java.security.NoSuchAlgorithmException; - -public class PasswordHashingAlgorithmBootstrapCheckTests extends ESTestCase { - - public void testPasswordHashingAlgorithmBootstrapCheck() { - Settings settings = Settings.EMPTY; - assertFalse(new PasswordHashingAlgorithmBootstrapCheck().check(new BootstrapContext(settings, null)).isFailure()); - // The following two will always pass because for now we only test in environments where PBKDF2WithHMACSHA512 is supported - assertTrue(isSecretkeyFactoryAlgoAvailable("PBKDF2WithHMACSHA512")); - settings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), "PBKDF2_10000").build(); - assertFalse(new PasswordHashingAlgorithmBootstrapCheck().check(new BootstrapContext(settings, null)).isFailure()); - - settings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), "PBKDF2").build(); - assertFalse(new PasswordHashingAlgorithmBootstrapCheck().check(new BootstrapContext(settings, null)).isFailure()); - - settings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), "BCRYPT").build(); - assertFalse(new PasswordHashingAlgorithmBootstrapCheck().check(new BootstrapContext(settings, null)).isFailure()); - - settings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), "BCRYPT11").build(); - assertFalse(new PasswordHashingAlgorithmBootstrapCheck().check(new BootstrapContext(settings, null)).isFailure()); - } - - private boolean isSecretkeyFactoryAlgoAvailable(String algorithmId) { - try { - SecretKeyFactory.getInstance(algorithmId); - return true; - } catch (NoSuchAlgorithmException e) { - return false; - } - } -} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmTests.java index a03f30abc33..04e0afcf882 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmTests.java @@ -83,7 +83,7 @@ public class ReservedRealmTests extends ESTestCase { IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> new ReservedRealm(mock(Environment.class), invalidSettings, usersStore, new AnonymousUser(Settings.EMPTY), securityIndex, threadPool)); assertThat(exception.getMessage(), containsString(invalidAlgoId)); - assertThat(exception.getMessage(), containsString("Only pbkdf2 or bcrypt family algorithms can be used for password hashing")); + assertThat(exception.getMessage(), containsString("Invalid algorithm")); } public void testReservedUserEmptyPasswordAuthenticationFails() throws Throwable {