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

[Failure Store] Has Privileges API #125329

Merged
merged 30 commits into from
Mar 26, 2025
+810 −51
Merged

[Failure Store] Has Privileges API #125329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
bd13e85
WIP has privileges
n1v0lg Mar 20, 2025
43716fa
Ugly but works maybe
n1v0lg Mar 20, 2025
b0eef69
Clean up
n1v0lg Mar 20, 2025
ccc1643
More
n1v0lg Mar 20, 2025
b4c7c88
WIP tests
n1v0lg Mar 21, 2025
922d516
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 21, 2025
ec1f294
More
n1v0lg Mar 21, 2025
dc28b4e
More
n1v0lg Mar 21, 2025
c150281
Clean up
n1v0lg Mar 21, 2025
6570ba0
Comments
n1v0lg Mar 21, 2025
d543195
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 21, 2025
d953911
API key coverage
n1v0lg Mar 21, 2025
4e2bf8b
More tests
n1v0lg Mar 21, 2025
8294e8b
More
n1v0lg Mar 21, 2025
978e99d
WIP test restricted indices
n1v0lg Mar 24, 2025
79aa787
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 24, 2025
edb45b9
More tests
n1v0lg Mar 24, 2025
e63fdac
Naming
n1v0lg Mar 24, 2025
c841004
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 24, 2025
f30473a
Validation
n1v0lg Mar 24, 2025
575098b
Nit
n1v0lg Mar 25, 2025
00989f9
API key tests
n1v0lg Mar 25, 2025
6c6445d
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 25, 2025
16f5e91
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 25, 2025
8455641
run-as test and ff
n1v0lg Mar 25, 2025
96b4a24
Merge branch 'failure-store-has-privileges' of github.com:n1v0lg/elas…
n1v0lg Mar 25, 2025
ca6f255
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 25, 2025
08b1a5a
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 26, 2025
143f0a7
Unit test
n1v0lg Mar 26, 2025
3988252
Merge branch 'main' into failure-store-has-privileges
n1v0lg Mar 26, 2025
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
42 changes: 15 additions & 27 deletions ...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,14 +340,27 @@ public ActionRequestValidationException validate(ActionRequestValidationExceptio
if (index == null) {
validationException = addValidationError("indexPrivileges must not be null", validationException);
} else {
for (int i = 0; i < index.length; i++) {
BytesReference query = index[i].getQuery();
for (RoleDescriptor.IndicesPrivileges indicesPrivileges : index) {
BytesReference query = indicesPrivileges.getQuery();
if (query != null) {
validationException = addValidationError(
"may only check index privileges without any DLS query [" + query.utf8ToString() + "]",
validationException
);
}
if (DataStream.isFailureStoreFeatureFlagEnabled()) {
// best effort prevent users from attempting to use selectors in privilege check
for (String indexPattern : indicesPrivileges.getIndices()) {
if (IndexNameExpressionResolver.hasSelector(indexPattern, IndexComponentSelector.FAILURES)
|| IndexNameExpressionResolver.hasSelector(indexPattern, IndexComponentSelector.DATA)) {
validationException = addValidationError(
"may only check index privileges without selectors in index patterns [" + indexPattern + "]",
validationException
);
break;
}
}
}
}
}
if (application == null) {
Expand All @@ -369,31 +382,6 @@ public ActionRequestValidationException validate(ActionRequestValidationExceptio
&& application.length == 0) {
validationException = addValidationError("must specify at least one privilege", validationException);
}
if (index != null) {
// no need to validate failure-store related constraints if it's not enabled
if (DataStream.isFailureStoreFeatureFlagEnabled()) {
for (RoleDescriptor.IndicesPrivileges indexPrivilege : index) {
if (indexPrivilege.getIndices() != null
&& Arrays.stream(indexPrivilege.getIndices())
// best effort prevent users from attempting to check failure selectors
.anyMatch(idx -> IndexNameExpressionResolver.hasSelector(idx, IndexComponentSelector.FAILURES))) {
validationException = addValidationError(
// TODO adjust message once HasPrivileges check supports checking failure store privileges
"failures selector is not supported in index patterns",
validationException
);
}
if (indexPrivilege.getPrivileges() != null
&& Arrays.stream(indexPrivilege.getPrivileges())
.anyMatch(p -> "read_failure_store".equals(p) || "manage_failure_store".equals(p))) {
validationException = addValidationError(
"checking failure store privileges is not supported",
validationException
);
}
}
}
}
return validationException;
}

Expand Down
101 changes: 78 additions & 23 deletions ...c/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,36 +317,58 @@ public boolean checkResourcePrivileges(
@Nullable ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder
) {
boolean allMatch = true;
Map<Automaton, Automaton> indexGroupAutomatons = indexGroupAutomatons(
combineIndexGroups && checkForIndexPatterns.stream().anyMatch(Automatons::isLuceneRegex)
Map<Automaton, Automaton> indexGroupAutomatonsForDataSelector = indexGroupAutomatons(
combineIndexGroups && checkForIndexPatterns.stream().anyMatch(Automatons::isLuceneRegex),
IndexComponentSelector.DATA
);
// optimization: if there are no failures selector privileges in the set of privileges to check, we can skip building
// the automaton map
final boolean containsPrivilegesForFailuresSelector = containsPrivilegesForFailuresSelector(checkForPrivileges);
Map<Automaton, Automaton> indexGroupAutomatonsForFailuresSelector = false == containsPrivilegesForFailuresSelector
? Map.of()
: indexGroupAutomatons(
combineIndexGroups && checkForIndexPatterns.stream().anyMatch(Automatons::isLuceneRegex),
IndexComponentSelector.FAILURES
);
for (String forIndexPattern : checkForIndexPatterns) {
IndexNameExpressionResolver.assertExpressionHasNullOrDataSelector(forIndexPattern);
Automaton checkIndexAutomaton = Automatons.patterns(forIndexPattern);
if (false == allowRestrictedIndices && false == isConcreteRestrictedIndex(forIndexPattern)) {
checkIndexAutomaton = Automatons.minusAndMinimize(checkIndexAutomaton, restrictedIndices.getAutomaton());
}
if (false == Operations.isEmpty(checkIndexAutomaton)) {
Automaton allowedIndexPrivilegesAutomaton = null;
for (var indexAndPrivilegeAutomaton : indexGroupAutomatons.entrySet()) {
if (Automatons.subsetOf(checkIndexAutomaton, indexAndPrivilegeAutomaton.getValue())) {
if (allowedIndexPrivilegesAutomaton != null) {
allowedIndexPrivilegesAutomaton = Automatons.unionAndMinimize(
Arrays.asList(allowedIndexPrivilegesAutomaton, indexAndPrivilegeAutomaton.getKey())
);
} else {
allowedIndexPrivilegesAutomaton = indexAndPrivilegeAutomaton.getKey();
}
}
}
Automaton allowedPrivilegesAutomatonForDataSelector = getIndexPrivilegesAutomaton(
indexGroupAutomatonsForDataSelector,
checkIndexAutomaton
);
Automaton allowedPrivilegesAutomatonForFailuresSelector = getIndexPrivilegesAutomaton(
indexGroupAutomatonsForFailuresSelector,
checkIndexAutomaton
);
for (String privilege : checkForPrivileges) {
IndexPrivilege indexPrivilege = IndexPrivilege.get(privilege);
if (allowedIndexPrivilegesAutomaton != null
&& Automatons.subsetOf(indexPrivilege.getAutomaton(), allowedIndexPrivilegesAutomaton)) {
final IndexPrivilege indexPrivilege = IndexPrivilege.get(privilege);
final boolean checkWithDataSelector = indexPrivilege.getSelectorPredicate().test(IndexComponentSelector.DATA);
final boolean checkWithFailuresSelector = indexPrivilege.getSelectorPredicate().test(IndexComponentSelector.FAILURES);
assert checkWithDataSelector || checkWithFailuresSelector
: "index privilege must map to at least one of [data, failures] selectors";
assert containsPrivilegesForFailuresSelector
|| indexPrivilege.getSelectorPredicate() != IndexComponentSelectorPredicate.FAILURES
: "no failures access privileges should be present in the set of privileges to check";
final Automaton automatonToCheck = indexPrivilege.getAutomaton();
if (checkWithDataSelector
&& allowedPrivilegesAutomatonForDataSelector != null
&& Automatons.subsetOf(automatonToCheck, allowedPrivilegesAutomatonForDataSelector)) {
if (resourcePrivilegesMapBuilder != null) {
resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.TRUE);
}
} else {
} else if (checkWithFailuresSelector
&& allowedPrivilegesAutomatonForFailuresSelector != null
Copy link
Contributor

Choose a reason for hiding this comment

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

one optimization to consider: we could wrap getIndexPrivilegesAutomaton calls into CachedSupplier and make allowedPrivilegesAutomatonForFailuresSelector and allowedPrivilegesAutomatonForDataSelector be computed lazily when checkWithDataSelector or checkWithFailuresSelector is true - respectively

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this too but CachedSupplier involves a synchronized block and might be a little too heavy for the context here (since we don't need synchronization here, I don't think) -- at least I can't immediately say if it would provide an actual gain or not. My thinking here is:

  1. Most calls will be checking for data selectors privileges (there are more of them, and it's the "normal" workflow to access regular data)
  2. This means we'll need the data selectors automata most of the time, which is why I added short-circuiting failures-only selector computation via containsPrivilegesForFailuresSelector. We could do the same for data selectors but that's another pass over the privileges with another set of Map look-ups, or something slightly more complicated

Let me know if you have a stronger intuition here. I'd love to benchmark this and get some real performance numbers but would not want to block on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point - we don't want synchronization here. FWIW We could implement a non-blocking version of cached supplier, but I don't feel this is something we should block on. My intuition is the same, we would be mostly checking data selectors and the optimization you already have could be sufficient.

&& Automatons.subsetOf(automatonToCheck, allowedPrivilegesAutomatonForFailuresSelector)) {
if (resourcePrivilegesMapBuilder != null) {
resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.TRUE);
}
}
// comment to force correct else-block indent
else {
if (resourcePrivilegesMapBuilder != null) {
resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.FALSE);
allMatch = false;
Expand Down Expand Up @@ -806,13 +828,11 @@ private static boolean containsPrivilegeThatGrantsMappingUpdatesForBwc(Group gro
*
* @return a map of all index and privilege pattern automatons
*/
private Map<Automaton, Automaton> indexGroupAutomatons(boolean combine) {
private Map<Automaton, Automaton> indexGroupAutomatons(boolean combine, IndexComponentSelector selector) {
// Map of privilege automaton object references (cached by IndexPrivilege::CACHE)
Map<Automaton, Automaton> allAutomatons = new HashMap<>();
for (Group group : groups) {
// TODO support failure store privileges
// we also check that the group does not support data access to avoid erroneously filtering out `all` privilege groups
if (group.checkSelector(IndexComponentSelector.FAILURES) && false == group.checkSelector(IndexComponentSelector.DATA)) {
if (false == group.checkSelector(selector)) {
continue;
}
Automaton indexAutomaton = group.getIndexMatcherAutomaton();
Expand Down Expand Up @@ -845,6 +865,41 @@ private Map<Automaton, Automaton> indexGroupAutomatons(boolean combine) {
return allAutomatons;
}

private static boolean containsPrivilegesForFailuresSelector(Set<String> checkForPrivileges) {
for (String privilege : checkForPrivileges) {
// use `getNamedOrNull` since only a named privilege can be a failures-only privilege (raw action names are always data access)
IndexPrivilege named = IndexPrivilege.getNamedOrNull(privilege);
// note: we are looking for failures-only privileges here, not `all` which does cover failures but is not a failures-only
// privilege
if (named != null && named.getSelectorPredicate() == IndexComponentSelectorPredicate.FAILURES) {
return true;
}
}
return false;
}

@Nullable
private static Automaton getIndexPrivilegesAutomaton(Map<Automaton, Automaton> indexGroupAutomatons, Automaton checkIndexAutomaton) {
if (indexGroupAutomatons.isEmpty()) {
return null;
}
Automaton allowedPrivilegesAutomaton = null;
for (Map.Entry<Automaton, Automaton> indexAndPrivilegeAutomaton : indexGroupAutomatons.entrySet()) {
Automaton indexNameAutomaton = indexAndPrivilegeAutomaton.getValue();
if (Automatons.subsetOf(checkIndexAutomaton, indexNameAutomaton)) {
Automaton privilegesAutomaton = indexAndPrivilegeAutomaton.getKey();
if (allowedPrivilegesAutomaton != null) {
allowedPrivilegesAutomaton = Automatons.unionAndMinimize(
Arrays.asList(allowedPrivilegesAutomaton, privilegesAutomaton)
);
} else {
allowedPrivilegesAutomaton = privilegesAutomaton;
}
}
}
return allowedPrivilegesAutomaton;
}

public static class Group {
public static final Group[] EMPTY_ARRAY = new Group[0];

Expand Down
Loading