[TEST] Reload secure settings transport IT #31180

albertzaharovits · 2018-06-07T15:59:48Z

Adds Integration Tests for the TransportNodesReloadSecureSettingsAction . Specifically, it tests:

SecureSettings are retrievable when plugins are notified
missing/invalid keystore is handled gracefully (no spurious reload calls)
reloadable plugins should fail independently (a throw during a plugin reload on any node will not interrupt the reload flow in any way - it will just return the exception in the response)

Previously there were tests only for the individual plugins implementing the reload handler, no tests for the "broadcast password - decrypt keystore - notify plugins - close keystore" logic.

One obstacle I struggled with was generating keystores programatically. They are test resources in the present implementation. The problem is that saving a generated keystores requires the Java SM permissions to change file permissions. This is not granted for the server codebase. In the real world the keystore is generated before the Security Manager is installed.

Relates: #29135
CC @elastic/es-security

elasticmachine · 2018-06-07T15:59:50Z

Pinging @elastic/es-core-infra

albertzaharovits · 2018-06-07T16:04:15Z

...elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -102,6 +104,8 @@ protected NodeRequest newNodeRequest(String nodeId, NodesReloadSecureSettingsReq
                try {
                    p.reload(settingsWithKeystore);
                } catch (final Exception e) {
+                    logger.warn((Supplier<?>) () -> new ParameterizedMessage("Plugin [{}] threw [{}] on node [{}]",
+                            p.getClass().getSimpleName(), e.getClass().getSimpleName(), clusterService.localNode().getName()), e);


1/2 non test change

albertzaharovits · 2018-06-07T16:07:12Z

server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java

-            assert password.length == 0;
+            if (password.length != 0) {
+                throw new IllegalArgumentException("Keystore format does not accept non-empty passwords");
+            }


2/2 non test change
This password is no longer internal API, there are code paths where the user supplies its value.

albertzaharovits · 2018-06-11T17:03:34Z

...elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -102,6 +104,8 @@ protected NodeRequest newNodeRequest(String nodeId, NodesReloadSecureSettingsReq
                try {
                    p.reload(settingsWithKeystore);
                } catch (final Exception e) {
+                    logger.warn((Supplier<?>) () -> new ParameterizedMessage("Plugin [{}] threw [{}] during reload",


1/2 non test change

I don't think we need the class name in the message since we also log the exception

jaymode

I left some comments. Regarding not being able to write a keystore in tests, maybe someone from @elastic/es-core-infra could provide some thoughts about whether there is a better way than checking the files in as a resource.

jaymode · 2018-06-13T13:43:19Z

...elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -102,6 +104,8 @@ protected NodeRequest newNodeRequest(String nodeId, NodesReloadSecureSettingsReq
                try {
                    p.reload(settingsWithKeystore);
                } catch (final Exception e) {
+                    logger.warn((Supplier<?>) () -> new ParameterizedMessage("Plugin [{}] threw [{}] during reload",


I don't think we need the class name in the message since we also log the exception

jaymode · 2018-06-13T13:52:22Z

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

+
+    @Override
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return Arrays.asList(MockReloadablePlugin.class, MisbehavingReloadablePlugin.class);


should we randomize the order? Sometimes the misbehaving goes first?

Yes! I was under the impression that the order here is not relevant when iterating oveer Plugin instances. But it is, and this is a good thing!

jaymode · 2018-06-13T14:45:16Z

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

+
+        @Override
+        public List<Setting<?>> getSettings() {
+            return Arrays.asList(DUMMY_SECRET_SETTING);


Collections.singletonList

jaymode · 2018-06-13T14:48:50Z

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

+
+    public static class MisbehavingReloadablePlugin extends CountingReloadablePlugin {
+
+        private volatile boolean sulky = false;


can we rename this to shouldThrow? Then just have a setter for the method? Also, let's make this non-volatile and the methods synchronized since we technically do a CAS in reload

Yes, I've been lax here.

albertzaharovits · 2018-06-13T17:37:53Z

Thank you @jaymode .
I have addressed your comments. I will wait a little for suggestions, but probably start pulling someone's coat over slack.

jaymode

LGTM pending discussion with the core-infra team regarding the updating of the keystore during the test

albertzaharovits · 2018-06-15T16:26:23Z

@jaymode The matter did not piqued any interest.
I have found a workaround which does not require any permission changes.
The trick simply ignores the RuntimePermission("accessUserInformation"). This is possible because the access exception is thrown after the keystore has been written, as a matter of precaution against broken umask settings. This is not required in the test environment.
Also, the keystore contains only the seed value, which is alright in this case, as the setString is package-private.

Please take another short look, thanks!

jaymode

Left one comment. Otherwise LGTM as long as CI is happy.

jaymode · 2018-06-15T20:53:04Z

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        final List<Class<? extends Plugin>> plugins = Arrays.asList(MockReloadablePlugin.class, MisbehavingReloadablePlugin.class);
+        // shuffle as reload is called in order
+        Collections.shuffle(plugins);


you need to use Collections.shuffle(plugins, random()) to fix the build failure

albertzaharovits · 2018-06-16T18:08:47Z

Appreciate the prompt review! Thank you, I will make sure CI runs several times before merging the feature branch!

albertzaharovits added 3 commits June 5, 2018 14:54

WIP

9a1cca3

Raw test, no keystore present

e8c4e12

Proper integ test for reload secret settings

2ed5ae1

albertzaharovits added >test Issues or PRs that are addressing/adding tests review :Core/Infra/Settings Settings infrastructure and APIs labels Jun 7, 2018

albertzaharovits self-assigned this Jun 7, 2018

albertzaharovits requested a review from jaymode June 7, 2018 15:59

albertzaharovits commented Jun 7, 2018

View reviewed changes

albertzaharovits added 2 commits June 9, 2018 20:19

Merge branch 'reload-secure-store-action' into ReloadIT

9cab952

Small adjustment to the log message

68368a9

albertzaharovits commented Jun 11, 2018

View reviewed changes

Merge branch 'reload-secure-store-action' into ReloadIT

9067cb4

jaymode requested changes Jun 13, 2018

View reviewed changes

Address feedback

afb0e11

jaymode approved these changes Jun 13, 2018

View reviewed changes

albertzaharovits added 2 commits June 15, 2018 19:13

Write keystore by ignoring the last Access Exception

c46d311

Removed test keystore resources

6f8f504

jaymode approved these changes Jun 15, 2018

View reviewed changes

Shuffle with random()

50b433c

albertzaharovits merged commit 80f6d9e into elastic:reload-secure-store-action Jun 16, 2018

albertzaharovits deleted the ReloadIT branch June 16, 2018 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEST] Reload secure settings transport IT #31180

[TEST] Reload secure settings transport IT #31180

albertzaharovits commented Jun 7, 2018 •

edited

Loading

elasticmachine commented Jun 7, 2018

albertzaharovits Jun 7, 2018

albertzaharovits Jun 7, 2018

albertzaharovits Jun 11, 2018

jaymode Jun 13, 2018

jaymode left a comment

jaymode Jun 13, 2018

jaymode Jun 13, 2018

albertzaharovits Jun 13, 2018

jaymode Jun 13, 2018

jaymode Jun 13, 2018

albertzaharovits Jun 13, 2018

albertzaharovits commented Jun 13, 2018

jaymode left a comment

albertzaharovits commented Jun 15, 2018

jaymode left a comment

jaymode Jun 15, 2018

albertzaharovits commented Jun 16, 2018 •

edited

Loading


		public static class MisbehavingReloadablePlugin extends CountingReloadablePlugin {

		private volatile boolean sulky = false;

[TEST] Reload secure settings transport IT #31180

[TEST] Reload secure settings transport IT #31180

Conversation

albertzaharovits commented Jun 7, 2018 • edited Loading

elasticmachine commented Jun 7, 2018

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

jaymode 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jun 13, 2018

jaymode left a comment

Choose a reason for hiding this comment

albertzaharovits commented Jun 15, 2018

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jun 16, 2018 • edited Loading

albertzaharovits commented Jun 7, 2018 •

edited

Loading

albertzaharovits commented Jun 16, 2018 •

edited

Loading