Skip to content

Commit

Permalink
Added insecureString options to allow to easier migration (#5496)
Browse files Browse the repository at this point in the history
Signed-off-by: Chris White <[email protected]>
  • Loading branch information
chriswhite199 authored Jun 16, 2023
1 parent 606255f commit 0b775e7
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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;
}
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}

0 comments on commit 0b775e7

Please sign in to comment.