Skip to content

Commit 54a78c3

Browse files
Fix Security's expression resolver to not remove unavailable but authorized names (#92625)
For non-wildcard resource names in expressions from requests with the `ignoreUnavailable` request option set to `true`, the Security filter removes names for "unavailable" resources. This behavior is theoretically correct, but in practice the cluster state might not be recovered at the point in time when the Security filter does the rewrite (this causes problems, see #90215); In any case, the logic to remove "unavailable" names is unnecessarily duplicated from Core's expression resolver. This PR makes the expression resolver in the Security filter to NOT: * remove missing but authorized names (indices, aliases, datastreams) * remove authorized datastreams if context disallows datastreams * remove backing indices of authorized datastreams if context disallows datastreams It will only remove names of unauthorized resources, and let the unavailable names go through to be handled (to be ignored or throw "not found" exception) by the expression resolver in Core. Fixes: #90215
1 parent 3f88061 commit 54a78c3

File tree

18 files changed

+296
-93
lines changed

18 files changed

+296
-93
lines changed

docs/changelog/92625.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 92625
2+
summary: Fix Security's expression resolver to not remove unavailable but authorized
3+
names
4+
area: Authorization
5+
type: bug
6+
issues: []

server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ && isIndexVisible(
8383
if (minus) {
8484
finalIndices.remove(indexAbstraction);
8585
} else if (indicesOptions.ignoreUnavailable() == false || isAuthorized.test(indexAbstraction)) {
86+
// Unauthorized names are considered unavailable, so if `ignoreUnavailable` is `true` they should be silently
87+
// discarded from the `finalIndices` list. Other "ways of unavailable" must be handled by the action
88+
// handler, see: https://github.com/elastic/elasticsearch/issues/90215
8689
finalIndices.add(indexAbstraction);
8790
}
8891
}

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.util.Set;
3939
import java.util.concurrent.ConcurrentHashMap;
4040
import java.util.concurrent.atomic.AtomicInteger;
41+
import java.util.function.BiPredicate;
4142
import java.util.function.Predicate;
4243
import java.util.function.Supplier;
4344

@@ -55,7 +56,7 @@ public final class IndicesPermission {
5556

5657
private static final Set<String> PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE = Set.of("create", "create_doc", "index", "write");
5758

58-
private final Map<String, Predicate<IndexAbstraction>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>();
59+
private final Map<String, IsResourceAuthorizedPredicate> allowedIndicesMatchersForAction = new ConcurrentHashMap<>();
5960

6061
private final RestrictedIndices restrictedIndices;
6162
private final Group[] groups;
@@ -127,15 +128,15 @@ public Group[] groups() {
127128
* @return A predicate that will match all the indices that this permission
128129
* has the privilege for executing the given action on.
129130
*/
130-
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
131+
public IsResourceAuthorizedPredicate allowedIndicesMatcher(String action) {
131132
return allowedIndicesMatchersForAction.computeIfAbsent(action, this::buildIndexMatcherPredicateForAction);
132133
}
133134

134135
public boolean hasFieldOrDocumentLevelSecurity() {
135136
return hasFieldOrDocumentLevelSecurity;
136137
}
137138

138-
private Predicate<IndexAbstraction> buildIndexMatcherPredicateForAction(String action) {
139+
private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String action) {
139140
final Set<String> ordinaryIndices = new HashSet<>();
140141
final Set<String> restrictedIndices = new HashSet<>();
141142
final Set<String> grantMappingUpdatesOnIndices = new HashSet<>();
@@ -160,10 +161,64 @@ private Predicate<IndexAbstraction> buildIndexMatcherPredicateForAction(String a
160161
}
161162
final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices);
162163
final StringMatcher bwcSpecialCaseMatcher = indexMatcher(grantMappingUpdatesOnIndices, grantMappingUpdatesOnRestrictedIndices);
163-
return indexAbstraction -> nameMatcher.test(indexAbstraction.getName())
164-
|| (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM
165-
&& (indexAbstraction.getParentDataStream() == null)
166-
&& bwcSpecialCaseMatcher.test(indexAbstraction.getName()));
164+
return new IsResourceAuthorizedPredicate(nameMatcher, bwcSpecialCaseMatcher);
165+
}
166+
167+
/**
168+
* This encapsulates the authorization test for resources.
169+
* There is an additional test for resources that are missing or that are not a datastream or a backing index.
170+
*/
171+
public static class IsResourceAuthorizedPredicate implements BiPredicate<String, IndexAbstraction> {
172+
173+
private final BiPredicate<String, IndexAbstraction> biPredicate;
174+
175+
// public for tests
176+
public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) {
177+
this((String name, @Nullable IndexAbstraction indexAbstraction) -> {
178+
assert indexAbstraction == null || name.equals(indexAbstraction.getName());
179+
return resourceNameMatcher.test(name)
180+
|| (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name));
181+
});
182+
}
183+
184+
private IsResourceAuthorizedPredicate(BiPredicate<String, IndexAbstraction> biPredicate) {
185+
this.biPredicate = biPredicate;
186+
}
187+
188+
/**
189+
* Given another {@link IsResourceAuthorizedPredicate} instance in {@param other},
190+
* return a new {@link IsResourceAuthorizedPredicate} instance that is equivalent to the conjunction of
191+
* authorization tests of that other instance and this one.
192+
*/
193+
@Override
194+
public final IsResourceAuthorizedPredicate and(BiPredicate<? super String, ? super IndexAbstraction> other) {
195+
return new IsResourceAuthorizedPredicate(this.biPredicate.and(other));
196+
}
197+
198+
/**
199+
* Verifies if access is authorized to the given {@param indexAbstraction} resource.
200+
* The resource must exist. Otherwise, use the {@link #test(String, IndexAbstraction)} method.
201+
* Returns {@code true} if access to the given resource is authorized or {@code false} otherwise.
202+
*/
203+
public final boolean test(IndexAbstraction indexAbstraction) {
204+
return test(indexAbstraction.getName(), indexAbstraction);
205+
}
206+
207+
/**
208+
* Verifies if access is authorized to the resource with the given {@param name}.
209+
* The {@param indexAbstraction}, which is the resource to be accessed, must be supplied if the resource exists or be {@code null}
210+
* if it doesn't.
211+
* Returns {@code true} if access to the given resource is authorized or {@code false} otherwise.
212+
*/
213+
@Override
214+
public boolean test(String name, @Nullable IndexAbstraction indexAbstraction) {
215+
return biPredicate.test(name, indexAbstraction);
216+
}
217+
218+
private static boolean isPartOfDatastream(IndexAbstraction indexAbstraction) {
219+
return indexAbstraction != null
220+
&& (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM || indexAbstraction.getParentDataStream() != null);
221+
}
167222
}
168223

