Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid redundant authz indices contains check for wildcard expansion #76540

Merged
merged 3 commits into from
Oct 19, 2021
Merged

Avoid redundant authz indices contains check for wildcard expansion #76540

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96f8d65
Avoid redundant available indices check
albertzaharovits Aug 15, 2021
ef0c792
Merge branch 'master' into redundant_authz_indices_check
elasticmachine Oct 19, 2021
f001516
Merge branch 'master' into redundant_authz_indices_check
elasticmachine Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public List<String> resolveIndexAbstractions(Iterable<String> indices, IndicesOp
} else if (dateMathName.equals(indexAbstraction)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the current behavior in Security is different the "core" behavior.
In core, excluding missing index names (ie "-nonexistent") is an error, but not in this case.
Most likely, the fix for it is to adopt the same behavior from the date math expression evaluation from a couple of lines above.

But, I'll stay away from it for now.
The robust, non-patchy fix, is to inject into the index name resolver from core the authorized indices view.

The proposed fix in this PR maintains the current (buggy) behavior, but avoids redundant availableIndexAbstractions.contains checks, hopefully having a small positive performance impact.

if (minus) {
finalIndices.remove(indexAbstraction);
} else {
} else if (indicesOptions.ignoreUnavailable() == false || availableIndexAbstractions.contains(indexAbstraction)) {
finalIndices.add(indexAbstraction);
}
}
Expand Down
5 changes: 0 additions & 5 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,6 @@ ResolvedIndices resolveIndicesAndAliases(String action, IndicesRequest indicesRe
}
List<String> replaced = indexAbstractionResolver.resolveIndexAbstractions(split.getLocal(), indicesOptions, metadata,
authorizedIndices, replaceWildcards, indicesRequest.includeDataStreams());
if (indicesOptions.ignoreUnavailable()) {
//out of all the explicit names (expanded from wildcards and original ones that were left untouched)
//remove all the ones that the current user is not authorized for and ignore them
replaced = replaced.stream().filter(authorizedIndices::contains).collect(Collectors.toList());
}
resolvedIndicesBuilder.addLocal(replaced);
resolvedIndicesBuilder.addRemote(split.getRemote());
}
Expand Down