System index permissions (opensearch-project#2887)

System index permissions Signed-off-by: Sam <[email protected]> Signed-off-by: Sam <[email protected]> Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]> Co-authored-by: Peter Nied <[email protected]>
DarshitChanpura · Sep 7, 2023 · 1379234 · 1379234
1 parent 1034cef
commit 1379234
Show file tree

Hide file tree

Showing 24 changed files with 2,594 additions and 726 deletions.
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
@@ -268,7 +268,7 @@ Checkstyle enforces several rules within this codebase. Sometimes it will be nec
 
 *Suppression All Checkstyle Rules*
 ```
-  // CS-SUPRESS-ALL: Legacy code to be deleted in Z.Y.X see http://github/issues/1234
+  // CS-SUPPRESS-ALL: Legacy code to be deleted in Z.Y.X see http://github/issues/1234
   ...
   // CS-ENFORCE-ALL
 ```

diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -1803,6 +1803,14 @@ public List<Setting<?>> getSettings() {
                     Property.Filtered
                 )
             );
+            settings.add(
+                Setting.boolSetting(
+                    ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
+                    ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT,
+                    Property.NodeScope,
+                    Property.Filtered
+                )
+            );
         }
 
         return settings;

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -314,7 +314,17 @@ public PrivilegesEvaluatorResponse evaluate(
         }
 
         // Security index access
-        if (securityIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse).isComplete()) {
+        if (securityIndexAccessEvaluator.evaluate(
+            request,
+            task,
+            action0,
+            requestedResolved,
+            presponse,
+            securityRoles,
+            user,
+            resolver,
+            clusterService
+        ).isComplete()) {
             return presponse;
         }
 

diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
@@ -448,6 +448,36 @@ public EvaluatedDlsFlsConfig getDlsFls(
             return new EvaluatedDlsFlsConfig(dlsQueries, flsFields, maskedFieldsMap);
         }
 
+        public boolean hasExplicitIndexPermission(
+            Resolved resolved,
+            User user,
+            String[] actions,
+            IndexNameExpressionResolver resolver,
+            ClusterService cs
+        ) {
+            final Set<String> indicesForRequest = new HashSet<>(resolved.getAllIndicesResolved(cs, resolver));
+            if (indicesForRequest.isEmpty()) {
+                // If no indices could be found on the request there is no way to check for the explicit permissions
+                return false;
+            }
+
+            final Set<String> explicitlyAllowedIndices = roles.stream()
+                .map(role -> role.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, true))
+                .flatMap(Collection::stream)
+                .collect(Collectors.toSet());
+
+            if (log.isDebugEnabled()) {
+                log.debug(
+                    "ExplicitIndexPermission check indices for request {}, explicitly allowed indices {}",
+                    indicesForRequest.toString(),
+                    explicitlyAllowedIndices.toString()
+                );
+            }
+
+            indicesForRequest.removeAll(explicitlyAllowedIndices);
+            return indicesForRequest.isEmpty();
+        }
+
         // opensearchDashboards special only, terms eval
         public Set<String> getAllPermittedIndicesForDashboards(
             Resolved resolved,
@@ -458,7 +488,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
         ) {
             Set<String> retVal = new HashSet<>();
             for (SecurityRole sr : roles) {
-                retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs));
+                retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs, false));
                 retVal.addAll(resolved.getRemoteIndices());
             }
             return Collections.unmodifiableSet(retVal);
