Skip to content

Commit cc0fdaf

Browse files
tvernumtvernum
authored
Avoid NPE in set_security_user without security (#52691)
If security was disabled (explicitly), then the SecurityContext would be null, but the set_security_user processor was still registered. Attempting to define a pipeline that used that processor would fail with an (intentional) NPE. This behaviour, introduced in #52032, is a regression from previous releases where the pipeline was allowed, but was no usable. This change restores the previous behaviour (with a new warning).
1 parent 0b38b6a commit cc0fdaf

File tree

9 files changed

+260
-43
lines changed

9 files changed

+260
-43
lines changed

‎x-pack/plugin/security/qa/security-disabled/build.gradle

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* This QA project tests the security plugin when security is explicitlt disabled.
3+
* It is intended to cover security functionality which is supposed to
4+
* function in a specific way even if security is disabled on the cluster
5+
* For example: If a cluster has a pipeline with the set_security_user processor
6+
* defined, it should be not fail
7+
*/
8+
9+
apply plugin: 'elasticsearch.testclusters'
10+
apply plugin: 'elasticsearch.standalone-rest-test'
11+
apply plugin: 'elasticsearch.rest-test'
12+
13+
dependencies {
14+
testCompile project(path: xpackModule('core'), configuration: 'default')
15+
testCompile project(path: xpackModule('security'), configuration: 'testArtifacts')
16+
testCompile project(path: xpackModule('core'), configuration: 'testArtifacts')
17+
}
18+
19+
testClusters.integTest {
20+
testDistribution = 'DEFAULT'
21+
numberOfNodes = 2
22+
23+
setting 'xpack.ilm.enabled', 'false'
24+
setting 'xpack.ml.enabled', 'false'
25+
// We run with a trial license, but explicitly disable security.
26+
// This means the security plugin is loaded and all feature are permitted, but they are not enabled
27+
setting 'xpack.license.self_generated.type', 'trial'
28+
setting 'xpack.security.enabled', 'false'
29+
}

‎x-pack/plugin/security/qa/security-disabled/src/test/java/org/elasticsearch/xpack/security/SetSecurityUserProcessorWithSecurityDisabledIT.java

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
package org.elasticsearch.xpack.security;
7+
8+
import org.apache.http.util.EntityUtils;
9+
import org.elasticsearch.client.Request;
10+
import org.elasticsearch.client.Response;
11+
import org.elasticsearch.client.ResponseException;
12+
import org.elasticsearch.test.rest.ESRestTestCase;
13+
14+
import static org.hamcrest.Matchers.containsString;
15+
16+
/**
17+
* Tests that it is possible to <em>define</em> a pipeline with the
18+
* {@link org.elasticsearch.xpack.security.ingest.SetSecurityUserProcessor} on a cluster with security disabled, but it is not possible
19+
* to use that pipeline for ingestion.
20+
*/
21+
public class SetSecurityUserProcessorWithSecurityDisabledIT extends ESRestTestCase {
22+
23+
public void testDefineAndUseProcessor() throws Exception {
24+
final String pipeline = "pipeline-" + getTestName();
25+
final String index = "index-" + getTestName();
26+
{
27+
final Request putPipeline = new Request("PUT", "/_ingest/pipeline/" + pipeline);
28+
putPipeline.setJsonEntity("{" +
29+
" \"description\": \"Test pipeline (" + getTestName() + ")\"," +
30+
" \"processors\":[{" +
31+
" \"set_security_user\":{ \"field\": \"user\" }" +
32+
" }]" +
33+
"}");
34+
final Response response = client().performRequest(putPipeline);
35+
assertOK(response);
36+
}
37+
38+
{
39+
final Request ingest = new Request("PUT", "/" + index + "/_doc/1?pipeline=" + pipeline);
40+
ingest.setJsonEntity("{\"field\":\"value\"}");
41+
final ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(ingest));
42+
final Response response = ex.getResponse();
43+
assertThat(EntityUtils.toString(response.getEntity()),
44+
containsString("Security (authentication) is not enabled on this cluster"));
45+
}
46+
}
47+
48+
}

‎x-pack/plugin/security/qa/security-not-enabled/build.gradle

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/*
2+
* This QA project tests the security plugin when security is not enabled.
3+
* It is intended to cover security functionality which is supposed to
4+
* function in a specific way even if security is not enabled on the cluster
5+
* For example: If a cluster has a pipeline with the set_security_user processor
6+
* defined, it should be not fail
7+
*/
8+
9+
apply plugin: 'elasticsearch.testclusters'
10+
apply plugin: 'elasticsearch.standalone-rest-test'
11+
apply plugin: 'elasticsearch.rest-test'
12+
13+
dependencies {
14+
testCompile project(path: xpackModule('core'), configuration: 'default')
15+
testCompile project(path: xpackModule('security'), configuration: 'testArtifacts')
16+
testCompile project(path: xpackModule('core'), configuration: 'testArtifacts')
17+
}
18+
19+
testClusters.integTest {
20+
testDistribution = 'DEFAULT'
21+
numberOfNodes = 2
22+
23+
setting 'xpack.ilm.enabled', 'false'
24+
setting 'xpack.ml.enabled', 'false'
25+
// We run with a trial license, but do not enable security.
26+
// This means the security plugin is loaded and all feature are permitted, but they are not enabled
27+
setting 'xpack.license.self_generated.type', 'trial'
28+
}

‎x-pack/plugin/security/qa/security-not-enabled/src/test/java/org/elasticsearch/xpack/security/SetSecurityUserProcessorWithSecurityNotEnabledIT.java

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
package org.elasticsearch.xpack.security;
7+
8+
import org.apache.http.util.EntityUtils;
9+
import org.elasticsearch.client.Request;
10+
import org.elasticsearch.client.Response;
11+
import org.elasticsearch.client.ResponseException;
12+
import org.elasticsearch.test.rest.ESRestTestCase;
13+
14+
import static org.hamcrest.Matchers.containsString;
15+
16+
/**
17+
* Tests that it is possible to <em>define</em> a pipeline with the
18+
* {@link org.elasticsearch.xpack.security.ingest.SetSecurityUserProcessor} on a cluster where security is not enabled,
19+
* but it is not possible to use that pipeline for ingestion.
20+
*/
21+
public class SetSecurityUserProcessorWithSecurityNotEnabledIT extends ESRestTestCase {
22+
23+
public void testDefineAndUseProcessor() throws Exception {
24+
final String pipeline = "pipeline-" + getTestName();
25+
final String index = "index-" + getTestName();
26+
{
27+
final Request putPipeline = new Request("PUT", "/_ingest/pipeline/" + pipeline);
28+
putPipeline.setJsonEntity("{" +
29+
" \"description\": \"Test pipeline (" + getTestName() + ")\"," +
30+
" \"processors\":[{" +
31+
" \"set_security_user\":{ \"field\": \"user\" }" +
32+
" }]" +
33+
"}");
34+
final Response response = client().performRequest(putPipeline);
35+
assertOK(response);
36+
}
37+
38+
{
39+
final Request ingest = new Request("PUT", "/" + index + "/_doc/1?pipeline=" + pipeline);
40+
ingest.setJsonEntity("{\"field\":\"value\"}");
41+
final ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(ingest));
42+
final Response response = ex.getResponse();
43+
assertThat(EntityUtils.toString(response.getEntity()),
44+
containsString("Security (authentication) is not enabled on this cluster"));
45+
}
46+
}
47+
48+
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,8 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
816816

817817
@Override
818818
public Map<String, Processor.Factory> getProcessors(Processor.Parameters parameters) {
819-
return Collections.singletonMap(SetSecurityUserProcessor.TYPE, new SetSecurityUserProcessor.Factory(securityContext::get));
819+
return Collections.singletonMap(SetSecurityUserProcessor.TYPE,
820+
new SetSecurityUserProcessor.Factory(securityContext::get, this::getLicenseState));
820821
}
821822

822823
/**

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@
55
*/
66
package org.elasticsearch.xpack.security.ingest;
77

