From 9f2492124c3bc2921142025c5e0fd1d835ec2b64 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 26 Jun 2024 09:52:03 +0200 Subject: [PATCH 1/7] Separated DLS/FLS privilege evaluation from action privilege evaluation Signed-off-by: Nils Bandener --- .../security/OpenSearchSecurityPlugin.java | 3 - .../configuration/DlsFlsRequestValve.java | 19 +--- .../configuration/DlsFlsValveImpl.java | 33 +++++-- .../security/filter/SecurityFilter.java | 13 ++- .../PrivilegesEvaluationContext.java | 92 +++++++++++++++++++ .../privileges/PrivilegesEvaluator.java | 80 ++++++++-------- .../PrivilegesEvaluatorResponse.java | 50 +++++++--- 7 files changed, 207 insertions(+), 83 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..dadabfb65a 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() ); diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java b/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java index 1152799bd5..e70f9c8024 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java @@ -31,19 +31,12 @@ 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(String action, ActionRequest request, ActionListener listener, PrivilegesEvaluationContext context); void handleSearchContext(SearchContext context, ThreadPool threadPool, NamedXContentRegistry namedXContentRegistry); @@ -52,13 +45,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(String action, ActionRequest request, ActionListener listener, PrivilegesEvaluationContext context) { 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..66a66abb89 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,6 +114,13 @@ 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; } /** @@ -115,13 +129,14 @@ public DlsFlsValveImpl( * @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(String action, ActionRequest request, final ActionListener listener, PrivilegesEvaluationContext context) { + + EvaluatedDlsFlsConfig evaluatedDlsFlsConfig = configModel.getSecurityRoles() + .filter(context.getMappedRoles()) + .getDlsFls(context.getUser(), dfmEmptyOverwritesAll, resolver, clusterService, namedXContentRegistry); + + IndexResolverReplacer.Resolved resolved = context.getResolvedRequest(); if (log.isDebugEnabled()) { log.debug( @@ -129,7 +144,7 @@ public boolean invoke( + request + "\nevaluatedDlsFlsConfig: " + evaluatedDlsFlsConfig - + "\nresolved: " + + "\ncontext: " + resolved + "\nmode: " + mode diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index b9d4a73967..509a407196 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,15 @@ private void ap log.trace("Evaluate permissions for user: {}", user.getName()); } - final PrivilegesEvaluatorResponse pres = eval.evaluate(user, action, request, task, injectedRoles); + PrivilegesEvaluationContext context = null; + PrivilegesEvaluatorResponse pres; + + try { + context = eval.createContext(user, action, request, task, injectedRoles); + pres = eval.evaluate(context); + } catch (PrivilegesEvaluatorResponse.NotAllowedException e) { + pres = e.getResponse(); + } if (log.isDebugEnabled()) { log.debug(pres.toString()); @@ -387,7 +396,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(action, request, listener, context)) { 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..b54426385a --- /dev/null +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java @@ -0,0 +1,92 @@ +/* + * 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 java.util.HashMap; +import java.util.Map; +import java.util.function.Supplier; + +import com.google.common.collect.ImmutableSet; + +import org.opensearch.action.ActionRequest; +import org.opensearch.cluster.ClusterState; +import org.opensearch.cluster.metadata.IndexNameExpressionResolver; +import org.opensearch.security.resolver.IndexResolverReplacer; +import org.opensearch.security.support.WildcardMatcher; +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 final 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; + } + +} diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index b2ae499879..d8985f9f90 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,45 @@ private void setUserInfoInThreadContext(User user) { } } - public PrivilegesEvaluatorResponse evaluate( - final User user, - String action0, - final ActionRequest request, - Task task, - final Set injectedRoles - ) { + public PrivilegesEvaluationContext createContext(User user, String action0, ActionRequest request, Task task, Set injectedRoles) + throws PrivilegesEvaluatorResponse.NotAllowedException { + 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); + final String injectedRolesValidationString = threadContext.getTransient( + ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION + ); + if (injectedRolesValidationString != null) { + HashSet injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(","))); + if (!mappedRoles.containsAll(injectedRolesValidationSet)) { + PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + presponse.allowed = false; + presponse.missingSecurityRoles.addAll(injectedRolesValidationSet); + presponse.reason("Injected roles validation failed"); + log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user); + throw new PrivilegesEvaluatorResponse.NotAllowedException(presponse); + } + mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet); + } + + 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"; } @@ -250,23 +272,8 @@ public PrivilegesEvaluatorResponse evaluate( action0 = PutMappingAction.NAME; } - final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + 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 - ); - if (injectedRolesValidationString != null) { - HashSet injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(","))); - if (!mappedRoles.containsAll(injectedRolesValidationSet)) { - presponse.allowed = false; - presponse.missingSecurityRoles.addAll(injectedRolesValidationSet); - log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user); - return presponse; - } - mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet); - } presponse.resolvedSecurityRoles.addAll(mappedRoles); final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); @@ -305,8 +312,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 +355,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 +433,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..30cf704221 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -30,23 +30,19 @@ 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; + private String reason; - public Resolved getResolved() { - return resolved; - } - + /** + * Returns true if the request can be fully allowed. See also isAllowedForSpecificIndices(). + */ public boolean isAllowed() { return allowed; } @@ -63,10 +59,6 @@ public Set getResolvedSecurityRoles() { return new HashSet<>(resolvedSecurityRoles); } - public EvaluatedDlsFlsConfig getEvaluatedDlsFlsConfig() { - return evaluatedDlsFlsConfig; - } - public CreateIndexRequestBuilder getCreateIndexRequestBuilder() { return createIndexRequestBuilder; } @@ -89,14 +81,21 @@ public boolean isPending() { return this.state == PrivilegesEvaluatorResponseState.PENDING; } + public String getReason() { + return this.reason; + } + + public PrivilegesEvaluatorResponse reason(String reason) { + this.reason = reason; + return this; + } + @Override public String toString() { return "PrivEvalResponse [allowed=" + allowed + ", missingPrivileges=" + missingPrivileges - + ", evaluatedDlsFlsConfig=" - + evaluatedDlsFlsConfig + "]"; } @@ -105,4 +104,27 @@ public static enum PrivilegesEvaluatorResponseState { COMPLETE; } + public static class NotAllowedException extends Exception { + private final PrivilegesEvaluatorResponse response; + + public NotAllowedException(PrivilegesEvaluatorResponse response) { + super(response.reason); + this.response = response; + } + + public NotAllowedException(PrivilegesEvaluatorResponse response, String message) { + super(message); + this.response = response; + } + + public NotAllowedException(PrivilegesEvaluatorResponse response, String message, Throwable cause) { + super(message, cause); + this.response = response; + } + + public PrivilegesEvaluatorResponse getResponse() { + return response; + } + } + } From 8fb019bfc129700f5bdace5fd578398687aaf143 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 26 Jun 2024 11:19:31 +0200 Subject: [PATCH 2/7] Fixes Signed-off-by: Nils Bandener --- .../security/OpenSearchSecurityPlugin.java | 3 +++ .../PrivilegesEvaluatorResponse.java | 22 ++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index dadabfb65a..4bee20f5ca 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1166,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/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index 30cf704221..63368ea201 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -40,9 +40,6 @@ public class PrivilegesEvaluatorResponse { CreateIndexRequestBuilder createIndexRequestBuilder; private String reason; - /** - * Returns true if the request can be fully allowed. See also isAllowedForSpecificIndices(). - */ public boolean isAllowed() { return allowed; } @@ -104,22 +101,21 @@ public static enum PrivilegesEvaluatorResponseState { COMPLETE; } + /** + * This exception can be used to indicate that a method denies a user access to an OpenSearch action. + * + * Note: As exceptions take their performance toll, please use this exception only when there is + * no other way. Prefer to use PrivilegesEvaluatorResponse directly as a return value. + */ public static class NotAllowedException extends Exception { private final PrivilegesEvaluatorResponse response; public NotAllowedException(PrivilegesEvaluatorResponse response) { super(response.reason); this.response = response; - } - - public NotAllowedException(PrivilegesEvaluatorResponse response, String message) { - super(message); - this.response = response; - } - - public NotAllowedException(PrivilegesEvaluatorResponse response, String message, Throwable cause) { - super(message, cause); - this.response = response; + if (response.allowed) { + throw new IllegalArgumentException("Only possible for PrivilegesEvaluatorResponse with allowed=false"); + } } public PrivilegesEvaluatorResponse getResponse() { From 648dc7bb3cd2f2e4a60df5a15e02d97e7b48af06 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 26 Jun 2024 11:33:07 +0200 Subject: [PATCH 3/7] Applied spotless Signed-off-by: Nils Bandener --- .../security/privileges/PrivilegesEvaluationContext.java | 7 ------- .../security/privileges/PrivilegesEvaluatorResponse.java | 6 +----- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java index b54426385a..bee5ad90e8 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java @@ -10,17 +10,10 @@ */ package org.opensearch.security.privileges; -import java.util.HashMap; -import java.util.Map; -import java.util.function.Supplier; - import com.google.common.collect.ImmutableSet; import org.opensearch.action.ActionRequest; -import org.opensearch.cluster.ClusterState; -import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.security.resolver.IndexResolverReplacer; -import org.opensearch.security.support.WildcardMatcher; import org.opensearch.security.user.User; import org.opensearch.tasks.Task; diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index 63368ea201..4c259871a5 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -89,11 +89,7 @@ public PrivilegesEvaluatorResponse reason(String reason) { @Override public String toString() { - return "PrivEvalResponse [allowed=" - + allowed - + ", missingPrivileges=" - + missingPrivileges - + "]"; + return "PrivEvalResponse [allowed=" + allowed + ", missingPrivileges=" + missingPrivileges + "]"; } public static enum PrivilegesEvaluatorResponseState { From 7d70bc6a0e48054d70835dfdfdc21c3453a07d01 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 26 Jun 2024 11:38:35 +0200 Subject: [PATCH 4/7] Fixed logging Signed-off-by: Nils Bandener --- .../org/opensearch/security/configuration/DlsFlsValveImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 66a66abb89..4985293aef 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -144,7 +144,7 @@ public boolean invoke(String action, ActionRequest request, final ActionListener + request + "\nevaluatedDlsFlsConfig: " + evaluatedDlsFlsConfig - + "\ncontext: " + + "\nresolved: " + resolved + "\nmode: " + mode From 16cb84037b5697bf7f201f7ce2af51863e0a6471 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Thu, 4 Jul 2024 10:24:27 +0200 Subject: [PATCH 5/7] Made PrivilegesEvaluationContext.mappedRoles non-final, moved OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION handling back to evaluate() Signed-off-by: Nils Bandener --- .../security/filter/SecurityFilter.java | 11 +---- .../PrivilegesEvaluationContext.java | 14 ++++++- .../privileges/PrivilegesEvaluator.java | 40 ++++++++++--------- .../PrivilegesEvaluatorResponse.java | 23 ----------- 4 files changed, 37 insertions(+), 51 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index 509a407196..a2c16eb549 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -379,15 +379,8 @@ private void ap log.trace("Evaluate permissions for user: {}", user.getName()); } - PrivilegesEvaluationContext context = null; - PrivilegesEvaluatorResponse pres; - - try { - context = eval.createContext(user, action, request, task, injectedRoles); - pres = eval.evaluate(context); - } catch (PrivilegesEvaluatorResponse.NotAllowedException e) { - pres = e.getResponse(); - } + PrivilegesEvaluationContext context = eval.createContext(user, action, request, task, injectedRoles); + PrivilegesEvaluatorResponse pres = eval.evaluate(context); if (log.isDebugEnabled()) { log.debug(pres.toString()); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java index bee5ad90e8..98ffddb3d3 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java @@ -32,7 +32,7 @@ public class PrivilegesEvaluationContext { private final ActionRequest request; private IndexResolverReplacer.Resolved resolvedRequest; private final Task task; - private final ImmutableSet mappedRoles; + private ImmutableSet mappedRoles; private final IndexResolverReplacer indexResolverReplacer; public PrivilegesEvaluationContext( @@ -82,4 +82,16 @@ 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 d8985f9f90..f7f8fb66a9 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -221,29 +221,19 @@ private void setUserInfoInThreadContext(User user) { } } - public PrivilegesEvaluationContext createContext(User user, String action0, ActionRequest request, Task task, Set injectedRoles) - throws PrivilegesEvaluatorResponse.NotAllowedException { + public PrivilegesEvaluationContext createContext( + User user, + String action0, + ActionRequest request, + Task task, + 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); - final String injectedRolesValidationString = threadContext.getTransient( - ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION - ); - if (injectedRolesValidationString != null) { - HashSet injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(","))); - if (!mappedRoles.containsAll(injectedRolesValidationSet)) { - PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); - presponse.allowed = false; - presponse.missingSecurityRoles.addAll(injectedRolesValidationSet); - presponse.reason("Injected roles validation failed"); - log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user); - throw new PrivilegesEvaluatorResponse.NotAllowedException(presponse); - } - mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet); - } return new PrivilegesEvaluationContext(user, mappedRoles, action0, request, task, irr); } @@ -272,8 +262,22 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) action0 = PutMappingAction.NAME; } - PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + final String injectedRolesValidationString = threadContext.getTransient( + ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION + ); + if (injectedRolesValidationString != null) { + HashSet injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(","))); + if (!mappedRoles.containsAll(injectedRolesValidationSet)) { + presponse.allowed = false; + presponse.missingSecurityRoles.addAll(injectedRolesValidationSet); + log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user); + return presponse; + } + mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet); + context.setMappedRoles(mappedRoles); + } presponse.resolvedSecurityRoles.addAll(mappedRoles); final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index 4c259871a5..1da88f62b7 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -96,27 +96,4 @@ public static enum PrivilegesEvaluatorResponseState { PENDING, COMPLETE; } - - /** - * This exception can be used to indicate that a method denies a user access to an OpenSearch action. - * - * Note: As exceptions take their performance toll, please use this exception only when there is - * no other way. Prefer to use PrivilegesEvaluatorResponse directly as a return value. - */ - public static class NotAllowedException extends Exception { - private final PrivilegesEvaluatorResponse response; - - public NotAllowedException(PrivilegesEvaluatorResponse response) { - super(response.reason); - this.response = response; - if (response.allowed) { - throw new IllegalArgumentException("Only possible for PrivilegesEvaluatorResponse with allowed=false"); - } - } - - public PrivilegesEvaluatorResponse getResponse() { - return response; - } - } - } From a3a257c1b8029e5458439977dfb0d4f89be3f898 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Thu, 4 Jul 2024 10:30:24 +0200 Subject: [PATCH 6/7] Removed reason attr again Signed-off-by: Nils Bandener --- .../privileges/PrivilegesEvaluatorResponse.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index 1da88f62b7..915514264c 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -38,7 +38,6 @@ public class PrivilegesEvaluatorResponse { Set resolvedSecurityRoles = new HashSet<>(); PrivilegesEvaluatorResponseState state = PrivilegesEvaluatorResponseState.PENDING; CreateIndexRequestBuilder createIndexRequestBuilder; - private String reason; public boolean isAllowed() { return allowed; @@ -78,15 +77,6 @@ public boolean isPending() { return this.state == PrivilegesEvaluatorResponseState.PENDING; } - public String getReason() { - return this.reason; - } - - public PrivilegesEvaluatorResponse reason(String reason) { - this.reason = reason; - return this; - } - @Override public String toString() { return "PrivEvalResponse [allowed=" + allowed + ", missingPrivileges=" + missingPrivileges + "]"; From 97d8d5420c59d714e5806b494d55ed53c9c41472 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Thu, 4 Jul 2024 10:48:39 +0200 Subject: [PATCH 7/7] Also changed DlsFlsValue to use context param Signed-off-by: Nils Bandener --- .../DlsFilterLevelActionHandler.java | 28 +++++++++---------- .../configuration/DlsFlsRequestValve.java | 5 ++-- .../configuration/DlsFlsValveImpl.java | 12 ++++---- .../security/filter/SecurityFilter.java | 2 +- 4 files changed, 21 insertions(+), 26 deletions(-) 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 e70f9c8024..d629cbaff3 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java @@ -26,7 +26,6 @@ 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; @@ -36,7 +35,7 @@ public interface DlsFlsRequestValve { - boolean invoke(String action, ActionRequest request, ActionListener listener, PrivilegesEvaluationContext context); + boolean invoke(PrivilegesEvaluationContext context, ActionListener listener); void handleSearchContext(SearchContext context, ThreadPool threadPool, NamedXContentRegistry namedXContentRegistry); @@ -45,7 +44,7 @@ public interface DlsFlsRequestValve { public static class NoopDlsFlsRequestValve implements DlsFlsRequestValve { @Override - public boolean invoke(String action, ActionRequest request, ActionListener listener, PrivilegesEvaluationContext context) { + 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 4985293aef..4141a3f8f5 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -125,17 +125,17 @@ public void onConfigModelChanged(ConfigModel configModel) { /** * - * @param request * @param listener * @return false on error */ @Override - public boolean invoke(String action, ActionRequest request, final ActionListener listener, PrivilegesEvaluationContext context) { + 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()) { @@ -303,7 +303,7 @@ public boolean invoke(String action, ActionRequest request, final ActionListener 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", @@ -339,11 +339,9 @@ public boolean invoke(String action, ActionRequest request, final ActionListener 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 a2c16eb549..1116e70845 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -389,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, context)) { + if (!dlsFlsValve.invoke(context, listener)) { return; } final CreateIndexRequestBuilder createIndexRequestBuilder = pres.getCreateIndexRequestBuilder();