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

[Spike] Replace default admin password #3560

Closed
23 tasks
DarshitChanpura opened this issue Oct 17, 2023 · 5 comments
Closed
23 tasks

[Spike] Replace default admin password #3560

DarshitChanpura opened this issue Oct 17, 2023 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@DarshitChanpura
Copy link
Member

DarshitChanpura commented Oct 17, 2023

Problem Statement:

The current security plugin requires the admin password to be hardcoded, posing a security risk. To enhance security, we are proposing to modify the security plugin to enable cluster accept a custom password from the customer. If none is provided, the cluster should generate a secure password and pass it back to the consuming eco-system. This eco-system could be any of the supported OpenSearch distributions.

Past Implementation:

To resolve this issue, a design proposal[1][2] was put forward and subsequently implemented. The new approach allowed for the provision of a fresh admin password either through an environment variable or by utilizing a simple single-word text file. In cases where neither was provided, the installation demo configuration script would terminate, preventing any further setup of the security plugin. However, this script termination did not halt the cluster setup (e.g., via docker or through the execution of ./opensearch-install.sh). Consequently, the cluster was established with the default credentials admin:admin. This outcome was contrary to the original intention of the implementation, which aimed to have the demo script modify the internal_users.yml file to substitute the hash value for the admin user. Since the hash value remained unchanged and the cluster setup persisted, the admin user was consequently created with the default admin password. This situation prompted a reevaluation of our strategy for replacing the default admin password.

Proposed Solution:

This is a multi-phase implementation that requires making changes in a few repositories. The most attention required outside security repository for this change would be opensearch-build as it would ultimately produce almost all distributions of Opensearch.

Phase 1: Designing with Security in mind

Maintainers of the security team would finalize the design.

The new design picks up the previous design, where a user is required to provide a custom password for admin. If none is provided, a new password will be provided.

If a password was generated, we can make the user aware about it one of the following two ways:

Option 1. Print it in the logs:

  • Pros:
    • This approach is picked up from the previous implementation, and would require the demo config setup flow to print the new password.
  • Cons
    • This may be inconvenient for users using programmatic access to query the cluster using admin cluster. They would have to modify their code to now filter out the logs and fetch the password.

Option 2. Print it to the text file:

  • Pros:
    • This requires the setup script to print the password out to a text file (similar to the way we expect a user to provide a password in a text file)
  • Cons:
    • This is much cleaner for user using programmatic access to now read the password from the text file (easier than filtering logs), and can be available for user to access it without the noise of logs. This file can also be deleted easily by the user to prevent leakage which could happen from logs since they are not deleted.

If a password was provided, it will be set for admin user, and the password will not be printed since the user is already aware of the password.

sequenceDiagram
    participant User
    participant Cluster
    participant Script

    User->>Cluster: Start cluster setup with security plugin
    Cluster->>Script: Execute demo script
    alt User provided password
        Script->>Script: Search for user provided password
    else No password provided
        Script->>Script: Generate new password
        Script->>User: Notify the user about the new password
    end
    Script->>Cluster: Continue with demo script execution
    alt Demo script execution successful
        Script->>Cluster: Notify demo script execution complete
    end
    Cluster->>Cluster: Continue with security plugin configuration
Loading

Phase 2: Ensuring the changes work with all OpenSearch distributions

Work with the opensearch-build team to finalize changes in the build repo, to ensure the distributions are shipped and will spin up the cluster as expected. (Either with the user provided password or one generated by the security plugin)

After Phase-1 is complete, next step would be to ensure that all the open-source distributions are shipped with this change.

To ensure this, work with the opensearch-build team to ensure all distribution have necessary changes, those changes are tested and ready to be shipped.

Refer to this comment to understand changes needed and the distributions supported.

Phase 3: Driving a campaign to fix all hard-coded instances across all repos

Post previous implementation, several hard-coded instances of admin:admin and demo configuration script usage were discovered across multiple repos. This phase will require driving a campaign to fix these.

During this phase, urge all the plugins currently using the hard-coded instance of admin-password to replace it with a custom generated value.

Here are the instances of demo-configuration script found across the repos: opensearch-project/opensearch-build#4094 (comment)

Maybe modify the demo config setup completely? SQL plugin has done something similar to fetch certificates from directly from security plugin repo: https://github.com/opensearch-project/sql/blob/297e26f9622e66cf01c777d229e9b13dbc19525d/integ-test/build.gradle#L82

Considerations:

  • Ensure the security of the environment variable and the password generation algorithm.
  • Validate the complexity and strength of the generated passwords.
  • Maintain backward compatibility with the previous versions of the security plugin.

Risks:

  • Potential leakage of admin password (secure practices should be implemented by user to block access to the new admin password)

Acceptance Criteria (Consumer perspective)

  • Docker : A docker container should spin-up without hardcoded admin password. If no password is provided one should be generated. A call with admin:admin should fail.
  • Tar: Tar installation script should succeed without hardcoded admin password. If no password is provided one should be generated. A call with admin:admin should fail.
  • ZIP: Zip installation script should succeed without hardcoded admin password. If no password is provided one should be generated. A call with admin:admin should fail.
  • RPM: RPM is meant to be one-click install. User should be notified about the admin password. A call with admin:admin should fail.
  • DEB: DEB is meant to be one-click install. User should be notified about the admin password. A call with admin:admin should fail.
  • Helm: Similar to docker, helm charts should spin-up the cluster without hardcoded admin password. The configuration should have an option to provide an admin password. If no password is provided one should be generated. A call with admin:admin should fail.
  • Ansible: Already has an option to set admin password with admin_password variable. If value is not provided, a password should be generated. A call with admin:admin should fail.
  • Automation tests: All automation tests should now provide a custom password. Calls with admin:admin should fail.

