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

[Backport 2.x] Demo configuration script requires admin password #3414

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

@opensearch-trigger-bot opensearch-trigger-bot bot commented Sep 27, 2023

This change requires an alternative to the default credentials
for the admin user.

The credentials can be provided to the script via:
- `initialAdminPassword` environment variable
- a file with a single line that contains the password.

The admin password for the cluster will be printed to the console output of the `tools/install_demo_configuration.(bat|sh)`

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Co-authored-by: Peter Nied <petern@amazon.com>
(cherry picked from commit 8628a89)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #3414 (eabc278) into 2.x (3841f14) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #3414      +/-   ##
============================================
+ Coverage     64.09%   64.10%   +0.01%     
+ Complexity     3369     3367       -2     
============================================
  Files           256      256              
  Lines         19515    19515              
  Branches       3297     3297              
============================================
+ Hits          12508    12511       +3     
+ Misses         5365     5362       -3     
  Partials       1642     1642              

see 2 files with indirect coverage changes

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring a password is a breaking change in 2.x

  • What do we think about taking this change as is versus modifying this change so the password is opt-in?

@stephen-crawford
Copy link
Collaborator

@peternied maybe you disagree but here is my thought process: for this change to be breaking, someone would have to have configured an automated flow which expected admin as the password. However, we only changed this behavior for the demo configuration so this would only change if they upgraded and then expected the demo configuration to use the admin password as before. There is nothing stopping someone from using their own configuration and setting to admin again. It is just when starting from nothing and installing the demo. Therefore, I am of the opinion that changes to demos should not count as breaking changes.

@DarshitChanpura
Copy link
Member

What is the disadvantage of making this change mandatory? IMO, we are promoting a better security posture, and since this affects demo configuration script, I'm onboard with bringing this change in unless you think this would be a deal-breaker for everyone using automated scripts for programatic access (in which case they can read the password from the logs). Thoughts?

@peternied
Copy link
Member

Thanks for the thoughts @scrawfor99 @DarshitChanpura


Since I didn't state my own opinion, I feel that we should make a breaking change to these scripts, if this is opt-in no one will make the conversion to use a password they provide.

Work around for impacted folks is they can provide the password as admin

@DarshitChanpura
Copy link
Member

DarshitChanpura commented Oct 2, 2023

Agreed @peternied.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has been reviewed and approved during the Security Triage team meeting on 10/2

@peternied peternied merged commit d995daf into 2.x Oct 3, 2023
99 of 101 checks passed
@peternied peternied deleted the backport/backport-3329-to-2.x branch October 3, 2023 17:30
DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants