Settings: Disallow secure setting to exist in normal settings (#23976)

This commit removes the "legacy" feature of secure settings, which setup
a parallel setting that was a fallback in the insecure
elasticsearch.yml. This was previously used to allow the new secure
setting name to be that of the old setting name, but is now not in use
due to other refactorings. It is much cleaner to just have all secure
settings use new setting names. If in the future we want to reuse the
previous setting name, once support for the insecure settings have been
removed, we can then rename the secure setting.  This also adds a test
for the behavior.

core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java

@@ -87,6 +87,10 @@ public abstract class SecureSetting<T> extends Setting<T> {
         checkDeprecation(settings);
         final SecureSettings secureSettings = settings.getSecureSettings();
         if (secureSettings == null || secureSettings.getSettingNames().contains(getKey()) == false) {
+            if (super.exists(settings)) {
+                throw new IllegalArgumentException("Setting [" + getKey() + "] is a secure setting" +
+                    " and must be stored inside the Elasticsearch keystore, but was found inside elasticsearch.yml");
+            }
             return getFallback(settings);
         }
         try {
@@ -117,14 +121,7 @@ public abstract class SecureSetting<T> extends Setting<T> {
      * This may be any sensitive string, e.g. a username, a password, an auth token, etc.
      */
     public static Setting<SecureString> secureString(String name, Setting<SecureString> fallback,
-                                                     boolean allowLegacy, Property... properties) {
-        final Setting<String> legacy;
-        if (allowLegacy) {
-            Property[] legacyProperties = ArrayUtils.concat(properties, LEGACY_PROPERTIES, Property.class);
-            legacy = Setting.simpleString(name, legacyProperties);
-        } else {
-            legacy = null;
-        }
+                                                     Property... properties) {
         return new SecureSetting<SecureString>(name, properties) {
             @Override
             protected SecureString getSecret(SecureSettings secureSettings) throws GeneralSecurityException {
@@ -132,26 +129,11 @@ public abstract class SecureSetting<T> extends Setting<T> {
             }
             @Override
             SecureString getFallback(Settings settings) {
-                if (legacy != null && legacy.exists(settings)) {
-                    return new SecureString(legacy.get(settings).toCharArray());
-                }
                 if (fallback != null) {
                     return fallback.get(settings);
                 }
                 return new SecureString(new char[0]); // this means "setting does not exist"
             }
-            @Override
-            protected void checkDeprecation(Settings settings) {
-                super.checkDeprecation(settings);
-                if (legacy != null) {
-                    legacy.checkDeprecation(settings);
-                }
-            }
-            @Override
-            public boolean exists(Settings settings) {
-                // handle legacy, which is internal to this setting
-                return super.exists(settings) || legacy != null && legacy.exists(settings);
-            }
         };
     }
 

core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

@@ -454,7 +454,7 @@ public class ScopedSettingsTests extends ESTestCase {
         assertThat(e.getMessage(), startsWith("unknown secure setting [some.secure.setting]"));
 
         ClusterSettings clusterSettings2 = new ClusterSettings(settings,
-            Collections.singleton(SecureSetting.secureString("some.secure.setting", null, false)));
+            Collections.singleton(SecureSetting.secureString("some.secure.setting", null)));
         clusterSettings2.validate(settings);
     }
 
@@ -463,7 +463,7 @@ public class ScopedSettingsTests extends ESTestCase {
         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)));
+            Collections.singleton(SecureSetting.secureString("some.secure.setting", null)));
 
         Settings diffed = clusterSettings.diff(Settings.EMPTY, settings);
         assertTrue(diffed.isEmpty());

core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -555,4 +555,11 @@ public class SettingsTests extends ESTestCase {
         MockSecureSettings secureSettings = new MockSecureSettings();
         assertTrue(Settings.builder().setSecureSettings(secureSettings).build().isEmpty());
     }
+
+    public void testSecureSettingConflict() {
+        Setting<SecureString> setting = SecureSetting.secureString("something.secure", null);
+        Settings settings = Settings.builder().put("something.secure", "notreallysecure").build();
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> setting.get(settings));
+        assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore"));
+    }
 }

core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java

@@ -178,7 +178,7 @@ public class InternalSettingsPreparerTests extends ESTestCase {
         secureSettings.setString("foo", "secret");
         Settings input = Settings.builder().put(baseEnvSettings).setSecureSettings(secureSettings).build();
         Environment env = InternalSettingsPreparer.prepareEnvironment(input, null);
-        Setting<SecureString> fakeSetting = SecureSetting.secureString("foo", null, false);
+        Setting<SecureString> fakeSetting = SecureSetting.secureString("foo", null);
         assertEquals("secret", fakeSetting.get(env.settings()).toString());
     }
 

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java

