Limit service account permissions to system index only(opensearch-pro…

…ject#3607)


Signed-off-by: Ryan Liang <[email protected]>

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

@@ -0,0 +1,142 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+package org.opensearch.security.http;
+
+import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import org.apache.hc.core5.http.HttpStatus;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.opensearch.test.framework.TestIndex;
+import org.opensearch.test.framework.TestSecurityConfig;
+import org.opensearch.test.framework.cluster.ClusterManager;
+import org.opensearch.test.framework.cluster.LocalCluster;
+import org.opensearch.test.framework.cluster.TestRestClient;
+
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertNotNull;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_KEY;
+import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
+import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
+
+@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
+@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
+public class ServiceAccountAuthenticationTest {
+
+    public static final String SERVICE_ATTRIBUTE = "service";
+
+    static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS);
+
+    public static final String SERVICE_ACCOUNT_USER_NAME = "test-service-account";
+
+    static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).attr(
+        SERVICE_ATTRIBUTE,
+        "true"
+    )
+        .roles(
+            new TestSecurityConfig.Role("test-service-account-role").clusterPermissions("*")
+                .indexPermissions("*", "system:admin/system_index")
+                .on("*")
+        );
+
+    private static final TestIndex TEST_NON_SYS_INDEX = TestIndex.name("test-non-sys-index")
+        .setting("index.number_of_shards", 1)
+        .setting("index.number_of_replicas", 0)
+        .build();
+
+    private static final TestIndex TEST_SYS_INDEX = TestIndex.name("test-sys-index")
+        .setting("index.number_of_shards", 1)
+        .setting("index.number_of_replicas", 0)
+        .build();
+
+    @ClassRule
+    public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
+        .anonymousAuth(false)
+        .users(ADMIN_USER, SERVICE_ACCOUNT_ADMIN_USER)
+        .nodeSettings(
+            Map.of(
+                SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
+                true,
+                SECURITY_SYSTEM_INDICES_ENABLED_KEY,
+                true,
+                SECURITY_RESTAPI_ROLES_ENABLED,
+                List.of("user_admin__all_access"),
+                SECURITY_SYSTEM_INDICES_KEY,
+                List.of(TEST_SYS_INDEX.getName())
+            )
+        )
+        .authc(AUTHC_HTTPBASIC_INTERNAL)
+        .indices(TEST_NON_SYS_INDEX, TEST_SYS_INDEX)
+        .build();
+
+    @Test
+    public void testClusterHealthWithServiceAccountCred() {
+        try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
+            client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
+            TestRestClient.HttpResponse response = client.get("_cluster/health");
+            response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
+
+            String responseBody = response.getBody();
+
+            assertNotNull("Response body should not be null", responseBody);
+            assertTrue(responseBody.contains("\"type\":\"security_exception\""));
+        }
+    }
+
+    @Test
+    public void testReadSysIndexWithServiceAccountCred() {
+        try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
+            client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
+            TestRestClient.HttpResponse response = client.get(TEST_SYS_INDEX.getName());
+            response.assertStatusCode(HttpStatus.SC_OK);
+
+            String responseBody = response.getBody();
+
+            assertNotNull("Response body should not be null", responseBody);
+            assertTrue(responseBody.contains(TEST_SYS_INDEX.getName()));
+        }
+    }
+
+    @Test
+    public void testReadNonSysIndexWithServiceAccountCred() {
+        try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER)) {
+            client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
+            TestRestClient.HttpResponse response = client.get(TEST_NON_SYS_INDEX.getName());
+            response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
+
+            String responseBody = response.getBody();
+
+            assertNotNull("Response body should not be null", responseBody);
+            assertTrue(responseBody.contains("\"type\":\"security_exception\""));
+        }
+    }
+
+    @Test
+    public void testReadBothWithServiceAccountCred() {
+        TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_ADMIN_USER);
+        client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME);
+        TestRestClient.HttpResponse response = client.get((TEST_SYS_INDEX.getName() + "," + TEST_NON_SYS_INDEX.getName()));
+        response.assertStatusCode(HttpStatus.SC_FORBIDDEN);
+
+        String responseBody = response.getBody();
+
+        assertNotNull("Response body should not be null", responseBody);
+        assertTrue(responseBody.contains("\"type\":\"security_exception\""));
+
+    }
+}

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

@@ -353,7 +353,15 @@ public PrivilegesEvaluatorResponse evaluate(
             namedXContentRegistry
         );
 
