Password-protected Keystore Feature Branch PR #51123

williamrandolph · 2020-01-16T19:07:43Z

After months of back-and-forth, here's the PR for the password-protected keystore feature.

Description

Elasticsearch uses a custom on-disk keystore for secure settings such as passwords and SSL certificates. Up until now, this prevented users with command-line access from viewing secure files by listing commands, but nothing prevented such users from changing values in the keystore, or removing values from it. Furthermore, the values were only obfuscated by a hash; no user-specific secret protected the secure settings.

This PR changes all of that by adding password-protection to the keystore. This will not be a breaking change for users. If a keystore has no password, there won’t be any new prompts. A user must choose to password-protect their keystore in order to see any new behavior.

This is a large feature branch, but the major pieces of production code are straightforward:

The keystore CLI has been extended to allow users to create a password-protected keystore, to add password-protection to an existing keystore, and to change or remove a password from an existing keystore. Once a keystore has a password, the CLI will prompt for a password when listing, adding, or removing secure settings. There are new subcommands for the keystore CLI: “passwd” is for changing a keystore’s password, and “has-passwd” is for checking whether a keystore is password-protected. The common code for password handling in keystore CLI commands is contained in a new BaseKeystoreCommand class.
The Elasticsearch startup scripts for Windows and Bash have been modified to read passwords on standard input. In the systemd case, a small entrypoint script can read the password from a file and pass it to the startup script. In the Docker case, the Docker entrypoint script can retrieve a password from an environment variable and provide it to the Elasticsearch startup script.
The REST API command for reloading secure settings now requires a password when a node’s keystore is password-protected. A call to the reload_secure_settings with a password is allowed in two cases: (1) When run against the local node ( single node cluster, or specifically targeting the local cluster via _local filter or nodeId) regardless of the TLS configuration in the transport layer. (2) When run for more nodes than the local one (some or all cluster) iff TLS is enabled in the transport layer.
The Command and Terminal classes have been modified to support finer-grained control over reading from standard input and writing errors to the terminal. Specifically, we read passwords into character arrays (wrapped as “secure strings”) up to a maximum length of 128 (arbitrarily chosen).
Much of the new code in this PR is test code. Many of our already-existing keystore unit tests now use password-protected keystrokes. We also have extensive “qa” tests: a “KeystoreManagementTests” class that can run for both archive (zip, tar.gz), package (rpm, deb), and Docker distributions as part of the packaging test matrix.

There is one relevant bit of outstanding work: #49340. This feature branch includes two integration tests that use expect to run the Elasticsearch command with an interactive terminal, as opposed to merely piping input to a non-interactive shell. However, we have only been able to install expect on Linux test instances so far, so we don't yet have Windows parity. This work is blocked by an outstanding ticket for Infra.

Closes #32691

