[Backport 2.x] System index permission grants reading access to docum…

…ents in the index (opensearch-project#4290)
DarshitChanpura · Apr 26, 2024 · 43cad44 · 43cad44
1 parent b612529
commit 43cad44
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 2 deletions.
diff --git a/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java
@@ -41,6 +41,7 @@
 import org.opensearch.index.IndexService;
 import org.opensearch.security.privileges.PrivilegesEvaluator;
 import org.opensearch.security.securityconf.ConfigModel;
+import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.support.HeaderHelper;
 import org.opensearch.security.support.WildcardMatcher;
@@ -64,6 +65,8 @@ public class SecurityIndexSearcherWrapper implements CheckedFunction<DirectoryRe
     private final Boolean systemIndexEnabled;
     private final WildcardMatcher systemIndexMatcher;
 
+    private final Boolean systemIndexPermissionEnabled;
+
     // constructor is called per index, so avoid costly operations here
     public SecurityIndexSearcherWrapper(
         final IndexService indexService,
@@ -91,6 +94,11 @@ public SecurityIndexSearcherWrapper(
             ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_DEFAULT
         );
         this.systemIndexMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.SECURITY_SYSTEM_INDICES_KEY));
+
+        this.systemIndexPermissionEnabled = settings.getAsBoolean(
+            ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
+            ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT
+        );
     }
 
     @Subscribe
@@ -144,6 +152,17 @@ protected final boolean isBlockedProtectedIndexRequest() {
     }
 
     protected final boolean isBlockedSystemIndexRequest() {
+        if (systemIndexPermissionEnabled) {
+            final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
+            if (user == null) {
+                // allow request without user from plugin.
+                return systemIndexMatcher.test(index.getName());
+            }
+            final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
+            final Set<String> mappedRoles = evaluator.mapRoles(user, caller);
+            final SecurityRoles securityRoles = evaluator.getSecurityRoles(mappedRoles);
+            return !securityRoles.isPermittedOnSystemIndex(index.getName());
+        }
         return systemIndexMatcher.test(index.getName());
     }
 

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -193,7 +193,7 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
         this.dcm = dcm;
     }
 
-    private SecurityRoles getSecurityRoles(Set<String> roles) {
+    public SecurityRoles getSecurityRoles(Set<String> roles) {
         return configModel.getSecurityRoles().filter(roles);
     }
 

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
@@ -542,6 +542,26 @@ public boolean impliesTypePermGlobal(
             roles.stream().forEach(p -> ipatterns.addAll(p.getIpatterns()));
             return ConfigModelV6.impliesTypePerm(ipatterns, resolved, user, actions, resolver, cs);
         }
+
+        @Override
+        public boolean isPermittedOnSystemIndex(String indexName) {
+            boolean isPatternMatched = false;
+            boolean isPermitted = false;
+            for (SecurityRole role : roles) {
+                for (IndexPattern ip : role.getIpatterns()) {
+                    WildcardMatcher wildcardMatcher = WildcardMatcher.from(ip.indexPattern);
+                    if (wildcardMatcher.test(indexName)) {
+                        isPatternMatched = true;
+                    }
+                    for (TypePerm tp : ip.getTypePerms()) {
+                        if (tp.perms.contains(ConfigConstants.SYSTEM_INDEX_PERMISSION)) {
+                            isPermitted = true;
+                        }
+                    }
+                }
+            }
+            return isPatternMatched && isPermitted;
+        }
     }
 
     public static class SecurityRole {

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -562,6 +562,24 @@ private boolean containsDlsFlsConfig() {
 
             return false;
         }
+
+        @Override
+        public boolean isPermittedOnSystemIndex(String indexName) {
+            boolean isPatternMatched = false;
+            boolean isPermitted = false;
+            for (SecurityRole role : roles) {
+                for (IndexPattern ip : role.getIpatterns()) {
+                    WildcardMatcher wildcardMatcher = WildcardMatcher.from(ip.indexPattern);
+                    if (wildcardMatcher.test(indexName)) {
+                        isPatternMatched = true;
+                    }
+                    if (ip.perms.contains(ConfigConstants.SYSTEM_INDEX_PERMISSION)) {
+                        isPermitted = true;
+                    }
+                }
+            }
+            return isPatternMatched && isPermitted;
+        }
     }
 
     public static class SecurityRole {

diff --git a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java
@@ -96,4 +96,6 @@ Set<String> getAllPermittedIndicesForDashboards(
     );
 
     SecurityRoles filter(Set<String> roles);
+
+    boolean isPermittedOnSystemIndex(String indexName);
 }
diff --git a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsV6Test.java b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsV6Test.java
@@ -126,6 +126,17 @@ public void hasExplicitIndexPermission() {
         );
     }
 
+    @Test
+    public void isPermittedOnSystemIndex() {
+        final SecurityRoles securityRoleWithExplicitAccess = configModel.getSecurityRoles()
+            .filter(ImmutableSet.of("has_system_index_permission"));
+        Assert.assertTrue(securityRoleWithExplicitAccess.isPermittedOnSystemIndex(TEST_INDEX));
+
+        final SecurityRoles securityRoleWithStarAccess = configModel.getSecurityRoles()
+            .filter(ImmutableSet.of("all_access_without_system_index_permission"));
+        Assert.assertFalse(securityRoleWithStarAccess.isPermittedOnSystemIndex(TEST_INDEX));
+    }
+
     static <T> SecurityDynamicConfiguration<T> createRolesConfig() throws IOException {
         final ObjectNode rolesNode = DefaultObjectMapper.objectMapper.createObjectNode();
         NO_EXPLICIT_SYSTEM_INDEX_PERMISSION.forEach(rolesNode::set);

diff --git a/src/test/java/org/opensearch/security/system_indices/SystemIndexPermissionEnabledTests.java b/src/test/java/org/opensearch/security/system_indices/SystemIndexPermissionEnabledTests.java
@@ -79,7 +79,8 @@ public void testSearchAsNormalUser() throws Exception {
             if (index.equals(ACCESSIBLE_ONLY_BY_SUPER_ADMIN) || index.equals(SYSTEM_INDEX_WITH_NO_ASSOCIATED_ROLE_PERMISSIONS)) {
                 validateForbiddenResponse(response, "", normalUser);
             } else {
-                validateSearchResponse(response, 0);
+                // got 1 hits because system index permissions are enabled
+                validateSearchResponse(response, 1);
             }
         }
-Original file line number
+Diff line change
@@ Expand Up / @@ -96,4 +96,6 @@ Set<String> getAllPermittedIndicesForDashboards( @@
         );
         SecurityRoles filter(Set<String> roles);
+        boolean isPermittedOnSystemIndex(String indexName);
     }