[Entitlements] Instrument write access to System properties (#120357) (#120544)

mosche · web-flow · commit 2b2ae57d042a · 2025-01-22T05:13:42.000+11:00
Instrument write access to System properties by means of the `WriteSystemPropertiesEntitlement`.
`System.setProperties(Properties)` is always denied.

Part of #ES-10359
diff --git a/libs/entitlement/bridge/src/main/java/org/elasticsearch/entitlement/bridge/EntitlementChecker.java b/libs/entitlement/bridge/src/main/java/org/elasticsearch/entitlement/bridge/EntitlementChecker.java
@@ -42,6 +42,7 @@
 import java.nio.channels.SocketChannel;
 import java.security.cert.CertStoreParameters;
 import java.util.List;
+import java.util.Properties;
 
 import javax.net.ssl.HostnameVerifier;
 import javax.net.ssl.HttpsURLConnection;
@@ -121,6 +122,15 @@ public interface EntitlementChecker {
 
     void check$java_lang_ProcessBuilder$$startPipeline(Class<?> callerClass, List<ProcessBuilder> builders);
 
+    ////////////////////
+    //
+    // System Properties and similar
+    //
+
+    void check$java_lang_System$$setProperty(Class<?> callerClass, String key, String value);
+
+    void check$java_lang_System$$clearProperty(Class<?> callerClass, String key);
+
     ////////////////////
     //
     // JVM-wide state changes
@@ -132,6 +142,8 @@ public interface EntitlementChecker {
 
     void check$java_lang_System$$setErr(Class<?> callerClass, PrintStream err);
 
+    void check$java_lang_System$$setProperties(Class<?> callerClass, Properties props);
+
     void check$java_lang_Runtime$addShutdownHook(Class<?> callerClass, Runtime runtime, Thread hook);
 
     void check$java_lang_Runtime$removeShutdownHook(Class<?> callerClass, Runtime runtime, Thread hook);
diff --git a/libs/entitlement/qa/common/src/main/java/org/elasticsearch/entitlement/qa/common/RestEntitlementsCheckAction.java b/libs/entitlement/qa/common/src/main/java/org/elasticsearch/entitlement/qa/common/RestEntitlementsCheckAction.java
@@ -124,6 +124,10 @@ static CheckAction alwaysDenied(CheckedRunnable<Exception> action) {
         entry("timeZoneNameProvider", alwaysDenied(RestEntitlementsCheckAction::timeZoneNameProvider$)),
         entry("logManager", alwaysDenied(RestEntitlementsCheckAction::logManager$)),
 
+        entry("system_setProperty", forPlugins(WritePropertiesCheckActions::setSystemProperty)),
+        entry("system_clearProperty", forPlugins(WritePropertiesCheckActions::clearSystemProperty)),
+        entry("system_setSystemProperties", alwaysDenied(WritePropertiesCheckActions::setSystemProperties)),
+
         // This group is a bit nasty: if entitlements don't prevent these, then networking is
         // irreparably borked for the remainder of the test run.
         entry(
diff --git a/libs/entitlement/qa/common/src/main/java/org/elasticsearch/entitlement/qa/common/WritePropertiesCheckActions.java b/libs/entitlement/qa/common/src/main/java/org/elasticsearch/entitlement/qa/common/WritePropertiesCheckActions.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.qa.common;
+
+import org.elasticsearch.core.SuppressForbidden;
+
+@SuppressForbidden(reason = "testing entitlements")
+class WritePropertiesCheckActions {
+    private WritePropertiesCheckActions() {}
+
+    static void setSystemProperty() {
+        System.setProperty("es.entitlements.checkSetSystemProperty", "true");
+        try {
+            System.clearProperty("es.entitlements.checkSetSystemProperty");
+        } catch (RuntimeException e) {
+            // ignore for this test case
+        }
+
+    }
+
+    static void clearSystemProperty() {
+        System.clearProperty("es.entitlements.checkClearSystemProperty");
+    }
+
+    static void setSystemProperties() {
+        System.setProperties(System.getProperties()); // no side effect in case if allowed (but shouldn't)
+    }
+}
diff --git a/libs/entitlement/qa/entitlement-allowed-nonmodular/src/main/plugin-metadata/entitlement-policy.yaml b/libs/entitlement/qa/entitlement-allowed-nonmodular/src/main/plugin-metadata/entitlement-policy.yaml
@@ -3,3 +3,7 @@ ALL-UNNAMED:
   - set_https_connection_properties
   - inbound_network
   - outbound_network
+  - write_system_properties:
+      properties:
+        - es.entitlements.checkSetSystemProperty
+        - es.entitlements.checkClearSystemProperty
diff --git a/libs/entitlement/qa/entitlement-allowed/src/main/plugin-metadata/entitlement-policy.yaml b/libs/entitlement/qa/entitlement-allowed/src/main/plugin-metadata/entitlement-policy.yaml
@@ -3,3 +3,7 @@ org.elasticsearch.entitlement.qa.common:
   - set_https_connection_properties
   - inbound_network
   - outbound_network
+  - write_system_properties:
+      properties:
+        - es.entitlements.checkSetSystemProperty
+        - es.entitlements.checkClearSystemProperty
diff --git a/libs/entitlement/qa/entitlement-denied-nonmodular/src/main/plugin-metadata/entitlement-policy.yaml b/libs/entitlement/qa/entitlement-denied-nonmodular/src/main/plugin-metadata/entitlement-policy.yaml
@@ -0,0 +1,5 @@
+ALL-UNNAMED:
+  - write_system_properties:
+      properties:
+        # entitlement itself not sufficient, also no wildcard support
+        - "*"
diff --git a/libs/entitlement/qa/entitlement-denied/src/main/plugin-metadata/entitlement-policy.yaml b/libs/entitlement/qa/entitlement-denied/src/main/plugin-metadata/entitlement-policy.yaml
@@ -0,0 +1,5 @@
+org.elasticsearch.entitlement.qa.common:
+  - write_system_properties:
+      properties:
+        # entitlement itself not sufficient, also no wildcard support
+        - "*"
diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
@@ -46,6 +46,7 @@
 import java.nio.channels.SocketChannel;
 import java.security.cert.CertStoreParameters;
 import java.util.List;
+import java.util.Properties;
 
 import javax.net.ssl.HostnameVerifier;
 import javax.net.ssl.HttpsURLConnection;
@@ -211,6 +212,21 @@ public ElasticsearchEntitlementChecker(PolicyManager policyManager) {
         policyManager.checkChangeJVMGlobalState(callerClass);
     }
 
+    @Override
+    public void check$java_lang_System$$clearProperty(Class<?> callerClass, String key) {
+        policyManager.checkWriteProperty(callerClass, key);
+    }
+
+    @Override
+    public void check$java_lang_System$$setProperty(Class<?> callerClass, String key, String value) {
+        policyManager.checkWriteProperty(callerClass, key);
+    }
+
+    @Override
+    public void check$java_lang_System$$setProperties(Class<?> callerClass, Properties props) {
+        policyManager.checkChangeJVMGlobalState(callerClass);
+    }
+
     @Override
     public void check$java_util_spi_LocaleServiceProvider$(Class<?> callerClass) {
         policyManager.checkChangeJVMGlobalState(callerClass);
diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
@@ -237,6 +237,34 @@ public void checkAllNetworkAccess(Class<?> callerClass) {
         );
     }
 
+    public void checkWriteProperty(Class<?> callerClass, String property) {
+        var requestingClass = requestingClass(callerClass);
+        if (isTriviallyAllowed(requestingClass)) {
+            return;
+        }
+
+        ModuleEntitlements entitlements = getEntitlements(requestingClass);
+        if (entitlements.getEntitlements(WriteSystemPropertiesEntitlement.class).anyMatch(e -> e.properties().contains(property))) {
+            logger.debug(
+                () -> Strings.format(
+                    "Entitled: class [%s], module [%s], entitlement [write_system_properties], property [%s]",
+                    requestingClass,
+                    requestingClass.getModule().getName(),
+                    property
+                )
+            );
+            return;
+        }
+        throw new NotEntitledException(
+            Strings.format(
+                "Missing entitlement: class [%s], module [%s], entitlement [write_system_properties], property [%s]",
+                requestingClass,
+                requestingClass.getModule().getName(),
+                property
+            )
+        );
+    }
+
     private void checkEntitlementPresent(Class<?> callerClass, Class<? extends Entitlement> entitlementClass) {
         var requestingClass = requestingClass(callerClass);
         if (isTriviallyAllowed(requestingClass)) {
diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java
@@ -40,7 +40,8 @@ public class PolicyParser {
         CreateClassLoaderEntitlement.class,
         SetHttpsConnectionPropertiesEntitlement.class,
         OutboundNetworkEntitlement.class,
-        InboundNetworkEntitlement.class
+        InboundNetworkEntitlement.class,
+        WriteSystemPropertiesEntitlement.class
     ).collect(Collectors.toUnmodifiableMap(PolicyParser::getEntitlementTypeName, Function.identity()));
 
     protected final XContentParser policyParser;
diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/WriteAllSystemPropertiesEntitlement.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/WriteAllSystemPropertiesEntitlement.java
@@ -0,0 +1,18 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.runtime.policy;
+
+/**
+ * An Entitlement to allow writing all properties such as system properties.
+ */
+public record WriteAllSystemPropertiesEntitlement() implements Entitlement {
+    @ExternalEntitlement
+    public WriteAllSystemPropertiesEntitlement() {}
+}
diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/WriteSystemPropertiesEntitlement.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/WriteSystemPropertiesEntitlement.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.runtime.policy;
+
+import java.util.List;
+import java.util.Set;
+
+/**
+ * An Entitlement to allow writing properties such as system properties.
+ */
+public record WriteSystemPropertiesEntitlement(Set<String> properties) implements Entitlement {
+
+    @ExternalEntitlement(parameterNames = { "properties" }, esModulesOnly = false)
+    public WriteSystemPropertiesEntitlement(List<String> properties) {
+        this(Set.copyOf(properties));
+    }
+}
diff --git a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyParserTests.java b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyParserTests.java
@@ -15,6 +15,7 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
+import java.util.Set;
 
 import static org.hamcrest.Matchers.equalTo;
 
@@ -82,6 +83,23 @@ public void testParseNetwork() throws IOException {
         assertEquals(expected, parsedPolicy);
     }
 
+    public void testParseWriteSystemProperties() throws IOException {
+        Policy parsedPolicy = new PolicyParser(new ByteArrayInputStream("""
+            entitlement-module-name:
+              - write_system_properties:
+                  properties:
+                    - es.property1
+                    - es.property2
+            """.getBytes(StandardCharsets.UTF_8)), "test-policy.yaml", false).parsePolicy();
+        Policy expected = new Policy(
+            "test-policy.yaml",
+            List.of(
+                new Scope("entitlement-module-name", List.of(new WriteSystemPropertiesEntitlement(Set.of("es.property1", "es.property2"))))
+            )
+        );
+        assertEquals(expected, parsedPolicy);
+    }
+
     public void testParseCreateClassloader() throws IOException {
         Policy parsedPolicy = new PolicyParser(new ByteArrayInputStream("""
             entitlement-module-name:
diff --git a/modules/apm/build.gradle b/modules/apm/build.gradle
@@ -5,6 +5,7 @@
  * 2.0.
  */
 apply plugin: 'elasticsearch.internal-es-plugin'
+apply plugin: 'elasticsearch.internal-java-rest-test'
 
 esplugin {
   name = 'apm'
@@ -20,6 +21,9 @@ dependencies {
   implementation "io.opentelemetry:opentelemetry-context:${otelVersion}"
   implementation "io.opentelemetry:opentelemetry-semconv:${otelSemconvVersion}"
   runtimeOnly    "co.elastic.apm:elastic-apm-agent:1.52.0"
+
+  javaRestTestImplementation project(':modules:apm')
+  javaRestTestImplementation project(':test:framework')
 }
 
 tasks.named("dependencyLicenses").configure {
diff --git a/modules/apm/src/javaRestTest/java/org/elasticsearch/telemetry/apm/ApmAgentSettingsIT.java b/modules/apm/src/javaRestTest/java/org/elasticsearch/telemetry/apm/ApmAgentSettingsIT.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.telemetry.apm;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.telemetry.apm.internal.APMAgentSettings;
+import org.elasticsearch.test.cluster.ElasticsearchCluster;
+import org.elasticsearch.test.rest.ESRestTestCase;
+import org.junit.ClassRule;
+
+public class ApmAgentSettingsIT extends ESRestTestCase {
+
+    @ClassRule
+    public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
+        .module("apm")
+        .systemProperty("es.entitlements.enabled", "true")
+        .build();
+
+    @Override
+    protected String getTestRestCluster() {
+        return cluster.getHttpAddresses();
+    }
+
+    public void testChangingApmAgentSettingsIsAllowed() throws Exception {
+        var settings = Settings.builder().put("telemetry.metrics.enabled", true);
+        APMAgentSettings.PERMITTED_AGENT_KEYS.stream().forEach(key -> settings.put("telemetry.agent." + key, "value"));
+        updateClusterSettings(settings.build());
+
+        updateClusterSettings(Settings.builder().put("telemetry.metrics.enabled", false).build());
+    }
+}
diff --git a/modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettings.java b/modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettings.java
@@ -81,6 +81,10 @@ public void initAgentSystemProperties(Settings settings) {
     /**
      * Copies a setting to the APM agent's system properties under <code>elastic.apm</code>, either
      * by setting the property if {@code value} has a value, or by deleting the property if it doesn't.
+     *
+     * All permitted agent properties must be covered by the <code>write_system_properties</code> entitlement,
+     * see the entitlement policy of this module!
+     *
      * @param key the config key to set, without any prefix
      * @param value the value to set, or <code>null</code>
      */
@@ -106,9 +110,11 @@ public void setAgentSetting(String key, String value) {
 
     /**
      * Allow-list of APM agent config keys users are permitted to configure.
+     * <p><b>WARNING</b>: Make sure to update the module entitlements if permitting additional agent keys
+     * </p>
      * @see <a href="https://www.elastic.co/guide/en/apm/agent/java/current/configuration.html">APM Java Agent Configuration</a>
      */
-    private static final Set<String> PERMITTED_AGENT_KEYS = Set.of(
+    public static final Set<String> PERMITTED_AGENT_KEYS = Set.of(
         // Circuit-Breaker:
         "circuit_breaker_enabled",
         "stress_monitoring_interval",
diff --git a/modules/apm/src/main/plugin-metadata/entitlement-policy.yaml b/modules/apm/src/main/plugin-metadata/entitlement-policy.yaml
diff --git a/plugins/repository-hdfs/src/main/plugin-metadata/entitlement-policy.yaml b/plugins/repository-hdfs/src/main/plugin-metadata/entitlement-policy.yaml
diff --git a/x-pack/plugin/core/src/main/plugin-metadata/entitlement-policy.yaml b/x-pack/plugin/core/src/main/plugin-metadata/entitlement-policy.yaml
diff --git a/x-pack/plugin/identity-provider/src/main/plugin-metadata/entitlement-policy.yaml b/x-pack/plugin/identity-provider/src/main/plugin-metadata/entitlement-policy.yaml
diff --git a/x-pack/plugin/security/src/main/plugin-metadata/entitlement-policy.yaml b/x-pack/plugin/security/src/main/plugin-metadata/entitlement-policy.yaml
