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

Add score based password verification #2557

Merged
merged 1 commit into from
May 15, 2023
+661 −178
Merged

Add score based password verification #2557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ dependencies {
implementation ('org.opensaml:opensaml-saml-impl:3.4.5') {
exclude(group: 'org.apache.velocity', module: 'velocity')
}
implementation "com.nulab-inc:zxcvbn:1.7.0"
testImplementation 'org.opensaml:opensaml-messaging-impl:3.4.5'
implementation 'org.opensaml:opensaml-messaging-api:3.4.5'
runtimeOnly 'org.opensaml:opensaml-profile-api:3.4.5'
Expand Down
4 changes: 2 additions & 2 deletions src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ public class SecurityConfigurationTests {
.roles(new Role("limited-role").indexPermissions("indices:data/read/search", "indices:data/read/get").on("user-${user.name}"));
public static final String LIMITED_USER_INDEX = "user-" + LIMITED_USER.getName();
public static final String ADDITIONAL_USER_1 = "additional00001";
public static final String ADDITIONAL_PASSWORD_1 = ADDITIONAL_USER_1;
public static final String ADDITIONAL_PASSWORD_1 = "user 1 fair password";

public static final String ADDITIONAL_USER_2 = "additional2";
public static final String ADDITIONAL_PASSWORD_2 = ADDITIONAL_USER_2;
public static final String ADDITIONAL_PASSWORD_2 = "user 2 fair password";
public static final String CREATE_USER_BODY = "{\"password\": \"%s\",\"opendistro_security_roles\": []}";
public static final String INTERNAL_USERS_RESOURCE = "_plugins/_security/api/internalusers/";
public static final String ID_1 = "one";
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@
import org.opensearch.security.configuration.Salt;
import org.opensearch.security.configuration.SecurityFlsDlsIndexSearcherWrapper;
import org.opensearch.security.dlic.rest.api.SecurityRestApiActions;
import org.opensearch.security.dlic.rest.validation.PasswordValidator;
import org.opensearch.security.filter.SecurityFilter;
import org.opensearch.security.filter.SecurityRestFilter;
import org.opensearch.security.http.SecurityHttpServerTransport;
Expand Down Expand Up @@ -1043,6 +1044,19 @@ public List<Setting<?>> getSettings() {
settings.add(Setting.simpleString(ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, Property.NodeScope, Property.Filtered));
settings.add(Setting.simpleString(ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE, Property.NodeScope, Property.Filtered));

settings.add(
Setting.intSetting(
ConfigConstants.SECURITY_RESTAPI_PASSWORD_MIN_LENGTH,
-1, -1, Property.NodeScope, Property.Filtered)
);
settings.add(
Setting.simpleString(
ConfigConstants.SECURITY_RESTAPI_PASSWORD_SCORE_BASED_VALIDATION_STRENGTH,
PasswordValidator.ScoreStrength.STRONG.name(),
PasswordValidator.ScoreStrength::fromConfiguration,
Property.NodeScope, Property.Filtered
)
);

// Compliance
settings.add(Setting.listSetting(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_WATCHED_INDICES, Collections.emptyList(), Function.identity(), Property.NodeScope)); //not filtered here
Expand Down
22 changes: 19 additions & 3 deletions ...ain/java/org/opensearch/security/dlic/rest/validation/AbstractConfigurationValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,19 @@ public XContentBuilder errorsAsXContent(RestChannel channel) {
break;
case INVALID_PASSWORD:
builder.field("status", "error");
builder.field("reason", opensearchSettings.get(ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE,
builder.field("reason", opensearchSettings.get(
ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE,
"Password does not match minimum criteria"));
break;
case WEAK_PASSWORD:
case SIMILAR_PASSWORD:
builder.field("status", "error");
builder.field(
"reason",
opensearchSettings.get(
ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE,
errorType.message));
break;
case WRONG_DATATYPE:
builder.field("status", "error");
builder.field("reason", ErrorType.WRONG_DATATYPE.getMessage());
Expand Down Expand Up @@ -289,8 +299,14 @@ public static enum DataType {
}

public static enum ErrorType {
NONE("ok"), INVALID_CONFIGURATION("Invalid configuration"), INVALID_PASSWORD("Invalid password"), WRONG_DATATYPE("Wrong datatype"),
BODY_NOT_PARSEABLE("Could not parse content of request."), PAYLOAD_NOT_ALLOWED("Request body not allowed for this action."),
NONE("ok"),
INVALID_CONFIGURATION("Invalid configuration"),
INVALID_PASSWORD("Invalid password"),
WEAK_PASSWORD("Weak password"),
SIMILAR_PASSWORD("Password is similar to user name"),
WRONG_DATATYPE("Wrong datatype"),
BODY_NOT_PARSEABLE("Could not parse content of request."),
PAYLOAD_NOT_ALLOWED("Request body not allowed for this action."),
PAYLOAD_MANDATORY("Request body required for this action."), SECURITY_NOT_INITIALIZED("Security index not initialized"),
NULL_ARRAY_ELEMENT("`null` is not allowed as json array element");

Expand Down
52 changes: 18 additions & 34 deletions src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
package org.opensearch.security.dlic.rest.validation;

import java.util.Map;
import java.util.regex.Pattern;

import org.opensearch.common.Strings;
import org.opensearch.common.bytes.BytesReference;
Expand All @@ -22,17 +21,21 @@
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.rest.RestRequest;
import org.opensearch.security.ssl.util.Utils;
import org.opensearch.security.support.ConfigConstants;

/**
* Validator for validating password and hash present in the payload
*/
public class CredentialsValidator extends AbstractConfigurationValidator {

public CredentialsValidator(final RestRequest request, BytesReference ref, final Settings opensearchSettings,
private final PasswordValidator passwordValidator;

public CredentialsValidator(final RestRequest request,
final BytesReference ref,
final Settings opensearchSettings,
Object... param) {
super(request, ref, opensearchSettings, param);
this.payloadMandatory = true;
this.passwordValidator = PasswordValidator.of(opensearchSettings);
allowedKeys.put("hash", DataType.STRING);
allowedKeys.put("password", DataType.STRING);
}
Expand All @@ -46,49 +49,29 @@ public boolean validate() {
if (!super.validate()) {
return false;
}

final String regex = this.opensearchSettings.get(ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, null);

if ((request.method() == RestRequest.Method.PUT || request.method() == RestRequest.Method.PATCH)
&& this.content != null
&& this.content.length() > 1) {
try {
final Map<String, Object> contentAsMap = XContentHelper.convertToMap(this.content, false, XContentType.JSON).v2();
String password = (String) contentAsMap.get("password");
final String password = (String) contentAsMap.get("password");
if (password != null) {
// Password is not allowed to be empty if present.
if (password.isEmpty()) {
this.errorType = ErrorType.INVALID_PASSWORD;
return false;
}

if (!Strings.isNullOrEmpty(regex)) {
// Password can be null for an existing user. Regex will validate password if present
if (!Pattern.compile("^"+regex+"$").matcher(password).matches()) {
if(log.isDebugEnabled()) {
log.debug("Regex does not match password");
}
this.errorType = ErrorType.INVALID_PASSWORD;
return false;
}

final String username = Utils.coalesce(request.param("name"), hasParams() ? (String) param[0] : null);
final boolean isDebugEnabled = log.isDebugEnabled();

if (username == null || username.isEmpty()) {
if (isDebugEnabled) {
log.debug("Unable to validate username because no user is given");
}
return false;
}

if (username.toLowerCase().equals(password.toLowerCase())) {
if (isDebugEnabled) {
log.debug("Username must not match password");
}
this.errorType = ErrorType.INVALID_PASSWORD;
return false;
final String username = Utils.coalesce(request.param("name"), hasParams() ? (String) param[0] : null);
if (Strings.isNullOrEmpty(username)) {
if (log.isDebugEnabled()) {
log.debug("Unable to validate username because no user is given");
}
return false;
}
final ErrorType passwordValidationResult = passwordValidator.validate(username, password);
if (passwordValidationResult != ErrorType.NONE) {
this.errorType = passwordValidationResult;
return false;
}
}
} catch (NotXContentException e) {
Expand All @@ -99,4 +82,5 @@ public boolean validate() {
}
return true;
}

}
185 changes: 185 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/validation/PasswordValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.dlic.rest.validation;

import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.StringJoiner;
import java.util.function.Predicate;
import java.util.regex.Pattern;

import com.google.common.collect.ImmutableList;
import com.nulabinc.zxcvbn.Strength;
import com.nulabinc.zxcvbn.Zxcvbn;
import com.nulabinc.zxcvbn.matchers.Match;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.common.Strings;
import org.opensearch.common.settings.Settings;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator.ErrorType;

import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_MIN_LENGTH;
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_SCORE_BASED_VALIDATION_STRENGTH;
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX;

public class PasswordValidator {

private static final int MAX_LENGTH = 100;

/**
* Checks a username similarity and a password
* names and passwords like:
* - some_user_name/456Some_uSer_Name_1234
* - some_user_name/some_user_name_Ydfge
* - some_user_name/eman_resu_emos
* are similar
* "user_inputs" - is a default dictionary zxcvbn creates for checking similarity
*/
private final static Predicate<Match> USERNAME_SIMILARITY_CHECK = m ->
m.pattern == com.nulabinc.zxcvbn.Pattern.Dictionary && "user_inputs".equals(m.dictionaryName);

private final Logger logger = LogManager.getLogger(this.getClass());

private final int minPasswordLength;

private final Pattern passwordRegexpPattern;

private final ScoreStrength scoreStrength;

private final Zxcvbn zxcvbn;

private PasswordValidator(final int minPasswordLength,
final Pattern passwordRegexpPattern,
final ScoreStrength scoreStrength) {
this.minPasswordLength = minPasswordLength;
this.passwordRegexpPattern = passwordRegexpPattern;
this.scoreStrength = scoreStrength;
this.zxcvbn = new Zxcvbn();
}

public static PasswordValidator of(final Settings settings) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
final String passwordRegex = settings.get(SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, null);
final ScoreStrength scoreStrength = ScoreStrength.fromConfiguration(
settings.get(SECURITY_RESTAPI_PASSWORD_SCORE_BASED_VALIDATION_STRENGTH, ScoreStrength.STRONG.name())
);
final int minPasswordLength = settings.getAsInt(SECURITY_RESTAPI_PASSWORD_MIN_LENGTH, -1);
return new PasswordValidator(
minPasswordLength,
!Strings.isNullOrEmpty(passwordRegex) ? Pattern.compile(String.format("^%s$", passwordRegex)) : null,
scoreStrength);
}

ErrorType validate(final String username, final String password) {
if (minPasswordLength > 0 && password.length() < minPasswordLength) {
logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

These seems like well written error messages seem valuable, could we return this messages instead to be visible to the user. If we did that, we could remove the password_validation_error_message (and related) value that is in the config. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I would prefer to stay it as is. The main reason, that some companies prefer to send the same error message when a user creates/updates the password (kinda less possibilities for attackers to know what is the password creation rules :-) which I personally do not believe in since if somebody wanna hack the system it will do it).

"Password is too short, the minimum required length is {}, but current length is {}",
minPasswordLength,
password.length()
);
return ErrorType.INVALID_PASSWORD;
}
if (password.length() > MAX_LENGTH) {
logger.debug(
"Password is too long, the maximum required length is {}, but current length is {}",
MAX_LENGTH,
password.length()
);
return ErrorType.INVALID_PASSWORD;
}
if (Objects.nonNull(passwordRegexpPattern)
&& !passwordRegexpPattern.matcher(password).matches()) {
logger.debug("Regex does not match password");
return ErrorType.INVALID_PASSWORD;
}
final Strength strength = zxcvbn.measure(password, ImmutableList.of(username));
if (strength.getScore() < scoreStrength.score()) {
logger.debug(
"Password is weak the required score is {}, but current is {}",
scoreStrength,
ScoreStrength.fromScore(strength.getScore())
);
return ErrorType.WEAK_PASSWORD;
}
final boolean similar = strength.getSequence()
.stream()
.anyMatch(USERNAME_SIMILARITY_CHECK);
if (similar) {
logger.debug("Password is too similar to the user name {}", username);
return ErrorType.SIMILAR_PASSWORD;
}
return ErrorType.NONE;
}

public enum ScoreStrength {

// The weak score defines here only for debugging information
// and doesn't use as a configuration setting value.
WEAK(0, "too guessable: risky password"),
FAIR(1, "very guessable: protection from throttled online attacks"),
GOOD(2, "somewhat guessable: protection from unthrottled online attacks"),
STRONG(3, "safely unguessable: moderate protection from offline slow-hash scenario"),
VERY_STRONG(4, "very unguessable: strong protection from offline slow-hash scenario");
peternied marked this conversation as resolved.
Show resolved Hide resolved

private final int score;

private final String description;
peternied marked this conversation as resolved.
Show resolved Hide resolved

static final List<ScoreStrength> CONFIGURATION_VALUES = ImmutableList.of(FAIR, STRONG, VERY_STRONG);

static final String EXPECTED_CONFIGURATION_VALUES =
new StringJoiner(",")
.add(FAIR.name().toLowerCase(Locale.ROOT))
.add(STRONG.name().toLowerCase(Locale.ROOT))
.add(VERY_STRONG.name().toLowerCase(Locale.ROOT))
.toString();

private ScoreStrength(final int score, final String description) {
this.score = score;
this.description = description;
}

public static ScoreStrength fromScore(final int score) {
for (final ScoreStrength strength : values()) {
if (strength.score == score)
return strength;
}
throw new IllegalArgumentException("Unknown score " + score);
}

public static ScoreStrength fromConfiguration(final String value) {
for (final ScoreStrength strength : CONFIGURATION_VALUES) {
if (strength.name().equalsIgnoreCase(value))
return strength;
}
throw new IllegalArgumentException(
String.format(
"Setting [%s] cannot be used with the configured: %s. Expected one of [%s]",
SECURITY_RESTAPI_PASSWORD_SCORE_BASED_VALIDATION_STRENGTH,
value,
EXPECTED_CONFIGURATION_VALUES
)
);
}

@Override
public String toString() {
return String.format("Password strength score %s. %s", score, description);
}

public int score() {
return this.score;
}

}
}
4 changes: 3 additions & 1 deletion src/main/java/org/opensearch/security/support/ConfigConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,9 @@ public enum RolesMappingResolution {
public static final String SECURITY_RESTAPI_ENDPOINTS_DISABLED = "plugins.security.restapi.endpoints_disabled";
public static final String SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX = "plugins.security.restapi.password_validation_regex";
public static final String SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE = "plugins.security.restapi.password_validation_error_message";

public static final String SECURITY_RESTAPI_PASSWORD_MIN_LENGTH = "plugins.security.restapi.password_min_length";
public static final String SECURITY_RESTAPI_PASSWORD_SCORE_BASED_VALIDATION_STRENGTH = "plugins.security.restapi.password_score_based_validation_strength";
// Illegal Opcodes from here on
public static final String SECURITY_UNSUPPORTED_DISABLE_REST_AUTH_INITIALLY = "plugins.security.unsupported.disable_rest_auth_initially";
public static final String SECURITY_UNSUPPORTED_DISABLE_INTERTRANSPORT_AUTH_INITIALLY = "plugins.security.unsupported.disable_intertransport_auth_initially";
public static final String SECURITY_UNSUPPORTED_PASSIVE_INTERTRANSPORT_AUTH_INITIALLY = "plugins.security.unsupported.passive_intertransport_auth_initially";
Expand Down
Loading