From a1a9c50fe5ae4d8650c03455efb6223ce1b2a8f7 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Wed, 25 Oct 2023 22:29:39 -0700 Subject: [PATCH 01/24] Limit Service Accounts permissions Signed-off-by: Ryan Liang --- .../security/privileges/PrivilegesEvaluator.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 48f4db38e9..c1a926581c 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -353,6 +353,12 @@ public PrivilegesEvaluatorResponse evaluate( namedXContentRegistry ); + if (isClusterPerm(action0) && isServiceAccount(user)) { + presponse.allowed = false; + log.info("{} is a service account which as no access to cluster level permission of {}.", user, action0); + return presponse; + } + if (isClusterPerm(action0)) { if (!securityRoles.impliesClusterPermissionPermission(action0)) { presponse.missingPrivileges.add(action0); @@ -569,6 +575,11 @@ public PrivilegesEvaluatorResponse evaluate( } + public Boolean isServiceAccount(final User user) { + Map userAttributesMap = user.getCustomAttributesMap(); + return userAttributesMap != null && "true".equals(userAttributesMap.get("service")); + } + public Set mapRoles(final User user, final TransportAddress caller) { return this.configModel.mapSecurityRoles(user, caller); } From 37a0954c2fd4524c3565815709560c3f0cda8009 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 10:02:43 -0700 Subject: [PATCH 02/24] Correct the key check for service account attribute Signed-off-by: Ryan Liang --- .../org/opensearch/security/privileges/PrivilegesEvaluator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index c1a926581c..ec8374862d 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -577,7 +577,7 @@ public PrivilegesEvaluatorResponse evaluate( public Boolean isServiceAccount(final User user) { Map userAttributesMap = user.getCustomAttributesMap(); - return userAttributesMap != null && "true".equals(userAttributesMap.get("service")); + return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service")); } public Set mapRoles(final User user, final TransportAddress caller) { From 57bf24086e03de4e52fc77e508d4e7bb7b80065f Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 10:51:57 -0700 Subject: [PATCH 03/24] Refactor the cluster permission filter of service accounts Signed-off-by: Ryan Liang --- .../security/privileges/PrivilegesEvaluator.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index ec8374862d..1c98cebcc3 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -353,13 +353,14 @@ public PrivilegesEvaluatorResponse evaluate( namedXContentRegistry ); - if (isClusterPerm(action0) && isServiceAccount(user)) { - presponse.allowed = false; - log.info("{} is a service account which as no access to cluster level permission of {}.", user, action0); - return presponse; - } - if (isClusterPerm(action0)) { + if (isServiceAccount(user)) { + presponse.missingPrivileges.add(action0); + presponse.allowed = false; + log.info("{} is a service account which as no access to cluster level permission of {}.", user, action0); + return presponse; + } + if (!securityRoles.impliesClusterPermissionPermission(action0)) { presponse.missingPrivileges.add(action0); presponse.allowed = false; From a167919566b5c76f877e630d1805a585828a586b Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 15:10:31 -0700 Subject: [PATCH 04/24] Add filter for service account access to non system indices Signed-off-by: Ryan Liang --- .../SecurityIndexAccessEvaluator.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 3743b27383..beac554d03 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -43,10 +43,7 @@ import org.opensearch.security.user.User; import org.opensearch.tasks.Task; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; /** @@ -153,6 +150,16 @@ public PrivilegesEvaluatorResponse evaluate( return presponse; } + /** + * Checks if user is a service account user + * @param user request which contains attribute of service account + * @return true if a match is found, false otherwise + */ + public Boolean isServiceAccount(final User user) { + Map userAttributesMap = user.getCustomAttributesMap(); + return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service")); + } + /** * Checks if request is for any system index * @param requestedResolved request which contains indices to be matched against system indices @@ -235,6 +242,13 @@ private void evaluateSystemIndicesAccess( boolean containsSystemIndex = requestContainsAnySystemIndices(requestedResolved); if (isSystemIndexPermissionEnabled) { + if (isServiceAccount(user) && !containsSystemIndex) { + auditLog.logSecurityIndexAttempt(request, action, task); + log.info("{} not permitted for a service account {} on non system indices.", action, securityRoles); + presponse.allowed = false; + presponse.markComplete(); + return; + } boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved); if (containsProtectedIndex) { auditLog.logSecurityIndexAttempt(request, action, task); From b421ac843b22b7b73b3f2c62d03e4cec2263ece8 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 15:13:07 -0700 Subject: [PATCH 05/24] Fix import Signed-off-by: Ryan Liang --- .../security/privileges/SecurityIndexAccessEvaluator.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index beac554d03..baa7de0b32 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -43,7 +43,11 @@ import org.opensearch.security.user.User; import org.opensearch.tasks.Task; -import java.util.*; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.Map; import java.util.stream.Collectors; /** From 81aa19a169d60c5d4ceea681238795ef6f8d09ae Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 16:11:36 -0700 Subject: [PATCH 06/24] Fix testSpecialUsernames() by make the test account non-service account Signed-off-by: Ryan Liang --- .../java/org/opensearch/security/IntegrationTests.java | 2 +- src/test/resources/internal_users.yml | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) 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/resources/internal_users.yml b/src/test/resources/internal_users.yml index b078d6920f..3447e7f583 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 From 41428908f954755353b77fb826a764013e0fd675 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 16:38:30 -0700 Subject: [PATCH 07/24] Refactor isServiceAccount() into User class Signed-off-by: Ryan Liang --- .../privileges/PrivilegesEvaluator.java | 8 ++------ .../SecurityIndexAccessEvaluator.java | 18 +++++------------- .../org/opensearch/security/user/User.java | 10 ++++++++++ 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 1c98cebcc3..36bfad4456 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -353,8 +353,9 @@ public PrivilegesEvaluatorResponse evaluate( namedXContentRegistry ); + final boolean serviceAccountUser = user.isServiceAccount(); if (isClusterPerm(action0)) { - if (isServiceAccount(user)) { + if (serviceAccountUser) { presponse.missingPrivileges.add(action0); presponse.allowed = false; log.info("{} is a service account which as no access to cluster level permission of {}.", user, action0); @@ -576,11 +577,6 @@ public PrivilegesEvaluatorResponse evaluate( } - public Boolean isServiceAccount(final User user) { - Map userAttributesMap = user.getCustomAttributesMap(); - return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service")); - } - public Set mapRoles(final User user, final TransportAddress caller) { return this.configModel.mapSecurityRoles(user, caller); } diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index baa7de0b32..0a72e7fb27 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -47,7 +47,6 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.Map; import java.util.stream.Collectors; /** @@ -154,16 +153,6 @@ public PrivilegesEvaluatorResponse evaluate( return presponse; } - /** - * Checks if user is a service account user - * @param user request which contains attribute of service account - * @return true if a match is found, false otherwise - */ - public Boolean isServiceAccount(final User user) { - Map userAttributesMap = user.getCustomAttributesMap(); - return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service")); - } - /** * Checks if request is for any system index * @param requestedResolved request which contains indices to be matched against system indices @@ -244,11 +233,14 @@ private void evaluateSystemIndicesAccess( ) { // Perform access check is system index permissions are enabled boolean containsSystemIndex = requestContainsAnySystemIndices(requestedResolved); + boolean serviceAccountUser = user.isServiceAccount(); if (isSystemIndexPermissionEnabled) { - if (isServiceAccount(user) && !containsSystemIndex) { + if (serviceAccountUser && !containsSystemIndex) { auditLog.logSecurityIndexAttempt(request, action, task); - log.info("{} not permitted for a service account {} on non system indices.", action, securityRoles); + if (log.isInfoEnabled()) { + log.info("{} not permitted for a service account {} on non system indices.", action, securityRoles); + } presponse.allowed = false; presponse.markComplete(); return; 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")); + } } From e0ce30dc48f448baa88cda5d7f7cc6729b63d644 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 21:26:35 -0700 Subject: [PATCH 08/24] Update the accounts number in UserServiceUnitTests Signed-off-by: Ryan Liang --- src/test/java/org/opensearch/security/UserServiceUnitTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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"; From ccc3beab2feae90d086bacd6bcf78970307acf85 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Thu, 26 Oct 2023 22:05:02 -0700 Subject: [PATCH 09/24] Minor fix of some formatting Signed-off-by: Ryan Liang --- .../security/privileges/PrivilegesEvaluator.java | 2 +- .../org/opensearch/security/IntegrationTests.java | 12 ++++++++++++ src/test/resources/internal_users.yml | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 36bfad4456..71d8e0e102 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -358,7 +358,7 @@ public PrivilegesEvaluatorResponse evaluate( if (serviceAccountUser) { presponse.missingPrivileges.add(action0); presponse.allowed = false; - log.info("{} is a service account which as no access to cluster level permission of {}.", user, action0); + log.info("{} is a service account which has no access to cluster level permission of {}.", user, action0); return presponse; } diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index 9a4bf7bba8..e0792de503 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/src/test/java/org/opensearch/security/IntegrationTests.java @@ -1074,4 +1074,16 @@ public void testMonitorHealth() throws Exception { RestHelper rh = nonSslRestHelper(); Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("_cat/health", encodeBasicHeader("picard", "picard")).getStatusCode()); } + + // TODO: SOMETHING LIKE THIS + @Test + public void testServiceAccountClusterPermissions() throws Exception { + + setup(); + + RestHelper rh = nonSslRestHelper(); + HttpResponse response = rh.executeGetRequest("_cat/health", encodeBasicHeader("bug.99", "nagilum")); + System.out.println("The service account cluster perm response is: " + response.toString()); + Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + } } diff --git a/src/test/resources/internal_users.yml b/src/test/resources/internal_users.yml index 3447e7f583..a5eeb6fddb 100644 --- a/src/test/resources/internal_users.yml +++ b/src/test/resources/internal_users.yml @@ -6,7 +6,7 @@ bug.88: hash: "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m" reserved: false hidden: false - backend_roles: [ ] + backend_roles: [] attributes: {} description: "Migrated from v6" bug.99: From 691b347f511ff397146a0ea73f65459a6242819c Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Fri, 27 Oct 2023 11:01:17 -0700 Subject: [PATCH 10/24] Remove the unused example Signed-off-by: Ryan Liang --- .../org/opensearch/security/IntegrationTests.java | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index e0792de503..9a4bf7bba8 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/src/test/java/org/opensearch/security/IntegrationTests.java @@ -1074,16 +1074,4 @@ public void testMonitorHealth() throws Exception { RestHelper rh = nonSslRestHelper(); Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("_cat/health", encodeBasicHeader("picard", "picard")).getStatusCode()); } - - // TODO: SOMETHING LIKE THIS - @Test - public void testServiceAccountClusterPermissions() throws Exception { - - setup(); - - RestHelper rh = nonSslRestHelper(); - HttpResponse response = rh.executeGetRequest("_cat/health", encodeBasicHeader("bug.99", "nagilum")); - System.out.println("The service account cluster perm response is: " + response.toString()); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - } } From 6d0a820e0d54edea1771a3925606d814c1362780 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Fri, 27 Oct 2023 13:21:57 -0700 Subject: [PATCH 11/24] Initial setup of service account testing env Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) 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..5802efc534 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -0,0 +1,98 @@ +/* + * 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 com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +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.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.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.opensearch.security.support.ConfigConstants.*; +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 DEFAULT_PASSWORD = "secret"; + + static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + public static final String SERVICE_ACCOUNT_USER_NAME = "admin-extension"; + + private static final TestSecurityConfig.Role SERVICE_ACCOUNT_ADMIN_ROLE = new TestSecurityConfig.Role("admin-extension-role") + .clusterPermissions("*") + .indexPermissions("*", "system:admin/system_index") + .on("*"); + + static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).roles( + SERVICE_ACCOUNT_ADMIN_ROLE + ).attr("service", true); + + @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_ALLOW_DEFAULT_INIT_SECURITYINDEX, + true, + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_admin__all_access") + ) + ) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .build(); + + // TODO: REMOVE THIS DEBUGGING TEST CASE + @Test + public void testClusterHealthWithAdminCred() { + try (TestRestClient client = cluster.getRestClient("admin", DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials("admin"); + TestRestClient.HttpResponse response = client.get("_cluster/health"); + response.assertStatusCode(HttpStatus.SC_OK); + System.out.println(response); + } + } + + @Test + public void testClusterHealthWithServiceAccountCred() throws JsonProcessingException { + try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials("admin-extension"); + TestRestClient.HttpResponse response = client.get("_cluster/health"); + response.assertStatusCode(HttpStatus.SC_FORBIDDEN); + + String responseBody = response.getBody(); + assertNotNull("Response body should not be null", responseBody); + + ObjectMapper objectMapper = new ObjectMapper(); + String typeField = objectMapper.readTree(responseBody).at("/error/root_cause/0/type").asText(); + + assertEquals("security_exception", typeField); + } + } +} From 8ecb683e51fdd1b85a9c4ec1b258348030d998ab Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Fri, 27 Oct 2023 19:08:03 -0700 Subject: [PATCH 12/24] Add indices test case Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 71 ++++++++++++++----- 1 file changed, 55 insertions(+), 16 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 5802efc534..653870104a 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -18,6 +18,7 @@ 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; @@ -38,6 +39,8 @@ public class ServiceAccountAuthenticationTest { public static final String DEFAULT_PASSWORD = "secret"; + 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 = "admin-extension"; @@ -47,9 +50,20 @@ public class ServiceAccountAuthenticationTest { .indexPermissions("*", "system:admin/system_index") .on("*"); - static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).roles( - SERVICE_ACCOUNT_ADMIN_ROLE - ).attr("service", true); + static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).attr( + SERVICE_ATTRIBUTE, + "true" + ).roles(SERVICE_ACCOUNT_ADMIN_ROLE); + + 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) @@ -59,26 +73,18 @@ public class ServiceAccountAuthenticationTest { Map.of( SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY, true, - SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, + SECURITY_SYSTEM_INDICES_ENABLED_KEY, true, SECURITY_RESTAPI_ROLES_ENABLED, - List.of("user_admin__all_access") + List.of("user_admin__all_access"), + SECURITY_SYSTEM_INDICES_KEY, + List.of("test-sys-index") ) ) .authc(AUTHC_HTTPBASIC_INTERNAL) + .indices(TEST_NON_SYS_INDEX, TEST_SYS_INDEX) .build(); - // TODO: REMOVE THIS DEBUGGING TEST CASE - @Test - public void testClusterHealthWithAdminCred() { - try (TestRestClient client = cluster.getRestClient("admin", DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials("admin"); - TestRestClient.HttpResponse response = client.get("_cluster/health"); - response.assertStatusCode(HttpStatus.SC_OK); - System.out.println(response); - } - } - @Test public void testClusterHealthWithServiceAccountCred() throws JsonProcessingException { try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { @@ -95,4 +101,37 @@ public void testClusterHealthWithServiceAccountCred() throws JsonProcessingExcep assertEquals("security_exception", typeField); } } + + @Test + public void testReadSysIndexWithServiceAccountCred() { + try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials("admin-extension"); + TestRestClient.HttpResponse response = client.get("test-sys-index"); + response.assertStatusCode(HttpStatus.SC_OK); + // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE + System.out.println(response); + } + } + + @Test + public void testReadNonSysIndexWithServiceAccountCred() { + try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials("admin-extension"); + TestRestClient.HttpResponse response = client.get("test-non-sys-index"); + response.assertStatusCode(HttpStatus.SC_FORBIDDEN); + // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE + System.out.println(response); + } + } + + // TODO: REMOVE THIS DEBUGGING TEST CASE + @Test + public void testReadNonSysIndexWithAdminCred() { + try (TestRestClient client = cluster.getRestClient("admin", DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials("admin"); + TestRestClient.HttpResponse response = client.get("test-non-sys-index"); + response.assertStatusCode(HttpStatus.SC_OK); + System.out.println(response); + } + } } From 1dc296b7ca501f2cc19a3c2773679cb6616f62eb Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Sat, 28 Oct 2023 10:02:50 -0700 Subject: [PATCH 13/24] Fix lint Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 653870104a..0c823d1d80 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -29,7 +29,10 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.opensearch.security.support.ConfigConstants.*; +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; @@ -43,8 +46,9 @@ public class ServiceAccountAuthenticationTest { static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings public static final String SERVICE_ACCOUNT_USER_NAME = "admin-extension"; - + // CS-ENFORCE-SINGLE private static final TestSecurityConfig.Role SERVICE_ACCOUNT_ADMIN_ROLE = new TestSecurityConfig.Role("admin-extension-role") .clusterPermissions("*") .indexPermissions("*", "system:admin/system_index") @@ -87,8 +91,8 @@ public class ServiceAccountAuthenticationTest { @Test public void testClusterHealthWithServiceAccountCred() throws JsonProcessingException { - try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials("admin-extension"); + try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("_cluster/health"); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); @@ -104,8 +108,8 @@ public void testClusterHealthWithServiceAccountCred() throws JsonProcessingExcep @Test public void testReadSysIndexWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials("admin-extension"); + try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-sys-index"); response.assertStatusCode(HttpStatus.SC_OK); // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE @@ -115,8 +119,8 @@ public void testReadSysIndexWithServiceAccountCred() { @Test public void testReadNonSysIndexWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient("admin-extension", DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials("admin-extension"); + try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-non-sys-index"); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE From ac9416ee018c75b986f87233608055d98d3b12c9 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Sat, 28 Oct 2023 11:38:27 -0700 Subject: [PATCH 14/24] Filter out the regular index when it is a mixed request with both system indices and regular indices Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 12 ++++ .../SecurityIndexAccessEvaluator.java | 61 ++++++++++++++++--- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 0c823d1d80..612adc49e6 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -128,6 +128,18 @@ public void testReadNonSysIndexWithServiceAccountCred() { } } + @Test + public void testReadBothWithServiceAccountCred() { + try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); + TestRestClient.HttpResponse response = client.get("test-non-sys-index,test-sys-index"); + response.assertStatusCode(HttpStatus.SC_OK); + // TODO: CHECK IT ONLY READ SYSTEM INDEX AND FILTERED OUT REGULAR INDEX + // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE + System.out.println(response); + } + } + // TODO: REMOVE THIS DEBUGGING TEST CASE @Test public void testReadNonSysIndexWithAdminCred() { diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 0a72e7fb27..6a4e40027f 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -199,6 +199,29 @@ 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); + + for (String index : allIndices) { + // Check if the index is not in the list of system or protected system indices + if (!allSystemIndices.contains(index) && !allProtectedSystemIndices.contains(index)) { + return true; + } + } + + return false; + } + /** * Is the current action allowed to be performed on security index * @param action request action on security index @@ -233,17 +256,41 @@ 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 && !containsSystemIndex) { - auditLog.logSecurityIndexAttempt(request, action, task); - if (log.isInfoEnabled()) { - log.info("{} not permitted for a service account {} on non system indices.", action, securityRoles); + if (serviceAccountUser) { + if (!containsSystemIndex && containsRegularIndex) { + auditLog.logSecurityIndexAttempt(request, action, task); + if (log.isInfoEnabled()) { + log.info("{} not permitted for a service account {} on non-system indices.", action, securityRoles); + } + presponse.allowed = false; + presponse.markComplete(); + return; + } else if (containsSystemIndex && containsRegularIndex) { + // Filter out regular indices + List systemIndices = getAllSystemIndices(requestedResolved); + irr.replace(request, false, systemIndices.toArray(new String[0])); + if (log.isDebugEnabled()) { + log.debug("Filtered out regular indices, resulting list is {}", systemIndices); + } + // Recalculate indices after filtering + Resolved filteredResolved = irr.resolveRequest(request); + evaluateSystemIndicesAccess( + action, + filteredResolved, + request, + task, + presponse, + securityRoles, + user, + resolver, + clusterService + ); + return; } - presponse.allowed = false; - presponse.markComplete(); - return; } boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved); if (containsProtectedIndex) { From 085325211a2dfc1992ce1b7cd5309d9bd051c7bc Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Sun, 29 Oct 2023 13:05:19 -0700 Subject: [PATCH 15/24] Finish the tests Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 59 +++++++++++-------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 612adc49e6..e0252cb0cd 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -13,6 +13,7 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.hc.core5.http.HttpStatus; import org.junit.ClassRule; @@ -27,6 +28,8 @@ import java.util.List; import java.util.Map; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY; @@ -107,47 +110,53 @@ public void testClusterHealthWithServiceAccountCred() throws JsonProcessingExcep } @Test - public void testReadSysIndexWithServiceAccountCred() { + public void testReadSysIndexWithServiceAccountCred() throws JsonProcessingException { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-sys-index"); response.assertStatusCode(HttpStatus.SC_OK); - // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE - System.out.println(response); + + ObjectMapper objectMapper = new ObjectMapper(); + JsonNode jsonResponse = objectMapper.readTree(response.getBody()); + + boolean hasTestSysIndex = jsonResponse.has("test-sys-index"); + assertTrue("Response should contain 'test-sys-index'", hasTestSysIndex); + } } @Test - public void testReadNonSysIndexWithServiceAccountCred() { + public void testReadNonSysIndexWithServiceAccountCred() throws JsonProcessingException { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-non-sys-index"); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); - // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE - System.out.println(response); - } - } + String responseBody = response.getBody(); - @Test - public void testReadBothWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); - TestRestClient.HttpResponse response = client.get("test-non-sys-index,test-sys-index"); - response.assertStatusCode(HttpStatus.SC_OK); - // TODO: CHECK IT ONLY READ SYSTEM INDEX AND FILTERED OUT REGULAR INDEX - // TODO: REMOVE THIS AND PARSING/CHECKING THE RESPONSE - System.out.println(response); + assertNotNull("Response body should not be null", responseBody); + + ObjectMapper objectMapper = new ObjectMapper(); + String typeField = objectMapper.readTree(responseBody).at("/error/root_cause/0/type").asText(); + + assertEquals("security_exception", typeField); } } - // TODO: REMOVE THIS DEBUGGING TEST CASE @Test - public void testReadNonSysIndexWithAdminCred() { - try (TestRestClient client = cluster.getRestClient("admin", DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials("admin"); - TestRestClient.HttpResponse response = client.get("test-non-sys-index"); - response.assertStatusCode(HttpStatus.SC_OK); - System.out.println(response); - } + public void testReadBothWithServiceAccountCred() throws JsonProcessingException { + TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD); + client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); + TestRestClient.HttpResponse response = client.get("test-non-sys-index,test-sys-index"); + response.assertStatusCode(HttpStatus.SC_OK); + + ObjectMapper objectMapper = new ObjectMapper(); + JsonNode jsonResponse = objectMapper.readTree(response.getBody()); + + boolean hasTestSysIndex = jsonResponse.has("test-sys-index"); + boolean hasTestNonSysIndex = jsonResponse.has("test-non-sys-index"); + + assertTrue("Response should contain 'test-sys-index'", hasTestSysIndex); + assertFalse("Response should not contain 'test-non-sys-index'", hasTestNonSysIndex); + } } From ec67ff2c594ffe3d61faa58798fd5ef96a581646 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 09:21:25 -0700 Subject: [PATCH 16/24] Fix some comments Signed-off-by: Ryan Liang --- .../security/privileges/PrivilegesEvaluator.java | 2 +- .../privileges/SecurityIndexAccessEvaluator.java | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 71d8e0e102..538b541754 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -358,7 +358,7 @@ public PrivilegesEvaluatorResponse evaluate( if (serviceAccountUser) { presponse.missingPrivileges.add(action0); presponse.allowed = false; - log.info("{} is a service account which has no access to cluster level permission of {}.", user, action0); + log.info("{} is a service account which doesn't have access to cluster level permission: {}", user, action0); return presponse; } diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 6a4e40027f..cde3b28c56 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -212,14 +212,7 @@ private boolean requestContainsAnyRegularIndices(final Resolved requestedResolve List allSystemIndices = getAllSystemIndices(requestedResolved); List allProtectedSystemIndices = getAllProtectedSystemIndices(requestedResolved); - for (String index : allIndices) { - // Check if the index is not in the list of system or protected system indices - if (!allSystemIndices.contains(index) && !allProtectedSystemIndices.contains(index)) { - return true; - } - } - - return false; + return allIndices.stream().anyMatch(index -> !allSystemIndices.contains(index) && !allProtectedSystemIndices.contains(index)); } /** From 1a811b1259ef2499e0af08ca8f98238066f7d29c Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 10:38:40 -0700 Subject: [PATCH 17/24] Remove object mapper from the test case Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index e0252cb0cd..778cd6841e 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -12,9 +12,6 @@ package org.opensearch.security.http; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.hc.core5.http.HttpStatus; import org.junit.ClassRule; import org.junit.Test; @@ -30,7 +27,6 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertEquals; 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; @@ -93,70 +89,59 @@ public class ServiceAccountAuthenticationTest { .build(); @Test - public void testClusterHealthWithServiceAccountCred() throws JsonProcessingException { + public void testClusterHealthWithServiceAccountCred() { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { 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); - - ObjectMapper objectMapper = new ObjectMapper(); - String typeField = objectMapper.readTree(responseBody).at("/error/root_cause/0/type").asText(); - assertEquals("security_exception", typeField); + assertNotNull("Response body should not be null", responseBody); + assertTrue(responseBody.contains("\"type\":\"security_exception\"")); } } @Test - public void testReadSysIndexWithServiceAccountCred() throws JsonProcessingException { + public void testReadSysIndexWithServiceAccountCred() { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-sys-index"); response.assertStatusCode(HttpStatus.SC_OK); - ObjectMapper objectMapper = new ObjectMapper(); - JsonNode jsonResponse = objectMapper.readTree(response.getBody()); - - boolean hasTestSysIndex = jsonResponse.has("test-sys-index"); - assertTrue("Response should contain 'test-sys-index'", hasTestSysIndex); + String responseBody = response.getBody(); + assertNotNull("Response body should not be null", responseBody); + assertTrue(responseBody.contains("test-sys-index")); } } @Test - public void testReadNonSysIndexWithServiceAccountCred() throws JsonProcessingException { + public void testReadNonSysIndexWithServiceAccountCred() { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-non-sys-index"); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); + String responseBody = response.getBody(); assertNotNull("Response body should not be null", responseBody); - - ObjectMapper objectMapper = new ObjectMapper(); - String typeField = objectMapper.readTree(responseBody).at("/error/root_cause/0/type").asText(); - - assertEquals("security_exception", typeField); + assertTrue(responseBody.contains("\"type\":\"security_exception\"")); } } @Test - public void testReadBothWithServiceAccountCred() throws JsonProcessingException { + public void testReadBothWithServiceAccountCred() { TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD); client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get("test-non-sys-index,test-sys-index"); response.assertStatusCode(HttpStatus.SC_OK); - ObjectMapper objectMapper = new ObjectMapper(); - JsonNode jsonResponse = objectMapper.readTree(response.getBody()); - - boolean hasTestSysIndex = jsonResponse.has("test-sys-index"); - boolean hasTestNonSysIndex = jsonResponse.has("test-non-sys-index"); + String responseBody = response.getBody(); - assertTrue("Response should contain 'test-sys-index'", hasTestSysIndex); - assertFalse("Response should not contain 'test-non-sys-index'", hasTestNonSysIndex); + assertNotNull("Response body should not be null", responseBody); + assertTrue(responseBody.contains("test-sys-index")); + assertFalse(responseBody.contains("test-non-sys-index")); } } From c697f9531b0de570a9919064c384737e9ff11e4b Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 10:45:57 -0700 Subject: [PATCH 18/24] Apply getName() Signed-off-by: Ryan Liang --- .../http/ServiceAccountAuthenticationTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 778cd6841e..15ac9ee921 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -81,7 +81,7 @@ public class ServiceAccountAuthenticationTest { SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_admin__all_access"), SECURITY_SYSTEM_INDICES_KEY, - List.of("test-sys-index") + List.of(TEST_SYS_INDEX.getName()) ) ) .authc(AUTHC_HTTPBASIC_INTERNAL) @@ -106,13 +106,13 @@ public void testClusterHealthWithServiceAccountCred() { public void testReadSysIndexWithServiceAccountCred() { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); - TestRestClient.HttpResponse response = client.get("test-sys-index"); + 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")); + assertTrue(responseBody.contains(TEST_SYS_INDEX.getName())); } } @@ -120,7 +120,7 @@ public void testReadSysIndexWithServiceAccountCred() { public void testReadNonSysIndexWithServiceAccountCred() { try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); - TestRestClient.HttpResponse response = client.get("test-non-sys-index"); + TestRestClient.HttpResponse response = client.get(TEST_NON_SYS_INDEX.getName()); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); String responseBody = response.getBody(); @@ -140,8 +140,8 @@ public void testReadBothWithServiceAccountCred() { String responseBody = response.getBody(); assertNotNull("Response body should not be null", responseBody); - assertTrue(responseBody.contains("test-sys-index")); - assertFalse(responseBody.contains("test-non-sys-index")); + assertTrue(responseBody.contains(TEST_SYS_INDEX.getName())); + assertFalse(responseBody.contains(TEST_NON_SYS_INDEX.getName())); } } From fcf91ebc26b8056ac1906f0b790289732a2a1f96 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 11:11:07 -0700 Subject: [PATCH 19/24] Fix some comments Signed-off-by: Ryan Liang --- .../http/ServiceAccountAuthenticationTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 15ac9ee921..906d257a31 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -48,15 +48,16 @@ public class ServiceAccountAuthenticationTest { // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings public static final String SERVICE_ACCOUNT_USER_NAME = "admin-extension"; // CS-ENFORCE-SINGLE - private static final TestSecurityConfig.Role SERVICE_ACCOUNT_ADMIN_ROLE = new TestSecurityConfig.Role("admin-extension-role") - .clusterPermissions("*") - .indexPermissions("*", "system:admin/system_index") - .on("*"); static final TestSecurityConfig.User SERVICE_ACCOUNT_ADMIN_USER = new TestSecurityConfig.User(SERVICE_ACCOUNT_USER_NAME).attr( SERVICE_ATTRIBUTE, "true" - ).roles(SERVICE_ACCOUNT_ADMIN_ROLE); + ) + .roles( + new TestSecurityConfig.Role("admin-extension-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) @@ -134,7 +135,7 @@ public void testReadNonSysIndexWithServiceAccountCred() { public void testReadBothWithServiceAccountCred() { TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD); client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); - TestRestClient.HttpResponse response = client.get("test-non-sys-index,test-sys-index"); + TestRestClient.HttpResponse response = client.get((TEST_SYS_INDEX.getName() + "," + TEST_NON_SYS_INDEX.getName())); response.assertStatusCode(HttpStatus.SC_OK); String responseBody = response.getBody(); From 4e0c3ad56345aa64aaf6a1153de7d9f79de90e6e Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 11:37:59 -0700 Subject: [PATCH 20/24] Fix lint Signed-off-by: Ryan Liang --- .../security/http/ServiceAccountAuthenticationTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 906d257a31..9a3c07c349 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -54,7 +54,9 @@ public class ServiceAccountAuthenticationTest { "true" ) .roles( + // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings new TestSecurityConfig.Role("admin-extension-role").clusterPermissions("*") + // CS-ENFORCE-SINGLE .indexPermissions("*", "system:admin/system_index") .on("*") ); From 3a22b793650eaa1d9c65645d44de90a3e72593cf Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 12:38:59 -0700 Subject: [PATCH 21/24] Switch to reject the request, if this is a mixed situation Signed-off-by: Ryan Liang --- .../ServiceAccountAuthenticationTest.java | 6 ++--- .../SecurityIndexAccessEvaluator.java | 27 +++++++------------ 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 9a3c07c349..62bb40ac5e 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -26,7 +26,6 @@ import java.util.Map; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertFalse; 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; @@ -138,13 +137,12 @@ public void testReadBothWithServiceAccountCred() { TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD); client.confirmCorrectCredentials(SERVICE_ACCOUNT_USER_NAME); TestRestClient.HttpResponse response = client.get((TEST_SYS_INDEX.getName() + "," + TEST_NON_SYS_INDEX.getName())); - response.assertStatusCode(HttpStatus.SC_OK); + response.assertStatusCode(HttpStatus.SC_FORBIDDEN); String responseBody = response.getBody(); assertNotNull("Response body should not be null", responseBody); - assertTrue(responseBody.contains(TEST_SYS_INDEX.getName())); - assertFalse(responseBody.contains(TEST_NON_SYS_INDEX.getName())); + assertTrue(responseBody.contains("\"type\":\"security_exception\"")); } } diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index cde3b28c56..6d8ad9d077 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -263,25 +263,18 @@ private void evaluateSystemIndicesAccess( presponse.markComplete(); return; } else if (containsSystemIndex && containsRegularIndex) { - // Filter out regular indices - List systemIndices = getAllSystemIndices(requestedResolved); - irr.replace(request, false, systemIndices.toArray(new String[0])); if (log.isDebugEnabled()) { - log.debug("Filtered out regular indices, resulting list is {}", systemIndices); + 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); } - // Recalculate indices after filtering - Resolved filteredResolved = irr.resolveRequest(request); - evaluateSystemIndicesAccess( - action, - filteredResolved, - request, - task, - presponse, - securityRoles, - user, - resolver, - clusterService - ); + presponse.allowed = false; + presponse.markComplete(); return; } } From 2323a4a7abbc26e73c4219c9414cbff2a65c9b30 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 13:04:00 -0700 Subject: [PATCH 22/24] Refactor Signed-off-by: Ryan Liang --- .../SecurityIndexAccessEvaluator.java | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 6d8ad9d077..0db70f09b9 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -253,30 +253,21 @@ private void evaluateSystemIndicesAccess( boolean serviceAccountUser = user.isServiceAccount(); if (isSystemIndexPermissionEnabled) { - if (serviceAccountUser) { - if (!containsSystemIndex && containsRegularIndex) { - auditLog.logSecurityIndexAttempt(request, action, task); - if (log.isInfoEnabled()) { - log.info("{} not permitted for a service account {} on non-system indices.", action, securityRoles); - } - presponse.allowed = false; - presponse.markComplete(); - return; - } else if (containsSystemIndex && containsRegularIndex) { - if (log.isDebugEnabled()) { - List regularIndices = requestedResolved.getAllIndices() + 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) - ) + .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; + log.debug("Service account cannot access regular indices: {}", regularIndices); } + presponse.allowed = false; + presponse.markComplete(); + return; } boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved); if (containsProtectedIndex) { From 0a6bd2d139feeb0f03e1f6b6889dc88e62634f82 Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 30 Oct 2023 13:04:27 -0700 Subject: [PATCH 23/24] Fix lint Signed-off-by: Ryan Liang --- .../privileges/SecurityIndexAccessEvaluator.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java index 0db70f09b9..4d5fb26050 100644 --- a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java @@ -259,10 +259,12 @@ private void evaluateSystemIndicesAccess( 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()); + .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; From 0c268bf1995648c25e57f9c5ce96c45f74e6578e Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Tue, 31 Oct 2023 08:09:04 -0700 Subject: [PATCH 24/24] Fix some comments Signed-off-by: Ryan Liang --- .../http/ServiceAccountAuthenticationTest.java | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java index 62bb40ac5e..04f943edcf 100644 --- a/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/ServiceAccountAuthenticationTest.java @@ -38,24 +38,18 @@ @ThreadLeakScope(ThreadLeakScope.Scope.NONE) public class ServiceAccountAuthenticationTest { - public static final String DEFAULT_PASSWORD = "secret"; - public static final String SERVICE_ATTRIBUTE = "service"; static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); - // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings - public static final String SERVICE_ACCOUNT_USER_NAME = "admin-extension"; - // CS-ENFORCE-SINGLE + 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( - // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings - new TestSecurityConfig.Role("admin-extension-role").clusterPermissions("*") - // CS-ENFORCE-SINGLE + new TestSecurityConfig.Role("test-service-account-role").clusterPermissions("*") .indexPermissions("*", "system:admin/system_index") .on("*") ); @@ -92,7 +86,7 @@ public class ServiceAccountAuthenticationTest { @Test public void testClusterHealthWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + 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); @@ -106,7 +100,7 @@ public void testClusterHealthWithServiceAccountCred() { @Test public void testReadSysIndexWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + 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); @@ -120,7 +114,7 @@ public void testReadSysIndexWithServiceAccountCred() { @Test public void testReadNonSysIndexWithServiceAccountCred() { - try (TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD)) { + 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); @@ -134,7 +128,7 @@ public void testReadNonSysIndexWithServiceAccountCred() { @Test public void testReadBothWithServiceAccountCred() { - TestRestClient client = cluster.getRestClient(SERVICE_ACCOUNT_USER_NAME, DEFAULT_PASSWORD); + 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);