Remove references to default admin creds #430

derek-ho · 2023-12-28T20:15:34Z

Description

With 2.12.0 release, the security plugin is making a change that requires an initial admin password to be set via an environment variable. This PR makes this change in CI/code/documentation.

Issues Resolved

[List any issues this PR will resolve]

Testing

New functionality includes testing

[Describe how this change was tested]

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: Derek Ho <dxho@amazon.com>

derek-ho · 2023-12-28T20:16:52Z

@IanHoang @gkamat can you folks take a look/ I am expecting this to fail security integration tests, but would be needed to fix the CI after 2.12.0 docker image gets released. I also have some questions if I made the changes correctly, since I didn't fully understand how this repo is using some files. I am assuming some docker files with reference to 1.x line don't need changes/have no plans to be changed in the near future.

derek-ho · 2023-12-28T20:17:12Z

opensearch-project/security#3624

derek-ho · 2023-12-28T20:18:05Z

osbenchmark/resources/docker-compose.yml.j2

@@ -39,7 +39,7 @@ services:
    networks:
      - opensearch-net
    healthcheck:
-          test: curl -f http://localhost:{{http_port}} -u admin:admin --insecure
+          test: curl -f http://localhost:{{http_port}} -u admin:myStrongPassword123! --insecure


This command would fail if os_version < 2.12.0. Not sure how this file is used/how we can conditionally switch this password. Can maintainers help out here?

@derek-ho Off the top of my head, there's a couple ways that we could get around this. Others might have better ideas

You can add an additioonal method that checks the os_version variable that's being passed through and if it's < 2.12.0, use admin. You'll need to make that additional check in this section of provisioner.py and this section of docker_installer.py

I believe you could also leverage a library such asyq https://stackoverflow.com/questions/76526508/replace-a-block-of-yaml but that would require upkeep with this library and adding it in setup.py

derek-ho · 2023-12-28T20:18:53Z

samples/ccr/start.sh

@@ -25,7 +25,7 @@ printf "Waiting for clusters to get ready "
 ALL_CLUSTERS_READY=false

 while ! $ALL_CLUSTERS_READY; do