169224
/**

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.transport.TransportRequest;
1414
import org.elasticsearch.xpack.core.security.authc.Authentication;
1515
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
16+
import org.elasticsearch.xpack.core.security.authz.permission.IndicesPermission.IsResourceAuthorizedPredicate;
1617
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
1718
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
1819
import org.elasticsearch.xpack.core.security.support.Automatons;
@@ -21,7 +22,6 @@
2122
import java.util.Map;
2223
import java.util.Objects;
2324
import java.util.Set;
24-
import java.util.function.Predicate;
2525

2626
/**
2727
* A {@link Role} limited by another role.<br>
@@ -123,8 +123,8 @@ public IndicesAccessControl authorize(
123123
* action on.
124124
*/
125125
@Override
126-
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
127-
Predicate<IndexAbstraction> predicate = baseRole.indices().allowedIndicesMatcher(action);
126+
public IsResourceAuthorizedPredicate allowedIndicesMatcher(String action) {
127+
IsResourceAuthorizedPredicate predicate = baseRole.indices().allowedIndicesMatcher(action);
128128
predicate = predicate.and(limitedByRole.indices().allowedIndicesMatcher(action));
129129
return predicate;
130130
}

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.elasticsearch.xpack.core.security.authz.RestrictedIndices;
1919
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
2020
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
21+
import org.elasticsearch.xpack.core.security.authz.permission.IndicesPermission.IsResourceAuthorizedPredicate;
2122
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
2223
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
2324
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
@@ -36,7 +37,6 @@
3637
import java.util.Map;
3738
import java.util.Objects;
3839
import java.util.Set;
39-
import java.util.function.Predicate;
4040

