Fix ReloadSecureSettings API to consume password #54771

jkakavas · 2020-04-05T13:42:32Z

The secure_settings_password was never taken into consideration in
the ReloadSecureSettings API. This commit fixes that and adds
necessary REST layer testing. Doing so, it also

Allows TestClusters to have a password protected keystore
so that it can be set for tests.
Adds a parameter to the run task so that elastisearch can
be run with a password protected keystore from source.

The secure_settings_password was never taken into consideration in the ReloadSecureSettings API. This commit fixes that and adds necessary REST layer testing. Doing so, it also - Allows TestClusters to have a password protected keystore so that it can be set for tests. - Adds a parameter to the run task so that elastisearch can be run with a password protected keystore. -

elasticmachine · 2020-04-05T13:42:34Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

elasticmachine · 2020-04-05T13:42:36Z

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

jkakavas · 2020-04-05T14:04:50Z

...rc/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java

        request.withContentOrSourceParamParserOrNull(parser -> {
            if (parser != null) {
-                final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request();
+                final NodesReloadSecureSettingsRequest nodesRequest = PARSER.parse(parser, null);


This is the result of a master merge gone wrong in the feature branch : 4780880#diff-0187f4b109fbd7256c987cceae7691caL76

This is the actual fix and the only change necessary. If the buildSrc changes need further discussion, we could just merge the fix and the rest of the test changes when ready.

ywangd · 2020-04-06T00:59:35Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

+        if (keystorePassword != null && keystorePassword.length() > 0) {
+            try {
+                Files.write(esStdinFile, (keystorePassword + "\n").getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE);
+                processBuilder.redirectInput(esStdinFile.toFile());


nits: the redirectInput call can be placed outside of the try block.

what do we gain by doing so? If Files#write throws, we would still fail with aTestClustersException before we reach redirectInput`

It makes no difference in actual execution outcome for this particular scenario. It's a choice of style, keep error handling specific and accurate to what is targeting for. It's akin to formatting and method extraction, but a bit more: if future changes make the extra method calls throw the same exception, it will not be handled automatically and potentially unwanted by the broader-than-necessary try-catch block. It does not apply to this particular case, that's why I called it "nits". But woud anyhow prefer keeping any context as small as possible.

tvernum · 2020-04-06T00:34:28Z

docs/reference/cluster/nodes-reload-secure-settings.asciidoc

+  "secure_settings_password": "s3cr3t"
 }
 --------------------------------------------------
 // NOTCONSOLE


Why are these NOTCONSOLE? They look console to me.

I realise it's existing, but it seems like this docs issue is due in part to these not being treated as testable snippets.

Because I forgot to enable them :) When this was first introduced, I think NOTCONSOLE was there to cater for the fact that we couldn't set the keystore password in a proper way in the docs cluster

tvernum · 2020-04-06T01:18:42Z

rest-api-spec/src/main/resources/rest-api-spec/api/nodes.reload_secure_settings.json

      }
+    },
+    "body": {
+      "description": "The password for the elasticsearch keystore",


This implies that the body should be the raw password, I think we want something more like:

Suggested change

"description": "The password for the elasticsearch keystore",

"description": "An object containing password for the elasticsearch keystore",

tvernum · 2020-04-06T01:20:47Z

rest-api-spec/src/main/resources/rest-api-spec/test/nodes.reload_secure_settings/10_basic.yml

  - is_true: cluster_name
+  - match: { nodes.$node_id.reload_exception.type: "security_exception" }
+  - match: { nodes.$node_id.reload_exception.reason: "Provided keystore password was incorrect" }
+


Can we have a version with no password as well?
That should fail with the same error, but I think it's worth a test that it's not failing on "password is required" or a NPE, etc.

jkakavas · 2020-04-06T14:24:52Z

@mark-vieira , @williamrandolph I'm not sure what is the best way to set keystorePassword is dor multiple qa projects that need this, open to ideas and suggestions. It's either we set a keystore password in all our test clusters ( on one hand why not, on the other this is not our default behavior ) we try to figure out all the tests that would run rest-api-spec/test/nodes.reload_secure_settings/10_basic.yml ( do we have a way to do this? ) and set the keystore password only for those test clusters

…luster that doesn't have transport TLS setup

…ngs-fix

jkakavas · 2020-04-06T16:20:47Z

Chatted with @rjernst about this, I will remove the keystore password from the core rest tests and I will add a new QA project with an ESRestTestCase test for reloading secure settings with a password protected keystore

…ngs-fix

jkakavas · 2020-04-07T13:37:56Z

x-pack/qa/password-protected-keystore/build.gradle

@@ -0,0 +1,52 @@
+/*


The reason this is created in x-pack is that calling the reload settings api with a password require transport SSL to be configured. We could move this to qa and run with OSS too, if we limit this to an 1 node cluster

jkakavas · 2020-04-07T15:11:22Z

@elasticmachine update branch

rjernst

A few suggestions, otherwise LGTM

rjernst · 2020-04-07T17:19:32Z

build.gradle

  }
+
+  @Option(
+    option = "keystore-password",


While I'm not opposed to this, I wonder how useful it is in practice since we have no ability to set values in the keystore via the run task?

Yes, I'm not sold on it either. And given that we currently don't have a way to set keystore properties in the run task, maybe it's not worth adding ? I don't know , it seemed like a good idea when I added it but I'm not so sure now

I assume this is only relevant when compiled with the --data-dir option to point at an existing directory which might have a manually setup keystore?

rjernst · 2020-04-07T17:26:04Z

...rch/password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java

+
+import java.util.Map;
+
+public class ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT extends ESRestTestCase {


Could this just be a yaml test?

I find these easier to write, debug and argue about so if you don't have a strong view on why we should make them a yaml test, I'd prefer to keep this

rjernst · 2020-04-07T17:28:41Z

x-pack/qa/password-protected-keystore/src/test/resources/ssl/ca.crt

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----


I'm just curious: we used to generate these types of materials within build.gradle in other qa tests: why are we now committing them instead of generating on the fly?

We used to depend on BouncyCastle to generate keys and certificates as JDK doesn't expose the necessary APIs for the certificate generation (https://bugs.openjdk.java.net/browse/JDK-8058778) and we removed BouncyCastle dependency as part of the FIPS 140 effort

jkakavas · 2020-04-08T11:27:14Z

@elasticmachine update branch

jkakavas · 2020-04-09T12:26:02Z

@mark-vieira , @williamrandolph any thoughts on #54771 (comment) or any other feedback ? If not, I'll merge this since @rjernst LGTM'd it

williamrandolph · 2020-04-10T13:12:47Z

This looks good to me — thanks for adding this coverage!

The secure_settings_password was never taken into consideration in the ReloadSecureSettings API. This commit fixes that and adds necessary REST layer testing. Doing so, it also - Allows TestClusters to have a password protected keystore so that it can be set for tests. - Adds a parameter to the run task so that elastisearch can be run with a password protected keystore from source.

The secure_settings_password was never taken into consideration in the ReloadSecureSettings API. This commit fixes that and adds necessary REST layer testing. Doing so, it also: - Allows TestClusters to have a password protected keystore so that it can be set for tests. - Adds a parameter to the run task so that elastisearch can be run with a password protected keystore from source.

jkakavas added 3 commits April 5, 2020 16:21

Fix spotless

3beb92c

remove extra logging

d44163d

jkakavas added >bug :Delivery/Build Build or test infrastructure :Security/Security Security issues without another label v8.0.0 v7.7.0 labels Apr 5, 2020

jkakavas requested review from mark-vieira and williamrandolph April 5, 2020 13:42

Create the keystore even if no password is set

92c6cfb

jkakavas commented Apr 5, 2020

View reviewed changes

jkakavas added 2 commits April 5, 2020 17:22

set the keystore password where needed

6ad7dfb

set password for all tests

ed1cc00

ywangd reviewed Apr 6, 2020

View reviewed changes

tvernum reviewed Apr 6, 2020

View reviewed changes

address feedback

e27bddf

jkakavas added 3 commits April 6, 2020 18:41

Call the API locally in tests as there might be the case we're in a c…

aa5358f

…luster that doesn't have transport TLS setup

correctly set the keystore password for integ-test-zip

8427317

Merge remote-tracking branch 'origin/master' into reload-secure-setti…

146eefd

…ngs-fix

jkakavas added 6 commits April 7, 2020 16:08

add QA project

ee31279

revert changes to existing core rest tests

78c7b39

fix license

5947a16

fix spec wording

2b63231

remove unecessary settings

9bb7583

Merge remote-tracking branch 'origin/master' into reload-secure-setti…

5233d90

…ngs-fix

jkakavas commented Apr 7, 2020

View reviewed changes

jkakavas added 2 commits April 7, 2020 16:50

fix yaml test

cf6315a

remove redundant leftover from applying stash

f07b504

Merge branch 'master' into reload-secure-settings-fix

c4a8496

rjernst approved these changes Apr 7, 2020

View reviewed changes

Merge branch 'master' into reload-secure-settings-fix

f740fd9

williamrandolph approved these changes Apr 10, 2020

View reviewed changes

jkakavas merged commit 16e9433 into elastic:master Apr 10, 2020

jkakavas mentioned this pull request Apr 10, 2020

[7.x] Fix ReloadSecureSettings API to consume password (#54771) #55059

Merged

jkakavas mentioned this pull request Apr 10, 2020

[7.7] Fix ReloadSecureSettings API to consume password (#54771) #55060

Merged

Mpdreamz mentioned this pull request Apr 16, 2020

Regnerate from new specifications elastic/elasticsearch-net#4602

Merged

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

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

	"description": "The password for the elasticsearch keystore",
	"description": "An object containing password for the elasticsearch keystore",


		import java.util.Map;

		public class ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT extends ESRestTestCase {

Fix ReloadSecureSettings API to consume password #54771

Fix ReloadSecureSettings API to consume password #54771

Uh oh!

Conversation

jkakavas commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 5, 2020

Uh oh!

elasticmachine commented Apr 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Apr 6, 2020

Uh oh!

jkakavas commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Apr 7, 2020

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Apr 8, 2020

Uh oh!

jkakavas commented Apr 9, 2020

Uh oh!

williamrandolph commented Apr 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jkakavas commented Apr 5, 2020 •

edited

Loading