Mandate X-Pack REST handler installed #71061

BigPandaToo · 2021-03-30T14:00:22Z

In order to provide a stronger guarantee to our solutions, that if a
cluster is running the default distribution and has security
(authentication) enabled,
then it will be provided by Elastic's security features, and users can
rely on it behaving in the ways they expect, this change mandate
that security in default distribution is provided by X-Pack by always
installing the Security Rest Filter.

Related: #70523

Related: elastic#70523

elasticmachine · 2021-03-30T14:01:09Z

Pinging @elastic/es-security (Team:Security)

Related: elastic#70523

BigPandaToo · 2021-03-30T18:25:12Z

@elasticmachine run elasticsearch-ci/bwc

Related: elastic#70523

BigPandaToo · 2021-03-30T19:20:04Z

@elasticmachine update branch

BigPandaToo · 2021-04-08T18:01:53Z

@elasticmachine update branch

tvernum

We also need to fill in the PR description

server/src/main/java/org/elasticsearch/action/ActionModule.java

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

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

Related: elastic#70523

BigPandaToo · 2021-04-15T15:26:29Z

@elasticmachine update branch

BigPandaToo · 2021-04-19T13:47:21Z

@elasticmachine update branch

tvernum · 2021-05-14T04:44:28Z

@BigPandaToo Can you please fill in the PR description.

server/src/main/java/org/elasticsearch/action/ActionModule.java

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

BigPandaToo · 2021-05-14T12:59:52Z

@elasticmachine update branch

BigPandaToo · 2021-05-26T18:49:34Z

@elasticmachine update branch

BigPandaToo · 2021-06-03T08:43:43Z

@elasticmachine update branch

ywangd

LGTM

A few nits and also I don't particular like the "unit test". But they don't need to block this PR. Thanks!

ywangd · 2021-06-07T12:00:08Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+                if (plugin.getClass().getCanonicalName() == null ||
+                    plugin.getClass().getCanonicalName().startsWith("org.elasticsearch.xpack") == false) {


Nit: I probably missed the discussion about why we prefer getCanonicalName over just getName here. But it seems to me that getName is more consistent since that is what we report in the error message (and also the debug log above)?

ywangd · 2021-06-07T12:01:45Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+                    logger.error("The " + plugin.getClass().getName() + " plugin tried to install a custom REST wrapper. This " +
+                        "functionality is not available anymore.");


Nit: Since an exception is thrown immediately after this and will be logged. It seems this logging is no longer necessary.

ywangd · 2021-06-07T12:06:59Z

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

+        boolean extractClientCertificate = false;
+        if (enabled && HTTP_SSL_ENABLED.get(settings)) {
+            final SSLConfiguration httpSSLConfig = getSslService().getHttpTransportSSLConfiguration();
+            extractClientCertificate = getSslService().isSSLClientAuthEnabled(httpSSLConfig);
        }


Nit: we can save the extra finalExtractClientCertificate variable.

Suggested change

boolean extractClientCertificate = false;

if (enabled && HTTP_SSL_ENABLED.get(settings)) {

final SSLConfiguration httpSSLConfig = getSslService().getHttpTransportSSLConfiguration();

extractClientCertificate = getSslService().isSSLClientAuthEnabled(httpSSLConfig);

}

final boolean extractClientCertificate;

if (enabled && HTTP_SSL_ENABLED.get(settings)) {

final SSLConfiguration httpSSLConfig = getSslService().getHttpTransportSSLConfiguration();

extractClientCertificate = getSslService().isSSLClientAuthEnabled(httpSSLConfig);

} else {

extractClientCertificate = false;

}

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

ywangd · 2021-06-07T12:13:37Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java

@@ -514,6 +529,81 @@ public void testLicenseUpdateFailureHandlerUpdate() throws Exception {
        }
    }

+    public void testSecurityHandlerIsAlwaysInstalled() throws IllegalAccessException {


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.

BigPandaToo · 2021-06-08T20:40:40Z

@elasticmachine update branch

elasticmachine · 2021-06-08T20:40:42Z

merge conflict between base and head

tvernum · 2021-06-09T12:28:57Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+                        "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.");
+                }


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

Moved it to ActionModuleTests

tvernum · 2021-06-09T12:32:37Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java

+        }
+    }
+
+    public void test3rdPartyHandlerIsNotInstalled() {


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

tvernum · 2021-06-09T12:35:34Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java

@@ -514,6 +529,81 @@ public void testLicenseUpdateFailureHandlerUpdate() throws Exception {
        }
    }

