Skip to content
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

Set elastic password from stored hash #76319

Closed
wants to merge 3 commits into from
Closed

Set elastic password from stored hash #76319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58f6269
Set elastic password from stored hash
jkakavas Aug 10, 2021
0356619
add a packaging test for verification
jkakavas Aug 11, 2021
c82c308
spotless fix
jkakavas Aug 11, 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
44 changes: 44 additions & 0 deletions ...src/test/java/org/elasticsearch/packaging/test/PackageSecurityAutoconfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.packaging.test;

import org.elasticsearch.packaging.util.Shell;
import org.junit.BeforeClass;

import static org.elasticsearch.packaging.util.Packages.assertInstalled;
import static org.elasticsearch.packaging.util.Packages.assertRemoved;
import static org.elasticsearch.packaging.util.Packages.installPackage;
import static org.elasticsearch.packaging.util.Packages.verifyPackageInstallation;
import static org.elasticsearch.packaging.util.ServerUtils.validateCredentials;
import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assume.assumeTrue;

public class PackageSecurityAutoconfigurationTests extends PackagingTestCase {

@BeforeClass
public static void filterDistros() {
assumeTrue("rpm or deb", distribution.isPackage());
}

public void test10ElasticPasswordHash() throws Exception {
assertRemoved(distribution());
installation = installPackage(sh, distribution());
assertInstalled(distribution());
verifyPackageInstallation(installation, distribution(), sh);
Shell.Result keystoreListResult = installation.executables().keystoreTool.run("list");
// Keystore should be created already by the installation and it should contain only "keystore.seed" at this point
assertThat(keystoreListResult.stdout, containsString("keystore.seed"));
// With future changes merged, this would be automatically populated on installation. For now, add it manually
installation.executables().keystoreTool.
// $2a$10$R2oFwbHR/9x9.e/bQpJ6IeHKUVP08KHQ9LcZPMlWeyuQuYboR82fm is the hash of thisisalongenoughpassword
run("add -x autoconfiguration.password_hash", "$2a$10$R2oFwbHR/9x9.e/bQpJ6IeHKUVP08KHQ9LcZPMlWeyuQuYboR82fm");
startElasticsearch();
validateCredentials("elastic", "thisisalongenoughpassword", null);
}
}
7 changes: 7 additions & 0 deletions qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ public static void runElasticsearchTests(String username, String password) throw
makeRequest(Request.Delete("http://localhost:9200/library"), username, password, null);
}

public static void validateCredentials(String username, String password, Path caCert) throws Exception {
final HttpResponse response = execute(Request.Get("http://localhost:9200/"), username, password, caCert);
if (response.getStatusLine().getStatusCode() == 401) {
throw new RuntimeException("Failed to authenticate as [" + username + ":" + password + "]");
}
}

public static String makeRequest(Request request) throws Exception {
return makeRequest(request, null, null, null);
}
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
package org.elasticsearch.xpack.core;

