Do not fetch reserved roles from native store when Get Role API is called

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: []

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<Resource> 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);
                                 }
                             });
                         }

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<String> 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<String> 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<String> 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<String> 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<String, Object> metadata) throws IOException {
+        Request request = new Request("POST", "/_security/role/" + roleName);
+        Map<String, Object> 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<String, Object> responseMap = responseAsMap(response);
+        assertTrue(ObjectPath.eval("role.created", responseMap));
+    }
+
+    private void getAllRolesAndAssert(RestClient client, Set<String> expectedRoles) throws IOException {
+        final Response response = client.performRequest(new Request("GET", "/_security/role"));
+        assertOK(response);
+        final Map<String, Object> responseMap = responseAsMap(response);
+        assertThat(responseMap.keySet(), equalTo(expectedRoles));
+    }
+
+    private void getRolesAndAssert(RestClient client, Set<String> rolesToGet) throws IOException {
+        final Response response = client.performRequest(new Request("GET", "/_security/role/" + String.join(",", rolesToGet)));
+        assertOK(response);
+        final Map<String, Object> responseMap = responseAsMap(response);
+        assertThat(responseMap.keySet(), equalTo(rolesToGet));
+    }
+}

x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/GeRoleDescriptorsTests.java

@@ -0,0 +1,99 @@
+/*
+ * 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.containsString;
+import static org.hamcrest.Matchers.notNullValue;
+
+/**
+ * Test for the {@link NativeRolesStore#getRoleDescriptors} method.
+ */
+public class GeRoleDescriptorsTests extends NativeRealmIntegTestCase {
+
+    private static Set<String> 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<RoleRetrievalResult> 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<RoleRetrievalResult> future = new PlainActionFuture<>();
+            Set<String> reservedRoles = randomUnique(() -> randomFrom(ReservedRolesStore.names()), randomIntBetween(1, 5));
+            AssertionError error = expectThrows(AssertionError.class, () -> rolesStore.getRoleDescriptors(reservedRoles, future));
+            assertThat(error.getMessage(), containsString("native roles store should not be called with reserved role names"));
+        }
+    }
+
+    public void testGetAllRoles() {
+        for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) {
+            PlainActionFuture<RoleRetrievalResult> 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()));
+        }
+    }
+}

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<GetRolesRequest, GetRolesResponse> {
 
     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,23 +51,25 @@ protected void doExecute(Task task, final GetRolesRequest request, final ActionL
 
         if (request.nativeOnly()) {
             final Set<String> 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;
         }
 
         final Set<String> rolesToSearchFor = new LinkedHashSet<>();
         final Set<RoleDescriptor> 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);