Skip to content

Commit

Permalink
DLS Fixes
Browse files Browse the repository at this point in the history 
Signed-off-by: Nils Bandener <[email protected]>
  • Loading branch information
@nibix
nibix committed Sep 12, 2024
1 parent 797352c commit a11bff0
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 75 deletions.
21 changes: 8 additions & 13 deletions src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.opensearch.OpenSearchException;
import org.opensearch.OpenSearchSecurityException;
import org.opensearch.SpecialPermission;
import org.opensearch.Version;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.DocWriteRequest;
import org.opensearch.action.RealtimeRequest;
Expand Down Expand Up @@ -108,6 +109,7 @@ public class DlsFlsValveImpl implements DlsFlsRequestValve {
private final FieldMasking.Config fieldMaskingConfig;
private final Settings settings;


public DlsFlsValveImpl(
Settings settings,
Client nodeClient,
Expand Down Expand Up @@ -147,7 +149,6 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<
DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
ActionRequest request = context.getRequest();
IndexResolverReplacer.Resolved resolved = context.getResolvedRequest();
boolean legacyHeadersRequired = true; // TODO implement way to determine whether legacy headers are required

try {
boolean hasDlsRestrictions = !config.getDocumentPrivileges().isUnrestricted(context, resolved);
Expand Down Expand Up @@ -200,7 +201,7 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<
}
}

if (legacyHeadersRequired) {
if (legacyHeadersRequired()) {
Set<String> indices = clusterService.state().metadata().indices().keySet();

if (!doFilterLevelDls) {
Expand Down Expand Up @@ -291,17 +292,6 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<
return false;
}

if (request instanceof BulkRequest) {
for (DocWriteRequest<?> inner : ((BulkRequest) request).requests()) {
if (inner instanceof UpdateRequest) {
listener.onFailure(
new OpenSearchSecurityException("Update is not supported when FLS or DLS or Fieldmasking is activated")
);
return false;
}
}
}

if (request instanceof BulkShardRequest) {
for (BulkItemRequest inner : ((BulkShardRequest) request).items()) {
if (inner.request() instanceof UpdateRequest) {
Expand Down Expand Up @@ -522,6 +512,11 @@ private static List<StringTerms.Bucket> mergeBuckets(
return buckets;
}

private boolean legacyHeadersRequired() {
// TODO this needs to be adapted if backported
return !clusterService.state().nodes().getMinNodeVersion().onOrAfter(Version.V_3_0_0);
}

private void setDlsHeaders(IndexToRuleMap<DlsRestriction> dlsRestrictionMap, ActionRequest request) {
if (!dlsRestrictionMap.getIndexMap().isEmpty()) {
Map<String, Set<String>> dlsQueriesByIndex = new HashMap<>();
Expand Down
96 changes: 34 additions & 62 deletions src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,23 +141,10 @@ public boolean isUnrestricted(PrivilegesEvaluationContext context, IndexResolver
// early and return true.

for (String index : resolved.getAllIndicesResolved(context.getClusterStateSupplier(), context.getIndexNameExpressionResolver())) {
IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);

if (indexAbstraction == null) {
// We have got a request for an index that does not exist.
// For non-existing indices, it is safe to assume that no documents can be accessed.

if (log.isDebugEnabled()) {
log.debug("ResolvedIndices {} contain non-existing indices. Assuming full document restriction.", resolved);
}

return false;
}

if (this.dfmEmptyOverridesAll) {
// We assume that we have a restriction unless there are roles without restriction.
// Thus, we only have to check the roles without restriction.
if (!this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
if (!this.hasUnrestrictedRulesExplicit(context, statefulRules, index)) {
return false;
}
} else {
Expand All @@ -166,10 +153,10 @@ public boolean isUnrestricted(PrivilegesEvaluationContext context, IndexResolver
// we check for the presence of unrestricted roles. If there are not any matching roles,
// we also assume full restrictions.

if (this.hasRestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
if (this.hasRestrictedRulesExplicit(context, statefulRules, index)) {
return false;
} else if (!CollectionUtils.containsAny(this.staticIndexRules.rolesWithIndexWildcardWithoutRule, context.getMappedRoles())
&& !this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
&& !this.hasUnrestrictedRulesExplicit(context, statefulRules, index)) {
return false;
}
}
Expand All @@ -192,27 +179,20 @@ public boolean isUnrestricted(PrivilegesEvaluationContext context, String index)
return true;
}

IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);
if (indexAbstraction == null) {
// We have got a request for an index that does not exist.
// For non-existing indices, it is safe to assume that no documents can be accessed.
return false;
}

if (this.dfmEmptyOverridesAll) {
// We assume that we have a restriction unless there are roles without restriction.
// Thus, we only have to check the roles without restriction.
return this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction);
return this.hasUnrestrictedRulesExplicit(context, statefulRules, index);
} else {
// if dfmEmptyOverwritesAll == false, we prefer restricted roles over unrestricted ones.
// Thus, we first check for restricted roles. Only if there are not any restricted roles,
// we check for the presence of unrestricted roles. If there are not any matching roles,
// we also assume full restrictions.

if (this.hasRestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
if (this.hasRestrictedRulesExplicit(context, statefulRules, index)) {
return false;
} else {
return this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction);
return this.hasUnrestrictedRulesExplicit(context, statefulRules, index);
}
}
}
Expand All @@ -224,11 +204,9 @@ public boolean isUnrestricted(PrivilegesEvaluationContext context, String index)
private boolean hasUnrestrictedRulesExplicit(
PrivilegesEvaluationContext context,
StatefulRules<SingleRule> statefulRules,
IndexAbstraction indexAbstraction
String index
) throws PrivilegesEvaluationException {

String index = indexAbstraction.getName();

if (statefulRules != null && statefulRules.covers(index)) {
Set<String> roleWithoutRule = statefulRules.indexToRoleWithoutRule.get(index);

Expand All @@ -245,9 +223,12 @@ private boolean hasUnrestrictedRulesExplicit(
return true;
}

for (IndexAbstraction parent : getParents(indexAbstraction, context.getIndicesLookup())) {
if (hasUnrestrictedRulesExplicit(context, statefulRules, parent)) {
return true;
IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);
if (indexAbstraction != null) {
for (String parent : getParents(indexAbstraction)) {
if (hasRestrictedRulesExplicit(context, statefulRules, parent)) {
return true;
}
}
}

Expand All @@ -262,11 +243,9 @@ private boolean hasUnrestrictedRulesExplicit(
private boolean hasRestrictedRulesExplicit(
PrivilegesEvaluationContext context,
StatefulRules<SingleRule> statefulRules,
IndexAbstraction indexAbstraction
String index
) throws PrivilegesEvaluationException {

String index = indexAbstraction.getName();

if (statefulRules != null && statefulRules.covers(index)) {
Map<String, SingleRule> roleWithRule = statefulRules.indexToRoleToRule.get(index);

Expand All @@ -283,9 +262,12 @@ private boolean hasRestrictedRulesExplicit(
return true;
}

for (IndexAbstraction parent : getParents(indexAbstraction, context.getIndicesLookup())) {
if (hasRestrictedRulesExplicit(context, statefulRules, parent)) {
return true;
IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);
if (indexAbstraction != null) {
for (String parent : getParents(indexAbstraction)) {
if (hasRestrictedRulesExplicit(context, statefulRules, parent)) {
return true;
}
}
}

Expand All @@ -306,30 +288,27 @@ protected JoinedRule getRestrictionImpl(PrivilegesEvaluationContext context, Str
return unrestricted();
}

IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);
if (indexAbstraction == null) {
// We have got a request for an index that does not exist.
// For non-existing indices, it is safe to assume that no documents can be accessed.
return fullyRestricted();
}

StatefulRules<SingleRule> statefulRules = this.statefulRules;
if (statefulRules != null && !statefulRules.covers(index)) {
statefulRules = null;
}

if (this.dfmEmptyOverridesAll && this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
if (this.dfmEmptyOverridesAll && this.hasUnrestrictedRulesExplicit(context, statefulRules, index)) {
// If dfmEmptyOverwritesAll == true, we can abort early in case unrestricted rules are present. These
// will overrule any other rules.
return unrestricted();
}

// Collect rules into ruleSink
Set<SingleRule> ruleSink = new HashSet<>();
collectRules(context, ruleSink, indexAbstraction, statefulRules);
collectRules(context, ruleSink, index, statefulRules);

IndexAbstraction indexAbstraction = context.getIndicesLookup().get(index);

for (IndexAbstraction parent : getParents(indexAbstraction, context.getIndicesLookup())) {
collectRules(context, ruleSink, parent, statefulRules);
if (indexAbstraction != null) {
for (String parent : getParents(indexAbstraction)) {
collectRules(context, ruleSink, parent, statefulRules);
}
}

if (ruleSink.isEmpty()) {
Expand All @@ -340,7 +319,7 @@ protected JoinedRule getRestrictionImpl(PrivilegesEvaluationContext context, Str
// In case dfmEmptyOverwritesAll == false, we now check for unrestricted rules. If these are present,
// we give full access. Otherwise, we also assume full restrictions
if (CollectionUtils.containsAny(this.staticIndexRules.rolesWithIndexWildcardWithoutRule, context.getMappedRoles())
|| this.hasUnrestrictedRulesExplicit(context, statefulRules, indexAbstraction)) {
|| this.hasUnrestrictedRulesExplicit(context, statefulRules, index)) {
return unrestricted();
} else {
return fullyRestricted();
Expand Down Expand Up @@ -381,10 +360,9 @@ public IndexToRuleMap<JoinedRule> getRestrictions(PrivilegesEvaluationContext co
private void collectRules(
PrivilegesEvaluationContext context,
Set<SingleRule> ruleSink,
IndexAbstraction indexAbstraction,
String index,
StatefulRules<SingleRule> statefulRules
) throws PrivilegesEvaluationException {
String index = indexAbstraction.getName();
Map<String, SingleRule> statefulRoleToRule = null;
boolean statefulRulesEffective;

Expand Down Expand Up @@ -461,28 +439,22 @@ public synchronized void updateIndices(Map<String, IndexAbstraction> indexMetada
/**
* Returns aliases and/or data streams containing the specified index.
*/
private Collection<IndexAbstraction> getParents(IndexAbstraction indexAbstraction, Map<String, IndexAbstraction> indexMetadata) {
private Collection<String> getParents(IndexAbstraction indexAbstraction) {
if (indexAbstraction instanceof IndexAbstraction.Index) {
IndexAbstraction.Index index = (IndexAbstraction.Index) indexAbstraction;

if (index.getWriteIndex().getAliases().isEmpty() && index.getParentDataStream() == null) {
return Collections.emptySet();
}

List<IndexAbstraction> result = new ArrayList<>(index.getWriteIndex().getAliases().size() + 1);
List<String> result = new ArrayList<>(index.getWriteIndex().getAliases().size() + 1);

for (String aliasName : index.getWriteIndex().getAliases().keySet()) {
IndexAbstraction alias = indexMetadata.get(aliasName);

if (alias == null) {
throw new RuntimeException("Inconsistent index lookup; cannot find " + aliasName);
}

result.add(alias);
result.add(aliasName);
}

if (indexAbstraction.getParentDataStream() != null) {
result.add(indexAbstraction.getParentDataStream());
result.add(indexAbstraction.getParentDataStream().getName());
}

return result;
Expand Down

0 comments on commit a11bff0

Please sign in to comment.