Skip to content

Commit

Permalink
Add validation of keystore setting names (#27626)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rjernst committed Dec 5, 2017
1 parent ed2caf2 commit 8139e3a
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 1 deletion.
Expand Up @@ -49,6 +49,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import org.apache.lucene.codecs.CodecUtil;
Expand All @@ -59,7 +60,6 @@
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.bootstrap.BootstrapSettings;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.common.Randomness;
Expand All @@ -75,6 +75,11 @@
*/
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);

/** Characters that may be used in the bootstrap seed setting added to all keystores. */
Expand Down Expand Up @@ -383,13 +388,26 @@ public void close() throws IOException {
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.
*
* @throws IllegalArgumentException if the value is not ASCII
*/
void setString(String setting, char[] value) throws GeneralSecurityException {
assert isLoaded();
validateSettingName(setting);
if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) {
throw new IllegalArgumentException("Value must be ascii");
}
Expand All @@ -401,6 +419,7 @@ void setString(String setting, char[] value) throws GeneralSecurityException {
/** Set a file setting. */
void setFile(String setting, byte[] bytes) throws GeneralSecurityException {
assert isLoaded();
validateSettingName(setting);
bytes = Base64.getEncoder().encode(bytes);
char[] chars = new char[bytes.length];
for (int i = 0; i < chars.length; ++i) {
Expand Down
Expand Up @@ -46,6 +46,7 @@ public abstract class SecureSetting<T> extends Setting<T> {
private SecureSetting(String key, Property... properties) {
super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class));
assert assertAllowedProperties(properties);
KeyStoreWrapper.validateSettingName(key);
}

private boolean assertAllowedProperties(Setting.Property... properties) {
Expand Down
Expand Up @@ -97,4 +97,14 @@ public void testUpgradeAddsSeed() throws Exception {
keystore.decrypt(new char[0]);
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"));
}
}
Expand Up @@ -486,6 +486,15 @@ public void testSecureSettingConflict() {
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() {
final IllegalStateException e = expectThrows(IllegalStateException.class, () -> Settings.builder()
.put("foobar.0", "bar")
Expand Down

0 comments on commit 8139e3a

Please sign in to comment.