+    public void testSecurityHandlerIsAlwaysInstalled() throws IllegalAccessException {


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

testSecurityPluginInstallsRestHandlerWrapperEvenIfSecurityIsDisabled which can simply be a test that Security.getRestHandlerWrapper() returns non-null even when disabled.

testSecurityRestHandlerWrapperCanBeInstalled() which tests that ActionModule doesn't reject the wrapper installed by security (essentially this test).

tvernum

LGTM, but I have a couple of suggestions.

tvernum · 2021-06-10T04:18:52Z

server/src/test/java/org/elasticsearch/action/ActionModuleTests.java

+    class SecPlugin implements ActionPlugin {
+        @Override
+        public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
+            return handler -> new FakeHandler();


This is fine - there's no need to change it, but it would have been fine to just make this a no-op operator.

Suggested change

return handler -> new FakeHandler();

return UnaryOperator.identity();

We really only care that the plugin returns something. We don't care what it does.

tvernum · 2021-06-10T04:20:10Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java

+            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);


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

In elastic#71061 we removed support for custom REST Handler Wrappers. This change adds that information to the migration guide under the "Plugin changes" section

In #71061 we removed support for custom REST Handler Wrappers. This change adds that information to the migration guide under the "Plugin changes" section

In elastic#71061 we removed support for custom REST Handler Wrappers. This change adds that information to the migration guide under the "Plugin changes" section

In #71061 we removed support for custom REST Handler Wrappers. This change adds that information to the migration guide under the "Plugin changes" section

Mandate X-Pack REST handler installed

a19e287

Related: elastic#70523

BigPandaToo requested review from ywangd and tvernum and removed request for ywangd March 30, 2021 14:00

BigPandaToo added v8.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Mar 30, 2021

elasticmachine added the Team:Security Meta label for security team label Mar 30, 2021

Mandate X-Pack REST handler installed

8bc8643

Related: elastic#70523

Mandate X-Pack REST handler installed

7fe071c

Related: elastic#70523

Merge branch 'master' into Rest_handler

e6b1581

Merge branch 'master' into Rest_handler

f707583

tvernum added the >breaking-java label Apr 13, 2021

tvernum reviewed Apr 13, 2021

View reviewed changes

Mandate X-Pack REST handler installed

b7bb95b

Related: elastic#70523

Merge branch 'master' into Rest_handler

b18a6be

elasticmachine and others added 2 commits April 19, 2021 14:47

Merge branch 'master' into Rest_handler

cc5bce6

Removing Warning headers

b1b00f0

BigPandaToo requested a review from tvernum April 20, 2021 15:15

tvernum requested changes May 14, 2021

View reviewed changes

Merge branch 'master' into Rest_handler

d0f0252

Merge branch 'master' into Rest_handler

a0fba39

Relaxing the plugin name verification check for testing purposes

6bfc65c

elasticmachine and others added 5 commits June 3, 2021 03:43

Merge branch 'master' into Rest_handler

80d75b3

Relaxing the plugin name verification check for testing purposes

356636d

Relaxing the plugin name verification check for testing purposes

e90511c

Relaxing the plugin name verification check for testing purposes

16dcd6c

Relaxing the plugin name verification check for testing purposes

5713478

BigPandaToo requested review from tvernum and ywangd June 3, 2021 12:30

ywangd approved these changes Jun 7, 2021

View reviewed changes

Addressing PR feedback

4a8aa48

Merge branch 'master' into Rest_handler

4fa2895

tvernum reviewed Jun 9, 2021

View reviewed changes

Addressing PR feedback

81b64d3

BigPandaToo requested a review from tvernum June 9, 2021 19:37

tvernum approved these changes Jun 10, 2021

View reviewed changes

Addressing PR feedback

f729748

BigPandaToo merged commit 3eea3fb into elastic:master Jun 10, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

tvernum mentioned this pull request Feb 7, 2022

Document removal of custom REST wrappers in 8.0 #83565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mandate X-Pack REST handler installed #71061

Mandate X-Pack REST handler installed #71061

BigPandaToo commented Mar 30, 2021 •

edited

Loading

elasticmachine commented Mar 30, 2021

BigPandaToo commented Mar 30, 2021

BigPandaToo commented Mar 30, 2021

BigPandaToo commented Apr 8, 2021

tvernum left a comment

BigPandaToo commented Apr 15, 2021

BigPandaToo commented Apr 19, 2021

tvernum commented May 14, 2021

BigPandaToo commented May 14, 2021

BigPandaToo commented May 26, 2021

BigPandaToo commented Jun 3, 2021

ywangd left a comment

ywangd Jun 7, 2021

ywangd Jun 7, 2021

ywangd Jun 7, 2021

ywangd Jun 7, 2021

BigPandaToo commented Jun 8, 2021

elasticmachine commented Jun 8, 2021

tvernum Jun 9, 2021

BigPandaToo Jun 9, 2021

tvernum Jun 9, 2021

tvernum Jun 9, 2021

tvernum left a comment

tvernum Jun 10, 2021

tvernum Jun 10, 2021

		if (plugin.getClass().getCanonicalName() == null \|\|
		plugin.getClass().getCanonicalName().startsWith("org.elasticsearch.xpack") == false) {

		logger.error("The " + plugin.getClass().getName() + " plugin tried to install a custom REST wrapper. This " +
		"functionality is not available anymore.");

	return handler -> new FakeHandler();
	return UnaryOperator.identity();

Mandate X-Pack REST handler installed #71061

Mandate X-Pack REST handler installed #71061

Conversation

BigPandaToo commented Mar 30, 2021 • edited Loading

elasticmachine commented Mar 30, 2021

BigPandaToo commented Mar 30, 2021

BigPandaToo commented Mar 30, 2021

BigPandaToo commented Apr 8, 2021

tvernum left a comment

Choose a reason for hiding this comment

BigPandaToo commented Apr 15, 2021

BigPandaToo commented Apr 19, 2021

tvernum commented May 14, 2021

BigPandaToo commented May 14, 2021

BigPandaToo commented May 26, 2021

BigPandaToo commented Jun 3, 2021

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigPandaToo commented Jun 8, 2021

elasticmachine commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigPandaToo commented Mar 30, 2021 •

edited

Loading