Don't allow secure settings in YML config (109115) (elastic#115779)

* Don't allow secure settings in YML config (109115)

Elasticsearch should refuse to start
if a secure setting is defined in elasticsearch.yml,
in order to protect users from accidentally putting their secrets
in a place where they are unexpectedly visible

Fixes elastic#109115

docs/changelog/115779.yaml

@@ -0,0 +1,6 @@
+pr: 115779
+summary: Don't allow secure settings in YML config (109115)
+area: Infra/Settings
+type: bug
+issues:
+ - 109115

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -599,6 +599,15 @@ void validate(final String key, final Settings settings, final boolean validateV
                     );
                 }
             }
+
+            if (setting instanceof SecureSetting && settings.hasValue(key)) {
+                throw new IllegalArgumentException(
+                    "Setting ["
+                        + key
+                        + "] is a secure setting"
+                        + " and must be stored inside the Elasticsearch keystore, but was found inside elasticsearch.yml"
+                );
+            }
         }
         if (validateValue) {
             setting.get(settings);

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

@@ -82,21 +82,14 @@ public boolean exists(Settings.Builder builder) {
     public T get(Settings settings) {
         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"
-                );
-            }
+        String key = getKey();
+        if (secureSettings == null || secureSettings.getSettingNames().contains(key) == false) {
             return getFallback(settings);
         }
         try {
             return getSecret(secureSettings);
         } catch (GeneralSecurityException e) {
-            throw new RuntimeException("failed to read secure setting " + getKey(), e);
+            throw new RuntimeException("failed to read secure setting " + key, e);
         }
     }
 

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

@@ -1138,6 +1138,42 @@ public void testDiffSecureSettings() {
         assertTrue(diffed.isEmpty());
     }
 
+    public void testValidateSecureSettingInsecureOverride() {
+        MockSecureSettings secureSettings = new MockSecureSettings();
+        String settingName = "something.secure";
+        secureSettings.setString(settingName, "secure");
+        Settings settings = Settings.builder().put(settingName, "notreallysecure").setSecureSettings(secureSettings).build();
+
+        ClusterSettings clusterSettings = new ClusterSettings(
+            settings,
+            Collections.singleton(SecureSetting.secureString(settingName, null))
+        );
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> clusterSettings.validate(settings, false));
+        assertEquals(
+            e.getMessage(),
+            "Setting [something.secure] is a secure setting "
+                + "and must be stored inside the Elasticsearch keystore, but was found inside elasticsearch.yml"
+        );
+    }
+
+    public void testValidateSecureSettingInInsecureSettings() {
+        String settingName = "something.secure";
+        Settings settings = Settings.builder().put(settingName, "notreallysecure").build();
+
+        ClusterSettings clusterSettings = new ClusterSettings(
+            settings,
+            Collections.singleton(SecureSetting.secureString(settingName, null))
+        );
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> clusterSettings.validate(settings, false));
+        assertEquals(
+            e.getMessage(),
+            "Setting [something.secure] is a secure setting "
+                + "and must be stored inside the Elasticsearch keystore, but was found inside elasticsearch.yml"
+        );
+    }
+
     public static IndexMetadata newIndexMeta(String name, Settings indexSettings) {
         return IndexMetadata.builder(name).settings(indexSettings(IndexVersion.current(), 1, 0).put(indexSettings)).build();
     }

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

@@ -473,13 +473,6 @@ public void testDiff() throws IOException {
         }
     }
 
-    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"));
-    }
-
     public void testSecureSettingIllegalName() {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> SecureSetting.secureString("*IllegalName", null));
         assertTrue(e.getMessage().contains("does not match the allowed setting name pattern"));