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

Enforce transport TLS on Basic with Security #42150

Merged
merged 4 commits into from
May 15, 2019
Merged

Enforce transport TLS on Basic with Security #42150

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d25bcb
Enforce transport TLS on Basic with Security
tvernum May 15, 2019
13b4f71
Improve message and tests in TLS join validator
tvernum May 15, 2019
de32e3e
Do not use default fallthrough
jasontedor May 15, 2019
9ce8d54
Update test to avoid unreachable statement
jasontedor May 15, 2019
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
18 changes: 0 additions & 18 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/License.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,22 +780,4 @@ public Builder validate() {
}
}

/**
* Returns <code>true</code> iff the license is a production licnese
*/
public boolean isProductionLicense() {
Copy link
Contributor Author

@tvernum tvernum May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I replaced this with an explicit "is TLS required" method in XPackLicenseState.

I droppped the "production license" terminology, because it was a bit confusing (which is why this gap has only just been identified).
Why wasn't basic a production license before? Should it be one? It's perfectly fine to run basic in production, even though it doesn't have paid support.

I moved it from License to XPackLicenseState because

  1. All the "is XXX allowed" methods are there, so that seemed more reasonable from an appropriate compartmentalisation problem (and helps with discoverability of the code)
  2. To do the "is TLS required" method well it needs to look at whether security is explicitly enabled, for which all the logic is in XPackLicenseState.

switch (operationMode()) {
case MISSING:
case TRIAL:
case BASIC:
return false;
case STANDARD:
case GOLD:
case PLATINUM:
return true;
default:
throw new AssertionError("unknown operation mode: " + operationMode());

}
}
}
5 changes: 4 additions & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/license/LicenseService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,13 @@ public void registerLicense(final PutLicenseRequest request, final ActionListene
}
}

