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

Replace admin:admin default credentials with configuration file password #3285

Closed
5 tasks done
stephen-crawford opened this issue Sep 1, 2023 · 13 comments · Fixed by #3329
Closed
5 tasks done

Replace admin:admin default credentials with configuration file password #3285

stephen-crawford opened this issue Sep 1, 2023 · 13 comments · Fixed by #3329
Assignees
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@stephen-crawford
Copy link
Contributor

stephen-crawford commented Sep 1, 2023

This issue describes the tasks required to replace the basic admin:admin default credentials with an alternative method which takes a provided password from the openseach.yml file to be used as the default password. This issue results from:

Feedback from @peternied and @cwperks on a spike I created.

While I think OTP could be a useful feature down the road, specifically for addressing requests to be able to run as a superadmin while SSL is disabled, for now, we can start with making the default password configurable via a configuration file value and a env. variable.

Craig said:

"
I like this approach and have seen it used in a number of places. Here's an example for keycloak where its passed in as an env variable in a docker-compose file:

version: '3'
services:
  postgres:
      image: postgres
      volumes:
        - postgres_data:/var/lib/postgresql/data
      environment:
        POSTGRES_DB: keycloak
        POSTGRES_USER: keycloak
        POSTGRES_PASSWORD: password
  keycloak:
      image: quay.io/keycloak/keycloak:legacy
      environment:
        DB_VENDOR: POSTGRES
        DB_ADDR: postgres
        DB_DATABASE: keycloak
        DB_USER: keycloak
        DB_SCHEMA: public
        DB_PASSWORD: password
        KEYCLOAK_USER: admin
        KEYCLOAK_PASSWORD: Pa55w0rd
        KEYCLOAK_FRONTEND_URL: http://localhost:8080/auth
        # Uncomment the line below if you want to specify JDBC parameters. The parameter below is just an example, and it shouldn't be used in production without knowledge. It is highly recommended that you read the PostgreSQL JDBC driver documentation in order to use it.
        #JDBC_PARAMS: "ssl=true"
      ports:
        - 8080:8080
      depends_on:
        - postgres

volumes:
  postgres_data:
      driver: local

networks:
  opensearch-net:

I'd like to try to keep the simplicity of starting a demo cluster with docker-compose, where someone trying to start a cluster with the sample docker-compose.yml file can do docker-compose up and the cluster is brought up.

With the env variable approach, there would be one additional step to bring up a node or starting the cluster with docker. It would be to supply OPENSEARCH_ADMIN_PASSWORD or the node could not be brought up. A sample docker-compose.yml file for a single node could look like:

version: '3'
services:
  opensearch-node1: # This is also the hostname of the container within the Docker network (i.e. https://opensearch-node1/)
    image: opensearchproject/opensearch:latest # Specifying the latest available image - modify if you want a specific version
    container_name: opensearch-node1
    environment:
      - cluster.name=opensearch-cluster # Name the cluster
      - node.name=opensearch-node1 # Name the node that will run in this container
      - discovery.seed_hosts=opensearch-node1,opensearch-node2 # Nodes to look for when discovering the cluster
      - cluster.initial_cluster_manager_nodes=opensearch-node1,opensearch-node2 # Nodes eligible to serve as cluster manager
      - bootstrap.memory_lock=true # Disable JVM heap memory swapping
      - "OPENSEARCH_JAVA_OPTS=-Xms512m -Xmx512m" # Set min and max JVM heap sizes to at least 50% of system RAM
      - "OPENSEARCH_ADMIN_PASSWORD=w3akP@ssw0rd2023!"
    ulimits:
      memlock:
        soft: -1 # Set memlock to unlimited (no soft or hard limit)
        hard: -1
      nofile:
        soft: 65536 # Maximum number of open files for the opensearch user - set to at least 65536
        hard: 65536
    volumes:
      - opensearch-data1:/usr/share/opensearch/data # Creates volume called opensearch-data1 and mounts it to the container

In addition to admin:admin being widely known there are a few other widely known secrets too:

The admin certificate and key should also be treated as a default password (they are) and anyone connecting with them can do anything
`kibanaserver:kibanaserver`

"

And I agree.

The tasks required for these are listed on the document linked above, so I won't duplicate them here. Support for the configuration file will be prioritized first but both should ultimately be supported.

The tasks required to complete this include:

@stephen-crawford stephen-crawford added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Sep 1, 2023
@stephen-crawford
Copy link
Contributor Author

Going to add the configuration value to the opensearch.yml file instead of as a separate file.

@stephen-crawford
Copy link
Contributor Author

I was able to confirm the setting parsing etc. is correct but there is still an issue I am facing with how to populate the updated password. I think the way to go is to hash the password and then use it to update the internal users configuration using the UserService. This is nice because we know this will be consistent and it reuses a lot of the logic.

The issue is determining when the config is accessible... still not sure how to do that.

@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Sep 7, 2023

I made a different solution which just modifies the yaml file directly during the creation of the configuration repository. Going to ask Craig which he thinks seems like a better idea... I am not convinced by either.

What I shared with Craig:

There are a couple problems, but they change on the implementation.

The easy part is done: add in a setting which someone can provide a new password to use as the default; if they don't provide one we use "admin" like before

After populating the settings, we need to then update everything that expects the admin password to be admin. So there are two ways to do that: 1) use the internal users api logic to replicate a user configuration change and use the new password provided as the only change; 2) directly modify the internal_users yaml so that before the file is registered we have already swapped the password.

Each has its own problems...

  1. Does not play well with the node start process because the configuration load is treated as a background thread and asynchronous. The service will inevitably be unavailable if we try to call the user service to update the password during the node start process.

  2. Relies on modifying the actual contents of the yaml file and has its own issues aside from being inelegant. I am not as convinced on the synchronicity of this change as the other version and I have to do string replacement which is highly dependent on the specific ordering and contents of the internal users yml file.

@davidlago
Copy link

The easy part is done: add in a setting which someone can provide a new password to use as the default; if they don't provide one we use "admin" like before

This goes against the spirit of this work: to not have admin:admin as a default. If they don't provide it and we use that as a default, we are back to square one.

@cwperks
Copy link
Member

cwperks commented Sep 8, 2023

@scrawfor99 This is the current block where all internal_users are created: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java#L155-L161

It would be great if the admin password can be hashed and saved in the internal user list in the same call as it is now. As Dave said above, no default, if the setting is missing the node should not be brought up. That being said, I would support adding this into the install_demo_configuration scripts and having a demo value as default with a log warning a user that they are relying on demo values and should be changed when using in a production environment.

Implementation wise, I think what you mentioned about editing yaml files is on the right track but no files need to be edited directly.

In the startup flow the internal_users.yml file is read in with this call: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/ConfigHelper.java#L88

Following the call stack it leads down to fromYamlReader here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/ConfigHelper.java#L157

Do you think it makes sense to add a condition in this block that if ctype is CType.INTERNAL_USERS that it calculates the hash and assigns the hash to the admin user within there?

@stephen-crawford
Copy link
Contributor Author

@davidlago, I understand that the intention is to replace the default admin password. However, there is no way to use a setting if we don't provide a default. We cannot have it empty as that will not allow any of the demos to work. If we want an out-of-the-box demo with securtiy to work, we either have to provide a password in all of the demo scripts--bringing us to where we are now--or set a default setting value as I did.

If we do neither of these the demos will not work. I was under the impression that this change was not expected to remove any functionality so was going to do the best I could to remove the defaults but still provide coverage where needed. If you are fine breaking the demos than I will remove any safe guards against an empty field.

@stephen-crawford
Copy link
Contributor Author

Hi @cwperks, thanks for taking a minute to provide some more thoughts. See above about my concerns for completely removing any default.

If @davidlago is okay with breaking out-of-the-box demos then I will do it, but I did not want to do that on my own.

For your other suggestions, I will take a look. I had not messed with the ConfigHelper yet so maybe that will work well. I did you try directly modifying the configuration repository before but I will give it another shot with the route you mentioned.

@davidlago
Copy link

davidlago commented Sep 8, 2023

I think it is acceptable that if you don't provide a value for something as important as an admin password, we fail to start the cluster.

I would decouple the problem into:

  1. Making sure we don't have defaults and one must be provided to start the cluster. Provided how?
    a. Configuration file (this issue)
    b. Environment value (Replace admin:admin default credentials with environment variable password  #3286)
    c. Something else? (OTP etc)
  2. Deal with the logistics of how demos and out-of-the-box container images work after this change. Perhaps have the demo scripts auto-generate a password, pass it down via writing to the config file or via env values, and then spit it out as part of the script output?

We can, also separately, think about what the backport and breaking changes considerations are as we merge any/all of these to 2.x. For example, we will have to have both 1 and 2 above before we backport all of them at once into 2.x, or else we'll be breaking demos.

@stephen-crawford
Copy link
Contributor Author

Hi @davidlago, sounds good. Sorry for the confusion on my end. To be clear, I did recognize that some of the changes I was proposing may seem counterintuitive but the intention was to preserve the demo scripts. I will remove that support.

@davidlago
Copy link

We can still preserve them, and adapt them to work with whichever new more secure approach we use... just not necessarily as part of this PR.

@stephen-crawford
Copy link
Contributor Author

I added a change to the SingleClusterTest so that every integration test will have a password to use but if one is not provided we throw a RTE.

@stephen-crawford
Copy link
Contributor Author

Changed the setting use so that the configuration repository would insert the new admin user into the internal user configuration directly upon start. However, there is an issue since the nodes won't load and the configurations are not getting properly updated. Looking for a potential fix but tried everything obvious that I could think of.

@stephen-crawford
Copy link
Contributor Author

[Triage] This issue is a common request and important for OpenSearch to exercise best security practices. Going to mark as triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Sep 11, 2023
peternied added a commit that referenced this issue Oct 3, 2023
Backport 8628a89 from #3329.

### Related Issues
-  Related #3285

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Peter Nied <[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>
Co-authored-by: Peter Nied <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants