Skip to content

Conversation

@tvernum
Copy link
Contributor

@tvernum tvernum commented Sep 6, 2021

In exceptional cases, there is a need for the ES process to print to
the user's "console" without the output appearing in log files.
An example is sensitive information such as the initial password for
an administrative user.

In these cases we would like to print to System.out instead of using
log4j.

However, we intentionally redirect stdout to go a log4j logger,
because that is the preffed place to capture the sorts of messages
that are typically printed to System.out

This commit makes two changes:

  1. The default Terminal instance is loaded during bootstrap so that
    it can make use of the original stdout (System.out) stream before
    that stream is redirect to the log file

  2. Adds a new field/method on BootstrapInfo to record whether the
    Terminal is useful. When daemonized, or in quiet mode,
    Elasticsearch closes the original output stream, so writing to the
    Terminal will fail. The new isTerminalOutputAvailable method tracks
    whether the terminal output is usable.

In exceptional cases, there is a need for the ES process to print to
the user's "console" without the output appearing in log files.
An example is sensitive information such as the initial password for
an administrative user.

In these cases we would like to print to System.out instead of using
log4j.

However, we intentionally redirect stdout to go a log4j logger,
because that is the preffed place to capture the sorts of messages
that are typically printed to System.out

This commit adds a new "ElasticsearchConsole" that retains a reference
to the original value of System.out and can be used for the very rare
cases where code should write to the console rather than the log file.

The ElasticsearchConsole is only initialised if Elasticsearch is
started in the foreground, without the --quiet CLI parameter
@tvernum tvernum added >enhancement :Core/Infra/Core Core issues without another label v8.0.0 v7.16.0 labels Sep 6, 2021
@tvernum tvernum requested review from jkakavas and rjernst September 6, 2021 08:22
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 6, 2021
@elasticmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor Author

tvernum commented Sep 6, 2021

@rjernst I think this is the sort of thing you had in mind when discussing moving the printing of the initial elastic password into the ES process.

I didn't include any tests because it really doesn't do anything that can be reasonably tests, and it will get real tests once it's used within security (but I wanted to factor it out into its own PR so it can be reviewed on its own)

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of reusing Terminal as we do in CLIs. It doesnt look like the logic about closing streams has changed, we would still have them open in the desired case because ES would be in the foreground. Am I missing something as to why we need another console wrapper?

@tvernum
Copy link
Contributor Author

tvernum commented Sep 6, 2021

My assumption was that we wouldn't want everything that terminal does (e.g. reading input) to end up mixed into non-CLI code and potentially need to be disentangled later.

I can make sure Terminal.DEFAULT is loaded before the streams are reassigned so that it can be used elsewhere.
We'll also need some method to retain the foreground and quiet flags so that we only use the Terminal if it's actually connected to a valid stream. BootstrapInfo.isTerminalAvailable ?

The other part of my concern is that Terminal isn't in any forbidden API lists, so it's valid for any code to reference Terminal.DEFAULT even though it's not guaranteed to work because System.out might have been closed.
At the moment no one does that because we simply don't use Terminal outside of a CLI tool, but we would be changing that.

Tomorrow, I'll update my PR to

  1. explicitly load the terminal before the LogConfigurator is called so that it has a reference to the right output stream
  2. add a field to BootstrapInfo (unless someone has a better suggestion) about whether a Terminal can be safely used.

In exceptional cases, there is a need for the ES process to print to
the user's "console" without the output appearing in log files.
An example is sensitive information such as the initial password for
an administrative user.

In these cases we would like to print to System.out instead of using
log4j.

However, we intentionally redirect stdout to go a log4j logger,
because that is the preffed place to capture the sorts of messages
that are typically printed to System.out

This commit makes two changes:

1. The default Terminal instance is loaded during bootstrap so that
   it can make use of the original stdout (System.out) stream before
   that stream is redirect to the log file

2. Adds a new field/method on BootstrapInfo to record whether the
   Terminal is useful. When daemonized, or in quiet mode,
   Elasticsearch closes the original output stream, so writing to the
   Terminal will fail. The new isTerminalOutputAvailable method tracks
   whether the terminal output is usable.
@rjernst
Copy link
Member

rjernst commented Sep 7, 2021

My assumption was that we wouldn't want everything that terminal does (e.g. reading input) to end up mixed into non-CLI code

You are right! I take back my suggestion, we should not use Terminal outside of cli.

We'll also need some method to retain the foreground and quiet flags

I wonder if we could do this without any additional state. We know that we want to write only the case that a console exists (ie interactive). Can we just grab System.console() directly in the security code? That returns null if there is no console OR if System.out has been closed (at least, it did in my testing against JDK 16). Even if it returned a console with System.out closed, we can catch the exceptional case and log a warning to the user that they need to run XYZ to get the join credentials.

@jkakavas
Copy link
Contributor

jkakavas commented Sep 7, 2021

Can we just grab System.console() directly in the security code? That returns null if there is no console OR if System.out has been closed (at least, it did in my testing against JDK 16).

I tried this out ( in foreground, not quiet) and I get System.console() returning null always even before we touch the streams in LogConfigurator.

@rjernst
Copy link
Member

rjernst commented Sep 7, 2021

foreground + not quiet doesn't guarantee a non-null console, but I believe the non-null console is what we ultimately want. It means that we are definitely attached to a terminal and not redirected to a file.

@tvernum
Copy link
Contributor Author

tvernum commented Sep 7, 2021

I think (without tracking it down) that we have a null console because stdin is reading from a heredoc (for keystore password)

@tvernum
Copy link
Contributor Author

tvernum commented Sep 7, 2021

I think (without tracking it down) that we have a null console because stdin is reading from a heredoc (for keystore password)

I've now confirmed this - the method by which we pass the keystore password cause stdin to not be the user's terminal, and that means System.console() is null.

"$@" <<<"$KEYSTORE_PASSWORD"

I suppose we could try and come up with a different method to pass in the keystore password, but it seems like a lot of work for not much value - nothing we're trying to do here cares about have console input, we just want to know if we have a reliable output stream to the console.

@rjernst
Copy link
Member

rjernst commented Sep 7, 2021

Fair enough, thanks for tracking that down.

If all we care about is being able to write to System.out, then could we catch the exceptional case (as I mentioned in a previous comment) when trying to write to System.out?

@tvernum
Copy link
Contributor Author

tvernum commented Sep 7, 2021

I wonder if we could do this without any additional state.

We can, provided we can retain a copy of System.out somewhere.

A combination of flush and checkError on the stream will tell us whether it's still open or not, which is a sufficient test for what we're doing here. But since System.console is null, we are dependent on having a place where we can get to the original System.out.

Terminal works (as long as it's loaded before the stream gets redirected) but if we don't want to use Terminal, we'll need something else.

@rjernst
Copy link
Member

rjernst commented Sep 8, 2021

Ok, how about we use BootstrapInfo to stash the original PrintStream, instead of relying on the Terminal+flag?

@tvernum
Copy link
Contributor Author

tvernum commented Sep 21, 2021

Replaced by #77460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >enhancement Team:Core/Infra Meta label for core/infra team v7.16.0 v8.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants