Addresses changes made to security demo config install tool #214

DarshitChanpura · 2023-12-15T21:30:43Z

Description

There were recent changes to security demo configuration setup which now requires a custom admin Password to be setup. This PR updates the documentation and related code change.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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: Darshit Chanpura <[email protected]>

codecov · 2023-12-15T21:33:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6dcd996) 83.30% compared to head (a5da908) 83.30%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #214   +/-   ##
=========================================
  Coverage     83.30%   83.30%           
  Complexity      337      337           
=========================================
  Files            43       43           
  Lines          1270     1270           
  Branches        155      155           
=========================================
  Hits           1058     1058           
  Misses          134      134           
  Partials         78       78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msfroh · 2023-12-15T22:01:31Z

Thanks @DarshitChanpura!

Before we merge this, I'm going to make some changes to update the quickstart scripts to actually use newer versions of OpenSearch. (Currently, the Kendra one still downloads OpenSearch 2.7 and the Personalize one downloads 2.9.)

derek-ho

LGTM

helpers/personalized_search_ranking_quickstart.sh

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2024-01-17T18:39:15Z

@msfroh @macohen @macohen @noCharger @mingshl @sejli Can one of the maintainers take a look at the failing CI and help bring this to closure?

msfroh · 2024-01-17T18:48:14Z

helpers/personalized_search_ranking_quickstart.sh

+if [ "$COMPARE_VERSION" != "$OPENSEARCH_REQUIRED_VERSION" ]; then
+  OPENSEARCH_INITIAL_ADMIN_PASSWORD="admin"
+else
+  OPENSEARCH_INITIAL_ADMIN_PASSWORD="myStrongPassword123!"


Rather than hard-coding, we should probably take the admin password as a command-line arg.

@DarshitChanpura -- did earlier versions support the OPENSEARCH_INITIAL_ADMIN_PASSWORD env var? Or does that change come in 2.12?

I'm wondering if it makes sense to change the scripts to require the initial admin password for all versions, where it will be required for 2.12 and a good idea for earlier versions.

Rather than hard-coding, we should probably take the admin password as a command-line arg.

Starting 2.12 a custom strong password is required.

@DarshitChanpura -- did earlier versions support the OPENSEARCH_INITIAL_ADMIN_PASSWORD env var? Or does that change come in 2.12?

This change is introduced in 2.12 and will be present for all versions 2.12 and above

I'm wondering if it makes sense to change the scripts to require the initial admin password for all versions, where it will be required for 2.12 and a good idea for earlier versions.

That might be your design decision as a maintainer. Since the admin password is required only for 2.12 and above, IMO we can keep a version check in place and require it only for >= 2.12

This doesn't answer my question. Allow me to rephrase:

Is it POSSIBLE to set the admin password in docker-compose.yml before 2.12?

If so, I will change the script to always require a custom password, for all versions, since it's a good idea.

Is it POSSIBLE to set the admin password in docker-compose.yml before 2.12?

You can set the env variable OPENSEARCH_INITIAL_ADMIN_PASSWORD in the docker-compose.yml. This value will only be picked up for 2.12 and above

You, can change the script to have a default strong password, and then based on the version it will be picked.

Does that answer your query?

Yes, thank you.

In that case, it does make more sense to accept and enforce it for version 2.12 and higher only, because requiring the parameter on earlier versions and then ignoring it (leaving the default password as admin) would be a terrible user experience.

In fact, we should reject the parameter (or at least output a warning that it's being ignored) if the version is pre-2.12.

I'll make those changes to the script.

Made those changes and pushed the commit to this PR.

@DarshitChanpura, please let me know what you think. Thanks!

For earlier versions, emit a warning if an admin password is specified on the command line, since it will be ignored. Signed-off-by: Michael Froh <[email protected]>

DarshitChanpura

LGTM!

noCharger

LGTM

Require password as an argument on 2.12 and higher For earlier versions, emit a warning if an admin password is specified on the command line, since it will be ignored. --------- Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Michael Froh <[email protected]> Co-authored-by: Michael Froh <[email protected]> (cherry picked from commit c31475e)

opensearch-trigger-bot · 2024-01-20T19:03:05Z

The backport to 2.12 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.12 2.12
# Navigate to the new working tree
cd .worktrees/backport-2.12
# Create a new branch
git switch --create backport/backport-214-to-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c31475e5465fdce80c7c9b86519a4d8d0141c06b
# Push it to GitHub
git push --set-upstream origin backport/backport-214-to-2.12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.12

Then, create a pull request where the base branch is 2.12 and the compare/head branch is backport/backport-214-to-2.12.

msfroh · 2024-01-20T19:12:55Z

The backport to 2.12 failed:

That makes sense, since we haven't cut a 2.12 branch yet -- it will get cut from 2.x.

I'm just going to remove the backport-2.12 label.

) Require password as an argument on 2.12 and higher For earlier versions, emit a warning if an admin password is specified on the command line, since it will be ignored. --------- Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Michael Froh <[email protected]> Co-authored-by: Michael Froh <[email protected]> (cherry picked from commit c31475e) Co-authored-by: Darshit Chanpura <[email protected]> Co-authored-by: Sean Li <[email protected]>