@@ -468,7 +498,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
         public Set<String> reduce(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs) {
             Set<String> retVal = new HashSet<>();
             for (SecurityRole sr : roles) {
-                retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs));
+                retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, false));
             }
             if (log.isDebugEnabled()) {
                 log.debug("Reduced requested resolved indices {} to permitted indices {}.", resolved, retVal.toString());
@@ -536,7 +566,8 @@ private Set<String> getAllResolvedPermittedIndices(
             User user,
             String[] actions,
             IndexNameExpressionResolver resolver,
-            ClusterService cs
+            ClusterService cs,
+            boolean matchExplicitly
         ) {
 
             final Set<String> retVal = new HashSet<>();
@@ -545,7 +576,11 @@ private Set<String> getAllResolvedPermittedIndices(
                 boolean patternMatch = false;
                 final Set<TypePerm> tperms = p.getTypePerms();
                 for (TypePerm tp : tperms) {
-                    if (tp.typeMatcher.matchAny(resolved.getTypes())) {
+                    // if matchExplicitly is true we don't want to match against `*` pattern
+                    WildcardMatcher matcher = matchExplicitly && (tp.getPerms() == WildcardMatcher.ANY)
+                        ? WildcardMatcher.NONE
+                        : tp.getTypeMatcher();
+                    if (matcher.matchAny(resolved.getTypes())) {
                         patternMatch = tp.getPerms().matchAll(actions);
                     }
                 }

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -34,6 +34,7 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -463,7 +464,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
         ) {
             Set<String> retVal = new HashSet<>();
             for (SecurityRole sr : roles) {
-                retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs));
+                retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs, Function.identity()));
                 retVal.addAll(resolved.getRemoteIndices());
             }
             return Collections.unmodifiableSet(retVal);
@@ -473,7 +474,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
         public Set<String> reduce(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs) {
             Set<String> retVal = new HashSet<>();
             for (SecurityRole sr : roles) {
-                retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs));
+                retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, Function.identity()));
             }
             if (log.isDebugEnabled()) {
                 log.debug("Reduced requested resolved indices {} to permitted indices {}.", resolved, retVal.toString());
@@ -498,10 +499,43 @@ public boolean impliesClusterPermissionPermission(String action) {
 
         @Override
         public boolean hasExplicitClusterPermissionPermission(String action) {
-            return roles.stream()
-                .map(r -> r.clusterPerms == WildcardMatcher.ANY ? WildcardMatcher.NONE : r.clusterPerms)
-                .filter(m -> m.test(action))
-                .count() > 0;
+            return roles.stream().map(r -> matchExplicitly(r.clusterPerms)).filter(m -> m.test(action)).count() > 0;
+        }
+
+        private static WildcardMatcher matchExplicitly(final WildcardMatcher matcher) {
+            return matcher == WildcardMatcher.ANY ? WildcardMatcher.NONE : matcher;
+        }
+
+        @Override
+        public boolean hasExplicitIndexPermission(
+            final Resolved resolved,
+            final User user,
+            final String[] actions,
+            final IndexNameExpressionResolver resolver,
+            final ClusterService cs
+        ) {
+
+            final Set<String> indicesForRequest = new HashSet<>(resolved.getAllIndicesResolved(cs, resolver));
+            if (indicesForRequest.isEmpty()) {
+                // If no indices could be found on the request there is no way to check for the explicit permissions
+                return false;
+            }
+
+            final Set<String> explicitlyAllowedIndices = roles.stream()
+                .map(role -> role.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, SecurityRoles::matchExplicitly))
+                .flatMap(Collection::stream)
+                .collect(Collectors.toSet());
+
+            if (log.isDebugEnabled()) {
+                log.debug(
+                    "ExplicitIndexPermission check indices for request {}, explicitly allowed indices {}",
+                    indicesForRequest.toString(),
+                    explicitlyAllowedIndices.toString()
+                );
+            }
+
+            indicesForRequest.removeAll(explicitlyAllowedIndices);
+            return indicesForRequest.isEmpty();
         }
 
         // rolespan
@@ -578,13 +612,14 @@ private Set<String> getAllResolvedPermittedIndices(
             User user,
             String[] actions,
             IndexNameExpressionResolver resolver,
-            ClusterService cs
+            ClusterService cs,
+            Function<WildcardMatcher, WildcardMatcher> matcherModification
         ) {
 
             final Set<String> retVal = new HashSet<>();
             for (IndexPattern p : ipatterns) {
                 // what if we cannot resolve one (for create purposes)
-                final boolean patternMatch = p.getPerms().matchAll(actions);
+                final boolean patternMatch = matcherModification.apply(p.getPerms()).matchAll(actions);
 
                 // final Set<TypePerm> tperms = p.getTypePerms();
                 // for (TypePerm tp : tperms) {

diff --git a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java
@@ -41,6 +41,18 @@ public interface SecurityRoles {
 
     boolean hasExplicitClusterPermissionPermission(String action);
 
+    /**
+     * Determines if the actions are explicitly granted for indices
+     * @return if all indices in the request have an explicit grant for all actions
+     */
+    boolean hasExplicitIndexPermission(
+        Resolved resolved,
+        User user,
+        String[] actions,
+        IndexNameExpressionResolver resolver,
+        ClusterService cs
+    );
+
     Set<String> getRoleNames();
 
     Set<String> reduce(
@@ -84,5 +96,4 @@ Set<String> getAllPermittedIndicesForDashboards(
     );
 
     SecurityRoles filter(Set<String> roles);
-
 }
diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java
@@ -311,8 +311,11 @@ public enum RolesMappingResolution {
         "opendistro_security_injected_roles_validation_header";
 
     // System indices settings
+    public static final String SYSTEM_INDEX_PERMISSION = "system:admin/system_index";
     public static final String SECURITY_SYSTEM_INDICES_ENABLED_KEY = "plugins.security.system_indices.enabled";
     public static final Boolean SECURITY_SYSTEM_INDICES_ENABLED_DEFAULT = false;
+    public static final String SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY = "plugins.security.system_indices.permission.enabled";
+    public static final Boolean SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT = false;
     public static final String SECURITY_SYSTEM_INDICES_KEY = "plugins.security.system_indices.indices";
     public static final List<String> SECURITY_SYSTEM_INDICES_DEFAULT = Collections.emptyList();
 

diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java
@@ -1010,6 +1010,7 @@ public void testSecurityIndexSecurity() throws Exception {
             encodeBasicHeader("nagilum", "nagilum")
         );
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, res.getStatusCode());
+
         res = rh.executePutRequest(
             "*dis*rit*/_mapping?pretty",
             "{\"properties\": {\"name\":{\"type\":\"text\"}}}",
@@ -1045,9 +1046,8 @@ public void testSecurityIndexSecurity() throws Exception {
             encodeBasicHeader("nagilum", "nagilum")
         );
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, res.getStatusCode());
-        // res = rh.executePostRequest(".opendistro_security/_freeze", "",
-        // encodeBasicHeader("nagilum", "nagilum"));
-        // Assert.assertTrue(res.getStatusCode() >= 400);
+        res = rh.executePostRequest(".opendistro_security/_freeze", "", encodeBasicHeader("nagilum", "nagilum"));
+        Assert.assertEquals(400, res.getStatusCode());
 
         String bulkBody = "{ \"index\" : { \"_index\" : \".opendistro_security\", \"_id\" : \"1\" } }\n"
             + "{ \"field1\" : \"value1\" }\n"

diff --git a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java
@@ -14,6 +14,7 @@
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpStatus;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 import org.opensearch.common.settings.Settings;
@@ -25,6 +26,7 @@ public class PrivilegesEvaluatorTest extends SingleClusterTest {
     private static final Header NegativeLookaheadUserHeader = encodeBasicHeader("negative_lookahead_user", "negative_lookahead_user");
     private static final Header NegatedRegexUserHeader = encodeBasicHeader("negated_regex_user", "negated_regex_user");
 
+    @Before
     public void setupSettingsIndexPattern() throws Exception {
         Settings settings = Settings.builder().build();
         setup(
@@ -39,7 +41,6 @@ public void setupSettingsIndexPattern() throws Exception {
 
     @Test
     public void testNegativeLookaheadPattern() throws Exception {
-        setupSettingsIndexPattern();
 
         RestHelper rh = nonSslRestHelper();
         RestHelper.HttpResponse response = rh.executeGetRequest("*/_search", NegativeLookaheadUserHeader);
@@ -50,8 +51,6 @@ public void testNegativeLookaheadPattern() throws Exception {
 
     @Test
     public void testRegexPattern() throws Exception {
-        setupSettingsIndexPattern();
-
         RestHelper rh = nonSslRestHelper();
         RestHelper.HttpResponse response = rh.executeGetRequest("*/_search", NegatedRegexUserHeader);
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());