From 41dd98686a179f3ddd539acb8d2a6531dd82fa3f Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Sat, 13 Jul 2024 08:57:46 +0200 Subject: [PATCH] Removed all references to SecurityRoles except DLS/FLS Signed-off-by: Nils Bandener --- .../SecurityIndexSearcherWrapper.java | 13 +++-- .../security/privileges/ActionPrivileges.java | 55 ++++++++++++++++--- .../privileges/PrivilegesEvaluator.java | 17 ++---- .../resolver/IndexResolverReplacer.java | 13 +++++ .../privileges/ActionPrivilegesTest.java | 33 ++++++++++- 5 files changed, 105 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java index 7a40e5dbd0..b666a96435 100644 --- a/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java +++ b/src/main/java/org/opensearch/security/configuration/SecurityIndexSearcherWrapper.java @@ -39,7 +39,10 @@ import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.core.index.Index; import org.opensearch.index.IndexService; +import org.opensearch.security.privileges.PrivilegesEvaluationContext; import org.opensearch.security.privileges.PrivilegesEvaluator; +import org.opensearch.security.privileges.PrivilegesEvaluatorResponse; +import org.opensearch.security.resolver.IndexResolverReplacer; import org.opensearch.security.securityconf.ConfigModel; import org.opensearch.security.securityconf.SecurityRoles; import org.opensearch.security.support.ConfigConstants; @@ -163,10 +166,12 @@ protected final boolean isBlockedSystemIndexRequest() { // allow request without user from plugin. return systemIndexMatcher.test(index.getName()); } - final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); - final Set mappedRoles = evaluator.mapRoles(user, caller); - final SecurityRoles securityRoles = evaluator.getSecurityRoles(mappedRoles); - return !securityRoles.isPermittedOnSystemIndex(index.getName()); + + String permission = ConfigConstants.SYSTEM_INDEX_PERMISSION; + PrivilegesEvaluationContext context = evaluator.createContext(user, permission); + PrivilegesEvaluatorResponse result = evaluator.getActionPrivileges().hasExplicitIndexPrivilege(context, Set.of(permission), IndexResolverReplacer.Resolved.ofIndex(index.getName())); + + return !result.isAllowed(); } return true; } diff --git a/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java b/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java index 3f6dc61fca..255d2e4b7b 100644 --- a/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java @@ -115,6 +115,10 @@ public PrivilegesEvaluatorResponse hasAnyClusterPrivilege(PrivilegesEvaluationCo return cluster.providesAnyPrivilege(context, actions, context.getMappedRoles()); } + public PrivilegesEvaluatorResponse hasExplicitClusterPrivilege(PrivilegesEvaluationContext context, String action) { + return cluster.providesExplicitPrivilege(context, action, context.getMappedRoles()); + } + public PrivilegesEvaluatorResponse hasIndexPrivilege( PrivilegesEvaluationContext context, Set actions, @@ -191,6 +195,9 @@ static class ClusterPrivileges { * Maps names of actions to the roles that provide a privilege for the respective action. * Note that the mapping is not comprehensive, additionally the data structures rolesWithWildcardPermissions * and rolesToActionMatcher need to be considered for a full view of the privileges. + * + * This does not include privileges obtained via "*" action patterns. This is both meant as a + * optimization and to support explicit privileges. */ private final ImmutableMap> actionToRoles; @@ -205,6 +212,9 @@ static class ClusterPrivileges { * This is only used as a last resort if the test with actionToRole and rolesWithWildcardPermissions failed. * This is only necessary for actions which are not contained in the list of "well-known" actions provided * during construction. + * + * This does not include privileges obtained via "*" action patterns. This is both meant as a + * optimization and to support explicit privileges. */ private final ImmutableMap rolesToActionMatcher; @@ -224,6 +234,7 @@ static class ClusterPrivileges { ImmutableSet wellKnownClusterActions ) { Map> actionToRoles = new HashMap<>(); + Map> explicitPermissionToRoles = new HashMap<>(); ImmutableSet.Builder rolesWithWildcardPermissions = ImmutableSet.builder(); ImmutableMap.Builder rolesToActionMatcher = ImmutableMap.builder(); @@ -236,13 +247,6 @@ static class ClusterPrivileges { // This list collects all the matchers for action names that will be found for the current role List wildcardMatchers = new ArrayList<>(); - // Special case: Roles with a wildcard "*" giving privileges for all actions. We will not resolve - // this stuff, but just note separately that this role just gets all the cluster privileges. - if (permissionPatterns.contains("*")) { - rolesWithWildcardPermissions.add(roleName); - continue; - } - for (String permission : permissionPatterns) { // If we have a permission which does not use any pattern, we just simply add it to the // "actionToRoles" map. @@ -252,6 +256,10 @@ static class ClusterPrivileges { if (WildcardMatcher.isExact(permission)) { actionToRoles.computeIfAbsent(permission, k -> new HashSet<>()).add(roleName); + } else if (permission.equals("*")) { + // Special case: Roles with a wildcard "*" giving privileges for all actions. We will not resolve + // this stuff, but just note separately that this role just gets all the cluster privileges. + rolesWithWildcardPermissions.add(roleName); } else { WildcardMatcher wildcardMatcher = WildcardMatcher.from(permission); Set matchedActions = wildcardMatcher.getMatchAny( @@ -316,6 +324,39 @@ PrivilegesEvaluatorResponse providesPrivilege(PrivilegesEvaluationContext contex return PrivilegesEvaluatorResponse.insufficient(action, context); } + /** + * Checks whether this instance provides explicit privileges for the combination of the provided action and the + * provided roles. + * + * Explicit means here that the privilege is not granted via a "*" action privilege wildcard. Other patterns + * are possible. See also: https://github.com/opensearch-project/security/pull/2411 and https://github.com/opensearch-project/security/issues/3038 + * + * Returns a PrivilegesEvaluatorResponse with allowed=true if privileges are available. + * Otherwise, allowed will be false and missingPrivileges will contain the name of the given action. + */ + PrivilegesEvaluatorResponse providesExplicitPrivilege(PrivilegesEvaluationContext context, String action, Set roles) { + + // 1: Check well-known actions - this should cover most cases + ImmutableSet rolesWithPrivileges = this.actionToRoles.get(action); + + if (rolesWithPrivileges != null && CollectionUtils.containsAny(roles, rolesWithPrivileges)) { + return PrivilegesEvaluatorResponse.ok(); + } + + // 2: Only if everything else fails: Check the matchers in case we have a non-well-known action + if (!this.wellKnownClusterActions.contains(action)) { + for (String role : roles) { + WildcardMatcher matcher = this.rolesToActionMatcher.get(role); + + if (matcher != null && matcher.test(action)) { + return PrivilegesEvaluatorResponse.ok(); + } + } + } + + return PrivilegesEvaluatorResponse.insufficient(action, context); + } + /** * Checks whether this instance provides privileges for the combination of any of the provided actions and the * provided roles. Returns a PrivilegesEvaluatorResponse with allowed=true if privileges are available. diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 77a8370f64..aec2b7275e 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -253,22 +253,13 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { this.dcm = dcm; } - ActionPrivileges getActionPrivileges() { + public ActionPrivileges getActionPrivileges() { return this.actionPrivileges; } - public SecurityRoles getSecurityRoles(Set roles) { - return configModel.getSecurityRoles().filter(roles); - } - - public boolean hasRestAdminPermissions(final User user, final TransportAddress remoteAddress, final String permissions) { - final Set userRoles = mapRoles(user, remoteAddress); - return hasRestAdminPermissions(userRoles, permissions); - } - - private boolean hasRestAdminPermissions(final Set roles, String permission) { - final SecurityRoles securityRoles = getSecurityRoles(roles); - return securityRoles.hasExplicitClusterPermissionPermission(permission); + public boolean hasRestAdminPermissions(final User user, final TransportAddress remoteAddress, final String permission) { + PrivilegesEvaluationContext context = createContext(user, permission); + return this.actionPrivileges.hasExplicitClusterPrivilege(context, permission).isAllowed(); } public boolean isInitialized() { diff --git a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java index ef372ad4dd..9c852ee094 100644 --- a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java @@ -30,6 +30,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -458,6 +459,12 @@ public final static class Resolved { SearchRequest.DEFAULT_INDICES_OPTIONS ); + + private static final IndicesOptions EXACT_INDEX_OPTIONS = new IndicesOptions( + EnumSet.of(IndicesOptions.Option.FORBID_ALIASES_TO_MULTIPLE_INDICES), + EnumSet.noneOf(IndicesOptions.WildcardStates.class) + ); + private final Set aliases; private final Set allIndices; private final Set originalRequested; @@ -465,6 +472,7 @@ public final static class Resolved { private final boolean isLocalAll; private final IndicesOptions indicesOptions; + public Resolved( final ImmutableSet aliases, final ImmutableSet allIndices, @@ -563,6 +571,11 @@ public boolean equals(Object obj) { } else if (!remoteIndices.equals(other.remoteIndices)) return false; return true; } + + public static Resolved ofIndex(String index) { + ImmutableSet indexSet = ImmutableSet.of(index); + return new Resolved(ImmutableSet.of(), indexSet, indexSet, ImmutableSet.of(), EXACT_INDEX_OPTIONS); + } } private List renamedIndices(final RestoreSnapshotRequest request, final List filteredIndices) { diff --git a/src/test/java/org/opensearch/security/privileges/ActionPrivilegesTest.java b/src/test/java/org/opensearch/security/privileges/ActionPrivilegesTest.java index 8fedc05683..831444ccfa 100644 --- a/src/test/java/org/opensearch/security/privileges/ActionPrivilegesTest.java +++ b/src/test/java/org/opensearch/security/privileges/ActionPrivilegesTest.java @@ -96,6 +96,8 @@ public void clusterAction_notWellKnown() throws Exception { isForbidden(missingPrivileges("cluster:monitor/nodes/something/else")) ); } + + // TODO explicit and stuff } /** @@ -173,7 +175,29 @@ public void negative_wrongRole() throws Exception { public void negative_wrongAction() throws Exception { PrivilegesEvaluationContext ctx = ctx("test_role"); PrivilegesEvaluatorResponse result = subject.hasIndexPrivilege(ctx, otherActions, resolved("index_a11")); - assertThat(result, isForbidden(missingPrivileges(otherActions))); + + if (actionSpec.givenPrivs.contains("*")) { + assertThat(result, isAllowed()); + } else { + assertThat(result, isForbidden(missingPrivileges(otherActions))); + } + } + + @Test + public void positive_hasExplicit_full() { + PrivilegesEvaluationContext ctx = ctx("test_role"); + PrivilegesEvaluatorResponse result = subject.hasExplicitIndexPrivilege(ctx, requiredActions, resolved("index_a11")); + + if (actionSpec.givenPrivs.contains("*")) { + // The * is forbidden for explicit privileges + assertThat(result, isForbidden(missingPrivileges(requiredActions))); + } else if (!requiredActions.contains("indices:data/read/search")) { + // For test purposes, we have designated "indices:data/read/search" as a action requiring explicit privileges + // Other actions are not covered here + assertThat(result, isForbidden(missingPrivileges(requiredActions))); + } else { + assertThat(result, isAllowed()); + } } private boolean covers(PrivilegesEvaluationContext ctx, String... indices) { @@ -198,6 +222,9 @@ public static Collection params() { new IndexSpec().givenIndexPrivs("alias_a1*") // )) { for (ActionSpec actionSpec : Arrays.asList( + new ActionSpec("wildcard")// + .givenPrivs("*") + .requiredPrivs("indices:data/read/search"), // new ActionSpec("constant, well known")// .givenPrivs("indices:data/read/search") .requiredPrivs("indices:data/read/search"), // @@ -239,7 +266,9 @@ public IndicesAndAliases(IndexSpec indexSpec, ActionSpec actionSpec, Statefulnes : ImmutableSet.of("indices:foobar/unknown"); this.indexSpec.indexMetadata = INDEX_METADATA; - this.subject = new ActionPrivileges(roles, FlattenedActionGroups.EMPTY, () -> INDEX_METADATA); + this.subject = new ActionPrivileges(roles, FlattenedActionGroups.EMPTY, () -> INDEX_METADATA, WellKnownActions.CLUSTER_ACTIONS, + WellKnownActions.INDEX_ACTIONS, + WellKnownActions.INDEX_ACTIONS); if (statefulness == Statefulness.STATEFUL) { this.subject.updateStatefulIndexPrivileges(INDEX_METADATA);