Skip to content

Commit

Permalink
Command cat/indices will filter results per the Do Not Fail On Forb…
Browse files Browse the repository at this point in the history 
…idden setting (opensearch-project#3236)

### Description
This change allows for DNFOF behavior on the _cat/_indices API. It adds
the required index permissions into the DNFOF regex to be picked up in
the DNFOF code path. Previously it was being skipped/returning 403,
since the index permissions were not in the regex.

### Issues Resolved
Fix: opensearch-project#1815

Is this a backport? If so, please add backport PR # and/or commits #

### Testing
[Please provide details of testing done: unit testing, integration
testing and manual testing]

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Derek Ho <[email protected]>
  • Loading branch information
@derek-ho
derek-ho authored Aug 29, 2023
1 parent 47a047c commit 4c095d2
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 15 deletions.
35 changes: 27 additions & 8 deletions src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java
View file
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"));
}
}

}
19 changes: 13 additions & 6 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
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
96 changes: 95 additions & 1 deletion src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java
View file
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(
"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));
}
}
}

0 comments on commit 4c095d2

Please sign in to comment.