Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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) set status to
* 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

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

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

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
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);
}
}
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
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 PasswordHashAndBootstrapPasswordIntegTests 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);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/*
* 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.action.support.PlainActionFuture;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.health.ClusterHealthStatus;
import org.elasticsearch.cluster.health.ClusterIndexHealth;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.Strings;
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.SecuritySettingsSource;
import org.elasticsearch.test.SecuritySingleNodeTestCase;
import org.elasticsearch.xpack.core.security.action.user.PutUserAction;
import org.elasticsearch.xpack.core.security.action.user.PutUserRequest;
import org.elasticsearch.xpack.core.security.action.user.PutUserResponse;
import org.elasticsearch.xpack.core.security.user.ElasticUser;

import java.util.Map;
import java.util.concurrent.ExecutionException;

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.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)
.setWaitForNodes(Integer.toString(1))
.setWaitForGreenStatus()
.get();

assertThat(response, notNullValue());
assertThat(response.isTimedOut(), equalTo(false));
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.getStatus(), equalTo(ClusterHealthStatus.GREEN));
assertTrue(securityIndexExists());

// Now as the document exists let's try to authenticate again
response = client()
.filterWithHeader(singletonMap("Authorization", basicAuthHeaderValue(ElasticUser.NAME,
new SecureString("password1".toCharArray()))))
.admin()
.cluster()
.prepareHealth()
.get();

assertThat(response, notNullValue());
assertThat(response.status(), equalTo(RestStatus.OK));
}

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

public void testSecurityIndexExistsButElasticuserNot() throws Exception {
// Create a user to create the Index
createUser("user", SecuritySettingsSource.TEST_PASSWORD_HASHED.toCharArray(), Strings.EMPTY_ARRAY);
assertTrue(securityIndexExists());

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

// TODO: Add a test a test where we index the document for the elastic user manually and then we try to authenticate with the
// autoconfigured password hash and it fails

private void createUser(String username, char[] password, String[] roles) throws ExecutionException, InterruptedException {
final PutUserRequest putUserRequest = new PutUserRequest();
putUserRequest.username(username);
putUserRequest.roles(roles);
putUserRequest.passwordHash(password);
PlainActionFuture<PutUserResponse> listener = new PlainActionFuture<>();
final Client client = client().filterWithHeader(
Map.of("Authorization", basicAuthHeaderValue("test_user", new SecureString("x-pack-test-password"
.toCharArray()))));
client.execute(PutUserAction.INSTANCE, putUserRequest, listener);
final PutUserResponse putUserResponse = listener.get();
assertTrue(putUserResponse.created());
}

private boolean securityIndexExists () {
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 securityIndexExists = false;
for (Map.Entry<String, ClusterIndexHealth> indexEntry: response.getIndices().entrySet()) {
if (indexEntry.getKey().startsWith(".security")) {
securityIndexExists = true;
break;
}
}
return securityIndexExists;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,15 @@ public void accept(SecurityIndexManager.State previousState, SecurityIndexManage
elasticPassword.getChars(),
DocWriteRequest.OpType.CREATE,
WriteRequest.RefreshPolicy.IMMEDIATE,
this::outputOnError,
ActionListener.wrap(result -> {
nativeUsersStore
.updateReservedUser(
KibanaSystemUser.NAME,
kibanaSystemPassword.getChars(),
DocWriteRequest.OpType.CREATE,
WriteRequest.RefreshPolicy.IMMEDIATE,
this::outputOnError,
ActionListener.wrap(
r -> {
outputOnSuccess(elasticPassword, kibanaSystemPassword);
Expand Down
Loading