mirror of
https://github.com/elastic/elasticsearch.git
synced 2025-06-28 09:28:55 -04:00
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.
This commit is contained in:
parent
68d5cc6973
commit
57d4e15b62
4 changed files with 114 additions and 1 deletions
5
docs/changelog/128532.yaml
Normal file
5
docs/changelog/128532.yaml
Normal file
|
@ -0,0 +1,5 @@
|
||||||
|
pr: 128532
|
||||||
|
summary: "Prevent invalid privileges in manage roles privilege"
|
||||||
|
area: "Authorization"
|
||||||
|
type: bug
|
||||||
|
issues: [127496]
|
|
@ -562,6 +562,11 @@ public final class ConfigurableClusterPrivileges {
|
||||||
}
|
}
|
||||||
for (String privilege : indexPrivilege.privileges) {
|
for (String privilege : indexPrivilege.privileges) {
|
||||||
IndexPrivilege namedPrivilege = IndexPrivilege.getNamedOrNull(privilege);
|
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) {
|
if (namedPrivilege != null && namedPrivilege.getSelectorPredicate() == IndexComponentSelectorPredicate.FAILURES) {
|
||||||
throw new IllegalArgumentException(
|
throw new IllegalArgumentException(
|
||||||
"Failure store related privileges are not supported as targets of manage roles but found [" + privilege + "]"
|
"Failure store related privileges are not supported as targets of manage roles but found [" + privilege + "]"
|
||||||
|
|
|
@ -34,10 +34,13 @@ import org.elasticsearch.xpack.core.security.test.TestRestrictedIndices;
|
||||||
|
|
||||||
import java.io.ByteArrayOutputStream;
|
import java.io.ByteArrayOutputStream;
|
||||||
import java.io.IOException;
|
import java.io.IOException;
|
||||||
|
import java.nio.charset.StandardCharsets;
|
||||||
import java.util.Arrays;
|
import java.util.Arrays;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
|
import java.util.Locale;
|
||||||
import java.util.Objects;
|
import java.util.Objects;
|
||||||
|
|
||||||
|
import static org.hamcrest.Matchers.containsString;
|
||||||
import static org.hamcrest.Matchers.nullValue;
|
import static org.hamcrest.Matchers.nullValue;
|
||||||
import static org.hamcrest.core.Is.is;
|
import static org.hamcrest.core.Is.is;
|
||||||
import static org.hamcrest.core.IsEqual.equalTo;
|
import static org.hamcrest.core.IsEqual.equalTo;
|
||||||
|
@ -257,6 +260,70 @@ public class ManageRolesPrivilegesTests extends AbstractNamedWriteableTestCase<C
|
||||||
assertThat(permissionCheck(permission, "cluster:admin/xpack/security/role/put", putRoleRequest), is(false));
|
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) {
|
private static boolean permissionCheck(ClusterPermission permission, String action, ActionRequest request) {
|
||||||
final Authentication authentication = AuthenticationTestHelper.builder().build();
|
final Authentication authentication = AuthenticationTestHelper.builder().build();
|
||||||
assertThat(request.validate(), nullValue());
|
assertThat(request.validate(), nullValue());
|
||||||
|
|
|
@ -21,7 +21,7 @@ import static org.hamcrest.Matchers.hasKey;
|
||||||
import static org.hamcrest.Matchers.hasSize;
|
import static org.hamcrest.Matchers.hasSize;
|
||||||
import static org.hamcrest.Matchers.not;
|
import static org.hamcrest.Matchers.not;
|
||||||
|
|
||||||
public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
|
public class PutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
|
||||||
public void testPutManyValidRoles() throws Exception {
|
public void testPutManyValidRoles() throws Exception {
|
||||||
Map<String, Object> responseMap = upsertRoles("""
|
Map<String, Object> responseMap = upsertRoles("""
|
||||||
{"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test2":
|
{"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());
|
||||||
|
}
|
||||||
}
|
}
|
Loading…
Add table
Add a link
Reference in a new issue