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 13 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
16 changes: 13 additions & 3 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,12 +21,15 @@
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
* handler will return an exception with a RestStatus of 401 and default failure
* response headers like 'WWW-Authenticate'
* response headers like 'WWW-Authenticate' or an ElasticSecurityException with a
* RestStatus of 500 (INTERNAL_SERVER_ERROR)
*/
public class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandler {
private volatile Map<String, List<String>> defaultFailureResponseHeaders;
Expand Down Expand Up @@ -126,7 +130,7 @@ public ElasticsearchSecurityException authenticationRequired(String action, Thre

/**
* Creates an instance of {@link ElasticsearchSecurityException} with
* {@link RestStatus#UNAUTHORIZED} status.
* {@link RestStatus#UNAUTHORIZED} or {@link RestStatus#INTERNAL_SERVER_ERROR}status.
* <p>
* Also adds default failure response headers as configured for this
* {@link DefaultAuthenticationFailureHandler}
Expand All @@ -137,7 +141,10 @@ public ElasticsearchSecurityException authenticationRequired(String action, Thre
* @param message error message
* @param t cause, if it is an instance of
* {@link ElasticsearchSecurityException} asserts status is
* RestStatus.UNAUTHORIZED and adds headers to it, else it will
* RestStatus.UNAUTHORIZED and adds headers to it,
* if it is an instance of {@link ElasticsearchStatusException}
* with status code 500 (INTERNAL_SERVER_ERROR) asserts status is
* RestStatus.UNAUTHORIZED, else it will
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

duplicated text

Suggested change
* RestStatus.UNAUTHORIZED and adds headers to it,
* if it is an instance of {@link ElasticsearchStatusException}
* with status code 500 (INTERNAL_SERVER_ERROR) asserts status is
* RestStatus.UNAUTHORIZED, else it will
* RestStatus.UNAUTHORIZED and adds headers to it.
* if it is an instance of {@link ElasticsearchStatusException}
* with status code 500 (INTERNAL_SERVER_ERROR), asserts status is
* RestStatus.INTERNAL_SERVER_ERROR.
* Else it will

* create a new instance of {@link ElasticsearchSecurityException}
* @param args error message args
* @return instance of {@link ElasticsearchSecurityException}
Expand All @@ -160,6 +167,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);
}
}
6 changes: 3 additions & 3 deletions ...urity/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
public abstract class SecuritySingleNodeTestCase extends ESSingleNodeTestCase {

private static SecuritySettingsSource SECURITY_DEFAULT_SETTINGS = null;
private static CustomSecuritySettingsSource customSecuritySettingsSource = null;
private static RestClient restClient = null;
private static SecureString BOOTSTRAP_PASSWORD = null;
protected static CustomSecuritySettingsSource customSecuritySettingsSource = null;

@BeforeClass
public static void generateBootstrapPassword() {
Expand Down Expand Up @@ -239,9 +239,9 @@ protected boolean transportSSLEnabled() {
return randomBoolean();
}

private class CustomSecuritySettingsSource extends SecuritySettingsSource {
protected class CustomSecuritySettingsSource extends SecuritySettingsSource {

private CustomSecuritySettingsSource(boolean sslEnabled, Path configDir, ESIntegTestCase.Scope scope) {
public CustomSecuritySettingsSource(boolean sslEnabled, Path configDir, ESIntegTestCase.Scope scope) {
super(sslEnabled, configDir, scope);
}

Expand Down
84 changes: 84 additions & 0 deletions ...org/elasticsearch/xpack/security/authc/esnative/PasswordHashAndBootstrpPwdIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.security.authc.esnative;

import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
import org.elasticsearch.cluster.health.ClusterHealthStatus;
import org.elasticsearch.cluster.health.ClusterIndexHealth;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.SecuritySingleNodeTestCase;
import org.elasticsearch.xpack.core.security.user.ElasticUser;

import java.util.Map;

import static java.util.Collections.singletonMap;
import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

public class PasswordHashAndBootstrpPwdIntegTests extends SecuritySingleNodeTestCase {

@Override
public Settings nodeSettings() {
Settings customSettings = customSecuritySettingsSource.nodeSettings(0, Settings.EMPTY);
MockSecureSettings mockSecuritySettings = new MockSecureSettings();
mockSecuritySettings.setString("autoconfiguration.password_hash", // password1
"{PBKDF2_STRETCH}1000$JnmgicthPZkczB8MaQeJiV6IX43h7mSfPSzESqnYYSA=$OZKH5XFNK+M65mcKal6zgugWRcpl6wUXmSQZ6hPy+iw=");
mockSecuritySettings.setString("bootstrap.password", "password");
Settings.Builder builder = Settings.builder().put(customSettings, true);
builder.setSecureSettings(mockSecuritySettings);
return builder.build();
}

@Override
protected boolean addMockHttpTransport() {
return false;
}

@Override
protected boolean transportSSLEnabled() { return false; }

public void testBootstrapPwdAuthenticatePwdHashNotIndexNotCreated() {
ElasticsearchStatusException e = expectThrows( ElasticsearchStatusException.class,
() -> client()
.filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue(ElasticUser.NAME,
new SecureString("password1".toCharArray()))))
.admin()
.cluster()
.prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus()
.get() );

assertThat(e.status(), equalTo(RestStatus.UNAUTHORIZED));

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

assertThat(response, notNullValue());
assertThat(response.isTimedOut(), equalTo(false));
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.getStatus(), equalTo(ClusterHealthStatus.GREEN));
boolean securityIndexCreated = false;
for (Map.Entry<String, ClusterIndexHealth> indexEntry: response.getIndices().entrySet()) {
if (indexEntry.getKey().startsWith(".security")) {
securityIndexCreated = true;
break;
}
}
assertFalse(securityIndexCreated);
}
}
130 changes: 130 additions & 0 deletions ...java/org/elasticsearch/xpack/security/authc/esnative/PasswordHashPromotionIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.security.authc.esnative;

import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
import org.elasticsearch.cluster.health.ClusterHealthStatus;
import org.elasticsearch.cluster.health.ClusterIndexHealth;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.license.LicenseService;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.SecuritySingleNodeTestCase;
import org.elasticsearch.xpack.core.security.user.ElasticUser;

import java.util.Map;

import static java.util.Collections.singletonMap;
import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

public class PasswordHashPromotionIntegTests extends SecuritySingleNodeTestCase {

@Override
public Settings nodeSettings() {
Settings customSettings = customSecuritySettingsSource.nodeSettings(0, Settings.EMPTY);
MockSecureSettings mockSecuritySettings = new MockSecureSettings();
mockSecuritySettings.setString("autoconfiguration.password_hash",
"{PBKDF2_STRETCH}1000$JnmgicthPZkczB8MaQeJiV6IX43h7mSfPSzESqnYYSA=$OZKH5XFNK+M65mcKal6zgugWRcpl6wUXmSQZ6hPy+iw=");
Settings.Builder builder = Settings.builder().put(customSettings, false); // don't bring in bootstrap.password
builder.put(LicenseService.SELF_GENERATED_LICENSE_TYPE.getKey(), "trial");
builder.put("transport.type", "security4");
builder.put("path.home", customSecuritySettingsSource.nodePath(0));
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

do we need this ?

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 7, 2021

Choose a reason for hiding this comment

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

Not really

builder.setSecureSettings(mockSecuritySettings);
return builder.build();
}

@Override
protected boolean addMockHttpTransport() {
return false;
}

@Override
protected boolean transportSSLEnabled() { return false; }

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

assertThat(response, notNullValue());
assertThat(response.isTimedOut(), equalTo(false));
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.getStatus(), equalTo(ClusterHealthStatus.GREEN));
boolean securityIndexCreated = false;
for (Map.Entry<String, ClusterIndexHealth> indexEntry: response.getIndices().entrySet()) {
if (indexEntry.getKey().startsWith(".security")) {
securityIndexCreated = true;
break;
}
}
assertTrue(securityIndexCreated);
securityIndexExistsAuthenticate();
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.

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

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 6, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

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

}

public void testInvalidPasswordHashNoSecurityIndex() {
ElasticsearchStatusException e = expectThrows( ElasticsearchStatusException.class,
() -> client()
.filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue(ElasticUser.NAME,
new SecureString("password".toCharArray()))))
.admin()
.cluster()
.prepareHealth()
.get() );

assertThat(e.status(), equalTo(RestStatus.UNAUTHORIZED));

ClusterHealthResponse response = client()
.filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue("test_user",
new SecureString("x-pack-test-password".toCharArray()))))
.admin()
.cluster()
.prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus()
.get();

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

public void securityIndexExistsAuthenticate() {
ClusterHealthResponse response = client()
.filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue(ElasticUser.NAME,
new SecureString("password1".toCharArray()))))
.admin()
.cluster()
.prepareHealth()
.get();

assertThat(response, notNullValue());
assertThat(response.isTimedOut(), equalTo(false));
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.getStatus(), equalTo(ClusterHealthStatus.GREEN));
boolean securityIndexCreated = false;
for (Map.Entry<String, ClusterIndexHealth> indexEntry: response.getIndices().entrySet()) {
if (indexEntry.getKey().startsWith(".security")) {
securityIndexCreated = true;
break;
}
}
assertTrue(securityIndexCreated);
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

we have asserted that before we even call this method

}
}
33 changes: 28 additions & 5 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 All @@ -21,6 +23,7 @@
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.support.ContextPreservingActionListener;
import org.elasticsearch.action.support.TransportActions;
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.action.support.WriteRequest.RefreshPolicy;
import org.elasticsearch.action.update.UpdateResponse;
import org.elasticsearch.client.Client;
Expand All @@ -47,6 +50,7 @@
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.security.user.ElasticUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.User.Fields;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
Expand All @@ -60,6 +64,9 @@
import java.util.function.Consumer;
import java.util.function.Supplier;

import static org.elasticsearch.action.DocWriteRequest.OpType.CREATE;
import static org.elasticsearch.action.DocWriteRequest.OpType.UPDATE;
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 @@ -252,7 +259,8 @@ public void onResponse(UpdateResponse updateResponse) {
public void onFailure(Exception e) {
if (isIndexNotFoundOrDocumentMissing(e)) {
if (docType.equals(RESERVED_USER_TYPE)) {
createReservedUser(username, request.passwordHash(), request.getRefreshPolicy(), listener);
createOrUpdateReservedUser(username, request.passwordHash(), request.getRefreshPolicy(),
UPDATE, listener::onFailure, listener);
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

This needs to be INDEX, not UPDATE

Copy link
Contributor Author

@BigPandaToo BigPandaToo Sep 7, 2021

Choose a reason for hiding this comment

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

Just figured it out!

} else {
logger.debug((org.apache.logging.log4j.util.Supplier<?>) () ->
new ParameterizedMessage("failed to change password for user [{}]", request.username()), e);
Expand All @@ -272,17 +280,32 @@ public void onFailure(Exception e) {
* Asynchronous method to create a reserved user with the given password hash. The cache for the user will be cleared after the document
* has been indexed
*/
private void createReservedUser(String username, char[] passwordHash, RefreshPolicy refresh, ActionListener<Void> listener) {
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
protected void createOrUpdateReservedUser(String username, char[] passwordHash, RefreshPolicy refresh, DocWriteRequest.OpType opType,
Consumer<Exception> consumer, ActionListener<Void> listener) {
securityIndex.prepareIndexIfNeededThenExecute(consumer, () -> {
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
client.prepareIndex(SECURITY_MAIN_ALIAS).setId(getIdForUser(RESERVED_USER_TYPE, username))
client.prepareIndex(SECURITY_MAIN_ALIAS).setOpType(opType)
.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) -> clearRealmCache(username, l, null)), client::index);
listener.<IndexResponse>delegateFailure((l, indexResponse) -> {
if (opType.equals(CREATE) == false) {
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

No need to differentiate here , see #77291 (comment)

clearRealmCache(username, l, null);
} else {
l.onResponse(null);
}
}), client::index);
});
}

void storeAutoconfiguredElasticUser(ReservedUserInfo elasticUserInfo, ActionListener<ReservedUserInfo> listener) {
createOrUpdateReservedUser(ElasticUser.NAME, elasticUserInfo.passwordHash, WriteRequest.RefreshPolicy.IMMEDIATE,
DocWriteRequest.OpType.CREATE, (e) -> {
listener.onFailure(new ElasticsearchStatusException(e.getMessage(), INTERNAL_SERVER_ERROR, e.getCause()));
Copy link
Contributor

@jkakavas jkakavas Sep 7, 2021

Choose a reason for hiding this comment

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

If this is a VersionConflictEngineException, it means that something set the password for the elastic user between the time we called getReservedUserInfo and now. We should return 401 in that case

}, listener.delegateFailure((l, v) -> l.onResponse(elasticUserInfo.deepClone())));
}
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
Loading