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

Mandate X-Pack REST handler installed #71061

Merged
merged 30 commits into from
Jun 10, 2021
Merged

Mandate X-Pack REST handler installed #71061

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a19e287
Mandate X-Pack REST handler installed
BigPandaToo Mar 30, 2021
8bc8643
Mandate X-Pack REST handler installed
BigPandaToo Mar 30, 2021
7fe071c
Mandate X-Pack REST handler installed
BigPandaToo Mar 30, 2021
e6b1581
Merge branch 'master' into Rest_handler
elasticmachine Mar 30, 2021
f707583
Merge branch 'master' into Rest_handler
elasticmachine Apr 8, 2021
b7bb95b
Mandate X-Pack REST handler installed
BigPandaToo Apr 15, 2021
b18a6be
Merge branch 'master' into Rest_handler
elasticmachine Apr 15, 2021
cc5bce6
Merge branch 'master' into Rest_handler
elasticmachine Apr 19, 2021
b1b00f0
Removing Warning headers
BigPandaToo Apr 19, 2021
d0f0252
Merge branch 'master' into Rest_handler
elasticmachine May 14, 2021
a0fba39
Merge branch 'master' into Rest_handler
elasticmachine May 26, 2021
065fb8e
Mandate X-Pack REST handler installed
BigPandaToo May 27, 2021
75dc96b
adding test
BigPandaToo May 27, 2021
0e99823
adding test
BigPandaToo May 27, 2021
9fd1481
adding test
BigPandaToo May 27, 2021
6e176b4
Addressing PR comments
BigPandaToo May 28, 2021
7651421
Addressing PR comments
BigPandaToo May 28, 2021
e3f28ff
Addressing PR comments
BigPandaToo May 28, 2021
b1b51fd
Addressing PR comments
BigPandaToo May 28, 2021
7ca2084
Merge branch 'master' into Rest_handler
elasticmachine May 30, 2021
6bfc65c
Relaxing the plugin name verification check for testing purposes
BigPandaToo Jun 3, 2021
80d75b3
Merge branch 'master' into Rest_handler
elasticmachine Jun 3, 2021
356636d
Relaxing the plugin name verification check for testing purposes
BigPandaToo Jun 3, 2021
e90511c
Relaxing the plugin name verification check for testing purposes
BigPandaToo Jun 3, 2021
16dcd6c
Relaxing the plugin name verification check for testing purposes
BigPandaToo Jun 3, 2021
5713478
Relaxing the plugin name verification check for testing purposes
BigPandaToo Jun 3, 2021
4a8aa48
Addressing PR feedback
BigPandaToo Jun 8, 2021
4fa2895
Merge branch 'master' into Rest_handler
BigPandaToo Jun 8, 2021
81b64d3
Addressing PR feedback
BigPandaToo Jun 9, 2021
f729748
Addressing PR feedback
BigPandaToo Jun 10, 2021
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
17 changes: 5 additions & 12 deletions server/src/main/java/org/elasticsearch/action/ActionModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,6 @@
import org.elasticsearch.common.inject.AbstractModule;
import org.elasticsearch.common.inject.TypeLiteral;
import org.elasticsearch.common.inject.multibindings.MapBinder;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -403,7 +401,6 @@
public class ActionModule extends AbstractModule {

private static final Logger logger = LogManager.getLogger(ActionModule.class);
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(logger.getName());

private final Settings settings;
private final IndexNameExpressionResolver indexNameExpressionResolver;
Expand Down Expand Up @@ -444,17 +441,13 @@ public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpr
UnaryOperator<RestHandler> newRestWrapper = plugin.getRestHandlerWrapper(threadPool.getThreadContext());
if (newRestWrapper != null) {
logger.debug("Using REST wrapper from plugin " + plugin.getClass().getName());
if (restWrapper != null) {
throw new IllegalArgumentException("Cannot have more than one plugin implementing a REST wrapper");
if ("org.elasticsearch.xpack.security.Security".equals(plugin.getClass().getCanonicalName()) == false) {
logger.warn("The " + plugin.getClass().getName() + " plugin tried to install a custom REST wrapper. This " +
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
"functionality is not available anymore.");
throw new IllegalArgumentException("The " + plugin.getClass().getName() + " plugin tried to install a custom REST " +
"wrapper. This functionality is not available anymore.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #73329, there should be tests for this functionality in ActionModuleTests.
Core shouldn't rely on x-pack to do its testing for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to ActionModuleTests

restWrapper = newRestWrapper;
if (restWrapper.getClass().getCanonicalName() == null ||
restWrapper.getClass().getCanonicalName().startsWith("org.elasticsearch") == false) {
deprecationLogger.deprecate(DeprecationCategory.PLUGINS, "3rd_party_rest_deprecation", "The " +
plugin.getClass().getName() + "plugin installs a custom REST wrapper. This functionality is deprecated and will " +
"not be possible in Elasticsearch 8.0. If this plugin is intended to provide security features for Elasticsearch " +
"then you should switch to using the built-in Elasticsearch features instead.");
}
}
}
mappingRequestValidators = new RequestValidators<>(
Expand Down
18 changes: 0 additions & 18 deletions ...rc/javaRestTest/java/org/elasticsearch/xpack/security/EnableSecurityOnBasicLicenseIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import org.apache.http.HttpHost;
import org.apache.http.util.EntityUtils;
import org.elasticsearch.Version;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
Expand All @@ -21,7 +20,6 @@
import org.elasticsearch.test.rest.ESRestTestCase;
import org.elasticsearch.test.rest.yaml.ObjectPath;
import org.elasticsearch.xpack.security.authc.InternalRealms;
import org.hamcrest.Matchers;
import org.junit.BeforeClass;

import java.io.IOException;
Expand Down Expand Up @@ -106,22 +104,6 @@ public void testSecuritySetup() throws Exception {
} else {
checkAllowedWrite(otherIndex);
}
checkSecurityDisabledWarning();
}

public void checkSecurityDisabledWarning() throws Exception {
final Request request = new Request("GET", "/_cat/indices");
Response response = client().performRequest(request);
List<String> warningHeaders = response.getWarnings();
if (securityExplicitlySet) {
assertThat (warningHeaders, Matchers.empty());
} else {
assertThat (warningHeaders, Matchers.hasSize(1));
assertThat (warningHeaders.get(0),
containsString("Elasticsearch built-in security features are not enabled. Without authentication, your cluster could be " +
"accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
Version.CURRENT.minor + "/security-minimal-setup.html to enable security."));
}
}

private String getClusterInfo() throws IOException {
Expand Down
9 changes: 3 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 @@ -1119,12 +1119,9 @@ public Map<String, Supplier<HttpServerTransport>> getHttpTransports(Settings set

@Override
public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
if (enabled == false) {
return null;
}
final boolean ssl = HTTP_SSL_ENABLED.get(settings);
final SSLConfiguration httpSSLConfig = getSslService().getHttpTransportSSLConfiguration();
boolean extractClientCertificate = ssl && getSslService().isSSLClientAuthEnabled(httpSSLConfig);
final boolean ssl = enabled && HTTP_SSL_ENABLED.get(settings);
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
final SSLConfiguration httpSSLConfig = ssl ? getSslService().getHttpTransportSSLConfiguration() : null;
boolean extractClientCertificate = httpSSLConfig != null && getSslService().isSSLClientAuthEnabled(httpSSLConfig);
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
return handler -> new SecurityRestFilter(getLicenseState(), threadContext, authcService.get(), secondayAuthc.get(),
handler, extractClientCertificate);
}
Expand Down
14 changes: 6 additions & 8 deletions ...ugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.logging.log4j.util.Supplier;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.MediaType;
Expand Down Expand Up @@ -64,8 +62,13 @@ public boolean allowSystemIndexAccessByDefault() {

@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
if (licenseState.isSecurityEnabled() && request.method() != Method.OPTIONS) {
if (request.method() == Method.OPTIONS) {
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
// CORS - allow for preflight unauthenticated OPTIONS request
restHandler.handleRequest(request, channel, client);
return;
}

if (licenseState.isSecurityEnabled()) {
if (extractClientCertificate) {
HttpChannel httpChannel = request.getHttpChannel();
SSLEngineUtils.extractClientCertificates(logger, threadContext, httpChannel);
Expand All @@ -90,11 +93,6 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
e -> handleException("Secondary authentication", request, channel, e)));
}, e -> handleException("Authentication", request, channel, e)));
} else {
if (request.method() != Method.OPTIONS) {
HeaderWarning.addWarning("Elasticsearch built-in security features are not enabled. Without authentication, your cluster " +
"could be accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major +
"." + Version.CURRENT.minor + "/security-minimal-setup.html to enable security.");
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
}
restHandler.handleRequest(request, channel, client);
}
}
Expand Down
90 changes: 90 additions & 0 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 @@ -6,10 +6,15 @@
*/
package org.elasticsearch.xpack.security;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionModule;
import org.elasticsearch.client.Client;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexMetadata;
Expand All @@ -18,10 +23,12 @@
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.network.NetworkModule;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsModule;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.env.Environment;
Expand All @@ -30,13 +37,19 @@
import org.elasticsearch.license.License;
import org.elasticsearch.license.TestUtils;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.MapperPlugin;
import org.elasticsearch.rest.RestChannel;
import org.elasticsearch.rest.RestHandler;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.MockLogAppender;
import org.elasticsearch.test.VersionUtils;
import org.elasticsearch.test.rest.FakeRestRequest;
import org.elasticsearch.threadpool.TestThreadPool;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.usage.UsageService;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.elasticsearch.xpack.core.XPackField;
import org.elasticsearch.xpack.core.XPackSettings;
Expand Down Expand Up @@ -71,6 +84,7 @@
import java.util.function.BiConsumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.UnaryOperator;
import java.util.stream.Collectors;

import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_FORMAT_SETTING;
Expand All @@ -81,6 +95,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -514,6 +529,81 @@ public void testLicenseUpdateFailureHandlerUpdate() throws Exception {
}
}

