Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autogenerate and print elastic pwd on startup #77291

Merged
merged 22 commits into from
Sep 8, 2021

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Sep 5, 2021

Introduces functionality to generate and set a password for the
elastic user during the initialization of the Security plugin if

  • bootstrap.password is not already set in the keystore
  • the security index doesn't already exist
  • and the password for the elastic user is not yet set ( the doc
    for the user doesn't exist in the security index )

This is printed in INFO level in the log, as we have already
wrapped System.out in Log4j and this is where stdout would be
printed either way. The password should not make it to the logs
and in a follow up we need to come up with a solution to this
problem.

Introduces functionality to generate and set a password for the
elastic user during the initilization of the Security plugin if

- `bootstrap.password` is not alredy set in the keystore
- the security index doesn't already exist
- and the password for the elastic user is not yet set ( the doc
for the user doesn't exist in the security index )

This is printed in INFO level in the log, as we have already
wrapped System.out in Log4j and this is where stdout would be
printed either way. The password should not make it to the logs
and in a follow up we need to come up with a solution to this
problem.
@jkakavas jkakavas added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 labels Sep 5, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Sep 5, 2021
@elasticmachine
Copy link
Collaborator

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

@jkakavas
Copy link
Member Author

jkakavas commented Sep 5, 2021

This is missing package tests, I plan to add some after/when we merge #77231, if we decide this is worth merging in its current format.

ElasticUser.NAME,
elasticPassword.getChars(),
ActionListener.wrap(
r -> {
Copy link
Member Author

@jkakavas jkakavas Sep 5, 2021

Choose a reason for hiding this comment

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

We could pass a reference to the original stdout PrintStream from Bootstrap to Node and then to the SecurityPlugin's createComponents so that we can temporarily call System.setOut to that , print to stdout and revert to log4j afterwards. This needs setIO RuntimePermission for the security plugin though and I'm not sure how comfortable I am with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you'd need to call System.setOut - you just need a reference to the PrintStream so you can use it.

What we need is for Bootstrap (or something close to it) to hold on to the original stdout in a place that we can get to it (but is on the forbidden APIs list).

Bootstrap already has code to close original stream if we're not in the foreground, so I think a change to add a SystemConsole class in org.elasticsearch.bootstrap is entirely feasible.

Do you want me to pull something together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why you'd need to call System.setOut - you just need a reference to the PrintStream so you can use it.

Yes, you are right. I misunderstood how we do it ( and how handling stdout works most probably ) in LogConfigurator, and I had presumed the PrintStream would be unusable.

Do you want me to pull something together?

That would be great

@tvernum
Copy link
Contributor

tvernum commented Sep 6, 2021

generate and set a password for the elastic user during the initialization of the Security plugin if

  • bootstrap.password is not already set in the keystore
  • the security index doesn't already exist
  • and the password for the elastic user is not yet set ( the doc for the user doesn't exist in the security index )

Do we intend, at a future time for there to be a setting that disables auto-configuration?

It feels like there might be use cases where a user wishes to not have a superuser password printed to the comsole, and we're currently saying the only way to achieve that is to set a bootstrap.password.
I think a way to opt-out of auto-security-configuration like this is reasonable.

@jkakavas
Copy link
Member Author

jkakavas commented Sep 6, 2021

generate and set a password for the elastic user during the initialization of the Security plugin if

  • bootstrap.password is not already set in the keystore
  • the security index doesn't already exist
  • and the password for the elastic user is not yet set ( the doc for the user doesn't exist in the security index )

Do we intend, at a future time for there to be a setting that disables auto-configuration?

It feels like there might be use cases where a user wishes to not have a superuser password printed to the comsole, and we're currently saying the only way to achieve that is to set a bootstrap.password.
I think a way to opt-out of auto-security-configuration like this is reasonable.

Yes, there is a relevant discussion in #77231 (comment). Even know we could do it with enrollment mode setting but I plan on adding the new setting in #77231

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I left a few comments, but I'm happy for you to merge when you and @BigPandaToo are happy.


final SecureString elasticPassword = new SecureString(generatePassword(20));
final SecureString kibanaSystemPassword = new SecureString(generatePassword(20));
final GroupedActionListener<Void> changePasswordListener = new GroupedActionListener<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a GroupedActionListener when you're handling the sequencing yourself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was actually unnecessary. I think I just misread the docs and got an incorrect impression that I could fire off the two async actions, wait for both to complete and then do the outputting. Thinking through this again, I could probably do that with a CountDownLatch, but I don't think there is anything inherently wrong with sequencing the two calls and output to terminal in the onResponse of the second

Copy link
Contributor

@BigPandaToo BigPandaToo left a comment

Choose a reason for hiding this comment

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

LGTM

@jkakavas
Copy link
Member Author

jkakavas commented Sep 7, 2021

I'll wait for a few hours in case we can merge #77299 which we can take advantage of in order to print to stdout only ( not to logs ) which is the desired end goal for this change

@rjernst
Copy link
Member

rjernst commented Sep 7, 2021

I'll wait for a few hours in case we can merge #77299

I left a suggestion in #77299 to use System.console() directly. I believe this will allow the desired behavior, to print only when in an interactive session, without needing any changes to the existing bootstrap code.

@jkakavas
Copy link
Member Author

jkakavas commented Sep 7, 2021

I left a suggestion in #77299 to use System.console() directly. I believe this will allow the desired behavior, to print only when in an interactive session, without needing any changes to the existing bootstrap code.

Sounds like a good idea. I'll give it a try here, do some manual testing and see if Packaging Tests are happy too.

UPDATE: Seems like System.console() returns null always ( #77299 (comment) ) not sure what causes that as this happens even early in Bootstrap code before we even touch standard out

@jkakavas
Copy link
Member Author

jkakavas commented Sep 7, 2021

I'll refrain from merging as-is for now just in case I am missing something obvious regarding #77291 (comment) and will revisit early in my morning in case there are any updates in #77299 and/or relevant discussion @tvernum @rjernst

@rjernst
Copy link
Member

rjernst commented Sep 7, 2021

Seems like System.console() returns null always

What environment was ES running in when it returned null? It will only return non-null if attached to an actual console.

@jkakavas
Copy link
Member Author

jkakavas commented Sep 7, 2021

What environment was ES running in when it returned null? It will only return non-null if attached to an actual console.

Archive installation on Ubuntu 20.04 , bin/elasticsearch run in the terminal, so definitely attached to an actual console

@tvernum
Copy link
Contributor

tvernum commented Sep 7, 2021

I've confirmed this is due to the way we pass the keystore password via stdin - it's no longer attached to a terminal, so the Console is null
More details here: #77299 (comment)

@jkakavas jkakavas merged commit 4250853 into elastic:master Sep 8, 2021
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Sep 8, 2021
In elastic#77291, we introduced the functionality to auto-generate a
password for the elastic user on startup. This means that the
setup-passwords tool cannot be used ( as it depends on the
password not being set ). This change fixes the QA project
test cluster to start with auto-configuration disabled
jkakavas added a commit that referenced this pull request Sep 8, 2021
In #77291, we introduced the functionality to auto-generate a
password for the elastic user on startup. This means that the
setup-passwords tool cannot be used ( as it depends on the
password not being set ). This change fixes the QA project
test cluster to start with auto-configuration disabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants