Addresses PR feedback

Signed-off-by: Darshit Chanpura <[email protected]>

DEVELOPER_GUIDE.md

@@ -84,7 +84,7 @@ rm -rf config/
 If you are working with an extension and want to set up demo users for the Hello-World extension, append following items to files inside `$OPENSEARCH_HOME/config/opensearch-security/`:
 1. In **internal_users.yml**
 ```yaml
-new-user:
+hw-user:
   hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG"
   reserved: true
   description: "Demo user for ext-test"
@@ -116,12 +116,12 @@ legacy_hw_greet_with_name:
 legacy_hw_greet_with_name:
   reserved: true
   users:
-    - "new-user"
+    - "hw-user"
 
 extension_hw_greet:
   reserved: true
   users:
-    - "new-user"
+    - "hw-user"
 ```
 
 To install the demo certificates and default configuration, answer `y` to the first two questions and `n` to the last one. The log should look like below:

REST_AUTHZ_FOR_PLUGINS.md

@@ -2,6 +2,8 @@
 
 This feature is introduced as an added layer of security on top of existing TransportLayer authorization framework. In order to leverage these feature some core changes need to be made at Route registration level. This document talks about how you can achieve this.
 
+**NOTE:** This doesn't replace Transport Layer Authorization. Plugin developers may choose to skip creating transport actions for APIs that do not need interaction with the Transport Layer.
+
 ## Pre-requisites
 
 The security plugin must be installed and operational in your OpenSearch cluster for this feature to work.

src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java

