Skip to content

Commit dc57eec

Browse files
authored
Security: fix joining cluster with production license (#31341)
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 license 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 #31332
1 parent 529e704 commit dc57eec

File tree

6 files changed

+156
-21
lines changed

6 files changed

+156
-21
lines changed

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,11 @@ private static class Status {
254254

255255
public XPackLicenseState(Settings settings) {
256256
this.isSecurityEnabled = XPackSettings.SECURITY_ENABLED.get(settings);
257-
this.isSecurityExplicitlyEnabled = settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) && isSecurityEnabled;
257+
// 6.0+ requires TLS for production licenses, so if TLS is enabled and security is enabled
258+
// we can interpret this as an explicit enabling of security if the security enabled
259+
// setting is not explicitly set
260+
this.isSecurityExplicitlyEnabled = isSecurityEnabled &&
261+
(settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) || XPackSettings.TRANSPORT_SSL_ENABLED.get(settings));
258262
}
259263

260264
/** Updates the current state of the license, which will change what features are available. */

x-pack/plugin/core/src/test/java/org/elasticsearch/license/XPackLicenseStateTests.java

+10
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,16 @@ public void testSecurityDefaults() {
7979
assertThat(licenseState.allowedRealmType(), is(XPackLicenseState.AllowedRealmType.ALL));
8080
assertThat(licenseState.isCustomRoleProvidersAllowed(), is(true));
8181

82+
licenseState =
83+
new XPackLicenseState(Settings.builder().put(XPackSettings.TRANSPORT_SSL_ENABLED.getKey(), true).build());
84+
assertThat(licenseState.isAuthAllowed(), is(true));
85+
assertThat(licenseState.isIpFilteringAllowed(), is(true));
86+
assertThat(licenseState.isAuditingAllowed(), is(true));
87+
assertThat(licenseState.isStatsAndHealthAllowed(), is(true));
88+
assertThat(licenseState.isDocumentAndFieldLevelSecurityAllowed(), is(true));
89+
assertThat(licenseState.allowedRealmType(), is(XPackLicenseState.AllowedRealmType.ALL));
90+
assertThat(licenseState.isCustomRoleProvidersAllowed(), is(true));
91+
8292
licenseState = new XPackLicenseState(Settings.EMPTY);
8393
assertThat(licenseState.isAuthAllowed(), is(true));
8494
assertThat(licenseState.isIpFilteringAllowed(), is(true));

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
472472
components.add(ipFilter.get());
473473
DestructiveOperations destructiveOperations = new DestructiveOperations(settings, clusterService.getClusterSettings());
474474
securityInterceptor.set(new SecurityServerTransportInterceptor(settings, threadPool, authcService.get(),
475-
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations));
475+
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations, clusterService));
476476

