-
Notifications
You must be signed in to change notification settings - Fork 121
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
Pass in initialAdminPassword #669
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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 |
Signed-off-by: Derek Ho <dxho@amazon.com>
I didn't get this point - do you just mean something that is random/doesn't contain whole words?
Done |
Signed-off-by: Derek Ho <dxho@amazon.com>
@dblock @nhtruong can you take another look at this PR? I'm wondering as to why the CI doesn't break, since |
@DarshitChanpura take a look at @derek-ho's comment above.
|
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. |
@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:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
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
.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be version based? admin
for <2.12 and myStrongPassword123!
for >=2.12.
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. |
@nhtruong Correct,
which is what you originally referred to here: #669 (comment) |
For released versions workflow |
We can make the tests using security plugin to pull password from an env variable and have it fallback to |
That fallback simply won't work as there is a strong password check in place and The proposed approach here is to conditionally use the custom (e.g. |
I meant setting the client's env var to the strong password when we use 2.12 and up. But either works :) |
@nhtruong Sounds good. Would you as a maintainer would like to help suggest/make necessary changes to bring this to completion? |
@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. |
Ty so much @nhtruong !! |
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. |
Cant push to someone else's protected branch (main) unfortunately. Imma create a new draft to avoid spamming yall with notifications anyway. |
@@ -13,6 +13,7 @@ services: | |||
- discovery.type=single-node | |||
- bootstrap.memory_lock=true | |||
- SECURE_INTEGRATION=${SECURE_INTEGRATION:-false} | |||
- OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
Closing this PR in lieu of #707 |
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
yarn run lint
doesn't show any errorsBy 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.