Setting elastic password from stored hash #77036

BigPandaToo · 2021-08-30T19:03:03Z

In package installations, we will be generating the password of
the elastic user on installation and we will be storing the hash
of it to the elasticsearch.keystore.
This change will

attempt to create the security index and elastic user document at the
time of the first successful authentication using the
the "promised" password
if bootstrap.password setting is detected, won’t attempt to use
autoconfiguration.password_hash setting and proceed with the normal
authentication path
if elastic user document exists, won't attempt to use
autoconfiguration.password_hash setting and proceed with the normal
authentication path
if unable to create the Security Index for any reason, returns
HTTP_SERVER_ERROR to indicate that the cluster is probably not ready yet

Resolves: #75704

In package installations, we will be generating the password of the elastic user on installation and we will be storing the hash of it to the elasticsearch.keystore. This change will - attempt to create the security index and elastic user document at the time of the first successful authentication using the the "promised" password - if bootstrap.password setting is detected, won’t attempt to use autoconfiguration.password_hash setting and proceed with the normal authentication path - if elastic user document exists, won't attempt to use autoconfiguration.password_hash setting and proceed with the normal authentication path - if unable to create the Security Index for any reason, returns HTTP_SERVER_ERROR to indicate that the cluster is probably not ready yet Resolves: elastic#75704

elasticmachine · 2021-08-30T19:03:07Z

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

BigPandaToo · 2021-08-31T07:36:38Z

@elasticmachine update branch

jkakavas

Thanks Lyudmila, I think this does what we've set out to do in the design doc. I think it's ++ to polish and open as a proper PR when you feel ready for it. We might be able to wire things differently in the ReservedRealm class but I don't have a concrete suggestion now, nor specific concerns around your approach. Let's see what @albertzaharovits thinks also

jkakavas · 2021-08-31T20:30:55Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