8+
import org.apache.logging.log4j.LogManager;
9+
import org.apache.logging.log4j.Logger;
810
import org.elasticsearch.ingest.AbstractProcessor;
911
import org.elasticsearch.ingest.IngestDocument;
1012
import org.elasticsearch.ingest.Processor;
13+
import org.elasticsearch.license.XPackLicenseState;
1114
import org.elasticsearch.xpack.core.security.SecurityContext;
1215
import org.elasticsearch.xpack.core.security.authc.Authentication;
1316
import org.elasticsearch.xpack.core.security.user.User;
@@ -34,27 +37,51 @@ public final class SetSecurityUserProcessor extends AbstractProcessor {
3437

3538
public static final String TYPE = "set_security_user";
3639

40+
private final Logger logger = LogManager.getLogger();
41+
3742
private final SecurityContext securityContext;
43+
private final XPackLicenseState licenseState;
3844
private final String field;
3945
private final Set<Property> properties;
4046

41-
public
42-
SetSecurityUserProcessor(String tag, SecurityContext securityContext, String field, Set<Property> properties) {
47+
public SetSecurityUserProcessor(String tag, SecurityContext securityContext, XPackLicenseState licenseState, String field,
48+
Set<Property> properties) {
4349
super(tag);
44-
this.securityContext = Objects.requireNonNull(securityContext, "security context must be provided");
50+
this.securityContext = securityContext;
51+
this.licenseState = Objects.requireNonNull(licenseState, "license state cannot be null");
52+
if (licenseState.isAuthAllowed() == false) {
53+
logger.warn("Creating processor [{}] (tag [{}]) on field [{}] but authentication is not currently enabled on this cluster " +
54+
" - this processor is likely to fail at runtime if it is used", TYPE, tag, field);
55+
} else if (this.securityContext == null) {
56+
throw new IllegalArgumentException("Authentication is allowed on this cluster state, but there is no security context");
57+
}
4558
this.field = field;
4659
this.properties = properties;
4760
}
4861

4962
@Override
5063
public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
51-
Authentication authentication = securityContext.getAuthentication();
52-
if (authentication == null) {
53-
throw new IllegalStateException("No user authenticated, only use this processor via authenticated user");
64+
Authentication authentication = null;
65+
User user = null;
66+
if (this.securityContext != null) {
67+
authentication = securityContext.getAuthentication();
68+
if (authentication != null) {
69+
user = authentication.getUser();
70+
}
5471
}
55-
User user = authentication.getUser();
72+
5673
if (user == null) {
57-
throw new IllegalStateException("No user for authentication");
74+
logger.debug(
75+
"Failed to find active user. SecurityContext=[{}] Authentication=[{}] User=[{}]", securityContext, authentication, user);
76+
if (licenseState.isAuthAllowed()) {
77+
// This shouldn't happen. If authentication is allowed (and active), then there _should_ always be an authenticated user.
78+
// If we ever see this error message, then one of our assumptions are wrong.
79+
throw new IllegalStateException("There is no authenticated user - the [" + TYPE
80+
+ "] processor requires an authenticated user");
81+
} else {
82+
throw new IllegalStateException("Security (authentication) is not enabled on this cluster, so there is no active user - " +
83+
"the [" + TYPE + "] processor cannot be used without security");
84+
}
5885
}
5986

6087
Object fieldValue = ingestDocument.getFieldValue(field, Object.class, true);
@@ -155,9 +182,11 @@ Set<Property> getProperties() {
155182
public static final class Factory implements Processor.Factory {
156183

157184
private final Supplier<SecurityContext> securityContext;
185+
private final Supplier<XPackLicenseState> licenseState;
158186

159-
public Factory(Supplier<SecurityContext> securityContext) {
187+
public Factory(Supplier<SecurityContext> securityContext, Supplier<XPackLicenseState> licenseState) {
160188
this.securityContext = securityContext;
189+
this.licenseState = licenseState;
161190
}
162191

163192
@Override
@@ -174,7 +203,7 @@ public SetSecurityUserProcessor create(Map<String, Processor.Factory> processorF
174203
} else {
175204
properties = EnumSet.allOf(Property.class);
176205
}
177-
return new SetSecurityUserProcessor(tag, securityContext.get(), field, properties);
206+
return new SetSecurityUserProcessor(tag, securityContext.get(), licenseState.get(), field, properties);
178207
}
179208
}
180209

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/apikey/RestGetApiKeyAction.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ public RestResponse buildResponse(GetApiKeyResponse getApiKeyResponse, XContentB
5959
}
6060
return new BytesRestResponse(RestStatus.OK, builder);
6161
}
62+
6263
});
6364
}
6465

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorFactoryTests.java

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,31 +8,36 @@
88
import org.elasticsearch.ElasticsearchParseException;
99
import org.elasticsearch.common.settings.Settings;
1010
import org.elasticsearch.common.util.concurrent.ThreadContext;
11+
import org.elasticsearch.license.XPackLicenseState;
1112
import org.elasticsearch.test.ESTestCase;
1213
import org.elasticsearch.xpack.core.security.SecurityContext;
1314
import org.elasticsearch.xpack.security.ingest.SetSecurityUserProcessor.Property;
1415
import org.junit.Before;
16+
import org.mockito.Mockito;
1517

