[Failure Store] Prevent explicit selectors in role index name patterns (#125843) (#125939)

Adding basic validation to prevent using `::` selectors when defining index permissions.
Index names do not allow colon character (`:`), hence the index name patterns that
would include double colon (`::`), would never match any of the index names.
To avoid confusion, we are preventing using `::` in role index name patterns.

For example, the `test-*::failures` will be rejected during `test-role` validation:

```
PUT /_security/role/test-role
{
    "indices": [
        {
            "names": ["test-*::failures"],
            "privileges": ["read"]
        }
    ]
}
```

(cherry picked from commit 1f7e26c1f72a0d3f2fb7d30f625ecb3bde219e91)

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java

@@ -8,6 +8,8 @@
 package org.elasticsearch.xpack.core.security.action.role;
 
 import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.cluster.metadata.DataStream;
+import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
 import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
 import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
@@ -53,6 +55,11 @@ public class RoleDescriptorRequestValidator {
                 } catch (IllegalArgumentException ile) {
                     validationException = addValidationError(ile.getMessage(), validationException);
                 }
+                if (DataStream.isFailureStoreFeatureFlagEnabled()) {
+                    for (final String indexName : idp.getIndices()) {
+                        validationException = validateIndexNameExpression(indexName, validationException);
+                    }
+                }
             }
         }
         final RoleDescriptor.RemoteIndicesPrivileges[] remoteIndicesPrivileges = roleDescriptor.getRemoteIndicesPrivileges();
@@ -71,6 +78,11 @@ public class RoleDescriptorRequestValidator {
             } catch (IllegalArgumentException ile) {
                 validationException = addValidationError(ile.getMessage(), validationException);
             }
+            if (DataStream.isFailureStoreFeatureFlagEnabled()) {
+                for (String indexName : ridp.indicesPrivileges().getIndices()) {
+                    validationException = validateIndexNameExpression(indexName, validationException);
+                }
+            }
         }
         if (roleDescriptor.hasRemoteClusterPermissions()) {
             try {
@@ -118,4 +130,21 @@ public class RoleDescriptorRequestValidator {
         }
         return validationException;
     }
+
+    private static ActionRequestValidationException validateIndexNameExpression(
+        String indexNameExpression,
+        ActionRequestValidationException validationException
+    ) {
+        if (IndexNameExpressionResolver.hasSelectorSuffix(indexNameExpression)) {
+            validationException = addValidationError(
+                "selectors ["
+                    + IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR
+                    + "] are not allowed in the index name expression ["
+                    + indexNameExpression
+                    + "]",
+                validationException
+            );
+        }
+        return validationException;
+    }
 }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java