+        final boolean serviceAccountUser = user.isServiceAccount();
         if (isClusterPerm(action0)) {
+            if (serviceAccountUser) {
+                presponse.missingPrivileges.add(action0);
+                presponse.allowed = false;
+                log.info("{} is a service account which doesn't have access to cluster level permission: {}", user, action0);
+                return presponse;
+            }
+
             if (!securityRoles.impliesClusterPermissionPermission(action0)) {
                 presponse.missingPrivileges.add(action0);
                 presponse.allowed = false;

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

@@ -199,6 +199,22 @@ private List<String> getAllProtectedSystemIndices(final Resolved requestedResolv
         return new ArrayList<>(superAdminAccessOnlyIndexMatcher.getMatchAny(requestedResolved.getAllIndices(), Collectors.toList()));
     }
 
+    /**
+     * Checks if the request contains any regular (non-system and non-protected) indices.
+     * Regular indices are those that are not categorized as system indices or protected system indices.
+     * This method helps in identifying requests that might be accessing regular indices alongside system indices.
+     * @param requestedResolved The resolved object of the request, which contains the list of indices from the original request.
+     * @return true if the request contains any regular indices, false otherwise.
+     */
+    private boolean requestContainsAnyRegularIndices(final Resolved requestedResolved) {
+        Set<String> allIndices = requestedResolved.getAllIndices();
+
+        List<String> allSystemIndices = getAllSystemIndices(requestedResolved);
+        List<String> allProtectedSystemIndices = getAllProtectedSystemIndices(requestedResolved);
+
+        return allIndices.stream().anyMatch(index -> !allSystemIndices.contains(index) && !allProtectedSystemIndices.contains(index));
+    }
+
     /**
      * Is the current action allowed to be performed on security index
      * @param action request action on security index
@@ -233,8 +249,28 @@ private void evaluateSystemIndicesAccess(
     ) {
         // Perform access check is system index permissions are enabled
         boolean containsSystemIndex = requestContainsAnySystemIndices(requestedResolved);
+        boolean containsRegularIndex = requestContainsAnyRegularIndices(requestedResolved);
+        boolean serviceAccountUser = user.isServiceAccount();
 
         if (isSystemIndexPermissionEnabled) {
+            if (serviceAccountUser && containsRegularIndex) {
+                auditLog.logSecurityIndexAttempt(request, action, task);
+                if (!containsSystemIndex && log.isInfoEnabled()) {
+                    log.info("{} not permitted for a service account {} on non-system indices.", action, securityRoles);
+                } else if (containsSystemIndex && log.isDebugEnabled()) {
+                    List<String> regularIndices = requestedResolved.getAllIndices()
+                        .stream()
+                        .filter(
+                            index -> !getAllSystemIndices(requestedResolved).contains(index)
+                                && !getAllProtectedSystemIndices(requestedResolved).contains(index)
+                        )
+                        .collect(Collectors.toList());
+                    log.debug("Service account cannot access regular indices: {}", regularIndices);
+                }
+                presponse.allowed = false;
+                presponse.markComplete();
+                return;
+            }
             boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved);
             if (containsProtectedIndex) {
                 auditLog.logSecurityIndexAttempt(request, action, task);

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

@@ -286,4 +286,14 @@ public final Set<String> getSecurityRoles() {
             ? Collections.synchronizedSet(Collections.emptySet())
             : Collections.unmodifiableSet(this.securityRoles);
     }
+
+    /**
+     * Check the custom attributes associated with this user
+     *
+     * @return true if it has a service account attributes. otherwise false
+     */
+    public boolean isServiceAccount() {
+        Map<String, String> userAttributesMap = this.getCustomAttributesMap();
+        return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service"));
+    }
 }

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

@@ -331,7 +331,7 @@ public void testSpecialUsernames() throws Exception {
         setup();
         RestHelper rh = nonSslRestHelper();
 
-        Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("bug.99", "nagilum")).getStatusCode());
+        Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("bug.88", "nagilum")).getStatusCode());
         Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("a", "b")).getStatusCode());
         Assert.assertEquals(
             HttpStatus.SC_OK,

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

@@ -43,7 +43,7 @@ public class UserServiceUnitTests {
     UserService userService;
 
     final int SERVICE_ACCOUNTS_IN_SETTINGS = 1;
-    final int INTERNAL_ACCOUNTS_IN_SETTINGS = 66;
+    final int INTERNAL_ACCOUNTS_IN_SETTINGS = 67;
     String serviceAccountUsername = "bug.99";
     String internalAccountUsername = "sarek";
 

src/test/resources/internal_users.yml

@@ -2,6 +2,13 @@
 _meta:
   type: "internalusers"
   config_version: 2
+bug.88:
+  hash: "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m"
+  reserved: false
+  hidden: false
+  backend_roles: []
+  attributes: {}
+  description: "Migrated from v6"
 bug.99:
   hash: "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m"
   reserved: false