-
Notifications
You must be signed in to change notification settings - Fork 273
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
[Security] Demo configuration script requires admin password #4094
Comments
cc: @DarshitChanpura, @peterzhuamazon, @davidlago, @bbarani, @peternied, @cwperks and @scrawfor99 |
Need more discussion on this as the solution proposed above would requires build to add the parameter to all the currently releasing distributions. Security also needs to have documentation on this potential breaking change from their side. |
@peterzhuamazon shared his perspective from build side and I share the sentiment. Here is an approach that allows us to gradually bring this change in :
|
To be more clear, the requirement is htat we should not ship with the default admin:admin, and this can only be used in the testing environment. |
Hey guys: I've done some digging into the recent changes. Although we've incorporated modifications into both the main and 2.x branches of our security repository, the outcomes aren't aligning with our expectations. Our initial goal was for both the demo configuration and the cluster initiation to fail when an initial admin password wasn't supplied. To achieve this, we implemented a strict failure mechanism in our demo configuration script that triggers when the initial admin password is absent (source code). However, from what @peterzhuamazon shared, the build team's script doesn't strictly enforce failure when the demo configuration script encounters an error. This means that even if the demo setup is unsuccessful, the cluster initiation still proceeds. Given this workflow, our current approach in the demo configuration—to substitute the default hash of "admin:admin" in |
Adding some painpoints that we will face when consuming the upcoming changes from security:
Thanks. |
@peterzhuamazon Thanks for the detail. Yes, let's have a discussion upon these 2 options, the changes of both options in security repo are ready now. (cc: @DarshitChanpura @peternied @bbarani @davidlago ). |
20231006 RC1: https://github.com/opensearch-project/security/commits/2.11 As of the making of 2.11.0 RC1, the commit of security plugin with demo configuration changes is still in 2.11 branch. This code only issue an
Thanks. |
@peterzhuamazon This is a known issue in the script where even though the script fails execution, the admin user is still created and that is because a default admin user is always present in |
BTW, should we be logging user password, see https://github.com/opensearch-project/security/blob/2.11/tools/install_demo_configuration.sh#L404-L406 @DarshitChanpura |
@rishabh6788 Yes, that is how the user will be able to pick up new password. As of now there is no other place for them to know the generated password (unless they have provided one). |
How about write it to the configuration file and ask user to read it from there? |
Can you elaborate this? What type of file are you thinking about?
This is if they have access to the logs. |
|
|
@DarshitChanpura We would like to get your inputs on the below 2 issues with this commit. 1.) User experience would not be great if we display this error message 2.) Printing out the user / password in log is going to create a non-optimal experience for users who are concerned with security. We are good to proceed if you think that the above 2 issues doesn't warrant a revert. We will post any other findings as and when we see it to this thread. |
Sure thing.
@peternied Would you like to add your thoughts here since you and @scrawfor99 were driving the original change? |
I have created a PR to address the confusing log: opensearch-project/security#3490. It points to 2.11 and should help resolve outstanding concerns. |
The latest RC on 2.11.0 as of 20231007 could not startup deb/rpm correctly. The pkg will proceed to run the demo configuration script and failed due to no password:
If you try to start the cluster it will then complain there is no certs ready as the demo certs are not copied due to above security demo scripts failures:
Tho this should fail the TAR as well, not sure why TAR is passing at the moment. Attach systemd logs:
Thanks. |
Hey @peterzhuamazon, we wouldn't expect it to fail this way because we already added the fix: opensearch-project/security#3490 |
Seems like the fix was merged after last RC generation. Thanks. |
Hey @DarshitChanpura @RyanL1997 I can see there are couple of issues opened related to initial admin password change, can you please create one META issue and link them so that It would be easy to track and we can have all the conversation on that global META issue. Also can you please provide the following details?
|
Hey @DarshitChanpura @derek-ho @RyanL1997 following up on this issue to see if its still targeted for 2.12.0 release? |
Closing this as all instances of admin credentials and requested changes are tracked as part this meta: opensearch-project/security#3624 |
Is your feature request related to a problem? Please describe
According to the changes in security plugin of security demo configuration script: opensearch-project/security#3329. This change introduces an alternative to the default credentials admin:admin the security plugin currently uses.
In this implementation,
a new setting is added to the opensearch.yml file. This setting,During the set up of the security demo config, it will ask user to provide an initial admin password. User can either provide that by defining an environment variable 'initialAdminPassword' with a password string, or create a file 'initialAdminPassword.txt' with a single line that contains the password string and place it under the config folder. The absent of this 'initialAdminPassword' will lead to the failure of the demo config script.plugins.security.authcz.admin.password
is parsed on plugin start up and then propagated into the internal user store of each node when they launchFor adapting this change, we need to change the build script to handle the setup for admin password.
Describe the solutions you'd like
We have a talk with @RyanL1997 @DarshitChanpura @peterzhuamazon on this for a bit:
Solution A: Adding a parameter for running the demo config script to set the default admin password as 'admin' (sample implementation in security):
We can add a parameter to security demo configuration setup script, which if not provide this parameter, user will be prompted to provide a secure admin password, and if user provides this parameters which means user decides to skip the setup of admin password, then we’ll proceed with "admin" as default admin password and tell user that they acknowledge to accept an insecure password for this non-prod env.
We believe this change is justifiable given that the primary intent of the demo configuration script is for non-production environments. Our documentation clearly underscores our recommendation against using this demo config for actual production setups. In line with this, introducing this parameter remains consistent with the script's intended use, emphasizing that it is not suitable for production deployments.
Solution B: Users have to be prompted to enter an initial admin password during the set up of security demo configuration script (sample implementation in security):
In the current design, the default
internal_user.yml
contains the defaultadmin:admin
credential inside of it (source code). According to @scrawfor99's original PR, it will only replace the default admin hash with the hash of new password. However, since the failure of demo configuration setup will not fail the initiation of the cluster, the admin credential will remain the same if there is a absent of provided admin password by user during the setup of demo configuration script.So instead of replace the default admin hash, we are removing the default admin credential from
internal_user.yml
. This solution introduces the method that if we failed to set up a admin credential during the set up of demo configuration script, it will lead to an absent of admin credential section in internal_users.yml, so that the initiation of the cluster will be failed.Additional context
Original Change in security repo: opensearch-project/security#3329
Discussions on the original issue: opensearch-project/security#3285
Issues
install_demo_configuration
script shows confusing log security#3489admin:admin
credential ifinitialAdminPassword
is not provided security#3508The text was updated successfully, but these errors were encountered: