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.

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

Startup check for security implicit behavior change #76879

Merged
merged 20 commits into from
Oct 25, 2021
Merged
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 @@ -947,6 +947,13 @@ public String nodeId() {
return nodeMetadata.nodeId();
}

/**
* Returns the loaded NodeMetadata for this node
*/
public NodeMetadata nodeMetadata() {
return nodeMetadata;
}

/**
* Returns an array of all of the {@link NodePath}s.
*/
Expand Down
47 changes: 36 additions & 11 deletions server/src/main/java/org/elasticsearch/env/NodeMetadata.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,37 @@ public final class NodeMetadata {

private final Version nodeVersion;

public NodeMetadata(final String nodeId, final Version nodeVersion) {
private final Version previousNodeVersion;

private NodeMetadata(final String nodeId, final Version nodeVersion, final Version previousNodeVersion) {
this.nodeId = Objects.requireNonNull(nodeId);
this.nodeVersion = Objects.requireNonNull(nodeVersion);
this.previousNodeVersion = Objects.requireNonNull(previousNodeVersion);
}

@Override
public boolean equals(Object o) {
public NodeMetadata(final String nodeId, final Version nodeVersion) {
this(nodeId, nodeVersion, nodeVersion);
}

@Override public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
NodeMetadata that = (NodeMetadata) o;
return nodeId.equals(that.nodeId) &&
nodeVersion.equals(that.nodeVersion);
return nodeId.equals(that.nodeId) && nodeVersion.equals(that.nodeVersion) && Objects.equals(
previousNodeVersion,
that.previousNodeVersion);
}

@Override
public int hashCode() {
return Objects.hash(nodeId, nodeVersion);
@Override public int hashCode() {
return Objects.hash(nodeId, nodeVersion, previousNodeVersion);
}

@Override
public String toString() {
return "NodeMetadata{" +
"nodeId='" + nodeId + '\'' +
", nodeVersion=" + nodeVersion +
", previousNodeVersion=" + previousNodeVersion +
'}';
}

Expand All @@ -68,10 +75,20 @@ public Version nodeVersion() {
return nodeVersion;
}

/**
* When a node starts we read the existing node metadata from disk (see NodeEnvironment@loadNodeMetadata), store a reference to the
* node version that we read from there in {@code previousNodeVersion} and then proceed to upgrade the version to
* the current version of the node ({@link NodeMetadata#upgradeToCurrentVersion()} before storing the node metadata again on disk.
* In doing so, {@code previousNodeVersion} refers to the previously last known version that this node was started on.
*/
public Version previousNodeVersion() {
return previousNodeVersion;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I'm fighting against the existing conventions of this class, but is it possible to get some sort of javadoc here?
What does previous mean exactly? I think it's "last time the node started" (or more accurately "the version of the metadata that was read from disk") ... but I'm sure there could be all sorts of nuace in rolling upgrades, master elections, etc, and I'd like to be able to consult javadocs so I can know how to reason about that.

Copy link
Member Author

@jkakavas jkakavas Oct 25, 2021

Choose a reason for hiding this comment

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

added in 80f893f, @DaveCTurner can keep me honest or suggest enhancements

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs LGTM 👍


public NodeMetadata upgradeToCurrentVersion() {
if (nodeVersion.equals(Version.V_EMPTY)) {
assert Version.CURRENT.major <= Version.V_7_0_0.major + 1 : "version is required in the node metadata from v9 onwards";
return new NodeMetadata(nodeId, Version.CURRENT);
return new NodeMetadata(nodeId, Version.CURRENT, Version.V_EMPTY);
}

if (nodeVersion.before(Version.CURRENT.minimumIndexCompatibilityVersion())) {
Expand All @@ -84,12 +101,13 @@ public NodeMetadata upgradeToCurrentVersion() {
"cannot downgrade a node from version [" + nodeVersion + "] to version [" + Version.CURRENT + "]");
}

return nodeVersion.equals(Version.CURRENT) ? this : new NodeMetadata(nodeId, Version.CURRENT);
return nodeVersion.equals(Version.CURRENT) ? this : new NodeMetadata(nodeId, Version.CURRENT, nodeVersion);
}

private static class Builder {
String nodeId;
Version nodeVersion;
Version previousNodeVersion;

public void setNodeId(String nodeId) {
this.nodeId = nodeId;
Expand All @@ -99,6 +117,10 @@ public void setNodeVersionId(int nodeVersionId) {
this.nodeVersion = Version.fromId(nodeVersionId);
}

public void setPreviousNodeVersionId(int previousNodeVersionId) {
this.previousNodeVersion = Version.fromId(previousNodeVersionId);
}

public NodeMetadata build() {
final Version nodeVersion;
if (this.nodeVersion == null) {
Expand All @@ -107,8 +129,11 @@ public NodeMetadata build() {
} else {
nodeVersion = this.nodeVersion;
}
if (this.previousNodeVersion == null) {
previousNodeVersion = nodeVersion;
}

return new NodeMetadata(nodeId, nodeVersion);
return new NodeMetadata(nodeId, nodeVersion, previousNodeVersion);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ public void testDoesNotUpgradeAncientVersion() {
allOf(startsWith("cannot upgrade a node from version ["), endsWith("] directly to version [" + Version.CURRENT + "]")));
}

public void testUpgradeMarksPreviousVersion() {
final String nodeId = randomAlphaOfLength(10);
final Version version = VersionUtils.randomVersionBetween(random(), Version.V_7_3_0, Version.V_7_16_0);
final NodeMetadata nodeMetadata = new NodeMetadata(nodeId, version).upgradeToCurrentVersion();
assertThat(nodeMetadata.nodeVersion(), equalTo(Version.CURRENT));
assertThat(nodeMetadata.previousNodeVersion(), equalTo(version));
}

public static Version tooNewVersion() {
return Version.fromId(between(Version.CURRENT.id + 1, 99999999));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class LicensesMetadata extends AbstractNamedDiffable<Metadata.Custom> imp
@Nullable
private Version trialVersion;

LicensesMetadata(License license, Version trialVersion) {
public LicensesMetadata(License license, Version trialVersion) {
this.license = license;
this.trialVersion = trialVersion;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.env.NodeMetadata;
import org.elasticsearch.http.HttpServerTransport;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.indices.ExecutorNames;
Expand Down Expand Up @@ -432,9 +433,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 Down Expand Up @@ -465,7 +463,7 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
Supplier<RepositoriesService> repositoriesServiceSupplier) {
try {
return createComponents(client, threadPool, clusterService, resourceWatcherService, scriptService, xContentRegistry,
environment, expressionResolver);
environment, nodeEnvironment.nodeMetadata(), expressionResolver);
} catch (final Exception e) {
throw new IllegalStateException("security initialization failed", e);
}
Expand All @@ -474,7 +472,7 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
// pkg private for testing - tests want to pass in their set of extensions hence we are not using the extension service directly
Collection<Object> createComponents(Client client, ThreadPool threadPool, ClusterService clusterService,
ResourceWatcherService resourceWatcherService, ScriptService scriptService,
NamedXContentRegistry xContentRegistry, Environment environment,
NamedXContentRegistry xContentRegistry, Environment environment, NodeMetadata nodeMetadata,
IndexNameExpressionResolver expressionResolver) throws Exception {
logger.info("Security is {}", enabled ? "enabled" : "disabled");
if (enabled == false) {
Expand All @@ -488,6 +486,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
checks.addAll(Arrays.asList(
new TokenSSLBootstrapCheck(),
new PkiRealmBootstrapCheck(getSslService()),
new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata),
new TransportTLSBootstrapCheck()));
checks.addAll(InternalRealms.getBootstrapChecks(settings, environment));
this.bootstrapChecks.set(Collections.unmodifiableList(checks));
Expand Down Expand Up @@ -522,7 +521,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste

// realms construction
final NativeUsersStore nativeUsersStore = new NativeUsersStore(settings, client, securityIndex.get());

final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(settings, client, securityIndex.get(),
scriptService);
final AnonymousUser anonymousUser = new AnonymousUser(settings);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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;

import org.elasticsearch.Version;
import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.bootstrap.BootstrapContext;
import org.elasticsearch.env.NodeMetadata;
import org.elasticsearch.license.License;
import org.elasticsearch.license.LicenseService;
import org.elasticsearch.xpack.core.XPackSettings;

public class SecurityImplicitBehaviorBootstrapCheck implements BootstrapCheck {

private final NodeMetadata nodeMetadata;

public SecurityImplicitBehaviorBootstrapCheck(NodeMetadata nodeMetadata) {
this.nodeMetadata = nodeMetadata;
}

@Override
public BootstrapCheckResult check(BootstrapContext context) {
if (nodeMetadata == null) {
return BootstrapCheckResult.success();
}
final License license = LicenseService.getLicense(context.metadata());
final Version lastKnownVersion = nodeMetadata.previousNodeVersion();
// pre v7.2.0 nodes have Version.EMPTY and its id is 0, so Version#before handles this successfully
if (lastKnownVersion.before(Version.V_8_0_0)
&& XPackSettings.SECURITY_ENABLED.exists(context.settings()) == false
&& (license.operationMode() == License.OperationMode.BASIC || license.operationMode() == License.OperationMode.TRIAL)) {
return BootstrapCheckResult.failure(
"The default value for ["
+ XPackSettings.SECURITY_ENABLED.getKey()
+ "] has changed in the current version. "
+ " Security features were implicitly disabled for this node but they would now be enabled, possibly"
+ " preventing access to the node. "
+ "See https://www.elastic.co/guide/en/elasticsearch/reference/"
+ Version.CURRENT.major
+ "."
+ Version.CURRENT.minor
+ "/security-minimal-setup.html to configure security, or explicitly disable security by "
+ "setting [xpack.security.enabled] to \"false\" in elasticsearch.yml before restarting the node."
);
} else {
return BootstrapCheckResult.success();
}
}

public boolean alwaysEnforce() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't love this always enabled Bootstrap check, but this is currently the only way for us to make a check on node startup that has a view ( albeit limited ) to the restored cluster state ( via the BootstrapContext )

Copy link
Contributor

@DaveCTurner DaveCTurner Oct 25, 2021

Choose a reason for hiding this comment

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

I'd forgotten that we expose the metadata read from disk like this, but I think this is fine - at least it's no worse than any of the other places that make decisions based on the contents of the on-disk cluster state despite the fact that this could be stale or even uncommitted.

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* 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;

import org.elasticsearch.Version;
import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.env.NodeMetadata;
import org.elasticsearch.license.License;
import org.elasticsearch.license.LicensesMetadata;
import org.elasticsearch.license.TestUtils;
import org.elasticsearch.test.AbstractBootstrapCheckTestCase;
import org.elasticsearch.test.VersionUtils;
import org.elasticsearch.xpack.core.XPackSettings;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;

public class SecurityImplicitBehaviorBootstrapCheckTests extends AbstractBootstrapCheckTestCase {

public void testFailureUpgradeFrom7xWithImplicitSecuritySettings() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need 2 methods:

  • testFailureUpgradeFrom7xWithImplicitSecuritySettingsOnTrialOrBasic
  • testSuccessfulUpgradeFrom7xWithImplicitSecuritySettingsOnGoldPlus

The 2nd one seems to be missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, will add now

Copy link
Member Author

Choose a reason for hiding this comment

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

added in 2acbd38

final Version previousVersion = VersionUtils.randomVersionBetween(random(), Version.V_7_2_0, Version.V_7_16_0);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(10), previousVersion);
nodeMetadata = nodeMetadata.upgradeToCurrentVersion();
BootstrapCheck.BootstrapCheckResult result = new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata).check(
createTestContext(Settings.EMPTY, createLicensesMetadata(previousVersion, randomFrom("basic", "trial")))
);
assertThat(result.isFailure(), is(true));
assertThat(
result.getMessage(),
equalTo(
"The default value for ["
+ XPackSettings.SECURITY_ENABLED.getKey()
+ "] has changed in the current version. "
+ " Security features were implicitly disabled for this node but they would now be enabled, possibly"
+ " preventing access to the node. "
+ "See https://www.elastic.co/guide/en/elasticsearch/reference/"
+ Version.CURRENT.major
+ "."
+ Version.CURRENT.minor
+ "/security-minimal-setup.html to configure security, or explicitly disable security by "
+ "setting [xpack.security.enabled] to \"false\" in elasticsearch.yml before restarting the node."
)
);
}

public void testUpgradeFrom7xWithImplicitSecuritySettingsOnGoldPlus() throws Exception {
final Version previousVersion = VersionUtils.randomVersionBetween(random(), Version.V_7_2_0, Version.V_7_16_0);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(10), previousVersion);
nodeMetadata = nodeMetadata.upgradeToCurrentVersion();
BootstrapCheck.BootstrapCheckResult result = new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata).check(
createTestContext(Settings.EMPTY, createLicensesMetadata(previousVersion, randomFrom("gold", "platinum")))
);
assertThat(result.isSuccess(), is(true));
}

public void testUpgradeFrom7xWithExplicitSecuritySettings() throws Exception {
final Version previousVersion = VersionUtils.randomVersionBetween(random(), Version.V_7_2_0, Version.V_7_16_0);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(10), previousVersion);
nodeMetadata = nodeMetadata.upgradeToCurrentVersion();
BootstrapCheck.BootstrapCheckResult result = new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata).check(
createTestContext(
Settings.builder().put(XPackSettings.SECURITY_ENABLED.getKey(), true).build(),
createLicensesMetadata(previousVersion, randomFrom("basic", "trial"))
)
);
assertThat(result.isSuccess(), is(true));
}

public void testUpgradeFrom8xWithImplicitSecuritySettings() throws Exception {
final Version previousVersion = VersionUtils.randomVersionBetween(random(), Version.V_8_0_0, null);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(10), previousVersion);
nodeMetadata = nodeMetadata.upgradeToCurrentVersion();
BootstrapCheck.BootstrapCheckResult result = new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata).check(
createTestContext(Settings.EMPTY, createLicensesMetadata(previousVersion, randomFrom("basic", "trial")))
);
assertThat(result.isSuccess(), is(true));
}

public void testUpgradeFrom8xWithExplicitSecuritySettings() throws Exception {
final Version previousVersion = VersionUtils.randomVersionBetween(random(), Version.V_8_0_0, null);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(10), previousVersion);
nodeMetadata = nodeMetadata.upgradeToCurrentVersion();
BootstrapCheck.BootstrapCheckResult result = new SecurityImplicitBehaviorBootstrapCheck(nodeMetadata).check(
createTestContext(
Settings.builder().put(XPackSettings.SECURITY_ENABLED.getKey(), true).build(),
createLicensesMetadata(previousVersion, randomFrom("basic", "trial"))
)
);
assertThat(result.isSuccess(), is(true));
}

private Metadata createLicensesMetadata(Version version, String licenseMode) throws Exception {
License license = TestUtils.generateSignedLicense(licenseMode, TimeValue.timeValueHours(2));
return Metadata.builder().putCustom(LicensesMetadata.TYPE, new LicensesMetadata(license, version)).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeMetadata;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.IndexSettings;
Expand Down Expand Up @@ -128,6 +129,7 @@ public Map<String, Realm.Factory> getRealms(SecurityComponents components) {

private Collection<Object> createComponentsUtil(Settings settings, SecurityExtension... extensions) throws Exception {
Environment env = TestEnvironment.newEnvironment(settings);
NodeMetadata nodeMetadata = new NodeMetadata(randomAlphaOfLength(8), Version.CURRENT);
licenseState = new TestUtils.UpdatableLicenseState(settings);
SSLService sslService = new SSLService(env);
security = new Security(settings, null, Arrays.asList(extensions)) {
Expand Down Expand Up @@ -155,7 +157,7 @@ protected SSLService getSslService() {
when(client.threadPool()).thenReturn(threadPool);
when(client.settings()).thenReturn(settings);
return security.createComponents(client, threadPool, clusterService, mock(ResourceWatcherService.class), mock(ScriptService.class),
xContentRegistry(), env, TestIndexNameExpressionResolver.newInstance(threadContext));
xContentRegistry(), env, nodeMetadata, TestIndexNameExpressionResolver.newInstance(threadContext));
}

private Collection<Object> createComponents(Settings testSettings, SecurityExtension... extensions) throws Exception {
Expand Down