Fixed integration tests with custom insecure settings

opensearch-project · Dec 6, 2022 · ec83367 · ec83367
1 parent b9e1833
commit ec83367
Show file tree

Hide file tree

Showing 4 changed files with 281 additions and 239 deletions.
diff --git a/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java b/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
@@ -319,8 +319,8 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
     public List<Setting<?>> getSettings() {
         List<Setting<?>> settings = new ArrayList<Setting<?>>();
 
-        // add secure settings
-        SecureSSLSettings.addSettingsToBuilder(settings);
+        // add secure settings (with fallbacks for legacy insecure settings)
+        settings.addAll(SecureSSLSettings.getSecureSettings());
 
         // add non secure settings
         settings.add(Setting.simpleString(SSLConfigConstants.SECURITY_SSL_HTTP_CLIENTAUTH_MODE, Property.NodeScope, Property.Filtered));

diff --git a/src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java b/src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java
@@ -17,6 +17,11 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 import org.opensearch.common.settings.SecureSetting;
 import org.opensearch.common.settings.SecureString;
@@ -29,6 +34,8 @@
  * Container for secured settings (passwords for certs, keystores) and the now deprecated original settings
  */
 public final class SecureSSLSettings {
+    private static final Logger LOG = LogManager.getLogger(SecureSSLSettings.class);
+
     private static final String SECURE_SUFFIX = "_secure";
     private static final String PREFIX = "plugins.security.ssl";
     private static final String HTTP_PREFIX = PREFIX + ".http";
@@ -57,7 +64,7 @@ public enum SSLSetting {
 
         SSLSetting(String insecurePropertyName, String defaultValue) {
             this.insecurePropertyName = insecurePropertyName;
-            this.propertyName = String.format("%s_%s", this.insecurePropertyName, SECURE_SUFFIX);
+            this.propertyName = String.format("%s%s", this.insecurePropertyName, SECURE_SUFFIX);
             this.defaultValue = defaultValue;
         }
 
@@ -69,7 +76,11 @@ public enum SSLSetting {
 
         public Setting<SecureString> asSetting() {
             return SecureSetting.secureString(this.propertyName,
-                    SecureSetting.insecureString(this.insecurePropertyName));
+                    new InsecureFallbackStringSetting(this.insecurePropertyName));
+        }
+
+        public Setting<SecureString> asInsecureSetting() {
+            return new InsecureFallbackStringSetting(this.insecurePropertyName);
         }
 
         public String getSetting(Settings settings) {
@@ -86,9 +97,31 @@ public String getSetting(Settings settings, String defaultValue) {
 
     private SecureSSLSettings() {}
 
-    public static void addSettingsToBuilder(List<Setting<?>> settings) {
-        Arrays.stream(SSLSetting.values()).forEach(setting -> {
-            settings.add(setting.asSetting());
-        });
+    public static List<Setting<?>> getSecureSettings() {
+        return Arrays.stream(SSLSetting.values())
+                .flatMap(setting -> Stream.of(setting.asSetting(), setting.asInsecureSetting()))
+                .collect(Collectors.toList());
+    }
+
+    /**
+     * Alternative to InsecureStringSetting, which doesn't raise an exception if allow_insecure_settings is false, but
+     * instead log.WARNs the violation. This is to appease a potential cyclic dependency between commons-utils
+     */
+    private static class InsecureFallbackStringSetting extends Setting<SecureString> {
+        private final String name;
+
+        private InsecureFallbackStringSetting(String name) {
+            super(name, "", s -> new SecureString(s.toCharArray()), Property.Deprecated, Property.Filtered, Property.NodeScope);
+            this.name = name;
+        }
+
+        public SecureString get(Settings settings) {
+            if (this.exists(settings)) {
+                LOG.warn("Setting [{}] has a secure counterpart [{}{}] which should be used instead - allowing for legacy SSL setups",
+                        this.name, this.name, SECURE_SUFFIX);
+            }
+
+            return super.get(settings);
+        }
     }
 }
diff --git a/src/test/java/org/opensearch/security/sanity/tests/SecurityRestTestCase.java b/src/test/java/org/opensearch/security/sanity/tests/SecurityRestTestCase.java
@@ -23,7 +23,6 @@
 import org.opensearch.client.RestClient;
 import org.opensearch.client.RestClientBuilder;
 import org.opensearch.common.io.PathUtils;
-import org.opensearch.common.settings.MockSecureSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.commons.rest.SecureRestClientBuilder;
 import org.opensearch.test.rest.OpenSearchRestTestCase;
@@ -55,10 +54,6 @@ private boolean securityEnabled() {
 
     @Override
     protected Settings restAdminSettings(){
-        MockSecureSettings mockSecureSettings = new MockSecureSettings();
-        mockSecureSettings.setString(SECURITY_SSL_HTTP_KEYSTORE_PASSWORD.propertyName, "changeit");
-        mockSecureSettings.setString(SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD.propertyName, "changeit");
-
         return Settings
                 .builder()
                 .put("http.port", 9200)
@@ -67,7 +62,8 @@ protected Settings restAdminSettings(){
                 .put(SECURITY_SSL_HTTP_PEMKEY_FILEPATH, CERT_FILE_DIRECTORY + "opensearch-node-key.pem")
                 .put(SECURITY_SSL_HTTP_PEMTRUSTEDCAS_FILEPATH, CERT_FILE_DIRECTORY + "root-ca.pem")
                 .put(SECURITY_SSL_HTTP_KEYSTORE_FILEPATH, CERT_FILE_DIRECTORY + "test-kirk.jks")
-                .setSecureSettings(mockSecureSettings)
+                .put(SECURITY_SSL_HTTP_KEYSTORE_PASSWORD.insecurePropertyName, "changeit")
+                .put(SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD.insecurePropertyName, "changeit")
                 .build();
     }