Skip to content

Commit d10475c

Browse files
RyanL1997RyanL1997
authored
Limit service account permissions to system index only(#3607)
 Signed-off-by: Ryan Liang <jiallian@amazon.com>
1 parent bd43eef commit d10475c

7 files changed

Lines changed: 205 additions & 2 deletions

File tree

‎src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
12+
package org.opensearch.security.http;
13+
14+
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
15+
import org.apache.hc.core5.http.HttpStatus;
16+
import org.junit.ClassRule;
17+
import org.junit.Test;
18+
import org.junit.runner.RunWith;
19+
import org.opensearch.test.framework.TestIndex;
20+
import org.opensearch.test.framework.TestSecurityConfig;
21+
import org.opensearch.test.framework.cluster.ClusterManager;
22+
import org.opensearch.test.framework.cluster.LocalCluster;
23+
import org.opensearch.test.framework.cluster.TestRestClient;
24+
25+
import java.util.List;
26+
import java.util.Map;
27+
28+
import static org.junit.Assert.assertTrue;
29+
import static org.junit.Assert.assertNotNull;
30+
import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY;
31+
import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY;
32+
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
33+
import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_KEY;
34+
import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
35+
import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
36+
37+
@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
38+
@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
39+
public class ServiceAccountAuthenticationTest {
40+
41+
public static final String SERVICE_ATTRIBUTE = "service";
42+
43+
static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS);
44+
45+
public static final String SERVICE_ACCOUNT_USER_NAME = "test-service-account";
46+
47+
static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).attr(
48+
SERVICE_ATTRIBUTE,
49+
"true"
50+
)
51+
.roles(
52+
new TestSecurityConfig.Role("test-service-account-role").clusterPermissions("*")
53+
.indexPermissions("*", "system:admin/system_index")
54+
.on("*")
55+
);
56+
57+
private static final TestIndex TEST_NON_SYS_INDEX = TestIndex.name("test-non-sys-index")
58+
.setting("index.number_of_shards", 1)
59+
.setting("index.number_of_replicas", 0)
60+
.build();
61+
62+
private static final TestIndex TEST_SYS_INDEX = TestIndex.name("test-sys-index")
63+
.setting("index.number_of_shards", 1)
64+
.setting("index.number_of_replicas", 0)
65+
.build();
66+
67+
@ClassRule
68+
public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
69+
.anonymousAuth(false)
70+
.users(ADMIN_USER, SERVICE_ACCOUNT_ADMIN_USER)
71+
.nodeSettings(
72+
Map.of(
73+
SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
74+
true,
75+
SECURITY_SYSTEM_INDICES_ENABLED_KEY,
76+
true,
77+
SECURITY_RESTAPI_ROLES_ENABLED,
78+
List.of("user_admin__all_access"),
79+
SECURITY_SYSTEM_INDICES_KEY,
80+
List.of(TEST_SYS_INDEX.getName())
81+
)
82+
)
83+
.authc(AUTHC_HTTPBASIC_INTERNAL)
84+
.indices(TEST_NON_SYS_INDEX, TEST_SYS_INDEX)
85+
.build();
86+
87+
@Test
88+
public void testClusterHealthWithServiceAccountCred() {
89+
try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
90+
client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
91+
TestRestClient.HttpResponse response = client.get("_cluster/health");
92+
response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
93+
94+
String responseBody = response.getBody();
95+
96+
assertNotNull("Response body should not be null", responseBody);
97+
assertTrue(responseBody.contains("\"type\":\"security_exception\""));
98+
}
99+
}
100+
101+
@Test
102+
public void testReadSysIndexWithServiceAccountCred() {
103+
try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
104+
client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
105+
TestRestClient.HttpResponse response = client.get(TEST_SYS_INDEX.getName());
106+
response.assertStatusCode(HttpStatus.SC_OK);
107+
108+
String responseBody = response.getBody();
109+
110+
assertNotNull("Response body should not be null", responseBody);
111+
assertTrue(responseBody.contains(TEST_SYS_INDEX.getName()));
112+
}
113+
}
114+
115+
@Test
116+
public void testReadNonSysIndexWithServiceAccountCred() {
117+
try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
118+
client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
119+
TestRestClient.HttpResponse response = client.get(TEST_NON_SYS_INDEX.getName());
120+
response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
121+
122+
String responseBody = response.getBody();
123+
124+
assertNotNull("Response body should not be null", responseBody);
125+
assertTrue(responseBody.contains("\"type\":\"security_exception\""));
126+
}
127+
}
128+
129+
@Test
130+
public void testReadBothWithServiceAccountCred() {
131+
TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER);
132+
client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
133+
TestRestClient.HttpResponse response = client.get((TEST_SYS_INDEX.getName() + "," + TEST_NON_SYS_INDEX.getName()));
134+
response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
135+
136+
String responseBody = response.getBody();
137+
138+
assertNotNull("Response body should not be null", responseBody);
139+
assertTrue(responseBody.contains("\"type\":\"security_exception\""));
140+
141+
}
142+
}

‎src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,15 @@ public PrivilegesEvaluatorResponse evaluate(
353353
namedXContentRegistry
354354
);
355355

