Skip to content

Commit

Permalink
Security: fix joining cluster with production license
Browse files Browse the repository at this point in the history 
The changes made to disable security for trial licenses unless security
is explicitly enabled caused issues when a 6.3 node attempts to join a
cluster that already has a production license installed. The new node
starts off with a trial licenses and `xpack.security.enabled` is not
set for the node, which causes the security code to skip attaching the
user to the request. The existing cluster has security enabled and the
lack of a user attached to the requests causes the request to be
rejected.

This commit changes the security code to check if the state has been
recovered yet when making the decision on whether or not to attach a
user. If the state has not yet been recovered, the code will attach
the user to the request in case security is enabled on the cluster
being joined.

Closes elastic#31332
  • Loading branch information
@jaymode
jaymode committed Jun 14, 2018
1 parent f7a0caf commit 1d3f935
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 21 deletions.
3 changes: 2 additions & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,8 @@ private static class Status {

public XPackLicenseState(Settings settings) {
this.isSecurityEnabled = XPackSettings.SECURITY_ENABLED.get(settings);
this.isSecurityExplicitlyEnabled = settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) && isSecurityEnabled;
this.isSecurityExplicitlyEnabled = (isSecurityEnabled && settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey())) ||
(settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false && XPackSettings.TRANSPORT_SSL_ENABLED.get(settings));
}

/** Updates the current state of the license, which will change what features are available. */
Expand Down
10 changes: 10 additions & 0 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/license/XPackLicenseStateTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,16 @@ public void testSecurityDefaults() {
assertThat(licenseState.allowedRealmType(), is(XPackLicenseState.AllowedRealmType.ALL));
assertThat(licenseState.isCustomRoleProvidersAllowed(), is(true));

licenseState =
new XPackLicenseState(Settings.builder().put(XPackSettings.TRANSPORT_SSL_ENABLED.getKey(), true).build());
assertThat(licenseState.isAuthAllowed(), is(true));
assertThat(licenseState.isIpFilteringAllowed(), is(true));
assertThat(licenseState.isAuditingAllowed(), is(true));
assertThat(licenseState.isStatsAndHealthAllowed(), is(true));
assertThat(licenseState.isDocumentAndFieldLevelSecurityAllowed(), is(true));
assertThat(licenseState.allowedRealmType(), is(XPackLicenseState.AllowedRealmType.ALL));
assertThat(licenseState.isCustomRoleProvidersAllowed(), is(true));

licenseState = new XPackLicenseState(Settings.EMPTY);
assertThat(licenseState.isAuthAllowed(), is(true));
assertThat(licenseState.isIpFilteringAllowed(), is(true));
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(ipFilter.get());
DestructiveOperations destructiveOperations = new DestructiveOperations(settings, clusterService.getClusterSettings());
securityInterceptor.set(new SecurityServerTransportInterceptor(settings, threadPool, authcService.get(),
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations));
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations, clusterService));

