Add score based password verification #2557

willyborankin · 2023-03-15T20:58:50Z

Description

The aim of this PR is to add support for a score-based password verification using zxcvbn library.
For that 2 new settings were added to the plugin configuration:

plugins.security.restapi.password_min_length - minimum password length, default and minimum is 8
plugins.security.restapi.password_score_based_validation_strength - the strength of the valid password
Possible values:
- fair - very guessable password: protection from throttled online attacks
- good - somewhat guessable password: protection from unthrottled online attacks
- strong - safely unguessable password: moderate protection from offline slow-hash scenario
- very_strong - very unguessable password: strong protection from offline slow-hash scenario
  By default the plugin always checks strength of the password and its minimal length together with the regular expression if its set.

The calculation time for passwords around 100 characters is ~100ms as result to avoid of performance degradation for big passwords I suggest to set max length of the password to 100.

All other settings I mentioned before are not needed.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

peternied

Nice security improvement around the passwords.

@willyborankin Would you mind creating a separate issue to discuss making these options the default? I think this would be a good security posture improvement and would want contributors / community members to have a chance to weight in without delaying the functionality getting merged.

A quick note on performance - looking at the documentation [link] there is a typically small impact of ~5-20ms I think this is a worthwhile tradeoff as password scoring only happens when passwords are set/updated.

src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

src/main/java/org/opensearch/security/dlic/rest/validation/AccountValidator.java

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

src/main/java/org/opensearch/security/dlic/rest/validation/InternalUsersValidator.java

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java

willyborankin · 2023-03-16T17:14:30Z

Nice security improvement around the passwords.

@willyborankin Would you mind creating a separate issue to discuss making these options the default? I think this would be a good security posture improvement and would want contributors / community members to have a chance to weight in without delaying the functionality getting merged.

A quick note on performance - looking at the documentation [link] there is a typically small impact of ~5-20ms I think this is a worthwhile tradeoff as password scoring only happens when passwords are set/updated.

Sure, I will prepare an issue to discuss and address your comments. I will try to do it today,.

DarshitChanpura

Nice work @willyborankin ! This is of great value to security posture!

One generic comment is to add a brief documentation on the new files. This would help in debugging in future.

DarshitChanpura · 2023-03-16T16:58:03Z

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

-                        final boolean isDebugEnabled = log.isDebugEnabled();
-
+                    final String username = Utils.coalesce(request.param("name"), hasParams() ? (String) param[0] : null);
+                    if (!Strings.isNullOrEmpty(regex) || useScoreBasedValidation) {


Can you add a brief comment here stating what this condition does?

i changed code so there is no such condition anymore

DarshitChanpura · 2023-03-16T16:59:32Z

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

                                log.debug("Username must not match password");
                            }
                            this.errorType = ErrorType.INVALID_PASSWORD;
                            return false;
                        }
                    }
