Fix bug where admin can read system index (#4774)

Signed-off-by: Derek Ho <[email protected]> (cherry picked from commit 5cb4dd2) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Oct 3, 2024 · c4d6309 · c4d6309
1 parent e253e24
commit c4d6309
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 5 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java
@@ -80,4 +80,29 @@ public void adminShouldNotBeAbleToDeleteSecurityIndex() {
             assertThat(response4.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
         }
     }
+
+    @Test
+    public void regularUserShouldGetNoResultsWhenSearchingSystemIndex() {
+        // Create system index and index a dummy document as the super admin user, data returned to super admin
+        try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
+            HttpResponse response1 = client.put(".system-index1");
+
+            assertThat(response1.getStatusCode(), equalTo(RestStatus.OK.getStatus()));
+            String doc = "{\"field\":\"value\"}";
+            HttpResponse adminPostResponse = client.postJson(".system-index1/_doc/1?refresh=true", doc);
+            assertThat(adminPostResponse.getStatusCode(), equalTo(RestStatus.CREATED.getStatus()));
+            HttpResponse response2 = client.get(".system-index1/_search");
+
+            assertThat(response2.getStatusCode(), equalTo(RestStatus.OK.getStatus()));
+            assertThat(response2.getBody(), response2.getBody().contains("\"hits\":{\"total\":{\"value\":1,\"relation\":\"eq\"}"));
+        }
+
+        // Regular users should not be able to read it
+        try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
+            // regular user cannot read system index
+            HttpResponse response1 = client.get(".system-index1/_search");
+
+            assertThat(response1.getBody(), response1.getBody().contains("\"hits\":{\"total\":{\"value\":0,\"relation\":\"eq\"}"));
+        }
+    }
 }
diff --git a/src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java
@@ -41,7 +41,7 @@
 import org.opensearch.security.support.HeaderHelper;
 import org.opensearch.security.support.SecurityUtils;
 
-public class SecurityFlsDlsIndexSearcherWrapper extends SecurityIndexSearcherWrapper {
+public class SecurityFlsDlsIndexSearcherWrapper extends SystemIndexSearcherWrapper {
 
     public final Logger log = LogManager.getLogger(this.getClass());
 

diff --git a/...uration/SecurityIndexSearcherWrapper.java → ...iguration/SystemIndexSearcherWrapper.java b/...uration/SecurityIndexSearcherWrapper.java → ...iguration/SystemIndexSearcherWrapper.java
@@ -39,6 +39,7 @@
 import org.opensearch.core.common.transport.TransportAddress;
 import org.opensearch.core.index.Index;
 import org.opensearch.index.IndexService;
+import org.opensearch.indices.SystemIndexRegistry;
 import org.opensearch.security.privileges.PrivilegesEvaluator;
 import org.opensearch.security.securityconf.ConfigModel;
 import org.opensearch.security.securityconf.SecurityRoles;
@@ -49,7 +50,7 @@
 
 import org.greenrobot.eventbus.Subscribe;
 
-public class SecurityIndexSearcherWrapper implements CheckedFunction<DirectoryReader, DirectoryReader, IOException> {
+public class SystemIndexSearcherWrapper implements CheckedFunction<DirectoryReader, DirectoryReader, IOException> {
 
     protected final Logger log = LogManager.getLogger(this.getClass());
     protected final ThreadContext threadContext;
@@ -68,7 +69,7 @@ public class SecurityIndexSearcherWrapper implements CheckedFunction<DirectoryRe
     private final Boolean systemIndexPermissionEnabled;
 
     // constructor is called per index, so avoid costly operations here
-    public SecurityIndexSearcherWrapper(
+    public SystemIndexSearcherWrapper(
         final IndexService indexService,
         final Settings settings,
         final AdminDNs adminDNs,
@@ -152,7 +153,8 @@ protected final boolean isBlockedProtectedIndexRequest() {
     }
 
     protected final boolean isBlockedSystemIndexRequest() {
-        boolean isSystemIndex = systemIndexMatcher.test(index.getName());
+        boolean matchesSystemIndexRegisteredWithCore = !SystemIndexRegistry.matchesSystemIndexPattern(Set.of(index.getName())).isEmpty();
+        boolean isSystemIndex = systemIndexMatcher.test(index.getName()) || matchesSystemIndexRegisteredWithCore;
         if (!isSystemIndex) {
             return false;
         }
@@ -161,7 +163,7 @@ protected final boolean isBlockedSystemIndexRequest() {
             final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
             if (user == null) {
                 // allow request without user from plugin.
-                return systemIndexMatcher.test(index.getName());
+                return systemIndexMatcher.test(index.getName()) || matchesSystemIndexRegisteredWithCore;
             }
             final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
             final Set<String> mappedRoles = evaluator.mapRoles(user, caller);