Skip to content

Commit 5dec9a3

Browse files
slobodanadamovicslobodanadamovic
authored
Do not fetch reserved roles from native store when Get Role API is called (elastic#121971) (elastic#122752)
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 just to be merged with "static" definitions.
1 parent 6c8d44b commit 5dec9a3

File tree

7 files changed

+361
-20
lines changed

7 files changed

+361
-20
lines changed

‎docs/changelog/121971.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 121971
2+
summary: Do not fetch reserved roles from native store when Get Role API is called
3+
area: Authorization
4+
type: enhancement
5+
issues: []

‎test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import java.nio.file.StandardCopyOption;
4747
import java.nio.file.StandardOpenOption;
4848
import java.time.Duration;
49+
import java.util.ArrayList;
4950
import java.util.Arrays;
5051
import java.util.HashMap;
5152
import java.util.HashSet;
@@ -567,6 +568,37 @@ private void writeSecureSecretsFile() {
567568
}
568569
}
569570

571+
private void updateRolesFileAtomically() throws IOException {
572+
final Path targetRolesFile = workingDir.resolve("config").resolve("roles.yml");
573+
final Path tempFile = Files.createTempFile(workingDir.resolve("config"), null, null);
574+
575+
// collect all roles.yml files that should be combined into a single roles file
576+
final List<Resource> rolesFiles = new ArrayList<>(spec.getRolesFiles().size() + 1);
577+
rolesFiles.add(Resource.fromFile(distributionDir.resolve("config").resolve("roles.yml")));
578+
rolesFiles.addAll(spec.getRolesFiles());
579+
580+
// append all roles files to the temp file
581+
rolesFiles.forEach(rolesFile -> {
582+
try (
583+
Writer writer = Files.newBufferedWriter(tempFile, StandardOpenOption.APPEND);
584+
Reader reader = new BufferedReader(new InputStreamReader(rolesFile.asStream()))
585+
) {
586+
reader.transferTo(writer);
587+
} catch (IOException e) {
588+
throw new UncheckedIOException("Failed to append roles file " + rolesFile + " to " + tempFile, e);
589+
}
590+
});
591+
592+
// move the temp file to the target roles file atomically
593+
try {
594+
Files.move(tempFile, targetRolesFile, StandardCopyOption.ATOMIC_MOVE);
595+
} catch (IOException e) {
596+
throw new UncheckedIOException("Failed to move tmp roles file [" + tempFile + "] to [" + targetRolesFile + "]", e);
597+
} finally {
598+
Files.deleteIfExists(tempFile);
599+
}
600+
}
601+
570602
private void configureSecurity() {
571603
if (spec.isSecurityEnabled()) {
572604
if (spec.getUsers().isEmpty() == false) {
@@ -576,13 +608,11 @@ private void configureSecurity() {
576608
if (resource instanceof MutableResource && roleFileListeners.add(resource)) {
577609
((MutableResource) resource).addUpdateListener(updated -> {
578610
LOGGER.info("Updating roles.yml for node '{}'", name);
579-
Path rolesFile = workingDir.resolve("config").resolve("roles.yml");
580611
try {
581-
Files.delete(rolesFile);
582-
Files.copy(distributionDir.resolve("config").resolve("roles.yml"), rolesFile);
583-
writeRolesFile();
612+
updateRolesFileAtomically();
613+
LOGGER.info("Successfully updated roles.yml for node '{}'", name);
584614
} catch (IOException e) {
585-
throw new UncheckedIOException(e);
615+
throw new UncheckedIOException("Failed to update roles.yml file for node [" + name + "]", e);
586616
}
587617
});
588618
}

‎x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.security;
9+
10+
import org.elasticsearch.client.Request;
11+
import org.elasticsearch.client.Response;
12+
import org.elasticsearch.client.ResponseException;
13+
import org.elasticsearch.client.RestClient;
14+
import org.elasticsearch.common.bytes.BytesReference;
15+
import org.elasticsearch.common.settings.SecureString;
16+
import org.elasticsearch.common.settings.Settings;
17+
import org.elasticsearch.common.util.concurrent.ThreadContext;
18+
import org.elasticsearch.test.cluster.ElasticsearchCluster;
19+
import org.elasticsearch.test.cluster.local.distribution.DistributionType;
20+
import org.elasticsearch.test.cluster.local.model.User;
21+
import org.elasticsearch.test.cluster.util.resource.Resource;
22+
import org.elasticsearch.xcontent.ObjectPath;
23+
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
24+
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
25+
import org.junit.Before;
26+
import org.junit.ClassRule;
27+
28+
import java.io.IOException;
29+
import java.util.HashMap;
30+
import java.util.HashSet;
31+
import java.util.Map;
32+
import java.util.Set;
33+
34+
import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
35+
import static org.hamcrest.Matchers.equalTo;
36+
37+
public class GetRolesIT extends SecurityInBasicRestTestCase {
38+
39+
private static final String ADMIN_USER = "admin_user";
40+
private static final SecureString ADMIN_PASSWORD = new SecureString("admin-password".toCharArray());
41+
protected static final String READ_SECURITY_USER = "read_security_user";
42+
private static final SecureString READ_SECURITY_PASSWORD = new SecureString("read-security-password".toCharArray());
43+
44+
@Before
45+
public void initialize() {
46+
new ReservedRolesStore();
47+
}
48+
49+
@ClassRule
50+
public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
51+
.distribution(DistributionType.DEFAULT)
52+
.nodes(2)
53+
.setting("xpack.security.enabled", "true")
54+
.setting("xpack.license.self_generated.type", "basic")
55+
.rolesFile(Resource.fromClasspath("roles.yml"))
56+
.user(ADMIN_USER, ADMIN_PASSWORD.toString(), User.ROOT_USER_ROLE, true)
57+
.user(READ_SECURITY_USER, READ_SECURITY_PASSWORD.toString(), "read_security_user_role", false)
58+
.build();
59+
60+
@Override
61+
protected Settings restAdminSettings() {
62+
String token = basicAuthHeaderValue(ADMIN_USER, ADMIN_PASSWORD);
63+
return Settings.builder().put(ThreadContext.PREFIX + ".Authorization", token).build();
64+
}
65+
66+
@Override
67+
protected Settings restClientSettings() {
68+
String token = basicAuthHeaderValue(READ_SECURITY_USER, READ_SECURITY_PASSWORD);
69+
return Settings.builder().put(ThreadContext.PREFIX + ".Authorization", token).build();
70+
}
71+
72+
@Override
73+
protected String getTestRestCluster() {
74+
return cluster.getHttpAddresses();
75+
}
76+
77+
public void testGetAllRolesNoNative() throws Exception {
78+
// Test get roles API with operator admin_user
79+
getAllRolesAndAssert(adminClient(), ReservedRolesStore.names());
80+
// Test get roles API with read_security_user
81+
getAllRolesAndAssert(client(), ReservedRolesStore.names());
82+
}
83+
84+
public void testGetAllRolesWithNative() throws Exception {
85+
createRole("custom_role", "Test custom native role.", Map.of("owner", "test"));
86+
87+
Set<String> expectedRoles = new HashSet<>(ReservedRolesStore.names());
88+
expectedRoles.add("custom_role");
89+
90+
// Test get roles API with operator admin_user
91+
getAllRolesAndAssert(adminClient(), expectedRoles);
92+
// Test get roles API with read_security_user
93+
getAllRolesAndAssert(client(), expectedRoles);
94+
}
95+
96+
public void testGetReservedOnly() throws Exception {
97+
createRole("custom_role", "Test custom native role.", Map.of("owner", "test"));
98+
99+
Set<String> rolesToGet = new HashSet<>();
100+
rolesToGet.add("custom_role");
101+
rolesToGet.addAll(randomSet(1, 5, () -> randomFrom(ReservedRolesStore.names())));
102+
103+
getRolesAndAssert(adminClient(), rolesToGet);
104+
getRolesAndAssert(client(), rolesToGet);
105+
}
106+
107+
public void testGetNativeOnly() throws Exception {
108+
createRole("custom_role1", "Test custom native role.", Map.of("owner", "test1"));
109+
createRole("custom_role2", "Test custom native role.", Map.of("owner", "test2"));
110+
111+
Set<String> rolesToGet = Set.of("custom_role1", "custom_role2");
112+
113+
getRolesAndAssert(adminClient(), rolesToGet);
114+
getRolesAndAssert(client(), rolesToGet);
115+
}
116+
117+
public void testGetMixedRoles() throws Exception {
118+
createRole("custom_role", "Test custom native role.", Map.of("owner", "test"));
119+
120+
Set<String> rolesToGet = new HashSet<>();
121+
rolesToGet.add("custom_role");
122+
rolesToGet.addAll(randomSet(1, 5, () -> randomFrom(ReservedRolesStore.names())));
123+
124+
getRolesAndAssert(adminClient(), rolesToGet);
125+
getRolesAndAssert(client(), rolesToGet);
126+
}
127+
128+
public void testNonExistentRole() {
129+
var e = expectThrows(
130+
ResponseException.class,
131+
() -> client().performRequest(new Request("GET", "/_security/role/non_existent_role"))
132+
);
133+
assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(404));
134+
}
135+
136+
private void createRole(String roleName, String description, Map<String, Object> metadata) throws IOException {
137+
Request request = new Request("POST", "/_security/role/" + roleName);
138+
Map<String, Object> requestMap = new HashMap<>();
139+
if (description != null) {
140+
requestMap.put(RoleDescriptor.Fields.DESCRIPTION.getPreferredName(), description);
141+
}
142+
if (metadata != null) {
143+
requestMap.put(RoleDescriptor.Fields.METADATA.getPreferredName(), metadata);
144+
}
145+
BytesReference source = BytesReference.bytes(jsonBuilder().map(requestMap));
146+
request.setJsonEntity(source.utf8ToString());
147+
Response response = adminClient().performRequest(request);
148+
assertOK(response);
149+
Map<String, Object> responseMap = responseAsMap(response);
150+
assertTrue(ObjectPath.eval("role.created", responseMap));
151+
}
152+
153+
private void getAllRolesAndAssert(RestClient client, Set<String> expectedRoles) throws IOException {
154+
final Response response = client.performRequest(new Request("GET", "/_security/role"));
155+
assertOK(response);
156+
final Map<String, Object> responseMap = responseAsMap(response);
157+
assertThat(responseMap.keySet(), equalTo(expectedRoles));
158+
}
159+
160+
private void getRolesAndAssert(RestClient client, Set<String> rolesToGet) throws IOException {
161+
final Response response = client.performRequest(new Request("GET", "/_security/role/" + String.join(",", rolesToGet)));
162+
assertOK(response);
163+
final Map<String, Object> responseMap = responseAsMap(response);
164+
assertThat(responseMap.keySet(), equalTo(rolesToGet));
165+
}
166+
}

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
package org.elasticsearch.integration;
8+
9+
import org.elasticsearch.action.support.PlainActionFuture;
10+
import org.elasticsearch.test.NativeRealmIntegTestCase;
11+
import org.elasticsearch.test.TestSecurityClient;
12+
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
13+
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
14+
import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult;
15+
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
16+
import org.elasticsearch.xpack.security.support.SecuritySystemIndices;
17+
import org.junit.Before;
18+
import org.junit.BeforeClass;
19+
20+
import java.io.IOException;
21+
import java.util.HashSet;
22+
import java.util.Set;
23+
24+
import static org.elasticsearch.test.SecuritySettingsSource.SECURITY_REQUEST_OPTIONS;
25+
import static org.hamcrest.Matchers.containsInAnyOrder;
26+
import static org.hamcrest.Matchers.containsString;
27+
import static org.hamcrest.Matchers.notNullValue;
28+
29+
/**
30+
* Test for the {@link NativeRolesStore#getRoleDescriptors} method.
31+
*/
32+
public class GeRoleDescriptorsTests extends NativeRealmIntegTestCase {
33+
34+
private static Set<String> customRoles;
35+
36+
@BeforeClass
37+
public static void init() throws Exception {
38+
new ReservedRolesStore();
39+
40+
final int numOfRoles = randomIntBetween(5, 10);
41+
customRoles = new HashSet<>(numOfRoles);
42+
for (int i = 0; i < numOfRoles; i++) {
43+
customRoles.add("custom_role_" + randomAlphaOfLength(10) + "_" + i);
44+
}
45+
}
46+
47+
@Before
48+
public void setup() throws IOException {
49+
final TestSecurityClient securityClient = new TestSecurityClient(getRestClient(), SECURITY_REQUEST_OPTIONS);
50+
for (String role : customRoles) {
51+
final RoleDescriptor descriptor = new RoleDescriptor(
52+
role,
53+
new String[0],
54+
new RoleDescriptor.IndicesPrivileges[] {
55+
RoleDescriptor.IndicesPrivileges.builder()
56+
.indices("*")
57+
.privileges("ALL")
58+
.allowRestrictedIndices(randomBoolean())
59+
.build() },
60+
new String[0]
61+
);
62+
securityClient.putRole(descriptor);
63+
logger.info("--> created role [{}]", role);
64+
}
65+
66+
ensureGreen(SecuritySystemIndices.SECURITY_MAIN_ALIAS);
67+
}
68+
69+
public void testGetCustomRoles() {
70+
for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) {
71+
PlainActionFuture<RoleRetrievalResult> future = new PlainActionFuture<>();
72+
rolesStore.getRoleDescriptors(customRoles, future);
73+
RoleRetrievalResult result = future.actionGet();
74+
assertThat(result, notNullValue());
75+
assertTrue(result.isSuccess());
76+
assertThat(result.getDescriptors().stream().map(RoleDescriptor::getName).toList(), containsInAnyOrder(customRoles.toArray()));
77+
}
78+
}
79+
80+
public void testGetReservedRoles() {
81+
for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) {
82+
PlainActionFuture<RoleRetrievalResult> future = new PlainActionFuture<>();
83+
Set<String> reservedRoles = randomUnique(() -> randomFrom(ReservedRolesStore.names()), randomIntBetween(1, 5));
84+
AssertionError error = expectThrows(AssertionError.class, () -> rolesStore.getRoleDescriptors(reservedRoles, future));
85+
assertThat(error.getMessage(), containsString("native roles store should not be called with reserved role names"));
86+
}
87+
}
88+
89+
public void testGetAllRoles() {
90+
for (NativeRolesStore rolesStore : internalCluster().getInstances(NativeRolesStore.class)) {
91+
PlainActionFuture<RoleRetrievalResult> future = new PlainActionFuture<>();
92+
rolesStore.getRoleDescriptors(randomBoolean() ? null : Set.of(), future);
93+
RoleRetrievalResult result = future.actionGet();
94+
assertThat(result, notNullValue());
95+
assertTrue(result.isSuccess());
96+
assertThat(result.getDescriptors().stream().map(RoleDescriptor::getName).toList(), containsInAnyOrder(customRoles.toArray()));
97+
}
98+
}
99+
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.elasticsearch.xpack.core.security.action.role.GetRolesResponse;
1919
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
2020
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
21+
import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
2122
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
2223

2324
import java.util.Arrays;
@@ -29,11 +30,18 @@
2930
public class TransportGetRolesAction extends TransportAction<GetRolesRequest, GetRolesResponse> {
3031

3132
private final NativeRolesStore nativeRolesStore;
33+
private final ReservedRoleNameChecker reservedRoleNameChecker;
3234

3335
@Inject
34-
public TransportGetRolesAction(ActionFilters actionFilters, NativeRolesStore nativeRolesStore, TransportService transportService) {
36+
public TransportGetRolesAction(
37+
ActionFilters actionFilters,
38+
NativeRolesStore nativeRolesStore,
39+
ReservedRoleNameChecker reservedRoleNameChecker,
40+
TransportService transportService
41+
) {
3542
super(GetRolesAction.NAME, actionFilters, transportService.getTaskManager(), EsExecutors.DIRECT_EXECUTOR_SERVICE);
3643
this.nativeRolesStore = nativeRolesStore;
44+
this.reservedRoleNameChecker = reservedRoleNameChecker;
3745
}
3846

3947
@Override
@@ -43,23 +51,25 @@ protected void doExecute(Task task, final GetRolesRequest request, final ActionL
4351

4452
if (request.nativeOnly()) {
4553
final Set<String> rolesToSearchFor = specificRolesRequested
46-
? Arrays.stream(requestedRoles).collect(Collectors.toSet())
54+
? Arrays.stream(requestedRoles).filter(r -> false == reservedRoleNameChecker.isReserved(r)).collect(Collectors.toSet())
4755
: Collections.emptySet();
48-
getNativeRoles(rolesToSearchFor, listener);
56+
if (specificRolesRequested && rolesToSearchFor.isEmpty()) {
57+
// specific roles were requested, but they were all reserved, no need to hit the native store
58+
listener.onResponse(new GetRolesResponse());
59+
} else {
60+
getNativeRoles(rolesToSearchFor, listener);
61+
}
4962
return;
5063
}
5164

5265
final Set<String> rolesToSearchFor = new LinkedHashSet<>();
5366
final Set<RoleDescriptor> reservedRoles = new LinkedHashSet<>();
5467
if (specificRolesRequested) {
5568
for (String role : requestedRoles) {
56-
if (ReservedRolesStore.isReserved(role)) {
69+
if (reservedRoleNameChecker.isReserved(role)) {
5770
RoleDescriptor rd = ReservedRolesStore.roleDescriptor(role);
5871
if (rd != null) {
5972
reservedRoles.add(rd);
60-
} else {
61-
listener.onFailure(new IllegalStateException("unable to obtain reserved role [" + role + "]"));
62-
return;
6373
}
6474
} else {
6575
rolesToSearchFor.add(role);

0 commit comments

Comments
 (0)