Fix add string key command for passsword protected keystore #76124
Conversation
albertzaharovits
added
>bug
v8.0.0
:Core/Infra/CLI
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
@elasticmachine update branch |
|
I had to make code more visible than I'm comfortable with, especially for tests. |
|
I don't think we should do this change as is. It breaks open too many abstractions inside the cli/terminal classes, and utilizes exceptions for control flow. However, I have an alternative suggestion. The root of the problem, as you stated, is that different readers are used over the same input stream. Currently the SystemTerminal has a
|
|
Hi @albertzaharovits, I've created a changelog YAML for you. |
There was a problem hiding this comment.
I was going through my backlog today and finally got to this review. As I was reviewing this, I merged master into this code, so I'd be happy to update the branch with my merge commit if given permissions to push to it.
I'd like to get the basic fix in, but I have an issue with how we're handling the end-of-stream exception here.
We can do black-box tests of the keystore utility in the KeystoreManagementTests packaging test, though these tests can be a little tedious to implement.
| terminal.addSecretInput(password); | ||
| setInput("secret value 1"); | ||
| execute("-x", "foo"); | ||
| Terminal.SystemTerminal systemTerminal = new Terminal.SystemTerminal( |
There was a problem hiding this comment.
I see that the reason we aren't using MockTerminal here is that we need to construct a specific input stream for the terminal that doesn't work with MockTerminal's lists of strings to return. But if we are using Terminal for all our reads and not using System.in directly at all, then it seems like we are just indirectly testing features of the terminal code, and we could do this in TerminalTests. I don't know that we really need to dig into mainWithoutErrorHandling in these tests.
| @@ -259,7 +270,7 @@ public String readText(String text) { | |||
| try { | |||
| final String line = getReader().readLine(); | |||
| if (line == null) { | |||
| throw new IllegalStateException("unable to read from standard input; is standard input open and a tty attached?"); | |||
| throw EMPTY_LINE_EXCEPTION; | |||
There was a problem hiding this comment.
I don't think it's right to call this an EMPTY_LINE_EXCEPTION; the readLine() method returns an empty string for an empty line. It returns null when the end of the stream has been reached without reading any characters.
| return terminal.readSecret(""); | ||
| } catch (Exception e) { | ||
| if (e == EMPTY_LINE_EXCEPTION) { | ||
| return new char[0]; |
There was a problem hiding this comment.
This does not actually occur on an empty line, for example when there are two successive newlines in the input stream: val1\n\nval3\n. We get this exception when the stream is closed. So if a user runs echo bar1 | elasticsearch-keystore add-string -x foo1 foo2 foo3, we'll blank out the foo2 and foo3 settings instead of throwing an error. I'm not sure this is what we want to do in this case. If the user wants to zero out those values, they should be able to run something like (echo bar1 ; echo "" ; echo "") | elasticsearch-keystore add-string -x foo1 foo2 foo3.
I'm going to leave this up to you. I haven't had bandwidth to wrap it up, and it has gotten stale. Thank you for the effort to review. |
This fixes a bug for the add string key keystore command.
The bug manifests when the keystore is password protected and the string value comes from stdin (
-xcmd line option, rather than the prompt).The root cause is that the password to decrypt the keystore as well as the value to be stored in the keystore are both read from the stdin, but using different stream readers. This is a problem because the first reader consumes more than necessary from stdin, at the expense of the second reader.