Prevent invalid privileges in manage roles privilege (#128532) (#128847) (#129155)

This PR addresses the bug reported in
[#127496](#127496)

**Changes:** - Added validation logic in `ConfigurableClusterPrivileges`
to ensure privileges defined for a global cluster manage role privilege
are valid  - Added unit test to `ManageRolePrivilegesTest` to ensure
invalid privilege is caught during role creation - Updated
`BulkPutRoleRestIT` to assert that an error is thrown and that the role
is not created.

Both existing and new unit/integration tests passed locally.

Author: gmjehovich

docs/changelog/128532.yaml

@@ -0,0 +1,5 @@
+pr: 128532
+summary: "Prevent invalid privileges in manage roles privilege"
+area: "Authorization"
+type: bug
+issues: [127496]

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ConfigurableClusterPrivileges.java

@@ -555,6 +555,13 @@ public static ManageRolesPrivilege parse(XContentParser parser) throws IOExcepti
                 if (indexPrivilege.privileges == null || indexPrivilege.privileges.length == 0) {
                     throw new IllegalArgumentException("Indices privileges must define at least one privilege");
                 }
+
+                for (String privilege : indexPrivilege.privileges) {
+                    // Use resolveBySelectorAccess to determine whether the passed privilege is valid.
+                    // IllegalArgumentException is thrown here when an invalid permission is encountered.
+                    IndexPrivilege.get(Set.of(privilege));
+                }
+
             }
             return new ManageRolesPrivilege(indexPrivileges);
         }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java

@@ -34,10 +34,13 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Locale;
 import java.util.Objects;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.core.Is.is;
 import static org.hamcrest.core.IsEqual.equalTo;
@@ -257,6 +260,70 @@ public void testPutRoleRequestContainsNonIndexPrivileges() {
         assertThat(permissionCheck(permission, "cluster:admin/xpack/security/role/put", putRoleRequest), is(false));
     }
 
+    public void testParseInvalidPrivilege() throws Exception {
+        final String unknownPrivilege = randomValueOtherThanMany(
+            i -> IndexPrivilege.values().containsKey(i),
+            () -> randomAlphaOfLength(10).toLowerCase(Locale.ROOT)
+        );
+
+        final String invalidJsonString = String.format(Locale.ROOT, """
+            {
+                "manage": {
+                    "indices": [
+                        {
+                            "names": ["test-*"],
+                            "privileges": ["%s"]
+                        }
+                    ]
+                }
+            }""", unknownPrivilege);
+        assertInvalidPrivilegeParsing(invalidJsonString, unknownPrivilege);
+    }
+
+    public void testParseMixedValidAndInvalidPrivileges() throws Exception {
+        final String unknownPrivilege = randomValueOtherThanMany(
+            i -> IndexPrivilege.values().containsKey(i),
+            () -> randomAlphaOfLength(10).toLowerCase(Locale.ROOT)
+        );
+
+        final String validPrivilege = "read";
+        final String mixedPrivilegesJson = String.format(Locale.ROOT, """
+            {
+                "manage": {
+                    "indices": [
+                        {
+                            "names": ["test-*"],
+                            "privileges": ["%s", "%s"]
+                        }
+                    ]
+                }
+            }""", validPrivilege, unknownPrivilege);
+
+        assertInvalidPrivilegeParsing(mixedPrivilegesJson, unknownPrivilege);
+    }
+
+    /**
+     * Helper method to assert that parsing the given JSON payload results in an
+     * IllegalArgumentException due to an unknown privilege.
+     *
+     * @param jsonPayload The JSON string containing the privilege data.
+     * @param expectedErrorDetail The specific unknown privilege name expected in the error message.
+     */
+    private static void assertInvalidPrivilegeParsing(final String jsonPayload, final String expectedErrorDetail) throws Exception {
+        final XContent xContent = XContentType.JSON.xContent();
+
+        try (
+            XContentParser parser = xContent.createParser(XContentParserConfiguration.EMPTY, jsonPayload.getBytes(StandardCharsets.UTF_8))
+        ) {
+            assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT));
+            assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME));
+
+            IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> ManageRolesPrivilege.parse(parser));
+
+            assertThat(exception.getMessage(), containsString("unknown index privilege [" + expectedErrorDetail + "]"));
+        }
+    }
+
     private static boolean permissionCheck(ClusterPermission permission, String action, ActionRequest request) {
         final Authentication authentication = AuthenticationTestHelper.builder().build();
         assertThat(request.validate(), nullValue());

x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/BulkPutRoleRestIT.java renamed to x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/PutRoleRestIT.java

@@ -21,7 +21,7 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.not;
 
-public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
+public class PutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
     public void testPutManyValidRoles() throws Exception {
         Map<String, Object> responseMap = upsertRoles("""
             {"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test2":
@@ -312,4 +312,40 @@ public void testBulkUpdates() throws Exception {
             );
         }
     }
+
+    public void testPutRoleWithInvalidManageRolesPrivilege() throws Exception {
+        final String badRoleName = "bad-role";
+
+        final ResponseException exception = expectThrows(ResponseException.class, () -> upsertRoles(String.format("""
+            {
+                "roles": {
+                    "%s": {
+                        "global": {
+                            "role": {
+                                "manage": {
+                                    "indices": [
+                                        {
+                                            "names": ["allowed-index-prefix-*"],
+                                            "privileges": ["foobar"]
+                                        }
+                                    ]
+                                }
+                            }
+                        }
+                    }
+                }
+            }""", badRoleName)));
+
+        assertThat(exception.getMessage(), containsString("unknown index privilege [foobar]"));
+        assertEquals(400, exception.getResponse().getStatusLine().getStatusCode());
+        assertRoleDoesNotExist(badRoleName);
+    }
+
+    private void assertRoleDoesNotExist(final String roleName) throws Exception {
+        final ResponseException roleNotFound = expectThrows(
+            ResponseException.class,
+            () -> adminClient().performRequest(new Request("GET", "/_security/role/" + roleName))
+        );
+        assertEquals(404, roleNotFound.getResponse().getStatusLine().getStatusCode());
+    }
 }