Skip to content

Commit 1c46d7d

Browse files
MmuzafMmuzaf
committed
Fix IllegalArgumentException when resolved indices are empty
Signed-off-by: Maxim Muzafarov <[email protected]>
1 parent cc1d27f commit 1c46d7d

File tree

4 files changed

+253
-4
lines changed

4 files changed

+253
-4
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
3434
- Updates DlsFlsValveImpl condition to return true if request is internal and not a protected resource request ([#5721](https://github.com/opensearch-project/security/pull/5721))
3535
- [Performance] Call AdminDns.isAdmin once per request ([#5752](https://github.com/opensearch-project/security/pull/5752))
3636
- Update operations on `.kibana` system index now work correctly with Dashboards multi tenancy enabled. ([#5778](https://github.com/opensearch-project/security/pull/5778))
37+
- Fix IllegalArgumentException when resolved indices are empty in PrivilegesEvaluator ([#5770](https://github.com/opensearch-project/security/pull/5797)
3738

3839
### Refactoring
3940
- [Resource Sharing] Make migrate api require default access level to be supplied and updates documentations + tests ([#5717](https://github.com/opensearch-project/security/pull/5717))

‎src/integrationTest/java/org/opensearch/security/privileges/actionlevel/RoleBasedActionPrivilegesTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.junit.runners.Suite;
3131

3232
import org.opensearch.action.support.IndicesOptions;
33+
import org.opensearch.cluster.ClusterState;
3334
import org.opensearch.cluster.metadata.IndexAbstraction;
3435
import org.opensearch.cluster.metadata.IndexMetadata;
3536
import org.opensearch.cluster.metadata.Metadata;
@@ -1034,6 +1035,30 @@ public void statefulDisabled() throws Exception {
10341035
subject.updateStatefulIndexPrivileges(metadata, 1);
10351036
assertEquals(0, subject.getEstimatedStatefulIndexByteSize());
10361037
}
1038+
1039+
/**
1040+
* Tests the behavior of hasIndexPrivilege when the resolved indices are empty.
1041+
* @throws Exception If failed.
1042+
*/
1043+
@Test
1044+
public void hasIndexPrivilegeEmptyResolvedIndices() throws Exception {
1045+
SecurityDynamicConfiguration<RoleV7> roles = SecurityDynamicConfiguration.fromYaml(
1046+
"test_role:\n"
1047+
+ " index_permissions:\n"
1048+
+ " - index_patterns: ['test_index*']\n"
1049+
+ " allowed_actions: ['indices:monitor/recovery']",
1050+
CType.ROLES
1051+
);
1052+
1053+
RoleBasedActionPrivileges subject = new RoleBasedActionPrivileges(roles, FlattenedActionGroups.EMPTY, Settings.EMPTY);
1054+
1055+
PrivilegesEvaluatorResponse result = subject.hasIndexPrivilege(
1056+
ctx().clusterState(ClusterState.EMPTY_STATE).roles("test_role").get(),
1057+
ImmutableSet.of("indices:monitor/recovery"),
1058+
IndexResolverReplacer.Resolved._LOCAL_ALL
1059+
);
1060+
assertThat(result, isAllowed());
1061+
}
10371062
}
10381063

10391064
/**

‎src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
12+
package org.opensearch.security.privileges.int_tests;
13+
14+
import java.util.ArrayList;
15+
import java.util.Collection;
16+
import java.util.List;
17+
18+
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
19+
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
20+
import com.google.common.collect.ImmutableList;
21+
import org.apache.logging.log4j.LogManager;
22+
import org.apache.logging.log4j.Logger;
23+
import org.junit.After;
24+
import org.junit.Before;
25+
import org.junit.Test;
26+
import org.junit.runner.RunWith;
27+
28+
import org.opensearch.script.mustache.MustacheModulePlugin;
29+
import org.opensearch.test.framework.TestSecurityConfig;
30+
import org.opensearch.test.framework.cluster.LocalCluster;
31+
import org.opensearch.test.framework.cluster.TestRestClient;
32+
import org.opensearch.test.framework.data.TestIndex;
33+
34+
import static org.hamcrest.MatcherAssert.assertThat;
35+
import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
36+
import static org.opensearch.test.framework.matcher.RestIndexMatchers.IndexMatcher;
37+
import static org.opensearch.test.framework.matcher.RestIndexMatchers.OnUserIndexMatcher.limitedToNone;
38+
import static org.opensearch.test.framework.matcher.RestMatchers.isForbidden;
39+
import static org.opensearch.test.framework.matcher.RestMatchers.isOk;
40+
41+
/**
42+
* Integration tests for index authorization when all indices are closed.
43+
* Tests operations that should work even when getAllIndicesResolved() returns empty.
44+
* This test suite verifies that operations like _cat/recovery can still be authorized
45+
* correctly when all indices are closed, which results in an empty resolved indices set.
46+
* <p>
47+
* We need this dedicated test suite because closing indices triggers cluster state changes,
48+
* affecting some threads and losing access to the RandomizedContext. Therefore, we isolate
49+
* these tests to avoid interference with other tests.
50+
*/
51+
@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
52+
@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
53+
public class IndexAuthorizationWithClosedIndicesIntTests {
54+
55+
private static final Logger log = LogManager.getLogger(IndexAuthorizationWithClosedIndicesIntTests.class);
56+
57+
// -------------------------------------------------------------------------------------------------------
58+
// Test indices used by this test suite
59+
// -------------------------------------------------------------------------------------------------------
60+
61+
static final TestIndex index_a1 = TestIndex.name("index_a1").documentCount(100).seed(1).build();
62+
static final TestIndex index_a2 = TestIndex.name("index_a2").documentCount(110).seed(2).build();
63+
static final TestIndex index_b1 = TestIndex.name("index_b1").documentCount(51).seed(3).build();
64+
static final TestIndex index_b2 = TestIndex.name("index_b2").documentCount(52).seed(4).build();
65+
66+
/**
67+
* This key identifies assertion reference data for index search/read permissions of individual users.
68+
*/
69+
static final TestSecurityConfig.User.MetadataKey<IndexMatcher> READ = new TestSecurityConfig.User.MetadataKey<>(
70+
"read",
71+
IndexMatcher.class
72+
);
73+
74+
/**
75+
* A user with indices:monitor/recovery permission on all indices to verify that it succeeds in case all indices are closed.
76+
*/
77+
static final TestSecurityConfig.User LIMITED_USER_RECOVERY = new TestSecurityConfig.User("limited_user_recovery").description(
78+
"indices:monitor/recovery on *"
79+
)
80+
.roles(
81+
new TestSecurityConfig.Role("r1").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor")
82+
.indexPermissions("indices:monitor/recovery")
83+
.on("*")
84+
)
85+
.reference(READ, limitedToNone());
86+
87+
/**
88+
* A user with indices:monitor/recovery permission on specific indices (not wildcard) to test empty resolved indices handling.
89+
* This user is crucial for testing the empty resolved indices scenario because it doesn't have wildcard privileges,
90+
* which means checkWildcardIndexPrivilegesOnWellKnownActions will return null, forcing execution to reach
91+
* the CheckTable.create() call with empty allIndicesResolved.
92+
*/
93+
static final TestSecurityConfig.User LIMITED_USER_RECOVERY_SPECIFIC = new TestSecurityConfig.User("limited_user_recovery_specific")
94+
.description("indices:monitor/recovery on index_a*")
95+
.roles(
96+
new TestSecurityConfig.Role("r1").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor")
97+
.indexPermissions("indices:monitor/recovery")
98+
.on("index_a*")
99+
)
100+
.reference(READ, limitedToNone());
101+
102+
/**
103+
* A user with "*" privileges on "*"; as it is a regular user, they are still subject to system index
104+
* restrictions and similar things.
105+
*/
106+
static final TestSecurityConfig.User UNLIMITED_USER = new TestSecurityConfig.User("unlimited_user")//
107+
.description("unlimited")
108+
.roles(new TestSecurityConfig.Role("r1").clusterPermissions("*").indexPermissions("*").on("*"))
109+
.reference(READ, limitedToNone());
110+
111+
/**
112+
* The SUPER_UNLIMITED_USER authenticates with an admin cert, which will cause all access control code to be skipped.
113+
* This serves as a base for comparison with the default behavior.
114+
*/
115+
static final TestSecurityConfig.User SUPER_UNLIMITED_USER = new TestSecurityConfig.User("super_unlimited_user")//
116+
.description("super unlimited (admin cert)")
117+
.adminCertUser()
118+
.reference(READ, limitedToNone());
119+
120+
/**
121+
* A user with no index privileges to test that operations are properly denied.
122+
*/
123+
static final TestSecurityConfig.User LIMITED_USER_NONE = new TestSecurityConfig.User("limited_user_none").description(
124+
"no index privileges"
125+
)
126+
.roles(new TestSecurityConfig.Role("r2").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor"))
127+
.reference(READ, limitedToNone());
128+
129+
static final List<TestSecurityConfig.User> USERS = ImmutableList.of(
130+
LIMITED_USER_NONE,
131+
LIMITED_USER_RECOVERY,
132+
LIMITED_USER_RECOVERY_SPECIFIC,
133+
SUPER_UNLIMITED_USER,
134+
UNLIMITED_USER
135+
);
136+
137+
static LocalCluster.Builder clusterBuilder() {
138+
return new LocalCluster.Builder().singleNode()
139+
.authc(AUTHC_HTTPBASIC_INTERNAL)
140+
.users(USERS)
141+
.indices(index_a1, index_a2, index_b1, index_b2)
142+
.plugin(MustacheModulePlugin.class);
143+
}
144+
145+
private final TestSecurityConfig.User user;
146+
private final LocalCluster cluster;
147+
private final ClusterConfig clusterConfig;
148+
149+
@ParametersFactory(shuffle = false, argumentFormatting = "%1$s, %3$s")
150+
public static Collection<Object[]> params() {
151+
List<Object[]> result = new ArrayList<>();
152+
153+
for (ClusterConfig clusterConfig : ClusterConfig.values()) {
154+
for (TestSecurityConfig.User user : USERS) {
155+
result.add(new Object[] { clusterConfig, user, user.getDescription() });
156+
}
157+
}
158+
return result;
159+
}
160+
161+
public IndexAuthorizationWithClosedIndicesIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description)
162+
throws Exception {
163+
this.user = user;
164+
this.cluster = clusterConfig.cluster(IndexAuthorizationWithClosedIndicesIntTests::clusterBuilder);
165+
this.clusterConfig = clusterConfig;
166+
}
167+
168+
@Before
169+
public void setup() {
170+
// In order to test index authorization when all indices are closed, we close all indices, including hidden ones.
171+
try (TestRestClient adminClient = cluster.getAdminCertRestClient()) {
172+
TestRestClient.HttpResponse hiddenCloseResponse = adminClient.post("_all/_close?expand_wildcards=all");
173+
assertThat(hiddenCloseResponse, isOk());
174+
}
175+
}
176+
177+
@After
178+
public void teardown() {
179+
try (TestRestClient adminClient = cluster.getAdminCertRestClient()) {
180+
try {
181+
adminClient.post("_all/_open?expand_wildcards=all");
182+
} catch (Exception e) {
183+
log.warn("Error reopening all indices during teardown", e);
184+
}
185+
}
186+
}
187+
188+
/**
189+
* Tests _cat/recovery operation succeeds when all indices are closed. This verifies that
190+
* the empty resolved indices check in RuntimeOptimizedActionPrivileges works correctly.
191+
*/
192+
@Test
193+
public void cat_recovery_allIndicesClosed() throws Exception {
194+
try (TestRestClient restClient = cluster.getRestClient(user)) {
195+
TestRestClient.HttpResponse httpResponse = restClient.get("_cat/recovery");
196+
197+
if (user == UNLIMITED_USER || user == SUPER_UNLIMITED_USER) {
198+
assertThat(httpResponse, isOk());
199+
} else if (user == LIMITED_USER_RECOVERY) {
200+
// This user has indices:monitor/recovery on wildcard "*".
201+
// checkWildcardIndexPrivilegesOnWellKnownActions() == null should handle this.
202+
assertThat(httpResponse, isOk());
203+
} else if (user == LIMITED_USER_RECOVERY_SPECIFIC) {
204+
// This user has indices:monitor/recovery on index_a* but not wildcard.
205+
// When all indices are closed, getAllIndicesResolved() returns empty,
206+
// then we must ensure that the empty resolved indices case is handled correctly.
207+
assertThat(httpResponse, isOk());
208+
} else if (user == LIMITED_USER_NONE) {
209+
// This user has no permission at the same time as all indices are closed.
210+
assertThat(httpResponse, isForbidden("/error/root_cause/0/reason", "no permissions for [indices:monitor/recovery]"));
211+
} else {
212+
assertThat(httpResponse, isForbidden("/error/root_cause/0/reason", "no permissions for [indices:monitor/recovery]"));
213+
}
214+
}
215+
}
216+
}

‎src/main/java/org/opensearch/security/privileges/actionlevel/RuntimeOptimizedActionPrivileges.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,19 @@ public PrivilegesEvaluatorResponse hasIndexPrivilege(
9898
return PrivilegesEvaluatorResponse.ok();
9999
}
100100

101+
Set<String> allIndicesResolved = resolvedIndices.getAllIndicesResolved(
102+
context.getClusterStateSupplier(),
103+
context.getIndexNameExpressionResolver()
104+
);
105+
106+
if (allIndicesResolved.isEmpty()) {
107+
log.debug("No resolved indices; grant the request (user has index privileges for the action)");
108+
return PrivilegesEvaluatorResponse.ok();
109+
}
110+
101111
// TODO one might want to consider to create a semantic wrapper for action in order to be better tell apart
102112
// what's the action and what's the index in the generic parameters of CheckTable.
103-
CheckTable<String, String> checkTable = CheckTable.create(
104-
resolvedIndices.getAllIndicesResolved(context.getClusterStateSupplier(), context.getIndexNameExpressionResolver()),
105-
actions
106-
);
113+
CheckTable<String, String> checkTable = CheckTable.create(allIndicesResolved, actions);
107114

108115
StatefulIndexPrivileges statefulIndex = this.currentStatefulIndexPrivileges();
109116
PrivilegesEvaluatorResponse resultFromStatefulIndex = null;

0 commit comments

Comments
 (0)