Skip to content

Setting elastic password from stored hash #77036

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.

Sign up for GitHub

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

Closed
BigPandaToo wants to merge 20 commits into from
Closed

Setting elastic password from stored hash #77036

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0ffe056
Setting elastic password from stored hash
BigPandaToo Aug 30, 2021
621c735
Merge branch 'master' into Store_hash
elasticmachine Aug 31, 2021
1aaf1f9
adding tests
BigPandaToo Sep 2, 2021
9fd474c
Merge branch 'master' into Store_hash
elasticmachine Sep 2, 2021
7260cb4
Merge branch 'master' into Store_hash
elasticmachine Sep 2, 2021
92a48eb
Addressing PR feedback and refactoring
BigPandaToo Sep 3, 2021
9c543fa
Addressing PR feedback and refactoring
BigPandaToo Sep 3, 2021
2fcca5c
Merge branch 'master' into Store_hash
elasticmachine Sep 6, 2021
c28a3ad
Merge branch 'master' into Store_hash
elasticmachine Sep 6, 2021
1e09d7a
Addressing PR feedback
BigPandaToo Sep 6, 2021
e985d4e
Merge branch 'master' into Store_hash
elasticmachine Sep 6, 2021
42cd17a
Merge branch 'master' into Store_hash
elasticmachine Sep 7, 2021
43ab0f5
Adding test
BigPandaToo Sep 7, 2021
d0fafd6
Addressing more feedback, adding tests
BigPandaToo Sep 7, 2021
25a97bb
Merge branch 'master' into Store_hash
elasticmachine Sep 7, 2021
6227d1f
Changing default for OpType
BigPandaToo Sep 7, 2021
a4424d9
Fixing a long line
BigPandaToo Sep 7, 2021
dce0ef5
Fixing tests/ making sure empty
BigPandaToo Sep 7, 2021
91d590a
Merge branch 'master' into Store_hash
BigPandaToo Sep 8, 2021
5322487
Resolving conflicts
BigPandaToo Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.elasticsearch.xpack.core.security.authc;

import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestStatus;
Expand All @@ -20,7 +21,9 @@
import java.util.Map;
import java.util.stream.Collectors;

import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError;
import static org.elasticsearch.xpack.core.security.support.Exceptions.internalServerError;