* Reload secure settings with password (#43197) If a password is not set, we assume an empty string to be compatible with previous behavior. Only allow the reload to be broadcast to other nodes if TLS is enabled for the transport layer. * Add passphrase support to elasticsearch-keystore (#38498) This change adds support for keystore passphrases to all subcommands of the elasticsearch-keystore cli tool and adds a subcommand for changing the passphrase of an existing keystore. The work to read the passphrase in Elasticsearch when loading, which will be addressed in a different PR. Subcommands of elasticsearch-keystore can handle (open and create) passphrase protected keystores When reading a keystore, a user is only prompted for a passphrase only if the keystore is passphrase protected. When creating a keystore, a user is allowed (default behavior) to create one with an empty passphrase Passphrase can be set to be empty when changing/setting it for an existing keystore Relates to: #32691 Supersedes: #37472 * Restore behavior for force parameter (#44847) Turns out that the behavior of `-f` for the add and add-file sub commands where it would also forcibly create the keystore if it didn't exist, was by design - although undocumented. This change restores that behavior auto-creating a keystore that is not password protected if the force flag is used. The force OptionSpec is moved to the BaseKeyStoreCommand as we will presumably want to maintain the same behavior in any other command that takes a force option. * Handle pwd protected keystores in all CLI tools (#45289) This change ensures that `elasticsearch-setup-passwords` and `elasticsearch-saml-metadata` can handle a password protected elasticsearch.keystore. For setup passwords the user would be prompted to add the elasticsearch keystore password upon running the tool. There is no option to pass the password as a parameter as we assume the user is present in order to enter the desired passwords for the built-in users. For saml-metadata, we prompt for the keystore password at all times even though we'd only need to read something from the keystore when there is a signing or encryption configuration. * Modify docs for setup passwords and saml metadata cli (#45797) Adds a sentence in the documentation of `elasticsearch-setup-passwords` and `elasticsearch-saml-metadata` to describe that users would be prompted for the keystore's password when running these CLI tools, when the keystore is password protected. Co-Authored-By: Lisa Cawley <[email protected]> * Elasticsearch keystore passphrase for startup scripts (#44775) This commit allows a user to provide a keystore password on Elasticsearch startup, but only prompts when the keystore exists and is encrypted. The entrypoint in Java code is standard input. When the Bootstrap class is checking for secure keystore settings, it checks whether or not the keystore is encrypted. If so, we read one line from standard input and use this as the password. For simplicity's sake, we allow a maximum passphrase length of 128 characters. (This is an arbitrary limit and could be increased or eliminated. It is also enforced in the keystore tools, so that a user can't create a password that's too long to enter at startup.) In order to provide a password on standard input, we have to account for four different ways of starting Elasticsearch: the bash startup script, the Windows batch startup script, systemd startup, and docker startup. We use wrapper scripts to reduce systemd and docker to the bash case: in both cases, a wrapper script can read a passphrase from the filesystem and pass it to the bash script. In order to simplify testing the need for a passphrase, I have added a has-passwd command to the keystore tool. This command can run silently, and exit with status 0 when the keystore has a password. It exits with status 1 if the keystore doesn't exist or exists and is unencrypted. A good deal of the code-change in this commit has to do with refactoring packaging tests to cleanly use the same tests for both the "archive" and the "package" cases. This required not only moving tests around, but also adding some convenience methods for an abstraction layer over distribution-specific commands. I will write some user-facing documentation for these changes in a follow-up commit. * Adjust docs for password protected keystore (#45054) This commit adds relevant parts in the elasticsearch-keystore sub-commands reference docs and in the reload secure settings API doc. * Cleanup after feature branch reconstruction The feature branch for the password-protected keystore, due to an accident, contains a large number of unrelated commits. In order to get a cleaner merge, I've cherry-picked the main commits that went into the feature branch against a branch derived from master — essentially, a rebase onto master. We've ignored some tests that will addressed in follow-up PRs to the feature branch.

One problem with the passphrase-from-file tests, as written, is that they would leave a SystemD environment variable set when they failed, and this setting would cause elasticsearch startup to fail for other tests as well. By using a try-finally, I hope that these tests will fail more gracefully. It appears that our Fedora and Ubuntu environments may be configured to store journald information under /var rather than under /run, so that it will persist between boots. Our destructive tests that read from the journal need to account for this in order to avoid trying to limit the output we check in tests.

* Add Docker handling to PackagingTestCase Keystore tests need to be able to run in the Docker case. We can do this by using a DockerShell instead of a plain Shell when Docker is running. * Improve ES startup check for docker Previously we were checking truncated output for the packaged JDK as an indication that Elasticsearch had started. With new preliminary password checks, we might get a false positive from ES keystore commands, so we have to check specifically that the Elasticsearch class from the Bootstrap package is what's running.

This commit adds two tests for the case where we mount a password-protected keystore into a Docker container and provide a password via a Docker environment variable. We also fix a logging bug where we were logging the identifier for an array of strings rather than the contents of that array.

When a keystore is password-protected, Elasticsearch will prompt at startup. This commit adds documentation for this prompt for the archive, systemd, and Docker cases. Co-authored-by: Lisa Cawley <[email protected]>

For Red Hat RPM upgrades, we warn if we can't upgrade the keystore. This commit brings the same logic to the code for Debian packages. See the posttrans file for gets executed for RPMs.

…/elasticsearch into feature-pwd-protected-keystore-2

elasticmachine · 2020-01-16T19:07:46Z

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

elasticmachine · 2020-01-16T19:07:48Z

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

williamrandolph · 2020-01-16T22:27:18Z

@elasticmachine run elasticsearch-ci/2

rjernst

I only did a cursory review since I've already looked at most of the PRs going into this branch, but this LGTM. Great work having all this behavior, tests and docs covered!

jkakavas

This LGTM ! However, as there are parts in here that I've written and parts that fall too much in the core-infra side ( docker, bash scripts ), I think it would make sense for another person from es-core-infra to make a pass as the second reviewer

pugnascotia · 2020-01-17T14:23:38Z

distribution/packages/src/common/scripts/postinst

-        /usr/share/elasticsearch/bin/elasticsearch-keystore upgrade
+        if /usr/share/elasticsearch/bin/elasticsearch-keystore has-passwd --silent ; then
+          echo "### Warning: unable to upgrade encrypted keystore" 1>&2
+          echo " Please run elasticsearch-keystore upgrade and enter password" 1>&2


Tiny nit / observation - is it worth putting the command in quotes?

Suggested change

echo " Please run elasticsearch-keystore upgrade and enter password" 1>&2

echo " Please run 'elasticsearch-keystore upgrade' and enter password" 1>&2

pugnascotia · 2020-01-17T14:36:37Z

distribution/src/bin/elasticsearch

+# conflicting GC configurations for the keystore tools
+unset KEYSTORE_PASSWORD
+KEYSTORE_PASSWORD=
+if ! echo $* | grep -E -q '(^-h |-h$| -h |--help$|--help |^-V |-V$| -V |--version$|--version )' \


This feels a bit fragile to me. What about something like:

CHECK_KEYSTORE=false for option in "$@"; do case "$option" in -h|--help|-V|--version) CHECK_KEYSTORE=true ;; esac done if [[ $CHECK_KEYSTORE == true ]] \ && "`dirname "$0"`"/elasticsearch-keystore has-passwd --silent # etc

