Added Password Setup Tool #1686

ryanbogan · 2022-03-16T23:05:34Z

Description

Adds a password setup tool and corresponding scripts to set passwords for internal users.

Issues Resolved

Progress on: Replace admin:admin default credentials

Testing

Manual testing: The plugin was built and installed into opensearch min version 1.2.4. The script was run to ensure all branches of the tool are functional.

Check List

[ X] Commits are signed 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.

… made progress on a password setup tool Signed-off-by: Ryan Bogan <[email protected]>

…e new password system Signed-off-by: Ryan Bogan <[email protected]>

Signed-off-by: Ryan Bogan <[email protected]>

…ting Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/security/tools/PasswordSetup.java

src/test/java/org/opensearch/security/SlowIntegrationTests.java

src/main/java/org/opensearch/security/tools/PasswordSetup.java

setiah · 2022-03-16T23:35:20Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+                        System.out.println("\nEnter password for " + user + ": ");
+                        String password = sc.nextLine();
+                        setPasswordSingleUser(user, lowLevelClient, password);
+                        isWeakPassword = false;


what is the use of this flag isWeakPassword?

It's to prevent the tool from closing if a password that doesn't meet the policy is entered. Instead of throwing an exception, it gives the user a chance to re-enter a new password for that user.

tools/install_demo_configuration.sh

src/main/java/org/opensearch/security/tools/PasswordSetup.java

Signed-off-by: Ryan Bogan <[email protected]>

