Skip to content

Commit 08d17ee

Browse files
github-actions[bot]github-actions[bot]
committed
Separated DLS/FLS privilege evaluation from action privilege evaluation (#4490)
Signed-off-by: Nils Bandener <[email protected]> (cherry picked from commit dabff35) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 6b31b04 commit 08d17ee

8 files changed

+176
-93
lines changed

‎src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -1115,8 +1115,6 @@ public Collection<Object> createComponents(
11151115

11161116
final CompatConfig compatConfig = new CompatConfig(environment, transportPassiveAuthSetting);
11171117

1118-
// DLS-FLS is enabled if not client and not disabled and not SSL only.
1119-
final boolean dlsFlsEnabled = !SSLConfig.isSslOnlyMode();
11201118
evaluator = new PrivilegesEvaluator(
11211119
clusterService,
11221120
threadPool,
@@ -1127,7 +1125,6 @@ public Collection<Object> createComponents(
11271125
privilegesInterceptor,
11281126
cih,
11291127
irr,
1130-
dlsFlsEnabled,
11311128
namedXContentRegistry.get()
11321129
);
11331130

@@ -1167,6 +1164,9 @@ public Collection<Object> createComponents(
11671164
// Don't register if advanced modules is disabled in which case auditlog is instance of NullAuditLog
11681165
dcf.registerDCFListener(auditLog);
11691166
}
1167+
if (dlsFlsValve instanceof DlsFlsValveImpl) {
1168+
dcf.registerDCFListener(dlsFlsValve);
1169+
}
11701170

11711171
cr.setDynamicConfigFactory(dcf);
11721172

‎src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

+13-15
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import org.opensearch.search.SearchHit;
5959
import org.opensearch.search.builder.SearchSourceBuilder;
6060
import org.opensearch.security.privileges.DocumentAllowList;
61+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
6162
import org.opensearch.security.queries.QueryBuilderTraverser;
6263
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
6364
import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig;
@@ -74,11 +75,9 @@ public class DlsFilterLevelActionHandler {
7475
);
7576

7677
public static boolean handle(
77-
String action,
78-
ActionRequest request,
79-
ActionListener<?> listener,
78+
PrivilegesEvaluationContext context,
8079
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig,
81-
Resolved resolved,
80+
ActionListener<?> listener,
8281
Client nodeClient,
8382
ClusterService clusterService,
8483
IndicesService indicesService,
@@ -91,6 +90,9 @@ public static boolean handle(
9190
return true;
9291
}
9392

93+
String action = context.getAction();
94+
ActionRequest request = context.getRequest();
95+
9496
if (action.startsWith("cluster:")
9597
|| action.startsWith("indices:admin/template/")
9698
|| action.startsWith("indices:admin/index_template/")) {
@@ -112,11 +114,9 @@ public static boolean handle(
112114
}
113115

114116
return new DlsFilterLevelActionHandler(
115-
action,
116-
request,
117-
listener,
117+
context,
118118
evaluatedDlsFlsConfig,
119-
resolved,
119+
listener,
120120
nodeClient,
121121
clusterService,
122122
indicesService,
@@ -142,23 +142,21 @@ public static boolean handle(
142142
private DocumentAllowList documentAllowlist;
143143

144144
DlsFilterLevelActionHandler(
145-
String action,
146-
ActionRequest request,
147-
ActionListener<?> listener,
145+
PrivilegesEvaluationContext context,
148146
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig,
149-
Resolved resolved,
147+
ActionListener<?> listener,
150148
Client nodeClient,
151149
ClusterService clusterService,
152150
IndicesService indicesService,
153151
IndexNameExpressionResolver resolver,
154152
DlsQueryParser dlsQueryParser,
155153
ThreadContext threadContext
156154
) {
157-
this.action = action;
158-
this.request = request;
155+
this.action = context.getAction();
156+
this.request = context.getRequest();
159157
this.listener = listener;
160158
this.evaluatedDlsFlsConfig = evaluatedDlsFlsConfig;
161-
this.resolved = resolved;
159+
this.resolved = context.getResolvedRequest();
162160
this.nodeClient = nodeClient;
163161
this.clusterService = clusterService;
164162
this.indicesService = indicesService;

‎src/main/java/org/opensearch/security/configuration/DlsFlsRequestValve.java

+3-17
Original file line numberDiff line numberDiff line change
@@ -26,24 +26,16 @@
2626

2727
package org.opensearch.security.configuration;
2828

29-
import org.opensearch.action.ActionRequest;
3029
import org.opensearch.core.action.ActionListener;
3130
import org.opensearch.core.xcontent.NamedXContentRegistry;
3231
import org.opensearch.search.internal.SearchContext;
3332
import org.opensearch.search.query.QuerySearchResult;
34-
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
35-
import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig;
33+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
3634
import org.opensearch.threadpool.ThreadPool;
3735

3836
public interface DlsFlsRequestValve {
3937

40-
boolean invoke(
41-
String action,
42-
ActionRequest request,
43-
ActionListener<?> listener,
44-
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig,
45-
Resolved resolved
46-
);
38+
boolean invoke(PrivilegesEvaluationContext context, ActionListener<?> listener);
4739

4840
void handleSearchContext(SearchContext context, ThreadPool threadPool, NamedXContentRegistry namedXContentRegistry);
4941

@@ -52,13 +44,7 @@ boolean invoke(
5244
public static class NoopDlsFlsRequestValve implements DlsFlsRequestValve {
5345

5446
@Override
55-
public boolean invoke(
56-
String action,
57-
ActionRequest request,
58-
ActionListener<?> listener,
59-
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig,
60-
Resolved resolved
61-
) {
47+
public boolean invoke(PrivilegesEvaluationContext context, ActionListener<?> listener) {
6248
return true;
6349
}
6450

‎src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java

+27-14
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,18 @@
7272
import org.opensearch.search.internal.SearchContext;
7373
import org.opensearch.search.query.QuerySearchResult;
7474
import org.opensearch.security.OpenSearchSecurityPlugin;
75-
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
75+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
76+
import org.opensearch.security.resolver.IndexResolverReplacer;
77+
import org.opensearch.security.securityconf.ConfigModel;
7678
import org.opensearch.security.securityconf.EvaluatedDlsFlsConfig;
7779
import org.opensearch.security.support.Base64Helper;
7880
import org.opensearch.security.support.ConfigConstants;
7981
import org.opensearch.security.support.HeaderHelper;
8082
import org.opensearch.security.support.SecurityUtils;
8183
import org.opensearch.threadpool.ThreadPool;
8284

85+
import org.greenrobot.eventbus.Subscribe;
86+
8387
public class DlsFlsValveImpl implements DlsFlsRequestValve {
8488

8589
private static final String MAP_EXECUTION_HINT = "map";
@@ -91,6 +95,9 @@ public class DlsFlsValveImpl implements DlsFlsRequestValve {
9195
private final Mode mode;
9296
private final DlsQueryParser dlsQueryParser;
9397
private final IndexNameExpressionResolver resolver;
98+
private final boolean dfmEmptyOverwritesAll;
99+
private final NamedXContentRegistry namedXContentRegistry;
100+
private volatile ConfigModel configModel;
94101

95102
public DlsFlsValveImpl(
96103
Settings settings,
@@ -107,21 +114,29 @@ public DlsFlsValveImpl(
107114
this.threadContext = threadContext;
108115
this.mode = Mode.get(settings);
109116
this.dlsQueryParser = new DlsQueryParser(namedXContentRegistry);
117+
this.dfmEmptyOverwritesAll = settings.getAsBoolean(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false);
118+
this.namedXContentRegistry = namedXContentRegistry;
119+
}
120+
121+
@Subscribe
122+
public void onConfigModelChanged(ConfigModel configModel) {
123+
this.configModel = configModel;
110124
}
111125

112126
/**
113127
*
114-
* @param request
115128
* @param listener
116129
* @return false on error
117130
*/
118-
public boolean invoke(
119-
String action,
120-
ActionRequest request,
121-
final ActionListener<?> listener,
122-
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig,
123-
final Resolved resolved
124-
) {
131+
@Override
132+
public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<?> listener) {
133+
134+
EvaluatedDlsFlsConfig evaluatedDlsFlsConfig = configModel.getSecurityRoles()
135+
.filter(context.getMappedRoles())
136+
.getDlsFls(context.getUser(), dfmEmptyOverwritesAll, resolver, clusterService, namedXContentRegistry);
137+
138+
ActionRequest request = context.getRequest();
139+
IndexResolverReplacer.Resolved resolved = context.getResolvedRequest();
125140

126141
if (log.isDebugEnabled()) {
127142
log.debug(
@@ -288,7 +303,7 @@ public boolean invoke(
288303
return false;
289304
}
290305

291-
if (action.contains("plugins/replication")) {
306+
if (context.getAction().contains("plugins/replication")) {
292307
listener.onFailure(
293308
new OpenSearchSecurityException(
294309
"Cross Cluster Replication is not supported when FLS or DLS or Fieldmasking is activated",
@@ -324,11 +339,9 @@ public boolean invoke(
324339

325340
if (doFilterLevelDls && filteredDlsFlsConfig.hasDls()) {
326341
return DlsFilterLevelActionHandler.handle(
327-
action,
328-
request,
329-
listener,
342+
context,
330343
evaluatedDlsFlsConfig,
331-
resolved,
344+
listener,
332345
nodeClient,
333346
clusterService,
334347
OpenSearchSecurityPlugin.GuiceHolder.getIndicesService(),

‎src/main/java/org/opensearch/security/filter/SecurityFilter.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
import org.opensearch.security.configuration.CompatConfig;
8383
import org.opensearch.security.configuration.DlsFlsRequestValve;
8484
import org.opensearch.security.http.XFFResolver;
85+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
8586
import org.opensearch.security.privileges.PrivilegesEvaluator;
8687
import org.opensearch.security.privileges.PrivilegesEvaluatorResponse;
8788
import org.opensearch.security.resolver.IndexResolverReplacer;
@@ -378,7 +379,8 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
378379
log.trace("Evaluate permissions for user: {}", user.getName());
379380
}
380381

381-
final PrivilegesEvaluatorResponse pres = eval.evaluate(user, action, request, task, injectedRoles);
382+
PrivilegesEvaluationContext context = eval.createContext(user, action, request, task, injectedRoles);
383+
PrivilegesEvaluatorResponse pres = eval.evaluate(context);
382384

383385
if (log.isDebugEnabled()) {
384386
log.debug(pres.toString());
@@ -387,7 +389,7 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
387389
if (pres.isAllowed()) {
388390
auditLog.logGrantedPrivileges(action, request, task);
389391
auditLog.logIndexEvent(action, request, task);
390-
if (!dlsFlsValve.invoke(action, request, listener, pres.getEvaluatedDlsFlsConfig(), pres.getResolved())) {
392+
if (!dlsFlsValve.invoke(context, listener)) {
391393
return;
392394
}
393395
final CreateIndexRequestBuilder createIndexRequestBuilder = pres.getCreateIndexRequestBuilder();

‎src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java

+97
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
package org.opensearch.security.privileges;
12+
13+
import com.google.common.collect.ImmutableSet;
14+
15+
import org.opensearch.action.ActionRequest;
16+
import org.opensearch.security.resolver.IndexResolverReplacer;
17+
import org.opensearch.security.user.User;
18+
import org.opensearch.tasks.Task;
19+
20+
/**
21+
* Request-scoped context information for privilege evaluation.
22+
*
23+
* This class carries metadata about the request and provides caching facilities for data which might need to be
24+
* evaluated several times per request.
25+
*
26+
* As this class is request-scoped, it is only used by a single thread. Thus, no thread synchronization mechanisms
27+
* are necessary.
28+
*/
29+
public class PrivilegesEvaluationContext {
30+
private final User user;
31+
private final String action;
32+
private final ActionRequest request;
33+
private IndexResolverReplacer.Resolved resolvedRequest;
34+
private final Task task;
35+
private ImmutableSet<String> mappedRoles;
36+
private final IndexResolverReplacer indexResolverReplacer;
37+
38+
public PrivilegesEvaluationContext(
39+
User user,
40+
ImmutableSet<String> mappedRoles,
41+
String action,
42+
ActionRequest request,
43+
Task task,
44+
IndexResolverReplacer indexResolverReplacer
45+
) {
46+
this.user = user;
47+
this.mappedRoles = mappedRoles;
48+
this.action = action;
49+
this.request = request;
50+
this.task = task;
51+
this.indexResolverReplacer = indexResolverReplacer;
52+
}
53+
54+
public User getUser() {
55+
return user;
56+
}
57+
58+
public String getAction() {
59+
return action;
60+
}
61+
62+
public ActionRequest getRequest() {
63+
return request;
64+
}
65+
66+
public IndexResolverReplacer.Resolved getResolvedRequest() {
67+
IndexResolverReplacer.Resolved result = this.resolvedRequest;
68+
69+
if (result == null) {
70+
result = indexResolverReplacer.resolveRequest(request);
71+
this.resolvedRequest = result;
72+
}
73+
74+
return result;
75+
}
76+
77+
public Task getTask() {
78+
return task;
79+
}
80+
81+
public ImmutableSet<String> getMappedRoles() {
82+
return mappedRoles;
83+
}
84+
85+
/**
86+
* Note: Ideally, mappedRoles would be an unmodifiable attribute. PrivilegesEvaluator however contains logic
87+
* related to OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION which first validates roles and afterwards modifies
88+
* them again. Thus, we need to be able to set this attribute.
89+
*
90+
* However, this method should be only used for this one particular phase. Normally, all roles should be determined
91+
* upfront and stay constant during the whole privilege evaluation process.
92+
*/
93+
void setMappedRoles(ImmutableSet<String> mappedRoles) {
94+
this.mappedRoles = mappedRoles;
95+
}
96+
97+
}

0 commit comments

Comments
 (0)