honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Jay Modi 2017-03-13 19:50:55 -07:00 committed by GitHub
parent a8117a2d77
commit 3200da0327
4 changed files with 146 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java
View File

@ -104,6 +104,13 @@ public abstract class SecureSetting<T> extends Setting<T> {
// 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.
*

31
core/src/main/java/org/elasticsearch/common/settings/SecureString.java
View File

@ -106,9 +106,40 @@ public final class SecureString implements CharSequence, Closeable {
*/
@Override
public synchronized void close() {
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:
*
* <pre>
* try (SecureString copy = secureString.clone()) {
* // pass thee char[] to a external API
* PasswordAuthentication auth = new PasswordAuthentication(username, copy.getChars());
* ...
* }
* </pre>
*/
@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. */
private void ensureNotClosed() {

10
core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
View File

@ -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)

96
core/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java Normal file
View File

@ -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);
}
}
}
}