From d10475cbf0cc8ccdf959964c5d30777ee72e7bcc Mon Sep 17 00:00:00 2001 From: Ryan Liang <109499885+RyanL1997@users.noreply.github.com> Date: Tue, 31 Oct 2023 08:37:39 -0700 Subject: [PATCH] Limit service account permissions to system index only(#3607) Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 142 ++++++++++++++++++ .../privileges/PrivilegesEvaluator.java | 8 + .../SecurityIndexAccessEvaluator.java | 36 +++++ .../org/opensearch/security/user/User.java | 10 ++ .../opensearch/security/IntegrationTests.java | 2 +- .../security/UserServiceUnitTests.java | 2 +- src/test/resources/internal_users.yml | 7 + 7 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java new file mode 100644 index 0000000000..04f943edcf --- /dev/null +++ b/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\"")); + + } +} diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 48f4db38e9..538b541754 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/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; diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 3743b27383..4d5fb26050 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -199,6 +199,22 @@ private List 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 allIndices = requestedResolved.getAllIndices(); + + List allSystemIndices = getAllSystemIndices(requestedResolved); + List 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 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); diff --git a/src/main/java/org/opensearch/security/user/User.java b/src/main/java/org/opensearch/security/user/User.java index 394b251271..aa9c09a469 100644 --- a/src/main/java/org/opensearch/security/user/User.java +++ b/src/main/java/org/opensearch/security/user/User.java @@ -286,4 +286,14 @@ public final Set 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 userAttributesMap = this.getCustomAttributesMap(); + return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service")); + } } diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index 63ae25316b..9a4bf7bba8 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/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, diff --git a/src/test/java/org/opensearch/security/UserServiceUnitTests.java b/src/test/java/org/opensearch/security/UserServiceUnitTests.java index 533a4c7366..6bdef8d167 100644 --- a/src/test/java/org/opensearch/security/UserServiceUnitTests.java +++ b/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"; diff --git a/src/test/resources/internal_users.yml b/src/test/resources/internal_users.yml index b078d6920f..a5eeb6fddb 100644 --- a/src/test/resources/internal_users.yml +++ b/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