From 110426fec9f26071da765d9241aa599209f0443d Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 12 Jun 2024 19:43:48 +0200 Subject: [PATCH] New algorithm for resolving action groups Signed-off-by: Nils Bandener --- .../security/securityconf/ConfigModelV7.java | 88 +--------- .../securityconf/FlattenedActionGroups.java | 161 ++++++++++++++++++ .../impl/SecurityDynamicConfiguration.java | 15 ++ .../FlattenedActionGroupsTest.java | 79 +++++++++ 4 files changed, 261 insertions(+), 82 deletions(-) create mode 100644 src/main/java/org/opensearch/security/securityconf/FlattenedActionGroups.java create mode 100644 src/test/java/org/opensearch/security/securityconf/FlattenedActionGroupsTest.java diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java index a96c9ef814..84902bba3e 100644 --- a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java +++ b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java @@ -75,7 +75,7 @@ public class ConfigModelV7 extends ConfigModel { protected final Logger log = LogManager.getLogger(this.getClass()); private ConfigConstants.RolesMappingResolution rolesMappingResolution; - private ActionGroupResolver agr = null; + private FlattenedActionGroups actionGroups; private SecurityRoles securityRoles = null; private TenantHolder tenantHolder; private RoleMappingHolder roleMappingHolder; @@ -106,7 +106,7 @@ public ConfigModelV7( rolesMappingResolution = ConfigConstants.RolesMappingResolution.MAPPING_ONLY; } - agr = reloadActionGroups(actiongroups); + actionGroups = actiongroups != null ? new FlattenedActionGroups(actiongroups) : FlattenedActionGroups.EMPTY; securityRoles = reload(roles); tenantHolder = new TenantHolder(roles, tenants); roleMappingHolder = new RoleMappingHolder(rolemappings, dcm.getHostsResolverMode()); @@ -120,82 +120,6 @@ public SecurityRoles getSecurityRoles() { return securityRoles; } - private static interface ActionGroupResolver { - Set resolvedActions(final List actions); - } - - private ActionGroupResolver reloadActionGroups(SecurityDynamicConfiguration actionGroups) { - return new ActionGroupResolver() { - - private Set getGroupMembers(final String groupname) { - - if (actionGroups == null) { - return Collections.emptySet(); - } - - return Collections.unmodifiableSet(resolve(actionGroups, groupname)); - } - - @SuppressWarnings("unchecked") - private Set resolve(final SecurityDynamicConfiguration actionGroups, final String entry) { - - // SG5 format, plain array - // List en = actionGroups.getAsList(DotPath.of(entry)); - // if (en.isEmpty()) { - // try SG6 format including readonly and permissions key - // en = actionGroups.getAsList(DotPath.of(entry + "." + ConfigConstants.CONFIGKEY_ACTION_GROUPS_PERMISSIONS)); - // } - - if (!actionGroups.getCEntries().containsKey(entry)) { - return Collections.emptySet(); - } - - final Set ret = new HashSet(); - - final Object actionGroupAsObject = actionGroups.getCEntries().get(entry); - - if (actionGroupAsObject instanceof List) { - - for (final String perm : ((List) actionGroupAsObject)) { - if (actionGroups.getCEntries().keySet().contains(perm)) { - ret.addAll(resolve(actionGroups, perm)); - } else { - ret.add(perm); - } - } - - } else if (actionGroupAsObject instanceof ActionGroupsV7) { - for (final String perm : ((ActionGroupsV7) actionGroupAsObject).getAllowed_actions()) { - if (actionGroups.getCEntries().keySet().contains(perm)) { - ret.addAll(resolve(actionGroups, perm)); - } else { - ret.add(perm); - } - } - } else { - throw new RuntimeException("Unable to handle " + actionGroupAsObject); - } - - return Collections.unmodifiableSet(ret); - } - - @Override - public Set resolvedActions(final List actions) { - final Set resolvedActions = new HashSet(); - for (String string : actions) { - final Set groups = getGroupMembers(string); - if (groups.isEmpty()) { - resolvedActions.add(string); - } else { - resolvedActions.addAll(groups); - } - } - - return Collections.unmodifiableSet(resolvedActions); - } - }; - } - private SecurityRoles reload(SecurityDynamicConfiguration settings) { final Set> futures = new HashSet<>(5000); @@ -213,7 +137,7 @@ public SecurityRole call() throws Exception { return null; } - final Set permittedClusterActions = agr.resolvedActions(securityRole.getValue().getCluster_permissions()); + final Set permittedClusterActions = actionGroups.resolve(securityRole.getValue().getCluster_permissions()); _securityRole.addClusterPerms(permittedClusterActions); /*for(RoleV7.Tenant tenant: securityRole.getValue().getTenant_permissions()) { @@ -240,7 +164,7 @@ public SecurityRole call() throws Exception { _indexPattern.setDlsQuery(dls); _indexPattern.addFlsFields(fls); _indexPattern.addMaskedFields(maskedFields); - _indexPattern.addPerm(agr.resolvedActions(permittedAliasesIndex.getAllowed_actions())); + _indexPattern.addPerm(actionGroups.resolve(permittedAliasesIndex.getAllowed_actions())); /*for(Entry> type: permittedAliasesIndex.getValue().getTypes(-).entrySet()) { TypePerm typePerm = new TypePerm(type.getKey()); @@ -1112,7 +1036,7 @@ public Tuple>> call() throws Exception { tuples.add( new Tuple( matchingTenant, - agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write") + actionGroups.resolve(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write") ) ); } @@ -1126,7 +1050,7 @@ public Tuple>> call() throws Exception { tuples.add( new Tuple( matchingParameterTenant, - agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write") + actionGroups.resolve(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write") ) ); } diff --git a/src/main/java/org/opensearch/security/securityconf/FlattenedActionGroups.java b/src/main/java/org/opensearch/security/securityconf/FlattenedActionGroups.java new file mode 100644 index 0000000000..17863bb8ca --- /dev/null +++ b/src/main/java/org/opensearch/security/securityconf/FlattenedActionGroups.java @@ -0,0 +1,161 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.securityconf; + +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7; + +/** + * This class pre-computes a flattened/resolved view of all provided action groups. Afterwards, the resolve() method + * can be used to retrieve the resolved actions with just a lookup instead of an expensive computation. + * + * Instead of a recursive algorithm, this class uses a iterative algorithm that terminates as soon as the result set + * did not change during the previous iteration (i.e., when the result set "settled"). This will also terminate early + * for loops within the action group definition, as loops do not add any more elements to the result set after the first + * encounter of them. + * + * Still, if the algorithm has not settled after 1000 iterations, it will terminate "early". This will be only the case + * for nested action group definitions with a nesting level of more than 1000. + * + * Instances of this class are immutable. If the action group configuration is updated, a new instance needs to be + * created. + */ +public class FlattenedActionGroups { + public static final FlattenedActionGroups EMPTY = new FlattenedActionGroups(); + + private static final Logger log = LogManager.getLogger(FlattenedActionGroups.class); + + private final ImmutableMap> resolvedActionGroups; + + public FlattenedActionGroups(SecurityDynamicConfiguration actionGroups) { + // Maps action group names to the actions and action groups the particular action group points to + Map> resolved = new HashMap<>(actionGroups.getCEntries().size()); + + // Maps action group names to further action group names found in the provided action group configuration. + // These will need an additional resolution step to resolve recursive definitions. + Map> needsResolution = new HashMap<>(actionGroups.getCEntries().size()); + + // First phase: Non-recursive definitions + // + // We iterate through all defined action groups and initialize the "resolved" map with the + // first, non-recursive action group mappings. If we discover that an action group maps to a value which + // is also a key in the action group config, we know that we have found a recursive definition. This is not + // yet resolved, but scheduled for resolution by putting the mapping additionally into "needsResolution". + for (Map.Entry entry : actionGroups.getCEntries().entrySet()) { + String key = entry.getKey(); + + Set actions = resolved.computeIfAbsent(key, (k) -> new HashSet<>()); + + for (String action : entry.getValue().getAllowed_actions()) { + actions.add(action); + + if (actionGroups.getCEntries().containsKey(action) && !action.equals(key)) { + needsResolution.computeIfAbsent(key, (k) -> new HashSet<>()).add(action); + } + } + } + + // Second phase: recursive definitions + // + // We iterate through "needsResolution", i.e., the discovered recursive definitions and use the already + // computed mappings in "resolved" to resolve these recursive definitions. In this course, the mappings in + // "resolved" grow. As "resolved" might be not complete in the first iteration, we iterate until no further + // change is observed - only then "resolved" can be considered as complete. + // + // Note: "needsResolution" will be not changed in this phase. We certainly will not discover additional + // recursive definitions. One could argue that it might be possible to remove some entries from "needsResolution" + // as soon as these are discovered to be complete. But that would require additional copy operations and + // complicate the algorithm which does not seem to be worth the possible gain. + boolean settled = false; + + for (int i = 0; !settled; i++) { + boolean changed = false; + + for (Map.Entry> entry : needsResolution.entrySet()) { + String key = entry.getKey(); + Set resolvedActions = resolved.get(key); + + for (String action : entry.getValue()) { + Set mappedActions = resolved.get(action); + changed |= resolvedActions.addAll(mappedActions); + } + } + + if (!changed) { + settled = true; + if (log.isDebugEnabled()) { + log.debug("Action groups settled after {} loops.\nResolved: {}", i, resolved); + } + } + + if (i >= 1000) { + log.error("Found too deeply nested action groups. Aborting resolution.\nResolved so far: {}", resolved); + break; + } + } + + this.resolvedActionGroups = ImmutableMap.copyOf(resolved); + } + + /** + * Resolves the given list of actions or action groups using the pre-computed flattened index. + * The result set will always contain AT LEAST the provided elements. IN ADDITION, any elements discovered + * in the index will be added. + * + * Thus, if you provide [a,b] as parameters, and the index contains [b=>1,2], then the result set + * will contain [a,b,1,2]. + * + * This method will not perform any pattern matching. It will also not give the strings any semantics based on their + * contents. + */ + public ImmutableSet resolve(Collection actions) { + ImmutableSet.Builder result = ImmutableSet.builder(); + + for (String action : actions) { + if (action == null) { + continue; + } + + result.add(action); + + Set mappedActions = this.resolvedActionGroups.get(action); + if (mappedActions != null) { + result.addAll(mappedActions); + } + } + + return result.build(); + } + + /** + * Private constructor for creating an empty instance + */ + private FlattenedActionGroups() { + this.resolvedActionGroups = ImmutableMap.of(); + } + + @Override + public String toString() { + return resolvedActionGroups.toString(); + } + +} diff --git a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java index 83553f2de7..e1dd1602ac 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java @@ -38,6 +38,7 @@ import com.fasterxml.jackson.annotation.JsonAnyGetter; import com.fasterxml.jackson.annotation.JsonAnySetter; import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; @@ -118,6 +119,20 @@ public static SecurityDynamicConfiguration fromJson( return sdc; } + /** + * For testing only + */ + public static SecurityDynamicConfiguration fromMap(Map map, CType ctype, int version) + throws JsonProcessingException { + Class implementationClass = ctype.getImplementationClass().get(version); + SecurityDynamicConfiguration result = DefaultObjectMapper.objectMapper.convertValue( + map, + DefaultObjectMapper.getTypeFactory().constructParametricType(SecurityDynamicConfiguration.class, implementationClass) + ); + result.ctype = ctype; + return result; + } + public static void validate(SecurityDynamicConfiguration sdc, int version, CType ctype) throws IOException { if (version < 2 && sdc.get_meta() != null) { throw new IOException("A version of " + version + " can not have a _meta key for " + ctype); diff --git a/src/test/java/org/opensearch/security/securityconf/FlattenedActionGroupsTest.java b/src/test/java/org/opensearch/security/securityconf/FlattenedActionGroupsTest.java new file mode 100644 index 0000000000..bdbfa0e19c --- /dev/null +++ b/src/test/java/org/opensearch/security/securityconf/FlattenedActionGroupsTest.java @@ -0,0 +1,79 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.securityconf; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import org.junit.Assert; +import org.junit.Test; + +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7; + +public class FlattenedActionGroupsTest { + @Test + public void basicTest() throws Exception { + TestActionGroup testActionGroups = new TestActionGroup().with("Z", "C", "A") + .with("A", "A1", "A2", "A3") + .with("B", "B1", "B2", "B3") + .with("C", "A", "B", "C1"); + SecurityDynamicConfiguration config = SecurityDynamicConfiguration.fromMap( + testActionGroups.map, + CType.ACTIONGROUPS, + 2 + ); + + FlattenedActionGroups actionGroups = new FlattenedActionGroups(config); + + Assert.assertEquals( + ImmutableSet.of("C", "A", "A1", "A2", "A3", "C1", "B", "B1", "B2", "B3", "Z"), + actionGroups.resolve(ImmutableSet.of("Z")) + ); + Assert.assertEquals(ImmutableSet.of("A", "A1", "A2", "A3"), actionGroups.resolve(ImmutableSet.of("A"))); + } + + /** + * This tests an action group definition containing a cycle. Still, the resolution should settle without any + * stack overflow. + */ + @Test + public void cycleTest() throws Exception { + TestActionGroup testActionGroups = new TestActionGroup().with("A", "A1", "B") + .with("B", "B1", "C") + .with("C", "C1", "A", "D") + .with("D", "D1"); + SecurityDynamicConfiguration config = SecurityDynamicConfiguration.fromMap( + testActionGroups.map, + CType.ACTIONGROUPS, + 2 + ); + + FlattenedActionGroups actionGroups = new FlattenedActionGroups(config); + + Assert.assertEquals(ImmutableSet.of("A", "A1", "B", "B1", "C", "C1", "D", "D1"), actionGroups.resolve(ImmutableSet.of("A"))); + Assert.assertEquals(ImmutableSet.of("A", "A1", "B", "B1", "C", "C1", "D", "D1"), actionGroups.resolve(ImmutableSet.of("C"))); + Assert.assertEquals(ImmutableSet.of("D", "D1"), actionGroups.resolve(ImmutableSet.of("D"))); + } + + private static class TestActionGroup { + private Map map = new HashMap<>(); + + TestActionGroup with(String key, String... actions) { + map.put(key, ImmutableMap.of("allowed_actions", Arrays.asList(actions))); + return this; + } + } +}