setiah · 2022-03-16T23:57:35Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+                        String password = sc.nextLine();
+                        setPasswordSingleUser(user, lowLevelClient, password);
+                        isWeakPassword = false;
+                    } catch(ResponseException e) {


Isn't this too broad? Are we sure we're only catching password policy validation failure here?

This should just catch the password validation failure as long as internal_users.yml is not modified. Is there another type of ResponseException that I'm missing?

setiah · 2022-03-16T23:58:12Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+                        setPasswordSingleUser(user, lowLevelClient, password);
+                        isWeakPassword = false;
+                    } catch(ResponseException e) {
+                        System.out.println("A password must be at least 8 characters long and contain at least one uppercase letter, one lowercase letter, one digit, and one special character.");


It should log the message in plugins.security.restapi.password_validation_error_message

peternied

Thank you for the contribution, we do require new functionality had unit tests added to ensure it keeps working, please add unit tests.

We also require that basic documentation is added so that other users can know where/how to use it, please create a basic markdown readme for high level usage, no need to duplicate the great work you've done with the Command line options

While not required, I would highly recommend breaking this single file into many smaller classes, this will make it easier to author tests. Some classes that could be extracted would be CommandLineOptions, CommandLineParser, CommandLineValidator, UserPasswordApplier, SSLContextCreator, ClusterHealthChecker (This seems like it might already be implemented elsewhere in the codebase.)

peternied · 2022-03-17T15:14:27Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+            final int returnCode = execute(args);
+            System.exit(returnCode);
+        } catch (Throwable e) {
+            System.out.println("Unexpected error");


Please include the error in the printed output

peternied · 2022-03-22T20:45:40Z

Hi @ryanbogan It has been many days since there was movement on this PR, what are you thoughts on moving forward?

ryanbogan · 2022-03-24T14:26:38Z

Hey @peternied, I have been out of the office since Monday, but last week I made most of the changes suggested above. I am currently working on unit testing, and then I want to explore breaking the file into separate classes.

Signed-off-by: Ryan Bogan <[email protected]>

codecov-commenter · 2022-04-04T22:43:23Z

Codecov Report

Merging #1686 (6026987) into main (81a43c2) will decrease coverage by 2.15%.
The diff coverage is 44.58%.

@@             Coverage Diff              @@
##               main    #1686      +/-   ##
============================================
- Coverage     64.56%   62.41%   -2.16%     
- Complexity     3239     3287      +48     
============================================
  Files           249      254       +5     
  Lines         17479    18591    +1112     
  Branches       3096     3354     +258     
============================================
+ Hits          11286    11604     +318     
- Misses         4635     5243     +608     
- Partials       1558     1744     +186

Impacted Files	Coverage Δ
...a/org/opensearch/security/tools/PasswordSetup.java	`44.58% <44.58%> (ø)`
...ensearch/security/action/whoami/WhoAmIRequest.java	`0.00% <0.00%> (-100.00%)`	⬇️
...nsearch/security/action/whoami/WhoAmIResponse.java	`0.00% <0.00%> (-82.86%)`	⬇️
.../security/action/whoami/TransportWhoAmIAction.java	`33.33% <0.00%> (-58.34%)`	⬇️
...nsearch/security/ssl/ExternalSecurityKeyStore.java	`8.00% <0.00%> (-40.00%)`	⬇️
...rity/action/configupdate/ConfigUpdateResponse.java	`64.28% <0.00%> (-35.72%)`	⬇️
...rch/security/transport/SecurityRequestHandler.java	`57.89% <0.00%> (-11.85%)`	⬇️
...search/security/configuration/DlsFlsValveImpl.java	`59.25% <0.00%> (-10.97%)`	⬇️
.../org/opensearch/security/auth/BackendRegistry.java	`48.43% <0.00%> (-10.94%)`	⬇️
...urity/ssl/transport/SecuritySSLNettyTransport.java	`62.36% <0.00%> (-6.79%)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81a43c2...6026987. Read the comment docs.

Signed-off-by: Ryan Bogan <[email protected]>

setiah · 2022-04-05T22:28:53Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+        options.addOption(Option.builder("era").longOpt("enable-replica-autoexpand").desc("Enable replica auto expand and exit").build());
+        options.addOption(Option.builder("dra").longOpt("disable-replica-autoexpand").desc("Disable replica auto expand and exit").build());


why are these options required in password setup?

setiah · 2022-04-05T22:29:52Z

src/main/java/org/opensearch/security/tools/PasswordSetup.java

+        options.addOption(Option.builder("ff").longOpt("fail-fast").desc("fail-fast if something goes wrong").build());
+        options.addOption(Option.builder("dg").longOpt("diagnose").desc("Log diagnostic trace into a file").build());
+        options.addOption(Option.builder("dci").longOpt("delete-config-index").desc("Delete '.opendistro_security' config index and exit.").build());
+        options.addOption(Option.builder("esa").longOpt("enable-shard-allocation").desc("Enable all shard allocation and exit.").build());


Seems unnecessary

There are a lot of input arguments that are not applicable to the tool (see below). We should remove them unless there's a reason i'm missing.

-arc,--accept-red-cluster Also operate on a red cluster. If not specified the cluster state has to be at least yellow. -backup <folder> Backup configuration to folder -dci,--delete-config-index Delete '.opendistro_security' config index and exit. -dra,--disable-replica-autoexpand Disable replica auto expand and exit -ec,--enabled-ciphers <cipers> Comma separated list of enabled TLS ciphers -ep,--enabled-protocols <protocols> Comma separated list of enabled TLS protocols -er,--explicit-replicas <number of replicas> Set explicit number of replicas or autoexpand expression for .opendistro_security index -era,--enable-replica-autoexpand Enable replica auto expand and exit -esa,--enable-shard-allocation Enable all shard allocation and exit. -f,--file <file> file -migrate <folder> Migrate and use folder to store migrated files -mo,--migrate-offline <folder> Migrate and use folder to store migrated files -us,--update_settings <number of replicas> Update the number of Security index replicas, reload configuration on all nodes and exit -vc,--validate-configs <version> Validate config for version 6 or 7 (default 7) -w,--whoami Show information about the used admin certificate -r,--retrieve retrieve current config -rev,--resolve-env-vars Resolve/Substitute env vars in config with their value before uploading -rl,--reload Reload the configuration on all nodes, flush all Security caches and exit -si,--show-info Show system and license info -sniff,--enable-sniffing Enable client.transport.sniff

Besides, some other settings like -cd (config directory), -key (admin key), -cert (admin cert), -cacert (root certs) should be optional to provide for users, with defaults set to what opensearch-security sets out of the box.

peternied · 2022-04-13T19:32:55Z

@ryanbogan After reviewing the state of our tooling within security - we are deprecating all of it in 3.0. As we are not investing in this space, this change will not be merged and I am closing this pull request.

This change is valuable and I think it should live on as a part of an OpenSearch operations tool. Updating configuration programmatically like swapping out certificates, updating cluster configuration, and setting up user accounts with secure password are great use cases.

setiah · 2022-04-13T19:35:32Z

On running this tool I see some unnecessary things going on, that need to be fixed

Seems like it can create .opendistro_security index if it does not exist. Should not happen. It should fail if the security index does not exist, as it points to improper bootstrap
It is updating configs like roles, actiongroups etc. Should not happen. It should only PATCH passwords for internalusers.

Pls fix this.

logs

.opendistro_security index already exists, so we do not need to create one.
Will update '_doc/config' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/config.yml
   SUCC: Configuration for 'config' created or updated
Will update '_doc/roles' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/roles.yml
   SUCC: Configuration for 'roles' created or updated
Will update '_doc/rolesmapping' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/roles_mapping.yml
   SUCC: Configuration for 'rolesmapping' created or updated
Will update '_doc/internalusers' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/internal_users.yml
   SUCC: Configuration for 'internalusers' created or updated
Will update '_doc/actiongroups' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/action_groups.yml
   SUCC: Configuration for 'actiongroups' created or updated
Will update '_doc/tenants' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/tenants.yml
   SUCC: Configuration for 'tenants' created or updated
Will update '_doc/nodesdn' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/nodes_dn.yml
   SUCC: Configuration for 'nodesdn' created or updated
Will update '_doc/whitelist' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/whitelist.yml
   SUCC: Configuration for 'whitelist' created or updated
Will update '_doc/audit' with /home/ubuntu/security4/opensearch-1.3.0/plugins/opensearch-security/securityconfig/audit.yml
   SUCC: Configuration for 'audit' created or updated
SUCC: Expected 9 config types for node {"updated_config_types":["tenants","rolesmapping","nodesdn","audit","roles","whitelist","internalusers","actiongroups","config"],"updat
ed_config_size":9,"message":null} is 9 (["tenants","rolesmapping","nodesdn","audit","roles","whitelist","internalusers","actiongroups","config"]) due to: null

setiah · 2022-04-13T19:38:38Z

@ryanbogan After reviewing the state of our tooling within security - we are deprecating all of it in 3.0. As we are not investing in this space, this change will not be merged and I am closing this pull request.

This change is valuable and I think it should live on as a part of an OpenSearch operations tool. Updating configuration programmatically like swapping out certificates, updating cluster configuration, and setting up user accounts with secure password are great use cases.

Yes, that's the plan. I'm going to post a commit on top of this PR/create a separate pr for opensearch-admin tool. Probably a good idea to mark this as draft PR and continue working on open comments/issues in the meantime. @ryanbogan @peternied

peternied · 2022-04-13T19:41:33Z

@setiah lets not author this change in the security repository and instead create it in a new repo dedicated to this tooling.

setiah · 2022-04-13T20:19:51Z

@peternied I would say let's do that independently. We should not block this change for any future plans on maintaining the tooling. We can move around the tooling as a separate consolidation effort with a separate issue, as-it-is, and package it in ways best for user experience. As part of that you would have to move other tools as well.

ryanbogan added 4 commits March 4, 2022 19:13

Removed admin:admin password, added strong password requirements, and…

16472bf

… made progress on a password setup tool Signed-off-by: Ryan Bogan <[email protected]>

Added more changes to password tool and modified unit tests to includ…

5c4caf7

…e new password system Signed-off-by: Ryan Bogan <[email protected]>

Restored default passwords for internal_users.yml

b6d4911

Signed-off-by: Ryan Bogan <[email protected]>

Added demo script for password setup tool and reverted changes on tes…

9059e4a

…ting Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan requested review from a team and setiah March 16, 2022 23:05

setiah reviewed Mar 16, 2022

View reviewed changes

src/main/java/org/opensearch/security/tools/PasswordSetup.java Outdated Show resolved Hide resolved

setiah reviewed Mar 16, 2022

View reviewed changes

src/main/java/org/opensearch/security/tools/PasswordSetup.java Outdated Show resolved Hide resolved

cliu123 reviewed Mar 16, 2022

View reviewed changes

src/test/java/org/opensearch/security/SlowIntegrationTests.java Outdated Show resolved Hide resolved

setiah reviewed Mar 16, 2022

View reviewed changes

src/main/java/org/opensearch/security/tools/PasswordSetup.java Outdated Show resolved Hide resolved

setiah reviewed Mar 16, 2022

View reviewed changes

tools/install_demo_configuration.sh Show resolved Hide resolved

setiah reviewed Mar 16, 2022

View reviewed changes

src/main/java/org/opensearch/security/tools/PasswordSetup.java Outdated Show resolved Hide resolved

Removed unused imports and unnecessary logic in Password Tool

736da71

Signed-off-by: Ryan Bogan <[email protected]>

setiah reviewed Mar 16, 2022

View reviewed changes

peternied requested changes Mar 17, 2022

View reviewed changes

ryanbogan added 4 commits March 24, 2022 22:57

Progress on unit tests

57b5e6a

Signed-off-by: Ryan Bogan <[email protected]>

Add unit tests and other required changes

bf5bfce

Signed-off-by: Ryan Bogan <[email protected]>

Added newline at end of files

f042c36

Signed-off-by: Ryan Bogan <[email protected]>

Fixed lint errors

6026987

Signed-off-by: Ryan Bogan <[email protected]>

One more lint error

9c9a820

Signed-off-by: Ryan Bogan <[email protected]>

setiah reviewed Apr 5, 2022

View reviewed changes

peternied closed this Apr 13, 2022

setiah mentioned this pull request May 11, 2022

Password setup tool for built-in users #1836

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Password Setup Tool #1686

Added Password Setup Tool #1686

ryanbogan commented Mar 16, 2022

setiah Mar 16, 2022

ryanbogan Mar 16, 2022

setiah Mar 16, 2022

ryanbogan Mar 17, 2022 •

edited

Loading

setiah Mar 16, 2022

peternied left a comment

peternied Mar 17, 2022

peternied commented Mar 22, 2022

ryanbogan commented Mar 24, 2022

codecov-commenter commented Apr 4, 2022 •

edited

Loading

setiah Apr 5, 2022

setiah Apr 5, 2022

setiah Apr 13, 2022

setiah Apr 13, 2022 •

edited

Loading

peternied commented Apr 13, 2022

setiah commented Apr 13, 2022

setiah commented Apr 13, 2022 •

edited

Loading

peternied commented Apr 13, 2022

setiah commented Apr 13, 2022

		options.addOption(Option.builder("era").longOpt("enable-replica-autoexpand").desc("Enable replica auto expand and exit").build());
		options.addOption(Option.builder("dra").longOpt("disable-replica-autoexpand").desc("Disable replica auto expand and exit").build());

Added Password Setup Tool #1686

Added Password Setup Tool #1686

Conversation

ryanbogan commented Mar 16, 2022

Description

Issues Resolved

Testing

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbogan Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied commented Mar 22, 2022

ryanbogan commented Mar 24, 2022

codecov-commenter commented Apr 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setiah Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

peternied commented Apr 13, 2022

setiah commented Apr 13, 2022

setiah commented Apr 13, 2022 • edited Loading

peternied commented Apr 13, 2022

setiah commented Apr 13, 2022

ryanbogan Mar 17, 2022 •

edited

Loading

codecov-commenter commented Apr 4, 2022 •

edited

Loading

setiah Apr 13, 2022 •

edited

Loading

setiah commented Apr 13, 2022 •

edited

Loading