477477
final Set<RequestInterceptor> requestInterceptors;
478478
if (XPackSettings.DLS_FLS_ENABLED.get(settings)) {

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

+23-10
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@
99
import org.elasticsearch.Version;
1010
import org.elasticsearch.action.ActionListener;
1111
import org.elasticsearch.action.support.DestructiveOperations;
12+
import org.elasticsearch.cluster.service.ClusterService;
1213
import org.elasticsearch.common.CheckedConsumer;
1314
import org.elasticsearch.common.component.AbstractComponent;
1415
import org.elasticsearch.common.settings.Setting;
1516
import org.elasticsearch.common.settings.Settings;
1617
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
1718
import org.elasticsearch.common.util.concurrent.ThreadContext;
19+
import org.elasticsearch.gateway.GatewayService;
1820
import org.elasticsearch.license.XPackLicenseState;
1921
import org.elasticsearch.tasks.Task;
2022
import org.elasticsearch.threadpool.ThreadPool;
@@ -72,14 +74,17 @@ public class SecurityServerTransportInterceptor extends AbstractComponent implem
7274
private final SecurityContext securityContext;
7375
private final boolean reservedRealmEnabled;
7476

77+
private volatile boolean isStateNotRecovered = true;
78+
7579
public SecurityServerTransportInterceptor(Settings settings,
7680
ThreadPool threadPool,
7781
AuthenticationService authcService,
7882
AuthorizationService authzService,
7983
XPackLicenseState licenseState,
8084
SSLService sslService,
8185
SecurityContext securityContext,
82-
DestructiveOperations destructiveOperations) {
86+
DestructiveOperations destructiveOperations,
87+
ClusterService clusterService) {
8388
super(settings);
8489
this.settings = settings;
8590
this.threadPool = threadPool;
@@ -90,6 +95,7 @@ public SecurityServerTransportInterceptor(Settings settings,
9095
this.securityContext = securityContext;
9196
this.profileFilters = initializeProfileFilters(destructiveOperations);
9297
this.reservedRealmEnabled = XPackSettings.RESERVED_REALM_ENABLED_SETTING.get(settings);
98+
clusterService.addListener(e -> isStateNotRecovered = e.state().blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK));
9399
}
94100

95101
@Override
@@ -98,7 +104,13 @@ public AsyncSender interceptSender(AsyncSender sender) {
98104
@Override
99105
public <T extends TransportResponse> void sendRequest(Transport.Connection connection, String action, TransportRequest request,
100106
TransportRequestOptions options, TransportResponseHandler<T> handler) {
101-
if (licenseState.isSecurityEnabled() && licenseState.isAuthAllowed()) {
107+
// make a local copy of isStateNotRecovered as this is a volatile variable and it
108+
// is used multiple times in the method. The copy to a local variable allows us to
109+
// guarantee we use the same value wherever we would check the value for the state
110+
// being recovered
111+
final boolean stateNotRecovered = isStateNotRecovered;
112+
final boolean sendWithAuth = (licenseState.isSecurityEnabled() && licenseState.isAuthAllowed()) || stateNotRecovered;
113+
if (sendWithAuth) {
102114
// the transport in core normally does this check, BUT since we are serializing to a string header we need to do it
103115
// ourselves otherwise we wind up using a version newer than what we can actually send
104116
final Version minVersion = Version.min(connection.getVersion(), Version.CURRENT);
@@ -108,20 +120,20 @@ public <T extends TransportResponse> void sendRequest(Transport.Connection conne
108120
if (AuthorizationUtils.shouldReplaceUserWithSystem(threadPool.getThreadContext(), action)) {
109121
securityContext.executeAsUser(SystemUser.INSTANCE, (original) -> sendWithUser(connection, action, request, options,
110122
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original)
111-
, handler), sender), minVersion);
123+
, handler), sender, stateNotRecovered), minVersion);
112124
} else if (AuthorizationUtils.shouldSetUserBasedOnActionOrigin(threadPool.getThreadContext())) {
113125
AuthorizationUtils.switchUserBasedOnActionOriginAndExecute(threadPool.getThreadContext(), securityContext,
114126
(original) -> sendWithUser(connection, action, request, options,
115127
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original)
116-
, handler), sender));
128+
, handler), sender, stateNotRecovered));
117129
} else if (securityContext.getAuthentication() != null &&
118130
securityContext.getAuthentication().getVersion().equals(minVersion) == false) {
119131
// re-write the authentication since we want the authentication version to match the version of the connection
120132
securityContext.executeAfterRewritingAuthentication(original -> sendWithUser(connection, action, request, options,
121-
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original), handler), sender),
122-
minVersion);
133+
new ContextRestoreResponseHandler<>(threadPool.getThreadContext().wrapRestorable(original), handler), sender,
134+
stateNotRecovered), minVersion);
123135
} else {
124-
sendWithUser(connection, action, request, options, handler, sender);
136+
sendWithUser(connection, action, request, options, handler, sender, stateNotRecovered);
125137
}
126138
} else {
127139
sender.sendRequest(connection, action, request, options, handler);
@@ -132,9 +144,10 @@ public <T extends TransportResponse> void sendRequest(Transport.Connection conne
132144

133145
private <T extends TransportResponse> void sendWithUser(Transport.Connection connection, String action, TransportRequest request,
134146
TransportRequestOptions options, TransportResponseHandler<T> handler,
135-
AsyncSender sender) {
136-
// There cannot be a request outgoing from this node that is not associated with a user.
137-
if (securityContext.getAuthentication() == null) {
147+
AsyncSender sender, final boolean stateNotRecovered) {
148+
// There cannot be a request outgoing from this node that is not associated with a user
149+
// unless we do not know the actual license of the cluster
150+
if (securityContext.getAuthentication() == null && stateNotRecovered == false) {
138151
// we use an assertion here to ensure we catch this in our testing infrastructure, but leave the ISE for cases we do not catch
139152
// in tests and may be hit by a user
140153
assertNoAuthentication(action);

x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java

+48
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,16 @@
2323
import org.elasticsearch.common.settings.SecureString;
2424
import org.elasticsearch.common.settings.Settings;
2525
import org.elasticsearch.common.util.concurrent.ThreadContext;
26+
import org.elasticsearch.discovery.DiscoveryModule;
27+
import org.elasticsearch.node.MockNode;
28+
import org.elasticsearch.node.Node;
2629
import org.elasticsearch.plugins.Plugin;
2730
import org.elasticsearch.rest.RestStatus;
31+
import org.elasticsearch.test.MockHttpTransport;
2832
import org.elasticsearch.test.SecurityIntegTestCase;
2933
import org.elasticsearch.test.SecuritySettingsSource;
3034
import org.elasticsearch.test.SecuritySettingsSourceField;
35+
import org.elasticsearch.test.discovery.TestZenDiscovery;
3136
import org.elasticsearch.test.junit.annotations.TestLogging;
3237
import org.elasticsearch.transport.Netty4Plugin;
3338
import org.elasticsearch.transport.Transport;
@@ -41,7 +46,10 @@
4146
import org.junit.After;
4247
import org.junit.Before;
4348

49+
import java.nio.file.Files;
50+
import java.nio.file.Path;
4451
import java.util.ArrayList;
52+
import java.util.Arrays;
4553
import java.util.Collection;
4654

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

126+
@Override
127+
protected int maxNumberOfNodes() {
128+
return super.maxNumberOfNodes() + 1;
129+
}
130+
131+
@Override
132+
public Settings nodeSettings(int nodeOrdinal) {
133+
return Settings.builder().put(super.nodeSettings(nodeOrdinal))
134+
.put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)
135+
.build();
136+
}
137+
118138
@Before
119139
public void resetLicensing() {
120140
enableLicensing();
@@ -250,6 +270,34 @@ public void testTransportClientAuthenticationByLicenseType() throws Exception {
250270
}
251271
}
252272

273+
public void testNodeJoinWithoutSecurityExplicitlyEnabled() throws Exception {
274+
License.OperationMode mode = randomFrom(License.OperationMode.GOLD, License.OperationMode.PLATINUM, License.OperationMode.STANDARD);
275+
enableLicensing(mode);
276+
ensureGreen();
277+
278+
Path home = createTempDir();
279+
Path conf = home.resolve("config");
280+
Files.createDirectories(conf);
281+
Settings nodeSettings = Settings.builder()
282+
.put(nodeSettings(maxNumberOfNodes() - 1).filter(s -> "xpack.security.enabled".equals(s) == false))
283+
.put("node.name", "my-test-node")
284+
.put("network.host", "localhost")
285+
.put("cluster.name", internalCluster().getClusterName())
286+
.put("discovery.zen.minimum_master_nodes",
287+
internalCluster().getInstance(Settings.class).get("discovery.zen.minimum_master_nodes"))
288+
.put("path.home", home)
289+
.put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)
290+
.put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "test-zen")
291+
.put(DiscoveryModule.DISCOVERY_HOSTS_PROVIDER_SETTING.getKey(), "test-zen")
292+
.build();
293+
Collection<Class<? extends Plugin>> mockPlugins = Arrays.asList(LocalStateSecurity.class, TestZenDiscovery.TestPlugin.class,
294+
MockHttpTransport.TestPlugin.class);
295+
try (Node node = new MockNode(nodeSettings, mockPlugins)) {
296+
node.start();
297+
ensureStableCluster(cluster().size() + 1);
298+
}
299+
}
300+
253301
private static void assertElasticsearchSecurityException(ThrowingRunnable runnable) {
254302
ElasticsearchSecurityException ee = expectThrows(ElasticsearchSecurityException.class, runnable);
255303
assertThat(ee.getMetadata(LicenseUtils.EXPIRED_FEATURE_METADATA), hasItem(XPackField.SECURITY));

0 commit comments

Comments
 (0)