Add validation of keystore setting names (#27626)

This commit restricts settings added to the keystore to have a lowercase
ascii name. The java Keystore javadocs state that case sensitivity of
key alias names are implementation dependent. This ensures regardless of
case sensitivity in a jvm implementation, the keys will be stored as we
expect.
This commit is contained in:
Ryan Ernst 2017-12-05 14:30:36 -08:00 committed by GitHub
parent ed2caf2bad
commit 8139e3a1c7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 40 additions and 1 deletions

View File

@ -49,6 +49,7 @@ import java.util.HashSet;
import java.util.Locale; import java.util.Locale;
import java.util.Map; import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors; import java.util.stream.Collectors;
import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.codecs.CodecUtil;
@ -59,7 +60,6 @@ import org.apache.lucene.store.IndexInput;
import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory; import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.util.SetOnce; import org.apache.lucene.util.SetOnce;
import org.elasticsearch.bootstrap.BootstrapSettings;
import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.UserException; import org.elasticsearch.cli.UserException;
import org.elasticsearch.common.Randomness; import org.elasticsearch.common.Randomness;
@ -75,6 +75,11 @@ import org.elasticsearch.common.Randomness;
*/ */
public class KeyStoreWrapper implements SecureSettings { public class KeyStoreWrapper implements SecureSettings {
/**
* A regex for the valid characters that a setting name in the keystore may use.
*/
private static final Pattern ALLOWED_SETTING_NAME = Pattern.compile("[a-z0-9_\\-.]+");
public static final Setting<SecureString> SEED_SETTING = SecureSetting.secureString("keystore.seed", null); public static final Setting<SecureString> SEED_SETTING = SecureSetting.secureString("keystore.seed", null);
/** Characters that may be used in the bootstrap seed setting added to all keystores. */ /** Characters that may be used in the bootstrap seed setting added to all keystores. */
@ -383,6 +388,18 @@ public class KeyStoreWrapper implements SecureSettings {
return Base64.getDecoder().wrap(bytesStream); return Base64.getDecoder().wrap(bytesStream);
} }
/**
* Ensure the given setting name is allowed.
*
* @throws IllegalArgumentException if the setting name is not valid
*/
public static void validateSettingName(String setting) {
if (ALLOWED_SETTING_NAME.matcher(setting).matches() == false) {
throw new IllegalArgumentException("Setting name [" + setting + "] does not match the allowed setting name pattern ["
+ ALLOWED_SETTING_NAME.pattern() + "]");
}
}
/** /**
* Set a string setting. * Set a string setting.
* *
@ -390,6 +407,7 @@ public class KeyStoreWrapper implements SecureSettings {
*/ */
void setString(String setting, char[] value) throws GeneralSecurityException { void setString(String setting, char[] value) throws GeneralSecurityException {
assert isLoaded(); assert isLoaded();
validateSettingName(setting);
if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) { if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) {
throw new IllegalArgumentException("Value must be ascii"); throw new IllegalArgumentException("Value must be ascii");
} }
@ -401,6 +419,7 @@ public class KeyStoreWrapper implements SecureSettings {
/** Set a file setting. */ /** Set a file setting. */
void setFile(String setting, byte[] bytes) throws GeneralSecurityException { void setFile(String setting, byte[] bytes) throws GeneralSecurityException {
assert isLoaded(); assert isLoaded();
validateSettingName(setting);
bytes = Base64.getEncoder().encode(bytes); bytes = Base64.getEncoder().encode(bytes);
char[] chars = new char[bytes.length]; char[] chars = new char[bytes.length];
for (int i = 0; i < chars.length; ++i) { for (int i = 0; i < chars.length; ++i) {

View File

@ -46,6 +46,7 @@ public abstract class SecureSetting<T> extends Setting<T> {
private SecureSetting(String key, Property... properties) { private SecureSetting(String key, Property... properties) {
super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class)); super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class));
assert assertAllowedProperties(properties); assert assertAllowedProperties(properties);
KeyStoreWrapper.validateSettingName(key);
} }
private boolean assertAllowedProperties(Setting.Property... properties) { private boolean assertAllowedProperties(Setting.Property... properties) {

View File

@ -97,4 +97,14 @@ public class KeyStoreWrapperTests extends ESTestCase {
keystore.decrypt(new char[0]); keystore.decrypt(new char[0]);
assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString());
} }
public void testIllegalSettingName() throws Exception {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> KeyStoreWrapper.validateSettingName("UpperCase"));
assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));
KeyStoreWrapper keystore = KeyStoreWrapper.create(new char[0]);
e = expectThrows(IllegalArgumentException.class, () -> keystore.setString("UpperCase", new char[0]));
assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));
e = expectThrows(IllegalArgumentException.class, () -> keystore.setFile("UpperCase", new byte[0]));
assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));
}
} }

View File

@ -486,6 +486,15 @@ public class SettingsTests extends ESTestCase {
assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore")); assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore"));
} }
public void testSecureSettingIllegalName() {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
SecureSetting.secureString("UpperCaseSetting", null));
assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));
e = expectThrows(IllegalArgumentException.class, () ->
SecureSetting.secureFile("UpperCaseSetting", null));
assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));
}
public void testGetAsArrayFailsOnDuplicates() { public void testGetAsArrayFailsOnDuplicates() {
final IllegalStateException e = expectThrows(IllegalStateException.class, () -> Settings.builder() final IllegalStateException e = expectThrows(IllegalStateException.class, () -> Settings.builder()
.put("foobar.0", "bar") .put("foobar.0", "bar")