Add passphrase support to elasticsearch-keystore #38498
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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. Relates to: elastic#32691
- Moves the code to read the passphrase to KeyStoreWrapper - Adds a subcommand to elasticsearch-keystore for setting or changing the keystore's password
…arameterized in a followup PR so that we can run tests with encrypted keystores
…ore-cli-passphrase
…tore-cli-passphrase
jkakavas
added
>enhancement
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/es-security |
…ntil we can provide the passphrase on startup
…tore-cli-passphrase
|
|
jaymode
previously requested changes
Feb 26, 2019
| @@ -434,7 +434,7 @@ class ClusterFormationTasks { | |||
| * getting the short name requiring the path to already exist. | |||
| */ | |||
| final Object esKeystoreUtil = "${-> node.binPath().resolve('elasticsearch-keystore').toString()}" | |||
| return configureExecTask(name, project, setup, node, esKeystoreUtil, 'create') | |||
| return configureExecTask(name, project, setup, node, esKeystoreUtil, 'create', '--nopass') | |||
There was a problem hiding this comment.
The requirement of --nopass is a breaking change for anyone with scripts to perform setup, so I'd lean for making nopass the default in 7.x with a deprecation
| } | ||
| keystore.save(env.configFile(), keyAndPass.getPassphrase()); | ||
| } catch (SecurityException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "Failed to access the keystore. Please make sure the passphrase was correct."); |
There was a problem hiding this comment.
lets not swallow this exception; we should add them to the UserException
| try { | ||
| keystore.setString(setting, value); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "String value must contain only ASCII"); |
There was a problem hiding this comment.
lets not swallow this exception; we should add them to the UserException
server/src/main/java/org/elasticsearch/common/settings/ChangeKeyStorePassphraseCommand.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/ChangeKeyStorePassphraseCommand.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/ChangeKeyStorePassphraseCommand.java
Outdated
Show resolved
Hide resolved
| import java.io.IOException; | ||
| import java.util.Arrays; | ||
|
|
||
| public class KeystoreAndPassphrase implements Closeable { |
There was a problem hiding this comment.
please add javadocs. I think the class can be package private and final as well?
| terminal.println(entry); | ||
| } | ||
| } catch (SecurityException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "Failed to access the keystore. Please make sure the passphrase was correct."); |
There was a problem hiding this comment.
lets not swallow this exception; we should add them to the UserException
| } | ||
| keystore.save(env.configFile(), passphrase); | ||
| } catch (SecurityException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "Failed to access the keystore. Please make sure the passphrase was correct."); |
There was a problem hiding this comment.
lets not swallow this exception; we should add them to the UserException
…into elasticsearch-keystore-cli-passphrase
- Default behavior when creating a keystore is now to set an empty string as the password (as is the current behavior) - Password can be set with the ChangeKeyStorePassword subcommand - Keystores can be created being password protected, by passing `-p` or `--password`. Users will be prompted to set the password. - Renamed passphrase to password everywhere for consistency
|
@rjernst would you be so kind as to take a look at this one, since Jay is out ? |
rjernst
reviewed
Jun 26, 2019
| @@ -923,7 +923,7 @@ class ClusterFormationTasks { | |||
| return project.tasks.create(name: name, type: LoggedExec, dependsOn: depends) { | |||
| onlyIf { node.pidFile.exists() } | |||
| // the pid file won't actually be read until execution time, since the read is wrapped within an inner closure of the GString | |||
| ext.pid = "${ -> node.pidFile.getText('UTF-8').trim()}" | |||
| ext.pid = "${-> node.pidFile.getText('UTF-8').trim()}" | |||
There was a problem hiding this comment.
It seems like all the changes in this file are reformatting? can you please revert those changes? I'm not sure I"m missing some subtle change, and if there were a change, it would need to be made for testclusters as well.
| keystore.setFile(setting, Files.readAllBytes(file)); | ||
| keystore.save(env.configFile(), keyAndPass.getPassword()); | ||
| } catch (SecurityException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "Failed to access the keystore. Please make sure the password was correct.", e); |
There was a problem hiding this comment.
UserExceptions do not have their stack trace emitted, so adding a cause is irrelevant (I think it was an oversight when that ctor was added to UserException). Are we absolutely sure here the only way a SecurityException can be thrown is when the keystore failed to open due to a bad password? Is there any relevant data we need to extract to get a clear error message to the user?
There was a problem hiding this comment.
Indeed that was sloppy. I gave it another try
| try { | ||
| keystore.setString(setting, value); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "String value must contain only ASCII"); |
There was a problem hiding this comment.
Why is this throwing away the error message from IAE we were using before?
There was a problem hiding this comment.
Not really sure, i don't remember explicitly changing this. In fact it appears as it was like that when I started meddling with this code c58f3aa#diff-6dce8a0b702485f34aba32c4f5738b3eR108 I'll correct it
| * @param forceCreate if set, the keystore is created without prompting the user | ||
| * @return a POJO with the {@link KeyStoreWrapper} and its password, null if the user elected to not create a keystore | ||
| */ | ||
| static KeystoreAndPassword readOrCreate(Terminal terminal, Path configFile, boolean forceCreate) throws Exception { |
There was a problem hiding this comment.
Rather than doing this inside KeyStoreWrapper, I wonder if it could be done in a base keystore cli class (this file is already quite large, and doesn't need to be concerned about cli stuff). Specifically, I think this would eliminate the need for KeystoreAndPassword, and also the need to return null, as to signal to exit. The main method in each CLI would be moved to an abstract method, and this common load/password stuff would be done in the abstract class, putting the keystore and password into member vars of the cli instance, and delegating the rest of the execution to another abstract method. Cleaning up the password and handling a bad password could also be done from this top level, wrapped around the delegate method.
There was a problem hiding this comment.
Ι went with the KeystoreAndPassword approach after your suggestion in #37472 (comment), but the new suggestion also makes sense and I think I also like it better.
…into elasticsearch-keystore-cli-passphrase
jkakavas
force-pushed
the
elasticsearch-keystore-cli-passphrase
branch
from
1d0f9d7 to
500c79a
Compare
rjernst
reviewed
Jul 9, 2019
| } | ||
|
|
||
| @Override | ||
| protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { |
There was a problem hiding this comment.
This should be final so we don't accidentally override
| } | ||
| // allow the user to decide if an auto-created keystore should be password protected | ||
| keyStorePassword = | ||
| terminal.promptYesNo("Do you want to protect the keystore with a password?", false) == false ? |
There was a problem hiding this comment.
This won't work from eg package installation. Can we just always auto create iwthout a password, and allow the user to change through the normal passphrase setting command?
|
|
||
| KeyStoreWrapper keyStore; | ||
| SecureString keyStorePassword; | ||
| boolean keyStoreMustExist = false; |
There was a problem hiding this comment.
this can be a super ctor parameter, then it can be final, and private
|
|
||
| public abstract class BaseKeyStoreCommand extends EnvironmentAwareCommand { | ||
|
|
||
| KeyStoreWrapper keyStore; |
There was a problem hiding this comment.
Since we don't have SetOnce in cli commands, I think these should be private, and accessed through protected getters
| } | ||
| } | ||
|
|
||
| private void clearPassword() { |
There was a problem hiding this comment.
why does this need to be a separate method?
There was a problem hiding this comment.
I think I was calling this from somewhere else originally, but forgot to amend when I made it private and left the only call in here.
| return password; | ||
| } | ||
|
|
||
| protected abstract void executeCommand(Terminal terminal, OptionSet options, Environment env) throws Exception; |
There was a problem hiding this comment.
please add a basic javadoc explaning when this is called, the keystore and keystore password are available.
| try { | ||
| newPassword = readPassword(terminal, true); | ||
| keyStore.save(env.configFile(), newPassword.getChars()); | ||
| terminal.println("Elasticsearch keystore password changed successfully." + env.configFile()); |
There was a problem hiding this comment.
Why is the config file printed here?
| password = options.has(passwordOption) ? | ||
| BaseKeyStoreCommand.readPassword(terminal, true) : new SecureString(new char[0]); | ||
| keystore.save(env.configFile(), password.getChars()); | ||
| terminal.println("Created elasticsearch keystore in " + env.configFile()); |
There was a problem hiding this comment.
configFile is elasticsearch.yml. Shouldn't this be the location of the keystore?
…into elasticsearch-keystore-cli-passphrase
|
ci/1 failed because of #40435 |
|
@elasticmachine run elasticsearch-ci/1 |
|
Thanks for the comments @rjernst , I've addressed all your feedback |
|
@elasticmachine run elasticsearch-ci/1 |
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Jul 11, 2019
This is initial and exporatory work for incorporating keystore passwords in the Elasticsearch startup process. The "user interface" that prompts for and reads a password is in the bash startup script. The bash script uses existing command-line utilities to test whether the keystore exists and a password is needed; if so, the password is given to the Java startup process using a "here string" and a new command-line lag that alerts Java to data coming in over standard input. Known issues: Java code that reads from standard input is not thread-safe, initial keystore verification is unacceptably slow. Relates elastic#32691, elastic#38498
rjernst
approved these changes
Jul 22, 2019
|
|
||
| @Override | ||
| protected void executeCommand(Terminal terminal, OptionSet options, Environment env) throws Exception { | ||
| SecureString newPassword = null; |
There was a problem hiding this comment.
Since SecureString is closeable, you can use a try-with-resources so you don't need the explicit finally below
| } | ||
| } | ||
| KeyStoreWrapper keystore = KeyStoreWrapper.create(); | ||
| password = options.has(passwordOption) ? |
There was a problem hiding this comment.
Ditto here, you can use try-with-resources
…tore-cli-passphrase
The docker entry script was depending on the -f flag of the add subcommand to create the keystore. This was an undocumented behavior of the -f flag that should originally only "force" the overwrite of the entry in the keystore and not the creation of the keystore itself. This behavior was changed as part of earlier commit in this PR
|
@elasticmachine run elasticsearch-ci/docs again. It was broken just now and we've reverted the break. |
@elasticmachine run elasticsearch-ci/packaging-sample |
williamrandolph
pushed a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Nov 18, 2019
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: elastic#32691 Supersedes: elastic#37472
williamrandolph
added a commit
that referenced
this pull request
Dec 11, 2019
* 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 <lcawley@elastic.co> * 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.
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Jan 16, 2020
) * Reload secure settings with password (elastic#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 (elastic#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: elastic#32691 Supersedes: elastic#37472 * Restore behavior for force parameter (elastic#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 (elastic#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 (elastic#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 <lcawley@elastic.co> * Elasticsearch keystore passphrase for startup scripts (elastic#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 (elastic#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.
williamrandolph
added a commit
that referenced
this pull request
Jan 28, 2020
* 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 <lcawley@elastic.co> * 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 <lcawley@elastic.co> * 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 <lcawley@elastic.co> Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>
williamrandolph
added a commit
that referenced
this pull request
Jan 28, 2020
* 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 <lcawley@elastic.co> * 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 <lcawley@elastic.co> * 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 <lcawley@elastic.co> Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds support for keystore passphrases to all subcommands
of the
elasticsearch-keystorecli tool and adds a subcommand forchanging the passphrase of an existing keystore.
This depends on the work to read the passphrase in Elasticsearch when
loading, which will be addressed in a different PR.
Subcommands of
elasticsearch-keystorecan 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 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