diff --git a/CHANGELOG.md b/CHANGELOG.md index d00370b5d4..af91cbf205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Enable basic authentication for gRPC transport ([#6005](https://github.com/opensearch-project/security/pull/6005)) - Allow specifying parentType and parentIdField in ResourceProvider ([#5735](https://github.com/opensearch-project/security/pull/5735)) - [Resource Sharing] Allow specifying default access level in resource access levels yml file ([#6018](https://github.com/opensearch-project/security/pull/6018)) +- Only update internal compiled privileges configuration when the base config objects have actually changed ([#6037](https://github.com/opensearch-project/security/pull/6037)) ### Bug Fixes - Fix audit log writing errors for rollover-enabled alias indices ([#5878](https://github.com/opensearch-project/security/pull/5878) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java b/src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java index 8fcaf55ca0..4f9d618844 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java @@ -12,6 +12,7 @@ import java.util.HashMap; import java.util.Map; +import java.util.Objects; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -61,8 +62,7 @@ public class PrivilegesConfiguration { private final AtomicReference tenantPrivileges = new AtomicReference<>(TenantPrivileges.EMPTY); private final AtomicReference privilegesEvaluator; - private final AtomicReference actionGroups = new AtomicReference<>(FlattenedActionGroups.EMPTY); - private final AtomicReference compiledRoles = new AtomicReference<>(); + private final AtomicReference rawConfiguration = new AtomicReference<>(); private final Map pluginIdToRolePrivileges = new HashMap<>(); private final AtomicReference multiTenancyConfiguration = new AtomicReference<>( DashboardsMultiTenancyConfiguration.DEFAULT @@ -121,16 +121,14 @@ public PrivilegesConfiguration( .withStaticConfig(); ConfigV7 generalConfiguration = configurationRepository.getConfiguration(CType.CONFIG).getCEntry(CType.CONFIG.name()); - FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsConfiguration.withStaticConfig()); - this.actionGroups.set(flattenedActionGroups); - - CompiledRoles newCompiledRoles = new CompiledRoles( - rolesConfiguration.withStaticConfig(), - flattenedActionGroups, - namedXContentRegistry, - fieldMaskingConfig + RawConfiguration rawConfiguration = new RawConfiguration( + actionGroupsConfiguration, + rolesConfiguration, + tenantConfiguration, + PrivilegesEvaluator.GlobalDynamicSettings.fromConfigV7(generalConfiguration) ); - this.compiledRoles.set(newCompiledRoles); + + RawConfiguration oldRawConfiguration = this.rawConfiguration.getAndSet(rawConfiguration); PrivilegesEvaluator currentPrivilegesEvaluator = privilegesEvaluator.get(); @@ -139,45 +137,74 @@ public PrivilegesConfiguration( PrivilegesEvaluator.PrivilegesEvaluatorType targetType = PrivilegesEvaluator.PrivilegesEvaluatorType.STANDARD; PrivilegesEvaluator.PrivilegesEvaluatorType currentType = currentPrivilegesEvaluator.type(); - if (currentType != targetType) { - PrivilegesEvaluator oldInstance = privilegesEvaluator.getAndSet( - new org.opensearch.security.privileges.PrivilegesEvaluatorImpl( - clusterService, - clusterStateSupplier, - roleMapper, - threadPool, - threadPool.getThreadContext(), - resolver, - auditLog, - settings, - privilegesInterceptor, - indexResolverReplacer, - flattenedActionGroups, - staticActionGroups, - newCompiledRoles, - generalConfiguration, - pluginIdToRolePrivileges - ) + boolean privilegesChanged = oldRawConfiguration == null + || !oldRawConfiguration.equals(rawConfiguration) + || currentType != targetType; + if (privilegesChanged) { + log.debug("Privileges for PrivilegesEvaluator and DLS/FLS did not change; updating."); + + FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsConfiguration.withStaticConfig()); + + CompiledRoles newCompiledRoles = new CompiledRoles( + rolesConfiguration.withStaticConfig(), + flattenedActionGroups, + namedXContentRegistry, + fieldMaskingConfig ); - if (oldInstance != null) { - oldInstance.shutdown(); + + if (currentType != targetType) { + PrivilegesEvaluator oldInstance = privilegesEvaluator.getAndSet( + new org.opensearch.security.privileges.PrivilegesEvaluatorImpl( + clusterService, + clusterStateSupplier, + roleMapper, + threadPool, + threadPool.getThreadContext(), + resolver, + auditLog, + settings, + privilegesInterceptor, + indexResolverReplacer, + flattenedActionGroups, + staticActionGroups, + newCompiledRoles, + rawConfiguration.privilegesEvaluatorGlobalSettings, + pluginIdToRolePrivileges + ) + ); + if (oldInstance != null) { + oldInstance.shutdown(); + } + } else { + privilegesEvaluator.get() + .updateConfiguration( + flattenedActionGroups, + newCompiledRoles, + rawConfiguration.privilegesEvaluatorGlobalSettings + ); } - } else { - privilegesEvaluator.get().updateConfiguration(flattenedActionGroups, newCompiledRoles, generalConfiguration); - } - try { - this.dlsFlsProcessedConfig.set( - new DlsFlsProcessedConfig( - newCompiledRoles, - clusterStateSupplier.get().metadata().getIndicesLookup(), - namedXContentRegistry, - settings, - fieldMaskingConfig - ) - ); - } catch (Exception e) { - log.error("Error while updating DlsFlsProcessedConfig", e); + try { + this.dlsFlsProcessedConfig.set( + new DlsFlsProcessedConfig( + newCompiledRoles, + clusterStateSupplier.get().metadata().getIndicesLookup(), + namedXContentRegistry, + settings, + fieldMaskingConfig + ) + ); + } catch (Exception e) { + log.error("Error while updating DlsFlsProcessedConfig", e); + } + + try { + this.tenantPrivileges.set(new TenantPrivileges(rolesConfiguration, tenantConfiguration, flattenedActionGroups)); + } catch (Exception e) { + log.error("Error while updating TenantPrivileges", e); + } + } else { + log.debug("Privileges for PrivilegesEvaluator and DLS/FLS did not change."); } try { @@ -185,12 +212,6 @@ public PrivilegesConfiguration( } catch (Exception e) { log.error("Error while updating DashboardsMultiTenancyConfiguration", e); } - - try { - this.tenantPrivileges.set(new TenantPrivileges(rolesConfiguration, tenantConfiguration, flattenedActionGroups)); - } catch (Exception e) { - log.error("Error while updating TenantPrivileges", e); - } }); } @@ -227,23 +248,6 @@ public PrivilegesEvaluator privilegesEvaluator() { return this.privilegesEvaluator.get(); } - /** - * Returns the current action groups configuration. Important: Do not store the references to the instances returned here; these will change - * after configuration updates. - */ - public FlattenedActionGroups actionGroups() { - return this.actionGroups.get(); - } - - /** - * Returns the current compiled roles. These are built once per configuration update and shared between the - * action privilege layer and the DLS/FLS layer to avoid redundant pattern compilation. - * Important: Do not store the references to the instances returned here; these will change after configuration updates. - */ - public CompiledRoles compiledRoles() { - return this.compiledRoles.get(); - } - /** * Returns the current Dashboards multi tenancy configuration. Important: Do not store the references to the instances returned here; these will change * after configuration updates. @@ -264,4 +268,39 @@ private static FlattenedActionGroups buildStaticActionGroups() { return new FlattenedActionGroups(DynamicConfigFactory.addStatics(SecurityDynamicConfiguration.empty(CType.ACTIONGROUPS))); } + private static class RawConfiguration { + final SecurityDynamicConfiguration actionGroupsConfiguration; + final SecurityDynamicConfiguration rolesConfiguration; + final SecurityDynamicConfiguration tenantConfiguration; + final PrivilegesEvaluator.GlobalDynamicSettings privilegesEvaluatorGlobalSettings; + + RawConfiguration( + SecurityDynamicConfiguration actionGroupsConfiguration, + SecurityDynamicConfiguration rolesConfiguration, + SecurityDynamicConfiguration tenantConfiguration, + PrivilegesEvaluator.GlobalDynamicSettings privilegesEvaluatorGlobalSettings + ) { + this.actionGroupsConfiguration = actionGroupsConfiguration; + this.rolesConfiguration = rolesConfiguration; + this.tenantConfiguration = tenantConfiguration; + this.privilegesEvaluatorGlobalSettings = privilegesEvaluatorGlobalSettings; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof RawConfiguration that)) { + return false; + } + return Objects.equals(actionGroupsConfiguration, that.actionGroupsConfiguration) + && Objects.equals(rolesConfiguration, that.rolesConfiguration) + && Objects.equals(tenantConfiguration, that.tenantConfiguration) + && Objects.equals(privilegesEvaluatorGlobalSettings, that.privilegesEvaluatorGlobalSettings); + } + + @Override + public int hashCode() { + return Objects.hash(actionGroupsConfiguration, rolesConfiguration, tenantConfiguration, privilegesEvaluatorGlobalSettings); + } + } + } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index b16228259f..de0af9ff1e 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -26,6 +26,7 @@ package org.opensearch.security.privileges; +import java.util.Objects; import java.util.function.Supplier; import org.opensearch.OpenSearchSecurityException; @@ -59,7 +60,11 @@ PrivilegesEvaluationContext createContext( boolean isClusterPermission(String action); - void updateConfiguration(FlattenedActionGroups actionGroups, CompiledRoles rolesConfiguration, ConfigV7 generalConfiguration); + void updateConfiguration( + FlattenedActionGroups actionGroups, + CompiledRoles rolesConfiguration, + GlobalDynamicSettings globalDynamicSettings + ); void updateClusterStateMetadata(ClusterService clusterService); @@ -118,7 +123,7 @@ public boolean isClusterPermission(String action) { public void updateConfiguration( FlattenedActionGroups actionGroups, CompiledRoles rolesConfiguration, - ConfigV7 generalConfiguration + GlobalDynamicSettings globalDynamicSettings ) { } @@ -152,4 +157,50 @@ private OpenSearchSecurityException exception() { } }; + /** + * Configuration that is sourced from the "general purpose mixed bag" configuration type called config. + * The purpose of this class is to provide a focused view to the needed settings. + */ + class GlobalDynamicSettings { + final boolean dnfofEnabled; + final boolean dnfofForEmptyResultsEnabled; + final String filteredAliasMode; + + GlobalDynamicSettings(boolean dnfofEnabled, boolean dnfofForEmptyResultsEnabled, String filteredAliasMode) { + this.dnfofEnabled = dnfofEnabled; + this.dnfofForEmptyResultsEnabled = dnfofForEmptyResultsEnabled; + this.filteredAliasMode = filteredAliasMode; + } + + public static GlobalDynamicSettings fromConfigV7(ConfigV7 configV7) { + return new GlobalDynamicSettings(isDnfofEnabled(configV7), isDnfofEmptyEnabled(configV7), getFilteredAliasMode(configV7)); + } + + private static boolean isDnfofEnabled(ConfigV7 generalConfiguration) { + return generalConfiguration.dynamic != null && generalConfiguration.dynamic.do_not_fail_on_forbidden; + } + + private static boolean isDnfofEmptyEnabled(ConfigV7 generalConfiguration) { + return generalConfiguration.dynamic != null && generalConfiguration.dynamic.do_not_fail_on_forbidden_empty; + } + + private static String getFilteredAliasMode(ConfigV7 generalConfiguration) { + return generalConfiguration.dynamic != null ? generalConfiguration.dynamic.filtered_alias_mode : "none"; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof GlobalDynamicSettings that)) { + return false; + } + return dnfofEnabled == that.dnfofEnabled + && dnfofForEmptyResultsEnabled == that.dnfofForEmptyResultsEnabled + && Objects.equals(filteredAliasMode, that.filteredAliasMode); + } + + @Override + public int hashCode() { + return Objects.hash(dnfofEnabled, dnfofForEmptyResultsEnabled, filteredAliasMode); + } + } } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java index b14c5aeff3..42aefc21a4 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java @@ -91,7 +91,6 @@ import org.opensearch.security.resolver.IndexResolverReplacer; import org.opensearch.security.resolver.IndexResolverReplacer.Resolved; import org.opensearch.security.securityconf.FlattenedActionGroups; -import org.opensearch.security.securityconf.impl.v7.ConfigV7; import org.opensearch.security.securityconf.impl.v7.RoleV7; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.WildcardMatcher; @@ -166,7 +165,7 @@ public PrivilegesEvaluatorImpl( FlattenedActionGroups actionGroups, FlattenedActionGroups staticActionGroups, CompiledRoles rolesConfiguration, - ConfigV7 generalConfiguration, + GlobalDynamicSettings globalDynamicSettings, Map pluginIdToRolePrivileges ) { @@ -198,7 +197,7 @@ public PrivilegesEvaluatorImpl( pitPrivilegesEvaluator = new PitPrivilegesEvaluator(); this.pluginIdToActionPrivileges = createActionPrivileges(pluginIdToRolePrivileges, staticActionGroups); - this.updateConfiguration(actionGroups, rolesConfiguration, generalConfiguration); + this.updateConfiguration(actionGroups, rolesConfiguration, globalDynamicSettings); } @Override @@ -207,10 +206,14 @@ public PrivilegesEvaluatorType type() { } @Override - public void updateConfiguration(FlattenedActionGroups flattenedActionGroups, CompiledRoles roles, ConfigV7 generalConfiguration) { - this.dnfofEnabled = isDnfofEnabled(generalConfiguration); - this.dnfofForEmptyResultsEnabled = isDnfofEmptyEnabled(generalConfiguration); - this.filteredAliasMode = getFilteredAliasMode(generalConfiguration); + public void updateConfiguration( + FlattenedActionGroups flattenedActionGroups, + CompiledRoles roles, + GlobalDynamicSettings globalDynamicSettings + ) { + this.dnfofEnabled = globalDynamicSettings.dnfofEnabled; + this.dnfofForEmptyResultsEnabled = globalDynamicSettings.dnfofForEmptyResultsEnabled; + this.filteredAliasMode = globalDynamicSettings.filteredAliasMode; try { RoleBasedActionPrivileges actionPrivileges = new RoleBasedActionPrivileges(roles, settings); @@ -735,15 +738,4 @@ private static ImmutableMap createActionPrivileges( return ImmutableMap.copyOf(result); } - private static boolean isDnfofEnabled(ConfigV7 generalConfiguration) { - return generalConfiguration.dynamic != null && generalConfiguration.dynamic.do_not_fail_on_forbidden; - } - - private static boolean isDnfofEmptyEnabled(ConfigV7 generalConfiguration) { - return generalConfiguration.dynamic != null && generalConfiguration.dynamic.do_not_fail_on_forbidden_empty; - } - - private static String getFilteredAliasMode(ConfigV7 generalConfiguration) { - return generalConfiguration.dynamic != null ? generalConfiguration.dynamic.filtered_alias_mode : "none"; - } } 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 10ea8c9bc7..bea720241e 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java @@ -34,6 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import com.fasterxml.jackson.annotation.JsonAnyGetter; import com.fasterxml.jackson.annotation.JsonAnySetter; @@ -434,4 +435,17 @@ public boolean isEmpty() { public SecurityDynamicConfiguration withStaticConfig() { return DynamicConfigFactory.addStatics(this.clone()); } + + @Override + public boolean equals(Object o) { + if (!(o instanceof SecurityDynamicConfiguration that)) { + return false; + } + return Objects.equals(ctype, that.ctype) && Objects.equals(centries, that.centries); + } + + @Override + public int hashCode() { + return centries.hashCode(); + } } diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/ActionGroupsV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/ActionGroupsV7.java index 0a86f2672e..93696d1682 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/v7/ActionGroupsV7.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/v7/ActionGroupsV7.java @@ -29,6 +29,7 @@ import java.util.Collections; import java.util.List; +import java.util.Objects; import com.fasterxml.jackson.annotation.JsonProperty; @@ -122,4 +123,16 @@ public String toString() { + "]"; } + @Override + public boolean equals(Object o) { + if (!(o instanceof ActionGroupsV7 that)) { + return false; + } + return Objects.equals(allowed_actions, that.allowed_actions) && Objects.equals(type, that.type); + } + + @Override + public int hashCode() { + return Objects.hash(allowed_actions, type); + } } diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/RoleV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/RoleV7.java index 8859b4586e..70c3173e5c 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/v7/RoleV7.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/v7/RoleV7.java @@ -36,6 +36,7 @@ import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; +import java.util.Objects; import com.fasterxml.jackson.annotation.JsonProperty; @@ -102,6 +103,21 @@ public static RoleV7 fromPluginPermissionsFile(URL pluginPermissionsFile) throws return role; } + @Override + public boolean equals(Object o) { + if (!(o instanceof RoleV7 roleV7)) { + return false; + } + return Objects.equals(cluster_permissions, roleV7.cluster_permissions) + && Objects.equals(index_permissions, roleV7.index_permissions) + && Objects.equals(tenant_permissions, roleV7.tenant_permissions); + } + + @Override + public int hashCode() { + return Objects.hash(cluster_permissions, index_permissions, tenant_permissions); + } + public static class Index { private List index_patterns = Collections.emptyList(); @@ -168,6 +184,23 @@ public String toString() { + allowed_actions + "]"; } + + @Override + public boolean equals(Object o) { + if (!(o instanceof Index index)) { + return false; + } + return Objects.equals(index_patterns, index.index_patterns) + && Objects.equals(dls, index.dls) + && Objects.equals(fls, index.fls) + && Objects.equals(masked_fields, index.masked_fields) + && Objects.equals(allowed_actions, index.allowed_actions); + } + + @Override + public int hashCode() { + return Objects.hash(index_patterns, dls, fls, masked_fields, allowed_actions); + } } public static class Tenant { @@ -175,19 +208,6 @@ public static class Tenant { private List tenant_patterns = Collections.emptyList(); private List allowed_actions = Collections.emptyList(); - /*public Index(String pattern, RoleV6.Index v6Index) { - super(); - index_patterns = Collections.singletonList(pattern); - dls = v6Index.get_dls_(); - fls = v6Index.get_fls_(); - masked_fields = v6Index.get_masked_fields_(); - Set tmpActions = new HashSet<>(); - for(Entry> type: v6Index.getTypes().entrySet()) { - tmpActions.addAll(type.getValue()); - } - allowed_actions = new ArrayList<>(tmpActions); - }*/ - public Tenant() { super(); } @@ -213,6 +233,18 @@ public String toString() { return "Tenant [tenant_patterns=" + tenant_patterns + ", allowed_actions=" + allowed_actions + "]"; } + @Override + public boolean equals(Object o) { + if (!(o instanceof Tenant tenant)) { + return false; + } + return Objects.equals(tenant_patterns, tenant.tenant_patterns) && Objects.equals(allowed_actions, tenant.allowed_actions); + } + + @Override + public int hashCode() { + return Objects.hash(tenant_patterns, allowed_actions); + } } public boolean isHidden() { diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/TenantV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/TenantV7.java index 8fe47dcbd7..e3d66e90d5 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/v7/TenantV7.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/v7/TenantV7.java @@ -27,6 +27,8 @@ package org.opensearch.security.securityconf.impl.v7; +import java.util.Objects; + import com.fasterxml.jackson.annotation.JsonProperty; import org.opensearch.security.securityconf.Hideable; @@ -79,4 +81,14 @@ public String toString() { return "TenantV7 [reserved=" + reserved + ", hidden=" + hidden + ", _static=" + _static + ", description=" + description + "]"; } + @Override + public boolean equals(Object o) { + if (!(o instanceof TenantV7 tenantV7)) return false; + return Objects.equals(description, tenantV7.description); + } + + @Override + public int hashCode() { + return Objects.hashCode(description); + } } diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index 51f622c528..b7a63d31ba 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -33,7 +33,6 @@ import org.opensearch.security.securityconf.FlattenedActionGroups; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; -import org.opensearch.security.securityconf.impl.v7.ConfigV7; import org.opensearch.security.securityconf.impl.v7.RoleV7; import org.opensearch.security.user.User; import org.opensearch.tasks.Task; @@ -177,7 +176,7 @@ public boolean isClusterPermission(String action) { public void updateConfiguration( FlattenedActionGroups actionGroups, CompiledRoles rolesConfiguration, - ConfigV7 generalConfiguration + PrivilegesEvaluator.GlobalDynamicSettings generalConfiguration ) { }