// This check would be incorrect if "basic" licenses were allowed here
// because the defaults there mean that security can be "off", even if the setting is "on"
// BUT basic licenses are explicitly excluded earlier in this method, so we don't need to worry
if (XPackSettings.SECURITY_ENABLED.get(settings)) {
// TODO we should really validate that all nodes have xpack installed and are consistently configured but this
// should happen on a different level and not in this code
if (newLicense.isProductionLicense()
if (XPackLicenseState.isTransportTlsRequired(newLicense, settings)
&& XPackSettings.TRANSPORT_SSL_ENABLED.get(settings) == false
&& isProductionMode(settings, clusterService.localNode())) {
// security is on but TLS is not configured we gonna fail the entire request and throw an exception
Expand Down
24 changes: 23 additions & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ private static class Status {
public XPackLicenseState(Settings settings) {
this.listeners = new CopyOnWriteArrayList<>();
this.isSecurityEnabled = XPackSettings.SECURITY_ENABLED.get(settings);
this.isSecurityExplicitlyEnabled = isSecurityEnabled && settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey());
this.isSecurityExplicitlyEnabled = isSecurityEnabled && isSecurityExplicitlyEnabled(settings);
}

private XPackLicenseState(XPackLicenseState xPackLicenseState) {
Expand All @@ -292,6 +292,10 @@ private XPackLicenseState(XPackLicenseState xPackLicenseState) {
this.status = xPackLicenseState.status;
}

private static boolean isSecurityExplicitlyEnabled(Settings settings) {
return settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey());
}

/**
* Updates the current state of the license, which will change what features are available.
*
Expand Down Expand Up @@ -727,6 +731,24 @@ public synchronized boolean isSecurityDisabledByLicenseDefaults() {
return false;
}

public static boolean isTransportTlsRequired(License license, Settings settings) {
if (license == null) {
return false;
}
switch (license.operationMode()) {
case STANDARD:
case GOLD:
case PLATINUM:
return XPackSettings.SECURITY_ENABLED.get(settings);
case BASIC:
return XPackSettings.SECURITY_ENABLED.get(settings) && isSecurityExplicitlyEnabled(settings);
case MISSING:
case TRIAL:
default:
return false;
}
}

private static boolean isSecurityEnabled(final OperationMode mode, final boolean isSecurityExplicitlyEnabled,
final boolean isSecurityEnabled) {
switch (mode) {
Expand Down
10 changes: 6 additions & 4 deletions .../plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/TLSLicenseBootstrapCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.bootstrap.BootstrapContext;
import org.elasticsearch.license.License;
import org.elasticsearch.license.LicenseService;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.xpack.core.XPackSettings;

/**
Expand All @@ -19,10 +20,11 @@ public final class TLSLicenseBootstrapCheck implements BootstrapCheck {
public BootstrapCheckResult check(BootstrapContext context) {
if (XPackSettings.TRANSPORT_SSL_ENABLED.get(context.settings()) == false) {
License license = LicenseService.getLicense(context.metaData());
if (license != null && license.isProductionLicense()) {
return BootstrapCheckResult.failure("Transport SSL must be enabled for setups with production licenses. Please set " +
"[xpack.security.transport.ssl.enabled] to [true] or disable security by setting [xpack.security.enabled] " +
"to [false]");
if (XPackLicenseState.isTransportTlsRequired(license, context.settings())) {
return BootstrapCheckResult.failure("Transport SSL must be enabled if security is enabled on a [" +
license.operationMode().description() + "] license. " +
"Please set [xpack.security.transport.ssl.enabled] to [true] or disable security by setting " +
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it also be enough to not explicitly set xpack.security.enabled to true when in basic. I don't think it's worth trying to fit in the message cause I can't think of a way to not make the wording too confusing, but just raising in case you have any ideas around it

"[xpack.security.enabled] to [false]");
}
}
return BootstrapCheckResult.success();
Expand Down
123 changes: 99 additions & 24 deletions ...in/core/src/test/java/org/elasticsearch/xpack/core/ssl/TLSLicenseBootstrapCheckTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,115 @@
*/
package org.elasticsearch.xpack.core.ssl;

import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.bootstrap.BootstrapContext;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.license.License;
import org.elasticsearch.license.License.OperationMode;
import org.elasticsearch.license.TestUtils;
import org.elasticsearch.test.AbstractBootstrapCheckTestCase;

import java.util.EnumSet;

public class TLSLicenseBootstrapCheckTests extends AbstractBootstrapCheckTestCase {
public void testBootstrapCheck() throws Exception {
public void testBootstrapCheckOnEmptyMetadata() {
assertTrue(new TLSLicenseBootstrapCheck().check(emptyContext).isSuccess());
assertTrue(new TLSLicenseBootstrapCheck().check(createTestContext(Settings.builder().put("xpack.security.transport.ssl.enabled"
, randomBoolean()).build(), MetaData.EMPTY_META_DATA)).isSuccess());
int numIters = randomIntBetween(1,10);
for (int i = 0; i < numIters; i++) {
License license = TestUtils.generateSignedLicense(TimeValue.timeValueHours(24));
EnumSet<License.OperationMode> productionModes = EnumSet.of(License.OperationMode.GOLD, License.OperationMode.PLATINUM,
License.OperationMode.STANDARD);
MetaData.Builder builder = MetaData.builder();
TestUtils.putLicense(builder, license);
MetaData build = builder.build();
if (productionModes.contains(license.operationMode()) == false) {
assertTrue(new TLSLicenseBootstrapCheck().check(createTestContext(
Settings.builder().put("xpack.security.transport.ssl.enabled", true).build(), build)).isSuccess());
} else {
assertTrue(new TLSLicenseBootstrapCheck().check(createTestContext(
Settings.builder().put("xpack.security.transport.ssl.enabled", false).build(), build)).isFailure());
assertEquals("Transport SSL must be enabled for setups with production licenses. Please set " +
"[xpack.security.transport.ssl.enabled] to [true] or disable security by setting " +
"[xpack.security.enabled] to [false]",
new TLSLicenseBootstrapCheck().check(createTestContext(
Settings.builder().put("xpack.security.transport.ssl.enabled", false).build(), build)).getMessage());
}
, randomBoolean()).build(), MetaData.EMPTY_META_DATA)).isSuccess());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old method felt very much like it was using randomness to test a variety of scenarios, so I took the opportunity to split it out.


public void testBootstrapCheckFailureOnPremiumLicense() throws Exception {
final OperationMode mode = randomFrom(OperationMode.PLATINUM, OperationMode.GOLD, OperationMode.STANDARD);
final Settings.Builder settings = Settings.builder();
if (randomBoolean()) {
// randomise between default-false & explicit-false
settings.put("xpack.security.transport.ssl.enabled", false);
}
if (randomBoolean()) {
// randomise between default-true & explicit-true
settings.put("xpack.security.enabled", true);
}

final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(mode, settings);
assertTrue("Expected bootstrap failure", result.isFailure());
assertEquals("Transport SSL must be enabled if security is enabled on a [" + mode.description() + "] license. Please set " +
"[xpack.security.transport.ssl.enabled] to [true] or disable security by setting " +
"[xpack.security.enabled] to [false]",
result.getMessage());
}

public void testBootstrapCheckSucceedsWithTlsEnabledOnPremiumLicense() throws Exception {
final OperationMode mode = randomFrom(OperationMode.PLATINUM, OperationMode.GOLD, OperationMode.STANDARD);
final Settings.Builder settings = Settings.builder().put("xpack.security.transport.ssl.enabled", true);
final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(mode, settings);
assertSuccess(result);
}

public void testBootstrapCheckFailureOnBasicLicense() throws Exception {
final Settings.Builder settings = Settings.builder().put("xpack.security.enabled", true);
if (randomBoolean()) {
// randomise between default-false & explicit-false
settings.put("xpack.security.transport.ssl.enabled", false);
}
final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(OperationMode.BASIC, settings);
assertTrue("Expected bootstrap failure", result.isFailure());
assertEquals("Transport SSL must be enabled if security is enabled on a [basic] license. Please set " +
"[xpack.security.transport.ssl.enabled] to [true] or disable security by setting " +
"[xpack.security.enabled] to [false]",
result.getMessage());
}

public void testBootstrapSucceedsIfSecurityIsNotEnabledOnBasicLicense() throws Exception {
final Settings.Builder settings = Settings.builder();
if (randomBoolean()) {
// randomise between default-false & explicit-false
settings.put("xpack.security.enabled", false);
}
if (randomBoolean()) {
// it does not matter whether or not this is set, as security is not enabled.
settings.put("xpack.security.transport.ssl.enabled", randomBoolean());
}
final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(OperationMode.BASIC, settings);
assertSuccess(result);
}

public void testBootstrapSucceedsIfTlsIsEnabledOnBasicLicense() throws Exception {
final Settings.Builder settings = Settings.builder().put("xpack.security.transport.ssl.enabled", true);
if (randomBoolean()) {
// it does not matter whether or not this is set, as TLS is enabled.
settings.put("xpack.security.enabled", randomBoolean());
}
final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(OperationMode.BASIC, settings);
assertSuccess(result);
}

public void testBootstrapCheckAlwaysSucceedsOnTrialLicense() throws Exception {
final Settings.Builder settings = Settings.builder();
if (randomBoolean()) {
// it does not matter whether this is set, or to which value.
settings.put("xpack.security.enabled", randomBoolean());
}
if (randomBoolean()) {
// it does not matter whether this is set, or to which value.
settings.put("xpack.security.transport.ssl.enabled", randomBoolean());
}
final BootstrapCheck.BootstrapCheckResult result = runBootstrapCheck(OperationMode.TRIAL, settings);
assertSuccess(result);
}

public BootstrapCheck.BootstrapCheckResult runBootstrapCheck(OperationMode mode, Settings.Builder settings) throws Exception {
final License license = TestUtils.generateSignedLicense(mode.description(), TimeValue.timeValueHours(24));
MetaData.Builder builder = MetaData.builder();
TestUtils.putLicense(builder, license);
MetaData metaData = builder.build();
final BootstrapContext context = createTestContext(settings.build(), metaData);
return new TLSLicenseBootstrapCheck().check(context);
}

public void assertSuccess(BootstrapCheck.BootstrapCheckResult result) {
if (result.isFailure()) {
fail("Bootstrap check failed unexpectedly: " + result.getMessage());
}
}

}
15 changes: 9 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 @@ -258,8 +258,8 @@
import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_FORMAT_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.API_KEY_SERVICE_ENABLED_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.HTTP_SSL_ENABLED;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.INTERNAL_MAIN_INDEX_FORMAT;
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.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_MAIN_TEMPLATE_7;

public class Security extends Plugin implements ActionPlugin, IngestPlugin, NetworkPlugin, ClusterPlugin,
Expand Down Expand Up @@ -1002,7 +1002,7 @@ public Function<String, Predicate<String>> getFieldFilter() {
public BiConsumer<DiscoveryNode, ClusterState> getJoinValidator() {
if (enabled) {
return new ValidateTLSOnJoin(XPackSettings.TRANSPORT_SSL_ENABLED.get(settings),
DiscoveryModule.DISCOVERY_TYPE_SETTING.get(settings))
DiscoveryModule.DISCOVERY_TYPE_SETTING.get(settings), settings)
.andThen(new ValidateUpgradedSecurityIndex())
.andThen(new ValidateLicenseForFIPS(XPackSettings.FIPS_MODE_ENABLED.get(settings)));
}
Expand All @@ -1012,18 +1012,21 @@ public BiConsumer<DiscoveryNode, ClusterState> getJoinValidator() {
static final class ValidateTLSOnJoin implements BiConsumer<DiscoveryNode, ClusterState> {
private final boolean isTLSEnabled;
private final String discoveryType;
private final Settings settings;

ValidateTLSOnJoin(boolean isTLSEnabled, String discoveryType) {
ValidateTLSOnJoin(boolean isTLSEnabled, String discoveryType, Settings settings) {
this.isTLSEnabled = isTLSEnabled;
this.discoveryType = discoveryType;
this.settings = settings;
}

@Override
public void accept(DiscoveryNode node, ClusterState state) {
License license = LicenseService.getLicense(state.metaData());
if (license != null && license.isProductionLicense() &&
isTLSEnabled == false && "single-node".equals(discoveryType) == false) {
throw new IllegalStateException("TLS setup is required for license type [" + license.operationMode().name() + "]");
if (isTLSEnabled == false && "single-node".equals(discoveryType) == false
&& XPackLicenseState.isTransportTlsRequired(license, settings)) {
throw new IllegalStateException("Transport TLS ([" + XPackSettings.TRANSPORT_SSL_ENABLED.getKey() +
"]) is required for license type [" + license.operationMode().description() + "] when security is enabled");
}
}
}
Expand Down
44 changes: 36 additions & 8 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 @@ -54,7 +54,6 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -67,8 +66,8 @@

import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_FORMAT_SETTING;
import static org.elasticsearch.discovery.DiscoveryModule.ZEN2_DISCOVERY_TYPE;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.INTERNAL_MAIN_INDEX_FORMAT;
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;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -253,17 +252,46 @@ public void testTLSJoinValidator() throws Exception {
int numIters = randomIntBetween(1, 10);
for (int i = 0; i < numIters; i++) {
boolean tlsOn = randomBoolean();
boolean securityExplicitlyEnabled = randomBoolean();
String discoveryType = randomFrom("single-node", ZEN2_DISCOVERY_TYPE, randomAlphaOfLength(4));
Security.ValidateTLSOnJoin validator = new Security.ValidateTLSOnJoin(tlsOn, discoveryType);

final Settings settings;
if (securityExplicitlyEnabled) {
settings = Settings.builder().put("xpack.security.enabled", true).build();
} else {
settings = Settings.EMPTY;
}
Security.ValidateTLSOnJoin validator = new Security.ValidateTLSOnJoin(tlsOn, discoveryType, settings);
MetaData.Builder builder = MetaData.builder();
License license = TestUtils.generateSignedLicense(TimeValue.timeValueHours(24));
License.OperationMode licenseMode = randomFrom(License.OperationMode.values());
License license = TestUtils.generateSignedLicense(licenseMode.description(), TimeValue.timeValueHours(24));
TestUtils.putLicense(builder, license);
ClusterState state = ClusterState.builder(ClusterName.DEFAULT).metaData(builder.build()).build();
EnumSet<License.OperationMode> productionModes = EnumSet.of(License.OperationMode.GOLD, License.OperationMode.PLATINUM,
License.OperationMode.STANDARD);
if (productionModes.contains(license.operationMode()) && tlsOn == false && "single-node".equals(discoveryType) == false) {

final boolean expectFailure;
switch (licenseMode) {
case PLATINUM:
case GOLD:
case STANDARD:
expectFailure = tlsOn == false && "single-node".equals(discoveryType) == false;
break;
case BASIC:
expectFailure = tlsOn == false && "single-node".equals(discoveryType) == false && securityExplicitlyEnabled;
break;
case MISSING:
case TRIAL:
expectFailure = false;
break;
default:
fail("Unexpected license mode: " + licenseMode);
expectFailure = true;
}
logger.info("Test TLS join; Lic:{} TLS:{} Disco:{} Settings:{} ; Expect Failure: {}",
licenseMode, tlsOn, discoveryType, settings.toDelimitedString(','), expectFailure);
if (expectFailure) {
IllegalStateException ise = expectThrows(IllegalStateException.class, () -> validator.accept(node, state));
assertEquals("TLS setup is required for license type [" + license.operationMode().name() + "]", ise.getMessage());
assertEquals("Transport TLS ([xpack.security.transport.ssl.enabled]) is required for license type ["
+ license.operationMode().description() + "] when security is enabled", ise.getMessage());
} else {
validator.accept(node, state);
}
Expand Down