+    /**
+     * 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,


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

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

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

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.

jkakavas · 2021-09-01T11:43:39Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

+            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));
+            }


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

++ 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(); }

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

albertzaharovits · 2021-09-02T13:18:24Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

+        final char[] hash = (bootstrapPasswordExists || autoconfigHashExists == false) ?
+            (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();


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.

I have refactored it

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(); }

?

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?

albertzaharovits · 2021-09-02T13:20:40Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

+                        Fields.ENABLED.getPreferredName(),
+                        true, Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE)
+                    .setRefreshPolicy(refresh).request(),
+                listener.<IndexResponse>delegateFailure((l, indexResponse) -> getReservedUserInfo(username, l)),


The get after a successful create is unnecessary.

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

albertzaharovits

The logic looks OK, the placement of it definitely needs a more careful consideration (eg we shouldn't do a store in a get).

I've left a rough code suggestion in https://github.com/elastic/elasticsearch/pull/77036/files#r701072274

BigPandaToo · 2021-09-02T15:03:22Z

@elasticmachine update branch

BigPandaToo · 2021-09-02T18:08:44Z

@elasticmachine update branch

jkakavas · 2021-09-03T19:25:41Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

-            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) ?


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

jkakavas · 2021-09-03T19:32:51Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

+        final char[] hash = (bootstrapPasswordExists || autoconfigHashExists == false) ?
+            (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();


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(); }

?

jkakavas · 2021-09-03T19:46:02Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

+        final char[] hash = (bootstrapPasswordExists || autoconfigHashExists == false) ?
+            (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();


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?

jkakavas · 2021-09-03T20:21:04Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

+            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));
+            }


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

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

jkakavas · 2021-09-03T20:39:07Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

+                        Fields.ENABLED.getPreferredName(),
+                        true, Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE)
+                    .setRefreshPolicy(refresh).request(),
+                listener.<IndexResponse>delegateFailure((l, indexResponse) -> getReservedUserInfo(username, l)),


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

jkakavas · 2021-09-03T20:46:12Z

...st/java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java

+            }
+        }
+        assertTrue(securityIndexCreated);
+        securityIndexExistsAuthenticate();


Why are we doing this call to another method that is actually the same method as we are in right now ?

Just validating that now when the index exists we can authenticate (not sure it is necessary...)

I'd simply do

ClusterHealthResponse response = client() .filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue(ElasticUser.NAME, new SecureString("password1".toCharArray())))) .admin() .cluster() .prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus() .get();

once more here

jkakavas · 2021-09-03T20:48:15Z

...st/java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java

+        boolean securityIndexNotCreated = true;
+        for (Map.Entry<String, ClusterIndexHealth>  indexEntry: response.getIndices().entrySet()) {
+            if (indexEntry.getKey().startsWith(".security")) {
+                securityIndexNotCreated = false;


I find this confusing ( the double negation ) , I think what you do above in the previous test is easier to read

jkakavas · 2021-09-03T20:50:03Z

...st/java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java

+        builder.put(LicenseService.SELF_GENERATED_LICENSE_TYPE.getKey(), "trial");
+        builder.put("transport.type", "security4");
+        builder.put("path.home", customSecuritySettingsSource.nodePath(0));
+        builder.setSecureSettings(new MockSecureSettings());


nit: I'd create the MockSecureSettings first , then call setString, then call builder.setSecureSettings

jkakavas · 2021-09-03T20:55:17Z

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java

        }
        ((MockSecureSettings) builder.getSecureSettings()).setString("bootstrap.password", BOOTSTRAP_PASSWORD.toString());
+        // Verify `autoconfiguration.password_hash` is not used when `bootstrap.password` set
+        ((MockSecureSettings) builder.getSecureSettings()).setString("autoconfiguration.password_hash",


If the intention here is to see that all other tests that implement SecurityIntegTestCase don't fail , now that you added autoconfiguration.passowrd_hash then I don'think this is the way to do this.
Instead of polluting the secure settings of any node in any test and check implicitly that nothing breaks, add a test case in the newly introduced integ test that ensures that autoconfiguration.password_hash is not used when bootstrap.password is set

jkakavas · 2021-09-03T20:58:16Z

...st/java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java

+    @Override
+    public Settings nodeSettings() {
+        Settings customSettings = customSecuritySettingsSource.nodeSettings(0, Settings.EMPTY);
+        Settings.Builder builder = Settings.builder().put(customSettings, false); // handle secure settings separately


"handle secure settings separately" == don't bring in bootstrap.password

BigPandaToo · 2021-09-06T09:00:26Z

@elasticmachine update branch

BigPandaToo · 2021-09-06T12:39:40Z

@elasticmachine update branch

jkakavas · 2021-09-07T15:42:07Z

...a/org/elasticsearch/xpack/security/authc/esnative/PasswordHashAndBootstrapPwdInterTests.java

 import static org.hamcrest.Matchers.notNullValue;

-public class PasswordHashAndBootstrpPwdIntegTests extends SecuritySingleNodeTestCase {
+public class PasswordHashAndBootstrapPwdInterTests extends SecuritySingleNodeTestCase {


PasswordHashAndBootstrapPasswordIntegTests

jkakavas · 2021-09-07T15:45:02Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java

        }
    }

+    private void getAutoconfiguredUser(final String username, SecureString credentials, ActionListener<ReservedUserInfo> listener) {


Naming is hard.. does this get the/an auto configured user ? Don't have a good suggestion

You are right, not necessary autoconfigured. May be getAutoconfiguredOrDefaultUser

...st/java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java

jkakavas

LGTM , I left some comments for consideration but won't block. Feel free to merge this once CI is green

jkakavas · 2021-09-07T16:13:24Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

                                if (docType.equals(RESERVED_USER_TYPE)) {
-                                    createReservedUser(username, request.passwordHash(), request.getRefreshPolicy(), listener);
+                                    createOrUpdateReservedUser(username, request.passwordHash(), request.getRefreshPolicy(),
+                                        UPDATE, listener::onFailure, listener);


This needs to be INDEX, not UPDATE

Just figured it out!

jkakavas

https://github.com/elastic/elasticsearch/pull/77036/files#r703651072 needs to be addressed before merging

`createOrUpdateReservedUser` to INDEX. More cleanup and renaming

BigPandaToo · 2021-09-07T18:05:29Z

@elasticmachine run elasticsearch-ci/part-2

BigPandaToo · 2021-09-07T18:29:14Z

@elasticmachine run elasticsearch-ci/part-2

BigPandaToo · 2021-09-07T18:30:51Z

@elasticmachine run elasticsearch-ci/part-2

BigPandaToo · 2021-09-07T18:33:10Z

@elasticmachine run elasticsearch-ci/part-2-fips

password won't work when both bootstrap.password and keystore.seed are missing

jkakavas

LGTM, lets hold off merging for now as we discussed

jkakavas · 2021-09-08T08:23:49Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

        char[] passwordHash,
        DocWriteRequest.OpType opType,
        RefreshPolicy refresh,
+        Consumer<Exception> consumer,


Do we need this ? We only ever pass the onFailure of the ActionListener we also pass as a parameter, so we can just simply leave this as it was ?

Happy to push this change if needed, no worries

Please do! I merged the conflict, but there are some failures I don't have time to look at (my flight is in 2hr ))).

Do we need this ? We only ever pass the onFailure of the ActionListener we also pass as a parameter, so we can just simply leave this as it was ?

I use it for the special error handling to return INTERNAL_SERVER_ERROR, look at storeAutoconfiguredElasticUser

jkakavas · 2021-09-12T19:02:46Z

@albertzaharovits we didn't have to merge this before you return so feel free to take another look when you're back and I can make all necessary changes and amendments while Lyudmila is away

albertzaharovits · 2021-09-14T16:08:12Z

...security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

+                listener.onFailure(new ElasticsearchStatusException(e.getMessage(), INTERNAL_SERVER_ERROR, e.getCause()));
+            }
+            }, listener.delegateFailure((l, v) -> l.onResponse(elasticUserInfo.deepClone())));
+    }


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.

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.

albertzaharovits · 2021-09-14T17:03:06Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+        } else if (t instanceof ElasticsearchStatusException && ((ElasticsearchStatusException) t).status() == INTERNAL_SERVER_ERROR) {
+            ese = internalServerError(message, t, args);
+            containsNegotiateWithToken = false;
        } else {


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.

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

if they successfully authenticate once then they can successfully authenticate with these credentials until something (not ES internal) changes the password.

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.

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)

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.

albertzaharovits · 2021-09-15T05:34:23Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java


+    private void getAutoconfiguredOrDefaultUser(final String username, SecureString credentials,
+                                                ActionListener<ReservedUserInfo> listener) {
+        if (autoconfigured && username.equals(ElasticUser.NAME) && bootstrapUserInfo.verifyPassword(credentials)) {


This condition here makes it so that the auto configured elastic password is actually checked twice (second time in ReservedRealm#doAuthenticate) when wrong.
Not a big issue in practice, but it shows that the control flow is not polished.

albertzaharovits · 2021-09-15T06:13:47Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealm.java



-    private void getUserInfo(final String username, ActionListener<ReservedUserInfo> listener) {
+    private void getUserInfo(final String username, @Nullable SecureString credentials, ActionListener<ReservedUserInfo> listener) {


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.

albertzaharovits · 2021-10-05T08:47:11Z

Closed in favor of #78306 .

BigPandaToo added >feature :Security/Security Security issues without another label v8.0.0 Team:Security Meta label for security team labels Aug 30, 2021

BigPandaToo marked this pull request as draft August 30, 2021 19:03

Merge branch 'master' into Store_hash

621c735

jkakavas reviewed Sep 1, 2021

View reviewed changes

albertzaharovits reviewed Sep 2, 2021

View reviewed changes

adding tests

1aaf1f9

BigPandaToo marked this pull request as ready for review September 2, 2021 14:47

Merge branch 'master' into Store_hash

9fd474c

elasticmachine and others added 3 commits September 3, 2021 04:08

Merge branch 'master' into Store_hash

7260cb4

Addressing PR feedback and refactoring

92a48eb

Addressing PR feedback and refactoring

9c543fa

BigPandaToo requested review from albertzaharovits and jkakavas September 3, 2021 18:20

jkakavas reviewed Sep 3, 2021

View reviewed changes

Merge branch 'master' into Store_hash

2fcca5c

elasticmachine and others added 2 commits September 6, 2021 22:39

Merge branch 'master' into Store_hash

c28a3ad

Addressing PR feedback

1e09d7a

jkakavas reviewed Sep 7, 2021

View reviewed changes

jkakavas approved these changes Sep 7, 2021

View reviewed changes

jkakavas reviewed Sep 7, 2021

View reviewed changes

jkakavas requested changes Sep 7, 2021

View reviewed changes

BigPandaToo added 2 commits September 7, 2021 18:57

Changing default for OpType

6227d1f

`createOrUpdateReservedUser` to INDEX. More cleanup and renaming

Fixing a long line

a4424d9

BigPandaToo requested a review from jkakavas September 7, 2021 17:29

Fixing tests/ making sure empty

dce0ef5

password won't work when both bootstrap.password and keystore.seed are missing

jkakavas approved these changes Sep 7, 2021

View reviewed changes

BigPandaToo added 2 commits September 8, 2021 09:36

Merge branch 'master' into Store_hash

91d590a

Resolving conflicts

5322487

jkakavas reviewed Sep 8, 2021

View reviewed changes

jkakavas self-assigned this Sep 12, 2021

albertzaharovits reviewed Sep 14, 2021

View reviewed changes

albertzaharovits reviewed Sep 15, 2021

View reviewed changes

jkakavas assigned albertzaharovits and unassigned jkakavas Sep 15, 2021

jkakavas self-requested a review September 15, 2021 06:52

albertzaharovits mentioned this pull request Sep 27, 2021

Auto-configure the elastic user password #78306

Merged

albertzaharovits closed this Oct 5, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021



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

Setting elastic password from stored hash #77036

Setting elastic password from stored hash #77036

Uh oh!

Conversation

BigPandaToo commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 30, 2021

Uh oh!

BigPandaToo commented Aug 31, 2021

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

BigPandaToo commented Sep 2, 2021

Uh oh!

BigPandaToo commented Sep 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigPandaToo commented Aug 30, 2021 •

edited

Loading

jkakavas Sep 3, 2021 •

edited

Loading

jkakavas Sep 3, 2021 •

edited

Loading

jkakavas Sep 7, 2021 •

edited

Loading