/**
* The default implementation of a {@link AuthenticationFailureHandler}. This
Expand Down Expand Up @@ -160,6 +163,9 @@ private ElasticsearchSecurityException createAuthenticationError(final String me
} else {
containsNegotiateWithToken = false;
}
} else if (t instanceof ElasticsearchStatusException && ((ElasticsearchStatusException) t).status() == INTERNAL_SERVER_ERROR) {
ese = internalServerError(message, t, args);
containsNegotiateWithToken = false;
} else {
Copy link
Contributor

@albertzaharovits albertzaharovits Sep 14, 2021

Choose a reason for hiding this comment

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

Seeing this code, I'm having second thoughts about the plan here.
I didn't realize we return 401 as a catch all error case.
From this perspective, it makes little sense to return 401 if, eg, the index cannot be created, but return 500 if the elastic password had already been changed.

Copy link
Contributor

@jkakavas jkakavas Sep 14, 2021

Choose a reason for hiding this comment

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

From this perspective, it makes little sense to return 401 if, eg, the index cannot be created, but return 500 if the elastic password had already been changed.

It's the other way round, right ?

We can simplify this very much by just returning 401 in all of our cases too. What we were trying to capture was a difference between:

  • "we checked, this is the wrong password - not the one in the keystore" (401)
  • "we checked, this is the wrong password - it's the one in the local keystore but something else has set it on the index" (401)
  • "we checked, this is the correct password but we can't persist it and some other node might set it to something else later, so next time you call it might be a 401 so it doesn't make sense to return 200" (500)

The contract to the user is that

  1. if they successfully authenticate once then they can successfully authenticate with these credentials until something (not ES internal) changes the password.
  2. If they get a 401 then the password is wrong and they should not try it again.

but maybe we're thinking this too much from the user's perspective? There is no mandate that a 401 is permanent or that the user should not retry the request etc. We were trying to use this to signal our logic.

Copy link
Contributor

@albertzaharovits albertzaharovits Sep 15, 2021

Choose a reason for hiding this comment

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

We can simplify this very much by just returning 401 in all of our cases too. What we were trying to capture was a difference between:

  1. "we checked, this is the wrong password - not the one in the keystore" (401)
  2. "we checked, this is the wrong password - it's the one in the local keystore but something else has set it on the index" (401)
  3. "we checked, this is the correct password but we can't persist it and some other node might set it to something else later, so next time you call it might be a 401 so it doesn't make sense to return 200" (500)

Thanks for writing this down!
I think this captures the intent when we set about to code this.

On point 2) above. That something else that already set the password could be an identical process racing to set the password after a successful validation of the hash in the keystore. The racing process could be on the same node, writing the same password hash in the index, or on a different node, potentially writing a different one, or it can even be a password change by the user through the API or the cmd line tool. In this general case (without knowing who set the elastic user in the index in the meantime) returning 401 or 200 are both wrong (the only two error codes we can currently return for authn failures). Authentication has to be retried, given the new state where there is an elastic user in the .security index (which would make authn validate that instead of the keystore value).
We either retry it internally or we signal it to the client to retry it themself.

Point 3) sounds right to me, apart that maybe we should return 503. But I missed that there are a bunch of other error conditions that should probably be 503 and are now 401, eg shards unavailable for the .security index (though those are "read" ops and we're going to return 503 for "write" ones).

Overall, after this conversation, I still think returning 503 if the "promised" password cannot be persisted (because the index or document cannot be created, or because they have been created in the meantime by something else) is the best course of action, because this "write" op during authentication is the only exception (which is not the approach this PR implements, FWIW).
I can also concede to returning 401 as well, given that the other "reads" that fail for system reasons also return 401.

ese = authenticationError(message, t, args);
containsNegotiateWithToken = false;
Expand Down
4 changes: 4 additions & 0 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Exceptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,8 @@ public static ElasticsearchSecurityException authorizationError(String msg, Obje
public static ElasticsearchSecurityException authorizationError(String msg, Exception cause, Object... args) {
return new ElasticsearchSecurityException(msg, RestStatus.FORBIDDEN, cause, args);
}

public static ElasticsearchSecurityException internalServerError(String msg, Throwable cause, Object... args) {
return new ElasticsearchSecurityException(msg, RestStatus.INTERNAL_SERVER_ERROR, cause, args);
}
}
22 changes: 22 additions & 0 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.delete.DeleteResponse;
Expand Down Expand Up @@ -60,6 +62,7 @@
import java.util.function.Consumer;
import java.util.function.Supplier;

import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
Expand Down Expand Up @@ -283,6 +286,25 @@ private void createReservedUser(String username, char[] passwordHash, RefreshPol
});
}

/**
* Asynchronous method to create a security index if necessary and a reserved user if one doesn't exist
*/
public void createReservedUserAndGetUserInfo(String username, char[] passwordHash, RefreshPolicy refresh,
Copy link
Contributor

@jkakavas jkakavas Aug 31, 2021

Choose a reason for hiding this comment

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

Can we split this in two methods and pass the listener from the first to the second ?

Copy link
Contributor

@jkakavas jkakavas Aug 31, 2021

Choose a reason for hiding this comment

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

Why is refresh policy a parameter if we always set it to IMMEDIATE?

Copy link
Contributor

@albertzaharovits albertzaharovits Sep 2, 2021

Choose a reason for hiding this comment

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

This method overall is poorly designed.

There is a very similar private method above. This one here chains that one with another one, for no apparent reason, to me, at least. Afterwards it is going to be used in a very particular circumstance (creating the automatically configured elastic user), but its interface is designed to be used in general, which goes against private qualifier of the method above.

Please follow my suggestion from https://github.com/elastic/elasticsearch/pull/77036/files#r701072274

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 3, 2021

Choose a reason for hiding this comment

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

Thanks for the suggestion @albertzaharovits! I tried this approach, I think the main problem with it is that if an elastic user document exists, we will fail instead of proceeding to validate against existing document information.
There are other differences justifying having it as a separate method: we need special exception handling to not return an UNAUTHORIZED on index creation failure, we don't need to clearRealmCache since the document wasn't updated.
I have refactored the code though.

ActionListener<ReservedUserInfo> listener) {
securityIndex.prepareIndexIfNeededThenExecute((e) -> { listener.onFailure(new ElasticsearchStatusException(e.getMessage(),
INTERNAL_SERVER_ERROR, e.getCause())); },
() -> { executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
client.prepareIndex(SECURITY_MAIN_ALIAS).setOpType(DocWriteRequest.OpType.CREATE)
.setId(getIdForUser(RESERVED_USER_TYPE, username))
.setSource(Fields.PASSWORD.getPreferredName(), String.valueOf(passwordHash),
Fields.ENABLED.getPreferredName(),
true, Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE)
.setRefreshPolicy(refresh).request(),
listener.<IndexResponse>delegateFailure((l, indexResponse) -> getReservedUserInfo(username, l)),
Copy link
Contributor

@albertzaharovits albertzaharovits Sep 2, 2021

Choose a reason for hiding this comment

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

The get after a successful create is unnecessary.

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 3, 2021

Choose a reason for hiding this comment

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

See ^^

Copy link
Contributor

@jkakavas jkakavas Sep 3, 2021

Choose a reason for hiding this comment

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

Apologies for jumping in but I had a similar comment when doing my review and I can't find where above (^^) this comment was addressed/explained

client::index);
});
}
Copy link
Contributor