+                    if (!Strings.isNullOrEmpty(regex)) {
+                        return regexBasedPasswordValidation(regex, password);
+                    } else if (useScoreBasedValidation) {


why either or here? Why not have both?

DarshitChanpura · 2023-03-16T17:49:32Z

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

+        }
+        final Strength strength = new Zxcvbn().measure(password, ImmutableList.of(username));
+        if (strength.getScore() < scoreStrength.score()) {
+            this.errorType = ErrorType.INVALID_PASSWORD;


What do you think about changing the errorType to say WEAK_PASSWORD here?

I added WEAK_PASSWORD an SIMILAR_PASSWORD error types

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

DarshitChanpura · 2023-03-16T17:51:04Z

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

+            final boolean similar = strength.getSequence()
+                    .stream()
+                    .filter(m -> m.matchedWord != null)
+                    .anyMatch(m -> m.matchedWord.equals("similar"));


src/main/java/org/opensearch/security/support/PasswordValidationScoreStrength.java

DarshitChanpura · 2023-03-16T17:59:11Z

src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java

+
+        addUserWithPassword("ok4", "$1aAAAAAAAAC", HttpStatus.SC_OK);
+
+        //its not allowed to use the username as password (case insensitive)


What is the behavior when username and password is only 1 or 2 letters off? Will the password be allowed as valid?

in this case the score == 0 which means a weak password

RyanL1997

Nice work @willyborankin! This is definitely a huge enhancement for current password mechanism. I also left some thoughts attach to the previous comments with some details about the human understandable wording and some of the validation conditions.

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

src/main/java/org/opensearch/security/support/PasswordValidationScoreStrength.java

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

willyborankin · 2023-03-19T22:50:05Z

@peternied, @DarshitChanpura and @RyanL1997 thank you for the feedback I will address all you comments asap. As @peternied suggested I created a feature request #2569 and tried to explain how the library works and how to improve existing password verification functionality in it. So lets move our discussion there.

src/main/java/org/opensearch/security/dlic/rest/validation/PasswordValidator.java

src/main/java/org/opensearch/security/support/ConfigConstants.java

src/main/java/org/opensearch/security/dlic/rest/validation/PasswordValidator.java

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

src/main/java/org/opensearch/security/dlic/rest/validation/CredentialsValidator.java

codecov-commenter · 2023-04-05T15:46:38Z

Codecov Report

Merging #2557 (8a7475d) into main (7c4e06d) will increase coverage by 0.03%.
The diff coverage is 84.69%.

❗ Current head 8a7475d differs from pull request most recent head df5109a. Consider uploading reports for the commit df5109a to get more accurate results

@@             Coverage Diff              @@
##               main    #2557      +/-   ##
============================================
+ Coverage     61.31%   61.34%   +0.03%     
+ Complexity     3408     3347      -61     
============================================
  Files           272      261      -11     
  Lines         18846    18591     -255     
  Branches       3295     3277      -18     
============================================
- Hits          11555    11405     -150     
+ Misses         5690     5592      -98     
+ Partials       1601     1594       -7

Impacted Files	Coverage Δ
...g/opensearch/security/support/ConfigConstants.java	`94.44% <ø> (ø)`
...ity/dlic/rest/validation/CredentialsValidator.java	`70.00% <63.63%> (+4.21%)`	⬆️
...curity/dlic/rest/validation/PasswordValidator.java	`84.05% <84.05%> (ø)`
.../opensearch/security/OpenSearchSecurityPlugin.java	`80.12% <100.00%> (-0.04%)`	⬇️
...est/validation/AbstractConfigurationValidator.java	`83.64% <100.00%> (+1.09%)`	⬆️

... and 36 files with indirect coverage changes

DarshitChanpura · 2023-04-05T16:56:14Z

src/main/java/org/opensearch/security/dlic/rest/validation/PasswordValidator.java

+
+    private static final int MAX_LENGTH = 100;
+    private final static Predicate<Match> USERNAME_SIMILARITY_CHECK = m ->
+            m.pattern == com.nulabinc.zxcvbn.Pattern.Dictionary && m.dictionaryName.equals("user_inputs");


For my understanding why the pattern check against hard code string "user_inputs"?

user_inputs the default dictionary which library creates automatically to check similarity. I will add a comment otherwise it is confusing. So all you need is just check the dictionary name since matchedWord is a password.

okay..thank yoU!

DarshitChanpura · 2023-04-05T17:04:08Z

src/test/java/org/opensearch/security/dlic/rest/validation/PasswordValidatorTest.java

+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 PasswordValidatorTest {


nicely done! 👏

peternied · 2023-04-06T15:57:44Z

src/main/java/org/opensearch/security/dlic/rest/validation/PasswordValidator.java

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


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?

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).

peternied

Still a couple of outstanding comments, but I'm happy with the change as is. Thanks for the great new feature @willyborankin !

RyanL1997 · 2023-04-06T21:02:32Z

Hi @willyborankin, thanks for all the follow-ups! My comments have been resolved, and I'm happy to merge this change if we resolve the branch conflicts.

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin · 2023-05-15T17:26:09Z

@peternied, @scrawfor99, @RyanL1997 and @DarshitChanpura all tests now green

stephen-crawford

Thank you @willyborankin. This looks great.

cwperks

Thank you for another great contribution @willyborankin !

opensearch-trigger-bot · 2023-05-16T16:02:48Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2557-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 15860b65f33861f875b62ed57b6d8b7ab3c8a65d
# Push it to GitHub
git push --set-upstream origin backport/backport-2557-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2557-to-2.x.

Signed-off-by: Andrey Pleskach <ples@aiven.io>

Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

Signed-off-by: Andrey Pleskach <ples@aiven.io>

Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

willyborankin requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners March 15, 2023 20:58

willyborankin force-pushed the score-based-password-validation branch from 57023ae to ac2175e Compare March 15, 2023 21:10

peternied requested changes Mar 15, 2023

View reviewed changes

DarshitChanpura reviewed Mar 16, 2023

View reviewed changes

RyanL1997 reviewed Mar 17, 2023

View reviewed changes

willyborankin force-pushed the score-based-password-validation branch from ac2175e to 2d21b17 Compare March 28, 2023 19:30

willyborankin requested a review from peternied March 28, 2023 19:30

willyborankin force-pushed the score-based-password-validation branch from 2d21b17 to c3c13ec Compare March 29, 2023 14:39

peternied reviewed Apr 4, 2023

View reviewed changes

willyborankin force-pushed the score-based-password-validation branch from c3c13ec to 1c2e47e Compare April 5, 2023 15:09

willyborankin requested a review from reta as a code owner April 5, 2023 15:09

willyborankin requested a review from peternied April 5, 2023 15:19

willyborankin force-pushed the score-based-password-validation branch 2 times, most recently from c2b2065 to a156c99 Compare April 5, 2023 15:48

DarshitChanpura previously approved these changes Apr 5, 2023

View reviewed changes

willyborankin dismissed DarshitChanpura’s stale review via 92c82bf April 5, 2023 17:34

willyborankin force-pushed the score-based-password-validation branch from a156c99 to 92c82bf Compare April 5, 2023 17:34

peternied reviewed Apr 6, 2023

View reviewed changes

peternied previously approved these changes Apr 6, 2023

View reviewed changes

willyborankin dismissed peternied’s stale review via 8a7475d April 6, 2023 22:02

This was referenced May 4, 2023

[Manual Backport 2.x] Manually backports auth tokens, service accounts, and multi tenancy changes #2737

Merged

[Flaky Test] Configuration is not available after an update for more than 10 seconds #2742

Closed

willyborankin force-pushed the score-based-password-validation branch from b6ba386 to 7411d38 Compare May 15, 2023 14:17

Add score based password verification

df5109a

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the score-based-password-validation branch from 7411d38 to df5109a Compare May 15, 2023 15:51

stephen-crawford approved these changes May 15, 2023

View reviewed changes

cwperks approved these changes May 15, 2023

View reviewed changes

stephen-crawford merged commit 15860b6 into opensearch-project:main May 15, 2023

cwperks added the backport 2.x backport to 2.x branch label May 16, 2023

cwillum mentioned this pull request May 16, 2023

Add documentation for support of score-based password verification[DOC] opensearch-project/documentation-website#4088

Closed

4 tasks

sebastianmichalski pushed a commit to sebastianmichalski/security that referenced this pull request May 19, 2023

Add score based password verification (opensearch-project#2557)

8eedb3d

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin deleted the score-based-password-validation branch May 22, 2023 15:18

willyborankin restored the score-based-password-validation branch May 22, 2023 15:23

willyborankin mentioned this pull request May 22, 2023

[Backport 2.x] Add score based password verification #2784

Merged

RyanL1997 mentioned this pull request Jun 2, 2023

Fix internaluser test to adapt the password validation opensearch-project/opensearch-dashboards-functional-test#707

Merged

1 task

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Jun 13, 2023

Add score based password verification (opensearch-project#2557)

82d51de

Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Jun 13, 2023

Add score based password verification (opensearch-project#2557)

54786a2

Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

willyborankin deleted the score-based-password-validation branch June 15, 2023 19:50

samuelcostae pushed a commit to samuelcostae/security that referenced this pull request Jun 19, 2023

Add score based password verification (opensearch-project#2557)

722aa38

Signed-off-by: Andrey Pleskach <ples@aiven.io>

samuelcostae pushed a commit to samuelcostae/security that referenced this pull request Jun 19, 2023

Add score based password verification (opensearch-project#2557)

eb1454b

Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

cwperks mentioned this pull request Jul 11, 2023

Use password message from /dashboardsinfo opensearch-project/security-dashboards-plugin#1503

Merged

3 tasks

This was referenced Aug 21, 2023

Able to reset password which does not meet the given password policy. opensearch-project/security-dashboards-plugin#817

Closed

[FEATURE] Score-based password strength verification #2569

Closed

DarshitChanpura mentioned this pull request Feb 27, 2024

Log password requirement details in demo environment #4071

Merged

3 tasks

DarshitChanpura mentioned this pull request May 14, 2024

[Feature Request] Provide guidance to users on complexity requirements of OPENSEARCH_INITIAL_ADMIN_PASSWORD before software installation #4343

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add score based password verification #2557

Add score based password verification #2557

willyborankin commented Mar 15, 2023 •

edited

Loading

peternied left a comment

willyborankin commented Mar 16, 2023

DarshitChanpura left a comment

DarshitChanpura Mar 16, 2023

willyborankin Apr 5, 2023 •

edited

Loading

DarshitChanpura Mar 16, 2023

willyborankin Apr 5, 2023

DarshitChanpura Mar 16, 2023

willyborankin Apr 5, 2023

DarshitChanpura Mar 16, 2023

DarshitChanpura Mar 16, 2023

willyborankin Apr 5, 2023 •

edited

Loading

RyanL1997 left a comment

willyborankin commented Mar 19, 2023

codecov-commenter commented Apr 5, 2023 •

edited by codecov bot

Loading

DarshitChanpura Apr 5, 2023

willyborankin Apr 5, 2023

DarshitChanpura Apr 5, 2023

DarshitChanpura Apr 5, 2023

peternied Apr 6, 2023

willyborankin Apr 6, 2023

peternied left a comment

RyanL1997 commented Apr 6, 2023

willyborankin commented May 15, 2023

stephen-crawford left a comment

cwperks left a comment

opensearch-trigger-bot bot commented May 16, 2023


		addUserWithPassword("ok4", "$1aAAAAAAAAC", HttpStatus.SC_OK);

		//its not allowed to use the username as password (case insensitive)

Add score based password verification #2557

Add score based password verification #2557

Conversation

willyborankin commented Mar 15, 2023 • edited Loading

Description

Check List

peternied left a comment

Choose a reason for hiding this comment

willyborankin commented Mar 16, 2023

DarshitChanpura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willyborankin Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willyborankin Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

RyanL1997 left a comment

Choose a reason for hiding this comment

willyborankin commented Mar 19, 2023

codecov-commenter commented Apr 5, 2023 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

RyanL1997 commented Apr 6, 2023

willyborankin commented May 15, 2023

stephen-crawford left a comment

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented May 16, 2023

willyborankin commented Mar 15, 2023 •

edited

Loading

willyborankin Apr 5, 2023 •

edited

Loading

willyborankin Apr 5, 2023 •

edited

Loading

codecov-commenter commented Apr 5, 2023 •

edited by codecov bot

Loading