Add 'show' command to the keystore CLI by tvernum · Pull Request #76693 · elastic/elasticsearch

tvernum · 2021-08-19T05:03:29Z

This adds a new "elasticsearch-keystore show" command that displays
the value of a single secure setting from the keystore.

Additionally, support for direct (stream) output has been added to
the Terminal class so that non-UTF8 values can be written to
stdout, and accordingly this command works correctly:

elasticsearch-keystore show xpack.watcher.encryption_key > watcher.key

Resolves: #57261

This adds a new "elasticsearch-keystore show" command that displays the value of a single secure setting from the keystore. An optional `-o` (or `--output`) parameter can be used to direct output to a file. The `-o` option is required for binary keystore values because the CLI `Terminal` class does not support writing binary data. Hence this command: elasticsearch-keystore show xpack.watcher.encryption_key > watcher.key would not produce a file with the correct contents.

elasticmachine · 2021-08-19T05:03:31Z

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

tvernum · 2021-08-19T05:05:48Z

+| [remove <setting>]
+| [show [-o <output-file>] <setting>]
+| [upgrade]
+) [-h, --help] ([-s, --silent] | [-v, --verbose])


I did some reformatting here because it was getting hard to read. Happy to revert if people don't like it.

tvernum · 2021-08-19T05:07:17Z

+node.
+Any cChanges to the keystore will take effect when you restart {es}.
+Some secure settings can be explicitly <<reloadable-secure-settings, reloaded>>
+without restart.


I noticed that this content was out of date, so I added some more explanation (the wording is adapted from the [reloadable-secure-settings] docs section)

tvernum · 2021-08-19T06:00:46Z

+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.settings.cli;


I put this in the .setting.cli package to deal with the split packages check.
I can move it back to .settings instead, and update the exceptions list, but I think our preference is to use a new package.

However, I couldn't put the test into the setting.cli package, because it needs to access package protected methods on KeyStoreWrapper.

We should add a TestKeyStoreWrapper that inherits KeyStoreWrapper and makes those public for tests to call. It could be in a followup though.

I tried to test `show` in packaging tests, but it doesn't work correctly. In fact we don't even test `add` in packaging tests, so testing `show` isn't really necessary either

jkakavas

Added one comment and one suggestion ( prompt to overwrite ). I don't think we should block this PR for any of the above

jkakavas · 2021-08-19T07:47:23Z

