Skip to content

Security: fix joining cluster with production license #31341

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

Merged
merged 2 commits into from
Jun 19, 2018
Merged

Security: fix joining cluster with production license #31341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d3f935
Security: fix joining cluster with production license
jaymode Jun 14, 2018
c670b24
add comments to code
jaymode Jun 15, 2018
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
3 changes: 2 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 @@ -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())) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a suggestion from @bleskes that I went ahead and adopted as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the reasoning behind doing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for cases where a user with a production license on 6.0 to 6.2.x upgrades to 6.3.1+. We require TLS for production mode, so if TLS is enabled we can take that as security being explicitly enabled.

(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
Open in desktop
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
Open in desktop
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
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

The purposes of this assignment (from volatile to local) initially confused me, and I'm worried someone might think it is redundant and remove it in the future. Can we add a comment to try and discourage that from happening?

final boolean sendWithAuth = (licenseState.isSecurityEnabled() && licenseState.isAuthAllowed()) || stateNotRecovered;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that if the state is not recovered, then we're going to sendWithAuth even if the local node explitily disables security.
If so, then why have the check at all? If it's safe to send the auth context it even if security is disabled, isn't it simpler to just send it for every outgoing request?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thought as @tvernum here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The interceptor is not registered at all if security is disabled.

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

Lines 808 to 829 in 3b70e94

@Override
public List<TransportInterceptor> getTransportInterceptors(NamedWriteableRegistry namedWriteableRegistry, ThreadContext threadContext) {
if (transportClientMode || enabled == false) { // don't register anything if we are not enabled
// interceptors are not installed if we are running on the transport client
return Collections.emptyList();
}
return Collections.singletonList(new TransportInterceptor() {
@Override
public <T extends TransportRequest> TransportRequestHandler<T> interceptHandler(String action, String executor,
boolean forceExecution,
TransportRequestHandler<T> actualHandler) {
assert securityInterceptor.get() != null;
return securityInterceptor.get().interceptHandler(action, executor, forceExecution, actualHandler);
}
@Override
public AsyncSender interceptSender(AsyncSender sender) {
assert securityInterceptor.get() != null;
return securityInterceptor.get().interceptSender(sender);
}
});
}

This change limits to only sending auth when security could possibly be enabled. Once we've recovered the state then the check is based upon the license. By having the check we maintain the ability to assert that all outgoing requests have authentication after the state has been recovered.

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
Open in desktop
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