Skip to content

Commit 30fcc0f

Browse files
nibixnibix
committed
Adapted RestLayerPrivilegesEvaluator
Signed-off-by: Nils Bandener <[email protected]>
1 parent 0500d12 commit 30fcc0f

10 files changed

+186
-180
lines changed

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -1142,7 +1142,7 @@ public Collection<Object> createComponents(
11421142
principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass);
11431143
}
11441144

1145-
restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool);
1145+
restLayerEvaluator = new RestLayerPrivilegesEvaluator(evaluator);
11461146

11471147
securityRestHandler = new SecurityRestFilter(
11481148
backendRegistry,
@@ -1161,7 +1161,6 @@ public Collection<Object> createComponents(
11611161
dcf.registerDCFListener(xffResolver);
11621162
dcf.registerDCFListener(evaluator);
11631163
dcf.registerDCFListener(restLayerEvaluator);
1164-
dcf.registerDCFListener(securityRestHandler);
11651164
dcf.registerDCFListener(tokenManager);
11661165
if (!(auditLog instanceof NullAuditLog)) {
11671166
// Don't register if advanced modules is disabled in which case auditlog is instance of NullAuditLog

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

+8-11
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,14 @@
2727
package org.opensearch.security.filter;
2828

2929
import java.nio.file.Path;
30+
import java.util.Collections;
3031
import java.util.List;
3132
import java.util.Optional;
3233
import java.util.Set;
3334
import java.util.regex.Pattern;
3435
import javax.net.ssl.SSLPeerUnverifiedException;
3536

37+
import com.google.common.collect.ImmutableSet;
3638
import org.apache.http.HttpStatus;
3739
import org.apache.logging.log4j.LogManager;
3840
import org.apache.logging.log4j.Logger;
@@ -225,17 +227,12 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User
225227
if (routeSupportsRestAuthorization) {
226228
PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse();
227229
NamedRoute route = ((NamedRoute) handler.get());
228-
// if actionNames are present evaluate those first
229-
Set<String> actionNames = route.actionNames();
230-
if (actionNames != null && !actionNames.isEmpty()) {
231-
pres = evaluator.evaluate(user, actionNames);
232-
}
233-
234-
// now if pres.allowed is still false check for the NamedRoute name as a permission
235-
if (!pres.isAllowed()) {
236-
String action = route.name();
237-
pres = evaluator.evaluate(user, Set.of(action));
238-
}
230+
// Check both route.actionNames() and route.name(). The presence of either is sufficient.
231+
Set<String> actionNames = ImmutableSet.<String>builder()
232+
.addAll(route.actionNames() != null ? route.actionNames() : Collections.emptySet())
233+
.add(route.name())
234+
.build();
235+
pres = evaluator.evaluate(user, route.name(), actionNames);
239236

240237
if (log.isDebugEnabled()) {
241238
log.debug(pres.toString());

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

+44
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,10 @@ public PrivilegesEvaluatorResponse hasClusterPrivilege(PrivilegesEvaluationConte
111111
return cluster.providesPrivilege(context, action, context.getMappedRoles());
112112
}
113113

114+
public PrivilegesEvaluatorResponse hasAnyClusterPrivilege(PrivilegesEvaluationContext context, Set<String> actions) {
115+
return cluster.providesAnyPrivilege(context, actions, context.getMappedRoles());
116+
}
117+
114118
public PrivilegesEvaluatorResponse hasIndexPrivilege(
115119
PrivilegesEvaluationContext context,
116120
Set<String> actions,
@@ -311,6 +315,46 @@ PrivilegesEvaluatorResponse providesPrivilege(PrivilegesEvaluationContext contex
311315

312316
return PrivilegesEvaluatorResponse.insufficient(action, context);
313317
}
318+
319+
/**
320+
* Checks whether this instance provides privileges for the combination of any of the provided actions and the
321+
* provided roles. Returns a PrivilegesEvaluatorResponse with allowed=true if privileges are available.
322+
* Otherwise, allowed will be false and missingPrivileges will contain the name of the given action.
323+
*/
324+
PrivilegesEvaluatorResponse providesAnyPrivilege(PrivilegesEvaluationContext context, Set<String> actions, Set<String> roles) {
325+
// 1: Check roles with wildcards
326+
if (CollectionUtils.containsAny(roles, this.rolesWithWildcardPermissions)) {
327+
return PrivilegesEvaluatorResponse.ok();
328+
}
329+
330+
// 2: Check well-known actions - this should cover most cases
331+
for (String action : actions) {
332+
ImmutableSet<String> rolesWithPrivileges = this.actionToRoles.get(action);
333+
334+
if (rolesWithPrivileges != null && CollectionUtils.containsAny(roles, rolesWithPrivileges)) {
335+
return PrivilegesEvaluatorResponse.ok();
336+
}
337+
}
338+
339+
// 3: Only if everything else fails: Check the matchers in case we have a non-well-known action
340+
for (String action : actions) {
341+
if (!this.wellKnownClusterActions.contains(action)) {
342+
for (String role : roles) {
343+
WildcardMatcher matcher = this.rolesToActionMatcher.get(role);
344+
345+
if (matcher != null && matcher.test(action)) {
346+
return PrivilegesEvaluatorResponse.ok();
347+
}
348+
}
349+
}
350+
}
351+
352+
if (actions.size() == 1) {
353+
return PrivilegesEvaluatorResponse.insufficient(actions.iterator().next(), context);
354+
} else {
355+
return PrivilegesEvaluatorResponse.insufficient("any of " + actions, context);
356+
}
357+
}
314358
}
315359

316360
/**

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public class PrivilegesEvaluationContext {
5151
*/
5252
private final Map<String, WildcardMatcher> renderedPatternTemplateCache = new HashMap<>();
5353

54-
public PrivilegesEvaluationContext(
54+
PrivilegesEvaluationContext(
5555
User user,
5656
ImmutableSet<String> mappedRoles,
5757
String action,

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

+27-15
Original file line numberDiff line numberDiff line change
@@ -188,18 +188,22 @@ public PrivilegesEvaluator(
188188
pitPrivilegesEvaluator = new PitPrivilegesEvaluator();
189189
this.namedXContentRegistry = namedXContentRegistry;
190190

191-
configurationRepository.subscribeOnChange(configMap -> {
192-
try {
193-
// TODO: It is not nice that we cannot use a concrete generic parameter for SecurityDynamicConfiguration
194-
// TODO: At the moment, the implementations only support the V7 config objects
195-
SecurityDynamicConfiguration<?> actionGroupsConfiguration = configurationRepository.getConfiguration(CType.ACTIONGROUPS);
196-
SecurityDynamicConfiguration<?> rolesConfiguration = configurationRepository.getConfiguration(CType.ROLES);
197-
198-
this.updateConfiguration(actionGroupsConfiguration, rolesConfiguration);
199-
} catch (Exception e) {
200-
log.error("Error while updating ActionPrivileges object with {}", configMap, e);
201-
}
202-
});
191+
if (configurationRepository != null) {
192+
configurationRepository.subscribeOnChange(configMap -> {
193+
try {
194+
// TODO: It is not nice that we cannot use a concrete generic parameter for SecurityDynamicConfiguration
195+
// TODO: At the moment, the implementations only support the V7 config objects
196+
SecurityDynamicConfiguration<?> actionGroupsConfiguration = configurationRepository.getConfiguration(
197+
CType.ACTIONGROUPS
198+
);
199+
SecurityDynamicConfiguration<?> rolesConfiguration = configurationRepository.getConfiguration(CType.ROLES);
200+
201+
this.updateConfiguration(actionGroupsConfiguration, rolesConfiguration);
202+
} catch (Exception e) {
203+
log.error("Error while updating ActionPrivileges object with {}", configMap, e);
204+
}
205+
});
206+
}
203207

204208
if (clusterService != null) {
205209
clusterService.addListener(new ClusterStateListener() {
@@ -226,11 +230,11 @@ void updateConfiguration(
226230
if (rolesConfiguration != null) {
227231
@SuppressWarnings("unchecked")
228232
SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsWithStatics = actionGroupsConfiguration != null
229-
? (SecurityDynamicConfiguration<ActionGroupsV7>) DynamicConfigFactory.addStatics(actionGroupsConfiguration.deepClone())
233+
? (SecurityDynamicConfiguration<ActionGroupsV7>) DynamicConfigFactory.addStatics(actionGroupsConfiguration.clone())
230234
: (SecurityDynamicConfiguration<ActionGroupsV7>) DynamicConfigFactory.addStatics(SecurityDynamicConfiguration.empty());
231235
FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsWithStatics);
232236
ActionPrivileges actionPrivileges = new ActionPrivileges(
233-
DynamicConfigFactory.addStatics(rolesConfiguration.deepClone()),
237+
DynamicConfigFactory.addStatics(rolesConfiguration.clone()),
234238
flattenedActionGroups,
235239
() -> clusterStateSupplier.get().metadata().getIndicesLookup()
236240
);
@@ -249,6 +253,10 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
249253
this.dcm = dcm;
250254
}
251255

256+
ActionPrivileges getActionPrivileges() {
257+
return this.actionPrivileges;
258+
}
259+
252260
public SecurityRoles getSecurityRoles(Set<String> roles) {
253261
return configModel.getSecurityRoles().filter(roles);
254262
}
@@ -264,7 +272,7 @@ private boolean hasRestAdminPermissions(final Set<String> roles, String permissi
264272
}
265273

266274
public boolean isInitialized() {
267-
return configModel != null && configModel.getSecurityRoles() != null && dcm != null;
275+
return configModel != null && dcm != null && actionPrivileges != null;
268276
}
269277

270278
private void setUserInfoInThreadContext(User user) {
@@ -281,6 +289,10 @@ private void setUserInfoInThreadContext(User user) {
281289
}
282290
}
283291

292+
public PrivilegesEvaluationContext createContext(User user, String action) {
293+
return createContext(user, action, null, null, null);
294+
}
295+
284296
public PrivilegesEvaluationContext createContext(
285297
User user,
286298
String action0,

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

+18-65
Original file line numberDiff line numberDiff line change
@@ -16,85 +16,38 @@
1616
import org.apache.logging.log4j.LogManager;
1717
import org.apache.logging.log4j.Logger;
1818

19-
import org.opensearch.OpenSearchSecurityException;
20-
import org.opensearch.cluster.service.ClusterService;
21-
import org.opensearch.common.util.concurrent.ThreadContext;
22-
import org.opensearch.core.common.transport.TransportAddress;
23-
import org.opensearch.security.securityconf.ConfigModel;
24-
import org.opensearch.security.securityconf.SecurityRoles;
25-
import org.opensearch.security.support.ConfigConstants;
2619
import org.opensearch.security.user.User;
27-
import org.opensearch.threadpool.ThreadPool;
28-
29-
import org.greenrobot.eventbus.Subscribe;
3020

3121
public class RestLayerPrivilegesEvaluator {
3222
protected final Logger log = LogManager.getLogger(this.getClass());
33-
private final ClusterService clusterService;
34-
private ThreadContext threadContext;
35-
private ConfigModel configModel;
36-
37-
public RestLayerPrivilegesEvaluator(final ClusterService clusterService, final ThreadPool threadPool) {
38-
this.clusterService = clusterService;
39-
this.threadContext = threadPool.getThreadContext();
40-
}
41-
42-
@Subscribe
43-
public void onConfigModelChanged(final ConfigModel configModel) {
44-
this.configModel = configModel;
45-
}
46-
47-
SecurityRoles getSecurityRoles(final Set<String> roles) {
48-
return configModel.getSecurityRoles().filter(roles);
49-
}
23+
private final PrivilegesEvaluator privilegesEvaluator;
5024

51-
boolean isInitialized() {
52-
return configModel != null && configModel.getSecurityRoles() != null;
25+
public RestLayerPrivilegesEvaluator(PrivilegesEvaluator privilegesEvaluator) {
26+
this.privilegesEvaluator = privilegesEvaluator;
5327
}
5428

55-
public PrivilegesEvaluatorResponse evaluate(final User user, final Set<String> actions) {
56-
if (!isInitialized()) {
57-
throw new OpenSearchSecurityException("OpenSearch Security is not initialized.");
58-
}
59-
60-
final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
61-
62-
final Set<String> mappedRoles = mapRoles(user, caller);
63-
64-
final SecurityRoles securityRoles = getSecurityRoles(mappedRoles);
29+
public PrivilegesEvaluatorResponse evaluate(final User user, final String routeName, final Set<String> actions) {
30+
PrivilegesEvaluationContext context = privilegesEvaluator.createContext(user, routeName);
6531

6632
final boolean isDebugEnabled = log.isDebugEnabled();
6733
if (isDebugEnabled) {
68-
log.debug("Evaluate permissions for {} on {}", user, clusterService.localNode().getName());
34+
log.debug("Evaluate permissions for {}", user);
6935
log.debug("Action: {}", actions);
70-
log.debug("Mapped roles: {}", mappedRoles.toString());
36+
log.debug("Mapped roles: {}", context.getMappedRoles().toString());
7137
}
7238

73-
for (final String action : actions) {
74-
if (!securityRoles.impliesClusterPermissionPermission(action)) {
75-
// TODO This will exhibit a weird behaviour when a REST action specifies two permissions, and
76-
// if the user has no permissions for the first one, but has permissions for the second one:
77-
// First, the information "No permission match" will be logged, but then the action will be
78-
// allowed nevertheless.
79-
log.info(
80-
"No permission match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
81-
user,
82-
action,
83-
securityRoles.getRoleNames(),
84-
action
85-
);
86-
} else {
87-
if (isDebugEnabled) {
88-
log.debug("Allowed because we have permissions for {}", actions);
89-
}
90-
return PrivilegesEvaluatorResponse.ok();
91-
}
92-
}
39+
PrivilegesEvaluatorResponse result = privilegesEvaluator.getActionPrivileges().hasAnyClusterPrivilege(context, actions);
9340

94-
return PrivilegesEvaluatorResponse.insufficient(actions).resolvedSecurityRoles(mappedRoles);
95-
}
41+
if (!result.allowed) {
42+
log.info(
43+
"No permission match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
44+
user,
45+
routeName,
46+
context.getMappedRoles(),
47+
result.getMissingPrivileges()
48+
);
49+
}
9650

97-
Set<String> mapRoles(final User user, final TransportAddress caller) {
98-
return this.configModel.mapSecurityRoles(user, caller);
51+
return result;
9952
}
10053
}

‎src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java

+13
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ public static <T> SecurityDynamicConfiguration<T> fromYaml(String yaml, CType ct
169169
DefaultObjectMapper.getTypeFactory().constructParametricType(SecurityDynamicConfiguration.class, implementationClass)
170170
);
171171
result.ctype = ctype;
172+
result.version = 2;
172173
return result;
173174
}
174175

@@ -327,6 +328,18 @@ public Class<?> getImplementingClass() {
327328
return getCType() == null ? null : getCType().getImplementationClass().get(getVersion());
328329
}
329330

331+
@JsonIgnore
332+
public SecurityDynamicConfiguration<T> clone() {
333+
SecurityDynamicConfiguration<T> result = new SecurityDynamicConfiguration<T>();
334+
result.version = this.version;
335+
result.ctype = this.ctype;
336+
result.primaryTerm = this.primaryTerm;
337+
result.seqNo = this.seqNo;
338+
result._meta = this._meta;
339+
result.centries.putAll(this.centries);
340+
return result;
341+
}
342+
330343
@JsonIgnore
331344
public SecurityDynamicConfiguration<T> deepClone() {
332345
try {

‎src/test/java/org/opensearch/security/privileges/ActionPrivilegesTest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -571,8 +571,8 @@ static PrivilegesEvaluationContext ctx(String... roles) {
571571
null,
572572
null,
573573
null,
574-
new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)),
575-
null
574+
new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)),
575+
null
576576
);
577577
}
578578
}

0 commit comments

Comments
 (0)