From 1380827b8309038e1e2fdf9b9ece9b1c57915f14 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 21 May 2019 11:39:22 +0300 Subject: [PATCH 1/3] Ensure SHA256 is not used in tests SHA256 was recently added to the Hasher class in order to be used in the TokenService. A few tests were still using values() to get the available algorithms from the Enum and it could happen that SHA256 would be picked up by these. This change adds an extra convenience method (Hasher#getAvailableAlgoCacheHash) and enures that only this and Hasher#getAvailableAlgoStoredHash are used for getting the list of available password hashing algorithms in our tests. --- .../xpack/core/security/authc/support/Hasher.java | 11 +++++++++++ .../xpack/security/authc/RealmSettingsTests.java | 2 +- .../xpack/security/authc/file/FileRealmTests.java | 2 +- .../support/CachingUsernamePasswordRealmTests.java | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java index 28f263748135f..0870cc520c86e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java @@ -571,6 +571,17 @@ public static List getAvailableAlgoStoredHash() { .collect(Collectors.toList()); } + /** + * Returns a list of lower case String identifiers for the Hashing algorithm and parameter + * combinations that can be used for password hashing in the cache. The identifiers can be used to get + * an instance of the appropriate {@link Hasher} by using {@link #resolve(String) resolve()} + */ + public static List getAvailableAlgoCacheHash() { + return Arrays.stream(Hasher.values()).map(Hasher::name).map(name -> name.toLowerCase(Locale.ROOT)) + .filter(name -> (name.equals("sha256") == false)) + .collect(Collectors.toList()); + } + public abstract char[] hash(SecureString data); public abstract boolean verify(SecureString data, char[] hash); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java index 7d7fd135349b1..ba8bb945b9658 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java @@ -28,7 +28,7 @@ import static org.hamcrest.Matchers.notNullValue; public class RealmSettingsTests extends ESTestCase { - private static final List CACHE_HASHING_ALGOS = Arrays.stream(Hasher.values()).map(Hasher::name).collect(Collectors.toList()); + private static final List CACHE_HASHING_ALGOS = Hasher.getAvailableAlgoCacheHash(); public void testRealmWithBlankTypeDoesNotValidate() throws Exception { final Settings.Builder builder = baseSettings(false); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java index 9d05495449805..967f302e87d26 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java @@ -93,7 +93,7 @@ private RealmConfig getRealmConfig(Settings settings) { public void testAuthenticateCaching() throws Exception { Settings settings = Settings.builder() .put(RealmSettings.realmSettingPrefix(REALM_IDENTIFIER) + "cache.hash_algo", - Hasher.values()[randomIntBetween(0, Hasher.values().length - 1)].name().toLowerCase(Locale.ROOT)) + randomFrom(Hasher.getAvailableAlgoCacheHash())) .put(globalSettings) .build(); RealmConfig config = getRealmConfig(settings); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java index 2fed720e23c09..efe7a11d2acd7 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java @@ -65,7 +65,7 @@ public void stop() { } public void testCacheSettings() { - String cachingHashAlgo = Hasher.values()[randomIntBetween(0, Hasher.values().length - 1)].name().toLowerCase(Locale.ROOT); + String cachingHashAlgo = randomFrom(Hasher.getAvailableAlgoCacheHash()); int maxUsers = randomIntBetween(10, 100); TimeValue ttl = TimeValue.timeValueMinutes(randomIntBetween(10, 20)); final RealmConfig.RealmIdentifier identifier = new RealmConfig.RealmIdentifier("caching", "test_realm"); From edf72ad1bc4c523ff0901d22f3e1cddfccac3e83 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 21 May 2019 16:11:34 +0300 Subject: [PATCH 2/3] Remove unused imports --- .../elasticsearch/xpack/security/authc/RealmSettingsTests.java | 2 -- .../elasticsearch/xpack/security/authc/file/FileRealmTests.java | 1 - .../authc/support/CachingUsernamePasswordRealmTests.java | 1 - 3 files changed, 4 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java index ba8bb945b9658..eb33408f338c6 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmSettingsTests.java @@ -17,12 +17,10 @@ import org.elasticsearch.xpack.core.security.authc.RealmSettings; import org.elasticsearch.xpack.core.security.authc.support.Hasher; -import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.stream.Collectors; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.notNullValue; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java index 967f302e87d26..d2ab879d4d4ff 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java @@ -22,7 +22,6 @@ import org.junit.Before; import org.mockito.stubbing.Answer; -import java.util.Locale; import java.util.Map; import java.util.function.Supplier; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java index efe7a11d2acd7..91a0fc9d94e2e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java @@ -30,7 +30,6 @@ import java.util.ArrayList; import java.util.List; -import java.util.Locale; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; From f6a8487333a29df0437a6f898da34e3b8db057a7 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 21 May 2019 17:55:28 +0300 Subject: [PATCH 3/3] Add Hasher#values() to the forbidden APIs --- x-pack/plugin/core/build.gradle | 4 ++++ x-pack/plugin/core/forbidden/hasher-signatures.txt | 2 ++ .../xpack/core/security/authc/support/Hasher.java | 3 +++ 3 files changed, 9 insertions(+) create mode 100644 x-pack/plugin/core/forbidden/hasher-signatures.txt diff --git a/x-pack/plugin/core/build.gradle b/x-pack/plugin/core/build.gradle index c20449724f8e0..d805a491e093a 100644 --- a/x-pack/plugin/core/build.gradle +++ b/x-pack/plugin/core/build.gradle @@ -95,6 +95,10 @@ forbiddenPatterns { exclude '**/*.zip' } +forbiddenApisMain { + signaturesFiles += files('forbidden/hasher-signatures.txt') +} + if (isEclipse) { // in eclipse the project is under a fake root, we need to change around the source sets sourceSets { diff --git a/x-pack/plugin/core/forbidden/hasher-signatures.txt b/x-pack/plugin/core/forbidden/hasher-signatures.txt new file mode 100644 index 0000000000000..98271161096a7 --- /dev/null +++ b/x-pack/plugin/core/forbidden/hasher-signatures.txt @@ -0,0 +1,2 @@ +@defaultMessage values should not be used as it can contain unwanted algorithms. Use Hasher#getAvailableAlgoStoredHash and Hasher#getAvailableAlgoCacheHash instead +org.elasticsearch.xpack.core.security.authc.support.Hasher#values() \ No newline at end of file diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java index 0870cc520c86e..5413a38bd6288 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java @@ -7,6 +7,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.CharArrays; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.hash.MessageDigests; import org.elasticsearch.common.settings.SecureString; @@ -565,6 +566,7 @@ private static boolean verifyBcryptHash(SecureString text, char[] hash) { * combinations that can be used for password hashing. The identifiers can be used to get * an instance of the appropriate {@link Hasher} by using {@link #resolve(String) resolve()} */ + @SuppressForbidden(reason = "This is the only allowed way to get available values") public static List getAvailableAlgoStoredHash() { return Arrays.stream(Hasher.values()).map(Hasher::name).map(name -> name.toLowerCase(Locale.ROOT)) .filter(name -> (name.startsWith("pbkdf2") || name.startsWith("bcrypt"))) @@ -576,6 +578,7 @@ public static List getAvailableAlgoStoredHash() { * combinations that can be used for password hashing in the cache. The identifiers can be used to get * an instance of the appropriate {@link Hasher} by using {@link #resolve(String) resolve()} */ + @SuppressForbidden(reason = "This is the only allowed way to get available values") public static List getAvailableAlgoCacheHash() { return Arrays.stream(Hasher.values()).map(Hasher::name).map(name -> name.toLowerCase(Locale.ROOT)) .filter(name -> (name.equals("sha256") == false))