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

Fail if reading from closed KeyStoreWrapper #30394

Merged
merged 6 commits into from
May 14, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fail if reading from closed KeyStoreWrapper
In #28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.

This change restores the previous behaviour as closely as possible.
It is possible to retrieve the _keys_ from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.

The only divergence from the previous behaviour is that "isLoaded"
will now return false if the keystore is closed, in keeping with the
"loaded and retrievable" description in the parent javadoc.
  • Loading branch information
tvernum committed May 4, 2018
commit 24a83f8a44e74144a8f626d872dd99d12383beba
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ private static class Entry {

/** The decrypted secret data. See {@link #decrypt(char[])}. */
private final SetOnce<Map<String, Entry>> entries = new SetOnce<>();
private volatile boolean closed;

private KeyStoreWrapper(int formatVersion, boolean hasPassword, byte[] dataBytes) {
this.formatVersion = formatVersion;
Expand Down Expand Up @@ -274,7 +275,7 @@ public static void upgrade(KeyStoreWrapper wrapper, Path configDir, char[] passw

@Override
public boolean isLoaded() {
return entries.get() != null;
return entries.get() != null && closed == false;
}

/** Return true iff calling {@link #decrypt(char[])} requires a non-empty password. */
Expand Down Expand Up @@ -500,16 +501,22 @@ public void save(Path configDir, char[] password) throws Exception {
}
}

/**
* It is possible to retrieve the setting names even if the keystore is closed.
* This allows {@link SecureSetting} to correctly determine that a entry exists even though it cannot be read. Thus attempting to
* read a secure setting after the keystore is closed will generate a "keystore is closed" exception rather than using the fallback
* setting.
*/
@Override
public Set<String> getSettingNames() {
assert isLoaded();
assert entries.get() != null : "Keystore is not loaded";
return entries.get().keySet();
}

// TODO: make settings accessible only to code that registered the setting
@Override
public SecureString getString(String setting) {
assert isLoaded();
ensureOpen();
Entry entry = entries.get().get(setting);
if (entry == null || entry.type != EntryType.STRING) {
throw new IllegalArgumentException("Secret setting " + setting + " is not a string");
Expand All @@ -521,12 +528,11 @@ public SecureString getString(String setting) {

@Override
public InputStream getFile(String setting) {
assert isLoaded();
ensureOpen();
Entry entry = entries.get().get(setting);
if (entry == null || entry.type != EntryType.FILE) {
throw new IllegalArgumentException("Secret setting " + setting + " is not a file");
}

return new ByteArrayInputStream(entry.bytes);
}

Expand All @@ -544,7 +550,7 @@ public static void validateSettingName(String setting) {

/** Set a string setting. */
void setString(String setting, char[] value) {
assert isLoaded();
ensureOpen();
validateSettingName(setting);

ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(value));
Expand All @@ -557,7 +563,7 @@ void setString(String setting, char[] value) {

/** Set a file setting. */
void setFile(String setting, byte[] bytes) {
assert isLoaded();
ensureOpen();
validateSettingName(setting);

Entry oldEntry = entries.get().put(setting, new Entry(EntryType.FILE, Arrays.copyOf(bytes, bytes.length)));
Expand All @@ -568,15 +574,23 @@ void setFile(String setting, byte[] bytes) {

/** Remove the given setting from the keystore. */
void remove(String setting) {
assert isLoaded();
ensureOpen();
Entry oldEntry = entries.get().remove(setting);
if (oldEntry != null) {
Arrays.fill(oldEntry.bytes, (byte)0);
}
}

private void ensureOpen() {
if (closed) {
throw new IllegalStateException("Keystore is closed");
}
assert isLoaded() : "Keystore is not loaded";
}

@Override
public void close() {
this.closed = true;
for (Entry entry : entries.get().values()) {
Arrays.fill(entry.bytes, (byte)0);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,27 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.CharBuffer;
import java.nio.charset.CharsetEncoder;
import java.nio.charset.StandardCharsets;
import java.nio.file.FileSystem;
import java.nio.file.Path;
import java.security.KeyStore;
import java.util.ArrayList;
import java.util.Base64;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.apache.lucene.codecs.CodecUtil;
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.bootstrap.BootstrapSettings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.Before;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

public class KeyStoreWrapperTests extends ESTestCase {

Expand Down Expand Up @@ -92,6 +89,19 @@ public void testCreate() throws Exception {
assertTrue(keystore.getSettingNames().contains(KeyStoreWrapper.SEED_SETTING.getKey()));
}

public void testCannotReadStringFromClosedKeystore() throws Exception {
KeyStoreWrapper keystore = KeyStoreWrapper.create();
assertThat(keystore.getSettingNames(), Matchers.hasItem(KeyStoreWrapper.SEED_SETTING.getKey()));
assertThat(keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()), notNullValue());

keystore.close();

assertThat(keystore.getSettingNames(), Matchers.hasItem(KeyStoreWrapper.SEED_SETTING.getKey()));
final IllegalStateException exception = expectThrows(IllegalStateException.class,
() -> keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()));
assertThat(exception.getMessage(), containsString("closed"));
}

public void testUpgradeNoop() throws Exception {
KeyStoreWrapper keystore = KeyStoreWrapper.create();
SecureString seed = keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey());
Expand Down