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

Add infrastructure for elasticsearch keystore #22335

Merged
merged 7 commits into from Jan 6, 2017

Conversation

Projects
None yet
6 participants
@rjernst
Copy link
Member

commented Dec 23, 2016

This change is the first towards providing the ability to store
sensitive settings in elasticsearch. It adds the
elasticsearch-keystore tool, which allows managing a java keystore.
The keystore is loaded upon node startup in Elasticsearch, and used by
the Setting infrastructure when a setting is configured as secure.

There are a lot of caveats to this PR. The most important is it only
provides the tool and setting infrastructure for secure strings. It does
not yet provide for keystore passwords, keypairs, certificates, or even
convert any existing string settings to secure string settings. Those
will all come in follow up PRs. But this PR was already too big, so this
at least gets a basic version of the infrastructure in.

The two main things to look at. The first is the SecureSetting class,
which extends Setting, but removes the assumption for the raw value of the
setting to be a string. SecureSetting provides, for now, a single
helper, stringSetting() to create a SecureSetting which will return a
SecureString (which is like String, but is closeable, so that the
underlying character array can be cleared). The second is the
KeyStoreWrapper class, which wraps the java KeyStore to provide a
simpler api (we do not need the entire keystore api) and also extend
the serialized format to add metadata needed for loading the keystore
with no assumptions about keystore type (so that we can change this in
the future) as well as whether the keystore has a password (so that we
can know whether prompting is necessary when we add support for keystore
passwords).

Settings: Add infrastructure for elasticsearch keystore
This change is the first towards providing the ability to store
sensitive settings in elasticsearch. It adds the
`elasticsearch-keystore` tool, which allows managing a java keystore.
The keystore is loaded upon node startup in Elasticsearch, and used by
the Setting infrastructure when a setting is configured as secure.

There are a lot of caveats to this PR. The most important is it only
provides the tool and setting infrastructure for secure strings. It does
not yet provide for keystore passwords, keypairs, certificates, or even
convert any existing string settings to secure string settings. Those
will all come in follow up PRs. But this PR was already too big, so this
at least gets a basic version of the infrastructure in.

The two main things to look at.  The first is the `SecureSetting` class,
which extends `Setting`, but removes the assumption for the raw value of the
setting to be a string. SecureSetting provides, for now, a single
helper, `stringSetting()` to create a SecureSetting which will return a
SecureString (which is like String, but is closeable, so that the
underlying character array can be cleared). The second is the
`KeyStoreWrapper` class, which wraps the java `KeyStore` to provide a
simpler api (we do not need the entire keystore api) and also extend
the serialized format to add metadata needed for loading the keystore
with no assumptions about keystore type (so that we can change this in
the future) as well as whether the keystore has a password (so that we
can know whether prompting is necessary when we add support for keystore
passwords).
@rjernst

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2016

For those just wanting to know what the interaction with the keystore tool looks like, here is the help:

$ bin/elasticsearch-keystore --help
A tool for managing settings stored in the elasticsearch keystore

Commands
--------
create - Creates a new elasticsearch keystore
list - List entries in the keystore
add - Add a string setting to the keystore
remove - Remove a setting from the keystore

Non-option arguments:
command

Option         Description
------         -----------
-h, --help     show help
-s, --silent   show minimal output
-v, --verbose  show verbose output

$ bin/elasticsearch-keystore add --help
Add a string setting to the keystore

Non-option arguments:
setting name

Option             Description
------             -----------
-E <KeyValuePair>  Configure a setting
-f, --force        Overwrite existing setting without prompting
-h, --help         show help
-s, --silent       show minimal output
-v, --verbose      show verbose output
-x, --stdin        Read setting value from stdin
@rjernst

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2016

One final note for reviewers: The shell scripts are copies of elasticsearch-plugin with the class name changed. I think it is time for us to move all of the environment/java init stuff out to a shared script infra, but I would like to do it in a separate PR.

@rjernst rjernst requested review from s1monw and jaymode Dec 23, 2016

@dadoonet

This comment has been minimized.

Copy link
Member

