From dabff35efaabb8872945183a32c69b357cb91b7d Mon Sep 17 00:00:00 2001 From: Nils Bandener <33570290+nibix@users.noreply.github.com> Date: Tue, 9 Jul 2024 22:20:59 +0200 Subject: [PATCH] Separated DLS/FLS privilege evaluation from action privilege evaluation (#4490) Signed-off-by: Nils Bandener --- .../security/OpenSearchSecurityPlugin.java | 6 +- .../DlsFilterLevelActionHandler.java | 28 +++--- .../configuration/DlsFlsRequestValve.java | 20 +--- .../configuration/DlsFlsValveImpl.java | 41 +++++--- .../security/filter/SecurityFilter.java | 6 +- .../PrivilegesEvaluationContext.java | 97 +++++++++++++++++++ .../privileges/PrivilegesEvaluator.java | 50 +++++----- .../PrivilegesEvaluatorResponse.java | 21 +--- 8 files changed, 176 insertions(+), 93 deletions(-) create mode 100644 src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index c3e00da109..4bee20f5ca 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1118,8 +1118,6 @@ public Collection createComponents( final CompatConfig compatConfig = new CompatConfig(environment, transportPassiveAuthSetting); - // DLS-FLS is enabled if not client and not disabled and not SSL only. - final boolean dlsFlsEnabled = !SSLConfig.isSslOnlyMode(); evaluator = new PrivilegesEvaluator( clusterService, threadPool, @@ -1130,7 +1128,6 @@ public Collection createComponents( privilegesInterceptor, cih, irr, - dlsFlsEnabled, namedXContentRegistry.get() ); @@ -1169,6 +1166,9 @@ public Collection createComponents( // Don't register if advanced modules is disabled in which case auditlog is instance of NullAuditLog dcf.registerDCFListener(auditLog); } + if (dlsFlsValve instanceof DlsFlsValveImpl) { + dcf.registerDCFListener(dlsFlsValve); + } cr.setDynamicConfigFactory(dcf); diff --git a/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java b/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java index 099e27c238..95d07cf0b2 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java @@ -58,6 +58,7 @@ import org.opensearch.search.SearchHit; import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.security.privileges.DocumentAllowList; +import org.opensearch.security.privileges.PrivilegesEvaluationContext; import org.opensearch.security.queries.QueryBuilderTraverser; import org.opensearch.security.resolver.IndexResolverReplacer.Resolved; import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig; @@ -74,11 +75,9 @@ public class DlsFilterLevelActionHandler { ); public static boolean handle( - String action, - ActionRequest request, - ActionListener listener, + PrivilegesEvaluationContext context, EvaluatedDlsFlsConfig evaluatedDlsFlsConfig, - Resolved resolved, + ActionListener listener, Client nodeClient, ClusterService clusterService, IndicesService indicesService, @@ -91,6 +90,9 @@ public static boolean handle( return true; } + String action = context.getAction(); + ActionRequest request = context.getRequest(); + if (action.startsWith("cluster:") || action.startsWith("indices:admin/template/") || action.startsWith("indices:admin/index_template/")) { @@ -112,11 +114,9 @@ public static boolean handle( } return new DlsFilterLevelActionHandler( - action, - request, - listener, + context, evaluatedDlsFlsConfig, - resolved, + listener, nodeClient, clusterService, indicesService, @@ -142,11 +142,9 @@ public static boolean handle( private DocumentAllowList documentAllowlist; DlsFilterLevelActionHandler( - String action, - ActionRequest request, - ActionListener listener, + PrivilegesEvaluationContext context, EvaluatedDlsFlsConfig evaluatedDlsFlsConfig, - Resolved resolved, + ActionListener listener, Client nodeClient, ClusterService clusterService, IndicesService indicesService, @@ -154,11 +152,11 @@ public static boolean handle( DlsQueryParser dlsQueryParser, ThreadContext threadContext ) { - this.action = action; - this.request = request; + this.action = context.getAction(); + this.request = context.getRequest(); this.listener = listener; this.evaluatedDlsFlsConfig = evaluatedDlsFlsConfig; - this.resolved = resolved; + this.resolved = context.getResolvedRequest(); this.nodeClient = nodeClient; this.clusterService = clusterService; this.indicesService = indicesService; diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java b/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java index 1152799bd5..d629cbaff3 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java @@ -26,24 +26,16 @@ package org.opensearch.security.configuration; -import org.opensearch.action.ActionRequest; import org.opensearch.core.action.ActionListener; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.search.internal.SearchContext; import org.opensearch.search.query.QuerySearchResult; -import org.opensearch.security.resolver.IndexResolverReplacer.Resolved; -import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig; +import org.opensearch.security.privileges.PrivilegesEvaluationContext; import org.opensearch.threadpool.ThreadPool; public interface DlsFlsRequestValve { - boolean invoke( - String action, - ActionRequest request, - ActionListener listener, - EvaluatedDlsFlsConfig evaluatedDlsFlsConfig, - Resolved resolved - ); + boolean invoke(PrivilegesEvaluationContext context, ActionListener listener); void handleSearchContext(SearchContext context, ThreadPool threadPool, NamedXContentRegistry namedXContentRegistry); @@ -52,13 +44,7 @@ boolean invoke( public static class NoopDlsFlsRequestValve implements DlsFlsRequestValve { @Override - public boolean invoke( - String action, - ActionRequest request, - ActionListener listener, - EvaluatedDlsFlsConfig evaluatedDlsFlsConfig, - Resolved resolved - ) { + public boolean invoke(PrivilegesEvaluationContext context, ActionListener listener) { return true; } diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 855db9e896..4141a3f8f5 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -72,7 +72,9 @@ import org.opensearch.search.internal.SearchContext; import org.opensearch.search.query.QuerySearchResult; import org.opensearch.security.OpenSearchSecurityPlugin; -import org.opensearch.security.resolver.IndexResolverReplacer.Resolved; +import org.opensearch.security.privileges.PrivilegesEvaluationContext; +import org.opensearch.security.resolver.IndexResolverReplacer; +import org.opensearch.security.securityconf.ConfigModel; import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig; import org.opensearch.security.support.Base64Helper; import org.opensearch.security.support.ConfigConstants; @@ -80,6 +82,8 @@ import org.opensearch.security.support.SecurityUtils; import org.opensearch.threadpool.ThreadPool; +import org.greenrobot.eventbus.Subscribe; + public class DlsFlsValveImpl implements DlsFlsRequestValve { private static final String MAP_EXECUTION_HINT = "map"; @@ -91,6 +95,9 @@ public class DlsFlsValveImpl implements DlsFlsRequestValve { private final Mode mode; private final DlsQueryParser dlsQueryParser; private final IndexNameExpressionResolver resolver; + private final boolean dfmEmptyOverwritesAll; + private final NamedXContentRegistry namedXContentRegistry; + private volatile ConfigModel configModel; public DlsFlsValveImpl( Settings settings, @@ -107,21 +114,29 @@ public DlsFlsValveImpl( this.threadContext = threadContext; this.mode = Mode.get(settings); this.dlsQueryParser = new DlsQueryParser(namedXContentRegistry); + this.dfmEmptyOverwritesAll = settings.getAsBoolean(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false); + this.namedXContentRegistry = namedXContentRegistry; + } + + @Subscribe + public void onConfigModelChanged(ConfigModel configModel) { + this.configModel = configModel; } /** * - * @param request * @param listener * @return false on error */ - public boolean invoke( - String action, - ActionRequest request, - final ActionListener listener, - EvaluatedDlsFlsConfig evaluatedDlsFlsConfig, - final Resolved resolved - ) { + @Override + public boolean invoke(PrivilegesEvaluationContext context, final ActionListener listener) { + + EvaluatedDlsFlsConfig evaluatedDlsFlsConfig = configModel.getSecurityRoles() + .filter(context.getMappedRoles()) + .getDlsFls(context.getUser(), dfmEmptyOverwritesAll, resolver, clusterService, namedXContentRegistry); + + ActionRequest request = context.getRequest(); + IndexResolverReplacer.Resolved resolved = context.getResolvedRequest(); if (log.isDebugEnabled()) { log.debug( @@ -288,7 +303,7 @@ public boolean invoke( return false; } - if (action.contains("plugins/replication")) { + if (context.getAction().contains("plugins/replication")) { listener.onFailure( new OpenSearchSecurityException( "Cross Cluster Replication is not supported when FLS or DLS or Fieldmasking is activated", @@ -324,11 +339,9 @@ public boolean invoke( if (doFilterLevelDls && filteredDlsFlsConfig.hasDls()) { return DlsFilterLevelActionHandler.handle( - action, - request, - listener, + context, evaluatedDlsFlsConfig, - resolved, + listener, nodeClient, clusterService, OpenSearchSecurityPlugin.GuiceHolder.getIndicesService(), diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index b9d4a73967..1116e70845 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -82,6 +82,7 @@ import org.opensearch.security.configuration.CompatConfig; import org.opensearch.security.configuration.DlsFlsRequestValve; import org.opensearch.security.http.XFFResolver; +import org.opensearch.security.privileges.PrivilegesEvaluationContext; import org.opensearch.security.privileges.PrivilegesEvaluator; import org.opensearch.security.privileges.PrivilegesEvaluatorResponse; import org.opensearch.security.resolver.IndexResolverReplacer; @@ -378,7 +379,8 @@ private void ap log.trace("Evaluate permissions for user: {}", user.getName()); } - final PrivilegesEvaluatorResponse pres = eval.evaluate(user, action, request, task, injectedRoles); + PrivilegesEvaluationContext context = eval.createContext(user, action, request, task, injectedRoles); + PrivilegesEvaluatorResponse pres = eval.evaluate(context); if (log.isDebugEnabled()) { log.debug(pres.toString()); @@ -387,7 +389,7 @@ private void ap if (pres.isAllowed()) { auditLog.logGrantedPrivileges(action, request, task); auditLog.logIndexEvent(action, request, task); - if (!dlsFlsValve.invoke(action, request, listener, pres.getEvaluatedDlsFlsConfig(), pres.getResolved())) { + if (!dlsFlsValve.invoke(context, listener)) { return; } final CreateIndexRequestBuilder createIndexRequestBuilder = pres.getCreateIndexRequestBuilder(); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java new file mode 100644 index 0000000000..98ffddb3d3 --- /dev/null +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java @@ -0,0 +1,97 @@ +/* + * 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.privileges; + +import com.google.common.collect.ImmutableSet; + +import org.opensearch.action.ActionRequest; +import org.opensearch.security.resolver.IndexResolverReplacer; +import org.opensearch.security.user.User; +import org.opensearch.tasks.Task; + +/** + * Request-scoped context information for privilege evaluation. + * + * This class carries metadata about the request and provides caching facilities for data which might need to be + * evaluated several times per request. + * + * As this class is request-scoped, it is only used by a single thread. Thus, no thread synchronization mechanisms + * are necessary. + */ +public class PrivilegesEvaluationContext { + private final User user; + private final String action; + private final ActionRequest request; + private IndexResolverReplacer.Resolved resolvedRequest; + private final Task task; + private ImmutableSet mappedRoles; + private final IndexResolverReplacer indexResolverReplacer; + + public PrivilegesEvaluationContext( + User user, + ImmutableSet mappedRoles, + String action, + ActionRequest request, + Task task, + IndexResolverReplacer indexResolverReplacer + ) { + this.user = user; + this.mappedRoles = mappedRoles; + this.action = action; + this.request = request; + this.task = task; + this.indexResolverReplacer = indexResolverReplacer; + } + + public User getUser() { + return user; + } + + public String getAction() { + return action; + } + + public ActionRequest getRequest() { + return request; + } + + public IndexResolverReplacer.Resolved getResolvedRequest() { + IndexResolverReplacer.Resolved result = this.resolvedRequest; + + if (result == null) { + result = indexResolverReplacer.resolveRequest(request); + this.resolvedRequest = result; + } + + return result; + } + + public Task getTask() { + return task; + } + + public ImmutableSet getMappedRoles() { + return mappedRoles; + } + + /** + * Note: Ideally, mappedRoles would be an unmodifiable attribute. PrivilegesEvaluator however contains logic + * related to OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION which first validates roles and afterwards modifies + * them again. Thus, we need to be able to set this attribute. + * + * However, this method should be only used for this one particular phase. Normally, all roles should be determined + * upfront and stay constant during the whole privilege evaluation process. + */ + void setMappedRoles(ImmutableSet mappedRoles) { + this.mappedRoles = mappedRoles; + } + +} diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index b2ae499879..f7f8fb66a9 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -140,8 +140,6 @@ public class PrivilegesEvaluator { private final ProtectedIndexAccessEvaluator protectedIndexAccessEvaluator; private final TermsAggregationEvaluator termsAggregationEvaluator; private final PitPrivilegesEvaluator pitPrivilegesEvaluator; - private final boolean dlsFlsEnabled; - private final boolean dfmEmptyOverwritesAll; private DynamicConfigModel dcm; private final NamedXContentRegistry namedXContentRegistry; @@ -155,7 +153,6 @@ public PrivilegesEvaluator( final PrivilegesInterceptor privilegesInterceptor, final ClusterInfoHolder clusterInfoHolder, final IndexResolverReplacer irr, - boolean dlsFlsEnabled, NamedXContentRegistry namedXContentRegistry ) { @@ -180,8 +177,6 @@ public PrivilegesEvaluator( termsAggregationEvaluator = new TermsAggregationEvaluator(); pitPrivilegesEvaluator = new PitPrivilegesEvaluator(); this.namedXContentRegistry = namedXContentRegistry; - this.dlsFlsEnabled = dlsFlsEnabled; - this.dfmEmptyOverwritesAll = settings.getAsBoolean(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false); } @Subscribe @@ -226,18 +221,35 @@ private void setUserInfoInThreadContext(User user) { } } - public PrivilegesEvaluatorResponse evaluate( - final User user, + public PrivilegesEvaluationContext createContext( + User user, String action0, - final ActionRequest request, + ActionRequest request, Task task, - final Set injectedRoles + Set injectedRoles ) { + if (!isInitialized()) { + throw new OpenSearchSecurityException("OpenSearch Security is not initialized."); + } + + TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); + ImmutableSet mappedRoles = ImmutableSet.copyOf((injectedRoles == null) ? mapRoles(user, caller) : injectedRoles); + + return new PrivilegesEvaluationContext(user, mappedRoles, action0, request, task, irr); + } + + public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) { if (!isInitialized()) { throw new OpenSearchSecurityException("OpenSearch Security is not initialized."); } + String action0 = context.getAction(); + ImmutableSet mappedRoles = context.getMappedRoles(); + User user = context.getUser(); + ActionRequest request = context.getRequest(); + Task task = context.getTask(); + if (action0.startsWith("internal:indices/admin/upgrade")) { action0 = "indices:admin/upgrade"; } @@ -252,8 +264,6 @@ public PrivilegesEvaluatorResponse evaluate( final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); - final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); - Set mappedRoles = (injectedRoles == null) ? mapRoles(user, caller) : injectedRoles; final String injectedRolesValidationString = threadContext.getTransient( ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION ); @@ -266,6 +276,7 @@ public PrivilegesEvaluatorResponse evaluate( return presponse; } mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet); + context.setMappedRoles(mappedRoles); } presponse.resolvedSecurityRoles.addAll(mappedRoles); final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); @@ -305,8 +316,7 @@ public PrivilegesEvaluatorResponse evaluate( return presponse; } - final Resolved requestedResolved = irr.resolveRequest(request); - presponse.resolved = requestedResolved; + final Resolved requestedResolved = context.getResolvedRequest(); if (isDebugEnabled) { log.debug("RequestedResolved : {}", requestedResolved); @@ -349,14 +359,6 @@ public PrivilegesEvaluatorResponse evaluate( log.trace("dnfof enabled? {}", dnfofEnabled); } - presponse.evaluatedDlsFlsConfig = getSecurityRoles(mappedRoles).getDlsFls( - user, - dfmEmptyOverwritesAll, - resolver, - clusterService, - namedXContentRegistry - ); - final boolean serviceAccountUser = user.isServiceAccount(); if (isClusterPerm(action0)) { if (serviceAccountUser) { @@ -435,7 +437,11 @@ public PrivilegesEvaluatorResponse evaluate( final String[] allIndexPermsRequiredA = allIndexPermsRequired.toArray(new String[0]); if (isDebugEnabled) { - log.debug("Requested {} from {}", allIndexPermsRequired, caller); + log.debug( + "Requested {} from {}", + allIndexPermsRequired, + threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS) + ); } presponse.missingPrivileges.clear(); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index eb082a4a9f..915514264c 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -30,23 +30,15 @@ import java.util.Set; import org.opensearch.action.admin.indices.create.CreateIndexRequestBuilder; -import org.opensearch.security.resolver.IndexResolverReplacer.Resolved; -import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig; public class PrivilegesEvaluatorResponse { boolean allowed = false; Set missingPrivileges = new HashSet(); Set missingSecurityRoles = new HashSet<>(); Set resolvedSecurityRoles = new HashSet<>(); - EvaluatedDlsFlsConfig evaluatedDlsFlsConfig; PrivilegesEvaluatorResponseState state = PrivilegesEvaluatorResponseState.PENDING; - Resolved resolved; CreateIndexRequestBuilder createIndexRequestBuilder; - public Resolved getResolved() { - return resolved; - } - public boolean isAllowed() { return allowed; } @@ -63,10 +55,6 @@ public Set getResolvedSecurityRoles() { return new HashSet<>(resolvedSecurityRoles); } - public EvaluatedDlsFlsConfig getEvaluatedDlsFlsConfig() { - return evaluatedDlsFlsConfig; - } - public CreateIndexRequestBuilder getCreateIndexRequestBuilder() { return createIndexRequestBuilder; } @@ -91,18 +79,11 @@ public boolean isPending() { @Override public String toString() { - return "PrivEvalResponse [allowed=" - + allowed - + ", missingPrivileges=" - + missingPrivileges - + ", evaluatedDlsFlsConfig=" - + evaluatedDlsFlsConfig - + "]"; + return "PrivEvalResponse [allowed=" + allowed + ", missingPrivileges=" + missingPrivileges + "]"; } public static enum PrivilegesEvaluatorResponseState { PENDING, COMPLETE; } - }