Skip to content

Commit

Permalink
Ensure KeyStoreWrapper decryption exceptions are handled (#32472)
Browse files Browse the repository at this point in the history
* Ensure decryption related exceptions are handled

This commit ensures that all possible Exceptions in
KeyStoreWrapper#decrypt() are handled. More specifically, in the
case that a wrong password is used for secure settings, calling readX
on the DataInputStream that wraps the CipherInputStream can throw an
IOException. It also adds a test for loading a KeyStoreWrapper with
a wrong password.

This is a backport of #32464
  • Loading branch information
jkakavas authored Jul 31, 2018
1 parent d768bf0 commit 45883a8
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio
if (input.read() != -1) {
throw new SecurityException("Keystore has been corrupted or tampered with");
}
} catch (EOFException e) {
} catch (IOException e) {
throw new SecurityException("Keystore has been corrupted or tampered with", e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.elasticsearch.plugins.ReloadablePlugin;
import org.elasticsearch.test.ESIntegTestCase;

import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
Expand Down Expand Up @@ -205,14 +204,7 @@ public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
assertThat(nodesMap.size(), equalTo(cluster().size()));
for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) {
assertThat(nodeResponse.reloadException(), notNullValue());
// Running in a JVM with a BouncyCastle FIPS Security Provider, decrypting the Keystore with the wrong
// password returns a SecurityException if the DataInputStream can't be fully consumed
if (inFipsJvm()) {
assertThat(nodeResponse.reloadException(), instanceOf(SecurityException.class));
} else {
assertThat(nodeResponse.reloadException(), instanceOf(IOException.class));
}

}
} catch (final AssertionError e) {
reloadSettingsError.set(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ public void testCreate() throws Exception {
assertTrue(keystore.getSettingNames().contains(KeyStoreWrapper.SEED_SETTING.getKey()));
}

public void testDecryptKeyStoreWithWrongPassword() throws Exception {
KeyStoreWrapper keystore = KeyStoreWrapper.create();
keystore.save(env.configFile(), new char[0]);
final KeyStoreWrapper loadedkeystore = KeyStoreWrapper.load(env.configFile());
final SecurityException exception = expectThrows(SecurityException.class,
() -> loadedkeystore.decrypt(new char[]{'i', 'n', 'v', 'a', 'l', 'i', 'd'}));
assertThat(exception.getMessage(), containsString("Keystore has been corrupted or tampered with"));
}

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

0 comments on commit 45883a8

Please sign in to comment.