Added insecureString options to allow to easier migration to secure setting variants

CHANGELOG.md

@@ -120,6 +120,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Compress and cache cluster state during validate join request ([#7321](https://github.com/opensearch-project/OpenSearch/pull/7321))
 - [Snapshot Interop] Add Changes in Create Snapshot Flow for remote store interoperability. ([#7118](https://github.com/opensearch-project/OpenSearch/pull/7118))
 - Add new query profile collector fields with concurrent search execution ([#7898](https://github.com/opensearch-project/OpenSearch/pull/7898))
+- Allow insecure string settings to warn-log usage and advise to migration of a newer secure variant ([#5496](https://github.com/opensearch-project/OpenSearch/pull/5496))
 
 ### Deprecated
 

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

@@ -40,6 +40,9 @@
 import java.util.EnumSet;
 import java.util.Set;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
 /**
  * A secure setting.
  *
@@ -161,7 +164,17 @@ public static Setting<SecureString> secureString(String name, Setting<SecureStri
      * @see #secureString(String, Setting, Property...)
      */
     public static Setting<SecureString> insecureString(String name) {
-        return new InsecureStringSetting(name);
+        return insecureString(name, null, false);
+    }
+
+    /**
+     * A setting which contains a sensitive string, but usage is logged when found outside secure settings, regardless
+     * of the opensearch.allow_insecure_settings value. Typically. this is used when migrating old legacy settings
+     * to secure variants while preserving existing functionality.
+     * @see #insecureString(String)
+     */
+    public static Setting<SecureString> insecureString(String name, String secureName, boolean allowWithWarning) {
+        return new InsecureStringSetting(name, secureName, allowWithWarning);
     }
 
     /**
@@ -206,22 +219,40 @@ SecureString getFallback(Settings settings) {
      * @opensearch.internal
      */
     private static class InsecureStringSetting extends Setting<SecureString> {
+        private static final Logger LOG = LogManager.getLogger(InsecureStringSetting.class);
         private final String name;
+        private final String secureName;
+        private final boolean allowWithWarning;
+
+        private boolean warningLogged;
 
-        private InsecureStringSetting(String name) {
+        private InsecureStringSetting(String name, String secureName, boolean allowWithWarning) {
             super(name, "", SecureString::new, Property.Deprecated, Property.Filtered, Property.NodeScope);
             this.name = name;
+            this.secureName = secureName;
+            this.allowWithWarning = allowWithWarning;
         }
 
         @Override
         public SecureString get(Settings settings) {
-            if (ALLOW_INSECURE_SETTINGS == false && exists(settings)) {
-                throw new IllegalArgumentException(
-                    "Setting [" + name + "] is insecure, " + "but property [allow_insecure_settings] is not set"
-                );
+            if (exists(settings)) {
+                logUsage();
+
+                if (ALLOW_INSECURE_SETTINGS == false && this.allowWithWarning == false) {
+                    throw new IllegalArgumentException(
+                        "Setting [" + name + "] is insecure, " + "but property [allow_insecure_settings] is not set"
+                    );
+                }
             }
             return super.get(settings);
         }
+
+        private synchronized void logUsage() {
+            if (!this.warningLogged) {
+                LOG.warn("Setting [{}] is insecure, but a secure variant [{}] is advised to be used instead", this.name, this.secureName);
+                this.warningLogged = true;
+            }
+        }
     }
 
     /**

server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java

@@ -0,0 +1,114 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.common.settings;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Property;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+
+import org.opensearch.common.logging.Loggers;
+import org.opensearch.test.OpenSearchTestCase;
+
+public class InsecureSettingTests extends OpenSearchTestCase {
+    private List<String> rootLogMsgs = new ArrayList<>();
+    private AbstractAppender rootAppender;
+
+    protected void assertSettingWarning() {
+        assertWarnings(
+            "[setting.name] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version."
+        );
+
+        Assert.assertTrue(
+            this.rootLogMsgs.stream()
+                .anyMatch(
+                    msg -> msg.equals(
+                        "Setting [setting.name] is insecure, but a secure variant [setting.name_secure] is advised to be used instead"
+                    )
+                )
+        );
+    }
+
+    @Before
+    public void addInsecureSettingsAppender() {
+        this.rootLogMsgs.clear();
+        rootAppender = new AbstractAppender("root", null, null, true, Property.EMPTY_ARRAY) {
+            @Override
+            public void append(LogEvent event) {
+                String message = event.getMessage().getFormattedMessage();
+                InsecureSettingTests.this.rootLogMsgs.add(message);
+            }
+        };
+        Loggers.addAppender(LogManager.getRootLogger(), rootAppender);
+        rootAppender.start();
+    }
+
+    @After
+    public void removeInsecureSettingsAppender() {
+        Loggers.removeAppender(LogManager.getRootLogger(), rootAppender);
+    }
+
+    public void testShouldRaiseExceptionByDefault() {
+        final var setting = SecureSetting.insecureString("setting.name");
+        final var settings = Settings.builder().put(setting.getKey(), "value").build();
+
+        final var exception = Assert.assertThrows(IllegalArgumentException.class, () -> setting.get(settings));
+        Assert.assertEquals(
+            "Setting [setting.name] is insecure, but property [allow_insecure_settings] is not set",
+            exception.getMessage()
+        );
+    }
+
+    public void testShouldLogWarn() {
+        final var setting = SecureSetting.insecureString("setting.name", "setting.name_secure", true);
+        final var settings = Settings.builder().put(setting.getKey(), "value").build();
+
+        Assert.assertEquals("value", setting.get(settings).toString());
+        assertSettingWarning();
+    }
+
+    public void testShouldLogWarnOnce() {
+        final var setting = SecureSetting.insecureString("setting.name", "setting.name_secure", true);
+        final var settings = Settings.builder().put(setting.getKey(), "value").build();
+
+        Assert.assertEquals("value", setting.get(settings).toString());
+        Assert.assertEquals("value", setting.get(settings).toString());
+        assertSettingWarning();
+
+        // check that warning was only logged once
+        Assert.assertEquals(1, this.rootLogMsgs.stream().filter(msg -> msg.contains("but a secure variant")).count());
+
+    }
+
+    public void testShouldRaiseExceptionIfConfigured() {
+        final var setting = SecureSetting.insecureString("setting.name", "setting.name_secure", false);
+        final var settings = Settings.builder().put(setting.getKey(), "value").build();
+
+        final var exception = Assert.assertThrows(IllegalArgumentException.class, () -> setting.get(settings));
+        Assert.assertEquals(
+            "Setting [setting.name] is insecure, but property [allow_insecure_settings] is not set",
+            exception.getMessage()
+        );
+    }
+
+    public void testShouldFallbackToInsecure() {
+        final var insecureSetting = SecureSetting.insecureString("setting.name", "setting.name_secure", true);
+        final var secureSetting = SecureSetting.secureString("setting.name_secure", insecureSetting);
+        final var settings = Settings.builder().put(insecureSetting.getKey(), "value").build();
+
+        Assert.assertEquals("value", secureSetting.get(settings).toString());
+        assertSettingWarning();
+    }
+}