Skip to content

Commit

Permalink
Simpler SecurityRoles interface
Browse files Browse the repository at this point in the history 
Signed-off-by: Peter Nied <[email protected]>
  • Loading branch information
@peternied @DarshitChanpura
peternied authored and DarshitChanpura committed Sep 5, 2023
1 parent 72021f7 commit 526a824
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 358 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ public PrivilegesEvaluatorResponse evaluate(
}

// Security index access
if (securityIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, securityRoles).isComplete()) {
if (securityIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, securityRoles, user, resolver, clusterService).isComplete()) {
return presponse;
}

Expand Down
63 changes: 23 additions & 40 deletions src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@
import org.opensearch.action.ActionRequest;
import org.opensearch.action.RealtimeRequest;
import org.opensearch.action.search.SearchRequest;
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.settings.Settings;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.resolver.IndexResolverReplacer;
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
import org.opensearch.security.securityconf.IndexPattern;
import org.opensearch.security.securityconf.SecurityRoles;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.support.WildcardMatcher;
import org.opensearch.security.user.User;
import org.opensearch.tasks.Task;

import java.util.ArrayList;
Expand Down Expand Up @@ -123,55 +125,34 @@ public PrivilegesEvaluatorResponse evaluate(
final String action,
final Resolved requestedResolved,
final PrivilegesEvaluatorResponse presponse,
final SecurityRoles securityRoles
final SecurityRoles securityRoles,
final User user,
final IndexNameExpressionResolver resolver,
final ClusterService clusterService
) {
final boolean isDebugEnabled = log.isDebugEnabled();

evaluateSystemIndicesAccess(action, requestedResolved, request, task, presponse, isDebugEnabled, securityRoles);
evaluateSystemIndicesAccess(action, requestedResolved, request, task, presponse, securityRoles, user, resolver, clusterService);

if (requestedResolved.isLocalAll()
|| requestedResolved.getAllIndices().contains(securityIndex)
|| requestContainsAnySystemIndices(requestedResolved)) {

if (request instanceof SearchRequest) {
((SearchRequest) request).requestCache(Boolean.FALSE);
if (isDebugEnabled) {
if (log.isDebugEnabled()) {
log.debug("Disable search request cache for this request");
}
}

if (request instanceof RealtimeRequest) {
((RealtimeRequest) request).realtime(Boolean.FALSE);
if (isDebugEnabled) {
if (log.isDebugEnabled()) {
log.debug("Disable realtime for this request");
}
}
}
return presponse;
}

/**
* Checks whether user has `system:admin/system_index` explicitly specified
* as an allowed action under any of its mapped roles
* @param securityRoles roles in which the permission needs to be checked
* @return true if user does not have permission, false otherwise
*/
private boolean isSystemIndexAccessProhibitedForUser(SecurityRoles securityRoles) {
// Excluding `*` allowed action as it shouldn't grant system index privilege
Set<WildcardMatcher> userPermissions = securityRoles.getRoles()
.stream()
.flatMap(role -> role.getIpatterns().stream())
.map(IndexPattern::getNonWildCardPerms)
.collect(Collectors.toSet());

for (WildcardMatcher userPermission : userPermissions) {
if (userPermission.matchAny(ConfigConstants.SYSTEM_INDEX_PERMISSION)) {
return false;
}
}
return true;
}

/**
* Checks if request is for any system index
* @param requestedResolved request which contains indices to be matched against system indices
Expand Down Expand Up @@ -238,13 +219,15 @@ private boolean isActionAllowed(String action) {
* @param securityRoles user's roles which will be used for access evaluation
*/
private void evaluateSystemIndicesAccess(
String action,
Resolved requestedResolved,
ActionRequest request,
Task task,
PrivilegesEvaluatorResponse presponse,
Boolean isDebugEnabled,
SecurityRoles securityRoles
final String action,
final Resolved requestedResolved,
final ActionRequest request,
final Task task,
final PrivilegesEvaluatorResponse presponse,
SecurityRoles securityRoles,
final User user,
final IndexNameExpressionResolver resolver,
final ClusterService clusterService
) {
// Perform access check is system index permissions are enabled
boolean containsSystemIndex = requestContainsAnySystemIndices(requestedResolved);
Expand All @@ -264,7 +247,7 @@ private void evaluateSystemIndicesAccess(
presponse.allowed = false;
presponse.markComplete();
return;
} else if (containsSystemIndex && isSystemIndexAccessProhibitedForUser(securityRoles)) {
} else if (containsSystemIndex && !securityRoles.hasExplicitIndexPermission(requestedResolved, user, new String[] {ConfigConstants.SYSTEM_INDEX_PERMISSION}, resolver, clusterService)) {
auditLog.logSecurityIndexAttempt(request, action, task);
if (log.isInfoEnabled()) {
log.info(
Expand All @@ -284,7 +267,7 @@ private void evaluateSystemIndicesAccess(
if (requestedResolved.isLocalAll()) {
if (filterSecurityIndex) {
irr.replace(request, false, "*", "-" + securityIndex);
if (isDebugEnabled) {
if (log.isDebugEnabled()) {
log.debug(
"Filtered '{}' from {}, resulting list with *,-{} is {}",
securityIndex,
Expand All @@ -307,15 +290,15 @@ private void evaluateSystemIndicesAccess(
Set<String> allWithoutSecurity = new HashSet<>(requestedResolved.getAllIndices());
allWithoutSecurity.remove(securityIndex);
if (allWithoutSecurity.isEmpty()) {
if (isDebugEnabled) {
if (log.isDebugEnabled()) {
log.debug("Filtered '{}' but resulting list is empty", securityIndex);
}
presponse.allowed = false;
presponse.markComplete();
return;
}
irr.replace(request, false, allWithoutSecurity.toArray(new String[0]));
if (isDebugEnabled) {
if (log.isDebugEnabled()) {
log.debug("Filtered '{}', resulting list is {}", securityIndex, allWithoutSecurity);
}
} else {
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,11 @@ public EvaluatedDlsFlsConfig getDlsFls(
return new EvaluatedDlsFlsConfig(dlsQueries, flsFields, maskedFieldsMap);
}

public boolean hasExplicitIndexPermission(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs) {
// TODO: Handle this scenario in V6 config
return false;
}

// opensearchDashboards special only, terms eval
public Set<String> getAllPermittedIndicesForDashboards(
Resolved resolved,
Expand Down
38 changes: 33 additions & 5 deletions src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -463,7 +464,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
) {
Set<String> retVal = new HashSet<>();
for (SecurityRole sr : roles) {
retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs));
retVal.addAll(sr.getAllResolvedPermittedIndices(Resolved._LOCAL_ALL, user, actions, resolver, cs, Function.identity()));
retVal.addAll(resolved.getRemoteIndices());
}
return Collections.unmodifiableSet(retVal);
Expand All @@ -473,7 +474,7 @@ public Set<String> getAllPermittedIndicesForDashboards(
public Set<String> reduce(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs) {
Set<String> retVal = new HashSet<>();
for (SecurityRole sr : roles) {
retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs));
retVal.addAll(sr.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, Function.identity()));
}
if (log.isDebugEnabled()) {
log.debug("Reduced requested resolved indices {} to permitted indices {}.", resolved, retVal.toString());
Expand All @@ -499,11 +500,37 @@ public boolean impliesClusterPermissionPermission(String action) {
@Override
public boolean hasExplicitClusterPermissionPermission(String action) {
return roles.stream()
.map(r -> r.clusterPerms == WildcardMatcher.ANY ? WildcardMatcher.NONE : r.clusterPerms)
.map(r -> matchExplicitly(r.clusterPerms))
.filter(m -> m.test(action))
.count() > 0;
}

private static WildcardMatcher matchExplicitly(final WildcardMatcher matcher) {
return matcher == WildcardMatcher.ANY ? WildcardMatcher.NONE : matcher;
}

@Override
public boolean hasExplicitIndexPermission(final Resolved resolved, final User user, final String[] actions, final IndexNameExpressionResolver resolver, final ClusterService cs) {

final Set<String> indicesForRequest = new HashSet<>(resolved.getAllIndicesResolved(cs, resolver));
if (indicesForRequest.isEmpty()) {
// If no indices could be found on the request there is no way to check for the explicit permissions
return false;
}

final Set<String> explicitlyAllowedIndices = roles.stream()
.map(role -> role.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, SecurityRoles::matchExplicitly))
.flatMap(s -> s.stream())
.collect(Collectors.toSet());

if (log.isDebugEnabled()) {
log.debug("ExplicitIndexPermission check indices for request {}, explicitly allowed indices {}", indicesForRequest.toString(), explicitlyAllowedIndices.toString());
}

indicesForRequest.removeAll(explicitlyAllowedIndices);
return indicesForRequest.isEmpty();
}

// rolespan
public boolean impliesTypePermGlobal(
Resolved resolved,
Expand Down Expand Up @@ -578,13 +605,14 @@ private Set<String> getAllResolvedPermittedIndices(
User user,
String[] actions,
IndexNameExpressionResolver resolver,
ClusterService cs
ClusterService cs,
Function<WildcardMatcher, WildcardMatcher> matcherModification
) {

final Set<String> retVal = new HashSet<>();
for (IndexPattern p : ipatterns) {
// what if we cannot resolve one (for create purposes)
final boolean patternMatch = p.getPerms().matchAll(actions);
final boolean patternMatch = matcherModification.apply(p.getPerms()).matchAll(actions);

// final Set<TypePerm> tperms = p.getTypePerms();
// for (TypePerm tp : tperms) {
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/org/opensearch/security/securityconf/SecurityRoles.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ public interface SecurityRoles {

boolean hasExplicitClusterPermissionPermission(String action);

/**
* Determines if the actions are explicitly granted for indices
* @return if all indices in the request have an explicit grant for all actions
*/
boolean hasExplicitIndexPermission(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs);

Set<String> getRoleNames();

Set<String> reduce(
Expand Down Expand Up @@ -84,5 +90,4 @@ Set<String> getAllPermittedIndicesForDashboards(
);

SecurityRoles filter(Set<String> roles);

}
Loading

0 comments on commit 526a824

Please sign in to comment.