[Backport 2.x] Redact sensitive configuration values when retrieving …

…security configuration (opensearch-project#4028) Backport a41b3f7 from opensearch-project#4024. Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
cwperks · Feb 8, 2024 · 4615b49 · 4615b49
1 parent cee8868
commit 4615b49
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 2 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/http/LdapAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/LdapAuthenticationTest.java
@@ -41,6 +41,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.equalTo;
 import static org.opensearch.security.http.CertificateAuthenticationTest.POINTER_BACKEND_ROLES;
 import static org.opensearch.security.http.DirectoryInformationTrees.CN_GROUP_ADMIN;
 import static org.opensearch.security.http.DirectoryInformationTrees.DN_CAPTAIN_SPOCK_PEOPLE_TEST_ORG;
@@ -55,6 +56,8 @@
 import static org.opensearch.security.http.DirectoryInformationTrees.USER_KIRK;
 import static org.opensearch.security.http.DirectoryInformationTrees.USER_SEARCH;
 import static org.opensearch.security.http.DirectoryInformationTrees.USER_SPOCK;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
 import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
 import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER;
 import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
@@ -83,7 +86,16 @@ public class LdapAuthenticationTest {
     public static LocalCluster cluster = new LocalCluster.Builder().testCertificates(TEST_CERTIFICATES)
         .clusterManager(ClusterManager.SINGLENODE)
         .anonymousAuth(false)
-        .nodeSettings(Map.of(ConfigConstants.SECURITY_AUTHCZ_REST_IMPERSONATION_USERS + "." + ADMIN_USER.getName(), List.of(USER_KIRK)))
+        .nodeSettings(
+            Map.of(
+                ConfigConstants.SECURITY_AUTHCZ_REST_IMPERSONATION_USERS + "." + ADMIN_USER.getName(),
+                List.of(USER_KIRK),
+                SECURITY_RESTAPI_ROLES_ENABLED,
+                List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()),
+                SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST,
+                false
+            )
+        )
         .authc(
             new AuthcDomain("ldap", BASIC_AUTH_DOMAIN_ORDER + 1, true).httpAuthenticator(new HttpAuthenticator("basic").challenge(false))
                 .backend(
@@ -190,4 +202,15 @@ public void testShouldCreateScrollWithLdapUserAndImpersonateWithAdmin() {
             scrollResponse.assertStatusCode(200);
         }
     }
+
+    @Test
+    public void testShouldRedactPasswordWhenGettingSecurityConfig() {
+        try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) {
+            TestRestClient.HttpResponse response = client.get("_plugins/_security/api/securityconfig");
+
+            response.assertStatusCode(200);
+            String redactedPassword = response.getTextFromJsonBody("/config/dynamic/authc/ldap/authentication_backend/config/password");
+            assertThat("******", equalTo(redactedPassword));
+        }
+    }
 }
diff --git a/src/main/java/org/opensearch/security/DefaultObjectMapper.java b/src/main/java/org/opensearch/security/DefaultObjectMapper.java
@@ -35,21 +35,48 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.InjectableValues;
 import com.fasterxml.jackson.databind.JavaType;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
 import com.fasterxml.jackson.databind.exc.InvalidFormatException;
 import com.fasterxml.jackson.databind.exc.MismatchedInputException;
 import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
 import com.fasterxml.jackson.databind.type.TypeFactory;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 
 import org.opensearch.SpecialPermission;
 
+class ConfigMapSerializer extends StdSerializer<Map<String, Object>> {
+    private static final Set<String> SENSITIVE_CONFIG_KEYS = Set.of("password");
+
+    @SuppressWarnings("unchecked")
+    public ConfigMapSerializer() {
+        // Pass Map<String, Object>.class to the superclass
+        super((Class<Map<String, Object>>) (Class<?>) Map.class);
+    }
+
+    @Override
+    public void serialize(Map<String, Object> value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
+        gen.writeStartObject();
+        for (Map.Entry<String, Object> entry : value.entrySet()) {
+            if (SENSITIVE_CONFIG_KEYS.contains(entry.getKey())) {
+                gen.writeStringField(entry.getKey(), "******"); // Redact
+            } else {
+                gen.writeObjectField(entry.getKey(), entry.getValue());
+            }
+        }
+        gen.writeEndObject();
+    }
+}
+
 public class DefaultObjectMapper {
     public static final ObjectMapper objectMapper = new ObjectMapper();
     public final static ObjectMapper YAML_MAPPER = new ObjectMapper(new YAMLFactory());
@@ -180,6 +207,27 @@ public static String writeValueAsString(Object value, boolean omitDefaults) thro
 
     }
 
+    @SuppressWarnings("removal")
+    public static String writeValueAsStringAndRedactSensitive(Object value) throws JsonProcessingException {
+        final SecurityManager sm = System.getSecurityManager();
+
+        if (sm != null) {
+            sm.checkPermission(new SpecialPermission());
+        }
+
+        SimpleModule module = new SimpleModule();
+        module.addSerializer(new ConfigMapSerializer());
+        ObjectMapper mapper = new ObjectMapper();
+        mapper.registerModule(module);
+
+        try {
+            return AccessController.doPrivileged((PrivilegedExceptionAction<String>) () -> mapper.writeValueAsString(value));
+        } catch (final PrivilegedActionException e) {
+            throw (JsonProcessingException) e.getCause();
+        }
+
+    }
+
     @SuppressWarnings("removal")
     public static <T> T readValue(String string, TypeReference<T> tr) throws IOException {
 

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
@@ -372,7 +372,12 @@ protected final ValidationResult<SecurityDynamicConfiguration<?>> loadConfigurat
         boolean omitSensitiveData,
         final boolean logComplianceEvent
     ) {
-        final var configuration = load(cType, logComplianceEvent);
+        SecurityDynamicConfiguration<?> configuration;
+        if (omitSensitiveData) {
+            configuration = loadAndRedact(cType, logComplianceEvent);
+        } else {
+            configuration = load(cType, logComplianceEvent);
+        }
         if (configuration.getSeqNo() < 0) {
 
             return ValidationResult.error(
@@ -448,6 +453,14 @@ protected final SecurityDynamicConfiguration<?> load(final CType config, boolean
         return DynamicConfigFactory.addStatics(loaded);
     }
 
+    protected final SecurityDynamicConfiguration<?> loadAndRedact(final CType config, boolean logComplianceEvent) {
+        SecurityDynamicConfiguration<?> loaded = securityApiDependencies.configurationRepository()
+            .getConfigurationsFromIndex(List.of(config), logComplianceEvent)
+            .get(config)
+            .deepCloneWithRedaction();
+        return DynamicConfigFactory.addStatics(loaded);
+    }
+
     protected boolean ensureIndexExists() {
         return clusterService.state().metadata().hasConcreteIndex(securityApiDependencies.securityIndexName());
     }

diff --git a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java
@@ -303,6 +303,15 @@ public SecurityDynamicConfiguration<T> deepClone() {
         }
     }
 
+    @JsonIgnore
+    public SecurityDynamicConfiguration<T> deepCloneWithRedaction() {
+        try {
+            return fromJson(DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), ctype, version, seqNo, primaryTerm);
+        } catch (Exception e) {
+            throw ExceptionsHelper.convertToOpenSearchException(e);
+        }
+    }
+
     @JsonIgnore
     public void remove(String key) {
         synchronized (modificationLock) {