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.

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

Log password requirement details in demo environment #4071

Merged

Conversation

camerondurham
Copy link
Contributor

@camerondurham camerondurham commented Feb 25, 2024

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?
  • 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 <u64.cam@gmail.com>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request @camerondurham - minor comments

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

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

@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Feb 27, 2024
Signed-off-by: Cameron Durham <u64.cam@gmail.com>
Copy link

codecov bot commented Feb 27, 2024

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

Impacted file tree graph

@@           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

@derek-ho
Copy link
Contributor

Thanks for the contribution @camerondurham !

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

@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?

Signed-off-by: Cameron Durham <u64.cam@gmail.com>
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

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

@DarshitChanpura
Copy link
Member

Thank you for this contribution @camerondurham !

@DarshitChanpura DarshitChanpura merged commit f3b5727 into opensearch-project:main Feb 28, 2024
82 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 28, 2024
Signed-off-by: Cameron Durham <u64.cam@gmail.com>
(cherry picked from commit f3b5727)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
DarshitChanpura pushed a commit that referenced this pull request Feb 28, 2024
…4082)

Backport f3b5727 from #4071.

Signed-off-by: Cameron Durham <u64.cam@gmail.com>
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>
dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Log more information about Password strength feature
4 participants