4141
public interface Role {
4242

@@ -64,7 +64,7 @@ public interface Role {
6464
* @return A predicate that will match all the indices that this role
6565
* has the privilege for executing the given action on.
6666
*/
67-
Predicate<IndexAbstraction> allowedIndicesMatcher(String action);
67+
IsResourceAuthorizedPredicate allowedIndicesMatcher(String action);
6868

6969
/**
7070
* Returns an {@link Automaton} that matches all action names allowed for the given index

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/SimpleRole.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.PrivilegesCheckResult;
1919
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.PrivilegesToCheck;
2020
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
21+
import org.elasticsearch.xpack.core.security.authz.permission.IndicesPermission.IsResourceAuthorizedPredicate;
2122
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
2223
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
2324

@@ -28,7 +29,6 @@
2829
import java.util.Set;
2930
import java.util.concurrent.ExecutionException;
3031
import java.util.concurrent.atomic.AtomicReference;
31-
import java.util.function.Predicate;
3232

3333
public class SimpleRole implements Role {
3434

@@ -97,7 +97,7 @@ public boolean hasFieldOrDocumentLevelSecurity() {
9797
}
9898

9999
@Override
100-
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
100+
public IsResourceAuthorizedPredicate allowedIndicesMatcher(String action) {
101101
return indices.allowedIndicesMatcher(action);
102102
}
103103

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlRestValidationTestCase.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,14 @@ public void prepareIndices() throws IOException {
5454

5555
protected abstract String getInexistentIndexErrorMessage();
5656

57+
protected String getInexistentWildcardErrorMessage() {
58+
return getInexistentIndexErrorMessage();
59+
}
60+
5761
protected abstract void assertErrorMessageWhenAllowNoIndicesIsFalse(String reqParameter) throws IOException;
5862

5963
public void testDefaultIndicesOptions() throws IOException {
60-
assertErrorMessages(inexistentIndexNameWithWildcard, EMPTY, getInexistentIndexErrorMessage());
64+
assertErrorMessages(inexistentIndexNameWithWildcard, EMPTY, getInexistentWildcardErrorMessage());
6165
assertErrorMessages(inexistentIndexNameWithoutWildcard, EMPTY, getInexistentIndexErrorMessage());
6266
assertValidRequestOnIndices(existentIndexWithWildcard, EMPTY);
6367
assertValidRequestOnIndices(existentIndexWithoutWildcard, EMPTY);
@@ -70,7 +74,7 @@ public void testAllowNoIndicesOption() throws IOException {
7074
String reqParameter = setAllowNoIndices ? "?allow_no_indices=" + allowNoIndices : EMPTY;
7175

7276
if (isAllowNoIndices) {
73-
assertErrorMessages(inexistentIndexNameWithWildcard, reqParameter, getInexistentIndexErrorMessage());
77+
assertErrorMessages(inexistentIndexNameWithWildcard, reqParameter, getInexistentWildcardErrorMessage());
7478
assertErrorMessages(inexistentIndexNameWithoutWildcard, reqParameter, getInexistentIndexErrorMessage());
7579
assertValidRequestOnIndices(existentIndexWithWildcard, reqParameter);
7680
assertValidRequestOnIndices(existentIndexWithoutWildcard, reqParameter);

x-pack/plugin/eql/qa/multi-cluster-with-security/src/javaRestTest/java/org/elasticsearch/xpack/eql/EqlRestValidationIT.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ protected void assertErrorMessageWhenAllowNoIndicesIsFalse(String reqParameter)
3737
assertErrorMessage(
3838
"inexistent1,inexistent2",
3939
reqParameter,
40-
getInexistentIndexErrorMessage() + "[" + indexPattern("inexistent1") + "," + indexPattern("inexistent2") + "]\""
40+
getInexistentIndexErrorMessage()
41+
+ "[null]\",\"resource.type\":\"index_expression\",\"resource.id\":[\"inexistent1\",\"inexistent2\"]}]"
4142
);
4243
}
4344

x-pack/plugin/eql/qa/rest/src/javaRestTest/java/org/elasticsearch/xpack/eql/EqlRestValidationIT.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,11 @@ protected void assertErrorMessageWhenAllowNoIndicesIsFalse(String reqParameter)
3030
// TODO: revisit after
3131
// https://github.com/elastic/elasticsearch/issues/64197
3232
// is closed
33-
assertErrorMessage("inexistent1,inexistent2", reqParameter, """
34-
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [null]\"""");
33+
assertErrorMessage(
34+
"inexistent1,inexistent2",
35+
reqParameter,
36+
"\"root_cause\":[{\"type\":\"index_not_found_exception\",\"reason\":\"no such index [null]\","
37+
+ "\"resource.type\":\"index_expression\",\"resource.id\":[\"inexistent1\",\"inexistent2\"]}]"
38+
);
3539
}
3640
}

x-pack/plugin/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/EqlRestValidationIT.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ protected Settings restClientSettings() {
2323

2424
@Override
2525
protected String getInexistentIndexErrorMessage() {
26+
return "\"root_cause\":[{\"type\":\"verification_exception\",\"reason\":\"Found 1 problem\\nline -1:-1: Unknown index ";
27+
}
28+
29+
@Override
30+
protected String getInexistentWildcardErrorMessage() {
2631
return """
2732
"root_cause":[{"type":"verification_exception","reason":"Found 1 problem\\nline -1:-1: Unknown index [*,-*]"}],\
2833
"type":"index_not_found_exception","reason":"no such index\s""";
@@ -38,9 +43,9 @@ protected void assertErrorMessageWhenAllowNoIndicesIsFalse(String reqParameter)
3843
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [inexistent*]\"""");
3944
// TODO: revisit the next two tests when https://github.com/elastic/elasticsearch/issues/64190 is closed
4045
assertErrorMessage("inexistent", reqParameter, """
41-
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [[inexistent]]\"""");
46+
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [inexistent]\"""");
4247
assertErrorMessage("inexistent1,inexistent2", reqParameter, """
43-
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [[inexistent1, inexistent2]]\"""");
48+
"root_cause":[{"type":"index_not_found_exception","reason":"no such index [null]\"""");
4449
}
4550

4651
}

0 commit comments

Comments
 (0)