I copied a pattern checking for daemonization below. I'd like to put this off for a follow-up PR so it can be discussed with some other team members in isolation. That being said I don't have any objection to doing a loop over options and it seems to work just fine in my quick local tests.

pugnascotia · 2020-01-17T14:42:50Z

...ools/keystore-cli/src/main/java/org/elasticsearch/common/settings/CreateKeyStoreCommand.java

+            readPassword(terminal, true) : new SecureString(new char[0])) {
+            Path keystoreFile = KeyStoreWrapper.keystorePath(env.configFile());
+            if (Files.exists(keystoreFile)) {
+                if (terminal.promptYesNo("An elasticsearch keystore already exists. Overwrite?", false) == false) {


Should be we capitalizing "elasticsearch" here, and elsewhere?

Suggested change

if (terminal.promptYesNo("An elasticsearch keystore already exists. Overwrite?", false) == false) {

if (terminal.promptYesNo("An Elasticsearch keystore already exists. Overwrite?", false) == false) {

I think we use non-capitalized elasticsearch everywhere else in our logs, prompts ,etc.

When we get Elasticsearch logs from journald, we want to fetch only log messages from the last run. There are two reasons for this. First, if there are many logs, we might get a string that's too large for our utility methods. Second, when we're looking for a specific message or error, we almost certainly want to look only at messages from the last execution. Previously, we've been trying to do this by clearing out the physical files under the journald process. But there seems to be some contention over these directories: if journald writes a log file in between when our deletion command deletes the file and when it deletes the log directory, the deletion will fail. It seems to me that we might be able to use journald's "--since" flag to retrieve only log messages from the last run, and that this might be less likely to fail due to race conditions in file deletion. Unfortunately, it looks as if the "--since" flag has a granularity of one-second. I've added a two-second sleep to make sure that there's a sufficient gap between the test that will read from journald and the test before it.

williamrandolph · 2020-01-23T20:08:58Z

@elasticmachine run elasticsearch-ci/1

@elasticmachine run elasticsearch-ci/2

* Reload secure settings with password (#43197) If a password is not set, we assume an empty string to be compatible with previous behavior. Only allow the reload to be broadcast to other nodes if TLS is enabled for the transport layer. * Add passphrase support to elasticsearch-keystore (#38498) This change adds support for keystore passphrases to all subcommands of the elasticsearch-keystore cli tool and adds a subcommand for changing the passphrase of an existing keystore. The work to read the passphrase in Elasticsearch when loading, which will be addressed in a different PR. Subcommands of elasticsearch-keystore can handle (open and create) passphrase protected keystores When reading a keystore, a user is only prompted for a passphrase only if the keystore is passphrase protected. When creating a keystore, a user is allowed (default behavior) to create one with an empty passphrase Passphrase can be set to be empty when changing/setting it for an existing keystore Relates to: #32691 Supersedes: #37472 * Restore behavior for force parameter (#44847) Turns out that the behavior of `-f` for the add and add-file sub commands where it would also forcibly create the keystore if it didn't exist, was by design - although undocumented. This change restores that behavior auto-creating a keystore that is not password protected if the force flag is used. The force OptionSpec is moved to the BaseKeyStoreCommand as we will presumably want to maintain the same behavior in any other command that takes a force option. * Handle pwd protected keystores in all CLI tools (#45289) This change ensures that `elasticsearch-setup-passwords` and `elasticsearch-saml-metadata` can handle a password protected elasticsearch.keystore. For setup passwords the user would be prompted to add the elasticsearch keystore password upon running the tool. There is no option to pass the password as a parameter as we assume the user is present in order to enter the desired passwords for the built-in users. For saml-metadata, we prompt for the keystore password at all times even though we'd only need to read something from the keystore when there is a signing or encryption configuration. * Modify docs for setup passwords and saml metadata cli (#45797) Adds a sentence in the documentation of `elasticsearch-setup-passwords` and `elasticsearch-saml-metadata` to describe that users would be prompted for the keystore's password when running these CLI tools, when the keystore is password protected. Co-Authored-By: Lisa Cawley <[email protected]> * Elasticsearch keystore passphrase for startup scripts (#44775) This commit allows a user to provide a keystore password on Elasticsearch startup, but only prompts when the keystore exists and is encrypted. The entrypoint in Java code is standard input. When the Bootstrap class is checking for secure keystore settings, it checks whether or not the keystore is encrypted. If so, we read one line from standard input and use this as the password. For simplicity's sake, we allow a maximum passphrase length of 128 characters. (This is an arbitrary limit and could be increased or eliminated. It is also enforced in the keystore tools, so that a user can't create a password that's too long to enter at startup.) In order to provide a password on standard input, we have to account for four different ways of starting Elasticsearch: the bash startup script, the Windows batch startup script, systemd startup, and docker startup. We use wrapper scripts to reduce systemd and docker to the bash case: in both cases, a wrapper script can read a passphrase from the filesystem and pass it to the bash script. In order to simplify testing the need for a passphrase, I have added a has-passwd command to the keystore tool. This command can run silently, and exit with status 0 when the keystore has a password. It exits with status 1 if the keystore doesn't exist or exists and is unencrypted. A good deal of the code-change in this commit has to do with refactoring packaging tests to cleanly use the same tests for both the "archive" and the "package" cases. This required not only moving tests around, but also adding some convenience methods for an abstraction layer over distribution-specific commands. * Adjust docs for password protected keystore (#45054) This commit adds relevant parts in the elasticsearch-keystore sub-commands reference docs and in the reload secure settings API doc. * Fix failing Keystore Passphrase test for feature branch (#50154) One problem with the passphrase-from-file tests, as written, is that they would leave a SystemD environment variable set when they failed, and this setting would cause elasticsearch startup to fail for other tests as well. By using a try-finally, I hope that these tests will fail more gracefully. It appears that our Fedora and Ubuntu environments may be configured to store journald information under /var rather than under /run, so that it will persist between boots. Our destructive tests that read from the journal need to account for this in order to avoid trying to limit the output we check in tests. * Run keystore management tests on docker distros (#50610) * Add Docker handling to PackagingTestCase Keystore tests need to be able to run in the Docker case. We can do this by using a DockerShell instead of a plain Shell when Docker is running. * Improve ES startup check for docker Previously we were checking truncated output for the packaged JDK as an indication that Elasticsearch had started. With new preliminary password checks, we might get a false positive from ES keystore commands, so we have to check specifically that the Elasticsearch class from the Bootstrap package is what's running. * Test password-protected keystore with Docker (#50803) This commit adds two tests for the case where we mount a password-protected keystore into a Docker container and provide a password via a Docker environment variable. We also fix a logging bug where we were logging the identifier for an array of strings rather than the contents of that array. * Add documentation for keystore startup prompting (#50821) When a keystore is password-protected, Elasticsearch will prompt at startup. This commit adds documentation for this prompt for the archive, systemd, and Docker cases. Co-authored-by: Lisa Cawley <[email protected]> * Warn when unable to upgrade keystore on debian (#51011) For Red Hat RPM upgrades, we warn if we can't upgrade the keystore. This commit brings the same logic to the code for Debian packages. See the posttrans file for gets executed for RPMs. * Restore handling of string input Adds tests that were mistakenly removed. One of these tests proved we were not handling the the stdin (-x) option correctly when no input was added. This commit restores the original approach of reading stdin one char at a time until there is no more (-1, \r, \n) instead of using readline() that might return null * Apply spotless reformatting * Use '--since' flag to get recent journal messages When we get Elasticsearch logs from journald, we want to fetch only log messages from the last run. There are two reasons for this. First, if there are many logs, we might get a string that's too large for our utility methods. Second, when we're looking for a specific message or error, we almost certainly want to look only at messages from the last execution. Previously, we've been trying to do this by clearing out the physical files under the journald process. But there seems to be some contention over these directories: if journald writes a log file in between when our deletion command deletes the file and when it deletes the log directory, the deletion will fail. It seems to me that we might be able to use journald's "--since" flag to retrieve only log messages from the last run, and that this might be less likely to fail due to race conditions in file deletion. Unfortunately, it looks as if the "--since" flag has a granularity of one-second. I've added a two-second sleep to make sure that there's a sufficient gap between the test that will read from journald and the test before it. * Use new journald wrapper pattern * Update version added in secure settings request Co-authored-by: Lisa Cawley <[email protected]> Co-authored-by: Ioannis Kakavas <[email protected]>

) PR #77155 updated the keystore instructions for Docker. However, it removed an example that included the `KEYSTORE_PASSWORD` env variable. This replaces a docker compose example with the original example from PR #51123.

) (#77592) PR #77155 updated the keystore instructions for Docker. However, it removed an example that included the `KEYSTORE_PASSWORD` env variable. This replaces a docker compose example with the original example from PR #51123.

) (#77591) PR #77155 updated the keystore instructions for Docker. However, it removed an example that included the `KEYSTORE_PASSWORD` env variable. This replaces a docker compose example with the original example from PR #51123. Co-authored-by: Elastic Machine <[email protected]>

) (#77590) PR #77155 updated the keystore instructions for Docker. However, it removed an example that included the `KEYSTORE_PASSWORD` env variable. This replaces a docker compose example with the original example from PR #51123. Co-authored-by: Elastic Machine <[email protected]>

williamrandolph and others added 14 commits December 11, 2019 17:05

Merging master to feature branch

fab3d56

Merge branch 'master' into feature-pwd-protected-keystore-2

d64ab04

Merge master into feature branch

6695e53

Merging master into feature branch

1c656f2

Merging changes from master

93ca7e6

Add documentation for keystore startup prompting (#50821)

663fc0b

When a keystore is password-protected, Elasticsearch will prompt at startup. This commit adds documentation for this prompt for the archive, systemd, and Docker cases. Co-authored-by: Lisa Cawley <[email protected]>

Merge branch 'master' into feature-pwd-protected-keystore-2

91cb2c9

Warn when unable to upgrade keystore on debian (#51011)

6238d85

For Red Hat RPM upgrades, we warn if we can't upgrade the keystore. This commit brings the same logic to the code for Debian packages. See the posttrans file for gets executed for RPMs.

Merge master into feature branch

75bdbdd

Merge branch 'feature-pwd-protected-keystore-2' of github.com:elastic…

0b2d5d1

…/elasticsearch into feature-pwd-protected-keystore-2

williamrandolph added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.7.0 labels Jan 16, 2020

williamrandolph requested review from rjernst, jkakavas and lcawl January 16, 2020 19:07

Increase Docker startup wait time for tests

81458e7

rjernst approved these changes Jan 16, 2020

View reviewed changes

jkakavas approved these changes Jan 17, 2020

View reviewed changes

pugnascotia reviewed Jan 17, 2020

View reviewed changes

williamrandolph added 5 commits January 22, 2020 16:31

Respond to PR comments

f61d756

Stop journald before trying to delete its files

9508733

Adjust clearing the journalctl logs

936ba91

Merging changes from master

8ede626

williamrandolph added 4 commits January 23, 2020 15:19

Use --since when reading journal

2f82284

Merging changes from master

6757785

Use new journald wrapper pattern

f93ab59

Update version added in secure settings request

a8103a6

williamrandolph merged commit c117c0c into master Jan 28, 2020

williamrandolph added the backport pending label Jan 28, 2020

This was referenced Jan 28, 2020

Reënable bwc tests #51522

Merged

Use an options loop in Elasticsearch startup script #51547

Merged

Use standard format for reload settings API #51560

Merged

williamrandolph removed the backport pending label Jan 31, 2020

jasontedor added the release highlight label Feb 20, 2020

williamrandolph mentioned this pull request Mar 17, 2020

Increase timeout for ES startup checks on Docker #53680

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

williamrandolph deleted the feature-pwd-protected-keystore-2 branch May 6, 2020 21:48

seut mentioned this pull request Jun 11, 2020

Re-enable IAM Roles for s3 repositories crate/crate#10071

Merged

5 tasks

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

jrodewig mentioned this pull request Sep 10, 2021

[DOCS] Re-add KEYSTORE_PASSWORD example to Docker install docs #77588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password-protected Keystore Feature Branch PR #51123

Password-protected Keystore Feature Branch PR #51123

williamrandolph commented Jan 16, 2020

elasticmachine commented Jan 16, 2020

elasticmachine commented Jan 16, 2020

williamrandolph commented Jan 16, 2020

rjernst left a comment

jkakavas left a comment

pugnascotia Jan 17, 2020

pugnascotia Jan 17, 2020

williamrandolph Jan 22, 2020

pugnascotia Jan 17, 2020

jkakavas Jan 22, 2020

williamrandolph commented Jan 23, 2020

	echo " Please run elasticsearch-keystore upgrade and enter password" 1>&2
	echo " Please run 'elasticsearch-keystore upgrade' and enter password" 1>&2

	if (terminal.promptYesNo("An elasticsearch keystore already exists. Overwrite?", false) == false) {
	if (terminal.promptYesNo("An Elasticsearch keystore already exists. Overwrite?", false) == false) {

Password-protected Keystore Feature Branch PR #51123

Password-protected Keystore Feature Branch PR #51123

Conversation

williamrandolph commented Jan 16, 2020

Description

elasticmachine commented Jan 16, 2020

elasticmachine commented Jan 16, 2020

williamrandolph commented Jan 16, 2020

rjernst left a comment

Choose a reason for hiding this comment

jkakavas left a comment

Choose a reason for hiding this comment

pugnascotia Jan 17, 2020

Choose a reason for hiding this comment

pugnascotia Jan 17, 2020

Choose a reason for hiding this comment

williamrandolph Jan 22, 2020

Choose a reason for hiding this comment

pugnascotia Jan 17, 2020

Choose a reason for hiding this comment

jkakavas Jan 22, 2020

Choose a reason for hiding this comment

williamrandolph commented Jan 23, 2020