@@ -0,0 +1,142 @@
+/*
+ * 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; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.core.security.action.role;
+
+import org.elasticsearch.cluster.metadata.DataStream;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
+import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges;
+import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
+
+public class RoleDescriptorRequestValidatorTests extends ESTestCase {
+
+    public void testSelectorsValidation() {
+        assumeTrue("failure store feature flag must be enabled", DataStream.isFailureStoreFeatureFlagEnabled());
+        String[] invalidIndexNames = {
+            "index::failures",
+            ".fs-*::failures",
+            ".ds-*::data",
+            "*::failures",
+            "*::",
+            "?::?",
+            "test?-*::data",
+            "test-*::*", // actual selector is not relevant and not validated
+            "test::irrelevant",
+            "::test",
+            "test::",
+            "::",
+            ":: ",
+            " ::",
+            ":::",
+            "::::",
+            randomAlphaOfLengthBetween(5, 10) + "\u003a\u003afailures",
+            randomAlphaOfLengthBetween(5, 10) + "\072\072failures" };
+        for (String indexName : invalidIndexNames) {
+            validateAndAssertSelectorNotAllowed(roleWithIndexPrivileges(indexName), indexName);
+            validateAndAssertSelectorNotAllowed(roleWithRemoteIndexPrivileges(indexName), indexName);
+        }
+
+        // these are not necessarily valid index names, but they should not trigger the selector validation
+        String[] validIndexNames = {
+            "index:failures", // single colon is allowed
+            ":failures",
+            "no double colon",
+            ":",
+            ": :",
+            "",
+            " ",
+            ":\n:",
+            null,
+            "a:",
+            ":b:",
+            "*",
+            "c?-*",
+            "d-*e",
+            "f:g:h",
+            "/[a-b]*test:[a-b]*:failures/", // while this regex can match test::failures, it is not rejected - doing so would be too complex
+            randomIntBetween(-10, 10) + "",
+            randomAlphaOfLengthBetween(1, 10),
+            randomAlphanumericOfLength(10) };
+        for (String indexName : validIndexNames) {
+            validateAndAssertNoException(roleWithIndexPrivileges(indexName), indexName);
+            validateAndAssertNoException(roleWithRemoteIndexPrivileges(indexName), indexName);
+        }
+    }
+
+    private static void validateAndAssertSelectorNotAllowed(RoleDescriptor roleDescriptor, String indexName) {
+        var validationException = RoleDescriptorRequestValidator.validate(roleDescriptor);
+        assertThat("expected validation exception for " + indexName, validationException, notNullValue());
+        assertThat(
+            validationException.validationErrors(),
+            containsInAnyOrder("selectors [::] are not allowed in the index name expression [" + indexName + "]")
+        );
+    }
+
+    private static void validateAndAssertNoException(RoleDescriptor roleDescriptor, String indexName) {
+        var validationException = RoleDescriptorRequestValidator.validate(roleDescriptor);
+        assertThat("expected no validation exception for " + indexName, validationException, nullValue());
+    }
+
+    private static RoleDescriptor roleWithIndexPrivileges(String... indices) {
+        return new RoleDescriptor(
+            "test-role",
+            null,
+            new IndicesPrivileges[] {
+                IndicesPrivileges.builder()
+                    .allowRestrictedIndices(randomBoolean())
+                    .indices(indices)
+                    .privileges(randomSubsetOf(randomIntBetween(1, IndexPrivilege.names().size()), IndexPrivilege.names()))
+                    .build() },
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null
+        );
+    }
+
+    private static RoleDescriptor roleWithRemoteIndexPrivileges(String... indices) {
+        Set<String> privileges = IndexPrivilege.names()
+            .stream()
+            .filter(p -> false == (p.equals("read_failure_store") || p.equals("manage_failure_store")))
+            .collect(Collectors.toSet());
+        return new RoleDescriptor(
+            "remote-test-role",
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            new RoleDescriptor.RemoteIndicesPrivileges[] {
+                new RoleDescriptor.RemoteIndicesPrivileges(
+                    IndicesPrivileges.builder()
+                        .allowRestrictedIndices(randomBoolean())
+                        .indices(indices)
+                        .privileges(randomSubsetOf(randomIntBetween(1, privileges.size()), privileges))
+                        .build(),
+                    "my-remote-cluster"
+                ) },
+            null,
+            null,
+            null
+        );
+    }
+}

x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java

@@ -48,6 +48,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
@@ -877,62 +878,56 @@ public class FailureStoreSecurityRestIT extends ESRestTestCase {
 
     public void testRoleWithSelectorInIndexPattern() throws Exception {
         setupDataStream();
-
         createUser("user", PASSWORD, "role");
-        upsertRole("""
-            {
-              "cluster": ["all"],
-              "indices": [
-                {
-                  "names": ["*::failures"],
-                  "privileges": ["read"]
-                }
-              ]
-            }""", "role");
-        createAndStoreApiKey("user", null);
-
-        expectThrows("user", new Search("test1::failures"), 403);
-        expectSearch("user", new Search("*::failures"));
-
-        upsertRole("""
-            {
-              "cluster": ["all"],
-              "indices": [
-                {
-                  "names": ["test1::failures"],
-                  "privileges": ["read"]
-                }
-              ]
-            }""", "role");
+        expectThrowsSelectorsNotAllowed(
+            () -> upsertRole(
+                Strings.format("""
+                    {
+                      "cluster": ["all"],
+                      "indices": [
+                        {
+                          "names": ["%s"],
+                          "privileges": ["%s"]
+                        }
+                      ]
+                    }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store")),
+                "role",
+                false
+            )
+        );
 
-        expectThrows("user", new Search("test1::failures"), 403);
-        expectSearch("user", new Search("*::failures"));
+        AssertionError bulkFailedError = expectThrows(
+            AssertionError.class,
+            () -> upsertRole(
+                Strings.format("""
+                    {
+                      "cluster": ["all"],
+                      "indices": [
+                        {
+                          "names": ["%s"],
+                          "privileges": ["%s"]
+                        }
+                      ]
+                    }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store")),
+                "role",
+                true
+            )
+        );
+        assertThat(bulkFailedError.getMessage(), containsString("selectors [::] are not allowed in the index name expression"));
 
-        upsertRole("""
+        expectThrowsSelectorsNotAllowed(() -> createApiKey("user", Strings.format("""
             {
-              "cluster": ["all"],
-              "indices": [
-                {
-                  "names": ["*::failures"],
-                  "privileges": ["read_failure_store"]
+                "role": {
+                    "cluster": ["all"],
+                    "indices": [
+                        {
+                            "names": ["%s"],
+                            "privileges": ["%s"]
+                        }
+                    ]
                 }
-              ]
-            }""", "role");
-        expectThrows("user", new Search("test1::failures"), 403);
-        expectSearch("user", new Search("*::failures"));
+            }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store"))));
 
-        upsertRole("""
-            {
-              "cluster": ["all"],
-              "indices": [
-                {
-                  "names": ["test1::failures"],
-                  "privileges": ["read_failure_store"]
-                }
-              ]
-            }""", "role");
-        expectThrows("user", new Search("test1::failures"), 403);
-        expectSearch("user", new Search("*::failures"));
     }
 
     public void testFailureStoreAccess() throws Exception {
@@ -2489,7 +2484,7 @@ public class FailureStoreSecurityRestIT extends ESRestTestCase {
     protected String createAndStoreApiKey(String username, @Nullable String roleDescriptors) throws IOException {
         assertThat("API key already registered for user: " + username, apiKeys.containsKey(username), is(false));
         apiKeys.put(username, createApiKey(username, roleDescriptors));
-        return createApiKey(username, roleDescriptors);
+        return apiKeys.get(username);
     }
 
     private String createApiKey(String username, String roleDescriptors) throws IOException {
@@ -2514,22 +2509,35 @@ public class FailureStoreSecurityRestIT extends ESRestTestCase {
         return (String) responseAsMap.get("encoded");
     }
 
-    protected void upsertRole(String roleDescriptor, String roleName) throws IOException {
-        Request createRoleRequest = roleRequest(roleDescriptor, roleName);
+    protected Response upsertRole(String roleDescriptor, String roleName) throws IOException {
+        return upsertRole(roleDescriptor, roleName, randomBoolean());
+    }
+
+    protected Response upsertRole(String roleDescriptor, String roleName, boolean bulk) throws IOException {
+        Request createRoleRequest = roleRequest(roleDescriptor, roleName, bulk);
         Response createRoleResponse = adminClient().performRequest(createRoleRequest);
         assertOK(createRoleResponse);
+        if (bulk) {
+            Map<String, Object> flattenedResponse = Maps.flatten(responseAsMap(createRoleResponse), true, true);
+            if (flattenedResponse.containsKey("errors.count") && (int) flattenedResponse.get("errors.count") > 0) {
+                throw new AssertionError(
+                    "Failed to create role [" + roleName + "], reason: " + flattenedResponse.get("errors.details." + roleName + ".reason")
+                );
+            }
+        }
+        return createRoleResponse;
     }
 
-    protected Request roleRequest(String roleDescriptor, String roleName) {
+    protected Request roleRequest(String roleDescriptor, String roleName, boolean bulk) {
         Request createRoleRequest;
-        if (randomBoolean()) {
-            createRoleRequest = new Request(randomFrom(HttpPut.METHOD_NAME, HttpPost.METHOD_NAME), "/_security/role/" + roleName);
-            createRoleRequest.setJsonEntity(roleDescriptor);
-        } else {
+        if (bulk) {
             createRoleRequest = new Request(HttpPost.METHOD_NAME, "/_security/role");
             createRoleRequest.setJsonEntity(org.elasticsearch.core.Strings.format("""
                 {"roles": {"%s": %s}}
                 """, roleName, roleDescriptor));
+        } else {
+            createRoleRequest = new Request(randomFrom(HttpPut.METHOD_NAME, HttpPost.METHOD_NAME), "/_security/role/" + roleName);
+            createRoleRequest.setJsonEntity(roleDescriptor);
         }
         return createRoleRequest;
     }
@@ -2592,4 +2600,10 @@ public class FailureStoreSecurityRestIT extends ESRestTestCase {
         Response response = performRequestWithApiKey(apiKey, req);
         assertThat(responseAsMap(response), equalTo(mapFromJson(expectedResponse)));
     }
+
+    private static void expectThrowsSelectorsNotAllowed(ThrowingRunnable runnable) {
+        ResponseException exception = expectThrows(ResponseException.class, runnable);
+        assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(400));
+        assertThat(exception.getMessage(), containsString("selectors [::] are not allowed in the index name expression"));
+    }
 }