commented Dec 23, 2016

@rjernst Something which is unclear to me. Is the keystore operation something that people will have to do on every single node? Or do it on a first node, then copy with scp the keytore files to the other nodes?

I'm trying to understand how complex it will be for the end user.
I'm asking that because it has been a pain for discovery-azure. So basically people where creating once the keystore, then an image of their elasticsearch setup (including the keystore), then distributed all that on other nodes.

Probably something we will have to document in details.

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2016

@dadoonet Users could do either. I don't see it different than the node settings in elasticsearch.yml. They have to get to each node somehow. Either the editing work is done on each node, or a single file is copied to each node.

@jaymode
Copy link
Member

left a comment

I left some comments, but I think this is awesome to finally have

@Override
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
KeyStoreWrapper keystore = KeyStoreWrapper.loadMetadata(env.configFile());
if (keystore == null) {

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

More of a user friendliness / usability aspect, but it would be great if we invoked the CreateKeyStoreCommand here. It is pretty minor since the create command would ideally only be run once.

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 3, 2017

Author Member

I don't think we should. I would rather it be an error and explicit rather than magic/leniency.

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

That's also fine with me. It's not a big deal since it's probably a one time only operation

String setting = arguments.value(options);
if (keystore.getSettings().contains(setting) && options.has(forceOption) == false) {
String answer = terminal.readText("Setting " + setting + " already exists. Overwrite? [y/N]");
if (answer.equals("y") == false) {

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

Another minor thing from a usability standpoint, can we check the input and make sure it was either y or N and re-prompt if it was a typo? It would apply in other places as well where we do the same thing

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 3, 2017

Author Member

Sure I can add that.

private static final SecretKeyFactory passwordFactory;
static {
try {
passwordFactory = SecretKeyFactory.getInstance("PBE");

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

The Standard Algorithm Names docs don't list PBE as a algorithm for the SecretKeyFactory. I'm thinking we should be more explicit and use something like PBEWithHmacSHA256AndAES_128

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 3, 2017

Author Member

Ok, I made this explicit, and added the algorithm name to the metadata we store before the keystore, so that we can change this in the future.

terminal.println(entry);
}

// TODO:

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

can we drop this TODO or move it to a more appropriate place?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 3, 2017

Author Member

Removed. These were leftover notes to myself which were already addressed.

}

/**
* Convert to a {@link String}. This only be used with APIs that do not take {@link CharSequence}.

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

missing should I think

import java.util.Objects;

/**
* A String implementations which allows clearing the underlying char array.

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

can we add something about how this is not a thread-safe object (one closing while the other is reading)

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

lets synchronized all methods here and we are good

@@ -613,6 +636,13 @@ public String get(String key) {
return map.get(key);
}

/** Sets the secret store for these settings. */
public void setKeyStore(KeyStoreWrapper keystore) {
assert this.keystore == null; // only set once!

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

I think we should use a SetOnce here, especially with a public method

/** Sets the secret store for these settings. */
public void setKeyStore(KeyStoreWrapper keystore) {
assert this.keystore == null; // only set once!
assert keystore.isLoaded();

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 3, 2017

Member

can this be a hard check that throws a IAE if the keystore is not loaded?

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

@jaymode I believe I addressed all your feedback in 4e4a40d. The only outstanding issue is using the explicit PBE algo. Unfortunately for some reason PKCS12 does not work with those explicit algos (it works with some others, but not of the hmac based ones, getting an unrecognized algorithm name when adding a key). But I did make the algorithm changeable, so when we figure it out, we can simply change the algo used and any new/updated keystore will use the new algo.

@jaymode

jaymode approved these changes Jan 4, 2017

Copy link
Member

left a comment

Left a minor comment but other than that LGTM

@@ -613,6 +637,14 @@ public String get(String key) {
return map.get(key);
}

/** Sets the secret store for these settings. */
public void setKeyStore(KeyStoreWrapper keystore) {
if (keystore.isLoaded()) {

This comment has been minimized.

Copy link
@jaymode

jaymode Jan 4, 2017

Member

Probably want to move the requireNonNull here

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

we can just remove it it will fail with NPE either way

@s1monw
Copy link
Contributor

left a comment

I left a bunch of comments. This looks awesome thanks ryan

/**
* A subcommand for the keystore cli which adds a string setting.
*/
class AddStringKeyStoreCommand extends EnvironmentAwareCommand {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

final?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

These can't be final because they are subclassed in tests in order to manipulate eg the environment the command runs with.

/**
* A subcommand for the keystore cli to create a new keystore.
*/
class CreateKeyStoreCommand extends EnvironmentAwareCommand {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

final?

}

/** Retrieve a string setting. The {@link SecureString} should be closed once it is used. */
SecureString getStringSetting(String setting) throws GeneralSecurityException {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

I wonder if we should think about adding a special security permission that we have to grant to all plugins that want to access the keystore. That way we can prevent unintended access to it and users will be prompted when they install the plugin. Only certain plugins should be able to access this stuff right?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

I don't think certain plugins should be able to access, but I have thought about allowing plugins to only read the secure settings they have registered, and also only allowing reading a setting from the keystore wrapper a single time. But this should be another followup investigation.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

sure maybe add a todo to the code so we don't forget

if (Files.exists(keystoreFile) == false) {
return null;
}
DataInputStream inputStream = new DataInputStream(Files.newInputStream(keystoreFile));

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

can we make use of CodecUtil and write a head as well as a footer. I really want this to be checksummed given that we embed this keystore in our own file. We already have InputStreamIndexInput and OutputStreamIndexOutput so I think it should be doable?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

The keystore itself already has its own integrity checks, so I'm not sure this is needed.

/** Loads the keystore this metadata wraps. This may only be called once. */
public void loadKeystore(char[] password) throws GeneralSecurityException, IOException {
this.keystore.set(KeyStore.getInstance(type));
try (InputStream in = input) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

I think we should not hold on to this InputStream... rather go an open it again, skip the metadata, check the checksum and load all values. That way we never leak any resources and everything is contained. it's also less complex since you don't have that input member that could be closed at some point...

}

/** Write the keystore to the given config directory. */
void save(Path configDir) throws Exception {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

I think just to be safe you should synchronized this method so it won't be invoked concurrently

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

same is true for load

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

I also wonder if the mutators here like save, remove etc. should require some security permission that we only give to the key tool in it's main method? - we run without sec manager on that anyway so I think we are safe but can prevent unintended access

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

Regarding permissions, the only way that could work is if we make the tool a separate jar. We would then need to still have the main ES jar (and probably lucene jars if we start using indexinput/output stuff) with permissions, so I'm not sure what exactly it buys us. I have the write methods as package private already. A more immediate change we could make which could help would be to seal the settings package within the core jar (although we would need to think about how testing works then). But I think this should be a follow up investigation, after many other things are achieved (password protection, existing settings migrated).

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

I am confused, afaik this entire thing is only for plugins. Where in core do we need a secret/password?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 6, 2017

Author Member

We don't. My point was code outside of core should not be calling save (hence it is package private). But it is moot since we do not give permissions to write to the config dir, so only the keystore tool (which doesn't run under SM) can write it.

* Convert to a {@link String}. This should only be used with APIs that do not take {@link CharSequence}.
*/
@Override
public String toString() {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

won't this render this entire thing useless? I mean we are copying the secure setting here?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

Yes, but unfortunately the external things we use these secure settings with mostly take String, eg aws credentials. To allow passing SecureString, we would need to get them to change to accepting CharSequence.

import java.util.Objects;

/**
* A String implementations which allows clearing the underlying char array.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

lets synchronized all methods here and we are good

@@ -613,6 +637,14 @@ public String get(String key) {
return map.get(key);
}

/** Sets the secret store for these settings. */
public void setKeyStore(KeyStoreWrapper keystore) {
if (keystore.isLoaded()) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

we can just remove it it will fail with NPE either way

@@ -57,9 +57,9 @@ protected Environment createEnv(Terminal terminal, Map<String, String> settings)
return new Environment(realSettings);
}
@Override
void init(final boolean daemonize, final Path pidFile, final boolean quiet, Settings initialSettings) {
void init(final boolean daemonize, final Path pidFile, final boolean quiet, Environment env0) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 4, 2017

Contributor

can't we just name it env instead?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 4, 2017

Author Member

I called it env0 because it is not the "real" env, it is an initial environment that will be recreated. In the bootstrap method, we have env which is created using settings from env0. But I'm happy to change it to env here in this intermediate layer if you feel strongly.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

nah I was just curious maybe a better name would be initialEnv?

@jaymode

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Unfortunately for some reason PKCS12 does not work with those explicit algos (it works with some others, but not of the hmac based ones, getting an unrecognized algorithm name when adding a key)

I dug into the JDK source for this and I have a workaround. There is no OID for those PBE with HMAC sha2 algorithms, so it fails there. However, there is special handling elsewhere in the PKCS12KeyStore class for these algorithms when they are used for the actual encryption and decryption of the secret value. My workaround is to use PBE only for generating the SecretKey and we can use the more explicit algorithms for the actual password protection.

--- a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java
+++ b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java
@@ -100,11 +100,12 @@ public class KeyStoreWrapper implements Closeable {
 
     /** Constructs a new keystore with the given password. */
     static KeyStoreWrapper create(char[] password) throws Exception {
-        KeyStoreWrapper wrapper = new KeyStoreWrapper(password.length != 0, NEW_KEYSTORE_TYPE, NEW_KEYSTORE_SECRET_KEY_ALGO, null);
+        // PBE is ok here... we just use it to generate a secret key
+        KeyStoreWrapper wrapper = new KeyStoreWrapper(password.length != 0, NEW_KEYSTORE_TYPE, "PBE", null);
         KeyStore keyStore = KeyStore.getInstance(NEW_KEYSTORE_TYPE);
         keyStore.load(null, null);
         wrapper.keystore.set(keyStore);
-        wrapper.keystorePassword.set(new KeyStore.PasswordProtection(password));
+        wrapper.keystorePassword.set(new KeyStore.PasswordProtection(password, NEW_KEYSTORE_SECRET_KEY_ALGO, null));
         return wrapper;
     }
 
@@ -147,7 +148,7 @@ public class KeyStoreWrapper implements Closeable {
             keystore.get().load(in, password);
         }
 
-        this.keystorePassword.set(new KeyStore.PasswordProtection(password));
+        this.keystorePassword.set(new KeyStore.PasswordProtection(password, NEW_KEYSTORE_SECRET_KEY_ALGO, null));
         Arrays.fill(password, '\0');
 
         // convert keystore aliases enum into a set for easy lookup

Digging into the jdk pkcs12 code didn't leave me with a good feeling, but it is the best option we have for a keystore out of the box...

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2017

@jaymode I'm not sure what advantage setting the algo on PasswordProtection provides, given that it is only in memory? The issue with pkcs12 and explicit pbe is supposed to be fixed in java 9:
https://bugs.openjdk.java.net/browse/JDK-8149411

@jaymode

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

I'm not sure what advantage setting the algo on PasswordProtection provides, given that it is only in memory?

The password protection algorithm is used in conjunction with the password to encrypt the bytes of the secretkey both in memory and in the bytes stored on disk.

@jaymode

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

@rjernst there is another issue with the use of a PBE key factory. The use of non ascii characters results in:

java.security.spec.InvalidKeySpecException: Password is not ASCII
	at com.sun.crypto.provider.PBEKey.<init>(PBEKey.java:64)
	at com.sun.crypto.provider.PBEKeyFactory.engineGenerateSecret(PBEKeyFactory.java:219)
	at javax.crypto.SecretKeyFactory.generateSecret(SecretKeyFactory.java:336)
	at org.elasticsearch.common.settings.KeyStoreWrapper.setStringSetting(KeyStoreWrapper.java:199)

I'd expect that we should have the ability to store UTF-8 characters; my test used a character with an umlaut.

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2017

@s1monw @jaymode I pushed more changes. The wrapper now uses IndexInput/Output with a header and footer. It also reads the entire wrapped keystore into a byte array, which is then loaded later (I also renamed the methods to make a little more sense, load for the wrapper, and decrypt for decrypting the inner keystore). Finally, I added validation of the string values, so they can only be ASCII. All the docs I can find for java 8 show all the PBE algorithms to always use the lower 8 bits of each character (and actually have checks for ascii internally).

Jay, I tried setting the algorithm as you described, on PasswordProtection, but it did not work for me (all of the unit tests which add a string value failed, with java.security.NoSuchAlgorithmException: unrecognized algorithm name: PBE. I'm happy to investigate further, but I think we can work on it in a followup?

@s1monw

s1monw approved these changes Jan 6, 2017

Copy link
Contributor

left a comment

left some minor suggestions

if (format != FORMAT_VERSION) {
throw new IllegalStateException("Unknown keystore metadata format [" + format + "]");

NIOFSDirectory directory = new NIOFSDirectory(configDir);

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

maybe just use SimpleFsDir here?

keystore.get().store(outputStream, password);
char[] password = this.keystorePassword.get().getPassword();

NIOFSDirectory directory = new NIOFSDirectory(configDir);

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

same here just use simple fs?

}

Path keystoreFile = keystorePath(configDir);
Files.move(configDir.resolve(tmpFile), keystoreFile, StandardCopyOption.REPLACE_EXISTING);

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2017

Contributor

do you want to use atomic move here too?

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2017

@s1monw I pushed eb596d7 with your new suggestions (good ones!). I'll wait to push until my morning once Jay has had a chance to look.

@rjernst rjernst added the v5.3.0 label Jan 6, 2017

@rjernst rjernst removed the v5.2.0 label Jan 6, 2017

@jaymode

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

I'm happy to investigate further, but I think we can work on it in a followup?

I agree that we should iterate on this outside of this PR and everything else still LGTM

@rjernst rjernst referenced this pull request Jan 6, 2017

Open

Secure Settings #22475

2 of 8 tasks complete

@rjernst rjernst merged commit d235e48 into elastic:master Jan 6, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@rjernst rjernst deleted the rjernst:keystore branch Jan 6, 2017

rjernst added a commit that referenced this pull request Jan 6, 2017

Settings: Add infrastructure for elasticsearch keystore (#22335)
This change is the first towards providing the ability to store
sensitive settings in elasticsearch. It adds the
elasticsearch-keystore tool, which allows managing a java keystore.
The keystore is loaded upon node startup in Elasticsearch, and used by
the Setting infrastructure when a setting is configured as secure.

There are a lot of caveats to this PR. The most important is it only
provides the tool and setting infrastructure for secure strings. It does
not yet provide for keystore passwords, keypairs, certificates, or even
convert any existing string settings to secure string settings. Those
will all come in follow up PRs. But this PR was already too big, so this
at least gets a basic version of the infrastructure in.

The two main things to look at. The first is the SecureSetting class,
which extends Setting, but removes the assumption for the raw value of
the
setting to be a string. SecureSetting provides, for now, a single
helper, stringSetting() to create a SecureSetting which will return a
SecureString (which is like String, but is closeable, so that the
underlying character array can be cleared). The second is the
KeyStoreWrapper class, which wraps the java KeyStore to provide a
simpler api (we do not need the entire keystore api) and also extend
the serialized format to add metadata needed for loading the keystore
with no assumptions about keystore type (so that we can change this in
the future) as well as whether the keystore has a password (so that we
can know whether prompting is necessary when we add support for keystore
passwords).

@clintongormley clintongormley changed the title Settings: Add infrastructure for elasticsearch keystore Add infrastructure for elasticsearch keystore Jan 9, 2017

PosixFileAttributeView attrs = Files.getFileAttributeView(keystoreFile, PosixFileAttributeView.class);
if (attrs != null) {
// don't rely on umask: ensure the keystore has minimal permissions
attrs.setPermissions(PosixFilePermissions.fromString("rw-------"));

This comment has been minimized.

Copy link
@tlrx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.