@albertzaharovits albertzaharovits Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The logic here is wrong, I did some testing.
Firstly, the branching on the instance type should be on the listener::onFailure argument, not on the consumer. But even then, intercepting VersionConflictEngineException exceptions in order to return 401 instead of 500 is not correct. It is possible that multiple concurrent requests on the same node try to set the auto configured elastic user password simultaneously, and only one will succeed, but the others will be returned with 401 which is wrong (should signal authn success).

This is tricky I need to think some more for the proper fix.

Copy link
Contributor

@albertzaharovits albertzaharovits Sep 15, 2021

Choose a reason for hiding this comment

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

From #77036 (comment) , I think we should return 503 (slightly more suggestive than 500, I think) when the promised password cannot be persisted. This is what we set about to do, though back then I haven't considered that any other failures return 401.
Also, in particular, returning 401 because the elastic user becomes present in the .security index in the meantime, is technically wrong because it assumes the update must be with a different password, which might be true in practice, but only because of reserved realm caching, which means that the update comes from a different node, or the user, which probably use a different password to set for elastic, but not necessarily.


/**
* Asynchronous method to put a user. A put user request without a password hash is treated as an update and will fail with a
* {@link ValidationException} if the user does not exist. If a password hash is provided, then we issue a update request with an
Expand Down
41 changes: 31 additions & 10 deletions ...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.logging.log4j.util.Supplier;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.KeyStoreWrapper;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.env.Environment;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.XPackSettings;
Expand Down Expand Up @@ -59,14 +61,16 @@ public class ReservedRealm extends CachingUsernamePasswordRealm {
private final ReservedUserInfo bootstrapUserInfo;
public static final Setting<SecureString> BOOTSTRAP_ELASTIC_PASSWORD = SecureSetting.secureString("bootstrap.password",
KeyStoreWrapper.SEED_SETTING);
public static final Setting<SecureString> AUTOCONFIG_BOOOTSTRAP_ELASTIC_PASSWORD_HASH =
SecureSetting.secureString("autoconfig.password_hash", null);
public static final Setting<SecureString> AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH =
SecureSetting.secureString("autoconfiguration.password_hash", null);

private final NativeUsersStore nativeUsersStore;
private final AnonymousUser anonymousUser;
private final boolean realmEnabled;
private final boolean anonymousEnabled;
private final SecurityIndexManager securityIndex;
private final boolean bootstrapPasswordExists;
private final boolean autoconfigHashExists;

private final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(logger.getName());

Expand All @@ -83,8 +87,12 @@ public ReservedRealm(Environment env, Settings settings, NativeUsersStore native
this.anonymousEnabled = AnonymousUser.isAnonymousEnabled(settings);
this.securityIndex = securityIndex;
final Hasher reservedRealmHasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(settings));
final char[] hash = BOOTSTRAP_ELASTIC_PASSWORD.get(settings).length() == 0 ? new char[0] :
reservedRealmHasher.hash(BOOTSTRAP_ELASTIC_PASSWORD.get(settings));
autoconfigHashExists = AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.exists(settings);
bootstrapPasswordExists = BOOTSTRAP_ELASTIC_PASSWORD.exists(settings);
final char[] hash = (bootstrapPasswordExists || autoconfigHashExists == false) ?
Copy link
Contributor

@jkakavas jkakavas Sep 3, 2021

Choose a reason for hiding this comment

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

I find nested ternary operators really hard to read, can we write this in a clearer way please?

(BOOTSTRAP_ELASTIC_PASSWORD.get(settings).length() == 0 ?
new char[0] : reservedRealmHasher.hash(BOOTSTRAP_ELASTIC_PASSWORD.get(settings))) :
AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.get(settings).getChars();
Copy link
Contributor

@albertzaharovits albertzaharovits Sep 2, 2021

Choose a reason for hiding this comment

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

This complication here is unnecessary, because when you are going to use the bootstrapInfo, you are still going to duplicate the checks. Better to have two separate variables.

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 3, 2021

Choose a reason for hiding this comment

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

I have refactored it

Copy link
Contributor

@jkakavas jkakavas Sep 3, 2021

Choose a reason for hiding this comment

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

The only way for BOOTSTRAP_ELASTIC_PASSWORD.get(settings).length() == 0 to be true is if both bootstrap.password and keystore.seed are missing, which is not a proper state.

Can't we simplify this to just :

if (BOOTSTRAP_ELASTIC_PASSWORD.exists(settings)) {
    hash = reservedRealmHasher.hash(BOOTSTRAP_ELASTIC_PASSWORD.get(settings);
} else {
    hash = AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.get(settings).getChars();
}

?

Copy link
Contributor

@jkakavas jkakavas Sep 3, 2021

Choose a reason for hiding this comment

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

Still, I think Albert's comment was about the fact that you try to use only bootstrapUserInfo here in both cases but this is unncessary complexity since below when you actually going to check against it, you will run the same checks about bootstrap.password and autoconfiguration.password_hash again.
You could have an autoconfiguredElasticUserInfo ReservedUserInfovariable here that gets populated with AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.get(settings).getChars() and below in getUserInfo you can call verifyPassword on that when needed. Or maybe not even that and use a local ReservedUserInfo variable just there if you need it?

bootstrapUserInfo = new ReservedUserInfo(hash, true);
}

Expand All @@ -95,7 +103,7 @@ protected void doAuthenticate(UsernamePasswordToken token, ActionListener<Authen
} else if (ClientReservedRealm.isReserved(token.principal(), config.settings()) == false) {
listener.onResponse(AuthenticationResult.notHandled());
} else {
getUserInfo(token.principal(), ActionListener.wrap((userInfo) -> {
getUserInfo(token.principal(), token.credentials(), ActionListener.wrap((userInfo) -> {
AuthenticationResult result;
if (userInfo != null) {
try {
Expand All @@ -113,7 +121,7 @@ protected void doAuthenticate(UsernamePasswordToken token, ActionListener<Authen
Arrays.fill(userInfo.passwordHash, (char) 0);
}
} else {
result = AuthenticationResult.terminate("failed to authenticate user [" + token.principal() + "]", null);
result = AuthenticationResult.terminate("] [" + token.principal() + "]", null);
}
// we want the finally block to clear out the chars before we proceed further so we handle the result here
listener.onResponse(result);
Expand All @@ -134,7 +142,7 @@ protected void doLookupUser(String username, ActionListener<User> listener) {
} else if (AnonymousUser.isAnonymousUsername(username, config.settings())) {
listener.onResponse(anonymousEnabled ? anonymousUser : null);
} else {
getUserInfo(username, ActionListener.wrap((userInfo) -> {
getUserInfo(username, null, ActionListener.wrap((userInfo) -> {
if (userInfo != null) {
listener.onResponse(getUser(username, userInfo));
} else {
Expand Down Expand Up @@ -212,13 +220,25 @@ public void users(ActionListener<Collection<User>> listener) {
}


private void getUserInfo(final String username, ActionListener<ReservedUserInfo> listener) {
private void getUserInfo(final String username, @Nullable SecureString credentials, ActionListener<ReservedUserInfo> listener) {
Copy link
Contributor

@albertzaharovits albertzaharovits Sep 15, 2021

Choose a reason for hiding this comment

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

I still don't like that a "get" does a "set", and that it takes a credentials parameter for it, which can be null but then the callees can't deal with a null parameter: this code will fail the lookup of the elastic user if it hasn't been created yet.

if (securityIndex.indexExists() == false) {
listener.onResponse(getDefaultUserInfo(username));
if ((bootstrapPasswordExists == false && autoconfigHashExists) && username.equals(ElasticUser.NAME)
&& bootstrapUserInfo.verifyPassword(credentials)) {
nativeUsersStore.createReservedUserAndGetUserInfo(username, bootstrapUserInfo.passwordHash,
WriteRequest.RefreshPolicy.IMMEDIATE, listener);
} else {
listener.onResponse(getDefaultUserInfo(username));
}
Copy link
Contributor

@jkakavas jkakavas Sep 1, 2021

Choose a reason for hiding this comment

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

we can extract this in a method instead of duplicating the code here

Copy link
Contributor

@albertzaharovits albertzaharovits Sep 2, 2021

Choose a reason for hiding this comment

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

++ But we don't need a new method even, we can move this logic in the caller:

@@ -283,6 +284,11 @@ public class NativeUsersStore {
         });
     }

+    void storeAutoConfigElasticUser(ActionListener<User> listener) {
+        createReservedUser(ElasticUser.NAME, autoConfigUserInfo.hash, RefreshPolicy.IMMEDIATE,
+                listener.map(aVoid -> new ElasticUser(true)));
+    }
+
     /**
      * Asynchronous method to put a user. A put user request without a password hash is treated as an update and will fail with a
      * {@link ValidationException} if the user does not exist. If a password hash is provided, then we issue a update request with an
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java
index 2dea4b8b1f2..65382de98ca 100644
--- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java
+++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java
@@ -102,6 +102,10 @@ public class ReservedRealm extends CachingUsernamePasswordRealm {
                         if (userInfo.hasEmptyPassword()) {
                             result = AuthenticationResult.terminate("failed to authenticate user [" + token.principal() + "]", null);
                         } else if (userInfo.verifyPassword(token.credentials())) {
+                            if (userInfo == autoconfigUserInfo)) {
+                                nativeUsersStore.storeAutoConfigElasticUser(listener.map(user -> AuthenticationResult.success(user)));
+                                return;
+                            }
                             final User user = getUser(token.principal(), userInfo);
                             logDeprecatedUser(user);
                             result = AuthenticationResult.success(user);
@@ -242,7 +246,12 @@ public class ReservedRealm extends CachingUsernamePasswordRealm {

     private ReservedUserInfo getDefaultUserInfo(String username) {
         if (ElasticUser.NAME.equals(username)) {
-            return bootstrapUserInfo.deepClone();
+            if (autoconfigHashExists && false == bootstrapPasswordExists) {
+                assert autoconfigUserInfo != null;
+                return autoconfigUserInfo;
+            } else {
+                return bootstrapUserInfo.deepClone();
+            }
         } else {
             return ReservedUserInfo.defaultEnabledUserInfo();
         }

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 3, 2021

Choose a reason for hiding this comment

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

See ^^

Copy link
Contributor

@jkakavas jkakavas Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the main problem with leaving this to the caller of getUserInfo is that it has no way of knowing if the ReservedUserInfo it got is from the keystore or the index (i.e. differentiate between the 1st authentication with that password and subsequent ones )
What if we move the call to nativeUsersStore.storeAutoConfigElasticUser here ? just re-read about the need to return a custom exception

} else {
nativeUsersStore.getReservedUserInfo(username, ActionListener.wrap((userInfo) -> {
if (userInfo == null) {
listener.onResponse(getDefaultUserInfo(username));
if ((bootstrapPasswordExists == false && autoconfigHashExists) && username.equals(ElasticUser.NAME)
&& bootstrapUserInfo.verifyPassword(credentials)) {
nativeUsersStore.createReservedUserAndGetUserInfo(username, bootstrapUserInfo.passwordHash,
WriteRequest.RefreshPolicy.IMMEDIATE, listener);
} else {
listener.onResponse(getDefaultUserInfo(username));
}
} else {
listener.onResponse(userInfo);
}
Expand Down Expand Up @@ -250,5 +270,6 @@ private ReservedUserInfo getDefaultUserInfo(String username) {

public static void addSettings(List<Setting<?>> settingsList) {
settingsList.add(BOOTSTRAP_ELASTIC_PASSWORD);
settingsList.add(AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH);
}
}
4 changes: 2 additions & 2 deletions ...g/elasticsearch/xpack/security/enrollment/tool/AutoConfigGenerateElasticPasswordHash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;

import static org.elasticsearch.xpack.security.authc.esnative.ReservedRealm.AUTOCONFIG_BOOOTSTRAP_ELASTIC_PASSWORD_HASH;
import static org.elasticsearch.xpack.security.authc.esnative.ReservedRealm.AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH;
import static org.elasticsearch.xpack.security.tool.CommandUtils.generatePassword;

/**
Expand Down Expand Up @@ -53,7 +53,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
SecureString elasticPassword = new SecureString(generatePassword(20));
KeyStoreWrapper nodeKeystore = KeyStoreWrapper.bootstrap(env.configFile(), () -> new SecureString(new char[0]))
) {
nodeKeystore.setString(AUTOCONFIG_BOOOTSTRAP_ELASTIC_PASSWORD_HASH.getKey(), hasher.hash(elasticPassword));
nodeKeystore.setString(AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.getKey(), hasher.hash(elasticPassword));
nodeKeystore.save(env.configFile(), new char[0]);
terminal.print(Terminal.Verbosity.NORMAL, elasticPassword.toString());
} catch (Exception e) {
Expand Down
4 changes: 2 additions & 2 deletions ...sticsearch/xpack/security/enrollment/tool/AutoConfigGenerateElasticPasswordHashTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import java.util.Map;

import static org.elasticsearch.test.SecurityIntegTestCase.getFastStoredHashAlgoForTests;
import static org.elasticsearch.xpack.security.authc.esnative.ReservedRealm.AUTOCONFIG_BOOOTSTRAP_ELASTIC_PASSWORD_HASH;
import static org.elasticsearch.xpack.security.authc.esnative.ReservedRealm.AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.emptyString;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -98,7 +98,7 @@ public void testSuccessfullyGenerateAndStoreHash() throws Exception {
assertNotNull(keyStoreWrapper);
keyStoreWrapper.decrypt(new char[0]);
assertThat(keyStoreWrapper.getSettingNames(),
containsInAnyOrder(AUTOCONFIG_BOOOTSTRAP_ELASTIC_PASSWORD_HASH.getKey(), "keystore.seed"));
containsInAnyOrder(AUTOCONFIG_BOOTSTRAP_ELASTIC_PASSWORD_HASH.getKey(), "keystore.seed"));
}

public void testExistingKeystoreWithWrongPassword() throws Exception {
Expand Down