Add tier preference to security index settings allowlist and update default tier preference (#111818)

gwbrown · web-flow · commit b5da668ad940 · 2024-08-30T10:05:40.000+10:00
This commit allows tier preference for the security system indices to be
set using the Security Settings API, and adds validation to prevent
using the `data_frozen` tier for security system indices.

Also updates the default tier preference to `data_hot,data_content`.
diff --git a/docs/changelog/111818.yaml b/docs/changelog/111818.yaml
@@ -0,0 +1,5 @@
+pr: 111818
+summary: Add tier preference to security index settings allowlist
+area: Security
+type: enhancement
+issues: []
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsAction.java
@@ -14,9 +14,9 @@
 import org.elasticsearch.action.support.master.AcknowledgedRequest;
 import org.elasticsearch.action.support.master.AcknowledgedResponse;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.routing.allocation.DataTier;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
@@ -28,6 +28,8 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.stream.Collectors;
 
 import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
@@ -42,10 +44,35 @@ public class UpdateSecuritySettingsAction {
     public static final String TOKENS_INDEX_NAME = "security-tokens";
     public static final String PROFILES_INDEX_NAME = "security-profile";
 
-    public static final Set<String> ALLOWED_SETTING_KEYS = Set.of(
-        IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
-        IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS
-    );
+    /**
+     * A map of allowed settings to validators for those settings. Values should take the value which is being assigned to the setting
+     * and an existing {@link ActionRequestValidationException}, to which they should add if the value is disallowed.
+     */
+    public static final Map<
+        String,
+        BiFunction<Object, ActionRequestValidationException, ActionRequestValidationException>> ALLOWED_SETTING_VALIDATORS = Map.of(
+            IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
+            (it, ex) -> ex, // no additional validation
+            IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS,
+            (it, ex) -> ex, // no additional validation
+            DataTier.TIER_PREFERENCE,
+            (it, ex) -> {
+                Set<String> allowedTiers = Set.of(DataTier.DATA_CONTENT, DataTier.DATA_HOT, DataTier.DATA_WARM, DataTier.DATA_COLD);
+                if (it instanceof String preference) {
+                    String disallowedTiers = DataTier.parseTierList(preference)
+                        .stream()
+                        .filter(tier -> allowedTiers.contains(tier) == false)
+                        .collect(Collectors.joining(","));
+                    if (disallowedTiers.isEmpty() == false) {
+                        return ValidateActions.addValidationError(
+                            "disallowed data tiers [" + disallowedTiers + "] found, allowed tiers are [" + String.join(",", allowedTiers),
+                            ex
+                        );
+                    }
+                }
+                return ex;
+            }
+        );
 
     private UpdateSecuritySettingsAction() {/* no instances */}
 
@@ -154,19 +181,26 @@ private static ActionRequestValidationException validateIndexSettings(
             String indexName,
             ActionRequestValidationException existingExceptions
         ) {
-            Set<String> forbiddenSettings = Sets.difference(indexSettings.keySet(), ALLOWED_SETTING_KEYS);
-            if (forbiddenSettings.size() > 0) {
-                return ValidateActions.addValidationError(
-                    "illegal settings for index ["
-                        + indexName
-                        + "]: "
-                        + forbiddenSettings
-                        + ", these settings may not be configured. Only the following settings may be configured for that index: "
-                        + ALLOWED_SETTING_KEYS,
-                    existingExceptions
-                );
+            ActionRequestValidationException errors = existingExceptions;
+
+            for (Map.Entry<String, Object> entry : indexSettings.entrySet()) {
+                String setting = entry.getKey();
+                if (ALLOWED_SETTING_VALIDATORS.containsKey(setting)) {
+                    errors = ALLOWED_SETTING_VALIDATORS.get(setting).apply(entry.getValue(), errors);
+                } else {
+                    errors = ValidateActions.addValidationError(
+                        "illegal setting for index ["
+                            + indexName
+                            + "]: ["
+                            + setting
+                            + "], this setting may not be configured. Only the following settings may be configured for that index: "
+                            + ALLOWED_SETTING_VALIDATORS.keySet(),
+                        existingExceptions
+                    );
+                }
             }
-            return existingExceptions;
+
+            return errors;
         }
     }
 }
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsActionTests.java
@@ -7,15 +7,19 @@
 
 package org.elasticsearch.xpack.core.security.action.settings;
 
+import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.routing.allocation.DataTier;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.function.Supplier;
 import java.util.regex.Pattern;
 
-import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.ALLOWED_SETTING_KEYS;
+import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.ALLOWED_SETTING_VALIDATORS;
 import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.MAIN_INDEX_NAME;
 import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.PROFILES_INDEX_NAME;
 import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.TOKENS_INDEX_NAME;
@@ -27,6 +31,15 @@
 
 public class UpdateSecuritySettingsActionTests extends ESTestCase {
 
+    static final Map<String, Supplier<String>> ALLOWED_SETTING_GENERATORS = Map.of(
+        IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
+        () -> randomAlphaOfLength(5), // no additional validation
+        IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS,
+        () -> randomAlphaOfLength(5), // no additional validation
+        DataTier.TIER_PREFERENCE,
+        () -> randomFrom(DataTier.DATA_CONTENT, DataTier.DATA_HOT, DataTier.DATA_WARM, DataTier.DATA_COLD)
+    );
+
     public void testValidateSettingsEmpty() {
         var req = new UpdateSecuritySettingsAction.Request(
             TEST_REQUEST_TIMEOUT,
@@ -43,9 +56,10 @@ public void testValidateSettingsEmpty() {
 
     public void testAllowedSettingsOk() {
         Map<String, Object> allAllowedSettingsMap = new HashMap<>();
-        for (String allowedSetting : ALLOWED_SETTING_KEYS) {
-            Map<String, Object> allowedSettingMap = Map.of(allowedSetting, randomAlphaOfLength(5));
-            allAllowedSettingsMap.put(allowedSetting, randomAlphaOfLength(5));
+        for (String allowedSetting : ALLOWED_SETTING_VALIDATORS.keySet()) {
+            String settingValue = ALLOWED_SETTING_GENERATORS.get(allowedSetting).get();
+            Map<String, Object> allowedSettingMap = Map.of(allowedSetting, settingValue);
+            allAllowedSettingsMap.put(allowedSetting, settingValue);
             var req = new UpdateSecuritySettingsAction.Request(
                 TEST_REQUEST_TIMEOUT,
                 TEST_REQUEST_TIMEOUT,
@@ -86,11 +100,12 @@ public void testAllowedSettingsOk() {
 
     public void testDisallowedSettingsFailsValidation() {
         String disallowedSetting = "index."
-            + randomValueOtherThanMany((value) -> ALLOWED_SETTING_KEYS.contains("index." + value), () -> randomAlphaOfLength(5));
+            + randomValueOtherThanMany((value) -> ALLOWED_SETTING_VALIDATORS.containsKey("index." + value), () -> randomAlphaOfLength(5));
         Map<String, Object> disallowedSettingMap = Map.of(disallowedSetting, randomAlphaOfLength(5));
+        String validSetting = randomFrom(ALLOWED_SETTING_VALIDATORS.keySet());
         Map<String, Object> validOrEmptySettingMap = randomFrom(
             Collections.emptyMap(),
-            Map.of(randomFrom(ALLOWED_SETTING_KEYS), randomAlphaOfLength(5))
+            Map.of(validSetting, ALLOWED_SETTING_GENERATORS.get(validSetting).get())
         );
         {
             var req = new UpdateSecuritySettingsAction.Request(
@@ -106,11 +121,11 @@ public void testDisallowedSettingsFailsValidation() {
                 assertThat(
                     errorMsg,
                     matchesRegex(
-                        "illegal settings for index \\["
+                        "illegal setting for index \\["
                             + Pattern.quote(TOKENS_INDEX_NAME)
                             + "\\]: \\["
                             + disallowedSetting
-                            + "\\], these settings may not be configured. Only the following settings may be configured for that index.*"
+                            + "\\], this setting may not be configured. Only the following settings may be configured for that index.*"
                     )
                 );
             }
@@ -130,13 +145,13 @@ public void testDisallowedSettingsFailsValidation() {
                 assertThat(
                     errorMsg,
                     matchesRegex(
-                        "illegal settings for index \\[("
+                        "illegal setting for index \\[("
                             + Pattern.quote(MAIN_INDEX_NAME)
                             + "|"
                             + Pattern.quote(PROFILES_INDEX_NAME)
                             + ")\\]: \\["
                             + disallowedSetting
-                            + "\\], these settings may not be configured. Only the following settings may be configured for that index.*"
+                            + "\\], this setting may not be configured. Only the following settings may be configured for that index.*"
                     )
                 );
             }
@@ -156,19 +171,72 @@ public void testDisallowedSettingsFailsValidation() {
                 assertThat(
                     errorMsg,
                     matchesRegex(
-                        "illegal settings for index \\[("
+                        "illegal setting for index \\[("
                             + Pattern.quote(MAIN_INDEX_NAME)
                             + "|"
                             + Pattern.quote(TOKENS_INDEX_NAME)
                             + "|"
                             + Pattern.quote(PROFILES_INDEX_NAME)
                             + ")\\]: \\["
                             + disallowedSetting
-                            + "\\], these settings may not be configured. Only the following settings may be configured for that index.*"
+                            + "\\], this setting may not be configured. Only the following settings may be configured for that index.*"
                     )
                 );
             }
         }
     }
 
+    public void testSettingValuesAreValidated() {
+        Map<String, Object> forbiddenSettingsMap = Map.of(DataTier.TIER_PREFERENCE, DataTier.DATA_FROZEN);
+        String badTier = randomAlphaOfLength(5);
+        Map<String, Object> badSettingsMap = Map.of(DataTier.TIER_PREFERENCE, badTier);
+        Map<String, Object> allowedSettingMap = Map.of(
+            DataTier.TIER_PREFERENCE,
+            randomFrom(DataTier.DATA_HOT, DataTier.DATA_WARM, DataTier.DATA_CONTENT, DataTier.DATA_COLD)
+        );
+        {
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                allowedSettingMap,
+                Collections.emptyMap(),
+                Collections.emptyMap()
+            );
+            assertThat(req.validate(), nullValue());
+        }
+
+        {
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                forbiddenSettingsMap,
+                Collections.emptyMap(),
+                Collections.emptyMap()
+            );
+            ActionRequestValidationException exception = req.validate();
+            assertThat(exception, notNullValue());
+            assertThat(exception.validationErrors(), hasSize(1));
+            assertThat(
+                exception.validationErrors().get(0),
+                containsString("disallowed data tiers [" + DataTier.DATA_FROZEN + "] found, allowed tiers are ")
+            );
+        }
+
+        {
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                badSettingsMap,
+                Collections.emptyMap(),
+                Collections.emptyMap()
+            );
+            var exception = req.validate();
+            assertThat(exception, notNullValue());
+            assertThat(exception.validationErrors(), hasSize(1));
+            assertThat(
+                exception.validationErrors().get(0),
+                containsString("disallowed data tiers [" + badTier + "] found, allowed tiers are ")
+            );
+        }
+    }
 }
diff --git a/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/SecuritySettingsIT.java b/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/SecuritySettingsIT.java
@@ -19,6 +19,7 @@
 import static org.elasticsearch.test.XContentTestUtils.createJsonMapView;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.nullValue;
 
 public class SecuritySettingsIT extends SecurityInBasicRestTestCase {
 
@@ -70,6 +71,54 @@ public void testBasicWorkflow() throws IOException {
         assertOK(getResp);
         final XContentTestUtils.JsonMapView mapView = createJsonMapView(getResp.getEntity().getContent());
         assertThat(mapView.get("security.index.auto_expand_replicas"), equalTo("0-all"));
+        assertThat(mapView.get("security-profile.index.auto_expand_replicas"), equalTo("0-all"));
+    }
+
+    public void testTierPreference() throws IOException {
+        {
+            Request req = new Request("PUT", "/_security/settings");
+            req.setJsonEntity("""
+                {
+                    "security": {
+                        "index.routing.allocation.include._tier_preference": "data_hot"
+                    },
+                    "security-profile": {
+                        "index.routing.allocation.include._tier_preference": "data_hot"
+                    }
+                }
+                """);
+            Response resp = adminClient().performRequest(req);
+            assertOK(resp);
+            Request getRequest = new Request("GET", "/_security/settings");
+            Response getResp = adminClient().performRequest(getRequest);
+            assertOK(getResp);
+            final XContentTestUtils.JsonMapView mapView = createJsonMapView(getResp.getEntity().getContent());
+            assertThat(mapView.get("security.index.routing.allocation.include._tier_preference"), equalTo("data_hot"));
+            assertThat(mapView.get("security-profile.index.routing.allocation.include._tier_preference"), equalTo("data_hot"));
+        }
+
+        {
+            Request req = new Request("PUT", "/_security/settings");
+            req.setJsonEntity("""
+                {
+                    "security": {
+                        "index.routing.allocation.include._tier_preference": null
+                    },
+                    "security-profile": {
+                        "index.routing.allocation.include._tier_preference": null
+                    }
+                }
+                """);
+            Response resp = adminClient().performRequest(req);
+            assertOK(resp);
+            Request getRequest = new Request("GET", "/_security/settings");
+            Response getResp = adminClient().performRequest(getRequest);
+            assertOK(getResp);
+            final XContentTestUtils.JsonMapView mapView = createJsonMapView(getResp.getEntity().getContent());
+            assertThat(mapView.get("security.index.routing.allocation.include._tier_preference"), nullValue());
+            assertThat(mapView.get("security-profile.index.routing.allocation.include._tier_preference"), nullValue());
+        }
+
     }
 
     public void testNoUpdatesThrowsException() throws IOException {
@@ -85,7 +134,7 @@ public void testDisallowedSettingThrowsException() throws IOException {
         ResponseException ex = expectThrows(ResponseException.class, () -> adminClient().performRequest(req));
         assertThat(
             EntityUtils.toString(ex.getResponse().getEntity()),
-            containsString("illegal settings for index [security]: " + "[index.max_ngram_diff], these settings may not be configured.")
+            containsString("illegal setting for index [security]: " + "[index.max_ngram_diff], this setting may not be configured.")
         );
     }
 
diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java
@@ -133,7 +133,7 @@ public void testProfileIndexAutoCreation() {
         final Settings settings = getIndexResponse.getSettings().get(INTERNAL_SECURITY_PROFILE_INDEX_8);
         assertThat(settings.get("index.number_of_shards"), equalTo("1"));
         assertThat(settings.get("index.auto_expand_replicas"), equalTo("0-1"));
-        assertThat(settings.get("index.routing.allocation.include._tier_preference"), equalTo("data_content"));
+        assertThat(settings.get("index.routing.allocation.include._tier_preference"), equalTo("data_hot,data_content"));
 
         final Map<String, Object> mappings = getIndexResponse.getMappings().get(INTERNAL_SECURITY_PROFILE_INDEX_8).getSourceAsMap();
 
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportGetSecuritySettingsAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportGetSecuritySettingsAction.java
@@ -85,7 +85,7 @@ private static Settings getFilteredSettingsForIndex(String indexName, ClusterSta
             .map(IndexMetadata::getSettings)
             .map(settings -> {
                 Settings.Builder builder = Settings.builder();
-                for (String settingName : UpdateSecuritySettingsAction.ALLOWED_SETTING_KEYS) {
+                for (String settingName : UpdateSecuritySettingsAction.ALLOWED_SETTING_VALIDATORS.keySet()) {
                     if (settings.hasValue(settingName)) {
                         builder.put(settingName, settings.get(settingName));
                     }
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecuritySystemIndices.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecuritySystemIndices.java
@@ -13,6 +13,7 @@
 import org.elasticsearch.client.internal.Client;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.node.DiscoveryNode;
+import org.elasticsearch.cluster.routing.allocation.DataTier;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.VersionId;
 import org.elasticsearch.common.settings.Settings;
@@ -154,6 +155,7 @@ private static Settings getMainIndexSettings() {
         return Settings.builder()
             .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
             .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
+            .put(DataTier.TIER_PREFERENCE, "data_hot,data_content")
             .put(IndexMetadata.SETTING_PRIORITY, 1000)
             .put(IndexMetadata.INDEX_FORMAT_SETTING.getKey(), INTERNAL_MAIN_INDEX_FORMAT)
             .put("analysis.filter.email.type", "pattern_capture")
@@ -702,6 +704,7 @@ private static Settings getTokenIndexSettings() {
         return Settings.builder()
             .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
             .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
+            .put(DataTier.TIER_PREFERENCE, "data_hot,data_content")
             .put(IndexMetadata.SETTING_PRIORITY, 1000)
             .put(IndexMetadata.INDEX_FORMAT_SETTING.getKey(), INTERNAL_TOKENS_INDEX_FORMAT)
             .build();
@@ -902,6 +905,7 @@ private static Settings getProfileIndexSettings(Settings settings) {
         final Settings.Builder settingsBuilder = Settings.builder()
             .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
             .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
+            .put(DataTier.TIER_PREFERENCE, "data_hot,data_content")
             .put(IndexMetadata.SETTING_PRIORITY, 1000)
             .put(IndexMetadata.INDEX_FORMAT_SETTING.getKey(), INTERNAL_PROFILE_INDEX_FORMAT)
             .put("analysis.filter.email.type", "pattern_capture")

Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ private static Settings getFilteredSettingsForIndex(String indexName, ClusterSta`
`85`	`85`	`.map(IndexMetadata::getSettings)`
`86`	`86`	`.map(settings -> {`
`87`	`87`	`Settings.Builder builder = Settings.builder();`
`88`		`- for (String settingName : UpdateSecuritySettingsAction.ALLOWED_SETTING_KEYS) {`
	`88`	`+ for (String settingName : UpdateSecuritySettingsAction.ALLOWED_SETTING_VALIDATORS.keySet()) {`
`89`	`89`	`if (settings.hasValue(settingName)) {`
`90`	`90`	`builder.put(settingName, settings.get(settingName));`
`91`	`91`	`}`