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

Handle pwd protected keystores in all CLI tools #45289

Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,15 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.KeyStoreAwareCommand;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.env.Environment;

import java.nio.file.Path;
import java.util.Arrays;

public abstract class BaseKeyStoreCommand extends EnvironmentAwareCommand {
public abstract class BaseKeyStoreCommand extends KeyStoreAwareCommand {

private KeyStoreWrapper keyStore;
private SecureString keyStorePassword;
Expand Down Expand Up @@ -82,30 +81,6 @@ protected SecureString getKeyStorePassword() {
return keyStorePassword;
}

/**
* Reads the keystore password from the {@link Terminal}, prompting for verification where applicable and returns it as a
* {@link SecureString}.
*
* @param terminal the terminal to use for user inputs
* @param withVerification whether the user should be prompted for password verification
* @return a SecureString with the password the user entered
* @throws UserException If the user is prompted for verification and enters a different password
*/
static SecureString readPassword(Terminal terminal, boolean withVerification) throws UserException {
final char[] passwordArray;
if (withVerification) {
passwordArray = terminal.readSecret("Enter new password for the elasticsearch keystore (empty for no password): ");
char[] passwordVerification = terminal.readSecret("Enter same password again: ");
if (Arrays.equals(passwordArray, passwordVerification) == false) {
throw new UserException(ExitCodes.DATA_ERROR, "Passwords are not equal, exiting.");
}
Arrays.fill(passwordVerification, '\u0000');
} else {
passwordArray = terminal.readSecret("Enter password for the elasticsearch keystore : ");
}
return new SecureString(passwordArray);
}

/**
* This is called after the keystore password has been read from the stdin and the keystore is decrypted and
* loaded. The keystore and keystore passwords are available to classes extending {@link BaseKeyStoreCommand}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.KeyStoreAwareCommand;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.env.Environment;

/**
* A sub-command for the keystore cli to create a new keystore.
*/
class CreateKeyStoreCommand extends EnvironmentAwareCommand {
class CreateKeyStoreCommand extends KeyStoreAwareCommand {

private final OptionSpec<Void> passwordOption;

Expand All @@ -46,7 +46,7 @@ class CreateKeyStoreCommand extends EnvironmentAwareCommand {
@Override
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
try (SecureString password = options.has(passwordOption) ?
BaseKeyStoreCommand.readPassword(terminal, true) : new SecureString(new char[0])) {
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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.cli;

import joptsimple.OptionSet;
import org.elasticsearch.common.settings.KeyStoreWrapper;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.env.Environment;

import javax.crypto.AEADBadTagException;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.util.Arrays;

/**
* An {@link org.elasticsearch.cli.EnvironmentAwareCommand} that needs to access the elasticsearch keystore, possibly
* decrypting it if it is password protected.
*/
public abstract class KeyStoreAwareCommand extends EnvironmentAwareCommand {
public KeyStoreAwareCommand(String description) {
super(description);
}

/**
* Reads the keystore password from the {@link Terminal}, prompting for verification where applicable and returns it as a
* {@link SecureString}.
*
* @param terminal the terminal to use for user inputs
* @param withVerification whether the user should be prompted for password verification
* @return a SecureString with the password the user entered
* @throws UserException If the user is prompted for verification and enters a different password
*/
protected static SecureString readPassword(Terminal terminal, boolean withVerification) throws UserException {
final char[] passwordArray;
if (withVerification) {
passwordArray = terminal.readSecret("Enter new password for the elasticsearch keystore (empty for no password): ");
char[] passwordVerification = terminal.readSecret("Enter same password again: ");
if (Arrays.equals(passwordArray, passwordVerification) == false) {
throw new UserException(ExitCodes.DATA_ERROR, "Passwords are not equal, exiting.");
}
Arrays.fill(passwordVerification, '\u0000');
} else {
passwordArray = terminal.readSecret("Enter password for the elasticsearch keystore : ");
}
return new SecureString(passwordArray);
}

/**
* Decrypt the {@code keyStore}, prompting the user to enter the password in the {@link Terminal} if it is password protected
*/
protected static void decryptKeyStore(KeyStoreWrapper keyStore, Terminal terminal)
throws UserException, GeneralSecurityException, IOException {
try (SecureString keystorePassword = keyStore.hasPassword() ?
readPassword(terminal, false) : new SecureString(new char[0])) {
keyStore.decrypt(keystorePassword.getChars());
} catch (SecurityException e) {
if (e.getCause() instanceof AEADBadTagException) {
throw new UserException(ExitCodes.DATA_ERROR, "Wrong password for elasticsearch.keystore");
}
}
}

protected abstract void execute(Terminal terminal, OptionSet options, Environment env) throws Exception;
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,15 @@ public String execute(String... args) throws Exception {
command.mainWithoutErrorHandling(args, terminal);
return terminal.getOutput();
}

/**
* Runs the specified command with the given args.
* <p>
* Output can be found in {@link #terminal}.
*/
public String execute(Command command, String... args) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just call mainWithoutErrorHandling directly in the one case where we need this? Making that method public seems better than overloading this method for one test case.

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 have a strong preference and don't mind following your suggestion but could you explain your reasoning/thinking regarding:

Making that method public seems better than overloading this method for one test case.

for future reference?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I misunderstood where this method existed. It looks like we don't actually need a command member of the test case class. I would remove that, and make the other execute method call this method with newCommand(). Then I'm ok with having another method that takes command.

this.command = command;
this.command.mainWithoutErrorHandling(args, terminal);
return terminal.getOutput();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.KeyStoreAwareCommand;
import org.elasticsearch.cli.LoggingAwareMultiCommand;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.Terminal.Verbosity;
Expand Down Expand Up @@ -125,7 +125,7 @@ class AutoSetup extends SetupCommand {
@Override
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
terminal.println(Verbosity.VERBOSE, "Running with configuration path: " + env.configFile());
setupOptions(options, env);
setupOptions(terminal, options, env);
checkElasticKeystorePasswordValid(terminal, env);
checkClusterHealth(terminal);

Expand Down Expand Up @@ -171,7 +171,7 @@ class InteractiveSetup extends SetupCommand {
@Override
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
terminal.println(Verbosity.VERBOSE, "Running with configuration path: " + env.configFile());
setupOptions(options, env);
setupOptions(terminal, options, env);
checkElasticKeystorePasswordValid(terminal, env);
checkClusterHealth(terminal);

Expand Down Expand Up @@ -221,7 +221,7 @@ private void changedPasswordCallback(Terminal terminal, String user, SecureStrin
* An abstract class that provides functionality common to both the auto and
* interactive setup modes.
*/
private abstract class SetupCommand extends EnvironmentAwareCommand {
private abstract class SetupCommand extends KeyStoreAwareCommand {

boolean shouldPrompt;

Expand All @@ -248,10 +248,9 @@ public void close() {
}
}

void setupOptions(OptionSet options, Environment env) throws Exception {
void setupOptions(Terminal terminal, OptionSet options, Environment env) throws Exception {
keyStoreWrapper = keyStoreFunction.apply(env);
// TODO: We currently do not support keystore passwords
keyStoreWrapper.decrypt(new char[0]);
decryptKeyStore(keyStoreWrapper, terminal);

Settings.Builder settingsBuilder = Settings.builder();
settingsBuilder.put(env.settings(), true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.KeyStoreAwareCommand;
import org.elasticsearch.cli.SuppressForbidden;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
Expand Down Expand Up @@ -68,7 +68,7 @@
/**
* CLI tool to generate SAML Metadata for a Service Provider (realm)
*/
public class SamlMetadataCommand extends EnvironmentAwareCommand {
public class SamlMetadataCommand extends KeyStoreAwareCommand {

static final String METADATA_SCHEMA = "saml-schema-metadata-2.0.xsd";

Expand Down Expand Up @@ -414,13 +414,12 @@ private SortedSet<String> sorted(Set<String> strings) {
/**
* @TODO REALM-SETTINGS[TIM] This can be redone a lot now the realm settings are keyed by type
*/
private RealmConfig findRealm(Terminal terminal, OptionSet options, Environment env) throws UserException, IOException, Exception {
private RealmConfig findRealm(Terminal terminal, OptionSet options, Environment env) throws Exception {

keyStoreWrapper = keyStoreFunction.apply(env);
final Settings settings;
if (keyStoreWrapper != null) {
// TODO: We currently do not support keystore passwords
keyStoreWrapper.decrypt(new char[0]);
decryptKeyStore(keyStoreWrapper, terminal);

final Settings.Builder settingsBuilder = Settings.builder();
settingsBuilder.put(env.settings(), true);
Expand Down
Loading