final Set<RequestInterceptor> requestInterceptors;
if (XPackSettings.DLS_FLS_ENABLED.get(settings)) {
Expand Down
29 changes: 19 additions & 10 deletions ...n/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.DestructiveOperations;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.CheckedConsumer;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.gateway.GatewayService;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.threadpool.ThreadPool;
Expand Down Expand Up @@ -72,14 +74,17 @@ public class SecurityServerTransportInterceptor extends AbstractComponent implem
private final SecurityContext securityContext;
private final boolean reservedRealmEnabled;

private volatile boolean isStateNotRecovered = true;

public SecurityServerTransportInterceptor(Settings settings,
ThreadPool threadPool,
AuthenticationService authcService,
AuthorizationService authzService,
XPackLicenseState licenseState,
SSLService sslService,
SecurityContext securityContext,
DestructiveOperations destructiveOperations) {
DestructiveOperations destructiveOperations,
ClusterService clusterService) {
super(settings);
this.settings = settings;
this.threadPool = threadPool;
Expand All @@ -90,6 +95,7 @@ public SecurityServerTransportInterceptor(Settings settings,
this.securityContext = securityContext;
this.profileFilters = initializeProfileFilters(destructiveOperations);
this.reservedRealmEnabled = XPackSettings.RESERVED_REALM_ENABLED_SETTING.get(settings);
clusterService.addListener(e -> isStateNotRecovered = e.state().blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK));
}

@Override
Expand All @@ -98,7 +104,9 @@ public AsyncSender interceptSender(AsyncSender sender) {
@Override
public <T extends TransportResponse> void sendRequest(Transport.Connection connection, String action, TransportRequest request,
TransportRequestOptions options, TransportResponseHandler<T> handler) {
if (licenseState.isSecurityEnabled() && licenseState.isAuthAllowed()) {
final boolean stateNotRecovered = isStateNotRecovered;
final boolean sendWithAuth = (licenseState.isSecurityEnabled() && licenseState.isAuthAllowed()) || stateNotRecovered;
if (sendWithAuth) {
// the transport in core normally does this check, BUT since we are serializing to a string header we need to do it
// ourselves otherwise we wind up using a version newer than what we can actually send
final Version minVersion = Version.min(connection.getVersion(), Version.CURRENT);
Expand All @@ -108,20 +116,20 @@ public <T extends TransportResponse> void sendRequest(Transport.Connection conne
if (AuthorizationUtils.shouldReplaceUserWithSystem(threadPool.getThreadContext(), action)) {
securityContext.executeAsUser(SystemUser.INSTANCE, (original) -> sendWithUser(connection, action, request, options,
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original)
, handler), sender), minVersion);
, handler), sender, stateNotRecovered), minVersion);
} else if (AuthorizationUtils.shouldSetUserBasedOnActionOrigin(threadPool.getThreadContext())) {
AuthorizationUtils.switchUserBasedOnActionOriginAndExecute(threadPool.getThreadContext(), securityContext,
(original) -> sendWithUser(connection, action, request, options,
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original)
, handler), sender));
, handler), sender, stateNotRecovered));
} else if (securityContext.getAuthentication() != null &&
securityContext.getAuthentication().getVersion().equals(minVersion) == false) {
// re-write the authentication since we want the authentication version to match the version of the connection
securityContext.executeAfterRewritingAuthentication(original -> sendWithUser(connection, action, request, options,
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original), handler), sender),
minVersion);
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original), handler), sender,
stateNotRecovered), minVersion);
} else {
sendWithUser(connection, action, request, options, handler, sender);
sendWithUser(connection, action, request, options, handler, sender, stateNotRecovered);
}
} else {
sender.sendRequest(connection, action, request, options, handler);
Expand All @@ -132,9 +140,10 @@ public <T extends TransportResponse> void sendRequest(Transport.Connection conne

private <T extends TransportResponse> void sendWithUser(Transport.Connection connection, String action, TransportRequest request,
TransportRequestOptions options, TransportResponseHandler<T> handler,
AsyncSender sender) {
// There cannot be a request outgoing from this node that is not associated with a user.
if (securityContext.getAuthentication() == null) {
AsyncSender sender, final boolean stateNotRecovered) {
// There cannot be a request outgoing from this node that is not associated with a user
// unless we do not know the actual license of the cluster
if (securityContext.getAuthentication() == null && stateNotRecovered == false) {
// we use an assertion here to ensure we catch this in our testing infrastructure, but leave the ISE for cases we do not catch
// in tests and may be hit by a user
assertNoAuthentication(action);
Expand Down
48 changes: 48 additions & 0 deletions x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,16 @@
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.discovery.DiscoveryModule;
import org.elasticsearch.node.MockNode;
import org.elasticsearch.node.Node;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.MockHttpTransport;
import org.elasticsearch.test.SecurityIntegTestCase;
import org.elasticsearch.test.SecuritySettingsSource;
import org.elasticsearch.test.SecuritySettingsSourceField;
import org.elasticsearch.test.discovery.TestZenDiscovery;
import org.elasticsearch.test.junit.annotations.TestLogging;
import org.elasticsearch.transport.Netty4Plugin;
import org.elasticsearch.transport.Transport;
Expand All @@ -41,7 +46,10 @@
import org.junit.After;
import org.junit.Before;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
Expand Down Expand Up @@ -115,6 +123,18 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
return plugins;
}

@Override
protected int maxNumberOfNodes() {
return super.maxNumberOfNodes() + 1;
}

@Override
public Settings nodeSettings(int nodeOrdinal) {
return Settings.builder().put(super.nodeSettings(nodeOrdinal))
.put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)
.build();
}

@Before
public void resetLicensing() {
enableLicensing();
Expand Down Expand Up @@ -250,6 +270,34 @@ public void testTransportClientAuthenticationByLicenseType() throws Exception {
}
}

public void testNodeJoinWithoutSecurityExplicitlyEnabled() throws Exception {
License.OperationMode mode = randomFrom(License.OperationMode.GOLD, License.OperationMode.PLATINUM, License.OperationMode.STANDARD);
enableLicensing(mode);
ensureGreen();

Path home = createTempDir();
Path conf = home.resolve("config");
Files.createDirectories(conf);
Settings nodeSettings = Settings.builder()
.put(nodeSettings(maxNumberOfNodes() - 1).filter(s -> "xpack.security.enabled".equals(s) == false))
.put("node.name", "my-test-node")
.put("network.host", "localhost")
.put("cluster.name", internalCluster().getClusterName())
.put("discovery.zen.minimum_master_nodes",
internalCluster().getInstance(Settings.class).get("discovery.zen.minimum_master_nodes"))
.put("path.home", home)
.put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)
.put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "test-zen")
.put(DiscoveryModule.DISCOVERY_HOSTS_PROVIDER_SETTING.getKey(), "test-zen")
.build();
Collection<Class<? extends Plugin>> mockPlugins = Arrays.asList(LocalStateSecurity.class, TestZenDiscovery.TestPlugin.class,
MockHttpTransport.TestPlugin.class);
try (Node node = new MockNode(nodeSettings, mockPlugins)) {
node.start();
ensureStableCluster(cluster().size() + 1);
}
}

private static void assertElasticsearchSecurityException(ThrowingRunnable runnable) {
ElasticsearchSecurityException ee = expectThrows(ElasticsearchSecurityException.class, runnable);
assertThat(ee.getMetadata(LicenseUtils.EXPIRED_FEATURE_METADATA), hasItem(XPackField.SECURITY));
Expand Down
Loading

0 comments on commit 1d3f935

Please sign in to comment.