-    (curl -ks -u admin:admin https://localhost:9200 -o /dev/null && curl -ks -u admin:admin https://localhost:9201 -o /dev/null && ALL_CLUSTERS_READY=true) || (printf "." && sleep 5)
+    (curl -ks -u admin:myStrongPassword123! https://localhost:9200 -o /dev/null && curl -ks -u admin:admin https://localhost:9201 -o /dev/null && ALL_CLUSTERS_READY=true) || (printf "." && sleep 5)


Not sure where the second cluster is coming from, I think the first cluster is coming from docker-compose-metricstore, which is on the latest release. This change will fail CI until 2.12.0 docker image is released, at which point it would fix the CI.

Can maintainers help out here in understanding if the second cluster needs similar changes/where those that cluster's password should be set?

I do not believe this script is being used anymore. @gkamat can you confirm?

@IanHoang I will remove this file then, if its not being used. @gkamat can you also confirm here?

Signed-off-by: Derek Ho <dxho@amazon.com>

IanHoang · 2023-12-29T16:02:37Z

@derek-ho You made the correct changes but also need to make changes to the unittests. Can you update the following tests so that the assertions pass?

tests/builder/provisioner_test.py::DockerProvisionerTests::test_provisioning_with_defaults FAILED [ 20%]
tests/builder/provisioner_test.py::DockerProvisionerTests::test_provisioning_with_variables FAILED [ 20%]
tests/builder/installers/docker_installer_test.py::DockerProvisionerTests::test_provisioning_with_defaults FAILED [ 29%]
tests/builder/installers/docker_installer_test.py::DockerProvisionerTests::test_provisioning_with_variables FAILED [ 29%]

IanHoang

Left some comments

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho · 2024-01-18T15:44:01Z

I tried to add some logic around versioning in the PR. I will put it into draft for now, since it will be easier to take care of once 2.12.0 is released and we can really test the difference between the logic spitting out different health checks based on the version. In any case, the current CI is in a good state and will be so until after the release. @IanHoang let me know if that makes sense for you.

derek-ho · 2024-02-28T20:44:26Z

@IanHoang I just went through this PR once more. Are the dockerfiles that I touched only used to assert that they are read correctly? If that is the case then this PR may not be necessary. Can you point me to the versions of opensearch you are testing against? Does that matrix have 2.12 added to it after the release? I couldn't find it

IanHoang · 2024-03-05T18:24:07Z

@IanHoang I just went through this PR once more. Are the dockerfiles that I touched only used to assert that they are read correctly? If that is the case then this PR may not be necessary. Can you point me to the versions of opensearch you are testing against? Does that matrix have 2.12 added to it after the release? I couldn't find it

The dockerfiles that you touched are used in the integration tests and when users want to specifically run OSB against a Docker version of OpenSearch.

These are the distributions that OpenSearch Benchmark tests against:

DISTRIBUTIONS = ["1.3.9", "2.5.0"]

To test if your changes in docker-compose.yml.j2 really work, you can perform the procedure:

Alter the integration tests in it/__init__.py and change the DISTRIBUTIONS variable to point to a list containing 2.12.0.
In your forked OSB repository, go to Github actions and run the integration tests

In the logs, you can see if any errors occur related to distribution_test.py, specifically test_docker_distribution().

IanHoang · 2024-03-12T17:55:57Z

@derek-ho are you still working on this? If not, can we close this?

Remove references to default admin creds

553325e

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho requested review from IanHoang and gkamat as code owners December 28, 2023 20:15

derek-ho commented Dec 28, 2023

View reviewed changes

Add comment and copy changes to j2

f78b064

Signed-off-by: Derek Ho <dxho@amazon.com>

IanHoang requested changes Dec 29, 2023

View reviewed changes

DarshitChanpura mentioned this pull request Jan 2, 2024

[META] Update all instances of admin:admin across different repositories opensearch-project/security#3624

Closed

2 tasks

add conditional check

992ac3f

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho marked this pull request as draft January 18, 2024 15:44

DarshitChanpura mentioned this pull request Mar 1, 2024

[v2.12.0] Ensure CI/documentation reflect changes to default admin credentials #474

Open

2 tasks

IanHoang closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove references to default admin creds #430

Remove references to default admin creds #430

derek-ho commented Dec 28, 2023

derek-ho commented Dec 28, 2023

derek-ho commented Dec 28, 2023

derek-ho Dec 28, 2023

IanHoang Dec 29, 2023

derek-ho Dec 28, 2023

derek-ho Dec 28, 2023

IanHoang Dec 29, 2023

derek-ho Jan 18, 2024

IanHoang commented Dec 29, 2023 •

edited

Loading

IanHoang left a comment

derek-ho commented Jan 18, 2024

derek-ho commented Feb 28, 2024

IanHoang commented Mar 5, 2024 •

edited

Loading

IanHoang commented Mar 12, 2024

Remove references to default admin creds #430

Remove references to default admin creds #430

Conversation

derek-ho commented Dec 28, 2023

Description

Issues Resolved

Testing

derek-ho commented Dec 28, 2023

derek-ho commented Dec 28, 2023

derek-ho Dec 28, 2023

Choose a reason for hiding this comment

IanHoang Dec 29, 2023

Choose a reason for hiding this comment

derek-ho Dec 28, 2023

Choose a reason for hiding this comment

derek-ho Dec 28, 2023

Choose a reason for hiding this comment

IanHoang Dec 29, 2023

Choose a reason for hiding this comment

derek-ho Jan 18, 2024

Choose a reason for hiding this comment

IanHoang commented Dec 29, 2023 • edited Loading

IanHoang left a comment

Choose a reason for hiding this comment

derek-ho commented Jan 18, 2024

derek-ho commented Feb 28, 2024

IanHoang commented Mar 5, 2024 • edited Loading

IanHoang commented Mar 12, 2024

IanHoang commented Dec 29, 2023 •

edited

Loading

IanHoang commented Mar 5, 2024 •

edited

Loading