Skip to content

Commit

Permalink
Ensure SHA256 is not used in tests (#42289)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jkakavas committed May 22, 2019
1 parent cdf9485 commit 34dda75
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 7 deletions.
17 changes: 17 additions & 0 deletions x-pack/plugin/core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ 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 {
if (project.path == ":libs:core") {
main.java.srcDirs = ['java']
main.resources.srcDirs = ['resources']
} else {
test.java.srcDirs = ['java']
test.resources.srcDirs = ['resources']
}
}
}

compileJava.options.compilerArgs << "-Xlint:-rawtypes,-unchecked"
compileTestJava.options.compilerArgs << "-Xlint:-rawtypes,-unchecked"

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugin/core/forbidden/hasher-signatures.txt
Original file line number Diff line number Diff line change
@@ -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()
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -565,12 +566,25 @@ 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<String> getAvailableAlgoStoredHash() {
return Arrays.stream(Hasher.values()).map(Hasher::name).map(name -> name.toLowerCase(Locale.ROOT))
.filter(name -> (name.startsWith("pbkdf2") || name.startsWith("bcrypt")))
.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()}
*/
@SuppressForbidden(reason = "This is the only allowed way to get available values")
public static List<String> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,16 @@
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;

public class RealmSettingsTests extends ESTestCase {
private static final List<String> CACHE_HASHING_ALGOS = Arrays.stream(Hasher.values()).map(Hasher::name).collect(Collectors.toList());
private static final List<String> CACHE_HASHING_ALGOS = Hasher.getAvailableAlgoCacheHash();

public void testRealmWithBlankTypeDoesNotValidate() throws Exception {
final Settings.Builder builder = baseSettings(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -94,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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -66,7 +65,7 @@ public void stop() {

@AwaitsFix(bugUrl="https://github.com/elastic/elasticsearch/issues/42267")
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");
Expand Down

0 comments on commit 34dda75

Please sign in to comment.