+    }
+
+    private void show(KeyStoreWrapper keyStore, String setting, Terminal terminal) {
+        terminal.println(Terminal.Verbosity.SILENT, String.valueOf(keyStore.getString(setting)));


Any specific reason why we use String.valueOf to call the SecureString#toString here instead of doing toString ourselves? Just curious, happy eitherway

Force of habit mostly (for fear of NPEs).
terminal.println requires a string (not a CharSequence) so I have to explicitly call toString, I tend to favour valueOf for that since it's null safe.

Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

rjernst

The tests here are great, thank you!

Regarding output of binary files, I don't think we need to have an additional option.

There are two cases for Terminal: either we are connected to a Console, or we are not. We have the "system terminal" concept to simplify our interaction as most of the time with CLIs we are interacting with users. Since Console limits the developer to character data, we likewise limit our Terminal abstraction to character data.

I think we should do the following:

Read the key from the keystore as binary data, into memory. Keystore values are not large, by design. We can then use a utf8 validator (or for now just use a try/catch decoding to a string) to detect whether the data is utf8. This tells us whether it is character data.
If it is character data, print it using Terminal
If it is not character data, print it using System.out. However, for testability, we should add a getOutputStream() to Terminal, so that we can mock it out. Additionally, I think we can use that method to give an error message to the use if they are trying to write a binary value to the console. ConsoleTerminal.getOutputStream() can return null, and we can give an error to the user that it's binary data, and they should redirect to a file.

rjernst

LGTM, one small ask to ensure non UTF8 terminals work with this.

rjernst · 2021-09-03T00:07:24Z

+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.settings.cli;


We should add a TestKeyStoreWrapper that inherits KeyStoreWrapper and makes those public for tests to call. It could be in a followup though.

rjernst · 2021-09-03T00:15:32Z

+            final BytesReference bytes = org.elasticsearch.common.io.Streams.readFully(input);
+            final OutputStream output = terminal.getOutputStream();
+            if (output != null) {
+                bytes.writeTo(output);


This assumes that the terminal is using UTF8, which may not be the case. I think we should flip this around, so that we first try decoding utf8, and print if possible, then fall back to writing to the output stream (when we know the data is not utf8).

The reason I had it in this sequence is because in my (somewhat limited) testing, we never seem to have an output stream unless writing to a file, and if we're writing to a file then we probably want byte-equivalent output.

If we flip it around then

bin/elasticsearch-keystore add-file some.setting my-file1 bin/elasticsearch-keystore show some.setting > my-file2

might mean that my-file1 and my-file2 are not byte identical. At the very least we need to decide whether to append a newline or not (ideally with file output we wouldn't, with terminal output we would).

I'll make the switch in the sequence and select print vs println depending on whether the terminal has an output stream. It's not ideal, but I don't think there's a perfect answer.

tvernum · 2021-09-23T02:44:50Z

Argh. I left the old commit description in when I merged.
If anyone comes here because they're confused by the commit message - the commit message is wrong, the docs and PR description are correct.

This adds a new "elasticsearch-keystore show" command that displays the value of a single secure setting from the keystore. Additionally, support for direct (stream) output has been added to the Terminal class so that non-UTF8 values can be written to stdout, and accordingly this command works correctly: elasticsearch-keystore show xpack.watcher.encryption_key > watcher.key Backport of: elastic#76693 Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

This adds a new "elasticsearch-keystore show" command that displays the value of a single secure setting from the keystore. Additionally, support for direct (stream) output has been added to the Terminal class so that non-UTF8 values can be written to stdout, and accordingly this command works correctly: elasticsearch-keystore show xpack.watcher.encryption_key > watcher.key Backport of: #76693 Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com> Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

tvernum added >enhancement :Core/Infra/Settings Settings infrastructure and APIs v8.0.0 v7.16.0 labels Aug 19, 2021

tvernum requested review from jkakavas and rjernst August 19, 2021 05:03

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Aug 19, 2021

tvernum added 2 commits August 19, 2021 15:08

Merge branch 'master' into keystore-show

a568374

Precommit checks

644b988

tvernum commented Aug 19, 2021

View reviewed changes

Revert changes to packaging tests

ae8474a

I tried to test `show` in packaging tests, but it doesn't work correctly. In fact we don't even test `add` in packaging tests, so testing `show` isn't really necessary either

jkakavas approved these changes Aug 19, 2021

View reviewed changes

Update docs/reference/commands/keystore.asciidoc

b0aea8b

Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

rjernst reviewed Aug 20, 2021

View reviewed changes

tvernum added 5 commits August 24, 2021 16:00

Merge branch 'master' into keystore-show

e09b60a

Add binary output to Terminal class

2c62fb9

Merge remote-tracking branch 'tvernum/keystore-show' into keystore-show

4cfd8e4

Fix precommit

5aa6ba4

Merge branch 'master' into keystore-show

0bf7003

tvernum requested a review from rjernst August 30, 2021 08:04

rjernst approved these changes Sep 3, 2021

View reviewed changes

tvernum added 3 commits September 21, 2021 14:13

Merge branch 'master' into keystore-show

31ab832

Attempt UTF8 decoding first

b1928f1

Merge branch 'master' into keystore-show

f6dc250

tvernum merged commit 6125067 into elastic:master Sep 23, 2021

tvernum added the backport pending label Oct 5, 2021

tvernum mentioned this pull request Oct 5, 2021

[Backport] Add 'show' command to the keystore CLI #78661

Merged

tvernum removed the backport pending label Oct 6, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Uh oh!

Conversation

tvernum commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 19, 2021

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 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!

Uh oh!

Uh oh!

Uh oh!

rjernst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

tvernum commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tvernum commented Aug 19, 2021 •

edited

Loading

rjernst left a comment •

edited

Loading