-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Configurable password hashing algorithm/cost #31234
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.
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
Changes from 26 commits
a02c3dd
feafc97
ac10583
99a7f1c
f97d866
3625713
3635c11
ede105f
5577013
3ef1ea1
47f911f
4fad6cf
51ee743
3305765
6d28ef0
ef3dc4c
038bc6a
0ab9cb0
5e537fe
44949ca
15cbf2d
17d4028
e2f429a
af85aeb
8cd5ae2
c0d33a9
0f19d47
622a204
918301c
bb85c20
b18203c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
*/ | ||
public static Setting<String> simpleString(String key, String defaultValue, Property... properties) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add javadocs? I know there aren't docs for other methods here but we should be in the habit of adding them for public methods |
||
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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is breaking the java API, which I think is fine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is or is not ? I Can definitely reintroduce the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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() | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To stay consistent with my prev suggestion, here, use "ssha256" as default value. |
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty return in javadocs