Pass in initialAdminPassword #669

derek-ho · 2023-12-13T15:56:30Z

Description

Due to recent changes in the security plugin, the install demo configuration script now requires an initial admin password to be set. This sets it to some random variable so that the CI continues to work.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

Check List

New functionality includes testing.
- All tests pass
Linter check was successfull - yarn run lint doesn't show any errors
Commits are signed per the DCO using --signoff
Changelog was updated.

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>

dblock

To fix CI don't you just need to say initialAdminPassword=admin in docker-compose.yml?

I think the documentation should be left alone. Or we should explain how to obtain that initial password.

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

derek-ho · 2023-12-13T16:25:21Z

To fix CI don't you just need to say initialAdminPassword=admin in docker-compose.yml?

I think the documentation should be left alone. Or we should explain how to obtain that initial password.

Since "admin" is considered a weak password, this wouldn't be possible. There is weak validation in place, and it needs a strong password. Password validation is being done via this library: https://github.com/dropbox/zxcvbn. It is possible to pass in a weak password if running the install-demo-configuraiton script by yourself, but if you are using the distributions we don't currently support it.

dblock · 2023-12-13T17:06:32Z

To fix CI don't you just need to say initialAdminPassword=admin in docker-compose.yml?
I think the documentation should be left alone. Or we should explain how to obtain that initial password.

Since "admin" is considered a weak password, this wouldn't be possible. There is weak validation in place, and it needs a strong password. Password validation is being done via this library: https://github.com/dropbox/zxcvbn. It is possible to pass in a weak password if running the install-demo-configuraiton script by yourself, but if you are using the distributions we don't currently support it.

I see. I think we should hardcode a non-readable password in our docker setup.

I would say admin:<initial password> rather than ${initialAdminPassword} that implies some kind of variable.

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

derek-ho · 2023-12-14T15:06:20Z

To fix CI don't you just need to say initialAdminPassword=admin in docker-compose.yml?
I think the documentation should be left alone. Or we should explain how to obtain that initial password.

Since "admin" is considered a weak password, this wouldn't be possible. There is weak validation in place, and it needs a strong password. Password validation is being done via this library: https://github.com/dropbox/zxcvbn. It is possible to pass in a weak password if running the install-demo-configuraiton script by yourself, but if you are using the distributions we don't currently support it.

I see. I think we should hardcode a non-readable password in our docker setup.

I didn't get this point - do you just mean something that is random/doesn't contain whole words?

I would say admin:<initial password> rather than ${initialAdminPassword} that implies some kind of variable.

Done

guides/msearch.md

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

derek-ho · 2023-12-28T17:45:41Z

@dblock @nhtruong can you take another look at this PR? I'm wondering as to why the CI doesn't break, since admin:myStrongPassword123! shouldn't work on secure clusters until 2.12.0 is released. Let me know what you folks think is an appropriate path forward for this PR, thanks! CIs of other PRs are also passing, so I am suspecting we may not even need this PR - I don't think the healthcheck command is working correctly.

DarshitChanpura · 2024-01-17T16:55:10Z

@dblock @nhtruong Can we get more reviews on this PR?

dblock · 2024-01-17T22:18:27Z

@DarshitChanpura take a look at @derek-ho's comment above.

Why is this not breaking?
Do we need this change before 2.12?

derek-ho · 2024-01-22T15:38:53Z

Putting this PR in draft until 2.12.0 is released, since it looks like the workflows are only using released docker versions from opensearchproject. I am still confused why changing the healthcheck command to non - "admin" password is still passing, since released versions up until now should not take up the new password. Maybe something to look into after release.

DarshitChanpura · 2024-01-22T15:40:57Z

@dblock, Derek and I are looking to get feedback from maintainers here to understand whether CI is run against 2.12 and if so is there any codepath that we should address. Right now, based on our understanding, we suspect that the health checks might not be working, and we would like the feedback on the path forward for this PR.

We are not familiar with this code-base and initially raised this PR (and others across different repositories) to help with the changes needed as part of larger effort: opensearch-project/security#3624 and are seeking guidance from maintainers on the path forward. Here are some questions we had:

Is CI run against 2.12?
Is there any branching strategy required moving forward to pick admin password for requests against < 2.12?
If the CI is not run against 2.12, why is health check passing with the new password?

nhtruong · 2024-01-22T21:35:53Z

@DarshitChanpura

CI is running against 2.x and main: https://github.com/opensearch-project/opensearch-js/blob/main/.github/workflows/integration-unreleased.yml#L24-L25
Shouldn't the password used in this PR work against < 2.12 too?

DarshitChanpura

CI is running against 2.x and main: https://github.com/opensearch-project/opensearch-js/blob/main/.github/workflows/integration-unreleased.yml#L24-L25