public void testSecurityHandlerIsAlwaysInstalled() throws IllegalAccessException {
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of this test. As an unit test, I think it mixes two things:

  • Security - Ensure a non-null restWrapper is always return by getRestHandlerWrapper.
  • ActionModule - Ensure only restWrapper from Security can be installed

Each of the above can be tested independantly in a more unit-test-like fashion. And if we want to test the real thing, we can go for internal or external cluster.

But I am happy to let it pass if Tim is OK with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing the ActionModule is tricky unless we change how it works. It has a hardcoded dependency on a particular classname, so it's impossible to mock it out (see #73329).

We could soften that check to only check for the package name (org.elasticsearch.xpack.security), or to pass the name of the allowed plugin into the constructor. Either of those would then allow it to be tested in a more unit-test-like fashion. Otherwise the strict class-name dependency makes it difficult

Copy link
Member

Choose a reason for hiding this comment

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

The check is now relaxed to be package name. Do you think it is worth to have more unit test like tests?
Technically, even with the previous check for exact class name, it is possible to have an unit test for ActionModule: Since ActionModule is in a different module and does not depend on xpack security, it is possible to create a stub org.elasticsearch.xpack.security.Security class in its test package and use that for the unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's testing 2 things, and I think it would be better if they were split into

  1. testSecurityPluginInstallsRestHandlerWrapperEvenIfSecurityIsDisabled which can simply be a test that Security.getRestHandlerWrapper() returns non-null even when disabled.
  2. testSecurityRestHandlerWrapperCanBeInstalled() which tests that ActionModule doesn't reject the wrapper installed by security (essentially this test).

final Logger amLogger = LogManager.getLogger(ActionModule.class);
Loggers.setLevel(amLogger, Level.DEBUG);
final MockLogAppender appender = new MockLogAppender();
Loggers.addAppender(amLogger, appender);
appender.start();

Settings settings = Settings.builder()
.put("xpack.security.enabled", false)
.put("path.home", createTempDir())
.build();
SettingsModule settingsModule = new SettingsModule(Settings.EMPTY);
ThreadPool threadPool = new TestThreadPool(getTestName());

try {
UsageService usageService = new UsageService();
Security security = new Security(settings, null);

appender.addExpectation(new MockLogAppender.SeenEventExpectation(
"Security rest wrapper", ActionModule.class.getName(), Level.DEBUG,
"Using REST wrapper from plugin org.elasticsearch.xpack.security.Security"
));
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved

ActionModule actionModule = new ActionModule(settingsModule.getSettings(),
TestIndexNameExpressionResolver.newInstance(threadPool.getThreadContext()),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, Arrays.asList(security), null, null, usageService, null);
actionModule.initRestHandlers(null);
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?
I think you can skip all the ActionModule part since this is a simple unit test on Security


appender.assertAllExpectationsMatched();
} finally {
threadPool.shutdown();
appender.stop();
Loggers.removeAppender(amLogger, appender);
}
}

