From 57d4e15b6291fca135fde31e00fadc49b08b6c46 Mon Sep 17 00:00:00 2001 From: Graeme Mjehovich <59065536+gmjehovich@users.noreply.github.com> Date: Thu, 29 May 2025 12:14:54 -0400 Subject: [PATCH] Bugfix: Prevent invalid privileges in manage roles privilege (#128532) This PR addresses the bug reported in [#127496](https://github.com/elastic/elasticsearch/issues/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. --- docs/changelog/128532.yaml | 5 ++ .../ConfigurableClusterPrivileges.java | 5 ++ .../privilege/ManageRolesPrivilegesTests.java | 67 +++++++++++++++++++ ...kPutRoleRestIT.java => PutRoleRestIT.java} | 38 ++++++++++- 4 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 docs/changelog/128532.yaml rename x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/{BulkPutRoleRestIT.java => PutRoleRestIT.java} (88%) diff --git a/docs/changelog/128532.yaml b/docs/changelog/128532.yaml new file mode 100644 index 000000000000..926ad461ba6e --- /dev/null +++ b/docs/changelog/128532.yaml @@ -0,0 +1,5 @@ +pr: 128532 +summary: "Prevent invalid privileges in manage roles privilege" +area: "Authorization" +type: bug +issues: [127496] diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ConfigurableClusterPrivileges.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ConfigurableClusterPrivileges.java index d5e350b20521..7eeaf8d19c49 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ConfigurableClusterPrivileges.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ConfigurableClusterPrivileges.java @@ -562,6 +562,11 @@ public final class ConfigurableClusterPrivileges { } for (String privilege : indexPrivilege.privileges) { IndexPrivilege namedPrivilege = IndexPrivilege.getNamedOrNull(privilege); + + // Use resolveBySelectorAccess to determine whether the passed privilege is valid. + // IllegalArgumentException is thrown here when an invalid permission is encountered. + IndexPrivilege.resolveBySelectorAccess(Set.of(privilege)); + if (namedPrivilege != null && namedPrivilege.getSelectorPredicate() == IndexComponentSelectorPredicate.FAILURES) { throw new IllegalArgumentException( "Failure store related privileges are not supported as targets of manage roles but found [" + privilege + "]" diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java index 2d47752063d9..40fd8cc1718c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java @@ -34,10 +34,13 @@ import org.elasticsearch.xpack.core.security.test.TestRestrictedIndices; 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 class ManageRolesPrivilegesTests extends AbstractNamedWriteableTestCase 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()); diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/BulkPutRoleRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/PutRoleRestIT.java similarity index 88% rename from x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/BulkPutRoleRestIT.java rename to x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/PutRoleRestIT.java index 88b952f33394..377e774a9f96 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/BulkPutRoleRestIT.java +++ b/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.hasKey; 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 responseMap = upsertRoles(""" {"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test2": @@ -312,4 +312,40 @@ public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase { ); } } + + 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()); + } }