Exit Criteria (Developer team perspective)

  • Implement/Modify the solution in security plugin
    • A custom admin password should be accepted via an environment variable or a file
    • A custom admin password must be generated if none was provided, and it should be stored in a file
  • Update CI checks
    • Ensure the intended behavior by testing it locally
    • Update affected CI checks to supply a custom admin password
  • Make necessary updates to all supported distribution build files in opensearch-build repo
    • Identify and update all necessary distribution build files
    • Exhaustive testing to verify the behavior in the new distribution builds is as expected
  • Modify all plugin repos affected with this change
    • Identify all plugins outside security affected by this change
    • Drive a campaign to update these references to provide a custom password value
  • Documentation
    • Document all the changes related to demo script configuration
    • Write a blog post to update the community

# Appendix

Previous Implementation references:

Design:

Implementation:

@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Oct 17, 2023
@DarshitChanpura DarshitChanpura added the enhancement New feature or request label Oct 17, 2023
@peternied peternied pinned this issue Oct 17, 2023
@DarshitChanpura DarshitChanpura self-assigned this Oct 18, 2023
@RyanL1997
Copy link
Collaborator

RyanL1997 commented Oct 19, 2023

Hi @DarshitChanpura, first of all, thank you for creating this issue. I'd like to share my thoughts regarding the current spike and the options we have.

The main point of discussion in phase 1 seems to revolve around the design and approach we should adopt. Here are the two options we discussed before:

Option 1: Continuing with the Existing Demo Configuration Script:

In my view, this option appears to be more of a short-term solution or a mitigation strategy. Based on the previous work done in PR #3329 and PR #3449, it seems we can address this by entirely removing the default admin credential from the internal_users.yml configuration file. This would require users to set up their own admin credentials during the setup script execution. If there is an absence of the initialAdminPassword or, in other words, no admin credential in the internal_users.yml config file, cluster initialization would fail. One potential implementation idea is to check internal_users.yml during cluster initialization and exit/terminate the initialization process if the admin section is missing.

  • Pros:

    • Relatively straightforward implementation.
    • Likely to have the smallest impact on the current build flow.
  • Cons:

    • The demo configuration will likely be deprecated in the future, which means users may need to adjust their behavior again. However, it's worth noting that this approach is intended as a short-term solution or mitigation of the new behavior.

Option 2: Transitioning to a Java Tool for Demo Configuration Setup:

In my opinion, this option aims to be a more long-term solution. It aligns with the original plan to deprecate the current demo configuration script. Some internal discussions suggest that this approach is in line with our broader goals.

  • Pros:

    • One-time change: We only need to make this switch to introduce our new behavior.
  • Cons:

    • It may have a larger impact on the current build flow.
    • Longer release cycle -relatively more complicated implementation so that it may require app sec review??? (Not sure)

Just a heads-up, the information above is based on my understanding. I'd like to hear your thoughts and any additional ideas you might have on these options.

@peternied
Copy link
Member

To enhance security, we aim to modify the plugin to accept a new password from the customer in the form of an environment variable. If none is provided, the system should generate a secure password.

I disagree with this problem statement - maybe we should start here before we go into solutions? I think we should accept a provided password - or if none is provided make one and pass it back out to the consuming ecosystem.


I think we are missing specific platforms and how the change will be picked up by them - I think working backwards from how a change gets to the platforms we want has been the biggest missing piece of the previous attempt.

What do you think?

@davidlago
Copy link

Thanks @DarshitChanpura for putting this together (and everyone else for your feedback as well).

I think we are coalescing around a path forward here, but I echo @peternied's feedback that the way we are getting there is not quite working backwards from a clear problem statement. The exit criteria looks too much like a laundry list of things we need to do vs. a list of verifications we can do from the problem statement's point of view. For example, I would like to have a clear exit criteria checkbox that reads:

  • A container spun up with our Docker image with default settings spins up and does not have a hardcoded admin password (calling it with admin:admin should fail)

The same for every other way we can validate that the problem statement was in fact addressed for every distribution. Then we work backwards from those changes and identify what needs to be done for each in terms of build changes, CI updates, and ramifications to other repos.

Makes sense? to reiterate, I think the solution we are arriving at here makes sense, but I would love for us to arrive at it working backwards from a clear exit criteria, as a) it'll keep us honest and make sure we did address it and b) it will also help with us getting too much tunnel vision with our pre-conceived ideas/approaches.

Food for thought: one should be able to write the exit criteria from the problem statement alone, without even having settled on an approach.

@cwperks cwperks removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Oct 23, 2023
@peternied
Copy link
Member

Around the phasing, I'd suggest that we frontload building out an end to end flow for a specific distribution. There might be one that can be done independently of the others. This could allow for learnings in a lower risk timeline before we start clearing out all the distributions implementations.

@DarshitChanpura
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants