diff --git a/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java b/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java index af6e563897c..a8a96a57941 100644 --- a/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java +++ b/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java @@ -685,8 +685,15 @@ public class BCrypt { */ - // the next line is the SecuredString replacement for the above commented-out section - passwordb = ( minor >= 'a' ? password.concat("\000"): password ).utf8Bytes(); + // the next lines are the SecuredString replacement for the above commented-out section + if (minor >= 'a') { + SecuredString securedString = password.concat("\000"); + passwordb = securedString.utf8Bytes(); + // clear here since this is a new object and we don't need to reuse it + securedString.clear(); + } else { + passwordb = password.utf8Bytes(); + } /*************************** ES CHANGE END *************************/ saltb = decode_base64(real_salt, BCRYPT_SALT_LEN); diff --git a/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java b/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java index f8eb8e5dd39..d317cc1aab5 100644 --- a/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java +++ b/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java @@ -24,8 +24,7 @@ public class SecuredString implements CharSequence { private boolean cleared = false; /** - * Note: the passed in chars are not duplicated, but used directly for performance/optimization. DO NOT - * modify or clear the chars after it has been passed into this constructor. + * Note: the passed in chars are duplicated */ public SecuredString(char[] chars) { this.chars = new char[chars.length]; @@ -33,8 +32,7 @@ public class SecuredString implements CharSequence { } /** - * This constructor is used internally for the concatenate method. It DOES duplicate the passed in array, unlike - * the public constructor + * This constructor is used internally for the concatenate method. */ private SecuredString(char[] chars, int start, int end) { this.chars = new char[end - start]; @@ -89,7 +87,7 @@ public class SecuredString implements CharSequence { } /** - * @return A copy of the internal charachters. May be usd for caching. + * @return A copy of the internal characters. May be used for caching. */ public char[] copyChars() { return Arrays.copyOf(chars, chars.length); @@ -129,12 +127,6 @@ public class SecuredString implements CharSequence { Arrays.fill(chars, (char) 0); } - @Override - public void finalize() throws Throwable { - clear(); - super.finalize(); - } - /** * @param toAppend String to combine with this SecureString * @return a new SecureString with toAppend concatenated diff --git a/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java b/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java new file mode 100644 index 00000000000..0f5c3d7763c --- /dev/null +++ b/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java @@ -0,0 +1,70 @@ +/* + * 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.shield.authc.support; + +import org.elasticsearch.test.ESTestCase; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.hamcrest.Matchers.is; + +/** + * Tests for the Bcrypt implementation specifically around modifications we have made + */ +public class BCryptTests extends ESTestCase { + + + /* + * This test checks that the BCrypt implementation can verify passwords correctly when being invoked from multiple + * threads all the time. This attempts to simulate authentication of many clients at once (without a cache). + * + * This test can be used to reproduce the issue in https://github.com/elastic/x-plugins/issues/589, but it is not + * 100% reliable unless memory parameters are changed such as lowering the heap size to something really small like + * 16M and the test is really slow since the issue depends on garbage collection and object finalization. + */ + @Test + @AwaitsFix(bugUrl = "need a better way to test this") + public void testUnderLoad() throws Exception { + final String password = randomAsciiOfLengthBetween(10, 32); + final String bcrypt = BCrypt.hashpw(SecuredStringTests.build(password), BCrypt.gensalt()); + + ExecutorService threadPoolExecutor = Executors.newFixedThreadPool(100); + try { + List> callables = new ArrayList<>(100); + + final AtomicBoolean failed = new AtomicBoolean(false); + for (int i = 0; i < 100; i++) { + callables.add(new Callable() { + @Override + public Boolean call() throws Exception { + for (int i = 0; i < 10000 && !failed.get(); i++) { + if (BCrypt.checkpw(SecuredStringTests.build(password), bcrypt) == false) { + failed.set(true); + return false; + } + } + return true; + } + }); + } + + List> futures = threadPoolExecutor.invokeAll(callables); + for (Future future : futures) { + assertThat(future.get(), is(true)); + } + } finally { + threadPoolExecutor.shutdownNow(); + } + + } +}