From 3200da0327337edf1dc4a9699a7c792b7a31eedb Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Mon, 13 Mar 2017 19:50:55 -0700 Subject: [PATCH] Provide a method to retrieve a closeable char[] from a SecureString (#23389) This change adds a new method that returns the underlying char[] of a SecureString and the ability to clone the SecureString so that the original SecureString is not vulnerable to modification. Closing the cloned SecureString will wipe the char[] that backs the clone but the original SecureString remains unaffected. Additionally, while making a separate change I found that SecureSettings will fail when diff is called on them and there is no fallback setting. Given the idea behind SecureSetting, I think that diff should just be a no-op and I have implemented this here as well. --- .../common/settings/SecureSetting.java | 7 ++ .../common/settings/SecureString.java | 35 ++++++- .../common/settings/ScopedSettingsTests.java | 10 ++ .../common/settings/SecureStringTests.java | 96 +++++++++++++++++++ 4 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index 4c2f3a6d48a..16757187196 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -104,6 +104,13 @@ public abstract class SecureSetting extends Setting { // TODO: override toXContent + /** + * Overrides the diff operation to make this a no-op for secure settings as they shouldn't be returned in a diff + */ + @Override + public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { + } + /** * A setting which contains a sensitive string. * diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java index 36982ddde1c..85c4c566db1 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java @@ -106,8 +106,39 @@ public final class SecureString implements CharSequence, Closeable { */ @Override public synchronized void close() { - Arrays.fill(chars, '\0'); - chars = null; + if (chars != null) { + Arrays.fill(chars, '\0'); + chars = null; + } + } + + /** + * Returns a new copy of this object that is backed by its own char array. Closing the new instance has no effect on the instance it + * was created from. This is useful for APIs which accept a char array and you want to be safe about the API potentially modifying the + * char array. For example: + * + * 
+     *     try (SecureString copy = secureString.clone()) {
+     *         // pass thee char[] to a external API
+     *         PasswordAuthentication auth = new PasswordAuthentication(username, copy.getChars());
+     *         ...
+     *     }
+     *
+ */ + @Override + public synchronized SecureString clone() { + ensureNotClosed(); + return new SecureString(Arrays.copyOf(chars, chars.length)); + } + + /** + * Returns the underlying char[]. This is a dangerous operation as the array may be modified while it is being used by other threads + * or a consumer may modify the values in the array. For safety, it is preferable to use {@link #clone()} and pass its chars to the + * consumer when the chars are needed multiple times. + */ + public synchronized char[] getChars() { + ensureNotClosed(); + return chars; } /** Throw an exception if this string has been closed, indicating something is trying to access the data after being closed. */ diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 98da8239775..7ec8f41034f 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -439,6 +439,16 @@ public class ScopedSettingsTests extends ESTestCase { clusterSettings2.validate(settings); } + public void testDiffSecureSettings() { + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("some.secure.setting", "secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, + Collections.singleton(SecureSetting.secureString("some.secure.setting", null, false))); + + Settings diffed = clusterSettings.diff(Settings.EMPTY, settings); + assertTrue(diffed.isEmpty()); + } public static IndexMetaData newIndexMeta(String name, Settings indexSettings) { Settings build = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) diff --git a/core/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java b/core/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java new file mode 100644 index 00000000000..4f9ed8ed4b9 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java @@ -0,0 +1,96 @@ +/* + * 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.common.settings; + +import org.elasticsearch.test.ESTestCase; + +import java.util.Arrays; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.sameInstance; + +public class SecureStringTests extends ESTestCase { + + public void testCloseableCharsDoesNotModifySecureString() { + final char[] password = randomAsciiOfLengthBetween(1, 32).toCharArray(); + SecureString secureString = new SecureString(password); + assertSecureStringEqualToChars(password, secureString); + try (SecureString copy = secureString.clone()) { + assertArrayEquals(password, copy.getChars()); + assertThat(copy.getChars(), not(sameInstance(password))); + } + assertSecureStringEqualToChars(password, secureString); + } + + public void testClosingSecureStringDoesNotModifyCloseableChars() { + final char[] password = randomAsciiOfLengthBetween(1, 32).toCharArray(); + SecureString secureString = new SecureString(password); + assertSecureStringEqualToChars(password, secureString); + SecureString copy = secureString.clone(); + assertArrayEquals(password, copy.getChars()); + assertThat(copy.getChars(), not(sameInstance(password))); + final char[] passwordCopy = Arrays.copyOf(password, password.length); + assertArrayEquals(password, passwordCopy); + secureString.close(); + assertNotEquals(password[0], passwordCopy[0]); + assertArrayEquals(passwordCopy, copy.getChars()); + } + + public void testClosingChars() { + final char[] password = randomAsciiOfLengthBetween(1, 32).toCharArray(); + SecureString secureString = new SecureString(password); + assertSecureStringEqualToChars(password, secureString); + SecureString copy = secureString.clone(); + assertArrayEquals(password, copy.getChars()); + assertThat(copy.getChars(), not(sameInstance(password))); + copy.close(); + if (randomBoolean()) { + // close another time and no exception is thrown + copy.close(); + } + IllegalStateException e = expectThrows(IllegalStateException.class, copy::getChars); + assertThat(e.getMessage(), containsString("already been closed")); + } + + public void testGetCloseableCharsAfterSecureStringClosed() { + final char[] password = randomAsciiOfLengthBetween(1, 32).toCharArray(); + SecureString secureString = new SecureString(password); + assertSecureStringEqualToChars(password, secureString); + secureString.close(); + if (randomBoolean()) { + // close another time and no exception is thrown + secureString.close(); + } + IllegalStateException e = expectThrows(IllegalStateException.class, secureString::clone); + assertThat(e.getMessage(), containsString("already been closed")); + } + + private void assertSecureStringEqualToChars(char[] expected, SecureString secureString) { + int pos = 0; + for (int i : secureString.chars().toArray()) { + if (pos >= expected.length) { + fail("Index " + i + " greated than or equal to array length " + expected.length); + } else { + assertEquals(expected[pos++], (char) i); + } + } + } +}