@@ -31,7 +31,7 @@
 public class WhoAmIAction extends ActionType<WhoAmIResponse> {
 
     public static final WhoAmIAction INSTANCE = new WhoAmIAction();
-    public static final String NAME = "cluster:admin/opendistro_security/whoamiprotected";
+    public static final String NAME = "cluster:admin/opendistro_security/whoami";
 
     protected WhoAmIAction() {
         super(NAME, WhoAmIResponse::new);

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -338,10 +338,7 @@ public PrivilegesEvaluatorResponse evaluate(
         );
 
         if (isClusterPerm(action0)) {
-            // We add a check here for `impliesLegacyPermission` to ensure that no new action names miss evaluation here
-            // e.g. ad:get/detector/info will be evaluated as `cluster:admin/opensearch/ad/get/detector/info` against user's granted
-            // permissions
-            if (!securityRoles.impliesClusterPermissionPermission(action0) && !securityRoles.impliesLegacyPermission(action0)) {
+            if (!securityRoles.impliesClusterPermissionPermission(action0)) {
                 presponse.missingPrivileges.add(action0);
                 presponse.allowed = false;
                 log.info(

src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java

@@ -99,7 +99,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set<String> action0
         }
 
         for (String action : action0) {
-            if (!securityRoles.impliesClusterPermissionPermission(action) && !securityRoles.impliesLegacyPermission(action)) {
+            if (!securityRoles.impliesClusterPermissionPermission(action)) {
                 presponse.missingPrivileges.add(action);
                 presponse.allowed = false;
                 log.info(

src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java

@@ -491,12 +491,6 @@ public boolean impliesClusterPermissionPermission(String action) {
             return roles.stream().filter(r -> r.impliesClusterPermission(action)).count() > 0;
         }
 
-        @Override
-        public boolean impliesLegacyPermission(String action0) {
-            // no support for ES 6 and below
-            return false;
-        }
-
         @Override
         public boolean hasExplicitClusterPermissionPermission(String action) {
             return roles.stream().map(r -> {

src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java

@@ -34,7 +34,6 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Predicate;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -497,78 +496,6 @@ public boolean impliesClusterPermissionPermission(String action) {
             return roles.stream().filter(r -> r.impliesClusterPermission(action)).count() > 0;
         }
 
-        /**
-         * Checks if the route is accessible via legacy naming convention.
-         * We check against cluster permissions because the legacy convention for cluster permissions map 1-1 with a transport
-         * action call initiated via REST API handler. Hence we use the same to allow/block request forwarding to ext.
-         * This ensures backwards-compatibility
-         *
-         * This assumes the convention that every new route register will be of type `plugin:routeName` and will then c
-         *
-         * NOTE: THIS CHECK WILL BE REMOVED ONCE ALL ACTIONS HAVE BEEN MIGRATED TO THE NEW CONVENTION
-         *
-         * E.g For ext `hw`, following are two possible ways actions an be defined in roles:
-         *
-         * ext_hw_greet:
-         *   reserved: true
-         *   cluster_permissions:
-         *     - 'hw:greet'
-         *
-         * legacy_hw_greet:
-         *   reserved: true
-         *   cluster_permissions:
-         *     - 'cluster:admin/opensearch/hw/greet'
-         *
-         *
-         * @param action - The action to be checked against its legacy version
-         * @return true, if a legacy version was found and validated, false otherwise
-         */
-        @Override
-        public boolean impliesLegacyPermission(String action) {
-            boolean isAlreadyLegacy = action.startsWith("cluster:admin/open");
-            if (isAlreadyLegacy) {
-                return false; // this check was already made, so return false
-            }
-
-            log.info("Checking legacy permissions for {}", action);
-
-            // This assumes that the route name will be one of these two formats: `shortName:action` OR `action`
-            String[] parts = action.split(":");
-            if (parts.length > 1) {
-                action = parts[1];  // e.g. `hw:greet` would check for action `greet` for plugin/ext `hw`
-            } else {
-                action = parts[0]; // e.g. `greet` would check for action `greet`
-            }
-
-            /* Regex: `/(?:cluster:admin\/\b(open(distro|search))\b\/[a-zA-Z]+\/|\*)action\/?(?:\*|[\/a-zA-Z0-9]*)/gm`
-             *  matches:
-             *  *action*
-             *  cluster:admin/opensearch/abcd/action
-             *  cluster:admin/opensearch/abcd/action*
-             *  cluster:admin/opensearch/abcd/action/*
-             *  cluster:admin/opensearch/abcd/action/a/*
-             *  cluster:admin/opensearch/abcd/action/a/b/c
-             *  *action*
-             *  *action/abc
-             *
-             *  doesn't match:
-             *  action
-             *  action*
-             *  action/
-             *  indices:admin/action/
-             *
-             * This regex is plugin name/shortName agnostic, to provide flexibility in providing shortnames for plugin devs when registering routes.
-             *
-             *  For more details on regex, please visit regex101.com and paste the regex
-             */
-            String legacyActionMatchRegex = "(?:cluster:admin/\\\\b(open(distro|search))\\\\b/[a-zA-Z]+/|\\\\*)%s/?(?:\\\\*|[/a-zA-Z0-9]*)";
-
-            String regex = String.format(legacyActionMatchRegex, action);
-            Predicate<String> pattern = Pattern.compile(regex).asPredicate();
-
-            return roles.stream().filter(r -> r.impliesLegacyPermission(pattern)).count() > 0;
-        }
-
         @Override
         public boolean hasExplicitClusterPermissionPermission(String action) {
             return roles.stream()
@@ -607,8 +534,6 @@ public static class SecurityRole {
         private final String name;
         private final Set<IndexPattern> ipatterns;
         private final WildcardMatcher clusterPerms;
-        // Holds all legacy permissions of this role in a Set format to then be matched by a predicate via impliesLegacyPermission()
-        private final Set<String> legacyPerms;
 
         public static final class Builder {
             private final String name;
@@ -632,25 +557,20 @@ public Builder addClusterPerms(Collection<String> clusterPerms) {
             }
 
             public SecurityRole build() {
-                return new SecurityRole(name, ipatterns, WildcardMatcher.from(clusterPerms), clusterPerms);
+                return new SecurityRole(name, ipatterns, WildcardMatcher.from(clusterPerms));
             }
         }
 
-        private SecurityRole(String name, Set<IndexPattern> ipatterns, WildcardMatcher clusterPerms, Set<String> legacyPerms) {
+        private SecurityRole(String name, Set<IndexPattern> ipatterns, WildcardMatcher clusterPerms) {
             this.name = Objects.requireNonNull(name);
             this.ipatterns = ipatterns;
             this.clusterPerms = clusterPerms;
-            this.legacyPerms = legacyPerms;
         }
 
         private boolean impliesClusterPermission(String action) {
             return clusterPerms.test(action);
         }
 
-        private boolean impliesLegacyPermission(Predicate<String> action) {
-            return legacyPerms.stream().anyMatch(action);
-        }
-
         // get indices which are permitted for the given types and actions
         // dnfof + opensearchDashboards special only
         private Set<String> getAllResolvedPermittedIndices(

src/main/java/org/opensearch/security/securityconf/SecurityRoles.java

@@ -39,8 +39,6 @@ public interface SecurityRoles {
 
     boolean impliesClusterPermissionPermission(String action0);
 
-    boolean impliesLegacyPermission(String action0);
-
     boolean hasExplicitClusterPermissionPermission(String action);
 
     Set<String> getRoleNames();

src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java

@@ -92,7 +92,6 @@ public void testEvaluate_Initialized_Success() {
         when(configModel.getSecurityRoles()).thenReturn(securityRoles);
         when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles);
         when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(false);
-        when(securityRoles.impliesLegacyPermission(action)).thenReturn(false);
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
@@ -127,13 +126,11 @@ public void testEvaluate_Successful_NewPermission() {
         when(configModel.getSecurityRoles()).thenReturn(securityRoles);
         when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles);
         when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(true);
-        when(securityRoles.impliesLegacyPermission(action)).thenReturn(true);
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
         assertTrue(response.allowed);
         verify(securityRoles).impliesClusterPermissionPermission(any());
-        verify(securityRoles, times(0)).impliesLegacyPermission(any());
     }
 
     @Test
@@ -142,14 +139,12 @@ public void testEvaluate_Successful_LegacyPermission() {
         SecurityRoles securityRoles = mock(SecurityRoles.class);
         when(configModel.getSecurityRoles()).thenReturn(securityRoles);
         when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles);
-        when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(false);
-        when(securityRoles.impliesLegacyPermission(action)).thenReturn(true);
+        when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(true);
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
         assertTrue(response.allowed);
         verify(securityRoles).impliesClusterPermissionPermission(any());
-        verify(securityRoles).impliesLegacyPermission(any());
     }
 
     @Test
@@ -159,12 +154,10 @@ public void testEvaluate_Unsuccessful() {
         when(configModel.getSecurityRoles()).thenReturn(securityRoles);
         when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles);
         when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(false);
-        when(securityRoles.impliesLegacyPermission(action)).thenReturn(false);
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
         assertFalse(response.allowed);
         verify(securityRoles).impliesClusterPermissionPermission(any());
-        verify(securityRoles).impliesLegacyPermission(any());
     }
 }