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.

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

Command cat/indices will filter results per the Do Not Fail On Forbidden setting #3236

Merged
merged 9 commits into from
Aug 29, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
*/
package org.opensearch.security;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.List;
import java.util.stream.Collectors;

import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
import org.hamcrest.Matchers;
Expand All @@ -31,19 +35,16 @@
import org.opensearch.action.search.SearchResponse;
import org.opensearch.action.search.SearchScrollRequest;
import org.opensearch.client.Client;
import org.opensearch.client.Request;
import org.opensearch.client.Response;
import org.opensearch.client.RestHighLevelClient;
import org.opensearch.test.framework.TestSecurityConfig;
import org.opensearch.test.framework.TestSecurityConfig.User;
import org.opensearch.test.framework.cluster.ClusterManager;
import org.opensearch.test.framework.cluster.LocalCluster;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.aMapWithSize;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.hasKey;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.*;
import static org.opensearch.action.admin.indices.alias.IndicesAliasesRequest.AliasActions.Type.ADD;
import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
import static org.opensearch.client.RequestOptions.DEFAULT;
Expand Down Expand Up @@ -102,15 +103,19 @@ public class DoNotFailOnForbiddenTests {
new TestSecurityConfig.Role("limited-role").clusterPermissions(
"indices:data/read/mget",
"indices:data/read/msearch",
"indices:data/read/scroll"
"indices:data/read/scroll",
"cluster:monitor/state",
"cluster:monitor/health"
)
.indexPermissions(
"indices:data/read/search",
"indices:data/read/mget*",
"indices:data/read/field_caps",
"indices:data/read/field_caps*",
"indices:data/read/msearch",
"indices:data/read/scroll"
"indices:data/read/scroll",
"indices:monitor/settings/get",
"indices:monitor/stats"
)
.on(MARVELOUS_SONGS)
);
Expand Down Expand Up @@ -408,4 +413,18 @@ public void shouldPerformStatAggregation_negative() throws IOException {
}
}

@Test
public void shouldPerformCatIndices_positive() throws IOException {
try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
Request getIndicesRequest = new Request("GET", "/_cat/indices");
// High level client doesn't support _cat/_indices API
Response getIndicesResponse = restHighLevelClient.getLowLevelClient().performRequest(getIndicesRequest);
List<String> indexes = new BufferedReader(new InputStreamReader(getIndicesResponse.getEntity().getContent())).lines()
.collect(Collectors.toList());

assertThat(indexes.size(), equalTo(1));
assertThat(indexes.get(0), containsString("marvelous_songs"));
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import java.util.Map;
import java.util.Set;
import java.util.StringJoiner;
import java.util.regex.Pattern;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -101,12 +101,19 @@

public class PrivilegesEvaluator {

private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*");

private static final Pattern DNFOF_PATTERNS = Pattern.compile(
"indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index)))"
static final WildcardMatcher DNFOF_MATCHER = WildcardMatcher.from(
ImmutableList.of(
"indices:data/read/*",
"indices:admin/mappings/fields/get*",
"indices:admin/shards/search_shards",
"indices:admin/resolve/index",
"indices:monitor/settings/get",
"indices:monitor/stats"
)
);

private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*");

private static final IndicesOptions ALLOW_EMPTY = IndicesOptions.fromOptions(true, true, false, false);

protected final Logger log = LogManager.getLogger(this.getClass());
Expand Down Expand Up @@ -473,7 +480,7 @@ public PrivilegesEvaluatorResponse evaluate(
}
}

if (dnfofEnabled && DNFOF_PATTERNS.matcher(action0).matches()) {
if (dnfofEnabled && DNFOF_MATCHER.test(action0)) {

if (requestedResolved.getAllIndices().isEmpty()) {
presponse.missingPrivileges.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,94 @@

package org.opensearch.security.privileges;

import com.google.common.collect.ImmutableList;
import org.junit.Test;

import java.util.List;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm;
import static org.opensearch.security.privileges.PrivilegesEvaluator.*;

public class PrivilegesEvaluatorUnitTest {

private static final List<String> allowedDnfof = ImmutableList.of(
derek-ho marked this conversation as resolved.
Show resolved Hide resolved
"indices:admin/mappings/fields/get",
"indices:admin/resolve/index",
"indices:admin/shards/search_shards",
"indices:data/read/explain",
"indices:data/read/field_caps",
"indices:data/read/get",
"indices:data/read/mget",
"indices:data/read/msearch",
"indices:data/read/msearch/template",
"indices:data/read/mtv",
"indices:data/read/plugins/replication/file_chunk",
"indices:data/read/plugins/replication/changes",
"indices:data/read/scroll",
"indices:data/read/scroll/clear",
"indices:data/read/search",
"indices:data/read/search/template",
"indices:data/read/tv",
"indices:monitor/settings/get",
"indices:monitor/stats"
);

private static final List<String> disallowedDnfof = ImmutableList.of(
"indices:admin/aliases",
"indices:admin/aliases/exists",
"indices:admin/aliases/get",
"indices:admin/analyze",
"indices:admin/cache/clear",
"indices:admin/close",
"indices:admin/create",
"indices:admin/data_stream/create",
"indices:admin/data_stream/delete",
"indices:admin/data_stream/get",
"indices:admin/delete",
"indices:admin/exists",
"indices:admin/flush",
"indices:admin/forcemerge",
"indices:admin/get",
"indices:admin/mapping/put",
"indices:admin/mappings/get",
"indices:admin/open",
"indices:admin/plugins/replication/index/setup/validate",
"indices:admin/plugins/replication/index/start",
"indices:admin/plugins/replication/index/pause",
"indices:admin/plugins/replication/index/resume",
"indices:admin/plugins/replication/index/stop",
"indices:admin/plugins/replication/index/update",
"indices:admin/plugins/replication/index/status_check",
"indices:admin/refresh",
"indices:admin/rollover",
"indices:admin/seq_no/global_checkpoint_sync",
"indices:admin/settings/update",
"indices:admin/shrink",
"indices:admin/synced_flush",
"indices:admin/template/delete",
"indices:admin/template/get",
"indices:admin/template/put",
"indices:admin/types/exists",
"indices:admin/upgrade",
"indices:admin/validate/query",
"indices:data/write/bulk",
"indices:data/write/delete",
"indices:data/write/delete/byquery",
"indices:data/write/plugins/replication/changes",
"indices:data/write/index",
"indices:data/write/reindex",
"indices:data/write/update",
"indices:data/write/update/byquery",
"indices:monitor/data_stream/stats",
"indices:monitor/recovery",
"indices:monitor/segments",
"indices:monitor/shard_stores",
"indices:monitor/upgrade"
);

@Test
public void testClusterPerm() {
String multiSearchTemplate = "indices:data/read/msearch/template";
Expand All @@ -33,4 +113,18 @@ public void testClusterPerm() {
assertFalse(isClusterPerm(adminClose));
assertFalse(isClusterPerm(monitorUpgrade));
}

@Test
public void testDnfofPermissions_negative() {
for (final String permission : disallowedDnfof) {
assertThat(DNFOF_MATCHER.test(permission), equalTo(false));
}
}

@Test
public void testDnfofPermissions_positive() {
for (final String permission : allowedDnfof) {
assertThat(DNFOF_MATCHER.test(permission), equalTo(true));
}
}
}