public void test3rdPartyHandlerIsNotInstalled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This belongs in ActionModuleTests, it's a testing the behaviour of ActionModule, so it shouldn't be here.

Settings settings = Settings.builder()
.put("xpack.security.enabled", false)
.put("path.home", createTempDir())
.build();

SettingsModule settingsModule = new SettingsModule(Settings.EMPTY);
ThreadPool threadPool = new TestThreadPool(getTestName());

class FakeHandler implements RestHandler {
@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
}
}
ActionPlugin secPlugin = new ActionPlugin() {
@Override
public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
return handler -> new FakeHandler();
}
};

try {
UsageService usageService = new UsageService();
Security security = new Security(settings, null);

Exception e = expectThrows(IllegalArgumentException.class, () ->
new ActionModule(settingsModule.getSettings(),
TestIndexNameExpressionResolver.newInstance(threadPool.getThreadContext()),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, Arrays.asList(security, secPlugin), null, null, usageService, null)
);
assertThat(e.getMessage(), is("The org.elasticsearch.xpack.security.SecurityTests$3 plugin tried to install a custom REST" +
BigPandaToo marked this conversation as resolved.
Show resolved Hide resolved
" wrapper. This functionality is not available anymore."));
} finally {
threadPool.shutdown();
}
}

private void logAndFail(Exception e) {
logger.error("unexpected exception", e);
fail("unexpected exception " + e.getMessage());
Expand Down
4 changes: 0 additions & 4 deletions ...security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import com.nimbusds.jose.util.StandardCharset;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.bytes.BytesArray;
Expand Down Expand Up @@ -144,9 +143,6 @@ public void testProcessBasicLicense() throws Exception {
RestRequest request = mock(RestRequest.class);
when(licenseState.isSecurityEnabled()).thenReturn(false);
filter.handleRequest(request, channel, null);
assertWarnings("Elasticsearch built-in security features are not enabled. Without authentication, your cluster " +
"could be accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
Version.CURRENT.minor + "/security-minimal-setup.html to enable security.");
verify(restHandler).handleRequest(request, channel, null);
verifyZeroInteractions(channel, authcService);
}
Expand Down