Skip to content

Use SecureString for password length validation #42884

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

Merged
merged 3 commits into from
Jun 14, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public ChangePasswordRequestBuilder username(String username) {
}

public static char[] validateAndHashPassword(SecureString password, Hasher hasher) {
Validation.Error error = Validation.Users.validatePassword(password.getChars());
Validation.Error error = Validation.Users.validatePassword(password);
if (error != null) {
ValidationException validationException = new ValidationException();
validationException.addValidationError(error.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.Arrays;
import java.util.Map;
import java.util.Objects;

Expand All @@ -50,7 +49,15 @@ public PutUserRequestBuilder roles(String... roles) {
return this;
}

/**
* @deprecated Use {@link #password(SecureString, Hasher)} instead.
*/
@Deprecated
public PutUserRequestBuilder password(char[] password, Hasher hasher) {
return password(new SecureString(password), hasher);
}

public PutUserRequestBuilder password(SecureString password, Hasher hasher) {
if (password != null) {
Validation.Error error = Validation.Users.validatePassword(password);
if (error != null) {
Expand All @@ -59,7 +66,7 @@ public PutUserRequestBuilder password(char[] password, Hasher hasher) {
if (request.passwordHash() != null) {
throw validationException("password_hash has already been set");
}
request.passwordHash(hasher.hash(new SecureString(password)));
request.passwordHash(hasher.hash(password));
} else {
request.passwordHash(null);
}
Expand Down Expand Up @@ -119,9 +126,9 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon
} else if (User.Fields.PASSWORD.match(currentFieldName, parser.getDeprecationHandler())) {
if (token == XContentParser.Token.VALUE_STRING) {
String password = parser.text();
char[] passwordChars = password.toCharArray();
password(passwordChars, hasher);
Arrays.fill(passwordChars, (char) 0);
try(SecureString securePassword = new SecureString(password.toCharArray())) {
password(securePassword, hasher);
}
} else {
throw new ElasticsearchParseException(
"expected field [{}] to be of type string, but found [{}] instead", currentFieldName, token);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.core.security.support;

import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
Expand Down Expand Up @@ -81,10 +82,10 @@ public static Error validateUsername(String username, boolean allowReserved, Set
return null;
}

public static Error validatePassword(char[] password) {
return password.length >= MIN_PASSWD_LENGTH ?
null :
new Error("passwords must be at least [" + MIN_PASSWD_LENGTH + "] characters long");
public static Error validatePassword(SecureString password) {
return password.length() >= MIN_PASSWD_LENGTH ?
null :
new Error("passwords must be at least [" + MIN_PASSWD_LENGTH + "] characters long");
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.core.security.support;

import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
Expand Down Expand Up @@ -59,12 +60,12 @@ public void testUsernameInvalidWhitespace() throws Exception {
}

public void testUsersValidatePassword() throws Exception {
String passwd = randomAlphaOfLength(randomIntBetween(0, 20));
SecureString passwd = new SecureString(randomAlphaOfLength(randomIntBetween(0, 20)).toCharArray());
logger.info("{}[{}]", passwd, passwd.length());
if (passwd.length() >= 6) {
assertThat(Users.validatePassword(passwd.toCharArray()), nullValue());
assertThat(Users.validatePassword(passwd), nullValue());
} else {
assertThat(Users.validatePassword(passwd.toCharArray()), notNullValue());
assertThat(Users.validatePassword(passwd), notNullValue());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ private SecureString promptForPassword(Terminal terminal, String user) throws Us
// loop for two consecutive good passwords
while (true) {
SecureString password1 = new SecureString(terminal.readSecret("Enter password for [" + user + "]: "));
Validation.Error err = Validation.Users.validatePassword(password1.getChars());
Validation.Error err = Validation.Users.validatePassword(password1);
if (err != null) {
terminal.println(err.toString());
terminal.println("Try again.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.LoggingAwareMultiCommand;
Expand Down Expand Up @@ -111,7 +110,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
throw new UserException(ExitCodes.DATA_ERROR, "Invalid username [" + username + "]... " + validationError);
}

char[] password = parsePassword(terminal, passwordOption.value(options));
final char[] passwordHash = getPasswordHash(terminal, env, passwordOption.value(options));
String[] roles = parseRoles(terminal, env, rolesOption.value(options));

Path passwordFile = FileUserPasswdStore.resolveFile(env);
Expand All @@ -125,9 +124,8 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
if (users.containsKey(username)) {
throw new UserException(ExitCodes.CODE_ERROR, "User [" + username + "] already exists");
}
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
users = new HashMap<>(users); // make modifiable
users.put(username, hasher.hash(new SecureString(password)));
users.put(username, passwordHash);
FileUserPasswdStore.writeFile(users, passwordFile);

if (roles.length > 0) {
Expand Down Expand Up @@ -218,7 +216,7 @@ protected void printAdditionalHelp(Terminal terminal) {
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {

String username = parseUsername(arguments.values(options), env.settings());
char[] password = parsePassword(terminal, passwordOption.value(options));
char[] passwordHash = getPasswordHash(terminal, env, passwordOption.value(options));

Path file = FileUserPasswdStore.resolveFile(env);
FileAttributesChecker attributesChecker = new FileAttributesChecker(file);
Expand All @@ -229,9 +227,8 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
if (users.containsKey(username) == false) {
throw new UserException(ExitCodes.NO_USER, "User [" + username + "] doesn't exist");
}
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
users = new HashMap<>(users); // make modifiable
users.put(username, hasher.hash(new SecureString(password)));
users.put(username, passwordHash);
FileUserPasswdStore.writeFile(users, file);

attributesChecker.check(terminal);
Expand Down Expand Up @@ -440,23 +437,32 @@ static String parseUsername(List<String> args, Settings settings) throws UserExc
return username;
}

private static char[] getPasswordHash(Terminal terminal, Environment env, String cliPasswordValue) throws UserException {
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
final char[] passwordHash;
try (SecureString password = parsePassword(terminal, cliPasswordValue)) {
passwordHash = hasher.hash(password);
}
return passwordHash;
}

// pkg private for testing
static char[] parsePassword(Terminal terminal, String passwordStr) throws UserException {
char[] password;
static SecureString parsePassword(Terminal terminal, String passwordStr) throws UserException {
SecureString password;
if (passwordStr != null) {
password = passwordStr.toCharArray();
password = new SecureString(passwordStr.toCharArray());
Validation.Error validationError = Users.validatePassword(password);
if (validationError != null) {
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
}
} else {
password = terminal.readSecret("Enter new password: ");
password = new SecureString(terminal.readSecret("Enter new password: "));
Validation.Error validationError = Users.validatePassword(password);
if (validationError != null) {
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
}
char[] retyped = terminal.readSecret("Retype new password: ");
if (Arrays.equals(password, retyped) == false) {
if (Arrays.equals(password.getChars(), retyped) == false) {
throw new UserException(ExitCodes.DATA_ERROR, "Password mismatch");
}
}
Expand Down
Loading