356+
final boolean serviceAccountUser = user.isServiceAccount();
356357
if (isClusterPerm(action0)) {
358+
if (serviceAccountUser) {
359+
presponse.missingPrivileges.add(action0);
360+
presponse.allowed = false;
361+
log.info("{} is a service account which doesn't have access to cluster level permission: {}", user, action0);
362+
return presponse;
363+
}
364+
357365
if (!securityRoles.impliesClusterPermissionPermission(action0)) {
358366
presponse.missingPrivileges.add(action0);
359367
presponse.allowed = false;

‎src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,22 @@ private List<String> getAllProtectedSystemIndices(final Resolved requestedResolv
199199
return new ArrayList<>(superAdminAccessOnlyIndexMatcher.getMatchAny(requestedResolved.getAllIndices(), Collectors.toList()));
200200
}
201201

202+
/**
203+
* Checks if the request contains any regular (non-system and non-protected) indices.
204+
* Regular indices are those that are not categorized as system indices or protected system indices.
205+
* This method helps in identifying requests that might be accessing regular indices alongside system indices.
206+
* @param requestedResolved The resolved object of the request, which contains the list of indices from the original request.
207+
* @return true if the request contains any regular indices, false otherwise.
208+
*/
209+
private boolean requestContainsAnyRegularIndices(final Resolved requestedResolved) {
210+
Set<String> allIndices = requestedResolved.getAllIndices();
211+
212+
List<String> allSystemIndices = getAllSystemIndices(requestedResolved);
213+
List<String> allProtectedSystemIndices = getAllProtectedSystemIndices(requestedResolved);
214+
215+
return allIndices.stream().anyMatch(index -> !allSystemIndices.contains(index) && !allProtectedSystemIndices.contains(index));
216+
}
217+
202218
/**
203219
* Is the current action allowed to be performed on security index
204220
* @param action request action on security index
@@ -233,8 +249,28 @@ private void evaluateSystemIndicesAccess(
233249
) {
234250
// Perform access check is system index permissions are enabled
235251
boolean containsSystemIndex = requestContainsAnySystemIndices(requestedResolved);
252+
boolean containsRegularIndex = requestContainsAnyRegularIndices(requestedResolved);
253+
boolean serviceAccountUser = user.isServiceAccount();
236254

237255
if (isSystemIndexPermissionEnabled) {
256+
if (serviceAccountUser && containsRegularIndex) {
257+
auditLog.logSecurityIndexAttempt(request, action, task);
258+
if (!containsSystemIndex && log.isInfoEnabled()) {
259+
log.info("{} not permitted for a service account {} on non-system indices.", action, securityRoles);
260+
} else if (containsSystemIndex && log.isDebugEnabled()) {
261+
List<String> regularIndices = requestedResolved.getAllIndices()
262+
.stream()
263+
.filter(
264+
index -> !getAllSystemIndices(requestedResolved).contains(index)
265+
&& !getAllProtectedSystemIndices(requestedResolved).contains(index)
266+
)
267+
.collect(Collectors.toList());
268+
log.debug("Service account cannot access regular indices: {}", regularIndices);
269+
}
270+
presponse.allowed = false;
271+
presponse.markComplete();
272+
return;
273+
}
238274
boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved);
239275
if (containsProtectedIndex) {
240276
auditLog.logSecurityIndexAttempt(request, action, task);

‎src/main/java/org/opensearch/security/user/User.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,4 +286,14 @@ public final Set<String> getSecurityRoles() {
286286
? Collections.synchronizedSet(Collections.emptySet())
287287
: Collections.unmodifiableSet(this.securityRoles);
288288
}
289+
290+
/**
291+
* Check the custom attributes associated with this user
292+
*
293+
* @return true if it has a service account attributes. otherwise false
294+
*/
295+
public boolean isServiceAccount() {
296+
Map<String, String> userAttributesMap = this.getCustomAttributesMap();
297+
return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service"));
298+
}
289299
}

‎src/test/java/org/opensearch/security/IntegrationTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ public void testSpecialUsernames() throws Exception {
331331
setup();
332332
RestHelper rh = nonSslRestHelper();
333333

334-
Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("bug.99", "nagilum")).getStatusCode());
334+
Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("bug.88", "nagilum")).getStatusCode());
335335
Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("a", "b")).getStatusCode());
336336
Assert.assertEquals(
337337
HttpStatus.SC_OK,

‎src/test/java/org/opensearch/security/UserServiceUnitTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public class UserServiceUnitTests {
4343
UserService userService;
4444

4545
final int SERVICE_ACCOUNTS_IN_SETTINGS = 1;
46-
final int INTERNAL_ACCOUNTS_IN_SETTINGS = 66;
46+
final int INTERNAL_ACCOUNTS_IN_SETTINGS = 67;
4747
String serviceAccountUsername = "bug.99";
4848
String internalAccountUsername = "sarek";
4949

‎src/test/resources/internal_users.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@
22
_meta:
33
type: "internalusers"
44
config_version: 2
5+
bug.88:
6+
hash: "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m"
7+
reserved: false
8+
hidden: false
9+
backend_roles: []
10+
attributes: {}
11+
description: "Migrated from v6"
512
bug.99:
613
hash: "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m"
714
reserved: false

0 commit comments

Comments
 (0)