Log password requirement details in demo environment by camerondurham · Pull Request #4071 · opensearch-project/security

camerondurham · 2024-02-25T03:12:55Z

Description

Logs details about the password validations done when demo clusters are created. The current message "Password $ADMIN_PASSWORD is weak. Please re-try with a stronger password." doesn't provide enough information communicate what the user needs to set the password to.

Category: Enhancement
Why these changes are required?
- See [FEATURE] Log more information about Password strength feature #4048 - to improve UX when creating test OpenSearch clusters by logging details on password requirements
What is the old behavior before changes and new behavior after changes?
- Before: OS 2.12 fails to start with message Password sample@1029 is weak. Please re-try with a stronger password.
- After: security plugin fails to start with message Password weakpassword failed validation with error "Weak password". Please re-try with a minimum 8 character password that is "strong" per zxcvbn validation.

Issues Resolved

Potentially can resolve #4048, though would need issue author @derek-ho to confirm this is the intended change.

Testing

Updated existing unit test that tests "password validation failure".

Check List

New functionality includes testing
New functionality has been documented <-- In a separate PR, I also want to update relevant docs, including docker getting started docs docs with perhaps the zxcvbn "validation" tool.
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.

Signed-off-by: Cameron Durham <[email protected]>

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java

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

peternied

Thanks for the pull request @camerondurham - minor comments

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

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java

DarshitChanpura

Thank you @camerondurham for this improvement. Left some comments after taking an initial pass.

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

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java

Signed-off-by: Cameron Durham <[email protected]>

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java

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

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

codecov · 2024-02-27T14:06:13Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.89%. Comparing base (77ffba4) to head (ae6177a).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4071   +/-   ##
=======================================
  Coverage   65.88%   65.89%           
=======================================
  Files         298      298           
  Lines       21338    21341    +3     
  Branches     3473     3472    -1     
=======================================
+ Hits        14058    14062    +4     
+ Misses       5541     5539    -2     
- Partials     1739     1740    +1

Files	Coverage Δ
...y/tools/democonfig/SecuritySettingsConfigurer.java	`74.64% <88.88%> (+0.54%)`	⬆️

... and 1 file with indirect coverage changes

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

derek-ho · 2024-02-27T14:46:29Z

Thanks for the contribution @camerondurham !

DarshitChanpura

@camerondurham the settings plugins.security.restapi.password_min_length and plugins.security.restapi.password_score_based_validation_strength come into play once the security plugin is setup and cluster is ready, which is when all the config settings are read.

However, in case of demo configuration script execution, these settings are not available to be read, as the tools write opensearch.yml with security-related settings necessary for cluster start-up. [See this]

In case if we try to pass these settings in opensearch.yml, the demo script will stop execution since it thinks that security plugin is already setup because it is able to find plugins.security.<setting> already defined in opensearch.yml [check this out]

I think we are overthinking this. The regex + password length supplied here are one time use before a fresh cluster start-up. We can leave those settings as is and modify the log message to focus on the scope of this PR. Thoughts?

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java

Signed-off-by: Cameron Durham <[email protected]>

DarshitChanpura

Thank you for addressing all the feedback! Nothing more from my end; waiting for CI to be green before approval.

DarshitChanpura · 2024-02-28T15:41:21Z

Thank you for this contribution @camerondurham !

Signed-off-by: Cameron Durham <[email protected]> (cherry picked from commit f3b5727) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…4082) Backport f3b5727 from #4071. Signed-off-by: Cameron Durham <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ect#4071) Signed-off-by: Cameron Durham <[email protected]>

Log password requirement details in demo environment

c8ad604

Signed-off-by: Cameron Durham <[email protected]>

camerondurham requested review from DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied, reta, stephen-crawford and willyborankin as code owners February 25, 2024 03:12

camerondurham commented Feb 25, 2024

View reviewed changes

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java Outdated Show resolved Hide resolved

camerondurham commented Feb 25, 2024

View reviewed changes

src/main/java/org/opensearch/security/support/ConfigConstants.java Outdated Show resolved Hide resolved

peternied reviewed Feb 26, 2024

View reviewed changes

DarshitChanpura reviewed Feb 27, 2024

View reviewed changes

DarshitChanpura added the backport 2.x label Feb 27, 2024

Move *_DEFAULTs and add password length test

09dcdc9

Signed-off-by: Cameron Durham <[email protected]>

camerondurham commented Feb 27, 2024

View reviewed changes

derek-ho reviewed Feb 27, 2024

View reviewed changes

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java Show resolved Hide resolved

DarshitChanpura reviewed Feb 27, 2024

View reviewed changes

src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java Outdated Show resolved Hide resolved

Consolidate password validation in test

ae6177a

Signed-off-by: Cameron Durham <[email protected]>

DarshitChanpura reviewed Feb 27, 2024

View reviewed changes

peternied approved these changes Feb 27, 2024

View reviewed changes

DarshitChanpura mentioned this pull request Feb 28, 2024

[DOC] Password validation settings used in demo config tool are not the same as the ones shipped with OpenSearch. #4081

Open

DarshitChanpura approved these changes Feb 28, 2024

View reviewed changes

DarshitChanpura merged commit f3b5727 into opensearch-project:main Feb 28, 2024

opensearch-trigger-bot bot mentioned this pull request Feb 28, 2024

[Backport 2.x] Log password requirement details in demo environment #4082

Merged

dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024

Log password requirement details in demo environment (opensearch-proj…

57adb29

…ect#4071) Signed-off-by: Cameron Durham <[email protected]>

Conversation

camerondurham commented Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

Uh oh!

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

derek-ho commented Feb 27, 2024

Uh oh!

DarshitChanpura left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DarshitChanpura left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

camerondurham commented Feb 25, 2024 •

edited

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading

DarshitChanpura left a comment •

edited

Loading