If I am reading the workflow right, the workflow uses min OpenSearch distribution in which case security will not be installed (as it doesn't exist as part of min distribution) and no admin password is required.

opensearch-js/.github/workflows/integration-unreleased.yml

Line 61 in 18389de

tar xf opensearch-min-*

Shouldn't the password used in this PR work against < 2.12 too?

The password change is only for versions 2.12 and later. For versions <2.12 the password is still admin.

DarshitChanpura · 2024-01-23T18:47:54Z

.ci/opensearch/Dockerfile.opensearch

@@ -9,6 +9,6 @@ ARG SECURE_INTEGRATION
 HEALTHCHECK --start-period=20s --interval=5s --retries=2 --timeout=1s \
  CMD if [ "$SECURE_INTEGRATION" != "true" ]; \
  then curl --fail localhost:9200/_cat/health; \
-  else curl --fail -k https:/localhost:9200/_cat/health -u admin:admin; fi
+  else curl --fail -k https:/localhost:9200/_cat/health -u admin:myStrongPassword123!; fi


Should this change be version based? admin for <2.12 and myStrongPassword123! for >=2.12.

nhtruong · 2024-01-23T19:01:54Z

If I am reading the workflow right, the workflow uses min OpenSearch distribution in which case security will not be installed (as it doesn't exist as part of min distribution) and no admin password is required.

The CI uses 2 different configuration for security: On and Off https://github.com/opensearch-project/opensearch-js/blob/main/.ci/opensearch/Dockerfile.opensearch#L14

I believe we need to turn it on for that workflow like this and run this set of tests instead.

DarshitChanpura · 2024-01-23T19:06:37Z

The CI uses 2 different configuration for security: On and Off https://github.com/opensearch-project/opensearch-js/blob/main/.ci/opensearch/Dockerfile.opensearch#L14

@nhtruong Correct, integration-unreleased.yml workflow seems to be using a min distribution of opensearch:

opensearch-js/.github/workflows/integration-unreleased.yml

Line 61 in 18389de

tar xf opensearch-min-*

which is what you originally referred to here: #669 (comment)

DarshitChanpura · 2024-01-23T19:11:31Z

For released versions workflow integration.yml the changes introduced in this PR (with a modification I suggested above) will be required once 2.12 is released and added to the workflow matrix

nhtruong · 2024-01-23T19:12:21Z

The password change is only for versions 2.12 and later. For versions <2.12 the password is still admin.

We can make the tests using security plugin to pull password from an env variable and have it fallback to admin if the variable does not exist.

DarshitChanpura · 2024-01-23T19:16:28Z

We can make the tests using security plugin to pull password from an env variable and have it fallback to admin if the variable does not exist.

That fallback simply won't work as there is a strong password check in place and admin is not a strong password.

The proposed approach here is to conditionally use the custom (e.g. myStrongPassword123!) / default (admin) password based on the version being tested.

nhtruong · 2024-01-23T19:41:10Z

I meant setting the client's env var to the strong password when we use 2.12 and up. But either works :)

DarshitChanpura · 2024-01-23T20:22:32Z

@nhtruong Sounds good. Would you as a maintainer would like to help suggest/make necessary changes to bring this to completion?

nhtruong · 2024-01-23T20:28:56Z

@DarshitChanpura for sure. Lemme find a good stopping point for my current project before switching context. I will be able get on this tomorrow or Thu.

DarshitChanpura · 2024-01-23T21:21:14Z

Ty so much @nhtruong !!

nhtruong · 2024-01-29T16:07:34Z

I've started taking a crack at this. Will create a new PR but leaving this open for discussion.

DarshitChanpura · 2024-01-29T16:39:31Z

I've started taking a crack at this. Will create a new PR but leaving this open for discussion.

Thank you @nhtruong. Btw, as a maintainer you should have access to pushing changes directly on this PR.
You can simply checkout this PR: gh pr checkout 669 and starting commiting/pushing the changes.

nhtruong · 2024-01-29T20:01:58Z

Cant push to someone else's protected branch (main) unfortunately. Imma create a new draft to avoid spamming yall with notifications anyway.

nhtruong · 2024-01-29T21:03:16Z

.ci/opensearch/docker-compose.yml

@@ -13,6 +13,7 @@ services:
      - discovery.type=single-node
      - bootstrap.memory_lock=true
      - SECURE_INTEGRATION=${SECURE_INTEGRATION:-false}
+      - OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123!


@DarshitChanpura with OS 2.12 and Up, would this be enough to set the admin password? Does the plugin just grab what stored in OPENSEARCH_INITIAL_ADMIN_PASSWORD, check if it's strong enough, and then assign it as password for admin user?

I also saw this in the security plugin repo: https://github.com/opensearch-project/security/pull/3843/files
Does that mean that if we set initialAdminPassword to myStrongPassword123!, that password will also work for older version of OS?

@DarshitChanpura with OS 2.12 and Up, would this be enough to set the admin password? Does the plugin just grab what stored in OPENSEARCH_INITIAL_ADMIN_PASSWORD, check if it's strong enough, and then assign it as password for admin user?

Yes, that is correct. The env variable will be picked up when running demo configuration.

I also saw this in the security plugin repo: https://github.com/opensearch-project/security/pull/3843/files
Does that mean that if we set initialAdminPassword to myStrongPassword123!, that password will also work for older version of OS?

There is no concept of custom admin passwords for versions prior to 2.12. This will be introduced in 2.12 and will be required henceforth when using security plugin's demo configuration. Please note that variable is OPENSEARCH_INITIAL_ADMIN_PASSWORD and not initialAdminPassword.

DarshitChanpura · 2024-02-22T21:28:07Z

Closing this PR in lieu of #707

derek-ho added 2 commits December 13, 2023 10:53

Pass in initialAdminPassword

eebf1ae

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

Pass in env variable

5cd431d

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

derek-ho requested review from ananzh, kavilla, dblock, VachaShah, nhtruong, harshavamsi and timursaurus as code owners December 13, 2023 15:56

Also update all the instances in documentation

40393a1

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

derek-ho mentioned this pull request Dec 13, 2023

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

Closed

2 tasks

Update changelog

667d3b6

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

dblock requested changes Dec 13, 2023

View reviewed changes

Update instances to use backticks

9d987bf

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

Change according to new variable and PR review suggestion

d221bc7

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

nhtruong reviewed Dec 18, 2023

View reviewed changes

guides/msearch.md Show resolved Hide resolved

Change according to PR suggestions

47b573e

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

derek-ho marked this pull request as draft January 22, 2024 15:38

wbeckler mentioned this pull request Jan 22, 2024

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

Closed

2 tasks

DarshitChanpura reviewed Jan 23, 2024

View reviewed changes

nhtruong reviewed Jan 29, 2024

View reviewed changes

DarshitChanpura closed this Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass in initialAdminPassword #669

Pass in initialAdminPassword #669

derek-ho commented Dec 13, 2023

dblock left a comment

derek-ho commented Dec 13, 2023 •

edited

Loading

dblock commented Dec 13, 2023

derek-ho commented Dec 14, 2023

derek-ho commented Dec 28, 2023

DarshitChanpura commented Jan 17, 2024

dblock commented Jan 17, 2024

derek-ho commented Jan 22, 2024

DarshitChanpura commented Jan 22, 2024 •

edited

Loading

nhtruong commented Jan 22, 2024 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading

DarshitChanpura Jan 23, 2024

nhtruong commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 23, 2024

DarshitChanpura commented Jan 23, 2024 •

edited

Loading

nhtruong commented Jan 23, 2024

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 29, 2024

DarshitChanpura commented Jan 29, 2024

nhtruong commented Jan 29, 2024

nhtruong Jan 29, 2024

nhtruong Jan 29, 2024 •

edited

Loading

DarshitChanpura Jan 30, 2024

DarshitChanpura commented Feb 22, 2024

Pass in initialAdminPassword #669

Pass in initialAdminPassword #669

Conversation

derek-ho commented Dec 13, 2023

Description

Issues Resolved

Check List

dblock left a comment

Choose a reason for hiding this comment

derek-ho commented Dec 13, 2023 • edited Loading

dblock commented Dec 13, 2023

derek-ho commented Dec 14, 2023

derek-ho commented Dec 28, 2023

DarshitChanpura commented Jan 17, 2024

dblock commented Jan 17, 2024

derek-ho commented Jan 22, 2024

DarshitChanpura commented Jan 22, 2024 • edited Loading

nhtruong commented Jan 22, 2024 • edited Loading

DarshitChanpura left a comment • edited Loading

Choose a reason for hiding this comment

DarshitChanpura Jan 23, 2024

Choose a reason for hiding this comment

nhtruong commented Jan 23, 2024 • edited Loading

DarshitChanpura commented Jan 23, 2024 • edited Loading

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 23, 2024 • edited Loading

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 23, 2024

DarshitChanpura commented Jan 23, 2024 • edited Loading

nhtruong commented Jan 23, 2024

DarshitChanpura commented Jan 23, 2024

nhtruong commented Jan 29, 2024

DarshitChanpura commented Jan 29, 2024

nhtruong commented Jan 29, 2024

nhtruong Jan 29, 2024

Choose a reason for hiding this comment

nhtruong Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

DarshitChanpura Jan 30, 2024

Choose a reason for hiding this comment

DarshitChanpura commented Feb 22, 2024

derek-ho commented Dec 13, 2023 •

edited

Loading

DarshitChanpura commented Jan 22, 2024 •

edited

Loading

nhtruong commented Jan 22, 2024 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading

nhtruong commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024 •

edited

Loading

nhtruong commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024 •

edited

Loading

nhtruong Jan 29, 2024 •

edited

Loading