From 771aaee07d3f58c55b98a84a239b32b1ec76b5ac Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Thu, 6 Feb 2025 22:47:19 +0100 Subject: [PATCH 1/9] Do not fetch reserved roles from native store when Get Role API is called The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index when Get Roles API is called. --- .../xpack/security/GetRolesIT.java | 166 ++++++++++++++++++ .../authz/store/NativeRolesStore.java | 4 +- 2 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java diff --git a/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java b/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java new file mode 100644 index 0000000000000..c8499aa3a9eba --- /dev/null +++ b/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java @@ -0,0 +1,166 @@ +/* + * 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.security; + +import org.elasticsearch.client.Request; +import org.elasticsearch.client.Response; +import org.elasticsearch.client.ResponseException; +import org.elasticsearch.client.RestClient; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.test.cluster.ElasticsearchCluster; +import org.elasticsearch.test.cluster.local.distribution.DistributionType; +import org.elasticsearch.test.cluster.local.model.User; +import org.elasticsearch.test.cluster.util.resource.Resource; +import org.elasticsearch.xcontent.ObjectPath; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; +import org.junit.Before; +import org.junit.ClassRule; + +import java.io.IOException; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.equalTo; + +public class GetRolesIT extends SecurityInBasicRestTestCase { + + private static final String ADMIN_USER = "admin_user"; + private static final SecureString ADMIN_PASSWORD = new SecureString("admin-password".toCharArray()); + protected static final String READ_SECURITY_USER = "read_security_user"; + private static final SecureString READ_SECURITY_PASSWORD = new SecureString("read-security-password".toCharArray()); + + @Before + public void initialize() { + new ReservedRolesStore(); + } + + @ClassRule + public static ElasticsearchCluster cluster = ElasticsearchCluster.local() + .distribution(DistributionType.DEFAULT) + .nodes(2) + .setting("xpack.security.enabled", "true") + .setting("xpack.license.self_generated.type", "basic") + .rolesFile(Resource.fromClasspath("roles.yml")) + .user(ADMIN_USER, ADMIN_PASSWORD.toString(), User.ROOT_USER_ROLE, true) + .user(READ_SECURITY_USER, READ_SECURITY_PASSWORD.toString(), "read_security_user_role", false) + .build(); + + @Override + protected Settings restAdminSettings() { + String token = basicAuthHeaderValue(ADMIN_USER, ADMIN_PASSWORD); + return Settings.builder().put(ThreadContext.PREFIX + ".Authorization", token).build(); + } + + @Override + protected Settings restClientSettings() { + String token = basicAuthHeaderValue(READ_SECURITY_USER, READ_SECURITY_PASSWORD); + return Settings.builder().put(ThreadContext.PREFIX + ".Authorization", token).build(); + } + + @Override + protected String getTestRestCluster() { + return cluster.getHttpAddresses(); + } + + public void testGetAllRolesNoNative() throws Exception { + // Test get roles API with operator admin_user + getAllRolesAndAssert(adminClient(), ReservedRolesStore.names()); + // Test get roles API with read_security_user + getAllRolesAndAssert(client(), ReservedRolesStore.names()); + } + + public void testGetAllRolesWithNative() throws Exception { + createRole("custom_role", "Test custom native role.", Map.of("owner", "test")); + + Set expectedRoles = new HashSet<>(ReservedRolesStore.names()); + expectedRoles.add("custom_role"); + + // Test get roles API with operator admin_user + getAllRolesAndAssert(adminClient(), expectedRoles); + // Test get roles API with read_security_user + getAllRolesAndAssert(client(), expectedRoles); + } + + public void testGetReservedOnly() throws Exception { + createRole("custom_role", "Test custom native role.", Map.of("owner", "test")); + + Set rolesToGet = new HashSet<>(); + rolesToGet.add("custom_role"); + rolesToGet.addAll(randomSet(1, 5, () -> randomFrom(ReservedRolesStore.names()))); + + getRolesAndAssert(adminClient(), rolesToGet); + getRolesAndAssert(client(), rolesToGet); + } + + public void testGetNativeOnly() throws Exception { + createRole("custom_role1", "Test custom native role.", Map.of("owner", "test1")); + createRole("custom_role2", "Test custom native role.", Map.of("owner", "test2")); + + Set rolesToGet = Set.of("custom_role1", "custom_role2"); + + getRolesAndAssert(adminClient(), rolesToGet); + getRolesAndAssert(client(), rolesToGet); + } + + public void testGetMixedRoles() throws Exception { + createRole("custom_role", "Test custom native role.", Map.of("owner", "test")); + + Set rolesToGet = new HashSet<>(); + rolesToGet.add("custom_role"); + rolesToGet.addAll(randomSet(1, 5, () -> randomFrom(ReservedRolesStore.names()))); + + getRolesAndAssert(adminClient(), rolesToGet); + getRolesAndAssert(client(), rolesToGet); + } + + public void testNonExistentRole() { + var e = expectThrows( + ResponseException.class, + () -> client().performRequest(new Request("GET", "/_security/role/non_existent_role")) + ); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(404)); + } + + private void createRole(String roleName, String description, Map metadata) throws IOException { + Request request = new Request("POST", "/_security/role/" + roleName); + Map requestMap = new HashMap<>(); + if (description != null) { + requestMap.put(RoleDescriptor.Fields.DESCRIPTION.getPreferredName(), description); + } + if (metadata != null) { + requestMap.put(RoleDescriptor.Fields.METADATA.getPreferredName(), metadata); + } + BytesReference source = BytesReference.bytes(jsonBuilder().map(requestMap)); + request.setJsonEntity(source.utf8ToString()); + Response response = adminClient().performRequest(request); + assertOK(response); + Map responseMap = responseAsMap(response); + assertTrue(ObjectPath.eval("role.created", responseMap)); + } + + private void getAllRolesAndAssert(RestClient client, Set expectedRoles) throws IOException { + final Response response = client.performRequest(new Request("GET", "/_security/role")); + assertOK(response); + final Map responseMap = responseAsMap(response); + assertThat(responseMap.keySet(), equalTo(expectedRoles)); + } + + private void getRolesAndAssert(RestClient client, Set rolesToGet) throws IOException { + final Response response = client.performRequest(new Request("GET", "/_security/role/" + String.join(",", rolesToGet))); + assertOK(response); + final Map responseMap = responseAsMap(response); + assertThat(responseMap.keySet(), equalTo(rolesToGet)); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index dc20b1e28ba78..c8431458cfb5e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -194,7 +194,9 @@ public void getRoleDescriptors(Set names, final ActionListener { - QueryBuilder query = QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE); + QueryBuilder query = QueryBuilders.boolQuery() + .must(QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE)) + .mustNot(QueryBuilders.termQuery("metadata_flattened._reserved", true)); final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); try (ThreadContext.StoredContext ignore = client.threadPool().getThreadContext().stashWithOrigin(SECURITY_ORIGIN)) { SearchRequest request = client.prepareSearch(SECURITY_MAIN_ALIAS) From db41a402301d0b9b554899987f0faa84810d5456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Slobodan=20Adamovi=C4=87?= Date: Thu, 6 Feb 2025 22:55:02 +0100 Subject: [PATCH 2/9] Update docs/changelog/121971.yaml --- docs/changelog/121971.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/121971.yaml diff --git a/docs/changelog/121971.yaml b/docs/changelog/121971.yaml new file mode 100644 index 0000000000000..92d66f81c2e60 --- /dev/null +++ b/docs/changelog/121971.yaml @@ -0,0 +1,5 @@ +pr: 121971 +summary: Do not fetch reserved roles from native store when Get Role API is called +area: Authorization +type: enhancement +issues: [] From 0e5024a3e4bc35e0609357b211c10bea65f4a1a2 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 7 Feb 2025 00:50:43 +0100 Subject: [PATCH 3/9] filter reserved roles in other cases and add tests --- .../integration/GeRoleDescriptorsTests.java | 103 ++++++++++++++++++ .../authz/store/NativeRolesStore.java | 29 ++++- 2 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java new file mode 100644 index 0000000000000..97c1fee214173 --- /dev/null +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java @@ -0,0 +1,103 @@ +/* + * 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.integration; + +import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.test.NativeRealmIntegTestCase; +import org.elasticsearch.test.TestSecurityClient; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; +import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult; +import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; +import org.elasticsearch.xpack.security.support.SecuritySystemIndices; +import org.junit.Before; +import org.junit.BeforeClass; + +import java.io.IOException; +import java.util.HashSet; +import java.util.Set; + +import static org.elasticsearch.test.SecuritySettingsSource.SECURITY_REQUEST_OPTIONS; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; + +/** + * Test for the {@link NativeRolesStore#getRoleDescriptors} method. + */ +public class GeRoleDescriptorsTests extends NativeRealmIntegTestCase { + + private static Set customRoles; + + @BeforeClass + public static void init() throws Exception { + new ReservedRolesStore(); + + final int numOfRoles = randomIntBetween(5, 10); + customRoles = new HashSet<>(numOfRoles); + for (int i = 0; i < numOfRoles; i++) { + customRoles.add("custom_role_" + randomAlphaOfLength(10) + "_" + i); + } + } + + @Before + public void setup() throws IOException { + final TestSecurityClient securityClient = new TestSecurityClient(getRestClient(), SECURITY_REQUEST_OPTIONS); + for (String role : customRoles) { + final RoleDescriptor descriptor = new RoleDescriptor( + role, + new String[0], + new RoleDescriptor.IndicesPrivileges[] { + RoleDescriptor.IndicesPrivileges.builder() + .indices("*") + .privileges("ALL") + .allowRestrictedIndices(randomBoolean()) + .build() }, + new String[0] + ); + securityClient.putRole(descriptor); + logger.info("--> created role [{}]", role); + } + + ensureGreen(SecuritySystemIndices.SECURITY_MAIN_ALIAS); + } + + public void testGetCustomRoles() { + for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) { + PlainActionFuture future = new PlainActionFuture<>(); + rolesStore.getRoleDescriptors(customRoles, future); + RoleRetrievalResult result = future.actionGet(); + assertThat(result, notNullValue()); + assertTrue(result.isSuccess()); + assertThat(result.getDescriptors().stream().map(RoleDescriptor::getName).toList(), containsInAnyOrder(customRoles.toArray())); + } + } + + public void testGetReservedRoles() { + for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) { + PlainActionFuture future = new PlainActionFuture<>(); + Set reservedRoles = randomUnique(() -> randomFrom(ReservedRolesStore.names()), randomIntBetween(1, 5)); + rolesStore.getRoleDescriptors(reservedRoles, future); + RoleRetrievalResult result = future.actionGet(); + assertThat(result, notNullValue()); + assertTrue(result.isSuccess()); + assertThat(result.getDescriptors(), is(empty())); + } + } + + public void testGetAllRoles() { + for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) { + PlainActionFuture future = new PlainActionFuture<>(); + rolesStore.getRoleDescriptors(randomBoolean() ? null : Set.of(), future); + RoleRetrievalResult result = future.actionGet(); + assertThat(result, notNullValue()); + assertTrue(result.isSuccess()); + assertThat(result.getDescriptors().stream().map(RoleDescriptor::getName).toList(), containsInAnyOrder(customRoles.toArray())); + } + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index c8431458cfb5e..fda9e420d7322 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -177,6 +177,19 @@ public void accept(Set names, ActionListener listen getRoleDescriptors(names, listener); } + private Set filterReservedRoles(Set names) { + if (names == null || names.isEmpty()) { + return names; + } + Set filtered = new HashSet<>(names.size()); + for (String name : names) { + if (false == reservedRoleNameChecker.isReserved(name)) { + filtered.add(name); + } + } + return filtered; + } + /** * Retrieve a list of roles, if rolesToGet is null or empty, fetch all roles */ @@ -186,13 +199,21 @@ public void getRoleDescriptors(Set names, final ActionListener rolesToGet = filterReservedRoles(names); + if ((names != null && names.size() > 0) && (rolesToGet == null || rolesToGet.isEmpty())) { + // if we have no roles to get, it means all requested roles were reserved. + // we can short-circuit and return an empty set here. + listener.onResponse(RoleRetrievalResult.success(Set.of())); + return; + } + final SecurityIndexManager frozenSecurityIndex = this.securityIndex.defensiveCopy(); if (frozenSecurityIndex.indexExists() == false) { // TODO remove this short circuiting and fix tests that fail without this! listener.onResponse(RoleRetrievalResult.success(Collections.emptySet())); } else if (frozenSecurityIndex.isAvailable(SEARCH_SHARDS) == false) { listener.onResponse(RoleRetrievalResult.failure(frozenSecurityIndex.getUnavailableReason(SEARCH_SHARDS))); - } else if (names == null || names.isEmpty()) { + } else if (rolesToGet == null || rolesToGet.isEmpty()) { securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { QueryBuilder query = QueryBuilders.boolQuery() .must(QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE)) @@ -220,11 +241,11 @@ public void getRoleDescriptors(Set names, final ActionListener { - final String[] roleIds = names.stream().map(NativeRolesStore::getIdForRole).toArray(String[]::new); + final String[] roleIds = rolesToGet.stream().map(NativeRolesStore::getIdForRole).toArray(String[]::new); MultiGetRequest multiGetRequest = client.prepareMultiGet().addIds(SECURITY_MAIN_ALIAS, roleIds).request(); executeAsyncWithOrigin( client.threadPool().getThreadContext(), From bc9f4bee810bb266f8df48e548513cfaa3bf9ae8 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 7 Feb 2025 10:47:05 +0100 Subject: [PATCH 4/9] make roles.yml file updates atomic --- .../local/AbstractLocalClusterFactory.java | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java index c2d274bb0b3eb..680acdccf2f39 100644 --- a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java +++ b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java @@ -46,6 +46,7 @@ import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.time.Duration; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; @@ -567,6 +568,37 @@ private void writeSecureSecretsFile() { } } + private void updateRolesFileAtomically() throws IOException { + final Path targetRolesFile = workingDir.resolve("config").resolve("roles.yml"); + final Path tempFile = Files.createTempFile(workingDir.resolve("config"), null, null); + + // collect all roles.yml files that should be combined into a single roles file + final List rolesFiles = new ArrayList<>(spec.getRolesFiles().size() + 1); + rolesFiles.add(Resource.fromFile(distributionDir.resolve("config").resolve("roles.yml"))); + rolesFiles.addAll(spec.getRolesFiles()); + + // append all roles files to the temp file + rolesFiles.forEach(rolesFile -> { + try ( + Writer writer = Files.newBufferedWriter(tempFile, StandardOpenOption.APPEND); + Reader reader = new BufferedReader(new InputStreamReader(rolesFile.asStream())) + ) { + reader.transferTo(writer); + } catch (IOException e) { + throw new UncheckedIOException("Failed to append roles file " + rolesFile + " to " + tempFile, e); + } + }); + + // move the temp file to the target roles file atomically + try { + Files.move(tempFile, targetRolesFile, StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { + throw new UncheckedIOException("Failed to move tmp roles file [" + tempFile + "] to [" + targetRolesFile + "]", e); + } finally { + Files.deleteIfExists(tempFile); + } + } + private void configureSecurity() { if (spec.isSecurityEnabled()) { if (spec.getUsers().isEmpty() == false) { @@ -576,13 +608,11 @@ private void configureSecurity() { if (resource instanceof MutableResource && roleFileListeners.add(resource)) { ((MutableResource) resource).addUpdateListener(updated -> { LOGGER.info("Updating roles.yml for node '{}'", name); - Path rolesFile = workingDir.resolve("config").resolve("roles.yml"); try { - Files.delete(rolesFile); - Files.copy(distributionDir.resolve("config").resolve("roles.yml"), rolesFile); - writeRolesFile(); + updateRolesFileAtomically(); + LOGGER.info("Successfully updated roles.yml for node '{}'", name); } catch (IOException e) { - throw new UncheckedIOException(e); + throw new UncheckedIOException("Failed to update roles.yml file for node [" + name + "]", e); } }); } From f6585904faf097e01a5794919e0cae8cd3d29b78 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 7 Feb 2025 10:51:53 +0100 Subject: [PATCH 5/9] naming nit --- .../xpack/security/authz/store/NativeRolesStore.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index fda9e420d7322..6f9960accac86 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -177,7 +177,7 @@ public void accept(Set names, ActionListener listen getRoleDescriptors(names, listener); } - private Set filterReservedRoles(Set names) { + private Set filterReservedRoleNames(Set names) { if (names == null || names.isEmpty()) { return names; } @@ -199,7 +199,7 @@ public void getRoleDescriptors(Set names, final ActionListener rolesToGet = filterReservedRoles(names); + final Set rolesToGet = filterReservedRoleNames(names); if ((names != null && names.size() > 0) && (rolesToGet == null || rolesToGet.isEmpty())) { // if we have no roles to get, it means all requested roles were reserved. // we can short-circuit and return an empty set here. From 104339f891db053eb2df0d522125d0bc6353d003 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 14 Feb 2025 11:55:31 +0100 Subject: [PATCH 6/9] remove unnecessary reserved roles filtering and add an asserition --- .../integration/GeRoleDescriptorsTests.java | 10 ++----- .../authz/store/NativeRolesStore.java | 30 ++++--------------- 2 files changed, 9 insertions(+), 31 deletions(-) diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java index 97c1fee214173..3e7312f67b08f 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java @@ -23,8 +23,7 @@ import static org.elasticsearch.test.SecuritySettingsSource.SECURITY_REQUEST_OPTIONS; import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.empty; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.notNullValue; /** @@ -82,11 +81,8 @@ public void testGetReservedRoles() { for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) { PlainActionFuture future = new PlainActionFuture<>(); Set reservedRoles = randomUnique(() -> randomFrom(ReservedRolesStore.names()), randomIntBetween(1, 5)); - rolesStore.getRoleDescriptors(reservedRoles, future); - RoleRetrievalResult result = future.actionGet(); - assertThat(result, notNullValue()); - assertTrue(result.isSuccess()); - assertThat(result.getDescriptors(), is(empty())); + AssertionError error = expectThrows(AssertionError.class, () -> rolesStore.getRoleDescriptors(reservedRoles, future)); + assertThat(error.getMessage(), containsString("native roles store should not be called with reserved role names")); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index 6f9960accac86..5a3bc897fbe9b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -177,19 +177,6 @@ public void accept(Set names, ActionListener listen getRoleDescriptors(names, listener); } - private Set filterReservedRoleNames(Set names) { - if (names == null || names.isEmpty()) { - return names; - } - Set filtered = new HashSet<>(names.size()); - for (String name : names) { - if (false == reservedRoleNameChecker.isReserved(name)) { - filtered.add(name); - } - } - return filtered; - } - /** * Retrieve a list of roles, if rolesToGet is null or empty, fetch all roles */ @@ -199,13 +186,8 @@ public void getRoleDescriptors(Set names, final ActionListener rolesToGet = filterReservedRoleNames(names); - if ((names != null && names.size() > 0) && (rolesToGet == null || rolesToGet.isEmpty())) { - // if we have no roles to get, it means all requested roles were reserved. - // we can short-circuit and return an empty set here. - listener.onResponse(RoleRetrievalResult.success(Set.of())); - return; - } + assert names == null || names.stream().noneMatch(reservedRoleNameChecker::isReserved) + : "native roles store should not be called with reserved role names"; final SecurityIndexManager frozenSecurityIndex = this.securityIndex.defensiveCopy(); if (frozenSecurityIndex.indexExists() == false) { @@ -213,7 +195,7 @@ public void getRoleDescriptors(Set names, final ActionListener { QueryBuilder query = QueryBuilders.boolQuery() .must(QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE)) @@ -241,11 +223,11 @@ public void getRoleDescriptors(Set names, final ActionListener { - final String[] roleIds = rolesToGet.stream().map(NativeRolesStore::getIdForRole).toArray(String[]::new); + final String[] roleIds = names.stream().map(NativeRolesStore::getIdForRole).toArray(String[]::new); MultiGetRequest multiGetRequest = client.prepareMultiGet().addIds(SECURITY_MAIN_ALIAS, roleIds).request(); executeAsyncWithOrigin( client.threadPool().getThreadContext(), From ad864c7623c7cf044b1ab12843ea971a557de01d Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 14 Feb 2025 14:57:04 +0100 Subject: [PATCH 7/9] filter reserved roles --- .../action/role/TransportGetRolesAction.java | 19 ++++++++-- .../role/TransportGetRolesActionTests.java | 36 ++++++++++++++++--- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java index cdeac51e1f492..5b9733aa82dc7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.core.security.action.role.GetRolesResponse; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; +import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker; import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; import java.util.Arrays; @@ -29,11 +30,18 @@ public class TransportGetRolesAction extends TransportAction { private final NativeRolesStore nativeRolesStore; + private final ReservedRoleNameChecker reservedRoleNameChecker; @Inject - public TransportGetRolesAction(ActionFilters actionFilters, NativeRolesStore nativeRolesStore, TransportService transportService) { + public TransportGetRolesAction( + ActionFilters actionFilters, + NativeRolesStore nativeRolesStore, + ReservedRoleNameChecker reservedRoleNameChecker, + TransportService transportService + ) { super(GetRolesAction.NAME, actionFilters, transportService.getTaskManager(), EsExecutors.DIRECT_EXECUTOR_SERVICE); this.nativeRolesStore = nativeRolesStore; + this.reservedRoleNameChecker = reservedRoleNameChecker; } @Override @@ -43,9 +51,14 @@ protected void doExecute(Task task, final GetRolesRequest request, final ActionL if (request.nativeOnly()) { final Set rolesToSearchFor = specificRolesRequested - ? Arrays.stream(requestedRoles).collect(Collectors.toSet()) + ? Arrays.stream(requestedRoles).filter(r -> false == reservedRoleNameChecker.isReserved(r)).collect(Collectors.toSet()) : Collections.emptySet(); - getNativeRoles(rolesToSearchFor, listener); + if (specificRolesRequested && rolesToSearchFor.isEmpty()) { + // specific roles were requested, but they were all reserved, no need to hit the native store + listener.onResponse(new GetRolesResponse()); + } else { + getNativeRoles(rolesToSearchFor, listener); + } return; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java index f1b1f194e5fbf..9db4af1049d1d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java @@ -22,6 +22,7 @@ import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult; import org.elasticsearch.xpack.core.security.user.UsernamesField; +import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker; import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; import org.junit.BeforeClass; @@ -67,7 +68,12 @@ public void testReservedRoles() { null, Collections.emptySet() ); - TransportGetRolesAction action = new TransportGetRolesAction(mock(ActionFilters.class), rolesStore, transportService); + TransportGetRolesAction action = new TransportGetRolesAction( + mock(ActionFilters.class), + rolesStore, + new ReservedRoleNameChecker.Default(), + transportService + ); final int size = randomIntBetween(1, ReservedRolesStore.names().size()); final List names = randomSubsetOf(size, ReservedRolesStore.names()); @@ -139,7 +145,12 @@ private void testStoreRoles(List storeRoleDescriptors) { null, Collections.emptySet() ); - TransportGetRolesAction action = new TransportGetRolesAction(mock(ActionFilters.class), rolesStore, transportService); + TransportGetRolesAction action = new TransportGetRolesAction( + mock(ActionFilters.class), + rolesStore, + new ReservedRoleNameChecker.Default(), + transportService + ); GetRolesRequest request = new GetRolesRequest(); request.names(storeRoleDescriptors.stream().map(RoleDescriptor::getName).collect(Collectors.toList()).toArray(Strings.EMPTY_ARRAY)); @@ -200,7 +211,12 @@ public void testGetAllOrMix() { null, Collections.emptySet() ); - TransportGetRolesAction action = new TransportGetRolesAction(mock(ActionFilters.class), rolesStore, transportService); + TransportGetRolesAction action = new TransportGetRolesAction( + mock(ActionFilters.class), + rolesStore, + new ReservedRoleNameChecker.Default(), + transportService + ); final List expectedNames = new ArrayList<>(); if (all) { @@ -286,7 +302,12 @@ public void testGetWithNativeOnly() { null, Collections.emptySet() ); - final TransportGetRolesAction action = new TransportGetRolesAction(mock(ActionFilters.class), rolesStore, transportService); + final TransportGetRolesAction action = new TransportGetRolesAction( + mock(ActionFilters.class), + rolesStore, + new ReservedRoleNameChecker.Default(), + transportService + ); final GetRolesRequest request = new GetRolesRequest(); request.names(requestedNames.toArray(Strings.EMPTY_ARRAY)); @@ -358,7 +379,12 @@ public void testException() { null, Collections.emptySet() ); - TransportGetRolesAction action = new TransportGetRolesAction(mock(ActionFilters.class), rolesStore, transportService); + TransportGetRolesAction action = new TransportGetRolesAction( + mock(ActionFilters.class), + rolesStore, + new ReservedRoleNameChecker.Default(), + transportService + ); GetRolesRequest request = new GetRolesRequest(); request.names(storeRoleDescriptors.stream().map(RoleDescriptor::getName).collect(Collectors.toList()).toArray(Strings.EMPTY_ARRAY)); From 9a21687ca5dea93362715e3108d26315cb917343 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 14 Feb 2025 15:48:08 +0100 Subject: [PATCH 8/9] fix transport get action tests the native roles store should never be called to resolve reserved built-in roles --- .../security/action/role/TransportGetRolesActionTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java index 9db4af1049d1d..c1d82505c27fe 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesActionTests.java @@ -291,7 +291,7 @@ public void testGetWithNativeOnly() { requestedNames.addAll(requestedStoreNames); } - final NativeRolesStore rolesStore = mockNativeRolesStore(requestedNames, storeRoleDescriptors); + final NativeRolesStore rolesStore = mockNativeRolesStore(requestedStoreNames, storeRoleDescriptors); final TransportService transportService = new TransportService( Settings.EMPTY, @@ -319,7 +319,7 @@ public void testGetWithNativeOnly() { verify(rolesStore, times(1)).getRoleDescriptors(eq(new HashSet<>()), anyActionListener()); } else { assertThat(actualRoleNames, containsInAnyOrder(requestedStoreNames.toArray(Strings.EMPTY_ARRAY))); - verify(rolesStore, times(1)).getRoleDescriptors(eq(new HashSet<>(requestedNames)), anyActionListener()); + verify(rolesStore, times(1)).getRoleDescriptors(eq(new HashSet<>(requestedStoreNames)), anyActionListener()); } } From c6c7b51c3d6049f332ea480a46049a5f392e5085 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 14 Feb 2025 15:53:39 +0100 Subject: [PATCH 9/9] use ReservedRoleNameChecker.isReserved consistently --- .../xpack/security/action/role/TransportGetRolesAction.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java index 5b9733aa82dc7..38545281928b1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java @@ -66,13 +66,10 @@ protected void doExecute(Task task, final GetRolesRequest request, final ActionL final Set reservedRoles = new LinkedHashSet<>(); if (specificRolesRequested) { for (String role : requestedRoles) { - if (ReservedRolesStore.isReserved(role)) { + if (reservedRoleNameChecker.isReserved(role)) { RoleDescriptor rd = ReservedRolesStore.roleDescriptor(role); if (rd != null) { reservedRoles.add(rd); - } else { - listener.onFailure(new IllegalStateException("unable to obtain reserved role [" + role + "]")); - return; } } else { rolesToSearchFor.add(role);