@@ -178,10 +178,10 @@ interface AwsEc2Service {
         }
 
         /** The access key (ie login id) for connecting to ec2. */
-        Setting<SecureString> ACCESS_KEY_SETTING = SecureSetting.secureString("discovery.ec2.access_key", CLOUD_EC2.KEY_SETTING, false);
+        Setting<SecureString> ACCESS_KEY_SETTING = SecureSetting.secureString("discovery.ec2.access_key", CLOUD_EC2.KEY_SETTING);
 
         /** The secret key (ie password) for connecting to ec2. */
-        Setting<SecureString> SECRET_KEY_SETTING = SecureSetting.secureString("discovery.ec2.secret_key", CLOUD_EC2.SECRET_SETTING, false);
+        Setting<SecureString> SECRET_KEY_SETTING = SecureSetting.secureString("discovery.ec2.secret_key", CLOUD_EC2.SECRET_SETTING);
 
         /** An override for the ec2 endpoint to connect to. */
         Setting<String> ENDPOINT_SETTING = new Setting<>("discovery.ec2.endpoint", CLOUD_EC2.ENDPOINT_SETTING,
@@ -201,11 +201,11 @@ interface AwsEc2Service {
 
         /** The username of a proxy to connect to s3 through. */
         Setting<SecureString> PROXY_USERNAME_SETTING = SecureSetting.secureString("discovery.ec2.proxy.username",
-            CLOUD_EC2.PROXY_USERNAME_SETTING, false);
+            CLOUD_EC2.PROXY_USERNAME_SETTING);
 
         /** The password of a proxy to connect to s3 through. */
         Setting<SecureString> PROXY_PASSWORD_SETTING =  SecureSetting.secureString("discovery.ec2.proxy.password",
-            CLOUD_EC2.PROXY_PASSWORD_SETTING, false);
+            CLOUD_EC2.PROXY_PASSWORD_SETTING);
 
         /** The socket timeout for connecting to s3. */
         Setting<TimeValue> READ_TIMEOUT_SETTING = Setting.timeSetting("discovery.ec2.read_timeout",

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

@@ -65,11 +65,11 @@ class S3Repository extends BlobStoreRepository {
 
     /** The access key (ie login id) for connecting to s3. */
     public static final AffixSetting<SecureString> ACCESS_KEY_SETTING = Setting.affixKeySetting(PREFIX, "access_key",
-        key -> SecureSetting.secureString(key, Repositories.KEY_SETTING, false));
+        key -> SecureSetting.secureString(key, Repositories.KEY_SETTING));
 
     /** The secret key (ie password) for connecting to s3. */
     public static final AffixSetting<SecureString> SECRET_KEY_SETTING = Setting.affixKeySetting(PREFIX, "secret_key",
-        key -> SecureSetting.secureString(key, Repositories.SECRET_SETTING, false));
+        key -> SecureSetting.secureString(key, Repositories.SECRET_SETTING));
 
     /** An override for the s3 endpoint to connect to. */
     public static final AffixSetting<String> ENDPOINT_SETTING = Setting.affixKeySetting(PREFIX, "endpoint",
@@ -89,11 +89,11 @@ class S3Repository extends BlobStoreRepository {
 
     /** The username of a proxy to connect to s3 through. */
     public static final AffixSetting<SecureString> PROXY_USERNAME_SETTING = Setting.affixKeySetting(PREFIX, "proxy.username",
-        key -> SecureSetting.secureString(key, AwsS3Service.PROXY_USERNAME_SETTING, false));
+        key -> SecureSetting.secureString(key, AwsS3Service.PROXY_USERNAME_SETTING));
 
     /** The password of a proxy to connect to s3 through. */
     public static final AffixSetting<SecureString> PROXY_PASSWORD_SETTING = Setting.affixKeySetting(PREFIX, "proxy.password",
-        key -> SecureSetting.secureString(key, AwsS3Service.PROXY_PASSWORD_SETTING, false));
+        key -> SecureSetting.secureString(key, AwsS3Service.PROXY_PASSWORD_SETTING));
 
     /** The socket timeout for connecting to s3. */
     public static final AffixSetting<TimeValue> READ_TIMEOUT_SETTING = Setting.affixKeySetting(PREFIX, "read_timeout",