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

Password setup tool for built-in users #1836

Closed
wants to merge 21 commits into from

Conversation

setiah
Copy link

@setiah setiah commented May 11, 2022

Description

This PR primarily provides new password setup tool for setting up passwords for inbuilt security users (admin, kibanaserver, etc.) and also provides a new unified interface for extending tooling in opensearch-security.

Details of all the changes

  1. Provides a new unified interface to simplify and extend tooling in opensearch-security.
  2. Adds setup-password functionality to allow users to setup secure passwords for built-in users, instead of relying on weak defaults such as admin:admin. The functionality is provided in both manual and auto mode for auto-password generation.
  3. Adds default secure password policy to opensearch.yml

What is the old behavior before changes and new behavior after changes?

In the old behavior, users had to update the internal_users.yml file on all nodes with the hash of the password to change the passwords from admin/admin. While that still works, with the new tool, they can simply run the tool to setup secure passwords after installation. The tool runs as opensearch-admin setup-passwords [--auto] or to setup policy compliant passwords for inbuilt users (both manually and automatically). It can be extended to support other functions such as setup-certificates, reload-configs, through its simplified extensible interface.

Issues Resolved

#1576

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

Unit testing, Manual testing

Check List

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

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.

@setiah setiah requested a review from a team May 11, 2022 21:19
@setiah setiah marked this pull request as draft May 11, 2022 21:19
ryanbogan and others added 21 commits May 13, 2022 14:33
… made progress on a password setup tool

Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #1836 (b77dfe0) into main (3d3362a) will decrease coverage by 0.06%.
The diff coverage is 57.18%.

@@             Coverage Diff              @@
##               main    #1836      +/-   ##
============================================
- Coverage     60.87%   60.80%   -0.07%     
- Complexity     3214     3248      +34     
============================================
  Files           256      259       +3     
  Lines         18006    18312     +306     
  Branches       3209     3252      +43     
============================================
+ Hits          10961    11135     +174     
- Misses         5466     5562      +96     
- Partials       1579     1615      +36     
Impacted Files Coverage Δ
...in/java/org/opensearch/security/tools/Command.java 45.45% <45.45%> (ø)
...a/org/opensearch/security/tools/PasswordSetup.java 53.51% <53.51%> (ø)
...org/opensearch/security/tools/OpenSearchAdmin.java 84.61% <84.61%> (ø)
...a/org/opensearch/security/tools/SecurityAdmin.java 37.31% <0.00%> (-0.25%) ⬇️
...urity/configuration/PrivilegesInterceptorImpl.java 51.28% <0.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d3362a...b77dfe0. Read the comment docs.

@setiah setiah marked this pull request as ready for review May 17, 2022 00:07
@setiah
Copy link
Author

setiah commented May 17, 2022

@peternied can you help with reviewing this PR?
cc: @ryanbogan

@CEHENKLE
Copy link
Member

@opensearch-project/security Hey folks -- can we get some code review luv over here?

Thanks :)

/C

@@ -30,6 +30,7 @@

package org.opensearch.security;


Copy link
Member

Choose a reason for hiding this comment

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

Please remove this empty line if not necessary.

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. Making sure that new clusters have strong security posture is an important space for us.

This pull request changes the responsibility of the security plugin by adding a new OpenSearch-Admin script. This is a public interface in the security repository that over time will likely need to be deprecated based on de-emphasizing security as a plugin. We should avoid this, I would recommend baking these changes into a cluster initialization process.

Accepting custom passwords creates the opportunity for bad password practices. OpenSearch would have a better posture if we had a standard password generation of say 20+ characters of entropy. This could be done with modifications to the existing create user API or as part of the initialization process to standard out.

Please reduce the scope of this PR to an automatic (strong) password generation during the initialization process, and extract the admin tool/script creation into a separate draft PR so we can have that larger discussion over there, and weigh the pros and cons of adding a tool that will need to be deprecated vs postponing that effort until we have a broader plan for admin tools moving forward.

@setiah
Copy link
Author

setiah commented Jun 7, 2022

@peternied

There is a separate issue tracking auto-password setup during cluster bootstrap, which leverages the PasswordSetup logic added by this tool to autogenerate passwords. But passwords are not just setup during the first time setup. They also need to be rotated many times during the lifecycle of a cluster based on security needs of an organization. This tool provides an easy way to for cluster administrator to set the passwords first time (until we have this) and rotate/update them later. Hence it would be helpful.

This pull request changes the responsibility of the security plugin by adding a new OpenSearch-Admin script. This is a public interface in the security repository that over time will likely need to be deprecated based on de-emphasizing security as a plugin.

The public interface is for Security plugin tooling and is not needed in other plugins. We can rename it accordingly. The idea is all tools in Security plugin (hasher, securityadmin, install_demo_configurations, etc.) can extend this interface to inherit some common functions, and just build their business logic. This will enable java-fying the security plugin tools so they are unit testable and maintainable like code, unlike some shell tools in security plugin repo today that need to be validated manually.

Accepting custom passwords creates the opportunity for bad password practices. This could be done with modifications to the existing create user API or as part of the initialization process to standard out.

I think you missed this change mentioned in the PR description - "Adds default secure password policy to opensearch.yml". The custom passwords are validated against this default secure password policy that is being adding with this change. If the password is not policy compliant, it gets rejected.
Lack of a default secure password policy is a gap today which allows users to create weak passwords

I'd request you to take a look again.

@peternied
Copy link
Member

This tool provides an easy way to for cluster administrator to set the passwords first time (until we have #1787) and rotate/update them later. Hence it would be helpful.

The security plugin already has an API, there is no extra burden for documentation, maintenance or release management. A new endpoint such as PUT user/{user}/rotateCredentials, or improving existing endpoints for this scenario seems very reasonable. Exposing this functionality with an API expands the utility so admins could make API calls directly, use the Dashboard UX, or use another plugin to go through the same workflow. If this is built as a tool its utility is limited, lets build this functionality so it can be reused. Is there a reason to limit the functionality?

I think you missed this change mentioned in the PR description - "Adds default secure password policy to opensearch.yml".

This change was done in the tools/install_demo_configuration.sh file, outside of the recommended configuration. There would be much more benefit to make this change as part of the security default configuration. What do you think about separating password policy into another pull request?

@cliu123 cliu123 added the backport 2.x backport to 2.x branch label Jun 16, 2022
@setiah
Copy link
Author

setiah commented Jun 21, 2022

The security plugin already has an API, there is no extra burden for documentation, maintenance or release management. A new endpoint such as PUT user/{user}/rotateCredentials, or improving existing endpoints for this scenario seems very reasonable. Exposing this functionality with an API expands the utility so admins could make API calls directly, use the Dashboard UX, or use another plugin to go through the same workflow. If this is built as a tool its utility is limited, lets build this functionality so it can be reused. Is there a reason to limit the functionality?

The functionality already exists today with a PATCH API. The tool just builds on the capability of existing PATCH API to simplify password setup process for all internal users. I do think there is value in having a tool based mechanism over and above REST API as it makes life easier for users.

This change was done in the tools/install_demo_configuration.sh file, outside of the recommended configuration. There would be much more benefit to make this change as part of the security default configuration. What do you think about separating password policy into another pull request?

I'm okay with this suggestion.

@peternied
Copy link
Member

@setiah I - we all appreciate your passion in this space and it is unfortunate that we are not reaching better alignment with this change.

We would be happy to review a new pull request aligned with the areas of concern I have, a draft might be a good starting point to ensure we can tackle design related concerns early in the process.

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.

6 participants