Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Configurable password hashing algorithm/cost #31234

Merged
merged 31 commits into from
Jun 28, 2018
Merged

Configurable password hashing algorithm/cost #31234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a02c3dd
Make password hashing algorithm/cost configurable
jkakavas Jun 10, 2018
feafc97
Rework and test default cost
jkakavas Jun 10, 2018
ac10583
Remove irrelevant test
jkakavas Jun 10, 2018
99a7f1c
Remove * import
jkakavas Jun 10, 2018
f97d866
Fix a few more tests
jkakavas Jun 10, 2018
3625713
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 13, 2018
3635c11
Addresses feedback
jkakavas Jun 14, 2018
ede105f
Revert unnecessary scope changes
jkakavas Jun 14, 2018
5577013
Adjust tests
jkakavas Jun 14, 2018
3ef1ea1
Remove unecessary setting constructor
jkakavas Jun 14, 2018
47f911f
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 14, 2018
4fad6cf
remove unused imports
jkakavas Jun 14, 2018
51ee743
fix leftover test
jkakavas Jun 15, 2018
3305765
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 15, 2018
6d28ef0
Handle unknown hash formats with error
jkakavas Jun 15, 2018
ef3dc4c
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 15, 2018
038bc6a
Handle invalid hash during validation
jkakavas Jun 17, 2018
0ab9cb0
Handle invalid hash during validation
jkakavas Jun 18, 2018
5e537fe
Addresses feedback
jkakavas Jun 19, 2018
44949ca
Address feedback
jkakavas Jun 19, 2018
15cbf2d
Address feedback
jkakavas Jun 25, 2018
17d4028
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 25, 2018
e2f429a
Remove unused imports
jkakavas Jun 25, 2018
af85aeb
Address feedback
jkakavas Jun 25, 2018
8cd5ae2
remove now invalid test
jkakavas Jun 26, 2018
c0d33a9
Recomputed test password hashes with correct salt length
jkakavas Jun 26, 2018
0f19d47
Address feedback
jkakavas Jun 27, 2018
622a204
Fix checkstyle
jkakavas Jun 27, 2018
918301c
Addresses final feedback
jkakavas Jun 28, 2018
bb85c20
Reflect change in behavior in tests
jkakavas Jun 28, 2018
b18203c
Merge remote-tracking branch 'origin/master' into configurable-pwd-hash
jkakavas Jun 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions server/src/main/java/org/elasticsearch/common/settings/Setting.java
View file
Open in desktop
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
20 changes: 20 additions & 0 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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 @@ -19,13 +20,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 @@ -105,6 +113,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<>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we discussed in the team meeting or somewhere else. Just adding a comment for discussion if we need to have extensible mechanism via security extensions. If need be customers can use 'Argon2' or 'scrypt' using non-default implementations? Is it too early to support them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part has not been discussed, but it is a good idea for a future enhancement. I'd rather wait on opening this up though; once we do that then it has to be supported by us and everything we add creates overhead. We also do not know if there is demand for this and I'd guess that the majority of users would not have a preference as long as we use something that is secure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was in the context of supporting a FIPS-140 compliant solution, hence only PBKDF2 was added. I also haven't seen any argon2 or scrypt JAVA implementations that have been used / tested sufficiently (there are bindings for the Argon2 C implementation though.. )

I'm definitely not against allowing for extensions with other algorithm implementations, but not in the context of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Jay and Ioannis. I just thought if this was something that we wanted to consider for security extensions. Yes, I would not trust yet not proven fairly new implementations, just wanted to bring it up so we can keep it in our thoughts.

"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 @@ -143,6 +162,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
13 changes: 7 additions & 6 deletions .../java/org/elasticsearch/xpack/core/security/action/user/ChangePasswordRequestBuilder.java
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is breaking the java API, which I think is fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is or is not ? I Can definitely reintroduce the ChangePasswordRequestBuilder password(char[] password) signature and call ChangePasswordRequestBuilder password(char[] password, Hasher hasher) with a default Hasher instance (BCrypt)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the incomplete thought. I think breaking is the right way to go; the issue with a non-breaking change is the default method will be trappy if they configure elasticsearch to use PBKDF2 but we hash with BCrypt on the client side by default. I wonder if we should add validation that the hash is the correct type in TransportChangePasswordAction?

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
10 changes: 4 additions & 6 deletions ...rc/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java
View file
Open in desktop
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>
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
3 changes: 2 additions & 1 deletion ...elasticsearch/xpack/core/security/authc/support/CachingUsernamePasswordRealmSettings.java
View file
Open in desktop
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