import org.apache.logging.log4j.LogManager;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.ssl.SslVerificationMode;
import org.elasticsearch.jdk.JavaVersion;
import org.elasticsearch.common.settings.Setting;
Expand Down Expand Up @@ -213,6 +215,8 @@ private XPackSettings() {
public static final String TRANSPORT_SSL_PREFIX = SecurityField.setting("transport.ssl.");
private static final SSLConfigurationSettings TRANSPORT_SSL = SSLConfigurationSettings.withPrefix(TRANSPORT_SSL_PREFIX, true);

public static final Setting<SecureString> ELASTIC_PASSWORD_HASH = SecureSetting.secureString("autoconfiguration.password_hash", null);

/** Returns all settings created in {@link XPackSettings}. */
public static List<Setting<?>> getAllSettings() {
ArrayList<Setting<?>> settings = new ArrayList<>();
Expand All @@ -232,6 +236,7 @@ public static List<Setting<?>> getAllSettings() {
settings.add(USER_SETTING);
settings.add(PASSWORD_HASHING_ALGORITHM);
settings.add(ENROLLMENT_ENABLED);
settings.add(ELASTIC_PASSWORD_HASH);
return Collections.unmodifiableList(settings);
}

Expand Down
34 changes: 28 additions & 6 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.common.network.NetworkService;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -131,6 +132,7 @@
import org.elasticsearch.xpack.core.security.action.token.RefreshTokenAction;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateAction;
import org.elasticsearch.xpack.core.security.action.user.ChangePasswordAction;
import org.elasticsearch.xpack.core.security.action.user.ChangePasswordRequest;
import org.elasticsearch.xpack.core.security.action.user.DeleteUserAction;
import org.elasticsearch.xpack.core.security.action.user.GetUserPrivilegesAction;
import org.elasticsearch.xpack.core.security.action.user.GetUsersAction;
Expand Down Expand Up @@ -335,6 +337,7 @@
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
import static org.elasticsearch.xpack.core.XPackSettings.API_KEY_SERVICE_ENABLED_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.ELASTIC_PASSWORD_HASH;
import static org.elasticsearch.xpack.core.XPackSettings.HTTP_SSL_ENABLED;
import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS;
import static org.elasticsearch.xpack.security.operator.OperatorPrivileges.OPERATOR_PRIVILEGES_ENABLED;
Expand Down Expand Up @@ -375,6 +378,8 @@ public class Security extends Plugin implements SystemIndexPlugin, IngestPlugin,
private final List<SecurityExtension> securityExtensions = new ArrayList<>();
private final SetOnce<Transport> transportReference = new SetOnce<>();
private final SetOnce<ScriptService> scriptServiceReference = new SetOnce<>();
private final SetOnce<SecureString> elasticPasswordHash = new SetOnce<>();
private final SetOnce<NativeUsersStore> nativeUsersStore = new SetOnce<>();

public Security(Settings settings, final Path configPath) {
this(settings, configPath, Collections.emptyList());
Expand All @@ -387,9 +392,6 @@ public Security(Settings settings, final Path configPath) {
this.enabled = XPackSettings.SECURITY_ENABLED.get(settings);
if (enabled) {
runStartupChecks(settings);
// we load them all here otherwise we can't access secure settings since they are closed once the checks are
// fetched

Automatons.updateConfiguration(settings);
} else {
this.bootstrapChecks.set(Collections.emptyList());
Expand All @@ -411,6 +413,7 @@ protected Clock getClock() {
}
protected SSLService getSslService() { return XPackPlugin.getSharedSslService(); }
protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); }
protected SecureString getElasticPasswordHash() { return this.elasticPasswordHash.get(); }

@Override
public Collection<Object> createComponents(Client client, ClusterService clusterService, ThreadPool threadPool,
Expand Down Expand Up @@ -439,8 +442,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste

scriptServiceReference.set(scriptService);

// We need to construct the checks here while the secure settings are still available.
// If we wait until #getBoostrapChecks the secure settings will have been cleared/closed.
final List<BootstrapCheck> checks = new ArrayList<>();
checks.addAll(Arrays.asList(
new ApiKeySSLBootstrapCheck(),
Expand All @@ -465,6 +466,12 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste

securityIndex.set(SecurityIndexManager.buildSecurityIndexManager(client, clusterService, SECURITY_MAIN_INDEX_DESCRIPTOR));

// Store this because when the listener we register will be called, secure settings will be closed
if (ELASTIC_PASSWORD_HASH.exists(settings)) {
elasticPasswordHash.set(ELASTIC_PASSWORD_HASH.get(settings));
securityIndex.get().addStateListener(this::possiblySetElasticPassword);
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed already on another channel, I think we need to better define the "lifecycle" of this new password setting. Right now, it is: "the auto-configured elastic password becomes effective soon after the Security index is created, hopefully".

We've discussed already that I was thinking that it has to be effective as soon as the node can handle requests. We can tease it apart, but, let me propose another alternative:

Currently, user authentication does not trigger the creation of the Security index. It doesn't have to. But I propose we make this new auto-configured password be different in this regard. When a client uses the auto-configured password to authenticate elastic, we try to create the index (if it doesn't already exist) and set the elastic user password, before we confirm the authentication as successful. If any operation fails, ie index creation, indexing, or racing with another node that sets a different password, authentication fails. If the index already exists, no magic happens.
The benefit of this approach is that the client is never going to receive an authentication failure for the "promised" password. They can still get an error (ie fail to create the index) but this is not technically breaking the promise, the node is broken, for other APIs too It also somewhat solves the issue with a different value across the nodes: only the password of the first successful authentication is going to be valid henceforth (at least it is behavior that we can easily explain in words)
I also think it's also worth it to make the auto-configured password take precedence over the bootstrap.password and keystore.seed (ie they are ignored if there is an auto configured password hash). This is a tangent conversation, and I can be convinced otherwise. My reasoning is that it is confusing to technically have multiple passwords valid for the same user at a point in time.

Let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've discussed already that I was thinking that it has to be effective as soon as the node can handle requests. We can tease it apart, but, let me propose another alternative:

Yeap, I have the code to validate the password of the elastic user against the hash in the keystore (until it is set in the index) in another branch as we discussed, I'll update this PR shortly

Right now, it is: "the auto-configured elastic password becomes effective soon after the Security index is created, hopefully".

It's not "created", it's "recovered" but I'm being pedantic :) We've agreed that this won't be the case ( see above ). It is more like "the auto-configured elastic password is effective immediately and will be moved to the security index after the Security index is recovered, hopefully"

let me propose another alternative:
...
They can still get an error (ie fail to create the index) but this is not technically breaking the promise

wouldn't this prevent the user to authenticate with the "promised" password before the node has read the cluster state or if it fails to do so ?

solves the issue with a different value across the nodes

I don't think this issue can manifest in packaged installations. It would need two nodes configured to be members of the same cluster (sharing configuration) starting up for the first time at the same time. But:

  • For any subsequent node to join an existing cluster, the first node needs to be up and running (so the password would be already set ). It might be the case that we failed to set the password during the boot of the first node, one can rightfully argue, but
  • For any subsequent node to join an existing cluster, the user needs to run an external tool to consume the enrollment token and reconfigure the node. This external tool can very well remove the autoconfiguration.password_hash from that node's keystore so that it wouldn't even try to promote this to the index.

}

final TokenService tokenService = new TokenService(
settings,
Clock.systemUTC(),
Expand All @@ -480,6 +487,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste

// realms construction
final NativeUsersStore nativeUsersStore = new NativeUsersStore(settings, client, securityIndex.get());
this.nativeUsersStore.set(nativeUsersStore);
final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(settings, client, securityIndex.get(),
scriptService);
final AnonymousUser anonymousUser = new AnonymousUser(settings);
Expand All @@ -506,7 +514,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(reservedRealm);

securityIndex.get().addStateListener(nativeRoleMappingStore::onSecurityIndexStateChange);

final CacheInvalidatorRegistry cacheInvalidatorRegistry = new CacheInvalidatorRegistry();
cacheInvalidatorRegistry.registerAlias("service", Set.of("file_service_account_token", "index_service_account_token"));
components.add(cacheInvalidatorRegistry);
Expand Down Expand Up @@ -612,6 +619,21 @@ auditTrailService, failureHandler, threadPool, anonymousUser, getAuthorizationEn
return components;
}

protected void possiblySetElasticPassword(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) {
if (previousState.equals(SecurityIndexManager.State.UNRECOVERED_STATE)
Copy link
Member Author

Choose a reason for hiding this comment

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

wait until the state of the security index has been recovered from either the disk or a cluster state update.

&& currentState.equals(SecurityIndexManager.State.UNRECOVERED_STATE) == false
&& securityIndex.get().indexExists() == false
Copy link
Member Author

Choose a reason for hiding this comment

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

what should happen if security index gets deleted ? Would bootstrapping this again to set value be valuable/surprising/wrong?

Copy link
Member Author

@jkakavas jkakavas Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The idea around securityIndex.get().indexExists() == false is that we should be only setting this as the password of the elastic user, the first time the node starts after a package installation and only if the password of the elastic user hasn't been already set ( which would have triggered the auto-creation of the security index ) Since we are not triggering the creation of the security index during package installation and this would run early enough in the node lifecycle, there should be nothing unrelated that has auto-created the security index.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the goal here is to have the elastic user's password hash in the .security index as soon as the index is created.
For this I think it is better to insert this action in the SecurityIndexManager#prepareIndexIfNeededThenExecute, right before calling the execute runnable argument.
Doing it in the action listener adds a level of ambiguity about when the password hash is actually going to take effect, i.e. the keystore seed is a valid password before the index is created, and also after the index is created but before the change password is executed (because cluster change listeners are executed on a different thread, and because the change password action is also "enqued" to be executed).

Copy link
Member Author

Choose a reason for hiding this comment

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

For this I think it is better to insert this action in the SecurityIndexManager#prepareIndexIfNeededThenExecute, right before calling the execute runnable argument.

This is part of the alternative approach you are discussing in the other comment , right ?

&& elasticPasswordHash.get() != null) {
final ChangePasswordRequest request = new ChangePasswordRequest();
request.username("elastic");
request.passwordHash(elasticPasswordHash.get().getChars());
nativeUsersStore.get().changePassword(request, ActionListener.wrap(
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails the first time we're in trouble, the "promised" password is never going to be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, because something else might have created the security index until the second time this node starts.. We could in theory extend the check to be "security index doesn't exist or the doc for the elastic user doesn't exist"

r -> {},
e -> logger.warn("failed to set the elastic user password from the value of [" + ELASTIC_PASSWORD_HASH.getKey() + "]")));
elasticPasswordHash.get().close();
}
}

private AuthorizationEngine getAuthorizationEngine() {
AuthorizationEngine authorizationEngine = null;
String extensionName = null;
Expand Down
15 changes: 15 additions & 0 deletions x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.network.NetworkModule;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsModule;
Expand Down Expand Up @@ -93,6 +94,7 @@

import static java.util.Collections.emptyMap;
import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_FORMAT_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.ELASTIC_PASSWORD_HASH;
import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.INTERNAL_MAIN_INDEX_FORMAT;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -651,6 +653,19 @@ public void testSecurityStatusMessageInLog() throws Exception{
}
}

public void testNoElasticPasswordHashInKeystore() throws Exception {
createComponents(Settings.EMPTY);
assertNull(security.getElasticPasswordHash());
}

public void testElasticPasswordHashInKeystore() throws Exception {
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString(ELASTIC_PASSWORD_HASH.getKey(), "some_hash_here");
Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
createComponents(settings);
assertThat(security.getElasticPasswordHash().toString(), equalTo("some_hash_here"));
}

private void logAndFail(Exception e) {
logger.error("unexpected exception", e);
fail("unexpected exception " + e.getMessage());
Expand Down