1618
import java.util.Arrays;
1719
import java.util.EnumSet;
1820
import java.util.HashMap;
1921
import java.util.Map;
2022

21-
import static org.elasticsearch.test.TestMatchers.throwableWithMessage;
22-
import static org.hamcrest.Matchers.containsString;
2323
import static org.hamcrest.Matchers.equalTo;
24+
import static org.hamcrest.Matchers.notNullValue;
25+
import static org.mockito.Mockito.when;
2426

2527
public class SetSecurityUserProcessorFactoryTests extends ESTestCase {
2628

2729
private SecurityContext securityContext;
30+
private XPackLicenseState licenseState;
2831

2932
@Before
3033
public void setupContext() {
3134
securityContext = new SecurityContext(Settings.EMPTY, new ThreadContext(Settings.EMPTY));
35+
licenseState = Mockito.mock(XPackLicenseState.class);
36+
when(licenseState.isAuthAllowed()).thenReturn(true);
3237
}
3338

3439
public void testProcessor() throws Exception {
35-
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext);
40+
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
3641
Map<String, Object> config = new HashMap<>();
3742
config.put("field", "_field");
3843
SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
@@ -41,7 +46,7 @@ public void testProcessor() throws Exception {
4146
}
4247

4348
public void testProcessor_noField() throws Exception {
44-
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext);
49+
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
4550
Map<String, Object> config = new HashMap<>();
4651
ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> factory.create(null, "_tag", config));
4752
assertThat(e.getMetadata("es.property_name").get(0), equalTo("field"));
@@ -50,7 +55,7 @@ public void testProcessor_noField() throws Exception {
5055
}
5156

5257
public void testProcessor_validProperties() throws Exception {
53-
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext);
58+
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
5459
Map<String, Object> config = new HashMap<>();
5560
config.put("field", "_field");
5661
config.put("properties", Arrays.asList(Property.USERNAME.name(), Property.ROLES.name()));
@@ -60,7 +65,7 @@ public void testProcessor_validProperties() throws Exception {
6065
}
6166

6267
public void testProcessor_invalidProperties() throws Exception {
63-
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext);
68+
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
6469
Map<String, Object> config = new HashMap<>();
6570
config.put("field", "_field");
6671
config.put("properties", Arrays.asList("invalid"));
@@ -70,12 +75,13 @@ public void testProcessor_invalidProperties() throws Exception {
7075
assertThat(e.getMetadata("es.processor_tag").get(0), equalTo("_tag"));
7176
}
7277

73-
public void testNullSecurityContextThrowsException() throws Exception {
74-
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> null);
78+
public void testCanConstructorProcessorWithoutSecurityEnabled() throws Exception {
79+
when(licenseState.isAuthAllowed()).thenReturn(false);
80+
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> null, () -> licenseState);
7581
Map<String, Object> config = new HashMap<>();
7682
config.put("field", "_field");
77-
NullPointerException e = expectThrows(NullPointerException.class, () -> factory.create(null, "_tag", config));
78-
assertThat(e, throwableWithMessage(containsString("security context")));
83+
final SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
84+
assertThat(processor, notNullValue());
7985
}
8086

8187
}

0 commit comments

Comments
 (0)