diff --git a/CHANGELOG.md b/CHANGELOG.md index b964dc5844757..b6f2b534eb379 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/server/src/main/java/org/opensearch/common/settings/SecureSetting.java b/server/src/main/java/org/opensearch/common/settings/SecureSetting.java index 13abbcdc706e0..8e304a506399a 100644 --- a/server/src/main/java/org/opensearch/common/settings/SecureSetting.java +++ b/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(String name, Setting 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 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 { + 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; + } + } } /** diff --git a/server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java b/server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java new file mode 100644 index 0000000000000..cca8deda2964d --- /dev/null +++ b/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 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(); + } +}