Skip to content

Commit

Permalink
Configurable password hashing algorithm/cost(#31234) (#32092)
Browse files Browse the repository at this point in the history
* Configurable password hashing algorithm/cost (#31234)

Make password hashing algorithm/cost configurable for the
stored passwords of users for the realms that this applies
(native, reserved). Replaces predefined choice of bcrypt with
cost factor 10.
This also introduces PBKDF2 with configurable cost
(number of iterations) as an algorithm option for password hashing
both for storing passwords and for the user cache.
Password hash validation algorithm selection takes into
consideration the stored hash prefix and only a specific number
of algorithnm and cost factor options for brypt and pbkdf2 are
whitelisted and can be selected in the relevant setting.

* resolveHasher defaults to NOOP (#31723)

This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
 #31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. Hash verification will subsequently
fail for malformed hashes.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

* Fix RealmInteg test failures

As part of the changes in #31234,the password verification logic
determines the algorithm used for hashing the password from the
format of the stored password hash itself. Thus, it is generally
possible to validate a password even if it's associated stored hash
was not created with the same algorithm than the one currently set
in the settings.
At the same time, we introduced a check for incoming client change
password requests to make sure that the request's password is hashed
with the same algorithm that is configured to be used in the node
settings.
In the spirit of randomizing the algorithms used, the
{@code SecurityClient} used in the {@code NativeRealmIntegTests} and
{@code ReservedRealmIntegTests} would send all requests dealing with
user passwords by randomly selecting a hashing algorithm each time.
This meant that some change password requests were using a different
password hashing algorithm than the one used for the node and the
request would fail.
This commit changes this behavior in the two aforementioned Integ
tests to use the same password hashing algorithm for the node and the
clients, no matter what the request is.
  • Loading branch information
jkakavas authored Jul 18, 2018
1 parent eed8dc9 commit ba0ecae
Show file tree
Hide file tree
Showing 61 changed files with 1,037 additions and 410 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,18 @@ public static Setting<String> simpleString(String key, Validator<String> validat
return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties);
}

/**
* Creates a new Setting instance with a String value
*
* @param key the settings key for this setting.
* @param defaultValue the default String value.
* @param properties properties for this setting like scope, filtering...
* @return the Setting Object
*/
public static Setting<String> simpleString(String key, String defaultValue, Property... properties) {
return new Setting<>(key, s -> defaultValue, Function.identity(), properties);
}

public static int parseInt(String s, int minValue, String key) {
return parseInt(s, minValue, Integer.MAX_VALUE, key);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.xpack.core.security.SecurityField;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.ssl.SSLClientAuth;
import org.elasticsearch.xpack.core.ssl.SSLConfigurationSettings;
import org.elasticsearch.xpack.core.ssl.VerificationMode;
Expand All @@ -20,13 +21,20 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.function.Function;

import static org.elasticsearch.xpack.core.security.SecurityField.USER_SETTING;

/**
* A container for xpack setting constants.
*/
public class XPackSettings {

private XPackSettings() {
throw new IllegalStateException("Utility class should not be instantiated");
}

/** Setting for enabling or disabling security. Defaults to true. */
public static final Setting<Boolean> SECURITY_ENABLED = Setting.boolSetting("xpack.security.enabled", true, Setting.Property.NodeScope);

Expand Down Expand Up @@ -113,6 +121,17 @@ public class XPackSettings {
DEFAULT_CIPHERS = ciphers;
}

/*
* Do not allow insecure hashing algorithms to be used for password hashing
*/
public static final Setting<String> 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.");
}
}, Setting.Property.NodeScope);

public static final List<String> DEFAULT_SUPPORTED_PROTOCOLS = Arrays.asList("TLSv1.2", "TLSv1.1", "TLSv1");
public static final SSLClientAuth CLIENT_AUTH_DEFAULT = SSLClientAuth.REQUIRED;
public static final SSLClientAuth HTTP_CLIENT_AUTH_DEFAULT = SSLClientAuth.NONE;
Expand Down Expand Up @@ -151,6 +170,7 @@ public static List<Setting<?>> getAllSettings() {
settings.add(SQL_ENABLED);
settings.add(USER_SETTING);
settings.add(ROLLUP_ENABLED);
settings.add(PASSWORD_HASHING_ALGORITHM);
return Collections.unmodifiableList(settings);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,22 @@ public ChangePasswordRequestBuilder username(String username) {
return this;
}

public static char[] validateAndHashPassword(SecureString password) {
public static char[] validateAndHashPassword(SecureString password, Hasher hasher) {
Validation.Error error = Validation.Users.validatePassword(password.getChars());
if (error != null) {
ValidationException validationException = new ValidationException();
validationException.addValidationError(error.toString());
throw validationException;
}
return Hasher.BCRYPT.hash(password);
return hasher.hash(password);
}

/**
* Sets the password. Note: the char[] passed to this method will be cleared.
*/
public ChangePasswordRequestBuilder password(char[] password) {
public ChangePasswordRequestBuilder password(char[] password, Hasher hasher) {
try (SecureString secureString = new SecureString(password)) {
char[] hash = validateAndHashPassword(secureString);
char[] hash = validateAndHashPassword(secureString, hasher);
request.passwordHash(hash);
}
return this;
Expand All @@ -65,7 +65,8 @@ public ChangePasswordRequestBuilder password(char[] password) {
/**
* Populate the change password request from the source in the provided content type
*/
public ChangePasswordRequestBuilder source(BytesReference source, XContentType xContentType) throws IOException {
public ChangePasswordRequestBuilder source(BytesReference source, XContentType xContentType, Hasher hasher) throws
IOException {
// EMPTY is ok here because we never call namedObject
try (InputStream stream = source.streamInput();
XContentParser parser = xContentType.xContent()
Expand All @@ -80,7 +81,7 @@ public ChangePasswordRequestBuilder source(BytesReference source, XContentType x
if (token == XContentParser.Token.VALUE_STRING) {
String password = parser.text();
final char[] passwordChars = password.toCharArray();
password(passwordChars);
password(passwordChars, hasher);
assert CharBuffer.wrap(passwordChars).chars().noneMatch((i) -> (char) i != (char) 0) : "expected password to " +
"clear the char[] but it did not!";
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.elasticsearch.action.ActionRequestBuilder;
import org.elasticsearch.action.support.WriteRequestBuilder;
import org.elasticsearch.client.ElasticsearchClient;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.ValidationException;
import org.elasticsearch.common.bytes.BytesReference;
Expand All @@ -33,8 +32,6 @@
public class PutUserRequestBuilder extends ActionRequestBuilder<PutUserRequest, PutUserResponse, PutUserRequestBuilder>
implements WriteRequestBuilder<PutUserRequestBuilder> {

private final Hasher hasher = Hasher.BCRYPT;

public PutUserRequestBuilder(ElasticsearchClient client) {
this(client, PutUserAction.INSTANCE);
}
Expand All @@ -53,7 +50,7 @@ public PutUserRequestBuilder roles(String... roles) {
return this;
}

public PutUserRequestBuilder password(@Nullable char[] password) {
public PutUserRequestBuilder password(char[] password, Hasher hasher) {
if (password != null) {
Validation.Error error = Validation.Users.validatePassword(password);
if (error != null) {
Expand Down Expand Up @@ -96,7 +93,8 @@ public PutUserRequestBuilder enabled(boolean enabled) {
/**
* Populate the put user request using the given source and username
*/
public PutUserRequestBuilder source(String username, BytesReference source, XContentType xContentType) throws IOException {
public PutUserRequestBuilder source(String username, BytesReference source, XContentType xContentType, Hasher hasher) throws
IOException {
Objects.requireNonNull(xContentType);
username(username);
// EMPTY is ok here because we never call namedObject
Expand All @@ -113,7 +111,7 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon
if (token == XContentParser.Token.VALUE_STRING) {
String password = parser.text();
char[] passwordChars = password.toCharArray();
password(passwordChars);
password(passwordChars, hasher);
Arrays.fill(passwordChars, (char) 0);
} else {
throw new ElasticsearchParseException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
import java.util.Set;

public final class CachingUsernamePasswordRealmSettings {
public static final Setting<String> CACHE_HASH_ALGO_SETTING = Setting.simpleString("cache.hash_algo", Setting.Property.NodeScope);
public static final Setting<String> CACHE_HASH_ALGO_SETTING = Setting.simpleString("cache.hash_algo", "ssha256",
Setting.Property.NodeScope);
private static final TimeValue DEFAULT_TTL = TimeValue.timeValueMinutes(20);
public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting("cache.ttl", DEFAULT_TTL, Setting.Property.NodeScope);
private static final int DEFAULT_MAX_USERS = 100_000; //100k users
Expand Down
Loading

0 comments on commit ba0ecae

Please sign in to comment.