Addresses changes made to security demo config install tool

958d33b

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura requested review from msfroh, macohen, noCharger, mingshl and sejli as code owners December 15, 2023 21:30

DarshitChanpura added backport 2.x Backport to 2.x branch v2.12.0 backport 2.12 labels Dec 15, 2023

DarshitChanpura mentioned this pull request Dec 15, 2023

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

Closed

2 tasks

derek-ho approved these changes Dec 28, 2023

View reviewed changes

derek-ho reviewed Dec 29, 2023

View reviewed changes

helpers/personalized_search_ranking_quickstart.sh Outdated Show resolved Hide resolved

DarshitChanpura and others added 3 commits January 2, 2024 10:33

Addresses PR feedback

b558a10

Signed-off-by: Darshit Chanpura <[email protected]>

Merge branch 'main' into admin-cred-update

6df16df

Updates version selection logic

7d7db74

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the admin-cred-update branch from 09e4d38 to 7d7db74 Compare January 13, 2024 18:40

msfroh reviewed Jan 17, 2024

View reviewed changes

DarshitChanpura and others added 2 commits January 18, 2024 11:22

Merge branch 'main' into admin-cred-update

901887e

Require password as an argument on 2.12 and higher

a5da908

For earlier versions, emit a warning if an admin password is specified on the command line, since it will be ignored. Signed-off-by: Michael Froh <[email protected]>

DarshitChanpura commented Jan 19, 2024

View reviewed changes

msfroh approved these changes Jan 19, 2024

View reviewed changes

noCharger approved these changes Jan 19, 2024

View reviewed changes

msfroh merged commit c31475e into opensearch-project:main Jan 20, 2024
21 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 20, 2024

[Backport 2.x] Addresses changes made to security demo config install tool #217

Merged

msfroh removed the backport 2.12 label Jan 20, 2024

DarshitChanpura mentioned this pull request Jan 22, 2024

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

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses changes made to security demo config install tool #214

Addresses changes made to security demo config install tool #214

DarshitChanpura commented Dec 15, 2023

codecov bot commented Dec 15, 2023 •

edited

Loading

msfroh commented Dec 15, 2023

derek-ho left a comment

DarshitChanpura commented Jan 17, 2024 •

edited

Loading

msfroh Jan 17, 2024

DarshitChanpura Jan 18, 2024

msfroh Jan 18, 2024

DarshitChanpura Jan 18, 2024

msfroh Jan 18, 2024

msfroh Jan 18, 2024

DarshitChanpura left a comment

noCharger left a comment

opensearch-trigger-bot bot commented Jan 20, 2024

msfroh commented Jan 20, 2024

Addresses changes made to security demo config install tool #214

Addresses changes made to security demo config install tool #214

Conversation

DarshitChanpura commented Dec 15, 2023

Description

Check List

codecov bot commented Dec 15, 2023 • edited Loading

Codecov Report

msfroh commented Dec 15, 2023

derek-ho left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Jan 17, 2024 • edited Loading

msfroh Jan 17, 2024

Choose a reason for hiding this comment

DarshitChanpura Jan 18, 2024

Choose a reason for hiding this comment

msfroh Jan 18, 2024

Choose a reason for hiding this comment

DarshitChanpura Jan 18, 2024

Choose a reason for hiding this comment

msfroh Jan 18, 2024

Choose a reason for hiding this comment

msfroh Jan 18, 2024

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

noCharger left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 20, 2024

msfroh commented Jan 20, 2024

codecov bot commented Dec 15, 2023 •

edited

Loading

DarshitChanpura commented Jan 17, 2024 •

edited

Loading