From a21c4c042c63c2bfb08f42433ace75d0da25847f Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 7 Jul 2023 16:26:05 -0400 Subject: [PATCH] Addresses PR feedback Signed-off-by: Darshit Chanpura --- DEVELOPER_GUIDE.md | 6 +- REST_AUTHZ_FOR_PLUGINS.md | 2 + .../security/action/whoami/WhoAmIAction.java | 2 +- .../privileges/PrivilegesEvaluator.java | 5 +- .../RestLayerPrivilegesEvaluator.java | 2 +- .../security/securityconf/ConfigModelV6.java | 6 -- .../security/securityconf/ConfigModelV7.java | 84 +------------------ .../security/securityconf/SecurityRoles.java | 2 - .../RestLayerPrivilegesEvaluatorTest.java | 9 +- 9 files changed, 11 insertions(+), 107 deletions(-) diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 25244581fa..9b0e9d62c4 100644 --- a/DEVELOPER_GUIDE.md +++ b/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: diff --git a/REST_AUTHZ_FOR_PLUGINS.md b/REST_AUTHZ_FOR_PLUGINS.md index 22d821ccb1..b0f30ed04b 100644 --- a/REST_AUTHZ_FOR_PLUGINS.md +++ b/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. diff --git a/src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java b/src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java index db835ceeae..6b7133cb51 100644 --- a/src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java +++ b/src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java @@ -31,7 +31,7 @@ public class WhoAmIAction extends ActionType { 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); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index b67cf723f9..a3738dadac 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/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( diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java index eadd47f390..edb7289d03 100644 --- a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -99,7 +99,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set 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( diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java index 74cf579501..837dc0cff0 100644 --- a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java +++ b/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 -> { diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java index d7bfbef395..1fb6e4da0e 100644 --- a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java +++ b/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 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 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 legacyPerms; public static final class Builder { private final String name; @@ -632,25 +557,20 @@ public Builder addClusterPerms(Collection 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 ipatterns, WildcardMatcher clusterPerms, Set legacyPerms) { + private SecurityRole(String name, Set 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 action) { - return legacyPerms.stream().anyMatch(action); - } - // get indices which are permitted for the given types and actions // dnfof + opensearchDashboards special only private Set getAllResolvedPermittedIndices( diff --git a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java index 675b0ef4c9..c52a3c1bad 100644 --- a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java +++ b/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 getRoleNames(); diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index 808a85a18c..3c8145c393 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/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()); } }