Reset elastic password cli tool #74892

jkakavas · 2021-07-05T07:12:15Z

This change introduces a new CLI tool that allows users to reset
the password for the elastic user, setting it to a user defined
or an auto-generated value.

Resolves: #70113
Depends on: #74890

This change introduces a CLI tool that can be used to create enrollment tokens. It doesn't require credentials, but simply write access to the local filesystem of a node. It uses an auto-generated user in the file-realm with superuser role. For this purpose, this change also introduces a base class for a CLI tool that can be used by any CLI tool needs to perform actions against an ES node as a superuser without requiring credentials from the user. It is worth noting that this doesn't change our existing thread model, because already an actor with write access to the fs of an ES node, can become superuser (again, by adding a superuser to the file realm, albeit manually).

This change introduces a new CLI tool that allows users to reset the password for the elastic user, setting it to a user defined or an auto-generated value. Resolves: elastic#70113

…ord-cli

elasticmachine · 2021-07-05T07:12:18Z

Pinging @elastic/es-security (Team:Security)

docs/reference/commands/reset-elastic-password.asciidoc

jkakavas · 2021-07-05T21:02:52Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+     * retries as the file realm might not have reloaded the users file yet in order to authenticate our
+     * newly created file realm user.
+     */
+    private void checkClusterHealthWithRetries(Environment env, Terminal terminal, int retries) throws Exception {


We were chatting around this with @albertzaharovits today. This seems reasonable as a base check but were wondering whether a more nuanced approach is necessary/helpful.

The concern is that for some tools that extend this command and have specialized purpose and needs, this check might be overly coarse grained. Point in case, for the reset password tool, we might not be interested if any data index is red( making the cluster health red ), as long as .security is green. On the other hand, we want this to be useful even if the .security index is not yet created so we'd need to check for the existence of the .security index before the health check so that it won't time out. We can't use the exists API in 8.0 for system indices, so we'd have to use _cat/indices or _cat/aliases and parse the response.

The question is whether we need this nuanced approach, and if we do, do we implement it here in the base class or do we abstract it and let the extending classes implement this as they see fit. Given in the two classes that extend this (CreateEnrollmentTokenTool and ResetElasticPasswordTool) so far, we only care about the security index, I lean on doing the former.

_cat/indices and/or _cat/aliases are not that friendly for parsing the output. Another idea would be to not attempt to do the health check up front, but depend on the error message that we would get from Elasticsearch in order to determine if the security index health is RED. There are a couple of issues with this too:

We don't consistently check the health of the security index in the SecurityIndexManager before making operations on it. Some methods do manual checks like isAvailable() and then call checkIndexVersionThenExecute, some call prepareIndexIfNeededThenExecute() directly which doesn't call isAvailable(). So we would need to depend on the behavior of the actual indexing of the document in the index.

Not all implementing classes of BaseRunAsSuperuser will be calling APIs with the CommandLineHttpClient themselves. Reset password does, but CreateEnrollmentTokenTool does not, instead depends on CreateEnrollmentToken methods that call the APIs internally and will wrap exceptions.

It should still be doable but some of it sounds like guesswork.

Is "if your cluster health is RED, you shouldn't be making additional operations on it (like using these tools) unless it's about resolving the issue, so the tool doesn't work if cluster health is RED" an appropriate stance or do we think that a nuanced approach ( i.e. each tool defines it's own health/index checks as it sees fit and the base class just validates file realm auto-generated credentials ) is helpful and necessary ?

Opening up the discussion, @tvernum I know from previous discussions that you have opinions on this type of behavior by our CLI tools, care to chime in?

Disclaimer: Speaking in principle, haven't checked the code too carefully:
Any check that we do before the actual operation has limited usefulness because the condition can change in between the time of check and the operation time (time of check, time of use).
I think it's worth investing in some tests that "pin" the errors (ideally consistent across APIs) for an unavailable/red .security index.

We chatted with Tim a little bit about this and we came to a similar conclusion which was that until we can identify all the ways this can fail in an actionable and informative way, replicating the behavior of other tools ( warn if cluster health is red and allow to override ) is preferable to complicate the checks for handling some of the cases. I plan on moving forward as is and open an issue to track the effort to do what you suggest, if you don't think this is blocking for the current PR

albertzaharovits · 2021-07-08T06:07:14Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+    protected final void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
+        validate(terminal, options, env);
+        KeyStoreWrapper keyStoreWrapper = keyStoreFunction.apply(env);
+        final Environment newEnv;


Looks like you're building an environment here.
There is the EnvironmentAwareCommand to handle that.

BaseRunAsSuperuserCommand extends KeyStoreAwareCommand which extends EnvironmentAwareCommand already. What I need to do here is to to factor in the secure settings we might have read from the keystore , thus the new environment. Not sure how EnvironmentAwareCommand can handle that for me ,let me know if I'm missing something !

albertzaharovits · 2021-07-08T06:17:20Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+            }
+            users = new HashMap<>(users);
+            users.put(username, hasher.hash(password));
+            FileUserPasswdStore.writeFile(users, passwordFile);


Do you think guaranteeing that the user has roles assigned when it exists is important?

To make it appear atomic, maybe it would be better to first update the roles file and then the users file (otherwise there are states where the user exists but doesn't have a role, and we ought to handle this case as a retry).
In addition, we would also have to change the server code to ensure that if the roles file is changed before the users file, changes to the roles file are always considered when updating the users.

Do you think guaranteeing that the user has roles assigned when it exists is important?

Yes!Thanks.

To make it appear atomic, maybe it would be better to first update the roles file and then the users file

I think that's a good idea nevertheless

In addition, we would also have to change the server code to ensure that if the roles file is changed before the users file, changes to the roles file are always considered when updating the users.

Not sure I follow this, maybe we can quickly chat in our sync

Changed the order of execution here and added 403 to the list of error codes we should retry upon

albertzaharovits · 2021-07-08T06:19:08Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+        if (fileRealmSettings.size() > 1) {
+            throw new UserException(
+                ExitCodes.CONFIG,
+                "Multiple file realms are configured. "


If possible, lets factor out the code in the server code that tells the file realm configuration is correct and that a file realm is enabled and its order.

This check is already happening on server side

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java

Line 321 in 4880cea

if (internalTypes.contains(identifier.getType())) {

I can remove the check if you think it's cleaner. The base class assumes that any tool that extends it already needs a running cluster and if a node attempts to start with a configuration file that has more than one file realms, then the node would fail to start because of that, so in theory we would never stumble upon this error in the tool.

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

…ord-cli

BigPandaToo · 2021-07-11T21:21:34Z

...rc/main/java/org/elasticsearch/xpack/security/enrollment/tool/CreateEnrollmentTokenTool.java

+        CheckedFunction<Environment, KeyStoreWrapper, Exception> keyStoreFunction,
+        CheckedFunction<Environment, CreateEnrollmentToken, Exception> createEnrollmentTokenFunction
+    ) {
+        super(clientFunction, keyStoreFunction, "Creates enrollment tokens for elasticsearch nodes and kibana instances");


As I understand this supposed to be a help message (Command$printHelp)

The description of the command is printed when the command is run with -h , yes. Can you elaborate on what your comment was about ? Thanks!

I would expect a description of allowed parameters (like node/kibana)

I would expect a description of allowed parameters (like node/kibana)

We don't put these in the description of the command. All of our CLI tools extend Command and it calls printHelp when the -h parameter is passed ( or on error ). printHelp prints the parameters and their description.

FWIW, the ouput of the -h is

Creates enrollment tokens for elasticsearch nodes and kibana instances Option (* = required) Description --------------------- ----------- -E <KeyValuePair> Configure a setting -f, --force Use this option to force execution of the command against a cluster that is currently unhealthy. -h, --help Show help * -s, --scope The scope of this enrollment token, can be either "node" or "kibana" -v, --verbose Show verbose output

...rc/main/java/org/elasticsearch/xpack/security/enrollment/tool/CreateEnrollmentTokenTool.java

BigPandaToo · 2021-07-11T21:23:28Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+
+        Map<String, char[]> users = FileUserPasswdStore.parseFile(passwordFile, null, env.settings());
+        if (users == null) {
+            throw new UserException(ExitCodes.CONFIG, "File realm configuration file [" + passwordFile + "] is missing");


Should we try to clean UserRolesStore anyway?

Yes good idea. I originally thought that worst case scenario is that the roles files remain but reference a non-existent user which is ok. It is a bit sloppy and it's a similar comment to what Albert made on the original PR about trying to make these 2 changes atomic. I will rework this

BigPandaToo · 2021-07-11T21:23:57Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+        users = new HashMap<>(users);
+        char[] passwd = users.remove(username);
+        if (passwd != null) {
+            // No need to overwrite, if the user was already removed


Can the password be null? If yes, the user will stay in the file

See java.util.Map#remove and FileUserPasswdStore#parseFile. In short, parseFile wlll not return a Map that has a null value because we would have bailed on parsing it. As such, if remove returns null is because the users Map did not contain an entry with the username key.

BigPandaToo · 2021-07-11T21:24:26Z

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java

+        userRoles = new HashMap<>(userRoles);
+        String[] roles = userRoles.remove(username);
+        if (roles != null) {
+            // No need to overwrite, if the user was already removed


Same as above

BigPandaToo · 2021-07-11T21:28:51Z

...n/security/src/main/java/org/elasticsearch/xpack/security/tool/ResetElasticPasswordTool.java

+    protected ResetElasticPasswordTool(
+        Function<Environment, CommandLineHttpClient> clientFunction,
+        CheckedFunction<Environment, KeyStoreWrapper, Exception> keyStoreFunction) {
+        super(clientFunction, keyStoreFunction, "Resets the password or the elastic built-in user");


or => of
Also, I believe the description is supposed to be the help message

Also, I believe the description is supposed to be the help message

It is !

...n/security/src/main/java/org/elasticsearch/xpack/security/tool/ResetElasticPasswordTool.java

jkakavas · 2021-07-12T07:42:59Z

I repeated some comments on Base class from the create enrollment token tool PR, pls ignore them if you address them there

Thanks ! Let's keep things contained in one of the PRs. Ideally, as discussed, we can review and merge CreateEnrollmentTokensTool first and then I'll rebase the changes here. No need to have the same discussion in multiple places.

…ord-cli

BigPandaToo

I added a comment about the help message, I may be wrong about my expectation from the -help, so I am leaving it to you
LGTM otherwise

jkakavas · 2021-07-13T07:14:22Z

I added a comment about the help message, I may be wrong about my expectation from the -help, so I am leaving it to you
LGTM otherwise

I explained in #74892 (comment)

lockewritesdocs · 2021-07-13T16:16:33Z

@elasticmachine run elasticsearch-ci/docs

lockewritesdocs

Left a number of comments and suggested changes. A few options seemed missing, so I added those or left questions about them.

docs/reference/commands/reset-elastic-password.asciidoc

...rc/main/java/org/elasticsearch/xpack/security/enrollment/tool/CreateEnrollmentTokenTool.java

...n/security/src/main/java/org/elasticsearch/xpack/security/tool/ResetElasticPasswordTool.java

...st/java/org/elasticsearch/xpack/security/enrollment/tool/BaseRunAsSuperuserCommandTests.java

Co-authored-by: Adam Locke <[email protected]>

…ord-cli

docs/reference/commands/reset-elastic-password.asciidoc

…ord-cli

albertzaharovits

LGTM

One minor question: the namespace for ResetElasticPasswordTool looks to me a bit generic. I would've assigned the same namespace as the SetupPasswordTool.

…ord-cli

jkakavas · 2021-07-20T08:10:34Z

the namespace for ResetElasticPasswordTool looks to me a bit generic. I would've assigned the same namespace as the SetupPasswordTool.

SGTM, I did the change, thanks!

This change introduces a CLI tool that can be used to create enrollment tokens. It doesn't require credentials, but simply write access to the local filesystem of a node. It uses an auto-generated user in the file-realm with superuser role. For this purpose, this change also introduces a base class for a CLI tool that can be used by any CLI tool needs to perform actions against an ES node as a superuser without requiring credentials from the user. It is worth noting that this doesn't change our existing thread model, because already an actor with write access to the fs of an ES node, can become superuser (again, by adding a superuser to the file realm, albeit manually).

jkakavas added 5 commits July 2, 2021 21:16

elastic user password reset CLI tool

f67eaf1

This change introduces a new CLI tool that allows users to reset the password for the elastic user, setting it to a user defined or an auto-generated value. Resolves: elastic#70113

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

173ce85

…ord-cli

qa test

3776bb9

add docs

d6d9385

jkakavas added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 labels Jul 5, 2021

jkakavas requested review from albertzaharovits and BigPandaToo July 5, 2021 07:12

elasticmachine added the Team:Security Meta label for security team label Jul 5, 2021

bytebilly reviewed Jul 5, 2021

View reviewed changes

docs/reference/commands/reset-elastic-password.asciidoc Outdated Show resolved Hide resolved

jkakavas added 2 commits July 5, 2021 15:40

support batch silent mode

7497bc9

remove leftover files

ad30c3b

jkakavas commented Jul 5, 2021

View reviewed changes

albertzaharovits reviewed Jul 8, 2021

View reviewed changes

.../security/src/main/java/org/elasticsearch/xpack/security/tool/BaseRunAsSuperuserCommand.java Show resolved Hide resolved

jkakavas added 2 commits July 8, 2021 13:30

Bring along updates from elastic#74890

c5c1759

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

7efae2c

…ord-cli

BigPandaToo reviewed Jul 11, 2021

View reviewed changes

...rc/main/java/org/elasticsearch/xpack/security/enrollment/tool/CreateEnrollmentTokenTool.java Outdated Show resolved Hide resolved

BigPandaToo reviewed Jul 11, 2021

View reviewed changes

...n/security/src/main/java/org/elasticsearch/xpack/security/tool/ResetElasticPasswordTool.java Outdated Show resolved Hide resolved

jkakavas added 2 commits July 12, 2021 11:15

Merge feedback from 74890

a40f4cc

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

f75e2db

…ord-cli

jkakavas requested a review from BigPandaToo July 12, 2021 09:06

jkakavas added 2 commits July 12, 2021 12:10

Include error code in exception message

4d8cfa4

don't explicitly check for multiple file realms, node does that already

1f14b39

jkakavas requested a review from lockewritesdocs July 12, 2021 14:52

BigPandaToo approved these changes Jul 13, 2021

View reviewed changes

Add link in top-level page

ed292c4

lockewritesdocs reviewed Jul 13, 2021

View reviewed changes

jkakavas and others added 3 commits July 13, 2021 21:41

Apply doc suggestions from code review

704b4d8

Co-authored-by: Adam Locke <[email protected]>

More suggestions from doc review

cc2404c

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

cb87765

…ord-cli

lockewritesdocs reviewed Jul 14, 2021

View reviewed changes

docs/reference/commands/reset-elastic-password.asciidoc Outdated Show resolved Hide resolved

Add note that you can't use the CLI if the file realm is disabled

aa73e85

lockewritesdocs mentioned this pull request Jul 14, 2021

[DOCS] Stack updates for security ON by default #75153

Closed

12 tasks

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

0d7c7aa

…ord-cli

albertzaharovits approved these changes Jul 20, 2021

View reviewed changes

jkakavas added 2 commits July 20, 2021 11:01

Merge remote-tracking branch 'origin/master' into reset-elastic-passw…

c2c64cf

…ord-cli

message

4a246cd

Fix package in CLI tool

d8eaca9

jkakavas merged commit 13c8ee1 into elastic:master Jul 20, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset elastic password cli tool #74892

Reset elastic password cli tool #74892

jkakavas commented Jul 5, 2021

elasticmachine commented Jul 5, 2021

jkakavas Jul 5, 2021

jkakavas Jul 6, 2021

albertzaharovits Jul 8, 2021

jkakavas Jul 8, 2021

albertzaharovits Jul 8, 2021

jkakavas Jul 8, 2021

albertzaharovits Jul 8, 2021

jkakavas Jul 8, 2021

jkakavas Jul 12, 2021

albertzaharovits Jul 8, 2021

jkakavas Jul 8, 2021

jkakavas Jul 12, 2021 •

edited

Loading

BigPandaToo Jul 11, 2021

jkakavas Jul 12, 2021

BigPandaToo Jul 13, 2021

jkakavas Jul 13, 2021

BigPandaToo Jul 11, 2021

jkakavas Jul 12, 2021

BigPandaToo Jul 11, 2021

jkakavas Jul 12, 2021

BigPandaToo Jul 11, 2021

BigPandaToo Jul 11, 2021

jkakavas Jul 12, 2021

jkakavas commented Jul 12, 2021

BigPandaToo left a comment

jkakavas commented Jul 13, 2021

lockewritesdocs commented Jul 13, 2021

lockewritesdocs left a comment

albertzaharovits left a comment

jkakavas commented Jul 20, 2021

Reset elastic password cli tool #74892

Reset elastic password cli tool #74892

Conversation

jkakavas commented Jul 5, 2021

elasticmachine commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Jul 12, 2021

BigPandaToo left a comment

Choose a reason for hiding this comment

jkakavas commented Jul 13, 2021

lockewritesdocs commented Jul 13, 2021

lockewritesdocs left a comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

jkakavas commented Jul 20, 2021

jkakavas Jul 12, 2021 •

edited

Loading